[PATCH 0/7] Simplify codes with devm_add_action_or_reset

[PATCH 0/7] Simplify codes with devm_add_action_or_reset

[Yicong Yang]

Some devm variants are implemented with devres_alloc() and devres_add()
manually to only manage a single pointer. This can be simplified with
devm_add_action_or_reset() which works the same. Simplify these functions.

Yicong Yang (7):
  iio: adc: adi-axi-adc: simplify devm_adi_axi_adc_conv_register
  iio: buffer-dmaengine: simplify __devm_iio_dmaengine_buffer_free
  iio: hw_consumer: simplify devm_iio_hw_consumer_alloc
  iio: triggered-buffer: simplify devm_iio_triggered_buffer_setup_ext
  iio: core: simplify some devm functions
  iio: trigger: simplify __devm_iio_trigger_register
  iio: inkern: simplify some devm functions

 drivers/iio/adc/adi-axi-adc.c                      | 22 ++++----
 drivers/iio/buffer/industrialio-buffer-dmaengine.c | 22 ++++----
 drivers/iio/buffer/industrialio-hw-consumer.c      | 25 ++++-----
 drivers/iio/buffer/industrialio-triggered-buffer.c | 19 ++-----
 drivers/iio/industrialio-core.c                    | 43 ++++++---------
 drivers/iio/industrialio-trigger.c                 | 18 ++-----
 drivers/iio/inkern.c                               | 61 ++++++++--------------
 7 files changed, 78 insertions(+), 132 deletions(-)

-- 
2.8.1

[PATCH 1/7] iio: adc: adi-axi-adc: simplify devm_adi_axi_adc_conv_register

[Yicong Yang]

Use devm_add_action_or_reset() instead of devres_alloc() and
devres_add(), which works the same. This will simplify the
code. There is no functional changes.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/iio/adc/adi-axi-adc.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
index 9109da2..575a63f 100644
--- a/drivers/iio/adc/adi-axi-adc.c
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -210,29 +210,25 @@ static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv)
 	kfree(cl);
 }
 
-static void devm_adi_axi_adc_conv_release(struct device *dev, void *res)
+static void devm_adi_axi_adc_conv_release(void *conv)
 {
-	adi_axi_adc_conv_unregister(*(struct adi_axi_adc_conv **)res);
+	adi_axi_adc_conv_unregister(conv);
 }
 
 struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev,
 							size_t sizeof_priv)
 {
-	struct adi_axi_adc_conv **ptr, *conv;
-
-	ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr),
-			   GFP_KERNEL);
-	if (!ptr)
-		return ERR_PTR(-ENOMEM);
+	struct adi_axi_adc_conv *conv;
+	int ret;
 
 	conv = adi_axi_adc_conv_register(dev, sizeof_priv);
-	if (IS_ERR(conv)) {
-		devres_free(ptr);
+	if (IS_ERR(conv))
 		return ERR_CAST(conv);
-	}
 
-	*ptr = conv;
-	devres_add(dev, ptr);
+	ret = devm_add_action_or_reset(dev, devm_adi_axi_adc_conv_release,
+				       conv);
+	if (ret)
+		return ERR_PTR(ret);
 
 	return conv;
 }
-- 
2.8.1

[PATCH 2/7] iio: buffer-dmaengine: simplify __devm_iio_dmaengine_buffer_free

[Yicong Yang]

Use devm_add_action_or_reset() instead of devres_alloc() and
devres_add(), which works the same. This will simplify the
code. There is no functional changes.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/iio/buffer/industrialio-buffer-dmaengine.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
index b0cb9a3..9ecc03f 100644
--- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
+++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
@@ -228,9 +228,9 @@ static void iio_dmaengine_buffer_free(struct iio_buffer *buffer)
 	iio_buffer_put(buffer);
 }
 
-static void __devm_iio_dmaengine_buffer_free(struct device *dev, void *res)
+static void __devm_iio_dmaengine_buffer_free(void *buffer)
 {
-	iio_dmaengine_buffer_free(*(struct iio_buffer **)res);
+	iio_dmaengine_buffer_free(buffer);
 }
 
 /**
@@ -247,21 +247,17 @@ static void __devm_iio_dmaengine_buffer_free(struct device *dev, void *res)
 struct iio_buffer *devm_iio_dmaengine_buffer_alloc(struct device *dev,
 	const char *channel)
 {
-	struct iio_buffer **bufferp, *buffer;
-
-	bufferp = devres_alloc(__devm_iio_dmaengine_buffer_free,
-			       sizeof(*bufferp), GFP_KERNEL);
-	if (!bufferp)
-		return ERR_PTR(-ENOMEM);
+	struct iio_buffer *buffer;
+	int ret;
 
 	buffer = iio_dmaengine_buffer_alloc(dev, channel);
-	if (IS_ERR(buffer)) {
-		devres_free(bufferp);
+	if (IS_ERR(buffer))
 		return buffer;
-	}
 
-	*bufferp = buffer;
-	devres_add(dev, bufferp);
+	ret = devm_add_action_or_reset(dev, __devm_iio_dmaengine_buffer_free,
+				       buffer);
+	if (ret)
+		return ERR_PTR(ret);
 
 	return buffer;
 }
-- 
2.8.1

[PATCH 3/7] iio: hw_consumer: simplify devm_iio_hw_consumer_alloc

[Yicong Yang]

Use devm_add_action_or_reset() instead of devres_alloc() and
devres_add(), which works the same. This will simplify the
code. There is no functional changes.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/iio/buffer/industrialio-hw-consumer.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-hw-consumer.c b/drivers/iio/buffer/industrialio-hw-consumer.c
index f2d2778..87d9aab 100644
--- a/drivers/iio/buffer/industrialio-hw-consumer.c
+++ b/drivers/iio/buffer/industrialio-hw-consumer.c
@@ -137,9 +137,9 @@ void iio_hw_consumer_free(struct iio_hw_consumer *hwc)
 }
 EXPORT_SYMBOL_GPL(iio_hw_consumer_free);
 
-static void devm_iio_hw_consumer_release(struct device *dev, void *res)
+static void devm_iio_hw_consumer_release(void *iio_hwc)
 {
-	iio_hw_consumer_free(*(struct iio_hw_consumer **)res);
+	iio_hw_consumer_free(iio_hwc);
 }
 
 /**
@@ -153,20 +153,17 @@ static void devm_iio_hw_consumer_release(struct device *dev, void *res)
  */
 struct iio_hw_consumer *devm_iio_hw_consumer_alloc(struct device *dev)
 {
-	struct iio_hw_consumer **ptr, *iio_hwc;
-
-	ptr = devres_alloc(devm_iio_hw_consumer_release, sizeof(*ptr),
-			   GFP_KERNEL);
-	if (!ptr)
-		return NULL;
+	struct iio_hw_consumer *iio_hwc;
+	int ret;
 
 	iio_hwc = iio_hw_consumer_alloc(dev);
-	if (IS_ERR(iio_hwc)) {
-		devres_free(ptr);
-	} else {
-		*ptr = iio_hwc;
-		devres_add(dev, ptr);
-	}
+	if (IS_ERR(iio_hwc))
+		return iio_hwc;
+
+	ret = devm_add_action_or_reset(dev, devm_iio_hw_consumer_release,
+				       iio_hwc);
+	if (ret)
+		return ERR_PTR(ret);
 
 	return iio_hwc;
 }
-- 
2.8.1

[PATCH 4/7] iio: triggered-buffer: simplify devm_iio_triggered_buffer_setup_ext

[Yicong Yang]

Use devm_add_action_or_reset() instead of devres_alloc() and
devres_add(), which works the same. This will simplify the
code. There is no functional changes.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/iio/buffer/industrialio-triggered-buffer.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-triggered-buffer.c b/drivers/iio/buffer/industrialio-triggered-buffer.c
index 92b8aea..63b4722 100644
--- a/drivers/iio/buffer/industrialio-triggered-buffer.c
+++ b/drivers/iio/buffer/industrialio-triggered-buffer.c
@@ -92,9 +92,9 @@ void iio_triggered_buffer_cleanup(struct iio_dev *indio_dev)
 }
 EXPORT_SYMBOL(iio_triggered_buffer_cleanup);
 
-static void devm_iio_triggered_buffer_clean(struct device *dev, void *res)
+static void devm_iio_triggered_buffer_clean(void *indio_dev)
 {
-	iio_triggered_buffer_cleanup(*(struct iio_dev **)res);
+	iio_triggered_buffer_cleanup(indio_dev);
 }
 
 int devm_iio_triggered_buffer_setup_ext(struct device *dev,
@@ -104,24 +104,15 @@ int devm_iio_triggered_buffer_setup_ext(struct device *dev,
 					const struct iio_buffer_setup_ops *ops,
 					const struct attribute **buffer_attrs)
 {
-	struct iio_dev **ptr;
 	int ret;
 
-	ptr = devres_alloc(devm_iio_triggered_buffer_clean, sizeof(*ptr),
-			   GFP_KERNEL);
-	if (!ptr)
-		return -ENOMEM;
-
-	*ptr = indio_dev;
-
 	ret = iio_triggered_buffer_setup_ext(indio_dev, h, thread, ops,
 					     buffer_attrs);
 	if (!ret)
-		devres_add(dev, ptr);
-	else
-		devres_free(ptr);
+		return ret;
 
-	return ret;
+	return devm_add_action_or_reset(dev, devm_iio_triggered_buffer_clean,
+					indio_dev);
 }
 EXPORT_SYMBOL_GPL(devm_iio_triggered_buffer_setup_ext);
 
-- 
2.8.1

[PATCH 5/7] iio: core: simplify some devm functions

[Yicong Yang]

Use devm_add_action_or_reset() instead of devres_alloc() and
devres_add(), which works the same. This will simplify the
code. There is no functional changes.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/iio/industrialio-core.c | 43 +++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 27 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 7db761a..2dfbed3 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1627,9 +1627,9 @@ void iio_device_free(struct iio_dev *dev)
 }
 EXPORT_SYMBOL(iio_device_free);
 
