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:

quoted

From: Arnd Bergmann <arnd@arndb.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@linux.intel.com>

But note some comments below.

...

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.

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.

...

quoted

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

quoted

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

...

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.

quoted

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

Thanks.

-- 
Dmitry