On 2011ë 06ì 09ì 00:10, Guenter Roeck wrote:
(snip)
>
> else statement is not needed here.
>
(snip)
>
> This implementation is quite expensive. Can you consider using binary search
> instead ?
>
I will use binary search at the next version.
(snip)
>
> You want to return ret here. Sure, it is the same, but that may change and you want
> to return the original error, not replace it.
>
(snip)
> Please use { } in the else statement as well (CodingStyle, chapter 3).
>
(snip)
>
> This overrides the original return value if set. The case should never happen,
> so you might want to use else above. Besides, either read_ohm or read_uV must be set,
> or ret won't even be initialized. A simple if/else (without checking if read_uV
> is actually set) should be sufficient here. And you can improve readability with
>
> int ret;
> unsigned int ohm;
>
> if (data->pdata->read_ohm)
> ohm = data->pdata->read_ohm();
> else
> ohm = get_ohm_of_thermistor(data, data->pdata->read_uV());
>
> ret = get_temp_mC(data, ohm, temp);
>
> One concern I have is that read_ohm() and read_uV() don't return errors. This may be true
> for the current back-end driver, but may not be true for future drivers. I would suggest
> to declare the function return codes as "int" and support (and check) error return values.
>
OK, I will fix it.
(snip)
>
> else is not needed here.
>
(snip)
>
> You want to return ret here, not the error code converted into a string.
>
(snip)
>
> Do _min and _max have any real value here ? What happens if a temperature is outside the
> reported range ? Seems all you do is to report the lowest/highest temperatures supported
> by the conversion functions, but there is no alarm associated with it nor does it really
> have a practical effect.
> In other words, all it does is to report the temperature range supported by the driver
> for a given chip, which isn't the idea. I would suggest to drop those attributes.
>
(snip)
>
> Odd comment, doesn't provide any value. Please remove.
>
(snip)
> You want to set this to NULL prior to releasing the memory.
>
I will follow all suggestions at the next patch.
Thank you.
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
