linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hwmon: Add LTC2471/LTC2473 driver
@ 2017-06-29 10:45 Mike Looijmans
  2017-06-29 11:18 ` Frans Klaver
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Mike Looijmans @ 2017-06-29 10:45 UTC (permalink / raw)
  To: linux-hwmon; +Cc: linux-kernel, linux, jdelvare, Mike Looijmans

The LTC2741 and LTC2473 are single voltage monitoring chips. The LTC2473
is similar to the LTC2471 but outputs a signed differential value.

Datasheet:
  http://cds.linear.com/docs/en/datasheet/24713fb.pdf

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
 drivers/hwmon/Kconfig   |  10 ++++
 drivers/hwmon/Makefile  |   1 +
 drivers/hwmon/ltc2471.c | 127 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 138 insertions(+)
 create mode 100644 drivers/hwmon/ltc2471.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index fbde226..c9a2a87 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -673,6 +673,16 @@ config SENSORS_LINEAGE
 	  This driver can also be built as a module.  If so, the module
 	  will be called lineage-pem.
 
+config SENSORS_LTC2471
+	tristate "Linear Technology LTC2471 and LTC2473"
+	depends on I2C
+	help
+	  If you say yes here you get support for Linear Technology LTC2471
+	  and LTC2473 voltage monitors.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called ltc2471.
+
 config SENSORS_LTC2945
 	tristate "Linear Technology LTC2945"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 58cc3ac..6f60fe7 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -99,6 +99,7 @@ obj-$(CONFIG_SENSORS_LM93)	+= lm93.o
 obj-$(CONFIG_SENSORS_LM95234)	+= lm95234.o
 obj-$(CONFIG_SENSORS_LM95241)	+= lm95241.o
 obj-$(CONFIG_SENSORS_LM95245)	+= lm95245.o
+obj-$(CONFIG_SENSORS_LTC2471)	+= ltc2471.o
 obj-$(CONFIG_SENSORS_LTC2945)	+= ltc2945.o
 obj-$(CONFIG_SENSORS_LTC2990)	+= ltc2990.o
 obj-$(CONFIG_SENSORS_LTC4151)	+= ltc4151.o
diff --git a/drivers/hwmon/ltc2471.c b/drivers/hwmon/ltc2471.c
new file mode 100644
index 0000000..17eaad8
--- /dev/null
+++ b/drivers/hwmon/ltc2471.c
@@ -0,0 +1,127 @@
+/*
+ * Driver for Linear Technology LTC2471 and LTC2473 voltage monitors
+ * The LTC2473 is identical to the 2471, but reports a differential signal.
+ *
+ * Copyright (C) 2017 Topic Embedded Products
+ * Author: Mike Looijmans <mike.looijmans@topic.nl>
+ *
+ * License: GPLv2
+ */
+
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+enum chips {
+	ltc2471,
+	ltc2473
+};
+
+struct ltc2471_data {
+	struct i2c_client *i2c;
+	bool differential;
+};
+
+/* Reference voltage is 1.25V */
+#define LTC2471_VREF 1250
+
+/* Read two bytes from the I2C bus to obtain the ADC result */
+static int ltc2471_get_value(struct i2c_client *i2c)
+{
+	int ret;
+	__be16 buf;
+
+	ret = i2c_master_recv(i2c, (char *)&buf, 2);
+	if (ret < 0)
+		return ret;
+	if (ret != 2)
+		return -EIO;
+
+	/* MSB first */
+	return be16_to_cpu(buf);
+}
+
+static ssize_t ltc2471_show_value(struct device *dev,
+				  struct device_attribute *da, char *buf)
+{
+	struct ltc2471_data *data = dev_get_drvdata(dev);
+	int value;
+
+	value = ltc2471_get_value(data->i2c);
+	if (unlikely(value < 0))
+		return value;
+
+	if (data->differential)
+		/* Ranges from -VREF to +VREF with "0" at 0x8000 */
+		value = ((s32)LTC2471_VREF * (s32)(value - 0x8000)) >> 15;
+	else
+		/* Ranges from 0 to +VREF */
+		value = ((u32)LTC2471_VREF * (u32)value) >> 16;
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", value);
+}
+
+static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ltc2471_show_value, NULL, 0);
+
+static struct attribute *ltc2471_attrs[] = {
+	&sensor_dev_attr_in0_input.dev_attr.attr,
+	NULL
+};
+
+ATTRIBUTE_GROUPS(ltc2471);
+
+static int ltc2471_i2c_probe(struct i2c_client *i2c,
+			     const struct i2c_device_id *id)
+{
+	int ret;
+	struct device *hwmon_dev;
+	struct ltc2471_data *data;
+
+	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C))
+		return -ENODEV;
+
+	data = devm_kzalloc(&i2c->dev, sizeof(struct ltc2471_data), GFP_KERNEL);
+	if (unlikely(!data))
+		return -ENOMEM;
+
+	data->i2c = i2c;
+	data->differential = (id->driver_data == ltc2473);
+
+	/* Trigger once to start conversion and check if chip is there */
+	ret = ltc2471_get_value(i2c);
+	if (ret < 0) {
+		dev_err(&i2c->dev, "Cannot read from device.\n");
+		return ret;
+	}
+
+	hwmon_dev = devm_hwmon_device_register_with_groups(&i2c->dev,
+							   i2c->name,
+							   data,
+							   ltc2471_groups);
+
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct i2c_device_id ltc2471_i2c_id[] = {
+	{ "ltc2471", ltc2471 },
+	{ "ltc2473", ltc2473 },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ltc2471_i2c_id);
+
+static struct i2c_driver ltc2471_i2c_driver = {
+	.driver = {
+		.name = "ltc2471",
+	},
+	.probe    = ltc2471_i2c_probe,
+	.id_table = ltc2471_i2c_id,
+};
+
+module_i2c_driver(ltc2471_i2c_driver);
+
+MODULE_DESCRIPTION("LTC2471/LTC2473 Sensor Driver");
+MODULE_AUTHOR("Topic Embedded Products");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] hwmon: Add LTC2471/LTC2473 driver
  2017-06-29 10:45 [PATCH] hwmon: Add LTC2471/LTC2473 driver Mike Looijmans
@ 2017-06-29 11:18 ` Frans Klaver
  2017-06-29 11:30   ` Mike Looijmans
  2017-06-29 13:37   ` Guenter Roeck
  2017-06-29 12:13 ` [PATCH v2] " Mike Looijmans
  2017-06-29 13:34 ` [PATCH] " Guenter Roeck
  2 siblings, 2 replies; 16+ messages in thread
From: Frans Klaver @ 2017-06-29 11:18 UTC (permalink / raw)
  To: Mike Looijmans; +Cc: linux-hwmon, linux-kernel, Guenter Roeck, jdelvare

On Thu, Jun 29, 2017 at 12:45 PM, Mike Looijmans
<mike.looijmans@topic.nl> wrote:
> The LTC2741 and LTC2473 are single voltage monitoring chips. The LTC2473
> is similar to the LTC2471 but outputs a signed differential value.
>
> Datasheet:
>   http://cds.linear.com/docs/en/datasheet/24713fb.pdf
>
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
>  drivers/hwmon/Kconfig   |  10 ++++
>  drivers/hwmon/Makefile  |   1 +
>  drivers/hwmon/ltc2471.c | 127 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 138 insertions(+)
>  create mode 100644 drivers/hwmon/ltc2471.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index fbde226..c9a2a87 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -673,6 +673,16 @@ config SENSORS_LINEAGE
>           This driver can also be built as a module.  If so, the module
>           will be called lineage-pem.
>
> +config SENSORS_LTC2471
> +       tristate "Linear Technology LTC2471 and LTC2473"
> +       depends on I2C
> +       help
> +         If you say yes here you get support for Linear Technology LTC2471
> +         and LTC2473 voltage monitors.
> +
> +         This driver can also be built as a module. If so, the module will
> +         be called ltc2471.
> +
>  config SENSORS_LTC2945
>         tristate "Linear Technology LTC2945"
>         depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 58cc3ac..6f60fe7 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -99,6 +99,7 @@ obj-$(CONFIG_SENSORS_LM93)    += lm93.o
>  obj-$(CONFIG_SENSORS_LM95234)  += lm95234.o
>  obj-$(CONFIG_SENSORS_LM95241)  += lm95241.o
>  obj-$(CONFIG_SENSORS_LM95245)  += lm95245.o
> +obj-$(CONFIG_SENSORS_LTC2471)  += ltc2471.o
>  obj-$(CONFIG_SENSORS_LTC2945)  += ltc2945.o
>  obj-$(CONFIG_SENSORS_LTC2990)  += ltc2990.o
>  obj-$(CONFIG_SENSORS_LTC4151)  += ltc4151.o
> diff --git a/drivers/hwmon/ltc2471.c b/drivers/hwmon/ltc2471.c
> new file mode 100644
> index 0000000..17eaad8
> --- /dev/null
> +++ b/drivers/hwmon/ltc2471.c
> @@ -0,0 +1,127 @@
> +/*
> + * Driver for Linear Technology LTC2471 and LTC2473 voltage monitors
> + * The LTC2473 is identical to the 2471, but reports a differential signal.
> + *
> + * Copyright (C) 2017 Topic Embedded Products
> + * Author: Mike Looijmans <mike.looijmans@topic.nl>
> + *
> + * License: GPLv2
> + */
> +
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +enum chips {
> +       ltc2471,
> +       ltc2473
> +};
> +
> +struct ltc2471_data {
> +       struct i2c_client *i2c;
> +       bool differential;
> +};

