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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aJcea90siAod5Apw@smile.fi.intel.com>
Date: Sat, 9 Aug 2025 13:09:47 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Arnd Bergmann <arnd@...nel.org>
Cc: Bartosz Golaszewski <brgl@...ev.pl>,
	Linus Walleij <linus.walleij@...aro.org>,
	linux-gpio@...r.kernel.org, Krzysztof Kozlowski <krzk@...nel.org>,
	Arnd Bergmann <arnd@...db.de>, Jakub Kicinski <kuba@...nel.org>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 17/21] nfc: marvell: convert to gpio descriptors

On Fri, Aug 08, 2025 at 05:18:01PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@...db.de>
> 
> 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@...ux.intel.com>

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);
> +	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.

...

> -	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);

...

>  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.

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

-- 
With Best Regards,
Andy Shevchenko