[PATCH v3 0/4] staging: iio: ad7816: Switch to the gpio descriptor interface

[PATCH v3 0/4] staging: iio: ad7816: Switch to the gpio descriptor interface

[Nishad Kamdar]

Changes in v4:
 - Drop busy pin in case of AD7818.
 - Set RD/WR pin and CONVST pin as outputs.
 - Add device tree table.

Nishad Kamdar (4):
  staging: iio: ad7816: Switch to the gpio descriptor interface
  staging: iio: ad7816: Do not use busy_pin in case of AD7818
  staging: iio: ad7816: Set RD/WR pin and CONVST pin as outputs.
  staging: iio: ad7816: Add device tree table.

 drivers/staging/iio/adc/ad7816.c | 111 +++++++++++++++++--------------
 1 file changed, 60 insertions(+), 51 deletions(-)

-- 
2.17.1

[PATCH v3 1/4] staging: iio: ad7816: Switch to the gpio descriptor interface

[Nishad Kamdar]

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>
---
 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);
-	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;
-- 
2.17.1

Re: [PATCH v3 1/4] staging: iio: ad7816: Switch to the gpio descriptor interface

[Ardelean, Alexandru]

On Fri, 2018-11-09 at 13:05 +0530, Nishad Kamdar wrote:
> Use the gpiod interface for rdwr_pin, convert_pin and busy_pin
> instead of the deprecated old non-descriptor interface.
> 

Patch looks good.
I do have some thoughts about it. See inline.


> Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
> ---
>  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;

Only if you want to do a re-spin, here's an idea.
If you don't want to, feel free to disregard, since your patch is good.

You could compact things a bit; I know this wasn't what the initial code
was doing, but it's an option.

Something like:

enum {
	GPIO_RWDR,
	GPIO_CONVERT,
	GPIO_BUSY,
	__GPIO_MAX,
}

Then, what you end up having is:

 struct ad7816_chip_info {
  	struct spi_device *spi_dev;
 -	u16 rdwr_pin;
 -	u16 convert_pin;
 -	u16 busy_pin;
 +	struct gpio_desc *gpios[__GPIO_MAX];
 

// Continued below


>  	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);

Obviously, in the above context, this becomes:
 +	gpiod_set_value(chip->gpios[GPIO_RWDR], 1);
 +	gpiod_set_value(chip->gpios[GPIO_RWDR], 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;
>  	}

But the real benefit if this approach comes here.
You could do:

static const char *gpio_names[__GPIO_MAX] = {
	[GPIO_RWDR] = "rwdr",
	[GPIO_CONVERT] = "convert",
	[GPIO_BUSY] = "busy",
};

// code for patch2	
bool have_busy_pin = (chip->id == ID_AD7816 || chip->id == ID_AD7817);
// end code for patch2	


