[RFC 0/2] i2c: sh_mobile: don't regress on deferred probing

[RFC 0/2] i2c: sh_mobile: don't regress on deferred probing

[Wolfram Sang]

Okay, here is my take on solving the problem found by Geert. I still don't like
it much but it is not as bad as I expected it to be ;) Let me know what you
think.

Thanks,

   Wolfram


Wolfram Sang (2):
  i2c: sh_mobile: refactor DMA setup
  i2c: sh_mobile: rework deferred probing

 drivers/i2c/busses/i2c-sh_mobile.c | 111 ++++++++++++++++++-------------------
 1 file changed, 53 insertions(+), 58 deletions(-)

-- 
2.1.3

[RFC 1/2] i2c: sh_mobile: refactor DMA setup

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>

Refactor DMA setup to keep the errno so we can implement better
deferred probe support in the next step.

Signed-off-by: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index b9c8cf2aac2c..636f2297143a 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -552,7 +552,7 @@ static void sh_mobile_i2c_xfer_dma(struct sh_mobile_i2c_data *pd)
 	dma_addr_t dma_addr;
 	dma_cookie_t cookie;
 
-	if (!chan)
+	if (IS_ERR(chan))
 		return;
 
 	dma_addr = dma_map_single(chan->device->dev, pd->msg->buf, pd->msg->len, dir);
@@ -749,21 +749,18 @@ static const struct of_device_id sh_mobile_i2c_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, sh_mobile_i2c_dt_ids);
 
