On 11/25/2014 09:50 AM, Bartosz Golaszewski wrote:
2014-11-25 17:59 GMT+01:00 Guenter Roeck <[email protected]>:
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
