On Fri, Jul 12, 2024 at 02:41:51PM +0000, Marius.Cristea@xxxxxxxxxxxxx wrote: > Hi Matteo, > > > On Thu, 2024-07-11 at 09:08 +0200, Matteo Martelli wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > know the content is safe > > > > Hi Marius, > > > > Marius.Cristea@ wrote: > > > > I think that the OUT pin might not be used at all in many use > > > > cases > > > > so I would > > > > leave the OUT pin setting as fixed for now and maybe extend it in > > > > the > > > > future > > > > when more use cases arise. I am open to reconsider this though. > > > > > > > > > > The OUT functionality could be set from the device tree. > > > > I think this should to be controlled during runtime since it's a > > configuration > > that changes the device operation mode and so also what measurements > > are > > exposed to the user. An additional DT property could be useful but I > > am not > > sure it would fit in the DT scope. > > Anyway I will leave this for future extensions. > > > > I think there are 2 different things here. Setting the configuration at > startup by hard-coding things at probe time or taken those from device > tree (we can add multiple properties here, as long those properties are > documented into the dt-binding file) and the user controlled part at > runtime. > Because there is no standard interface to change the functionality, it > will be easy to startup from the device tree and let the user to do > some minor adjustments and not hardcode configuration. > > > > ... > > > > > > --- > > > > > > .../ABI/testing/sysfs-bus-iio-adc-pac1921 | 45 + > > > > > > MAINTAINERS | 7 + > > > > > > drivers/iio/adc/Kconfig | 10 + > > > > > > drivers/iio/adc/Makefile | 1 + > > > > > > drivers/iio/adc/pac1921.c | 1038 > > > > > > ++++++++++++++++++++ > > > > > > 5 files changed, 1101 insertions(+) > > > > > > > > > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc- > > > > > > pac1921 > > > > > > b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 > > > > > > new file mode 100644 > > > > > > index 000000000000..4a32e2d4207b > > > > > > --- /dev/null > > > > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 > > > > > Quite a bit of custom ABI in here. > > > > > > > > > > Rule of thumb is that custom ABI is more or less pointless ABI > > > > > for > > > > > 99% of users > > > > > because standard userspace won't use it. So keep that in mind > > > > > when > > > > > defining it. > > > > > > > > > > > @@ -0,0 +1,45 @@ > > > > > > +What: > > > > > > /sys/bus/iio/devices/iio:deviceX/resolution_bits > > > > > > +KernelVersion: 6.10 > > > > > > +Contact: linux-iio@xxxxxxxxxxxxxxx > > > > > > +Description: > > > > > > + ADC measurement resolution. Can be either 11 bits > > > > > > or > > > > > > 14 bits > > > > > > + (default). The driver sets the same resolution > > > > > > for > > > > > > both VBUS and > > > > > > + VSENSE measurements even if the hardware could be > > > > > > configured to > > > > > > + measure VBUS and VSENSE with different > > > > > > resolutions. > > > > > > + This attribute affects the integration time: with > > > > > > 14 > > > > > > bits > > > > > > + resolution the integration time is increased by a > > > > > > factor of > > > > > > + 1.9 (the driver considers a factor of 2). See > > > > > > Table > > > > > > 4-5 in > > > > > > + device datasheet for details. > > > > > > > > > > Is the integration time ever high enough that it matters? > > > > > People tend not to do power measurement 'quickly'. > > > > > > > > > > If we are doing it quickly then you'll probably want to be > > > > > providing buffered > > > > > support and that does allow you to 'read' the resolution for a > > > > > part > > > > > where > > > > > it changes for some other reason. I haven't yet understood > > > > > this > > > > > case. > > > > > > > > I will remove this control and fix the resolution bits to 14 > > > > (highest > > > > value), > > > > same as the HW default. > > > > > > The resolution could be set from the device tree. As default it > > > could > > > be 14 bits like into the hardware. The user could add > > > "microchip,low_resolution_voltage" into the device tree in order to > > > use > > > only 11 bits for voltage samples. > > > > I think this should be controlled during runtime since it does not > > depend on > > the HW design but more on the user preferences about measurements > > precision. > > As Jonathan pointed out, since custom ABIs should be avoided when > > possible, I > > will leave it out from now until it becomes necessary and fix the > > resolution to > > 14 bits, as the HW default. > > > > Set the configuration from the device tree, will avoid custom ABI. The > device tree could be changed also at runtime. Custom ABI in devicetree is not a replacement for custom ABI in userspace. If things are fixed by the hardware and non-discoverable, then sure add devicetree properties - but if it is things like "the user wants 11-bit mode", then that does not sound suitable for a devicetree property at all. And no, you can't just change the devicetree at runtime like that either as far as I understand - that's gonna cause memory leaks etc and I don't think can be done from userspace without out-of-tree patches anyway. Cheers, Conor. > > > ... > > > > > > +What: > > > > > > /sys/bus/iio/devices/iio:devices/filters_en > > > > > > +KernelVersion: 6.10 > > > > > > +Contact: linux-iio@xxxxxxxxxxxxxxx > > > > > > +Description: > > > > > > + Attribute to enable/disable ADC post filters. > > > > > > Enabled > > > > > > by > > > > > > + default. > > > > > > + This attribute affects the integration time: with > > > > > > filters > > > > > > + enabled the integration time is increased by 50%. > > > > > > See > > > > > > Table 4-5 > > > > > > + in device datasheet for details. > > > > > > > > > > Do we have any idea what this filter is? Datasheet seems very > > > > > vague > > > > > indeed and from > > > > > a control point of view that makes this largely useless. How > > > > > does > > > > > userspace know > > > > > whether to turn it on? > > > > > > > > > > We have an existing filter ABI but with so little information > > > > > no > > > > > way to fit this in. > > > > > Gut feeling, leave it on all the time and drop the control > > > > > interface. > > > > > > > > I will remove this control and leave it on all the time as the HW > > > > default. > > > > > > > > > > The filters could be enabled from the device tree. As default it > > > could > > > be disabled. > > > As a small detail here this is a post processing digital filter > > > that > > > could be enabled/disabled inside the PAC module. > > > > > > > Same reasoning of the resolution_bits parameter applies here. I will > > fix the > > filters to enabled, as the HW default. If there is any particular > > reason to > > prefer the filters fixed as disabled I will change that. > > > If the user can change the on/off for the filters it doesn't matter > what will be the default behavior. Being a single channel device, the > probability for the user to change the filter behavior during runtime > is minimal, that was the main reason for letting the user to change the > configuration from the device tree and not hardcode it. > > > ... > > > Thanks, > > > Marius > > > > Thanks, > > Matteo > > Thanks, > Marius
Attachment:
signature.asc
Description: PGP signature
