Hi Herbert,
Thanks for your review.
On Sat, 24 Nov 2007 22:26:21 +0100, Herbert Valerio Riedel wrote:
> On Mon, 2007-10-22 at 17:46 +0200, Jean Delvare wrote:
> > This makes the code more readable and the binary smaller (by 5% or so).
>
> :-)
>
> > +static ssize_t show_fan_input(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + int nr = to_sensor_dev_attr(attr)->index;
> > + struct gl518_data *data = gl518_update_device(dev);
> > + return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan_in[nr],
> > + DIV_FROM_REG(data->fan_div[nr])));
> > +}
> > +
> > +static ssize_t show_fan_min(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + int nr = to_sensor_dev_attr(attr)->index;
> > + struct gl518_data *data = gl518_update_device(dev);
> > + return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan_min[nr],
> > + DIV_FROM_REG(data->fan_div[nr])));
> > +}
> > +
> > +static ssize_t show_fan_div(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + int nr = to_sensor_dev_attr(attr)->index;
> > + struct gl518_data *data = gl518_update_device(dev);
> > + return sprintf(buf, "%d\n", DIV_FROM_REG(data->fan_div[nr]));
> > +}
>
> imho, it would have been ok to reduce those 3 instantiations to a macro
> template and 3 expansions... but I guess it's a matter of preference :-)
Using a macro to define these functions would result in the same binary
code. The only thing it would save is a few lines in the code source,
at the price of a decreased readability (don't know about you, but my
text editor can't do syntax coloring on macro code). Macro-generated
functions are also grep-unfriendly, as they make the code look like the
functions are used but never defined, and in some cases, that some
macros are defined but never used.
So, as a general rule, I stay away from macro-generated functions.
> > - data->fan_min[0] = FAN_TO_REG(val,
> > - DIV_FROM_REG(data->fan_div[0]));
> > - regvalue = (regvalue & 0x00ff) | (data->fan_min[0] << 8);
> > + data->fan_min[nr] = FAN_TO_REG(val, DIV_FROM_REG(data->fan_div[nr]));
>
> > + regvalue = (regvalue & (0xff << (8 * nr)))
> > + | (data->fan_min[nr] << (8 * (1 - nr)));
>
> this shifting/masking looks a bit confusing to me; as 'nr' is supposed
> to take only 2 values, wouldn't it be more readable to use an if/else
> construct or an ?: operator?
>
> > + regvalue = (regvalue & ~(0xc0 >> (2 * nr)))
> > + | (data->fan_div[nr] << (6 - 2 * nr));
>
> same thing
I'll remember for my future patches. For this one, it was tested
successfully so I don't feel like changing the code again. I don't
have a GL518SM chip so I can't test myself.
Thanks,
--
Jean Delvare
