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.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/nfc/nfcmrvl/main.c | 47 +++++++++++------------------------
drivers/nfc/nfcmrvl/nfcmrvl.h | 5 ++--
drivers/nfc/nfcmrvl/uart.c | 5 ----
drivers/nfc/nfcmrvl/usb.c | 1 -
4 files changed, 18 insertions(+), 40 deletions(-)
diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index 141bc4b66dcb..1b465a19a262 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -6,7 +6,7 @@
*/
#include <linux/module.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
#include <linux/delay.h>
#include <linux/of_gpio.h>
#include <linux/nfc.h>
@@ -112,14 +112,10 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum nfcmrvl_phy phy,
memcpy(&priv->config, pdata, sizeof(*pdata));
- 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);
}
if (phy == NFCMRVL_PHY_SPI) {
@@ -172,8 +168,7 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum nfcmrvl_phy phy,
error_free_dev:
nci_free_device(priv->ndev);
error_free_gpio:
- if (gpio_is_valid(priv->config.reset_n_io))
- gpio_free(priv->config.reset_n_io);
+ gpiod_put(priv->reset_n_io);
kfree(priv);
return ERR_PTR(rc);
}
@@ -189,8 +184,7 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv)
nfcmrvl_fw_dnld_deinit(priv);
- if (gpio_is_valid(priv->config.reset_n_io))
- gpio_free(priv->config.reset_n_io);
+ gpiod_put(priv->reset_n_io);
nci_free_device(ndev);
kfree(priv);
@@ -232,35 +226,24 @@ void nfcmrvl_chip_reset(struct nfcmrvl_private *priv)
{
/* Reset possible fault of previous session */
clear_bit(NFCMRVL_PHY_ERROR, &priv->flags);
+ if (!priv->reset_n_io)
+ return;
- 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);
+ 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)
+ gpiod_set_value(priv->reset_n_io, 0);
}
int nfcmrvl_parse_dt(struct device_node *node,
struct nfcmrvl_platform_data *pdata)
{
- int reset_n_io;
-
- reset_n_io = of_get_named_gpio(node, "reset-n-io", 0);
- if (reset_n_io < 0) {
- pr_info("no reset-n-io config\n");
- } else if (!gpio_is_valid(reset_n_io)) {
- pr_err("invalid reset-n-io GPIO\n");
- return reset_n_io;
- }
- pdata->reset_n_io = reset_n_io;
pdata->hci_muxed = of_property_read_bool(node, "hci-muxed");
return 0;
diff --git a/drivers/nfc/nfcmrvl/nfcmrvl.h b/drivers/nfc/nfcmrvl/nfcmrvl.h
index f61a99e553db..156f46e1581f 100644
--- a/drivers/nfc/nfcmrvl/nfcmrvl.h
+++ b/drivers/nfc/nfcmrvl/nfcmrvl.h
@@ -53,8 +53,6 @@ struct nfcmrvl_platform_data {
* Generic
*/
- /* GPIO that is wired to RESET_N signal */
- int reset_n_io;
/* Tell if transport is muxed in HCI one */
bool hci_muxed;
@@ -83,6 +81,9 @@ struct nfcmrvl_private {
/* Platform configuration */
struct nfcmrvl_platform_data config;
+ /* RESET_N GPIO line */
+ struct gpio_desc *reset_n_io;
+
/* Parent dev */
struct nci_dev *ndev;
diff --git a/drivers/nfc/nfcmrvl/uart.c b/drivers/nfc/nfcmrvl/uart.c
index 2037cd6d4f4f..cb2da7a1d91f 100644
--- a/drivers/nfc/nfcmrvl/uart.c
+++ b/drivers/nfc/nfcmrvl/uart.c
@@ -20,7 +20,6 @@
static unsigned int hci_muxed;
static unsigned int flow_control;
static unsigned int break_control;
-static int reset_n_io = -EINVAL;
/*
* NFCMRVL NCI OPS
@@ -113,7 +112,6 @@ static int nfcmrvl_nci_uart_open(struct nci_uart *nu)
if (!pdata) {
pr_info("No platform data / DT -> fallback to module params\n");
config.hci_muxed = hci_muxed;
- config.reset_n_io = reset_n_io;
config.flow_control = flow_control;
config.break_control = break_control;
pdata = &config;
@@ -201,6 +199,3 @@ MODULE_PARM_DESC(break_control, "Tell if UART driver must drive break signal.");
module_param(hci_muxed, uint, 0);
MODULE_PARM_DESC(hci_muxed, "Tell if transport is muxed in HCI one.");
-
-module_param(reset_n_io, int, 0);
-MODULE_PARM_DESC(reset_n_io, "GPIO that is wired to RESET_N signal.");
diff --git a/drivers/nfc/nfcmrvl/usb.c b/drivers/nfc/nfcmrvl/usb.c
index ea7309453096..4cf19433fde1 100644
--- a/drivers/nfc/nfcmrvl/usb.c
+++ b/drivers/nfc/nfcmrvl/usb.c
@@ -294,7 +294,6 @@ static int nfcmrvl_probe(struct usb_interface *intf,
/* No configuration for USB */
memset(&config, 0, sizeof(config));
- config.reset_n_io = -EINVAL;
nfc_info(&udev->dev, "intf %p id %p\n", intf, id);
--
2.39.5On 08/08/2025 17:18, Arnd Bergmann wrote: > 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. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/nfc/nfcmrvl/main.c | 47 +++++++++++------------------------ > drivers/nfc/nfcmrvl/nfcmrvl.h | 5 ++-- > drivers/nfc/nfcmrvl/uart.c | 5 ---- > drivers/nfc/nfcmrvl/usb.c | 1 - > 4 files changed, 18 insertions(+), 40 deletions(-) > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On Fri, Aug 8, 2025 at 5:24 PM Arnd Bergmann <arnd@kernel.org> wrote: > > 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. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- Yes, please! Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
On Fri, Aug 08, 2025 at 05:18:01PM +0200, Arnd Bergmann wrote:
> 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.
...
> - 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
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@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.
>
> ...
>
> > - 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
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
© 2016 - 2025 Red Hat, Inc.