linux-kernel - 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] [day] [month] [year] [list]

Message-ID: <aJyGrhvViwoK2MeN@smile.fi.intel.com>
Date: Wed, 13 Aug 2025 15:35:58 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: Arnd Bergmann <arnd@...nel.org>, 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 Mon, Aug 11, 2025 at 02:43:51PM -0700, Dmitry Torokhov wrote:
> 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:

...

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

Good point.

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

It goes to user space, isn't it?
In any case it will give 1:1 transition from the look&fell perspective.

...

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

I disagree with the duplicate. It doesn't make any additional clearness as I
read it. When one reads the code the "here we set GPIO to the X state" without
any conditional is fine as one may check later in DT schema if the GPIO is
optional or not.

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



-- 
With Best Regards,
Andy Shevchenko