I haven't checked if there are similar drivers, but how about
ltc247x_data? It's a bit odd to tie all naming in this driver to one
chip and then support another one as well.

> +
> +/* Reference voltage is 1.25V */
> +#define LTC2471_VREF 1250
> +
> +/* Read two bytes from the I2C bus to obtain the ADC result */
> +static int ltc2471_get_value(struct i2c_client *i2c)
> +{
> +       int ret;
> +       __be16 buf;
> +
> +       ret = i2c_master_recv(i2c, (char *)&buf, 2);
> +       if (ret < 0)
> +               return ret;
> +       if (ret != 2)
> +               return -EIO;
> +
> +       /* MSB first */
> +       return be16_to_cpu(buf);
> +}
> +
> +static ssize_t ltc2471_show_value(struct device *dev,
> +                                 struct device_attribute *da, char *buf)
> +{
> +       struct ltc2471_data *data = dev_get_drvdata(dev);
> +       int value;
> +
> +       value = ltc2471_get_value(data->i2c);
> +       if (unlikely(value < 0))
> +               return value;
> +
> +       if (data->differential)
> +               /* Ranges from -VREF to +VREF with "0" at 0x8000 */
> +               value = ((s32)LTC2471_VREF * (s32)(value - 0x8000)) >> 15;
> +       else
> +               /* Ranges from 0 to +VREF */
> +               value = ((u32)LTC2471_VREF * (u32)value) >> 16;
> +
> +       return snprintf(buf, PAGE_SIZE, "%d\n", value);
> +}
> +
> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ltc2471_show_value, NULL, 0);

Octal permissions are preferred over S_IRUGO and such.

> +
> +static struct attribute *ltc2471_attrs[] = {
> +       &sensor_dev_attr_in0_input.dev_attr.attr,
> +       NULL
> +};
> +
> +ATTRIBUTE_GROUPS(ltc2471);
> +
> +static int ltc2471_i2c_probe(struct i2c_client *i2c,
> +                            const struct i2c_device_id *id)
> +{
> +       int ret;
> +       struct device *hwmon_dev;
> +       struct ltc2471_data *data;
> +
> +       if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C))
> +               return -ENODEV;
> +
> +       data = devm_kzalloc(&i2c->dev, sizeof(struct ltc2471_data), GFP_KERNEL);

sizeof(*data)

> +       if (unlikely(!data))
> +               return -ENOMEM;
> +
> +       data->i2c = i2c;
> +       data->differential = (id->driver_data == ltc2473);
> +
> +       /* Trigger once to start conversion and check if chip is there */
> +       ret = ltc2471_get_value(i2c);
> +       if (ret < 0) {
> +               dev_err(&i2c->dev, "Cannot read from device.\n");
> +               return ret;
> +       }
> +
> +       hwmon_dev = devm_hwmon_device_register_with_groups(&i2c->dev,
> +                                                          i2c->name,
> +                                                          data,
> +                                                          ltc2471_groups);
> +
> +       return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct i2c_device_id ltc2471_i2c_id[] = {
> +       { "ltc2471", ltc2471 },
> +       { "ltc2473", ltc2473 },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(i2c, ltc2471_i2c_id);
> +
> +static struct i2c_driver ltc2471_i2c_driver = {
> +       .driver = {
> +               .name = "ltc2471",
> +       },
> +       .probe    = ltc2471_i2c_probe,
> +       .id_table = ltc2471_i2c_id,
> +};
> +
> +module_i2c_driver(ltc2471_i2c_driver);
> +
> +MODULE_DESCRIPTION("LTC2471/LTC2473 Sensor Driver");
> +MODULE_AUTHOR("Topic Embedded Products");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>

Frans

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] hwmon: Add LTC2471/LTC2473 driver
  2017-06-29 11:18 ` Frans Klaver
@ 2017-06-29 11:30   ` Mike Looijmans
  2017-06-29 11:38     ` Frans Klaver
  2017-06-29 13:37   ` Guenter Roeck
  1 sibling, 1 reply; 16+ messages in thread
From: Mike Looijmans @ 2017-06-29 11:30 UTC (permalink / raw)
  To: Frans Klaver; +Cc: linux-hwmon, linux-kernel, Guenter Roeck, jdelvare

On 29-06-17 13:18, Frans Klaver wrote:
> On Thu, Jun 29, 2017 at 12:45 PM, Mike Looijmans
> <mike.looijmans@topic.nl> wrote:
>> The LTC2741 and LTC2473 are single voltage monitoring chips. The LTC2473
>> is similar to the LTC2471 but outputs a signed differential value.
>>
>> Datasheet:
>>    http://cds.linear.com/docs/en/datasheet/24713fb.pdf
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>> ---
>>   drivers/hwmon/Kconfig   |  10 ++++
>>   drivers/hwmon/Makefile  |   1 +
>>   drivers/hwmon/ltc2471.c | 127 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 138 insertions(+)
>>   create mode 100644 drivers/hwmon/ltc2471.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index fbde226..c9a2a87 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -673,6 +673,16 @@ config SENSORS_LINEAGE
>>            This driver can also be built as a module.  If so, the module
>>            will be called lineage-pem.
>>
>> +config SENSORS_LTC2471
>> +       tristate "Linear Technology LTC2471 and LTC2473"
>> +       depends on I2C
>> +       help
>> +         If you say yes here you get support for Linear Technology LTC2471
>> +         and LTC2473 voltage monitors.
>> +
>> +         This driver can also be built as a module. If so, the module will
>> +         be called ltc2471.
>> +
>>   config SENSORS_LTC2945
>>          tristate "Linear Technology LTC2945"
>>          depends on I2C
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 58cc3ac..6f60fe7 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -99,6 +99,7 @@ obj-$(CONFIG_SENSORS_LM93)    += lm93.o
>>   obj-$(CONFIG_SENSORS_LM95234)  += lm95234.o
>>   obj-$(CONFIG_SENSORS_LM95241)  += lm95241.o
>>   obj-$(CONFIG_SENSORS_LM95245)  += lm95245.o
>> +obj-$(CONFIG_SENSORS_LTC2471)  += ltc2471.o
>>   obj-$(CONFIG_SENSORS_LTC2945)  += ltc2945.o
>>   obj-$(CONFIG_SENSORS_LTC2990)  += ltc2990.o
>>   obj-$(CONFIG_SENSORS_LTC4151)  += ltc4151.o
>> diff --git a/drivers/hwmon/ltc2471.c b/drivers/hwmon/ltc2471.c
>> new file mode 100644
>> index 0000000..17eaad8
>> --- /dev/null
>> +++ b/drivers/hwmon/ltc2471.c
>> @@ -0,0 +1,127 @@
>> +/*
>> + * Driver for Linear Technology LTC2471 and LTC2473 voltage monitors
>> + * The LTC2473 is identical to the 2471, but reports a differential signal.
>> + *
>> + * Copyright (C) 2017 Topic Embedded Products
>> + * Author: Mike Looijmans <mike.looijmans@topic.nl>
>> + *
>> + * License: GPLv2
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +
>> +enum chips {
>> +       ltc2471,
>> +       ltc2473
>> +};
>> +
>> +struct ltc2471_data {
>> +       struct i2c_client *i2c;
>> +       bool differential;
>> +};
> 
> I haven't checked if there are similar drivers, but how about
> ltc247x_data? It's a bit odd to tie all naming in this driver to one
> chip and then support another one as well.

Did a query on the linear site, found the LTC2470 and LTC2472 which appear to 
be the SPI versions of the same chip. So yes, using "ltc247x" may cause 
confusion when other drivers arrive. Linear solves this by naming the 
datasheet "ltc24712" but I think that'd be even more confusing.

> 
>> +
>> +/* Reference voltage is 1.25V */
>> +#define LTC2471_VREF 1250
>> +
>> +/* Read two bytes from the I2C bus to obtain the ADC result */
>> +static int ltc2471_get_value(struct i2c_client *i2c)
>> +{
>> +       int ret;
>> +       __be16 buf;
>> +
>> +       ret = i2c_master_recv(i2c, (char *)&buf, 2);
>> +       if (ret < 0)
>> +               return ret;
>> +       if (ret != 2)
>> +               return -EIO;
>> +
>> +       /* MSB first */
>> +       return be16_to_cpu(buf);
>> +}
>> +
>> +static ssize_t ltc2471_show_value(struct device *dev,
>> +                                 struct device_attribute *da, char *buf)
>> +{
>> +       struct ltc2471_data *data = dev_get_drvdata(dev);
>> +       int value;
>> +
>> +       value = ltc2471_get_value(data->i2c);
>> +       if (unlikely(value < 0))
>> +               return value;
>> +
>> +       if (data->differential)
>> +               /* Ranges from -VREF to +VREF with "0" at 0x8000 */
>> +               value = ((s32)LTC2471_VREF * (s32)(value - 0x8000)) >> 15;
>> +       else
>> +               /* Ranges from 0 to +VREF */
>> +               value = ((u32)LTC2471_VREF * (u32)value) >> 16;
>> +
>> +       return snprintf(buf, PAGE_SIZE, "%d\n", value);
>> +}
>> +
>> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ltc2471_show_value, NULL, 0);
> 
> Octal permissions are preferred over S_IRUGO and such.

Uh, can elaborate on that, or give an example on what you mean by that remark?

A quick grep reveils that most (if not all) hwmon devices are currently using 
"S_IRUGO" and such.

