[PATCH 0/9] added helper macros to remove duplicate code from probe functions of the platform drivers

[PATCH 0/9] added helper macros to remove duplicate code from probe functions of the platform drivers

[Satendra Singh Thakur]

1. For most of the platform drivers's probe include following steps

-memory allocation for driver's private structure
-getting io resources
-io remapping resources
-getting irq number
-registering irq
-setting driver's private data
-getting clock
-preparing and enabling clock

2. We have defined a set of macros to combine some or all of
the above mentioned steps. This will remove redundant/duplicate
code in drivers' probe functions of platform drivers.

devm_platform_probe_helper(pdev, priv, clk_name);
devm_platform_probe_helper_clk(pdev, priv, clk_name);
devm_platform_probe_helper_irq(pdev, priv, clk_name,
irq_hndlr, irq_flags, irq_name, irq_devid);
devm_platform_probe_helper_all(pdev, priv, clk_name,
irq_hndlr, irq_flags, irq_name, irq_devid);
devm_platform_probe_helper_all_data(pdev, priv, clk_name,
irq_hndlr, irq_flags, irq_name, irq_devid);

3. Code is made devres compatible (wherever required)
The functions: clk_get, request_irq, kzalloc, platform_get_resource
are replaced with their devm_* counterparts.

4. Few bugs are also fixed.

Satendra Singh Thakur (9):
  probe/dma : added helper macros to remove redundant/duplicate code
    from probe functions of the dma controller drivers
  probe/dma/jz4740: removed redundant code from jz4740 dma controller's 
       probe function
  probe/dma/zx: removed redundant code from zx dma controller's probe
    function
  probe/dma/qcom-bam: removed redundant code from qcom bam dma
    controller's probe function
  probe/dma/mtk-hs: removed redundant code from mediatek hs dma
    controller's probe function
  probe/dma/sun6i: removed redundant code from sun6i dma controller's
    probe function
  probe/dma/sun4i: removed redundant code from sun4i dma controller's
    probe function
  probe/dma/axi: removed redundant code from axi dma controller's probe
    function
  probe/dma/owl: removed redundant code from owl dma controller's probe
    function

 drivers/dma/dma-axi-dmac.c       |  28 ++---
 drivers/dma/dma-jz4740.c         |  33 +++---
 drivers/dma/mediatek/mtk-hsdma.c |  38 +++----
 drivers/dma/owl-dma.c            |  29 ++---
 drivers/dma/qcom/bam_dma.c       |  71 +++++-------
 drivers/dma/sun4i-dma.c          |  30 ++----
 drivers/dma/sun6i-dma.c          |  30 ++----
 drivers/dma/zx_dma.c             |  35 ++----
 include/linux/probe-helper.h     | 179 +++++++++++++++++++++++++++++++
 9 files changed, 280 insertions(+), 193 deletions(-)
 create mode 100644 include/linux/probe-helper.h

-- 
2.17.1

[PATCH 1/9] probe/dma : added helper macros to remove redundant/duplicate code from probe functions of the dma controller drivers

[Satendra Singh Thakur]

1. For most of the drivers probe include following steps

a) memory allocation for driver's private structure
b) getting io resources
c) io remapping resources
d) getting clock
e) getting irq number
f) registering irq
g) preparing and enabling clock
i) setting platform's drv data

2. We have defined a set of macros to combine above mentioned
steps.
This will remove redundant/duplicate code in drivers' probe
functions.

3. This macro combines all the steps except f), g) and i).

devm_platform_probe_helper(pdev, priv, clk_name);

4. This macro combines all the steps except f) and i).

devm_platform_probe_helper_clk(pdev, priv, clk_name);

5. This macro combines all the steps except g) and i).

devm_platform_probe_helper_irq(pdev, priv, clk_name,
	irq_hndlr, irq_flags, irq_name, irq_devid);

6. This is because, some drivers perform step f) and g)
after hw init or subsys registration at very different points
in the probe function. The step i) is called at the end of
probe function by several drivers; while other drivers call it at
different points in probe function.

7. This macro combines above mentioned steps a) to g).

devm_platform_probe_helper_all(pdev, priv, clk_name,
	irq_hndlr, irq_flags, irq_name, irq_devid);

8. This macro combines all of the above mentioned steps a) to i).

devm_platform_probe_helper_all_data(pdev, priv, clk_name,
	irq_hndlr, irq_flags, irq_name, irq_devid);
9. Above macros will be useful for wide variety of probe
functions of different drivers.

Signed-off-by: Satendra Singh Thakur <satendrasingh.thakur@hcl.com>
Signed-off-by: Satendra Singh Thakur <sst2005@gmail.com>
---
 include/linux/probe-helper.h | 179 +++++++++++++++++++++++++++++++++++
 1 file changed, 179 insertions(+)
 create mode 100644 include/linux/probe-helper.h

