On Wed, 17 Jul 2024 16:22:40 +0200 Matteo Martelli <matteomartelli3@xxxxxxxxx> 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@xxxxxxxxxxxxxxx > > 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