for (i = 0; i < __GPIO_MAX; i++) {
// code for patch2	
	if (i == GPIO_BUSY && !have_busy_pin)
		continue;
// end code for patch2	

	chip->gpios[i] = devm_gpiod_get(&spi_dev->dev, gpio_names[i], 
					GPIOD_IN);
	if (IS_ERR(chip->gpios[i])) {
		ret = PTR_ERR(chip->gpios[i]);
		dev_err(&spi_dev->dev, "Failed to request rdwr GPIO: %d\n",
			ret;
		return ret;
	}
}

Then, in your patch2 (i.e " staging: iio: ad7816: Do not use busy_pin in
case of AD7818"), what you could do, is to add that code (along with the
other ID code).

And finaly what you could do in patch2, is:

	while (chip->gpios[GPIO_BUSY] &&
	       gpiod_get_value(chip->busy_pin))
		cpu_relax();



> -	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;

Re: [PATCH v3 1/4] staging: iio: ad7816: Switch to the gpio descriptor interface

[Ardelean, Alexandru]

On Fri, 2018-11-09 at 08:05 +0000, Ardelean, Alexandru wrote:
> On Fri, 2018-11-09 at 13:05 +0530, Nishad Kamdar wrote:
> > Use the gpiod interface for rdwr_pin, convert_pin and busy_pin
> > instead of the deprecated old non-descriptor interface.
> > 
> 
> Patch looks good.
> I do have some thoughts about it. See inline.
> 

Nevermind what I just said here; I just saw patch3: `iio: ad7816: Set RD/WR
pin and CONVST pin as outputs.`

This looks good as is in the context of the patch series.

> 
> > Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
> > ---
> >  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;
> 
> Only if you want to do a re-spin, here's an idea.
> If you don't want to, feel free to disregard, since your patch is good.
> 
> You could compact things a bit; I know this wasn't what the initial code
> was doing, but it's an option.
> 
> Something like:
> 
> enum {
> 	GPIO_RWDR,
> 	GPIO_CONVERT,
> 	GPIO_BUSY,
> 	__GPIO_MAX,
> }
> 
> Then, what you end up having is:
> 
>  struct ad7816_chip_info {
>   	struct spi_device *spi_dev;
>  -	u16 rdwr_pin;
>  -	u16 convert_pin;
>  -	u16 busy_pin;
>  +	struct gpio_desc *gpios[__GPIO_MAX];
>  
> 
> // Continued below
> 
> 
> >  	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);
> 
> Obviously, in the above context, this becomes:
>  +	gpiod_set_value(chip->gpios[GPIO_RWDR], 1);
>  +	gpiod_set_value(chip->gpios[GPIO_RWDR], 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;
> >  	}
> 
> But the real benefit if this approach comes here.
> You could do:
> 
> static const char *gpio_names[__GPIO_MAX] = {
> 	[GPIO_RWDR] = "rwdr",
> 	[GPIO_CONVERT] = "convert",
> 	[GPIO_BUSY] = "busy",
> };
> 
> // code for patch2	
> bool have_busy_pin = (chip->id == ID_AD7816 || chip->id == ID_AD7817);
> // end code for patch2	
> 
> 
> for (i = 0; i < __GPIO_MAX; i++) {
> // code for patch2	
> 	if (i == GPIO_BUSY && !have_busy_pin)
> 		continue;
> // end code for patch2	
> 
> 	chip->gpios[i] = devm_gpiod_get(&spi_dev->dev, gpio_names[i], 
> 					GPIOD_IN);
> 	if (IS_ERR(chip->gpios[i])) {
> 		ret = PTR_ERR(chip->gpios[i]);
> 		dev_err(&spi_dev->dev, "Failed to request rdwr GPIO: %d\n",
> 			ret;
> 		return ret;
> 	}
> }
> 
> Then, in your patch2 (i.e " staging: iio: ad7816: Do not use busy_pin in
> case of AD7818"), what you could do, is to add that code (along with the
> other ID code).
> 
> And finaly what you could do in patch2, is:
> 
> 	while (chip->gpios[GPIO_BUSY] &&
> 	       gpiod_get_value(chip->busy_pin))
> 		cpu_relax();
> 
> 
> 
> > -	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;

Re: [PATCH v3 1/4] staging: iio: ad7816: Switch to the gpio descriptor interface

[Jonathan Cameron]

On Fri, 9 Nov 2018 13:05:17 +0530
Nishad Kamdar <nishadkamdar@gmail.com> 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>
Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan

> ---
>  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);
> -	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;

Re: [PATCH v3 1/4] staging: iio: ad7816: Switch to the gpio descriptor interface

[Jonathan Cameron]

On Sun, 11 Nov 2018 12:24:05 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 9 Nov 2018 13:05:17 +0530
> Nishad Kamdar <nishadkamdar@gmail.com> 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>  
> Applied to the togreg branch of iio.git and pushed out as testing
> for the autobuilders to play with it.
Actually no it isn't, because I applied the previous version back in
October.  Please avoid resending patches that have already been applied.
It gets very confusing!

Jonathan


> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  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);
> > -	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;  
> 