-static void devm_iio_device_release(struct device *dev, void *res)
+static void devm_iio_device_release(void *iio_dev)
 {
-	iio_device_free(*(struct iio_dev **)res);
+	iio_device_free(iio_dev);
 }
 
 /**
@@ -1645,20 +1645,17 @@ static void devm_iio_device_release(struct device *dev, void *res)
  */
 struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv)
 {
-	struct iio_dev **ptr, *iio_dev;
-
-	ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr),
-			   GFP_KERNEL);
-	if (!ptr)
-		return NULL;
+	struct iio_dev *iio_dev;
+	int ret;
 
 	iio_dev = iio_device_alloc(parent, sizeof_priv);
-	if (iio_dev) {
-		*ptr = iio_dev;
-		devres_add(parent, ptr);
-	} else {
-		devres_free(ptr);
-	}
+	if (!iio_dev)
+		return iio_dev;
+
+	ret = devm_add_action_or_reset(parent, devm_iio_device_release,
+				       iio_dev);
+	if (ret)
+		return ERR_PTR(ret);
 
 	return iio_dev;
 }
@@ -1889,29 +1886,21 @@ void iio_device_unregister(struct iio_dev *indio_dev)
 }
 EXPORT_SYMBOL(iio_device_unregister);
 
-static void devm_iio_device_unreg(struct device *dev, void *res)
+static void devm_iio_device_unreg(void *indio_dev)
 {
-	iio_device_unregister(*(struct iio_dev **)res);
+	iio_device_unregister(indio_dev);
 }
 
 int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
 			       struct module *this_mod)
 {
-	struct iio_dev **ptr;
 	int ret;
 
-	ptr = devres_alloc(devm_iio_device_unreg, sizeof(*ptr), GFP_KERNEL);
-	if (!ptr)
-		return -ENOMEM;
-
-	*ptr = indio_dev;
 	ret = __iio_device_register(indio_dev, this_mod);
-	if (!ret)
-		devres_add(dev, ptr);
-	else
-		devres_free(ptr);
+	if (ret)
+		return ret;
 
-	return ret;
+	return devm_add_action_or_reset(dev, devm_iio_device_unreg, indio_dev);
 }
 EXPORT_SYMBOL_GPL(__devm_iio_device_register);
 
-- 
2.8.1

[PATCH 6/7] iio: trigger: simplify __devm_iio_trigger_register

[Yicong Yang]

Use devm_add_action_or_reset() instead of devres_alloc() and
devres_add(), which works the same. This will simplify the
code. There is no functional changes.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/iio/industrialio-trigger.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index ea3c985..0db0952 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -624,9 +624,9 @@ struct iio_trigger *devm_iio_trigger_alloc(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_iio_trigger_alloc);
 
-static void devm_iio_trigger_unreg(struct device *dev, void *res)
+static void devm_iio_trigger_unreg(void *trigger_info)
 {
-	iio_trigger_unregister(*(struct iio_trigger **)res);
+	iio_trigger_unregister(trigger_info);
 }
 
 /**
@@ -647,21 +647,13 @@ int __devm_iio_trigger_register(struct device *dev,
 				struct iio_trigger *trig_info,
 				struct module *this_mod)
 {
-	struct iio_trigger **ptr;
 	int ret;
 
-	ptr = devres_alloc(devm_iio_trigger_unreg, sizeof(*ptr), GFP_KERNEL);
-	if (!ptr)
-		return -ENOMEM;
-
-	*ptr = trig_info;
 	ret = __iio_trigger_register(trig_info, this_mod);
-	if (!ret)
-		devres_add(dev, ptr);
-	else
-		devres_free(ptr);
+	if (ret)
+		return ret;
 
-	return ret;
+	return devm_add_action_or_reset(dev, devm_iio_trigger_unreg, trig_info);
 }
 EXPORT_SYMBOL_GPL(__devm_iio_trigger_register);
 
-- 
2.8.1

[PATCH 7/7] iio: inkern: simplify some devm functions

[Yicong Yang]

Use devm_add_action_or_reset() instead of devres_alloc() and
devres_add(), which works the same. This will simplify the
code. There is no functional changes.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/iio/inkern.c | 61 ++++++++++++++++++++--------------------------------
 1 file changed, 23 insertions(+), 38 deletions(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index db77a2d..5de35cc 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -359,30 +359,24 @@ void iio_channel_release(struct iio_channel *channel)
 }
 EXPORT_SYMBOL_GPL(iio_channel_release);
 
-static void devm_iio_channel_free(struct device *dev, void *res)
+static void devm_iio_channel_free(void *iio_channel)
 {
-	struct iio_channel *channel = *(struct iio_channel **)res;
-
-	iio_channel_release(channel);
+	iio_channel_release(iio_channel);
 }
 
 struct iio_channel *devm_iio_channel_get(struct device *dev,
 					 const char *channel_name)
 {
-	struct iio_channel **ptr, *channel;
-
-	ptr = devres_alloc(devm_iio_channel_free, sizeof(*ptr), GFP_KERNEL);
-	if (!ptr)
-		return ERR_PTR(-ENOMEM);
+	struct iio_channel *channel;
+	int ret;
 
 	channel = iio_channel_get(dev, channel_name);
-	if (IS_ERR(channel)) {
-		devres_free(ptr);
+	if (IS_ERR(channel))
 		return channel;
-	}
 
-	*ptr = channel;
-	devres_add(dev, ptr);
+	ret = devm_add_action_or_reset(dev, devm_iio_channel_free, channel);
+	if (ret)
+		return ERR_PTR(ret);
 
 	return channel;
 }
@@ -392,20 +386,16 @@ struct iio_channel *devm_of_iio_channel_get_by_name(struct device *dev,
 						    struct device_node *np,
 						    const char *channel_name)
 {
-	struct iio_channel **ptr, *channel;
-
-	ptr = devres_alloc(devm_iio_channel_free, sizeof(*ptr), GFP_KERNEL);
-	if (!ptr)
-		return ERR_PTR(-ENOMEM);
+	struct iio_channel *channel;
+	int ret;
 
 	channel = of_iio_channel_get_by_name(np, channel_name);
-	if (IS_ERR(channel)) {
-		devres_free(ptr);
+	if (IS_ERR(channel))
 		return channel;
-	}
 
-	*ptr = channel;
-	devres_add(dev, ptr);
+	ret = devm_add_action_or_reset(dev, devm_iio_channel_free, channel);
+	if (ret)
+		return ERR_PTR(ret);
 
 	return channel;
 }
@@ -496,29 +486,24 @@ void iio_channel_release_all(struct iio_channel *channels)
 }
 EXPORT_SYMBOL_GPL(iio_channel_release_all);
 
-static void devm_iio_channel_free_all(struct device *dev, void *res)
+static void devm_iio_channel_free_all(void *iio_channels)
 {
-	struct iio_channel *channels = *(struct iio_channel **)res;
-
-	iio_channel_release_all(channels);
+	iio_channel_release_all(iio_channels);
 }
 
 struct iio_channel *devm_iio_channel_get_all(struct device *dev)
 {
-	struct iio_channel **ptr, *channels;
-
-	ptr = devres_alloc(devm_iio_channel_free_all, sizeof(*ptr), GFP_KERNEL);
-	if (!ptr)
-		return ERR_PTR(-ENOMEM);
+	struct iio_channel *channels;
+	int ret;
 
 	channels = iio_channel_get_all(dev);
-	if (IS_ERR(channels)) {
-		devres_free(ptr);
+	if (IS_ERR(channels))
 		return channels;
-	}
 
-	*ptr = channels;
-	devres_add(dev, ptr);
+	ret = devm_add_action_or_reset(dev, devm_iio_channel_free_all,
+				       channels);
+	if (ret)
+		return ERR_PTR(ret);
 
 	return channels;
 }
-- 
2.8.1

Re: [PATCH 1/7] iio: adc: adi-axi-adc: simplify devm_adi_axi_adc_conv_register

[Alexandru Ardelean]

On Thu, Apr 8, 2021 at 2:42 PM Yicong Yang <yangyicong@hisilicon.com> wrote:
>
> Use devm_add_action_or_reset() instead of devres_alloc() and
> devres_add(), which works the same. This will simplify the
> code. There is no functional changes.
>

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>

> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/iio/adc/adi-axi-adc.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
> index 9109da2..575a63f 100644
> --- a/drivers/iio/adc/adi-axi-adc.c
> +++ b/drivers/iio/adc/adi-axi-adc.c
> @@ -210,29 +210,25 @@ static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv)
>         kfree(cl);
>  }
>
> -static void devm_adi_axi_adc_conv_release(struct device *dev, void *res)
> +static void devm_adi_axi_adc_conv_release(void *conv)
>  {
> -       adi_axi_adc_conv_unregister(*(struct adi_axi_adc_conv **)res);
> +       adi_axi_adc_conv_unregister(conv);
>  }
>
>  struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev,
>                                                         size_t sizeof_priv)
>  {
> -       struct adi_axi_adc_conv **ptr, *conv;
> -
> -       ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr),
> -                          GFP_KERNEL);
> -       if (!ptr)
> -               return ERR_PTR(-ENOMEM);
> +       struct adi_axi_adc_conv *conv;
> +       int ret;
>
>         conv = adi_axi_adc_conv_register(dev, sizeof_priv);
> -       if (IS_ERR(conv)) {
> -               devres_free(ptr);
> +       if (IS_ERR(conv))
>                 return ERR_CAST(conv);
> -       }
>
> -       *ptr = conv;
> -       devres_add(dev, ptr);
> +       ret = devm_add_action_or_reset(dev, devm_adi_axi_adc_conv_release,
> +                                      conv);
> +       if (ret)
> +               return ERR_PTR(ret);
>
>         return conv;
>  }
> --
> 2.8.1
>

Re: [PATCH 1/7] iio: adc: adi-axi-adc: simplify devm_adi_axi_adc_conv_register

[Alexandru Ardelean]

On Thu, Apr 8, 2021 at 2:42 PM Yicong Yang <yangyicong@hisilicon.com> wrote:
>
> Use devm_add_action_or_reset() instead of devres_alloc() and
> devres_add(), which works the same. This will simplify the
> code. There is no functional changes.
>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/iio/adc/adi-axi-adc.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
> index 9109da2..575a63f 100644
> --- a/drivers/iio/adc/adi-axi-adc.c
> +++ b/drivers/iio/adc/adi-axi-adc.c
> @@ -210,29 +210,25 @@ static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv)
>         kfree(cl);
>  }
>
> -static void devm_adi_axi_adc_conv_release(struct device *dev, void *res)
> +static void devm_adi_axi_adc_conv_release(void *conv)
>  {
> -       adi_axi_adc_conv_unregister(*(struct adi_axi_adc_conv **)res);

On a second pass, I'm wondering if this requires a cast like

static void devm_adi_axi_adc_conv_release(void *data)
{
       struct adi_axi_adc_conv *conv = data;

If the compiler doesn't complain, that I'm fine

> +       adi_axi_adc_conv_unregister(conv);
>  }
>
>  struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev,
>                                                         size_t sizeof_priv)
>  {
> -       struct adi_axi_adc_conv **ptr, *conv;
> -
> -       ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr),
> -                          GFP_KERNEL);
> -       if (!ptr)
> -               return ERR_PTR(-ENOMEM);
> +       struct adi_axi_adc_conv *conv;
> +       int ret;
>
>         conv = adi_axi_adc_conv_register(dev, sizeof_priv);
> -       if (IS_ERR(conv)) {
> -               devres_free(ptr);
> +       if (IS_ERR(conv))
>                 return ERR_CAST(conv);
> -       }
>
> -       *ptr = conv;
> -       devres_add(dev, ptr);
> +       ret = devm_add_action_or_reset(dev, devm_adi_axi_adc_conv_release,
> +                                      conv);
> +       if (ret)
> +               return ERR_PTR(ret);
>
>         return conv;
>  }
> --
> 2.8.1
>

Re: [PATCH 5/7] iio: core: simplify some devm functions

[Alexandru Ardelean]

On Thu, Apr 8, 2021 at 2:41 PM Yicong Yang <yangyicong@hisilicon.com> wrote:
>
> Use devm_add_action_or_reset() instead of devres_alloc() and
> devres_add(), which works the same. This will simplify the
> code. There is no functional changes.
>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/iio/industrialio-core.c | 43 +++++++++++++++--------------------------
>  1 file changed, 16 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 7db761a..2dfbed3 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1627,9 +1627,9 @@ void iio_device_free(struct iio_dev *dev)
>  }
>  EXPORT_SYMBOL(iio_device_free);
>
> -static void devm_iio_device_release(struct device *dev, void *res)
> +static void devm_iio_device_release(void *iio_dev)
>  {
> -       iio_device_free(*(struct iio_dev **)res);
> +       iio_device_free(iio_dev);
>  }
>
>  /**
> @@ -1645,20 +1645,17 @@ static void devm_iio_device_release(struct device *dev, void *res)
>   */
>  struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv)
>  {
> -       struct iio_dev **ptr, *iio_dev;
> -
> -       ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr),
> -                          GFP_KERNEL);
> -       if (!ptr)
> -               return NULL;
> +       struct iio_dev *iio_dev;
> +       int ret;
>
>         iio_dev = iio_device_alloc(parent, sizeof_priv);
> -       if (iio_dev) {
> -               *ptr = iio_dev;
> -               devres_add(parent, ptr);
> -       } else {
> -               devres_free(ptr);
> -       }
> +       if (!iio_dev)
> +               return iio_dev;

