Re: [PATCH 17/21] nfc: marvell: convert to gpio descriptors

[Dmitry Torokhov]

On Sat, Aug 09, 2025 at 01:09:47PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 08, 2025 at 05:18:01PM +0200, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@xxxxxxxx>
> > 
> > The only reason this driver seems to still use the legacy gpio
> > numbers is for the module parameter that lets users pass a different
> > reset gpio.
> > 
> > Since the fixed numbers are on their way out, and none of the platforms
> > this driver is used on would have them any more, remove the module
> > parameter and instead just use the reset information from firmware.
> 
> This patch is my love in the series, thanks for doing it!
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> 
> But note some comments below.
> 
> ...
> 
> > -	if (gpio_is_valid(priv->config.reset_n_io)) {
> > -		rc = gpio_request_one(priv->config.reset_n_io,
> > -				      GPIOF_OUT_INIT_LOW,
> > -				      "nfcmrvl_reset_n");
> > -		if (rc < 0) {
> > -			priv->config.reset_n_io = -EINVAL;
> > -			nfc_err(dev, "failed to request reset_n io\n");
> > -		}
> > +	priv->reset_n_io = gpiod_get_optional(dev, "reset-n-io", GPIOD_OUT_LOW);

No, this should be "reset". gpiolib-of.c has a quirk to resolve to naked
"reset-n-io", otherwise this will resolve to "reset-n-io-gpios" in the
bowels of gpiolib.

> > +	if (IS_ERR(priv->reset_n_io)) {
> > +		nfc_err(dev, "failed to get reset_n gpio\n");
> > +		return ERR_CAST(priv->reset_n_io);
> >  	}
> 
> This also needs a call to gpiod_set_consumer_name(), IIRC the API name.

It does not have to... I am not sure who pays attention to names.

> 
> ...
> 
> > -	if (gpio_is_valid(priv->config.reset_n_io)) {
> > -		nfc_info(priv->dev, "reset the chip\n");
> > -		gpio_set_value(priv->config.reset_n_io, 0);
> > -		usleep_range(5000, 10000);
> > -		gpio_set_value(priv->config.reset_n_io, 1);
> > -	} else
> > -		nfc_info(priv->dev, "no reset available on this interface\n");
> > +	nfc_info(priv->dev, "reset the chip\n");
> > +	gpiod_set_value(priv->reset_n_io, 0);
> > +	usleep_range(5000, 10000);
> 
> Side note, this would be nice to use fsleep(), but I see that's just a
> copy'n'paste of the original piece.
> 
> > +	gpiod_set_value(priv->reset_n_io, 1);

Nope, this is not going to work. See
Documentation/devicetree/bindings/net/nfc/marvell,nci.yaml, this GPIO is
active low. We do not have any "live" DTS examples so I am not sure what
polarity is used in the wild. So either use logical level (my
preference) or switch to "_raw()" variant.

> 
> ...
> 
> >  void nfcmrvl_chip_halt(struct nfcmrvl_private *priv)
> >  {
> > -	if (gpio_is_valid(priv->config.reset_n_io))
> > -		gpio_set_value(priv->config.reset_n_io, 0);
> > +	if (priv->reset_n_io)
> 
> Not sure why we need this dup check.

I personally feel very uneasy when dealing with optional GPIO and not
checking if it exists or not, even though gpiod_set_value() handles
this. I think check makes logic clearer.

> 
> > +		gpiod_set_value(priv->reset_n_io, 0);
> >  }
> 

Thanks.

-- 
Dmitry