[RFC 1/2] devicetree: add lm90 thermal_zone sensor support

[RFC 1/2] devicetree: add lm90 thermal_zone sensor support

[Christian Lamparter]

This patch updates the LM90's devicetree definition to
include the #thermal-sensor-cells property as well as
the sensor constants in include/dt-bindings/thermal/lm90.h.

Cc: Wei Ni <wni@nvidia.com>
Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
I'm aware that there was atleast one previous attempt to add
thermal_zone temperature sensors for the LM90 module. This was
discussed on:
<http://www.gossamer-threads.com/lists/linux/kernel/1992853>
<https://lkml.org/lkml/2014/3/4/194>

This RFC is meant to get it going again. As I would really
like to have this functionality for the Netgear WNDR4700.
This router uses a G781 to measure the SoCs temperature
in order to regulate a TC654 fan-controller.
<https://git.lede-project.org/?p=source.git;a=commit;h=9e0fd1b52ad1f805a308bf6a5a13236f352fd962>
---
 Documentation/devicetree/bindings/hwmon/lm90.txt |  6 ++++++
 MAINTAINERS                                      |  1 +
 include/dt-bindings/thermal/lm90.h               | 12 ++++++++++++
 3 files changed, 19 insertions(+)
 create mode 100644 include/dt-bindings/thermal/lm90.h

diff --git a/Documentation/devicetree/bindings/hwmon/lm90.txt b/Documentation/devicetree/bindings/hwmon/lm90.txt
index e8632486b9ef..97581266e329 100644
--- a/Documentation/devicetree/bindings/hwmon/lm90.txt
+++ b/Documentation/devicetree/bindings/hwmon/lm90.txt
@@ -33,6 +33,11 @@ Optional properties:
               LM90 "-ALERT" pin output.
               See interrupt-controller/interrupts.txt for the format.
 