>> +
>> +static struct attribute *ltc2471_attrs[] = {
>> +       &sensor_dev_attr_in0_input.dev_attr.attr,
>> +       NULL
>> +};
>> +
>> +ATTRIBUTE_GROUPS(ltc2471);
>> +
>> +static int ltc2471_i2c_probe(struct i2c_client *i2c,
>> +                            const struct i2c_device_id *id)
>> +{
>> +       int ret;
>> +       struct device *hwmon_dev;
>> +       struct ltc2471_data *data;
>> +
>> +       if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C))
>> +               return -ENODEV;
>> +
>> +       data = devm_kzalloc(&i2c->dev, sizeof(struct ltc2471_data), GFP_KERNEL);
> 
> sizeof(*data)

OK.

> 
>> +       if (unlikely(!data))
>> +               return -ENOMEM;
>> +
>> +       data->i2c = i2c;
>> +       data->differential = (id->driver_data == ltc2473);
>> +
>> +       /* Trigger once to start conversion and check if chip is there */
>> +       ret = ltc2471_get_value(i2c);
>> +       if (ret < 0) {
>> +               dev_err(&i2c->dev, "Cannot read from device.\n");
>> +               return ret;
>> +       }
>> +
>> +       hwmon_dev = devm_hwmon_device_register_with_groups(&i2c->dev,
>> +                                                          i2c->name,
>> +                                                          data,
>> +                                                          ltc2471_groups);
>> +
>> +       return PTR_ERR_OR_ZERO(hwmon_dev);
>> +}
>> +
>> +static const struct i2c_device_id ltc2471_i2c_id[] = {
>> +       { "ltc2471", ltc2471 },
>> +       { "ltc2473", ltc2473 },
>> +       {}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ltc2471_i2c_id);
>> +
>> +static struct i2c_driver ltc2471_i2c_driver = {
>> +       .driver = {
>> +               .name = "ltc2471",
>> +       },
>> +       .probe    = ltc2471_i2c_probe,
>> +       .id_table = ltc2471_i2c_id,
>> +};
>> +
>> +module_i2c_driver(ltc2471_i2c_driver);
>> +
>> +MODULE_DESCRIPTION("LTC2471/LTC2473 Sensor Driver");
>> +MODULE_AUTHOR("Topic Embedded Products");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 1.9.1
>>
> 
> Frans
> 



Kind regards,

Mike Looijmans
System Expert

TOPIC Products
Materiaalweg 4, NL-5681 RJ Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] hwmon: Add LTC2471/LTC2473 driver
  2017-06-29 11:30   ` Mike Looijmans
@ 2017-06-29 11:38     ` Frans Klaver
  2017-06-29 12:08       ` Mike Looijmans
  0 siblings, 1 reply; 16+ messages in thread
From: Frans Klaver @ 2017-06-29 11:38 UTC (permalink / raw)
  To: Mike Looijmans; +Cc: linux-hwmon, linux-kernel, Guenter Roeck, jdelvare

On Thu, Jun 29, 2017 at 1:30 PM, Mike Looijmans <mike.looijmans@topic.nl> wrote:
> On 29-06-17 13:18, Frans Klaver wrote:
>>
>> On Thu, Jun 29, 2017 at 12:45 PM, Mike Looijmans
>> <mike.looijmans@topic.nl> wrote:
>>>
>>> The LTC2741 and LTC2473 are single voltage monitoring chips. The LTC2473
>>> is similar to the LTC2471 but outputs a signed differential value.
>>>
>>> Datasheet:
>>>    http://cds.linear.com/docs/en/datasheet/24713fb.pdf
>>>
>>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>>> ---
>>>   drivers/hwmon/Kconfig   |  10 ++++
>>>   drivers/hwmon/Makefile  |   1 +
>>>   drivers/hwmon/ltc2471.c | 127
>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 138 insertions(+)
>>>   create mode 100644 drivers/hwmon/ltc2471.c
>>>
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index fbde226..c9a2a87 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -673,6 +673,16 @@ config SENSORS_LINEAGE
>>>            This driver can also be built as a module.  If so, the module
>>>            will be called lineage-pem.
>>>
>>> +config SENSORS_LTC2471
>>> +       tristate "Linear Technology LTC2471 and LTC2473"
>>> +       depends on I2C
>>> +       help
>>> +         If you say yes here you get support for Linear Technology
>>> LTC2471
>>> +         and LTC2473 voltage monitors.
>>> +
>>> +         This driver can also be built as a module. If so, the module
>>> will
>>> +         be called ltc2471.
>>> +
>>>   config SENSORS_LTC2945
>>>          tristate "Linear Technology LTC2945"
>>>          depends on I2C
>>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>> index 58cc3ac..6f60fe7 100644
>>> --- a/drivers/hwmon/Makefile
>>> +++ b/drivers/hwmon/Makefile
>>> @@ -99,6 +99,7 @@ obj-$(CONFIG_SENSORS_LM93)    += lm93.o
>>>   obj-$(CONFIG_SENSORS_LM95234)  += lm95234.o
>>>   obj-$(CONFIG_SENSORS_LM95241)  += lm95241.o
>>>   obj-$(CONFIG_SENSORS_LM95245)  += lm95245.o
>>> +obj-$(CONFIG_SENSORS_LTC2471)  += ltc2471.o
>>>   obj-$(CONFIG_SENSORS_LTC2945)  += ltc2945.o
>>>   obj-$(CONFIG_SENSORS_LTC2990)  += ltc2990.o
>>>   obj-$(CONFIG_SENSORS_LTC4151)  += ltc4151.o
>>> diff --git a/drivers/hwmon/ltc2471.c b/drivers/hwmon/ltc2471.c
>>> new file mode 100644
>>> index 0000000..17eaad8
>>> --- /dev/null
>>> +++ b/drivers/hwmon/ltc2471.c
>>> @@ -0,0 +1,127 @@
>>> +/*
>>> + * Driver for Linear Technology LTC2471 and LTC2473 voltage monitors
>>> + * The LTC2473 is identical to the 2471, but reports a differential
>>> signal.
>>> + *
>>> + * Copyright (C) 2017 Topic Embedded Products
>>> + * Author: Mike Looijmans <mike.looijmans@topic.nl>
>>> + *
>>> + * License: GPLv2
>>> + */
>>> +
>>> +#include <linux/err.h>
>>> +#include <linux/hwmon.h>
>>> +#include <linux/hwmon-sysfs.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +
>>> +enum chips {
>>> +       ltc2471,
>>> +       ltc2473
>>> +};
>>> +
>>> +struct ltc2471_data {
>>> +       struct i2c_client *i2c;
>>> +       bool differential;
>>> +};
>>
>>
>> I haven't checked if there are similar drivers, but how about
>> ltc247x_data? It's a bit odd to tie all naming in this driver to one
>> chip and then support another one as well.
>
>
> Did a query on the linear site, found the LTC2470 and LTC2472 which appear
> to be the SPI versions of the same chip. So yes, using "ltc247x" may cause
> confusion when other drivers arrive. Linear solves this by naming the
> datasheet "ltc24712" but I think that'd be even more confusing.

OK.


>>
>>> +
>>> +/* Reference voltage is 1.25V */
>>> +#define LTC2471_VREF 1250
>>> +
>>> +/* Read two bytes from the I2C bus to obtain the ADC result */
>>> +static int ltc2471_get_value(struct i2c_client *i2c)
>>> +{
>>> +       int ret;
>>> +       __be16 buf;
>>> +
>>> +       ret = i2c_master_recv(i2c, (char *)&buf, 2);
>>> +       if (ret < 0)
>>> +               return ret;
>>> +       if (ret != 2)
>>> +               return -EIO;
>>> +
>>> +       /* MSB first */
>>> +       return be16_to_cpu(buf);
>>> +}
>>> +
>>> +static ssize_t ltc2471_show_value(struct device *dev,
>>> +                                 struct device_attribute *da, char *buf)
>>> +{
>>> +       struct ltc2471_data *data = dev_get_drvdata(dev);
>>> +       int value;
>>> +
>>> +       value = ltc2471_get_value(data->i2c);
>>> +       if (unlikely(value < 0))
>>> +               return value;
>>> +
>>> +       if (data->differential)
>>> +               /* Ranges from -VREF to +VREF with "0" at 0x8000 */
>>> +               value = ((s32)LTC2471_VREF * (s32)(value - 0x8000)) >>
>>> 15;
>>> +       else
>>> +               /* Ranges from 0 to +VREF */
>>> +               value = ((u32)LTC2471_VREF * (u32)value) >> 16;
>>> +
>>> +       return snprintf(buf, PAGE_SIZE, "%d\n", value);
>>> +}
>>> +
>>> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ltc2471_show_value, NULL,
>>> 0);
>>
>>
>> Octal permissions are preferred over S_IRUGO and such.
>
>
> Uh, can elaborate on that, or give an example on what you mean by that
> remark?
>
> A quick grep reveils that most (if not all) hwmon devices are currently
> using "S_IRUGO" and such.

I am aware of the fact that lots of drivers use these .Linus filed an
executive decision that octals are preferred over symbols when it
comes to permissions. checkpatch.pl should warn about it nowadays.

Frans

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] hwmon: Add LTC2471/LTC2473 driver
  2017-06-29 11:38     ` Frans Klaver
