diff mbox series

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

Message ID 20201118071724.4866-2-wens@kernel.org
State Superseded
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);