Patchwork [3/4] hwmon: Add a simple driver to read the MXS SoC temperature

login
register
mail settings
Submitter Alexandre Belloni
Date June 26, 2013, 8:51 a.m.
Message ID <1372236673-20725-4-git-send-email-alexandre.belloni@free-electrons.com>
Download mbox | patch
Permalink /patch/254831/
State New
Headers show

Comments

Alexandre Belloni - June 26, 2013, 8:51 a.m.
The low resolution ADC of the mxs is able to read an internal temperature
sensor, expose that using hwmon.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 .../devicetree/bindings/hwmon/mxs-cputemp.txt      |  18 +++
 Documentation/hwmon/mxs-cputemp                    |  29 +++++
 drivers/hwmon/Kconfig                              |  10 ++
 drivers/hwmon/Makefile                             |   1 +
 drivers/hwmon/mxs-cputemp.c                        | 132 +++++++++++++++++++++
 5 files changed, 190 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/mxs-cputemp.txt
 create mode 100644 Documentation/hwmon/mxs-cputemp
 create mode 100644 drivers/hwmon/mxs-cputemp.c
Maxime Ripard - June 27, 2013, 9:17 a.m.
On Wed, Jun 26, 2013 at 07:39:27AM -0700, Guenter Roeck wrote:
> On Wed, Jun 26, 2013 at 10:51:12AM +0200, Alexandre Belloni wrote:
> > The low resolution ADC of the mxs is able to read an internal temperature
> > sensor, expose that using hwmon.
> > 
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > ---
> 
> Wouldn't it make more sense to use iio-hwmon and improve it if necessary ?

Actually, I wonder if we should not just put the hwmon driver
capabilities directly into the mxs-lradc driver, just like it's already
been done in this driver for the touchscreen support.

The probing of this hwmon driver doesn't really belong to the DT, it's
not really realistic to probe it from the machine definition, and it
really is the IP that is wired that way.

Maxime
Guenter Roeck - June 27, 2013, 2:27 p.m.
On Thu, Jun 27, 2013 at 11:17:32AM +0200, Maxime Ripard wrote:
> On Wed, Jun 26, 2013 at 07:39:27AM -0700, Guenter Roeck wrote:
> > On Wed, Jun 26, 2013 at 10:51:12AM +0200, Alexandre Belloni wrote:
> > > The low resolution ADC of the mxs is able to read an internal temperature
> > > sensor, expose that using hwmon.
> > > 
> > > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > > ---
> > 
> > Wouldn't it make more sense to use iio-hwmon and improve it if necessary ?
> 
> Actually, I wonder if we should not just put the hwmon driver
> capabilities directly into the mxs-lradc driver, just like it's already
> been done in this driver for the touchscreen support.
> 
> The probing of this hwmon driver doesn't really belong to the DT, it's
> not really realistic to probe it from the machine definition, and it
> really is the IP that is wired that way.
> 
Merging iio-hwmon functionality into an adc driver seems just as bad
(or even worse) as copying it into a new driver.

If the lradc driver knows that the ADC channels are temperature sensors, it
should register them with the iio subsystem as IIO_TEMP type. Then you should
be able to use iio_hwmon as is.

Guenter
Alexandre Belloni - June 27, 2013, 7:26 p.m.
Hi,

On 27/06/2013 16:27, Guenter Roeck wrote:
> On Thu, Jun 27, 2013 at 11:17:32AM +0200, Maxime Ripard wrote:
>> On Wed, Jun 26, 2013 at 07:39:27AM -0700, Guenter Roeck wrote:
>>> On Wed, Jun 26, 2013 at 10:51:12AM +0200, Alexandre Belloni wrote:
>>>> The low resolution ADC of the mxs is able to read an internal temperature
>>>> sensor, expose that using hwmon.
>>>>
>>>> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>>>> ---
>>> Wouldn't it make more sense to use iio-hwmon and improve it if necessary ?
>> Actually, I wonder if we should not just put the hwmon driver
>> capabilities directly into the mxs-lradc driver, just like it's already
>> been done in this driver for the touchscreen support.
>>
>> The probing of this hwmon driver doesn't really belong to the DT, it's
>> not really realistic to probe it from the machine definition, and it
>> really is the IP that is wired that way.
>>
> Merging iio-hwmon functionality into an adc driver seems just as bad
> (or even worse) as copying it into a new driver.
>
> If the lradc driver knows that the ADC channels are temperature sensors, it
> should register them with the iio subsystem as IIO_TEMP type. Then you should
> be able to use iio_hwmon as is.

