| Message ID | 20220906204301.3736813-1-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | [1/2] PCI: histb: switch to using gpiod API | expand |
On Tuesday 06 September 2022 13:43:00 Dmitry Torokhov wrote: > + ret = gpiod_set_consumer_name(hipcie->reset_gpio, > + "PCIe device power control"); Just unrelated thing, I know it was there before, but I saw it just now and have to comment it: This is absolute nonsense name. "reset-gpios" device tree property specifies PERST# signal pin (PciE ReSeT) as defined in PCIe CEM (Card ElectroMagnetic) specification and it has absolute nothing with PCIe power control. My suggestion for maintainers would be to remove this critic name at all as it would just mislead other people reading that code. > + if (ret) { > + dev_err(dev, "unable to set reset gpio name: %d\n", ret); > + return ret; > }
On Tue, Sep 06, 2022 at 11:08:11PM +0200, Pali Rohár wrote: > On Tuesday 06 September 2022 13:43:00 Dmitry Torokhov wrote: > > + ret = gpiod_set_consumer_name(hipcie->reset_gpio, > > + "PCIe device power control"); > > Just unrelated thing, I know it was there before, but I saw it just now > and have to comment it: This is absolute nonsense name. "reset-gpios" > device tree property specifies PERST# signal pin (PciE ReSeT) as defined > in PCIe CEM (Card ElectroMagnetic) specification and it has absolute > nothing with PCIe power control. > > My suggestion for maintainers would be to remove this critic name at > all as it would just mislead other people reading that code. I can respin the patch is you suggest a more sensible label... > > > + if (ret) { > > + dev_err(dev, "unable to set reset gpio name: %d\n", ret); > > + return ret; > > } Thanks.
On Tuesday 06 September 2022 14:41:20 Dmitry Torokhov wrote: > On Tue, Sep 06, 2022 at 11:08:11PM +0200, Pali Rohár wrote: > > On Tuesday 06 September 2022 13:43:00 Dmitry Torokhov wrote: > > > + ret = gpiod_set_consumer_name(hipcie->reset_gpio, > > > + "PCIe device power control"); > > > > Just unrelated thing, I know it was there before, but I saw it just now > > and have to comment it: This is absolute nonsense name. "reset-gpios" > > device tree property specifies PERST# signal pin (PciE ReSeT) as defined > > in PCIe CEM (Card ElectroMagnetic) specification and it has absolute > > nothing with PCIe power control. > > > > My suggestion for maintainers would be to remove this critic name at > > all as it would just mislead other people reading that code. > > I can respin the patch is you suggest a more sensible label... Lets do renaming in different/separate patch. It is better to split API change patch (which should have any visible functional changes) and fixups (which will have some visible changes) in separate patches. Lorenzo, Bjorn, Krzysztof: This is something for you... Do you have any ideas or suggestions in unifying or fixing these names? I guess more drivers have misleading names and it is better to do any such changes globally and not just in one driver. > > > > > + if (ret) { > > > + dev_err(dev, "unable to set reset gpio name: %d\n", ret); > > > + return ret; > > > } > > Thanks. > > -- > Dmitry
On Tue, Sep 6, 2022 at 10:43 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > This patch switches the driver away from legacy gpio/of_gpio API to > gpiod API, and removes use of of_get_named_gpio_flags() which I want to > make private to gpiolib. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Tue, Sep 06, 2022 at 01:43:00PM -0700, Dmitry Torokhov wrote: > This patch switches the driver away from legacy gpio/of_gpio API to > gpiod API, and removes use of of_get_named_gpio_flags() which I want to > make private to gpiolib. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Should I pick this up ? On patch (2/2) I am not sure we reached a consensus - please let me know. Thanks, Lorenzo > --- > drivers/pci/controller/dwc/pcie-histb.c | 39 ++++++++++++------------- > 1 file changed, 19 insertions(+), 20 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-histb.c b/drivers/pci/controller/dwc/pcie-histb.c > index e2b80f10030d..43c27812dd6d 100644 > --- a/drivers/pci/controller/dwc/pcie-histb.c > +++ b/drivers/pci/controller/dwc/pcie-histb.c > @@ -10,11 +10,11 @@ > > #include <linux/clk.h> > #include <linux/delay.h> > +#include <linux/gpio/consumer.h> > #include <linux/interrupt.h> > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/of.h> > -#include <linux/of_gpio.h> > #include <linux/pci.h> > #include <linux/phy/phy.h> > #include <linux/platform_device.h> > @@ -60,7 +60,7 @@ struct histb_pcie { > struct reset_control *sys_reset; > struct reset_control *bus_reset; > void __iomem *ctrl; > - int reset_gpio; > + struct gpio_desc *reset_gpio; > struct regulator *vpcie; > }; > > @@ -212,8 +212,8 @@ static void histb_pcie_host_disable(struct histb_pcie *hipcie) > clk_disable_unprepare(hipcie->sys_clk); > clk_disable_unprepare(hipcie->bus_clk); > > - if (gpio_is_valid(hipcie->reset_gpio)) > - gpio_set_value_cansleep(hipcie->reset_gpio, 0); > + if (hipcie->reset_gpio) > + gpiod_set_value_cansleep(hipcie->reset_gpio, 1); > > if (hipcie->vpcie) > regulator_disable(hipcie->vpcie); > @@ -235,8 +235,8 @@ static int histb_pcie_host_enable(struct dw_pcie_rp *pp) > } > } > > - if (gpio_is_valid(hipcie->reset_gpio)) > - gpio_set_value_cansleep(hipcie->reset_gpio, 1); > + if (hipcie->reset_gpio) > + gpiod_set_value_cansleep(hipcie->reset_gpio, 0); > > ret = clk_prepare_enable(hipcie->bus_clk); > if (ret) { > @@ -298,10 +298,7 @@ static int histb_pcie_probe(struct platform_device *pdev) > struct histb_pcie *hipcie; > struct dw_pcie *pci; > struct dw_pcie_rp *pp; > - struct device_node *np = pdev->dev.of_node; > struct device *dev = &pdev->dev; > - enum of_gpio_flags of_flags; > - unsigned long flag = GPIOF_DIR_OUT; > int ret; > > hipcie = devm_kzalloc(dev, sizeof(*hipcie), GFP_KERNEL); > @@ -336,17 +333,19 @@ static int histb_pcie_probe(struct platform_device *pdev) > hipcie->vpcie = NULL; > } > > - hipcie->reset_gpio = of_get_named_gpio_flags(np, > - "reset-gpios", 0, &of_flags); > - if (of_flags & OF_GPIO_ACTIVE_LOW) > - flag |= GPIOF_ACTIVE_LOW; > - if (gpio_is_valid(hipcie->reset_gpio)) { > - ret = devm_gpio_request_one(dev, hipcie->reset_gpio, > - flag, "PCIe device power control"); > - if (ret) { > - dev_err(dev, "unable to request gpio\n"); > - return ret; > - } > + hipcie->reset_gpio = devm_gpiod_get_optional(dev, "reset", > + GPIOD_OUT_HIGH); > + ret = PTR_ERR_OR_ZERO(hipcie->reset_gpio); > + if (ret) { > + dev_err(dev, "unable to request reset gpio: %d\n", ret); > + return ret; > + } > + > + ret = gpiod_set_consumer_name(hipcie->reset_gpio, > + "PCIe device power control"); > + if (ret) { > + dev_err(dev, "unable to set reset gpio name: %d\n", ret); > + return ret; > } > > hipcie->aux_clk = devm_clk_get(dev, "aux"); > -- > 2.37.2.789.g6183377224-goog >
On November 11, 2022 7:01:15 AM PST, Lorenzo Pieralisi <lpieralisi@kernel.org> wrote: >On Tue, Sep 06, 2022 at 01:43:00PM -0700, Dmitry Torokhov wrote: >> This patch switches the driver away from legacy gpio/of_gpio API to >> gpiod API, and removes use of of_get_named_gpio_flags() which I want to >> make private to gpiolib. >> >> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > >Should I pick this up ? On patch (2/2) I am not sure we reached >a consensus - please let me know. Could you pick just this patch (1/2) for now - I owe Pali a respin on the other one, but as far as I remember there was no material disagreement on the patches themselves, just a discussion how to change gpiod API to be more expressive. Thanks.
On Tue, 6 Sep 2022 13:43:00 -0700, Dmitry Torokhov wrote: > This patch switches the driver away from legacy gpio/of_gpio API to > gpiod API, and removes use of of_get_named_gpio_flags() which I want to > make private to gpiolib. > > Applied to pci/dwc, thanks! [1/2] PCI: histb: switch to using gpiod API https://git.kernel.org/lpieralisi/pci/c/1d26a55fbeb9 Thanks, Lorenzo
diff --git a/drivers/pci/controller/dwc/pcie-histb.c b/drivers/pci/controller/dwc/pcie-histb.c index e2b80f10030d..43c27812dd6d 100644 --- a/drivers/pci/controller/dwc/pcie-histb.c +++ b/drivers/pci/controller/dwc/pcie-histb.c @@ -10,11 +10,11 @@ #include <linux/clk.h> #include <linux/delay.h> +#include <linux/gpio/consumer.h> #include <linux/interrupt.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/of.h> -#include <linux/of_gpio.h> #include <linux/pci.h> #include <linux/phy/phy.h> #include <linux/platform_device.h> @@ -60,7 +60,7 @@ struct histb_pcie { struct reset_control *sys_reset; struct reset_control *bus_reset; void __iomem *ctrl; - int reset_gpio; + struct gpio_desc *reset_gpio; struct regulator *vpcie; }; @@ -212,8 +212,8 @@ static void histb_pcie_host_disable(struct histb_pcie *hipcie) clk_disable_unprepare(hipcie->sys_clk); clk_disable_unprepare(hipcie->bus_clk); - if (gpio_is_valid(hipcie->reset_gpio)) - gpio_set_value_cansleep(hipcie->reset_gpio, 0); + if (hipcie->reset_gpio) + gpiod_set_value_cansleep(hipcie->reset_gpio, 1); if (hipcie->vpcie) regulator_disable(hipcie->vpcie); @@ -235,8 +235,8 @@ static int histb_pcie_host_enable(struct dw_pcie_rp *pp) } } - if (gpio_is_valid(hipcie->reset_gpio)) - gpio_set_value_cansleep(hipcie->reset_gpio, 1); + if (hipcie->reset_gpio) + gpiod_set_value_cansleep(hipcie->reset_gpio, 0); ret = clk_prepare_enable(hipcie->bus_clk); if (ret) { @@ -298,10 +298,7 @@ static int histb_pcie_probe(struct platform_device *pdev) struct histb_pcie *hipcie; struct dw_pcie *pci; struct dw_pcie_rp *pp; - struct device_node *np = pdev->dev.of_node; struct device *dev = &pdev->dev; - enum of_gpio_flags of_flags; - unsigned long flag = GPIOF_DIR_OUT; int ret; hipcie = devm_kzalloc(dev, sizeof(*hipcie), GFP_KERNEL); @@ -336,17 +333,19 @@ static int histb_pcie_probe(struct platform_device *pdev) hipcie->vpcie = NULL; } - hipcie->reset_gpio = of_get_named_gpio_flags(np, - "reset-gpios", 0, &of_flags); - if (of_flags & OF_GPIO_ACTIVE_LOW) - flag |= GPIOF_ACTIVE_LOW; - if (gpio_is_valid(hipcie->reset_gpio)) { - ret = devm_gpio_request_one(dev, hipcie->reset_gpio, - flag, "PCIe device power control"); - if (ret) { - dev_err(dev, "unable to request gpio\n"); - return ret; - } + hipcie->reset_gpio = devm_gpiod_get_optional(dev, "reset", + GPIOD_OUT_HIGH); + ret = PTR_ERR_OR_ZERO(hipcie->reset_gpio); + if (ret) { + dev_err(dev, "unable to request reset gpio: %d\n", ret); + return ret; + } + + ret = gpiod_set_consumer_name(hipcie->reset_gpio, + "PCIe device power control"); + if (ret) { + dev_err(dev, "unable to set reset gpio name: %d\n", ret); + return ret; } hipcie->aux_clk = devm_clk_get(dev, "aux");
This patch switches the driver away from legacy gpio/of_gpio API to gpiod API, and removes use of of_get_named_gpio_flags() which I want to make private to gpiolib. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/pci/controller/dwc/pcie-histb.c | 39 ++++++++++++------------- 1 file changed, 19 insertions(+), 20 deletions(-)