[PATCH] hwmon:ad7314 port from IIO.

[PATCH] hwmon:ad7314 port from IIO.

[Jonathan Cameron]

I've been moaning about this driver along with the
adt7310, adt7410 and adt75 even since they were
submitted to IIO.  They are all simple temperature
sensors directed at hardware monitoring.

This ad7314 is a very simple part so I've simple
done the port without hardware.  The others are
much more complex beasts.

If anyone wants it more fully featured (for example
I've dropped the clunky support that was in the IIO
driver for powering down between samples) then feel
free to add it.

Main changes from IIO driver.

* dropped mode support
* rewrote the sign extension in temperature read function
  (that was 'interesting' before.)

I'll squish the IIO driver if / when this goes into hwmon.

Entirely plausible I've messed the port up completely so
please take a look!

Thanks,

Jonathan

Jonathan Cameron (1):
  hwmon: ad7314 driver (ported from IIO).

 drivers/hwmon/Kconfig  |   10 +++
 drivers/hwmon/Makefile |    1 +
 drivers/hwmon/ad7314.c |  181 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 192 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/ad7314.c

-- 
1.7.3.4

[PATCH] hwmon: ad7314 driver (ported from IIO).

[Jonathan Cameron]

Currently dropped power down mode support.
Not tested.

Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 drivers/hwmon/Kconfig  |   10 +++
 drivers/hwmon/Makefile |    1 +
 drivers/hwmon/ad7314.c |  181 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 192 insertions(+), 0 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 0b62c3c..e5d4daa 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -68,6 +68,16 @@ config SENSORS_ABITUGURU3
 	  This driver can also be built as a module.  If so, the module
 	  will be called abituguru3.
 
+config SENSORS_AD7314
+	tristate "Analog Devices AD7314 and compatibles"
+	depends on SPI && EXPERIMENTAL
+	help
+	  If you say yes here you get support for the Analog Devices
+	  AD7314, ADT7301 and ADT7302 temperature sensors.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called ad7314.
+
 config SENSORS_AD7414
 	tristate "Analog Devices AD7414"
 	depends on I2C && EXPERIMENTAL
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 3c9ccef..228a4b7 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_SENSORS_W83791D)	+= w83791d.o
 
 obj-$(CONFIG_SENSORS_ABITUGURU)	+= abituguru.o
 obj-$(CONFIG_SENSORS_ABITUGURU3)+= abituguru3.o
+obj-$(CONFIG_SENSORS_AD7314)	+= ad7314.o
 obj-$(CONFIG_SENSORS_AD7414)	+= ad7414.o
 obj-$(CONFIG_SENSORS_AD7418)	+= ad7418.o
 obj-$(CONFIG_SENSORS_ADCXX)	+= adcxx.o
