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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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.

-- 
Dmitry