| Message ID | 20181017144716.GA11485@nishad (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] staging: iio: ad7816: Switch to the gpio descriptor interface | expand |
On 10/17/2018 04:47 PM, Nishad Kamdar wrote: > Use the gpiod interface for rdwr_pin, convert_pin and busy_pin > instead of the deprecated old non-descriptor interface. > > Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com> Acked-by: Lars-Peter Clausen <lars@metafoo.de> Thanks. > --- > 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(-) >
On Wed, Oct 17, 2018 at 08:17:20PM +0530, Nishad Kamdar wrote: >+ 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; > } 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
On Wed, Oct 17, 2018 at 10:58:23AM -0400, Sasha Levin wrote: > On Wed, Oct 17, 2018 at 08:17:20PM +0530, Nishad Kamdar wrote: > > + 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; > > } > > 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? These are devm_ allocations. They are freed when &spi_dev->dev is released. regards, dan carpenter
On 17/10/2018 10:47 PM, Nishad Kamdar wrote: > Use the gpiod interface for rdwr_pin, convert_pin and busy_pin > instead of the deprecated old non-descriptor interface. > > Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com> > --- > 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(-) > > diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c > index bf76a8620bdb..12c4e0ab4713 100644 > --- a/drivers/staging/iio/adc/ad7816.c > +++ b/drivers/staging/iio/adc/ad7816.c > @@ -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); The RD/WR pin is an input to the AD78xx. So this doesn't make sense being GPIOD_IN. > - 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. > - 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: > static 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. > > 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?
On 10/18/2018 09:28 AM, Phil Reid wrote: [...] >> + 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.
On Thu, 18 Oct 2018 09:40:00 +0200 Lars-Peter Clausen <lars@metafoo.de> wrote: > On 10/18/2018 09:28 AM, Phil Reid wrote: > [...] > >> + 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. 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
diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c index bf76a8620bdb..12c4e0ab4713 100644 --- a/drivers/staging/iio/adc/ad7816.c +++ b/drivers/staging/iio/adc/ad7816.c @@ -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;
Use the gpiod interface for rdwr_pin, convert_pin and busy_pin instead of the deprecated old non-descriptor interface. Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com> --- 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(-)