Re: [PATCH v3 1/4] staging: iio: ad7816: Switch to the gpio descriptor interface

[Nishad Kamdar]

On Sun, Nov 11, 2018 at 12:31:54PM +0000, Jonathan Cameron wrote:
> On Sun, 11 Nov 2018 12:24:05 +0000
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Fri, 9 Nov 2018 13:05:17 +0530
> > Nishad Kamdar <nishadkamdar@gmail.com> 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>  
> > Applied to the togreg branch of iio.git and pushed out as testing
> > for the autobuilders to play with it.
> Actually no it isn't, because I applied the previous version back in
> October.  Please avoid resending patches that have already been applied.
> It gets very confusing!
> 
> Jonathan
>

Sorry about that. I'll keep that in mind.

Thanks for the review.

regards,
Nishad

> 
> > 
> > Thanks,
> > 
> > Jonathan
> > 
> > > ---
> > >  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);
> > > -	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;  
> > 
> 

[PATCH v3 2/4] staging: iio: ad7816: Do not use busy_pin in case of AD7818

[Nishad Kamdar]

AD7818 does not support busy_pin functionality as per datasheet.
Hence drop busy_pin when AD7818 is used.

Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
---
 drivers/staging/iio/adc/ad7816.c | 35 ++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
index 12c4e0ab4713..3cda5cd09365 100644
--- a/drivers/staging/iio/adc/ad7816.c
+++ b/drivers/staging/iio/adc/ad7816.c
@@ -43,6 +43,7 @@
  */
 
 struct ad7816_chip_info {
+	kernel_ulong_t id;
 	struct spi_device *spi_dev;
 	struct gpio_desc *rdwr_pin;
 	struct gpio_desc *convert_pin;
@@ -52,6 +53,12 @@ struct ad7816_chip_info {
 	u8  mode;
 };
 
+enum ad7816_type {
+	ID_AD7816,
+	ID_AD7817,
+	ID_AD7818,
+};
+
 /*
  * ad7816 data access by SPI
  */
@@ -78,8 +85,10 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
 		gpiod_set_value(chip->convert_pin, 1);
 	}
 
-	while (gpiod_get_value(chip->busy_pin))
-		cpu_relax();
+	if (chip->id == ID_AD7816 || chip->id == ID_AD7817) {
+		while (gpiod_get_value(chip->busy_pin))
+			cpu_relax();
+	}
 
 	gpiod_set_value(chip->rdwr_pin, 0);
 	gpiod_set_value(chip->rdwr_pin, 1);
@@ -359,6 +368,7 @@ static int ad7816_probe(struct spi_device *spi_dev)
 	for (i = 0; i <= AD7816_CS_MAX; i++)
 		chip->oti_data[i] = 203;
 
+	chip->id = spi_get_device_id(spi_dev)->driver_data;
 	chip->rdwr_pin = devm_gpiod_get(&spi_dev->dev, "rdwr", GPIOD_IN);
 	if (IS_ERR(chip->rdwr_pin)) {
 		ret = PTR_ERR(chip->rdwr_pin);
@@ -373,12 +383,15 @@ static int ad7816_probe(struct spi_device *spi_dev)
 			ret);
 		return ret;
 	}
-	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;
+	if (chip->id == ID_AD7816 || chip->id == ID_AD7817) {
+		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;
+		}
 	}
 
 	indio_dev->name = spi_get_device_id(spi_dev)->name;
@@ -409,9 +422,9 @@ static int ad7816_probe(struct spi_device *spi_dev)
 }
 
 static const struct spi_device_id ad7816_id[] = {
-	{ "ad7816", 0 },
-	{ "ad7817", 0 },
-	{ "ad7818", 0 },
+	{ "ad7816", ID_AD7816 },
+	{ "ad7817", ID_AD7817 },
+	{ "ad7818", ID_AD7818 },
 	{}
 };
 
