On Tue, Feb 26, 2013 at 11:07:16PM +0100, Jean Delvare wrote:
> Hi Guenter,
>
> On Fri, 22 Feb 2013 18:19:31 -0800, Guenter Roeck wrote:
> > Peak attributes were not initialized and cleared correctly.
> >
> > Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> > ---
> > Candidate for -stable.
>
> Agreed, once it is correct...
>
Maybe I'll even manage to get it right at some point in the far distant future ...
> >
> > drivers/hwmon/pmbus/ltc2978.c | 24 ++++++++++++++----------
> > 1 file changed, 14 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
> > index 9652a2c..eec294a 100644
> > --- a/drivers/hwmon/pmbus/ltc2978.c
> > +++ b/drivers/hwmon/pmbus/ltc2978.c
> > @@ -248,26 +248,26 @@ static int ltc2978_write_word_data(struct i2c_client *client, int page,
> >
> > switch (reg) {
> > case PMBUS_VIRT_RESET_IOUT_HISTORY:
> > - data->iout_max[page] = 0x7fff;
> > + data->iout_max[page] = 0x7c00;
> > ret = ltc2978_clear_peaks(client, page, data->id);
> > break;
> > case PMBUS_VIRT_RESET_TEMP2_HISTORY:
> > - data->temp2_max[page] = 0x7fff;
> > + data->temp2_max[page] = 0x7c00;
> > ret = ltc2978_clear_peaks(client, page, data->id);
> > break;
> > case PMBUS_VIRT_RESET_VOUT_HISTORY:
> > data->vout_min[page] = 0xffff;
> > - data->vout_max[page] = 0;
> > + data->vout_max[page] = 0x0000;
>
> If the patch is intended for stable you may want to avoid this kind of
> avoidable change.
>
> > ret = ltc2978_clear_peaks(client, page, data->id);
> > break;
> > case PMBUS_VIRT_RESET_VIN_HISTORY:
> > data->vin_min = 0x7bff;
> > - data->vin_max = 0;
> > + data->vin_max = 0x7c00;
> > ret = ltc2978_clear_peaks(client, page, data->id);
> > break;
> > case PMBUS_VIRT_RESET_TEMP_HISTORY:
> > - data->temp_min = 0x7bff;
> > - data->temp_max = 0x7fff;
> > + data->temp_min = 0xfbff;
>
> This looks wrong, 0x7bff was correct as far as I can see? And that's
> what you still use during probe, BTW.
>
No idea what I am doing. Typo :(.
> > + data->temp_max = 0x7c00;
> > ret = ltc2978_clear_peaks(client, page, data->id);
> > break;
> > default:
> > @@ -321,10 +321,10 @@ static int ltc2978_probe(struct i2c_client *client,
> > info = &data->info;
> > info->write_word_data = ltc2978_write_word_data;
> >
> > - data->vout_min[0] = 0xffff;
> > data->vin_min = 0x7bff;
> > + data->vin_max = 0x7c00;
> > data->temp_min = 0x7bff;
> > - data->temp_max = 0x7fff;
> > + data->temp_max = 0x7c00;
> >
> > switch (id->driver_data) {
> > case ltc2978:
> > @@ -336,7 +336,6 @@ static int ltc2978_probe(struct i2c_client *client,
> > for (i = 1; i < 8; i++) {
> > info->func[i] = PMBUS_HAVE_VOUT
> > | PMBUS_HAVE_STATUS_VOUT;
> > - data->vout_min[i] = 0xffff;
> > }
> > break;
> > case ltc3880:
> > @@ -352,11 +351,16 @@ static int ltc2978_probe(struct i2c_client *client,
> > | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT
> > | PMBUS_HAVE_POUT
> > | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
> > - data->vout_min[1] = 0xffff;
> > + data->temp2_max[0] = 0x7c00;
> > + data->temp2_max[1] = 0x7c00;
>
> Unrelated to this patch, but how is data->temp2_max[1] is supposed to
> be used when only page 0 has PMBUS_HAVE_TEMP2 defined?
>
And why is it an array in the first place ? Yes, I know. That was removed
in one of the subsequent patches, but I might as well do it in this one.
> > break;
> > default:
> > return -ENODEV;
> > }
> > + for (i = 0; i < info->pages; i++) {
> > + data->vout_min[i] = 0xffff;
> > + data->iout_max[i] = 0x7c00;
>
> Looks very wrong, data->iout_max[] contains 2 elements and info->pages
> could be 8. Only the LTC3880 needs data->iout_max[] anyway, right?
>
> > + }
>
> IMHO it would make more sense to have this grouped with the other
> similar initializations. Or left where it was originally...
>
info->pages is only known here, so I would have to use the maximum number
of pages as loop terminator if I move it up.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