diff --git a/include/linux/probe-helper.h b/include/linux/probe-helper.h
new file mode 100644
index 000000000000..7baa468509e3
--- /dev/null
+++ b/include/linux/probe-helper.h
@@ -0,0 +1,179 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ *
+ * probe_helper.h - helper functions for platform drivers' probe
+ * function
+ * Author: Satendra Singh Thakur <satendrasingh.thakur@hcl.com> Sep 2019
+ *				  <sst2005@gmail.com>
+ */
+#ifndef _PROBE_HELPER_H_
+#define _PROBE_HELPER_H_
+
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+
+/* devm_platform_probe_helper - Macro for helping probe method
+ * of platform drivers
+ * This macro combines the functions:
+ * devm_kzalloc, platform_get_resource, devm_ioremap_resource,
+ * devm_clk_get, platform_get_irq
+ * @pdev platform device
+ * @priv driver's private object for memory allocation
+ * @clk_name clock name as in DT
+ */
+#define devm_platform_probe_helper(pdev, priv, clk_name)	\
+({	\
+	__label__ __out;	\
+	int __ret = 0;	\
+	priv = devm_kzalloc(&(pdev)->dev, sizeof(*priv), GFP_KERNEL);	\
+	if (!(priv)) {	\
+		dev_err(&(pdev)->dev, "devm_kzalloc failed\n");	\
+		__ret = -ENOMEM;	\
+		goto __out;	\
+	}	\
+	(priv)->base = devm_platform_ioremap_resource(pdev, 0);	\
+	if (IS_ERR((priv)->base)) {	\
+		dev_err(&(pdev)->dev,	\
+			"devm_platform_ioremap_resource failed\n");	\
+		__ret = PTR_ERR((priv)->base);	\
+		goto __out;	\
+	}	\
+	(priv)->clk = devm_clk_get(&(pdev)->dev, clk_name);	\
+	if (IS_ERR((priv)->clk)) {	\
+		dev_err(&(pdev)->dev, "devm_clk_get failed\n");	\
+		__ret = PTR_ERR((priv)->clk);	\
+		goto __out;	\
+	}	\
+	(priv)->irq = platform_get_irq(pdev, 0);	\
+	if ((priv)->irq < 0) {	\
+		dev_err(&(pdev)->dev, "platform_get_irq failed\n");	\
+		__ret = (priv)->irq;	\
+		goto __out;	\
+	}	\
+__out:	\
+	__ret;	\
+})
+
+/* devm_platform_probe_helper_irq - Macro for helping probe method
+ * of platform drivers
+ * This macro combines the functions:
+ * devm_kzalloc, platform_get_resource, devm_ioremap_resource,
+ * devm_clk_get, platform_get_irq, devm_request_irq
+ * @pdev platform device
+ * @priv driver's private object for memory allocation
+ * @clk_name clock name as in DT
+ * @irq_hndlr interrupt handler function (isr)
+ * @irq_flags flags for interrupt registration
+ * @irq_name name of the interrupt handler
+ * @irq_devid device identifier for irq
+ */
+#define devm_platform_probe_helper_irq(pdev, priv, clk_name,	\
+		irq_hndlr, irq_flags, irq_name, irq_devid)	\
+({	\
+	__label__ __out;	\
+	int __ret = 0;	\
+	__ret = devm_platform_probe_helper(pdev, priv, clk_name);	\
+	if (__ret < 0)	\
+		goto __out;	\
+	__ret = devm_request_irq(&(pdev)->dev, (priv)->irq, irq_hndlr,	\
+			irq_flags, irq_name, irq_devid);	\
+	if (__ret < 0) {	\
+		dev_err(&(pdev)->dev,	\
+			"devm_request_irq failed for irq num %d\n",	\
+			(priv)->irq);	\
+		goto __out;	\
+	}	\
+__out:	\
+	__ret;	\
+})
+
+/* devm_platform_probe_helper_clk Macro - for helping probe method
+ * of platform drivers
+ * This macro combines the functions:
+ * devm_kzalloc, platform_get_resource, devm_ioremap_resource,
+ * devm_clk_get, platform_get_irq, clk_prepare_enable
+ * @pdev platform device
+ * @priv driver's private object for memory allocation
+ * @clk_name clock name as in DT
+ */
+#define devm_platform_probe_helper_clk(pdev, priv, clk_name)	\
+({	\
+	__label__ __out;	\
+	int __ret = 0;	\
+	__ret = devm_platform_probe_helper(pdev, priv, clk_name);	\
+	if (__ret < 0)	\
+		goto __out;	\
+	__ret = clk_prepare_enable((priv)->clk);	\
+	if (__ret < 0) {	\
+		dev_err(&(pdev)->dev, "clk_prepare_enable failed\n");	\
+		goto __out;	\
+	}	\
+__out:	\
+	__ret;	\
+})
+
+/* devm_platform_probe_helper_all - Macro for helping probe method
+ * of platform drivers
+ * This macro combines the functions:
+ * devm_kzalloc, platform_get_resource, devm_ioremap_resource,
+ * devm_clk_get, platform_get_irq, devm_request_irq,
+ * clk_prepare_enable
+ * @pdev platform device
+ * @priv driver's private object for memory allocation
+ * @clk_name clock name as in DT
+ * @irq_hndlr interrupt handler function (isr)
+ * @irq_flags flags for interrupt registration
+ * @irq_name name of the interrupt handler
+ * @irq_devid device identifier for irq
+ */
+#define devm_platform_probe_helper_all(pdev, priv, clk_name,	\
+	irq_hndlr, irq_flags, irq_name, irq_devid)	\
+({	\
+	__label__ __out;	\
+	int __ret = 0;	\
+	__ret = devm_platform_probe_helper_clk(pdev, priv, clk_name);	\
+	if (__ret < 0)	\
+		goto __out;	\
+	__ret = devm_request_irq(&(pdev)->dev, (priv)->irq,	\
+		irq_hndlr, irq_flags, irq_name, irq_devid);	\
+	if (__ret < 0) {	\
+		dev_err(&(pdev)->dev,	\
+			"devm_request_irq failed for irq num %d\n",	\
+			(priv)->irq);	\
+		if ((priv)->clk)	\
+			clk_disable_unprepare((priv)->clk);	\
+		goto __out;	\
+	}	\
+__out:	\
+	__ret;	\
+})
+
+/* devm_platform_probe_helper_all_data - Macro for helping probe method
+ * of platform drivers
+ * This macro combines the functions:
+ * devm_kzalloc, platform_get_resource, devm_ioremap_resource,
+ * devm_clk_get, platform_get_irq, devm_request_irq,
+ * clk_prepare_enable, platform_set_drvdata
+ * @pdev platform device
+ * @priv driver's private object for memory allocation
+ * @clk_name clock name as in DT
+ * @irq_hndlr interrupt handler function (isr)
+ * @irq_flags flags for interrupt registration
+ * @irq_name name of the interrupt handler
+ * @irq_devid device identifier for irq
+ */
+#define devm_platform_probe_helper_all_data(pdev, priv, clk_name,	\
+	irq_hndlr, irq_flags, irq_name, irq_devid)	\
+({	\
+	__label__ __out;	\
+	int __ret = 0;	\
+	__ret = devm_platform_probe_helper_all(pdev, priv, clk_name,	\
+		irq_hndlr, irq_flags, irq_name, irq_devid);	\
+	if (__ret < 0)	\
+		goto __out;	\
+	platform_set_drvdata(pdev, priv);	\
+__out:	\
+	__ret;	\
+})
+
+#endif /*_PROBE_HELPER_H_*/
-- 
2.17.1

[PATCH 2/9] probe/dma/jz4740: removed redundant code from jz4740 dma controller's probe function

[Satendra Singh Thakur]

1. In order to remove duplicate code, following functions:
devm_kzalloc
platform_get_resource
devm_ioremap_resource
clk_get
clk_prepare_enable
platform_get_irq
are replaced with a macro devm_platform_probe_helper_clk.

2. Added irq field in the struct jz4740_dma_dev.
Removed platform_get_irq from remove method.

