Messages in this thread | | | Date | Sat, 09 Nov 2013 08:55:36 -0800 | From | Guenter Roeck <> | Subject | Re: [PATCHv0] hwmon: Add support for GMT G751 Temp. Sensor and Thermal Watchdog |
| |
On 11/09/2013 07:56 AM, Arnaud Ebalard wrote: > Hi, > > Guenter Roeck <linux@roeck-us.net> writes: > >> On 11/08/2013 03:31 PM, Arnaud Ebalard wrote: >>> >>> This patch adds support for GMT G751 Temperature Sensor and Thermal >>> Watchdog I2C chip. It has been tested via DT on a Netgear ReadyNAS >>> 2120 (Marvell Armada XP based ARM device). >>> >>> Signed-off-by: Arnaud Ebalard <arno@natisbad.org> >> >> Arnaud, >> >> unless I am missing something, this is just an lm75 with a different >> name. > > 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.
>> 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.
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), you could add support for it to the driver. That is the one improvement I could think of that might make sense.
Thanks, Guenter
| |