staging: iio: ad7816: Switch to the gpio descriptor interface
Subsystems (4)
PATCH v2 1/1 staging: iio: ad7816: Switch to the gpio descriptor interface
Use the gpiod interface for rdwr_pin, convert_pin and busy_pin instead of the deprecated old non-descriptor interface.
---
Changes in v2:
- Correct the error messages as pin number being showed has now been replaced by error code.
--- drivers/staging/iio/adc/ad7816.c | 80 ++++++++++++++------------------ 1 file changed, 34 insertions(+), 46 deletions(-)
drivers/staging/iio/adc/ad7816.c +34 -46
@@ -7,7 +7,7 @@*/ #include <linux/interrupt.h> -#include <linux/gpio.h> +#include <linux/gpio/consumer.h> #include <linux/device.h> #include <linux/kernel.h> #include <linux/slab.h>
@@ -44,9 +44,9 @@struct ad7816_chip_info {struct spi_device *spi_dev; -u16 rdwr_pin; -u16 convert_pin; -u16 busy_pin; +struct gpio_desc *rdwr_pin; +struct gpio_desc *convert_pin; +struct gpio_desc *busy_pin;u8 oti_data[AD7816_CS_MAX +1 ];u8 channel_id;/* 0 always be temperature */ u8 mode;
@@ -61,28 +61,28 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)int ret =0 ;__be16 buf; -gpio_set_value (chip->rdwr_pin,1 ); -gpio_set_value (chip->rdwr_pin,0 ); +gpiod_set_value (chip->rdwr_pin,1 ); +gpiod_set_value (chip->rdwr_pin,0 ); ret =spi_write (spi_dev, &chip->channel_id,sizeof (chip->channel_id));if (ret <0 ) {dev_err (&spi_dev->dev,"SPI channel setting error\n" );return ret; } -gpio_set_value (chip->rdwr_pin,1 ); +gpiod_set_value (chip->rdwr_pin,1 );if (chip->mode ==AD7816_PD ) {/* operating mode 2 */ -gpio_set_value (chip->convert_pin,1 ); -gpio_set_value (chip->convert_pin,0 ); +gpiod_set_value (chip->convert_pin,1 ); +gpiod_set_value (chip->convert_pin,0 ); }else {/* operating mode 1 */ -gpio_set_value (chip->convert_pin,0 ); -gpio_set_value (chip->convert_pin,1 ); +gpiod_set_value (chip->convert_pin,0 ); +gpiod_set_value (chip->convert_pin,1 ); } -while (gpio_get_value (chip->busy_pin)) +while (gpiod_get_value (chip->busy_pin))cpu_relax (); -gpio_set_value (chip->rdwr_pin,0 ); -gpio_set_value (chip->rdwr_pin,1 ); +gpiod_set_value (chip->rdwr_pin,0 ); +gpiod_set_value (chip->rdwr_pin,1 ); ret =spi_read (spi_dev, &buf,sizeof (*data));if (ret <0 ) {dev_err (&spi_dev->dev,"SPI data read error\n" );
@@ -99,8 +99,8 @@ static int ad7816_spi_write(struct ad7816_chip_info *chip, u8 data)struct spi_device *spi_dev = chip->spi_dev;int ret =0 ; -gpio_set_value (chip->rdwr_pin,1 ); -gpio_set_value (chip->rdwr_pin,0 ); +gpiod_set_value (chip->rdwr_pin,1 ); +gpiod_set_value (chip->rdwr_pin,0 ); ret =spi_write (spi_dev, &data,sizeof (data));if (ret <0 )dev_err (&spi_dev->dev,"SPI oti data write error\n" );
@@ -129,10 +129,10 @@ static ssize_t ad7816_store_mode(struct device *dev,struct ad7816_chip_info *chip =iio_priv (indio_dev);if (strcmp (buf,"full" )) { -gpio_set_value (chip->rdwr_pin,1 ); +gpiod_set_value (chip->rdwr_pin,1 ); chip->mode =AD7816_FULL ; }else { -gpio_set_value (chip->rdwr_pin,0 ); +gpiod_set_value (chip->rdwr_pin,0 ); chip->mode =AD7816_PD ; }
@@ -345,15 +345,9 @@ static int ad7816_probe(struct spi_device *spi_dev) {struct ad7816_chip_info *chip;struct iio_dev *indio_dev; -unsigned short *pins =dev_get_platdata (&spi_dev->dev);int ret =0 ;int i; -if (!pins) { -dev_err (&spi_dev->dev,"No necessary GPIO platform data.\n" ); -return -EINVAL ; - } - indio_dev =devm_iio_device_alloc (&spi_dev->dev,sizeof (*chip));if (!indio_dev)return -ENOMEM ;
@@ -364,34 +358,28 @@ static int ad7816_probe(struct spi_device *spi_dev) chip->spi_dev = spi_dev;for (i =0 ; i <=AD7816_CS_MAX ; i++) chip->oti_data[i] =203 ; - chip->rdwr_pin = pins[0 ]; - chip->convert_pin = pins[1 ]; - chip->busy_pin = pins[2 ]; - - ret =devm_gpio_request (&spi_dev->dev, chip->rdwr_pin, -spi_get_device_id (spi_dev)->name); -if (ret) { -dev_err (&spi_dev->dev,"Fail to request rdwr gpio PIN %d.\n" , - chip->rdwr_pin); + + chip->rdwr_pin =devm_gpiod_get (&spi_dev->dev,"rdwr" ,GPIOD_IN ); +if (IS_ERR (chip->rdwr_pin)) { + ret =PTR_ERR (chip->rdwr_pin); +dev_err (&spi_dev->dev,"Failed to request rdwr GPIO: %d\n" , + ret);return ret; } -gpio_direction_input (chip->rdwr_pin); - ret =devm_gpio_request (&spi_dev->dev, chip->convert_pin, -spi_get_device_id (spi_dev)->name); -if (ret) { -dev_err (&spi_dev->dev,"Fail to request convert gpio PIN %d.\n" , - chip->convert_pin); + chip->convert_pin =devm_gpiod_get (&spi_dev->dev,"convert" ,GPIOD_IN ); +if (IS_ERR (chip->convert_pin)) { + ret =PTR_ERR (chip->convert_pin); +dev_err (&spi_dev->dev,"Failed to request convert GPIO: %d\n" , + ret);return ret; } -gpio_direction_input (chip->convert_pin); - ret =devm_gpio_request (&spi_dev->dev, chip->busy_pin, -spi_get_device_id (spi_dev)->name); -if (ret) { -dev_err (&spi_dev->dev,"Fail to request busy gpio PIN %d.\n" , - chip->busy_pin); + chip->busy_pin =devm_gpiod_get (&spi_dev->dev,"busy" ,GPIOD_IN ); +if (IS_ERR (chip->busy_pin)) { + ret =PTR_ERR (chip->busy_pin); +dev_err (&spi_dev->dev,"Failed to request busy GPIO: %d\n" , + ret);return ret; } -gpio_direction_input (chip->busy_pin); indio_dev->name =spi_get_device_id (spi_dev)->name; indio_dev->dev.parent = &spi_dev->dev;
Signature · 1 line
2.17.1
Quoted 4 lines · Nishad Kamdar
Use the gpiod interface for
rdwr_pin,convert_pinandbusy_pininstead of the deprecated old non-descriptor interface.Signed-off-by Nishad Kamdar <[email protected]>
Thanks.
Quoted 8 lines
---
Changes in v2:
- Correct the error messages as pin number being showed has now been replaced by error code.--- drivers/staging/iio/adc/ad7816.c | 80 ++++++++++++++------------------ 1 file changed, 34 insertions(+), 46 deletions(-)
Quoted 31 lines · Nishad Kamdar
drivers/staging/iio/adc/ad7816.c +5
-0+ chip->rdwr_pin =devm_gpiod_get (&spi_dev->dev,"rdwr" ,GPIOD_IN ); +if (IS_ERR (chip->rdwr_pin)) { + ret =PTR_ERR (chip->rdwr_pin); +dev_err (&spi_dev->dev,"Failed to request rdwr GPIO: %d\n" , + ret);return ret; }drivers/staging/iio/adc/ad7816.c +5
-6-gpio_direction_input (chip->rdwr_pin); - ret =devm_gpio_request (&spi_dev->dev, chip->convert_pin, -spi_get_device_id (spi_dev)->name); -if (ret) { -dev_err (&spi_dev->dev,"Fail to request convert gpio PIN %d.\n" , - chip->convert_pin); + chip->convert_pin =devm_gpiod_get (&spi_dev->dev,"convert" ,GPIOD_IN ); +if (IS_ERR (chip->convert_pin)) { + ret =PTR_ERR (chip->convert_pin); +dev_err (&spi_dev->dev,"Failed to request convert GPIO: %d\n" , + ret);return ret; }drivers/staging/iio/adc/ad7816.c +5
-6-gpio_direction_input (chip->convert_pin); - ret =devm_gpio_request (&spi_dev->dev, chip->busy_pin, -spi_get_device_id (spi_dev)->name); -if (ret) { -dev_err (&spi_dev->dev,"Fail to request busy gpio PIN %d.\n" , - chip->busy_pin); + chip->busy_pin =devm_gpiod_get (&spi_dev->dev,"busy" ,GPIOD_IN ); +if (IS_ERR (chip->busy_pin)) { + ret =PTR_ERR (chip->busy_pin); +dev_err (&spi_dev->dev,"Failed to request busy GPIO: %d\n" , + ret);
Quoted 2 lines
return ret; }
Hm, from what I can tell devm_gpio_request() is allocating some memory, which makes this a series of 4 allocations.
What happens if the fourth allocation fails? Do we leak the first three?
--
Thanks,
Sasha
Quoted 38 lines · Sasha Levin
Quoted 31 lines · Nishad Kamdar
drivers/staging/iio/adc/ad7816.c +5
-0+ chip->rdwr_pin =devm_gpiod_get (&spi_dev->dev,"rdwr" ,GPIOD_IN ); +if (IS_ERR (chip->rdwr_pin)) { + ret =PTR_ERR (chip->rdwr_pin); +dev_err (&spi_dev->dev,"Failed to request rdwr GPIO: %d\n" , + ret);return ret; }drivers/staging/iio/adc/ad7816.c +5
-6-gpio_direction_input (chip->rdwr_pin); - ret =devm_gpio_request (&spi_dev->dev, chip->convert_pin, -spi_get_device_id (spi_dev)->name); -if (ret) { -dev_err (&spi_dev->dev,"Fail to request convert gpio PIN %d.\n" , - chip->convert_pin); + chip->convert_pin =devm_gpiod_get (&spi_dev->dev,"convert" ,GPIOD_IN ); +if (IS_ERR (chip->convert_pin)) { + ret =PTR_ERR (chip->convert_pin); +dev_err (&spi_dev->dev,"Failed to request convert GPIO: %d\n" , + ret);return ret; }drivers/staging/iio/adc/ad7816.c +5
-6-gpio_direction_input (chip->convert_pin); - ret =devm_gpio_request (&spi_dev->dev, chip->busy_pin, -spi_get_device_id (spi_dev)->name); -if (ret) { -dev_err (&spi_dev->dev,"Fail to request busy gpio PIN %d.\n" , - chip->busy_pin); + chip->busy_pin =devm_gpiod_get (&spi_dev->dev,"busy" ,GPIOD_IN ); +if (IS_ERR (chip->busy_pin)) { + ret =PTR_ERR (chip->busy_pin); +dev_err (&spi_dev->dev,"Failed to request busy GPIO: %d\n" , + ret);Quoted 2 lines
return ret; }Hm, from what I can tell
devm_gpio_request()is allocating some memory, which makes this a series of 4 allocations.
Quoted 1 line
What happens if the fourth allocation fails? Do we leak the first three?
These are devm_ allocations. They are freed when &spi_dev->dev is released.
regards,
dan carpenter
Quoted 129 lines · Nishad Kamdar
Use the gpiod interface for
rdwr_pin,convert_pinandbusy_pininstead of the deprecated old non-descriptor interface.Signed-off-by Nishad Kamdar <[email protected]>---
Changes in v2:
- Correct the error messages as pin number being showed has now been replaced by error code.--- drivers/staging/iio/adc/ad7816.c | 80 ++++++++++++++------------------ 1 file changed, 34 insertions(+), 46 deletions(-)drivers/staging/iio/adc/ad7816.c +18
-32@@ -7,7 +7,7 @@*/ #include <linux/interrupt.h> -#include <linux/gpio.h> +#include <linux/gpio/consumer.h> #include <linux/device.h> #include <linux/kernel.h> #include <linux/slab.h> @@ -44,9 +44,9 @@struct ad7816_chip_info {struct spi_device *spi_dev; -u16 rdwr_pin; -u16 convert_pin; -u16 busy_pin; +struct gpio_desc *rdwr_pin; +struct gpio_desc *convert_pin; +struct gpio_desc *busy_pin;u8 oti_data[AD7816_CS_MAX +1 ];u8 channel_id;/* 0 always be temperature */ u8 mode;@@ -61,28 +61,28 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)int ret =0 ;__be16 buf; -gpio_set_value (chip->rdwr_pin,1 ); -gpio_set_value (chip->rdwr_pin,0 ); +gpiod_set_value (chip->rdwr_pin,1 ); +gpiod_set_value (chip->rdwr_pin,0 ); ret =spi_write (spi_dev, &chip->channel_id,sizeof (chip->channel_id));if (ret <0 ) {dev_err (&spi_dev->dev,"SPI channel setting error\n" );return ret; } -gpio_set_value (chip->rdwr_pin,1 ); +gpiod_set_value (chip->rdwr_pin,1 );if (chip->mode ==AD7816_PD ) {/* operating mode 2 */ -gpio_set_value (chip->convert_pin,1 ); -gpio_set_value (chip->convert_pin,0 ); +gpiod_set_value (chip->convert_pin,1 ); +gpiod_set_value (chip->convert_pin,0 ); }else {/* operating mode 1 */ -gpio_set_value (chip->convert_pin,0 ); -gpio_set_value (chip->convert_pin,1 ); +gpiod_set_value (chip->convert_pin,0 ); +gpiod_set_value (chip->convert_pin,1 ); } -while (gpio_get_value (chip->busy_pin)) +while (gpiod_get_value (chip->busy_pin))cpu_relax (); -gpio_set_value (chip->rdwr_pin,0 ); -gpio_set_value (chip->rdwr_pin,1 ); +gpiod_set_value (chip->rdwr_pin,0 ); +gpiod_set_value (chip->rdwr_pin,1 ); ret =spi_read (spi_dev, &buf,sizeof (*data));if (ret <0 ) {dev_err (&spi_dev->dev,"SPI data read error\n" );@@ -99,8 +99,8 @@ static int ad7816_spi_write(struct ad7816_chip_info *chip, u8 data)struct spi_device *spi_dev = chip->spi_dev;int ret =0 ; -gpio_set_value (chip->rdwr_pin,1 ); -gpio_set_value (chip->rdwr_pin,0 ); +gpiod_set_value (chip->rdwr_pin,1 ); +gpiod_set_value (chip->rdwr_pin,0 ); ret =spi_write (spi_dev, &data,sizeof (data));if (ret <0 )dev_err (&spi_dev->dev,"SPI oti data write error\n" );@@ -129,10 +129,10 @@ static ssize_t ad7816_store_mode(struct device *dev,struct ad7816_chip_info *chip =iio_priv (indio_dev);if (strcmp (buf,"full" )) { -gpio_set_value (chip->rdwr_pin,1 ); +gpiod_set_value (chip->rdwr_pin,1 ); chip->mode =AD7816_FULL ; }else { -gpio_set_value (chip->rdwr_pin,0 ); +gpiod_set_value (chip->rdwr_pin,0 ); chip->mode =AD7816_PD ; }@@ -345,15 +345,9 @@ static int ad7816_probe(struct spi_device *spi_dev) {struct ad7816_chip_info *chip;struct iio_dev *indio_dev; -unsigned short *pins =dev_get_platdata (&spi_dev->dev);int ret =0 ;int i; -if (!pins) { -dev_err (&spi_dev->dev,"No necessary GPIO platform data.\n" ); -return -EINVAL ; - } - indio_dev =devm_iio_device_alloc (&spi_dev->dev,sizeof (*chip));if (!indio_dev)return -ENOMEM ;@@ -364,34 +358,28 @@ static int ad7816_probe(struct spi_device *spi_dev) chip->spi_dev = spi_dev;for (i =0 ; i <=AD7816_CS_MAX ; i++) chip->oti_data[i] =203 ; - chip->rdwr_pin = pins[0 ]; - chip->convert_pin = pins[1 ]; - chip->busy_pin = pins[2 ]; - - ret =devm_gpio_request (&spi_dev->dev, chip->rdwr_pin, -spi_get_device_id (spi_dev)->name); -if (ret) { - dev_err(&spi_dev->dev, "Fail to request rdwr gpio PIN %d.\n",
drivers/staging/iio/adc/ad7816.c static int ad7816_probe(struct spi_device *spi_dev) · line 378
Quoted 10 lines
drivers/staging/iio/adc/ad7816.c +6
-2- chip->rdwr_pin); + + chip->rdwr_pin =devm_gpiod_get (&spi_dev->dev,"rdwr" ,GPIOD_IN ); +if (IS_ERR (chip->rdwr_pin)) { + ret =PTR_ERR (chip->rdwr_pin); +dev_err (&spi_dev->dev,"Failed to request rdwr GPIO: %d\n" , + ret);return ret; } -gpio_direction_input (chip->rdwr_pin);
The RD/WR pin is an input to the AD78xx. So this doesn't make sense being GPIOD_IN.
drivers/staging/iio/adc/ad7816.c static int ad7816_probe(struct spi_device *spi_dev) · line 386
Quoted 13 lines
drivers/staging/iio/adc/ad7816.c +5
-6- ret =devm_gpio_request (&spi_dev->dev, chip->convert_pin, -spi_get_device_id (spi_dev)->name); -if (ret) { -dev_err (&spi_dev->dev,"Fail to request convert gpio PIN %d.\n" , - chip->convert_pin); + chip->convert_pin =devm_gpiod_get (&spi_dev->dev,"convert" ,GPIOD_IN ); +if (IS_ERR (chip->convert_pin)) { + ret =PTR_ERR (chip->convert_pin); +dev_err (&spi_dev->dev,"Failed to request convert GPIO: %d\n" , + ret);return ret; } -gpio_direction_input (chip->convert_pin);
ditto, the CONVST pin is an input to the AD78xx.
drivers/staging/iio/adc/ad7816.c static int ad7816_probe(struct spi_device *spi_dev) · line 394
Quoted 13 lines
drivers/staging/iio/adc/ad7816.c +5
-6- ret =devm_gpio_request (&spi_dev->dev, chip->busy_pin, -spi_get_device_id (spi_dev)->name); -if (ret) { -dev_err (&spi_dev->dev,"Fail to request busy gpio PIN %d.\n" , - chip->busy_pin); + chip->busy_pin =devm_gpiod_get (&spi_dev->dev,"busy" ,GPIOD_IN ); +if (IS_ERR (chip->busy_pin)) { + ret =PTR_ERR (chip->busy_pin); +dev_err (&spi_dev->dev,"Failed to request busy GPIO: %d\n" , + ret);return ret; } -gpio_direction_input (chip->busy_pin);
The busy pin doesn't exist on the ad7818. Which the driver claims to support in the id table:
Quoted 6 lines
drivers/staging/iio/adc/ad7816.c +0
-0static const struct spi_device_id ad7816_id[] = { {"ad7816" ,0 }, {"ad7817" ,0 },{ "ad7818", 0 }, {} };
See:
https://www.analog.com/media/en/technical-documentation/data-sheets/AD7817_7818.pdf
Page 9.
Quoted 4 lines
indio_dev->name = spi_get_device_id(spi_dev)->name; indio_dev->dev.parent = &spi_dev->dev;
Also should the pin names be documented in a device tree binding doc?
Signature · 2 lines
Regards Phil Reid
On 10/18/2018 09:28 AM, Phil Reid wrote:
[...]
Quoted 11 lines
Quoted 8 lines
+
chip->rdwr_pin=devm_gpiod_get(&spi_dev->dev, "rdwr", GPIOD_IN); + if (IS_ERR(chip->rdwr_pin)) {
+ ret =PTR_ERR(chip->rdwr_pin);
+dev_err(&spi_dev->dev, "Failed to request rdwr GPIO: %d\n", + ret);
return ret;
}
-gpio_direction_input(chip->rdwr_pin);The RD/WR pin is an input to the AD78xx. So this doesn't make sense being
GPIOD_IN.
One thing at a time. This patch is a straight forward conversion to the GPIO descriptor interface. It keeps the existing semantics of the driver as they are.
Now these semantics are obviously wrong and should be fixed but that should be a separate patch from changing the interface.
Jonathan Cameron
maintainerQuoted 11 lines · On Thu, 18 Oct 2018 09:40:00 +0200 Lars-Peter Clausen <[email protected]> wrote:
On 10/18/2018 09:28 AM, Phil Reid wrote:
[...]Quoted 9 lines
Quoted 8 lines
+
chip->rdwr_pin=devm_gpiod_get(&spi_dev->dev, "rdwr", GPIOD_IN); + if (IS_ERR(chip->rdwr_pin)) {
+ ret =PTR_ERR(chip->rdwr_pin);
+dev_err(&spi_dev->dev, "Failed to request rdwr GPIO: %d\n", + ret);
return ret;
}
-gpio_direction_input(chip->rdwr_pin);
Quoted 8 lines
Quoted 2 lines
The RD/WR pin is an input to the AD78xx. So this doesn't make sense being
GPIOD_IN.One thing at a time. This patch is a straight forward conversion to the GPIO descriptor interface. It keeps the existing semantics of the driver as they are.
Now these semantics are obviously wrong and should be fixed but that should be a separate patch from changing the interface.
Agreed. Useful to raise these issues however, and I've added a note to the patch to bring this to anyone's attention should they be interesting.
Thanks,
Jonathan