diff mbox series

[v2,1/4] PCI: rockchip: Make 'ep-gpios' DT property optional

Message ID 20201118071724.4866-2-wens@kernel.org (mailing list archive)
State Superseded, archived
Headers show
Series arm64: rockchip: Fix PCIe ep-gpios requirement and Add Nanopi M4B | expand

Commit Message

Chen-Yu Tsai Nov. 18, 2020, 7:17 a.m. UTC 
From: Chen-Yu Tsai <wens@csie.org>

The Rockchip PCIe controller DT binding clearly states that 'ep-gpios' is
an optional property. And indeed there are boards that don't require it.

Make the driver follow the binding by using devm_gpiod_get_optional()
instead of devm_gpiod_get().

Fixes: e77f847df54c ("PCI: rockchip: Add Rockchip PCIe controller support")
Fixes: 956cd99b35a8 ("PCI: rockchip: Separate common code from RC driver")
Fixes: 964bac9455be ("PCI: rockchip: Split out rockchip_pcie_parse_dt() to parse DT")
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
Changes since v1:

  - Rewrite subject to match existing convention and reference
    'ep-gpios' DT property instead of the 'ep_gpio' field
---
 drivers/pci/controller/pcie-rockchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Heiko Stuebner Nov. 18, 2020, 8:49 a.m. UTC | #1 
Am Mittwoch, 18. November 2020, 08:17:21 CET schrieb Chen-Yu Tsai:
> From: Chen-Yu Tsai <wens@csie.org>
> 
> The Rockchip PCIe controller DT binding clearly states that 'ep-gpios' is
> an optional property. And indeed there are boards that don't require it.
> 
> Make the driver follow the binding by using devm_gpiod_get_optional()
> instead of devm_gpiod_get().
> 
> Fixes: e77f847df54c ("PCI: rockchip: Add Rockchip PCIe controller support")
> Fixes: 956cd99b35a8 ("PCI: rockchip: Separate common code from RC driver")
> Fixes: 964bac9455be ("PCI: rockchip: Split out rockchip_pcie_parse_dt() to parse DT")
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Reviewed-by: Heiko Stuebner <heiko@sntech.de


I'll pick up patches 2-4 separately, after giving Rob a chance to look at
the simple binding.


Heiko