diff --git a/drivers/hwmon/ad7314.c b/drivers/hwmon/ad7314.c
new file mode 100644
index 0000000..9cabe7b
--- /dev/null
+++ b/drivers/hwmon/ad7314.c
@@ -0,0 +1,181 @@
+/*
+ * AD7314 digital temperature sensor driver for AD7314, ADT7301 and ADT7302
+ *
+ * Copyright 2010 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2 or later.
+ *
+ * Conversion to hwmon from IIO done by Jonathan Cameron <jic23@cam.ac.uk>
+ */
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/spi/spi.h>
+#include <linux/module.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+
+/*
+ * AD7314 power mode
+ */
+#define AD7314_PD		0x2000
+
+/*
+ * AD7314 temperature masks
+ */
+#define AD7314_TEMP_SIGN		0x200
+#define AD7314_TEMP_MASK		0x7FE0
+#define AD7314_TEMP_OFFSET		5
+#define AD7314_TEMP_FLOAT_OFFSET	2
+#define AD7314_TEMP_FLOAT_MASK		0x3
+
+/*
+ * ADT7301 and ADT7302 temperature masks
+ */
+#define ADT7301_TEMP_SIGN		0x2000
+#define ADT7301_TEMP_MASK		0x2FFF
+#define ADT7301_TEMP_FLOAT_OFFSET	5
+#define ADT7301_TEMP_FLOAT_MASK		0x1F
+
+enum ad7314_variant {
+	adt7301,
+	adt7302,
+	ad7314,
+};
+
+struct ad7314_data {
+	struct spi_device	*spi_dev;
+	struct device		*hwmon_dev;
+	u16 rx ____cacheline_aligned;
+};
+
+static int ad7314_spi_read(struct spi_device *spi_dev, u16 *data)
+{
+	int ret = 0;
+	u16 value;
+
+	ret = spi_read(spi_dev, (u8 *)&value, sizeof(value));
+	if (ret < 0) {
+		dev_err(&spi_dev->dev, "SPI read error\n");
+		return ret;
+	}
+
+	*data = be16_to_cpu((u16)value);
+
+	return ret;
+}
+
+static ssize_t ad7314_show_temperature(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	struct ad7314_data *chip = dev_get_drvdata(dev);
+	s16 data;
+	int ret;
+
+	ret = ad7314_spi_read(chip->spi_dev, &chip->rx);
+	if (ret < 0)
+		return ret;
+	data = chip->rx;
+	switch (spi_get_device_id(chip->spi_dev)->driver_data) {
+	case ad7314:
+		data = (data & AD7314_TEMP_MASK) >> AD7314_TEMP_OFFSET;
+		data = (data << 6) >> 6;
+
+		return sprintf(buf, "%d.%.2d\n",
+			       data >> AD7314_TEMP_FLOAT_OFFSET,
+			       (data & AD7314_TEMP_FLOAT_MASK) * 25);
+	case adt7301:
+	case adt7302:
+		data &= ADT7301_TEMP_MASK;
+		data = (data << 3) >> 3;
+
+		return sprintf(buf, "%d.%.5d\n",
+			       data >> ADT7301_TEMP_FLOAT_OFFSET,
+			       (data & ADT7301_TEMP_FLOAT_MASK) * 3125);
+	default:
+		return -EINVAL;
+	}
+}
+
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO,
+			  ad7314_show_temperature, NULL, 0);
+
+static struct attribute *ad7314_attributes[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group ad7314_group = {
+	.attrs = ad7314_attributes,
+};
+
+static int __devinit ad7314_probe(struct spi_device *spi_dev)
+{
+	int ret;
+	struct ad7314_data *chip;
+
+	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+	if (chip == NULL) {
+		ret = -ENOMEM;
+		goto error_ret;
+	}
+	dev_set_drvdata(&spi_dev->dev, chip);
+
+	ret = sysfs_create_group(&spi_dev->dev.kobj, &ad7314_group);
+	if (ret < 0)
+		goto error_free_chip;
+
+	return 0;
+error_free_chip:
+	kfree(chip);
+error_ret:
+	return ret;
+}
+
+static int __devexit ad7314_remove(struct spi_device *spi_dev)
+{
+	struct ad7314_data *chip = dev_get_drvdata(&spi_dev->dev);
+
+	hwmon_device_unregister(chip->hwmon_dev);
+	sysfs_remove_group(&spi_dev->dev.kobj, &ad7314_group);
+	kfree(chip);
+
+	return 0;
+}
+
+static const struct spi_device_id ad7314_id[] = {
+	{ "adt7301", adt7301 },
+	{ "adt7302", adt7302 },
+	{ "ad7314", ad7314 },
+	{}
+};
+
+static struct spi_driver ad7314_driver = {
+	.driver = {
+		.name = "ad7314",
+		.bus = &spi_bus_type,
+		.owner = THIS_MODULE,
+	},
+	.probe = ad7314_probe,
+	.remove = __devexit_p(ad7314_remove),
+	.id_table = ad7314_id,
+};
+
+static __init int ad7314_init(void)
+{
+	return spi_register_driver(&ad7314_driver);
+}
+module_init(ad7314_init);
+
+static __exit void ad7314_exit(void)
+{
+	spi_unregister_driver(&ad7314_driver);
+}
+module_exit(ad7314_exit);
+
+MODULE_AUTHOR("Sonic Zhang <sonic.zhang@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD7314, ADT7301 and ADT7302 digital"
+			" temperature sensor driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.3.4

Re: [lm-sensors] [PATCH] hwmon: ad7314 driver (ported from IIO).

[Guenter Roeck]

Hi Jonathan,

On Wed, 2011-09-28 at 08:57 -0400, Jonathan Cameron wrote:
> Currently dropped power down mode support.
> Not tested.
> 
Guess you don't really want the above in the commit log.

I requested samples for all three chips. Should be simple enough to test
once I have also written a SPI master driver for one of my
controllers ;).

> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
> ---
>  drivers/hwmon/Kconfig  |   10 +++
>  drivers/hwmon/Makefile |    1 +
>  drivers/hwmon/ad7314.c |  181 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 192 insertions(+), 0 deletions(-)
> 
We would also like to see
	Documentation/hwmon/ad7314

Does this file or an equivalent of it exist in the iio tree ?

> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 0b62c3c..e5d4daa 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -68,6 +68,16 @@ config SENSORS_ABITUGURU3
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called abituguru3.
>  
> +config SENSORS_AD7314
> +	tristate "Analog Devices AD7314 and compatibles"
> +	depends on SPI && EXPERIMENTAL
> +	help
> +	  If you say yes here you get support for the Analog Devices
> +	  AD7314, ADT7301 and ADT7302 temperature sensors.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called ad7314.
> +
>  config SENSORS_AD7414
>  	tristate "Analog Devices AD7414"
>  	depends on I2C && EXPERIMENTAL
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 3c9ccef..228a4b7 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_SENSORS_W83791D)	+= w83791d.o
>  
>  obj-$(CONFIG_SENSORS_ABITUGURU)	+= abituguru.o
>  obj-$(CONFIG_SENSORS_ABITUGURU3)+= abituguru3.o
> +obj-$(CONFIG_SENSORS_AD7314)	+= ad7314.o
>  obj-$(CONFIG_SENSORS_AD7414)	+= ad7414.o
>  obj-$(CONFIG_SENSORS_AD7418)	+= ad7418.o
>  obj-$(CONFIG_SENSORS_ADCXX)	+= adcxx.o
> diff --git a/drivers/hwmon/ad7314.c b/drivers/hwmon/ad7314.c
> new file mode 100644
> index 0000000..9cabe7b
> --- /dev/null
> +++ b/drivers/hwmon/ad7314.c
> @@ -0,0 +1,181 @@
> +/*
> + * AD7314 digital temperature sensor driver for AD7314, ADT7301 and ADT7302
> + *
> + * Copyright 2010 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2 or later.
> + *
> + * Conversion to hwmon from IIO done by Jonathan Cameron <jic23@cam.ac.uk>
> + */
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/spi/spi.h>
> +#include <linux/module.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +
> +/*
> + * AD7314 power mode
> + */
> +#define AD7314_PD		0x2000
> +
> +/*
> + * AD7314 temperature masks
> + */
> +#define AD7314_TEMP_SIGN		0x200
> +#define AD7314_TEMP_MASK		0x7FE0
> +#define AD7314_TEMP_OFFSET		5
> +#define AD7314_TEMP_FLOAT_OFFSET	2
> +#define AD7314_TEMP_FLOAT_MASK		0x3
> +
> +/*
> + * ADT7301 and ADT7302 temperature masks
> + */
> +#define ADT7301_TEMP_SIGN		0x2000
> +#define ADT7301_TEMP_MASK		0x2FFF
> +#define ADT7301_TEMP_FLOAT_OFFSET	5
> +#define ADT7301_TEMP_FLOAT_MASK		0x1F
> +
> +enum ad7314_variant {
> +	adt7301,
> +	adt7302,
> +	ad7314,
> +};
> +
> +struct ad7314_data {
> +	struct spi_device	*spi_dev;
> +	struct device		*hwmon_dev;
> +	u16 rx ____cacheline_aligned;
> +};
> +
> +static int ad7314_spi_read(struct spi_device *spi_dev, u16 *data)
> +{
> +	int ret = 0;

No need to initialize ret.

> +	u16 value;
> +
> +	ret = spi_read(spi_dev, (u8 *)&value, sizeof(value));

I am having a bit of trouble here. value is not cache aligned, yet you
use it for the spi transaction. data is cache aligned, but you only
store the result from the spi transaction into it.

Given this, either the code doesn't work, or "rx" is not needed at all
since you don't use it for a transaction which would require or ask for
cache alignment.

Am I missing something ?

> +	if (ret < 0) {
> +		dev_err(&spi_dev->dev, "SPI read error\n");
> +		return ret;
> +	}
> +
> +	*data = be16_to_cpu((u16)value);

value is defined as u16 and does not need a type cast.

> +
> +	return ret;
> +}
> +
> +static ssize_t ad7314_show_temperature(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	struct ad7314_data *chip = dev_get_drvdata(dev);
> +	s16 data;
> +	int ret;
> +
> +	ret = ad7314_spi_read(chip->spi_dev, &chip->rx);
> +	if (ret < 0)
> +		return ret;
> +	data = chip->rx;
> +	switch (spi_get_device_id(chip->spi_dev)->driver_data) {
> +	case ad7314:
> +		data = (data & AD7314_TEMP_MASK) >> AD7314_TEMP_OFFSET;
> +		data = (data << 6) >> 6;
> +
> +		return sprintf(buf, "%d.%.2d\n",
> +			       data >> AD7314_TEMP_FLOAT_OFFSET,
> +			       (data & AD7314_TEMP_FLOAT_MASK) * 25);

hwmon reports temperatures in milli-degrees C, and does not know about
decimal points. So this should be something like

		return sprintf(buf, "%d\n", data * 250);

> +	case adt7301:
> +	case adt7302:
> +		data &= ADT7301_TEMP_MASK;
> +		data = (data << 3) >> 3;
> +
> +		return sprintf(buf, "%d.%.5d\n",
> +			       data >> ADT7301_TEMP_FLOAT_OFFSET,
> +			       (data & ADT7301_TEMP_FLOAT_MASK) * 3125);

Similar, this should be something like

		return sprintf(buf, "%d\n", (data * 1000) >> 5);

> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO,
> +			  ad7314_show_temperature, NULL, 0);
> +
> +static struct attribute *ad7314_attributes[] = {
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group ad7314_group = {
> +	.attrs = ad7314_attributes,
> +};
> +
> +static int __devinit ad7314_probe(struct spi_device *spi_dev)
> +{
> +	int ret;
> +	struct ad7314_data *chip;
> +
> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> +	if (chip == NULL) {
> +		ret = -ENOMEM;
> +		goto error_ret;
> +	}
> +	dev_set_drvdata(&spi_dev->dev, chip);
> +
> +	ret = sysfs_create_group(&spi_dev->dev.kobj, &ad7314_group);
> +	if (ret < 0)
> +		goto error_free_chip;
> +
> +	return 0;
> +error_free_chip:
> +	kfree(chip);
> +error_ret:
> +	return ret;
> +}
> +
> +static int __devexit ad7314_remove(struct spi_device *spi_dev)
> +{
> +	struct ad7314_data *chip = dev_get_drvdata(&spi_dev->dev);
> +
> +	hwmon_device_unregister(chip->hwmon_dev);
> +	sysfs_remove_group(&spi_dev->dev.kobj, &ad7314_group);
> +	kfree(chip);
> +
> +	return 0;
> +}
> +
> +static const struct spi_device_id ad7314_id[] = {
> +	{ "adt7301", adt7301 },
> +	{ "adt7302", adt7302 },
> +	{ "ad7314", ad7314 },
> +	{}
> +};
> +
> +static struct spi_driver ad7314_driver = {
> +	.driver = {
> +		.name = "ad7314",
> +		.bus = &spi_bus_type,
> +		.owner = THIS_MODULE,
> +	},
> +	.probe = ad7314_probe,
> +	.remove = __devexit_p(ad7314_remove),
> +	.id_table = ad7314_id,
> +};
> +
> +static __init int ad7314_init(void)
> +{
> +	return spi_register_driver(&ad7314_driver);
> +}
> +module_init(ad7314_init);
> +
> +static __exit void ad7314_exit(void)
> +{
> +	spi_unregister_driver(&ad7314_driver);
> +}
> +module_exit(ad7314_exit);
> +
> +MODULE_AUTHOR("Sonic Zhang <sonic.zhang@analog.com>");
> +MODULE_DESCRIPTION("Analog Devices AD7314, ADT7301 and ADT7302 digital"
> +			" temperature sensor driver");
> +MODULE_LICENSE("GPL v2");

Re: [lm-sensors] [PATCH] hwmon: ad7314 driver (ported from IIO).

[Jonathan Cameron]

On 09/28/11 17:03, Guenter Roeck wrote:
> Hi Jonathan,
> 
> On Wed, 2011-09-28 at 08:57 -0400, Jonathan Cameron wrote:
>> Currently dropped power down mode support.
>> Not tested.
>>
> Guess you don't really want the above in the commit log.
Well, I'm crossing my fingers someone might give me a tested-by ;)

> 
> I requested samples for all three chips. Should be simple enough to test
> once I have also written a SPI master driver for one of my
> controllers ;).
Nice ;)

