Re: [PATCH] hwmon: Add LTC2990 sensor driver

From: Mike Looijmans
Date: Wed Jan 13 2016 - 07:37:36 EST


ïOn 08-01-16 16:09, Guenter Roeck wrote:
On 01/07/2016 10:59 AM, Mike Looijmans wrote:
Thank you very much for your review comments, I'll update the driver and
post a v2 patch.

Inlined some replies below. Assume that I "will do" for all comments I
didn't comment on inline...

On 06-01-16 16:22, Guenter Roeck wrote:
Hello Mike,

On 01/06/2016 12:07 AM, Mike Looijmans wrote:
This adds support for the Linear Technology LTC2990 I2C System Monitor.

s/ / /

The LTC2990 supports a combination of voltage, current and temperature
monitoring, but this driver currently only supports reading two currents
by measuring two differential voltages across series resistors.

Plus VCC, plus the internal temperature.

Yeah, I should give myself more credit :) I'll add that in Kconfig too.

This is sufficient to support the Topic Miami SOM which uses this chip
to monitor the currents flowing into the FPGA and the CPU parts.

Signed-off-by: Mike Looijmans <mike.looijmans@xxxxxxxx>
---
drivers/hwmon/Kconfig | 15 +++
drivers/hwmon/Makefile | 1 +
drivers/hwmon/ltc2990.c | 273
++++++++++++++++++++++++++++++++++++++++++++++++


[ ... ]

+}
+
+static struct ltc2990_data *ltc2990_update_device(struct device *dev)
+{
+ struct i2c_client *i2c = to_i2c_client(dev);
+ struct ltc2990_data *data = i2c_get_clientdata(i2c);
+ struct ltc2990_data *ret = data;
+ unsigned int timeout;
+
+ mutex_lock(&data->update_lock);
+
+ /* Update about 4 times per second max */
+ if (time_after(jiffies, data->last_updated + HZ / 4) ||
!data->valid) {
+ int val;
+ int i;
+

Please consider using continuous conversion. This would simplify the
code significantly
and reduce read delays.

It might increase power consumption though, as typically some user program
would poll this every 10 seconds or so. I'll check the data sheet.


I suspect that the power savings will be less than the added power
consumed by the CPU due to the more complex code.

Really, unless you have an application where a few mW power savings
are essential and warrant the additional code complexity, this is
the wrong approach.

Yeah, you're right, checked the datasheet and it reports a 1mA typical power usage when converting. Not worth the trouble.

Continuous conversion made the driver a LOT simpler, could even completely drop the private data structure.

I sent v2 already, but just noticed that some of the #includes were no longer
needed, so I'll send a v3 to fix that.

...


Kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans@xxxxxxxxxxxxxxxxx
Website: www.topicproducts.com

Please consider the environment before printing this e-mail