Hi Hans,
Thanks for the fast review :)
On Sun, 07 Oct 2007 21:11:20 +0200, Hans de Goede wrote:
> Jean Delvare wrote:
> > The upcoming libsensors 3 needs these individual alarm and beep files.
> > For the W83781D, this is quirky because this chip has a single alarm
> > bit for both temp2 and temp3.
>
> Here is a quick review:
>
> <snip>
>
> > +static ssize_t
> > +store_beep(struct device *dev, struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct w83781d_data *data = w83781d_update_device(dev);
> > + int bitnr = to_sensor_dev_attr(attr)->index;
> > + unsigned long bit;
> > + u8 reg;
> > +
>
> You've just thought me not to call foo_update_device() in store functions,
> since I agree with your assessment that calling foo_update_device() in store
> functions is bad, please fix this.
You're totally right. Probably the result of a careless copy-and-paste.
I'll fix and resubmit.
> > + bit = simple_strtoul(buf, NULL, 10);
> > + if (bit & ~1)
> > + return -EINVAL;
> > + bit <<= bitnr;
> > +
> > + mutex_lock(&data->update_lock);
> > + data->beep_mask &= ~(1 << bitnr);
> > + data->beep_mask |= bit;
>
> Hmm, we need a clearer convention for writes to sysfs attr which are a boolean.
> In both of my abituguru drivers and in the fschmd driver I use:
> unsigned long bool = bit = simple_strtoul(buf, NULL, 10);
>
> And then
> if (bool)
> ...
> else
> ...
>
> Your code above does things different. So I think we need a convention for this
> (and add the conention to the sysfs-interface doc). I prefer my way, if only
> for not having to change it everywhere.
We already have a convention. From sysfs-interface: "If it is not
continuous like for example a tempX_type, then when an invalid value is
written, -EINVAL should be returned." Boolean values fall under this
"not continuous" category, which means that unsupported values should
trigger an error.
> I would like to suggest the following
> for your code:
>
> > + bit = simple_strtoul(buf, NULL, 10);
> > +
> > + mutex_lock(&data->update_lock);
> > + if (bit)
> > + data->beep_mask |= 1 << bitnr;
> > + else
> > + data->beep_mask &= ~(1 << bitnr);
>
> I must say I also find this more readable / cleaner.
I was a bit reluctant at first because it makes the code larger, but
well, if you think it's more readable that way, this section isn't
time-critical so why not.
I'll retest and resubmit my patch later today.
--
Jean Delvare
