Hi Matteo, On Tue, 2024-07-16 at 11:20 +0200, Matteo Martelli wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > 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 For measuring current the usual "scale" is part of miliOhms in order to reduce the power dissipation. As a rule of thumb 0.1 miliOhms is a usual value for shunt resistors. I think the "correct" way is to setup the value in sub units of Ohms. Like the current is in miliAmps and the voltage is in miliVolts. > 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 don't think it will be a good idea to duplicate the same information into multiple attributes like: in_currentY_shunt_resistor, in_powerY_shunt_resistor and in_engergyY_shunt_resitor. The pac1934 device could be viewed like 4 devices that have only one measurement hardware. Changing the shunt for a hardware channel will impact multiple software measurements for that particular channel. For example "sampling_frequency" is only one property per device and not one property per channel. Also I'm not felling comfortable to remove the [dir_] from the name, because this value is dependent of the hardware and we can't have a "available" properties for it. > * 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 >
