[PATCH v2] hwmon: (sht3x) add devicetree support

[PATCH v2] hwmon: (sht3x) add devicetree support

[Wojciech Slenska]

Signed-off-by: Wojciech Slenska <wojciech.slenska@gmail.com>
---
 Documentation/devicetree/bindings/hwmon/sht3x.txt | 16 +++++++++++++
 drivers/hwmon/sht3x.c                             | 28 ++++++++++++++++++++---
 2 files changed, 41 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/sht3x.txt

diff --git a/Documentation/devicetree/bindings/hwmon/sht3x.txt b/Documentation/devicetree/bindings/hwmon/sht3x.txt
new file mode 100644
index 0000000..80b117e
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/sht3x.txt
@@ -0,0 +1,16 @@
+Sensirion SHT3x Humidity and Temperature Sensor
+
+Required node properties:
+- compatible: "sensirion,sht3x" or "sensirion,sts3x"
+- reg: I2C bus address of the device
+
+Optional properties:
+- sensirion,blocking-io: enable blocking mode on i2c
+- sensirion,no-high-precision: disable high accuracy
+
+Example sht3x node:
+
+sensor {
+	compatible = "sensirion,sht3x";
+	reg = <0x4a>;
+}
diff --git a/drivers/hwmon/sht3x.c b/drivers/hwmon/sht3x.c
index 370b57d..1f8b7e7 100644
--- a/drivers/hwmon/sht3x.c
+++ b/drivers/hwmon/sht3x.c
@@ -31,6 +31,7 @@
 #include <linux/slab.h>
 #include <linux/jiffies.h>
 #include <linux/platform_data/sht3x.h>
+#include <linux/of.h>

 /* commands (high precision mode) */
 static const unsigned char sht3x_cmd_measure_blocking_hpm[]    = { 0x2c, 0x06 };
