On Sun, Oct 03, 2010 at 06:02:26AM -0400, Jean Delvare wrote:
[ .. ]
> > > I think we want to come up with a unified way to set per-chip
> > > attributes, and stick to it. This could either be done with switch/case
> > > in lm90_probe(), as we already have, or using a chips-indexed array of
> > > property structures.
> >
> > I had thought about another switch/case in probe, but didn't like it because
> > it is getting large.
> >
> > I do like the idea of a chips-indexed structure. That could cover alarms, flags,
> > and conversion rates.
> >
> > One patch or two ? Seems to me that adding such a structure should be
> > done in a separate patch.
>
> Ideally, yes.
>
Already working on it.
> > > > (...)
> > > > +{
> > > > + int i;
> > > > +
> > > > + /* find the nearest update rate from the table */
> > > > + for (i = 0; i < ARRAY_SIZE(update_intervals) - 1; i++) {
> > > > + if (interval >= update_intervals[i]
> > > > + || i >= max_convrates[data->kind])
> > >
> > > This algorithm doesn't actually give you the nearest update rate. It
> > > gives you the immediately lesser supported value. To get the nearest
> > > supported value, you would have to compare with (update_intervals[i] +
> > > update_intervals[i + 1]) / 2. Your current algorithm would give 8000 ms
> > > is the user asks for 15000 ms, while 16000 ms is arguably a better
> > > match.
> > >
> > Good point. I'll use rounding instead. Should be
> > update_intervals[i] - update_intervals[i + 1]) / 2
> > though (-, not +).
>
> +, sorry to insist. You want to compare the requested value with the
> middle point between each supported value. See set_pwm_freq() in it87.c
> for an example.
>
Umm .. time for some thinking.
The output below was created with a test program which uses '-'.
Ok, I know what is different. I didn't use ().
Let's see.
ui[0] = 16000, ui[1] = 8000
(ui[0] + ui[1]) / 2 = 24000/2 = 12000
ui[0] - ui[1] / 2 = 16000 - 8000/2 = 12000
and with ui[i+1] = ui[1]/2:
(ui[1] + ui[i]/2) / 2 = ui[1]/2 + ui[i]/4 = ui[i] * 3 / 4
ui[i] - (ui[1]/2)/2 = ui[i] - ui[1]/4 = ui[i] * 3 / 4
So we are both right ;). I'll use your version - it looks a bit cleaner to me.
I'll also get rid of the table - it _is_ really unnecessary.
>
> > > > (...)
> > > > - if (time_after(jiffies, data->last_updated + HZ / 2 + HZ / 10)
> > > > - || !data->valid) {
> > > > + next_update = data->last_updated
> > > > + + msecs_to_jiffies(data->update_interval) + HZ / 10;
> > >
> > > We probably want to reconsider this HZ / 10. 100 ms on top of 500 ms
> > > wasn't too noticeable. 100 ms on top of 31 ms or even 16 ms is way too
> > > much.
> > >
> > How about the following ?
> >
> > next_update = data->last_updated +
> > msecs_to_jiffies(data->update_interval + data->update_interval / 8)
> >
> > I used /8 to avoid a divide operation. The old constant was 20% above
> > the update interval. This is 12.5% above. Should still be ok, even for HZ=100.
>
> This should work fine, even though the rationale for such a formula is
> hard to justify. The margin is to make sure the chip has the time to
> complete its conversion and isn't interrupted by serial communications.
> There is no technical reason I can see to make this safety margin
> depend on the cache lifetime.
>
Good point. Guess I have too much tendency to not changing behavior once in a while.
A better option would be to use
next_update = data->last_updated
+ msecs_to_jiffies(data->update_interval) + 1;
Should still be good enough, because it still guarantees that the chip can complete
at least one conversion.
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
