Message-ID: <20240716180004.606006d0@jic23-huawei>
Date: Tue, 16 Jul 2024 18:00:04 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: <Marius.Cristea@...rochip.com>
Cc: <matteomartelli3@...il.com>, <linux-iio@...r.kernel.org>,
 <devicetree@...r.kernel.org>, <robh@...nel.org>, <lars@...afoo.de>,
 <linux-kernel@...r.kernel.org>, <krzk+dt@...nel.org>,
 <Jonathan.Cameron@...wei.com>, <conor+dt@...nel.org>
Subject: Re: [PATCH v2 2/2] iio: adc: add support for pac1921

On Tue, 16 Jul 2024 11:19:53 +0000
<Marius.Cristea@...rochip.com> wrote:

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

The milli thing is historical curiosity (we were trying to maintain
interface compatibility with hwmon briefly many years ago - it rapidly
proved impractical). For almost all remotely recent units we have
gone with the SI base unit.

This is an corner of ABI so if things were consistent I'd say copy
the others, but failing that ohms for the reason you give.
Copy the in_resistance_raw value


> 
> 
> > 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

This one is a bit odd in that it describes it if it were a measurable
channel in of itself but we probably couldn't figure out a better way
to scope it to a specific channel.

> > 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?).

Keep it consistent.  It's valid to provide the in_ and in general
over restrict channel attributes, even if not strictly necessary.

> > 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.
Yup. You've 
> 
> For example "sampling_frequency" is only one property per device and
> not one property per channel.

Not necessarily.  If it varies per channel it is
in_voltageX_sampling_frequency etc
That is rare, but we have specific text to cover it in the ABI docs.

What:		/sys/bus/iio/devices/iio:deviceX/in_voltageX_sampling_frequency
What:		/sys/bus/iio/devices/iio:deviceX/in_powerY_sampling_frequency
What:		/sys/bus/iio/devices/iio:deviceX/in_currentZ_sampling_frequency
KernelVersion:	5.20
Contact:	linux-iio@...r.kernel.org
Description:
		Some devices have separate controls of sampling frequency for
		individual channels. If multiple channels are enabled in a scan,
		then the sampling_frequency of the scan may be computed from the
		per channel sampling frequencies.

> 
> 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.
Removing the dir is unnecessary.  Just leave that in place.
Note we can't change existing ABI of drivers for this sort of thing
that wasn't standardized (as we can't argue they break ABI) so
they are stuck as they stand.

Unfortunately the most consistent path is probably to treat it as a
normal attribute, even if that generates a bunch of silly duplication
if there is more than one shunt_resistance.
I agree it's ugly but it's not the only case of this sort of duplication.
It happens for that sampling_frequency case in a few corners were there is
on channel that is sampled different from all the others.

So I think
in_powerY_shut_resistor and in_energyY_shunt_resistor is
most consistent with existing 'standard' ABI.

This is one where I didn't do a great job in review unfortunately
so the one with the index on the end got through.

I'm not hugely worried about this mess though as runtime shunt resistor
calibration is not that common.  If people want good measurements they
tend to build their circuit with good components / PCB tracks etc.

> 
> 
> 
> > * 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?
That sounds good.  It is permissible when generalizing ABI like this
to include any particular quirks of the ones you are generalizing.
I doubt there are any here though.

> > 
> > * 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?
End is fine for this one.  We've never attempted to assign an order to that
file so grouping only occurs if there are related attributes and I don't
see this as closely related to much else.

Thanks,

Jonathan

> > 
> > Thanks,
> > Matteo
> >   
> 

Powered by blists - more mailing lists