Re: [PATCH v2 2/2] iio: adc: add support for pac1921

[<Marius.Cristea@microchip.com>]

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@gmail.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

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
>