This is correct.
But the preference is usually:
        if (!iio_dev)
            return NULL;

> +
> +       ret = devm_add_action_or_reset(parent, devm_iio_device_release,
> +                                      iio_dev);
> +       if (ret)
> +               return ERR_PTR(ret);
>
>         return iio_dev;
>  }
> @@ -1889,29 +1886,21 @@ void iio_device_unregister(struct iio_dev *indio_dev)
>  }
>  EXPORT_SYMBOL(iio_device_unregister);
>
> -static void devm_iio_device_unreg(struct device *dev, void *res)
> +static void devm_iio_device_unreg(void *indio_dev)
>  {
> -       iio_device_unregister(*(struct iio_dev **)res);
> +       iio_device_unregister(indio_dev);
>  }
>
>  int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
>                                struct module *this_mod)
>  {
> -       struct iio_dev **ptr;
>         int ret;
>
> -       ptr = devres_alloc(devm_iio_device_unreg, sizeof(*ptr), GFP_KERNEL);
> -       if (!ptr)
> -               return -ENOMEM;
> -
> -       *ptr = indio_dev;
>         ret = __iio_device_register(indio_dev, this_mod);
> -       if (!ret)
> -               devres_add(dev, ptr);
> -       else
> -               devres_free(ptr);
> +       if (ret)
> +               return ret;
>
> -       return ret;
> +       return devm_add_action_or_reset(dev, devm_iio_device_unreg, indio_dev);
>  }
>  EXPORT_SYMBOL_GPL(__devm_iio_device_register);
>
> --
> 2.8.1
>

Re: [PATCH 0/7] Simplify codes with devm_add_action_or_reset

[Alexandru Ardelean]

On Thu, Apr 8, 2021 at 2:41 PM Yicong Yang <yangyicong@hisilicon.com> wrote:
>
> Some devm variants are implemented with devres_alloc() and devres_add()
> manually to only manage a single pointer. This can be simplified with
> devm_add_action_or_reset() which works the same. Simplify these functions.
>

Series looks good.
There are a few elements that are a bit strange (from a habit perspective).

For the devm_add_action_or_reset() callback usually there's a cast being done.
But that's a preference I don't feel too strongly about.

> Yicong Yang (7):
>   iio: adc: adi-axi-adc: simplify devm_adi_axi_adc_conv_register
>   iio: buffer-dmaengine: simplify __devm_iio_dmaengine_buffer_free
>   iio: hw_consumer: simplify devm_iio_hw_consumer_alloc
>   iio: triggered-buffer: simplify devm_iio_triggered_buffer_setup_ext
>   iio: core: simplify some devm functions
>   iio: trigger: simplify __devm_iio_trigger_register
>   iio: inkern: simplify some devm functions
>
>  drivers/iio/adc/adi-axi-adc.c                      | 22 ++++----
>  drivers/iio/buffer/industrialio-buffer-dmaengine.c | 22 ++++----
>  drivers/iio/buffer/industrialio-hw-consumer.c      | 25 ++++-----
>  drivers/iio/buffer/industrialio-triggered-buffer.c | 19 ++-----
>  drivers/iio/industrialio-core.c                    | 43 ++++++---------
>  drivers/iio/industrialio-trigger.c                 | 18 ++-----
>  drivers/iio/inkern.c                               | 61 ++++++++--------------
>  7 files changed, 78 insertions(+), 132 deletions(-)
>
> --
> 2.8.1
>

Re: [PATCH 0/7] Simplify codes with devm_add_action_or_reset

[Lars-Peter Clausen]

On 4/8/21 1:38 PM, Yicong Yang wrote:
> Some devm variants are implemented with devres_alloc() and devres_add()
> manually to only manage a single pointer. This can be simplified with
> devm_add_action_or_reset() which works the same. Simplify these functions.
>
Whole series looks good to me, thanks.

Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>

Re: [PATCH 5/7] iio: core: simplify some devm functions

[Yicong Yang]

On 2021/4/8 21:09, Alexandru Ardelean wrote:
> On Thu, Apr 8, 2021 at 2:41 PM Yicong Yang <yangyicong@hisilicon.com> wrote:
>>
>> Use devm_add_action_or_reset() instead of devres_alloc() and
>> devres_add(), which works the same. This will simplify the
>> code. There is no functional changes.
>>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> ---
>>  drivers/iio/industrialio-core.c | 43 +++++++++++++++--------------------------
>>  1 file changed, 16 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
>> index 7db761a..2dfbed3 100644
>> --- a/drivers/iio/industrialio-core.c
>> +++ b/drivers/iio/industrialio-core.c
>> @@ -1627,9 +1627,9 @@ void iio_device_free(struct iio_dev *dev)
>>  }
>>  EXPORT_SYMBOL(iio_device_free);
>>
>> -static void devm_iio_device_release(struct device *dev, void *res)
>> +static void devm_iio_device_release(void *iio_dev)
>>  {
>> -       iio_device_free(*(struct iio_dev **)res);
>> +       iio_device_free(iio_dev);
>>  }
>>
>>  /**
>> @@ -1645,20 +1645,17 @@ static void devm_iio_device_release(struct device *dev, void *res)
>>   */
>>  struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv)
>>  {
>> -       struct iio_dev **ptr, *iio_dev;
>> -
>> -       ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr),
>> -                          GFP_KERNEL);
>> -       if (!ptr)
>> -               return NULL;
>> +       struct iio_dev *iio_dev;
>> +       int ret;
>>
>>         iio_dev = iio_device_alloc(parent, sizeof_priv);
>> -       if (iio_dev) {
>> -               *ptr = iio_dev;
>> -               devres_add(parent, ptr);
>> -       } else {
>> -               devres_free(ptr);
>> -       }
>> +       if (!iio_dev)
>> +               return iio_dev;
> 
> This is correct.
> But the preference is usually:
>         if (!iio_dev)
>             return NULL;
> 

since it returned iio_dev when failure before, so i just keep as is

