Hi All,
While actually updating the driver I realized I didn't respond to all points
raised (I think), so here is my reply to the other points.
>> +static struct f71882fg_data *f71882fg_update_device(struct device * dev)
>> +{
>> + struct f71882fg_data *data = dev_get_drvdata(dev);
>> + int nr, reg, reg2;
>> +
>> + mutex_lock(&data->update_lock);
>> +
>> + /* Update once every 60 seconds */
>> + if ( time_after(jiffies, data->last_updated + 60 * HZ ) ||
>> + !data->valid) {
>
> You test last_updated here but update last_limits below. Is that
> really what you intended? last_limits isn't used anywhere in
> the driver.
>
Good catch, will fix.
>> +static ssize_t store_in_max(struct device *dev, struct device_attribute
>> + *devattr, const char *buf, size_t count)
>> +{
>> + struct f71882fg_data *data = dev_get_drvdata(dev);
>> + int val = simple_strtoul(buf, NULL, 10) / 8;
>> +
>> + if (val > 255)
>> + val = 255;
>
> This check won't catch the case where I feed your sysfs file "-4096"
> and the value inside f71882fg_data won't match what gets written to the
> port. Granted, the maxim "If you write garbage to the control systems
> you deserve what you get" might apply here too.
>
Notice that I use simple_strtoul, so val will never be < 0, if you write -4096
strtoul will not recognize the - and return 0.
I know it looks strange to store the return value in an int then, but in some
of the other store methods I need val to be signed, and for consistency I've
thus stored it into an int everywhere.
Regards,
Hans
