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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 17 Jul 2024 16:22:40 +0200
Matteo Martelli <matteomartelli3@gmail.com> wrote:

> Jonathan Cameron wrote:

Oddly I thought I'd replied to this already but my email client says not...
I guess maybe I have a stray draft on another computer. Anyhow, let's
try again!

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

Bad editing of my reply!. Ignore that.

> 
> > > 
> > > 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@vger.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?
No. for the one that isn't compliant with our generalization, just leave
it where it is in a per device doc.

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

Ah. I wasn't paying attention to what was needed here. If you don't need them
then no need to define them.

> 
> 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.
It's an interesting question.  Is it obvious enough that the shut resistor
affects both current and power measurements?

I think it is and in general shunt resistor tuning is fairly uncommon
thing so just in_current_shunt_resistor sounds fine to me.

Jonathan

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