>> +
>> +       ret = devm_add_action_or_reset(parent, devm_iio_device_release,
>> +                                      iio_dev);
>> +       if (ret)
>> +               return ERR_PTR(ret);
>>
>>         return iio_dev;
>>  }
>> @@ -1889,29 +1886,21 @@ void iio_device_unregister(struct iio_dev *indio_dev)
>>  }
>>  EXPORT_SYMBOL(iio_device_unregister);
>>
>> -static void devm_iio_device_unreg(struct device *dev, void *res)
>> +static void devm_iio_device_unreg(void *indio_dev)
>>  {
>> -       iio_device_unregister(*(struct iio_dev **)res);
>> +       iio_device_unregister(indio_dev);
>>  }
>>
>>  int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
>>                                struct module *this_mod)
>>  {
>> -       struct iio_dev **ptr;
>>         int ret;
>>
>> -       ptr = devres_alloc(devm_iio_device_unreg, sizeof(*ptr), GFP_KERNEL);
>> -       if (!ptr)
>> -               return -ENOMEM;
>> -
>> -       *ptr = indio_dev;
>>         ret = __iio_device_register(indio_dev, this_mod);
>> -       if (!ret)
>> -               devres_add(dev, ptr);
>> -       else
>> -               devres_free(ptr);
>> +       if (ret)
>> +               return ret;
>>
>> -       return ret;
>> +       return devm_add_action_or_reset(dev, devm_iio_device_unreg, indio_dev);
>>  }
>>  EXPORT_SYMBOL_GPL(__devm_iio_device_register);
>>
>> --
>> 2.8.1
>>
> 
> .
> 

RE: [PATCH 0/7] Simplify codes with devm_add_action_or_reset

[Sa, Nuno]

> -----Original Message-----
> From: Yicong Yang <yangyicong@hisilicon.com>
> Sent: Thursday, April 8, 2021 1:38 PM
> To: jic23@kernel.org; linux-iio@vger.kernel.org
> Cc: lars@metafoo.de; Hennerich, Michael
> <Michael.Hennerich@analog.com>; pmeerw@pmeerw.net;
> prime.zeng@huawei.com; tiantao6@hisilicon.com;
> yangyicong@hisilicon.com
> Subject: [PATCH 0/7] Simplify codes with devm_add_action_or_reset
> 
> 
> Some devm variants are implemented with devres_alloc() and
> devres_add()
> manually to only manage a single pointer. This can be simplified with
> devm_add_action_or_reset() which works the same. Simplify these
> functions.
> 

LGTM,

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

Nuno Sá

Re: [PATCH 1/7] iio: adc: adi-axi-adc: simplify devm_adi_axi_adc_conv_register

[Yicong Yang]

On 2021/4/8 21:04, Alexandru Ardelean wrote:
> On Thu, Apr 8, 2021 at 2:42 PM Yicong Yang <yangyicong@hisilicon.com> wrote:
>>
>> Use devm_add_action_or_reset() instead of devres_alloc() and
>> devres_add(), which works the same. This will simplify the
>> code. There is no functional changes.
>>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> ---
>>  drivers/iio/adc/adi-axi-adc.c | 22 +++++++++-------------
>>  1 file changed, 9 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
>> index 9109da2..575a63f 100644
>> --- a/drivers/iio/adc/adi-axi-adc.c
>> +++ b/drivers/iio/adc/adi-axi-adc.c
>> @@ -210,29 +210,25 @@ static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv)
>>         kfree(cl);
>>  }
>>
>> -static void devm_adi_axi_adc_conv_release(struct device *dev, void *res)
>> +static void devm_adi_axi_adc_conv_release(void *conv)
>>  {
>> -       adi_axi_adc_conv_unregister(*(struct adi_axi_adc_conv **)res);
> 
> On a second pass, I'm wondering if this requires a cast like
> 
> static void devm_adi_axi_adc_conv_release(void *data)
> {
>        struct adi_axi_adc_conv *conv = data;
> 
> If the compiler doesn't complain, that I'm fine
> 

no warn noticed when make W=1.

>> +       adi_axi_adc_conv_unregister(conv);
>>  }
>>
>>  struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev,
>>                                                         size_t sizeof_priv)
>>  {
>> -       struct adi_axi_adc_conv **ptr, *conv;
>> -
>> -       ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr),
>> -                          GFP_KERNEL);
>> -       if (!ptr)
>> -               return ERR_PTR(-ENOMEM);
>> +       struct adi_axi_adc_conv *conv;
>> +       int ret;
>>
>>         conv = adi_axi_adc_conv_register(dev, sizeof_priv);
>> -       if (IS_ERR(conv)) {
>> -               devres_free(ptr);
>> +       if (IS_ERR(conv))
>>                 return ERR_CAST(conv);
>> -       }
>>
>> -       *ptr = conv;
>> -       devres_add(dev, ptr);
>> +       ret = devm_add_action_or_reset(dev, devm_adi_axi_adc_conv_release,
>> +                                      conv);
>> +       if (ret)
>> +               return ERR_PTR(ret);
>>
>>         return conv;
>>  }
>> --
>> 2.8.1
>>
> 
> .
> 

Re: [PATCH 0/7] Simplify codes with devm_add_action_or_reset

[Andy Shevchenko]

On Thu, Apr 8, 2021 at 2:41 PM Yicong Yang <yangyicong@hisilicon.com> wrote:
>
> Some devm variants are implemented with devres_alloc() and devres_add()
> manually to only manage a single pointer. This can be simplified with
> devm_add_action_or_reset() which works the same. Simplify these functions.

Thanks, I saw similar patches you have done for other subsystems, this
also LGTM!
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Yicong Yang (7):
>   iio: adc: adi-axi-adc: simplify devm_adi_axi_adc_conv_register
>   iio: buffer-dmaengine: simplify __devm_iio_dmaengine_buffer_free
>   iio: hw_consumer: simplify devm_iio_hw_consumer_alloc
>   iio: triggered-buffer: simplify devm_iio_triggered_buffer_setup_ext
>   iio: core: simplify some devm functions
>   iio: trigger: simplify __devm_iio_trigger_register
>   iio: inkern: simplify some devm functions
>
>  drivers/iio/adc/adi-axi-adc.c                      | 22 ++++----
>  drivers/iio/buffer/industrialio-buffer-dmaengine.c | 22 ++++----
>  drivers/iio/buffer/industrialio-hw-consumer.c      | 25 ++++-----
>  drivers/iio/buffer/industrialio-triggered-buffer.c | 19 ++-----
>  drivers/iio/industrialio-core.c                    | 43 ++++++---------
>  drivers/iio/industrialio-trigger.c                 | 18 ++-----
>  drivers/iio/inkern.c                               | 61 ++++++++--------------
>  7 files changed, 78 insertions(+), 132 deletions(-)
>
> --
> 2.8.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/7] iio: core: simplify some devm functions

[Andy Shevchenko]

On Fri, Apr 9, 2021 at 10:22 AM Yicong Yang <yangyicong@hisilicon.com> wrote:
> On 2021/4/8 21:09, Alexandru Ardelean wrote:
> > On Thu, Apr 8, 2021 at 2:41 PM Yicong Yang <yangyicong@hisilicon.com> wrote:

...

> >> -       if (!ptr)
> >> -               return NULL;

> >> +       if (!iio_dev)
> >> +               return iio_dev;
> >
> > This is correct.
> > But the preference is usually:
> >         if (!iio_dev)
> >             return NULL;
> >
>
> since it returned iio_dev when failure before, so i just keep as is

Actually Alexandru has a good point. Compare the pieces I left above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/7] iio: core: simplify some devm functions

[Yicong Yang]

On 2021/4/9 17:19, Andy Shevchenko wrote:
> On Fri, Apr 9, 2021 at 10:22 AM Yicong Yang <yangyicong@hisilicon.com> wrote:
>> On 2021/4/8 21:09, Alexandru Ardelean wrote:
>>> On Thu, Apr 8, 2021 at 2:41 PM Yicong Yang <yangyicong@hisilicon.com> wrote:
> 
> ...
> 
>>>> -       if (!ptr)
>>>> -               return NULL;
> 
>>>> +       if (!iio_dev)
>>>> +               return iio_dev;
>>>
>>> This is correct.
>>> But the preference is usually:
>>>         if (!iio_dev)
>>>             return NULL;
>>>
>>
>> since it returned iio_dev when failure before, so i just keep as is
> 
> Actually Alexandru has a good point. Compare the pieces I left above.
> 

a little different from my perspective when I did this.

previously:

-       ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr),
-                          GFP_KERNEL);
-       if (!ptr)
-               return NULL;

if devres_alloc() failure we return NULL.


        iio_dev = iio_device_alloc(parent, sizeof_priv);
-       if (iio_dev) {
-               *ptr = iio_dev;
-               devres_add(parent, ptr);
-       } else {
-               devres_free(ptr);
-       }

        return iio_dev;

if iio_device_alloc() failed, we return what it returns.

now we remove devres_alloc() and stay iio_device_alloc(), so I just
keep to return iio_dev.

but actually iio_device_alloc() will return NULL at failure. :)
so return NULL here is definitely correct. i'll change to that if
it's recommended.

thanks,
Yicong

Re: [PATCH 5/7] iio: core: simplify some devm functions

[Andy Shevchenko]

On Fri, Apr 9, 2021 at 12:42 PM Yicong Yang <yangyicong@hisilicon.com> wrote:
> On 2021/4/9 17:19, Andy Shevchenko wrote:
> > On Fri, Apr 9, 2021 at 10:22 AM Yicong Yang <yangyicong@hisilicon.com> wrote:
> >> On 2021/4/8 21:09, Alexandru Ardelean wrote:
> >>> On Thu, Apr 8, 2021 at 2:41 PM Yicong Yang <yangyicong@hisilicon.com> wrote:

...

> > Actually Alexandru has a good point. Compare the pieces I left above.
> >
>
> a little different from my perspective when I did this.
>
> previously:
>
> -       ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr),
> -                          GFP_KERNEL);
> -       if (!ptr)
> -               return NULL;
>
> if devres_alloc() failure we return NULL.
>
>
>         iio_dev = iio_device_alloc(parent, sizeof_priv);
> -       if (iio_dev) {
> -               *ptr = iio_dev;
> -               devres_add(parent, ptr);
> -       } else {
> -               devres_free(ptr);
> -       }
>
>         return iio_dev;
>
> if iio_device_alloc() failed, we return what it returns.
>
> now we remove devres_alloc() and stay iio_device_alloc(), so I just
> keep to return iio_dev.
>
> but actually iio_device_alloc() will return NULL at failure. :)
> so return NULL here is definitely correct. i'll change to that if
> it's recommended.