3. This patch depends on the file include/linux/probe-helper.h
which is pushed in previous patch [01/09].

Signed-off-by: Satendra Singh Thakur <satendrasingh.thakur@hcl.com>
Signed-off-by: Satendra Singh Thakur <sst2005@gmail.com>
---
 drivers/dma/dma-jz4740.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/dma/dma-jz4740.c b/drivers/dma/dma-jz4740.c
index 39c676c47082..b012896d02bb 100644
--- a/drivers/dma/dma-jz4740.c
+++ b/drivers/dma/dma-jz4740.c
@@ -15,6 +15,7 @@
 #include <linux/spinlock.h>
 #include <linux/irq.h>
 #include <linux/clk.h>
+#include <linux/probe-helper.h>
 
 #include "virt-dma.h"
 
@@ -121,6 +122,7 @@ struct jz4740_dma_dev {
 	struct dma_device ddev;
 	void __iomem *base;
 	struct clk *clk;
+	int irq;
 
 	struct jz4740_dmaengine_chan chan[JZ_DMA_NR_CHANS];
 };
@@ -519,27 +521,19 @@ static int jz4740_dma_probe(struct platform_device *pdev)
 	struct jz4740_dma_dev *dmadev;
 	struct dma_device *dd;
 	unsigned int i;
-	struct resource *res;
 	int ret;
-	int irq;
 
-	dmadev = devm_kzalloc(&pdev->dev, sizeof(*dmadev), GFP_KERNEL);
-	if (!dmadev)
-		return -EINVAL;
+	/*
+	 * This macro internally combines following functions:
+	 * devm_kzalloc, platform_get_resource, devm_ioremap_resource,
+	 * devm_clk_get, platform_get_irq, clk_prepare_enable
+	 */
+	ret = devm_platform_probe_helper_clk(pdev, dmadev, "dma");
+	if (ret < 0)
+		return ret;
 
 	dd = &dmadev->ddev;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	dmadev->base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(dmadev->base))
-		return PTR_ERR(dmadev->base);
-
-	dmadev->clk = clk_get(&pdev->dev, "dma");
-	if (IS_ERR(dmadev->clk))
-		return PTR_ERR(dmadev->clk);
-
-	clk_prepare_enable(dmadev->clk);
-
 	dma_cap_set(DMA_SLAVE, dd->cap_mask);
 	dma_cap_set(DMA_CYCLIC, dd->cap_mask);
 	dd->device_free_chan_resources = jz4740_dma_free_chan_resources;
@@ -567,8 +561,8 @@ static int jz4740_dma_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_clk;
 
-	irq = platform_get_irq(pdev, 0);
-	ret = request_irq(irq, jz4740_dma_irq, 0, dev_name(&pdev->dev), dmadev);
+	ret = request_irq(dmadev->irq, jz4740_dma_irq, 0,
+			dev_name(&pdev->dev), dmadev);
 	if (ret)
 		goto err_unregister;
 
