staging: iio: ad7816: Switch to the gpio descriptor interface

Quoted 3 lines · Nishad Kamdar

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.

Quoted 21 lines

Signed-off-by Nishad Kamdar <[email protected]>

---
 drivers/staging/iio/adc/ad7816.c | 80 ++++++++++++++------------------
 1 file changed, 34 insertions(+), 46 deletions(-)

drivers/staging/iio/adc/ad7816.c +1 -1

index bf76a8620bdb..12c4e0ab4713 100644
@@ -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>

drivers/staging/iio/adc/ad7816.c line 49

Quoted 9 lines

drivers/staging/iio/adc/ad7816.c +3 -3

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

drivers/staging/iio/adc/ad7816.c static int ad7816_spi_read(struct ad7816_chip_info · line 66

Quoted 12 lines

drivers/staging/iio/adc/ad7816.c +2 -2

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

Quoted 90 lines

drivers/staging/iio/adc/ad7816.c +8 -8

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

drivers/staging/iio/adc/ad7816.c +2 -2

 	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)

drivers/staging/iio/adc/ad7816.c +2 -8

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

drivers/staging/iio/adc/ad7816.c +0 -8

 	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

drivers/staging/iio/adc/ad7816.c static int ad7816_probe(struct spi_device · line 370

Quoted 10 lines

drivers/staging/iio/adc/ad7816.c +6 -1

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

Quoted 34 lines

drivers/staging/iio/adc/ad7816.c +10 -13

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

Add device tree table for matching vendor ID.

Signed-off-by Nishad Kamdar <[email protected]>

---
 drivers/staging/iio/adc/ad7816.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/staging/iio/adc/ad7816.c +0 -0

index a2fead85cd46..b8a9149fbac1 100644

drivers/staging/iio/adc/ad7816.c +5 -0

+	{ }
+};
+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,

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 <[email protected]>

---
 drivers/staging/iio/adc/ad7816.c | 80 ++++++++++++++------------------
 1 file changed, 34 insertions(+), 46 deletions(-)

drivers/staging/iio/adc/ad7816.c +34 -46

index bf76a8620bdb..12c4e0ab4713 100644
@@ -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;

Quoted 4 lines · On Fri, 9 Nov 2018 13:05:17 +0530 Nishad Kamdar <[email protected]> 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 <[email protected]>

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

Thanks,

Jonathan

Quoted 160 lines

---
 drivers/staging/iio/adc/ad7816.c | 80 ++++++++++++++------------------
 1 file changed, 34 insertions(+), 46 deletions(-)

drivers/staging/iio/adc/ad7816.c +34 -46

index bf76a8620bdb..12c4e0ab4713 100644
@@ -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;  

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

Signed-off-by Nishad Kamdar <[email protected]>

---
 drivers/staging/iio/adc/ad7816.c | 35 ++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 11 deletions(-)

drivers/staging/iio/adc/ad7816.c +24 -11

index 12c4e0ab4713..3cda5cd09365 100644
@@ -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 },
 	{}
 };
 

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 <[email protected]>

---
 drivers/staging/iio/adc/ad7816.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/staging/iio/adc/ad7816.c +3 -2

index 3cda5cd09365..a2fead85cd46 100644
@@ -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",

Quoted 1 line · Nishad Kamdar

Add device tree table for matching vendor ID.

One comment inline for this.

Thanks
Alex

Quoted 12 lines

Signed-off-by Nishad Kamdar <[email protected]>

---
 drivers/staging/iio/adc/ad7816.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/staging/iio/adc/ad7816.c +0 -0

index a2fead85cd46..b8a9149fbac1 100644

drivers/staging/iio/adc/ad7816.c static int ad7816_probe(struct spi_device *spi_dev) · line 426

Quoted 5 lines

drivers/staging/iio/adc/ad7816.c +2 -0

@@ -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:

Quoted 15 lines

drivers/staging/iio/adc/ad7816.c +5 -0

+	{ }
+};
+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,  

Quoted 1 line · Nishad Kamdar

Add device tree table for matching vendor ID.

One comment inline for this.

Thanks
Alex

Quoted 12 lines

Signed-off-by Nishad Kamdar <[email protected]>

---
 drivers/staging/iio/adc/ad7816.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/staging/iio/adc/ad7816.c +0 -0

index a2fead85cd46..b8a9149fbac1 100644

drivers/staging/iio/adc/ad7816.c static int ad7816_probe(struct spi_device *spi_dev) · line 426

Quoted 5 lines

drivers/staging/iio/adc/ad7816.c +2 -0

@@ -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:

(quoted diff) +2 -0

+	{ .compatible = "adi,ad7817", },
+	{ .compatible = "adi,ad7818", },

Quoted 8 lines

drivers/staging/iio/adc/ad7816.c +4 -0

+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad7816_of_match);
+
 static const struct spi_device_id ad7816_id[] = {
 	{ "ad7816", ID_AD7816 },
 	{ "ad7817", ID_AD7817 },

drivers/staging/iio/adc/ad7816.c MODULE_DEVICE_TABLE(spi, ad7816_id); · line 446

Quoted 7 lines

drivers/staging/iio/adc/ad7816.c +1 -0

@@ -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,

Quoted 28 lines · Alexandru

Quoted 1 line · Nishad Kamdar

Add device tree table for matching vendor ID.

One comment inline for this.

Thanks
Alex

Quoted 12 lines

Signed-off-by Nishad Kamdar <[email protected]>

---
 drivers/staging/iio/adc/ad7816.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/staging/iio/adc/ad7816.c +0 -0

index a2fead85cd46..b8a9149fbac1 100644

drivers/staging/iio/adc/ad7816.c static int ad7816_probe(struct spi_device *spi_dev) · line 426

Quoted 5 lines

drivers/staging/iio/adc/ad7816.c +2 -0

@@ -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:

Review

Quoted 2 lines

(quoted diff) +2 -0

+	{ .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

Quoted 7 lines

Quoted 7 lines

drivers/staging/iio/adc/ad7816.c +4 -0

+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad7816_of_match);
+
 static const struct spi_device_id ad7816_id[] = {
 	{ "ad7816", ID_AD7816 },
 	{ "ad7817", ID_AD7817 },

Quoted 8 lines

drivers/staging/iio/adc/ad7816.c MODULE_DEVICE_TABLE(spi, ad7816_id); · line 446

Quoted 8 lines

drivers/staging/iio/adc/ad7816.c +1 -0

@@ -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,  

Quoted 6 lines · On Sun, 11 Nov 2018 12:24:05 +0000 Jonathan Cameron <[email protected]> wrote:

Quoted 3 lines · On Fri, 9 Nov 2018 13:05:17 +0530 Nishad Kamdar <[email protected]> wrote:

Use the gpiod interface for rdwr_pin, convert_pin and busy_pin instead of the deprecated old non-descriptor interface.

Quoted 3 lines

Quoted 1 line

Signed-off-by Nishad Kamdar <[email protected]>

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

Quoted 167 lines

Thanks,

Jonathan

Quoted 160 lines

---
 drivers/staging/iio/adc/ad7816.c | 80 ++++++++++++++------------------
 1 file changed, 34 insertions(+), 46 deletions(-)

drivers/staging/iio/adc/ad7816.c +34 -46

index bf76a8620bdb..12c4e0ab4713 100644
@@ -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;