@ 2017-06-29 12:08       ` Mike Looijmans
  0 siblings, 0 replies; 16+ messages in thread
From: Mike Looijmans @ 2017-06-29 12:08 UTC (permalink / raw)
  To: Frans Klaver; +Cc: linux-hwmon, linux-kernel, Guenter Roeck, jdelvare

On 29-06-17 13:38, Frans Klaver wrote:
> On Thu, Jun 29, 2017 at 1:30 PM, Mike Looijmans <mike.looijmans@topic.nl> wrote:
>> On 29-06-17 13:18, Frans Klaver wrote:
>>>
>>> On Thu, Jun 29, 2017 at 12:45 PM, Mike Looijmans
>>> <mike.looijmans@topic.nl> wrote:
>>>>
>>>> The LTC2741 and LTC2473 are single voltage monitoring chips. The LTC2473
>>>> is similar to the LTC2471 but outputs a signed differential value.
>>>>
>>>> Datasheet:
>>>>     http://cds.linear.com/docs/en/datasheet/24713fb.pdf
>>>>
>>>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>>>> ---
>>>>    drivers/hwmon/Kconfig   |  10 ++++
>>>>    drivers/hwmon/Makefile  |   1 +
>>>>    drivers/hwmon/ltc2471.c | 127
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 138 insertions(+)
>>>>    create mode 100644 drivers/hwmon/ltc2471.c
>>>>
>>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>>> index fbde226..c9a2a87 100644
>>>> --- a/drivers/hwmon/Kconfig
>>>> +++ b/drivers/hwmon/Kconfig
>>>> @@ -673,6 +673,16 @@ config SENSORS_LINEAGE
>>>>             This driver can also be built as a module.  If so, the module
>>>>             will be called lineage-pem.
>>>>
>>>> +config SENSORS_LTC2471
>>>> +       tristate "Linear Technology LTC2471 and LTC2473"
>>>> +       depends on I2C
>>>> +       help
>>>> +         If you say yes here you get support for Linear Technology
>>>> LTC2471
>>>> +         and LTC2473 voltage monitors.
>>>> +
>>>> +         This driver can also be built as a module. If so, the module
>>>> will
>>>> +         be called ltc2471.
>>>> +
>>>>    config SENSORS_LTC2945
>>>>           tristate "Linear Technology LTC2945"
>>>>           depends on I2C
>>>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>>> index 58cc3ac..6f60fe7 100644
>>>> --- a/drivers/hwmon/Makefile
>>>> +++ b/drivers/hwmon/Makefile
>>>> @@ -99,6 +99,7 @@ obj-$(CONFIG_SENSORS_LM93)    += lm93.o
>>>>    obj-$(CONFIG_SENSORS_LM95234)  += lm95234.o
>>>>    obj-$(CONFIG_SENSORS_LM95241)  += lm95241.o
>>>>    obj-$(CONFIG_SENSORS_LM95245)  += lm95245.o
>>>> +obj-$(CONFIG_SENSORS_LTC2471)  += ltc2471.o
>>>>    obj-$(CONFIG_SENSORS_LTC2945)  += ltc2945.o
>>>>    obj-$(CONFIG_SENSORS_LTC2990)  += ltc2990.o
>>>>    obj-$(CONFIG_SENSORS_LTC4151)  += ltc4151.o
>>>> diff --git a/drivers/hwmon/ltc2471.c b/drivers/hwmon/ltc2471.c
>>>> new file mode 100644
>>>> index 0000000..17eaad8
>>>> --- /dev/null
>>>> +++ b/drivers/hwmon/ltc2471.c
>>>> @@ -0,0 +1,127 @@
>>>> +/*
>>>> + * Driver for Linear Technology LTC2471 and LTC2473 voltage monitors
>>>> + * The LTC2473 is identical to the 2471, but reports a differential
>>>> signal.
>>>> + *
>>>> + * Copyright (C) 2017 Topic Embedded Products
>>>> + * Author: Mike Looijmans <mike.looijmans@topic.nl>
>>>> + *
>>>> + * License: GPLv2
>>>> + */
>>>> +
>>>> +#include <linux/err.h>
>>>> +#include <linux/hwmon.h>
>>>> +#include <linux/hwmon-sysfs.h>
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +
>>>> +enum chips {
>>>> +       ltc2471,
>>>> +       ltc2473
>>>> +};
>>>> +
>>>> +struct ltc2471_data {
>>>> +       struct i2c_client *i2c;
>>>> +       bool differential;
>>>> +};
>>>
>>>
>>> I haven't checked if there are similar drivers, but how about
>>> ltc247x_data? It's a bit odd to tie all naming in this driver to one
>>> chip and then support another one as well.
>>
>>
>> Did a query on the linear site, found the LTC2470 and LTC2472 which appear
>> to be the SPI versions of the same chip. So yes, using "ltc247x" may cause
>> confusion when other drivers arrive. Linear solves this by naming the
>> datasheet "ltc24712" but I think that'd be even more confusing.
> 
> OK.
> 
> 
>>>
>>>> +
>>>> +/* Reference voltage is 1.25V */
>>>> +#define LTC2471_VREF 1250
>>>> +
>>>> +/* Read two bytes from the I2C bus to obtain the ADC result */
>>>> +static int ltc2471_get_value(struct i2c_client *i2c)
>>>> +{
>>>> +       int ret;
>>>> +       __be16 buf;
>>>> +
>>>> +       ret = i2c_master_recv(i2c, (char *)&buf, 2);
>>>> +       if (ret < 0)
>>>> +               return ret;
>>>> +       if (ret != 2)
>>>> +               return -EIO;
>>>> +
>>>> +       /* MSB first */
>>>> +       return be16_to_cpu(buf);
>>>> +}
>>>> +
>>>> +static ssize_t ltc2471_show_value(struct device *dev,
>>>> +                                 struct device_attribute *da, char *buf)
>>>> +{
>>>> +       struct ltc2471_data *data = dev_get_drvdata(dev);
>>>> +       int value;
>>>> +
>>>> +       value = ltc2471_get_value(data->i2c);
>>>> +       if (unlikely(value < 0))
>>>> +               return value;
>>>> +
>>>> +       if (data->differential)
>>>> +               /* Ranges from -VREF to +VREF with "0" at 0x8000 */
>>>> +               value = ((s32)LTC2471_VREF * (s32)(value - 0x8000)) >>
>>>> 15;
>>>> +       else
>>>> +               /* Ranges from 0 to +VREF */
>>>> +               value = ((u32)LTC2471_VREF * (u32)value) >> 16;
>>>> +
>>>> +       return snprintf(buf, PAGE_SIZE, "%d\n", value);
>>>> +}
>>>> +
>>>> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ltc2471_show_value, NULL,
>>>> 0);
>>>
>>>
>>> Octal permissions are preferred over S_IRUGO and such.
>>
>>
>> Uh, can elaborate on that, or give an example on what you mean by that
>> remark?
>>
>> A quick grep reveils that most (if not all) hwmon devices are currently
>> using "S_IRUGO" and such.
> 
> I am aware of the fact that lots of drivers use these .Linus filed an
> executive decision that octals are preferred over symbols when it
> comes to permissions. checkpatch.pl should warn about it nowadays.

rebased on current master and checkpatch now indeed suggests to use "0444" 
instead. I'll submit a v2 with changes as suggested.


Kind regards,

Mike Looijmans
System Expert

TOPIC Products
Materiaalweg 4, NL-5681 RJ Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2] hwmon: Add LTC2471/LTC2473 driver
  2017-06-29 10:45 [PATCH] hwmon: Add LTC2471/LTC2473 driver Mike Looijmans
  2017-06-29 11:18 ` Frans Klaver