-- 
2.17.1

Re: [PATCH v3 2/4] staging: iio: ad7816: Do not use busy_pin in case of AD7818

[Jonathan Cameron]

On Fri, 9 Nov 2018 13:06:24 +0530
Nishad Kamdar <nishadkamdar@gmail.com> wrote:

> AD7818 does not support busy_pin functionality as per datasheet.
> Hence drop busy_pin when AD7818 is used.
> 
> Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

A slight concern that I can't immediately find an answer to,
is whether we should be waiting a short while for the ad7818 given
we can't wait on the gpio?  We can add this in a follow up patch
if it is necessary.

Thanks,

Jonathan
> ---
>  drivers/staging/iio/adc/ad7816.c | 35 ++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
> index 12c4e0ab4713..3cda5cd09365 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -43,6 +43,7 @@
>   */
>  
>  struct ad7816_chip_info {
> +	kernel_ulong_t id;
>  	struct spi_device *spi_dev;
>  	struct gpio_desc *rdwr_pin;
>  	struct gpio_desc *convert_pin;
> @@ -52,6 +53,12 @@ struct ad7816_chip_info {
>  	u8  mode;
>  };
>  
> +enum ad7816_type {
> +	ID_AD7816,
> +	ID_AD7817,
> +	ID_AD7818,
> +};
> +
>  /*
>   * ad7816 data access by SPI
>   */
> @@ -78,8 +85,10 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
>  		gpiod_set_value(chip->convert_pin, 1);
>  	}
>  
> -	while (gpiod_get_value(chip->busy_pin))
> -		cpu_relax();
> +	if (chip->id == ID_AD7816 || chip->id == ID_AD7817) {
> +		while (gpiod_get_value(chip->busy_pin))
> +			cpu_relax();
> +	}
>  
>  	gpiod_set_value(chip->rdwr_pin, 0);
>  	gpiod_set_value(chip->rdwr_pin, 1);
> @@ -359,6 +368,7 @@ static int ad7816_probe(struct spi_device *spi_dev)
>  	for (i = 0; i <= AD7816_CS_MAX; i++)
>  		chip->oti_data[i] = 203;
>  
> +	chip->id = spi_get_device_id(spi_dev)->driver_data;
>  	chip->rdwr_pin = devm_gpiod_get(&spi_dev->dev, "rdwr", GPIOD_IN);
>  	if (IS_ERR(chip->rdwr_pin)) {
>  		ret = PTR_ERR(chip->rdwr_pin);
> @@ -373,12 +383,15 @@ static int ad7816_probe(struct spi_device *spi_dev)
>  			ret);
>  		return ret;
>  	}
> -	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;
> +	if (chip->id == ID_AD7816 || chip->id == ID_AD7817) {
> +		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;
> +		}
>  	}
>  
>  	indio_dev->name = spi_get_device_id(spi_dev)->name;
> @@ -409,9 +422,9 @@ static int ad7816_probe(struct spi_device *spi_dev)
>  }
>  
>  static const struct spi_device_id ad7816_id[] = {
> -	{ "ad7816", 0 },
> -	{ "ad7817", 0 },
> -	{ "ad7818", 0 },
> +	{ "ad7816", ID_AD7816 },
> +	{ "ad7817", ID_AD7817 },
> +	{ "ad7818", ID_AD7818 },
>  	{}
>  };
>  

[PATCH v3 3/4] staging: iio: ad7816: Set RD/WR pin and CONVST pin as outputs.

[Nishad Kamdar]

The RD/WR pin and CONVST pin are logical inputs to the AD78xx
chip as per the datasheet. Hence convert them to outputs.

Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
---
 drivers/staging/iio/adc/ad7816.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