@@ -598,9 +592,8 @@ static void jz4740_cleanup_vchan(struct dma_device *dmadev)
 static int jz4740_dma_remove(struct platform_device *pdev)
 {
 	struct jz4740_dma_dev *dmadev = platform_get_drvdata(pdev);
-	int irq = platform_get_irq(pdev, 0);
 
-	free_irq(irq, dmadev);
+	free_irq(dmadev->irq, dmadev);
 
 	jz4740_cleanup_vchan(&dmadev->ddev);
 	dma_async_device_unregister(&dmadev->ddev);
-- 
2.17.1

[PATCH 3/9] probe/dma/zx: removed redundant code from zx dma controller's probe function

[Satendra Singh Thakur]

1. In order to remove duplicate code, following functions:
platform_get_resource
devm_kzalloc
devm_ioremap_resource
devm_clk_get
platform_get_irq
devm_request_irq
are replaced with a macro devm_platform_probe_helper_irq.

2. Removed dmam_pool_destroy from remove method as dmam_pool_create
is already used in probe function.

3. This patch depends on the file include/linux/probe-helper.h
which is pushed in previous patch [01/09].

Signed-off-by: Satendra Singh Thakur <satendrasingh.thakur@hcl.com>
Signed-off-by: Satendra Singh Thakur <sst2005@gmail.com>
---
 drivers/dma/zx_dma.c | 35 ++++++++++-------------------------
 1 file changed, 10 insertions(+), 25 deletions(-)

diff --git a/drivers/dma/zx_dma.c b/drivers/dma/zx_dma.c
index 9f4436f7c914..d8c2fbe9766c 100644
--- a/drivers/dma/zx_dma.c
+++ b/drivers/dma/zx_dma.c
@@ -18,6 +18,7 @@
 #include <linux/of.h>
 #include <linux/clk.h>
 #include <linux/of_dma.h>
+#include <linux/probe-helper.h>
 
 #include "virt-dma.h"
 
@@ -754,20 +755,17 @@ static struct dma_chan *zx_of_dma_simple_xlate(struct of_phandle_args *dma_spec,
 static int zx_dma_probe(struct platform_device *op)
 {
 	struct zx_dma_dev *d;
-	struct resource *iores;
 	int i, ret = 0;
 
-	iores = platform_get_resource(op, IORESOURCE_MEM, 0);
-	if (!iores)
-		return -EINVAL;
-
-	d = devm_kzalloc(&op->dev, sizeof(*d), GFP_KERNEL);
-	if (!d)
-		return -ENOMEM;
-
-	d->base = devm_ioremap_resource(&op->dev, iores);
-	if (IS_ERR(d->base))
-		return PTR_ERR(d->base);
+	/*
+	 * This macro internally combines following functions:
+	 * devm_kzalloc, platform_get_resource, devm_ioremap_resource,
+	 * devm_clk_get, platform_get_irq, devm_request_irq,
+	 */
+	ret = devm_platform_probe_helper_irq(op, d, NULL,
+		zx_dma_int_handler, 0, DRIVER_NAME, d);
+	if (ret < 0)
+		return ret;
 
 	of_property_read_u32((&op->dev)->of_node,
 			     "dma-channels", &d->dma_channels);
@@ -776,18 +774,6 @@ static int zx_dma_probe(struct platform_device *op)
 	if (!d->dma_requests || !d->dma_channels)
 		return -EINVAL;
 
-	d->clk = devm_clk_get(&op->dev, NULL);
-	if (IS_ERR(d->clk)) {
-		dev_err(&op->dev, "no dma clk\n");
-		return PTR_ERR(d->clk);
-	}
-
-	d->irq = platform_get_irq(op, 0);
-	ret = devm_request_irq(&op->dev, d->irq, zx_dma_int_handler,
-			       0, DRIVER_NAME, d);
-	if (ret)
-		return ret;
-
 	/* A DMA memory pool for LLIs, align on 32-byte boundary */
 	d->pool = dmam_pool_create(DRIVER_NAME, &op->dev,
 			LLI_BLOCK_SIZE, 32, 0);
@@ -894,7 +880,6 @@ static int zx_dma_remove(struct platform_device *op)
 		list_del(&c->vc.chan.device_node);
 	}
 	clk_disable_unprepare(d->clk);
-	dmam_pool_destroy(d->pool);
 
 	return 0;
 }
-- 
2.17.1

[PATCH 4/9] probe/dma/qcom-bam: removed redundant code from qcom bam dma controller's probe function

[Satendra Singh Thakur]

1. In order to remove duplicate code, following functions:
platform_get_resource
devm_kzalloc
devm_ioremap_resource
devm_clk_get
platform_get_irq
clk_prepare_enable
are replaced with a macro devm_platform_probe_helper_clk.

2. Renamed variables regs and bamclk so that helper macro can
be applied.

3. This patch depends on the file include/linux/probe-helper.h
which is pushed in previous patch [01/09].

Signed-off-by: Satendra Singh Thakur <satendrasingh.thakur@hcl.com>
Signed-off-by: Satendra Singh Thakur <sst2005@gmail.com>
---
 drivers/dma/qcom/bam_dma.c | 71 ++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 42 deletions(-)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index 8e90a405939d..06c136ca8e40 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -41,6 +41,7 @@
 #include <linux/clk.h>
 #include <linux/dmaengine.h>
 #include <linux/pm_runtime.h>
+#include <linux/probe-helper.h>
 
 #include "../dmaengine.h"
 #include "../virt-dma.h"
@@ -378,7 +379,7 @@ static inline struct bam_chan *to_bam_chan(struct dma_chan *common)
 }
 
 struct bam_device {
-	void __iomem *regs;
+	void __iomem *base;
 	struct device *dev;
 	struct dma_device common;
 	struct device_dma_parameters dma_parms;
@@ -392,7 +393,7 @@ struct bam_device {
 
 	const struct reg_offset_data *layout;
 
-	struct clk *bamclk;
+	struct clk *clk;
 	int irq;
 
 	/* dma start transaction tasklet */
@@ -410,7 +411,7 @@ static inline void __iomem *bam_addr(struct bam_device *bdev, u32 pipe,
 {
 	const struct reg_offset_data r = bdev->layout[reg];
 
-	return bdev->regs + r.base_offset +
+	return bdev->base + r.base_offset +
 		r.pipe_mult * pipe +
 		r.evnt_mult * pipe +
 		r.ee_mult * bdev->ee;
@@ -1209,41 +1210,41 @@ static int bam_dma_probe(struct platform_device *pdev)
 {
 	struct bam_device *bdev;
 	const struct of_device_id *match;
-	struct resource *iores;
 	int ret, i;
-
-	bdev = devm_kzalloc(&pdev->dev, sizeof(*bdev), GFP_KERNEL);
-	if (!bdev)
-		return -ENOMEM;
+	/*
+	 * This macro internally combines following functions:
+	 * devm_kzalloc, platform_get_resource, devm_ioremap_resource,
+	 * devm_clk_get, platform_get_irq, clk_prepare_enable
+	 */
+	ret = devm_platform_probe_helper_clk(pdev, bdev, "bam_clk");
+	bdev->controlled_remotely = of_property_read_bool(pdev->dev.of_node,
+						"qcom,controlled-remotely");
+	if (ret < 0) {
+		if (IS_ERR(bdev->clk)) {
+			if (!bdev->controlled_remotely)
+				return ret;
+			bdev->clk = NULL;
+		} else
+			return ret;
+	}
 
 	bdev->dev = &pdev->dev;
 
 	match = of_match_node(bam_of_match, pdev->dev.of_node);
 	if (!match) {
 		dev_err(&pdev->dev, "Unsupported BAM module\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto err_disable_clk;
 	}
 
 	bdev->layout = match->data;
 
-	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	bdev->regs = devm_ioremap_resource(&pdev->dev, iores);
-	if (IS_ERR(bdev->regs))
-		return PTR_ERR(bdev->regs);
-
-	bdev->irq = platform_get_irq(pdev, 0);
-	if (bdev->irq < 0)
-		return bdev->irq;
-
 	ret = of_property_read_u32(pdev->dev.of_node, "qcom,ee", &bdev->ee);
 	if (ret) {
 		dev_err(bdev->dev, "Execution environment unspecified\n");
-		return ret;
+		goto err_disable_clk;
 	}
 
-	bdev->controlled_remotely = of_property_read_bool(pdev->dev.of_node,
-						"qcom,controlled-remotely");
-
 	if (bdev->controlled_remotely) {
 		ret = of_property_read_u32(pdev->dev.of_node, "num-channels",
 					   &bdev->num_channels);
@@ -1256,20 +1257,6 @@ static int bam_dma_probe(struct platform_device *pdev)
 			dev_err(bdev->dev, "num-ees unspecified in dt\n");
 	}
 
-	bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk");
-	if (IS_ERR(bdev->bamclk)) {
-		if (!bdev->controlled_remotely)
-			return PTR_ERR(bdev->bamclk);
-
-		bdev->bamclk = NULL;
-	}
-
-	ret = clk_prepare_enable(bdev->bamclk);
-	if (ret) {
-		dev_err(bdev->dev, "failed to prepare/enable clock\n");
-		return ret;
-	}
-
 	ret = bam_init(bdev);
 	if (ret)
 		goto err_disable_clk;
@@ -1359,7 +1346,7 @@ static int bam_dma_probe(struct platform_device *pdev)
 err_tasklet_kill:
 	tasklet_kill(&bdev->task);
 err_disable_clk:
-	clk_disable_unprepare(bdev->bamclk);
+	clk_disable_unprepare(bdev->clk);
 
 	return ret;
 }
@@ -1393,7 +1380,7 @@ static int bam_dma_remove(struct platform_device *pdev)
 
 	tasklet_kill(&bdev->task);
 
-	clk_disable_unprepare(bdev->bamclk);
+	clk_disable_unprepare(bdev->clk);
 
 	return 0;
 }
@@ -1402,7 +1389,7 @@ static int __maybe_unused bam_dma_runtime_suspend(struct device *dev)
 {
 	struct bam_device *bdev = dev_get_drvdata(dev);
 
-	clk_disable(bdev->bamclk);
+	clk_disable(bdev->clk);
 
 	return 0;
 }
@@ -1412,7 +1399,7 @@ static int __maybe_unused bam_dma_runtime_resume(struct device *dev)
 	struct bam_device *bdev = dev_get_drvdata(dev);
 	int ret;
 
-	ret = clk_enable(bdev->bamclk);
+	ret = clk_enable(bdev->clk);
 	if (ret < 0) {
 		dev_err(dev, "clk_enable failed: %d\n", ret);
 		return ret;
@@ -1428,7 +1415,7 @@ static int __maybe_unused bam_dma_suspend(struct device *dev)
 	if (!bdev->controlled_remotely)
 		pm_runtime_force_suspend(dev);
 
-	clk_unprepare(bdev->bamclk);
+	clk_unprepare(bdev->clk);
 
 	return 0;
 }
@@ -1438,7 +1425,7 @@ static int __maybe_unused bam_dma_resume(struct device *dev)
 	struct bam_device *bdev = dev_get_drvdata(dev);
 	int ret;
 
-	ret = clk_prepare(bdev->bamclk);
+	ret = clk_prepare(bdev->clk);
 	if (ret)
 		return ret;
 
-- 
2.17.1

[PATCH 5/9] probe/dma/mtk-hs: removed redundant code from mediatek hs dma controller's probe function

[Satendra Singh Thakur]

1. In order to remove duplicate code, following functions:
platform_get_resource
devm_kzalloc
devm_ioremap_resource
devm_clk_get
platform_get_irq
are replaced with a macro devm_platform_probe_helper.

2. Fixed a memory leak when devm_request_irq fails,
Called of_dma_controller_free in such case.

3. This patch depends on the file include/linux/probe-helper.h
which is pushed in previous patch [01/09].

Signed-off-by: Satendra Singh Thakur <satendrasingh.thakur@hcl.com>
Signed-off-by: Satendra Singh Thakur <sst2005@gmail.com>
---
 drivers/dma/mediatek/mtk-hsdma.c | 38 ++++++++++----------------------
 1 file changed, 12 insertions(+), 26 deletions(-)

diff --git a/drivers/dma/mediatek/mtk-hsdma.c b/drivers/dma/mediatek/mtk-hsdma.c
index 1a2028e1c29e..6fc01093aeea 100644
--- a/drivers/dma/mediatek/mtk-hsdma.c
+++ b/drivers/dma/mediatek/mtk-hsdma.c
@@ -23,6 +23,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/refcount.h>
 #include <linux/slab.h>
+#include <linux/probe-helper.h>
 
 #include "../virt-dma.h"
 
@@ -896,41 +897,24 @@ static int mtk_hsdma_probe(struct platform_device *pdev)
 	struct mtk_hsdma_device *hsdma;
 	struct mtk_hsdma_vchan *vc;
 	struct dma_device *dd;
-	struct resource *res;
 	int i, err;
 
-	hsdma = devm_kzalloc(&pdev->dev, sizeof(*hsdma), GFP_KERNEL);
-	if (!hsdma)
-		return -ENOMEM;
-
+	/*
+	 * This macro internally combines following functions:
+	 * devm_kzalloc, platform_get_resource, devm_ioremap_resource,
+	 * devm_clk_get, platform_get_irq
+	 */
+	err = devm_platform_probe_helper(pdev, hsdma, "hsdma");
+	if (err < 0)
+		return err;
 	dd = &hsdma->ddev;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	hsdma->base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(hsdma->base))
-		return PTR_ERR(hsdma->base);
-
 	hsdma->soc = of_device_get_match_data(&pdev->dev);
 	if (!hsdma->soc) {
 		dev_err(&pdev->dev, "No device match found\n");
 		return -ENODEV;
 	}
 
-	hsdma->clk = devm_clk_get(&pdev->dev, "hsdma");
-	if (IS_ERR(hsdma->clk)) {
-		dev_err(&pdev->dev, "No clock for %s\n",
-			dev_name(&pdev->dev));
-		return PTR_ERR(hsdma->clk);
-	}
-
-	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (!res) {
-		dev_err(&pdev->dev, "No irq resource for %s\n",
-			dev_name(&pdev->dev));
-		return -EINVAL;
-	}
-	hsdma->irq = res->start;
-
 	refcount_set(&hsdma->pc_refcnt, 0);
 	spin_lock_init(&hsdma->lock);
 
@@ -997,7 +981,7 @@ static int mtk_hsdma_probe(struct platform_device *pdev)
 	if (err) {
 		dev_err(&pdev->dev,
 			"request_irq failed with err %d\n", err);
-		goto err_unregister;
+		goto err_free;
 	}
 
 	platform_set_drvdata(pdev, hsdma);
@@ -1006,6 +990,8 @@ static int mtk_hsdma_probe(struct platform_device *pdev)
 
 	return 0;
 
+err_free:
+	of_dma_controller_free(pdev->dev.of_node);
 err_unregister:
 	dma_async_device_unregister(dd);
 
-- 
2.17.1

[PATCH 6/9] probe/dma/sun6i: removed redundant code from sun6i dma controller's probe function

[Satendra Singh Thakur]

1. In order to remove duplicate code, following functions:
platform_get_resource
devm_kzalloc
devm_ioremap_resource
devm_clk_get
platform_get_irq
are replaced with a macro devm_platform_probe_helper.

2. This patch depends on the file include/linux/probe-helper.h
which is pushed in previous patch [01/09].

Signed-off-by: Satendra Singh Thakur <satendrasingh.thakur@hcl.com>
Signed-off-by: Satendra Singh Thakur <sst2005@gmail.com>
---
 drivers/dma/sun6i-dma.c | 30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index ed5b68dcfe50..41ee054bbeeb 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -19,6 +19,7 @@
 #include <linux/reset.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/probe-helper.h>
 
 #include "virt-dma.h"
 
@@ -1234,34 +1235,21 @@ static int sun6i_dma_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct sun6i_dma_dev *sdc;
-	struct resource *res;
 	int ret, i;
 
-	sdc = devm_kzalloc(&pdev->dev, sizeof(*sdc), GFP_KERNEL);
-	if (!sdc)
-		return -ENOMEM;
+	/*
+	 * This macro internally combines following functions:
+	 * devm_kzalloc, platform_get_resource, devm_ioremap_resource,
+	 * devm_clk_get, platform_get_irq
+	 */
+	ret = devm_platform_probe_helper(pdev, sdc, NULL);
+	if (ret < 0)
+		return ret;
 
 	sdc->cfg = of_device_get_match_data(&pdev->dev);
 	if (!sdc->cfg)
 		return -ENODEV;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	sdc->base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(sdc->base))
-		return PTR_ERR(sdc->base);
-
-	sdc->irq = platform_get_irq(pdev, 0);
-	if (sdc->irq < 0) {
-		dev_err(&pdev->dev, "Cannot claim IRQ\n");
-		return sdc->irq;
-	}
-
-	sdc->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(sdc->clk)) {
-		dev_err(&pdev->dev, "No clock specified\n");
-		return PTR_ERR(sdc->clk);
-	}
-
 	if (sdc->cfg->has_mbus_clk) {
 		sdc->clk_mbus = devm_clk_get(&pdev->dev, "mbus");
 		if (IS_ERR(sdc->clk_mbus)) {
-- 
2.17.1

[PATCH 7/9] probe/dma/sun4i: removed redundant code from sun4i dma controller's probe function

[Satendra Singh Thakur]

1. In order to remove duplicate code, following functions:
platform_get_resource
devm_kzalloc
devm_ioremap_resource
devm_clk_get
platform_get_irq
are replaced with a macro devm_platform_probe_helper.

2. This patch depends on the file include/linux/probe-helper.h
which is pushed in previous patch [01/09].

Signed-off-by: Satendra Singh Thakur <satendrasingh.thakur@hcl.com>
Signed-off-by: Satendra Singh Thakur <sst2005@gmail.com>
---
 drivers/dma/sun4i-dma.c | 30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/drivers/dma/sun4i-dma.c b/drivers/dma/sun4i-dma.c
index 1f80568b2613..5db139ff43ac 100644
--- a/drivers/dma/sun4i-dma.c
+++ b/drivers/dma/sun4i-dma.c
@@ -15,6 +15,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/probe-helper.h>
 
 #include "virt-dma.h"
 
@@ -1119,29 +1120,16 @@ static irqreturn_t sun4i_dma_interrupt(int irq, void *dev_id)
 static int sun4i_dma_probe(struct platform_device *pdev)
 {
 	struct sun4i_dma_dev *priv;
-	struct resource *res;
 	int i, j, ret;
 
-	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	priv->base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(priv->base))
-		return PTR_ERR(priv->base);
-
-	priv->irq = platform_get_irq(pdev, 0);
-	if (priv->irq < 0) {
-		dev_err(&pdev->dev, "Cannot claim IRQ\n");
-		return priv->irq;
-	}
-
-	priv->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(priv->clk)) {
-		dev_err(&pdev->dev, "No clock specified\n");
-		return PTR_ERR(priv->clk);
-	}
+	/*
+	 * This macro internally combines following functions:
+	 * devm_kzalloc, platform_get_resource, devm_ioremap_resource,
+	 * devm_clk_get, platform_get_irq
+	 */
+	ret = devm_platform_probe_helper(pdev, priv, NULL);
+	if (ret < 0)
+		return ret;
 
 	platform_set_drvdata(pdev, priv);
 	spin_lock_init(&priv->lock);
-- 
2.17.1

[PATCH 8/9] probe/dma/axi: removed redundant code from axi dma controller's probe function

[Satendra Singh Thakur]

1. In order to remove duplicate code, following functions:
platform_get_resource
devm_kzalloc
devm_ioremap_resource
devm_clk_get
platform_get_irq
clk_prepare_enable
are replaced with a macro devm_platform_probe_helper.

2. This patch depends on the file include/linux/probe-helper.h
which is pushed in previous patch [01/09].

Signed-off-by: Satendra Singh Thakur <satendrasingh.thakur@hcl.com>
Signed-off-by: Satendra Singh Thakur <sst2005@gmail.com>
---
 drivers/dma/dma-axi-dmac.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/dma/dma-axi-dmac.c b/drivers/dma/dma-axi-dmac.c
index a0ee404b736e..ac8a2355b299 100644
--- a/drivers/dma/dma-axi-dmac.c
+++ b/drivers/dma/dma-axi-dmac.c
@@ -21,6 +21,7 @@
 #include <linux/regmap.h>
 #include <linux/slab.h>
 #include <linux/fpga/adi-axi-common.h>
+#include <linux/probe-helper.h>
 
 #include <dt-bindings/dma/axi-dmac.h>
 
@@ -829,28 +830,19 @@ static int axi_dmac_probe(struct platform_device *pdev)
 	struct device_node *of_channels, *of_chan;
 	struct dma_device *dma_dev;
 	struct axi_dmac *dmac;
-	struct resource *res;
 	int ret;
 
-	dmac = devm_kzalloc(&pdev->dev, sizeof(*dmac), GFP_KERNEL);
-	if (!dmac)
-		return -ENOMEM;
-
-	dmac->irq = platform_get_irq(pdev, 0);
-	if (dmac->irq < 0)
-		return dmac->irq;
-	if (dmac->irq == 0)
+	/*
+	 * This macro internally combines following functions:
+	 * devm_kzalloc, platform_get_resource, devm_ioremap_resource,
+	 * devm_clk_get, platform_get_irq
+	 */
+	ret = devm_platform_probe_helper(pdev, dmac, NULL);
+	if (ret < 0)
+		return ret;
+	else if (!dmac->irq)
 		return -EINVAL;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	dmac->base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(dmac->base))
-		return PTR_ERR(dmac->base);
-
-	dmac->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(dmac->clk))
-		return PTR_ERR(dmac->clk);
-
 	INIT_LIST_HEAD(&dmac->chan.active_descs);
 
 	of_channels = of_get_child_by_name(pdev->dev.of_node, "adi,channels");
