diff mbox series

[1/3] net: davicom: dm9000: switch to using gpiod API

Message ID 20220906204922.3789922-1-dmitry.torokhov@gmail.com (mailing list archive)
State Accepted
Commit db49ca38579d44db7c246c5b74824b6406319b40
Delegated to: Netdev Maintainers
Headers show
Series [1/3] net: davicom: dm9000: switch to using gpiod API | expand

Checks

Context Check Description
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter warning Series does not have a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 7 maintainers not CCed: edumazet@google.com pabeni@redhat.com khalasa@piap.pl mw@semihalf.com kuba@kernel.org petrm@nvidia.com wsa+renesas@sang-engineering.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 50 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Dmitry Torokhov Sept. 6, 2022, 8:49 p.m. UTC
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/net/ethernet/davicom/dm9000.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

Comments

Linus Walleij Sept. 7, 2022, 9:45 p.m. UTC | #1
On Tue, Sep 6, 2022 at 10:49 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>

I think net patches need [PATCH net-next] in the subject to get
applied.

Yours,
Linus Walleij
Andrew Lunn Sept. 8, 2022, 12:06 p.m. UTC | #2
On Wed, Sep 07, 2022 at 11:45:48PM +0200, Linus Walleij wrote:
> On Tue, Sep 6, 2022 at 10:49 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>
> 
> I think net patches need [PATCH net-next] in the subject to get
> applied.

Correct.

https://docs.kernel.org/process/maintainer-netdev.html

For the odd drive by patch, the Maintainers often do accept patches
without it. So wait and see.

	Andrew
Paolo Abeni Sept. 15, 2022, 9:58 a.m. UTC | #3
On Thu, 2022-09-08 at 14:06 +0200, Andrew Lunn wrote:
> On Wed, Sep 07, 2022 at 11:45:48PM +0200, Linus Walleij wrote:
> > On Tue, Sep 6, 2022 at 10:49 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>
> > 
> > I think net patches need [PATCH net-next] in the subject to get
> > applied.
> 
> Correct.
> 
> https://docs.kernel.org/process/maintainer-netdev.html
> 
> For the odd drive by patch, the Maintainers often do accept patches
> without it. So wait and see.

Due to a series of unfortunate coincidences the netdev backlog has
grown unusually long. Since the patches LGMT I'm going to apply them to
net-next without asking a repost.

@Dmitry: next time, please add the relevant tag, thanks!

Paolo
patchwork-bot+netdevbpf@kernel.org Sept. 15, 2022, 10:10 a.m. UTC | #4
Hello:

This series was applied to netdev/net-next.git (master)
by Paolo Abeni <pabeni@redhat.com>:

On Tue,  6 Sep 2022 13:49:20 -0700 you 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>
> ---
>  drivers/net/ethernet/davicom/dm9000.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)

Here is the summary with links:
  - [1/3] net: davicom: dm9000: switch to using gpiod API
    https://git.kernel.org/netdev/net-next/c/db49ca38579d
  - [2/3] net: ks8851: switch to using gpiod API
    https://git.kernel.org/netdev/net-next/c/7b77bb5c8130
  - [3/3] net: phy: spi_ks8895: switch to using gpiod API
    https://git.kernel.org/netdev/net-next/c/006534ec2804

You are awesome, thank you!
Paul Cercueil Nov. 18, 2022, 3:33 p.m. UTC | #5
Hi Dmitry,

