[PATCH 1/3] iio: kfifo: add iio_device_attach_kfifo_buffer() helper

[PATCH 1/3] iio: kfifo: add iio_device_attach_kfifo_buffer() helper

[Alexandru Ardelean]

This change adds the iio_device_attach_kfifo_buffer() helper/short-hand,
which groups the simple routine of allocating a kfifo buffers via
devm_iio_kfifo_allocate() and calling iio_device_attach_buffer().

The mode_flags parameter is required. The setup_ops parameter is optional.

This function will be a bit more useful when needing to define multiple
buffers per IIO device.

One requirement [that is more a recommendation] for this helper, is to call
it after 'indio_dev' has been populated.

Also, one consequence related to using this helper is that the resource
management of the buffer will be tied to 'indio_dev->dev'. Previously it
was open-coded, and each driver does it slightly differently. Most of them
tied it to the parent device, some of them to 'indio_dev->dev'.
This shouldn't be a problem, and may be a good idea when adding more
buffers per-device.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/buffer/kfifo_buf.c | 37 ++++++++++++++++++++++++++++++++++
 include/linux/iio/kfifo_buf.h  |  4 ++++
 2 files changed, 41 insertions(+)

diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
index 3150f8ab984b..05b7c5fc6f1d 100644
--- a/drivers/iio/buffer/kfifo_buf.c
+++ b/drivers/iio/buffer/kfifo_buf.c
@@ -228,4 +228,41 @@ void devm_iio_kfifo_free(struct device *dev, struct iio_buffer *r)
 }
 EXPORT_SYMBOL(devm_iio_kfifo_free);
 
+/**
+ * iio_device_attach_kfifo_buffer - Allocate a kfifo buffer & attach it to an IIO device
+ * @indio_dev: The device the buffer should be attached to
+ * @mode_flags: The mode flags for this buffer (INDIO_BUFFER_SOFTWARE and/or
+ *		INDIO_BUFFER_TRIGGERED).
+ * @setup_ops: The setup_ops required to configure the HW part of the buffer (optional)
+ *
+ * This function allocates a kfifo buffer via devm_iio_kfifo_allocate() and
+ * attaches it to the IIO device via iio_device_attach_buffer().
+ * This is meant to be a bit of a short-hand/helper function as many driver
+ * seem to do this.
+ */
+int iio_device_attach_kfifo_buffer(struct iio_dev *indio_dev,
+				   int mode_flags,
+				   const struct iio_buffer_setup_ops *setup_ops)
+{
+	struct iio_buffer *buffer;
+
+	if (mode_flags)
+		mode_flags &= kfifo_access_funcs.modes;
+
+	if (!mode_flags)
+		return -EINVAL;
+
+	buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
+	if (!buffer)
+		return -ENOMEM;
+
+	iio_device_attach_buffer(indio_dev, buffer);
+
+	indio_dev->modes |= mode_flags;
+	indio_dev->setup_ops = setup_ops;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(iio_device_attach_kfifo_buffer);
+
 MODULE_LICENSE("GPL");
diff --git a/include/linux/iio/kfifo_buf.h b/include/linux/iio/kfifo_buf.h
index 764659e01b68..2363a931be14 100644
--- a/include/linux/iio/kfifo_buf.h
+++ b/include/linux/iio/kfifo_buf.h
@@ -11,4 +11,8 @@ void iio_kfifo_free(struct iio_buffer *r);
 struct iio_buffer *devm_iio_kfifo_allocate(struct device *dev);
 void devm_iio_kfifo_free(struct device *dev, struct iio_buffer *r);
 
+int iio_device_attach_kfifo_buffer(struct iio_dev *indio_dev,
+				   int mode_flags,
+				   const struct iio_buffer_setup_ops *setup_ops);
+
 #endif
-- 
2.17.1

[PATCH 2/3] iio: make use of iio_device_attach_kfifo_buffer() where straightforward

[Alexandru Ardelean]

All drivers that already call devm_iio_kfifo_allocate() &
iio_device_attach_buffer() are simple to convert to
iio_device_attach_kfifo_buffer() in a single go/patch/.

This change does that.

For drivers max30100 & max30102 this helper is called after indio_dev has
been populated. This doesn't make any difference [at this point in time].

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/accel/sca3000.c                    | 18 ++----------------
 drivers/iio/accel/ssp_accel_sensor.c           | 13 ++++---------
 drivers/iio/adc/ina2xx-adc.c                   | 13 +++++--------
 drivers/iio/gyro/ssp_gyro_sensor.c             | 13 ++++---------
 drivers/iio/health/max30100.c                  | 15 ++++++---------
 drivers/iio/health/max30102.c                  | 15 ++++++---------
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 17 +++++++----------
 drivers/iio/light/acpi-als.c                   | 11 +++++------
 drivers/iio/light/apds9960.c                   | 15 ++++++---------
 9 files changed, 45 insertions(+), 85 deletions(-)

diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index 66d768d971e1..a0db76082ba6 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -1272,20 +1272,6 @@ static int sca3000_write_event_config(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static int sca3000_configure_ring(struct iio_dev *indio_dev)
-{
-	struct iio_buffer *buffer;
-
-	buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
-	if (!buffer)
-		return -ENOMEM;
-
-	iio_device_attach_buffer(indio_dev, buffer);
-	indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
-
-	return 0;
-}
-
 static inline
 int __sca3000_hw_ring_state_set(struct iio_dev *indio_dev, bool state)
 {
@@ -1480,7 +1466,8 @@ static int sca3000_probe(struct spi_device *spi)
 	}
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
-	ret = sca3000_configure_ring(indio_dev);
+	ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE,
+					     &sca3000_ring_setup_ops);
 	if (ret)
 		return ret;
 
@@ -1494,7 +1481,6 @@ static int sca3000_probe(struct spi_device *spi)
 		if (ret)
 			return ret;
 	}
-	indio_dev->setup_ops = &sca3000_ring_setup_ops;
 	ret = sca3000_clean_setup(st);
 	if (ret)
 		goto error_free_irq;
diff --git a/drivers/iio/accel/ssp_accel_sensor.c b/drivers/iio/accel/ssp_accel_sensor.c
index c32647abce20..1d9971db949d 100644
--- a/drivers/iio/accel/ssp_accel_sensor.c
+++ b/drivers/iio/accel/ssp_accel_sensor.c
@@ -96,7 +96,6 @@ static int ssp_accel_probe(struct platform_device *pdev)
 	int ret;
 	struct iio_dev *indio_dev;
 	struct ssp_sensor_data *spd;
-	struct iio_buffer *buffer;
 
 	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*spd));
 	if (!indio_dev)
@@ -111,18 +110,14 @@ static int ssp_accel_probe(struct platform_device *pdev)
 	indio_dev->dev.parent = &pdev->dev;
 	indio_dev->dev.of_node = pdev->dev.of_node;
 	indio_dev->info = &ssp_accel_iio_info;
-	indio_dev->modes = INDIO_BUFFER_SOFTWARE;
 	indio_dev->channels = ssp_acc_channels;
 	indio_dev->num_channels = ARRAY_SIZE(ssp_acc_channels);
 	indio_dev->available_scan_masks = ssp_accel_scan_mask;
 
