Hartmut Knaack schrieb:
> Hi,
> first off, a short description would have been nice, would make it easier to
> understand the problem and your solution.
> So, from what I could see, this driver is already in here for 1 year and 4
> months, though I could not yet find it upstream. How comes? Last time I
> submitted a driver to hwmon, it made it into mainline within less than 4
> months. And the hwmon-maintainer, Guenter Roeck, is very experienced and
> could have pointed you to some of the problems in that driver. And keep in
> mind, that the patches in openwrt should just be a temporary solution (also
> to backport drivers to older kernel versions), but should go upstream ASAP.
> Btw, a link to the datasheet (which is mandatory for hwmon) would also be of
> big help for reviewing.
> Some comments inline.
>
> Tim Harvey schrieb:
>> Signed-off-by: Tim Harvey <thar...@gateworks.com>
>> ---
>> .../generic/patches-3.10/880-gateworks_system_controller.patch | 7
>> +++++--
>> .../generic/patches-3.12/880-gateworks_system_controller.patch | 7
>> +++++--
>> .../generic/patches-3.13/880-gateworks_system_controller.patch | 7
>> +++++--
>> .../generic/patches-3.3/880-gateworks_system_controller.patch | 7
>> +++++--
>> .../generic/patches-3.6/880-gateworks_system_controller.patch | 7
>> +++++--
>> .../generic/patches-3.8/880-gateworks_system_controller.patch | 7
>> +++++--
>> .../generic/patches-3.9/880-gateworks_system_controller.patch | 7
>> +++++--
>> 7 files changed, 35 insertions(+), 14 deletions(-)
>>
>> diff --git
>> a/target/linux/generic/patches-3.10/880-gateworks_system_controller.patch
>> b/target/linux/generic/patches-3.10/880-gateworks_system_controller.patch
>> index d923485..450551a 100644
>> --- a/target/linux/generic/patches-3.10/880-gateworks_system_controller.patch
>> +++ b/target/linux/generic/patches-3.10/880-gateworks_system_controller.patch
>> @@ -28,7 +28,7 @@
>>
>> --- /dev/null
>> +++ b/drivers/hwmon/gsc.c
>> -@@ -0,0 +1,308 @@
>> +@@ -0,0 +1,311 @@
>> +/*
>> + * A hwmon driver for the Gateworks System Controller
>> + * Copyright (C) 2009 Gateworks Corporation
>> @@ -132,11 +132,14 @@
>> + */
>> +static inline int gsp_read(struct i2c_client *client, u8 reg)
>> +{
>> -+ unsigned int adc = 0;
>> ++ int adc = 0;
> adc should be of type s16, and for the config you would need an int ret
>> + if (reg == GSP_REG_TEMP_IN || reg > GSP_REG_CURRENT)
>> + {
>> + adc |= i2c_smbus_read_byte_data(client, reg);
>> + adc |= i2c_smbus_read_byte_data(client, reg + 1) << 8;
> adc = i2c_smbus_read_word_data(client, reg);
Sorry, my bad. This doesn't allow error-handling. Better the following:
ret = i2c_smbus_read_word_data(client, reg);
if (ret < 0)
do some error handling, because read failed;
adc = ret;
But basically, there is a major design flaw in this function: it is not
designed to handle errors. So, you should start to rework show_adc() and then
scp_read(). The same applies to store_fan() and gsp_write(). I also strongly
recommend to have a look at recent hwmon drivers - you don't need to reinvent
what's already present.
>> ++ if (adc > 0x8000) { /* neg temps from two's-complement */
>> ++ adc = adc - 0xffff;
>> ++ }
> not needed, you just may have to check if adc gets casted to int during
> return.
>> + return adc;
>> + }
>> + else
> Some error-check after calling a function is also the regular behavior when
> coding properly. Then, from what I remember, hwmon also moved to the devm-API
> about a year ago.
> Take care
>
> Hartmut
>
_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