They are already registered as IIO_TEMP but only implement read_raw. Also,

iio_hwmon_read_val() is using iio_read_channel_processed() and that will
basically only read one of the 2 channels. As I documented, you actually
need to read both channel 8 and channel 9 and then compute the value in
Kelvins. I'm not sure how you want me to do that in the current framework.

Regards,
Lars-Peter Clausen - June 28, 2013, 2:18 p.m.
On 06/27/2013 09:26 PM, Alexandre Belloni wrote:
> Hi,
> 
> On 27/06/2013 16:27, Guenter Roeck wrote:
>> On Thu, Jun 27, 2013 at 11:17:32AM +0200, Maxime Ripard wrote:
>>> On Wed, Jun 26, 2013 at 07:39:27AM -0700, Guenter Roeck wrote:
>>>> On Wed, Jun 26, 2013 at 10:51:12AM +0200, Alexandre Belloni wrote:
>>>>> The low resolution ADC of the mxs is able to read an internal temperature
>>>>> sensor, expose that using hwmon.
>>>>>
>>>>> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>>>>> ---
>>>> Wouldn't it make more sense to use iio-hwmon and improve it if necessary ?
>>> Actually, I wonder if we should not just put the hwmon driver
>>> capabilities directly into the mxs-lradc driver, just like it's already
>>> been done in this driver for the touchscreen support.
>>>
>>> The probing of this hwmon driver doesn't really belong to the DT, it's
>>> not really realistic to probe it from the machine definition, and it
>>> really is the IP that is wired that way.
>>>
>> Merging iio-hwmon functionality into an adc driver seems just as bad
>> (or even worse) as copying it into a new driver.
>>
>> If the lradc driver knows that the ADC channels are temperature sensors, it
>> should register them with the iio subsystem as IIO_TEMP type. Then you should
>> be able to use iio_hwmon as is.
> 
> They are already registered as IIO_TEMP but only implement read_raw. Also,
> 
> iio_hwmon_read_val() is using iio_read_channel_processed() and that will
> basically only read one of the 2 channels. As I documented, you actually
> need to read both channel 8 and channel 9 and then compute the value in
> Kelvins. I'm not sure how you want me to do that in the current framework.

What are these two channels actually measuring? Is the value of a single
channel meaningful on it's own? If not it might make sense to update the IIO
driver to just have one temperature channel.

- Lars
Alexandre Belloni - June 28, 2013, 2:50 p.m.
On 28/06/2013 16:18, Lars-Peter Clausen wrote:
> On 06/27/2013 09:26 PM, Alexandre Belloni wrote:
>>
>> They are already registered as IIO_TEMP but only implement read_raw. Also,
>>
>> iio_hwmon_read_val() is using iio_read_channel_processed() and that will
>> basically only read one of the 2 channels. As I documented, you actually
>> need to read both channel 8 and channel 9 and then compute the value in
>> Kelvins. I'm not sure how you want me to do that in the current framework.
> What are these two channels actually measuring? Is the value of a single
> channel meaningful on it's own? If not it might make sense to update the IIO
> driver to just have one temperature channel.

It's not actually meaningful on its own. So, what you would do is expose
one iio channel for two ADC channels and do the computation in read_raw
? or read_processed ? Then using iio-hwon to export it. ?

Regards,
Lars-Peter Clausen - June 28, 2013, 3:24 p.m.
On 06/28/2013 04:50 PM, Alexandre Belloni wrote:
> On 28/06/2013 16:18, Lars-Peter Clausen wrote:
>> On 06/27/2013 09:26 PM, Alexandre Belloni wrote:
>>>
>>> They are already registered as IIO_TEMP but only implement read_raw. Also,
>>>
>>> iio_hwmon_read_val() is using iio_read_channel_processed() and that will
>>> basically only read one of the 2 channels. As I documented, you actually
>>> need to read both channel 8 and channel 9 and then compute the value in
>>> Kelvins. I'm not sure how you want me to do that in the current framework.
>> What are these two channels actually measuring? Is the value of a single
>> channel meaningful on it's own? If not it might make sense to update the IIO
>> driver to just have one temperature channel.
> 
> It's not actually meaningful on its own. So, what you would do is expose
> one iio channel for two ADC channels and do the computation in read_raw
> ? or read_processed ? Then using iio-hwon to export it. ?
> 
> Regards,
> 

Yes, return channel9 - channel8 as the raw value for the temperature channel
and provide proper scale and offset values, so that
iio_read_channel_processed() will return the correct value.