+- #thermal-sensor-cells: should be set to 1. See thermal/thermal.txt for
+	      details. See <include/dt-bindings/thermal/lm90.h> for the
+	      definition of the local, remote and 2nd remote sensor index
+	      constants.
+
 Example LM90 node:
 
 temp-sensor {
@@ -41,4 +46,5 @@ temp-sensor {
 	vcc-supply = <&palmas_ldo6_reg>;
 	interrupt-parent = <&gpio>;
 	interrupts = <TEGRA_GPIO(O, 4) IRQ_TYPE_LEVEL_LOW>;
+	#thermal-sensor-cells = <1>;
 }
diff --git a/MAINTAINERS b/MAINTAINERS
index 5e637e2b3ff9..8c2ea8a18064 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7541,6 +7541,7 @@ S:	Maintained
 F:	Documentation/hwmon/lm90
 F:	Documentation/devicetree/bindings/hwmon/lm90.txt
 F:	drivers/hwmon/lm90.c
+F:	include/dt-bindings/thermal/lm90.h
 
 LM95234 HARDWARE MONITOR DRIVER
 M:	Guenter Roeck <linux@roeck-us.net>
diff --git a/include/dt-bindings/thermal/lm90.h b/include/dt-bindings/thermal/lm90.h
new file mode 100644
index 000000000000..39d90f3e63ee
--- /dev/null
+++ b/include/dt-bindings/thermal/lm90.h
@@ -0,0 +1,12 @@
+/*
+ * This header provides constants for the LM90 thermal bindings.
+ */
+
+#ifndef _DT_BINDINGS_THERMAL_LM90_H_
+#define _DT_BINDINGS_THERMAL_LM90_H_
+
+#define LM90_REMOTE_TEMPERATURE 0
+#define LM90_LOCAL_TEMPERATURE 1
+#define LM90_REMOTE2_TEMPERATURE 2
+
+#endif
-- 
2.11.0

[RFC 1/2] devicetree: add lm90 thermal_zone sensor support

[Christian Lamparter]

This patch updates the LM90's devicetree definition to
include the #thermal-sensor-cells property as well as
the sensor constants in include/dt-bindings/thermal/lm90.h.

Cc: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Christian Lamparter <chunkeey-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
---
I'm aware that there was atleast one previous attempt to add
thermal_zone temperature sensors for the LM90 module. This was
discussed on:
<http://www.gossamer-threads.com/lists/linux/kernel/1992853>
<https://lkml.org/lkml/2014/3/4/194>

This RFC is meant to get it going again. As I would really
like to have this functionality for the Netgear WNDR4700.
This router uses a G781 to measure the SoCs temperature
in order to regulate a TC654 fan-controller.
<https://git.lede-project.org/?p=source.git;a=commit;h=9e0fd1b52ad1f805a308bf6a5a13236f352fd962>
---
 Documentation/devicetree/bindings/hwmon/lm90.txt |  6 ++++++
 MAINTAINERS                                      |  1 +
 include/dt-bindings/thermal/lm90.h               | 12 ++++++++++++
 3 files changed, 19 insertions(+)
 create mode 100644 include/dt-bindings/thermal/lm90.h

diff --git a/Documentation/devicetree/bindings/hwmon/lm90.txt b/Documentation/devicetree/bindings/hwmon/lm90.txt
index e8632486b9ef..97581266e329 100644
--- a/Documentation/devicetree/bindings/hwmon/lm90.txt
+++ b/Documentation/devicetree/bindings/hwmon/lm90.txt
@@ -33,6 +33,11 @@ Optional properties:
               LM90 "-ALERT" pin output.
               See interrupt-controller/interrupts.txt for the format.
 
+- #thermal-sensor-cells: should be set to 1. See thermal/thermal.txt for
+	      details. See <include/dt-bindings/thermal/lm90.h> for the
+	      definition of the local, remote and 2nd remote sensor index
+	      constants.
+
 Example LM90 node:
 
 temp-sensor {
@@ -41,4 +46,5 @@ temp-sensor {
 	vcc-supply = <&palmas_ldo6_reg>;
 	interrupt-parent = <&gpio>;
 	interrupts = <TEGRA_GPIO(O, 4) IRQ_TYPE_LEVEL_LOW>;
+	#thermal-sensor-cells = <1>;
 }
diff --git a/MAINTAINERS b/MAINTAINERS
index 5e637e2b3ff9..8c2ea8a18064 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7541,6 +7541,7 @@ S:	Maintained
 F:	Documentation/hwmon/lm90
 F:	Documentation/devicetree/bindings/hwmon/lm90.txt
 F:	drivers/hwmon/lm90.c
+F:	include/dt-bindings/thermal/lm90.h
 
 LM95234 HARDWARE MONITOR DRIVER
 M:	Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
diff --git a/include/dt-bindings/thermal/lm90.h b/include/dt-bindings/thermal/lm90.h
new file mode 100644
index 000000000000..39d90f3e63ee
--- /dev/null
+++ b/include/dt-bindings/thermal/lm90.h
@@ -0,0 +1,12 @@
+/*
+ * This header provides constants for the LM90 thermal bindings.
+ */
+
+#ifndef _DT_BINDINGS_THERMAL_LM90_H_
+#define _DT_BINDINGS_THERMAL_LM90_H_
+
+#define LM90_REMOTE_TEMPERATURE 0
+#define LM90_LOCAL_TEMPERATURE 1
+#define LM90_REMOTE2_TEMPERATURE 2
+
+#endif
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 2/2] hwmon: lm90: add thermal_zone temperature sensor support

[Christian Lamparter]

This patch adds thermal_zone temperature sensor support
to the lm90 module. The feature has to be enabled
separately via the Kconfig option:
	CONFIG_SENSORS_LM90_THERMAL_DEVICE

The LM90 supports two (three for MAX6695 and MAX6696)
temperature sensors. The local sensor is integrated
into the LM90 chip. The remote sensors are connected
to external temperature sensing diodes.

Cc: Wei Ni <wni@nvidia.com>
Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
---
 drivers/hwmon/Kconfig | 11 +++++++
 drivers/hwmon/lm90.c  | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 102 insertions(+)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 190d270b20a2..9df70ad21a21 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1085,6 +1085,17 @@ config SENSORS_LM90
 	  This driver can also be built as a module.  If so, the module
 	  will be called lm90.
 
+config SENSORS_LM90_THERMAL_DEVICE
+	bool "Support for thermal-zone temperature sensors"
+	depends on SENSORS_LM90 && THERMAL_OF
+	help
+	  If you say yes here you get support for thermal-zone temperature
+	  sensors for the lm90 module and all supported chips.
+
+	  Refer to Documentation/devicetree/bindings/thermal/thermal.txt
+	  and Documentation/devicetree/bindings/hwmon/lm90.txt on how to
+	  use it for your platform.
+
 config SENSORS_LM92
 	tristate "National Semiconductor LM92 and compatibles"
 	depends on I2C
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 841f2428e84a..584550c7336d 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -95,6 +95,9 @@
 #include <linux/sysfs.h>
 #include <linux/interrupt.h>
 #include <linux/regulator/consumer.h>
+#include <linux/of.h>
+#include <linux/thermal.h>
+#include <dt-bindings/thermal/lm90.h>
 
 /*
  * Addresses to scan
@@ -1643,6 +1646,90 @@ static const struct hwmon_ops lm90_ops = {
 	.write = lm90_write,
 };
 
+#ifdef CONFIG_SENSORS_LM90_THERMAL_DEVICE
+
+static int lm90_read_local_temp(void *data, int *temp)
+{
+	*temp = lm90_get_temp11(data, LOCAL_TEMP);
+	return 0;
+}
+
+static int lm90_read_remote_temp(void *data, int *temp)
+{
+	*temp = lm90_get_temp11(data, REMOTE_TEMP);
+	return 0;
+}
+
+static int lm90_read_remote2_temp(void *data, int *temp)
+{
+	*temp = lm90_get_temp11(data, REMOTE2_TEMP);
+	return 0;
+}
+
+static const struct thermal_zone_of_device_ops local_temp_sensor = {
+	.get_temp = lm90_read_local_temp,
+};
+
+static const struct thermal_zone_of_device_ops remote_temp_sensor = {
+	.get_temp = lm90_read_remote_temp,
+};
+
+static const struct thermal_zone_of_device_ops remote2_temp_sensor = {
+	.get_temp = lm90_read_remote2_temp,
+};
+
+static const struct thermal_zone_sensors_struct {
+	int sensor_id;
+	const char *name;
+	const struct thermal_zone_of_device_ops *ops;
+	u32 flag_mask;
+	u32 flag_required;
+} thermal_zone_sensors[] = {
+	{ LM90_REMOTE_TEMPERATURE, "remote", &remote_temp_sensor },
+	{ LM90_LOCAL_TEMPERATURE, "local", &local_temp_sensor },
+	{ LM90_REMOTE2_TEMPERATURE, "second remote", &remote2_temp_sensor,
+	  LM90_HAVE_TEMP3, LM90_HAVE_TEMP3 },
+};
+
+static int lm90_setup_thermal_device(struct device *dev,
+				     struct lm90_data *data)
+{
+	const struct thermal_zone_sensors_struct *entry;
+	struct thermal_zone_device *therm_dev;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(thermal_zone_sensors); i++) {
+		entry = &thermal_zone_sensors[i];
+
+		if ((data->flags & entry->flag_mask) != entry->flag_required)
+			continue;
+
+		therm_dev = devm_thermal_zone_of_sensor_register(dev,
+			entry->sensor_id, data, entry->ops);
+
+		/*
+		 * if a sensor device isn't requested in a thermal-zone the
+		 * call to devm_thermal_zone_of_sensor_register will fail
+		 * with -ENODEV. In this case we can ignore/skip it.
+		 */
+
+		if (IS_ERR(therm_dev) && (PTR_ERR(therm_dev) != -ENODEV)) {
+			dev_err(dev, "Failed to register %s thermal_zone sensor device\n",
+				entry->name);
+			return PTR_ERR(therm_dev);
+		}
+	}
+	return 0;
+}
+#else
+static int lm90_setup_thermal_device(struct device __maybe_unused *dev,
+				     struct lm90_data __maybe_unused *data)
+{
+	return 0;
+}
+#endif
+
+
 static int lm90_probe(struct i2c_client *client,
 		      const struct i2c_device_id *id)
 {
@@ -1765,6 +1852,10 @@ static int lm90_probe(struct i2c_client *client,
 	if (IS_ERR(hwmon_dev))
 		return PTR_ERR(hwmon_dev);
 
+	err = lm90_setup_thermal_device(dev, data);
+	if (err)
+		return err;
+
 	if (client->irq) {
 		dev_dbg(dev, "IRQ: %d\n", client->irq);
 		err = devm_request_threaded_irq(dev, client->irq,
-- 
2.11.0

Re: [RFC 2/2] hwmon: lm90: add thermal_zone temperature sensor support

[Guenter Roeck]

On 02/05/2017 01:03 PM, Christian Lamparter wrote:
> This patch adds thermal_zone temperature sensor support
> to the lm90 module. The feature has to be enabled
> separately via the Kconfig option:
> 	CONFIG_SENSORS_LM90_THERMAL_DEVICE
>
> The LM90 supports two (three for MAX6695 and MAX6696)
> temperature sensors. The local sensor is integrated
> into the LM90 chip. The remote sensors are connected
> to external temperature sensing diodes.
>
> Cc: Wei Ni <wni@nvidia.com>
> Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>

Since the hwnmon core now supports thermal registration, and since the lm90
driver has already been converted to using the new hwmon API, I would
rather like to understand why using the hwmon core for this purpose is
insufficient, and I would prefer to address the deficiencies in the hwmon
core and not in the driver.

Thanks,
Guenter

> ---
> ---
>  drivers/hwmon/Kconfig | 11 +++++++
>  drivers/hwmon/lm90.c  | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 102 insertions(+)
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 190d270b20a2..9df70ad21a21 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1085,6 +1085,17 @@ config SENSORS_LM90
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called lm90.
>
> +config SENSORS_LM90_THERMAL_DEVICE
> +	bool "Support for thermal-zone temperature sensors"
> +	depends on SENSORS_LM90 && THERMAL_OF
> +	help
> +	  If you say yes here you get support for thermal-zone temperature
> +	  sensors for the lm90 module and all supported chips.
> +
> +	  Refer to Documentation/devicetree/bindings/thermal/thermal.txt
> +	  and Documentation/devicetree/bindings/hwmon/lm90.txt on how to
> +	  use it for your platform.
> +
>  config SENSORS_LM92
>  	tristate "National Semiconductor LM92 and compatibles"
>  	depends on I2C
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 841f2428e84a..584550c7336d 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -95,6 +95,9 @@
>  #include <linux/sysfs.h>
>  #include <linux/interrupt.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/of.h>
> +#include <linux/thermal.h>
> +#include <dt-bindings/thermal/lm90.h>
>
>  /*
>   * Addresses to scan
> @@ -1643,6 +1646,90 @@ static const struct hwmon_ops lm90_ops = {
>  	.write = lm90_write,
>  };
>
> +#ifdef CONFIG_SENSORS_LM90_THERMAL_DEVICE
> +
> +static int lm90_read_local_temp(void *data, int *temp)
> +{
> +	*temp = lm90_get_temp11(data, LOCAL_TEMP);
> +	return 0;
> +}
> +
> +static int lm90_read_remote_temp(void *data, int *temp)
> +{
> +	*temp = lm90_get_temp11(data, REMOTE_TEMP);
> +	return 0;
> +}
> +
> +static int lm90_read_remote2_temp(void *data, int *temp)
> +{
> +	*temp = lm90_get_temp11(data, REMOTE2_TEMP);
> +	return 0;
> +}
> +
> +static const struct thermal_zone_of_device_ops local_temp_sensor = {
> +	.get_temp = lm90_read_local_temp,
> +};
> +
> +static const struct thermal_zone_of_device_ops remote_temp_sensor = {
> +	.get_temp = lm90_read_remote_temp,
> +};
> +
> +static const struct thermal_zone_of_device_ops remote2_temp_sensor = {
> +	.get_temp = lm90_read_remote2_temp,
> +};
> +
> +static const struct thermal_zone_sensors_struct {
> +	int sensor_id;
> +	const char *name;
> +	const struct thermal_zone_of_device_ops *ops;
> +	u32 flag_mask;
> +	u32 flag_required;
> +} thermal_zone_sensors[] = {
> +	{ LM90_REMOTE_TEMPERATURE, "remote", &remote_temp_sensor },
> +	{ LM90_LOCAL_TEMPERATURE, "local", &local_temp_sensor },
> +	{ LM90_REMOTE2_TEMPERATURE, "second remote", &remote2_temp_sensor,
> +	  LM90_HAVE_TEMP3, LM90_HAVE_TEMP3 },
> +};
> +
> +static int lm90_setup_thermal_device(struct device *dev,
> +				     struct lm90_data *data)
> +{
> +	const struct thermal_zone_sensors_struct *entry;
> +	struct thermal_zone_device *therm_dev;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(thermal_zone_sensors); i++) {
> +		entry = &thermal_zone_sensors[i];
> +
> +		if ((data->flags & entry->flag_mask) != entry->flag_required)
> +			continue;
> +
> +		therm_dev = devm_thermal_zone_of_sensor_register(dev,
> +			entry->sensor_id, data, entry->ops);
> +
> +		/*
> +		 * if a sensor device isn't requested in a thermal-zone the
> +		 * call to devm_thermal_zone_of_sensor_register will fail
> +		 * with -ENODEV. In this case we can ignore/skip it.
> +		 */
> +
> +		if (IS_ERR(therm_dev) && (PTR_ERR(therm_dev) != -ENODEV)) {
> +			dev_err(dev, "Failed to register %s thermal_zone sensor device\n",
> +				entry->name);
> +			return PTR_ERR(therm_dev);
> +		}
> +	}
> +	return 0;
> +}
> +#else
> +static int lm90_setup_thermal_device(struct device __maybe_unused *dev,
> +				     struct lm90_data __maybe_unused *data)
> +{
> +	return 0;
> +}
> +#endif
> +
> +
>  static int lm90_probe(struct i2c_client *client,
>  		      const struct i2c_device_id *id)
>  {
> @@ -1765,6 +1852,10 @@ static int lm90_probe(struct i2c_client *client,
>  	if (IS_ERR(hwmon_dev))
>  		return PTR_ERR(hwmon_dev);
>
> +	err = lm90_setup_thermal_device(dev, data);
> +	if (err)
> +		return err;
> +
>  	if (client->irq) {
>  		dev_dbg(dev, "IRQ: %d\n", client->irq);
>  		err = devm_request_threaded_irq(dev, client->irq,
>

Re: [RFC 2/2] hwmon: lm90: add thermal_zone temperature sensor support

[Christian Lamparter]

On Sunday, February 5, 2017 7:10:53 PM CET Guenter Roeck wrote:
> On 02/05/2017 01:03 PM, Christian Lamparter wrote:
> > This patch adds thermal_zone temperature sensor support
> > to the lm90 module. The feature has to be enabled
> > separately via the Kconfig option:
> > 	CONFIG_SENSORS_LM90_THERMAL_DEVICE
> >
> > The LM90 supports two (three for MAX6695 and MAX6696)
> > temperature sensors. The local sensor is integrated
> > into the LM90 chip. The remote sensors are connected
> > to external temperature sensing diodes.
> >
> > Cc: Wei Ni <wni@nvidia.com>
> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> 
> Since the hwnmon core now supports thermal registration, and since the lm90
> driver has already been converted to using the new hwmon API, I would
> rather like to understand why using the hwmon core for this purpose is
> insufficient, and I would prefer to address the deficiencies in the hwmon
> core and not in the driver.

Hey, that's great, I completely missed that! Yes, this makes the changes
to lm90.c obsolete.

However, what about the device-tree updates in 1/2? I'm asking because
the hwmon device node still needs the #thermal-sensor-cells property
defined for thermal_zone_of_sensor_register(). Without it, the sensor
will be skipped and the thermal-zones will do nothing (no regulation).
I can respin the device-tree patch (local and remote need to trade 
places). What do you think?

Regards,
Christian

[0] <http://lxr.free-electrons.com/source/drivers/thermal/of-thermal.c#L495>

Re: [RFC 2/2] hwmon: lm90: add thermal_zone temperature sensor support

[Christian Lamparter]

On Sunday, February 5, 2017 7:10:53 PM CET Guenter Roeck wrote:
> On 02/05/2017 01:03 PM, Christian Lamparter wrote:
> > This patch adds thermal_zone temperature sensor support
> > to the lm90 module. The feature has to be enabled
> > separately via the Kconfig option:
> > 	CONFIG_SENSORS_LM90_THERMAL_DEVICE
> >
> > The LM90 supports two (three for MAX6695 and MAX6696)
> > temperature sensors. The local sensor is integrated
> > into the LM90 chip. The remote sensors are connected
> > to external temperature sensing diodes.
> >
> > Cc: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Christian Lamparter <chunkeey-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> 
> Since the hwnmon core now supports thermal registration, and since the lm90
> driver has already been converted to using the new hwmon API, I would
> rather like to understand why using the hwmon core for this purpose is
> insufficient, and I would prefer to address the deficiencies in the hwmon
> core and not in the driver.

Hey, that's great, I completely missed that! Yes, this makes the changes
to lm90.c obsolete.

However, what about the device-tree updates in 1/2? I'm asking because
the hwmon device node still needs the #thermal-sensor-cells property
defined for thermal_zone_of_sensor_register(). Without it, the sensor
will be skipped and the thermal-zones will do nothing (no regulation).
I can respin the device-tree patch (local and remote need to trade 
places). What do you think?

Regards,
Christian

[0] <http://lxr.free-electrons.com/source/drivers/thermal/of-thermal.c#L495>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 2/2] hwmon: lm90: add thermal_zone temperature sensor support

[Guenter Roeck]

On Mon, Feb 06, 2017 at 05:01:31PM +0100, Christian Lamparter wrote:
> On Sunday, February 5, 2017 7:10:53 PM CET Guenter Roeck wrote:
> > On 02/05/2017 01:03 PM, Christian Lamparter wrote:
> > > This patch adds thermal_zone temperature sensor support
> > > to the lm90 module. The feature has to be enabled
> > > separately via the Kconfig option:
> > > 	CONFIG_SENSORS_LM90_THERMAL_DEVICE
> > >
> > > The LM90 supports two (three for MAX6695 and MAX6696)
> > > temperature sensors. The local sensor is integrated
> > > into the LM90 chip. The remote sensors are connected
> > > to external temperature sensing diodes.
> > >
> > > Cc: Wei Ni <wni@nvidia.com>
> > > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> > 
> > Since the hwnmon core now supports thermal registration, and since the lm90
> > driver has already been converted to using the new hwmon API, I would
> > rather like to understand why using the hwmon core for this purpose is
> > insufficient, and I would prefer to address the deficiencies in the hwmon
> > core and not in the driver.
> 
> Hey, that's great, I completely missed that! Yes, this makes the changes
> to lm90.c obsolete.
> 
> However, what about the device-tree updates in 1/2? I'm asking because
> the hwmon device node still needs the #thermal-sensor-cells property
> defined for thermal_zone_of_sensor_register(). Without it, the sensor
> will be skipped and the thermal-zones will do nothing (no regulation).
> I can respin the device-tree patch (local and remote need to trade 
> places). What do you think?
> 
Sure, no problem with that. Does supporting this require changes in the hwmon
core ?

Thanks,
Guenter

Re: [RFC 1/2] devicetree: add lm90 thermal_zone sensor support

[Rob Herring]

On Sun, Feb 05, 2017 at 10:03:15PM +0100, Christian Lamparter wrote:
> This patch updates the LM90's devicetree definition to
> include the #thermal-sensor-cells property as well as
> the sensor constants in include/dt-bindings/thermal/lm90.h.
> 
> Cc: Wei Ni <wni@nvidia.com>
> Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> ---
> I'm aware that there was atleast one previous attempt to add
> thermal_zone temperature sensors for the LM90 module. This was
> discussed on:
> <http://www.gossamer-threads.com/lists/linux/kernel/1992853>
> <https://lkml.org/lkml/2014/3/4/194>
> 
> This RFC is meant to get it going again. As I would really
> like to have this functionality for the Netgear WNDR4700.
> This router uses a G781 to measure the SoCs temperature
> in order to regulate a TC654 fan-controller.
> <https://git.lede-project.org/?p=source.git;a=commit;h=9e0fd1b52ad1f805a308bf6a5a13236f352fd962>
> ---
>  Documentation/devicetree/bindings/hwmon/lm90.txt |  6 ++++++
>  MAINTAINERS                                      |  1 +
>  include/dt-bindings/thermal/lm90.h               | 12 ++++++++++++
>  3 files changed, 19 insertions(+)
>  create mode 100644 include/dt-bindings/thermal/lm90.h

Acked-by: Rob Herring <robh@kernel.org>

Re: [RFC 1/2] devicetree: add lm90 thermal_zone sensor support

[Christian Lamparter]

On Wednesday, February 8, 2017 4:30:34 PM CET Rob Herring wrote:
> On Sun, Feb 05, 2017 at 10:03:15PM +0100, Christian Lamparter wrote:
> > This patch updates the LM90's devicetree definition to
> > include the #thermal-sensor-cells property as well as
> > the sensor constants in include/dt-bindings/thermal/lm90.h.
> > 
> > Cc: Wei Ni <wni@nvidia.com>
> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> > ---
> > I'm aware that there was atleast one previous attempt to add
> > thermal_zone temperature sensors for the LM90 module. This was
> > discussed on:
> > <http://www.gossamer-threads.com/lists/linux/kernel/1992853>
> > <https://lkml.org/lkml/2014/3/4/194>
> > 
> > This RFC is meant to get it going again. As I would really
> > like to have this functionality for the Netgear WNDR4700.
> > This router uses a G781 to measure the SoCs temperature
> > in order to regulate a TC654 fan-controller.
> > <https://git.lede-project.org/?p=source.git;a=commit;h=9e0fd1b52ad1f805a308bf6a5a13236f352fd962>
> > ---
> >  Documentation/devicetree/bindings/hwmon/lm90.txt |  6 ++++++
> >  MAINTAINERS                                      |  1 +
> >  include/dt-bindings/thermal/lm90.h               | 12 ++++++++++++
> >  3 files changed, 19 insertions(+)
> >  create mode 100644 include/dt-bindings/thermal/lm90.h
> 
> Acked-by: Rob Herring <robh@kernel.org>
> 
Ok thanks. Just a quick note: Don't apply this yet. I'm working
on a patch, but this has to wait for the weekend.

OnT: From Guenter Roeck:
> Sure, no problem with that. Does supporting this require changes in the hwmon
> core ?
No changes to hwmon's core are required. It's basically this 1/1 patch with
+#define LM90_LOCAL_TEMPERATURE 1 (updated to 0)
+#define LM90_REMOTE_TEMPERATURE 0 (updated to 1)

swapped and a small update to lm90.c. This way the
defines are doing something in the driver [0]:

static const u8 lm90_temp_index[3] = {
        LOCAL_TEMP, REMOTE_TEMP, REMOTE2_TEMP
};

will become something like:

static const u8 lm90_temp_index[3] = {
	[LM90_LOCAL_TEMPERATURE] = LOCAL_TEMP,
	[LM90_REMOTE_TEMPERATURE] = REMOTE_TEMP,
	[LM90_REMOTE2_TEMPERATURE] = REMOTE2_TEMP
};

Regards,
Christian

[0] <http://lxr.free-electrons.com/source/drivers/hwmon/lm90.c#L1018>

[PATCH 1/2] devicetree: add lm90 thermal_zone sensor support

[Christian Lamparter]

This patch updates the LM90's devicetree definition to
include the #thermal-sensor-cells property as well as
the sensor constants in include/dt-bindings/thermal/lm90.h.

Cc: Wei Ni <wni@nvidia.com>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
I've reordered LM90_LOCAL_TEMPERATURE and LM90_REMOTE_TEMPERATURE.
Everything else is the same as in the RFC. This is the only
required patch and it only updates the documentation and binding.
[PATCH 2/2] is optional (as I said I would look at it).
---
 Documentation/devicetree/bindings/hwmon/lm90.txt |  6 ++++++
 MAINTAINERS                                      |  1 +
 include/dt-bindings/thermal/lm90.h               | 12 ++++++++++++
 3 files changed, 19 insertions(+)
 create mode 100644 include/dt-bindings/thermal/lm90.h

diff --git a/Documentation/devicetree/bindings/hwmon/lm90.txt b/Documentation/devicetree/bindings/hwmon/lm90.txt
index e8632486b9ef..97581266e329 100644
--- a/Documentation/devicetree/bindings/hwmon/lm90.txt
+++ b/Documentation/devicetree/bindings/hwmon/lm90.txt
@@ -33,6 +33,11 @@ Optional properties:
               LM90 "-ALERT" pin output.
               See interrupt-controller/interrupts.txt for the format.
 
+- #thermal-sensor-cells: should be set to 1. See thermal/thermal.txt for
+	      details. See <include/dt-bindings/thermal/lm90.h> for the
+	      definition of the local, remote and 2nd remote sensor index
+	      constants.
+
 Example LM90 node:
 
 temp-sensor {
@@ -41,4 +46,5 @@ temp-sensor {
 	vcc-supply = <&palmas_ldo6_reg>;
 	interrupt-parent = <&gpio>;
 	interrupts = <TEGRA_GPIO(O, 4) IRQ_TYPE_LEVEL_LOW>;
+	#thermal-sensor-cells = <1>;
 }
diff --git a/MAINTAINERS b/MAINTAINERS
index 2be620cea1ed..73972ccebc56 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7541,6 +7541,7 @@ S:	Maintained
 F:	Documentation/hwmon/lm90
 F:	Documentation/devicetree/bindings/hwmon/lm90.txt
 F:	drivers/hwmon/lm90.c
+F:	include/dt-bindings/thermal/lm90.h
 
 LM95234 HARDWARE MONITOR DRIVER
 M:	Guenter Roeck <linux@roeck-us.net>
diff --git a/include/dt-bindings/thermal/lm90.h b/include/dt-bindings/thermal/lm90.h
new file mode 100644
index 000000000000..8c2e3095f704
--- /dev/null
+++ b/include/dt-bindings/thermal/lm90.h
@@ -0,0 +1,12 @@
+/*
+ * This header provides constants for the LM90 thermal bindings.
+ */
+
+#ifndef _DT_BINDINGS_THERMAL_LM90_H_
+#define _DT_BINDINGS_THERMAL_LM90_H_
+
+#define LM90_LOCAL_TEMPERATURE 0
+#define LM90_REMOTE_TEMPERATURE 1
+#define LM90_REMOTE2_TEMPERATURE 2
+
+#endif
-- 
2.11.0

[PATCH 2/2] hwmon: lm90: integration of channel map in dt-bindings

[Christian Lamparter]

This patch integrates the LOCAL, REMOTE and REMOTE2
channel definitions into the lm90.c driver.

Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
This is an optional patch to showcase how the channel definition
in the dt-bindings are mapped into the driver.
In theory, this makes it possible to easily remap the channel
indices. However, it does make the driver harder to read.
---
 drivers/hwmon/lm90.c | 61 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 40 insertions(+), 21 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 841f2428e84a..aa67810000f9 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -95,6 +95,7 @@
 #include <linux/sysfs.h>
 #include <linux/interrupt.h>
 #include <linux/regulator/consumer.h>
+#include <dt-bindings/thermal/lm90.h>
 
 /*
  * Addresses to scan
@@ -1016,23 +1017,33 @@ static int lm90_set_temphyst(struct lm90_data *data, long val)
 }
 
 static const u8 lm90_temp_index[3] = {
-	LOCAL_TEMP, REMOTE_TEMP, REMOTE2_TEMP
+	[LM90_LOCAL_TEMPERATURE] = LOCAL_TEMP,
+	[LM90_REMOTE_TEMPERATURE] = REMOTE_TEMP,
+	[LM90_REMOTE2_TEMPERATURE] =  REMOTE2_TEMP
 };
 
 static const u8 lm90_temp_min_index[3] = {
-	LOCAL_LOW, REMOTE_LOW, REMOTE2_LOW
+	[LM90_LOCAL_TEMPERATURE] = LOCAL_LOW,
+	[LM90_REMOTE_TEMPERATURE] = REMOTE_LOW,
+	[LM90_REMOTE2_TEMPERATURE] = REMOTE2_LOW
 };
 
 static const u8 lm90_temp_max_index[3] = {
-	LOCAL_HIGH, REMOTE_HIGH, REMOTE2_HIGH
+	[LM90_LOCAL_TEMPERATURE] = LOCAL_HIGH,
+	[LM90_REMOTE_TEMPERATURE] = REMOTE_HIGH,
+	[LM90_REMOTE2_TEMPERATURE] = REMOTE2_HIGH
 };
 
 static const u8 lm90_temp_crit_index[3] = {
-	LOCAL_CRIT, REMOTE_CRIT, REMOTE2_CRIT
+	[LM90_LOCAL_TEMPERATURE] = LOCAL_CRIT,
+	[LM90_REMOTE_TEMPERATURE] = REMOTE_CRIT,
+	[LM90_REMOTE2_TEMPERATURE] = REMOTE2_CRIT
 };
 
 static const u8 lm90_temp_emerg_index[3] = {
-	LOCAL_EMERG, REMOTE_EMERG, REMOTE2_EMERG
+	[LM90_LOCAL_TEMPERATURE] = LOCAL_EMERG,
+	[LM90_REMOTE_TEMPERATURE] = REMOTE_EMERG,
+	[LM90_REMOTE2_TEMPERATURE] = REMOTE2_EMERG
 };
 
 static const u8 lm90_min_alarm_bits[3] = { 5, 3, 11 };
@@ -1654,6 +1665,10 @@ static int lm90_probe(struct i2c_client *client,
 	struct lm90_data *data;
 	int err;
 
+	BUILD_BUG_ON(LM90_LOCAL_TEMPERATURE == LM90_REMOTE_TEMPERATURE ||
+		     LM90_REMOTE_TEMPERATURE == LM90_REMOTE2_TEMPERATURE ||
+		     LM90_REMOTE2_TEMPERATURE == LM90_LOCAL_TEMPERATURE);
+
 	regulator = devm_regulator_get(dev, "vcc");
 	if (IS_ERR(regulator))
 		return PTR_ERR(regulator);
@@ -1695,37 +1710,41 @@ static int lm90_probe(struct i2c_client *client,
 	data->chip.ops = &lm90_ops;
 	data->chip.info = data->info;
 
-	data->info[0] = &lm90_chip_info;
-	data->info[1] = &data->temp_info;
+	data->info[LM90_LOCAL_TEMPERATURE] = &lm90_chip_info;
+	data->info[LM90_REMOTE_TEMPERATURE] = &data->temp_info;
 
 	info = &data->temp_info;
 	info->type = hwmon_temp;
 	info->config = data->channel_config;
 
-	data->channel_config[0] = HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX |
-		HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MIN_ALARM |
-		HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM;
-	data->channel_config[1] = HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX |
-		HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MIN_ALARM |
-		HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM | HWMON_T_FAULT;
+	data->channel_config[LM90_LOCAL_TEMPERATURE] = HWMON_T_INPUT |
+		HWMON_T_MIN | HWMON_T_MAX | HWMON_T_CRIT | HWMON_T_CRIT_HYST |
+		HWMON_T_MIN_ALARM | HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM;
+	data->channel_config[LM90_REMOTE_TEMPERATURE] = HWMON_T_INPUT |
+		HWMON_T_MIN | HWMON_T_MAX | HWMON_T_CRIT | HWMON_T_CRIT_HYST |
+		HWMON_T_MIN_ALARM | HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+		HWMON_T_FAULT;
 
 	if (data->flags & LM90_HAVE_OFFSET)
-		data->channel_config[1] |= HWMON_T_OFFSET;
+		data->channel_config[LM90_REMOTE_TEMPERATURE] |= HWMON_T_OFFSET;
 
 	if (data->flags & LM90_HAVE_EMERGENCY) {
-		data->channel_config[0] |= HWMON_T_EMERGENCY |
-			HWMON_T_EMERGENCY_HYST;
-		data->channel_config[1] |= HWMON_T_EMERGENCY |
-			HWMON_T_EMERGENCY_HYST;
+		data->channel_config[LM90_LOCAL_TEMPERATURE] |=
+			HWMON_T_EMERGENCY | HWMON_T_EMERGENCY_HYST;
+		data->channel_config[LM90_REMOTE_TEMPERATURE] |=
+			HWMON_T_EMERGENCY | HWMON_T_EMERGENCY_HYST;
 	}
 
 	if (data->flags & LM90_HAVE_EMERGENCY_ALARM) {
-		data->channel_config[0] |= HWMON_T_EMERGENCY_ALARM;
-		data->channel_config[1] |= HWMON_T_EMERGENCY_ALARM;
+		data->channel_config[LM90_LOCAL_TEMPERATURE] |=
+			HWMON_T_EMERGENCY_ALARM;
+		data->channel_config[LM90_REMOTE_TEMPERATURE] |=
+			HWMON_T_EMERGENCY_ALARM;
 	}
 
 	if (data->flags & LM90_HAVE_TEMP3) {
-		data->channel_config[2] = HWMON_T_INPUT |
+		data->channel_config[LM90_REMOTE2_TEMPERATURE] =
+			HWMON_T_INPUT |
 			HWMON_T_MIN | HWMON_T_MAX |
 			HWMON_T_CRIT | HWMON_T_CRIT_HYST |
 			HWMON_T_EMERGENCY | HWMON_T_EMERGENCY_HYST |
-- 
2.11.0

[PATCH 2/2] hwmon: lm90: integration of channel map in dt-bindings

[Christian Lamparter]

This patch integrates the LOCAL, REMOTE and REMOTE2
channel definitions into the lm90.c driver.

Signed-off-by: Christian Lamparter <chunkeey-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
---
This is an optional patch to showcase how the channel definition
in the dt-bindings are mapped into the driver.
In theory, this makes it possible to easily remap the channel
indices. However, it does make the driver harder to read.
---
 drivers/hwmon/lm90.c | 61 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 40 insertions(+), 21 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 841f2428e84a..aa67810000f9 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -95,6 +95,7 @@
 #include <linux/sysfs.h>
 #include <linux/interrupt.h>
 #include <linux/regulator/consumer.h>
+#include <dt-bindings/thermal/lm90.h>
 
 /*
  * Addresses to scan
@@ -1016,23 +1017,33 @@ static int lm90_set_temphyst(struct lm90_data *data, long val)
 }
 
 static const u8 lm90_temp_index[3] = {
-	LOCAL_TEMP, REMOTE_TEMP, REMOTE2_TEMP
+	[LM90_LOCAL_TEMPERATURE] = LOCAL_TEMP,
+	[LM90_REMOTE_TEMPERATURE] = REMOTE_TEMP,
+	[LM90_REMOTE2_TEMPERATURE] =  REMOTE2_TEMP
 };
 
 static const u8 lm90_temp_min_index[3] = {
-	LOCAL_LOW, REMOTE_LOW, REMOTE2_LOW
+	[LM90_LOCAL_TEMPERATURE] = LOCAL_LOW,
+	[LM90_REMOTE_TEMPERATURE] = REMOTE_LOW,
+	[LM90_REMOTE2_TEMPERATURE] = REMOTE2_LOW
 };
 
 static const u8 lm90_temp_max_index[3] = {
-	LOCAL_HIGH, REMOTE_HIGH, REMOTE2_HIGH
+	[LM90_LOCAL_TEMPERATURE] = LOCAL_HIGH,
+	[LM90_REMOTE_TEMPERATURE] = REMOTE_HIGH,
+	[LM90_REMOTE2_TEMPERATURE] = REMOTE2_HIGH
 };
 
 static const u8 lm90_temp_crit_index[3] = {
-	LOCAL_CRIT, REMOTE_CRIT, REMOTE2_CRIT
+	[LM90_LOCAL_TEMPERATURE] = LOCAL_CRIT,
+	[LM90_REMOTE_TEMPERATURE] = REMOTE_CRIT,
+	[LM90_REMOTE2_TEMPERATURE] = REMOTE2_CRIT
 };
 
 static const u8 lm90_temp_emerg_index[3] = {
-	LOCAL_EMERG, REMOTE_EMERG, REMOTE2_EMERG
+	[LM90_LOCAL_TEMPERATURE] = LOCAL_EMERG,
+	[LM90_REMOTE_TEMPERATURE] = REMOTE_EMERG,
+	[LM90_REMOTE2_TEMPERATURE] = REMOTE2_EMERG
 };
 
 static const u8 lm90_min_alarm_bits[3] = { 5, 3, 11 };
@@ -1654,6 +1665,10 @@ static int lm90_probe(struct i2c_client *client,
 	struct lm90_data *data;
 	int err;
 
+	BUILD_BUG_ON(LM90_LOCAL_TEMPERATURE == LM90_REMOTE_TEMPERATURE ||
+		     LM90_REMOTE_TEMPERATURE == LM90_REMOTE2_TEMPERATURE ||
+		     LM90_REMOTE2_TEMPERATURE == LM90_LOCAL_TEMPERATURE);
+
 	regulator = devm_regulator_get(dev, "vcc");
 	if (IS_ERR(regulator))
 		return PTR_ERR(regulator);
@@ -1695,37 +1710,41 @@ static int lm90_probe(struct i2c_client *client,
 	data->chip.ops = &lm90_ops;
 	data->chip.info = data->info;
 
-	data->info[0] = &lm90_chip_info;
-	data->info[1] = &data->temp_info;
+	data->info[LM90_LOCAL_TEMPERATURE] = &lm90_chip_info;
+	data->info[LM90_REMOTE_TEMPERATURE] = &data->temp_info;
 
 	info = &data->temp_info;
 	info->type = hwmon_temp;
 	info->config = data->channel_config;
 
-	data->channel_config[0] = HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX |
-		HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MIN_ALARM |
-		HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM;
-	data->channel_config[1] = HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX |
-		HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MIN_ALARM |
-		HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM | HWMON_T_FAULT;
+	data->channel_config[LM90_LOCAL_TEMPERATURE] = HWMON_T_INPUT |
+		HWMON_T_MIN | HWMON_T_MAX | HWMON_T_CRIT | HWMON_T_CRIT_HYST |
+		HWMON_T_MIN_ALARM | HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM;
+	data->channel_config[LM90_REMOTE_TEMPERATURE] = HWMON_T_INPUT |
+		HWMON_T_MIN | HWMON_T_MAX | HWMON_T_CRIT | HWMON_T_CRIT_HYST |
+		HWMON_T_MIN_ALARM | HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+		HWMON_T_FAULT;
 
 	if (data->flags & LM90_HAVE_OFFSET)
-		data->channel_config[1] |= HWMON_T_OFFSET;
+		data->channel_config[LM90_REMOTE_TEMPERATURE] |= HWMON_T_OFFSET;
 
 	if (data->flags & LM90_HAVE_EMERGENCY) {
-		data->channel_config[0] |= HWMON_T_EMERGENCY |
-			HWMON_T_EMERGENCY_HYST;
-		data->channel_config[1] |= HWMON_T_EMERGENCY |
-			HWMON_T_EMERGENCY_HYST;
+		data->channel_config[LM90_LOCAL_TEMPERATURE] |=
+			HWMON_T_EMERGENCY | HWMON_T_EMERGENCY_HYST;
+		data->channel_config[LM90_REMOTE_TEMPERATURE] |=
+			HWMON_T_EMERGENCY | HWMON_T_EMERGENCY_HYST;
 	}
 
 	if (data->flags & LM90_HAVE_EMERGENCY_ALARM) {
-		data->channel_config[0] |= HWMON_T_EMERGENCY_ALARM;
-		data->channel_config[1] |= HWMON_T_EMERGENCY_ALARM;
+		data->channel_config[LM90_LOCAL_TEMPERATURE] |=
+			HWMON_T_EMERGENCY_ALARM;
+		data->channel_config[LM90_REMOTE_TEMPERATURE] |=
+			HWMON_T_EMERGENCY_ALARM;
 	}
 
 	if (data->flags & LM90_HAVE_TEMP3) {
-		data->channel_config[2] = HWMON_T_INPUT |
+		data->channel_config[LM90_REMOTE2_TEMPERATURE] =
+			HWMON_T_INPUT |
 			HWMON_T_MIN | HWMON_T_MAX |
 			HWMON_T_CRIT | HWMON_T_CRIT_HYST |
 			HWMON_T_EMERGENCY | HWMON_T_EMERGENCY_HYST |
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] hwmon: lm90: integration of channel map in dt-bindings

[Guenter Roeck]

On Fri, Feb 10, 2017 at 05:12:30PM +0100, Christian Lamparter wrote:
> This patch integrates the LOCAL, REMOTE and REMOTE2
> channel definitions into the lm90.c driver.
> 
> Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> ---
> This is an optional patch to showcase how the channel definition
> in the dt-bindings are mapped into the driver.
> In theory, this makes it possible to easily remap the channel
> indices. However, it does make the driver harder to read.

It also makes the driver dependent on external defines which are not controlled
by the driver. If anyone changes those defines to be non-sequential or to not
start with 0, we would be in trouble. Sure, that might and likely would result
in compile errors, but still ...

Besides, it is not complete. Anyone changing channel index values would
(at least) mess up alarm bit association.

If we want to do that kind of thing, it might make more sense to add some code
to provide the desired mapping to the hwmon core, and to let the hwmon core
handle it. No idea if that is even possible, though.

Is that really worth it ?

Thanks,
Guenter

> ---
>  drivers/hwmon/lm90.c | 61 ++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 40 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 841f2428e84a..aa67810000f9 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -95,6 +95,7 @@
>  #include <linux/sysfs.h>
>  #include <linux/interrupt.h>
>  #include <linux/regulator/consumer.h>
> +#include <dt-bindings/thermal/lm90.h>
>  
>  /*
>   * Addresses to scan
> @@ -1016,23 +1017,33 @@ static int lm90_set_temphyst(struct lm90_data *data, long val)
>  }
>  
>  static const u8 lm90_temp_index[3] = {
> -	LOCAL_TEMP, REMOTE_TEMP, REMOTE2_TEMP
> +	[LM90_LOCAL_TEMPERATURE] = LOCAL_TEMP,
> +	[LM90_REMOTE_TEMPERATURE] = REMOTE_TEMP,
> +	[LM90_REMOTE2_TEMPERATURE] =  REMOTE2_TEMP
>  };
>  
>  static const u8 lm90_temp_min_index[3] = {
> -	LOCAL_LOW, REMOTE_LOW, REMOTE2_LOW
> +	[LM90_LOCAL_TEMPERATURE] = LOCAL_LOW,
> +	[LM90_REMOTE_TEMPERATURE] = REMOTE_LOW,
> +	[LM90_REMOTE2_TEMPERATURE] = REMOTE2_LOW
>  };
>  
>  static const u8 lm90_temp_max_index[3] = {
> -	LOCAL_HIGH, REMOTE_HIGH, REMOTE2_HIGH
> +	[LM90_LOCAL_TEMPERATURE] = LOCAL_HIGH,
> +	[LM90_REMOTE_TEMPERATURE] = REMOTE_HIGH,
> +	[LM90_REMOTE2_TEMPERATURE] = REMOTE2_HIGH
>  };
>  
>  static const u8 lm90_temp_crit_index[3] = {
> -	LOCAL_CRIT, REMOTE_CRIT, REMOTE2_CRIT
> +	[LM90_LOCAL_TEMPERATURE] = LOCAL_CRIT,
> +	[LM90_REMOTE_TEMPERATURE] = REMOTE_CRIT,
> +	[LM90_REMOTE2_TEMPERATURE] = REMOTE2_CRIT
>  };
>  
>  static const u8 lm90_temp_emerg_index[3] = {
> -	LOCAL_EMERG, REMOTE_EMERG, REMOTE2_EMERG
> +	[LM90_LOCAL_TEMPERATURE] = LOCAL_EMERG,
> +	[LM90_REMOTE_TEMPERATURE] = REMOTE_EMERG,
> +	[LM90_REMOTE2_TEMPERATURE] = REMOTE2_EMERG
>  };
>  
>  static const u8 lm90_min_alarm_bits[3] = { 5, 3, 11 };
> @@ -1654,6 +1665,10 @@ static int lm90_probe(struct i2c_client *client,
>  	struct lm90_data *data;
>  	int err;
>  
> +	BUILD_BUG_ON(LM90_LOCAL_TEMPERATURE == LM90_REMOTE_TEMPERATURE ||
> +		     LM90_REMOTE_TEMPERATURE == LM90_REMOTE2_TEMPERATURE ||
> +		     LM90_REMOTE2_TEMPERATURE == LM90_LOCAL_TEMPERATURE);
> +
>  	regulator = devm_regulator_get(dev, "vcc");
>  	if (IS_ERR(regulator))
>  		return PTR_ERR(regulator);
> @@ -1695,37 +1710,41 @@ static int lm90_probe(struct i2c_client *client,
>  	data->chip.ops = &lm90_ops;
>  	data->chip.info = data->info;
>  
> -	data->info[0] = &lm90_chip_info;
> -	data->info[1] = &data->temp_info;
> +	data->info[LM90_LOCAL_TEMPERATURE] = &lm90_chip_info;
> +	data->info[LM90_REMOTE_TEMPERATURE] = &data->temp_info;
>  
>  	info = &data->temp_info;
>  	info->type = hwmon_temp;
>  	info->config = data->channel_config;
>  
> -	data->channel_config[0] = HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX |
> -		HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MIN_ALARM |
> -		HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM;
> -	data->channel_config[1] = HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX |
> -		HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MIN_ALARM |
> -		HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM | HWMON_T_FAULT;
> +	data->channel_config[LM90_LOCAL_TEMPERATURE] = HWMON_T_INPUT |
> +		HWMON_T_MIN | HWMON_T_MAX | HWMON_T_CRIT | HWMON_T_CRIT_HYST |
> +		HWMON_T_MIN_ALARM | HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM;
> +	data->channel_config[LM90_REMOTE_TEMPERATURE] = HWMON_T_INPUT |
> +		HWMON_T_MIN | HWMON_T_MAX | HWMON_T_CRIT | HWMON_T_CRIT_HYST |
> +		HWMON_T_MIN_ALARM | HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
> +		HWMON_T_FAULT;
>  
>  	if (data->flags & LM90_HAVE_OFFSET)
> -		data->channel_config[1] |= HWMON_T_OFFSET;
> +		data->channel_config[LM90_REMOTE_TEMPERATURE] |= HWMON_T_OFFSET;
>  
>  	if (data->flags & LM90_HAVE_EMERGENCY) {
> -		data->channel_config[0] |= HWMON_T_EMERGENCY |
> -			HWMON_T_EMERGENCY_HYST;
> -		data->channel_config[1] |= HWMON_T_EMERGENCY |
> -			HWMON_T_EMERGENCY_HYST;
> +		data->channel_config[LM90_LOCAL_TEMPERATURE] |=
> +			HWMON_T_EMERGENCY | HWMON_T_EMERGENCY_HYST;
> +		data->channel_config[LM90_REMOTE_TEMPERATURE] |=
> +			HWMON_T_EMERGENCY | HWMON_T_EMERGENCY_HYST;
>  	}
>  
>  	if (data->flags & LM90_HAVE_EMERGENCY_ALARM) {
> -		data->channel_config[0] |= HWMON_T_EMERGENCY_ALARM;
> -		data->channel_config[1] |= HWMON_T_EMERGENCY_ALARM;
> +		data->channel_config[LM90_LOCAL_TEMPERATURE] |=
> +			HWMON_T_EMERGENCY_ALARM;
> +		data->channel_config[LM90_REMOTE_TEMPERATURE] |=
> +			HWMON_T_EMERGENCY_ALARM;
>  	}
>  
>  	if (data->flags & LM90_HAVE_TEMP3) {
> -		data->channel_config[2] = HWMON_T_INPUT |
> +		data->channel_config[LM90_REMOTE2_TEMPERATURE] =
> +			HWMON_T_INPUT |
>  			HWMON_T_MIN | HWMON_T_MAX |
>  			HWMON_T_CRIT | HWMON_T_CRIT_HYST |
>  			HWMON_T_EMERGENCY | HWMON_T_EMERGENCY_HYST |
> -- 
> 2.11.0
> 

Re: [PATCH 2/2] hwmon: lm90: integration of channel map in dt-bindings

[Christian Lamparter]

On Friday, February 10, 2017 9:21:06 AM CET Guenter Roeck wrote:
> On Fri, Feb 10, 2017 at 05:12:30PM +0100, Christian Lamparter wrote:
> > This patch integrates the LOCAL, REMOTE and REMOTE2
> > channel definitions into the lm90.c driver.
> > 
> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> > ---
> > This is an optional patch to showcase how the channel definition
> > in the dt-bindings are mapped into the driver.
> > In theory, this makes it possible to easily remap the channel
> > indices. However, it does make the driver harder to read.
> 
> It also makes the driver dependent on external defines which are not controlled
> by the driver. If anyone changes those defines to be non-sequential or to not
> start with 0, we would be in trouble. Sure, that might and likely would result
> in compile errors, but still ...
Yes, gcc will complain with "array out of bounds errors" if any
of the LM90_SENSORS_ defines are less than 0 or higher than 2.
This is because of: static const u8 lm90_temp_index[3] and 
lm90_temp_min_index, ...

The BUILD_BUG_ON(LOCAL == REMOTE || ... || REMOTE2 == LOCAL) will
prevent duplicated values so LOCAL, REMOTE, REMOTE2 have to be
different.

> Besides, it is not complete. Anyone changing channel index values would
> (at least) mess up alarm bit association.
Yes, that's true. I missed lm90_is_tripped. But...

> If we want to do that kind of thing, it might make more sense to add some code
> to provide the desired mapping to the hwmon core, and to let the hwmon core
> handle it. No idea if that is even possible, though.
> 
> Is that really worth it ?
No, it's not worth it ;-). But thank you for your in-depth
analysis. So, let's leave it with just the first patch for
4.12-ish.

Regards,
Christian

Re: [PATCH 1/2] devicetree: add lm90 thermal_zone sensor support

[Guenter Roeck]

On Fri, Feb 10, 2017 at 05:12:29PM +0100, Christian Lamparter wrote:
> This patch updates the LM90's devicetree definition to
> include the #thermal-sensor-cells property as well as
> the sensor constants in include/dt-bindings/thermal/lm90.h.
> 
> Cc: Wei Ni <wni@nvidia.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>

I don't really know who would normally apply a patch like this,
so I added it to hwmon-next.

Thanks,
Guenter

> ---
> I've reordered LM90_LOCAL_TEMPERATURE and LM90_REMOTE_TEMPERATURE.
> Everything else is the same as in the RFC. This is the only
> required patch and it only updates the documentation and binding.
> [PATCH 2/2] is optional (as I said I would look at it).
> ---
>  Documentation/devicetree/bindings/hwmon/lm90.txt |  6 ++++++
>  MAINTAINERS                                      |  1 +
>  include/dt-bindings/thermal/lm90.h               | 12 ++++++++++++
>  3 files changed, 19 insertions(+)
>  create mode 100644 include/dt-bindings/thermal/lm90.h
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/lm90.txt b/Documentation/devicetree/bindings/hwmon/lm90.txt
> index e8632486b9ef..97581266e329 100644
> --- a/Documentation/devicetree/bindings/hwmon/lm90.txt
> +++ b/Documentation/devicetree/bindings/hwmon/lm90.txt
> @@ -33,6 +33,11 @@ Optional properties:
>                LM90 "-ALERT" pin output.
>                See interrupt-controller/interrupts.txt for the format.
>  
> +- #thermal-sensor-cells: should be set to 1. See thermal/thermal.txt for
> +	      details. See <include/dt-bindings/thermal/lm90.h> for the
> +	      definition of the local, remote and 2nd remote sensor index
> +	      constants.
> +
>  Example LM90 node:
>  
>  temp-sensor {
> @@ -41,4 +46,5 @@ temp-sensor {
>  	vcc-supply = <&palmas_ldo6_reg>;
>  	interrupt-parent = <&gpio>;
>  	interrupts = <TEGRA_GPIO(O, 4) IRQ_TYPE_LEVEL_LOW>;
> +	#thermal-sensor-cells = <1>;
>  }
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2be620cea1ed..73972ccebc56 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7541,6 +7541,7 @@ S:	Maintained
>  F:	Documentation/hwmon/lm90
>  F:	Documentation/devicetree/bindings/hwmon/lm90.txt
>  F:	drivers/hwmon/lm90.c
> +F:	include/dt-bindings/thermal/lm90.h
>  
>  LM95234 HARDWARE MONITOR DRIVER
>  M:	Guenter Roeck <linux@roeck-us.net>
> diff --git a/include/dt-bindings/thermal/lm90.h b/include/dt-bindings/thermal/lm90.h
> new file mode 100644
> index 000000000000..8c2e3095f704
> --- /dev/null
> +++ b/include/dt-bindings/thermal/lm90.h
> @@ -0,0 +1,12 @@
> +/*
> + * This header provides constants for the LM90 thermal bindings.
> + */
> +
> +#ifndef _DT_BINDINGS_THERMAL_LM90_H_
> +#define _DT_BINDINGS_THERMAL_LM90_H_
> +
> +#define LM90_LOCAL_TEMPERATURE 0
> +#define LM90_REMOTE_TEMPERATURE 1
> +#define LM90_REMOTE2_TEMPERATURE 2
> +
> +#endif
> -- 
> 2.11.0
>