Thanks for the review and sorry for the dumb ass mistakes.
Not my best work. oops.
> 
>> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
>> ---
>>  drivers/hwmon/Kconfig  |   10 +++
>>  drivers/hwmon/Makefile |    1 +
>>  drivers/hwmon/ad7314.c |  181 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 192 insertions(+), 0 deletions(-)
>>
> We would also like to see
> 	Documentation/hwmon/ad7314
> 
> Does this file or an equivalent of it exist in the iio tree ?
Nope, I'll make one up.
> 
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 0b62c3c..e5d4daa 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -68,6 +68,16 @@ config SENSORS_ABITUGURU3
>>  	  This driver can also be built as a module.  If so, the module
>>  	  will be called abituguru3.
>>  
>> +config SENSORS_AD7314
>> +	tristate "Analog Devices AD7314 and compatibles"
>> +	depends on SPI && EXPERIMENTAL
>> +	help
>> +	  If you say yes here you get support for the Analog Devices
>> +	  AD7314, ADT7301 and ADT7302 temperature sensors.
>> +
>> +	  This driver can also be built as a module. If so, the module
>> +	  will be called ad7314.
>> +
>>  config SENSORS_AD7414
>>  	tristate "Analog Devices AD7414"
>>  	depends on I2C && EXPERIMENTAL
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 3c9ccef..228a4b7 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -21,6 +21,7 @@ obj-$(CONFIG_SENSORS_W83791D)	+= w83791d.o
>>  
>>  obj-$(CONFIG_SENSORS_ABITUGURU)	+= abituguru.o
>>  obj-$(CONFIG_SENSORS_ABITUGURU3)+= abituguru3.o
>> +obj-$(CONFIG_SENSORS_AD7314)	+= ad7314.o
>>  obj-$(CONFIG_SENSORS_AD7414)	+= ad7414.o
>>  obj-$(CONFIG_SENSORS_AD7418)	+= ad7418.o
>>  obj-$(CONFIG_SENSORS_ADCXX)	+= adcxx.o
>> diff --git a/drivers/hwmon/ad7314.c b/drivers/hwmon/ad7314.c
>> new file mode 100644
>> index 0000000..9cabe7b
>> --- /dev/null
>> +++ b/drivers/hwmon/ad7314.c
>> @@ -0,0 +1,181 @@
>> +/*
>> + * AD7314 digital temperature sensor driver for AD7314, ADT7301 and ADT7302
>> + *
>> + * Copyright 2010 Analog Devices Inc.
>> + *
>> + * Licensed under the GPL-2 or later.
>> + *
>> + * Conversion to hwmon from IIO done by Jonathan Cameron <jic23@cam.ac.uk>
>> + */
>> +#include <linux/device.h>
>> +#include <linux/kernel.h>
>> +#include <linux/slab.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/module.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +
>> +/*
>> + * AD7314 power mode
>> + */
>> +#define AD7314_PD		0x2000
>> +
>> +/*
>> + * AD7314 temperature masks
>> + */
>> +#define AD7314_TEMP_SIGN		0x200
>> +#define AD7314_TEMP_MASK		0x7FE0
>> +#define AD7314_TEMP_OFFSET		5
>> +#define AD7314_TEMP_FLOAT_OFFSET	2
>> +#define AD7314_TEMP_FLOAT_MASK		0x3
>> +
>> +/*
>> + * ADT7301 and ADT7302 temperature masks
>> + */
>> +#define ADT7301_TEMP_SIGN		0x2000
>> +#define ADT7301_TEMP_MASK		0x2FFF
>> +#define ADT7301_TEMP_FLOAT_OFFSET	5
>> +#define ADT7301_TEMP_FLOAT_MASK		0x1F
>> +
>> +enum ad7314_variant {
>> +	adt7301,
>> +	adt7302,
>> +	ad7314,
>> +};
>> +
>> +struct ad7314_data {
>> +	struct spi_device	*spi_dev;
>> +	struct device		*hwmon_dev;
>> +	u16 rx ____cacheline_aligned;
>> +};
>> +
>> +static int ad7314_spi_read(struct spi_device *spi_dev, u16 *data)
>> +{
>> +	int ret = 0;
> 
> No need to initialize ret.
> 
>> +	u16 value;
>> +
>> +	ret = spi_read(spi_dev, (u8 *)&value, sizeof(value));
> 
> I am having a bit of trouble here. value is not cache aligned, yet you
> use it for the spi transaction. data is cache aligned, but you only
> store the result from the spi transaction into in.

> 
> Given this, either the code doesn't work, or "rx" is not needed at all
> since you don't use it for a transaction which would require or ask for
> cache alignment.
> 
> Am I missing something ?
Nope, I'm just being stupid. Sometimes I assume code does what I
expect and then don't notice it isn't doing so.
Will fix.
> 
>> +	if (ret < 0) {
>> +		dev_err(&spi_dev->dev, "SPI read error\n");
>> +		return ret;
>> +	}
>> +
>> +	*data = be16_to_cpu((u16)value);
> 
> value is defined as u16 and does not need a type cast.
> 
>> +
>> +	return ret;
>> +}
>> +
>> +static ssize_t ad7314_show_temperature(struct device *dev,
>> +		struct device_attribute *attr,
>> +		char *buf)
>> +{
>> +	struct ad7314_data *chip = dev_get_drvdata(dev);
>> +	s16 data;
>> +	int ret;
>> +
>> +	ret = ad7314_spi_read(chip->spi_dev, &chip->rx);
>> +	if (ret < 0)
>> +		return ret;
>> +	data = chip->rx;
>> +	switch (spi_get_device_id(chip->spi_dev)->driver_data) {
>> +	case ad7314:
>> +		data = (data & AD7314_TEMP_MASK) >> AD7314_TEMP_OFFSET;
>> +		data = (data << 6) >> 6;
>> +
>> +		return sprintf(buf, "%d.%.2d\n",
>> +			       data >> AD7314_TEMP_FLOAT_OFFSET,
>> +			       (data & AD7314_TEMP_FLOAT_MASK) * 25);
> 
> hwmon reports temperatures in milli-degrees C, and does not know about
> decimal points. So this should be something like
> 
> 		return sprintf(buf, "%d\n", data * 250);
Sorry, I'm half asleep today and forgot that.  Will fix. 
> 
>> +	case adt7301:
>> +	case adt7302:
>> +		data &= ADT7301_TEMP_MASK;
>> +		data = (data << 3) >> 3;
>> +
>> +		return sprintf(buf, "%d.%.5d\n",
>> +			       data >> ADT7301_TEMP_FLOAT_OFFSET,
>> +			       (data & ADT7301_TEMP_FLOAT_MASK) * 3125);
> 
> Similar, this should be something like
> 
> 		return sprintf(buf, "%d\n", (data * 1000) >> 5);
> 
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO,
>> +			  ad7314_show_temperature, NULL, 0);
>> +
>> +static struct attribute *ad7314_attributes[] = {
>> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
>> +	NULL,
>> +};
>> +
>> +static const struct attribute_group ad7314_group = {
>> +	.attrs = ad7314_attributes,
>> +};
>> +
>> +static int __devinit ad7314_probe(struct spi_device *spi_dev)
>> +{
>> +	int ret;
>> +	struct ad7314_data *chip;
>> +
>> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>> +	if (chip == NULL) {
>> +		ret = -ENOMEM;
>> +		goto error_ret;
>> +	}
>> +	dev_set_drvdata(&spi_dev->dev, chip);
>> +
>> +	ret = sysfs_create_group(&spi_dev->dev.kobj, &ad7314_group);
>> +	if (ret < 0)
>> +		goto error_free_chip;
>> +
>> +	return 0;
>> +error_free_chip:
>> +	kfree(chip);
>> +error_ret:
>> +	return ret;
>> +}
>> +
>> +static int __devexit ad7314_remove(struct spi_device *spi_dev)
>> +{
>> +	struct ad7314_data *chip = dev_get_drvdata(&spi_dev->dev);
>> +
>> +	hwmon_device_unregister(chip->hwmon_dev);
>> +	sysfs_remove_group(&spi_dev->dev.kobj, &ad7314_group);
>> +	kfree(chip);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct spi_device_id ad7314_id[] = {
>> +	{ "adt7301", adt7301 },
>> +	{ "adt7302", adt7302 },
>> +	{ "ad7314", ad7314 },
>> +	{}
>> +};
>> +
>> +static struct spi_driver ad7314_driver = {
>> +	.driver = {
>> +		.name = "ad7314",
>> +		.bus = &spi_bus_type,
>> +		.owner = THIS_MODULE,
>> +	},
>> +	.probe = ad7314_probe,
>> +	.remove = __devexit_p(ad7314_remove),
>> +	.id_table = ad7314_id,
>> +};
>> +
>> +static __init int ad7314_init(void)
>> +{
>> +	return spi_register_driver(&ad7314_driver);
>> +}
>> +module_init(ad7314_init);
>> +
>> +static __exit void ad7314_exit(void)
>> +{
>> +	spi_unregister_driver(&ad7314_driver);
>> +}
>> +module_exit(ad7314_exit);
>> +
>> +MODULE_AUTHOR("Sonic Zhang <sonic.zhang@analog.com>");
>> +MODULE_DESCRIPTION("Analog Devices AD7314, ADT7301 and ADT7302 digital"
>> +			" temperature sensor driver");
>> +MODULE_LICENSE("GPL v2");
> 
> 
> 

[PATCH V2] hwmon: ad7314 driver (ported from IIO).

[Jonathan Cameron]

Currently dropped power down mode support.
V2: Fixes:
- messed up use of cache aligned store fixed.
- correct output format to be milli degrees.
- add documentation file.

Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
Untested, but trivial port.

 Documentation/hwmon/ad7314 |   25 ++++++
 drivers/hwmon/Kconfig      |   10 +++
 drivers/hwmon/Makefile     |    1 +
 drivers/hwmon/ad7314.c     |  176 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 212 insertions(+), 0 deletions(-)

diff --git a/Documentation/hwmon/ad7314 b/Documentation/hwmon/ad7314
new file mode 100644
index 0000000..1b026a0
--- /dev/null
+++ b/Documentation/hwmon/ad7314
@@ -0,0 +1,25 @@
+Kernel driver ad7314
+====================
+
+Supported chips:
+   * Analog Devices AD7314
+     Prefx: 'ad7314'
+     Datasheet: Publicaly available at Analog Devices website.
+   * Analog Devices ADT7301
+     Prefx: 'adt7301'
+     Datasheet: Publicaly available at Analog Devices website.
+   * Analog Devices ADT7302
+     Prefx: 'adt7302'
+     Datasheet: Publicaly available at Analog Devices website.
+
+Description
+-----------
+
+Driver supports the above parts.  The ad7314 has a 10 bit
+sensor with 1lsb = 0.25 degrees centigrade. The adt7301 and
+adt7302 have 14 bit sensors with 1lsb = 0.03125 degrees centigrade.
+
+Notes
+-----
+
+Currently power down mode is not supported.
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 0b62c3c..e5d4daa 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -68,6 +68,16 @@ config SENSORS_ABITUGURU3
 	  This driver can also be built as a module.  If so, the module
 	  will be called abituguru3.
 
+config SENSORS_AD7314
+	tristate "Analog Devices AD7314 and compatibles"
+	depends on SPI && EXPERIMENTAL
+	help
+	  If you say yes here you get support for the Analog Devices
+	  AD7314, ADT7301 and ADT7302 temperature sensors.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called ad7314.
+
 config SENSORS_AD7414
 	tristate "Analog Devices AD7414"
 	depends on I2C && EXPERIMENTAL
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 3c9ccef..228a4b7 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_SENSORS_W83791D)	+= w83791d.o
 
 obj-$(CONFIG_SENSORS_ABITUGURU)	+= abituguru.o
 obj-$(CONFIG_SENSORS_ABITUGURU3)+= abituguru3.o
+obj-$(CONFIG_SENSORS_AD7314)	+= ad7314.o
 obj-$(CONFIG_SENSORS_AD7414)	+= ad7414.o
 obj-$(CONFIG_SENSORS_AD7418)	+= ad7418.o
 obj-$(CONFIG_SENSORS_ADCXX)	+= adcxx.o
diff --git a/drivers/hwmon/ad7314.c b/drivers/hwmon/ad7314.c
new file mode 100644
index 0000000..a1fea9b
--- /dev/null
+++ b/drivers/hwmon/ad7314.c
@@ -0,0 +1,176 @@
+/*
+ * AD7314 digital temperature sensor driver for AD7314, ADT7301 and ADT7302
+ *
+ * Copyright 2010 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2 or later.
+ *
+ * Conversion to hwmon from IIO done by Jonathan Cameron <jic23@cam.ac.uk>
+ */
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/spi/spi.h>
+#include <linux/module.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+
+/*
+ * AD7314 power mode
+ */
+#define AD7314_PD		0x2000
+
+/*
+ * AD7314 temperature masks
+ */
+#define AD7314_TEMP_SIGN		0x200
+#define AD7314_TEMP_MASK		0x7FE0
+#define AD7314_TEMP_OFFSET		5
+
+/*
+ * ADT7301 and ADT7302 temperature masks
+ */
+#define ADT7301_TEMP_SIGN		0x2000
+#define ADT7301_TEMP_MASK		0x3FFF
+
+enum ad7314_variant {
+	adt7301,
+	adt7302,
+	ad7314,
+};
+
+struct ad7314_data {
+	struct spi_device	*spi_dev;
+	struct device		*hwmon_dev;
+	u16 rx ____cacheline_aligned;
+};
+
+static int ad7314_spi_read(struct ad7314_data *chip, s16 *data)
+{
+	int ret;
+
+	ret = spi_read(chip->spi_dev, (u8 *)&chip->rx, sizeof(chip->rx));
+	if (ret < 0) {
+		dev_err(&chip->spi_dev->dev, "SPI read error\n");
+		return ret;
+	}
+
+	*data = be16_to_cpu(chip->rx);
+
+	return ret;
+}
+
+static ssize_t ad7314_show_temperature(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	struct ad7314_data *chip = dev_get_drvdata(dev);
+	s16 data;
+	int ret;
+
+	ret = ad7314_spi_read(chip, &data);
+	if (ret < 0)
+		return ret;
+	switch (spi_get_device_id(chip->spi_dev)->driver_data) {
+	case ad7314:
+		data = (data & AD7314_TEMP_MASK) >> AD7314_TEMP_OFFSET;
+		data = (data << 6) >> 6;
+
+		return sprintf(buf, "%d\n", 250 * data);
+	case adt7301:
+	case adt7302:
+		/*
+		 * Documented as a 13 bit twos complement register
+		 * with a sign bit - which is a 14 bit 2's complement
+		 * register.  1lsb - 31.25 milli degrees centigrade
+		 */
+		data &= ADT7301_TEMP_MASK;
+		data = (data << 2) >> 2;
+
+		return sprintf(buf, "%d\n", (3125 * (int)data)/100);
+	default:
+		return -EINVAL;
+	}
+}
+
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO,
+			  ad7314_show_temperature, NULL, 0);
+
+static struct attribute *ad7314_attributes[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group ad7314_group = {
+	.attrs = ad7314_attributes,
+};
+
+static int __devinit ad7314_probe(struct spi_device *spi_dev)
+{
+	int ret;
+	struct ad7314_data *chip;
+
+	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+	if (chip == NULL) {
+		ret = -ENOMEM;
+		goto error_ret;
+	}
+	dev_set_drvdata(&spi_dev->dev, chip);
+
+	ret = sysfs_create_group(&spi_dev->dev.kobj, &ad7314_group);
+	if (ret < 0)
+		goto error_free_chip;
+
+	return 0;
+error_free_chip:
+	kfree(chip);
+error_ret:
+	return ret;
+}
+
+static int __devexit ad7314_remove(struct spi_device *spi_dev)
+{
+	struct ad7314_data *chip = dev_get_drvdata(&spi_dev->dev);
+
+	hwmon_device_unregister(chip->hwmon_dev);
+	sysfs_remove_group(&spi_dev->dev.kobj, &ad7314_group);
+	kfree(chip);
+
+	return 0;
+}
+
+static const struct spi_device_id ad7314_id[] = {
+	{ "adt7301", adt7301 },
+	{ "adt7302", adt7302 },
+	{ "ad7314", ad7314 },
+	{}
+};
+
+static struct spi_driver ad7314_driver = {
+	.driver = {
+		.name = "ad7314",
+		.bus = &spi_bus_type,
+		.owner = THIS_MODULE,
+	},
+	.probe = ad7314_probe,
+	.remove = __devexit_p(ad7314_remove),
+	.id_table = ad7314_id,
+};
+
+static __init int ad7314_init(void)
+{
+	return spi_register_driver(&ad7314_driver);
+}
+module_init(ad7314_init);
+
+static __exit void ad7314_exit(void)
+{
+	spi_unregister_driver(&ad7314_driver);
+}
+module_exit(ad7314_exit);
+
+MODULE_AUTHOR("Sonic Zhang <sonic.zhang@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD7314, ADT7301 and ADT7302 digital"
+			" temperature sensor driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.3.4

Re: [PATCH V2] hwmon: ad7314 driver (ported from IIO).

[Guenter Roeck]

On Thu, 2011-09-29 at 08:11 -0400, Jonathan Cameron wrote:
> Currently dropped power down mode support.
> V2: Fixes:
> - messed up use of cache aligned store fixed.
> - correct output format to be milli degrees.
> - add documentation file.
> 
> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>

Hi Jonathan,

almost there ...

> ---
> Untested, but trivial port.
> 
>  Documentation/hwmon/ad7314 |   25 ++++++
>  drivers/hwmon/Kconfig      |   10 +++
>  drivers/hwmon/Makefile     |    1 +
>  drivers/hwmon/ad7314.c     |  176 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 212 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/hwmon/ad7314 b/Documentation/hwmon/ad7314
> new file mode 100644
> index 0000000..1b026a0
> --- /dev/null
> +++ b/Documentation/hwmon/ad7314
> @@ -0,0 +1,25 @@
> +Kernel driver ad7314
> +====================
> +
> +Supported chips:
> +   * Analog Devices AD7314
> +     Prefx: 'ad7314'
> +     Datasheet: Publicaly available at Analog Devices website.
> +   * Analog Devices ADT7301
> +     Prefx: 'adt7301'
> +     Datasheet: Publicaly available at Analog Devices website.
> +   * Analog Devices ADT7302
> +     Prefx: 'adt7302'
> +     Datasheet: Publicaly available at Analog Devices website.

Publicaly -> Publicly

> +
> +Description
> +-----------
> +
> +Driver supports the above parts.  The ad7314 has a 10 bit
> +sensor with 1lsb = 0.25 degrees centigrade. The adt7301 and
> +adt7302 have 14 bit sensors with 1lsb = 0.03125 degrees centigrade.
> +
> +Notes
> +-----
> +
> +Currently power down mode is not supported.
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 0b62c3c..e5d4daa 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -68,6 +68,16 @@ config SENSORS_ABITUGURU3
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called abituguru3.
>  
> +config SENSORS_AD7314
> +	tristate "Analog Devices AD7314 and compatibles"
> +	depends on SPI && EXPERIMENTAL
> +	help
> +	  If you say yes here you get support for the Analog Devices
> +	  AD7314, ADT7301 and ADT7302 temperature sensors.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called ad7314.
> +
>  config SENSORS_AD7414
>  	tristate "Analog Devices AD7414"
>  	depends on I2C && EXPERIMENTAL
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 3c9ccef..228a4b7 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_SENSORS_W83791D)	+= w83791d.o
>  
>  obj-$(CONFIG_SENSORS_ABITUGURU)	+= abituguru.o
>  obj-$(CONFIG_SENSORS_ABITUGURU3)+= abituguru3.o
> +obj-$(CONFIG_SENSORS_AD7314)	+= ad7314.o
>  obj-$(CONFIG_SENSORS_AD7414)	+= ad7414.o
>  obj-$(CONFIG_SENSORS_AD7418)	+= ad7418.o
>  obj-$(CONFIG_SENSORS_ADCXX)	+= adcxx.o
> diff --git a/drivers/hwmon/ad7314.c b/drivers/hwmon/ad7314.c
> new file mode 100644
> index 0000000..a1fea9b
> --- /dev/null
> +++ b/drivers/hwmon/ad7314.c
> @@ -0,0 +1,176 @@
> +/*
> + * AD7314 digital temperature sensor driver for AD7314, ADT7301 and ADT7302
> + *
> + * Copyright 2010 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2 or later.
> + *
> + * Conversion to hwmon from IIO done by Jonathan Cameron <jic23@cam.ac.uk>
> + */
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/spi/spi.h>
> +#include <linux/module.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +
> +/*
> + * AD7314 power mode
> + */
> +#define AD7314_PD		0x2000
> +
> +/*
> + * AD7314 temperature masks
> + */
> +#define AD7314_TEMP_SIGN		0x200
> +#define AD7314_TEMP_MASK		0x7FE0
> +#define AD7314_TEMP_OFFSET		5
> +
> +/*
> + * ADT7301 and ADT7302 temperature masks
> + */
> +#define ADT7301_TEMP_SIGN		0x2000
> +#define ADT7301_TEMP_MASK		0x3FFF
> +
> +enum ad7314_variant {
> +	adt7301,
> +	adt7302,
> +	ad7314,
> +};
> +
> +struct ad7314_data {
> +	struct spi_device	*spi_dev;
> +	struct device		*hwmon_dev;
> +	u16 rx ____cacheline_aligned;
> +};
> +
> +static int ad7314_spi_read(struct ad7314_data *chip, s16 *data)
> +{
> +	int ret;
> +
> +	ret = spi_read(chip->spi_dev, (u8 *)&chip->rx, sizeof(chip->rx));
> +	if (ret < 0) {
> +		dev_err(&chip->spi_dev->dev, "SPI read error\n");
> +		return ret;
> +	}
> +
> +	*data = be16_to_cpu(chip->rx);
> +
> +	return ret;
> +}
> +
> +static ssize_t ad7314_show_temperature(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	struct ad7314_data *chip = dev_get_drvdata(dev);
> +	s16 data;
> +	int ret;
> +
> +	ret = ad7314_spi_read(chip, &data);
> +	if (ret < 0)
> +		return ret;
> +	switch (spi_get_device_id(chip->spi_dev)->driver_data) {
> +	case ad7314:
> +		data = (data & AD7314_TEMP_MASK) >> AD7314_TEMP_OFFSET;
> +		data = (data << 6) >> 6;
> +
> +		return sprintf(buf, "%d\n", 250 * data);
> +	case adt7301:
> +	case adt7302:
> +		/*
> +		 * Documented as a 13 bit twos complement register
> +		 * with a sign bit - which is a 14 bit 2's complement
> +		 * register.  1lsb - 31.25 milli degrees centigrade
> +		 */
> +		data &= ADT7301_TEMP_MASK;
> +		data = (data << 2) >> 2;
> +
> +		return sprintf(buf, "%d\n", (3125 * (int)data)/100);

You don't need a typecast here (like with the first sprintf). There
should be spaces around '/' (CodingStyle 3.1). 
Even better would be to use
		DIV_ROUND_CLOSEST(data * 3125, 100)
to reduce the rounding error.

> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO,
> +			  ad7314_show_temperature, NULL, 0);
> +
> +static struct attribute *ad7314_attributes[] = {
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group ad7314_group = {
> +	.attrs = ad7314_attributes,
> +};
> +
> +static int __devinit ad7314_probe(struct spi_device *spi_dev)
> +{
> +	int ret;
> +	struct ad7314_data *chip;
> +
> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> +	if (chip == NULL) {
> +		ret = -ENOMEM;
> +		goto error_ret;
> +	}
> +	dev_set_drvdata(&spi_dev->dev, chip);
> +
> +	ret = sysfs_create_group(&spi_dev->dev.kobj, &ad7314_group);
> +	if (ret < 0)
> +		goto error_free_chip;
> +
Something missing here which I didn't notice at first ... but you do
remove the hwmon device below, so it makes sense to create it ;).

	chip->hwmon_dev = hwmon_device_register(&spi_dev->dev);
        if (IS_ERR(chip->hwmon_dev)) {
                err = PTR_ERR(chip->hwmon_dev);
                goto error_remove_group;
        }

> +	return 0;
error_remove_group:
	sysfs_remove_group(&spi_dev->dev.kobj, &ad7314_group);

> +error_free_chip:
> +	kfree(chip);
> +error_ret:
> +	return ret;
> +}
> +
> +static int __devexit ad7314_remove(struct spi_device *spi_dev)
> +{
> +	struct ad7314_data *chip = dev_get_drvdata(&spi_dev->dev);
> +
> +	hwmon_device_unregister(chip->hwmon_dev);
> +	sysfs_remove_group(&spi_dev->dev.kobj, &ad7314_group);
> +	kfree(chip);
> +
> +	return 0;
> +}
> +
> +static const struct spi_device_id ad7314_id[] = {
> +	{ "adt7301", adt7301 },
> +	{ "adt7302", adt7302 },
> +	{ "ad7314", ad7314 },
> +	{}
> +};
> +
> +static struct spi_driver ad7314_driver = {
> +	.driver = {
> +		.name = "ad7314",
> +		.bus = &spi_bus_type,
> +		.owner = THIS_MODULE,
> +	},
> +	.probe = ad7314_probe,
> +	.remove = __devexit_p(ad7314_remove),
> +	.id_table = ad7314_id,
> +};
> +
> +static __init int ad7314_init(void)
> +{
> +	return spi_register_driver(&ad7314_driver);
> +}
> +module_init(ad7314_init);
> +
> +static __exit void ad7314_exit(void)
> +{
> +	spi_unregister_driver(&ad7314_driver);
> +}
> +module_exit(ad7314_exit);
> +
> +MODULE_AUTHOR("Sonic Zhang <sonic.zhang@analog.com>");
> +MODULE_DESCRIPTION("Analog Devices AD7314, ADT7301 and ADT7302 digital"
> +			" temperature sensor driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH V2] hwmon: ad7314 driver (ported from IIO).

[Jonathan Cameron]

On 09/29/11 16:36, Guenter Roeck wrote:
> On Thu, 2011-09-29 at 08:11 -0400, Jonathan Cameron wrote:
>> Currently dropped power down mode support.
>> V2: Fixes:
>> - messed up use of cache aligned store fixed.
>> - correct output format to be milli degrees.
>> - add documentation file.
>>
>> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
> 
> Hi Jonathan,
> 
> almost there ...
:)
>> +====================
>> +
>> +Supported chips:
>> +   * Analog Devices AD7314
>> +     Prefx: 'ad7314'
>> +     Datasheet: Publicaly available at Analog Devices website.
>> +   * Analog Devices ADT7301
>> +     Prefx: 'adt7301'
>> +     Datasheet: Publicaly available at Analog Devices website.
>> +   * Analog Devices ADT7302
>> +     Prefx: 'adt7302'
>> +     Datasheet: Publicaly available at Analog Devices website.
> 
> Publicaly -> Publicly
drat, forgot to spell check. For that matter Prefx -> Prefix.

>> +		/*
>> +		 * Documented as a 13 bit twos complement register
>> +		 * with a sign bit - which is a 14 bit 2's complement
>> +		 * register.  1lsb - 31.25 milli degrees centigrade
>> +		 */
>> +		data &= ADT7301_TEMP_MASK;
>> +		data = (data << 2) >> 2;
>> +
>> +		return sprintf(buf, "%d\n", (3125 * (int)data)/100);
> 
> You don't need a typecast here (like with the first sprintf). There
> should be spaces around '/' (CodingStyle 3.1). 
> Even better would be to use
> 		DIV_ROUND_CLOSEST(data * 3125, 100)
> to reduce the rounding error.
Nice. Didn't know about that one.
> 
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO,
>> +			  ad7314_show_temperature, NULL, 0);
>> +
>> +static struct attribute *ad7314_attributes[] = {
>> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
>> +	NULL,
>> +};
>> +
>> +static const struct attribute_group ad7314_group = {
>> +	.attrs = ad7314_attributes,
>> +};
>> +
>> +static int __devinit ad7314_probe(struct spi_device *spi_dev)
>> +{
>> +	int ret;
>> +	struct ad7314_data *chip;
>> +
>> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>> +	if (chip == NULL) {
>> +		ret = -ENOMEM;
>> +		goto error_ret;
>> +	}
>> +	dev_set_drvdata(&spi_dev->dev, chip);
>> +
>> +	ret = sysfs_create_group(&spi_dev->dev.kobj, &ad7314_group);
>> +	if (ret < 0)
>> +		goto error_free_chip;
>> +
> Something missing here which I didn't notice at first ... but you do
> remove the hwmon device below, so it makes sense to create it ;).
> 
> 	chip->hwmon_dev = hwmon_device_register(&spi_dev->dev);
>         if (IS_ERR(chip->hwmon_dev)) {
>                 err = PTR_ERR(chip->hwmon_dev);
>                 goto error_remove_group;
>         }
> 
Gah!

That's the problem with developing without hardware.  This sort of
dumbass error always seem to sneak in.

Good spot.

New version along shortly.

[PATCH V3] hwmon: ad7314 driver (ported from IIO).

[Jonathan Cameron]

Currently dropped power down mode support.

V3:
Guenter's suggestions
- typos
- DIV_ROUND_NEAREST use
- Actually register hwmon driver.

V2: Fixes:
- messed up use of cache aligned store fixed.
- correct output format to be milli degrees.
- add documentation file.

Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 Documentation/hwmon/ad7314 |   25 ++++++
 drivers/hwmon/Kconfig      |   10 +++
 drivers/hwmon/Makefile     |    1 +
 drivers/hwmon/ad7314.c     |  185 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 221 insertions(+), 0 deletions(-)

diff --git a/Documentation/hwmon/ad7314 b/Documentation/hwmon/ad7314
new file mode 100644
index 0000000..1912549
--- /dev/null
+++ b/Documentation/hwmon/ad7314
@@ -0,0 +1,25 @@
+Kernel driver ad7314
+====================
+
+Supported chips:
+   * Analog Devices AD7314
+     Prefix: 'ad7314'
+     Datasheet: Publicly available at Analog Devices website.
+   * Analog Devices ADT7301
+     Prefix: 'adt7301'
+     Datasheet: Publicly available at Analog Devices website.
+   * Analog Devices ADT7302
+     Prefix: 'adt7302'
+     Datasheet: Publicly available at Analog Devices website.
+
+Description
+-----------
+
+Driver supports the above parts.  The ad7314 has a 10 bit
+sensor with 1lsb = 0.25 degrees centigrade. The adt7301 and
+adt7302 have 14 bit sensors with 1lsb = 0.03125 degrees centigrade.
+
+Notes
+-----
+
+Currently power down mode is not supported.
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 0b62c3c..e5d4daa 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -68,6 +68,16 @@ config SENSORS_ABITUGURU3
 	  This driver can also be built as a module.  If so, the module
 	  will be called abituguru3.
 
+config SENSORS_AD7314
+	tristate "Analog Devices AD7314 and compatibles"
+	depends on SPI && EXPERIMENTAL
+	help
+	  If you say yes here you get support for the Analog Devices
+	  AD7314, ADT7301 and ADT7302 temperature sensors.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called ad7314.
+
 config SENSORS_AD7414
 	tristate "Analog Devices AD7414"
 	depends on I2C && EXPERIMENTAL
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 3c9ccef..228a4b7 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_SENSORS_W83791D)	+= w83791d.o
 
 obj-$(CONFIG_SENSORS_ABITUGURU)	+= abituguru.o
 obj-$(CONFIG_SENSORS_ABITUGURU3)+= abituguru3.o
