Hi Matteo, On Mon, 2024-07-08 at 15:39 +0200, Matteo Martelli wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > > > > 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. > > > > > --- > > > .../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. > > > > + > > > +What: > > > /sys/bus/iio/devices/iio:deviceX/resolution_bits_available > > > +KernelVersion: 6.10 > > > +Contact: linux-iio@xxxxxxxxxxxxxxx > > > +Description: > > > + List all possible ADC measurement resolutions: "11 > > > 14" > > > + > > > +What: > > > /sys/bus/iio/devices/iio:deviceX/integration_samples > > > +KernelVersion: 6.10 > > > +Contact: linux-iio@xxxxxxxxxxxxxxx > > > +Description: > > > + Number of samples taken during a full integration > > > period. Can be > > > + set to any power of 2 value from 1 (default) to 2048. > > > + This attribute affects the integration time: higher > > > the number > > > + of samples, longer the integration time. See Table 4- > > > 5 in device > > > + datasheet for details. > > > > Sounds like oversampling_ratio which is standards ABI. So use that > > or explain > > why you can't here. > > I am not sure that this is an oversampling ratio but correct me if I > am wrong: > generally by increasing the oversampling you would have additional > samples in a > fixed time period, while in this case by increasing the number of > samples you > would still have the same number of samples in a fixed time period, > but you > would have a longer integration period. So maybe the comment is not > very > clear since this parameter actually means "the number of samples > required to > complete the integration period". > > Initially I thought to let the user edit this by writing the > integration_time > control (which is currently read-only), but since the integration > period > depends also on the resolution and whether filters are enabled or > not, it would > have introduced some confusion: what parameter is being changed upon > integretion_time write? Maybe after removing resolution and filter > controls > there would be no such confusion anymore. > > > > + > > > +What: > > > /sys/bus/iio/devices/iio:deviceX/integration_samples_available > > > +KernelVersion: 6.10 > > > +Contact: linux-iio@xxxxxxxxxxxxxxx > > > +Description: > > > + List all possible numbers of integration samples: > > > + "1 2 4 8 16 32 64 128 256 512 1024 2048" > > > + > > > +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. > > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > > > index f60fe85a30d5..b56e494da970 100644 > > > --- a/drivers/iio/adc/Kconfig > > > +++ b/drivers/iio/adc/Kconfig > > > @@ -991,6 +991,16 @@ config NPCM_ADC > > > This driver can also be built as a module. If so, the > > > module > > > will be called npcm_adc. > > > > > > +config PAC1921 > > > + tristate "Microchip Technology PAC1921 driver" > > > + depends on I2C > > > > > > Thanks again, > Matteo Thanks, Marius