Confusing here (in your case) is that:

if (!FOO)
 return BAR;

...

return BAR;

I.e. twice returning BAR for different occasions. This makes
additional cognitive work and decrease readability / maintainability
of the code.

I.o.w., yes, it's preferred.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/7] iio: core: simplify some devm functions

[Yicong Yang]

On 2021/4/9 17:55, Andy Shevchenko wrote:
> On Fri, Apr 9, 2021 at 12:42 PM Yicong Yang <yangyicong@hisilicon.com> wrote:
>> On 2021/4/9 17:19, Andy Shevchenko wrote:
>>> On Fri, Apr 9, 2021 at 10:22 AM Yicong Yang <yangyicong@hisilicon.com> wrote:
>>>> On 2021/4/8 21:09, Alexandru Ardelean wrote:
>>>>> On Thu, Apr 8, 2021 at 2:41 PM Yicong Yang <yangyicong@hisilicon.com> wrote:
> 
> ...
> 
>>> Actually Alexandru has a good point. Compare the pieces I left above.
>>>
>>
>> a little different from my perspective when I did this.
>>
>> previously:
>>
>> -       ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr),
>> -                          GFP_KERNEL);
>> -       if (!ptr)
>> -               return NULL;
>>
>> if devres_alloc() failure we return NULL.
>>
>>
>>         iio_dev = iio_device_alloc(parent, sizeof_priv);
>> -       if (iio_dev) {
>> -               *ptr = iio_dev;
>> -               devres_add(parent, ptr);
>> -       } else {
>> -               devres_free(ptr);
>> -       }
>>
>>         return iio_dev;
>>
>> if iio_device_alloc() failed, we return what it returns.
>>
>> now we remove devres_alloc() and stay iio_device_alloc(), so I just
>> keep to return iio_dev.
>>
>> but actually iio_device_alloc() will return NULL at failure. :)
>> so return NULL here is definitely correct. i'll change to that if
>> it's recommended.
> 
> Confusing here (in your case) is that:
> 
> if (!FOO)
>  return BAR;
> 
> ...
> 
> return BAR;
> 
> I.e. twice returning BAR for different occasions. This makes
> additional cognitive work and decrease readability / maintainability
> of the code.
> 
> I.o.w., yes, it's preferred.
> 

got it. will return NULL and will check if other patches in this series
met the same condition.

thanks.

Re: [PATCH 1/7] iio: adc: adi-axi-adc: simplify devm_adi_axi_adc_conv_register

[Jonathan Cameron]

On Thu, 8 Apr 2021 19:38:10 +0800
Yicong Yang <yangyicong@hisilicon.com> wrote:

> Use devm_add_action_or_reset() instead of devres_alloc() and
> devres_add(), which works the same. This will simplify the
> code. There is no functional changes.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/iio/adc/adi-axi-adc.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
> index 9109da2..575a63f 100644
> --- a/drivers/iio/adc/adi-axi-adc.c
> +++ b/drivers/iio/adc/adi-axi-adc.c
> @@ -210,29 +210,25 @@ static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv)
>  	kfree(cl);
>  }
>  
> -static void devm_adi_axi_adc_conv_release(struct device *dev, void *res)
> +static void devm_adi_axi_adc_conv_release(void *conv)
>  {
> -	adi_axi_adc_conv_unregister(*(struct adi_axi_adc_conv **)res);
> +	adi_axi_adc_conv_unregister(conv);
>  }
>  
>  struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev,
>  							size_t sizeof_priv)
>  {
> -	struct adi_axi_adc_conv **ptr, *conv;
> -
> -	ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr),
> -			   GFP_KERNEL);
> -	if (!ptr)
> -		return ERR_PTR(-ENOMEM);
> +	struct adi_axi_adc_conv *conv;
> +	int ret;
>  
>  	conv = adi_axi_adc_conv_register(dev, sizeof_priv);
> -	if (IS_ERR(conv)) {
> -		devres_free(ptr);
> +	if (IS_ERR(conv))
>  		return ERR_CAST(conv);

Is that ERR_CAST() needed here?  conv is already of the
right type so we don't need to cast it to a void * and back gain.
Obviously was there before an not needed either, but might as well
tidy it up whilst we are here!

Thanks,

Jonathan



> -	}
>  
> -	*ptr = conv;
> -	devres_add(dev, ptr);
> +	ret = devm_add_action_or_reset(dev, devm_adi_axi_adc_conv_release,
> +				       conv);
> +	if (ret)
> +		return ERR_PTR(ret);
>  
>  	return conv;
>  }

Re: [PATCH 4/7] iio: triggered-buffer: simplify devm_iio_triggered_buffer_setup_ext

[Jonathan Cameron]

On Thu, 8 Apr 2021 19:38:13 +0800
Yicong Yang <yangyicong@hisilicon.com> wrote:

> Use devm_add_action_or_reset() instead of devres_alloc() and
> devres_add(), which works the same. This will simplify the
> code. There is no functional changes.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/iio/buffer/industrialio-triggered-buffer.c | 19 +++++--------------
>  1 file changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/buffer/industrialio-triggered-buffer.c b/drivers/iio/buffer/industrialio-triggered-buffer.c
> index 92b8aea..63b4722 100644
> --- a/drivers/iio/buffer/industrialio-triggered-buffer.c
> +++ b/drivers/iio/buffer/industrialio-triggered-buffer.c
> @@ -92,9 +92,9 @@ void iio_triggered_buffer_cleanup(struct iio_dev *indio_dev)
>  }
>  EXPORT_SYMBOL(iio_triggered_buffer_cleanup);
>  
> -static void devm_iio_triggered_buffer_clean(struct device *dev, void *res)
> +static void devm_iio_triggered_buffer_clean(void *indio_dev)
>  {
> -	iio_triggered_buffer_cleanup(*(struct iio_dev **)res);
> +	iio_triggered_buffer_cleanup(indio_dev);
>  }
>  
>  int devm_iio_triggered_buffer_setup_ext(struct device *dev,
> @@ -104,24 +104,15 @@ int devm_iio_triggered_buffer_setup_ext(struct device *dev,
>  					const struct iio_buffer_setup_ops *ops,
>  					const struct attribute **buffer_attrs)
>  {
> -	struct iio_dev **ptr;
>  	int ret;
>  
> -	ptr = devres_alloc(devm_iio_triggered_buffer_clean, sizeof(*ptr),
> -			   GFP_KERNEL);
> -	if (!ptr)
> -		return -ENOMEM;
> -
> -	*ptr = indio_dev;
> -
>  	ret = iio_triggered_buffer_setup_ext(indio_dev, h, thread, ops,
>  					     buffer_attrs);
>  	if (!ret)
> -		devres_add(dev, ptr);
> -	else
> -		devres_free(ptr);
> +		return ret;

This looks inverted from what it should be.

	if (ret)
		return ret;

	return devm_add_action_or_reset()...

>  
> -	return ret;
> +	return devm_add_action_or_reset(dev, devm_iio_triggered_buffer_clean,
> +					indio_dev);
>  }
>  EXPORT_SYMBOL_GPL(devm_iio_triggered_buffer_setup_ext);
>  

Re: [PATCH 0/7] Simplify codes with devm_add_action_or_reset

[Jonathan Cameron]

On Thu, 8 Apr 2021 19:38:09 +0800
Yicong Yang <yangyicong@hisilicon.com> wrote:

> Some devm variants are implemented with devres_alloc() and devres_add()
> manually to only manage a single pointer. This can be simplified with
> devm_add_action_or_reset() which works the same. Simplify these functions.
> 
> Yicong Yang (7):
>   iio: adc: adi-axi-adc: simplify devm_adi_axi_adc_conv_register
>   iio: buffer-dmaengine: simplify __devm_iio_dmaengine_buffer_free
>   iio: hw_consumer: simplify devm_iio_hw_consumer_alloc
>   iio: triggered-buffer: simplify devm_iio_triggered_buffer_setup_ext
>   iio: core: simplify some devm functions
>   iio: trigger: simplify __devm_iio_trigger_register
>   iio: inkern: simplify some devm functions

Nice set.  Note no rush for a v2 as IIO is effectively closed for the
coming merge window.  Hence these won't hit linux-next now until after
the merge window closes.

Thanks,

Jonathan

> 
>  drivers/iio/adc/adi-axi-adc.c                      | 22 ++++----
>  drivers/iio/buffer/industrialio-buffer-dmaengine.c | 22 ++++----
>  drivers/iio/buffer/industrialio-hw-consumer.c      | 25 ++++-----
>  drivers/iio/buffer/industrialio-triggered-buffer.c | 19 ++-----
>  drivers/iio/industrialio-core.c                    | 43 ++++++---------
>  drivers/iio/industrialio-trigger.c                 | 18 ++-----
>  drivers/iio/inkern.c                               | 61 ++++++++--------------
>  7 files changed, 78 insertions(+), 132 deletions(-)
> 

Re: [PATCH 4/7] iio: triggered-buffer: simplify devm_iio_triggered_buffer_setup_ext

[Yicong Yang]

On 2021/4/11 22:16, Jonathan Cameron wrote:
> On Thu, 8 Apr 2021 19:38:13 +0800
> Yicong Yang <yangyicong@hisilicon.com> wrote:
> 
>> Use devm_add_action_or_reset() instead of devres_alloc() and
>> devres_add(), which works the same. This will simplify the
>> code. There is no functional changes.
>>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> ---
>>  drivers/iio/buffer/industrialio-triggered-buffer.c | 19 +++++--------------
>>  1 file changed, 5 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/iio/buffer/industrialio-triggered-buffer.c b/drivers/iio/buffer/industrialio-triggered-buffer.c
>> index 92b8aea..63b4722 100644
>> --- a/drivers/iio/buffer/industrialio-triggered-buffer.c
>> +++ b/drivers/iio/buffer/industrialio-triggered-buffer.c
>> @@ -92,9 +92,9 @@ void iio_triggered_buffer_cleanup(struct iio_dev *indio_dev)
>>  }
>>  EXPORT_SYMBOL(iio_triggered_buffer_cleanup);
>>  
>> -static void devm_iio_triggered_buffer_clean(struct device *dev, void *res)
>> +static void devm_iio_triggered_buffer_clean(void *indio_dev)
>>  {
>> -	iio_triggered_buffer_cleanup(*(struct iio_dev **)res);
>> +	iio_triggered_buffer_cleanup(indio_dev);
>>  }
>>  
>>  int devm_iio_triggered_buffer_setup_ext(struct device *dev,
>> @@ -104,24 +104,15 @@ int devm_iio_triggered_buffer_setup_ext(struct device *dev,
>>  					const struct iio_buffer_setup_ops *ops,
>>  					const struct attribute **buffer_attrs)
>>  {
>> -	struct iio_dev **ptr;
>>  	int ret;
>>  
>> -	ptr = devres_alloc(devm_iio_triggered_buffer_clean, sizeof(*ptr),
>> -			   GFP_KERNEL);
>> -	if (!ptr)
>> -		return -ENOMEM;
>> -
>> -	*ptr = indio_dev;
>> -
>>  	ret = iio_triggered_buffer_setup_ext(indio_dev, h, thread, ops,
>>  					     buffer_attrs);
>>  	if (!ret)
>> -		devres_add(dev, ptr);
>> -	else
>> -		devres_free(ptr);
>> +		return ret;
> 
> This looks inverted from what it should be.
> 
> 	if (ret)
> 		return ret;
> 
> 	return devm_add_action_or_reset()...
> 

yes. it does. will fix.

thanks

>>  
>> -	return ret;
>> +	return devm_add_action_or_reset(dev, devm_iio_triggered_buffer_clean,
>> +					indio_dev);
>>  }
>>  EXPORT_SYMBOL_GPL(devm_iio_triggered_buffer_setup_ext);
>>  
> 
> 
> .
> 

Re: [PATCH 1/7] iio: adc: adi-axi-adc: simplify devm_adi_axi_adc_conv_register

[Yicong Yang]

On 2021/4/11 22:12, Jonathan Cameron wrote:
> On Thu, 8 Apr 2021 19:38:10 +0800
> Yicong Yang <yangyicong@hisilicon.com> wrote:
> 
>> Use devm_add_action_or_reset() instead of devres_alloc() and
>> devres_add(), which works the same. This will simplify the
>> code. There is no functional changes.
>>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> ---
>>  drivers/iio/adc/adi-axi-adc.c | 22 +++++++++-------------
>>  1 file changed, 9 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
>> index 9109da2..575a63f 100644
>> --- a/drivers/iio/adc/adi-axi-adc.c
>> +++ b/drivers/iio/adc/adi-axi-adc.c
>> @@ -210,29 +210,25 @@ static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv)
>>  	kfree(cl);
>>  }
>>  
>> -static void devm_adi_axi_adc_conv_release(struct device *dev, void *res)
>> +static void devm_adi_axi_adc_conv_release(void *conv)
>>  {
>> -	adi_axi_adc_conv_unregister(*(struct adi_axi_adc_conv **)res);
>> +	adi_axi_adc_conv_unregister(conv);
>>  }
>>  
>>  struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev,
>>  							size_t sizeof_priv)
>>  {
>> -	struct adi_axi_adc_conv **ptr, *conv;
>> -
>> -	ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr),
>> -			   GFP_KERNEL);
>> -	if (!ptr)
>> -		return ERR_PTR(-ENOMEM);
>> +	struct adi_axi_adc_conv *conv;
>> +	int ret;
>>  
>>  	conv = adi_axi_adc_conv_register(dev, sizeof_priv);
>> -	if (IS_ERR(conv)) {
>> -		devres_free(ptr);
>> +	if (IS_ERR(conv))
>>  		return ERR_CAST(conv);
> 
> Is that ERR_CAST() needed here?  conv is already of the
> right type so we don't need to cast it to a void * and back gain.
> Obviously was there before an not needed either, but might as well
> tidy it up whilst we are here!

sure. thanks for the hint. I didn't notice this. will drop the ERR_CAST.

thanks!

> 
> Thanks,
> 
> Jonathan
> 
> 
> 
>> -	}
>>  
>> -	*ptr = conv;
>> -	devres_add(dev, ptr);
>> +	ret = devm_add_action_or_reset(dev, devm_adi_axi_adc_conv_release,
>> +				       conv);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>>  
>>  	return conv;
>>  }
> 
> 
> .
> 

Re: [PATCH 1/7] iio: adc: adi-axi-adc: simplify devm_adi_axi_adc_conv_register

[Jonathan Cameron]

On Mon, 12 Apr 2021 17:10:17 +0800
Yicong Yang <yangyicong@hisilicon.com> wrote:

> On 2021/4/11 22:12, Jonathan Cameron wrote:
> > On Thu, 8 Apr 2021 19:38:10 +0800
> > Yicong Yang <yangyicong@hisilicon.com> wrote:
> >   
> >> Use devm_add_action_or_reset() instead of devres_alloc() and
> >> devres_add(), which works the same. This will simplify the
> >> code. There is no functional changes.
> >>
> >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> >> ---
> >>  drivers/iio/adc/adi-axi-adc.c | 22 +++++++++-------------
> >>  1 file changed, 9 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
> >> index 9109da2..575a63f 100644
> >> --- a/drivers/iio/adc/adi-axi-adc.c
> >> +++ b/drivers/iio/adc/adi-axi-adc.c
> >> @@ -210,29 +210,25 @@ static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv)
> >>  	kfree(cl);
> >>  }
> >>  
> >> -static void devm_adi_axi_adc_conv_release(struct device *dev, void *res)
> >> +static void devm_adi_axi_adc_conv_release(void *conv)
> >>  {
> >> -	adi_axi_adc_conv_unregister(*(struct adi_axi_adc_conv **)res);
> >> +	adi_axi_adc_conv_unregister(conv);
> >>  }
> >>  
> >>  struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev,
> >>  							size_t sizeof_priv)
> >>  {
> >> -	struct adi_axi_adc_conv **ptr, *conv;
> >> -
> >> -	ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr),
> >> -			   GFP_KERNEL);
> >> -	if (!ptr)
> >> -		return ERR_PTR(-ENOMEM);
> >> +	struct adi_axi_adc_conv *conv;
> >> +	int ret;
> >>  
> >>  	conv = adi_axi_adc_conv_register(dev, sizeof_priv);
> >> -	if (IS_ERR(conv)) {
> >> -		devres_free(ptr);
> >> +	if (IS_ERR(conv))
> >>  		return ERR_CAST(conv);  
> > 
> > Is that ERR_CAST() needed here?  conv is already of the
> > right type so we don't need to cast it to a void * and back gain.
> > Obviously was there before an not needed either, but might as well
> > tidy it up whilst we are here!  
> 
> sure. thanks for the hint. I didn't notice this. will drop the ERR_CAST.
> 
> thanks!

Rather than waste your time doing a v2 for such a trivial change, I've
applied the patch and tweaked that whilst doing so.

Applied to the togreg branch of iio.git and pushed out as testing to
let 0-day poke at it etc.

Thanks,

Jonathan

> 
> > 
> > Thanks,
> > 
> > Jonathan
> > 
> > 
> >   
> >> -	}
> >>  
> >> -	*ptr = conv;
> >> -	devres_add(dev, ptr);
> >> +	ret = devm_add_action_or_reset(dev, devm_adi_axi_adc_conv_release,
> >> +				       conv);
> >> +	if (ret)
> >> +		return ERR_PTR(ret);
> >>  
> >>  	return conv;
> >>  }  
> > 
> > 
> > .
> >   
> 

Re: [PATCH 2/7] iio: buffer-dmaengine: simplify __devm_iio_dmaengine_buffer_free

[Jonathan Cameron]

On Thu, 8 Apr 2021 19:38:11 +0800
Yicong Yang <yangyicong@hisilicon.com> wrote:

> Use devm_add_action_or_reset() instead of devres_alloc() and
> devres_add(), which works the same. This will simplify the
> code. There is no functional changes.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Tiny amount of fuzz, but nothing that mattered.

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to poke at.

Thanks,

Jonathan