- Lars
Guenter Roeck - June 28, 2013, 4:35 p.m.
On Fri, Jun 28, 2013 at 05:24:43PM +0200, Lars-Peter Clausen wrote:
> On 06/28/2013 04:50 PM, Alexandre Belloni wrote:
> > On 28/06/2013 16:18, Lars-Peter Clausen wrote:
> >> On 06/27/2013 09:26 PM, Alexandre Belloni wrote:
> >>>
> >>> They are already registered as IIO_TEMP but only implement read_raw. Also,
> >>>
> >>> iio_hwmon_read_val() is using iio_read_channel_processed() and that will
> >>> basically only read one of the 2 channels. As I documented, you actually
> >>> need to read both channel 8 and channel 9 and then compute the value in
> >>> Kelvins. I'm not sure how you want me to do that in the current framework.
> >> What are these two channels actually measuring? Is the value of a single
> >> channel meaningful on it's own? If not it might make sense to update the IIO
> >> driver to just have one temperature channel.
> > 
> > It's not actually meaningful on its own. So, what you would do is expose
> > one iio channel for two ADC channels and do the computation in read_raw
> > ? or read_processed ? Then using iio-hwon to export it. ?
> > 
> > Regards,
> > 
> 
> Yes, return channel9 - channel8 as the raw value for the temperature channel
> and provide proper scale and offset values, so that
> iio_read_channel_processed() will return the correct value.
> 
Agreed.

Guenter

Patch

diff --git a/Documentation/devicetree/bindings/hwmon/mxs-cputemp.txt b/Documentation/devicetree/bindings/hwmon/mxs-cputemp.txt
new file mode 100644
index 0000000..7d3ae47
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/mxs-cputemp.txt
@@ -0,0 +1,18 @@ 
+mxs cputemp hwmon sensors
+-------------------------
+
+See: Documentation/hwmon/mxs-cputemp
+
+Required properties:
+- compatible: should be "fsl,mxs-internal-temp"
+- io-channels: should list the two adc channels needed to calculate the
+	       temperature
+- io-channel-names: should map the previously listed adc channels to the "min"
+		    and "max" value
+
+Example:
+	temp {
+		compatible = "fsl,mxs-internal-temp";
+		io-channels = <&lradc 8>, <&lradc 9>;
+		io-channel-names = "min", "max";
+	};
diff --git a/Documentation/hwmon/mxs-cputemp b/Documentation/hwmon/mxs-cputemp
new file mode 100644
index 0000000..6c6201f
--- /dev/null
+++ b/Documentation/hwmon/mxs-cputemp
@@ -0,0 +1,29 @@ 
+Kernel driver mxs-cputemp
+=========================
+
+Supported chips:
+  * Freescale i.mx28
+    Datasheet: i.MX28 Applications Processor Reference Manual, Rev. 1, 2010
+    	       http://cache.freescale.com/files/dsp/doc/ref_manual/MCIMX28RM.pdf
+
+Author: Alexandre Belloni
+
+Description
+-----------
+This driver permits reading the internal die temperature sensor embedded inside
+Freescale i.mx28 SoCs. This sensor is read through two channels of the on chip
+Low-Resolution ADC. After calculation, the three-sigma error of the temperature
+sensor should be within ± 1.5% in degrees Kelvin. Additionally, the temperature
+sampling has a three-sigma sample-to-sample variation of 2 degrees Kelvin. If
+desired, this error can be removed by oversampling and averaging the temperature
+result.
+
+The formula is:
+	(Channel9 – Channel8) * Gain_correction/4
+
+As recommended by the datasheet, Gain_correction is equal to 1.012.
+
+sysfs entries
+-------------
+temp1_input	Measured and corrected temperature in millidegrees Celsius
+
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 0428e8a..2daf794 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -929,6 +929,16 @@  config SENSORS_MCP3021
 	  This driver can also be built as a module.  If so, the module
 	  will be called mcp3021.
 
+config SENSORS_MXS_CPU
+	tristate "MXS internal CPU temperature sensor"
+	depends on MXS_LRADC
+	help
+	  If you say yes here you get support for the i.mx28 internal
+	  temperature sensor.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called mxs-cputemp
+
 config SENSORS_NCT6775
 	tristate "Nuvoton NCT6775F and compatibles"
 	depends on !PPC
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index d17d3e6..366c92d 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -108,6 +108,7 @@  obj-$(CONFIG_SENSORS_MAX6650)	+= max6650.o
 obj-$(CONFIG_SENSORS_MAX6697)	+= max6697.o
 obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o
 obj-$(CONFIG_SENSORS_MCP3021)	+= mcp3021.o