-	buffer = devm_iio_kfifo_allocate(&pdev->dev);
-	if (!buffer)
-		return -ENOMEM;
-
-	iio_device_attach_buffer(indio_dev, buffer);
-
-	indio_dev->setup_ops = &ssp_accel_buffer_ops;
+	ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE,
+					     &ssp_accel_buffer_ops);
+	if (ret)
+		return ret;
 
 	platform_set_drvdata(pdev, indio_dev);
 
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index bdd7cba6f6b0..2070809e1acc 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -950,7 +950,6 @@ static int ina2xx_probe(struct i2c_client *client,
 {
 	struct ina2xx_chip_info *chip;
 	struct iio_dev *indio_dev;
-	struct iio_buffer *buffer;
 	unsigned int val;
 	enum ina2xx_ids type;
 	int ret;
@@ -1014,7 +1013,7 @@ static int ina2xx_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
+	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->dev.of_node = client->dev.of_node;
 	if (id->driver_data == ina226) {
@@ -1027,13 +1026,11 @@ static int ina2xx_probe(struct i2c_client *client,
 		indio_dev->info = &ina219_info;
 	}
 	indio_dev->name = id->name;
-	indio_dev->setup_ops = &ina2xx_setup_ops;
 
-	buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
-	if (!buffer)
-		return -ENOMEM;
-
-	iio_device_attach_buffer(indio_dev, buffer);
+	ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE,
+					     &ina2xx_setup_ops);
+	if (ret)
+		return ret;
 
 	return iio_device_register(indio_dev);
 }
diff --git a/drivers/iio/gyro/ssp_gyro_sensor.c b/drivers/iio/gyro/ssp_gyro_sensor.c
index 4e4ee4167544..c12d3db5a951 100644
--- a/drivers/iio/gyro/ssp_gyro_sensor.c
+++ b/drivers/iio/gyro/ssp_gyro_sensor.c
@@ -96,7 +96,6 @@ static int ssp_gyro_probe(struct platform_device *pdev)
 	int ret;
 	struct iio_dev *indio_dev;
 	struct ssp_sensor_data *spd;
-	struct iio_buffer *buffer;
 
 	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*spd));
 	if (!indio_dev)
@@ -110,18 +109,14 @@ static int ssp_gyro_probe(struct platform_device *pdev)
 	indio_dev->name = ssp_gyro_name;
 	indio_dev->dev.parent = &pdev->dev;
 	indio_dev->info = &ssp_gyro_iio_info;
-	indio_dev->modes = INDIO_BUFFER_SOFTWARE;
 	indio_dev->channels = ssp_gyro_channels;
 	indio_dev->num_channels = ARRAY_SIZE(ssp_gyro_channels);
 	indio_dev->available_scan_masks = ssp_gyro_scan_mask;
 
-	buffer = devm_iio_kfifo_allocate(&pdev->dev);
-	if (!buffer)
-		return -ENOMEM;
-
-	iio_device_attach_buffer(indio_dev, buffer);
-
-	indio_dev->setup_ops = &ssp_gyro_buffer_ops;
+	ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE,
+					     &ssp_gyro_buffer_ops);
+	if (ret)
+		return ret;
 
 	platform_set_drvdata(pdev, indio_dev);
 
diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
index 546fc37ad75d..f4e9866f4c3d 100644
--- a/drivers/iio/health/max30100.c
+++ b/drivers/iio/health/max30100.c
@@ -418,7 +418,6 @@ static int max30100_probe(struct i2c_client *client,
 			  const struct i2c_device_id *id)
 {
 	struct max30100_data *data;
-	struct iio_buffer *buffer;
 	struct iio_dev *indio_dev;
 	int ret;
 
@@ -426,21 +425,19 @@ static int max30100_probe(struct i2c_client *client,
 	if (!indio_dev)
 		return -ENOMEM;
 
-	buffer = devm_iio_kfifo_allocate(&client->dev);
-	if (!buffer)
-		return -ENOMEM;
-
-	iio_device_attach_buffer(indio_dev, buffer);
-
 	indio_dev->name = MAX30100_DRV_NAME;
 	indio_dev->channels = max30100_channels;
 	indio_dev->info = &max30100_info;
 	indio_dev->num_channels = ARRAY_SIZE(max30100_channels);
 	indio_dev->available_scan_masks = max30100_scan_masks;
-	indio_dev->modes = (INDIO_BUFFER_SOFTWARE | INDIO_DIRECT_MODE);
-	indio_dev->setup_ops = &max30100_buffer_setup_ops;
+	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->dev.parent = &client->dev;
 
+	ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE,
+					     &max30100_buffer_setup_ops);
+	if (ret)
+		return ret;
+
 	data = iio_priv(indio_dev);
 	data->indio_dev = indio_dev;
 	data->client = client;
diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
index 74fc260b957e..e15126d23dfb 100644
--- a/drivers/iio/health/max30102.c
+++ b/drivers/iio/health/max30102.c
@@ -507,7 +507,6 @@ static int max30102_probe(struct i2c_client *client,
 			  const struct i2c_device_id *id)
 {
 	struct max30102_data *data;
-	struct iio_buffer *buffer;
 	struct iio_dev *indio_dev;
 	int ret;
 	unsigned int reg;
@@ -516,16 +515,9 @@ static int max30102_probe(struct i2c_client *client,
 	if (!indio_dev)
 		return -ENOMEM;
 
-	buffer = devm_iio_kfifo_allocate(&client->dev);
-	if (!buffer)
-		return -ENOMEM;
-
-	iio_device_attach_buffer(indio_dev, buffer);
-
 	indio_dev->name = MAX30102_DRV_NAME;
 	indio_dev->info = &max30102_info;
-	indio_dev->modes = (INDIO_BUFFER_SOFTWARE | INDIO_DIRECT_MODE);
-	indio_dev->setup_ops = &max30102_buffer_setup_ops;
+	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->dev.parent = &client->dev;
 
 	data = iio_priv(indio_dev);
@@ -551,6 +543,11 @@ static int max30102_probe(struct i2c_client *client,
 		return -ENODEV;
 	}
 
+	ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE,
+					     &max30102_buffer_setup_ops);
+	if (ret)
+		return ret;
+
 	data->regmap = devm_regmap_init_i2c(client, &max30102_regmap_config);
 	if (IS_ERR(data->regmap)) {
 		dev_err(&client->dev, "regmap initialization failed\n");
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
index bb899345f2bb..4ba3d5551570 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
@@ -30,8 +30,8 @@
  * Denis Ciocca <denis.ciocca@st.com>
  */
 #include <linux/module.h>
-#include <linux/iio/kfifo_buf.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/kfifo_buf.h>
 #include <linux/iio/buffer.h>
 #include <linux/regmap.h>
 #include <linux/bitfield.h>
@@ -713,20 +713,17 @@ static const struct iio_buffer_setup_ops st_lsm6dsx_buffer_ops = {
 
 int st_lsm6dsx_fifo_setup(struct st_lsm6dsx_hw *hw)
 {
-	struct iio_buffer *buffer;
-	int i;
+	int i, ret;
 
 	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
 		if (!hw->iio_devs[i])
 			continue;
 
-		buffer = devm_iio_kfifo_allocate(hw->dev);
-		if (!buffer)
-			return -ENOMEM;
-
-		iio_device_attach_buffer(hw->iio_devs[i], buffer);
-		hw->iio_devs[i]->modes |= INDIO_BUFFER_SOFTWARE;
-		hw->iio_devs[i]->setup_ops = &st_lsm6dsx_buffer_ops;
+		ret = iio_device_attach_kfifo_buffer(hw->iio_devs[i],
+						     INDIO_BUFFER_SOFTWARE,
+						     &st_lsm6dsx_buffer_ops);
+		if (ret)
+			return ret;
 	}
 
 	return 0;
diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
index 1eafd0b24e18..ea99705c3387 100644
--- a/drivers/iio/light/acpi-als.c
+++ b/drivers/iio/light/acpi-als.c
@@ -166,6 +166,7 @@ static int acpi_als_add(struct acpi_device *device)
 	struct acpi_als *als;
 	struct iio_dev *indio_dev;
 	struct iio_buffer *buffer;
+	int ret;
 
 	indio_dev = devm_iio_device_alloc(&device->dev, sizeof(*als));
 	if (!indio_dev)
@@ -180,15 +181,13 @@ static int acpi_als_add(struct acpi_device *device)
 	indio_dev->name = ACPI_ALS_DEVICE_NAME;
 	indio_dev->dev.parent = &device->dev;
 	indio_dev->info = &acpi_als_info;
-	indio_dev->modes = INDIO_BUFFER_SOFTWARE;
 	indio_dev->channels = acpi_als_channels;
 	indio_dev->num_channels = ARRAY_SIZE(acpi_als_channels);
 
-	buffer = devm_iio_kfifo_allocate(&device->dev);
-	if (!buffer)
-		return -ENOMEM;
-
-	iio_device_attach_buffer(indio_dev, buffer);
+	ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE,
+					     NULL);
+	if (ret)
+		return ret;
 
 	return devm_iio_device_register(&device->dev, indio_dev);
 }
diff --git a/drivers/iio/light/apds9960.c b/drivers/iio/light/apds9960.c
index 52f86bc777dd..8acc319445b6 100644
--- a/drivers/iio/light/apds9960.c
+++ b/drivers/iio/light/apds9960.c
@@ -987,7 +987,6 @@ static int apds9960_probe(struct i2c_client *client,
 			  const struct i2c_device_id *id)
 {
 	struct apds9960_data *data;
-	struct iio_buffer *buffer;
 	struct iio_dev *indio_dev;
 	int ret;
 
@@ -995,20 +994,18 @@ static int apds9960_probe(struct i2c_client *client,
 	if (!indio_dev)
 		return -ENOMEM;
 
-	buffer = devm_iio_kfifo_allocate(&client->dev);
-	if (!buffer)
-		return -ENOMEM;
-
-	iio_device_attach_buffer(indio_dev, buffer);
-
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->info = &apds9960_info;
 	indio_dev->name = APDS9960_DRV_NAME;
 	indio_dev->channels = apds9960_channels;
 	indio_dev->num_channels = ARRAY_SIZE(apds9960_channels);
 	indio_dev->available_scan_masks = apds9960_scan_masks;
-	indio_dev->modes = (INDIO_BUFFER_SOFTWARE | INDIO_DIRECT_MODE);
-	indio_dev->setup_ops = &apds9960_buffer_setup_ops;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE,
+					     &apds9960_buffer_setup_ops);
+	if (ret)
+		return ret;
 
 	data = iio_priv(indio_dev);
 	i2c_set_clientdata(client, indio_dev);
-- 
2.17.1

Re: [PATCH 2/3] iio: make use of iio_device_attach_kfifo_buffer() where straightforward

[Jonathan Cameron]

On Wed, 1 Apr 2020 15:59:35 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> All drivers that already call devm_iio_kfifo_allocate() &
> iio_device_attach_buffer() are simple to convert to
> iio_device_attach_kfifo_buffer() in a single go/patch/.
> 
> This change does that.
> 
> For drivers max30100 & max30102 this helper is called after indio_dev has
> been populated. This doesn't make any difference [at this point in time].
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Comments inline refer back to the question of whether this results in
any order changes that might matter.  Unfortunately it does. I think
we need to allow the new function to take the struct device *
that the driver author wants it to us and to have the naming make it
clear it's a managed function.

The alternative is to go tidy up the allocations so all of the managed
calls scopes are correct.  I.e. any that are associated with the iio_dev
(or iio_priv obviously) use the indio_dev->dev including irq requests
etc.  That might be a good exercise to do but it's not a small one
and the benefits aren't obvious.  We'd move from a simple linear unwind
to a nested one.

devm_iio_device_alloc
devm_iio_*_alloc
devm_request_threaded_irq etc
devm_kzalloc
devm_iio_device_register

devm_iio_device_alloc
	devm_iio*alloc
	devm_request_threaded_irq
	devm_kzalloc
	devm_iio_device_register

So the release of the parent in the second cause the unwind of the
device setup in devm_iio_device_alloc and take out all of the items below.

If we'd structure this right in the first place the second option might be
more elegant but we didn't so retrofitting it now will be messy.

Jonathan

> ---
>  drivers/iio/accel/sca3000.c                    | 18 ++----------------
>  drivers/iio/accel/ssp_accel_sensor.c           | 13 ++++---------
>  drivers/iio/adc/ina2xx-adc.c                   | 13 +++++--------
>  drivers/iio/gyro/ssp_gyro_sensor.c             | 13 ++++---------
>  drivers/iio/health/max30100.c                  | 15 ++++++---------
>  drivers/iio/health/max30102.c                  | 15 ++++++---------
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 17 +++++++----------
>  drivers/iio/light/acpi-als.c                   | 11 +++++------
>  drivers/iio/light/apds9960.c                   | 15 ++++++---------
>  9 files changed, 45 insertions(+), 85 deletions(-)
> 
> diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> index 66d768d971e1..a0db76082ba6 100644
> --- a/drivers/iio/accel/sca3000.c
> +++ b/drivers/iio/accel/sca3000.c
> @@ -1272,20 +1272,6 @@ static int sca3000_write_event_config(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> -static int sca3000_configure_ring(struct iio_dev *indio_dev)
> -{
> -	struct iio_buffer *buffer;
> -
> -	buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
> -	if (!buffer)
> -		return -ENOMEM;
> -
> -	iio_device_attach_buffer(indio_dev, buffer);
> -	indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
> -
> -	return 0;
> -}
> -
>  static inline
>  int __sca3000_hw_ring_state_set(struct iio_dev *indio_dev, bool state)
>  {
> @@ -1480,7 +1466,8 @@ static int sca3000_probe(struct spi_device *spi)
>  	}
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> -	ret = sca3000_configure_ring(indio_dev);
> +	ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE,
> +					     &sca3000_ring_setup_ops);

This one is fine (for a moment I thought we had a bug in the current code)
as this call is the next thing that needs unwinding after the devm_iio_device_alloc
anyway.  It would however have been more consistent if original code had
used the parent dev.

>  	if (ret)
>  		return ret;
>  
> @@ -1494,7 +1481,6 @@ static int sca3000_probe(struct spi_device *spi)
>  		if (ret)
>  			return ret;
>  	}
> -	indio_dev->setup_ops = &sca3000_ring_setup_ops;
>  	ret = sca3000_clean_setup(st);
>  	if (ret)
>  		goto error_free_irq;
> diff --git a/drivers/iio/accel/ssp_accel_sensor.c b/drivers/iio/accel/ssp_accel_sensor.c
> index c32647abce20..1d9971db949d 100644
> --- a/drivers/iio/accel/ssp_accel_sensor.c
> +++ b/drivers/iio/accel/ssp_accel_sensor.c
> @@ -96,7 +96,6 @@ static int ssp_accel_probe(struct platform_device *pdev)
>  	int ret;
>  	struct iio_dev *indio_dev;
>  	struct ssp_sensor_data *spd;
> -	struct iio_buffer *buffer;
>  
>  	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*spd));
>  	if (!indio_dev)
> @@ -111,18 +110,14 @@ static int ssp_accel_probe(struct platform_device *pdev)
>  	indio_dev->dev.parent = &pdev->dev;
>  	indio_dev->dev.of_node = pdev->dev.of_node;
>  	indio_dev->info = &ssp_accel_iio_info;
> -	indio_dev->modes = INDIO_BUFFER_SOFTWARE;
>  	indio_dev->channels = ssp_acc_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(ssp_acc_channels);
>  	indio_dev->available_scan_masks = ssp_accel_scan_mask;
>  
> -	buffer = devm_iio_kfifo_allocate(&pdev->dev);
> -	if (!buffer)
> -		return -ENOMEM;
> -
> -	iio_device_attach_buffer(indio_dev, buffer);
> -
> -	indio_dev->setup_ops = &ssp_accel_buffer_ops;
> +	ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE,
> +					     &ssp_accel_buffer_ops);
> +	if (ret)
> +		return ret;
>  
>  	platform_set_drvdata(pdev, indio_dev);
>  
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index bdd7cba6f6b0..2070809e1acc 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -950,7 +950,6 @@ static int ina2xx_probe(struct i2c_client *client,
>  {
>  	struct ina2xx_chip_info *chip;
>  	struct iio_dev *indio_dev;
> -	struct iio_buffer *buffer;
>  	unsigned int val;
>  	enum ina2xx_ids type;
>  	int ret;
> @@ -1014,7 +1013,7 @@ static int ina2xx_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> -	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->dev.of_node = client->dev.of_node;
>  	if (id->driver_data == ina226) {
> @@ -1027,13 +1026,11 @@ static int ina2xx_probe(struct i2c_client *client,
>  		indio_dev->info = &ina219_info;
>  	}
>  	indio_dev->name = id->name;
> -	indio_dev->setup_ops = &ina2xx_setup_ops;
>  
> -	buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
> -	if (!buffer)
> -		return -ENOMEM;
> -
> -	iio_device_attach_buffer(indio_dev, buffer);
> +	ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE,
> +					     &ina2xx_setup_ops);
> +	if (ret)
> +		return ret;

This changes the unwind order.  Not good from an obviously correct point of
view.

>  
>  	return iio_device_register(indio_dev);
>  }
> diff --git a/drivers/iio/gyro/ssp_gyro_sensor.c b/drivers/iio/gyro/ssp_gyro_sensor.c
> index 4e4ee4167544..c12d3db5a951 100644
> --- a/drivers/iio/gyro/ssp_gyro_sensor.c
> +++ b/drivers/iio/gyro/ssp_gyro_sensor.c
> @@ -96,7 +96,6 @@ static int ssp_gyro_probe(struct platform_device *pdev)
>  	int ret;
>  	struct iio_dev *indio_dev;
>  	struct ssp_sensor_data *spd;
> -	struct iio_buffer *buffer;
>  
>  	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*spd));
>  	if (!indio_dev)
> @@ -110,18 +109,14 @@ static int ssp_gyro_probe(struct platform_device *pdev)
>  	indio_dev->name = ssp_gyro_name;
>  	indio_dev->dev.parent = &pdev->dev;
>  	indio_dev->info = &ssp_gyro_iio_info;
> -	indio_dev->modes = INDIO_BUFFER_SOFTWARE;
>  	indio_dev->channels = ssp_gyro_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(ssp_gyro_channels);
>  	indio_dev->available_scan_masks = ssp_gyro_scan_mask;
>  
> -	buffer = devm_iio_kfifo_allocate(&pdev->dev);
> -	if (!buffer)
> -		return -ENOMEM;
> -
> -	iio_device_attach_buffer(indio_dev, buffer);
> -
> -	indio_dev->setup_ops = &ssp_gyro_buffer_ops;
> +	ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE,
> +					     &ssp_gyro_buffer_ops);
> +	if (ret)
> +		return ret;
>  
>  	platform_set_drvdata(pdev, indio_dev);
>  
> diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
> index 546fc37ad75d..f4e9866f4c3d 100644
> --- a/drivers/iio/health/max30100.c
> +++ b/drivers/iio/health/max30100.c
> @@ -418,7 +418,6 @@ static int max30100_probe(struct i2c_client *client,
>  			  const struct i2c_device_id *id)
>  {
>  	struct max30100_data *data;
> -	struct iio_buffer *buffer;
>  	struct iio_dev *indio_dev;
>  	int ret;
>  
> @@ -426,21 +425,19 @@ static int max30100_probe(struct i2c_client *client,
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
> -	buffer = devm_iio_kfifo_allocate(&client->dev);
> -	if (!buffer)
> -		return -ENOMEM;
> -
> -	iio_device_attach_buffer(indio_dev, buffer);
> -
>  	indio_dev->name = MAX30100_DRV_NAME;
>  	indio_dev->channels = max30100_channels;
>  	indio_dev->info = &max30100_info;
>  	indio_dev->num_channels = ARRAY_SIZE(max30100_channels);
>  	indio_dev->available_scan_masks = max30100_scan_masks;
> -	indio_dev->modes = (INDIO_BUFFER_SOFTWARE | INDIO_DIRECT_MODE);
> -	indio_dev->setup_ops = &max30100_buffer_setup_ops;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->dev.parent = &client->dev;
>  
> +	ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE,
> +					     &max30100_buffer_setup_ops);
> +	if (ret)
> +		return ret;
> +
>  	data = iio_priv(indio_dev);
>  	data->indio_dev = indio_dev;
>  	data->client = client;
> diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
> index 74fc260b957e..e15126d23dfb 100644
> --- a/drivers/iio/health/max30102.c
> +++ b/drivers/iio/health/max30102.c
> @@ -507,7 +507,6 @@ static int max30102_probe(struct i2c_client *client,
>  			  const struct i2c_device_id *id)
>  {
>  	struct max30102_data *data;
> -	struct iio_buffer *buffer;
>  	struct iio_dev *indio_dev;
>  	int ret;
>  	unsigned int reg;
> @@ -516,16 +515,9 @@ static int max30102_probe(struct i2c_client *client,
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
> -	buffer = devm_iio_kfifo_allocate(&client->dev);
> -	if (!buffer)
> -		return -ENOMEM;
> -
> -	iio_device_attach_buffer(indio_dev, buffer);
> -
>  	indio_dev->name = MAX30102_DRV_NAME;
>  	indio_dev->info = &max30102_info;
> -	indio_dev->modes = (INDIO_BUFFER_SOFTWARE | INDIO_DIRECT_MODE);
> -	indio_dev->setup_ops = &max30102_buffer_setup_ops;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->dev.parent = &client->dev;
>  
>  	data = iio_priv(indio_dev);
> @@ -551,6 +543,11 @@ static int max30102_probe(struct i2c_client *client,
>  		return -ENODEV;
>  	}
>  
> +	ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE,
> +					     &max30102_buffer_setup_ops);
> +	if (ret)
> +		return ret;
> +
>  	data->regmap = devm_regmap_init_i2c(client, &max30102_regmap_config);
>  	if (IS_ERR(data->regmap)) {
>  		dev_err(&client->dev, "regmap initialization failed\n");
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> index bb899345f2bb..4ba3d5551570 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c

I'm not going to layout the details, but this one isn't unwinding in the same
order after this change.  There are devm_kzalloc calls between the devm_iio_device_alloc and
the new call.

> @@ -30,8 +30,8 @@
>   * Denis Ciocca <denis.ciocca@st.com>
>   */
>  #include <linux/module.h>
> -#include <linux/iio/kfifo_buf.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/kfifo_buf.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/regmap.h>
>  #include <linux/bitfield.h>
> @@ -713,20 +713,17 @@ static const struct iio_buffer_setup_ops st_lsm6dsx_buffer_ops = {
>  
>  int st_lsm6dsx_fifo_setup(struct st_lsm6dsx_hw *hw)
>  {
> -	struct iio_buffer *buffer;
> -	int i;
> +	int i, ret;
>  
>  	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
>  		if (!hw->iio_devs[i])
>  			continue;
>  
> -		buffer = devm_iio_kfifo_allocate(hw->dev);
> -		if (!buffer)
> -			return -ENOMEM;
> -
> -		iio_device_attach_buffer(hw->iio_devs[i], buffer);
> -		hw->iio_devs[i]->modes |= INDIO_BUFFER_SOFTWARE;
> -		hw->iio_devs[i]->setup_ops = &st_lsm6dsx_buffer_ops;
> +		ret = iio_device_attach_kfifo_buffer(hw->iio_devs[i],
> +						     INDIO_BUFFER_SOFTWARE,
> +						     &st_lsm6dsx_buffer_ops);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	return 0;
> diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
> index 1eafd0b24e18..ea99705c3387 100644
> --- a/drivers/iio/light/acpi-als.c
> +++ b/drivers/iio/light/acpi-als.c
> @@ -166,6 +166,7 @@ static int acpi_als_add(struct acpi_device *device)
>  	struct acpi_als *als;
>  	struct iio_dev *indio_dev;
>  	struct iio_buffer *buffer;
> +	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(&device->dev, sizeof(*als));
>  	if (!indio_dev)
> @@ -180,15 +181,13 @@ static int acpi_als_add(struct acpi_device *device)
>  	indio_dev->name = ACPI_ALS_DEVICE_NAME;
>  	indio_dev->dev.parent = &device->dev;
>  	indio_dev->info = &acpi_als_info;
> -	indio_dev->modes = INDIO_BUFFER_SOFTWARE;
>  	indio_dev->channels = acpi_als_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(acpi_als_channels);
>  
> -	buffer = devm_iio_kfifo_allocate(&device->dev);
> -	if (!buffer)
> -		return -ENOMEM;
> -
> -	iio_device_attach_buffer(indio_dev, buffer);
> +	ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE,
> +					     NULL);
> +	if (ret)
> +		return ret;

This one is fine as next to the devm_iio_device_alloc anyway.

>  
>  	return devm_iio_device_register(&device->dev, indio_dev);
>  }
> diff --git a/drivers/iio/light/apds9960.c b/drivers/iio/light/apds9960.c
> index 52f86bc777dd..8acc319445b6 100644
> --- a/drivers/iio/light/apds9960.c
> +++ b/drivers/iio/light/apds9960.c
> @@ -987,7 +987,6 @@ static int apds9960_probe(struct i2c_client *client,
>  			  const struct i2c_device_id *id)
>  {
>  	struct apds9960_data *data;
> -	struct iio_buffer *buffer;
>  	struct iio_dev *indio_dev;
>  	int ret;
>  
> @@ -995,20 +994,18 @@ static int apds9960_probe(struct i2c_client *client,
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
> -	buffer = devm_iio_kfifo_allocate(&client->dev);
> -	if (!buffer)
> -		return -ENOMEM;
> -
> -	iio_device_attach_buffer(indio_dev, buffer);
> -
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->info = &apds9960_info;
>  	indio_dev->name = APDS9960_DRV_NAME;
>  	indio_dev->channels = apds9960_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(apds9960_channels);
>  	indio_dev->available_scan_masks = apds9960_scan_masks;
> -	indio_dev->modes = (INDIO_BUFFER_SOFTWARE | INDIO_DIRECT_MODE);
> -	indio_dev->setup_ops = &apds9960_buffer_setup_ops;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE,
> +					     &apds9960_buffer_setup_ops);
> +	if (ret)
> +		return ret;

In this case we do have a managed call after this, but as there is nothing between
the devm_iio_device_alloc and iio_device_attach_kfifo_buffer I think we are fine.

>  
>  	data = iio_priv(indio_dev);
>  	i2c_set_clientdata(client, indio_dev);

[PATCH 3/3] staging: iio: ad5933: use iio_device_attach_kfifo_buffer() helper

[Alexandru Ardelean]

This driver calls iio_kfifo_allocate() vs devm_iio_kfifo_allocate(). But
the conversion is still simpler here, and cleans-up/reduces some error
paths.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 .../staging/iio/impedance-analyzer/ad5933.c   | 28 ++++---------------
 1 file changed, 5 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
index af0bcf95ee8a..7bde93c6dd74 100644
--- a/drivers/staging/iio/impedance-analyzer/ad5933.c
+++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
@@ -602,22 +602,6 @@ static const struct iio_buffer_setup_ops ad5933_ring_setup_ops = {
 	.postdisable = ad5933_ring_postdisable,
 };
 
-static int ad5933_register_ring_funcs_and_init(struct iio_dev *indio_dev)
-{
-	struct iio_buffer *buffer;
-
-	buffer = iio_kfifo_allocate();
-	if (!buffer)
-		return -ENOMEM;
-
-	iio_device_attach_buffer(indio_dev, buffer);
-
-	/* Ring buffer functions - here trigger setup related */
-	indio_dev->setup_ops = &ad5933_ring_setup_ops;
-
-	return 0;
-}
-
 static void ad5933_work(struct work_struct *work)
 {
 	struct ad5933_state *st = container_of(work,
@@ -738,26 +722,25 @@ static int ad5933_probe(struct i2c_client *client,
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->info = &ad5933_info;
 	indio_dev->name = id->name;
-	indio_dev->modes = (INDIO_BUFFER_SOFTWARE | INDIO_DIRECT_MODE);
+	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = ad5933_channels;
 	indio_dev->num_channels = ARRAY_SIZE(ad5933_channels);
 
-	ret = ad5933_register_ring_funcs_and_init(indio_dev);
+	ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE,
+					     &ad5933_ring_setup_ops);
 	if (ret)
 		goto error_disable_mclk;
 
 	ret = ad5933_setup(st);
 	if (ret)
-		goto error_unreg_ring;
+		goto error_disable_mclk;
 
 	ret = iio_device_register(indio_dev);
 	if (ret)
-		goto error_unreg_ring;
+		goto error_disable_mclk;
 
 	return 0;
 
-error_unreg_ring:
-	iio_kfifo_free(indio_dev->buffer);
 error_disable_mclk:
 	clk_disable_unprepare(st->mclk);
 error_disable_reg:
@@ -772,7 +755,6 @@ static int ad5933_remove(struct i2c_client *client)
 	struct ad5933_state *st = iio_priv(indio_dev);
 
 	iio_device_unregister(indio_dev);
-	iio_kfifo_free(indio_dev->buffer);
 	regulator_disable(st->reg);
 	clk_disable_unprepare(st->mclk);
 
-- 
2.17.1

Re: [PATCH 3/3] staging: iio: ad5933: use iio_device_attach_kfifo_buffer() helper

[Jonathan Cameron]

On Wed, 1 Apr 2020 15:59:36 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This driver calls iio_kfifo_allocate() vs devm_iio_kfifo_allocate(). But
> the conversion is still simpler here, and cleans-up/reduces some error
> paths.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

This mixes devm managed stuff an unmanaged.  Hence it fails the 'obviously correct'
test.  If you wanted to do this you'd first need to sort out the unmanaged
bits to be automatically unwound (regulators and clocks). Or potentially reorder
the driver so those happen after this allocation is done.

Thanks,

Jonathan

> ---
>  .../staging/iio/impedance-analyzer/ad5933.c   | 28 ++++---------------
>  1 file changed, 5 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
> index af0bcf95ee8a..7bde93c6dd74 100644
> --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> @@ -602,22 +602,6 @@ static const struct iio_buffer_setup_ops ad5933_ring_setup_ops = {
>  	.postdisable = ad5933_ring_postdisable,
>  };
>  
> -static int ad5933_register_ring_funcs_and_init(struct iio_dev *indio_dev)
> -{
> -	struct iio_buffer *buffer;
> -
> -	buffer = iio_kfifo_allocate();
> -	if (!buffer)
> -		return -ENOMEM;
> -
> -	iio_device_attach_buffer(indio_dev, buffer);
> -
> -	/* Ring buffer functions - here trigger setup related */
> -	indio_dev->setup_ops = &ad5933_ring_setup_ops;
> -
> -	return 0;
> -}
> -
>  static void ad5933_work(struct work_struct *work)
>  {
>  	struct ad5933_state *st = container_of(work,
> @@ -738,26 +722,25 @@ static int ad5933_probe(struct i2c_client *client,
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->info = &ad5933_info;
>  	indio_dev->name = id->name;
> -	indio_dev->modes = (INDIO_BUFFER_SOFTWARE | INDIO_DIRECT_MODE);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->channels = ad5933_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(ad5933_channels);
>  
> -	ret = ad5933_register_ring_funcs_and_init(indio_dev);
> +	ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE,
> +					     &ad5933_ring_setup_ops);
>  	if (ret)
>  		goto error_disable_mclk;
>  
>  	ret = ad5933_setup(st);
>  	if (ret)
> -		goto error_unreg_ring;
> +		goto error_disable_mclk;
>  
>  	ret = iio_device_register(indio_dev);
>  	if (ret)
> -		goto error_unreg_ring;
> +		goto error_disable_mclk;
>  
>  	return 0;
>  
> -error_unreg_ring:
> -	iio_kfifo_free(indio_dev->buffer);
>  error_disable_mclk:
>  	clk_disable_unprepare(st->mclk);
>  error_disable_reg:
> @@ -772,7 +755,6 @@ static int ad5933_remove(struct i2c_client *client)
>  	struct ad5933_state *st = iio_priv(indio_dev);
>  
>  	iio_device_unregister(indio_dev);
> -	iio_kfifo_free(indio_dev->buffer);
>  	regulator_disable(st->reg);
>  	clk_disable_unprepare(st->mclk);
>  

Re: [PATCH 3/3] staging: iio: ad5933: use iio_device_attach_kfifo_buffer() helper

[Ardelean, Alexandru]

On Sun, 2020-04-05 at 11:49 +0100, Jonathan Cameron wrote:
> [External]
> 
> On Wed, 1 Apr 2020 15:59:36 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> 
> > This driver calls iio_kfifo_allocate() vs devm_iio_kfifo_allocate(). But
> > the conversion is still simpler here, and cleans-up/reduces some error
> > paths.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> 
> This mixes devm managed stuff an unmanaged.  Hence it fails the 'obviously
> correct'
> test.  If you wanted to do this you'd first need to sort out the unmanaged
> bits to be automatically unwound (regulators and clocks). Or potentially
> reorder
> the driver so those happen after this allocation is done.
> 

Yeah.
I was a bit sloppy here.
I think tried a broader cleanup/rework would be a better idea here.


> Thanks,
> 
> Jonathan
> 
> > ---
> >  .../staging/iio/impedance-analyzer/ad5933.c   | 28 ++++---------------
> >  1 file changed, 5 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c
> > b/drivers/staging/iio/impedance-analyzer/ad5933.c
> > index af0bcf95ee8a..7bde93c6dd74 100644
> > --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> > +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> > @@ -602,22 +602,6 @@ static const struct iio_buffer_setup_ops
> > ad5933_ring_setup_ops = {
> >  	.postdisable = ad5933_ring_postdisable,
> >  };
> >  
> > -static int ad5933_register_ring_funcs_and_init(struct iio_dev *indio_dev)
> > -{
> > -	struct iio_buffer *buffer;
> > -
> > -	buffer = iio_kfifo_allocate();
> > -	if (!buffer)
> > -		return -ENOMEM;
> > -
> > -	iio_device_attach_buffer(indio_dev, buffer);
> > -
> > -	/* Ring buffer functions - here trigger setup related */
> > -	indio_dev->setup_ops = &ad5933_ring_setup_ops;
> > -
> > -	return 0;
> > -}
> > -
> >  static void ad5933_work(struct work_struct *work)
> >  {
> >  	struct ad5933_state *st = container_of(work,
> > @@ -738,26 +722,25 @@ static int ad5933_probe(struct i2c_client *client,
> >  	indio_dev->dev.parent = &client->dev;
> >  	indio_dev->info = &ad5933_info;
> >  	indio_dev->name = id->name;
> > -	indio_dev->modes = (INDIO_BUFFER_SOFTWARE | INDIO_DIRECT_MODE);
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> >  	indio_dev->channels = ad5933_channels;
> >  	indio_dev->num_channels = ARRAY_SIZE(ad5933_channels);
> >  
> > -	ret = ad5933_register_ring_funcs_and_init(indio_dev);
> > +	ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE,
> > +					     &ad5933_ring_setup_ops);
> >  	if (ret)
> >  		goto error_disable_mclk;
> >  
> >  	ret = ad5933_setup(st);
> >  	if (ret)
> > -		goto error_unreg_ring;
> > +		goto error_disable_mclk;
> >  
> >  	ret = iio_device_register(indio_dev);
> >  	if (ret)
> > -		goto error_unreg_ring;
> > +		goto error_disable_mclk;
> >  
> >  	return 0;
> >  
> > -error_unreg_ring:
> > -	iio_kfifo_free(indio_dev->buffer);
> >  error_disable_mclk:
> >  	clk_disable_unprepare(st->mclk);
> >  error_disable_reg:
> > @@ -772,7 +755,6 @@ static int ad5933_remove(struct i2c_client *client)
> >  	struct ad5933_state *st = iio_priv(indio_dev);
> >  
> >  	iio_device_unregister(indio_dev);
> > -	iio_kfifo_free(indio_dev->buffer);
> >  	regulator_disable(st->reg);
> >  	clk_disable_unprepare(st->mclk);
> >  

Re: [PATCH 1/3] iio: kfifo: add iio_device_attach_kfifo_buffer() helper

[Jonathan Cameron]

On Wed, 1 Apr 2020 15:59:34 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This change adds the iio_device_attach_kfifo_buffer() helper/short-hand,
> which groups the simple routine of allocating a kfifo buffers via
> devm_iio_kfifo_allocate() and calling iio_device_attach_buffer().
> 
> The mode_flags parameter is required. The setup_ops parameter is optional.
> 
> This function will be a bit more useful when needing to define multiple
> buffers per IIO device.
> 
> One requirement [that is more a recommendation] for this helper, is to call
> it after 'indio_dev' has been populated.
> 
> Also, one consequence related to using this helper is that the resource
> management of the buffer will be tied to 'indio_dev->dev'. Previously it
> was open-coded, and each driver does it slightly differently. Most of them
> tied it to the parent device, some of them to 'indio_dev->dev'.
> This shouldn't be a problem, and may be a good idea when adding more
> buffers per-device.

I'm glad you highlighted this subtlety.  I'm not sure it's safe in all cases
because the result is that the managed cleanup for this will occur once we
get to the cleanup for devm_iio_device_alloc and we release the indio_dev->dev

That would put it 'after' any other devm calls that are still hung off the parent
device.

Now the question is whether that ever causes us problems... See next patch.
It potentially does.  I think we need to provide the dev separately even
if it feels a bit silly to do so.  Scope management is complex so I don't
really want to force people to mix and match between different devices
and so get it wrong by accident.

The other issue is that it's not readily apparent from the naming that
this function is registering stuff that is cleaned up automatically or
that it even allocates anything that might need that..

devm_iio_device_attach_new_kfifo_buffer maybe?

I'm sort of wondering if we should do what dma did and have

iiom_device_attach_new_kfifo_buffer to indicate it's managed in the
scope of the iio device?

What do people think?

However, see patch 2 before commenting.  Reality is I'm not sure forcing
managed calls to hang off iio_dev->dev is a good idea (at this stage given
where we are).

Thanks

Jonathan


> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/iio/buffer/kfifo_buf.c | 37 ++++++++++++++++++++++++++++++++++
>  include/linux/iio/kfifo_buf.h  |  4 ++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
> index 3150f8ab984b..05b7c5fc6f1d 100644
> --- a/drivers/iio/buffer/kfifo_buf.c
> +++ b/drivers/iio/buffer/kfifo_buf.c
> @@ -228,4 +228,41 @@ void devm_iio_kfifo_free(struct device *dev, struct iio_buffer *r)
>  }
>  EXPORT_SYMBOL(devm_iio_kfifo_free);
>  
> +/**
> + * iio_device_attach_kfifo_buffer - Allocate a kfifo buffer & attach it to an IIO device
> + * @indio_dev: The device the buffer should be attached to
> + * @mode_flags: The mode flags for this buffer (INDIO_BUFFER_SOFTWARE and/or
> + *		INDIO_BUFFER_TRIGGERED).
> + * @setup_ops: The setup_ops required to configure the HW part of the buffer (optional)
> + *
> + * This function allocates a kfifo buffer via devm_iio_kfifo_allocate() and
> + * attaches it to the IIO device via iio_device_attach_buffer().
> + * This is meant to be a bit of a short-hand/helper function as many driver
> + * seem to do this.
> + */
> +int iio_device_attach_kfifo_buffer(struct iio_dev *indio_dev,
> +				   int mode_flags,
> +				   const struct iio_buffer_setup_ops *setup_ops)
> +{
> +	struct iio_buffer *buffer;
> +
> +	if (mode_flags)
> +		mode_flags &= kfifo_access_funcs.modes;
> +
> +	if (!mode_flags)
> +		return -EINVAL;
> +
> +	buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	iio_device_attach_buffer(indio_dev, buffer);
> +
> +	indio_dev->modes |= mode_flags;
> +	indio_dev->setup_ops = setup_ops;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(iio_device_attach_kfifo_buffer);
> +
>  MODULE_LICENSE("GPL");
> diff --git a/include/linux/iio/kfifo_buf.h b/include/linux/iio/kfifo_buf.h
> index 764659e01b68..2363a931be14 100644
> --- a/include/linux/iio/kfifo_buf.h
> +++ b/include/linux/iio/kfifo_buf.h
> @@ -11,4 +11,8 @@ void iio_kfifo_free(struct iio_buffer *r);
>  struct iio_buffer *devm_iio_kfifo_allocate(struct device *dev);
>  void devm_iio_kfifo_free(struct device *dev, struct iio_buffer *r);
>  
> +int iio_device_attach_kfifo_buffer(struct iio_dev *indio_dev,
> +				   int mode_flags,
> +				   const struct iio_buffer_setup_ops *setup_ops);
> +
>  #endif

Re: [PATCH 1/3] iio: kfifo: add iio_device_attach_kfifo_buffer() helper

[Ardelean, Alexandru]

On Sun, 2020-04-05 at 11:46 +0100, Jonathan Cameron wrote:
> [External]
> 
> On Wed, 1 Apr 2020 15:59:34 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> 
> > This change adds the iio_device_attach_kfifo_buffer() helper/short-hand,
> > which groups the simple routine of allocating a kfifo buffers via
> > devm_iio_kfifo_allocate() and calling iio_device_attach_buffer().
> > 
> > The mode_flags parameter is required. The setup_ops parameter is optional.
> > 
> > This function will be a bit more useful when needing to define multiple
> > buffers per IIO device.
> > 
> > One requirement [that is more a recommendation] for this helper, is to call
> > it after 'indio_dev' has been populated.
> > 
> > Also, one consequence related to using this helper is that the resource
> > management of the buffer will be tied to 'indio_dev->dev'. Previously it
> > was open-coded, and each driver does it slightly differently. Most of them
> > tied it to the parent device, some of them to 'indio_dev->dev'.
> > This shouldn't be a problem, and may be a good idea when adding more
> > buffers per-device.
> 
> I'm glad you highlighted this subtlety.  I'm not sure it's safe in all cases
> because the result is that the managed cleanup for this will occur once we
> get to the cleanup for devm_iio_device_alloc and we release the indio_dev->dev
> 
> That would put it 'after' any other devm calls that are still hung off the
> parent
> device.
> 
> Now the question is whether that ever causes us problems... See next patch.
> It potentially does.  I think we need to provide the dev separately even
> if it feels a bit silly to do so.  Scope management is complex so I don't
> really want to force people to mix and match between different devices
> and so get it wrong by accident.
> 
> The other issue is that it's not readily apparent from the naming that
> this function is registering stuff that is cleaned up automatically or
> that it even allocates anything that might need that..
> 
> devm_iio_device_attach_new_kfifo_buffer maybe?
> 
> I'm sort of wondering if we should do what dma did and have
> 
> iiom_device_attach_new_kfifo_buffer to indicate it's managed in the
> scope of the iio device?
> 
> What do people think?
> 
> However, see patch 2 before commenting.  Reality is I'm not sure forcing
> managed calls to hang off iio_dev->dev is a good idea (at this stage given
> where we are).

What I am really after with this patch is to hide away these:
     iio_kfifo_free(indio_dev->buffer);
     iio_buffer_set_attrs(indio_dev->buffer, xxxx_fifo_attributes); 
i.e. not have 'indio_dev->buffer' open-coded in drivers, and hide it in IIO core
somewhere.
Some ideas can go in parallel [like this one] to add support for multiple
buffers.

So, I will think of a better [less sloppy] V2 for this.

One intermediate alternative is to do 'iio_device_kfifo_free(indio_dev)', but
I'll still try to think of a better devm_ approach.
devm_iio_device_attach_new_kfifo_buffer() sounds a bit long but may work.
iiom_device_attach_new_kfifo_buffer() can also work.

What if we just default attaching to the parent device?

Would it work to also attach the parent device in devm_iio_device_alloc() by
default?
Or change 'iio_device_alloc()' to take a parent device as argument?
Which for devm_iio_device_alloc(dev,...) would implicitly mean that 'dev' is
'parent'?

These are just some thoughts.


> 
> Thanks
> 
> Jonathan
> 
> 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  drivers/iio/buffer/kfifo_buf.c | 37 ++++++++++++++++++++++++++++++++++
> >  include/linux/iio/kfifo_buf.h  |  4 ++++
> >  2 files changed, 41 insertions(+)
> > 
> > diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
> > index 3150f8ab984b..05b7c5fc6f1d 100644
> > --- a/drivers/iio/buffer/kfifo_buf.c
> > +++ b/drivers/iio/buffer/kfifo_buf.c
> > @@ -228,4 +228,41 @@ void devm_iio_kfifo_free(struct device *dev, struct
> > iio_buffer *r)
> >  }
> >  EXPORT_SYMBOL(devm_iio_kfifo_free);
> >  
> > +/**
> > + * iio_device_attach_kfifo_buffer - Allocate a kfifo buffer & attach it to
> > an IIO device
> > + * @indio_dev: The device the buffer should be attached to
> > + * @mode_flags: The mode flags for this buffer (INDIO_BUFFER_SOFTWARE
> > and/or
> > + *		INDIO_BUFFER_TRIGGERED).
> > + * @setup_ops: The setup_ops required to configure the HW part of the
> > buffer (optional)
> > + *
> > + * This function allocates a kfifo buffer via devm_iio_kfifo_allocate() and
> > + * attaches it to the IIO device via iio_device_attach_buffer().
> > + * This is meant to be a bit of a short-hand/helper function as many driver
> > + * seem to do this.
> > + */
> > +int iio_device_attach_kfifo_buffer(struct iio_dev *indio_dev,
> > +				   int mode_flags,
> > +				   const struct iio_buffer_setup_ops *setup_ops)
> > +{
> > +	struct iio_buffer *buffer;
> > +
> > +	if (mode_flags)
> > +		mode_flags &= kfifo_access_funcs.modes;
> > +
> > +	if (!mode_flags)
> > +		return -EINVAL;
> > +
> > +	buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
> > +	if (!buffer)
> > +		return -ENOMEM;
> > +
> > +	iio_device_attach_buffer(indio_dev, buffer);
> > +
> > +	indio_dev->modes |= mode_flags;
> > +	indio_dev->setup_ops = setup_ops;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(iio_device_attach_kfifo_buffer);
> > +
> >  MODULE_LICENSE("GPL");
> > diff --git a/include/linux/iio/kfifo_buf.h b/include/linux/iio/kfifo_buf.h
> > index 764659e01b68..2363a931be14 100644
> > --- a/include/linux/iio/kfifo_buf.h
> > +++ b/include/linux/iio/kfifo_buf.h
> > @@ -11,4 +11,8 @@ void iio_kfifo_free(struct iio_buffer *r);
> >  struct iio_buffer *devm_iio_kfifo_allocate(struct device *dev);
> >  void devm_iio_kfifo_free(struct device *dev, struct iio_buffer *r);
> >  
> > +int iio_device_attach_kfifo_buffer(struct iio_dev *indio_dev,
> > +				   int mode_flags,
> > +				   const struct iio_buffer_setup_ops
> > *setup_ops);
> > +
> >  #endif

Re: [PATCH 1/3] iio: kfifo: add iio_device_attach_kfifo_buffer() helper

[Jonathan Cameron]

On Mon, 6 Apr 2020 08:12:42 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Sun, 2020-04-05 at 11:46 +0100, Jonathan Cameron wrote:
> > [External]
> > 
> > On Wed, 1 Apr 2020 15:59:34 +0300
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> >   
> > > This change adds the iio_device_attach_kfifo_buffer() helper/short-hand,
> > > which groups the simple routine of allocating a kfifo buffers via
> > > devm_iio_kfifo_allocate() and calling iio_device_attach_buffer().
> > > 
> > > The mode_flags parameter is required. The setup_ops parameter is optional.
> > > 
> > > This function will be a bit more useful when needing to define multiple
> > > buffers per IIO device.
> > > 
> > > One requirement [that is more a recommendation] for this helper, is to call
> > > it after 'indio_dev' has been populated.
> > > 
> > > Also, one consequence related to using this helper is that the resource
> > > management of the buffer will be tied to 'indio_dev->dev'. Previously it
> > > was open-coded, and each driver does it slightly differently. Most of them
> > > tied it to the parent device, some of them to 'indio_dev->dev'.
> > > This shouldn't be a problem, and may be a good idea when adding more
> > > buffers per-device.  
> > 
> > I'm glad you highlighted this subtlety.  I'm not sure it's safe in all cases
> > because the result is that the managed cleanup for this will occur once we
> > get to the cleanup for devm_iio_device_alloc and we release the indio_dev->dev
> > 
> > That would put it 'after' any other devm calls that are still hung off the
> > parent
> > device.
> > 
> > Now the question is whether that ever causes us problems... See next patch.
> > It potentially does.  I think we need to provide the dev separately even
> > if it feels a bit silly to do so.  Scope management is complex so I don't
> > really want to force people to mix and match between different devices
> > and so get it wrong by accident.
> > 
> > The other issue is that it's not readily apparent from the naming that
> > this function is registering stuff that is cleaned up automatically or
> > that it even allocates anything that might need that..
> > 
> > devm_iio_device_attach_new_kfifo_buffer maybe?
> > 
> > I'm sort of wondering if we should do what dma did and have
> > 
> > iiom_device_attach_new_kfifo_buffer to indicate it's managed in the
> > scope of the iio device?
> > 
> > What do people think?
> > 
> > However, see patch 2 before commenting.  Reality is I'm not sure forcing
> > managed calls to hang off iio_dev->dev is a good idea (at this stage given
> > where we are).  
> 
> What I am really after with this patch is to hide away these:
>      iio_kfifo_free(indio_dev->buffer);
>      iio_buffer_set_attrs(indio_dev->buffer, xxxx_fifo_attributes); 
> i.e. not have 'indio_dev->buffer' open-coded in drivers, and hide it in IIO core
> somewhere.
> Some ideas can go in parallel [like this one] to add support for multiple
> buffers.
> 
> So, I will think of a better [less sloppy] V2 for this.
> 
> One intermediate alternative is to do 'iio_device_kfifo_free(indio_dev)', but
> I'll still try to think of a better devm_ approach.
> devm_iio_device_attach_new_kfifo_buffer() sounds a bit long but may work.
> iiom_device_attach_new_kfifo_buffer() can also work.
> 
> What if we just default attaching to the parent device?

That would work and be consistent with the vast majority of current cases.

> 
> Would it work to also attach the parent device in devm_iio_device_alloc() by
> default?

That would need a thorough audit to check nothing crazy is done by
a driver with an odd structure.  Such a driver would (I think) be
buggy though as the child lifetime should be dependent on the parent
and not some other device.

> Or change 'iio_device_alloc()' to take a parent device as argument?

I think there are only a couple of users, so that would work.

> Which for devm_iio_device_alloc(dev,...) would implicitly mean that 'dev' is
> 'parent'?

I think that's a fair assumption (though needs a sanity check)

> 
> These are just some thoughts.
> 
> 
> > 
> > Thanks
> > 
> > Jonathan
> > 
> >   
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > ---
> > >  drivers/iio/buffer/kfifo_buf.c | 37 ++++++++++++++++++++++++++++++++++
> > >  include/linux/iio/kfifo_buf.h  |  4 ++++
> > >  2 files changed, 41 insertions(+)
> > > 
> > > diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
> > > index 3150f8ab984b..05b7c5fc6f1d 100644
> > > --- a/drivers/iio/buffer/kfifo_buf.c
> > > +++ b/drivers/iio/buffer/kfifo_buf.c
> > > @@ -228,4 +228,41 @@ void devm_iio_kfifo_free(struct device *dev, struct
> > > iio_buffer *r)
> > >  }
> > >  EXPORT_SYMBOL(devm_iio_kfifo_free);
> > >  
> > > +/**
> > > + * iio_device_attach_kfifo_buffer - Allocate a kfifo buffer & attach it to
> > > an IIO device
> > > + * @indio_dev: The device the buffer should be attached to
> > > + * @mode_flags: The mode flags for this buffer (INDIO_BUFFER_SOFTWARE
> > > and/or
> > > + *		INDIO_BUFFER_TRIGGERED).
> > > + * @setup_ops: The setup_ops required to configure the HW part of the
> > > buffer (optional)
> > > + *
> > > + * This function allocates a kfifo buffer via devm_iio_kfifo_allocate() and
> > > + * attaches it to the IIO device via iio_device_attach_buffer().
> > > + * This is meant to be a bit of a short-hand/helper function as many driver
> > > + * seem to do this.
> > > + */
> > > +int iio_device_attach_kfifo_buffer(struct iio_dev *indio_dev,
> > > +				   int mode_flags,
> > > +				   const struct iio_buffer_setup_ops *setup_ops)
> > > +{
> > > +	struct iio_buffer *buffer;
> > > +
> > > +	if (mode_flags)
> > > +		mode_flags &= kfifo_access_funcs.modes;
> > > +
> > > +	if (!mode_flags)
> > > +		return -EINVAL;
> > > +
> > > +	buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
> > > +	if (!buffer)
> > > +		return -ENOMEM;
> > > +
> > > +	iio_device_attach_buffer(indio_dev, buffer);
> > > +
> > > +	indio_dev->modes |= mode_flags;
> > > +	indio_dev->setup_ops = setup_ops;
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(iio_device_attach_kfifo_buffer);
> > > +
> > >  MODULE_LICENSE("GPL");
> > > diff --git a/include/linux/iio/kfifo_buf.h b/include/linux/iio/kfifo_buf.h
> > > index 764659e01b68..2363a931be14 100644
> > > --- a/include/linux/iio/kfifo_buf.h
> > > +++ b/include/linux/iio/kfifo_buf.h
> > > @@ -11,4 +11,8 @@ void iio_kfifo_free(struct iio_buffer *r);
> > >  struct iio_buffer *devm_iio_kfifo_allocate(struct device *dev);
> > >  void devm_iio_kfifo_free(struct device *dev, struct iio_buffer *r);
> > >  
> > > +int iio_device_attach_kfifo_buffer(struct iio_dev *indio_dev,
> > > +				   int mode_flags,
> > > +				   const struct iio_buffer_setup_ops
> > > *setup_ops);
> > > +
> > >  #endif