Message-ID: <20110623153556.GG28311@ericsson.com>
Date:	Thu, 23 Jun 2011 08:35:56 -0700
From:	Guenter Roeck <guenter.roeck@...csson.com>
To:	Alexander Stein <alexander.stein@...tec-electronic.com>
CC:	Jean Delvare <khali@...ux-fr.org>,
	"lm-sensors@...sensors.org" <lm-sensors@...sensors.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] hwmon: LM95245 driver

On Thu, Jun 23, 2011 at 11:14:19AM -0400, Alexander Stein wrote:
> On Thursday 23 June 2011 16:47:55 Guenter Roeck wrote:
> > > Another point is the optional offset register (not implemented, see
> > > below). I could not found much about it, but I guess this is immediately
> > > added to the remote temperature register.
> > 
> > You could set it to some value and see what happens.
> 
> I might look into that, when I implement the offset feature.
> 
> > > > Other comments:
> > > > 
> > > > For the interval attribute, idea would be to write the value into the
> > > > conversion rate register. Your code does not match the interval with
> > > > the rate programmed into the chip (which is the idea), nor does it
> > > > update the rate if the interval is changed.
> > > 
> > > Well, I noticed that. But I went the way lm95241 does. I'm also unsure
> > > which interval to choose, if user specify a unsupported interval. Choose
> > > the next small or the next greater one? Maybe you can give me a hint
> > > here.
> > 
> > Two bad or wrong implementations don't make it a good one. If the value can
> > be configured into the lm95241, the code should do it.
> > 
> > The lm90 driver tries to find the closest match, which would be one way to
> > go and is what I usually do. Another possibility would be to select the
> > next larger valid interval.
> 
> I think the closest match might be the best. As there is fixed set of 
> possibilities, what do think about printing the possiblities upon read and 
> marking the used one by an asterisk?
> 
Please stick with the ABI. libsensors won't be able to decode it if you make it
fancy, and it would violate the one-object-per-entry rule for sysfs attributes.

> > Another thing I noticed: You are re-configuring the chip during
> > initialization. This is even though the chip may already have been
> > configured through ROMMON and/or BIOS, and specifically overrides the
> > external sensor type. That is not a good idea; even though it may make
> > sense in your application, it may screw up chip configuration for other
> > users of the chip.
> >
> > If your BIOS/Firmware does not configure the chip, and you really have to
> > do it in the driver, one option would be to provide configuration data
> > through optional platform data.
> 
> Ehm, where do I add platform data on x86? It gets detected by device probing. 
> For boards like arm, sure no problem there.
> 
Good point ;).

Question is if you really need to reconfigure the chip. On X86 that should really have been
done by the BIOS. Only reasonable option here would really be to enable the chip
if it is disabled by default, and leave the rest to userspace based initialization
(ie use sensors3.conf to set desired values).

For most of the configured values (such as the update interval) it doesn't matter much
if you overwrite it when starting the driver. I am mostly concerned about the diode type,
which is the one relevant piece of configuration which should be known to the firmware
and should already be configured to the correct value when the driver is started. Even if
that is not the case with your board, it might and should be for other boards.

Thanks,
Guenter

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

Powered by blists - more mailing lists