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

[Guenter Roeck]

On 11/09/2013 07:56 AM, Arnaud Ebalard wrote:
> Hi,
>
> Guenter Roeck <linux@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> >
> > 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/