| Message ID | 20250328143646.27678-2-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | PCI/arm64/ath11k/ath12k: Rename pwrctrl Kconfig symbols | expand |
| Context | Check | Description |
|---|---|---|
| wifibot/tree_selection | success | Guessing tree name failed - patch did not apply |
On Fri, Mar 28, 2025 at 03:36:43PM +0100, Johan Hovold wrote: > Commits b88cbaaa6fa1 ("PCI/pwrctrl: Rename pwrctl files to pwrctrl") and > 3f925cd62874 ("PCI/pwrctrl: Rename pwrctrl functions and structures") > renamed the "pwrctl" framework to "pwrctrl" for consistency reasons. > > Rename also the Kconfig symbols so that they reflect the new name while > adding entries for the deprecated ones. The old symbols can be removed > once everything that depends on them has been updated. I considered changing the Kconfig symbols at the time, but didn't do it because I didn't want to break existing .config files. Is that not a concern? Or do you think the long-term benefit is worth having users re-answer these config questions? We have lots of Kconfig symbols that are not what we would choose today, e.g., my misguided suggestion years ago to use "CONFIG_PCI_*" instead of "CONFIG_PCIE_*" for PCIe controller drivers that didn't have any PCIe content. If we do want this, I would think we should squash all these so we don't have breakage between this patch and the following ones. Bjorn
On Fri, Mar 28, 2025 at 10:13:43AM -0500, Bjorn Helgaas wrote: > On Fri, Mar 28, 2025 at 03:36:43PM +0100, Johan Hovold wrote: > > Commits b88cbaaa6fa1 ("PCI/pwrctrl: Rename pwrctl files to pwrctrl") and > > 3f925cd62874 ("PCI/pwrctrl: Rename pwrctrl functions and structures") > > renamed the "pwrctl" framework to "pwrctrl" for consistency reasons. > > > > Rename also the Kconfig symbols so that they reflect the new name while > > adding entries for the deprecated ones. The old symbols can be removed > > once everything that depends on them has been updated. > > I considered changing the Kconfig symbols at the time, but didn't do > it because I didn't want to break existing .config files. Is that not > a concern? Or do you think the long-term benefit is worth having > users re-answer these config questions? I added entries for the old (deprecated) symbol names that select the new ones so that should not be a problem. > If we do want this, I would think we should squash all these so we > don't have breakage between this patch and the following ones. The entries using the old names make sure that there is no breakage as long as the patches are applied in order. They also handle any new users being added to other subsystem trees. (As I mentioned in the cover letter we can drop those entries after a couple of releases.) Johan
Hi, I have some nitpicks ... On Fri, Mar 28, 2025 at 3:41 PM Johan Hovold <johan+linaro@kernel.org> wrote: > > Commits b88cbaaa6fa1 ("PCI/pwrctrl: Rename pwrctl files to pwrctrl") and > 3f925cd62874 ("PCI/pwrctrl: Rename pwrctrl functions and structures") > renamed the "pwrctl" framework to "pwrctrl" for consistency reasons. > > Rename also the Kconfig symbols so that they reflect the new name while > adding entries for the deprecated ones. The old symbols can be removed > once everything that depends on them has been updated. > > The new slot module is also renamed to reflect the framework name and > match the other pwrctrl modules. > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > drivers/pci/pwrctrl/Kconfig | 27 +++++++++++++++++++++------ > drivers/pci/pwrctrl/Makefile | 8 ++++---- > 2 files changed, 25 insertions(+), 10 deletions(-) > > diff --git a/drivers/pci/pwrctrl/Kconfig b/drivers/pci/pwrctrl/Kconfig > index 990cab67d413..62f176e42e33 100644 > --- a/drivers/pci/pwrctrl/Kconfig > +++ b/drivers/pci/pwrctrl/Kconfig > @@ -1,19 +1,19 @@ > # SPDX-License-Identifier: GPL-2.0-only > > -config HAVE_PWRCTL > +config HAVE_PWRCTRL > bool > > -config PCI_PWRCTL > +config PCI_PWRCTRL > tristate > > -config PCI_PWRCTL_PWRSEQ > +config PCI_PWRCTRL_PWRSEQ > tristate > select POWER_SEQUENCING > - select PCI_PWRCTL > + select PCI_PWRCTRL > > -config PCI_PWRCTL_SLOT > +config PCI_PWRCTRL_SLOT > tristate "PCI Power Control driver for PCI slots" > - select PCI_PWRCTL > + select PCI_PWRCTRL > help > Say Y here to enable the PCI Power Control driver to control the power > state of PCI slots. > @@ -21,3 +21,18 @@ config PCI_PWRCTL_SLOT > This is a generic driver that controls the power state of different > PCI slots. The voltage regulators powering the rails of the PCI slots > are expected to be defined in the devicetree node of the PCI bridge. > + > +# deprecated > +config HAVE_PWRCTL > + bool > + select HAVE_PWRCTRL I'm not sure this will work as intended. This symbol can only be != n if anything selects it, but there may also be (outdated) config symbols that depend on its value. E.g. ath1*k has "select PCI_PWRCTL_PWRSEQ if HAVE_PWRCTL", and if there is nothing selecting HAVE_PWRCTL, but HAVE_PWRCTRL directly instead, HAVE_PWRCTL will be =n and the condition will fail. Since you rename the only one selecting HAVE_PWRCTL in patch 2/4, but update ath1*k in 3/4 and 4/4, their select PCI_PWRCT(R)L_PWRSEQ use is temporarily ineffective. Moving the arm64 patch last would avoid that though, at least for the current state. The alternative would be split this by config symbol instead of per tree, so all users would be atomatically updated as well. These patches need to go through the same tree anyways, so I see no issue doing it that way. > + > +# deprecated > +config PCI_PWRCTL_PWRSEQ > + tristate > + select PCI_PWRCTRL_PWRSEQ Similar issue, but there are no conditionals based on this, so this may be fine. > + > +# deprecated > +config PCI_PWRCTL_SLOT > + tristate > + select PCI_PWRCTRL_SLOT This one won't work. Its value will be automatically calculated based on other symbols selecting it, and since there is nothing selecting it, it will always be n, regardless what any existing .config says. So unless you make this a user selectable symbol as well, this will (potentially) break existing .configs since its value will be then automatically calculated as =n, and the new symbol takes the default =n (unless explicitly enabled, or selected by ath1*k). Best regards, Jonas
On Wed, Apr 02, 2025 at 01:54:36PM +0200, Jonas Gorski wrote: > I have some nitpicks ... Thanks for taking a look. > On Fri, Mar 28, 2025 at 3:41 PM Johan Hovold <johan+linaro@kernel.org> wrote: > > > > Commits b88cbaaa6fa1 ("PCI/pwrctrl: Rename pwrctl files to pwrctrl") and > > 3f925cd62874 ("PCI/pwrctrl: Rename pwrctrl functions and structures") > > renamed the "pwrctl" framework to "pwrctrl" for consistency reasons. > > > > Rename also the Kconfig symbols so that they reflect the new name while > > adding entries for the deprecated ones. The old symbols can be removed > > once everything that depends on them has been updated. > > > > The new slot module is also renamed to reflect the framework name and > > match the other pwrctrl modules. > > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > --- > > drivers/pci/pwrctrl/Kconfig | 27 +++++++++++++++++++++------ > > drivers/pci/pwrctrl/Makefile | 8 ++++---- > > 2 files changed, 25 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/pci/pwrctrl/Kconfig b/drivers/pci/pwrctrl/Kconfig > > index 990cab67d413..62f176e42e33 100644 > > --- a/drivers/pci/pwrctrl/Kconfig > > +++ b/drivers/pci/pwrctrl/Kconfig > > @@ -1,19 +1,19 @@ > > # SPDX-License-Identifier: GPL-2.0-only > > > > -config HAVE_PWRCTL > > +config HAVE_PWRCTRL > > bool > > > > -config PCI_PWRCTL > > +config PCI_PWRCTRL > > tristate > > > > -config PCI_PWRCTL_PWRSEQ > > +config PCI_PWRCTRL_PWRSEQ > > tristate > > select POWER_SEQUENCING > > - select PCI_PWRCTL > > + select PCI_PWRCTRL > > > > -config PCI_PWRCTL_SLOT > > +config PCI_PWRCTRL_SLOT > > tristate "PCI Power Control driver for PCI slots" > > - select PCI_PWRCTL > > + select PCI_PWRCTRL > > help > > Say Y here to enable the PCI Power Control driver to control the power > > state of PCI slots. > > @@ -21,3 +21,18 @@ config PCI_PWRCTL_SLOT > > This is a generic driver that controls the power state of different > > PCI slots. The voltage regulators powering the rails of the PCI slots > > are expected to be defined in the devicetree node of the PCI bridge. > > + > > +# deprecated > > +config HAVE_PWRCTL > > + bool > > + select HAVE_PWRCTRL > > I'm not sure this will work as intended. This symbol can only be != n > if anything selects it, but there may also be (outdated) config > symbols that depend on its value. E.g. ath1*k has "select > PCI_PWRCTL_PWRSEQ if HAVE_PWRCTL", and if there is nothing selecting > HAVE_PWRCTL, but HAVE_PWRCTRL directly instead, HAVE_PWRCTL will be =n > and the condition will fail. > > Since you rename the only one selecting HAVE_PWRCTL in patch 2/4, but > update ath1*k in 3/4 and 4/4, their select PCI_PWRCT(R)L_PWRSEQ use is > temporarily ineffective. Moving the arm64 patch last would avoid that > though, at least for the current state. You're right, thanks for catching this. > The alternative would be split this by config symbol instead of per > tree, so all users would be atomatically updated as well. These > patches need to go through the same tree anyways, so I see no issue > doing it that way. Moving the arm64 patch last would take care of the temporary glitch, but since it seems we need to take this through one tree (e.g. to avoid merging it over three cycles) perhaps we can just squash them. That said, it seems I would need to reverse the logic for HAVE_PWRCTL so that it is selected by HAVE_PWRCTRL if we think we still need to worry about new users of HAVE_PWRCTL being adding to other trees. If we could get this renamed already in rc1, we probably do no need to care about that though. > > + > > +# deprecated > > +config PCI_PWRCTL_PWRSEQ > > + tristate > > + select PCI_PWRCTRL_PWRSEQ > > Similar issue, but there are no conditionals based on this, so this may be fine. Yeah, this one should be fine. > > + > > +# deprecated > > +config PCI_PWRCTL_SLOT > > + tristate > > + select PCI_PWRCTRL_SLOT > > This one won't work. Its value will be automatically calculated based > on other symbols selecting it, and since there is nothing selecting > it, it will always be n, regardless what any existing .config says. > > So unless you make this a user selectable symbol as well, this will > (potentially) break existing .configs since its value will be then > automatically calculated as =n, and the new symbol takes the default > =n (unless explicitly enabled, or selected by ath1*k). Right. Since PCI_PWRCTL_SLOT will be new in rc1 and there is nothing selecting it, I think we can just rename this one. At least if we can get this into rc1 and make sure that the pending defconfig update is respun. Otherwise it would indeed need to be user selectable. Let me respin this. Since this is all quite new, we should be able to just rip of the band-aid for rc1 and not need to worry about this going forward. Johan
diff --git a/drivers/pci/pwrctrl/Kconfig b/drivers/pci/pwrctrl/Kconfig index 990cab67d413..62f176e42e33 100644 --- a/drivers/pci/pwrctrl/Kconfig +++ b/drivers/pci/pwrctrl/Kconfig @@ -1,19 +1,19 @@ # SPDX-License-Identifier: GPL-2.0-only -config HAVE_PWRCTL +config HAVE_PWRCTRL bool -config PCI_PWRCTL +config PCI_PWRCTRL tristate -config PCI_PWRCTL_PWRSEQ +config PCI_PWRCTRL_PWRSEQ tristate select POWER_SEQUENCING - select PCI_PWRCTL + select PCI_PWRCTRL -config PCI_PWRCTL_SLOT +config PCI_PWRCTRL_SLOT tristate "PCI Power Control driver for PCI slots" - select PCI_PWRCTL + select PCI_PWRCTRL help Say Y here to enable the PCI Power Control driver to control the power state of PCI slots. @@ -21,3 +21,18 @@ config PCI_PWRCTL_SLOT This is a generic driver that controls the power state of different PCI slots. The voltage regulators powering the rails of the PCI slots are expected to be defined in the devicetree node of the PCI bridge. + +# deprecated +config HAVE_PWRCTL + bool + select HAVE_PWRCTRL + +# deprecated +config PCI_PWRCTL_PWRSEQ + tristate + select PCI_PWRCTRL_PWRSEQ + +# deprecated +config PCI_PWRCTL_SLOT + tristate + select PCI_PWRCTRL_SLOT diff --git a/drivers/pci/pwrctrl/Makefile b/drivers/pci/pwrctrl/Makefile index ddfb12c5aadf..a4e5808d7850 100644 --- a/drivers/pci/pwrctrl/Makefile +++ b/drivers/pci/pwrctrl/Makefile @@ -1,9 +1,9 @@ # SPDX-License-Identifier: GPL-2.0-only -obj-$(CONFIG_PCI_PWRCTL) += pci-pwrctrl-core.o +obj-$(CONFIG_PCI_PWRCTRL) += pci-pwrctrl-core.o pci-pwrctrl-core-y := core.o -obj-$(CONFIG_PCI_PWRCTL_PWRSEQ) += pci-pwrctrl-pwrseq.o +obj-$(CONFIG_PCI_PWRCTRL_PWRSEQ) += pci-pwrctrl-pwrseq.o -obj-$(CONFIG_PCI_PWRCTL_SLOT) += pci-pwrctl-slot.o -pci-pwrctl-slot-y := slot.o +obj-$(CONFIG_PCI_PWRCTRL_SLOT) += pci-pwrctrl-slot.o +pci-pwrctrl-slot-y := slot.o
Commits b88cbaaa6fa1 ("PCI/pwrctrl: Rename pwrctl files to pwrctrl") and 3f925cd62874 ("PCI/pwrctrl: Rename pwrctrl functions and structures") renamed the "pwrctl" framework to "pwrctrl" for consistency reasons. Rename also the Kconfig symbols so that they reflect the new name while adding entries for the deprecated ones. The old symbols can be removed once everything that depends on them has been updated. The new slot module is also renamed to reflect the framework name and match the other pwrctrl modules. Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- drivers/pci/pwrctrl/Kconfig | 27 +++++++++++++++++++++------ drivers/pci/pwrctrl/Makefile | 8 ++++---- 2 files changed, 25 insertions(+), 10 deletions(-)