> ---
>  drivers/iio/buffer/industrialio-buffer-dmaengine.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> index b0cb9a3..9ecc03f 100644
> --- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> +++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> @@ -228,9 +228,9 @@ static void iio_dmaengine_buffer_free(struct iio_buffer *buffer)
>  	iio_buffer_put(buffer);
>  }
>  
> -static void __devm_iio_dmaengine_buffer_free(struct device *dev, void *res)
> +static void __devm_iio_dmaengine_buffer_free(void *buffer)
>  {
> -	iio_dmaengine_buffer_free(*(struct iio_buffer **)res);
> +	iio_dmaengine_buffer_free(buffer);
>  }
>  
>  /**
> @@ -247,21 +247,17 @@ static void __devm_iio_dmaengine_buffer_free(struct device *dev, void *res)
>  struct iio_buffer *devm_iio_dmaengine_buffer_alloc(struct device *dev,
>  	const char *channel)
>  {
> -	struct iio_buffer **bufferp, *buffer;
> -
> -	bufferp = devres_alloc(__devm_iio_dmaengine_buffer_free,
> -			       sizeof(*bufferp), GFP_KERNEL);
> -	if (!bufferp)
> -		return ERR_PTR(-ENOMEM);
> +	struct iio_buffer *buffer;
> +	int ret;
>  
>  	buffer = iio_dmaengine_buffer_alloc(dev, channel);
> -	if (IS_ERR(buffer)) {
> -		devres_free(bufferp);
> +	if (IS_ERR(buffer))
>  		return buffer;
> -	}
>  
> -	*bufferp = buffer;
> -	devres_add(dev, bufferp);
> +	ret = devm_add_action_or_reset(dev, __devm_iio_dmaengine_buffer_free,
> +				       buffer);
> +	if (ret)
> +		return ERR_PTR(ret);
>  
>  	return buffer;
>  }

Re: [PATCH 3/7] iio: hw_consumer: simplify devm_iio_hw_consumer_alloc

[Jonathan Cameron]

On Thu, 8 Apr 2021 19:38:12 +0800
Yicong Yang <yangyicong@hisilicon.com> wrote:

> Use devm_add_action_or_reset() instead of devres_alloc() and
> devres_add(), which works the same. This will simplify the
> code. There is no functional changes.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Applied,

Thanks,
> ---
>  drivers/iio/buffer/industrialio-hw-consumer.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/buffer/industrialio-hw-consumer.c b/drivers/iio/buffer/industrialio-hw-consumer.c
> index f2d2778..87d9aab 100644
> --- a/drivers/iio/buffer/industrialio-hw-consumer.c
> +++ b/drivers/iio/buffer/industrialio-hw-consumer.c
> @@ -137,9 +137,9 @@ void iio_hw_consumer_free(struct iio_hw_consumer *hwc)
>  }
>  EXPORT_SYMBOL_GPL(iio_hw_consumer_free);
>  
> -static void devm_iio_hw_consumer_release(struct device *dev, void *res)
> +static void devm_iio_hw_consumer_release(void *iio_hwc)
>  {
> -	iio_hw_consumer_free(*(struct iio_hw_consumer **)res);
> +	iio_hw_consumer_free(iio_hwc);
>  }
>  
>  /**
> @@ -153,20 +153,17 @@ static void devm_iio_hw_consumer_release(struct device *dev, void *res)
>   */
>  struct iio_hw_consumer *devm_iio_hw_consumer_alloc(struct device *dev)
>  {
> -	struct iio_hw_consumer **ptr, *iio_hwc;
> -
> -	ptr = devres_alloc(devm_iio_hw_consumer_release, sizeof(*ptr),
> -			   GFP_KERNEL);
> -	if (!ptr)
> -		return NULL;
> +	struct iio_hw_consumer *iio_hwc;
> +	int ret;
>  
>  	iio_hwc = iio_hw_consumer_alloc(dev);
> -	if (IS_ERR(iio_hwc)) {
> -		devres_free(ptr);
> -	} else {
> -		*ptr = iio_hwc;
> -		devres_add(dev, ptr);
> -	}
> +	if (IS_ERR(iio_hwc))
> +		return iio_hwc;
> +
> +	ret = devm_add_action_or_reset(dev, devm_iio_hw_consumer_release,
> +				       iio_hwc);
> +	if (ret)
> +		return ERR_PTR(ret);
>  
>  	return iio_hwc;
>  }

Re: [PATCH 4/7] iio: triggered-buffer: simplify devm_iio_triggered_buffer_setup_ext

[Jonathan Cameron]

On Mon, 12 Apr 2021 17:05:15 +0800
Yicong Yang <yangyicong@hisilicon.com> wrote:

> On 2021/4/11 22:16, Jonathan Cameron wrote:
> > On Thu, 8 Apr 2021 19:38:13 +0800
> > Yicong Yang <yangyicong@hisilicon.com> wrote:
> >   
> >> Use devm_add_action_or_reset() instead of devres_alloc() and
> >> devres_add(), which works the same. This will simplify the
> >> code. There is no functional changes.
> >>
> >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> >> ---
> >>  drivers/iio/buffer/industrialio-triggered-buffer.c | 19 +++++--------------
> >>  1 file changed, 5 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/iio/buffer/industrialio-triggered-buffer.c b/drivers/iio/buffer/industrialio-triggered-buffer.c
> >> index 92b8aea..63b4722 100644
> >> --- a/drivers/iio/buffer/industrialio-triggered-buffer.c
> >> +++ b/drivers/iio/buffer/industrialio-triggered-buffer.c
> >> @@ -92,9 +92,9 @@ void iio_triggered_buffer_cleanup(struct iio_dev *indio_dev)
> >>  }
> >>  EXPORT_SYMBOL(iio_triggered_buffer_cleanup);
> >>  
> >> -static void devm_iio_triggered_buffer_clean(struct device *dev, void *res)
> >> +static void devm_iio_triggered_buffer_clean(void *indio_dev)
> >>  {
> >> -	iio_triggered_buffer_cleanup(*(struct iio_dev **)res);
> >> +	iio_triggered_buffer_cleanup(indio_dev);
> >>  }
> >>  
> >>  int devm_iio_triggered_buffer_setup_ext(struct device *dev,
> >> @@ -104,24 +104,15 @@ int devm_iio_triggered_buffer_setup_ext(struct device *dev,
> >>  					const struct iio_buffer_setup_ops *ops,
> >>  					const struct attribute **buffer_attrs)
> >>  {
> >> -	struct iio_dev **ptr;
> >>  	int ret;
> >>  
> >> -	ptr = devres_alloc(devm_iio_triggered_buffer_clean, sizeof(*ptr),
> >> -			   GFP_KERNEL);
> >> -	if (!ptr)
> >> -		return -ENOMEM;
> >> -
> >> -	*ptr = indio_dev;
> >> -
> >>  	ret = iio_triggered_buffer_setup_ext(indio_dev, h, thread, ops,
> >>  					     buffer_attrs);
> >>  	if (!ret)
> >> -		devres_add(dev, ptr);
> >> -	else
> >> -		devres_free(ptr);
> >> +		return ret;  
> > 
> > This looks inverted from what it should be.
> > 
> > 	if (ret)
> > 		return ret;
> > 
> > 	return devm_add_action_or_reset()...
> >   
> 
> yes. it does. will fix.
I fixed it up and applied to the togreg branch of iio.git. Pushed out as
testing for all the usual reasons.

Thanks,

Jonathan

> 
> thanks
> 
> >>  
> >> -	return ret;
> >> +	return devm_add_action_or_reset(dev, devm_iio_triggered_buffer_clean,
> >> +					indio_dev);
> >>  }
> >>  EXPORT_SYMBOL_GPL(devm_iio_triggered_buffer_setup_ext);
> >>    
> > 
> > 
> > .
> >   
> 

Re: [PATCH 5/7] iio: core: simplify some devm functions

[Jonathan Cameron]

On Fri, 9 Apr 2021 21:05:49 +0800
Yicong Yang <yangyicong@hisilicon.com> wrote:

> On 2021/4/9 17:55, Andy Shevchenko wrote:
> > On Fri, Apr 9, 2021 at 12:42 PM Yicong Yang <yangyicong@hisilicon.com> wrote:  
> >> On 2021/4/9 17:19, Andy Shevchenko wrote:  
> >>> On Fri, Apr 9, 2021 at 10:22 AM Yicong Yang <yangyicong@hisilicon.com> wrote:  
> >>>> On 2021/4/8 21:09, Alexandru Ardelean wrote:  
> >>>>> On Thu, Apr 8, 2021 at 2:41 PM Yicong Yang <yangyicong@hisilicon.com> wrote:  
> > 
> > ...
> >   
> >>> Actually Alexandru has a good point. Compare the pieces I left above.
> >>>  
> >>
> >> a little different from my perspective when I did this.
> >>
> >> previously:
> >>
> >> -       ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr),
> >> -                          GFP_KERNEL);
> >> -       if (!ptr)
> >> -               return NULL;
> >>
> >> if devres_alloc() failure we return NULL.
> >>
> >>
> >>         iio_dev = iio_device_alloc(parent, sizeof_priv);
> >> -       if (iio_dev) {
> >> -               *ptr = iio_dev;
> >> -               devres_add(parent, ptr);
> >> -       } else {
> >> -               devres_free(ptr);
> >> -       }
> >>
> >>         return iio_dev;
> >>
> >> if iio_device_alloc() failed, we return what it returns.
> >>
> >> now we remove devres_alloc() and stay iio_device_alloc(), so I just
> >> keep to return iio_dev.
> >>
> >> but actually iio_device_alloc() will return NULL at failure. :)
> >> so return NULL here is definitely correct. i'll change to that if
> >> it's recommended.  
> > 
> > Confusing here (in your case) is that:
> > 
> > if (!FOO)
> >  return BAR;
> > 
> > ...
> > 
> > return BAR;
> > 
> > I.e. twice returning BAR for different occasions. This makes
> > additional cognitive work and decrease readability / maintainability
> > of the code.
> > 
> > I.o.w., yes, it's preferred.
> >   
> 
> got it. will return NULL and will check if other patches in this series
> met the same condition.
> 
> thanks.
> 
Rather than go around a gain, I fixed this up as discussed above and applied
to the togreg branch of iio.git and pushed out as testing.

Thanks,

Jonathan

Re: [PATCH 6/7] iio: trigger: simplify __devm_iio_trigger_register

[Jonathan Cameron]

On Thu, 8 Apr 2021 19:38:15 +0800
Yicong Yang <yangyicong@hisilicon.com> wrote:

> Use devm_add_action_or_reset() instead of devres_alloc() and
> devres_add(), which works the same. This will simplify the
> code. There is no functional changes.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Applied.

Thanks,

Jonathan

