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

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

[Andre Prendel]

On Thu, May 14, 2009 at 11:02:40AM +0200, Hans de Goede wrote:
> 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 ?

D'oh, I have missed to CC you and the list.

Andre

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

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

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

[Andre Prendel]

On Mon, May 18, 2009 at 12:18:10PM +0200, Hans de Goede wrote:
> 
> 
> On 05/18/2009 11:37 AM, Andre Prendel wrote:
> > Having a closer look at the datasheet
> >
> > http://focus.ti.com/docs/prod/folders/print/tmp411.html
> >
> > , I've seen two ways to reset the history.
> >
> > 1. Writing any value to the history registers (0x30-0x37). That should
> > reset all these registers. I've attached a patch (delta) doing this. Hans,
> > could you please test this one. echo "1">  .../temp_reset_history
> > should reset the history for tempX_lowest (0xFFF0) and tempX_highest (0x0000).
> >
> > 2. SOFTWARE RESET
> >
> > Writing the first e-mail I meant this command in my question. You can
> > find it in datasheet under SOFTWARE RESET. This command restores the
> > power-on values to all registers (including limits etc.).
> >
> > I'd prefer 1. for reseting the history. That's what I'd expect from
> > temp_reset_history.
> >
> 
> Ack, I think 1 is much better too.
> 
> > BTW, what about write-only attributes? I've seen a reset functionality
> > in the fscpos driver (so I did it like that). show_temp_reset always
> > returns 1. Using WO attributes could make more sense, but in
> > sysfs-interface I can only see RO and RW.
> >
> 
> The fscpos driver is an old obsolete driver, so not the best place to look
> for inspiration. I think that using 0200 as rights for something like
> temp_reset_history makes perfect sense.
> 
> I've tested you're latest patch and temp_reset_history works as advertised.
> 
> If you fix the mode for the temp_reset_history, and do a new patch adding
> docs, then I think this is good to go to Jean's tree for merging into 2.6.31.
> Jean do you agree?

Alright, I will do so.

> 
> Regards,
> 
> Hans

Thanks,
Andre

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

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