| Message ID | 20250303164928.1466246-4-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ieee802154: ca8210: Sparse fix and GPIOd conversion | expand |
On Mon, Mar 3, 2025 at 5:49 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > This updates the driver to gpiod API, and removes yet another use of > of_get_named_gpio(). > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> But note: > @@ -632,10 +630,10 @@ static void ca8210_reset_send(struct spi_device *spi, unsigned int ms) > struct ca8210_priv *priv = spi_get_drvdata(spi); > long status; > > - gpio_set_value(pdata->gpio_reset, 0); > + gpiod_set_value(pdata->reset_gpio, 0); > reinit_completion(&priv->ca8210_is_awake); > msleep(ms); > - gpio_set_value(pdata->gpio_reset, 1); > + gpiod_set_value(pdata->reset_gpio, 1); This drives the GPIO low to assert reset, meaning it is something that should have GPIO_ACTIVE_LOW set in the device tree, and it might even have, so let's check what we can check: git grep cascoda,ca8210 Documentation/devicetree/bindings/net/ieee802154/ca8210.txt: - compatible: Should be "cascoda,ca8210" Documentation/devicetree/bindings/net/ieee802154/ca8210.txt: compatible = "cascoda,ca8210"; drivers/net/ieee802154/ca8210.c: {.compatible = "cascoda,ca8210", }, well ain't that typical, all users are out of tree. The example in the bindings file is wrong, setting ACTIVE_HIGH. Sigh, let's assume all those DTS files somewhere are wrong and they didn't set GPIO_ACTIVE_LOW in them... Maybe add a comment in the code that this is wrong and the driver and DTS files should be fixed. Alternatively patch Documentation/devicetree/bindings/net/ieee802154/ca8210.txt to set GPIO_ACTIVE_LOW and fix the code to invert it both here and when getting the GPIO, but it could cause problems for outoftree users. Either way, this is good progress: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
Mon, Mar 03, 2025 at 09:00:39PM +0100, Linus Walleij kirjoitti: > On Mon, Mar 3, 2025 at 5:49 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: ... > > reinit_completion(&priv->ca8210_is_awake); > > msleep(ms); > > - gpio_set_value(pdata->gpio_reset, 1); > > + gpiod_set_value(pdata->reset_gpio, 1); > > This drives the GPIO low to assert reset, meaning it is something > that should have GPIO_ACTIVE_LOW set in the device tree, Yeah, the pin 27 is marked as NRESET and description is pointing out to it as active low. > and it might even have, so let's check what we can check: > > git grep cascoda,ca8210 > Documentation/devicetree/bindings/net/ieee802154/ca8210.txt: - > compatible: Should be "cascoda,ca8210" > Documentation/devicetree/bindings/net/ieee802154/ca8210.txt: > compatible = "cascoda,ca8210"; > drivers/net/ieee802154/ca8210.c: {.compatible = "cascoda,ca8210", }, > > well ain't that typical, all users are out of tree. The example > in the bindings file is wrong, setting ACTIVE_HIGH. Sigh, let's > assume all those DTS files somewhere are wrong and they > didn't set GPIO_ACTIVE_LOW in them... > Maybe add a comment in the code that this is wrong and the > driver and DTS files should be fixed. Or maybe fix in the driver and schema and add a quirk to gpiolib-of.c? > Alternatively patch Documentation/devicetree/bindings/net/ieee802154/ca8210.txt > to set GPIO_ACTIVE_LOW and fix the code to invert it both > here and when getting the GPIO, but it could cause problems > for outoftree users. Would it? We have such quirks to fix a polarity for other drivers/devices. > Either way, this is good progress: > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Thank you!
On Mon, Mar 3, 2025 at 9:39 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > Maybe add a comment in the code that this is wrong and the > > driver and DTS files should be fixed. > > Or maybe fix in the driver and schema and add a quirk to gpiolib-of.c? Even better! Yours, Linus Walleij
diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c index 65f042c8734b..d4c1f00d29fe 100644 --- a/drivers/net/ieee802154/ca8210.c +++ b/drivers/net/ieee802154/ca8210.c @@ -52,12 +52,10 @@ #include <linux/debugfs.h> #include <linux/delay.h> #include <linux/gpio/consumer.h> -#include <linux/gpio.h> #include <linux/ieee802154.h> #include <linux/io.h> #include <linux/kfifo.h> #include <linux/of.h> -#include <linux/of_gpio.h> #include <linux/module.h> #include <linux/mutex.h> #include <linux/poll.h> @@ -350,8 +348,8 @@ struct work_priv_container { * @extclockenable: true if the external clock is to be enabled * @extclockfreq: frequency of the external clock * @extclockgpio: ca8210 output gpio of the external clock - * @gpio_reset: gpio number of ca8210 reset line - * @gpio_irq: gpio number of ca8210 interrupt line + * @reset_gpio: ca8210 reset GPIO descriptor + * @irq_gpio: ca8210 interrupt GPIO descriptor * @irq_id: identifier for the ca8210 irq * */ @@ -359,8 +357,8 @@ struct ca8210_platform_data { bool extclockenable; unsigned int extclockfreq; unsigned int extclockgpio; - int gpio_reset; - int gpio_irq; + struct gpio_desc *reset_gpio; + struct gpio_desc *irq_gpio; int irq_id; }; @@ -632,10 +630,10 @@ static void ca8210_reset_send(struct spi_device *spi, unsigned int ms) struct ca8210_priv *priv = spi_get_drvdata(spi); long status; - gpio_set_value(pdata->gpio_reset, 0); + gpiod_set_value(pdata->reset_gpio, 0); reinit_completion(&priv->ca8210_is_awake); msleep(ms); - gpio_set_value(pdata->gpio_reset, 1); + gpiod_set_value(pdata->reset_gpio, 1); priv->promiscuous = false; /* Wait until wakeup indication seen */ @@ -2788,24 +2786,14 @@ static int ca8210_reset_init(struct spi_device *spi) { struct device *dev = &spi->dev; struct ca8210_platform_data *pdata = dev_get_platdata(dev); - int ret; - pdata->gpio_reset = of_get_named_gpio( - spi->dev.of_node, - "reset-gpio", - 0 - ); - - ret = gpio_direction_output(pdata->gpio_reset, 1); - if (ret < 0) { - dev_crit( - &spi->dev, - "Reset GPIO %d did not set to output mode\n", - pdata->gpio_reset - ); + pdata->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); + if (IS_ERR(pdata->reset_gpio)) { + dev_crit(dev, "Reset GPIO did not set to output mode\n"); + return PTR_ERR(pdata->reset_gpio); } - return ret; + return 0; } /** @@ -2820,20 +2808,15 @@ static int ca8210_interrupt_init(struct spi_device *spi) struct ca8210_platform_data *pdata = dev_get_platdata(dev); int ret; - pdata->gpio_irq = of_get_named_gpio( - spi->dev.of_node, - "irq-gpio", - 0 - ); + pdata->irq_gpio = devm_gpiod_get(dev, "irq", GPIOD_IN); + if (IS_ERR(pdata->irq_gpio)) { + dev_crit(dev, "Could not retrieve IRQ GPIO\n"); + return PTR_ERR(pdata->irq_gpio); + } - pdata->irq_id = gpio_to_irq(pdata->gpio_irq); + pdata->irq_id = gpiod_to_irq(pdata->irq_gpio); if (pdata->irq_id < 0) { - dev_crit( - &spi->dev, - "Could not get irq for gpio pin %d\n", - pdata->gpio_irq - ); - gpio_free(pdata->gpio_irq); + dev_crit(dev, "Could not get irq for IRQ GPIO\n"); return pdata->irq_id; } @@ -2844,10 +2827,8 @@ static int ca8210_interrupt_init(struct spi_device *spi) "ca8210-irq", spi_get_drvdata(spi) ); - if (ret) { + if (ret) dev_crit(&spi->dev, "request_irq %d failed\n", pdata->irq_id); - gpio_free(pdata->gpio_irq); - } return ret; }
This updates the driver to gpiod API, and removes yet another use of of_get_named_gpio(). Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/net/ieee802154/ca8210.c | 57 +++++++++++---------------------- 1 file changed, 19 insertions(+), 38 deletions(-)