-static int sh_mobile_i2c_request_dma_chan(struct device *dev, enum dma_transfer_direction dir,
-					  dma_addr_t port_addr, struct dma_chan **chan_ptr)
+static struct dma_chan *sh_mobile_i2c_request_dma_chan(struct device *dev,
+				enum dma_transfer_direction dir, dma_addr_t port_addr)
 {
 	struct dma_chan *chan;
 	struct dma_slave_config cfg;
 	char *chan_name = dir == DMA_MEM_TO_DEV ? "tx" : "rx";
 	int ret;
 
-	*chan_ptr = NULL;
-
 	chan = dma_request_slave_channel_reason(dev, chan_name);
 	if (IS_ERR(chan)) {
-		ret = PTR_ERR(chan);
 		dev_dbg(dev, "request_channel failed for %s (%d)\n", chan_name, ret);
-		return ret;
+		return chan;
 	}
 
 	memset(&cfg, 0, sizeof(cfg));
@@ -780,25 +777,23 @@ static int sh_mobile_i2c_request_dma_chan(struct device *dev, enum dma_transfer_
 	if (ret) {
 		dev_dbg(dev, "slave_config failed for %s (%d)\n", chan_name, ret);
 		dma_release_channel(chan);
-		return ret;
+		return ERR_PTR(ret);
 	}
 
-	*chan_ptr = chan;
-
 	dev_dbg(dev, "got DMA channel for %s\n", chan_name);
-	return 0;
+	return chan;
 }
 
 static void sh_mobile_i2c_release_dma(struct sh_mobile_i2c_data *pd)
 {
-	if (pd->dma_tx) {
+	if (!IS_ERR(pd->dma_tx)) {
 		dma_release_channel(pd->dma_tx);
-		pd->dma_tx = NULL;
+		pd->dma_tx = ERR_PTR(-EPROBE_DEFER);
 	}
 
-	if (pd->dma_rx) {
+	if (!IS_ERR(pd->dma_rx)) {
 		dma_release_channel(pd->dma_rx);
-		pd->dma_rx = NULL;
+		pd->dma_rx = ERR_PTR(-EPROBE_DEFER);
 	}
 }
 
@@ -891,13 +886,15 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
 	/* Init DMA */
 	sg_init_table(&pd->sg, 1);
 	pd->dma_direction = DMA_NONE;
-	ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
-					     res->start + ICDR, &pd->dma_rx);
+	pd->dma_rx = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
+					     res->start + ICDR);
+	ret = PTR_ERR(pd->dma_rx);
 	if (ret == -EPROBE_DEFER)
 		return ret;
 
-	ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
-					     res->start + ICDR, &pd->dma_tx);
+	pd->dma_tx = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
+					     res->start + ICDR);
+	ret = PTR_ERR(pd->dma_tx);
 	if (ret == -EPROBE_DEFER) {
 		sh_mobile_i2c_release_dma(pd);
 		return ret;
-- 
2.1.3

[RFC 2/2] i2c: sh_mobile: rework deferred probing

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

DMA is opt-in for this driver. So, we can't use deferred probing in
probe, because our driver would get endlessly deferred if DMA support is
compiled in but only the DMA driver is missing. Because we can't know
when the DMA driver might show up, we always try again when a DMA
transfer would be possible. The downside is that there is more overhead
for setting up every DMA transfer.

Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 98 +++++++++++++++++++-------------------
 1 file changed, 48 insertions(+), 50 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 636f2297143a..ea7b99212fa6 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -144,6 +144,7 @@ struct sh_mobile_i2c_data {
 	int sr;
 	bool send_stop;
 
+	struct resource *res;
 	struct dma_chan *dma_tx;
 	struct dma_chan *dma_rx;
 	struct scatterlist sg;
@@ -543,6 +544,41 @@ static void sh_mobile_i2c_dma_callback(void *data)
 	iic_set_clr(pd, ICIC, 0, ICIC_TDMAE | ICIC_RDMAE);
 }
 
+static struct dma_chan *sh_mobile_i2c_request_dma_chan(struct device *dev,
+				enum dma_transfer_direction dir, dma_addr_t port_addr)
+{
+	struct dma_chan *chan;
+	struct dma_slave_config cfg;
+	char *chan_name = dir == DMA_MEM_TO_DEV ? "tx" : "rx";
+	int ret;
+
+	chan = dma_request_slave_channel_reason(dev, chan_name);
+	if (IS_ERR(chan)) {
+		dev_dbg(dev, "request_channel failed for %s (%d)\n", chan_name, ret);
+		return chan;
+	}
+
+	memset(&cfg, 0, sizeof(cfg));
+	cfg.direction = dir;
+	if (dir == DMA_MEM_TO_DEV) {
+		cfg.dst_addr = port_addr;
+		cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	} else {
+		cfg.src_addr = port_addr;
+		cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	}
+
+	ret = dmaengine_slave_config(chan, &cfg);
+	if (ret) {
+		dev_dbg(dev, "slave_config failed for %s (%d)\n", chan_name, ret);
+		dma_release_channel(chan);
+		return ERR_PTR(ret);
+	}
+
+	dev_dbg(dev, "got DMA channel for %s\n", chan_name);
+	return chan;
+}
+
 static void sh_mobile_i2c_xfer_dma(struct sh_mobile_i2c_data *pd)
 {
 	bool read = pd->msg->flags & I2C_M_RD;
@@ -552,6 +588,15 @@ static void sh_mobile_i2c_xfer_dma(struct sh_mobile_i2c_data *pd)
 	dma_addr_t dma_addr;
 	dma_cookie_t cookie;
 
+	if (PTR_ERR(chan) == -EPROBE_DEFER) {
+		if (read)
+			chan = pd->dma_rx = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
+							     pd->res->start + ICDR);
+		else
+			chan = pd->dma_tx = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
+							     pd->res->start + ICDR);
+	}
+
 	if (IS_ERR(chan))
 		return;
 
@@ -749,41 +794,6 @@ static const struct of_device_id sh_mobile_i2c_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, sh_mobile_i2c_dt_ids);
 
-static struct dma_chan *sh_mobile_i2c_request_dma_chan(struct device *dev,
-				enum dma_transfer_direction dir, dma_addr_t port_addr)
-{
-	struct dma_chan *chan;
-	struct dma_slave_config cfg;
-	char *chan_name = dir == DMA_MEM_TO_DEV ? "tx" : "rx";
-	int ret;
-
-	chan = dma_request_slave_channel_reason(dev, chan_name);
-	if (IS_ERR(chan)) {
-		dev_dbg(dev, "request_channel failed for %s (%d)\n", chan_name, ret);
-		return chan;
-	}
-
-	memset(&cfg, 0, sizeof(cfg));
-	cfg.direction = dir;
-	if (dir == DMA_MEM_TO_DEV) {
-		cfg.dst_addr = port_addr;
-		cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
-	} else {
-		cfg.src_addr = port_addr;
-		cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
-	}
-
-	ret = dmaengine_slave_config(chan, &cfg);
-	if (ret) {
-		dev_dbg(dev, "slave_config failed for %s (%d)\n", chan_name, ret);
-		dma_release_channel(chan);
-		return ERR_PTR(ret);
-	}
-
-	dev_dbg(dev, "got DMA channel for %s\n", chan_name);
-	return chan;
-}
-
 static void sh_mobile_i2c_release_dma(struct sh_mobile_i2c_data *pd)
 {
 	if (!IS_ERR(pd->dma_tx)) {
@@ -846,6 +856,7 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
 
 	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
 
+	pd->res = res;
 	pd->reg = devm_ioremap_resource(&dev->dev, res);
 	if (IS_ERR(pd->reg))
 		return PTR_ERR(pd->reg);
@@ -886,19 +897,7 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
 	/* Init DMA */
 	sg_init_table(&pd->sg, 1);
 	pd->dma_direction = DMA_NONE;
-	pd->dma_rx = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
-					     res->start + ICDR);
-	ret = PTR_ERR(pd->dma_rx);
-	if (ret == -EPROBE_DEFER)
-		return ret;
-
-	pd->dma_tx = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
-					     res->start + ICDR);
-	ret = PTR_ERR(pd->dma_tx);
-	if (ret == -EPROBE_DEFER) {
-		sh_mobile_i2c_release_dma(pd);
-		return ret;
-	}
+	pd->dma_rx = pd->dma_tx = ERR_PTR(-EPROBE_DEFER);
 
 	/* Enable Runtime PM for this device.
 	 *
@@ -936,8 +935,7 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
 		return ret;
 	}
 
-	dev_info(&dev->dev, "I2C adapter %d, bus speed %lu Hz, DMA=%c\n",
-		 adap->nr, pd->bus_speed, (pd->dma_rx || pd->dma_tx) ? 'y' : 'n');
+	dev_info(&dev->dev, "I2C adapter %d, bus speed %lu Hz\n", adap->nr, pd->bus_speed);
 
 	return 0;
 }
-- 
2.1.3

Re: [RFC 2/2] i2c: sh_mobile: rework deferred probing

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, Dec 10, 2014 at 2:21 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> DMA is opt-in for this driver. So, we can't use deferred probing in
> probe, because our driver would get endlessly deferred if DMA support is
> compiled in but only the DMA driver is missing. Because we can't know
> when the DMA driver might show up, we always try again when a DMA
> transfer would be possible. The downside is that there is more overhead
> for setting up every DMA transfer.

... for setting up every PIO transfer?

Once DMA is available, it's just one extra variable check.

> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/busses/i2c-sh_mobile.c | 98 +++++++++++++++++++-------------------
>  1 file changed, 48 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
> index 636f2297143a..ea7b99212fa6 100644
> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> @@ -144,6 +144,7 @@ struct sh_mobile_i2c_data {
>         int sr;
>         bool send_stop;
>
> +       struct resource *res;

I thinkk you can just store dma_addr_t port_addr here.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC 0/2] i2c: sh_mobile: don't regress on deferred probing

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, Dec 10, 2014 at 2:21 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> Okay, here is my take on solving the problem found by Geert. I still don't like
> it much but it is not as bad as I expected it to be ;) Let me know what you
> think.

Thanks, it's indeed less ugly than I would have expected ;-)

Note that in spi-rspi.c and spi-sh-msiof, any error returned by *_request_dma()
is considered an error, and -EPROBE_DEFER is not handled specially.
So it won't retry if the DMA engine driver isn't available, but just use PIO
(until unbind/bind).

Now we have a nice sample implementation, perhaps I should port it to
spi-rspi and spi-sh-msiof, too?

Let's wait and see for other comments...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC 0/2] i2c: sh_mobile: don't regress on deferred probing

[Wolfram Sang]

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


> On Wed, Dec 10, 2014 at 2:21 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> > Okay, here is my take on solving the problem found by Geert. I still don't like
> > it much but it is not as bad as I expected it to be ;) Let me know what you
> > think.
> 
> Thanks, it's indeed less ugly than I would have expected ;-)

:D

> Note that in spi-rspi.c and spi-sh-msiof, any error returned by *_request_dma()
> is considered an error, and -EPROBE_DEFER is not handled specially.
> So it won't retry if the DMA engine driver isn't available, but just use PIO
> (until unbind/bind).

For historic reasons, i2c-sh_mobile uses subsys_initcall() and at that
time, DMA is never available. Converting to module_init() will just
create its own set of potential regressions :(

> Now we have a nice sample implementation, perhaps I should port it to
> spi-rspi and spi-sh-msiof, too?
> 
> Let's wait and see for other comments...

Yeah, let's see first...


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 1/2] i2c: sh_mobile: refactor DMA setup

[Sergei Shtylyov]

Hello.

On 12/10/2014 04:21 PM, Wolfram Sang wrote:

> From: Wolfram Sang <wsa+renesas@sang-engineering.com>

> Refactor DMA setup to keep the errno so we can implement better
> deferred probe support in the next step.

> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>   drivers/i2c/busses/i2c-sh_mobile.c | 35 ++++++++++++++++-------------------
>   1 file changed, 16 insertions(+), 19 deletions(-)

> diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
> index b9c8cf2aac2c..636f2297143a 100644
> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
[...]
> @@ -749,21 +749,18 @@ static const struct of_device_id sh_mobile_i2c_dt_ids[] = {
>   };
>   MODULE_DEVICE_TABLE(of, sh_mobile_i2c_dt_ids);
>
> -static int sh_mobile_i2c_request_dma_chan(struct device *dev, enum dma_transfer_direction dir,
> -					  dma_addr_t port_addr, struct dma_chan **chan_ptr)
> +static struct dma_chan *sh_mobile_i2c_request_dma_chan(struct device *dev,
> +				enum dma_transfer_direction dir, dma_addr_t port_addr)

    Hm, alignment style changed...

[...]
> @@ -891,13 +886,15 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
>   	/* Init DMA */
>   	sg_init_table(&pd->sg, 1);
>   	pd->dma_direction = DMA_NONE;
> -	ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
> -					     res->start + ICDR, &pd->dma_rx);
> +	pd->dma_rx = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
> +					     res->start + ICDR);

    The alignment of this line is strange: it's neither here nor there, i.e. 
not aligned to the next char after ( above and still contains spaces. I think 
we want the old way of the alignment kept, i.e. the line starting under 
'pd->dev'...

> +	ret = PTR_ERR(pd->dma_rx);
>   	if (ret == -EPROBE_DEFER)
>   		return ret;
>
> -	ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
> -					     res->start + ICDR, &pd->dma_tx);
> +	pd->dma_tx = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
> +					     res->start + ICDR);

    Likewise.

[...]

WBR, Sergei

Re: [RFC 1/2] i2c: sh_mobile: refactor DMA setup

[Wolfram Sang]

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


>    Hm, alignment style changed...

I am not too strict on that as long as the result is somewhat readable.

I'd be more interested in your thoughts about the general approach taken
in this series.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 2/2] i2c: sh_mobile: rework deferred probing

[Wolfram Sang]

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

On Wed, Dec 10, 2014 at 02:44:58PM +0100, Geert Uytterhoeven wrote:
> Hi Wolfram,
> 
> On Wed, Dec 10, 2014 at 2:21 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> > DMA is opt-in for this driver. So, we can't use deferred probing in
> > probe, because our driver would get endlessly deferred if DMA support is
> > compiled in but only the DMA driver is missing. Because we can't know
> > when the DMA driver might show up, we always try again when a DMA
> > transfer would be possible. The downside is that there is more overhead
> > for setting up every DMA transfer.
> 
> ... for setting up every PIO transfer?
> 
> Once DMA is available, it's just one extra variable check.

ACK. Mixed that up.

> 
> > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > ---
> >  drivers/i2c/busses/i2c-sh_mobile.c | 98 +++++++++++++++++++-------------------
> >  1 file changed, 48 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
> > index 636f2297143a..ea7b99212fa6 100644
> > --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> > @@ -144,6 +144,7 @@ struct sh_mobile_i2c_data {
> >         int sr;
> >         bool send_stop;
> >
> > +       struct resource *res;
> 
> I thinkk you can just store dma_addr_t port_addr here.

Yes, but I liked the resource better :)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 1/2] i2c: sh_mobile: refactor DMA setup

[Sergei Shtylyov]

Hello.

On 12/16/2014 02:34 PM, Wolfram Sang wrote:

>>     Hm, alignment style changed...

> I am not too strict on that as long as the result is somewhat readable.

> I'd be more interested in your thoughts about the general approach taken
> in this series.

    Sorry, I'm not very familiar with this driver (and busy with other stuff 
currently).

WBR, Sergei