-- 
2.17.1

[PATCH 9/9] probe/dma/owl: removed redundant code from owl dma controller's probe function

[Satendra Singh Thakur]

1. In order to remove duplicate code, following functions:
platform_get_resource
devm_kzalloc
devm_ioremap_resource
devm_clk_get
platform_get_irq
are replaced with a macro devm_platform_probe_helper.

2. This patch depends on the file include/linux/probe-helper.h
which is pushed in previous patch [01/09].

Signed-off-by: Satendra Singh Thakur <satendrasingh.thakur@hcl.com>
Signed-off-by: Satendra Singh Thakur <sst2005@gmail.com>
---
 drivers/dma/owl-dma.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/dma/owl-dma.c b/drivers/dma/owl-dma.c
index 90bbcef99ef8..03e692fc25a1 100644
--- a/drivers/dma/owl-dma.c
+++ b/drivers/dma/owl-dma.c
@@ -23,6 +23,7 @@
 #include <linux/of_device.h>
 #include <linux/of_dma.h>
 #include <linux/slab.h>
+#include <linux/probe-helper.h>
 #include "virt-dma.h"
 
 #define OWL_DMA_FRAME_MAX_LENGTH		0xfffff
@@ -1045,20 +1046,15 @@ static int owl_dma_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct owl_dma *od;
-	struct resource *res;
 	int ret, i, nr_channels, nr_requests;
