[PATCH net-next v1 0/2] ieee802154: ca8210: Sparse fix and GPIOd conversion

[PATCH net-next v1 0/2] ieee802154: ca8210: Sparse fix and GPIOd conversion

[Andy Shevchenko]

The main part is the patch 2 that converts the driver to GPIO descriptor APIs,
the first one is just an ad-hoc fix WRT sparse complains on the bitwise
types misuse.

Andy Shevchenko (2):
  ieee802154: ca8210: Use proper setter and getters for bitwise types
  ieee802154: ca8210: Switch to using gpiod API

 drivers/net/ieee802154/ca8210.c | 72 ++++++++++++---------------------
 1 file changed, 26 insertions(+), 46 deletions(-)

-- 
2.47.2

[PATCH net-next v1 1/2] ieee802154: ca8210: Use proper setter and getters for bitwise types

[Andy Shevchenko]

Sparse complains that the driver doesn't respect the bitwise types:

drivers/net/ieee802154/ca8210.c:1796:27: warning: incorrect type in assignment (different base types)
drivers/net/ieee802154/ca8210.c:1796:27:    expected restricted __le16 [addressable] [assigned] [usertype] pan_id
drivers/net/ieee802154/ca8210.c:1796:27:    got unsigned short [usertype]
drivers/net/ieee802154/ca8210.c:1801:25: warning: incorrect type in assignment (different base types)
drivers/net/ieee802154/ca8210.c:1801:25:    expected restricted __le16 [addressable] [assigned] [usertype] pan_id
drivers/net/ieee802154/ca8210.c:1801:25:    got unsigned short [usertype]
drivers/net/ieee802154/ca8210.c:1928:28: warning: incorrect type in argument 3 (different base types)
drivers/net/ieee802154/ca8210.c:1928:28:    expected unsigned short [usertype] dst_pan_id
drivers/net/ieee802154/ca8210.c:1928:28:    got restricted __le16 [addressable] [usertype] pan_id

Use proper setter and getters for bitwise types.

Note, in accordance with [1] the protocol is little endian.