@ 2017-06-29 12:13 ` Mike Looijmans
  2017-06-29 12:30   ` Lars-Peter Clausen
  2017-06-29 13:34 ` [PATCH] " Guenter Roeck
  2 siblings, 1 reply; 16+ messages in thread
From: Mike Looijmans @ 2017-06-29 12:13 UTC (permalink / raw)
  To: linux-hwmon; +Cc: linux-kernel, linux, jdelvare, Mike Looijmans

The LTC2741 and LTC2473 are single voltage monitoring chips. The LTC2473
is similar to the LTC2471 but outputs a signed differential value.

Datasheet:
  http://cds.linear.com/docs/en/datasheet/24713fb.pdf

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
v2: Use sizeof(*data) instead of struct name
    Use octal permissions

 drivers/hwmon/Kconfig   |  10 ++++
 drivers/hwmon/Makefile  |   1 +
 drivers/hwmon/ltc2471.c | 127 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 138 insertions(+)
 create mode 100644 drivers/hwmon/ltc2471.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 5ef2814..5acaa2d 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -696,6 +696,16 @@ config SENSORS_LINEAGE
 	  This driver can also be built as a module.  If so, the module
 	  will be called lineage-pem.
 
+config SENSORS_LTC2471
+	tristate "Linear Technology LTC2471 and LTC2473"
+	depends on I2C
+	help
+	  If you say yes here you get support for Linear Technology LTC2471
+	  and LTC2473 voltage monitors.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called ltc2471.
+
 config SENSORS_LTC2945
 	tristate "Linear Technology LTC2945"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index d4641a9..49badff 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -102,6 +102,7 @@ obj-$(CONFIG_SENSORS_LM93)	+= lm93.o
 obj-$(CONFIG_SENSORS_LM95234)	+= lm95234.o
 obj-$(CONFIG_SENSORS_LM95241)	+= lm95241.o
 obj-$(CONFIG_SENSORS_LM95245)	+= lm95245.o
+obj-$(CONFIG_SENSORS_LTC2471)	+= ltc2471.o
 obj-$(CONFIG_SENSORS_LTC2945)	+= ltc2945.o
 obj-$(CONFIG_SENSORS_LTC2990)	+= ltc2990.o
 obj-$(CONFIG_SENSORS_LTC4151)	+= ltc4151.o
diff --git a/drivers/hwmon/ltc2471.c b/drivers/hwmon/ltc2471.c
new file mode 100644
index 0000000..f77f7b5
--- /dev/null
+++ b/drivers/hwmon/ltc2471.c
@@ -0,0 +1,127 @@
+/*
+ * Driver for Linear Technology LTC2471 and LTC2473 voltage monitors
+ * The LTC2473 is identical to the 2471, but reports a differential signal.
+ *
+ * Copyright (C) 2017 Topic Embedded Products
+ * Author: Mike Looijmans <mike.looijmans@topic.nl>
+ *
+ * License: GPLv2
+ */
+
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+enum chips {
+	ltc2471,
+	ltc2473
+};
+
+struct ltc2471_data {
+	struct i2c_client *i2c;
+	bool differential;
+};
+
+/* Reference voltage is 1.25V */
+#define LTC2471_VREF 1250
+
+/* Read two bytes from the I2C bus to obtain the ADC result */
+static int ltc2471_get_value(struct i2c_client *i2c)
+{
+	int ret;
+	__be16 buf;
+
+	ret = i2c_master_recv(i2c, (char *)&buf, 2);
+	if (ret < 0)
+		return ret;
+	if (ret != 2)
+		return -EIO;
+
+	/* MSB first */
+	return be16_to_cpu(buf);
+}
+
+static ssize_t ltc2471_show_value(struct device *dev,
+				  struct device_attribute *da, char *buf)
+{
+	struct ltc2471_data *data = dev_get_drvdata(dev);
+	int value;
+
+	value = ltc2471_get_value(data->i2c);
+	if (unlikely(value < 0))
+		return value;
+
+	if (data->differential)
+		/* Ranges from -VREF to +VREF with "0" at 0x8000 */
+		value = ((s32)LTC2471_VREF * (s32)(value - 0x8000)) >> 15;
+	else
+		/* Ranges from 0 to +VREF */
+		value = ((u32)LTC2471_VREF * (u32)value) >> 16;
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", value);
+}
+
+static SENSOR_DEVICE_ATTR(in0_input, 0444, ltc2471_show_value, NULL, 0);
+
+static struct attribute *ltc2471_attrs[] = {
+	&sensor_dev_attr_in0_input.dev_attr.attr,
+	NULL
+};
+
+ATTRIBUTE_GROUPS(ltc2471);
+
+static int ltc2471_i2c_probe(struct i2c_client *i2c,
+			     const struct i2c_device_id *id)
+{
+	int ret;
+	struct device *hwmon_dev;
+	struct ltc2471_data *data;
+
+	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C))
+		return -ENODEV;
+
+	data = devm_kzalloc(&i2c->dev, sizeof(*data), GFP_KERNEL);
+	if (unlikely(!data))
+		return -ENOMEM;
+
+	data->i2c = i2c;
+	data->differential = (id->driver_data == ltc2473);
+
+	/* Trigger once to start conversion and check if chip is there */
+	ret = ltc2471_get_value(i2c);
+	if (ret < 0) {
+		dev_err(&i2c->dev, "Cannot read from device.\n");
+		return ret;
+	}
+
+	hwmon_dev = devm_hwmon_device_register_with_groups(&i2c->dev,
+							   i2c->name,
+							   data,
+							   ltc2471_groups);
+
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct i2c_device_id ltc2471_i2c_id[] = {
+	{ "ltc2471", ltc2471 },
+	{ "ltc2473", ltc2473 },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ltc2471_i2c_id);
+
+static struct i2c_driver ltc2471_i2c_driver = {
+	.driver = {
+		.name = "ltc2471",
+	},
+	.probe    = ltc2471_i2c_probe,
+	.id_table = ltc2471_i2c_id,
+};
+
+module_i2c_driver(ltc2471_i2c_driver);
+
+MODULE_DESCRIPTION("LTC2471/LTC2473 Sensor Driver");
+MODULE_AUTHOR("Topic Embedded Products");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] hwmon: Add LTC2471/LTC2473 driver
  2017-06-29 12:13 ` [PATCH v2] " Mike Looijmans
@ 2017-06-29 12:30   ` Lars-Peter Clausen
  2017-06-29 13:22     ` Mike Looijmans
  2017-06-29 13:40     ` Guenter Roeck
  0 siblings, 2 replies; 16+ messages in thread
From: Lars-Peter Clausen @ 2017-06-29 12:30 UTC (permalink / raw)
  To: Mike Looijmans, linux-hwmon
  Cc: linux-kernel, linux, jdelvare, linux-iio, Jonathan Cameron

On 06/29/2017 02:13 PM, Mike Looijmans wrote:
> The LTC2741 and LTC2473 are single voltage monitoring chips. The LTC2473
> is similar to the LTC2471 but outputs a signed differential value.
> 
> Datasheet:
>   http://cds.linear.com/docs/en/datasheet/24713fb.pdf

This looks more like a general purpose ADC rather than a specialized voltage
monitoring chip. In my opinion this driver would be better suited for the
IIO framework. But before you start working on that lets try to hear what
others think about this.

- Lars

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] hwmon: Add LTC2471/LTC2473 driver
  2017-06-29 12:30   ` Lars-Peter Clausen
@ 2017-06-29 13:22     ` Mike Looijmans
  2017-06-29 13:41       ` Guenter Roeck
  2017-06-29 13:40     ` Guenter Roeck
  1 sibling, 1 reply; 16+ messages in thread
From: Mike Looijmans @ 2017-06-29 13:22 UTC (permalink / raw)
  To: Lars-Peter Clausen, linux-hwmon
  Cc: linux-kernel, linux, jdelvare, linux-iio, Jonathan Cameron

On 29-06-17 14:30, Lars-Peter Clausen wrote:
> On 06/29/2017 02:13 PM, Mike Looijmans wrote:
>> The LTC2741 and LTC2473 are single voltage monitoring chips. The LTC2473
>> is similar to the LTC2471 but outputs a signed differential value.
>>
>> Datasheet:
>>    http://cds.linear.com/docs/en/datasheet/24713fb.pdf
> 
> This looks more like a general purpose ADC rather than a specialized voltage
> monitoring chip. In my opinion this driver would be better suited for the
> IIO framework. But before you start working on that lets try to hear what
> others think about this.

I'm neutral on it, it's being used as a hardware monitor on the board I have, 
but it's indeed a generic I2C ADC.

Do you have a suggestion for a similar device (simple I2C ADC) that I could 
base the IIO driver on? I've come across the AD799x which seems a good 
starting point.



Kind regards,

Mike Looijmans
System Expert

TOPIC Products
Materiaalweg 4, NL-5681 RJ Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] hwmon: Add LTC2471/LTC2473 driver
  2017-06-29 10:45 [PATCH] hwmon: Add LTC2471/LTC2473 driver Mike Looijmans
  2017-06-29 11:18 ` Frans Klaver
  2017-06-29 12:13 ` [PATCH v2] " Mike Looijmans
@ 2017-06-29 13:34 ` Guenter Roeck
  2017-07-01  9:58   ` Jonathan Cameron
  2 siblings, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2017-06-29 13:34 UTC (permalink / raw)
  To: Mike Looijmans, linux-hwmon
  Cc: linux-kernel, Jonathan Cameron, linux-iio, jdelvare

On 06/29/2017 03:45 AM, Mike Looijmans wrote:
> The LTC2741 and LTC2473 are single voltage monitoring chips. The LTC2473
> is similar to the LTC2471 but outputs a signed differential value.
> 
> Datasheet:
>    http://cds.linear.com/docs/en/datasheet/24713fb.pdf
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

Hi Mike,

that looks like a perfect candidate for an iio driver, since its use is
not limited to hardware monitoring, it has a high sample rate, and it
doesn't have limit registers or actions.

Jonathan, what do you think ?

Thanks,
Guenter

> ---
>   drivers/hwmon/Kconfig   |  10 ++++
>   drivers/hwmon/Makefile  |   1 +
>   drivers/hwmon/ltc2471.c | 127 ++++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 138 insertions(+)
>   create mode 100644 drivers/hwmon/ltc2471.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index fbde226..c9a2a87 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -673,6 +673,16 @@ config SENSORS_LINEAGE
>   	  This driver can also be built as a module.  If so, the module
>   	  will be called lineage-pem.
>   
> +config SENSORS_LTC2471
> +	tristate "Linear Technology LTC2471 and LTC2473"
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for Linear Technology LTC2471
> +	  and LTC2473 voltage monitors.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called ltc2471.
> +
>   config SENSORS_LTC2945
>   	tristate "Linear Technology LTC2945"
>   	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 58cc3ac..6f60fe7 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -99,6 +99,7 @@ obj-$(CONFIG_SENSORS_LM93)	+= lm93.o
>   obj-$(CONFIG_SENSORS_LM95234)	+= lm95234.o
>   obj-$(CONFIG_SENSORS_LM95241)	+= lm95241.o
>   obj-$(CONFIG_SENSORS_LM95245)	+= lm95245.o
> +obj-$(CONFIG_SENSORS_LTC2471)	+= ltc2471.o
>   obj-$(CONFIG_SENSORS_LTC2945)	+= ltc2945.o
>   obj-$(CONFIG_SENSORS_LTC2990)	+= ltc2990.o
>   obj-$(CONFIG_SENSORS_LTC4151)	+= ltc4151.o
> diff --git a/drivers/hwmon/ltc2471.c b/drivers/hwmon/ltc2471.c
> new file mode 100644
> index 0000000..17eaad8
> --- /dev/null
> +++ b/drivers/hwmon/ltc2471.c
> @@ -0,0 +1,127 @@
> +/*
> + * Driver for Linear Technology LTC2471 and LTC2473 voltage monitors
> + * The LTC2473 is identical to the 2471, but reports a differential signal.
> + *
> + * Copyright (C) 2017 Topic Embedded Products
> + * Author: Mike Looijmans <mike.looijmans@topic.nl>
> + *
> + * License: GPLv2
> + */
> +
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +enum chips {
> +	ltc2471,
> +	ltc2473
> +};
> +
> +struct ltc2471_data {
> +	struct i2c_client *i2c;
> +	bool differential;
> +};
> +
> +/* Reference voltage is 1.25V */
> +#define LTC2471_VREF 1250
> +
> +/* Read two bytes from the I2C bus to obtain the ADC result */
> +static int ltc2471_get_value(struct i2c_client *i2c)
> +{
> +	int ret;
> +	__be16 buf;
> +
> +	ret = i2c_master_recv(i2c, (char *)&buf, 2);
> +	if (ret < 0)
> +		return ret;
> +	if (ret != 2)
> +		return -EIO;
> +
> +	/* MSB first */
> +	return be16_to_cpu(buf);
> +}
> +
> +static ssize_t ltc2471_show_value(struct device *dev,
> +				  struct device_attribute *da, char *buf)
> +{
> +	struct ltc2471_data *data = dev_get_drvdata(dev);
> +	int value;
> +
> +	value = ltc2471_get_value(data->i2c);
> +	if (unlikely(value < 0))
> +		return value;
> +
> +	if (data->differential)
> +		/* Ranges from -VREF to +VREF with "0" at 0x8000 */
> +		value = ((s32)LTC2471_VREF * (s32)(value - 0x8000)) >> 15;
> +	else
> +		/* Ranges from 0 to +VREF */
> +		value = ((u32)LTC2471_VREF * (u32)value) >> 16;
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", value);
> +}
> +
> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ltc2471_show_value, NULL, 0);
> +
> +static struct attribute *ltc2471_attrs[] = {
> +	&sensor_dev_attr_in0_input.dev_attr.attr,
> +	NULL
> +};
> +
> +ATTRIBUTE_GROUPS(ltc2471);
> +
> +static int ltc2471_i2c_probe(struct i2c_client *i2c,
> +			     const struct i2c_device_id *id)
> +{
> +	int ret;
> +	struct device *hwmon_dev;
> +	struct ltc2471_data *data;
> +
> +	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C))
> +		return -ENODEV;
> +
> +	data = devm_kzalloc(&i2c->dev, sizeof(struct ltc2471_data), GFP_KERNEL);
> +	if (unlikely(!data))
> +		return -ENOMEM;
> +
> +	data->i2c = i2c;
> +	data->differential = (id->driver_data == ltc2473);
> +
> +	/* Trigger once to start conversion and check if chip is there */
> +	ret = ltc2471_get_value(i2c);
> +	if (ret < 0) {
> +		dev_err(&i2c->dev, "Cannot read from device.\n");
> +		return ret;
> +	}
> +
> +	hwmon_dev = devm_hwmon_device_register_with_groups(&i2c->dev,
> +							   i2c->name,
> +							   data,
> +							   ltc2471_groups);
> +
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct i2c_device_id ltc2471_i2c_id[] = {
> +	{ "ltc2471", ltc2471 },
> +	{ "ltc2473", ltc2473 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, ltc2471_i2c_id);
> +
> +static struct i2c_driver ltc2471_i2c_driver = {
> +	.driver = {
> +		.name = "ltc2471",
> +	},
> +	.probe    = ltc2471_i2c_probe,
> +	.id_table = ltc2471_i2c_id,
> +};
> +
> +module_i2c_driver(ltc2471_i2c_driver);
> +
> +MODULE_DESCRIPTION("LTC2471/LTC2473 Sensor Driver");
> +MODULE_AUTHOR("Topic Embedded Products");
> +MODULE_LICENSE("GPL v2");
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] hwmon: Add LTC2471/LTC2473 driver
  2017-06-29 11:18 ` Frans Klaver
  2017-06-29 11:30   ` Mike Looijmans
@ 2017-06-29 13:37   ` Guenter Roeck
  1 sibling, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2017-06-29 13:37 UTC (permalink / raw)
  To: Frans Klaver, Mike Looijmans; +Cc: linux-hwmon, linux-kernel, jdelvare

