2014-11-25 17:59 GMT+01:00 Guenter Roeck <li...@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(): 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
