On Tue, 8 Sep 2009 17:06:49 -0700, Andrew Morton wrote:
> On Sat, 5 Sep 2009 14:08:34 +0200
> tomaz.mertelj@xxxxxxxxxxxxxx wrote:
>
> > + int temp1_input;
> > + int temp1_min;
> > + int temp1_max;
> > + int temp1_crit;
> > +
> > + int temp2_input;
> > + int temp2_min;
> > + int temp2_max;
> > + int temp2_crit;
> > +
> > + u16 fan1_input;
> > + u16 fan1_min;
> > + u16 fan1_max;
> > + u8 fan1_div;
> > +
> > + u8 pwm1;
> > + u8 temp1_auto_point_temp[3];
> > + u8 temp2_auto_point_temp[3];
> > + u8 pwm1_auto_point_pwm[3];
> > + u8 pwm1_enable;
> > + u8 pwm1_auto_channels_temp;
> > +
> > + u8 stat1;
> > + u8 stat2;
> > +};
> > +
> > +
> > +#define get_temp_para(name) \
> > +static ssize_t get_##name(\
> > + struct device *dev,\
> > + struct device_attribute *devattr,\
> > + char *buf)\
> > +{\
> > + struct amc6821_data *data = amc6821_update_device(dev);\
> > + return sprintf(buf, "%d\n", data->name * 1000);\
> > +}
> > +
> > +get_temp_para(temp1_input);
> > +get_temp_para(temp1_min);
> > +get_temp_para(temp1_max);
> > +get_temp_para(temp2_input);
> > +get_temp_para(temp2_min);
> > +get_temp_para(temp2_max);
> > +get_temp_para(temp1_crit);
> > +get_temp_para(temp2_crit);
> > +
> > +#define set_temp_para(name, reg)\
> > +static ssize_t set_##name(\
> > + 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 val = simple_strtol(buf, NULL, 10); \
> > + \
> > + mutex_lock(&data->update_lock); \
> > + data->name = SENSORS_LIMIT(val / 1000, -128, 127); \
> > + if (i2c_smbus_write_byte_data(client, reg, data->name)) {\
> > + dev_err(&client->dev, "Register write error, aborting.\n");\
> > + count = -EIO;\
> > + } \
> > + mutex_unlock(&data->update_lock); \
> > + return count; \
> > +}
>
> 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.
>
> 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.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
