[PATCH v2] hwmon/sht15: Root out platform data

[PATCH v2] hwmon/sht15: Root out platform data

[Linus Walleij]

After finding out there are active users of this sensor I noticed:

- It has a single PXA27x board file using the platform data
- The platform data is only used to carry two GPIO pins, all other
  fields are unused
- The driver does not use GPIO descriptors but the legacy GPIO
  API

I saw we can swiftly fix this by:

- Killing off the platform data entirely
- Define a GPIO descriptor lookup table in the board file
- Use the standard devm_gpiod_get() to grab the GPIO descriptors
  from either the device tree or the board file table.

This compiles, but needs testing.

Cc: arm@kernel.org
Cc: Marco Franchi <marco.franchi@nxp.com>
Cc: Davide Hug <d@videhug.ch>
Cc: Jonathan Cameron <jic23@cam.ac.uk>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Fix the return value from devm_gpiod_get() with PTR_ERR()
  so we defer etc correctly.

ARM SoC folks: please ACK this so the HWMON maintainer can merge
it when it is in reasonable shape.

Marco: can you test this patch with your setup?

Davide: can you test this patch with your setup?

Jonathan: I gues you may feel this sensor needs migrating to IIO or
so, like the Imote and Stargate2 needs migrating to device tree,
but this helps a bit with that.
---
 Documentation/hwmon/sht15           |   3 +-
 arch/arm/mach-pxa/stargate2.c       |  17 ++--
 drivers/hwmon/sht15.c               | 167 ++++++++++++------------------------
 include/linux/platform_data/sht15.h |  38 --------
 4 files changed, 65 insertions(+), 160 deletions(-)
 delete mode 100644 include/linux/platform_data/sht15.h

diff --git a/Documentation/hwmon/sht15 b/Documentation/hwmon/sht15
index 778987d1856f..5e3207c3b177 100644
--- a/Documentation/hwmon/sht15
+++ b/Documentation/hwmon/sht15
@@ -42,8 +42,7 @@ chip. These coefficients are used to internally calibrate the signals from the
 sensors. Disabling the reload of those coefficients allows saving 10ms for each
 measurement and decrease power consumption, while losing on precision.
 
-Some options may be set directly in the sht15_platform_data structure
-or via sysfs attributes.
+Some options may be set via sysfs attributes.
 
 Notes:
   * The regulator supply name is set to "vcc".
diff --git a/arch/arm/mach-pxa/stargate2.c b/arch/arm/mach-pxa/stargate2.c
index 2d45d18b1a5e..6b7df6fd2448 100644
--- a/arch/arm/mach-pxa/stargate2.c
+++ b/arch/arm/mach-pxa/stargate2.c
@@ -29,6 +29,7 @@
 #include <linux/platform_data/pcf857x.h>
 #include <linux/platform_data/at24.h>
 #include <linux/smc91x.h>
+#include <linux/gpio/machine.h>
 #include <linux/gpio.h>
 #include <linux/leds.h>
 
@@ -52,7 +53,6 @@
 #include <linux/spi/spi.h>
 #include <linux/spi/pxa2xx_spi.h>
 #include <linux/mfd/da903x.h>
-#include <linux/platform_data/sht15.h>
 
 #include "devices.h"
 #include "generic.h"
@@ -137,17 +137,18 @@ static unsigned long sg2_im2_unified_pin_config[] __initdata = {
 	GPIO10_GPIO, /* large basic connector pin 23 */
 };
 