On 06/29/2017 04:18 AM, Frans Klaver wrote:
> On Thu, Jun 29, 2017 at 12:45 PM, Mike Looijmans
> <mike.looijmans@topic.nl> wrote:
>> The LTC2741 and LTC2473 are single voltage monitoring chips. The LTC2473
>> is similar to the LTC2471 but outputs a signed differential value.
>>
>> Datasheet:
>>    http://cds.linear.com/docs/en/datasheet/24713fb.pdf
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>> ---
>>   drivers/hwmon/Kconfig   |  10 ++++
>>   drivers/hwmon/Makefile  |   1 +
>>   drivers/hwmon/ltc2471.c | 127 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 138 insertions(+)
>>   create mode 100644 drivers/hwmon/ltc2471.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index fbde226..c9a2a87 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -673,6 +673,16 @@ config SENSORS_LINEAGE
>>            This driver can also be built as a module.  If so, the module
>>            will be called lineage-pem.
>>
>> +config SENSORS_LTC2471
>> +       tristate "Linear Technology LTC2471 and LTC2473"
>> +       depends on I2C
>> +       help
>> +         If you say yes here you get support for Linear Technology LTC2471
>> +         and LTC2473 voltage monitors.
>> +
>> +         This driver can also be built as a module. If so, the module will
>> +         be called ltc2471.
>> +
>>   config SENSORS_LTC2945
>>          tristate "Linear Technology LTC2945"
>>          depends on I2C
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 58cc3ac..6f60fe7 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -99,6 +99,7 @@ obj-$(CONFIG_SENSORS_LM93)    += lm93.o
>>   obj-$(CONFIG_SENSORS_LM95234)  += lm95234.o
>>   obj-$(CONFIG_SENSORS_LM95241)  += lm95241.o
>>   obj-$(CONFIG_SENSORS_LM95245)  += lm95245.o
>> +obj-$(CONFIG_SENSORS_LTC2471)  += ltc2471.o
>>   obj-$(CONFIG_SENSORS_LTC2945)  += ltc2945.o
>>   obj-$(CONFIG_SENSORS_LTC2990)  += ltc2990.o
>>   obj-$(CONFIG_SENSORS_LTC4151)  += ltc4151.o
>> diff --git a/drivers/hwmon/ltc2471.c b/drivers/hwmon/ltc2471.c
>> new file mode 100644
>> index 0000000..17eaad8
>> --- /dev/null
>> +++ b/drivers/hwmon/ltc2471.c
>> @@ -0,0 +1,127 @@
>> +/*
>> + * Driver for Linear Technology LTC2471 and LTC2473 voltage monitors
>> + * The LTC2473 is identical to the 2471, but reports a differential signal.
>> + *
>> + * Copyright (C) 2017 Topic Embedded Products
>> + * Author: Mike Looijmans <mike.looijmans@topic.nl>
>> + *
>> + * License: GPLv2
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +
>> +enum chips {
>> +       ltc2471,
>> +       ltc2473
>> +};
>> +
>> +struct ltc2471_data {
>> +       struct i2c_client *i2c;
>> +       bool differential;
>> +};
> 
> I haven't checked if there are similar drivers, but how about
> ltc247x_data? It's a bit odd to tie all naming in this driver to one
> chip and then support another one as well.
> 

No, we don't do that, since it implies ltc247[0-9] which could be completely different chips.

Guenter

>> +
>> +/* Reference voltage is 1.25V */
>> +#define LTC2471_VREF 1250
>> +
>> +/* Read two bytes from the I2C bus to obtain the ADC result */
>> +static int ltc2471_get_value(struct i2c_client *i2c)
>> +{
>> +       int ret;
>> +       __be16 buf;
>> +
>> +       ret = i2c_master_recv(i2c, (char *)&buf, 2);
>> +       if (ret < 0)
>> +               return ret;
>> +       if (ret != 2)
>> +               return -EIO;
>> +
>> +       /* MSB first */
>> +       return be16_to_cpu(buf);
>> +}
>> +
>> +static ssize_t ltc2471_show_value(struct device *dev,
>> +                                 struct device_attribute *da, char *buf)
>> +{
>> +       struct ltc2471_data *data = dev_get_drvdata(dev);
>> +       int value;
>> +
>> +       value = ltc2471_get_value(data->i2c);
>> +       if (unlikely(value < 0))
>> +               return value;
>> +
>> +       if (data->differential)
>> +               /* Ranges from -VREF to +VREF with "0" at 0x8000 */
>> +               value = ((s32)LTC2471_VREF * (s32)(value - 0x8000)) >> 15;
>> +       else
>> +               /* Ranges from 0 to +VREF */
>> +               value = ((u32)LTC2471_VREF * (u32)value) >> 16;
>> +
>> +       return snprintf(buf, PAGE_SIZE, "%d\n", value);
>> +}
>> +
>> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ltc2471_show_value, NULL, 0);
> 
> Octal permissions are preferred over S_IRUGO and such.
> 
>> +
>> +static struct attribute *ltc2471_attrs[] = {
>> +       &sensor_dev_attr_in0_input.dev_attr.attr,
>> +       NULL
>> +};
>> +
>> +ATTRIBUTE_GROUPS(ltc2471);
>> +
>> +static int ltc2471_i2c_probe(struct i2c_client *i2c,
>> +                            const struct i2c_device_id *id)
>> +{
>> +       int ret;
>> +       struct device *hwmon_dev;
>> +       struct ltc2471_data *data;
>> +
>> +       if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C))
>> +               return -ENODEV;
>> +
>> +       data = devm_kzalloc(&i2c->dev, sizeof(struct ltc2471_data), GFP_KERNEL);
> 
> sizeof(*data)
> 
>> +       if (unlikely(!data))
>> +               return -ENOMEM;
>> +
>> +       data->i2c = i2c;
>> +       data->differential = (id->driver_data == ltc2473);
>> +
>> +       /* Trigger once to start conversion and check if chip is there */
>> +       ret = ltc2471_get_value(i2c);
>> +       if (ret < 0) {
>> +               dev_err(&i2c->dev, "Cannot read from device.\n");
>> +               return ret;
>> +       }
>> +
>> +       hwmon_dev = devm_hwmon_device_register_with_groups(&i2c->dev,
>> +                                                          i2c->name,
>> +                                                          data,
>> +                                                          ltc2471_groups);
>> +
>> +       return PTR_ERR_OR_ZERO(hwmon_dev);
>> +}
>> +
>> +static const struct i2c_device_id ltc2471_i2c_id[] = {
>> +       { "ltc2471", ltc2471 },
>> +       { "ltc2473", ltc2473 },
>> +       {}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ltc2471_i2c_id);
>> +
>> +static struct i2c_driver ltc2471_i2c_driver = {
>> +       .driver = {
>> +               .name = "ltc2471",
>> +       },
>> +       .probe    = ltc2471_i2c_probe,
>> +       .id_table = ltc2471_i2c_id,
>> +};
>> +
>> +module_i2c_driver(ltc2471_i2c_driver);
>> +
>> +MODULE_DESCRIPTION("LTC2471/LTC2473 Sensor Driver");
>> +MODULE_AUTHOR("Topic Embedded Products");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 1.9.1
>>
> 
> Frans
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] hwmon: Add LTC2471/LTC2473 driver
  2017-06-29 12:30   ` Lars-Peter Clausen
  2017-06-29 13:22     ` Mike Looijmans