+obj-$(CONFIG_SENSORS_MXS_CPU)	+= mxs-cputemp.o
 obj-$(CONFIG_SENSORS_NCT6775)	+= nct6775.o
 obj-$(CONFIG_SENSORS_NTC_THERMISTOR)	+= ntc_thermistor.o
 obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
diff --git a/drivers/hwmon/mxs-cputemp.c b/drivers/hwmon/mxs-cputemp.c
new file mode 100644
index 0000000..a312fb5
--- /dev/null
+++ b/drivers/hwmon/mxs-cputemp.c
@@ -0,0 +1,132 @@ 
+/*
+ * Driver for the mxs internal temperature sensor
+ *
+ * Copyright 2013 Free Electrons
+ *
+ * Licensed under the GPLv2 or later.
+ */
+
+#define DRVNAME "mxs-hwmon"
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/iio/consumer.h>
+
+#define GAIN_CORRECTION 1012
+
+/* The value we calculate from the ADCs is in Kelvins, don't forget to convert
+ * it to Celsius */
+#define VALUES_TO_MILLIC(min, max) ((max - min) * GAIN_CORRECTION / 4 - 272150)
+
+struct mxs_hwmon_data {
+	struct device *hwmon_dev;
+	struct iio_channel *chan_min;
+	struct iio_channel *chan_max;
+};
+
+static int mxs_hwmon_show_temp(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	int err, val_min, val_max;
+
+	struct mxs_hwmon_data *data = dev_get_drvdata(dev);
+
+	err = iio_read_channel_raw(data->chan_min, &val_min);
+	if (err < 0)
+		return err;
+
+	err = iio_read_channel_raw(data->chan_max, &val_max);
+	if (err < 0)
+		return err;
+
+	return sprintf(buf, "%u\n", VALUES_TO_MILLIC(val_min, val_max));
+}
+
+static DEVICE_ATTR(temp1_input, S_IRUGO, mxs_hwmon_show_temp, NULL);
+
+static int mxs_hwmon_probe(struct platform_device *pdev)
+{
+	int err;
+	struct mxs_hwmon_data *data;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, data);
+
+	err = device_create_file(&pdev->dev, &dev_attr_temp1_input);
+	if (err)
+		return err;
+
+	data->chan_min = iio_channel_get(&pdev->dev, "min");
+	if (IS_ERR(data->chan_min)) {
+		err = PTR_ERR(data->chan_min);
+		goto error_chan_min;
+	}
+
+	data->chan_max = iio_channel_get(&pdev->dev, "max");
+	if (IS_ERR(data->chan_max)) {
+		err = PTR_ERR(data->chan_max);
+		goto error_chan_max;
+	}
+
+	data->hwmon_dev = hwmon_device_register(&pdev->dev);
+	if (IS_ERR(data->hwmon_dev)) {
+		err = PTR_ERR(data->hwmon_dev);
+		goto error_hwmon;
+	}
+
+	return 0;
+
+error_hwmon:
+	iio_channel_release(data->chan_max);
+error_chan_max:
+	iio_channel_release(data->chan_min);
+error_chan_min:
+	device_remove_file(&pdev->dev, &dev_attr_temp1_input);
+
+	return err;
+}
+
+static int mxs_hwmon_remove(struct platform_device *pdev)
+{
+	struct mxs_hwmon_data *data = platform_get_drvdata(pdev);
+
+	iio_channel_release(data->chan_min);
+	iio_channel_release(data->chan_max);
+	hwmon_device_unregister(data->hwmon_dev);
+
+	device_remove_file(&pdev->dev, &dev_attr_temp1_input);
+
+	return 0;
+}
+
+static struct of_device_id mxs_hwmon_of_match[] = {
+	{ .compatible = "fsl,mxs-internal-temp", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, mxs_hwmon_of_match);
+
+static struct platform_driver mxs_hwmon_driver = {
+	.probe = mxs_hwmon_probe,
+	.remove = mxs_hwmon_remove,
+	.driver	= {
+		.name = DRVNAME,
+		.owner = THIS_MODULE,
+		.of_match_table = mxs_hwmon_of_match,
+	},
+};
+
+module_platform_driver(mxs_hwmon_driver);
+
+MODULE_AUTHOR("Alexandre Belloni <alexandre.belloni@free-electrons.com>");
+MODULE_DESCRIPTION("Freescale i.MX28 hwmon sensor driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:mxs-hwmon");