Message ID | 20200925083743.46469-1-alexandru.ardelean@analog.com (mailing list archive) |
---|---|
Headers | show |
Series | iio: remove iio_buffer_set_attrs() and assign buffer attrs during alloc | expand |
On Fri, 25 Sep 2020 11:37:38 +0300 Alexandru Ardelean <alexandru.ardelean@analog.com> 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. 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(-) >
On Sat, Sep 26, 2020 at 7:20 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Fri, 25 Sep 2020 11:37:38 +0300 > Alexandru Ardelean <alexandru.ardelean@analog.com> 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(-) > > >