diff mbox series

at86rf230: convert to gpio descriptors

Message ID 20230126135215.3387820-1-arnd@kernel.org (mailing list archive)
State Superseded
Headers show
Series at86rf230: convert to gpio descriptors | expand

Commit Message

Arnd Bergmann Jan. 26, 2023, 1:51 p.m. UTC 
From: Arnd Bergmann <arnd@arndb.de>

There are no remaining in-tree users of the platform_data,
so this driver can be converted to using the simpler gpiod
interfaces.

Any out-of-tree users that rely on the platform data can
provide the data using the device_property and gpio_lookup
interfaces instead.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/net/ieee802154/at86rf230.c | 82 +++++++++---------------------
 include/linux/spi/at86rf230.h      | 20 --------
 2 files changed, 25 insertions(+), 77 deletions(-)
 delete mode 100644 include/linux/spi/at86rf230.h

Comments

Miquel Raynal Jan. 26, 2023, 2:12 p.m. UTC | #1 
Hi Arnd,

arnd@kernel.org wrote on Thu, 26 Jan 2023 14:51:23 +0100:

> From: Arnd Bergmann <arnd@arndb.de>
> 
> There are no remaining in-tree users of the platform_data,
> so this driver can be converted to using the simpler gpiod
> interfaces.
> 
> Any out-of-tree users that rely on the platform data can
> provide the data using the device_property and gpio_lookup
> interfaces instead.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/net/ieee802154/at86rf230.c | 82 +++++++++---------------------
>  include/linux/spi/at86rf230.h      | 20 --------
>  2 files changed, 25 insertions(+), 77 deletions(-)
>  delete mode 100644 include/linux/spi/at86rf230.h
> 

[...]