Link: https://www.cascoda.com/wp-content/uploads/2018/11/CA-8210_datasheet_0418.pdf [1]
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/net/ieee802154/ca8210.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
index 753215ebc67c..a036910f6082 100644
--- a/drivers/net/ieee802154/ca8210.c
+++ b/drivers/net/ieee802154/ca8210.c
@@ -1446,8 +1446,7 @@ static u8 mcps_data_request(
 	command.pdata.data_req.src_addr_mode = src_addr_mode;
 	command.pdata.data_req.dst.mode = dst_address_mode;
 	if (dst_address_mode != MAC_MODE_NO_ADDR) {
-		command.pdata.data_req.dst.pan_id[0] = LS_BYTE(dst_pan_id);
-		command.pdata.data_req.dst.pan_id[1] = MS_BYTE(dst_pan_id);
+		put_unaligned_le16(dst_pan_id, command.pdata.data_req.dst.pan_id);
 		if (dst_address_mode == MAC_MODE_SHORT_ADDR) {
 			command.pdata.data_req.dst.address[0] = LS_BYTE(
 				dst_addr->short_address
@@ -1795,12 +1794,12 @@ static int ca8210_skb_rx(
 	}
 	hdr.source.mode = data_ind[0];
 	dev_dbg(&priv->spi->dev, "srcAddrMode: %#03x\n", hdr.source.mode);
-	hdr.source.pan_id = *(u16 *)&data_ind[1];
+	hdr.source.pan_id = cpu_to_le16(get_unaligned_le16(&data_ind[1]));
 	dev_dbg(&priv->spi->dev, "srcPanId: %#06x\n", hdr.source.pan_id);
 	memcpy(&hdr.source.extended_addr, &data_ind[3], 8);
 	hdr.dest.mode = data_ind[11];
 	dev_dbg(&priv->spi->dev, "dstAddrMode: %#03x\n", hdr.dest.mode);
-	hdr.dest.pan_id = *(u16 *)&data_ind[12];
+	hdr.dest.pan_id = cpu_to_le16(get_unaligned_le16(&data_ind[12]));
 	dev_dbg(&priv->spi->dev, "dstPanId: %#06x\n", hdr.dest.pan_id);
 	memcpy(&hdr.dest.extended_addr, &data_ind[14], 8);
 
@@ -1927,7 +1926,7 @@ static int ca8210_skb_tx(
 	status =  mcps_data_request(
 		header.source.mode,
 		header.dest.mode,
-		header.dest.pan_id,
+		le16_to_cpu(header.dest.pan_id),
 		(union macaddr *)&header.dest.extended_addr,
 		skb->len - mac_len,
 		&skb->data[mac_len],
-- 
2.47.2

Re: [PATCH net-next v1 1/2] ieee802154: ca8210: Use proper setter and getters for bitwise types

[Miquel Raynal]

Hello,

On 03/03/2025 at 17:07:39 +02, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Sparse complains that the driver doesn't respect the bitwise types:
>
> drivers/net/ieee802154/ca8210.c:1796:27: warning: incorrect type in assignment (different base types)
> drivers/net/ieee802154/ca8210.c:1796:27:    expected restricted __le16 [addressable] [assigned] [usertype] pan_id
> drivers/net/ieee802154/ca8210.c:1796:27:    got unsigned short [usertype]
> drivers/net/ieee802154/ca8210.c:1801:25: warning: incorrect type in assignment (different base types)
> drivers/net/ieee802154/ca8210.c:1801:25:    expected restricted __le16 [addressable] [assigned] [usertype] pan_id
> drivers/net/ieee802154/ca8210.c:1801:25:    got unsigned short [usertype]
> drivers/net/ieee802154/ca8210.c:1928:28: warning: incorrect type in argument 3 (different base types)
> drivers/net/ieee802154/ca8210.c:1928:28:    expected unsigned short [usertype] dst_pan_id
> drivers/net/ieee802154/ca8210.c:1928:28:    got restricted __le16 [addressable] [usertype] pan_id
>
> Use proper setter and getters for bitwise types.
>
> Note, in accordance with [1] the protocol is little endian.
>
> Link: https://www.cascoda.com/wp-content/uploads/2018/11/CA-8210_datasheet_0418.pdf [1]
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Looks correct indeed,

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

Re: [PATCH net-next v1 1/2] ieee802154: ca8210: Use proper setter and getters for bitwise types

[Andy Shevchenko]

On Mon, Mar 03, 2025 at 05:06:32PM +0100, Miquel Raynal wrote:
> On 03/03/2025 at 17:07:39 +02, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > Sparse complains that the driver doesn't respect the bitwise types:
> >
> > drivers/net/ieee802154/ca8210.c:1796:27: warning: incorrect type in assignment (different base types)
> > drivers/net/ieee802154/ca8210.c:1796:27:    expected restricted __le16 [addressable] [assigned] [usertype] pan_id
> > drivers/net/ieee802154/ca8210.c:1796:27:    got unsigned short [usertype]
> > drivers/net/ieee802154/ca8210.c:1801:25: warning: incorrect type in assignment (different base types)
> > drivers/net/ieee802154/ca8210.c:1801:25:    expected restricted __le16 [addressable] [assigned] [usertype] pan_id
> > drivers/net/ieee802154/ca8210.c:1801:25:    got unsigned short [usertype]
> > drivers/net/ieee802154/ca8210.c:1928:28: warning: incorrect type in argument 3 (different base types)
> > drivers/net/ieee802154/ca8210.c:1928:28:    expected unsigned short [usertype] dst_pan_id
> > drivers/net/ieee802154/ca8210.c:1928:28:    got restricted __le16 [addressable] [usertype] pan_id
> >
> > Use proper setter and getters for bitwise types.
> >
> > Note, in accordance with [1] the protocol is little endian.
> >
> > Link: https://www.cascoda.com/wp-content/uploads/2018/11/CA-8210_datasheet_0418.pdf [1]
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Looks correct indeed,

TBH, my guts feeling is that this driver was never tested on BE platforms...

> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks for review!

-- 
With Best Regards,
Andy Shevchenko

[PATCH net-next v1 2/2] ieee802154: ca8210: Switch to using gpiod API

[Andy Shevchenko]

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

Re: [PATCH net-next v1 2/2] ieee802154: ca8210: Switch to using gpiod API

[Miquel Raynal]

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

Re: [PATCH net-next v1 2/2] ieee802154: ca8210: Switch to using gpiod API

[Andy Shevchenko]

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

Re: [PATCH net-next v1 2/2] ieee802154: ca8210: Switch to using gpiod API

[Miquel Raynal]

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!