Re: [lm-sensors] [PATCH] hwmon: Driver for Texas Instruments amc6821 chip

[Tomaz Mertelj]

At 09:34 9. 9. 2009 +0200, Jean Delvare wrote:
> > I'm wondering if these functions need to be so huge.  Couldn't you do
> >
> > #define set_temp_para(name, reg)\
> > static ssize_t set_##name(\
> >               struct device *dev,\
> >               struct device_attribute *attr,\
> >               const char *buf,\
> >               size_t count)\
> > {\
> >       return set_helper(dev, attr, buf, count, &dev->name);\
> > }
> >
> > And then do all the real work in a common function?  Rather than
> > expanding tens of copies of the same thing?
>
> Yes please. We got rid of macro-generated callbacks in most hwmon
> drivers a couple years ago already.

I do not like macro-generated callbacks myself as well. However, I was 
impatient to get the
driver working and since I have seen similar things in a few other drivers ...

I would prefer a single callback (would require some more work):

static ssize_t set_temp(
        struct device *dev,
        struct device_attribute *attr,
        const char *buf,
        size_t count)
{
        struct i2c_client *client = to_i2c_client(dev);
        struct amc6821_data *data = i2c_get_clientdata(client);
        int nr = to_sensor_dev_attr(attr)->index;
        int val = simple_strtol(buf, NULL, 10);
        val = SENSORS_LIMIT(val / 1000, -128, 127);
        int *pvar;
        u8 reg;

        switch (nr) {
        case GET_SET_TEMP1_MIN:
                pvar=&data->temp1_min;
                reg=AMC6821_REG_LTEMP_LIMIT_MIN;
                break;
        case ...

        ...

        default:
                dev_dbg(dev, "Unknown attr->index (%d)\n", nr);
                return SOME_ERROR;
        }
        mutex_lock(&data->update_lock);
        *pvar=val;
        if (i2c_smbus_write_byte_data(client, reg, *pvar)) {
                dev_err(&client->dev, "Register write error, aborting.\n");
                count = -EIO;
        }
        mutex_unlock(&data->update_lock);
        return count;
}


static SENSOR_DEVICE_ATTR(temp1_min, S_IRUGO | S_IWUSR, get_temp,
        set_temp, GET_SET_TEMP1_MIN);
...



> >
> > Also, the checkpatch warning
> >
> > WARNING: consider using strict_strtol in preference to simple_strtol
> > #381: FILE: drivers/hwmon/amc6821.c:228:
> > +       int val = simple_strtol(buf, NULL, 10); \
> >
> > is valid.  The problem with simple_strtol() is that it will treat input
> > of the form "43foo" as "43".  Even though the input was invalid.  A
> > minor thing, but easily fixed too.
>
> Is there any legitimate use of simple_strtol then? I'm wondering why we
> don't just get rid of it and rename strict_strtol to just strtol.

I have seen simple_strtol in many hwmon drivers so I thought there might be 
a reason to do it this way?


***********************************************************************************
Tomaz Mertelj
E-mail: tomaz.mertelj@xxxxxxxxxxxxxx    Home page: 
http://optlab.ijs.si/tmertelj


Staniceva 14
1000 Ljubljana
Slovenia
***********************************************************************************



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/