Re: [lm-sensors] [PATCH RFC 0/2] tmp401: Add support for Texas

[Hans de Goede <hdegoede@redhat.com>]

Hi Andre,

On 05/14/2009 10:06 AM, Andre Prendel wrote:
> Hello Jean, Hans,
>
> here is my first work at TI's 401/411 temperatur sensor. The patch(es)
> aren't intented to be applied. I just want some comments and maybe
> real testing by Hans.
>
> The first patch is the original one from Hans sent to me.
>
> The second patch contains some work of Hans' students. I've brought
> it in shape. The driver now has multichip support in the usual way.
>
> TODO:
> Update Kconfig
> Writing documentation
>

Great, thanks for working on this!

> There are a few remaining questions.
>
> 1. Hans' students implemented two additional sysfs entries
> (tempX_[lowest|highest]).
>
> The TMP411 chip has some additional registers for minimum and maximum
> temperatures measured since power-on and chip-reset.
>
> AFAIK there are no standard sysfs entries for these values. I don't
> remove the support so far. Should it be removed? Are there other chips
> with such features?
>

Ah, good point I forgot about that, I told my student to name the new
tempX_[lowest|highest] based on the existing and documented
(Documentation/hwmon/sysfs-interface) powerX_average_[lowest|highest]
sysfs attributes, which do the same (tracking min / max over time) for
powermeter IC's. So unless someone objects, I think these should stay
and get documented in Documentation/hwmon/sysfs-interface.

> 2. There is another command to reset the chip. Is this a useful feature?
>

An other very good point! My first response was no, but actually it is,
as this can be used to reset the min / max temp tracking. Modelling
after the powermeter API again, you should add a temp_reset_history, notice
no number there as it resets the history for all temps at once (AFAIK).

Again this needs to be documented, in 2 variants: temp_reset_history and
tempX_reset_history

> 3. Indentation
>
> What is the right way to indent arguments in function declaration?
>
> a)
> static int tmp401_probe(struct i2c_client *client,
>                          const struct i2c_device_id *id)
>
> b)
> static int tmp401_probe(struct i2c_client *client,
>         const struct i2c_device_id *id)
>
> My emacs prefered the first one, but sometimes this doesn't look very
> well.
>

Erm, I think both are allowed, I myself usually use b, but I don't think
I'm consistent with that.

> 4. May I add a copyright note of me in the header?
>

Sure.


> Again, thank you very much for your help. It's a pleasure to work with
> you.

And it is a pleasure to work with you sir!


Regards,

Hans


p.s.

I somehow am missing patch 2/2, or am I just lacking patience ?


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors