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:

quoted

On Fri, Aug 08, 2025 at 05:18:01PM +0200, Arnd Bergmann wrote:

...

quoted

quoted

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

quoted

quoted

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

...

quoted

quoted

 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.

quoted

quoted

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

-- 
With Best Regards,
Andy Shevchenko