linux-kernel - Re: [PATCH] hwmon: Add LTC2990 sensor driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 13 Jan 2016 12:22:58 +0100
From:	Mike Looijmans <mike.looijmans@...ic.nl>
To:	Guenter Roeck <linux@...ck-us.net>, <lm-sensors@...sensors.org>
CC:	<jdelvare@...e.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] hwmon: Add LTC2990 sensor driver

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@...ic.nl>
>>>> ---
>>>>   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@...icproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail