[PATCH -next 0/6] spi: Switch to use devm_spi_alloc_master() in some drivers

[PATCH -next 0/6] spi: Switch to use devm_spi_alloc_master() in some drivers

[Yang Yingliang]

This patchset is trying to replace spi_alloc_master() with
devm_spi_alloc_master() in some spi drivers. With this helper,
spi_master_put() is called in devres_release_all() whenever
the device is unbound, so the spi_master_put() in error path
can be removed.

Yang Yingliang (6):
  spi: oc-tiny: Switch to use devm_spi_alloc_master()
  spi: ath79: Switch to use devm_spi_alloc_master()
  spi: omap-uwire: Switch to use devm_spi_alloc_master()
  spi: ppc4xx: Switch to use devm_spi_alloc_master()
  spi: sh-sci: Switch to use devm_spi_alloc_master()
  spi: altera: Switch to use devm_spi_alloc_master()

 drivers/spi/spi-altera-platform.c | 23 ++++++++-------------
 drivers/spi/spi-ath79.c           | 18 ++++++----------
 drivers/spi/spi-oc-tiny.c         | 18 ++++++----------
 drivers/spi/spi-omap-uwire.c      |  6 ++----
 drivers/spi/spi-ppc4xx.c          | 17 ++++++----------
 drivers/spi/spi-sh-sci.c          | 34 ++++++++++++-------------------
 6 files changed, 41 insertions(+), 75 deletions(-)

-- 
2.25.1

[PATCH -next 1/6] spi: oc-tiny: Switch to use devm_spi_alloc_master()

[Yang Yingliang]

Switch to use devm_spi_alloc_master() to simpify error path.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/spi/spi-oc-tiny.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/spi/spi-oc-tiny.c b/drivers/spi/spi-oc-tiny.c
index 38c14c4e4e21..1ab807741d26 100644
--- a/drivers/spi/spi-oc-tiny.c
+++ b/drivers/spi/spi-oc-tiny.c
@@ -215,7 +215,7 @@ static int tiny_spi_probe(struct platform_device *pdev)
 	struct spi_master *master;
 	int err = -ENODEV;
 
-	master = spi_alloc_master(&pdev->dev, sizeof(struct tiny_spi));
+	master = devm_spi_alloc_master(&pdev->dev, sizeof(struct tiny_spi));
 	if (!master)
 		return err;
 
@@ -235,10 +235,8 @@ static int tiny_spi_probe(struct platform_device *pdev)
 
 	/* find and map our resources */
 	hw->base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(hw->base)) {
-		err = PTR_ERR(hw->base);
-		goto exit;
-	}
+	if (IS_ERR(hw->base))
+		return PTR_ERR(hw->base);
 	/* irq is optional */
 	hw->irq = platform_get_irq(pdev, 0);
 	if (hw->irq >= 0) {
@@ -246,7 +244,7 @@ static int tiny_spi_probe(struct platform_device *pdev)
 		err = devm_request_irq(&pdev->dev, hw->irq, tiny_spi_irq, 0,
 				       pdev->name, hw);
 		if (err)
-			goto exit;
+			return err;
 	}
 	/* find platform data */
 	if (platp) {
@@ -255,20 +253,16 @@ static int tiny_spi_probe(struct platform_device *pdev)
 	} else {
 		err = tiny_spi_of_probe(pdev);
 		if (err)
-			goto exit;
+			return err;
 	}
 
 	/* register our spi controller */
 	err = spi_bitbang_start(&hw->bitbang);
 	if (err)
-		goto exit;
+		return err;
 	dev_info(&pdev->dev, "base %p, irq %d\n", hw->base, hw->irq);
 
 	return 0;
-
-exit:
-	spi_master_put(master);
-	return err;
 }
 
 static int tiny_spi_remove(struct platform_device *pdev)
-- 
2.25.1

[PATCH -next 2/6] spi: ath79: Switch to use devm_spi_alloc_master()

[Yang Yingliang]

Switch to use devm_spi_alloc_master() to simpify error path.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/spi/spi-ath79.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/spi/spi-ath79.c b/drivers/spi/spi-ath79.c
index 607e7a49fb89..4cc5c4e8504e 100644
--- a/drivers/spi/spi-ath79.c
+++ b/drivers/spi/spi-ath79.c
@@ -173,7 +173,7 @@ static int ath79_spi_probe(struct platform_device *pdev)
 	unsigned long rate;
 	int ret;
 
-	master = spi_alloc_master(&pdev->dev, sizeof(*sp));
+	master = devm_spi_alloc_master(&pdev->dev, sizeof(*sp));
 	if (master == NULL) {
 		dev_err(&pdev->dev, "failed to allocate spi master\n");
 		return -ENOMEM;
@@ -195,20 +195,16 @@ static int ath79_spi_probe(struct platform_device *pdev)
 	sp->bitbang.flags = SPI_CS_HIGH;
 
 	sp->base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(sp->base)) {
-		ret = PTR_ERR(sp->base);
-		goto err_put_master;
-	}
+	if (IS_ERR(sp->base))
+		return PTR_ERR(sp->base);
 
 	sp->clk = devm_clk_get(&pdev->dev, "ahb");
-	if (IS_ERR(sp->clk)) {
-		ret = PTR_ERR(sp->clk);
-		goto err_put_master;
-	}
+	if (IS_ERR(sp->clk))
+		return PTR_ERR(sp->clk);
 
 	ret = clk_prepare_enable(sp->clk);
 	if (ret)
-		goto err_put_master;
+		return ret;
 
 	rate = DIV_ROUND_UP(clk_get_rate(sp->clk), MHZ);
 	if (!rate) {
@@ -231,8 +227,6 @@ static int ath79_spi_probe(struct platform_device *pdev)
 	ath79_spi_disable(sp);
 err_clk_disable:
 	clk_disable_unprepare(sp->clk);
-err_put_master:
-	spi_master_put(sp->bitbang.master);
 
 	return ret;
 }
-- 
2.25.1

[PATCH -next 3/6] spi: omap-uwire: Switch to use devm_spi_alloc_master()

[Yang Yingliang]

Switch to use devm_spi_alloc_master() to simpify error path.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/spi/spi-omap-uwire.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-omap-uwire.c b/drivers/spi/spi-omap-uwire.c
index 29198e6815b2..f1e66f758d45 100644
--- a/drivers/spi/spi-omap-uwire.c
+++ b/drivers/spi/spi-omap-uwire.c
@@ -448,7 +448,6 @@ static void uwire_off(struct uwire_spi *uwire)
 {
 	uwire_write_reg(UWIRE_SR3, 0);
 	clk_disable_unprepare(uwire->ck);
-	spi_master_put(uwire->bitbang.master);
 }
 
 static int uwire_probe(struct platform_device *pdev)
@@ -457,7 +456,7 @@ static int uwire_probe(struct platform_device *pdev)
 	struct uwire_spi	*uwire;
 	int			status;
 
-	master = spi_alloc_master(&pdev->dev, sizeof(*uwire));
+	master = devm_spi_alloc_master(&pdev->dev, sizeof(*uwire));
 	if (!master)
 		return -ENODEV;
 
@@ -466,7 +465,6 @@ static int uwire_probe(struct platform_device *pdev)
 	uwire_base = devm_ioremap(&pdev->dev, UWIRE_BASE_PHYS, UWIRE_IO_SIZE);
 	if (!uwire_base) {
 		dev_dbg(&pdev->dev, "can't ioremap UWIRE\n");
-		spi_master_put(master);
 		return -ENOMEM;
 	}
 
@@ -476,7 +474,6 @@ static int uwire_probe(struct platform_device *pdev)
 	if (IS_ERR(uwire->ck)) {
 		status = PTR_ERR(uwire->ck);
 		dev_dbg(&pdev->dev, "no functional clock?\n");
-		spi_master_put(master);
 		return status;
 	}
 	clk_prepare_enable(uwire->ck);
@@ -518,6 +515,7 @@ static int uwire_remove(struct platform_device *pdev)
 
 	spi_bitbang_stop(&uwire->bitbang);
 	uwire_off(uwire);
+	spi_master_put(uwire->bitbang.master);
 	return 0;
 }
 
-- 
2.25.1

[PATCH -next 4/6] spi: ppc4xx: Switch to use devm_spi_alloc_master()

[Yang Yingliang]

Switch to use devm_spi_alloc_master() to simpify error path.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/spi/spi-ppc4xx.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-ppc4xx.c b/drivers/spi/spi-ppc4xx.c
index d65f047b6c82..b49db0acdbdb 100644
--- a/drivers/spi/spi-ppc4xx.c
+++ b/drivers/spi/spi-ppc4xx.c
@@ -349,7 +349,7 @@ static int spi_ppc4xx_of_probe(struct platform_device *op)
 	int ret;
 	const unsigned int *clk;
 
-	master = spi_alloc_master(dev, sizeof(*hw));
+	master = devm_spi_alloc_master(dev, sizeof(*hw));
 	if (master == NULL)
 		return -ENOMEM;
 	master->dev.of_node = np;
@@ -384,16 +384,14 @@ static int spi_ppc4xx_of_probe(struct platform_device *op)
 	opbnp = of_find_compatible_node(NULL, NULL, "ibm,opb");
 	if (opbnp == NULL) {
 		dev_err(dev, "OPB: cannot find node\n");
-		ret = -ENODEV;
-		goto free_master;
+		return -ENODEV;
 	}
 	/* Get the clock (Hz) for the OPB */
 	clk = of_get_property(opbnp, "clock-frequency", NULL);
 	if (clk == NULL) {
 		dev_err(dev, "OPB: no clock-frequency property set\n");
 		of_node_put(opbnp);
-		ret = -ENODEV;
-		goto free_master;
+		return -ENODEV;
 	}
 	hw->opb_freq = *clk;
 	hw->opb_freq >>= 2;
@@ -402,7 +400,7 @@ static int spi_ppc4xx_of_probe(struct platform_device *op)
 	ret = of_address_to_resource(np, 0, &resource);
 	if (ret) {
 		dev_err(dev, "error while parsing device node resource\n");
-		goto free_master;
+		return ret;
 	}
 	hw->mapbase = resource.start;
 	hw->mapsize = resource_size(&resource);
@@ -410,8 +408,7 @@ static int spi_ppc4xx_of_probe(struct platform_device *op)
 	/* Sanity check */
 	if (hw->mapsize < sizeof(struct spi_ppc4xx_regs)) {
 		dev_err(dev, "too small to map registers\n");
-		ret = -EINVAL;
-		goto free_master;
+		return -EINVAL;
 	}
 
 	/* Request IRQ */
@@ -420,7 +417,7 @@ static int spi_ppc4xx_of_probe(struct platform_device *op)
 			  0, "spi_ppc4xx_of", (void *)hw);
 	if (ret) {
 		dev_err(dev, "unable to allocate interrupt\n");
-		goto free_master;
+		return ret;
 	}
 
 	if (!request_mem_region(hw->mapbase, hw->mapsize, DRIVER_NAME)) {
@@ -457,8 +454,6 @@ static int spi_ppc4xx_of_probe(struct platform_device *op)
 	release_mem_region(hw->mapbase, hw->mapsize);
 request_mem_error:
 	free_irq(hw->irqnum, hw);
-free_master:
-	spi_master_put(master);
 
 	dev_err(dev, "initialization failed\n");
 	return ret;
-- 
2.25.1

[PATCH -next 5/6] spi: sh-sci: Switch to use devm_spi_alloc_master()

[Yang Yingliang]

Switch to use devm_spi_alloc_master() to simpify error path.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/spi/spi-sh-sci.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

diff --git a/drivers/spi/spi-sh-sci.c b/drivers/spi/spi-sh-sci.c
index 8f30531e1418..2cd5cd23e37c 100644
--- a/drivers/spi/spi-sh-sci.c
+++ b/drivers/spi/spi-sh-sci.c
@@ -118,11 +118,10 @@ static int sh_sci_spi_probe(struct platform_device *dev)
 	struct sh_sci_spi *sp;
 	int ret;
 
-	master = spi_alloc_master(&dev->dev, sizeof(struct sh_sci_spi));
+	master = devm_spi_alloc_master(&dev->dev, sizeof(struct sh_sci_spi));
 	if (master == NULL) {
 		dev_err(&dev->dev, "failed to allocate spi master\n");
-		ret = -ENOMEM;
-		goto err0;
+		return -ENOMEM;
 	}
 
 	sp = spi_master_get_devdata(master);
@@ -131,8 +130,7 @@ static int sh_sci_spi_probe(struct platform_device *dev)
 	sp->info = dev_get_platdata(&dev->dev);
 	if (!sp->info) {
 		dev_err(&dev->dev, "platform data is missing\n");
-		ret = -ENOENT;
-		goto err1;
+		return -ENOENT;
 	}
 
 	/* setup spi bitbang adaptor */
@@ -147,28 +145,22 @@ static int sh_sci_spi_probe(struct platform_device *dev)
 	sp->bitbang.txrx_word[SPI_MODE_3] = sh_sci_spi_txrx_mode3;
 
 	r = platform_get_resource(dev, IORESOURCE_MEM, 0);
-	if (r == NULL) {
-		ret = -ENOENT;
-		goto err1;
-	}
+	if (r == NULL)
+		return -ENOENT;
 	sp->membase = ioremap(r->start, resource_size(r));
-	if (!sp->membase) {
-		ret = -ENXIO;
-		goto err1;
-	}
+	if (!sp->membase)
+		return -ENXIO;
 	sp->val = ioread8(SCSPTR(sp));
 	setbits(sp, PIN_INIT, 1);
 
 	ret = spi_bitbang_start(&sp->bitbang);
-	if (!ret)
-		return 0;
+	if (ret) {
+		setbits(sp, PIN_INIT, 0);
+		iounmap(sp->membase);
+		return ret;
+	}
 
-	setbits(sp, PIN_INIT, 0);
-	iounmap(sp->membase);
- err1:
-	spi_master_put(sp->bitbang.master);
- err0:
-	return ret;
+	return 0;
 }
 
 static int sh_sci_spi_remove(struct platform_device *dev)
-- 
2.25.1

[PATCH -next 6/6] spi: altera: Switch to use devm_spi_alloc_master()

[Yang Yingliang]

Switch to use devm_spi_alloc_master() to simpify error path.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/spi/spi-altera-platform.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/spi/spi-altera-platform.c b/drivers/spi/spi-altera-platform.c
index 65147aae82a1..376df7b6f543 100644
--- a/drivers/spi/spi-altera-platform.c
+++ b/drivers/spi/spi-altera-platform.c
@@ -43,7 +43,7 @@ static int altera_spi_probe(struct platform_device *pdev)
 	int err = -ENODEV;
 	u16 i;
 
-	master = spi_alloc_master(&pdev->dev, sizeof(struct altera_spi));
+	master = devm_spi_alloc_master(&pdev->dev, sizeof(struct altera_spi));
 	if (!master)
 		return err;
 
@@ -55,8 +55,7 @@ static int altera_spi_probe(struct platform_device *pdev)
 			dev_err(&pdev->dev,
 				"Invalid number of chipselect: %u\n",
 				pdata->num_chipselect);
-			err = -EINVAL;
-			goto exit;
+			return -EINVAL;
 		}
 
 		master->num_chipselect = pdata->num_chipselect;
@@ -83,7 +82,7 @@ static int altera_spi_probe(struct platform_device *pdev)
 		hw->regmap = dev_get_regmap(pdev->dev.parent, NULL);
 		if (!hw->regmap) {
 			dev_err(&pdev->dev, "get regmap failed\n");
-			goto exit;
+			return err;
 		}
 
 		regoff = platform_get_resource(pdev, IORESOURCE_REG, 0);
@@ -93,17 +92,14 @@ static int altera_spi_probe(struct platform_device *pdev)
 		void __iomem *res;
 
 		res = devm_platform_ioremap_resource(pdev, 0);
-		if (IS_ERR(res)) {
-			err = PTR_ERR(res);
-			goto exit;
-		}
+		if (IS_ERR(res))
+			return PTR_ERR(res);
 
 		hw->regmap = devm_regmap_init_mmio(&pdev->dev, res,
 						   &spi_altera_config);
 		if (IS_ERR(hw->regmap)) {
 			dev_err(&pdev->dev, "regmap mmio init failed\n");
-			err = PTR_ERR(hw->regmap);
-			goto exit;
+			return PTR_ERR(hw->regmap);
 		}
 	}
 
@@ -115,12 +111,12 @@ static int altera_spi_probe(struct platform_device *pdev)
 		err = devm_request_irq(&pdev->dev, hw->irq, altera_spi_irq, 0,
 				       pdev->name, master);
 		if (err)
-			goto exit;
+			return err;
 	}
 
 	err = devm_spi_register_master(&pdev->dev, master);
 	if (err)
-		goto exit;
+		return err;
 
 	if (pdata) {
 		for (i = 0; i < pdata->num_devices; i++) {
@@ -134,9 +130,6 @@ static int altera_spi_probe(struct platform_device *pdev)
 	dev_info(&pdev->dev, "regoff %u, irq %d\n", hw->regoff, hw->irq);
 
 	return 0;
-exit:
-	spi_master_put(master);
-	return err;
 }
 
 #ifdef CONFIG_OF
-- 
2.25.1

Re: [PATCH -next 0/6] spi: Switch to use devm_spi_alloc_master() in some drivers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 435 bytes --]

On Tue, Sep 20, 2022 at 09:48:13PM +0800, Yang Yingliang wrote:
> This patchset is trying to replace spi_alloc_master() with
> devm_spi_alloc_master() in some spi drivers. With this helper,
> spi_master_put() is called in devres_release_all() whenever
> the device is unbound, so the spi_master_put() in error path
> can be removed.

If we're switching please update to the modern naming and use
"controller" rather than the old name.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH -next 0/6] spi: Switch to use devm_spi_alloc_master() in some drivers

[Yang Yingliang]

Hi Mark,

On 2022/9/21 2:33, Mark Brown wrote:
> On Tue, Sep 20, 2022 at 09:48:13PM +0800, Yang Yingliang wrote:
>> This patchset is trying to replace spi_alloc_master() with
>> devm_spi_alloc_master() in some spi drivers. With this helper,
>> spi_master_put() is called in devres_release_all() whenever
>> the device is unbound, so the spi_master_put() in error path
>> can be removed.
> If we're switching please update to the modern naming and use
> "controller" rather than the old name.
Do you mean to use spi_controller instead of spi_master? Something like 
this:
'struct spi_controller * ctlr = devm_spi_alloc_master();'
Dose spi_master_get_devdata()  need be changed to 
spi_controller_get_devdata() ?

Thanks,
Yang

Re: [PATCH -next 0/6] spi: Switch to use devm_spi_alloc_master() in some drivers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 519 bytes --]

On Wed, Sep 21, 2022 at 10:02:25AM +0800, Yang Yingliang wrote:
> On 2022/9/21 2:33, Mark Brown wrote:

> > If we're switching please update to the modern naming and use
> > "controller" rather than the old name.

> Do you mean to use spi_controller instead of spi_master? Something like
> this:
> 'struct spi_controller * ctlr = devm_spi_alloc_master();'

Or just use devm_spi_alloc_controller() directly.

> Dose spi_master_get_devdata()  need be changed to
> spi_controller_get_devdata() ?

Ideally.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH -next 0/6] spi: Switch to use devm_spi_alloc_master() in some drivers

[Yang Yingliang]

On 2022/9/21 20:37, Mark Brown wrote:
> On Wed, Sep 21, 2022 at 10:02:25AM +0800, Yang Yingliang wrote:
>> On 2022/9/21 2:33, Mark Brown wrote:
>>> If we're switching please update to the modern naming and use
>>> "controller" rather than the old name.
>> Do you mean to use spi_controller instead of spi_master? Something like
>> this:
>> 'struct spi_controller * ctlr = devm_spi_alloc_master();'
> Or just use devm_spi_alloc_controller() directly.
Does __devm_spi_alloc_controller() need be changed to 
devm_spi_alloc_controller(), then use
it, or just use __devm_spi_alloc_controller() directly.

Thanks,
Yang
>
>> Dose spi_master_get_devdata()  need be changed to
>> spi_controller_get_devdata() ?
> Ideally.

Re: [PATCH -next 0/6] spi: Switch to use devm_spi_alloc_master() in some drivers

[Lukas Wunner]

On Wed, Sep 21, 2022 at 01:37:49PM +0100, Mark Brown wrote:
> On Wed, Sep 21, 2022 at 10:02:25AM +0800, Yang Yingliang wrote:
> > On 2022/9/21 2:33, Mark Brown wrote:
> > > If we're switching please update to the modern naming and use
> > > "controller" rather than the old name.
> 
> > Do you mean to use spi_controller instead of spi_master? Something like
> > this:
> > 'struct spi_controller * ctlr = devm_spi_alloc_master();'
> 
> Or just use devm_spi_alloc_controller() directly.

There's no such thing.  The driver needs to explicitly allocate a
master or slave and that will result in the slave bit being set
correctly in struct spi_controller.

Yang's v2 series now calls __devm_spi_alloc_controller()
but drivers should never call that directly.

Thanks,

Lukas

Re: [PATCH -next 1/6] spi: oc-tiny: Switch to use devm_spi_alloc_master()

[Lukas Wunner]

On Tue, Sep 20, 2022 at 09:48:14PM +0800, Yang Yingliang wrote:
> Switch to use devm_spi_alloc_master() to simpify error path.

That's not sufficient.  You also need to remove the call to
spi_master_put() from tiny_spi_remove(), lest you introduce
a use-after-free.

Thanks,

Lukas

Re: [PATCH -next 2/6] spi: ath79: Switch to use devm_spi_alloc_master()

[Lukas Wunner]

On Tue, Sep 20, 2022 at 09:48:15PM +0800, Yang Yingliang wrote:
> Switch to use devm_spi_alloc_master() to simpify error path.

Unfortunately you're not removing the spi_master_put() from
ath79_spi_remove() here either.  So another use-after-free. :(

Thanks,

Lukas

Re: [PATCH -next 3/6] spi: omap-uwire: Switch to use devm_spi_alloc_master()

[Lukas Wunner]

On Tue, Sep 20, 2022 at 09:48:16PM +0800, Yang Yingliang wrote:
> @@ -476,7 +474,6 @@ static int uwire_probe(struct platform_device *pdev)
>  	if (IS_ERR(uwire->ck)) {
>  		status = PTR_ERR(uwire->ck);
>  		dev_dbg(&pdev->dev, "no functional clock?\n");
> -		spi_master_put(master);
>  		return status;
>  	}
>  	clk_prepare_enable(uwire->ck);

Change this hunk to return PTR_ERR(uwire->ck) directly without
assigning to status first.

> @@ -518,6 +515,7 @@ static int uwire_remove(struct platform_device *pdev)
>  
>  	spi_bitbang_stop(&uwire->bitbang);
>  	uwire_off(uwire);
> +	spi_master_put(uwire->bitbang.master);
>  	return 0;
>  }
>  

No, drop this hunk, it results in a use-after-free.

Thanks,

Lukas

Re: [PATCH -next 0/6] spi: Switch to use devm_spi_alloc_master() in some drivers

[Lukas Wunner]

On Tue, Sep 20, 2022 at 09:48:13PM +0800, Yang Yingliang wrote:
> This patchset is trying to replace spi_alloc_master() with
> devm_spi_alloc_master() in some spi drivers. With this helper,
> spi_master_put() is called in devres_release_all() whenever
> the device is unbound, so the spi_master_put() in error path
> can be removed.
> 
> Yang Yingliang (6):
>   spi: oc-tiny: Switch to use devm_spi_alloc_master()
>   spi: ath79: Switch to use devm_spi_alloc_master()
>   spi: omap-uwire: Switch to use devm_spi_alloc_master()
>   spi: ppc4xx: Switch to use devm_spi_alloc_master()
>   spi: sh-sci: Switch to use devm_spi_alloc_master()
>   spi: altera: Switch to use devm_spi_alloc_master()

I'm withdrawing my objections to patches 1, 2 and 3:
I failed to appreciate that these drivers use spi_bitbang_start(),
which takes an extra reference on the controller.  Sorry for the noise.

Whole series is
Reviewed-by: Lukas Wunner <lukas@wunner.de>

This pertains to v1 of the series, not v2 (which incorrectly uses
__devm_spi_alloc_controller()).

Thanks,

Lukas

Re: [PATCH -next 0/6] spi: Switch to use devm_spi_alloc_master() in some drivers

[Lukas Wunner]

On Wed, Sep 21, 2022 at 09:19:35PM +0800, Yang Yingliang wrote:
> On 2022/9/21 20:37, Mark Brown wrote:
> > On Wed, Sep 21, 2022 at 10:02:25AM +0800, Yang Yingliang wrote:
> > > On 2022/9/21 2:33, Mark Brown wrote:
> > > > If we're switching please update to the modern naming and use
> > > > "controller" rather than the old name.
> > > Do you mean to use spi_controller instead of spi_master? Something like
> > > this:
> > > 'struct spi_controller * ctlr = devm_spi_alloc_master();'
> > 
> > Or just use devm_spi_alloc_controller() directly.
> 
> Does __devm_spi_alloc_controller() need be changed to
> devm_spi_alloc_controller(), then use
> it, or just use __devm_spi_alloc_controller() directly.

Modern drivers only differentiate between master and slave on
allocation.  They use struct spi_controller instead of spi_master
and generally only call the spi_controller_*() functions (again,
except on allocation).  I think Mark wanted you to convert the
drivers so that they use struct spi_controller everywhere,
but that can be done in separate patches.

Thanks,

Lukas

Re: [PATCH -next 0/6] spi: Switch to use devm_spi_alloc_master() in some drivers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 582 bytes --]

On Fri, Sep 23, 2022 at 06:42:58AM +0200, Lukas Wunner wrote:
> On Wed, Sep 21, 2022 at 01:37:49PM +0100, Mark Brown wrote:

> > Or just use devm_spi_alloc_controller() directly.

> There's no such thing.  The driver needs to explicitly allocate a
> master or slave and that will result in the slave bit being set
> correctly in struct spi_controller.

> Yang's v2 series now calls __devm_spi_alloc_controller()
> but drivers should never call that directly.

Right, we should probably make the actual function to wrap that though -
I'd misremembered that that hadn't been created.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH -next 0/6] spi: Switch to use devm_spi_alloc_master() in some drivers

[Yang Yingliang]

On 2022/9/23 18:12, Mark Brown wrote:
> On Fri, Sep 23, 2022 at 06:42:58AM +0200, Lukas Wunner wrote:
>> On Wed, Sep 21, 2022 at 01:37:49PM +0100, Mark Brown wrote:
>>> Or just use devm_spi_alloc_controller() directly.
>> There's no such thing.  The driver needs to explicitly allocate a
>> master or slave and that will result in the slave bit being set
>> correctly in struct spi_controller.
>> Yang's v2 series now calls __devm_spi_alloc_controller()
>> but drivers should never call that directly.
> Right, we should probably make the actual function to wrap that though -
> I'd misremembered that that hadn't been created.
How about introduce devm_spi_alloc_controller() like this:

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index f089ee1ead58..67e510c8d15e 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -763,6 +763,8 @@ struct spi_controller 
*__devm_spi_alloc_controller(struct device *dev,
                                                    unsigned int size,
                                                    bool slave);

+#define devm_spi_alloc_controller devm_spi_alloc_master
+
Thanks,
Yang