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

[Paul Cercueil <paul@crapouillou.net>]

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