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

[Guenter Roeck <linux@roeck-us.net>]

On 11/25/2014 09:50 AM, Bartosz Golaszewski wrote:
> 2014-11-25 17:59 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
>> 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():
>

It seems to me this is a problem of your architecture. That doesn't
make it a generic problem. Your architecture should detect that a
probe was disconnected and de-instantiate the device automatically
if so, and re-instantiate it if it is re-inserted. Ultimately this
should be done with, for example, devicetree overlays.

Even worse, the remove/reinsertion sequence results in a non-initialized
chip.

This makes me wonder: Is the shunt resistor value set by software,
or by replacing one probe with another ?

Guenter

> 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
>