-
-	od = devm_kzalloc(&pdev->dev, sizeof(*od), GFP_KERNEL);
-	if (!od)
-		return -ENOMEM;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
-		return -EINVAL;
-
-	od->base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(od->base))
-		return PTR_ERR(od->base);
+	/*
+	 * This macro internally combines following functions:
+	 * devm_kzalloc, platform_get_resource, devm_ioremap_resource,
+	 * devm_clk_get, platform_get_irq
+	 */
+	ret = devm_platform_probe_helper(pdev, od, NULL);
+	if (ret < 0)
+		return ret;
 
 	ret = of_property_read_u32(np, "dma-channels", &nr_channels);
 	if (ret) {
@@ -1105,18 +1101,11 @@ static int owl_dma_probe(struct platform_device *pdev)
 
 	INIT_LIST_HEAD(&od->dma.channels);
 
-	od->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(od->clk)) {
-		dev_err(&pdev->dev, "unable to get clock\n");
-		return PTR_ERR(od->clk);
-	}
-
 	/*
 	 * Eventhough the DMA controller is capable of generating 4
 	 * IRQ's for DMA priority feature, we only use 1 IRQ for
 	 * simplification.
 	 */
-	od->irq = platform_get_irq(pdev, 0);
 	ret = devm_request_irq(&pdev->dev, od->irq, owl_dma_interrupt, 0,
 			       dev_name(&pdev->dev), od);
 	if (ret) {
-- 
2.17.1

Re: [PATCH 0/9] added helper macros to remove duplicate code from probe functions of the platform drivers

[Vinod Koul]

On 15-09-19, 12:30, Satendra Singh Thakur wrote:
> 1. For most of the platform drivers's probe include following steps
> 
> -memory allocation for driver's private structure
> -getting io resources
> -io remapping resources
> -getting irq number
> -registering irq
> -setting driver's private data
> -getting clock
> -preparing and enabling clock

And we have perfect set of APIs for these tasks!

> 2. We have defined a set of macros to combine some or all of
> the above mentioned steps. This will remove redundant/duplicate
> code in drivers' probe functions of platform drivers.

Why, how does it help and you do realize it also introduces bugs

> devm_platform_probe_helper(pdev, priv, clk_name);
> devm_platform_probe_helper_clk(pdev, priv, clk_name);
> devm_platform_probe_helper_irq(pdev, priv, clk_name,
> irq_hndlr, irq_flags, irq_name, irq_devid);
> devm_platform_probe_helper_all(pdev, priv, clk_name,
> irq_hndlr, irq_flags, irq_name, irq_devid);
> devm_platform_probe_helper_all_data(pdev, priv, clk_name,
> irq_hndlr, irq_flags, irq_name, irq_devid);
> 
> 3. Code is made devres compatible (wherever required)
> The functions: clk_get, request_irq, kzalloc, platform_get_resource
> are replaced with their devm_* counterparts.

We already have devres APIs for people to use!
> 
> 4. Few bugs are also fixed.

Which ones..?

> 
> Satendra Singh Thakur (9):
>   probe/dma : added helper macros to remove redundant/duplicate code
>     from probe functions of the dma controller drivers
>   probe/dma/jz4740: removed redundant code from jz4740 dma controller's 
>        probe function
>   probe/dma/zx: removed redundant code from zx dma controller's probe
>     function
>   probe/dma/qcom-bam: removed redundant code from qcom bam dma
>     controller's probe function
>   probe/dma/mtk-hs: removed redundant code from mediatek hs dma
>     controller's probe function
>   probe/dma/sun6i: removed redundant code from sun6i dma controller's
>     probe function
>   probe/dma/sun4i: removed redundant code from sun4i dma controller's
>     probe function
>   probe/dma/axi: removed redundant code from axi dma controller's probe
>     function
>   probe/dma/owl: removed redundant code from owl dma controller's probe
>     function
> 
>  drivers/dma/dma-axi-dmac.c       |  28 ++---
>  drivers/dma/dma-jz4740.c         |  33 +++---
>  drivers/dma/mediatek/mtk-hsdma.c |  38 +++----
>  drivers/dma/owl-dma.c            |  29 ++---
>  drivers/dma/qcom/bam_dma.c       |  71 +++++-------
>  drivers/dma/sun4i-dma.c          |  30 ++----
>  drivers/dma/sun6i-dma.c          |  30 ++----
>  drivers/dma/zx_dma.c             |  35 ++----
>  include/linux/probe-helper.h     | 179 +++++++++++++++++++++++++++++++
>  9 files changed, 280 insertions(+), 193 deletions(-)
>  create mode 100644 include/linux/probe-helper.h
> 
> -- 
> 2.17.1

-- 
~Vinod

Re: Re: [PATCH 0/9] added helper macros to remove duplicate code from probe functions of the platform drivers

[Satendra Singh Thakur]

On Wed, Sep 18, 2019 at 3:57 PM, Vinod Koul wrote:
> On 15-09-19, 12:30, Satendra Singh Thakur wrote:
> > 1. For most of the platform drivers's probe include following steps
> > 
> > -memory allocation for driver's private structure
> > -getting io resources
> > -io remapping resources
> > -getting irq number
> > -registering irq
> > -setting driver's private data
> > -getting clock
> > -preparing and enabling clock
>
> And we have perfect set of APIs for these tasks!
Hi Vinod,
Thanks for the comments.
You are right, we already have set of APIs for these tasks.
The proposed macros combine the very same APIs to remove 
duplicate/redundant code.
A new driver author can use these macros to easily write probe 
function without having to worry about signatures of internal APIs.
In the past, people have combined some of them e.g.
a) clk_prepare_enable combines clk_prepare and clk_enable
b) devm_platform_ioremap_resource combines
platform_get_resource (for type IORESOURCE_MEM)
and devm_ioremap_resource
c) module_platform_driver macro encompasses module_init/exit 
and driver_register/unregister functions.
The basic idea is to simplyfy coding.
> > 2. We have defined a set of macros to combine some or all of
> > the above mentioned steps. This will remove redundant/duplicate
> > code in drivers' probe functions of platform drivers.
> 
> Why, how does it help and you do realize it also introduces bugs
This will make probe function shorter by removing repeated code.
This will also reduce bugs caused due to improper handling
of failure cases because of these reasons:
a) If the developer calls each API individualy one might miss
proper handling of the failure for some API; Whereas the macro
properly handles failure of each API.
b) Macros are devres compatible which leaves less room for
memory leaks.

