Hi Juerg,
On Mon, 12 May 2008 21:41:05 -0700, Juerg Haefliger wrote:
> Add support for the SCH5027. The differences to the DME1737 are:
> - No support for programmable temp offsets
> - In auto mode, PWM outputs stay on min value if temp goes below low threshold
> and can't be programmed to fully turn off
> - Different voltage scaling
> - No VID input
>
> Signed-off-by: Juerg Haefliger <juergh at gmail.com>
>
> ---
> Incorporated suggestions from Jean Delvare.
Almost OK, I only have 2 remaining comments, the first of which I think
really needs to be addressed:
> - kind = dme1737;
> - name = "dme1737";
> + if (kind == sch5027) {
> + name = "sch5027";
> + } else {
> + name = "dme1737";
> + }
> data->type = kind;
At this point data->type could be 0 (if the user passed a force
parameter). It _happens_ that right now your driver makes no explicit
test on data->type == dme1737 (nor != dme1737) and all the tests happen
to default to the dme1737 case so 0 and dme1737 will be treated the
same (as far as I can see at least), so it works, however this is
pretty fragile I think. A future code change could break this case and
you probably wouldn't notice. I'd rather see you set kind = dme1737 at
the same time you're setting name = "dme1737", to make sure that
data->type always has a correct value and this value in sync with the
device name.
> + data->in_nominal = IN_NOMINAL(data->type);
This is common to the I2C and ISA cases so it could be moved to
dme1737_init_device() to avoid code redundancy.
Thanks,
--
Jean Delvare
