Re: [PATCH 1/5] hwmon: ina2xx: bail-out from ina2xx_probe() in case of configuration errors

[Bartosz Golaszewski]

2014-11-25 17:59 GMT+01:00 Guenter Roeck <linux@xxxxxxxxxxxx>:
> The new functions _might_ make a bit more sense if you would
> implement the necessary calculations in the functions, but you are
> not doing that. Instead, in the next patch, you introduce yet
> another function to do the calibration calculation, just to add it
> as parameter to the actual calibration function whenever you call it.

This wasn't my intention, but I'll keep that in mind for the next version.

> - I don't accept unnecessary ( ).
> - I don't accept unnecessary typecasts.
> - If you don't accept negative values, use kstrtoul().
> - kstrtol can at least in theory return other errors besides -EINVAL.

I'll fix those in the next version.

> - Locking should be done in the calling code. It is not needed during
>   probe and more appropriate in set functions.

I don't use locks in probe - they're used directly in
ina2xx_set_value() or indirectly in ina226_update_avg(), but these
functions are never called from probe.

> - Any reason for selecting "rshunt" as sysfs attribute name instead
>   of, say, shunt-resistor or maybe shunt_resistor ?

I'll change it to shunt_resistor for more readability.

> - Returning -ENODEV from a set function doesn't make much sense.

It does make sense in case of ACME (http://baylibre.com/acme/) - a
probe can be disconnected at run-time, which, even without these
patches, would cause ina2xx_update_device() to error out when called
by ina2xx_show_value():

231                         int rv = i2c_smbus_read_word_swapped(client, i);
232                         if (rv < 0) {
233                                 ret = ERR_PTR(rv);
234                                 goto abort;

I just reproduced this behavior in ina2xx_set_value().

> - We don't overwrite error codes except in probe functions.

I'll fix that too.

Bart
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/