index 3cda5cd09365..a2fead85cd46 100644
--- a/drivers/staging/iio/adc/ad7816.c
+++ b/drivers/staging/iio/adc/ad7816.c
@@ -369,14 +369,15 @@ static int ad7816_probe(struct spi_device *spi_dev)
 		chip->oti_data[i] = 203;
 
 	chip->id = spi_get_device_id(spi_dev)->driver_data;
-	chip->rdwr_pin = devm_gpiod_get(&spi_dev->dev, "rdwr", GPIOD_IN);
+	chip->rdwr_pin = devm_gpiod_get(&spi_dev->dev, "rdwr", GPIOD_OUT_HIGH);
 	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;
 	}
-	chip->convert_pin = devm_gpiod_get(&spi_dev->dev, "convert", GPIOD_IN);
+	chip->convert_pin = devm_gpiod_get(&spi_dev->dev, "convert",
+					   GPIOD_OUT_HIGH);
 	if (IS_ERR(chip->convert_pin)) {
 		ret = PTR_ERR(chip->convert_pin);
 		dev_err(&spi_dev->dev, "Failed to request convert GPIO: %d\n",
-- 
2.17.1

Re: [PATCH v3 3/4] staging: iio: ad7816: Set RD/WR pin and CONVST pin as outputs.

[Jonathan Cameron]

On Fri, 9 Nov 2018 13:07:18 +0530
Nishad Kamdar <nishadkamdar@gmail.com> wrote:

> The RD/WR pin and CONVST pin are logical inputs to the AD78xx
> chip as per the datasheet. Hence convert them to outputs.
> 
> Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
Hi Nishad,

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

This is the sort of patch we might want to think about backporting
for stable, but it is sufficiently complex to do that I' think we'll
wait for someone to actually request it.

Thanks,

Jonathan

> ---
>  drivers/staging/iio/adc/ad7816.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
> index 3cda5cd09365..a2fead85cd46 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -369,14 +369,15 @@ static int ad7816_probe(struct spi_device *spi_dev)
>  		chip->oti_data[i] = 203;
>  
>  	chip->id = spi_get_device_id(spi_dev)->driver_data;
> -	chip->rdwr_pin = devm_gpiod_get(&spi_dev->dev, "rdwr", GPIOD_IN);
> +	chip->rdwr_pin = devm_gpiod_get(&spi_dev->dev, "rdwr", GPIOD_OUT_HIGH);
>  	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;
>  	}
> -	chip->convert_pin = devm_gpiod_get(&spi_dev->dev, "convert", GPIOD_IN);
> +	chip->convert_pin = devm_gpiod_get(&spi_dev->dev, "convert",
> +					   GPIOD_OUT_HIGH);
>  	if (IS_ERR(chip->convert_pin)) {
>  		ret = PTR_ERR(chip->convert_pin);
>  		dev_err(&spi_dev->dev, "Failed to request convert GPIO: %d\n",

[PATCH v3 4/4] staging: iio: ad7816: Add device tree table.

[Nishad Kamdar]

Add device tree table for matching vendor ID.

Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
---
 drivers/staging/iio/adc/ad7816.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
index a2fead85cd46..b8a9149fbac1 100644
--- a/drivers/staging/iio/adc/ad7816.c
+++ b/drivers/staging/iio/adc/ad7816.c
@@ -422,6 +422,12 @@ static int ad7816_probe(struct spi_device *spi_dev)
 	return 0;
 }
 
+static const struct of_device_id ad7816_of_match[] = {
+	{ .compatible = "adi,ad7816", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad7816_of_match);
+
 static const struct spi_device_id ad7816_id[] = {
 	{ "ad7816", ID_AD7816 },
 	{ "ad7817", ID_AD7817 },
@@ -434,6 +440,7 @@ MODULE_DEVICE_TABLE(spi, ad7816_id);
 static struct spi_driver ad7816_driver = {
 	.driver = {
 		.name = "ad7816",
+		.of_match_table = of_match_ptr(ad7816_of_match),
 	},
 	.probe = ad7816_probe,
 	.id_table = ad7816_id,
-- 
2.17.1

Re: [PATCH v3 4/4] staging: iio: ad7816: Add device tree table.

[Ardelean, Alexandru]

On Fri, 2018-11-09 at 13:08 +0530, Nishad Kamdar wrote:
> Add device tree table for matching vendor ID.

One comment inline for this.

Thanks
Alex

> 
> Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
> ---
>  drivers/staging/iio/adc/ad7816.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/staging/iio/adc/ad7816.c
> b/drivers/staging/iio/adc/ad7816.c
> index a2fead85cd46..b8a9149fbac1 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -422,6 +422,12 @@ static int ad7816_probe(struct spi_device *spi_dev)
>  	return 0;
>  }
>  
> +static const struct of_device_id ad7816_of_match[] = {
> +	{ .compatible = "adi,ad7816", },

I think you also need to add:
+	{ .compatible = "adi,ad7817", },
+	{ .compatible = "adi,ad7818", },

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ad7816_of_match);
> +
>  static const struct spi_device_id ad7816_id[] = {
>  	{ "ad7816", ID_AD7816 },
>  	{ "ad7817", ID_AD7817 },
> @@ -434,6 +440,7 @@ MODULE_DEVICE_TABLE(spi, ad7816_id);
>  static struct spi_driver ad7816_driver = {
>  	.driver = {
>  		.name = "ad7816",
> +		.of_match_table = of_match_ptr(ad7816_of_match),
>  	},
>  	.probe = ad7816_probe,
>  	.id_table = ad7816_id,

Re: [PATCH v3 4/4] staging: iio: ad7816: Add device tree table.

[Jonathan Cameron]

On Fri, 9 Nov 2018 08:11:57 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Fri, 2018-11-09 at 13:08 +0530, Nishad Kamdar wrote:
> > Add device tree table for matching vendor ID.  
> 
> One comment inline for this.
> 
> Thanks
> Alex
> 
> > 
> > Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
> > ---
> >  drivers/staging/iio/adc/ad7816.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/staging/iio/adc/ad7816.c
> > b/drivers/staging/iio/adc/ad7816.c
> > index a2fead85cd46..b8a9149fbac1 100644
> > --- a/drivers/staging/iio/adc/ad7816.c
> > +++ b/drivers/staging/iio/adc/ad7816.c
> > @@ -422,6 +422,12 @@ static int ad7816_probe(struct spi_device *spi_dev)
> >  	return 0;
> >  }
> >  
> > +static const struct of_device_id ad7816_of_match[] = {
> > +	{ .compatible = "adi,ad7816", },  
> 
> I think you also need to add:
> +	{ .compatible = "adi,ad7817", },
> +	{ .compatible = "adi,ad7818", },
> 

Yes, please add all parts to the dt binding.
Particularly as they are functionally different in this case!

Please send a v4 for just this patch as the rest have now been
applied.

Thanks,


Jonathan

> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ad7816_of_match);
> > +
> >  static const struct spi_device_id ad7816_id[] = {
> >  	{ "ad7816", ID_AD7816 },
> >  	{ "ad7817", ID_AD7817 },
> > @@ -434,6 +440,7 @@ MODULE_DEVICE_TABLE(spi, ad7816_id);
> >  static struct spi_driver ad7816_driver = {
> >  	.driver = {
> >  		.name = "ad7816",
> > +		.of_match_table = of_match_ptr(ad7816_of_match),
> >  	},
> >  	.probe = ad7816_probe,
> >  	.id_table = ad7816_id,  

Re: [PATCH v3 4/4] staging: iio: ad7816: Add device tree table.

[Nishad Kamdar]

On Sun, Nov 11, 2018 at 12:38:02PM +0000, Jonathan Cameron wrote:
> On Fri, 9 Nov 2018 08:11:57 +0000
> "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> 
> > On Fri, 2018-11-09 at 13:08 +0530, Nishad Kamdar wrote:
> > > Add device tree table for matching vendor ID.  
> > 
> > One comment inline for this.
> > 
> > Thanks
> > Alex
> > 
> > > 
> > > Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
> > > ---
> > >  drivers/staging/iio/adc/ad7816.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/staging/iio/adc/ad7816.c
> > > b/drivers/staging/iio/adc/ad7816.c
> > > index a2fead85cd46..b8a9149fbac1 100644
> > > --- a/drivers/staging/iio/adc/ad7816.c
> > > +++ b/drivers/staging/iio/adc/ad7816.c
> > > @@ -422,6 +422,12 @@ static int ad7816_probe(struct spi_device *spi_dev)
> > >  	return 0;
> > >  }
> > >  
> > > +static const struct of_device_id ad7816_of_match[] = {
> > > +	{ .compatible = "adi,ad7816", },  
> > 
> > I think you also need to add:
> > +	{ .compatible = "adi,ad7817", },
> > +	{ .compatible = "adi,ad7818", },
> > 
> 
> Yes, please add all parts to the dt binding.
> Particularly as they are functionally different in this case!
> 
> Please send a v4 for just this patch as the rest have now been
> applied.
> 
> Thanks,
> 
> 
> Jonathan
> 
> > > +	{ }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, ad7816_of_match);
> > > +
> > >  static const struct spi_device_id ad7816_id[] = {
> > >  	{ "ad7816", ID_AD7816 },
> > >  	{ "ad7817", ID_AD7817 },
> > > @@ -434,6 +440,7 @@ MODULE_DEVICE_TABLE(spi, ad7816_id);
> > >  static struct spi_driver ad7816_driver = {
> > >  	.driver = {
> > >  		.name = "ad7816",
> > > +		.of_match_table = of_match_ptr(ad7816_of_match),
> > >  	},
> > >  	.probe = ad7816_probe,
> > >  	.id_table = ad7816_id,  
>

Ok. I'll do that.

Thanks for the reivew.

regards,
Nishad

Re: [PATCH v3 4/4] staging: iio: ad7816: Add device tree table.

[Nishad Kamdar]

On Fri, Nov 09, 2018 at 08:11:57AM +0000, Ardelean, Alexandru wrote:
> On Fri, 2018-11-09 at 13:08 +0530, Nishad Kamdar wrote:
> > Add device tree table for matching vendor ID.
> 
> One comment inline for this.
> 
> Thanks
> Alex
> 
> > 
> > Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
> > ---
> >  drivers/staging/iio/adc/ad7816.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/staging/iio/adc/ad7816.c
> > b/drivers/staging/iio/adc/ad7816.c
> > index a2fead85cd46..b8a9149fbac1 100644
> > --- a/drivers/staging/iio/adc/ad7816.c
> > +++ b/drivers/staging/iio/adc/ad7816.c
> > @@ -422,6 +422,12 @@ static int ad7816_probe(struct spi_device *spi_dev)
> >  	return 0;
> >  }
> >  
> > +static const struct of_device_id ad7816_of_match[] = {
> > +	{ .compatible = "adi,ad7816", },
> 
> I think you also need to add:
> +	{ .compatible = "adi,ad7817", },
> +	{ .compatible = "adi,ad7818", },
> 
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ad7816_of_match);
> > +
> >  static const struct spi_device_id ad7816_id[] = {
> >  	{ "ad7816", ID_AD7816 },
> >  	{ "ad7817", ID_AD7817 },
> > @@ -434,6 +440,7 @@ MODULE_DEVICE_TABLE(spi, ad7816_id);
> >  static struct spi_driver ad7816_driver = {
> >  	.driver = {
> >  		.name = "ad7816",
> > +		.of_match_table = of_match_ptr(ad7816_of_match),
> >  	},
> >  	.probe = ad7816_probe,
> >  	.id_table = ad7816_id,

Ok. I'll do that.

Thanks for the review.

regards,
Nishad