> @@ -1682,7 +1650,7 @@ MODULE_DEVICE_TABLE(spi, at86rf230_device_id);
>  static struct spi_driver at86rf230_driver = {
>  	.id_table = at86rf230_device_id,
>  	.driver = {
> -		.of_match_table = of_match_ptr(at86rf230_of_match),
> +		.of_match_table = at86rf230_of_match,linux-gnueabihf embed a C library which relies on kernel headers (for example, to provide an open API which translates to an open syscall), for exam

Looks like an unrelated change? Or is it a consequence of "not having
any in-tree users of platform_data" that plays a role here?

Anyhow, the changes in the driver look good, so:

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

Thanks,
Miquèl
Andy Shevchenko Jan. 26, 2023, 2:13 p.m. UTC | #2 
On Thu, Jan 26, 2023 at 02:51:23PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> There are no remaining in-tree users of the platform_data,
> so this driver can be converted to using the simpler gpiod
> interfaces.
> 
> Any out-of-tree users that rely on the platform data can
> provide the data using the device_property and gpio_lookup
> interfaces instead.

Precisely!

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Some not-picks below.

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/net/ieee802154/at86rf230.c | 82 +++++++++---------------------
>  include/linux/spi/at86rf230.h      | 20 --------
>  2 files changed, 25 insertions(+), 77 deletions(-)
>  delete mode 100644 include/linux/spi/at86rf230.h
> 
> diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
> index 15f283b26721..66193756c891 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -15,14 +15,13 @@
>  #include <linux/jiffies.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/delay.h>
>  #include <linux/spi/spi.h>
> -#include <linux/spi/at86rf230.h>
>  #include <linux/regmap.h>
>  #include <linux/skbuff.h>
> -#include <linux/of_gpio.h>
>  #include <linux/ieee802154.h>
> +#include <linux/property.h>

This can be sorted a bit better, i.e. squeezing property.h before regmap.h,
gpio/consumer.h after delay.h.

>  #include <net/mac802154.h>
>  #include <net/cfg802154.h>
> @@ -82,7 +81,7 @@ struct at86rf230_local {
>  	struct ieee802154_hw *hw;
>  	struct at86rf2xx_chip_data *data;
>  	struct regmap *regmap;
> -	int slp_tr;
> +	struct gpio_desc *slp_tr;
>  	bool sleep;
>  
>  	struct completion state_complete;
> @@ -107,8 +106,8 @@ at86rf230_async_state_change(struct at86rf230_local *lp,
>  static inline void
>  at86rf230_sleep(struct at86rf230_local *lp)
>  {
> -	if (gpio_is_valid(lp->slp_tr)) {
> -		gpio_set_value(lp->slp_tr, 1);
> +	if (lp->slp_tr) {
> +		gpiod_set_value(lp->slp_tr, 1);
>  		usleep_range(lp->data->t_off_to_sleep,
>  			     lp->data->t_off_to_sleep + 10);
>  		lp->sleep = true;
> @@ -118,8 +117,8 @@ at86rf230_sleep(struct at86rf230_local *lp)
>  static inline void
>  at86rf230_awake(struct at86rf230_local *lp)
>  {
> -	if (gpio_is_valid(lp->slp_tr)) {
> -		gpio_set_value(lp->slp_tr, 0);
> +	if (lp->slp_tr) {
> +		gpiod_set_value(lp->slp_tr, 0);
>  		usleep_range(lp->data->t_sleep_to_off,
>  			     lp->data->t_sleep_to_off + 100);
>  		lp->sleep = false;
> @@ -204,9 +203,9 @@ at86rf230_write_subreg(struct at86rf230_local *lp,
>  static inline void
>  at86rf230_slp_tr_rising_edge(struct at86rf230_local *lp)
>  {
> -	gpio_set_value(lp->slp_tr, 1);
> +	gpiod_set_value(lp->slp_tr, 1);
>  	udelay(1);
> -	gpio_set_value(lp->slp_tr, 0);
> +	gpiod_set_value(lp->slp_tr, 0);
>  }
>  
>  static bool
> @@ -819,7 +818,7 @@ at86rf230_write_frame_complete(void *context)
>  
>  	ctx->trx.len = 2;
>  
> -	if (gpio_is_valid(lp->slp_tr))
> +	if (lp->slp_tr)
>  		at86rf230_slp_tr_rising_edge(lp);
>  	else
>  		at86rf230_async_write_reg(lp, RG_TRX_STATE, STATE_BUSY_TX, ctx,
> @@ -1415,32 +1414,6 @@ static int at86rf230_hw_init(struct at86rf230_local *lp, u8 xtal_trim)
>  	return at86rf230_write_subreg(lp, SR_SLOTTED_OPERATION, 0);
>  }
>  
> -static int
> -at86rf230_get_pdata(struct spi_device *spi, int *rstn, int *slp_tr,
> -		    u8 *xtal_trim)
> -{
> -	struct at86rf230_platform_data *pdata = spi->dev.platform_data;
> -	int ret;
> -
> -	if (!IS_ENABLED(CONFIG_OF) || !spi->dev.of_node) {
> -		if (!pdata)
> -			return -ENOENT;
> -
> -		*rstn = pdata->rstn;
> -		*slp_tr = pdata->slp_tr;
> -		*xtal_trim = pdata->xtal_trim;
> -		return 0;
> -	}
> -
> -	*rstn = of_get_named_gpio(spi->dev.of_node, "reset-gpio", 0);
> -	*slp_tr = of_get_named_gpio(spi->dev.of_node, "sleep-gpio", 0);
> -	ret = of_property_read_u8(spi->dev.of_node, "xtal-trim", xtal_trim);
> -	if (ret < 0 && ret != -EINVAL)
> -		return ret;
> -
> -	return 0;
> -}
> -
>  static int
>  at86rf230_detect_device(struct at86rf230_local *lp)
>  {
> @@ -1547,7 +1520,8 @@ static int at86rf230_probe(struct spi_device *spi)
>  	struct ieee802154_hw *hw;
>  	struct at86rf230_local *lp;
>  	unsigned int status;
> -	int rc, irq_type, rstn, slp_tr;
> +	int rc, irq_type;
> +	struct gpio_desc *rstn, *slp_tr;
>  	u8 xtal_trim = 0;
>  
>  	if (!spi->irq) {
> @@ -1555,32 +1529,26 @@ static int at86rf230_probe(struct spi_device *spi)
>  		return -EINVAL;
>  	}
>  
> -	rc = at86rf230_get_pdata(spi, &rstn, &slp_tr, &xtal_trim);
> -	if (rc < 0) {
> -		dev_err(&spi->dev, "failed to parse platform_data: %d\n", rc);
> +	rc = device_property_read_u8(&spi->dev, "xtal-trim", &xtal_trim);
> +	if (rc < 0 && rc != -EINVAL) {
> +		dev_err(&spi->dev, "failed to parse xtal-trim: %d\n", rc);
>  		return rc;
>  	}
>  
> -	if (gpio_is_valid(rstn)) {
> -		rc = devm_gpio_request_one(&spi->dev, rstn,
> -					   GPIOF_OUT_INIT_HIGH, "rstn");
> -		if (rc)
> -			return rc;
> -	}
> +	rstn = devm_gpiod_get_optional(&spi->dev, "rstn", GPIOD_OUT_HIGH);
> +	if (IS_ERR(rstn))
> +		return PTR_ERR(rstn);
>  
> -	if (gpio_is_valid(slp_tr)) {
> -		rc = devm_gpio_request_one(&spi->dev, slp_tr,
> -					   GPIOF_OUT_INIT_LOW, "slp_tr");
> -		if (rc)
> -			return rc;
> -	}
> +	slp_tr = devm_gpiod_get_optional(&spi->dev, "slp_tr", GPIOD_OUT_LOW);
> +	if (IS_ERR(slp_tr))
> +		return PTR_ERR(slp_tr);
>  
>  	/* Reset */
> -	if (gpio_is_valid(rstn)) {
> +	if (rstn) {
>  		udelay(1);
> -		gpio_set_value_cansleep(rstn, 0);
> +		gpiod_set_value_cansleep(rstn, 0);
>  		udelay(1);
> -		gpio_set_value_cansleep(rstn, 1);
> +		gpiod_set_value_cansleep(rstn, 1);
>  		usleep_range(120, 240);
>  	}
>  
> @@ -1682,7 +1650,7 @@ MODULE_DEVICE_TABLE(spi, at86rf230_device_id);
>  static struct spi_driver at86rf230_driver = {
>  	.id_table = at86rf230_device_id,
>  	.driver = {
> -		.of_match_table = of_match_ptr(at86rf230_of_match),
> +		.of_match_table = at86rf230_of_match,
>  		.name	= "at86rf230",
>  	},
>  	.probe      = at86rf230_probe,
> diff --git a/include/linux/spi/at86rf230.h b/include/linux/spi/at86rf230.h
> deleted file mode 100644
> index d278576ab692..000000000000
> --- a/include/linux/spi/at86rf230.h
> +++ /dev/null
> @@ -1,20 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -/*
> - * AT86RF230/RF231 driver
> - *
> - * Copyright (C) 2009-2012 Siemens AG
> - *
> - * Written by:
> - * Dmitry Eremin-Solenikov <dmitry.baryshkov@siemens.com>
> - */
> -#ifndef AT86RF230_H
> -#define AT86RF230_H
> -
> -struct at86rf230_platform_data {
> -	int rstn;
> -	int slp_tr;
> -	int dig2;
> -	u8 xtal_trim;
> -};
> -
> -#endif
> -- 
> 2.39.0
>
Andy Shevchenko Jan. 26, 2023, 2:24 p.m. UTC | #3 
On Thu, Jan 26, 2023 at 03:12:43PM +0100, Miquel Raynal wrote:
> arnd@kernel.org wrote on Thu, 26 Jan 2023 14:51:23 +0100:

...

> > There are no remaining in-tree users of the platform_data,
> > so this driver can be converted to using the simpler gpiod
> > interfaces.
> > 
> > Any out-of-tree users that rely on the platform data can
> > provide the data using the device_property and gpio_lookup
> > interfaces instead.

[...]

> > @@ -1682,7 +1650,7 @@ MODULE_DEVICE_TABLE(spi, at86rf230_device_id);
> >  static struct spi_driver at86rf230_driver = {
> >  	.id_table = at86rf230_device_id,
> >  	.driver = {
> > -		.of_match_table = of_match_ptr(at86rf230_of_match),
> > +		.of_match_table = at86rf230_of_match,linux-gnueabihf embed a C library which relies on kernel headers (for example, to provide an open API which translates to an open syscall), for exam
> 
> Looks like an unrelated change? Or is it a consequence of "not having
> any in-tree users of platform_data" that plays a role here?

This enables this driver to work on ACPI-based platforms without needed the
legacy platform data. Dunno if it will be ever the case, but still...

> Anyhow, the changes in the driver look good, so:
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thank you!
Arnd Bergmann Jan. 26, 2023, 2:39 p.m. UTC | #4 
On Thu, Jan 26, 2023, at 15:12, Miquel Raynal wrote:

>> @@ -1682,7 +1650,7 @@ MODULE_DEVICE_TABLE(spi, at86rf230_device_id);
>>  static struct spi_driver at86rf230_driver = {
>>  	.id_table = at86rf230_device_id,
>>  	.driver = {
>> -		.of_match_table = of_match_ptr(at86rf230_of_match),
>> +		.of_match_table = at86rf230_of_match,
>
> Looks like an unrelated change? Or is it a consequence of "not having
> any in-tree users of platform_data" that plays a role here?

I probably did it because I thought I had removed the matching #ifdef
for at86rf230_of_match in the process of making the driver DT-only.

Without this trivial change, building the driver as built-in with
CONFIG_OF=n can result in a warning like

drivers/net/ieee802154/at86rf230.c:1632:28: error: unused variable 'at86rf230_of_match' [-Werror,-Wunused-variable]

It looks like this was already removed in a8b66db804f0 ("at86rf230:
remove #ifdef CONFIG_OF"), which was not technically correct, but
nobody noticed, including me.

I could split this out as a separate patch, but it's probably
not worth it.

> Anyhow, the changes in the driver look good, so:
>
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,

    Arnd
diff mbox series

Patch

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 15f283b26721..66193756c891 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -15,14 +15,13 @@ 
 #include <linux/jiffies.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/delay.h>
 #include <linux/spi/spi.h>
-#include <linux/spi/at86rf230.h>
 #include <linux/regmap.h>
 #include <linux/skbuff.h>
-#include <linux/of_gpio.h>
 #include <linux/ieee802154.h>
+#include <linux/property.h>
 
 #include <net/mac802154.h>
 #include <net/cfg802154.h>
@@ -82,7 +81,7 @@  struct at86rf230_local {
 	struct ieee802154_hw *hw;
 	struct at86rf2xx_chip_data *data;
 	struct regmap *regmap;
-	int slp_tr;
+	struct gpio_desc *slp_tr;
 	bool sleep;
 
 	struct completion state_complete;
@@ -107,8 +106,8 @@  at86rf230_async_state_change(struct at86rf230_local *lp,
 static inline void
 at86rf230_sleep(struct at86rf230_local *lp)
 {
-	if (gpio_is_valid(lp->slp_tr)) {
-		gpio_set_value(lp->slp_tr, 1);
+	if (lp->slp_tr) {
+		gpiod_set_value(lp->slp_tr, 1);
 		usleep_range(lp->data->t_off_to_sleep,
 			     lp->data->t_off_to_sleep + 10);
 		lp->sleep = true;
@@ -118,8 +117,8 @@  at86rf230_sleep(struct at86rf230_local *lp)
 static inline void
 at86rf230_awake(struct at86rf230_local *lp)
 {
-	if (gpio_is_valid(lp->slp_tr)) {
-		gpio_set_value(lp->slp_tr, 0);
+	if (lp->slp_tr) {
+		gpiod_set_value(lp->slp_tr, 0);
 		usleep_range(lp->data->t_sleep_to_off,
 			     lp->data->t_sleep_to_off + 100);
 		lp->sleep = false;
@@ -204,9 +203,9 @@  at86rf230_write_subreg(struct at86rf230_local *lp,
 static inline void
 at86rf230_slp_tr_rising_edge(struct at86rf230_local *lp)
 {
-	gpio_set_value(lp->slp_tr, 1);
+	gpiod_set_value(lp->slp_tr, 1);
 	udelay(1);
-	gpio_set_value(lp->slp_tr, 0);
+	gpiod_set_value(lp->slp_tr, 0);
 }
 
 static bool
@@ -819,7 +818,7 @@  at86rf230_write_frame_complete(void *context)
 
 	ctx->trx.len = 2;
 
-	if (gpio_is_valid(lp->slp_tr))
+	if (lp->slp_tr)
 		at86rf230_slp_tr_rising_edge(lp);
 	else
 		at86rf230_async_write_reg(lp, RG_TRX_STATE, STATE_BUSY_TX, ctx,
@@ -1415,32 +1414,6 @@  static int at86rf230_hw_init(struct at86rf230_local *lp, u8 xtal_trim)
 	return at86rf230_write_subreg(lp, SR_SLOTTED_OPERATION, 0);
 }
 
-static int
-at86rf230_get_pdata(struct spi_device *spi, int *rstn, int *slp_tr,
-		    u8 *xtal_trim)
-{
-	struct at86rf230_platform_data *pdata = spi->dev.platform_data;
-	int ret;
-
-	if (!IS_ENABLED(CONFIG_OF) || !spi->dev.of_node) {
-		if (!pdata)
-			return -ENOENT;
-
-		*rstn = pdata->rstn;
-		*slp_tr = pdata->slp_tr;
-		*xtal_trim = pdata->xtal_trim;
-		return 0;
-	}
-
-	*rstn = of_get_named_gpio(spi->dev.of_node, "reset-gpio", 0);
-	*slp_tr = of_get_named_gpio(spi->dev.of_node, "sleep-gpio", 0);
-	ret = of_property_read_u8(spi->dev.of_node, "xtal-trim", xtal_trim);
-	if (ret < 0 && ret != -EINVAL)
-		return ret;
-
-	return 0;
-}
-
 static int
 at86rf230_detect_device(struct at86rf230_local *lp)
 {
@@ -1547,7 +1520,8 @@  static int at86rf230_probe(struct spi_device *spi)
 	struct ieee802154_hw *hw;
 	struct at86rf230_local *lp;
 	unsigned int status;
-	int rc, irq_type, rstn, slp_tr;
+	int rc, irq_type;
+	struct gpio_desc *rstn, *slp_tr;
 	u8 xtal_trim = 0;
 
 	if (!spi->irq) {
@@ -1555,32 +1529,26 @@  static int at86rf230_probe(struct spi_device *spi)
 		return -EINVAL;
 	}
 
-	rc = at86rf230_get_pdata(spi, &rstn, &slp_tr, &xtal_trim);
-	if (rc < 0) {
-		dev_err(&spi->dev, "failed to parse platform_data: %d\n", rc);
+	rc = device_property_read_u8(&spi->dev, "xtal-trim", &xtal_trim);
+	if (rc < 0 && rc != -EINVAL) {
+		dev_err(&spi->dev, "failed to parse xtal-trim: %d\n", rc);
 		return rc;
 	}
 
-	if (gpio_is_valid(rstn)) {
-		rc = devm_gpio_request_one(&spi->dev, rstn,
-					   GPIOF_OUT_INIT_HIGH, "rstn");
-		if (rc)
-			return rc;
-	}
+	rstn = devm_gpiod_get_optional(&spi->dev, "rstn", GPIOD_OUT_HIGH);
+	if (IS_ERR(rstn))
+		return PTR_ERR(rstn);
 
-	if (gpio_is_valid(slp_tr)) {
-		rc = devm_gpio_request_one(&spi->dev, slp_tr,
-					   GPIOF_OUT_INIT_LOW, "slp_tr");
-		if (rc)
-			return rc;
-	}
+	slp_tr = devm_gpiod_get_optional(&spi->dev, "slp_tr", GPIOD_OUT_LOW);
+	if (IS_ERR(slp_tr))
+		return PTR_ERR(slp_tr);
 
 	/* Reset */
-	if (gpio_is_valid(rstn)) {
+	if (rstn) {
 		udelay(1);
-		gpio_set_value_cansleep(rstn, 0);
+		gpiod_set_value_cansleep(rstn, 0);
 		udelay(1);
-		gpio_set_value_cansleep(rstn, 1);
+		gpiod_set_value_cansleep(rstn, 1);
 		usleep_range(120, 240);
 	}
 
@@ -1682,7 +1650,7 @@  MODULE_DEVICE_TABLE(spi, at86rf230_device_id);
 static struct spi_driver at86rf230_driver = {
 	.id_table = at86rf230_device_id,
 	.driver = {
-		.of_match_table = of_match_ptr(at86rf230_of_match),
+		.of_match_table = at86rf230_of_match,
 		.name	= "at86rf230",
 	},
 	.probe      = at86rf230_probe,
diff --git a/include/linux/spi/at86rf230.h b/include/linux/spi/at86rf230.h
deleted file mode 100644
index d278576ab692..000000000000
--- a/include/linux/spi/at86rf230.h
+++ /dev/null
@@ -1,20 +0,0 @@ 
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * AT86RF230/RF231 driver
- *
- * Copyright (C) 2009-2012 Siemens AG
- *
- * Written by:
- * Dmitry Eremin-Solenikov <dmitry.baryshkov@siemens.com>
- */
-#ifndef AT86RF230_H
-#define AT86RF230_H
-
-struct at86rf230_platform_data {
-	int rstn;
-	int slp_tr;
-	int dig2;
-	u8 xtal_trim;
-};
-
-#endif