Re: [PATCHv0] hwmon: Add support for GMT G751 Temp. Sensor and Thermal Watchdog

[arno@natisbad.org (Arnaud Ebalard)]

Hi Guenter,

Guenter Roeck <linux@roeck-us.net> writes:

>> Sadly (for me), you are not: I compared the GMT G751 datasheet to an
>> original (1996) National semiconductor LM75 datasheet and they are
>> identical. I mean both the structure and full content (text, diagrams,
>> etc) is the same. Lesson learned: next time I start a driver, I will ask
>> if it ressembles an existing supported chip beforehand.
>>
>
> Hi Arnaud,
>
> that is interesting; I thought it is Yet Another Clone, not really exactly
> the same chip.

If you want to compare:

http://www.ieap.uni-kiel.de/surface/ag-berndt/lehre/fpmc/ns/lm75.pdf
http://natisbad.org/NAS4/refs/GMT_G751.pdf

>>> Please use the lm75 driver and add the g751 parameters to it.
>>
>> I will test if the driver does indeed work as expected to drive the G751
>> and will send a patch to document compatibility w/ GMT G751 (Kconfig,
>> i2c_device_id struct and lm75_detect function). While I am at it, if you
>> see something in the patch I pushed which could be useful for current
>> lm75 driver (doc, sysfs, of_ part for polarity, ...), just tell me.
>>
>
> Depends on what you need. The fault_queue and mode sysfs attributes are neither
> necessary nor  acceptable - hwmon has well defined attributes, and new ones
> are only added after discussion. If you _need_ to configure polarity,
> interrupt mode, or fault queue depth in your application to anything but
> the default, we might discuss adding those as devicetree properties.
> However, you would have to make sure that it does not negatively affect
> the other chips supported by the driver, and we should then discuss
> if other properties should be supported as well. Overall, I strongly suspect
> that the HW is happy with the default configuration. If so, we should just leave
> it alone.

Let's keep lm75 in I2C trivial devices list.

> Power control (the shutdown attribute) should be handled through the PM
> subsystem; see CONFIG_PM / CONFIG_PM_SLEEP in other drivers. If your hardware
> can sleep (which may be somewhat unlikely for a NAS),

lm75 driver does have #ifdef CONFIG_PM (or am I missing something?), but
you are right, I don't think the NAS can take advantage of it ATM.

I just finished the a first version of a patch for lm75 to reference
g751. I'll send it in a separate email.

Anyway, thanks for the quick feedback.

Cheers,

a+