Re: [PATCH] hwmon: Add LTC2990 sensor driver

[Mike Looijmans]

ï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