Yes, the macros which enable clock and request irqs
might cause bugs if they are not used carefully.
For instance, enabling the clock or requesting the irq
earlier than actually required. So, the macros with _clk
and _irq, _all suffix should be used carefully.

Please let me know if I miss any specific type of bug
here.
> 
> > devm_platform_probe_helper(pdev, priv, clk_name);
> > devm_platform_probe_helper_clk(pdev, priv, clk_name);
> > devm_platform_probe_helper_irq(pdev, priv, clk_name,
> > irq_hndlr, irq_flags, irq_name, irq_devid);
> > devm_platform_probe_helper_all(pdev, priv, clk_name,
> > irq_hndlr, irq_flags, irq_name, irq_devid);
> > devm_platform_probe_helper_all_data(pdev, priv, clk_name,
> > irq_hndlr, irq_flags, irq_name, irq_devid);
> > 
> > 3. Code is made devres compatible (wherever required)
> > The functions: clk_get, request_irq, kzalloc, platform_get_resource
> > are replaced with their devm_* counterparts.
> 
> We already have devres APIs for people to use!
Yes, we have devres APIs and many drivers do use them.
But still there are many which don't use them.
The proposed macros provides just another way to use devres APIs.
> > 
> > 4. Few bugs are also fixed.
> 
> Which ones..?
The bug is that the failure of request_irq 
is not handled properly in mtk-hsdma.c. This is fixed in patch [5/9].
https://lkml.org/lkml/2019/9/15/35