> ---
>  drivers/iio/industrialio-trigger.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index ea3c985..0db0952 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -624,9 +624,9 @@ struct iio_trigger *devm_iio_trigger_alloc(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(devm_iio_trigger_alloc);
>  
> -static void devm_iio_trigger_unreg(struct device *dev, void *res)
> +static void devm_iio_trigger_unreg(void *trigger_info)
>  {
> -	iio_trigger_unregister(*(struct iio_trigger **)res);
> +	iio_trigger_unregister(trigger_info);
>  }
>  
>  /**
> @@ -647,21 +647,13 @@ int __devm_iio_trigger_register(struct device *dev,
>  				struct iio_trigger *trig_info,
>  				struct module *this_mod)
>  {
> -	struct iio_trigger **ptr;
>  	int ret;
>  
> -	ptr = devres_alloc(devm_iio_trigger_unreg, sizeof(*ptr), GFP_KERNEL);
> -	if (!ptr)
> -		return -ENOMEM;
> -
> -	*ptr = trig_info;
>  	ret = __iio_trigger_register(trig_info, this_mod);
> -	if (!ret)
> -		devres_add(dev, ptr);
> -	else
> -		devres_free(ptr);
> +	if (ret)
> +		return ret;
>  
> -	return ret;
> +	return devm_add_action_or_reset(dev, devm_iio_trigger_unreg, trig_info);
>  }
>  EXPORT_SYMBOL_GPL(__devm_iio_trigger_register);
>  

Re: [PATCH 7/7] iio: inkern: simplify some devm functions

[Jonathan Cameron]

On Thu, 8 Apr 2021 19:38:16 +0800
Yicong Yang <yangyicong@hisilicon.com> wrote:

> Use devm_add_action_or_reset() instead of devres_alloc() and
> devres_add(), which works the same. This will simplify the
> code. There is no functional changes.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Jonathan

> ---
>  drivers/iio/inkern.c | 61 ++++++++++++++++++++--------------------------------
>  1 file changed, 23 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index db77a2d..5de35cc 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -359,30 +359,24 @@ void iio_channel_release(struct iio_channel *channel)
>  }
>  EXPORT_SYMBOL_GPL(iio_channel_release);
>  
> -static void devm_iio_channel_free(struct device *dev, void *res)
> +static void devm_iio_channel_free(void *iio_channel)
>  {
> -	struct iio_channel *channel = *(struct iio_channel **)res;
> -
> -	iio_channel_release(channel);
> +	iio_channel_release(iio_channel);
>  }
>  
>  struct iio_channel *devm_iio_channel_get(struct device *dev,
>  					 const char *channel_name)
>  {
> -	struct iio_channel **ptr, *channel;
> -
> -	ptr = devres_alloc(devm_iio_channel_free, sizeof(*ptr), GFP_KERNEL);
> -	if (!ptr)
> -		return ERR_PTR(-ENOMEM);
> +	struct iio_channel *channel;
> +	int ret;
>  
>  	channel = iio_channel_get(dev, channel_name);
> -	if (IS_ERR(channel)) {
> -		devres_free(ptr);
> +	if (IS_ERR(channel))
>  		return channel;
> -	}
>  
> -	*ptr = channel;
> -	devres_add(dev, ptr);
> +	ret = devm_add_action_or_reset(dev, devm_iio_channel_free, channel);
> +	if (ret)
> +		return ERR_PTR(ret);
>  
>  	return channel;
>  }
> @@ -392,20 +386,16 @@ struct iio_channel *devm_of_iio_channel_get_by_name(struct device *dev,
>  						    struct device_node *np,
>  						    const char *channel_name)
>  {
> -	struct iio_channel **ptr, *channel;
> -
> -	ptr = devres_alloc(devm_iio_channel_free, sizeof(*ptr), GFP_KERNEL);
> -	if (!ptr)
> -		return ERR_PTR(-ENOMEM);
> +	struct iio_channel *channel;
> +	int ret;
>  
>  	channel = of_iio_channel_get_by_name(np, channel_name);
> -	if (IS_ERR(channel)) {
> -		devres_free(ptr);
> +	if (IS_ERR(channel))
>  		return channel;
> -	}
>  
> -	*ptr = channel;
> -	devres_add(dev, ptr);
> +	ret = devm_add_action_or_reset(dev, devm_iio_channel_free, channel);
> +	if (ret)
> +		return ERR_PTR(ret);
>  
>  	return channel;
>  }
> @@ -496,29 +486,24 @@ void iio_channel_release_all(struct iio_channel *channels)
>  }
>  EXPORT_SYMBOL_GPL(iio_channel_release_all);
>  
> -static void devm_iio_channel_free_all(struct device *dev, void *res)
> +static void devm_iio_channel_free_all(void *iio_channels)
>  {
> -	struct iio_channel *channels = *(struct iio_channel **)res;
> -
> -	iio_channel_release_all(channels);
> +	iio_channel_release_all(iio_channels);
>  }
>  
>  struct iio_channel *devm_iio_channel_get_all(struct device *dev)
>  {
> -	struct iio_channel **ptr, *channels;
> -
> -	ptr = devres_alloc(devm_iio_channel_free_all, sizeof(*ptr), GFP_KERNEL);
> -	if (!ptr)
> -		return ERR_PTR(-ENOMEM);
> +	struct iio_channel *channels;
> +	int ret;
>  
>  	channels = iio_channel_get_all(dev);
> -	if (IS_ERR(channels)) {
> -		devres_free(ptr);
> +	if (IS_ERR(channels))
>  		return channels;
> -	}
>  
> -	*ptr = channels;
> -	devres_add(dev, ptr);
> +	ret = devm_add_action_or_reset(dev, devm_iio_channel_free_all,
> +				       channels);
> +	if (ret)
> +		return ERR_PTR(ret);
>  
>  	return channels;
>  }

Re: [PATCH 0/7] Simplify codes with devm_add_action_or_reset

[Jonathan Cameron]

On Sun, 11 Apr 2021 15:21:20 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Thu, 8 Apr 2021 19:38:09 +0800
> Yicong Yang <yangyicong@hisilicon.com> wrote:
> 
> > Some devm variants are implemented with devres_alloc() and devres_add()
> > manually to only manage a single pointer. This can be simplified with
> > devm_add_action_or_reset() which works the same. Simplify these functions.
> > 
> > Yicong Yang (7):
> >   iio: adc: adi-axi-adc: simplify devm_adi_axi_adc_conv_register
> >   iio: buffer-dmaengine: simplify __devm_iio_dmaengine_buffer_free
> >   iio: hw_consumer: simplify devm_iio_hw_consumer_alloc
> >   iio: triggered-buffer: simplify devm_iio_triggered_buffer_setup_ext
> >   iio: core: simplify some devm functions
> >   iio: trigger: simplify __devm_iio_trigger_register
> >   iio: inkern: simplify some devm functions  
> 
> Nice set.  Note no rush for a v2 as IIO is effectively closed for the
> coming merge window.  Hence these won't hit linux-next now until after
> the merge window closes.

On second thoughts was just easier for me to make the minor tweaks and
apply so I did that.

All applied to the togreg branch of iio.git and pushed out as testing
for autobuilders etc to poke at.

Thanks,

Jonathan

> 
> Thanks,
> 
> Jonathan
> 
> > 
> >  drivers/iio/adc/adi-axi-adc.c                      | 22 ++++----
> >  drivers/iio/buffer/industrialio-buffer-dmaengine.c | 22 ++++----
> >  drivers/iio/buffer/industrialio-hw-consumer.c      | 25 ++++-----
> >  drivers/iio/buffer/industrialio-triggered-buffer.c | 19 ++-----
> >  drivers/iio/industrialio-core.c                    | 43 ++++++---------
> >  drivers/iio/industrialio-trigger.c                 | 18 ++-----
> >  drivers/iio/inkern.c                               | 61 ++++++++--------------
> >  7 files changed, 78 insertions(+), 132 deletions(-)
> >   
> 

Re: [PATCH 0/7] Simplify codes with devm_add_action_or_reset

[Yicong Yang]

On 2021/4/24 22:15, Jonathan Cameron wrote:
> On Sun, 11 Apr 2021 15:21:20 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
>> On Thu, 8 Apr 2021 19:38:09 +0800
>> Yicong Yang <yangyicong@hisilicon.com> wrote:
>>
>>> Some devm variants are implemented with devres_alloc() and devres_add()
>>> manually to only manage a single pointer. This can be simplified with
>>> devm_add_action_or_reset() which works the same. Simplify these functions.
>>>
>>> Yicong Yang (7):
>>>   iio: adc: adi-axi-adc: simplify devm_adi_axi_adc_conv_register
>>>   iio: buffer-dmaengine: simplify __devm_iio_dmaengine_buffer_free
>>>   iio: hw_consumer: simplify devm_iio_hw_consumer_alloc
>>>   iio: triggered-buffer: simplify devm_iio_triggered_buffer_setup_ext
>>>   iio: core: simplify some devm functions
>>>   iio: trigger: simplify __devm_iio_trigger_register
>>>   iio: inkern: simplify some devm functions  
>>
>> Nice set.  Note no rush for a v2 as IIO is effectively closed for the
>> coming merge window.  Hence these won't hit linux-next now until after
>> the merge window closes.
> 
> On second thoughts was just easier for me to make the minor tweaks and
> apply so I did that.
> 
> All applied to the togreg branch of iio.git and pushed out as testing
> for autobuilders etc to poke at.
> 

Thanks a lot! Sorry for didn't update, I have intended to send a v2 series after
the tree reopened. :)

Regards,
Yicong

> Thanks,
> 
> Jonathan
> 
>>
>> Thanks,
>>
>> Jonathan
>>
>>>
>>>  drivers/iio/adc/adi-axi-adc.c                      | 22 ++++----
>>>  drivers/iio/buffer/industrialio-buffer-dmaengine.c | 22 ++++----
>>>  drivers/iio/buffer/industrialio-hw-consumer.c      | 25 ++++-----
>>>  drivers/iio/buffer/industrialio-triggered-buffer.c | 19 ++-----
>>>  drivers/iio/industrialio-core.c                    | 43 ++++++---------
>>>  drivers/iio/industrialio-trigger.c                 | 18 ++-----
>>>  drivers/iio/inkern.c                               | 61 ++++++++--------------
>>>  7 files changed, 78 insertions(+), 132 deletions(-)
>>>   
>>
> 
> 
> .
>