@ 2017-06-29 13:40     ` Guenter Roeck
  2017-06-29 14:11       ` Mike Looijmans
  1 sibling, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2017-06-29 13:40 UTC (permalink / raw)
  To: Lars-Peter Clausen, Mike Looijmans, linux-hwmon
  Cc: linux-kernel, jdelvare, linux-iio, Jonathan Cameron

On 06/29/2017 05:30 AM, Lars-Peter Clausen wrote:
> On 06/29/2017 02:13 PM, Mike Looijmans wrote:
>> The LTC2741 and LTC2473 are single voltage monitoring chips. The LTC2473
>> is similar to the LTC2471 but outputs a signed differential value.
>>
>> Datasheet:
>>    http://cds.linear.com/docs/en/datasheet/24713fb.pdf
> 
> This looks more like a general purpose ADC rather than a specialized voltage
> monitoring chip. In my opinion this driver would be better suited for the
> IIO framework. But before you start working on that lets try to hear what
> others think about this.
> 

Agreed (I just replied to that on another thread).

Guenter

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] hwmon: Add LTC2471/LTC2473 driver
  2017-06-29 13:22     ` Mike Looijmans
@ 2017-06-29 13:41       ` Guenter Roeck
  0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2017-06-29 13:41 UTC (permalink / raw)
  To: Mike Looijmans, Lars-Peter Clausen, linux-hwmon
  Cc: linux-kernel, jdelvare, linux-iio, Jonathan Cameron

On 06/29/2017 06:22 AM, Mike Looijmans wrote:
> On 29-06-17 14:30, Lars-Peter Clausen wrote:
>> On 06/29/2017 02:13 PM, Mike Looijmans wrote:
>>> The LTC2741 and LTC2473 are single voltage monitoring chips. The LTC2473
>>> is similar to the LTC2471 but outputs a signed differential value.
>>>
>>> Datasheet:
>>>    http://cds.linear.com/docs/en/datasheet/24713fb.pdf
>>
>> This looks more like a general purpose ADC rather than a specialized voltage
>> monitoring chip. In my opinion this driver would be better suited for the
>> IIO framework. But before you start working on that lets try to hear what
>> others think about this.
> 
> I'm neutral on it, it's being used as a hardware monitor on the board I have, but it's indeed a generic I2C ADC.
> 
> Do you have a suggestion for a similar device (simple I2C ADC) that I could base the IIO driver on? I've come across the AD799x which seems a good starting point.
> 

ltc2485, ltc2497 ? No idea how close they are.

Guenter

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] hwmon: Add LTC2471/LTC2473 driver
  2017-06-29 13:40     ` Guenter Roeck
@ 2017-06-29 14:11       ` Mike Looijmans
  2017-06-29 15:08         ` Guenter Roeck
  2017-06-29 15:30         ` Lars-Peter Clausen
  0 siblings, 2 replies; 16+ messages in thread
From: Mike Looijmans @ 2017-06-29 14:11 UTC (permalink / raw)
  To: Guenter Roeck, Lars-Peter Clausen, linux-hwmon
  Cc: linux-kernel, jdelvare, linux-iio, Jonathan Cameron

On 29-06-17 15:40, Guenter Roeck wrote:
> On 06/29/2017 05:30 AM, Lars-Peter Clausen wrote:
>> On 06/29/2017 02:13 PM, Mike Looijmans wrote:
>>> The LTC2741 and LTC2473 are single voltage monitoring chips. The LTC2473
>>> is similar to the LTC2471 but outputs a signed differential value.
>>>
>>> Datasheet:
>>>    http://cds.linear.com/docs/en/datasheet/24713fb.pdf
>>
>> This looks more like a general purpose ADC rather than a specialized voltage
>> monitoring chip. In my opinion this driver would be better suited for the
>> IIO framework. But before you start working on that lets try to hear what
>> others think about this.
>>
> 
> Agreed (I just replied to that on another thread).

Took a peek at the IIO drivers, looks simple enough, so I suggest to drop this 
patch and I'll create an IIO driver instead.

The ltc2485 (as suggested by Guenter on that other thread) is pretty similar 
indeed, it might be feasible to join the two and have one driver handle them 
both. That would be preferred over a separat driver, right?


Kind regards,

Mike Looijmans
System Expert

TOPIC Products
Materiaalweg 4, NL-5681 RJ Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] hwmon: Add LTC2471/LTC2473 driver
  2017-06-29 14:11       ` Mike Looijmans
@ 2017-06-29 15:08         ` Guenter Roeck
  2017-06-29 15:30         ` Lars-Peter Clausen
  1 sibling, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2017-06-29 15:08 UTC (permalink / raw)
  To: Mike Looijmans
  Cc: Lars-Peter Clausen, linux-hwmon, linux-kernel, jdelvare,
	linux-iio, Jonathan Cameron

On Thu, Jun 29, 2017 at 04:11:42PM +0200, Mike Looijmans wrote:
> On 29-06-17 15:40, Guenter Roeck wrote:
> >On 06/29/2017 05:30 AM, Lars-Peter Clausen wrote:
> >>On 06/29/2017 02:13 PM, Mike Looijmans wrote:
> >>>The LTC2741 and LTC2473 are single voltage monitoring chips. The LTC2473
> >>>is similar to the LTC2471 but outputs a signed differential value.
> >>>
> >>>Datasheet:
> >>>   http://cds.linear.com/docs/en/datasheet/24713fb.pdf
> >>
> >>This looks more like a general purpose ADC rather than a specialized voltage
> >>monitoring chip. In my opinion this driver would be better suited for the
> >>IIO framework. But before you start working on that lets try to hear what
> >>others think about this.
> >>
> >
> >Agreed (I just replied to that on another thread).
> 
> Took a peek at the IIO drivers, looks simple enough, so I suggest to drop
> this patch and I'll create an IIO driver instead.
> 
> The ltc2485 (as suggested by Guenter on that other thread) is pretty similar
> indeed, it might be feasible to join the two and have one driver handle them
> both. That would be preferred over a separat driver, right?
> 
In general yes, but it really depends on the similarities and differences. If
the single driver ends up twice as large, it is probably better to keep them
separate. If all you need to do is to add chip IDs, obviously a single driver
is better. Everything in between really depends on the amount of chip specific
code.

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] hwmon: Add LTC2471/LTC2473 driver
  2017-06-29 14:11       ` Mike Looijmans
  2017-06-29 15:08         ` Guenter Roeck
@ 2017-06-29 15:30         ` Lars-Peter Clausen
  1 sibling, 0 replies; 16+ messages in thread
From: Lars-Peter Clausen @ 2017-06-29 15:30 UTC (permalink / raw)
  To: Mike Looijmans, Guenter Roeck, linux-hwmon
  Cc: linux-kernel, jdelvare, linux-iio, Jonathan Cameron