@@ -699,6 +700,7 @@ static int sht3x_probe(struct i2c_client *client,
 	struct i2c_adapter *adap = client->adapter;
 	struct device *dev = &client->dev;
 	const struct attribute_group **attribute_groups;
+	struct device_node *np = client->dev.of_node;

 	/*
 	 * we require full i2c support since the sht3x uses multi-byte read and
@@ -724,8 +726,16 @@ static int sht3x_probe(struct i2c_client *client,
 	data->client = client;
 	crc8_populate_msb(sht3x_crc8_table, SHT3X_CRC8_POLYNOMIAL);

-	if (client->dev.platform_data)
-		data->setup = *(struct sht3x_platform_data *)dev->platform_data;
+	if (np) {
+		if (of_property_read_bool(np, "sensirion,blocking-io"))
+			data->setup.blocking_io = true;
+		if (of_property_read_bool(np, "sensirion,no-high-precision"))
+			data->setup.high_precision = false;
+	} else {
+		if (client->dev.platform_data)
+			data->setup = *(struct sht3x_platform_data *)
+				dev->platform_data;
+	}

 	sht3x_select_command(data);

@@ -768,8 +778,20 @@ static const struct i2c_device_id sht3x_ids[] = {

 MODULE_DEVICE_TABLE(i2c, sht3x_ids);

+#ifdef CONFIG_OF
+static const struct of_device_id sht3x_dt_match[] = {
+	{ .compatible = "sensirion,sht3x" },
+	{ .compatible = "sensirion,sts3x" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, sht3x_dt_match);
+#endif
+
 static struct i2c_driver sht3x_i2c_driver = {
-	.driver.name = "sht3x",
+	.driver = {
+		.name = "sht3x",
+		.of_match_table = of_match_ptr(sht3x_dt_match),
+	},
 	.probe       = sht3x_probe,
 	.id_table    = sht3x_ids,
 };
--
2.7.4

Re: [PATCH v2] hwmon: (sht3x) add devicetree support

[Rob Herring]

On Fri, Oct 05, 2018 at 07:38:35AM +0200, Wojciech Slenska wrote:

Commit msg?

> Signed-off-by: Wojciech Slenska <wojciech.slenska@gmail.com>
> ---
>  Documentation/devicetree/bindings/hwmon/sht3x.txt | 16 +++++++++++++

Please split bindings to separate patch.

>  drivers/hwmon/sht3x.c                             | 28 ++++++++++++++++++++---
>  2 files changed, 41 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/sht3x.txt
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/sht3x.txt b/Documentation/devicetree/bindings/hwmon/sht3x.txt
> new file mode 100644
> index 0000000..80b117e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/sht3x.txt
> @@ -0,0 +1,16 @@
> +Sensirion SHT3x Humidity and Temperature Sensor
> +
> +Required node properties:
> +- compatible: "sensirion,sht3x" or "sensirion,sts3x"
> +- reg: I2C bus address of the device
> +
> +Optional properties:
> +- sensirion,blocking-io: enable blocking mode on i2c

This is not a h/w parameter and shouldn't be in DT.

> +- sensirion,no-high-precision: disable high accuracy

Maybe this one is okay, but couldn't the user want to set this? If so, 
then it should be a sysfs attr.

> +
> +Example sht3x node:
> +
> +sensor {

sensor@4a

> +	compatible = "sensirion,sht3x";
> +	reg = <0x4a>;
> +}

Re: [PATCH v2] hwmon: (sht3x) add devicetree support

[Wojciech Sleńska]

pt., 12 paź 2018 o 22:28 Rob Herring <robh@kernel.org> napisał(a):
>
> On Fri, Oct 05, 2018 at 07:38:35AM +0200, Wojciech Slenska wrote:
>
> Commit msg?
>
> > Signed-off-by: Wojciech Slenska <wojciech.slenska@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/hwmon/sht3x.txt | 16 +++++++++++++
>
> Please split bindings to separate patch.

I will do this.

>
> >  drivers/hwmon/sht3x.c                             | 28 ++++++++++++++++++++---
> >  2 files changed, 41 insertions(+), 3 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/hwmon/sht3x.txt
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/sht3x.txt b/Documentation/devicetree/bindings/hwmon/sht3x.txt
> > new file mode 100644
> > index 0000000..80b117e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/sht3x.txt
> > @@ -0,0 +1,16 @@
> > +Sensirion SHT3x Humidity and Temperature Sensor
> > +
> > +Required node properties:
> > +- compatible: "sensirion,sht3x" or "sensirion,sts3x"
> > +- reg: I2C bus address of the device
> > +
> > +Optional properties:
> > +- sensirion,blocking-io: enable blocking mode on i2c
>
> This is not a h/w parameter and shouldn't be in DT.
>
> > +- sensirion,no-high-precision: disable high accuracy
>
> Maybe this one is okay, but couldn't the user want to set this? If so,
> then it should be a sysfs attr.

Those two parameters have been just moved from
linux/include/linux/platform_data/sht3x.h
Currently, those two parameters can be set in board file, so for me
was natural to move it to dts.
Of course, I can remove it from dts completely.

>
> > +
> > +Example sht3x node:
> > +
> > +sensor {
>
> sensor@4a
>

I will fix this.

> > +     compatible = "sensirion,sht3x";
> > +     reg = <0x4a>;
> > +}
>

BR
Wojciech Slenska

Re: [PATCH v2] hwmon: (sht3x) add devicetree support

[Guenter Roeck]

On Mon, Oct 15, 2018 at 07:55:58AM +0200, Wojciech Sleńska wrote:
> pt., 12 paź 2018 o 22:28 Rob Herring <robh@kernel.org> napisał(a):
> >
> > On Fri, Oct 05, 2018 at 07:38:35AM +0200, Wojciech Slenska wrote:
> >
> > Commit msg?
> >
> > > Signed-off-by: Wojciech Slenska <wojciech.slenska@gmail.com>
> > > ---
> > >  Documentation/devicetree/bindings/hwmon/sht3x.txt | 16 +++++++++++++
> >
> > Please split bindings to separate patch.
> 
> I will do this.
> 
> >
> > >  drivers/hwmon/sht3x.c                             | 28 ++++++++++++++++++++---
> > >  2 files changed, 41 insertions(+), 3 deletions(-)
> > >  create mode 100644 Documentation/devicetree/bindings/hwmon/sht3x.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/hwmon/sht3x.txt b/Documentation/devicetree/bindings/hwmon/sht3x.txt
> > > new file mode 100644
> > > index 0000000..80b117e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/hwmon/sht3x.txt
> > > @@ -0,0 +1,16 @@
> > > +Sensirion SHT3x Humidity and Temperature Sensor
> > > +
> > > +Required node properties:
> > > +- compatible: "sensirion,sht3x" or "sensirion,sts3x"
> > > +- reg: I2C bus address of the device
> > > +
> > > +Optional properties:
> > > +- sensirion,blocking-io: enable blocking mode on i2c
> >
> > This is not a h/w parameter and shouldn't be in DT.
> >
> > > +- sensirion,no-high-precision: disable high accuracy
> >
> > Maybe this one is okay, but couldn't the user want to set this? If so,
> > then it should be a sysfs attr.
> 
> Those two parameters have been just moved from
> linux/include/linux/platform_data/sht3x.h
> Currently, those two parameters can be set in board file, so for me
> was natural to move it to dts.
> Of course, I can remove it from dts completely.
> 

I wonder if there is an authoritative document explaining the current policy
in respect to devicetree properties. Sometimes I hear that platform
configuration parameters are now permitted, sometimes I hear that we are
back to hardware description only.

Guenter

> >
> > > +
> > > +Example sht3x node:
> > > +
> > > +sensor {
> >
> > sensor@4a
> >
> 
> I will fix this.
> 
> > > +     compatible = "sensirion,sht3x";
> > > +     reg = <0x4a>;
> > > +}
> >
> 
> BR
> Wojciech Slenska

Re: [PATCH v2] hwmon: (sht3x) add devicetree support

[Rob Herring]

On Mon, Oct 15, 2018 at 2:54 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Mon, Oct 15, 2018 at 07:55:58AM +0200, Wojciech Sleńska wrote:
> > pt., 12 paź 2018 o 22:28 Rob Herring <robh@kernel.org> napisał(a):
> > >
> > > On Fri, Oct 05, 2018 at 07:38:35AM +0200, Wojciech Slenska wrote:
> > >
> > > Commit msg?
> > >
> > > > Signed-off-by: Wojciech Slenska <wojciech.slenska@gmail.com>
> > > > ---
> > > >  Documentation/devicetree/bindings/hwmon/sht3x.txt | 16 +++++++++++++
> > >
> > > Please split bindings to separate patch.
> >
> > I will do this.
> >
> > >
> > > >  drivers/hwmon/sht3x.c                             | 28 ++++++++++++++++++++---
> > > >  2 files changed, 41 insertions(+), 3 deletions(-)
> > > >  create mode 100644 Documentation/devicetree/bindings/hwmon/sht3x.txt
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/hwmon/sht3x.txt b/Documentation/devicetree/bindings/hwmon/sht3x.txt
> > > > new file mode 100644
> > > > index 0000000..80b117e
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/hwmon/sht3x.txt
> > > > @@ -0,0 +1,16 @@
> > > > +Sensirion SHT3x Humidity and Temperature Sensor
> > > > +
> > > > +Required node properties:
> > > > +- compatible: "sensirion,sht3x" or "sensirion,sts3x"
> > > > +- reg: I2C bus address of the device
> > > > +
> > > > +Optional properties:
> > > > +- sensirion,blocking-io: enable blocking mode on i2c
> > >
> > > This is not a h/w parameter and shouldn't be in DT.
> > >
> > > > +- sensirion,no-high-precision: disable high accuracy
> > >
> > > Maybe this one is okay, but couldn't the user want to set this? If so,
> > > then it should be a sysfs attr.
> >
> > Those two parameters have been just moved from
> > linux/include/linux/platform_data/sht3x.h
> > Currently, those two parameters can be set in board file, so for me
> > was natural to move it to dts.
> > Of course, I can remove it from dts completely.
> >
>
> I wonder if there is an authoritative document explaining the current policy
> in respect to devicetree properties.

No. In the end it is often a judgement call.

> Sometimes I hear that platform
> configuration parameters are now permitted, sometimes I hear that we are
> back to hardware description only.

Yes, configuration is permitted, but there are some constraints. The
questions I ask are is it OS specific or specific to current
implementation of an OS, who or what decides what the setting is? The
last question results in lots of things dropped in favor of a sysfs
control.

Rob

Re: [PATCH v2] hwmon: (sht3x) add devicetree support

[Guenter Roeck]

On Tue, Oct 16, 2018 at 01:02:08PM -0500, Rob Herring wrote:
> On Mon, Oct 15, 2018 at 2:54 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On Mon, Oct 15, 2018 at 07:55:58AM +0200, Wojciech Sleńska wrote:
> > > pt., 12 paź 2018 o 22:28 Rob Herring <robh@kernel.org> napisał(a):
> > > >
> > > > On Fri, Oct 05, 2018 at 07:38:35AM +0200, Wojciech Slenska wrote:
> > > >
> > > > Commit msg?
> > > >
> > > > > Signed-off-by: Wojciech Slenska <wojciech.slenska@gmail.com>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/hwmon/sht3x.txt | 16 +++++++++++++
> > > >
> > > > Please split bindings to separate patch.
> > >
> > > I will do this.
> > >
> > > >
> > > > >  drivers/hwmon/sht3x.c                             | 28 ++++++++++++++++++++---
> > > > >  2 files changed, 41 insertions(+), 3 deletions(-)
> > > > >  create mode 100644 Documentation/devicetree/bindings/hwmon/sht3x.txt
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/hwmon/sht3x.txt b/Documentation/devicetree/bindings/hwmon/sht3x.txt
> > > > > new file mode 100644
> > > > > index 0000000..80b117e
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/hwmon/sht3x.txt
> > > > > @@ -0,0 +1,16 @@
> > > > > +Sensirion SHT3x Humidity and Temperature Sensor
> > > > > +
> > > > > +Required node properties:
> > > > > +- compatible: "sensirion,sht3x" or "sensirion,sts3x"
> > > > > +- reg: I2C bus address of the device
> > > > > +
> > > > > +Optional properties:
> > > > > +- sensirion,blocking-io: enable blocking mode on i2c
> > > >
> > > > This is not a h/w parameter and shouldn't be in DT.
> > > >
> > > > > +- sensirion,no-high-precision: disable high accuracy
> > > >
> > > > Maybe this one is okay, but couldn't the user want to set this? If so,
> > > > then it should be a sysfs attr.
> > >
> > > Those two parameters have been just moved from
> > > linux/include/linux/platform_data/sht3x.h
> > > Currently, those two parameters can be set in board file, so for me
> > > was natural to move it to dts.
> > > Of course, I can remove it from dts completely.
> > >
> >
> > I wonder if there is an authoritative document explaining the current policy
> > in respect to devicetree properties.
> 
> No. In the end it is often a judgement call.
> 
> > Sometimes I hear that platform
> > configuration parameters are now permitted, sometimes I hear that we are
> > back to hardware description only.
> 
> Yes, configuration is permitted, but there are some constraints. The
> questions I ask are is it OS specific or specific to current
> implementation of an OS, who or what decides what the setting is? The
> last question results in lots of things dropped in favor of a sysfs
> control.
> 
In this driver, both parameters were considered system / platform parameters.
One controls if the chip uses i2c clock stretching or if the system has to poll
for results. This is determined by the system's i2c controller and may also be
influenced by considerations such as if there is another chip on the same bus.
The other parameter controls accuracy which directly translates into power
consumption. While there could be applications where both parameters are
controlled by user space, that would be the exception. In the expected use
case, ie for hardware monitoring, user space control would neither be feasible
nor desirable. As such, sysfs controls for the parameters don't even exist.

I don't mind if you reject adding those dt properties for now, but I would
resist adding sysfs attributes unless someone provides me with an actual
and specific use case.

Thanks,
Guenter

Re: [PATCH v2] hwmon: (sht3x) add devicetree support

[Rob Herring]

On Tue, Oct 16, 2018 at 6:38 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Tue, Oct 16, 2018 at 01:02:08PM -0500, Rob Herring wrote:
> > On Mon, Oct 15, 2018 at 2:54 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> > > On Mon, Oct 15, 2018 at 07:55:58AM +0200, Wojciech Sleńska wrote:
> > > > pt., 12 paź 2018 o 22:28 Rob Herring <robh@kernel.org> napisał(a):
> > > > >
> > > > > On Fri, Oct 05, 2018 at 07:38:35AM +0200, Wojciech Slenska wrote:
> > > > >
> > > > > Commit msg?
> > > > >
> > > > > > Signed-off-by: Wojciech Slenska <wojciech.slenska@gmail.com>
> > > > > > ---
> > > > > >  Documentation/devicetree/bindings/hwmon/sht3x.txt | 16 +++++++++++++
> > > > >
> > > > > Please split bindings to separate patch.
> > > >
> > > > I will do this.
> > > >
> > > > >
> > > > > >  drivers/hwmon/sht3x.c                             | 28 ++++++++++++++++++++---
> > > > > >  2 files changed, 41 insertions(+), 3 deletions(-)
> > > > > >  create mode 100644 Documentation/devicetree/bindings/hwmon/sht3x.txt
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/hwmon/sht3x.txt b/Documentation/devicetree/bindings/hwmon/sht3x.txt
> > > > > > new file mode 100644
> > > > > > index 0000000..80b117e
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/hwmon/sht3x.txt
> > > > > > @@ -0,0 +1,16 @@
> > > > > > +Sensirion SHT3x Humidity and Temperature Sensor
> > > > > > +
> > > > > > +Required node properties:
> > > > > > +- compatible: "sensirion,sht3x" or "sensirion,sts3x"
> > > > > > +- reg: I2C bus address of the device
> > > > > > +
> > > > > > +Optional properties:
> > > > > > +- sensirion,blocking-io: enable blocking mode on i2c
> > > > >
> > > > > This is not a h/w parameter and shouldn't be in DT.
> > > > >
> > > > > > +- sensirion,no-high-precision: disable high accuracy
> > > > >
> > > > > Maybe this one is okay, but couldn't the user want to set this? If so,
> > > > > then it should be a sysfs attr.
> > > >
> > > > Those two parameters have been just moved from
> > > > linux/include/linux/platform_data/sht3x.h
> > > > Currently, those two parameters can be set in board file, so for me
> > > > was natural to move it to dts.
> > > > Of course, I can remove it from dts completely.
> > > >
> > >
> > > I wonder if there is an authoritative document explaining the current policy
> > > in respect to devicetree properties.
> >
> > No. In the end it is often a judgement call.
> >
> > > Sometimes I hear that platform
> > > configuration parameters are now permitted, sometimes I hear that we are
> > > back to hardware description only.
> >
> > Yes, configuration is permitted, but there are some constraints. The
> > questions I ask are is it OS specific or specific to current
> > implementation of an OS, who or what decides what the setting is? The
> > last question results in lots of things dropped in favor of a sysfs
> > control.
> >
> In this driver, both parameters were considered system / platform parameters.
> One controls if the chip uses i2c clock stretching or if the system has to poll
> for results. This is determined by the system's i2c controller and may also be
> influenced by considerations such as if there is another chip on the same bus.

Okay, makes sense. As it is named, it sounded like picking a s/w API
to use (sync vs. async). So please improve the description based on
Guenter's explanation.

> The other parameter controls accuracy which directly translates into power
> consumption. While there could be applications where both parameters are
> controlled by user space, that would be the exception. In the expected use
> case, ie for hardware monitoring, user space control would neither be feasible
> nor desirable. As such, sysfs controls for the parameters don't even exist.

I could imagine you may want to switch modes based on battery powered
vs. plugged in. But that's not to say you couldn't want it one way or
the other for a system.

> I don't mind if you reject adding those dt properties for now, but I would
> resist adding sysfs attributes unless someone provides me with an actual
> and specific use case.

I'm not rejecting them. My next question though is should these be
common properties?

Rob

Re: [PATCH v2] hwmon: (sht3x) add devicetree support

[Guenter Roeck]

On Thu, Oct 18, 2018 at 08:47:43AM -0500, Rob Herring wrote:
> On Tue, Oct 16, 2018 at 6:38 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On Tue, Oct 16, 2018 at 01:02:08PM -0500, Rob Herring wrote:
> > > On Mon, Oct 15, 2018 at 2:54 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > > >
> > > > On Mon, Oct 15, 2018 at 07:55:58AM +0200, Wojciech Sleńska wrote:
> > > > > pt., 12 paź 2018 o 22:28 Rob Herring <robh@kernel.org> napisał(a):
> > > > > >
> > > > > > On Fri, Oct 05, 2018 at 07:38:35AM +0200, Wojciech Slenska wrote:
> > > > > >
> > > > > > Commit msg?
> > > > > >
> > > > > > > Signed-off-by: Wojciech Slenska <wojciech.slenska@gmail.com>
> > > > > > > ---
> > > > > > >  Documentation/devicetree/bindings/hwmon/sht3x.txt | 16 +++++++++++++
> > > > > >
> > > > > > Please split bindings to separate patch.
> > > > >
> > > > > I will do this.
> > > > >
> > > > > >
> > > > > > >  drivers/hwmon/sht3x.c                             | 28 ++++++++++++++++++++---
> > > > > > >  2 files changed, 41 insertions(+), 3 deletions(-)
> > > > > > >  create mode 100644 Documentation/devicetree/bindings/hwmon/sht3x.txt
> > > > > > >
> > > > > > > diff --git a/Documentation/devicetree/bindings/hwmon/sht3x.txt b/Documentation/devicetree/bindings/hwmon/sht3x.txt
> > > > > > > new file mode 100644
> > > > > > > index 0000000..80b117e
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/devicetree/bindings/hwmon/sht3x.txt
> > > > > > > @@ -0,0 +1,16 @@
> > > > > > > +Sensirion SHT3x Humidity and Temperature Sensor
> > > > > > > +
> > > > > > > +Required node properties:
> > > > > > > +- compatible: "sensirion,sht3x" or "sensirion,sts3x"
> > > > > > > +- reg: I2C bus address of the device
> > > > > > > +
> > > > > > > +Optional properties:
> > > > > > > +- sensirion,blocking-io: enable blocking mode on i2c
> > > > > >
> > > > > > This is not a h/w parameter and shouldn't be in DT.
> > > > > >
> > > > > > > +- sensirion,no-high-precision: disable high accuracy
> > > > > >
> > > > > > Maybe this one is okay, but couldn't the user want to set this? If so,
> > > > > > then it should be a sysfs attr.
> > > > >
> > > > > Those two parameters have been just moved from
> > > > > linux/include/linux/platform_data/sht3x.h
> > > > > Currently, those two parameters can be set in board file, so for me
> > > > > was natural to move it to dts.
> > > > > Of course, I can remove it from dts completely.
> > > > >
> > > >
> > > > I wonder if there is an authoritative document explaining the current policy
> > > > in respect to devicetree properties.
> > >
> > > No. In the end it is often a judgement call.
> > >
> > > > Sometimes I hear that platform
> > > > configuration parameters are now permitted, sometimes I hear that we are
> > > > back to hardware description only.
> > >
> > > Yes, configuration is permitted, but there are some constraints. The
> > > questions I ask are is it OS specific or specific to current
> > > implementation of an OS, who or what decides what the setting is? The
> > > last question results in lots of things dropped in favor of a sysfs
> > > control.
> > >
> > In this driver, both parameters were considered system / platform parameters.
> > One controls if the chip uses i2c clock stretching or if the system has to poll
> > for results. This is determined by the system's i2c controller and may also be
> > influenced by considerations such as if there is another chip on the same bus.
> 
> Okay, makes sense. As it is named, it sounded like picking a s/w API
> to use (sync vs. async). So please improve the description based on
> Guenter's explanation.
> 
> > The other parameter controls accuracy which directly translates into power
> > consumption. While there could be applications where both parameters are
> > controlled by user space, that would be the exception. In the expected use
> > case, ie for hardware monitoring, user space control would neither be feasible
> > nor desirable. As such, sysfs controls for the parameters don't even exist.
> 
> I could imagine you may want to switch modes based on battery powered
> vs. plugged in. But that's not to say you couldn't want it one way or
> the other for a system.
> 
> > I don't mind if you reject adding those dt properties for now, but I would
> > resist adding sysfs attributes unless someone provides me with an actual
> > and specific use case.
> 
> I'm not rejecting them. My next question though is should these be
> common properties?

No, they are chip specific.

Guenter

Re: [PATCH v2] hwmon: (sht3x) add devicetree support

[Rob Herring]

Adding Wolfram and i2c list...

On Thu, Oct 18, 2018 at 9:41 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Thu, Oct 18, 2018 at 08:47:43AM -0500, Rob Herring wrote:
> > On Tue, Oct 16, 2018 at 6:38 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> > > On Tue, Oct 16, 2018 at 01:02:08PM -0500, Rob Herring wrote:
> > > > On Mon, Oct 15, 2018 at 2:54 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > > > >
> > > > > On Mon, Oct 15, 2018 at 07:55:58AM +0200, Wojciech Sleńska wrote:
> > > > > > pt., 12 paź 2018 o 22:28 Rob Herring <robh@kernel.org> napisał(a):
> > > > > > >
> > > > > > > On Fri, Oct 05, 2018 at 07:38:35AM +0200, Wojciech Slenska wrote:
> > > > > > >
> > > > > > > Commit msg?
> > > > > > >
> > > > > > > > Signed-off-by: Wojciech Slenska <wojciech.slenska@gmail.com>
> > > > > > > > ---
> > > > > > > >  Documentation/devicetree/bindings/hwmon/sht3x.txt | 16 +++++++++++++
> > > > > > >
> > > > > > > Please split bindings to separate patch.
> > > > > >
> > > > > > I will do this.
> > > > > >
> > > > > > >
> > > > > > > >  drivers/hwmon/sht3x.c                             | 28 ++++++++++++++++++++---
> > > > > > > >  2 files changed, 41 insertions(+), 3 deletions(-)
> > > > > > > >  create mode 100644 Documentation/devicetree/bindings/hwmon/sht3x.txt
> > > > > > > >
> > > > > > > > diff --git a/Documentation/devicetree/bindings/hwmon/sht3x.txt b/Documentation/devicetree/bindings/hwmon/sht3x.txt
> > > > > > > > new file mode 100644
> > > > > > > > index 0000000..80b117e
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/Documentation/devicetree/bindings/hwmon/sht3x.txt
> > > > > > > > @@ -0,0 +1,16 @@
> > > > > > > > +Sensirion SHT3x Humidity and Temperature Sensor
> > > > > > > > +
> > > > > > > > +Required node properties:
> > > > > > > > +- compatible: "sensirion,sht3x" or "sensirion,sts3x"
> > > > > > > > +- reg: I2C bus address of the device
> > > > > > > > +
> > > > > > > > +Optional properties:
> > > > > > > > +- sensirion,blocking-io: enable blocking mode on i2c
> > > > > > >
> > > > > > > This is not a h/w parameter and shouldn't be in DT.
> > > > > > >
> > > > > > > > +- sensirion,no-high-precision: disable high accuracy
> > > > > > >
> > > > > > > Maybe this one is okay, but couldn't the user want to set this? If so,
> > > > > > > then it should be a sysfs attr.
> > > > > >
> > > > > > Those two parameters have been just moved from
> > > > > > linux/include/linux/platform_data/sht3x.h
> > > > > > Currently, those two parameters can be set in board file, so for me
> > > > > > was natural to move it to dts.
> > > > > > Of course, I can remove it from dts completely.
> > > > > >
> > > > >
> > > > > I wonder if there is an authoritative document explaining the current policy
> > > > > in respect to devicetree properties.
> > > >
> > > > No. In the end it is often a judgement call.
> > > >
> > > > > Sometimes I hear that platform
> > > > > configuration parameters are now permitted, sometimes I hear that we are
> > > > > back to hardware description only.
> > > >
> > > > Yes, configuration is permitted, but there are some constraints. The
> > > > questions I ask are is it OS specific or specific to current
> > > > implementation of an OS, who or what decides what the setting is? The
> > > > last question results in lots of things dropped in favor of a sysfs
> > > > control.
> > > >
> > > In this driver, both parameters were considered system / platform parameters.
> > > One controls if the chip uses i2c clock stretching or if the system has to poll
> > > for results. This is determined by the system's i2c controller and may also be
> > > influenced by considerations such as if there is another chip on the same bus.
> >
> > Okay, makes sense. As it is named, it sounded like picking a s/w API
> > to use (sync vs. async). So please improve the description based on
> > Guenter's explanation.
> >
> > > The other parameter controls accuracy which directly translates into power
> > > consumption. While there could be applications where both parameters are
> > > controlled by user space, that would be the exception. In the expected use
> > > case, ie for hardware monitoring, user space control would neither be feasible
> > > nor desirable. As such, sysfs controls for the parameters don't even exist.
> >
> > I could imagine you may want to switch modes based on battery powered
> > vs. plugged in. But that's not to say you couldn't want it one way or
> > the other for a system.
> >
> > > I don't mind if you reject adding those dt properties for now, but I would
> > > resist adding sysfs attributes unless someone provides me with an actual
> > > and specific use case.
> >
> > I'm not rejecting them. My next question though is should these be
> > common properties?
>
> No, they are chip specific.

Really?

Trading off accuracy vs. sampling frequency is very much common for
ADCs. Not exactly the same thing here, but surely there's other cases
out there.

And here's another device with programmable clock stretching[1]. Lots
of discussions in searching about masters not supporting clock
stretching more than I found of slaves which are configurable. At
least in that case, we should be able to derive it from master
compatible strings. But for the cases where both ends can support
stretching or not, seems like we need an i2c property.

Rob

[1] https://www.infineon.com/dgdl/Infineon-Infineon-TLE493D-A2B6-UM-v01_00-UM-v01_00-EN.pdf?fileId=5546d46262b31d2e01633b4bba5f3c5a

>
> Guenter

Re: [PATCH v2] hwmon: (sht3x) add devicetree support

[Guenter Roeck]

On Thu, Oct 18, 2018 at 11:06:52AM -0500, Rob Herring wrote:
> Adding Wolfram and i2c list...
> 
> On Thu, Oct 18, 2018 at 9:41 AM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On Thu, Oct 18, 2018 at 08:47:43AM -0500, Rob Herring wrote:
> > > On Tue, Oct 16, 2018 at 6:38 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > > >
> > > > On Tue, Oct 16, 2018 at 01:02:08PM -0500, Rob Herring wrote:
> > > > > On Mon, Oct 15, 2018 at 2:54 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > > > > >
> > > > > > On Mon, Oct 15, 2018 at 07:55:58AM +0200, Wojciech Sleńska wrote:
> > > > > > > pt., 12 paź 2018 o 22:28 Rob Herring <robh@kernel.org> napisał(a):
> > > > > > > >
> > > > > > > > On Fri, Oct 05, 2018 at 07:38:35AM +0200, Wojciech Slenska wrote:
> > > > > > > >
> > > > > > > > Commit msg?
> > > > > > > >
> > > > > > > > > Signed-off-by: Wojciech Slenska <wojciech.slenska@gmail.com>
> > > > > > > > > ---
> > > > > > > > >  Documentation/devicetree/bindings/hwmon/sht3x.txt | 16 +++++++++++++
> > > > > > > >
> > > > > > > > Please split bindings to separate patch.
> > > > > > >
> > > > > > > I will do this.
> > > > > > >
> > > > > > > >
> > > > > > > > >  drivers/hwmon/sht3x.c                             | 28 ++++++++++++++++++++---
> > > > > > > > >  2 files changed, 41 insertions(+), 3 deletions(-)
> > > > > > > > >  create mode 100644 Documentation/devicetree/bindings/hwmon/sht3x.txt
> > > > > > > > >
> > > > > > > > > diff --git a/Documentation/devicetree/bindings/hwmon/sht3x.txt b/Documentation/devicetree/bindings/hwmon/sht3x.txt
> > > > > > > > > new file mode 100644
> > > > > > > > > index 0000000..80b117e
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/Documentation/devicetree/bindings/hwmon/sht3x.txt
> > > > > > > > > @@ -0,0 +1,16 @@
> > > > > > > > > +Sensirion SHT3x Humidity and Temperature Sensor
> > > > > > > > > +
> > > > > > > > > +Required node properties:
> > > > > > > > > +- compatible: "sensirion,sht3x" or "sensirion,sts3x"
> > > > > > > > > +- reg: I2C bus address of the device
> > > > > > > > > +
> > > > > > > > > +Optional properties:
> > > > > > > > > +- sensirion,blocking-io: enable blocking mode on i2c
> > > > > > > >
> > > > > > > > This is not a h/w parameter and shouldn't be in DT.
> > > > > > > >
> > > > > > > > > +- sensirion,no-high-precision: disable high accuracy
> > > > > > > >
> > > > > > > > Maybe this one is okay, but couldn't the user want to set this? If so,
> > > > > > > > then it should be a sysfs attr.
> > > > > > >
> > > > > > > Those two parameters have been just moved from
> > > > > > > linux/include/linux/platform_data/sht3x.h
> > > > > > > Currently, those two parameters can be set in board file, so for me
> > > > > > > was natural to move it to dts.
> > > > > > > Of course, I can remove it from dts completely.
> > > > > > >
> > > > > >
> > > > > > I wonder if there is an authoritative document explaining the current policy
> > > > > > in respect to devicetree properties.
> > > > >
> > > > > No. In the end it is often a judgement call.
> > > > >
> > > > > > Sometimes I hear that platform
> > > > > > configuration parameters are now permitted, sometimes I hear that we are
> > > > > > back to hardware description only.
> > > > >
> > > > > Yes, configuration is permitted, but there are some constraints. The
> > > > > questions I ask are is it OS specific or specific to current
> > > > > implementation of an OS, who or what decides what the setting is? The
> > > > > last question results in lots of things dropped in favor of a sysfs
> > > > > control.
> > > > >
> > > > In this driver, both parameters were considered system / platform parameters.
> > > > One controls if the chip uses i2c clock stretching or if the system has to poll
> > > > for results. This is determined by the system's i2c controller and may also be
> > > > influenced by considerations such as if there is another chip on the same bus.
> > >
> > > Okay, makes sense. As it is named, it sounded like picking a s/w API
> > > to use (sync vs. async). So please improve the description based on
> > > Guenter's explanation.
> > >
> > > > The other parameter controls accuracy which directly translates into power
> > > > consumption. While there could be applications where both parameters are
> > > > controlled by user space, that would be the exception. In the expected use
> > > > case, ie for hardware monitoring, user space control would neither be feasible
> > > > nor desirable. As such, sysfs controls for the parameters don't even exist.
> > >
> > > I could imagine you may want to switch modes based on battery powered
> > > vs. plugged in. But that's not to say you couldn't want it one way or
> > > the other for a system.
> > >
> > > > I don't mind if you reject adding those dt properties for now, but I would
> > > > resist adding sysfs attributes unless someone provides me with an actual
> > > > and specific use case.
> > >
> > > I'm not rejecting them. My next question though is should these be
> > > common properties?
> >
> > No, they are chip specific.
> 
> Really?
> 
> Trading off accuracy vs. sampling frequency is very much common for
> ADCs. Not exactly the same thing here, but surely there's other cases
> out there.
> 

The means are different, though. The most common means to control accuracy
(indirectly) and power consumption is through the ADC update interval.
We commonly use that in hwmon drivers. Another possible common attribute/
property would the the accuracy as explicit number (9/10/11/12 bit,
or in degrees C for thermal sensors). sht3x lets users set the update
interval and the accuracy (called repeatability) separately. Repeatability
is defined as "high, medium, low", which translates into 0.15% RH for high
repeatability to 0.25% RH for low repeatability. Not sure how to translate
that into a common property which would apply to multiple chips.

> And here's another device with programmable clock stretching[1]. Lots
> of discussions in searching about masters not supporting clock
> stretching more than I found of slaves which are configurable. At
> least in that case, we should be able to derive it from master
> compatible strings. But for the cases where both ends can support
> stretching or not, seems like we need an i2c property.
> 
Good point, and makes sense.

Guenter

> Rob
> 
> [1] https://www.infineon.com/dgdl/Infineon-Infineon-TLE493D-A2B6-UM-v01_00-UM-v01_00-EN.pdf?fileId=5546d46262b31d2e01633b4bba5f3c5a
> 
> >
> > Guenter

Re: [PATCH v2] hwmon: (sht3x) add devicetree support

[Wolfram Sang]

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

Hi,

> And here's another device with programmable clock stretching[1]. Lots
> of discussions in searching about masters not supporting clock
> stretching more than I found of slaves which are configurable. At
> least in that case, we should be able to derive it from master
> compatible strings. But for the cases where both ends can support
> stretching or not, seems like we need an i2c property.

I haven't read all this, but regarding clock stretching, this is the
current state:

* clock streching is defined in the I2C specs and thus masters are
  assumed to handle it properly
* if they can't, this is a quirk and we have a flag for it:
  I2C_AQ_NO_CLK_STRETCH
* client drivers can check for quirks and act accordingly:
  i2c_check_quirks(struct i2c_adapter *adap, u64 quirks)

Note: there is currently no user of this feature because mainlining the
client got stuck for some reason. So, I'd be happy if all this is useful
to you.

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]