+obj-$(CONFIG_SENSORS_AD7314)	+= ad7314.o
 obj-$(CONFIG_SENSORS_AD7414)	+= ad7414.o
 obj-$(CONFIG_SENSORS_AD7418)	+= ad7418.o
 obj-$(CONFIG_SENSORS_ADCXX)	+= adcxx.o
diff --git a/drivers/hwmon/ad7314.c b/drivers/hwmon/ad7314.c
new file mode 100644
index 0000000..6c69a10
--- /dev/null
+++ b/drivers/hwmon/ad7314.c
@@ -0,0 +1,185 @@
+/*
+ * AD7314 digital temperature sensor driver for AD7314, ADT7301 and ADT7302
+ *
+ * Copyright 2010 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2 or later.
+ *
+ * Conversion to hwmon from IIO done by Jonathan Cameron <jic23@cam.ac.uk>
+ */
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/spi/spi.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+
+/*
+ * AD7314 power mode
+ */
+#define AD7314_PD		0x2000
+
+/*
+ * AD7314 temperature masks
+ */
+#define AD7314_TEMP_SIGN		0x200
+#define AD7314_TEMP_MASK		0x7FE0
+#define AD7314_TEMP_OFFSET		5
+
+/*
+ * ADT7301 and ADT7302 temperature masks
+ */
+#define ADT7301_TEMP_SIGN		0x2000
+#define ADT7301_TEMP_MASK		0x3FFF
+
+enum ad7314_variant {
+	adt7301,
+	adt7302,
+	ad7314,
+};
+
+struct ad7314_data {
+	struct spi_device	*spi_dev;
+	struct device		*hwmon_dev;
+	u16 rx ____cacheline_aligned;
+};
+
+static int ad7314_spi_read(struct ad7314_data *chip, s16 *data)
+{
+	int ret;
+
+	ret = spi_read(chip->spi_dev, (u8 *)&chip->rx, sizeof(chip->rx));
+	if (ret < 0) {
+		dev_err(&chip->spi_dev->dev, "SPI read error\n");
+		return ret;
+	}
+
+	*data = be16_to_cpu(chip->rx);
+
+	return ret;
+}
+
+static ssize_t ad7314_show_temperature(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	struct ad7314_data *chip = dev_get_drvdata(dev);
+	s16 data;
+	int ret;
+
+	ret = ad7314_spi_read(chip, &data);
+	if (ret < 0)
+		return ret;
+	switch (spi_get_device_id(chip->spi_dev)->driver_data) {
+	case ad7314:
+		data = (data & AD7314_TEMP_MASK) >> AD7314_TEMP_OFFSET;
+		data = (data << 6) >> 6;
+
+		return sprintf(buf, "%d\n", 250 * data);
+	case adt7301:
+	case adt7302:
+		/*
+		 * Documented as a 13 bit twos complement register
+		 * with a sign bit - which is a 14 bit 2's complement
+		 * register.  1lsb - 31.25 milli degrees centigrade
+		 */
+		data &= ADT7301_TEMP_MASK;
+		data = (data << 2) >> 2;
+
+		return sprintf(buf, "%d\n",
+			       DIV_ROUND_CLOSEST(data * 3125, 100));
+	default:
+		return -EINVAL;
+	}
+}
+
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO,
+			  ad7314_show_temperature, NULL, 0);
+
+static struct attribute *ad7314_attributes[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group ad7314_group = {
+	.attrs = ad7314_attributes,
+};
+
+static int __devinit ad7314_probe(struct spi_device *spi_dev)
+{
+	int ret;
+	struct ad7314_data *chip;
+
+	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+	if (chip == NULL) {
+		ret = -ENOMEM;
+		goto error_ret;
+	}
+	dev_set_drvdata(&spi_dev->dev, chip);
+
+	ret = sysfs_create_group(&spi_dev->dev.kobj, &ad7314_group);
+	if (ret < 0)
+		goto error_free_chip;
+	chip->hwmon_dev = hwmon_device_register(&spi_dev->dev);
+	if (IS_ERR(chip->hwmon_dev)) {
+		ret = PTR_ERR(chip->hwmon_dev);
+		goto error_remove_group;
+	}
+
+	return 0;
+error_remove_group:
+	sysfs_remove_group(&spi_dev->dev.kobj, &ad7314_group);
+error_free_chip:
+	kfree(chip);
+error_ret:
+	return ret;
+}
+
+static int __devexit ad7314_remove(struct spi_device *spi_dev)
+{
+	struct ad7314_data *chip = dev_get_drvdata(&spi_dev->dev);
+
+	hwmon_device_unregister(chip->hwmon_dev);
+	sysfs_remove_group(&spi_dev->dev.kobj, &ad7314_group);
+	kfree(chip);
+
+	return 0;
+}
+
+static const struct spi_device_id ad7314_id[] = {
+	{ "adt7301", adt7301 },
+	{ "adt7302", adt7302 },
+	{ "ad7314", ad7314 },
+	{}
+};
+
+static struct spi_driver ad7314_driver = {
+	.driver = {
+		.name = "ad7314",
+		.bus = &spi_bus_type,
+		.owner = THIS_MODULE,
+	},
+	.probe = ad7314_probe,
+	.remove = __devexit_p(ad7314_remove),
+	.id_table = ad7314_id,
+};
+
+static __init int ad7314_init(void)
+{
+	return spi_register_driver(&ad7314_driver);
+}
+module_init(ad7314_init);
+
+static __exit void ad7314_exit(void)
+{
+	spi_unregister_driver(&ad7314_driver);
+}
+module_exit(ad7314_exit);
+
+MODULE_AUTHOR("Sonic Zhang <sonic.zhang@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD7314, ADT7301 and ADT7302 digital"
+			" temperature sensor driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.3.4

Re: [PATCH V3] hwmon: ad7314 driver (ported from IIO).

[Guenter Roeck]

Hi Jonathan,

On Thu, 2011-09-29 at 12:50 -0400, Jonathan Cameron wrote:
> Currently dropped power down mode support.
> 
> V3:
> Guenter's suggestions
> - typos
> - DIV_ROUND_NEAREST use
> - Actually register hwmon driver.
> 
> V2: Fixes:
> - messed up use of cache aligned store fixed.
> - correct output format to be milli degrees.
> - add documentation file.
> 
> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>

Applied to -next, with one other minor update.

> ---
>  Documentation/hwmon/ad7314 |   25 ++++++
>  drivers/hwmon/Kconfig      |   10 +++
>  drivers/hwmon/Makefile     |    1 +
>  drivers/hwmon/ad7314.c     |  185 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 221 insertions(+), 0 deletions(-)
> 
[ ... ]

> +
> +static const struct spi_device_id ad7314_id[] = {
> +	{ "adt7301", adt7301 },
> +	{ "adt7302", adt7302 },
> +	{ "ad7314", ad7314 },
> +	{}
> +};

MODULE_DEVICE_TABLE(spi, ad7314_id);

Thanks,
Guenter

Re: [PATCH V3] hwmon: ad7314 driver (ported from IIO).

[Jonathan Cameron]

On 09/29/11 18:25, Guenter Roeck wrote:
> Hi Jonathan,
> 
> On Thu, 2011-09-29 at 12:50 -0400, Jonathan Cameron wrote:
>> Currently dropped power down mode support.
>>
>> V3:
>> Guenter's suggestions
>> - typos
>> - DIV_ROUND_NEAREST use
>> - Actually register hwmon driver.
>>
>> V2: Fixes:
>> - messed up use of cache aligned store fixed.
>> - correct output format to be milli degrees.
>> - add documentation file.
>>
>> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
> 
> Applied to -next, with one other minor update.
> 
>> ---
>>  Documentation/hwmon/ad7314 |   25 ++++++
>>  drivers/hwmon/Kconfig      |   10 +++
>>  drivers/hwmon/Makefile     |    1 +
>>  drivers/hwmon/ad7314.c     |  185 ++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 221 insertions(+), 0 deletions(-)
>>
> [ ... ]
> 
>> +
>> +static const struct spi_device_id ad7314_id[] = {
>> +	{ "adt7301", adt7301 },
>> +	{ "adt7302", adt7302 },
>> +	{ "ad7314", ad7314 },
>> +	{}
>> +};
> 
> MODULE_DEVICE_TABLE(spi, ad7314_id);
Good point.  I'll kill off the IIO driver tomorrow.

Thanks,

Jonathan