On Sun, 31 May 2009 21:08:17 +0200, Hans de Goede wrote:
>
>
> On 05/30/2009 08:58 AM, Jean Delvare wrote:
> > Hi Hans,
> >
> > On Fri, 29 May 2009 11:06:06 +0200, Hans de Goede wrote:
> >> Add support for the hwmon part of the Fintek f71858fg superio IC to the
> >> f71882fg driver. Many thanks to Jelle de Jong for lending me a motherboard
> >> with this superio on it.
> >>
> >> Signed-off-by: Hans de Goede<hdegoede at redhat.com>
> >
> > Review:
> >
>
> Many thanks for the quick review !
>
> >> (...)
> >> static struct f71882fg_data *f71882fg_update_device(struct device *dev)
> >> {
> >> struct f71882fg_data *data = dev_get_drvdata(dev);
> >> int nr, reg = 0, reg2;
> >> int nr_fans = (data->type == f71882fg) ? 4 : 3;
> >> - int nr_ins = (data->type == f8000) ? 3 : 9;
> >> - int temp_start = (data->type == f8000) ? 0 : 1;
> >> + int nr_ins = (data->type == f71858fg || data->type == f8000) ? 3 : 9;
> >> + int temp_start =
> >> + (data->type == f71858fg || data->type == f8000) ? 0 : 1;
> >
> > It might be a good idea to add these values to struct f71882fg_data
> > (maybe in a separate patch) so that you don't have to compute them
> > every time. Especially temp_start is used in several functions.
> >
>
> I've done this for temp_start.
>
> >> (...)
> >> @@ -1166,8 +1230,22 @@ static ssize_t show_temp(struct device *
> >> {
> >> struct f71882fg_data *data = f71882fg_update_device(dev);
> >> int nr = to_sensor_dev_attr_2(devattr)->index;
> >> + int sign, temp;
> >> +
> >> + if (data->type == f71858fg) {
> >> + if ((data->temp_config& 3) == 3)
> >
> > If I read the datasheet correctly, this should be:
> >
> > if ((data->temp_config& 1) == 1)
> >
> > Both modes 1 and 3 have the sign in the LSb while modes 0 and 2 have
> > the sign in the MSb.
> >
> >> + sign = data->temp[nr]& 0x0001;
> >> + else
> >> + sign = data->temp[nr]& 0x8000;
> >> +
> >> + temp = (data->temp[nr]>> 5)& 0x3ff;
> >
> > This is only correct for modes 0 and 2. For modes 1 and 3, there are 11
> > bits of data, not 10, so the mask should be 0x7ff.
> >
>
> You are right on both accounts, both fixed.
>
> <snip>
>
> >> @@ -1773,6 +1873,12 @@ static int __devinit f71882fg_probe(stru
> >>
> >> /* Sanity check the pwm settings */
> >> switch (data->type) {
> >> + case f71858fg:
> >> + err = 0;
> >> + for (i = 0; i< nr_fans; i++)
> >> + if (((data->pwm_enable>> i * 2)& 3) == 3)
> >
> > Ditto for parentheses.
> >
>
> Fixed for both occurences.
>
> Updated version attached.
Looks good this time, applied, thanks.
--
Jean Delvare
