Message-ID: <66963b764ac3c_706370bd@njaxe.notmuch>
Date: Tue, 16 Jul 2024 11:20:54 +0200
From: Matteo Martelli <matteomartelli3@...il.com>
To: Jonathan Cameron <jic23@...nel.org>, 
 Matteo Martelli <matteomartelli3@...il.com>
Cc: Jonathan Cameron <Jonathan.Cameron@...wei.com>, 
 Lars-Peter Clausen <lars@...afoo.de>, 
 Rob Herring <robh@...nel.org>, 
 Krzysztof Kozlowski <krzk+dt@...nel.org>, 
 Conor Dooley <conor+dt@...nel.org>, 
 Marius Cristea <marius.cristea@...rochip.com>, 
 linux-iio@...r.kernel.org, 
 devicetree@...r.kernel.org, 
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] iio: adc: add support for pac1921

Jonathan Cameron wrote:
> On Tue, 09 Jul 2024 10:21:07 +0200
> Matteo Martelli <matteomartelli3@...il.com> 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

Powered by blists - more mailing lists