Jonathan Cameron wrote: > On Tue, 09 Jul 2024 10:21:07 +0200 > Matteo Martelli <matteomartelli3@xxxxxxxxx> wrote: > > > Jonathan Cameron wrote: > > ... > > > > I could add the shunt-resistor controls to allow calibration as Marius > > > > suggested, but that's also a custom ABI, what are your thoughts on this? > > > > > > This would actually be a generalization of existing device specific ABI > > > that has been through review in the past. > > > See Documentation/ABI/testing/sysfs-bus-iio-adc-pac1934 > > > for example (similar in other places). > > > So if you want to do this move that ABI up a level to cover multiple devices > > > (removing the entries in specific files as you do so). > > > > > I would do this in a separate commit, would you prefer it in this same patch > > set or in another separate patch? > > Separate commit in this series as otherwise it's not obvious why we are > doing it. In theory should be before this patch as then what you use here > is already documented, but I don't care that much on the order. > Just a few more questions about this point. * I see 3 other drivers exposing the shunt resistor attribute: ina2xx, max9611 and pac1934. While the unit for first two is in Ohms, for the latter it's in micro-Ohms. What should be the unit for the generalized ABI? I would guess Ohms as /sys/bus/iio/devices/iio:deviceX/in_resistance_raw. * If for instance the generalized ABI unit is going to be Ohms, should I still remove the entry from the pac1934 even though it would not be fully compliant with the generalized ABI? * To cover the current exposed attributes, the "What" fields would look like: from max9611: What: /sys/.../iio:deviceX/in_current_shunt_resistor What: /sys/.../iio:deviceX/in_power_shunt_resistor from ina2xx: What: /sys/.../iio:deviceX/in_shunt_resistor from pac1934: What: /sys/.../iio:deviceX/in_shunt_resistorY Does this look correct? I think that for the first two drivers the shunt_resistor can be considered as a channel info property, shared by type for max9611 case and shared by direction for ina2xx case (maybe better to remove "in_" from the What field if the type is not specified?). What seems odd to me is the pac1934 case, since it doesn't fit in the format <type>[Y_]shunt_resistor referred in many other attributes (where I assume <type> is actually [dir_][type_]?). Doesn't it look like pac1934 is exposing additional input channels, that are also writeable? Maybe such case would more clear if the shunt resistor would be an info property of specific channels? For example: in_currentY_shunt_resistor, in_powerY_shunt_resistor and in_engergyY_shunt_resitor. * I would go for a simple and generic description such as: "The value of current sense resistor in Ohms." like it is in Documentation/devicetree/bindings/hwmon/hwmon-common.yaml. Should it include any additional detail? * I am assuming the generalized API would have Date and KernelVersion of today even though the original attributes are older. * Should this ABI be inserted at any particular place of Documentation/ABI/testing/sysfs-bus-iio or just appended at its end? Thanks, Matteo