Please let me know if I am missing something here.
Thanks
-Satendra 

Re: Re: [PATCH 0/9] added helper macros to remove duplicate code from probe functions of the platform drivers

[Vinod Koul]

On 21-09-19, 20:27, Satendra Singh Thakur wrote:
> On Wed, Sep 18, 2019 at 3:57 PM, Vinod Koul wrote:
> > On 15-09-19, 12:30, Satendra Singh Thakur wrote:
> > > 1. For most of the platform drivers's probe include following steps
> > > 
> > > -memory allocation for driver's private structure
> > > -getting io resources
> > > -io remapping resources
> > > -getting irq number
> > > -registering irq
> > > -setting driver's private data
> > > -getting clock
> > > -preparing and enabling clock
> >
> > And we have perfect set of APIs for these tasks!
> Hi Vinod,
> Thanks for the comments.
> You are right, we already have set of APIs for these tasks.
> The proposed macros combine the very same APIs to remove 
> duplicate/redundant code.
> A new driver author can use these macros to easily write probe 

Nope they can write each APIs, know the exact flow and do it!

> function without having to worry about signatures of internal APIs.
> In the past, people have combined some of them e.g.
> a) clk_prepare_enable combines clk_prepare and clk_enable

As it is clock, it is called in sequence, whereas different drivers may
have different sequencing.

Btw I am not for adding maanged irq functions, they are really not the
correct way to manage!

> b) devm_platform_ioremap_resource combines
> platform_get_resource (for type IORESOURCE_MEM)
> and devm_ioremap_resource
> c) module_platform_driver macro encompasses module_init/exit 
> and driver_register/unregister functions.

All examples you are quoting do a set of functions (clk, resources etc
and not do N things!

> The basic idea is to simplyfy coding.
> > > 2. We have defined a set of macros to combine some or all of
> > > the above mentioned steps. This will remove redundant/duplicate
> > > code in drivers' probe functions of platform drivers.
> > 
> > Why, how does it help and you do realize it also introduces bugs
> This will make probe function shorter by removing repeated code.
> This will also reduce bugs caused due to improper handling
> of failure cases because of these reasons:
> a) If the developer calls each API individualy one might miss
> proper handling of the failure for some API; Whereas the macro
> properly handles failure of each API.
> b) Macros are devres compatible which leaves less room for
> memory leaks.

No we review the code and if we miss we fix later!

> Yes, the macros which enable clock and request irqs
> might cause bugs if they are not used carefully.
> For instance, enabling the clock or requesting the irq
> earlier than actually required. So, the macros with _clk
> and _irq, _all suffix should be used carefully.

Precisely!


> Please let me know if I miss any specific type of bug
> here.
> > 
> > > devm_platform_probe_helper(pdev, priv, clk_name);
> > > devm_platform_probe_helper_clk(pdev, priv, clk_name);
> > > devm_platform_probe_helper_irq(pdev, priv, clk_name,
> > > irq_hndlr, irq_flags, irq_name, irq_devid);
> > > devm_platform_probe_helper_all(pdev, priv, clk_name,
> > > irq_hndlr, irq_flags, irq_name, irq_devid);
> > > devm_platform_probe_helper_all_data(pdev, priv, clk_name,
> > > irq_hndlr, irq_flags, irq_name, irq_devid);
> > > 
> > > 3. Code is made devres compatible (wherever required)
> > > The functions: clk_get, request_irq, kzalloc, platform_get_resource
> > > are replaced with their devm_* counterparts.
> > 
> > We already have devres APIs for people to use!
> Yes, we have devres APIs and many drivers do use them.
> But still there are many which don't use them.
> The proposed macros provides just another way to use devres APIs.
> > > 
> > > 4. Few bugs are also fixed.
> > 
> > Which ones..?
> The bug is that the failure of request_irq 
> is not handled properly in mtk-hsdma.c. This is fixed in patch [5/9].
> https://lkml.org/lkml/2019/9/15/35

Please send the fix individually and I would be glad to review.

-- 
~Vinod