| Message ID | 20250303150855.1294188-3-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ieee802154: ca8210: Sparse fix and GPIOd conversion | expand |
Hi Andy, > @@ -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: GPIO of ca8210 reset line What about "CA8210 Reset GPIO line"? Or Just "Reset GPIO line"? Or even "Reset GPIO descriptor" (whatever). > + * @irq_gpio: GPIO of ca8210 interrupt line Same > * @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; > }; [...] > /* Wait until wakeup indication seen */ > @@ -2784,25 +2782,14 @@ static void ca8210_unregister_ext_clock(struct spi_device *spi) > */ > static int ca8210_reset_init(struct spi_device *spi) > { > - int ret; > - struct ca8210_platform_data *pdata = spi->dev.platform_data; > + struct device *dev = &spi->dev; > + struct ca8210_platform_data *pdata = dev_get_platdata(dev); > Can you either mention the additional cleanup that you do in the commit log or split it in a separate commit? (splitting is probably not necessary here given that most of the cleanup anyway is related to the actual changes. > - pdata->gpio_reset = of_get_named_gpio( > - spi->dev.of_node, > - "reset-gpio", > - 0 > - ); > + 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"); > > - 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 > - ); > - } > - > - return ret; > + return PTR_ERR_OR_ZERO(pdata->reset_gpio); This is not a strong request, but in general I think it is preferred to return immediately, so this looks easier to understand: + 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_pgio); + } + + return 0; Otherwise the rest lgtm. Thanks, Miquèl
On Mon, Mar 03, 2025 at 05:20:59PM +0100, Miquel Raynal wrote: ... > > - * @gpio_reset: gpio number of ca8210 reset line > > - * @gpio_irq: gpio number of ca8210 interrupt line > > + * @reset_gpio: GPIO of ca8210 reset line > > What about "CA8210 Reset GPIO line"? Or Just "Reset GPIO line"? Or even > "Reset GPIO descriptor" (whatever). > > > + * @irq_gpio: GPIO of ca8210 interrupt line > > Same Sure. [...] > > - int ret; > > - struct ca8210_platform_data *pdata = spi->dev.platform_data; > > + struct device *dev = &spi->dev; > > + struct ca8210_platform_data *pdata = dev_get_platdata(dev); > > Can you either mention the additional cleanup that you do in the commit > log or split it in a separate commit? (splitting is probably not > necessary here given that most of the cleanup anyway is related to the > actual changes. Do you mean the platform_data accessors? I can actually split it to a separate change as I had done some of that in the past in other drivers. ... > > - 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 > > - ); > > - } > > - > > - return ret; > > + return PTR_ERR_OR_ZERO(pdata->reset_gpio); > > This is not a strong request, but in general I think it is preferred to return > immediately, so this looks easier to understand: I used the same logic as in the original flow. > + 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_pgio); > + } > + > + return 0; Sure I can do this in v2. ... > Otherwise the rest lgtm. Thank you for the review!
On 03/03/2025 at 18:28:41 +02, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Mon, Mar 03, 2025 at 05:20:59PM +0100, Miquel Raynal wrote: > > ... > >> > - * @gpio_reset: gpio number of ca8210 reset line >> > - * @gpio_irq: gpio number of ca8210 interrupt line >> > + * @reset_gpio: GPIO of ca8210 reset line >> >> What about "CA8210 Reset GPIO line"? Or Just "Reset GPIO line"? Or even >> "Reset GPIO descriptor" (whatever). >> >> > + * @irq_gpio: GPIO of ca8210 interrupt line >> >> Same > > Sure. > > [...] > >> > - int ret; >> > - struct ca8210_platform_data *pdata = spi->dev.platform_data; >> > + struct device *dev = &spi->dev; >> > + struct ca8210_platform_data *pdata = dev_get_platdata(dev); >> >> Can you either mention the additional cleanup that you do in the commit >> log or split it in a separate commit? (splitting is probably not >> necessary here given that most of the cleanup anyway is related to the >> actual changes. > > Do you mean the platform_data accessors? Yes. > I can actually split it to a separate > change as I had done some of that in the past in other drivers. Up to you, either way, as long as it is mentioned in the commit log, I'm happy. > > ... > >> > - 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 >> > - ); >> > - } >> > - >> > - return ret; >> > + return PTR_ERR_OR_ZERO(pdata->reset_gpio); >> >> This is not a strong request, but in general I think it is preferred to return >> immediately, so this looks easier to understand: > > I used the same logic as in the original flow. That's true, and I understand your choice in the first place. But given that you're also doing a bit of cleanup, one more misc change feels okay. > >> + 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_pgio); >> + } >> + >> + return 0; > > Sure I can do this in v2. Great! > ... > >> Otherwise the rest lgtm. > > Thank you for the review!
diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c index a036910f6082..2342f1927dc9 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: GPIO of ca8210 reset line + * @irq_gpio: GPIO of ca8210 interrupt line * @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; }; @@ -631,10 +629,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 */ @@ -2784,25 +2782,14 @@ static void ca8210_unregister_ext_clock(struct spi_device *spi) */ static int ca8210_reset_init(struct spi_device *spi) { - int ret; - struct ca8210_platform_data *pdata = spi->dev.platform_data; + struct device *dev = &spi->dev; + struct ca8210_platform_data *pdata = dev_get_platdata(dev); - pdata->gpio_reset = of_get_named_gpio( - spi->dev.of_node, - "reset-gpio", - 0 - ); + 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"); - 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 - ); - } - - return ret; + return PTR_ERR_OR_ZERO(pdata->reset_gpio); } /** @@ -2813,23 +2800,19 @@ static int ca8210_reset_init(struct spi_device *spi) */ static int ca8210_interrupt_init(struct spi_device *spi) { + struct device *dev = &spi->dev; + struct ca8210_platform_data *pdata = dev_get_platdata(dev); int ret; - struct ca8210_platform_data *pdata = spi->dev.platform_data; - 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; } @@ -2840,10 +2823,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 | 63 ++++++++++++--------------------- 1 file changed, 22 insertions(+), 41 deletions(-)