On 06/29/2017 04:11 PM, Mike Looijmans wrote:
> On 29-06-17 15:40, Guenter Roeck wrote:
>> On 06/29/2017 05:30 AM, Lars-Peter Clausen wrote:
>>> On 06/29/2017 02:13 PM, Mike Looijmans wrote:
>>>> The LTC2741 and LTC2473 are single voltage monitoring chips. The LTC2473
>>>> is similar to the LTC2471 but outputs a signed differential value.
>>>>
>>>> Datasheet:
>>>>    http://cds.linear.com/docs/en/datasheet/24713fb.pdf
>>>
>>> This looks more like a general purpose ADC rather than a specialized voltage
>>> monitoring chip. In my opinion this driver would be better suited for the
>>> IIO framework. But before you start working on that lets try to hear what
>>> others think about this.
>>>
>>
>> Agreed (I just replied to that on another thread).
> 
> Took a peek at the IIO drivers, looks simple enough, so I suggest to drop
> this patch and I'll create an IIO driver instead.
> 
> The ltc2485 (as suggested by Guenter on that other thread) is pretty similar
> indeed, it might be feasible to join the two and have one driver handle them
> both. That would be preferred over a separat driver, right?
> 

I think the ltc2485 is close enough. It's even listed in the datasheet as a
related part. The main difference seems to be the resolution of the ADC and
the extra temperature sensor channel.

Although one thing to consider is that their control/mode register is quite
different. But the current driver doesn't write it.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] hwmon: Add LTC2471/LTC2473 driver
  2017-06-29 13:34 ` [PATCH] " Guenter Roeck
@ 2017-07-01  9:58   ` Jonathan Cameron
  0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2017-07-01  9:58 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Mike Looijmans, linux-hwmon, linux-kernel, linux-iio, jdelvare

On Thu, 29 Jun 2017 06:34:22 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 06/29/2017 03:45 AM, Mike Looijmans wrote:
> > The LTC2741 and LTC2473 are single voltage monitoring chips. The LTC2473
> > is similar to the LTC2471 but outputs a signed differential value.
> > 
> > Datasheet:
> >    http://cds.linear.com/docs/en/datasheet/24713fb.pdf
> > 
> > Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>  
> 
> Hi Mike,
> 
> that looks like a perfect candidate for an iio driver, since its use is
> not limited to hardware monitoring, it has a high sample rate, and it
> doesn't have limit registers or actions.
> 
> Jonathan, what do you think ?
Late response given I just applied the IIO driver ;)
Yes, fine for IIO.  Whilst they list hwmon as one of the possible
uses they list a whole load of others as well so IIO probably
makes more sense for this one.

Thanks,

Jonathan
> 
> Thanks,
> Guenter
> 
> > ---
> >   drivers/hwmon/Kconfig   |  10 ++++
> >   drivers/hwmon/Makefile  |   1 +
> >   drivers/hwmon/ltc2471.c | 127 ++++++++++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 138 insertions(+)
> >   create mode 100644 drivers/hwmon/ltc2471.c
> > 
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index fbde226..c9a2a87 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -673,6 +673,16 @@ config SENSORS_LINEAGE
> >   	  This driver can also be built as a module.  If so, the module
> >   	  will be called lineage-pem.
> >   
> > +config SENSORS_LTC2471
> > +	tristate "Linear Technology LTC2471 and LTC2473"
> > +	depends on I2C
> > +	help
> > +	  If you say yes here you get support for Linear Technology LTC2471
> > +	  and LTC2473 voltage monitors.
> > +
> > +	  This driver can also be built as a module. If so, the module will
> > +	  be called ltc2471.
> > +
> >   config SENSORS_LTC2945
> >   	tristate "Linear Technology LTC2945"
> >   	depends on I2C
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index 58cc3ac..6f60fe7 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -99,6 +99,7 @@ obj-$(CONFIG_SENSORS_LM93)	+= lm93.o
> >   obj-$(CONFIG_SENSORS_LM95234)	+= lm95234.o
> >   obj-$(CONFIG_SENSORS_LM95241)	+= lm95241.o
> >   obj-$(CONFIG_SENSORS_LM95245)	+= lm95245.o
> > +obj-$(CONFIG_SENSORS_LTC2471)	+= ltc2471.o
> >   obj-$(CONFIG_SENSORS_LTC2945)	+= ltc2945.o
> >   obj-$(CONFIG_SENSORS_LTC2990)	+= ltc2990.o
> >   obj-$(CONFIG_SENSORS_LTC4151)	+= ltc4151.o
> > diff --git a/drivers/hwmon/ltc2471.c b/drivers/hwmon/ltc2471.c
> > new file mode 100644
> > index 0000000..17eaad8
> > --- /dev/null
> > +++ b/drivers/hwmon/ltc2471.c
> > @@ -0,0 +1,127 @@
> > +/*
> > + * Driver for Linear Technology LTC2471 and LTC2473 voltage monitors
> > + * The LTC2473 is identical to the 2471, but reports a differential signal.
> > + *
> > + * Copyright (C) 2017 Topic Embedded Products
> > + * Author: Mike Looijmans <mike.looijmans@topic.nl>
> > + *
> > + * License: GPLv2
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
> > +#include <linux/i2c.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +
> > +enum chips {
> > +	ltc2471,
> > +	ltc2473
> > +};
> > +
> > +struct ltc2471_data {
> > +	struct i2c_client *i2c;
> > +	bool differential;
> > +};
> > +
> > +/* Reference voltage is 1.25V */
> > +#define LTC2471_VREF 1250
> > +
> > +/* Read two bytes from the I2C bus to obtain the ADC result */
> > +static int ltc2471_get_value(struct i2c_client *i2c)
> > +{
> > +	int ret;
> > +	__be16 buf;
> > +
> > +	ret = i2c_master_recv(i2c, (char *)&buf, 2);
> > +	if (ret < 0)
> > +		return ret;
> > +	if (ret != 2)
> > +		return -EIO;
> > +
> > +	/* MSB first */
> > +	return be16_to_cpu(buf);
> > +}
> > +
> > +static ssize_t ltc2471_show_value(struct device *dev,
> > +				  struct device_attribute *da, char *buf)
> > +{
> > +	struct ltc2471_data *data = dev_get_drvdata(dev);
> > +	int value;
> > +
> > +	value = ltc2471_get_value(data->i2c);
> > +	if (unlikely(value < 0))
> > +		return value;
> > +
> > +	if (data->differential)
> > +		/* Ranges from -VREF to +VREF with "0" at 0x8000 */
> > +		value = ((s32)LTC2471_VREF * (s32)(value - 0x8000)) >> 15;
> > +	else
> > +		/* Ranges from 0 to +VREF */
> > +		value = ((u32)LTC2471_VREF * (u32)value) >> 16;
> > +
> > +	return snprintf(buf, PAGE_SIZE, "%d\n", value);
> > +}
> > +
> > +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ltc2471_show_value, NULL, 0);
> > +
> > +static struct attribute *ltc2471_attrs[] = {
> > +	&sensor_dev_attr_in0_input.dev_attr.attr,
> > +	NULL
> > +};
> > +
> > +ATTRIBUTE_GROUPS(ltc2471);
> > +
> > +static int ltc2471_i2c_probe(struct i2c_client *i2c,
> > +			     const struct i2c_device_id *id)
> > +{
> > +	int ret;
> > +	struct device *hwmon_dev;
> > +	struct ltc2471_data *data;
> > +
> > +	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C))
> > +		return -ENODEV;
> > +
> > +	data = devm_kzalloc(&i2c->dev, sizeof(struct ltc2471_data), GFP_KERNEL);
> > +	if (unlikely(!data))
> > +		return -ENOMEM;
> > +
> > +	data->i2c = i2c;
> > +	data->differential = (id->driver_data == ltc2473);
> > +
> > +	/* Trigger once to start conversion and check if chip is there */
> > +	ret = ltc2471_get_value(i2c);
> > +	if (ret < 0) {
> > +		dev_err(&i2c->dev, "Cannot read from device.\n");
> > +		return ret;
> > +	}
> > +
> > +	hwmon_dev = devm_hwmon_device_register_with_groups(&i2c->dev,
> > +							   i2c->name,
> > +							   data,
> > +							   ltc2471_groups);
> > +
> > +	return PTR_ERR_OR_ZERO(hwmon_dev);
> > +}
> > +
> > +static const struct i2c_device_id ltc2471_i2c_id[] = {
> > +	{ "ltc2471", ltc2471 },
> > +	{ "ltc2473", ltc2473 },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ltc2471_i2c_id);
> > +
> > +static struct i2c_driver ltc2471_i2c_driver = {
> > +	.driver = {
> > +		.name = "ltc2471",
> > +	},
> > +	.probe    = ltc2471_i2c_probe,
> > +	.id_table = ltc2471_i2c_id,
> > +};
> > +
> > +module_i2c_driver(ltc2471_i2c_driver);
> > +
> > +MODULE_DESCRIPTION("LTC2471/LTC2473 Sensor Driver");
> > +MODULE_AUTHOR("Topic Embedded Products");
> > +MODULE_LICENSE("GPL v2");
> >   
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2017-07-01  9:58 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-29 10:45 [PATCH] hwmon: Add LTC2471/LTC2473 driver Mike Looijmans
2017-06-29 11:18 ` Frans Klaver
2017-06-29 11:30   ` Mike Looijmans
2017-06-29 11:38     ` Frans Klaver
2017-06-29 12:08       ` Mike Looijmans
2017-06-29 13:37   ` Guenter Roeck
2017-06-29 12:13 ` [PATCH v2] " Mike Looijmans
2017-06-29 12:30   ` Lars-Peter Clausen
2017-06-29 13:22     ` Mike Looijmans
2017-06-29 13:41       ` Guenter Roeck
2017-06-29 13:40     ` Guenter Roeck
2017-06-29 14:11       ` Mike Looijmans
2017-06-29 15:08         ` Guenter Roeck
2017-06-29 15:30         ` Lars-Peter Clausen
2017-06-29 13:34 ` [PATCH] " Guenter Roeck
2017-07-01  9:58   ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).