Re: [RFC PATCH 0/5] iio: remove iio_buffer_set_attrs() and assign buffer attrs during alloc

[Alexandru Ardelean]

On Sat, Sep 26, 2020 at 7:20 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Fri, 25 Sep 2020 11:37:38 +0300
> Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> wrote:
>
> > I've been mulling this over for a while, and I am still not 100% convinced
> > that this is the best approach, but this feels closer to something
> > correct.
>
> I'd not worry too much.  It might not be perfect but it's also not hideous
> - though that won't stop me suggesting an alternative :)
>
> >
> > A few things about this patchset:
> > 1. it hasn't been tested; it just compiles
> > 2. there are some patches that have went out [from my side] in the last
> >    few days that deal with trying to organize iio_buffer_set_attrs()
> >    in order to get to this point [as-in: this patch-set]
> > 2a. the only reason patch [1] is in this set, is so that [5] applies
> > 2b. the main reason patch [2] is in this set, is so that [5] compiles
> > 3. This patch is only meant as RFC, as I'd like that some cleanup
> >    patches go in before this set; this RFC is meant to provide an idea
> >    of where I want to get at.
> >
> > This tries to address the issue with iio_buffer_set_attrs(), where a
> > driver needs to provide a reference to a buffer to assign extra sysfs
> > attributes.
> > Something like 'iio_buffer_set_attrs(indio_dev->buffer, <buffer_attrs>)'
> >
> > This works well for the 1 IIO buffer == 1 IIO device case.
> > The driver access the 'buffer' reference attached to the IIO device.
> >
> > But for a multiple IIO buffers per 1 IIO device, this is cumbersome, as
> > a driver would need to:
> > 1. allocate a set of buffers
> > 2. dig out of IIO the buffers to which it wants to assign extra sysfs
> >    attributes
> >
> > The main change here is to move the attributes [usually HW FIFO
> > attributes] to become arguments of the devm_}iio_triggered_buffer_setup()
> > functions.
> > Other buffer allocation functions would need to do the same, but right
> > now, it's only these functions that assign extra buffer attributes.
>
> Probably worth stating here that we have a confusing mess of where
> these attributes are and will probably look long term to move them
> or at least think about doing so.
>
> The buffer directory in modern IIO isn't strictly matched to a hwfifo
> any more (it used to be - see sca3000 for the history - the model
> was not a nice solution, but it took a few years to figure that out).
> buffer/* represent a software buffer fed by the hardware fifo. There
> are potentially other software buffers being fed in the form of
> other consumers.
>
> The hardware fifo is a separate entity.  However as Alex adds
> support for multiple streams of data (that may come from mulitple hardware
> fifos) we are going to still need a mapping of hardware buffer X
> to software buffer X.  So separate entities, but with a one to one
> (technically many of the consumer side, but only one in the IIO
> userspace interface) mapping.
>
> Anyhow, Alex and others know this from previous discussions but perhaps others
> reading this don't hence I thought I'd mention it :)
>
> >
> > Some alternative ideas to doing this change [as-is]:
> > * move the attributes to 'struct iio_buffer_setup_ops' ; this has a
> >   minimal patch/change-impact, but it doesn't feel correct
> > * introduce a new type called 'struct iio_trigerred_buffer_args' that
> >   wrap all the triggered-buffer arguments into a single object; this is
> >   still a big patch [like this], but future-wise, we would just be
> >   extending that object [if more args are needed]; the main point
> >   here is that right now 'devm_iio_triggered_buffer_setup()' has 6
> >   arguments, which feels a bit much
>
> So here is another one:
> * Let us assume that we actually don't want to use this interface in new
>   drivers because we decide to deprecate putting the hardware fifo stuff
>   where it currently is. (big assumption - dependent on what userspace
>   code is using this and it will take a while).
> * With that assumption, we could add a new function that takes the attrs
>   e.g. iio_setup_triggered_buffer_ext()
>   and only replace the instances where we need to provide the attrs.
>

Fine by me as well, to do a iio_setup_triggered_buffer_ext() that
holds the attrs as an extra arg.
I guess the thought [of doing it like that] didn't cross my mind, but
maybe also because I keep forgetting about the full context of the
buffer attributes.

That would convert "iio_setup_triggered_buffer()" into a wrapper for
iio_setup_triggered_buffer_ext() with NULL in place.
And is indeed a smaller patchset.

I'll send a V2 for this.
Maybe I'll wait until the at91-sama5d2_adc patch [1] is applied, or V2
will include a copy of that patch.
The cross_ec one was applied, so one less hurdle for this.
These cleanup patches [in various drivers] do help with making some of
the IIO core changes easier to grasp [for me particularly].

On a side-note, I was super happy when "scan_el_attrs" was dropped [2].
It's one less worry for this whole IIO buffer work.

[1] https://lore.kernel.org/linux-iio/20200926165549.23dc420b@archlinux/T/#t
[2] https://lore.kernel.org/linux-iio/20200412110253.52d93e71@archlinux/

> To be honest, as I said above, I'm not that bothered which route is
> taken.
>
> Jonathan
>
> >
> >
> > Alexandru Ardelean (5):
> >   [1] iio: cros_ec: unify hw fifo attributes into the core file
> >   [2] iio: buffer: dmaengine: unwrap the use of iio_buffer_set_attrs()
> >   [3] iio: triggered-buffer: add sysfs buffer attributes as args for setup
> >   [4] iio: buffer: extend arg list for {devm_}iio_triggered_buffer_setup()
> >   [5] iio: remove iio_buffer_set_attrs() and assign buffer attrs during
> >     alloc
> >
> >  drivers/iio/accel/adxl372.c                   |  5 ++--
> >  drivers/iio/accel/bma180.c                    |  2 +-
> >  drivers/iio/accel/bma220_spi.c                |  2 +-
> >  drivers/iio/accel/bmc150-accel-core.c         | 19 ++++++++------
> >  drivers/iio/accel/cros_ec_accel_legacy.c      |  2 +-
> >  drivers/iio/accel/kxcjk-1013.c                |  2 +-
> >  drivers/iio/accel/kxsd9.c                     |  2 +-
> >  drivers/iio/accel/mma7455_core.c              |  2 +-
> >  drivers/iio/accel/mma8452.c                   |  2 +-
> >  drivers/iio/accel/mxc4005.c                   |  2 +-
> >  drivers/iio/accel/st_accel_buffer.c           |  2 +-
> >  drivers/iio/accel/stk8312.c                   |  2 +-
> >  drivers/iio/accel/stk8ba50.c                  |  2 +-
> >  drivers/iio/adc/ad7266.c                      |  3 ++-
> >  drivers/iio/adc/ad7298.c                      |  2 +-
> >  drivers/iio/adc/ad7476.c                      |  2 +-
> >  drivers/iio/adc/ad7606.c                      |  2 +-
> >  drivers/iio/adc/ad7766.c                      |  2 +-
> >  drivers/iio/adc/ad7768-1.c                    |  2 +-
> >  drivers/iio/adc/ad7887.c                      |  2 +-
> >  drivers/iio/adc/ad7923.c                      |  2 +-
> >  drivers/iio/adc/ad799x.c                      |  2 +-
> >  drivers/iio/adc/ad_sigma_delta.c              |  2 +-
> >  drivers/iio/adc/at91-sama5d2_adc.c            | 25 +++++++++++--------
> >  drivers/iio/adc/at91_adc.c                    |  2 +-
> >  drivers/iio/adc/cc10001_adc.c                 |  2 +-
> >  drivers/iio/adc/dln2-adc.c                    |  3 ++-
> >  drivers/iio/adc/hx711.c                       |  3 ++-
> >  drivers/iio/adc/max1027.c                     |  2 +-
> >  drivers/iio/adc/max1118.c                     |  2 +-
> >  drivers/iio/adc/max1363.c                     |  2 +-
> >  drivers/iio/adc/mxs-lradc-adc.c               |  2 +-
> >  drivers/iio/adc/rockchip_saradc.c             |  2 +-
> >  drivers/iio/adc/stm32-adc.c                   |  2 +-
> >  drivers/iio/adc/stm32-dfsdm-adc.c             |  2 +-
> >  drivers/iio/adc/ti-adc081c.c                  |  3 ++-
> >  drivers/iio/adc/ti-adc0832.c                  |  2 +-
> >  drivers/iio/adc/ti-adc084s021.c               |  3 ++-
> >  drivers/iio/adc/ti-adc108s102.c               |  3 ++-
> >  drivers/iio/adc/ti-adc12138.c                 |  2 +-
> >  drivers/iio/adc/ti-adc161s626.c               |  2 +-
> >  drivers/iio/adc/ti-ads1015.c                  |  2 +-
> >  drivers/iio/adc/ti-ads124s08.c                |  3 ++-
> >  drivers/iio/adc/ti-ads7950.c                  |  3 ++-
> >  drivers/iio/adc/ti-ads8688.c                  |  3 ++-
> >  drivers/iio/adc/ti-tlc4541.c                  |  2 +-
> >  drivers/iio/adc/vf610_adc.c                   |  3 ++-
> >  drivers/iio/adc/xilinx-xadc-core.c            |  2 +-
> >  .../buffer/industrialio-buffer-dmaengine.c    |  3 +--
> >  .../buffer/industrialio-triggered-buffer.c    | 13 +++++++---
> >  drivers/iio/chemical/atlas-sensor.c           |  2 +-
> >  drivers/iio/chemical/ccs811.c                 |  2 +-
> >  drivers/iio/chemical/pms7003.c                |  3 ++-
> >  drivers/iio/chemical/scd30_core.c             |  3 ++-
> >  drivers/iio/chemical/sps30.c                  |  3 ++-
> >  .../cros_ec_sensors/cros_ec_lid_angle.c       |  5 ++--
> >  .../common/cros_ec_sensors/cros_ec_sensors.c  |  5 ++--
> >  .../cros_ec_sensors/cros_ec_sensors_core.c    | 16 +++++++++---
> >  .../common/hid-sensors/hid-sensor-trigger.c   | 19 ++++++--------
> >  drivers/iio/gyro/adxrs290.c                   |  3 ++-
> >  drivers/iio/gyro/bmg160_core.c                |  2 +-
> >  drivers/iio/gyro/fxas21002c_core.c            |  3 ++-
> >  drivers/iio/gyro/itg3200_buffer.c             |  2 +-
> >  drivers/iio/gyro/mpu3050-core.c               |  2 +-
> >  drivers/iio/gyro/st_gyro_buffer.c             |  2 +-
> >  drivers/iio/health/afe4403.c                  |  2 +-
> >  drivers/iio/health/afe4404.c                  |  2 +-
> >  drivers/iio/humidity/am2315.c                 |  2 +-
> >  drivers/iio/humidity/hdc100x.c                |  2 +-
> >  drivers/iio/humidity/hts221_buffer.c          |  2 +-
> >  drivers/iio/imu/adis_buffer.c                 |  2 +-
> >  drivers/iio/imu/bmi160/bmi160_core.c          |  3 ++-
> >  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    |  2 +-
> >  drivers/iio/imu/kmx61.c                       |  4 +--
> >  drivers/iio/industrialio-buffer.c             | 12 ---------
> >  drivers/iio/light/adjd_s311.c                 |  2 +-
> >  drivers/iio/light/as73211.c                   |  3 ++-
> >  drivers/iio/light/cros_ec_light_prox.c        |  5 ++--
> >  drivers/iio/light/gp2ap020a00f.c              |  3 ++-
> >  drivers/iio/light/isl29125.c                  |  2 +-
> >  drivers/iio/light/ltr501.c                    |  2 +-
> >  drivers/iio/light/max44000.c                  |  3 ++-
> >  drivers/iio/light/rpr0521.c                   |  2 +-
> >  drivers/iio/light/si1145.c                    |  2 +-
> >  drivers/iio/light/st_uvis25_core.c            |  2 +-
> >  drivers/iio/light/tcs3414.c                   |  2 +-
> >  drivers/iio/light/tcs3472.c                   |  2 +-
> >  drivers/iio/light/vcnl4000.c                  |  3 ++-
> >  drivers/iio/light/vcnl4035.c                  |  2 +-
> >  drivers/iio/magnetometer/ak8974.c             |  2 +-
> >  drivers/iio/magnetometer/ak8975.c             |  2 +-
> >  drivers/iio/magnetometer/bmc150_magn.c        |  2 +-
> >  drivers/iio/magnetometer/hmc5843_core.c       |  2 +-
> >  drivers/iio/magnetometer/mag3110.c            |  2 +-
> >  drivers/iio/magnetometer/rm3100-core.c        |  2 +-
> >  drivers/iio/magnetometer/st_magn_buffer.c     |  2 +-
> >  drivers/iio/potentiostat/lmp91000.c           |  2 +-
> >  drivers/iio/pressure/cros_ec_baro.c           |  5 ++--
> >  drivers/iio/pressure/dlhl60d.c                |  2 +-
> >  drivers/iio/pressure/mpl3115.c                |  2 +-
> >  drivers/iio/pressure/ms5611_core.c            |  2 +-
> >  drivers/iio/pressure/st_pressure_buffer.c     |  2 +-
> >  drivers/iio/pressure/zpa2326.c                |  2 +-
> >  drivers/iio/proximity/as3935.c                |  3 ++-
> >  drivers/iio/proximity/isl29501.c              |  2 +-
> >  drivers/iio/proximity/mb1232.c                |  3 ++-
> >  .../iio/proximity/pulsedlight-lidar-lite-v2.c |  2 +-
> >  drivers/iio/proximity/srf08.c                 |  3 ++-
> >  drivers/iio/proximity/sx9310.c                |  2 +-
> >  drivers/iio/proximity/sx9500.c                |  2 +-
> >  drivers/iio/temperature/maxim_thermocouple.c  |  3 ++-
> >  include/linux/iio/buffer.h                    |  3 ---
> >  .../linux/iio/common/cros_ec_sensors_core.h   |  4 +--
> >  include/linux/iio/triggered_buffer.h          |  7 ++++--
> >  114 files changed, 198 insertions(+), 172 deletions(-)
> >
>