Le mar. 6 sept. 2022 à 13:49:20 -0700, Dmitry Torokhov 
<dmitry.torokhov@gmail.com> a écrit :
> 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/net/ethernet/davicom/dm9000.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/davicom/dm9000.c 
> b/drivers/net/ethernet/davicom/dm9000.c
> index 77229e53b04e..c85a6ebd79fc 100644
> --- a/drivers/net/ethernet/davicom/dm9000.c
> +++ b/drivers/net/ethernet/davicom/dm9000.c
> @@ -28,8 +28,7 @@
>  #include <linux/irq.h>
>  #include <linux/slab.h>
>  #include <linux/regulator/consumer.h>
> -#include <linux/gpio.h>
> -#include <linux/of_gpio.h>
> +#include <linux/gpio/consumer.h>
> 
>  #include <asm/delay.h>
>  #include <asm/irq.h>
> @@ -1421,8 +1420,7 @@ dm9000_probe(struct platform_device *pdev)
>  	int iosize;
>  	int i;
>  	u32 id_val;
> -	int reset_gpios;
> -	enum of_gpio_flags flags;
> +	struct gpio_desc *reset_gpio;
>  	struct regulator *power;
>  	bool inv_mac_addr = false;
>  	u8 addr[ETH_ALEN];
> @@ -1442,20 +1440,24 @@ dm9000_probe(struct platform_device *pdev)
>  		dev_dbg(dev, "regulator enabled\n");
>  	}
> 
> -	reset_gpios = of_get_named_gpio_flags(dev->of_node, "reset-gpios", 
> 0,
> -					      &flags);
> -	if (gpio_is_valid(reset_gpios)) {
> -		ret = devm_gpio_request_one(dev, reset_gpios, flags,
> -					    "dm9000_reset");
> +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	ret = PTR_ERR_OR_ZERO(reset_gpio);
> +	if (ret) {
> +		dev_err(dev, "failed to request reset gpio: %d\n", ret);
> +		goto out_regulator_disable;
> +	}
> +
> +	if (reset_gpio) {
> +		ret = gpiod_set_consumer_name(reset_gpio, "dm9000_reset");
>  		if (ret) {
> -			dev_err(dev, "failed to request reset gpio %d: %d\n",
> -				reset_gpios, ret);
> +			dev_err(dev, "failed to set reset gpio name: %d\n",
> +				ret);
>  			goto out_regulator_disable;
>  		}
> 
>  		/* According to manual PWRST# Low Period Min 1ms */
>  		msleep(2);
> -		gpio_set_value(reset_gpios, 1);
> +		gpiod_set_value_cansleep(reset_gpio, 0);

Why is that 1 magically turned into a 0?

On my CI20 board I can't get the DM9000 chip to probe correctly with 
this patch (it fails to read the ID).
If I revert this patch then everything works fine.

Cheers,
-Paul

>  		/* Needs 3ms to read eeprom when PWRST is deasserted */
>  		msleep(4);
>  	}
> --
> 2.37.2.789.g6183377224-goog
>
Dmitry Torokhov Nov. 18, 2022, 3:58 p.m. UTC | #6
Hi Paul,

On Fri, Nov 18, 2022 at 03:33:44PM +0000, Paul Cercueil wrote:
> Hi Dmitry,
> 
> Le mar. 6 sept. 2022 à 13:49:20 -0700, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> a écrit :
> > 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/net/ethernet/davicom/dm9000.c | 26 ++++++++++++++------------
> >  1 file changed, 14 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/davicom/dm9000.c
> > b/drivers/net/ethernet/davicom/dm9000.c
> > index 77229e53b04e..c85a6ebd79fc 100644
> > --- a/drivers/net/ethernet/davicom/dm9000.c
> > +++ b/drivers/net/ethernet/davicom/dm9000.c
> > @@ -28,8 +28,7 @@
> >  #include <linux/irq.h>
> >  #include <linux/slab.h>
> >  #include <linux/regulator/consumer.h>
> > -#include <linux/gpio.h>
> > -#include <linux/of_gpio.h>
> > +#include <linux/gpio/consumer.h>
> > 
> >  #include <asm/delay.h>
> >  #include <asm/irq.h>
> > @@ -1421,8 +1420,7 @@ dm9000_probe(struct platform_device *pdev)
> >  	int iosize;
> >  	int i;
> >  	u32 id_val;
> > -	int reset_gpios;
> > -	enum of_gpio_flags flags;
> > +	struct gpio_desc *reset_gpio;
> >  	struct regulator *power;
> >  	bool inv_mac_addr = false;
> >  	u8 addr[ETH_ALEN];
> > @@ -1442,20 +1440,24 @@ dm9000_probe(struct platform_device *pdev)
> >  		dev_dbg(dev, "regulator enabled\n");
> >  	}
> > 
> > -	reset_gpios = of_get_named_gpio_flags(dev->of_node, "reset-gpios", 0,
> > -					      &flags);
> > -	if (gpio_is_valid(reset_gpios)) {
> > -		ret = devm_gpio_request_one(dev, reset_gpios, flags,
> > -					    "dm9000_reset");
> > +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > +	ret = PTR_ERR_OR_ZERO(reset_gpio);
> > +	if (ret) {
> > +		dev_err(dev, "failed to request reset gpio: %d\n", ret);
> > +		goto out_regulator_disable;
> > +	}
> > +
> > +	if (reset_gpio) {
> > +		ret = gpiod_set_consumer_name(reset_gpio, "dm9000_reset");
> >  		if (ret) {
> > -			dev_err(dev, "failed to request reset gpio %d: %d\n",
> > -				reset_gpios, ret);
> > +			dev_err(dev, "failed to set reset gpio name: %d\n",
> > +				ret);
> >  			goto out_regulator_disable;
> >  		}
> > 
> >  		/* According to manual PWRST# Low Period Min 1ms */
> >  		msleep(2);
> > -		gpio_set_value(reset_gpios, 1);
> > +		gpiod_set_value_cansleep(reset_gpio, 0);
> 
> Why is that 1 magically turned into a 0?

Because gpiod uses logical states (think active/inactive), not absolute
ones. Here we are deasserting the reset line.

> 
> On my CI20 board I can't get the DM9000 chip to probe correctly with this
> patch (it fails to read the ID).
> If I revert this patch then everything works fine.

Sorry, it is my fault of course: I missed that board has incorrect
annotation for the reset line. I will send out the patch below
(formatted properly of course):

diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
index 37c46720c719..f38c39572a9e 100644
--- a/arch/mips/boot/dts/ingenic/ci20.dts
+++ b/arch/mips/boot/dts/ingenic/ci20.dts
@@ -438,7 +438,7 @@ dm9000@6 {
 		ingenic,nemc-tAW = <50>;
 		ingenic,nemc-tSTRV = <100>;
 
-		reset-gpios = <&gpf 12 GPIO_ACTIVE_HIGH>;
+		reset-gpios = <&gpf 12 GPIO_ACTIVE_LOW>;
 		vcc-supply = <&eth0_power>;
 
 		interrupt-parent = <&gpe>;


Thanks.
Paul Cercueil Nov. 18, 2022, 4:26 p.m. UTC | #7
Le ven. 18 nov. 2022 à 07:58:21 -0800, Dmitry Torokhov 
<dmitry.torokhov@gmail.com> a écrit :
> Hi Paul,
> 
> On Fri, Nov 18, 2022 at 03:33:44PM +0000, Paul Cercueil wrote:
>>  Hi Dmitry,
>> 
>>  Le mar. 6 sept. 2022 à 13:49:20 -0700, Dmitry Torokhov
>>  <dmitry.torokhov@gmail.com> a écrit :
>>  > 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/net/ethernet/davicom/dm9000.c | 26 
>> ++++++++++++++------------
>>  >  1 file changed, 14 insertions(+), 12 deletions(-)
>>  >
>>  > diff --git a/drivers/net/ethernet/davicom/dm9000.c
>>  > b/drivers/net/ethernet/davicom/dm9000.c
>>  > index 77229e53b04e..c85a6ebd79fc 100644
>>  > --- a/drivers/net/ethernet/davicom/dm9000.c
>>  > +++ b/drivers/net/ethernet/davicom/dm9000.c
>>  > @@ -28,8 +28,7 @@
>>  >  #include <linux/irq.h>
>>  >  #include <linux/slab.h>
>>  >  #include <linux/regulator/consumer.h>
>>  > -#include <linux/gpio.h>
>>  > -#include <linux/of_gpio.h>
>>  > +#include <linux/gpio/consumer.h>
>>  >
>>  >  #include <asm/delay.h>
>>  >  #include <asm/irq.h>
>>  > @@ -1421,8 +1420,7 @@ dm9000_probe(struct platform_device *pdev)
>>  >  	int iosize;
>>  >  	int i;
>>  >  	u32 id_val;
>>  > -	int reset_gpios;
>>  > -	enum of_gpio_flags flags;
>>  > +	struct gpio_desc *reset_gpio;
>>  >  	struct regulator *power;
>>  >  	bool inv_mac_addr = false;
>>  >  	u8 addr[ETH_ALEN];
>>  > @@ -1442,20 +1440,24 @@ dm9000_probe(struct platform_device *pdev)
>>  >  		dev_dbg(dev, "regulator enabled\n");
>>  >  	}
>>  >
>>  > -	reset_gpios = of_get_named_gpio_flags(dev->of_node, 
>> "reset-gpios", 0,
>>  > -					      &flags);
>>  > -	if (gpio_is_valid(reset_gpios)) {
>>  > -		ret = devm_gpio_request_one(dev, reset_gpios, flags,
>>  > -					    "dm9000_reset");
>>  > +	reset_gpio = devm_gpiod_get_optional(dev, "reset", 
>> GPIOD_OUT_HIGH);
>>  > +	ret = PTR_ERR_OR_ZERO(reset_gpio);
>>  > +	if (ret) {
>>  > +		dev_err(dev, "failed to request reset gpio: %d\n", ret);
>>  > +		goto out_regulator_disable;
>>  > +	}
>>  > +
>>  > +	if (reset_gpio) {
>>  > +		ret = gpiod_set_consumer_name(reset_gpio, "dm9000_reset");
>>  >  		if (ret) {
>>  > -			dev_err(dev, "failed to request reset gpio %d: %d\n",
>>  > -				reset_gpios, ret);
>>  > +			dev_err(dev, "failed to set reset gpio name: %d\n",
>>  > +				ret);
>>  >  			goto out_regulator_disable;
>>  >  		}
>>  >
>>  >  		/* According to manual PWRST# Low Period Min 1ms */
>>  >  		msleep(2);
>>  > -		gpio_set_value(reset_gpios, 1);
>>  > +		gpiod_set_value_cansleep(reset_gpio, 0);
>> 
>>  Why is that 1 magically turned into a 0?
> 
> Because gpiod uses logical states (think active/inactive), not 
> absolute
> ones. Here we are deasserting the reset line.
> 
>> 
>>  On my CI20 board I can't get the DM9000 chip to probe correctly 
>> with this
>>  patch (it fails to read the ID).
>>  If I revert this patch then everything works fine.
> 
> Sorry, it is my fault of course: I missed that board has incorrect
> annotation for the reset line. I will send out the patch below
> (formatted properly of course):

So in *theory* you wouldn't fix it like that, because the driver should 
work with old Device Tree files, even if it had a broken property, as 
long as it used to work in the past.

The ci20.dts file however is always built into the kernel and I'm not 
aware of anybody doing things differently. As long as you make that 
explicit in your commit message I think Rob won't mind.

If he does, or if more boards are affected, an alternative is to switch 
the polarity of the GPIO in the driver, like so:

if (of_machine_is_compatible("mips,ci20") &&
    gpiod_is_active_low(reset_gpio)) {
	gpiod_toggle_active_low(reset_gpio);
}

Cheers,
-Paul

> diff --git a/arch/mips/boot/dts/ingenic/ci20.dts 
> b/arch/mips/boot/dts/ingenic/ci20.dts
> index 37c46720c719..f38c39572a9e 100644
> --- a/arch/mips/boot/dts/ingenic/ci20.dts
> +++ b/arch/mips/boot/dts/ingenic/ci20.dts
> @@ -438,7 +438,7 @@ dm9000@6 {
>  		ingenic,nemc-tAW = <50>;
>  		ingenic,nemc-tSTRV = <100>;
> 
> -		reset-gpios = <&gpf 12 GPIO_ACTIVE_HIGH>;
> +		reset-gpios = <&gpf 12 GPIO_ACTIVE_LOW>;
>  		vcc-supply = <&eth0_power>;
> 
>  		interrupt-parent = <&gpe>;
> 
> 
> Thanks.
> 
> --
> Dmitry
Dmitry Torokhov Nov. 18, 2022, 4:30 p.m. UTC | #8
On Fri, Nov 18, 2022 at 04:26:38PM +0000, Paul Cercueil wrote:
> 
> 
> Le ven. 18 nov. 2022 à 07:58:21 -0800, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> a écrit :
> > Hi Paul,
> > 
> > On Fri, Nov 18, 2022 at 03:33:44PM +0000, Paul Cercueil wrote:
> > >  Hi Dmitry,
> > > 
> > >  Le mar. 6 sept. 2022 à 13:49:20 -0700, Dmitry Torokhov
> > >  <dmitry.torokhov@gmail.com> a écrit :
> > >  > 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/net/ethernet/davicom/dm9000.c | 26
> > > ++++++++++++++------------
> > >  >  1 file changed, 14 insertions(+), 12 deletions(-)
> > >  >
> > >  > diff --git a/drivers/net/ethernet/davicom/dm9000.c
> > >  > b/drivers/net/ethernet/davicom/dm9000.c
> > >  > index 77229e53b04e..c85a6ebd79fc 100644
> > >  > --- a/drivers/net/ethernet/davicom/dm9000.c
> > >  > +++ b/drivers/net/ethernet/davicom/dm9000.c
> > >  > @@ -28,8 +28,7 @@
> > >  >  #include <linux/irq.h>
> > >  >  #include <linux/slab.h>
> > >  >  #include <linux/regulator/consumer.h>
> > >  > -#include <linux/gpio.h>
> > >  > -#include <linux/of_gpio.h>
> > >  > +#include <linux/gpio/consumer.h>
> > >  >
> > >  >  #include <asm/delay.h>
> > >  >  #include <asm/irq.h>
> > >  > @@ -1421,8 +1420,7 @@ dm9000_probe(struct platform_device *pdev)
> > >  >  	int iosize;
> > >  >  	int i;
> > >  >  	u32 id_val;
> > >  > -	int reset_gpios;
> > >  > -	enum of_gpio_flags flags;
> > >  > +	struct gpio_desc *reset_gpio;
> > >  >  	struct regulator *power;
> > >  >  	bool inv_mac_addr = false;
> > >  >  	u8 addr[ETH_ALEN];
> > >  > @@ -1442,20 +1440,24 @@ dm9000_probe(struct platform_device *pdev)
> > >  >  		dev_dbg(dev, "regulator enabled\n");
> > >  >  	}
> > >  >
> > >  > -	reset_gpios = of_get_named_gpio_flags(dev->of_node,
> > > "reset-gpios", 0,
> > >  > -					      &flags);
> > >  > -	if (gpio_is_valid(reset_gpios)) {
> > >  > -		ret = devm_gpio_request_one(dev, reset_gpios, flags,
> > >  > -					    "dm9000_reset");
> > >  > +	reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > > GPIOD_OUT_HIGH);
> > >  > +	ret = PTR_ERR_OR_ZERO(reset_gpio);
> > >  > +	if (ret) {
> > >  > +		dev_err(dev, "failed to request reset gpio: %d\n", ret);
> > >  > +		goto out_regulator_disable;
> > >  > +	}
> > >  > +
> > >  > +	if (reset_gpio) {
> > >  > +		ret = gpiod_set_consumer_name(reset_gpio, "dm9000_reset");
> > >  >  		if (ret) {
> > >  > -			dev_err(dev, "failed to request reset gpio %d: %d\n",
> > >  > -				reset_gpios, ret);
> > >  > +			dev_err(dev, "failed to set reset gpio name: %d\n",
> > >  > +				ret);
> > >  >  			goto out_regulator_disable;
> > >  >  		}
> > >  >
> > >  >  		/* According to manual PWRST# Low Period Min 1ms */
> > >  >  		msleep(2);
> > >  > -		gpio_set_value(reset_gpios, 1);
> > >  > +		gpiod_set_value_cansleep(reset_gpio, 0);
> > > 
> > >  Why is that 1 magically turned into a 0?
> > 
> > Because gpiod uses logical states (think active/inactive), not absolute
> > ones. Here we are deasserting the reset line.
> > 
> > > 
> > >  On my CI20 board I can't get the DM9000 chip to probe correctly
> > > with this
> > >  patch (it fails to read the ID).
> > >  If I revert this patch then everything works fine.
> > 
> > Sorry, it is my fault of course: I missed that board has incorrect
> > annotation for the reset line. I will send out the patch below
> > (formatted properly of course):
> 
> So in *theory* you wouldn't fix it like that, because the driver should work
> with old Device Tree files, even if it had a broken property, as long as it
> used to work in the past.
> 
> The ci20.dts file however is always built into the kernel and I'm not aware
> of anybody doing things differently. As long as you make that explicit in
> your commit message I think Rob won't mind.
> 
> If he does, or if more boards are affected, an alternative is to switch the
> polarity of the GPIO in the driver, like so:
> 
> if (of_machine_is_compatible("mips,ci20") &&
>    gpiod_is_active_low(reset_gpio)) {
> 	gpiod_toggle_active_low(reset_gpio);
> }

Right, we are typically hiding this kind of quirks in gpiolib-of instead
of polluting drivers, but yes, it is possible.

Thanks.
Andrew Lunn Nov. 18, 2022, 5:50 p.m. UTC | #9
> > Why is that 1 magically turned into a 0?
> 
> Because gpiod uses logical states (think active/inactive), not absolute
> ones. Here we are deasserting the reset line.

This is the same question/answer you had with me. Maybe it is worth
putting this into the commit message for other patches in your series
to prevent this question/answer again and again.

   Andrew
Dmitry Torokhov Nov. 18, 2022, 5:55 p.m. UTC | #10
On Fri, Nov 18, 2022 at 06:50:54PM +0100, Andrew Lunn wrote:
> > > Why is that 1 magically turned into a 0?
> > 
> > Because gpiod uses logical states (think active/inactive), not absolute
> > ones. Here we are deasserting the reset line.
> 
> This is the same question/answer you had with me. Maybe it is worth
> putting this into the commit message for other patches in your series
> to prevent this question/answer again and again.

Right... Actually I think I'll go and define that GPIO_STATE_ACTIVE/
GPIO_STATE_INACTIVE and try to get Linus and Bart to accept it as code
speaks louder than words ;)

Thanks.
Linus Walleij Nov. 21, 2022, 1:35 p.m. UTC | #11
On Fri, Nov 18, 2022 at 6:55 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Nov 18, 2022 at 06:50:54PM +0100, Andrew Lunn wrote:
> > > > Why is that 1 magically turned into a 0?
> > >
> > > Because gpiod uses logical states (think active/inactive), not absolute
> > > ones. Here we are deasserting the reset line.
> >
> > This is the same question/answer you had with me. Maybe it is worth
> > putting this into the commit message for other patches in your series
> > to prevent this question/answer again and again.
>
> Right... Actually I think I'll go and define that GPIO_STATE_ACTIVE/
> GPIO_STATE_INACTIVE and try to get Linus and Bart to accept it as code
> speaks louder than words ;)

What I have said about that is that it should be accompanied by some sed
or cocinelle script to change this everywhere in the kernel instead
of using 0/1 to the gpiod_set/direction etc functions. Then Torvalds
can run that toward the end of the merge window to just change this
everywhere at once and be done with it.

The reason I want it that way is that I am royally tired of changes that
begin in one tiny corner and then the change keeps confusing users
for years until it is finally fixed up 15 kernel revisions later.

Since that has created a support nightmare in the past, I am now
advocating an all-or-nothing approach with that type of change.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
index 77229e53b04e..c85a6ebd79fc 100644
--- a/drivers/net/ethernet/davicom/dm9000.c
+++ b/drivers/net/ethernet/davicom/dm9000.c
@@ -28,8 +28,7 @@ 
 #include <linux/irq.h>
 #include <linux/slab.h>
 #include <linux/regulator/consumer.h>
-#include <linux/gpio.h>
-#include <linux/of_gpio.h>
+#include <linux/gpio/consumer.h>
 
 #include <asm/delay.h>
 #include <asm/irq.h>
@@ -1421,8 +1420,7 @@  dm9000_probe(struct platform_device *pdev)
 	int iosize;
 	int i;
 	u32 id_val;
-	int reset_gpios;
-	enum of_gpio_flags flags;
+	struct gpio_desc *reset_gpio;
 	struct regulator *power;
 	bool inv_mac_addr = false;
 	u8 addr[ETH_ALEN];
@@ -1442,20 +1440,24 @@  dm9000_probe(struct platform_device *pdev)
 		dev_dbg(dev, "regulator enabled\n");
 	}
 
-	reset_gpios = of_get_named_gpio_flags(dev->of_node, "reset-gpios", 0,
-					      &flags);
-	if (gpio_is_valid(reset_gpios)) {
-		ret = devm_gpio_request_one(dev, reset_gpios, flags,
-					    "dm9000_reset");
+	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	ret = PTR_ERR_OR_ZERO(reset_gpio);
+	if (ret) {
+		dev_err(dev, "failed to request reset gpio: %d\n", ret);
+		goto out_regulator_disable;
+	}
+
+	if (reset_gpio) {
+		ret = gpiod_set_consumer_name(reset_gpio, "dm9000_reset");
 		if (ret) {
-			dev_err(dev, "failed to request reset gpio %d: %d\n",
-				reset_gpios, ret);
+			dev_err(dev, "failed to set reset gpio name: %d\n",
+				ret);
 			goto out_regulator_disable;
 		}
 
 		/* According to manual PWRST# Low Period Min 1ms */
 		msleep(2);
-		gpio_set_value(reset_gpios, 1);
+		gpiod_set_value_cansleep(reset_gpio, 0);
 		/* Needs 3ms to read eeprom when PWRST is deasserted */
 		msleep(4);
 	}