> ---
> Changes since v1:
> 
>   - Rewrite subject to match existing convention and reference
>     'ep-gpios' DT property instead of the 'ep_gpio' field
> ---
>  drivers/pci/controller/pcie-rockchip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c
> index 904dec0d3a88..c95950e9004f 100644
> --- a/drivers/pci/controller/pcie-rockchip.c
> +++ b/drivers/pci/controller/pcie-rockchip.c
> @@ -118,7 +118,7 @@ int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
>  	}
>  
>  	if (rockchip->is_rc) {
> -		rockchip->ep_gpio = devm_gpiod_get(dev, "ep", GPIOD_OUT_HIGH);
> +		rockchip->ep_gpio = devm_gpiod_get_optional(dev, "ep", GPIOD_OUT_HIGH);
>  		if (IS_ERR(rockchip->ep_gpio)) {
>  			dev_err(dev, "missing ep-gpios property in node\n");
>  			return PTR_ERR(rockchip->ep_gpio);
>
Chen-Yu Tsai Dec. 7, 2020, 3:30 a.m. UTC | #2 
Ping

On Wed, Nov 18, 2020 at 4:49 PM Heiko Stübner <heiko@sntech.de> wrote:
>
> Am Mittwoch, 18. November 2020, 08:17:21 CET schrieb Chen-Yu Tsai:
> > From: Chen-Yu Tsai <wens@csie.org>
> >
> > The Rockchip PCIe controller DT binding clearly states that 'ep-gpios' is
> > an optional property. And indeed there are boards that don't require it.
> >
> > Make the driver follow the binding by using devm_gpiod_get_optional()
> > instead of devm_gpiod_get().
> >
> > Fixes: e77f847df54c ("PCI: rockchip: Add Rockchip PCIe controller support")
> > Fixes: 956cd99b35a8 ("PCI: rockchip: Separate common code from RC driver")
> > Fixes: 964bac9455be ("PCI: rockchip: Split out rockchip_pcie_parse_dt() to parse DT")
> > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de

It's been close to three weeks since this was sent.
Any chance we can get this into v5.10 or v5.11?


Regards
ChenYu

> I'll pick up patches 2-4 separately, after giving Rob a chance to look at
> the simple binding.
>
>
> Heiko
>
> > ---
> > Changes since v1:
> >
> >   - Rewrite subject to match existing convention and reference
> >     'ep-gpios' DT property instead of the 'ep_gpio' field
> > ---
> >  drivers/pci/controller/pcie-rockchip.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c
> > index 904dec0d3a88..c95950e9004f 100644
> > --- a/drivers/pci/controller/pcie-rockchip.c
> > +++ b/drivers/pci/controller/pcie-rockchip.c
> > @@ -118,7 +118,7 @@ int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
> >       }
> >
> >       if (rockchip->is_rc) {
> > -             rockchip->ep_gpio = devm_gpiod_get(dev, "ep", GPIOD_OUT_HIGH);
> > +             rockchip->ep_gpio = devm_gpiod_get_optional(dev, "ep", GPIOD_OUT_HIGH);
> >               if (IS_ERR(rockchip->ep_gpio)) {
> >                       dev_err(dev, "missing ep-gpios property in node\n");
> >                       return PTR_ERR(rockchip->ep_gpio);
> >
>
>
>
>
Rob Herring Dec. 7, 2020, 2:11 p.m. UTC | #3 
On Wed, Nov 18, 2020 at 1:17 AM Chen-Yu Tsai <wens@kernel.org> wrote:
>
> From: Chen-Yu Tsai <wens@csie.org>
>
> The Rockchip PCIe controller DT binding clearly states that 'ep-gpios' is
> an optional property. And indeed there are boards that don't require it.
>
> Make the driver follow the binding by using devm_gpiod_get_optional()
> instead of devm_gpiod_get().
>
> Fixes: e77f847df54c ("PCI: rockchip: Add Rockchip PCIe controller support")
> Fixes: 956cd99b35a8 ("PCI: rockchip: Separate common code from RC driver")
> Fixes: 964bac9455be ("PCI: rockchip: Split out rockchip_pcie_parse_dt() to parse DT")
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
> Changes since v1:
>
>   - Rewrite subject to match existing convention and reference
>     'ep-gpios' DT property instead of the 'ep_gpio' field
> ---
>  drivers/pci/controller/pcie-rockchip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c
> index 904dec0d3a88..c95950e9004f 100644
> --- a/drivers/pci/controller/pcie-rockchip.c
> +++ b/drivers/pci/controller/pcie-rockchip.c
> @@ -118,7 +118,7 @@ int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
>         }
>
>         if (rockchip->is_rc) {
> -               rockchip->ep_gpio = devm_gpiod_get(dev, "ep", GPIOD_OUT_HIGH);
> +               rockchip->ep_gpio = devm_gpiod_get_optional(dev, "ep", GPIOD_OUT_HIGH);
>                 if (IS_ERR(rockchip->ep_gpio)) {
>                         dev_err(dev, "missing ep-gpios property in node\n");

You should drop the error message. What it says is now never the
reason for the error and it could most likely be a deferred probe
which you don't want an error message for.

>                         return PTR_ERR(rockchip->ep_gpio);
> --
> 2.29.1
>
Chen-Yu Tsai Dec. 12, 2020, 3:18 p.m. UTC | #4 
On Mon, Dec 7, 2020 at 10:11 PM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Nov 18, 2020 at 1:17 AM Chen-Yu Tsai <wens@kernel.org> wrote:
> >
> > From: Chen-Yu Tsai <wens@csie.org>
> >
> > The Rockchip PCIe controller DT binding clearly states that 'ep-gpios' is
> > an optional property. And indeed there are boards that don't require it.
> >
> > Make the driver follow the binding by using devm_gpiod_get_optional()
> > instead of devm_gpiod_get().
> >
> > Fixes: e77f847df54c ("PCI: rockchip: Add Rockchip PCIe controller support")
> > Fixes: 956cd99b35a8 ("PCI: rockchip: Separate common code from RC driver")
> > Fixes: 964bac9455be ("PCI: rockchip: Split out rockchip_pcie_parse_dt() to parse DT")
> > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> > ---
> > Changes since v1:
> >
> >   - Rewrite subject to match existing convention and reference
> >     'ep-gpios' DT property instead of the 'ep_gpio' field
> > ---
> >  drivers/pci/controller/pcie-rockchip.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c
> > index 904dec0d3a88..c95950e9004f 100644
> > --- a/drivers/pci/controller/pcie-rockchip.c
> > +++ b/drivers/pci/controller/pcie-rockchip.c
> > @@ -118,7 +118,7 @@ int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
> >         }
> >
> >         if (rockchip->is_rc) {
> > -               rockchip->ep_gpio = devm_gpiod_get(dev, "ep", GPIOD_OUT_HIGH);
> > +               rockchip->ep_gpio = devm_gpiod_get_optional(dev, "ep", GPIOD_OUT_HIGH);
> >                 if (IS_ERR(rockchip->ep_gpio)) {
> >                         dev_err(dev, "missing ep-gpios property in node\n");
>
> You should drop the error message. What it says is now never the
> reason for the error and it could most likely be a deferred probe
> which you don't want an error message for.

What about switching to dev_err_probe() instead?

That way deferred probe errors get silenced (or get a better debug message),
and error messages for other issues, such as miswritten gpio properties are
still produced.

ChenYu

> >                         return PTR_ERR(rockchip->ep_gpio);
> > --
> > 2.29.1
> >
Rob Herring Dec. 14, 2020, 2:17 p.m. UTC | #5 
On Sat, Dec 12, 2020 at 9:18 AM Chen-Yu Tsai <wens@kernel.org> wrote:
>
> On Mon, Dec 7, 2020 at 10:11 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Wed, Nov 18, 2020 at 1:17 AM Chen-Yu Tsai <wens@kernel.org> wrote:
> > >
> > > From: Chen-Yu Tsai <wens@csie.org>
> > >
> > > The Rockchip PCIe controller DT binding clearly states that 'ep-gpios' is
> > > an optional property. And indeed there are boards that don't require it.
> > >
> > > Make the driver follow the binding by using devm_gpiod_get_optional()
> > > instead of devm_gpiod_get().
> > >
> > > Fixes: e77f847df54c ("PCI: rockchip: Add Rockchip PCIe controller support")
> > > Fixes: 956cd99b35a8 ("PCI: rockchip: Separate common code from RC driver")
> > > Fixes: 964bac9455be ("PCI: rockchip: Split out rockchip_pcie_parse_dt() to parse DT")
> > > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> > > ---
> > > Changes since v1:
> > >
> > >   - Rewrite subject to match existing convention and reference
> > >     'ep-gpios' DT property instead of the 'ep_gpio' field
> > > ---
> > >  drivers/pci/controller/pcie-rockchip.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c
> > > index 904dec0d3a88..c95950e9004f 100644
> > > --- a/drivers/pci/controller/pcie-rockchip.c
> > > +++ b/drivers/pci/controller/pcie-rockchip.c
> > > @@ -118,7 +118,7 @@ int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
> > >         }
> > >
> > >         if (rockchip->is_rc) {
> > > -               rockchip->ep_gpio = devm_gpiod_get(dev, "ep", GPIOD_OUT_HIGH);
> > > +               rockchip->ep_gpio = devm_gpiod_get_optional(dev, "ep", GPIOD_OUT_HIGH);
> > >                 if (IS_ERR(rockchip->ep_gpio)) {
> > >                         dev_err(dev, "missing ep-gpios property in node\n");
> >
> > You should drop the error message. What it says is now never the
> > reason for the error and it could most likely be a deferred probe
> > which you don't want an error message for.
>
> What about switching to dev_err_probe() instead?
>
> That way deferred probe errors get silenced (or get a better debug message),
> and error messages for other issues, such as miswritten gpio properties are
> still produced.

I guess, but those errors should be in the subsystem code IMO rather
than every driver.

Rob
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c
index 904dec0d3a88..c95950e9004f 100644
--- a/drivers/pci/controller/pcie-rockchip.c
+++ b/drivers/pci/controller/pcie-rockchip.c
@@ -118,7 +118,7 @@  int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
 	}
 
 	if (rockchip->is_rc) {
-		rockchip->ep_gpio = devm_gpiod_get(dev, "ep", GPIOD_OUT_HIGH);
+		rockchip->ep_gpio = devm_gpiod_get_optional(dev, "ep", GPIOD_OUT_HIGH);
 		if (IS_ERR(rockchip->ep_gpio)) {
 			dev_err(dev, "missing ep-gpios property in node\n");
 			return PTR_ERR(rockchip->ep_gpio);