Message-ID: <6697d3b0d33f6_1fc333707f@njaxe.notmuch>
Date: Wed, 17 Jul 2024 16:22:40 +0200
From: Matteo Martelli <matteomartelli3@...il.com>
To: Jonathan Cameron <jic23@...nel.org>, 
 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

Jonathan Cameron wrote:
> > > 
> > > * 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 

Sorry Jonathan, is there anything missing in this sentence? Looks like
unintentionally truncated: 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.
> 

>From your comments I get that in_shunt_resistorY should be added in the
generalized ABI (as in the example above) since it is already used and can't be
changed. Is this correct?

I am still not sure whether in_currentY_shunt_resistor,
in_powerY_shunt_resistor and in_energyY_shunt_resistor, should be added or not
until a new driver using it comes through.

Regarding pac1921, would it be more clear to expose a single in_shunt_resistor
(keeping [dir_] for consistency as you suggested) as it is for ina2xx or
in_current_shunt_resistor plus in_power_shunt_resistor as it is for max9611? I
agree that just exposing it once would be more clear for the user, so I would
go for the first case but maybe I am missing something.

> 
> Thanks,
> 
> Jonathan
> 

Thanks,
Matteo

Powered by blists - more mailing lists