-static struct sht15_platform_data platform_data_sht15 = {
-	.gpio_data =  100,
-	.gpio_sck  =  98,
+static struct gpiod_lookup_table sht15_gpiod_table = {
+	.dev_id = "sht15",
+	.table = {
+		/* FIXME: should this have |GPIO_OPEN_DRAIN set? */
+		GPIO_LOOKUP("gpio-pxa", 100, "data", GPIO_ACTIVE_HIGH),
+		GPIO_LOOKUP("gpio-pxa", 98, "clk", GPIO_ACTIVE_HIGH),
+	},
 };
 
 static struct platform_device sht15 = {
 	.name = "sht15",
 	.id = -1,
-	.dev = {
-		.platform_data = &platform_data_sht15,
-	},
 };
 
 static struct regulator_consumer_supply stargate2_sensor_3_con[] = {
@@ -608,6 +609,7 @@ static void __init imote2_init(void)
 
 	imote2_stargate2_init();
 
+	gpiod_add_lookup_table(&sht15_gpiod_table);
 	platform_add_devices(imote2_devices, ARRAY_SIZE(imote2_devices));
 
 	i2c_register_board_info(0, imote2_i2c_board_info,
@@ -988,6 +990,7 @@ static void __init stargate2_init(void)
 
 	imote2_stargate2_init();
 
+	gpiod_add_lookup_table(&sht15_gpiod_table);
 	platform_add_devices(ARRAY_AND_SIZE(stargate2_devices));
 
 	i2c_register_board_info(0, ARRAY_AND_SIZE(stargate2_i2c_board_info));
diff --git a/drivers/hwmon/sht15.c b/drivers/hwmon/sht15.c
index e4d642b673c6..0e3e5f83f5cf 100644
--- a/drivers/hwmon/sht15.c
+++ b/drivers/hwmon/sht15.c
@@ -18,13 +18,11 @@
 
 #include <linux/interrupt.h>
 #include <linux/irq.h>
-#include <linux/gpio.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
 #include <linux/mutex.h>
-#include <linux/platform_data/sht15.h>
 #include <linux/platform_device.h>
 #include <linux/sched.h>
 #include <linux/delay.h>
@@ -34,7 +32,8 @@
 #include <linux/slab.h>
 #include <linux/atomic.h>
 #include <linux/bitrev.h>
-#include <linux/of_gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/of.h>
 
 /* Commands */
 #define SHT15_MEASURE_TEMP		0x03
@@ -122,7 +121,8 @@ static const u8 sht15_crc8_table[] = {
 
 /**
  * struct sht15_data - device instance specific data
- * @pdata:		platform data (gpio's etc).
+ * @sck:		clock GPIO line
+ * @data:		data GPIO line
  * @read_work:		bh of interrupt handler.
  * @wait_queue:		wait queue for getting values from device.
  * @val_temp:		last temperature value read from device.
@@ -150,7 +150,8 @@ static const u8 sht15_crc8_table[] = {
  * @interrupt_handled:	flag used to indicate a handler has been scheduled.
  */
 struct sht15_data {
-	struct sht15_platform_data	*pdata;
+	struct gpio_desc		*sck;
+	struct gpio_desc		*data;
 	struct work_struct		read_work;
 	wait_queue_head_t		wait_queue;
 	uint16_t			val_temp;
@@ -205,16 +206,16 @@ static int sht15_connection_reset(struct sht15_data *data)
 {
 	int i, err;
 
-	err = gpio_direction_output(data->pdata->gpio_data, 1);
+	err = gpiod_direction_output(data->data, 1);
 	if (err)
 		return err;
 	ndelay(SHT15_TSCKL);
-	gpio_set_value(data->pdata->gpio_sck, 0);
+	gpiod_set_value(data->sck, 0);
 	ndelay(SHT15_TSCKL);
 	for (i = 0; i < 9; ++i) {
-		gpio_set_value(data->pdata->gpio_sck, 1);
+		gpiod_set_value(data->sck, 1);
 		ndelay(SHT15_TSCKH);
-		gpio_set_value(data->pdata->gpio_sck, 0);
+		gpiod_set_value(data->sck, 0);
 		ndelay(SHT15_TSCKL);
 	}
 	return 0;
@@ -227,11 +228,11 @@ static int sht15_connection_reset(struct sht15_data *data)
  */
 static inline void sht15_send_bit(struct sht15_data *data, int val)
 {
-	gpio_set_value(data->pdata->gpio_data, val);
+	gpiod_set_value(data->data, val);
 	ndelay(SHT15_TSU);
-	gpio_set_value(data->pdata->gpio_sck, 1);
+	gpiod_set_value(data->sck, 1);
 	ndelay(SHT15_TSCKH);
-	gpio_set_value(data->pdata->gpio_sck, 0);
+	gpiod_set_value(data->sck, 0);
 	ndelay(SHT15_TSCKL); /* clock low time */
 }
 
@@ -248,23 +249,23 @@ static int sht15_transmission_start(struct sht15_data *data)
 	int err;
 
 	/* ensure data is high and output */
-	err = gpio_direction_output(data->pdata->gpio_data, 1);
+	err = gpiod_direction_output(data->data, 1);
 	if (err)
 		return err;
 	ndelay(SHT15_TSU);
-	gpio_set_value(data->pdata->gpio_sck, 0);
+	gpiod_set_value(data->sck, 0);
 	ndelay(SHT15_TSCKL);
-	gpio_set_value(data->pdata->gpio_sck, 1);
+	gpiod_set_value(data->sck, 1);
 	ndelay(SHT15_TSCKH);
-	gpio_set_value(data->pdata->gpio_data, 0);
+	gpiod_set_value(data->data, 0);
 	ndelay(SHT15_TSU);
-	gpio_set_value(data->pdata->gpio_sck, 0);
+	gpiod_set_value(data->sck, 0);
 	ndelay(SHT15_TSCKL);
-	gpio_set_value(data->pdata->gpio_sck, 1);
+	gpiod_set_value(data->sck, 1);
 	ndelay(SHT15_TSCKH);
-	gpio_set_value(data->pdata->gpio_data, 1);
+	gpiod_set_value(data->data, 1);
 	ndelay(SHT15_TSU);
-	gpio_set_value(data->pdata->gpio_sck, 0);
+	gpiod_set_value(data->sck, 0);
 	ndelay(SHT15_TSCKL);
 	return 0;
 }
@@ -292,20 +293,20 @@ static int sht15_wait_for_response(struct sht15_data *data)
 {
 	int err;
 
-	err = gpio_direction_input(data->pdata->gpio_data);
+	err = gpiod_direction_input(data->data);
 	if (err)
 		return err;
-	gpio_set_value(data->pdata->gpio_sck, 1);
+	gpiod_set_value(data->sck, 1);
 	ndelay(SHT15_TSCKH);
-	if (gpio_get_value(data->pdata->gpio_data)) {
-		gpio_set_value(data->pdata->gpio_sck, 0);
+	if (gpiod_get_value(data->data)) {
+		gpiod_set_value(data->sck, 0);
 		dev_err(data->dev, "Command not acknowledged\n");
 		err = sht15_connection_reset(data);
 		if (err)
 			return err;
 		return -EIO;
 	}
-	gpio_set_value(data->pdata->gpio_sck, 0);
+	gpiod_set_value(data->sck, 0);
 	ndelay(SHT15_TSCKL);
 	return 0;
 }
@@ -360,17 +361,17 @@ static int sht15_ack(struct sht15_data *data)
 {
 	int err;
 
-	err = gpio_direction_output(data->pdata->gpio_data, 0);
+	err = gpiod_direction_output(data->data, 0);
 	if (err)
 		return err;
 	ndelay(SHT15_TSU);
-	gpio_set_value(data->pdata->gpio_sck, 1);
+	gpiod_set_value(data->sck, 1);
 	ndelay(SHT15_TSU);
-	gpio_set_value(data->pdata->gpio_sck, 0);
+	gpiod_set_value(data->sck, 0);
 	ndelay(SHT15_TSU);
-	gpio_set_value(data->pdata->gpio_data, 1);
+	gpiod_set_value(data->data, 1);
 
-	return gpio_direction_input(data->pdata->gpio_data);
+	return gpiod_direction_input(data->data);
 }
 
 /**
@@ -383,13 +384,13 @@ static int sht15_end_transmission(struct sht15_data *data)
 {
 	int err;
 
-	err = gpio_direction_output(data->pdata->gpio_data, 1);
+	err = gpiod_direction_output(data->data, 1);
 	if (err)
 		return err;
 	ndelay(SHT15_TSU);
-	gpio_set_value(data->pdata->gpio_sck, 1);
+	gpiod_set_value(data->sck, 1);
 	ndelay(SHT15_TSCKH);
-	gpio_set_value(data->pdata->gpio_sck, 0);
+	gpiod_set_value(data->sck, 0);
 	ndelay(SHT15_TSCKL);
 	return 0;
 }
@@ -405,10 +406,10 @@ static u8 sht15_read_byte(struct sht15_data *data)
 
 	for (i = 0; i < 8; ++i) {
 		byte <<= 1;
-		gpio_set_value(data->pdata->gpio_sck, 1);
+		gpiod_set_value(data->sck, 1);
 		ndelay(SHT15_TSCKH);
-		byte |= !!gpio_get_value(data->pdata->gpio_data);
-		gpio_set_value(data->pdata->gpio_sck, 0);
+		byte |= !!gpiod_get_value(data->data);
+		gpiod_set_value(data->sck, 0);
 		ndelay(SHT15_TSCKL);
 	}
 	return byte;
@@ -428,7 +429,7 @@ static int sht15_send_status(struct sht15_data *data, u8 status)
 	err = sht15_send_cmd(data, SHT15_WRITE_STATUS);
 	if (err)
 		return err;
-	err = gpio_direction_output(data->pdata->gpio_data, 1);
+	err = gpiod_direction_output(data->data, 1);
 	if (err)
 		return err;
 	ndelay(SHT15_TSU);
@@ -528,14 +529,14 @@ static int sht15_measurement(struct sht15_data *data,
 	if (ret)
 		return ret;
 
-	ret = gpio_direction_input(data->pdata->gpio_data);
+	ret = gpiod_direction_input(data->data);
 	if (ret)
 		return ret;
 	atomic_set(&data->interrupt_handled, 0);
 
-	enable_irq(gpio_to_irq(data->pdata->gpio_data));
-	if (gpio_get_value(data->pdata->gpio_data) == 0) {
-		disable_irq_nosync(gpio_to_irq(data->pdata->gpio_data));
+	enable_irq(gpiod_to_irq(data->data));
+	if (gpiod_get_value(data->data) == 0) {
+		disable_irq_nosync(gpiod_to_irq(data->data));
 		/* Only relevant if the interrupt hasn't occurred. */
 		if (!atomic_read(&data->interrupt_handled))
 			schedule_work(&data->read_work);
@@ -547,7 +548,7 @@ static int sht15_measurement(struct sht15_data *data,
 		data->state = SHT15_READING_NOTHING;
 		return -EIO;
 	} else if (ret == 0) { /* timeout occurred */
-		disable_irq_nosync(gpio_to_irq(data->pdata->gpio_data));
+		disable_irq_nosync(gpiod_to_irq(data->data));
 		ret = sht15_connection_reset(data);
 		if (ret)
 			return ret;
@@ -826,15 +827,15 @@ static void sht15_bh_read_data(struct work_struct *work_s)
 			       read_work);
 
 	/* Firstly, verify the line is low */
-	if (gpio_get_value(data->pdata->gpio_data)) {
+	if (gpiod_get_value(data->data)) {
 		/*
 		 * If not, then start the interrupt again - care here as could
 		 * have gone low in meantime so verify it hasn't!
 		 */
 		atomic_set(&data->interrupt_handled, 0);
-		enable_irq(gpio_to_irq(data->pdata->gpio_data));
+		enable_irq(gpiod_to_irq(data->data));
 		/* If still not occurred or another handler was scheduled */
-		if (gpio_get_value(data->pdata->gpio_data)
+		if (gpiod_get_value(data->data)
 		    || atomic_read(&data->interrupt_handled))
 			return;
 	}
@@ -918,46 +919,6 @@ static const struct of_device_id sht15_dt_match[] = {
 	{ },
 };
 MODULE_DEVICE_TABLE(of, sht15_dt_match);
-
-/*
- * This function returns NULL if pdev isn't a device instatiated by dt,
- * a pointer to pdata if it could successfully get all information
- * from dt or a negative ERR_PTR() on error.
- */
-static struct sht15_platform_data *sht15_probe_dt(struct device *dev)
-{
-	struct device_node *np = dev->of_node;
-	struct sht15_platform_data *pdata;
-
-	/* no device tree device */
-	if (!np)
-		return NULL;
-
-	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
-	if (!pdata)
-		return ERR_PTR(-ENOMEM);
-
-	pdata->gpio_data = of_get_named_gpio(np, "data-gpios", 0);
-	if (pdata->gpio_data < 0) {
-		if (pdata->gpio_data != -EPROBE_DEFER)
-			dev_err(dev, "data-gpios not found\n");
-		return ERR_PTR(pdata->gpio_data);
-	}
-
-	pdata->gpio_sck = of_get_named_gpio(np, "clk-gpios", 0);
-	if (pdata->gpio_sck < 0) {
-		if (pdata->gpio_sck != -EPROBE_DEFER)
-			dev_err(dev, "clk-gpios not found\n");
-		return ERR_PTR(pdata->gpio_sck);
-	}
-
-	return pdata;
-}
-#else
-static inline struct sht15_platform_data *sht15_probe_dt(struct device *dev)
-{
-	return NULL;
-}
 #endif
 
 static int sht15_probe(struct platform_device *pdev)
@@ -977,25 +938,6 @@ static int sht15_probe(struct platform_device *pdev)
 	data->dev = &pdev->dev;
 	init_waitqueue_head(&data->wait_queue);
 
-	data->pdata = sht15_probe_dt(&pdev->dev);
-	if (IS_ERR(data->pdata))
-		return PTR_ERR(data->pdata);
-	if (data->pdata == NULL) {
-		data->pdata = dev_get_platdata(&pdev->dev);
-		if (data->pdata == NULL) {
-			dev_err(&pdev->dev, "no platform data supplied\n");
-			return -EINVAL;
-		}
-	}
-
-	data->supply_uv = data->pdata->supply_mv * 1000;
-	if (data->pdata->checksum)
-		data->checksumming = true;
-	if (data->pdata->no_otp_reload)
-		status |= SHT15_STATUS_NO_OTP_RELOAD;
-	if (data->pdata->low_resolution)
-		status |= SHT15_STATUS_LOW_RESOLUTION;
-
 	/*
 	 * If a regulator is available,
 	 * query what the supply voltage actually is!
@@ -1030,21 +972,20 @@ static int sht15_probe(struct platform_device *pdev)
 	}
 
 	/* Try requesting the GPIOs */
-	ret = devm_gpio_request_one(&pdev->dev, data->pdata->gpio_sck,
-			GPIOF_OUT_INIT_LOW, "SHT15 sck");
-	if (ret) {
+	data->sck = devm_gpiod_get(&pdev->dev, "clk", GPIOD_OUT_LOW);
+	if (IS_ERR(data->sck)) {
+		ret = PTR_ERR(data->sck);
 		dev_err(&pdev->dev, "clock line GPIO request failed\n");
 		goto err_release_reg;
 	}
-
-	ret = devm_gpio_request(&pdev->dev, data->pdata->gpio_data,
-				"SHT15 data");
-	if (ret) {
+	data->data = devm_gpiod_get(&pdev->dev, "data", GPIOD_IN);
+	if (IS_ERR(data->data)) {
+		ret = PTR_ERR(data->data);
 		dev_err(&pdev->dev, "data line GPIO request failed\n");
 		goto err_release_reg;
 	}
 
-	ret = devm_request_irq(&pdev->dev, gpio_to_irq(data->pdata->gpio_data),
+	ret = devm_request_irq(&pdev->dev, gpiod_to_irq(data->data),
 			       sht15_interrupt_fired,
 			       IRQF_TRIGGER_FALLING,
 			       "sht15 data",
@@ -1053,7 +994,7 @@ static int sht15_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "failed to get irq for data line\n");
 		goto err_release_reg;
 	}
-	disable_irq_nosync(gpio_to_irq(data->pdata->gpio_data));
+	disable_irq_nosync(gpiod_to_irq(data->data));
 	ret = sht15_connection_reset(data);
 	if (ret)
 		goto err_release_reg;
diff --git a/include/linux/platform_data/sht15.h b/include/linux/platform_data/sht15.h
deleted file mode 100644
index 12289c1e9413..000000000000
--- a/include/linux/platform_data/sht15.h
+++ /dev/null
@@ -1,38 +0,0 @@
-/*
- * sht15.h - support for the SHT15 Temperature and Humidity Sensor
- *
- * Copyright (c) 2009 Jonathan Cameron
- *
- * Copyright (c) 2007 Wouter Horre
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * For further information, see the Documentation/hwmon/sht15 file.
- */
-
-#ifndef _PDATA_SHT15_H
-#define _PDATA_SHT15_H
-
-/**
- * struct sht15_platform_data - sht15 connectivity info
- * @gpio_data:		no. of gpio to which bidirectional data line is
- *			connected.
- * @gpio_sck:		no. of gpio to which the data clock is connected.
- * @supply_mv:		supply voltage in mv. Overridden by regulator if
- *			available.
- * @checksum:		flag to indicate the checksum should be validated.
- * @no_otp_reload:	flag to indicate no reload from OTP.
- * @low_resolution:	flag to indicate the temp/humidity resolution to use.
- */
-struct sht15_platform_data {
-	int gpio_data;
-	int gpio_sck;
-	int supply_mv;
-	bool checksum;
-	bool no_otp_reload;
-	bool low_resolution;
-};
-
-#endif /* _PDATA_SHT15_H */
-- 
2.13.5

Re: [PATCH v2] hwmon/sht15: Root out platform data

[Arnd Bergmann]

On Sun, Sep 10, 2017 at 11:44 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> After finding out there are active users of this sensor I noticed:
>
> - It has a single PXA27x board file using the platform data
> - The platform data is only used to carry two GPIO pins, all other
>   fields are unused
> - The driver does not use GPIO descriptors but the legacy GPIO
>   API
>
> I saw we can swiftly fix this by:
>
> - Killing off the platform data entirely
> - Define a GPIO descriptor lookup table in the board file
> - Use the standard devm_gpiod_get() to grab the GPIO descriptors
>   from either the device tree or the board file table.
>
> This compiles, but needs testing.
>
> Cc: arm@kernel.org
> Cc: Marco Franchi <marco.franchi@nxp.com>
> Cc: Davide Hug <d@videhug.ch>
> Cc: Jonathan Cameron <jic23@cam.ac.uk>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Fix the return value from devm_gpiod_get() with PTR_ERR()
>   so we defer etc correctly.
>
> ARM SoC folks: please ACK this so the HWMON maintainer can merge
> it when it is in reasonable shape.
>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH v2] hwmon/sht15: Root out platform data

[Davide Hug]

On Sun, 10 Sep 2017 11:44:46 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> After finding out there are active users of this sensor I noticed:

I would like to point out that I'm doing this for learning purposes and that I
don't have any pressing needs for this driver to work. But of course I'm really
grateful for all the replays!

> Cc: arm@kernel.org
> Cc: Marco Franchi <marco.franchi@nxp.com>
> Cc: Davide Hug <d@videhug.ch>
> Cc: Jonathan Cameron <jic23@cam.ac.uk>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Fix the return value from devm_gpiod_get() with PTR_ERR()
>   so we defer etc correctly.
> 
> ARM SoC folks: please ACK this so the HWMON maintainer can merge
> it when it is in reasonable shape.
> 
> Marco: can you test this patch with your setup?
> 
> Davide: can you test this patch with your setup?

I tested it and I can make it work (as the current driver) by modifying gpiolib
to not test for the FLAG_USED_AS_IRQ in _gpiod_direction_output_raw.
Otherwise again I get:

	gpio-48 (SHT15 data): _gpiod_direction_output_raw: tried to set a GPIO tied to an IRQ as output

as with the current driver. But I guess this is as expected.
(I did this on a beaglebone black with kernel 4.9.10. I'll compile a newer
kernel and try it there to.)

Thanks!
Davide

Re: [PATCH v2] hwmon/sht15: Root out platform data

[Jonathan Cameron]

On Sun, 10 Sep 2017 11:44:46 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> After finding out there are active users of this sensor I noticed:
> 
> - It has a single PXA27x board file using the platform data
> - The platform data is only used to carry two GPIO pins, all other
>   fields are unused
> - The driver does not use GPIO descriptors but the legacy GPIO
>   API
> 
> I saw we can swiftly fix this by:
> 
> - Killing off the platform data entirely
> - Define a GPIO descriptor lookup table in the board file
> - Use the standard devm_gpiod_get() to grab the GPIO descriptors
>   from either the device tree or the board file table.
> 
> This compiles, but needs testing.
> 
> Cc: arm@kernel.org
> Cc: Marco Franchi <marco.franchi@nxp.com>
> Cc: Davide Hug <d@videhug.ch>
> Cc: Jonathan Cameron <jic23@cam.ac.uk>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Hmm. Firing up that board isn't something I'm going to manage for a few
weeks at least.   This looks safe though and as I suspect I'm the only
person who still has working boards running recent kernels I doubt
anyone else will notice :)

Perhaps give me a month and if I seem to have forgotten it ping
me again.  Or if someone else gives a tested by in the meantime
for another platform then I'm happy to assume the trivial bits
for the stargate2 and imote2 will work fine.

Jonathan

> ---
> ChangeLog v1->v2:
> - Fix the return value from devm_gpiod_get() with PTR_ERR()
>   so we defer etc correctly.
> 
> ARM SoC folks: please ACK this so the HWMON maintainer can merge
> it when it is in reasonable shape.
> 
> Marco: can you test this patch with your setup?
> 
> Davide: can you test this patch with your setup?
> 
> Jonathan: I gues you may feel this sensor needs migrating to IIO or
> so, like the Imote and Stargate2 needs migrating to device tree,
> but this helps a bit with that.

It was my fault it went into hwmon in the first place ;)

Humidity sensors have always occupied one of those odd corners.
They are actually present in some servers (or so Guenter once
once told me).  They are happy keeping them in hwmon and I'm not
that fussed about moving them until someone has a use case that
really needs it.  Humidity doesn't change quickly so sysfs alone
is fine.

Hmm. Device tree on these boards has always been on the todo list, but
I'm afraid it's a fair way down it.  It'll be 'interesting' as they
only uboot patches there ever were disappeared years ago and
we have a limited space to get the kernel in.  Not sure what other
fun I'll hit on that.  Also not sure I actually have cards to
test some of the ports any more so there will be a fair bit
of untested code in any port.

Jonathan

p.s. I'll send patches for my email address when I get a bored moment.
Could have sworn I did 5 years ago or more, but maybe it forgot.


> ---
>  Documentation/hwmon/sht15           |   3 +-
>  arch/arm/mach-pxa/stargate2.c       |  17 ++--
>  drivers/hwmon/sht15.c               | 167 ++++++++++++------------------------
>  include/linux/platform_data/sht15.h |  38 --------
>  4 files changed, 65 insertions(+), 160 deletions(-)
>  delete mode 100644 include/linux/platform_data/sht15.h
> 
> diff --git a/Documentation/hwmon/sht15 b/Documentation/hwmon/sht15
> index 778987d1856f..5e3207c3b177 100644
> --- a/Documentation/hwmon/sht15
> +++ b/Documentation/hwmon/sht15
> @@ -42,8 +42,7 @@ chip. These coefficients are used to internally calibrate the signals from the
>  sensors. Disabling the reload of those coefficients allows saving 10ms for each
>  measurement and decrease power consumption, while losing on precision.
>  
> -Some options may be set directly in the sht15_platform_data structure
> -or via sysfs attributes.
> +Some options may be set via sysfs attributes.
>  
>  Notes:
>    * The regulator supply name is set to "vcc".
> diff --git a/arch/arm/mach-pxa/stargate2.c b/arch/arm/mach-pxa/stargate2.c
> index 2d45d18b1a5e..6b7df6fd2448 100644
> --- a/arch/arm/mach-pxa/stargate2.c
> +++ b/arch/arm/mach-pxa/stargate2.c
> @@ -29,6 +29,7 @@
>  #include <linux/platform_data/pcf857x.h>
>  #include <linux/platform_data/at24.h>
>  #include <linux/smc91x.h>
> +#include <linux/gpio/machine.h>
>  #include <linux/gpio.h>
>  #include <linux/leds.h>
>  
> @@ -52,7 +53,6 @@
>  #include <linux/spi/spi.h>
>  #include <linux/spi/pxa2xx_spi.h>
>  #include <linux/mfd/da903x.h>
> -#include <linux/platform_data/sht15.h>
>  
>  #include "devices.h"
>  #include "generic.h"
> @@ -137,17 +137,18 @@ static unsigned long sg2_im2_unified_pin_config[] __initdata = {
>  	GPIO10_GPIO, /* large basic connector pin 23 */
>  };
>  
> -static struct sht15_platform_data platform_data_sht15 = {
> -	.gpio_data =  100,
> -	.gpio_sck  =  98,
> +static struct gpiod_lookup_table sht15_gpiod_table = {
> +	.dev_id = "sht15",
> +	.table = {
> +		/* FIXME: should this have |GPIO_OPEN_DRAIN set? */
> +		GPIO_LOOKUP("gpio-pxa", 100, "data", GPIO_ACTIVE_HIGH),
> +		GPIO_LOOKUP("gpio-pxa", 98, "clk", GPIO_ACTIVE_HIGH),
> +	},
>  };
>  
>  static struct platform_device sht15 = {
>  	.name = "sht15",
>  	.id = -1,
> -	.dev = {
> -		.platform_data = &platform_data_sht15,
> -	},
>  };
>  
>  static struct regulator_consumer_supply stargate2_sensor_3_con[] = {
> @@ -608,6 +609,7 @@ static void __init imote2_init(void)
>  
>  	imote2_stargate2_init();
>  
> +	gpiod_add_lookup_table(&sht15_gpiod_table);
>  	platform_add_devices(imote2_devices, ARRAY_SIZE(imote2_devices));
>  
>  	i2c_register_board_info(0, imote2_i2c_board_info,
> @@ -988,6 +990,7 @@ static void __init stargate2_init(void)
>  
>  	imote2_stargate2_init();
>  
> +	gpiod_add_lookup_table(&sht15_gpiod_table);
>  	platform_add_devices(ARRAY_AND_SIZE(stargate2_devices));
>  
>  	i2c_register_board_info(0, ARRAY_AND_SIZE(stargate2_i2c_board_info));
> diff --git a/drivers/hwmon/sht15.c b/drivers/hwmon/sht15.c
> index e4d642b673c6..0e3e5f83f5cf 100644
> --- a/drivers/hwmon/sht15.c
> +++ b/drivers/hwmon/sht15.c
> @@ -18,13 +18,11 @@
>  
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
> -#include <linux/gpio.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/hwmon.h>
>  #include <linux/hwmon-sysfs.h>
>  #include <linux/mutex.h>
> -#include <linux/platform_data/sht15.h>
>  #include <linux/platform_device.h>
>  #include <linux/sched.h>
>  #include <linux/delay.h>
> @@ -34,7 +32,8 @@
>  #include <linux/slab.h>
>  #include <linux/atomic.h>
>  #include <linux/bitrev.h>
> -#include <linux/of_gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/of.h>
>  
>  /* Commands */
>  #define SHT15_MEASURE_TEMP		0x03
> @@ -122,7 +121,8 @@ static const u8 sht15_crc8_table[] = {
>  
>  /**
>   * struct sht15_data - device instance specific data
> - * @pdata:		platform data (gpio's etc).
> + * @sck:		clock GPIO line
> + * @data:		data GPIO line
>   * @read_work:		bh of interrupt handler.
>   * @wait_queue:		wait queue for getting values from device.
>   * @val_temp:		last temperature value read from device.
> @@ -150,7 +150,8 @@ static const u8 sht15_crc8_table[] = {
>   * @interrupt_handled:	flag used to indicate a handler has been scheduled.
>   */
>  struct sht15_data {
> -	struct sht15_platform_data	*pdata;
> +	struct gpio_desc		*sck;
> +	struct gpio_desc		*data;
>  	struct work_struct		read_work;
>  	wait_queue_head_t		wait_queue;
>  	uint16_t			val_temp;
> @@ -205,16 +206,16 @@ static int sht15_connection_reset(struct sht15_data *data)
>  {
>  	int i, err;
>  
> -	err = gpio_direction_output(data->pdata->gpio_data, 1);
> +	err = gpiod_direction_output(data->data, 1);
>  	if (err)
>  		return err;
>  	ndelay(SHT15_TSCKL);
> -	gpio_set_value(data->pdata->gpio_sck, 0);
> +	gpiod_set_value(data->sck, 0);
>  	ndelay(SHT15_TSCKL);
>  	for (i = 0; i < 9; ++i) {
> -		gpio_set_value(data->pdata->gpio_sck, 1);
> +		gpiod_set_value(data->sck, 1);
>  		ndelay(SHT15_TSCKH);
> -		gpio_set_value(data->pdata->gpio_sck, 0);
> +		gpiod_set_value(data->sck, 0);
>  		ndelay(SHT15_TSCKL);
>  	}
>  	return 0;
> @@ -227,11 +228,11 @@ static int sht15_connection_reset(struct sht15_data *data)
>   */
>  static inline void sht15_send_bit(struct sht15_data *data, int val)
>  {
> -	gpio_set_value(data->pdata->gpio_data, val);
> +	gpiod_set_value(data->data, val);
>  	ndelay(SHT15_TSU);
> -	gpio_set_value(data->pdata->gpio_sck, 1);
> +	gpiod_set_value(data->sck, 1);
>  	ndelay(SHT15_TSCKH);
> -	gpio_set_value(data->pdata->gpio_sck, 0);
> +	gpiod_set_value(data->sck, 0);
>  	ndelay(SHT15_TSCKL); /* clock low time */
>  }
>  
> @@ -248,23 +249,23 @@ static int sht15_transmission_start(struct sht15_data *data)
>  	int err;
>  
>  	/* ensure data is high and output */
> -	err = gpio_direction_output(data->pdata->gpio_data, 1);
> +	err = gpiod_direction_output(data->data, 1);
>  	if (err)
>  		return err;
>  	ndelay(SHT15_TSU);
> -	gpio_set_value(data->pdata->gpio_sck, 0);
> +	gpiod_set_value(data->sck, 0);
>  	ndelay(SHT15_TSCKL);
> -	gpio_set_value(data->pdata->gpio_sck, 1);
> +	gpiod_set_value(data->sck, 1);
>  	ndelay(SHT15_TSCKH);
> -	gpio_set_value(data->pdata->gpio_data, 0);
> +	gpiod_set_value(data->data, 0);
>  	ndelay(SHT15_TSU);
> -	gpio_set_value(data->pdata->gpio_sck, 0);
> +	gpiod_set_value(data->sck, 0);
>  	ndelay(SHT15_TSCKL);
> -	gpio_set_value(data->pdata->gpio_sck, 1);
> +	gpiod_set_value(data->sck, 1);
>  	ndelay(SHT15_TSCKH);
> -	gpio_set_value(data->pdata->gpio_data, 1);
> +	gpiod_set_value(data->data, 1);
>  	ndelay(SHT15_TSU);
> -	gpio_set_value(data->pdata->gpio_sck, 0);
> +	gpiod_set_value(data->sck, 0);
>  	ndelay(SHT15_TSCKL);
>  	return 0;
>  }
> @@ -292,20 +293,20 @@ static int sht15_wait_for_response(struct sht15_data *data)
>  {
>  	int err;
>  
> -	err = gpio_direction_input(data->pdata->gpio_data);
> +	err = gpiod_direction_input(data->data);
>  	if (err)
>  		return err;
> -	gpio_set_value(data->pdata->gpio_sck, 1);
> +	gpiod_set_value(data->sck, 1);
>  	ndelay(SHT15_TSCKH);
> -	if (gpio_get_value(data->pdata->gpio_data)) {
> -		gpio_set_value(data->pdata->gpio_sck, 0);
> +	if (gpiod_get_value(data->data)) {
> +		gpiod_set_value(data->sck, 0);
>  		dev_err(data->dev, "Command not acknowledged\n");
>  		err = sht15_connection_reset(data);
>  		if (err)
>  			return err;
>  		return -EIO;
>  	}
> -	gpio_set_value(data->pdata->gpio_sck, 0);
> +	gpiod_set_value(data->sck, 0);
>  	ndelay(SHT15_TSCKL);
>  	return 0;
>  }
> @@ -360,17 +361,17 @@ static int sht15_ack(struct sht15_data *data)
>  {
>  	int err;
>  
> -	err = gpio_direction_output(data->pdata->gpio_data, 0);
> +	err = gpiod_direction_output(data->data, 0);
>  	if (err)
>  		return err;
>  	ndelay(SHT15_TSU);
> -	gpio_set_value(data->pdata->gpio_sck, 1);
> +	gpiod_set_value(data->sck, 1);
>  	ndelay(SHT15_TSU);
> -	gpio_set_value(data->pdata->gpio_sck, 0);
> +	gpiod_set_value(data->sck, 0);
>  	ndelay(SHT15_TSU);
> -	gpio_set_value(data->pdata->gpio_data, 1);
> +	gpiod_set_value(data->data, 1);
>  
> -	return gpio_direction_input(data->pdata->gpio_data);
> +	return gpiod_direction_input(data->data);
>  }
>  
>  /**
> @@ -383,13 +384,13 @@ static int sht15_end_transmission(struct sht15_data *data)
>  {
>  	int err;
>  
> -	err = gpio_direction_output(data->pdata->gpio_data, 1);
> +	err = gpiod_direction_output(data->data, 1);
>  	if (err)
>  		return err;
>  	ndelay(SHT15_TSU);
> -	gpio_set_value(data->pdata->gpio_sck, 1);
> +	gpiod_set_value(data->sck, 1);
>  	ndelay(SHT15_TSCKH);
> -	gpio_set_value(data->pdata->gpio_sck, 0);
> +	gpiod_set_value(data->sck, 0);
>  	ndelay(SHT15_TSCKL);
>  	return 0;
>  }
> @@ -405,10 +406,10 @@ static u8 sht15_read_byte(struct sht15_data *data)
>  
>  	for (i = 0; i < 8; ++i) {
>  		byte <<= 1;
> -		gpio_set_value(data->pdata->gpio_sck, 1);
> +		gpiod_set_value(data->sck, 1);
>  		ndelay(SHT15_TSCKH);
> -		byte |= !!gpio_get_value(data->pdata->gpio_data);
> -		gpio_set_value(data->pdata->gpio_sck, 0);
> +		byte |= !!gpiod_get_value(data->data);
> +		gpiod_set_value(data->sck, 0);
>  		ndelay(SHT15_TSCKL);
>  	}
>  	return byte;
> @@ -428,7 +429,7 @@ static int sht15_send_status(struct sht15_data *data, u8 status)
>  	err = sht15_send_cmd(data, SHT15_WRITE_STATUS);
>  	if (err)
>  		return err;
> -	err = gpio_direction_output(data->pdata->gpio_data, 1);
> +	err = gpiod_direction_output(data->data, 1);
>  	if (err)
>  		return err;
>  	ndelay(SHT15_TSU);
> @@ -528,14 +529,14 @@ static int sht15_measurement(struct sht15_data *data,
>  	if (ret)
>  		return ret;
>  
> -	ret = gpio_direction_input(data->pdata->gpio_data);
> +	ret = gpiod_direction_input(data->data);
>  	if (ret)
>  		return ret;
>  	atomic_set(&data->interrupt_handled, 0);
>  
> -	enable_irq(gpio_to_irq(data->pdata->gpio_data));
> -	if (gpio_get_value(data->pdata->gpio_data) == 0) {
> -		disable_irq_nosync(gpio_to_irq(data->pdata->gpio_data));
> +	enable_irq(gpiod_to_irq(data->data));
> +	if (gpiod_get_value(data->data) == 0) {
> +		disable_irq_nosync(gpiod_to_irq(data->data));
>  		/* Only relevant if the interrupt hasn't occurred. */
>  		if (!atomic_read(&data->interrupt_handled))
>  			schedule_work(&data->read_work);
> @@ -547,7 +548,7 @@ static int sht15_measurement(struct sht15_data *data,
>  		data->state = SHT15_READING_NOTHING;
>  		return -EIO;
>  	} else if (ret == 0) { /* timeout occurred */
> -		disable_irq_nosync(gpio_to_irq(data->pdata->gpio_data));
> +		disable_irq_nosync(gpiod_to_irq(data->data));
>  		ret = sht15_connection_reset(data);
>  		if (ret)
>  			return ret;
> @@ -826,15 +827,15 @@ static void sht15_bh_read_data(struct work_struct *work_s)
>  			       read_work);
>  
>  	/* Firstly, verify the line is low */
> -	if (gpio_get_value(data->pdata->gpio_data)) {
> +	if (gpiod_get_value(data->data)) {
>  		/*
>  		 * If not, then start the interrupt again - care here as could
>  		 * have gone low in meantime so verify it hasn't!
>  		 */
>  		atomic_set(&data->interrupt_handled, 0);
> -		enable_irq(gpio_to_irq(data->pdata->gpio_data));
> +		enable_irq(gpiod_to_irq(data->data));
>  		/* If still not occurred or another handler was scheduled */
> -		if (gpio_get_value(data->pdata->gpio_data)
> +		if (gpiod_get_value(data->data)
>  		    || atomic_read(&data->interrupt_handled))
>  			return;
>  	}
> @@ -918,46 +919,6 @@ static const struct of_device_id sht15_dt_match[] = {
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, sht15_dt_match);
> -
> -/*
> - * This function returns NULL if pdev isn't a device instatiated by dt,
> - * a pointer to pdata if it could successfully get all information
> - * from dt or a negative ERR_PTR() on error.
> - */
> -static struct sht15_platform_data *sht15_probe_dt(struct device *dev)
> -{
> -	struct device_node *np = dev->of_node;
> -	struct sht15_platform_data *pdata;
> -
> -	/* no device tree device */
> -	if (!np)
> -		return NULL;
> -
> -	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> -	if (!pdata)
> -		return ERR_PTR(-ENOMEM);
> -
> -	pdata->gpio_data = of_get_named_gpio(np, "data-gpios", 0);
> -	if (pdata->gpio_data < 0) {
> -		if (pdata->gpio_data != -EPROBE_DEFER)
> -			dev_err(dev, "data-gpios not found\n");
> -		return ERR_PTR(pdata->gpio_data);
> -	}
> -
> -	pdata->gpio_sck = of_get_named_gpio(np, "clk-gpios", 0);
> -	if (pdata->gpio_sck < 0) {
> -		if (pdata->gpio_sck != -EPROBE_DEFER)
> -			dev_err(dev, "clk-gpios not found\n");
> -		return ERR_PTR(pdata->gpio_sck);
> -	}
> -
> -	return pdata;
> -}
> -#else
> -static inline struct sht15_platform_data *sht15_probe_dt(struct device *dev)
> -{
> -	return NULL;
> -}
>  #endif
>  
>  static int sht15_probe(struct platform_device *pdev)
> @@ -977,25 +938,6 @@ static int sht15_probe(struct platform_device *pdev)
>  	data->dev = &pdev->dev;
>  	init_waitqueue_head(&data->wait_queue);
>  
> -	data->pdata = sht15_probe_dt(&pdev->dev);
> -	if (IS_ERR(data->pdata))
> -		return PTR_ERR(data->pdata);
> -	if (data->pdata == NULL) {
> -		data->pdata = dev_get_platdata(&pdev->dev);
> -		if (data->pdata == NULL) {
> -			dev_err(&pdev->dev, "no platform data supplied\n");
> -			return -EINVAL;
> -		}
> -	}
> -
> -	data->supply_uv = data->pdata->supply_mv * 1000;
> -	if (data->pdata->checksum)
> -		data->checksumming = true;
> -	if (data->pdata->no_otp_reload)
> -		status |= SHT15_STATUS_NO_OTP_RELOAD;
> -	if (data->pdata->low_resolution)
> -		status |= SHT15_STATUS_LOW_RESOLUTION;
> -
>  	/*
>  	 * If a regulator is available,
>  	 * query what the supply voltage actually is!
> @@ -1030,21 +972,20 @@ static int sht15_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Try requesting the GPIOs */
> -	ret = devm_gpio_request_one(&pdev->dev, data->pdata->gpio_sck,
> -			GPIOF_OUT_INIT_LOW, "SHT15 sck");
> -	if (ret) {
> +	data->sck = devm_gpiod_get(&pdev->dev, "clk", GPIOD_OUT_LOW);
> +	if (IS_ERR(data->sck)) {
> +		ret = PTR_ERR(data->sck);
>  		dev_err(&pdev->dev, "clock line GPIO request failed\n");
>  		goto err_release_reg;
>  	}
> -
> -	ret = devm_gpio_request(&pdev->dev, data->pdata->gpio_data,
> -				"SHT15 data");
> -	if (ret) {
> +	data->data = devm_gpiod_get(&pdev->dev, "data", GPIOD_IN);
> +	if (IS_ERR(data->data)) {
> +		ret = PTR_ERR(data->data);
>  		dev_err(&pdev->dev, "data line GPIO request failed\n");
>  		goto err_release_reg;
>  	}
>  
> -	ret = devm_request_irq(&pdev->dev, gpio_to_irq(data->pdata->gpio_data),
> +	ret = devm_request_irq(&pdev->dev, gpiod_to_irq(data->data),
>  			       sht15_interrupt_fired,
>  			       IRQF_TRIGGER_FALLING,
>  			       "sht15 data",
> @@ -1053,7 +994,7 @@ static int sht15_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "failed to get irq for data line\n");
>  		goto err_release_reg;
>  	}
> -	disable_irq_nosync(gpio_to_irq(data->pdata->gpio_data));
> +	disable_irq_nosync(gpiod_to_irq(data->data));
>  	ret = sht15_connection_reset(data);
>  	if (ret)
>  		goto err_release_reg;
> diff --git a/include/linux/platform_data/sht15.h b/include/linux/platform_data/sht15.h
> deleted file mode 100644
> index 12289c1e9413..000000000000
> --- a/include/linux/platform_data/sht15.h
> +++ /dev/null
> @@ -1,38 +0,0 @@
> -/*
> - * sht15.h - support for the SHT15 Temperature and Humidity Sensor
> - *
> - * Copyright (c) 2009 Jonathan Cameron
> - *
> - * Copyright (c) 2007 Wouter Horre
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
> - * For further information, see the Documentation/hwmon/sht15 file.
> - */
> -
> -#ifndef _PDATA_SHT15_H
> -#define _PDATA_SHT15_H
> -
> -/**
> - * struct sht15_platform_data - sht15 connectivity info
> - * @gpio_data:		no. of gpio to which bidirectional data line is
> - *			connected.
> - * @gpio_sck:		no. of gpio to which the data clock is connected.
> - * @supply_mv:		supply voltage in mv. Overridden by regulator if
> - *			available.
> - * @checksum:		flag to indicate the checksum should be validated.
> - * @no_otp_reload:	flag to indicate no reload from OTP.
> - * @low_resolution:	flag to indicate the temp/humidity resolution to use.
> - */
> -struct sht15_platform_data {
> -	int gpio_data;
> -	int gpio_sck;
> -	int supply_mv;
> -	bool checksum;
> -	bool no_otp_reload;
> -	bool low_resolution;
> -};
> -
> -#endif /* _PDATA_SHT15_H */

Re: [PATCH v2] hwmon/sht15: Root out platform data

[Jonathan Cameron]

On Sun, 10 Sep 2017 16:05:28 +0200
Davide Hug <d@videhug.ch> wrote:

> On Sun, 10 Sep 2017 11:44:46 +0200
> Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> > After finding out there are active users of this sensor I noticed:  
> 
> I would like to point out that I'm doing this for learning purposes and that I
> don't have any pressing needs for this driver to work. But of course I'm really
> grateful for all the replays!
> 

I can't claim I ever had any real use for this part either.  It was
just looking at me from a dev board and asking for me to mainline
the driver :)

> > Cc: arm@kernel.org
> > Cc: Marco Franchi <marco.franchi@nxp.com>
> > Cc: Davide Hug <d@videhug.ch>
> > Cc: Jonathan Cameron <jic23@cam.ac.uk>
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> > ChangeLog v1->v2:
> > - Fix the return value from devm_gpiod_get() with PTR_ERR()
> >   so we defer etc correctly.
> > 
> > ARM SoC folks: please ACK this so the HWMON maintainer can merge
> > it when it is in reasonable shape.
> > 
> > Marco: can you test this patch with your setup?
> > 
> > Davide: can you test this patch with your setup?  
> 
> I tested it and I can make it work (as the current driver) by modifying gpiolib
> to not test for the FLAG_USED_AS_IRQ in _gpiod_direction_output_raw.
> Otherwise again I get:
> 
> 	gpio-48 (SHT15 data): _gpiod_direction_output_raw: tried to set a GPIO tied to an IRQ as output
> 
> as with the current driver. But I guess this is as expected.
> (I did this on a beaglebone black with kernel 4.9.10. I'll compile a newer
> kernel and try it there to.)

Cool.

> 
> Thanks!

Thanks to you.  It's always nice when people fix issues with / update a
driver you've ignored for perhaps 8 years ;)

As you know this particular part has a truely horrendous custom bus.
I'm always amazed these old sensiron parts work at all.

I'm also really surprised to see they are still readily available.

Jonathan

> Davide

Re: [PATCH v2] hwmon/sht15: Root out platform data

[Marco Franchi]

[-- Attachment #1: Type: text/plain, Size: 1593 bytes --]

On Sun, 10 Sep 2017 11:44:46 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:


> After finding out there are active users of this sensor I noticed:
>
> - It has a single PXA27x board file using the platform data
> - The platform data is only used to carry two GPIO pins, all other
>   fields are unused
> - The driver does not use GPIO descriptors but the legacy GPIO
>   API
>
> I saw we can swiftly fix this by:
>
> - Killing off the platform data entirely
> - Define a GPIO descriptor lookup table in the board file
> - Use the standard devm_gpiod_get() to grab the GPIO descriptors
>   from either the device tree or the board file table.
>
> This compiles, but needs testing.
>
> Cc: arm@kernel.org
> Cc: Marco Franchi <marco.franchi@nxp.com>
> Cc: Davide Hug <d@videhug.ch>
> Cc: Jonathan Cameron <jic23@cam.ac.uk>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Fix the return value from devm_gpiod_get() with PTR_ERR()
>   so we defer etc correctly.
>
> ARM SoC folks: please ACK this so the HWMON maintainer can merge
> it when it is in reasonable shape.
>
> Marco: can you test this patch with your setup?

I tested it using the i.MX 7D SABRE-SD and device tree support and it works good.

>
> Davide: can you test this patch with your setup?
>
> Jonathan: I gues you may feel this sensor needs migrating to IIO or
> so, like the Imote and Stargate2 needs migrating to device tree,
> but this helps a bit with that.

Thanks for your help!

Tested by: Marco Franchi <marco.franchi@nxp.com>

[-- Attachment #2: Type: text/html, Size: 3805 bytes --]

Re: [PATCH v2] hwmon/sht15: Root out platform data

[Guenter Roeck]

On 09/10/2017 02:44 AM, Linus Walleij wrote:
> After finding out there are active users of this sensor I noticed:
> 
> - It has a single PXA27x board file using the platform data
> - The platform data is only used to carry two GPIO pins, all other
>    fields are unused
> - The driver does not use GPIO descriptors but the legacy GPIO
>    API
> 
> I saw we can swiftly fix this by:
> 
> - Killing off the platform data entirely
> - Define a GPIO descriptor lookup table in the board file
> - Use the standard devm_gpiod_get() to grab the GPIO descriptors
>    from either the device tree or the board file table.
> 
> This compiles, but needs testing.
> 
> Cc: arm@kernel.org
> Cc: Marco Franchi <marco.franchi@nxp.com>
> Cc: Davide Hug <d@videhug.ch>
> Cc: Jonathan Cameron <jic23@cam.ac.uk>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Applied to hwmon-next.

Thanks,
Guenter

> ---
> ChangeLog v1->v2:
> - Fix the return value from devm_gpiod_get() with PTR_ERR()
>    so we defer etc correctly.
> 
> ARM SoC folks: please ACK this so the HWMON maintainer can merge
> it when it is in reasonable shape.
> 
> Marco: can you test this patch with your setup?
> 
> Davide: can you test this patch with your setup?
> 
> Jonathan: I gues you may feel this sensor needs migrating to IIO or
> so, like the Imote and Stargate2 needs migrating to device tree,
> but this helps a bit with that.
> ---
>   Documentation/hwmon/sht15           |   3 +-
>   arch/arm/mach-pxa/stargate2.c       |  17 ++--
>   drivers/hwmon/sht15.c               | 167 ++++++++++++------------------------
>   include/linux/platform_data/sht15.h |  38 --------
>   4 files changed, 65 insertions(+), 160 deletions(-)
>   delete mode 100644 include/linux/platform_data/sht15.h
> 
> diff --git a/Documentation/hwmon/sht15 b/Documentation/hwmon/sht15
> index 778987d1856f..5e3207c3b177 100644
> --- a/Documentation/hwmon/sht15
> +++ b/Documentation/hwmon/sht15
> @@ -42,8 +42,7 @@ chip. These coefficients are used to internally calibrate the signals from the
>   sensors. Disabling the reload of those coefficients allows saving 10ms for each
>   measurement and decrease power consumption, while losing on precision.
>   
> -Some options may be set directly in the sht15_platform_data structure
> -or via sysfs attributes.
> +Some options may be set via sysfs attributes.
>   
>   Notes:
>     * The regulator supply name is set to "vcc".
> diff --git a/arch/arm/mach-pxa/stargate2.c b/arch/arm/mach-pxa/stargate2.c
> index 2d45d18b1a5e..6b7df6fd2448 100644
> --- a/arch/arm/mach-pxa/stargate2.c
> +++ b/arch/arm/mach-pxa/stargate2.c
> @@ -29,6 +29,7 @@
>   #include <linux/platform_data/pcf857x.h>
>   #include <linux/platform_data/at24.h>
>   #include <linux/smc91x.h>
> +#include <linux/gpio/machine.h>
>   #include <linux/gpio.h>
>   #include <linux/leds.h>
>   
> @@ -52,7 +53,6 @@
>   #include <linux/spi/spi.h>
>   #include <linux/spi/pxa2xx_spi.h>
>   #include <linux/mfd/da903x.h>
> -#include <linux/platform_data/sht15.h>
>   
>   #include "devices.h"
>   #include "generic.h"
> @@ -137,17 +137,18 @@ static unsigned long sg2_im2_unified_pin_config[] __initdata = {
>   	GPIO10_GPIO, /* large basic connector pin 23 */
>   };
>   
> -static struct sht15_platform_data platform_data_sht15 = {
> -	.gpio_data =  100,
> -	.gpio_sck  =  98,
> +static struct gpiod_lookup_table sht15_gpiod_table = {
> +	.dev_id = "sht15",
> +	.table = {
> +		/* FIXME: should this have |GPIO_OPEN_DRAIN set? */
> +		GPIO_LOOKUP("gpio-pxa", 100, "data", GPIO_ACTIVE_HIGH),
> +		GPIO_LOOKUP("gpio-pxa", 98, "clk", GPIO_ACTIVE_HIGH),
> +	},
>   };
>   
>   static struct platform_device sht15 = {
>   	.name = "sht15",
>   	.id = -1,
> -	.dev = {
> -		.platform_data = &platform_data_sht15,
> -	},
>   };
>   
>   static struct regulator_consumer_supply stargate2_sensor_3_con[] = {
> @@ -608,6 +609,7 @@ static void __init imote2_init(void)
>   
>   	imote2_stargate2_init();
>   
> +	gpiod_add_lookup_table(&sht15_gpiod_table);
>   	platform_add_devices(imote2_devices, ARRAY_SIZE(imote2_devices));
>   
>   	i2c_register_board_info(0, imote2_i2c_board_info,
> @@ -988,6 +990,7 @@ static void __init stargate2_init(void)
>   
>   	imote2_stargate2_init();
>   
> +	gpiod_add_lookup_table(&sht15_gpiod_table);
>   	platform_add_devices(ARRAY_AND_SIZE(stargate2_devices));
>   
>   	i2c_register_board_info(0, ARRAY_AND_SIZE(stargate2_i2c_board_info));
> diff --git a/drivers/hwmon/sht15.c b/drivers/hwmon/sht15.c
> index e4d642b673c6..0e3e5f83f5cf 100644
> --- a/drivers/hwmon/sht15.c
> +++ b/drivers/hwmon/sht15.c
> @@ -18,13 +18,11 @@
>   
>   #include <linux/interrupt.h>
>   #include <linux/irq.h>
> -#include <linux/gpio.h>
>   #include <linux/module.h>
>   #include <linux/init.h>
>   #include <linux/hwmon.h>
>   #include <linux/hwmon-sysfs.h>
>   #include <linux/mutex.h>
> -#include <linux/platform_data/sht15.h>
>   #include <linux/platform_device.h>
>   #include <linux/sched.h>
>   #include <linux/delay.h>
> @@ -34,7 +32,8 @@
>   #include <linux/slab.h>
>   #include <linux/atomic.h>
>   #include <linux/bitrev.h>
> -#include <linux/of_gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/of.h>
>   
>   /* Commands */
>   #define SHT15_MEASURE_TEMP		0x03
> @@ -122,7 +121,8 @@ static const u8 sht15_crc8_table[] = {
>   
>   /**
>    * struct sht15_data - device instance specific data
> - * @pdata:		platform data (gpio's etc).
> + * @sck:		clock GPIO line
> + * @data:		data GPIO line
>    * @read_work:		bh of interrupt handler.
>    * @wait_queue:		wait queue for getting values from device.
>    * @val_temp:		last temperature value read from device.
> @@ -150,7 +150,8 @@ static const u8 sht15_crc8_table[] = {
>    * @interrupt_handled:	flag used to indicate a handler has been scheduled.
>    */
>   struct sht15_data {
> -	struct sht15_platform_data	*pdata;
> +	struct gpio_desc		*sck;
> +	struct gpio_desc		*data;
>   	struct work_struct		read_work;
>   	wait_queue_head_t		wait_queue;
>   	uint16_t			val_temp;
> @@ -205,16 +206,16 @@ static int sht15_connection_reset(struct sht15_data *data)
>   {
>   	int i, err;
>   
> -	err = gpio_direction_output(data->pdata->gpio_data, 1);
> +	err = gpiod_direction_output(data->data, 1);
>   	if (err)
>   		return err;
>   	ndelay(SHT15_TSCKL);
> -	gpio_set_value(data->pdata->gpio_sck, 0);
> +	gpiod_set_value(data->sck, 0);
>   	ndelay(SHT15_TSCKL);
>   	for (i = 0; i < 9; ++i) {
> -		gpio_set_value(data->pdata->gpio_sck, 1);
> +		gpiod_set_value(data->sck, 1);
>   		ndelay(SHT15_TSCKH);
> -		gpio_set_value(data->pdata->gpio_sck, 0);
> +		gpiod_set_value(data->sck, 0);
>   		ndelay(SHT15_TSCKL);
>   	}
>   	return 0;
> @@ -227,11 +228,11 @@ static int sht15_connection_reset(struct sht15_data *data)
>    */
>   static inline void sht15_send_bit(struct sht15_data *data, int val)
>   {
> -	gpio_set_value(data->pdata->gpio_data, val);
> +	gpiod_set_value(data->data, val);
>   	ndelay(SHT15_TSU);
> -	gpio_set_value(data->pdata->gpio_sck, 1);
> +	gpiod_set_value(data->sck, 1);
>   	ndelay(SHT15_TSCKH);
> -	gpio_set_value(data->pdata->gpio_sck, 0);
> +	gpiod_set_value(data->sck, 0);
>   	ndelay(SHT15_TSCKL); /* clock low time */
>   }
>   
> @@ -248,23 +249,23 @@ static int sht15_transmission_start(struct sht15_data *data)
>   	int err;
>   
>   	/* ensure data is high and output */
> -	err = gpio_direction_output(data->pdata->gpio_data, 1);
> +	err = gpiod_direction_output(data->data, 1);
>   	if (err)
>   		return err;
>   	ndelay(SHT15_TSU);
> -	gpio_set_value(data->pdata->gpio_sck, 0);
> +	gpiod_set_value(data->sck, 0);
>   	ndelay(SHT15_TSCKL);
> -	gpio_set_value(data->pdata->gpio_sck, 1);
> +	gpiod_set_value(data->sck, 1);
>   	ndelay(SHT15_TSCKH);
> -	gpio_set_value(data->pdata->gpio_data, 0);
> +	gpiod_set_value(data->data, 0);
>   	ndelay(SHT15_TSU);
> -	gpio_set_value(data->pdata->gpio_sck, 0);
> +	gpiod_set_value(data->sck, 0);
>   	ndelay(SHT15_TSCKL);
> -	gpio_set_value(data->pdata->gpio_sck, 1);
> +	gpiod_set_value(data->sck, 1);
>   	ndelay(SHT15_TSCKH);
> -	gpio_set_value(data->pdata->gpio_data, 1);
> +	gpiod_set_value(data->data, 1);
>   	ndelay(SHT15_TSU);
> -	gpio_set_value(data->pdata->gpio_sck, 0);
> +	gpiod_set_value(data->sck, 0);
>   	ndelay(SHT15_TSCKL);
>   	return 0;
>   }
> @@ -292,20 +293,20 @@ static int sht15_wait_for_response(struct sht15_data *data)
>   {
>   	int err;
>   
> -	err = gpio_direction_input(data->pdata->gpio_data);
> +	err = gpiod_direction_input(data->data);
>   	if (err)
>   		return err;
> -	gpio_set_value(data->pdata->gpio_sck, 1);
> +	gpiod_set_value(data->sck, 1);
>   	ndelay(SHT15_TSCKH);
> -	if (gpio_get_value(data->pdata->gpio_data)) {
> -		gpio_set_value(data->pdata->gpio_sck, 0);
> +	if (gpiod_get_value(data->data)) {
> +		gpiod_set_value(data->sck, 0);
>   		dev_err(data->dev, "Command not acknowledged\n");
>   		err = sht15_connection_reset(data);
>   		if (err)
>   			return err;
>   		return -EIO;
>   	}
> -	gpio_set_value(data->pdata->gpio_sck, 0);
> +	gpiod_set_value(data->sck, 0);
>   	ndelay(SHT15_TSCKL);
>   	return 0;
>   }
> @@ -360,17 +361,17 @@ static int sht15_ack(struct sht15_data *data)
>   {
>   	int err;
>   
> -	err = gpio_direction_output(data->pdata->gpio_data, 0);
> +	err = gpiod_direction_output(data->data, 0);
>   	if (err)
>   		return err;
>   	ndelay(SHT15_TSU);
> -	gpio_set_value(data->pdata->gpio_sck, 1);
> +	gpiod_set_value(data->sck, 1);
>   	ndelay(SHT15_TSU);
> -	gpio_set_value(data->pdata->gpio_sck, 0);
> +	gpiod_set_value(data->sck, 0);
>   	ndelay(SHT15_TSU);
> -	gpio_set_value(data->pdata->gpio_data, 1);
> +	gpiod_set_value(data->data, 1);
>   
> -	return gpio_direction_input(data->pdata->gpio_data);
> +	return gpiod_direction_input(data->data);
>   }
>   
>   /**
> @@ -383,13 +384,13 @@ static int sht15_end_transmission(struct sht15_data *data)
>   {
>   	int err;
>   
> -	err = gpio_direction_output(data->pdata->gpio_data, 1);
> +	err = gpiod_direction_output(data->data, 1);
>   	if (err)
>   		return err;
>   	ndelay(SHT15_TSU);
> -	gpio_set_value(data->pdata->gpio_sck, 1);
> +	gpiod_set_value(data->sck, 1);
>   	ndelay(SHT15_TSCKH);
> -	gpio_set_value(data->pdata->gpio_sck, 0);
> +	gpiod_set_value(data->sck, 0);
>   	ndelay(SHT15_TSCKL);
>   	return 0;
>   }
> @@ -405,10 +406,10 @@ static u8 sht15_read_byte(struct sht15_data *data)
>   
>   	for (i = 0; i < 8; ++i) {
>   		byte <<= 1;
> -		gpio_set_value(data->pdata->gpio_sck, 1);
> +		gpiod_set_value(data->sck, 1);
>   		ndelay(SHT15_TSCKH);
> -		byte |= !!gpio_get_value(data->pdata->gpio_data);
> -		gpio_set_value(data->pdata->gpio_sck, 0);
> +		byte |= !!gpiod_get_value(data->data);
> +		gpiod_set_value(data->sck, 0);
>   		ndelay(SHT15_TSCKL);
>   	}
>   	return byte;
> @@ -428,7 +429,7 @@ static int sht15_send_status(struct sht15_data *data, u8 status)
>   	err = sht15_send_cmd(data, SHT15_WRITE_STATUS);
>   	if (err)
>   		return err;
> -	err = gpio_direction_output(data->pdata->gpio_data, 1);
> +	err = gpiod_direction_output(data->data, 1);
>   	if (err)
>   		return err;
>   	ndelay(SHT15_TSU);
> @@ -528,14 +529,14 @@ static int sht15_measurement(struct sht15_data *data,
>   	if (ret)
>   		return ret;
>   
> -	ret = gpio_direction_input(data->pdata->gpio_data);
> +	ret = gpiod_direction_input(data->data);
>   	if (ret)
>   		return ret;
>   	atomic_set(&data->interrupt_handled, 0);
>   
> -	enable_irq(gpio_to_irq(data->pdata->gpio_data));
> -	if (gpio_get_value(data->pdata->gpio_data) == 0) {
> -		disable_irq_nosync(gpio_to_irq(data->pdata->gpio_data));
> +	enable_irq(gpiod_to_irq(data->data));
> +	if (gpiod_get_value(data->data) == 0) {
> +		disable_irq_nosync(gpiod_to_irq(data->data));
>   		/* Only relevant if the interrupt hasn't occurred. */
>   		if (!atomic_read(&data->interrupt_handled))
>   			schedule_work(&data->read_work);
> @@ -547,7 +548,7 @@ static int sht15_measurement(struct sht15_data *data,
>   		data->state = SHT15_READING_NOTHING;
>   		return -EIO;
>   	} else if (ret == 0) { /* timeout occurred */
> -		disable_irq_nosync(gpio_to_irq(data->pdata->gpio_data));
> +		disable_irq_nosync(gpiod_to_irq(data->data));
>   		ret = sht15_connection_reset(data);
>   		if (ret)
>   			return ret;
> @@ -826,15 +827,15 @@ static void sht15_bh_read_data(struct work_struct *work_s)
>   			       read_work);
>   
>   	/* Firstly, verify the line is low */
> -	if (gpio_get_value(data->pdata->gpio_data)) {
> +	if (gpiod_get_value(data->data)) {
>   		/*
>   		 * If not, then start the interrupt again - care here as could
>   		 * have gone low in meantime so verify it hasn't!
>   		 */
>   		atomic_set(&data->interrupt_handled, 0);
> -		enable_irq(gpio_to_irq(data->pdata->gpio_data));
> +		enable_irq(gpiod_to_irq(data->data));
>   		/* If still not occurred or another handler was scheduled */
> -		if (gpio_get_value(data->pdata->gpio_data)
> +		if (gpiod_get_value(data->data)
>   		    || atomic_read(&data->interrupt_handled))
>   			return;
>   	}
> @@ -918,46 +919,6 @@ static const struct of_device_id sht15_dt_match[] = {
>   	{ },
>   };
>   MODULE_DEVICE_TABLE(of, sht15_dt_match);
> -
> -/*
> - * This function returns NULL if pdev isn't a device instatiated by dt,
> - * a pointer to pdata if it could successfully get all information
> - * from dt or a negative ERR_PTR() on error.
> - */
> -static struct sht15_platform_data *sht15_probe_dt(struct device *dev)
> -{
> -	struct device_node *np = dev->of_node;
> -	struct sht15_platform_data *pdata;
> -
> -	/* no device tree device */
> -	if (!np)
> -		return NULL;
> -
> -	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> -	if (!pdata)
> -		return ERR_PTR(-ENOMEM);
> -
> -	pdata->gpio_data = of_get_named_gpio(np, "data-gpios", 0);
> -	if (pdata->gpio_data < 0) {
> -		if (pdata->gpio_data != -EPROBE_DEFER)
> -			dev_err(dev, "data-gpios not found\n");
> -		return ERR_PTR(pdata->gpio_data);
> -	}
> -
> -	pdata->gpio_sck = of_get_named_gpio(np, "clk-gpios", 0);
> -	if (pdata->gpio_sck < 0) {
> -		if (pdata->gpio_sck != -EPROBE_DEFER)
> -			dev_err(dev, "clk-gpios not found\n");
> -		return ERR_PTR(pdata->gpio_sck);
> -	}
> -
> -	return pdata;
> -}
> -#else
> -static inline struct sht15_platform_data *sht15_probe_dt(struct device *dev)
> -{
> -	return NULL;
> -}
>   #endif
>   
>   static int sht15_probe(struct platform_device *pdev)
> @@ -977,25 +938,6 @@ static int sht15_probe(struct platform_device *pdev)
>   	data->dev = &pdev->dev;
>   	init_waitqueue_head(&data->wait_queue);
>   
> -	data->pdata = sht15_probe_dt(&pdev->dev);
> -	if (IS_ERR(data->pdata))
> -		return PTR_ERR(data->pdata);
> -	if (data->pdata == NULL) {
> -		data->pdata = dev_get_platdata(&pdev->dev);
> -		if (data->pdata == NULL) {
> -			dev_err(&pdev->dev, "no platform data supplied\n");
> -			return -EINVAL;
> -		}
> -	}
> -
> -	data->supply_uv = data->pdata->supply_mv * 1000;
> -	if (data->pdata->checksum)
> -		data->checksumming = true;
> -	if (data->pdata->no_otp_reload)
> -		status |= SHT15_STATUS_NO_OTP_RELOAD;
> -	if (data->pdata->low_resolution)
> -		status |= SHT15_STATUS_LOW_RESOLUTION;
> -
>   	/*
>   	 * If a regulator is available,
>   	 * query what the supply voltage actually is!
> @@ -1030,21 +972,20 @@ static int sht15_probe(struct platform_device *pdev)
>   	}
>   
>   	/* Try requesting the GPIOs */
> -	ret = devm_gpio_request_one(&pdev->dev, data->pdata->gpio_sck,
> -			GPIOF_OUT_INIT_LOW, "SHT15 sck");
> -	if (ret) {
> +	data->sck = devm_gpiod_get(&pdev->dev, "clk", GPIOD_OUT_LOW);
> +	if (IS_ERR(data->sck)) {
> +		ret = PTR_ERR(data->sck);
>   		dev_err(&pdev->dev, "clock line GPIO request failed\n");
>   		goto err_release_reg;
>   	}
> -
> -	ret = devm_gpio_request(&pdev->dev, data->pdata->gpio_data,
> -				"SHT15 data");
> -	if (ret) {
> +	data->data = devm_gpiod_get(&pdev->dev, "data", GPIOD_IN);
> +	if (IS_ERR(data->data)) {
> +		ret = PTR_ERR(data->data);
>   		dev_err(&pdev->dev, "data line GPIO request failed\n");
>   		goto err_release_reg;
>   	}
>   
> -	ret = devm_request_irq(&pdev->dev, gpio_to_irq(data->pdata->gpio_data),
> +	ret = devm_request_irq(&pdev->dev, gpiod_to_irq(data->data),
>   			       sht15_interrupt_fired,
>   			       IRQF_TRIGGER_FALLING,
>   			       "sht15 data",
> @@ -1053,7 +994,7 @@ static int sht15_probe(struct platform_device *pdev)
>   		dev_err(&pdev->dev, "failed to get irq for data line\n");
>   		goto err_release_reg;
>   	}
> -	disable_irq_nosync(gpio_to_irq(data->pdata->gpio_data));
> +	disable_irq_nosync(gpiod_to_irq(data->data));
>   	ret = sht15_connection_reset(data);
>   	if (ret)
>   		goto err_release_reg;
> diff --git a/include/linux/platform_data/sht15.h b/include/linux/platform_data/sht15.h
> deleted file mode 100644
> index 12289c1e9413..000000000000
> --- a/include/linux/platform_data/sht15.h
> +++ /dev/null
> @@ -1,38 +0,0 @@
> -/*
> - * sht15.h - support for the SHT15 Temperature and Humidity Sensor
> - *
> - * Copyright (c) 2009 Jonathan Cameron
> - *
> - * Copyright (c) 2007 Wouter Horre
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
> - * For further information, see the Documentation/hwmon/sht15 file.
> - */
> -
> -#ifndef _PDATA_SHT15_H
> -#define _PDATA_SHT15_H
> -
> -/**
> - * struct sht15_platform_data - sht15 connectivity info
> - * @gpio_data:		no. of gpio to which bidirectional data line is
> - *			connected.
> - * @gpio_sck:		no. of gpio to which the data clock is connected.
> - * @supply_mv:		supply voltage in mv. Overridden by regulator if
> - *			available.
> - * @checksum:		flag to indicate the checksum should be validated.
> - * @no_otp_reload:	flag to indicate no reload from OTP.
> - * @low_resolution:	flag to indicate the temp/humidity resolution to use.
> - */
> -struct sht15_platform_data {
> -	int gpio_data;
> -	int gpio_sck;
> -	int supply_mv;
> -	bool checksum;
> -	bool no_otp_reload;
> -	bool low_resolution;
> -};
> -
> -#endif /* _PDATA_SHT15_H */
> 

Re: [PATCH v2] hwmon/sht15: Root out platform data

[Davide Hug]

Sorry for the huge delay.

On Sun, 10 Sep 2017 17:23:49 +0100
Jonathan Cameron <jic23@kernel.org> wrote:
> Thanks to you.  It's always nice when people fix issues with / update a
> driver you've ignored for perhaps 8 years ;)

Thank you for first implementing it. It's amazing how one can take any strange
sensor and then discover that there is already a driver for it in Linux.

> As you know this particular part has a truely horrendous custom bus.
> I'm always amazed these old sensiron parts work at all.

Yes, strange.

> I'm also really surprised to see they are still readily available.

I originally bought it to play with a pic16f84. That's really long ago. But I
checked now and they still can be ordered but "Last order date: 31.12.2018 /
Last delivery date: 31.12.2019"[1]

Best,
Davide

[1]: https://www.sensirion.com/en/environmental-sensors/humidity-sensors/digital-humidity-sensors-for-accurate-measurements/

Re: [PATCH v2] hwmon/sht15: Root out platform data

[Davide Hug]

Sorry for the delayed question.

On Mon, 11 Sep 2017 17:35:35 +0000
Marco Franchi <marco.franchi@nxp.com> wrote:

> I tested it using the i.MX 7D SABRE-SD and device tree support and it works good.

It would be great if you could tell me what GPIO driver this platform uses?
I would like to check if I understand when the sht15 driver fails to load and
when it loads.

Thanks,
Davide

Re: [PATCH v2] hwmon/sht15: Root out platform data

[Marco Franchi]

On Ter, 2017-11-21 at 00:28 +0100, Davide Hug wrote:
> Sorry for the delayed question.
> 
> On Mon, 11 Sep 2017 17:35:35 +0000
> Marco Franchi <marco.franchi@nxp.com> wrote:
> 
> > 
> > I tested it using the i.MX 7D SABRE-SD and device tree support and
> > it works good.
> It would be great if you could tell me what GPIO driver this platform
> uses?
> I would like to check if I understand when the sht15 driver fails to
> load and
> when it loads.
Hi Davide,

Please, check if this dts file can help you:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/comm
it/?h=v4.14&id=fd1eb46c382a69d0fbad85def3a2f3008a76efc1
> 
> Thanks,
> Davide
Regards,
Marco

Re: [PATCH v2] hwmon/sht15: Root out platform data

[Davide Hug]

On Tue, 21 Nov 2017 15:49:39 +0000
Marco Franchi <marco.franchi@nxp.com> wrote:

> On Ter, 2017-11-21 at 00:28 +0100, Davide Hug wrote:
> > Sorry for the delayed question.
> > 
> > On Mon, 11 Sep 2017 17:35:35 +0000
> > Marco Franchi <marco.franchi@nxp.com> wrote:
> > 
> > > 
> > > I tested it using the i.MX 7D SABRE-SD and device tree support and
> > > it works good.
> > It would be great if you could tell me what GPIO driver this platform
> > uses?
> > I would like to check if I understand when the sht15 driver fails to
> > load and
> > when it loads.
> Hi Davide,
> 
> Please, check if this dts file can help you:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/comm
> it/?h=v4.14&id=fd1eb46c382a69d0fbad85def3a2f3008a76efc1

Thanks. As I understand it imx7d-sdb-sht11.dts uses gpio4 specified in
arch/arm/boot/dts/imx7s.dtsi, which has compatible = "fsl,imx7d-gpio",
"fsl,imx35-gpio".
I don't find a driver compatible fsl,imx7d-gpio, so I guess it is the
gpio/gpio-mxc.c driver compatible fsl,imx35-gpio.
This driver doesn't use GPIOLIB_IRQCHIP, so I would understand why hwmon/sht15
loads here.

Thanks,
Davide

Re: [PATCH v2] hwmon/sht15: Root out platform data

[Fabio Estevam]

Hi Davide,

On Wed, Nov 22, 2017 at 8:45 PM, Davide Hug <d@videhug.ch> wrote:

> Thanks. As I understand it imx7d-sdb-sht11.dts uses gpio4 specified in
> arch/arm/boot/dts/imx7s.dtsi, which has compatible = "fsl,imx7d-gpio",
> "fsl,imx35-gpio".
> I don't find a driver compatible fsl,imx7d-gpio, so I guess it is the
> gpio/gpio-mxc.c driver compatible fsl,imx35-gpio.

Correct.

> This driver doesn't use GPIOLIB_IRQCHIP, so I would understand why hwmon/sht15
> loads here.

We build the kernel with imx_v6_v7_defconfig. GPIOLIB_IRQCHIP is selected there.

Regards,

Fabio Estevam

Re: [PATCH v2] hwmon/sht15: Root out platform data

[Davide Hug]

Hi Fabio,

On Wed, 22 Nov 2017 20:53:33 -0200
Fabio Estevam <festevam@gmail.com> wrote:

> Hi Davide,
> 
> On Wed, Nov 22, 2017 at 8:45 PM, Davide Hug <d@videhug.ch> wrote:
> 
> > Thanks. As I understand it imx7d-sdb-sht11.dts uses gpio4 specified in
> > arch/arm/boot/dts/imx7s.dtsi, which has compatible = "fsl,imx7d-gpio",
> > "fsl,imx35-gpio".
> > I don't find a driver compatible fsl,imx7d-gpio, so I guess it is the
> > gpio/gpio-mxc.c driver compatible fsl,imx35-gpio.
> 
> Correct.

Great. Thanks!

> 
> > This driver doesn't use GPIOLIB_IRQCHIP, so I would understand why hwmon/sht15
> > loads here.
> 
> We build the kernel with imx_v6_v7_defconfig. GPIOLIB_IRQCHIP is selected there.

So the GPIOLIB_IRQCHIP code in gpiolib.c is enabled. But the driver
gpio-mxc doesn't use it. In particular it doesn't call gpiochip_add_irqchip
which assigns the .irq_request_resources/.irq_release_resources callbacks that
toggle the FLAG_USED_AS_IRQ.
So in the gpio-mxc driver the guard against setting a GPIO with active IRQ as
output is not in place. And it is this guard that makes the sht15 driver fail
to load when in place.

Thanks,
Davide

Re: [PATCH v2] hwmon/sht15: Root out platform data

[Fabio Estevam]

Hi Davide,

On Fri, Nov 24, 2017 at 10:03 PM, Davide Hug <d@videhug.ch> wrote:

> So the GPIOLIB_IRQCHIP code in gpiolib.c is enabled. But the driver
> gpio-mxc doesn't use it. In particular it doesn't call gpiochip_add_irqchip
> which assigns the .irq_request_resources/.irq_release_resources callbacks that
> toggle the FLAG_USED_AS_IRQ.
> So in the gpio-mxc driver the guard against setting a GPIO with active IRQ as
> output is not in place. And it is this guard that makes the sht15 driver fail
> to load when in place.

What is the gpio driver you are using that shows the error?

Re: [PATCH v2] hwmon/sht15: Root out platform data

[Davide Hug]

Hi Estevam

On Sat, 25 Nov 2017 00:06:44 -0200
Fabio Estevam <festevam@gmail.com> wrote:

> Hi Davide,
> 
> On Fri, Nov 24, 2017 at 10:03 PM, Davide Hug <d@videhug.ch> wrote:
> 
> > So the GPIOLIB_IRQCHIP code in gpiolib.c is enabled. But the driver
> > gpio-mxc doesn't use it. In particular it doesn't call gpiochip_add_irqchip
> > which assigns the .irq_request_resources/.irq_release_resources callbacks that
> > toggle the FLAG_USED_AS_IRQ.
> > So in the gpio-mxc driver the guard against setting a GPIO with active IRQ as
> > output is not in place. And it is this guard that makes the sht15 driver fail
> > to load when in place.
> 
> What is the gpio driver you are using that shows the error?

The following two drivers:

  - gpio/gpio-omap (on the beaglebone black)
  - pinctrl/bcm/pinctrl-bcm2835 (on the raspberry pi 2).

Where pinctrl-bcm2835 has to include commit
85ae9e512f437cd09bf61564bdba29ab88bab3e3 where the driver was converted to
using the GPIOLIB_IRQCHIP helper library.

Regards
Davide

Re: [PATCH v2] hwmon/sht15: Root out platform data

[Davide Hug]

On Sat, 25 Nov 2017 10:41:26 +0100
Davide Hug <d@videhug.ch> wrote:

> Hi Estevam

Hi Fabio,

sorry for that.

> 
> On Sat, 25 Nov 2017 00:06:44 -0200
> Fabio Estevam <festevam@gmail.com> wrote:
> 
> > Hi Davide,
> > 
> > On Fri, Nov 24, 2017 at 10:03 PM, Davide Hug <d@videhug.ch> wrote:
> > 
> > > So the GPIOLIB_IRQCHIP code in gpiolib.c is enabled. But the driver
> > > gpio-mxc doesn't use it. In particular it doesn't call gpiochip_add_irqchip
> > > which assigns the .irq_request_resources/.irq_release_resources callbacks that
> > > toggle the FLAG_USED_AS_IRQ.
> > > So in the gpio-mxc driver the guard against setting a GPIO with active IRQ as
> > > output is not in place. And it is this guard that makes the sht15 driver fail
> > > to load when in place.
> > 
> > What is the gpio driver you are using that shows the error?
> 
> The following two drivers:
> 
>   - gpio/gpio-omap (on the beaglebone black)
>   - pinctrl/bcm/pinctrl-bcm2835 (on the raspberry pi 2).
> 
> Where pinctrl-bcm2835 has to include commit
> 85ae9e512f437cd09bf61564bdba29ab88bab3e3 where the driver was converted to
> using the GPIOLIB_IRQCHIP helper library.
> 
> Regards
> Davide
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] hwmon/sht15: Root out platform data

[Fabio Estevam]

Hi Davide,

On Sat, Nov 25, 2017 at 7:41 AM, Davide Hug <d@videhug.ch> wrote:

>> What is the gpio driver you are using that shows the error?
>
> The following two drivers:
>
>   - gpio/gpio-omap (on the beaglebone black)
>   - pinctrl/bcm/pinctrl-bcm2835 (on the raspberry pi 2).
>
> Where pinctrl-bcm2835 has to include commit
> 85ae9e512f437cd09bf61564bdba29ab88bab3e3 where the driver was converted to
> using the GPIOLIB_IRQCHIP helper library.

Maybe you could start a new thread in the linux-gpio mailing list to
discuss this problem on these two drivers.

Regards,

Fabio Estevam

Re: [PATCH v2] hwmon/sht15: Root out platform data

[Davide Hug]

Hi Fabio,

On Sat, 25 Nov 2017 12:04:19 -0200
Fabio Estevam <festevam@gmail.com> wrote:

> On Sat, Nov 25, 2017 at 7:41 AM, Davide Hug <d@videhug.ch> wrote:
> 
> >> What is the gpio driver you are using that shows the error?
> >
> > The following two drivers:
> >
> >   - gpio/gpio-omap (on the beaglebone black)
> >   - pinctrl/bcm/pinctrl-bcm2835 (on the raspberry pi 2).
> >
> > Where pinctrl-bcm2835 has to include commit
> > 85ae9e512f437cd09bf61564bdba29ab88bab3e3 where the driver was converted to
> > using the GPIOLIB_IRQCHIP helper library.
> 
> Maybe you could start a new thread in the linux-gpio mailing list to
> discuss this problem on these two drivers.

There is a thread in the linux-gpio mailing list on this topic[1]. There it was
suggested to me to fix the sht15 driver, so I started the thread here. But in
the meantime in the linux-gpio thread it was suggested to make gpiolib support
what the sht15 driver does.
So my plan would be to see if that works out and otherwise propose a patch for
the sht15 driver here on linux-hwmon. It was proposed to use a simple polling
loop and avoid using irq and this works really well for me.

Regards,
Davide Hug

[1]: http://www.spinics.net/lists/linux-gpio/msg24019.html

Re: [PATCH v2] hwmon/sht15: Root out platform data

[Fabio Estevam]

Hi Davide,

On Sun, Nov 26, 2017 at 8:38 PM, Davide Hug <d@videhug.ch> wrote:

> There is a thread in the linux-gpio mailing list on this topic[1]. There it was
> suggested to me to fix the sht15 driver, so I started the thread here. But in
> the meantime in the linux-gpio thread it was suggested to make gpiolib support
> what the sht15 driver does.
> So my plan would be to see if that works out and otherwise propose a patch for
> the sht15 driver here on linux-hwmon. It was proposed to use a simple polling
> loop and avoid using irq and this works really well for me.

Ok, great.

In case you post a patch, please keep myself and Marco on Cc so that
we can test it on our imx7d-sdb board.

Thanks