| Message ID | 20240423172208.2723892-4-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: controller: Move to agnostic GPIO API | expand |
On Tue, Apr 23, 2024 at 08:19:06PM +0300, Andy Shevchenko wrote: > The of_gpio.h is going to be removed. In preparation of that convert > the driver to the agnostic API. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/pci/controller/dwc/pci-imx6.c | 37 ++++++++++----------------- > 1 file changed, 14 insertions(+), 23 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index 917c69edee1d..d620f1e1a43c 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -11,14 +11,13 @@ > #include <linux/bitfield.h> > #include <linux/clk.h> > #include <linux/delay.h> > -#include <linux/gpio.h> > +#include <linux/gpio/consumer.h> > #include <linux/kernel.h> > #include <linux/mfd/syscon.h> > #include <linux/mfd/syscon/imx6q-iomuxc-gpr.h> > #include <linux/mfd/syscon/imx7-iomuxc-gpr.h> > #include <linux/module.h> > #include <linux/of.h> > -#include <linux/of_gpio.h> > #include <linux/of_address.h> > #include <linux/pci.h> > #include <linux/platform_device.h> > @@ -107,7 +106,7 @@ struct imx6_pcie_drvdata { > > struct imx6_pcie { > struct dw_pcie *pci; > - int reset_gpio; > + struct gpio_desc *reset_gpiod; > bool gpio_active_high; > bool link_is_up; > struct clk_bulk_data clks[IMX6_PCIE_MAX_CLKS]; > @@ -721,9 +720,8 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie) > } > > /* Some boards don't have PCIe reset GPIO. */ > - if (gpio_is_valid(imx6_pcie->reset_gpio)) > - gpio_set_value_cansleep(imx6_pcie->reset_gpio, > - imx6_pcie->gpio_active_high); > + gpiod_set_raw_value_cansleep(imx6_pcie->reset_gpiod, > + imx6_pcie->gpio_active_high); > } > > static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) > @@ -771,10 +769,10 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) > } > > /* Some boards don't have PCIe reset GPIO. */ > - if (gpio_is_valid(imx6_pcie->reset_gpio)) { > + if (imx6_pcie->reset_gpiod) { > msleep(100); > - gpio_set_value_cansleep(imx6_pcie->reset_gpio, > - !imx6_pcie->gpio_active_high); > + gpiod_set_raw_value_cansleep(imx6_pcie->reset_gpiod, > + !imx6_pcie->gpio_active_high); > /* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */ > msleep(100); > } > @@ -1285,22 +1283,15 @@ static int imx6_pcie_probe(struct platform_device *pdev) > return PTR_ERR(pci->dbi_base); > > /* Fetch GPIOs */ > - imx6_pcie->reset_gpio = of_get_named_gpio(node, "reset-gpio", 0); > imx6_pcie->gpio_active_high = of_property_read_bool(node, > "reset-gpio-active-high"); > - if (gpio_is_valid(imx6_pcie->reset_gpio)) { > - ret = devm_gpio_request_one(dev, imx6_pcie->reset_gpio, > - imx6_pcie->gpio_active_high ? > - GPIOF_OUT_INIT_HIGH : > - GPIOF_OUT_INIT_LOW, > - "PCIe reset"); > - if (ret) { > - dev_err(dev, "unable to get reset gpio\n"); > - return ret; > - } > - } else if (imx6_pcie->reset_gpio == -EPROBE_DEFER) { > - return imx6_pcie->reset_gpio; > - } > + imx6_pcie->reset_gpiod = > + devm_gpiod_get_optional(dev, "reset", > + imx6_pcie->gpio_active_high ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW); > + if (IS_ERR(imx6_pcie->reset_gpiod)) > + return dev_err_probe(dev, PTR_ERR(imx6_pcie->reset_gpiod), > + "unable to get reset gpio\n"); Small problem here. err message "unable to get reset gpio\n" will print when -EPROBE_DEFER happen. EPROBE_DEFER is quite common when use i2c expand gpio chip. Frank > + gpiod_set_consumer_name(imx6_pcie->reset_gpiod, "PCIe reset"); > > if (imx6_pcie->drvdata->clks_cnt >= IMX6_PCIE_MAX_CLKS) > return dev_err_probe(dev, -ENOMEM, "clks_cnt is too big\n"); > -- > 2.43.0.rc1.1336.g36b5255a03ac >
On Tue, Apr 23, 2024 at 03:56:56PM -0400, Frank Li wrote: > On Tue, Apr 23, 2024 at 08:19:06PM +0300, Andy Shevchenko wrote: ... > > + imx6_pcie->reset_gpiod = > > + devm_gpiod_get_optional(dev, "reset", > > + imx6_pcie->gpio_active_high ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW); > > + if (IS_ERR(imx6_pcie->reset_gpiod)) > > + return dev_err_probe(dev, PTR_ERR(imx6_pcie->reset_gpiod), > > + "unable to get reset gpio\n"); > > Small problem here. err message "unable to get reset gpio\n" will print > when -EPROBE_DEFER happen. EPROBE_DEFER is quite common when use i2c > expand gpio chip. I'm not sure how you come to this conclusion. Can you elaborate, please? P.S> I do not see a problem as described.
On Tue, Apr 23, 2024 at 11:03:59PM +0300, Andy Shevchenko wrote: > On Tue, Apr 23, 2024 at 03:56:56PM -0400, Frank Li wrote: > > On Tue, Apr 23, 2024 at 08:19:06PM +0300, Andy Shevchenko wrote: > > ... > > > > + imx6_pcie->reset_gpiod = > > > + devm_gpiod_get_optional(dev, "reset", > > > + imx6_pcie->gpio_active_high ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW); > > > + if (IS_ERR(imx6_pcie->reset_gpiod)) > > > + return dev_err_probe(dev, PTR_ERR(imx6_pcie->reset_gpiod), > > > + "unable to get reset gpio\n"); > > > > Small problem here. err message "unable to get reset gpio\n" will print > > when -EPROBE_DEFER happen. EPROBE_DEFER is quite common when use i2c > > expand gpio chip. > > I'm not sure how you come to this conclusion. Can you elaborate, please? > P.S> I do not see a problem as described. If i2c gpio-expander driver have not load when imx6_pcie probe, I supposed devm_gpiod_get_optional() will return -EPROBE_DEFER, if (IS_ERR(imx6_pcie->reset_gpiod)) should be true. then dev_err_probe() will run and print "unable to get reset gpio\n" with error code -EPROBE_DEFER. driver framework will retry imx6_pcie probe again when a new device appear. it may retry sevial times utill i2c gpio-expander driver probe success or timeout. Frank > > -- > With Best Regards, > Andy Shevchenko > >
On Tue, Apr 23, 2024 at 5:16 PM Frank Li <Frank.li@nxp.com> wrote: > if (IS_ERR(imx6_pcie->reset_gpiod)) should be true. then dev_err_probe() > will run and print "unable to get reset gpio\n" with error code > -EPROBE_DEFER. dev_err_probe() will not print an error message when the error code is -EPROBE_DEFER. That's exactly the point of using dev_err_probe().
On Tue, Apr 23, 2024 at 04:16:36PM -0400, Frank Li wrote: > On Tue, Apr 23, 2024 at 11:03:59PM +0300, Andy Shevchenko wrote: > > On Tue, Apr 23, 2024 at 03:56:56PM -0400, Frank Li wrote: > > > On Tue, Apr 23, 2024 at 08:19:06PM +0300, Andy Shevchenko wrote: > > > > ... > > > > > > + imx6_pcie->reset_gpiod = > > > > + devm_gpiod_get_optional(dev, "reset", > > > > + imx6_pcie->gpio_active_high ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW); > > > > + if (IS_ERR(imx6_pcie->reset_gpiod)) > > > > + return dev_err_probe(dev, PTR_ERR(imx6_pcie->reset_gpiod), > > > > + "unable to get reset gpio\n"); > > > > > > Small problem here. err message "unable to get reset gpio\n" will print > > > when -EPROBE_DEFER happen. EPROBE_DEFER is quite common when use i2c > > > expand gpio chip. > > > > I'm not sure how you come to this conclusion. Can you elaborate, please? > > P.S> I do not see a problem as described. > > If i2c gpio-expander driver have not load when imx6_pcie probe, I supposed > devm_gpiod_get_optional() will return -EPROBE_DEFER, > > if (IS_ERR(imx6_pcie->reset_gpiod)) should be true. then dev_err_probe() > will run and print "unable to get reset gpio\n" with error code > -EPROBE_DEFER. Sorry for that. dev_err_probe() already consider this. Please forget my comments. dev_err_probe() { if (err != -EPROBE_DEFER) { dev_err(dev, "error %pe: %pV", ERR_PTR(err), &vaf); } } So: Reviewed-by: Frank Li <Frank.Li@nxp.com> > > driver framework will retry imx6_pcie probe again when a new device appear. > it may retry sevial times utill i2c gpio-expander driver probe success or > timeout. > > Frank > > > > > -- > > With Best Regards, > > Andy Shevchenko > > > >
On Tue, Apr 23, 2024 at 04:23:06PM -0400, Frank Li wrote: > On Tue, Apr 23, 2024 at 04:16:36PM -0400, Frank Li wrote: ... > Reviewed-by: Frank Li <Frank.Li@nxp.com> Thank you!
On Tue, Apr 23, 2024 at 08:19:06PM +0300, Andy Shevchenko wrote: > The of_gpio.h is going to be removed. In preparation of that convert > the driver to the agnostic API. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > drivers/pci/controller/dwc/pci-imx6.c | 37 ++++++++++----------------- > 1 file changed, 14 insertions(+), 23 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index 917c69edee1d..d620f1e1a43c 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -11,14 +11,13 @@ > #include <linux/bitfield.h> > #include <linux/clk.h> > #include <linux/delay.h> > -#include <linux/gpio.h> > +#include <linux/gpio/consumer.h> > #include <linux/kernel.h> > #include <linux/mfd/syscon.h> > #include <linux/mfd/syscon/imx6q-iomuxc-gpr.h> > #include <linux/mfd/syscon/imx7-iomuxc-gpr.h> > #include <linux/module.h> > #include <linux/of.h> > -#include <linux/of_gpio.h> > #include <linux/of_address.h> > #include <linux/pci.h> > #include <linux/platform_device.h> > @@ -107,7 +106,7 @@ struct imx6_pcie_drvdata { > > struct imx6_pcie { > struct dw_pcie *pci; > - int reset_gpio; > + struct gpio_desc *reset_gpiod; > bool gpio_active_high; > bool link_is_up; > struct clk_bulk_data clks[IMX6_PCIE_MAX_CLKS]; > @@ -721,9 +720,8 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie) > } > > /* Some boards don't have PCIe reset GPIO. */ > - if (gpio_is_valid(imx6_pcie->reset_gpio)) > - gpio_set_value_cansleep(imx6_pcie->reset_gpio, > - imx6_pcie->gpio_active_high); > + gpiod_set_raw_value_cansleep(imx6_pcie->reset_gpiod, > + imx6_pcie->gpio_active_high); > } > > static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) > @@ -771,10 +769,10 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) > } > > /* Some boards don't have PCIe reset GPIO. */ > - if (gpio_is_valid(imx6_pcie->reset_gpio)) { > + if (imx6_pcie->reset_gpiod) { > msleep(100); > - gpio_set_value_cansleep(imx6_pcie->reset_gpio, > - !imx6_pcie->gpio_active_high); > + gpiod_set_raw_value_cansleep(imx6_pcie->reset_gpiod, > + !imx6_pcie->gpio_active_high); > /* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */ > msleep(100); > } > @@ -1285,22 +1283,15 @@ static int imx6_pcie_probe(struct platform_device *pdev) > return PTR_ERR(pci->dbi_base); > > /* Fetch GPIOs */ > - imx6_pcie->reset_gpio = of_get_named_gpio(node, "reset-gpio", 0); > imx6_pcie->gpio_active_high = of_property_read_bool(node, > "reset-gpio-active-high"); > - if (gpio_is_valid(imx6_pcie->reset_gpio)) { > - ret = devm_gpio_request_one(dev, imx6_pcie->reset_gpio, > - imx6_pcie->gpio_active_high ? > - GPIOF_OUT_INIT_HIGH : > - GPIOF_OUT_INIT_LOW, > - "PCIe reset"); > - if (ret) { > - dev_err(dev, "unable to get reset gpio\n"); > - return ret; > - } > - } else if (imx6_pcie->reset_gpio == -EPROBE_DEFER) { > - return imx6_pcie->reset_gpio; > - } > + imx6_pcie->reset_gpiod = > + devm_gpiod_get_optional(dev, "reset", > + imx6_pcie->gpio_active_high ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW); > + if (IS_ERR(imx6_pcie->reset_gpiod)) > + return dev_err_probe(dev, PTR_ERR(imx6_pcie->reset_gpiod), > + "unable to get reset gpio\n"); > + gpiod_set_consumer_name(imx6_pcie->reset_gpiod, "PCIe reset"); > > if (imx6_pcie->drvdata->clks_cnt >= IMX6_PCIE_MAX_CLKS) > return dev_err_probe(dev, -ENOMEM, "clks_cnt is too big\n"); > -- > 2.43.0.rc1.1336.g36b5255a03ac > >
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c index 917c69edee1d..d620f1e1a43c 100644 --- a/drivers/pci/controller/dwc/pci-imx6.c +++ b/drivers/pci/controller/dwc/pci-imx6.c @@ -11,14 +11,13 @@ #include <linux/bitfield.h> #include <linux/clk.h> #include <linux/delay.h> -#include <linux/gpio.h> +#include <linux/gpio/consumer.h> #include <linux/kernel.h> #include <linux/mfd/syscon.h> #include <linux/mfd/syscon/imx6q-iomuxc-gpr.h> #include <linux/mfd/syscon/imx7-iomuxc-gpr.h> #include <linux/module.h> #include <linux/of.h> -#include <linux/of_gpio.h> #include <linux/of_address.h> #include <linux/pci.h> #include <linux/platform_device.h> @@ -107,7 +106,7 @@ struct imx6_pcie_drvdata { struct imx6_pcie { struct dw_pcie *pci; - int reset_gpio; + struct gpio_desc *reset_gpiod; bool gpio_active_high; bool link_is_up; struct clk_bulk_data clks[IMX6_PCIE_MAX_CLKS]; @@ -721,9 +720,8 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie) } /* Some boards don't have PCIe reset GPIO. */ - if (gpio_is_valid(imx6_pcie->reset_gpio)) - gpio_set_value_cansleep(imx6_pcie->reset_gpio, - imx6_pcie->gpio_active_high); + gpiod_set_raw_value_cansleep(imx6_pcie->reset_gpiod, + imx6_pcie->gpio_active_high); } static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) @@ -771,10 +769,10 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) } /* Some boards don't have PCIe reset GPIO. */ - if (gpio_is_valid(imx6_pcie->reset_gpio)) { + if (imx6_pcie->reset_gpiod) { msleep(100); - gpio_set_value_cansleep(imx6_pcie->reset_gpio, - !imx6_pcie->gpio_active_high); + gpiod_set_raw_value_cansleep(imx6_pcie->reset_gpiod, + !imx6_pcie->gpio_active_high); /* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */ msleep(100); } @@ -1285,22 +1283,15 @@ static int imx6_pcie_probe(struct platform_device *pdev) return PTR_ERR(pci->dbi_base); /* Fetch GPIOs */ - imx6_pcie->reset_gpio = of_get_named_gpio(node, "reset-gpio", 0); imx6_pcie->gpio_active_high = of_property_read_bool(node, "reset-gpio-active-high"); - if (gpio_is_valid(imx6_pcie->reset_gpio)) { - ret = devm_gpio_request_one(dev, imx6_pcie->reset_gpio, - imx6_pcie->gpio_active_high ? - GPIOF_OUT_INIT_HIGH : - GPIOF_OUT_INIT_LOW, - "PCIe reset"); - if (ret) { - dev_err(dev, "unable to get reset gpio\n"); - return ret; - } - } else if (imx6_pcie->reset_gpio == -EPROBE_DEFER) { - return imx6_pcie->reset_gpio; - } + imx6_pcie->reset_gpiod = + devm_gpiod_get_optional(dev, "reset", + imx6_pcie->gpio_active_high ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW); + if (IS_ERR(imx6_pcie->reset_gpiod)) + return dev_err_probe(dev, PTR_ERR(imx6_pcie->reset_gpiod), + "unable to get reset gpio\n"); + gpiod_set_consumer_name(imx6_pcie->reset_gpiod, "PCIe reset"); if (imx6_pcie->drvdata->clks_cnt >= IMX6_PCIE_MAX_CLKS) return dev_err_probe(dev, -ENOMEM, "clks_cnt is too big\n");
The of_gpio.h is going to be removed. In preparation of that convert the driver to the agnostic API. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/pci/controller/dwc/pci-imx6.c | 37 ++++++++++----------------- 1 file changed, 14 insertions(+), 23 deletions(-)