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(-)
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;
}
--
2.47.2Hi 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!
--
With Best Regards,
Andy Shevchenko
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!
© 2016 - 2026 Red Hat, Inc.