[v3,4/4] i2c: ocores: Add support for polling interrupt status

Message ID 20190208010507.12411-5-andrew@lunn.ch
State Superseded
Headers show
Series
  • i2c-ocores: Add IO mapped polled support
Related show

Commit Message

Andrew Lunn Feb. 8, 2019, 1:05 a.m.
Not all implementations of the OCORES i2c bus master support
generating interrupts. Add support for polling the interrupt status
bit when no interrupt is defined.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/i2c/busses/i2c-ocores.c | 64 ++++++++++++++++++++++++++-------
 1 file changed, 51 insertions(+), 13 deletions(-)

Comments

Federico Vaga Feb. 8, 2019, 9:52 a.m. | #1
Hi Andrew,

My patch for this (that is waiting since October 2018) has been reviewed by 
Peter at least once, and we agreed to make it ready for a 
master_xfer_irq_less() proposal. In case that proposal evolves in an actual 
change, then the driver will be ready for it with minimal changes.

If you have improvements or comments can you do it on the patch (V2) I sent

https://patchwork.ozlabs.org/patch/990283/

@Wolfram how seriously should we consider the possibility of having IRQ less 
and atomic safe transfers?

(add Peter in CC)

On Friday, February 8, 2019 2:05:07 AM CET Andrew Lunn wrote:
> Not all implementations of the OCORES i2c bus master support
> generating interrupts. Add support for polling the interrupt status
> bit when no interrupt is defined.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/i2c/busses/i2c-ocores.c | 64 ++++++++++++++++++++++++++-------
>  1 file changed, 51 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-ocores.c
> b/drivers/i2c/busses/i2c-ocores.c index d2730efe6bec..9576e842e557 100644
> --- a/drivers/i2c/busses/i2c-ocores.c
> +++ b/drivers/i2c/busses/i2c-ocores.c
> @@ -13,6 +13,7 @@
>   */
> 
>  #include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> @@ -40,10 +41,14 @@ struct ocores_i2c {
>  	struct clk *clk;
>  	int ip_clock_khz;
>  	int bus_clock_khz;
> +	int flags;
>  	void (*setreg)(struct ocores_i2c *i2c, int reg, u8 value);
>  	u8 (*getreg)(struct ocores_i2c *i2c, int reg);
>  };
> 
> +/* flags */
> +#define I2C_FLAG_NOIRQ		BIT(0)
> +
>  /* registers */
>  #define OCI2C_PRELOW		0
>  #define OCI2C_PREHIGH		1
> @@ -224,6 +229,27 @@ static irqreturn_t ocores_isr(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
> 
> +static void ocores_poll_irq(struct ocores_i2c *i2c)
> +{
> +	int timeout = 500;
> +	u8 stat;
> +
> +	do {
> +		/* delay 2-3 SCL (5-7usec for 400KHz) */
> +		usleep_range(5, 7);
> +		stat = oc_getreg(i2c, OCI2C_STATUS);
> +	} while ((stat & OCI2C_STAT_TIP) && timeout--);
> +
> +
> +	if (!(stat & OCI2C_STAT_TIP)) {
> +		ocores_process(i2c);
> +	} else {
> +		i2c->state = STATE_ERROR;
> +		dev_dbg(&i2c->adap.dev, "Timeout while polling %02x\n", stat);
> +		oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
> +	}
> +}
> +
>  static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int
> num) {
>  	struct ocores_i2c *i2c = i2c_get_adapdata(adap);
> @@ -236,11 +262,18 @@ static int ocores_xfer(struct i2c_adapter *adap,
> struct i2c_msg *msgs, int num) oc_setreg(i2c, OCI2C_DATA,
> i2c_8bit_addr_from_msg(i2c->msg));
>  	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
> 
> -	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
> -			       (i2c->state == STATE_DONE), HZ))
> +	if (i2c->flags & I2C_FLAG_NOIRQ) {
> +		while (!((i2c->state == STATE_DONE) ||
> +			 (i2c->state == STATE_ERROR)))
> +			ocores_poll_irq(i2c);
>  		return (i2c->state == STATE_DONE) ? num : -EIO;
> -	else
> -		return -ETIMEDOUT;
> +	} else {
> +		if (wait_event_timeout(i2c->wait,
> +				       (i2c->state == STATE_ERROR) ||
> +				       (i2c->state == STATE_DONE), HZ))
> +			return (i2c->state == STATE_DONE) ? num : -EIO;
> +	}
> +	return -ETIMEDOUT;
>  }
> 
>  static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
> @@ -425,14 +458,17 @@ static int ocores_i2c_probe(struct platform_device
> *pdev) int ret;
>  	int i;
> 
> -	irq = platform_get_irq(pdev, 0);
> -	if (irq < 0)
> -		return irq;
> -
>  	i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
>  	if (!i2c)
>  		return -ENOMEM;
> 
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq == -ENXIO)
> +		i2c->flags |= I2C_FLAG_NOIRQ;
> +	else
> +		if (irq < 0)
> +			return irq;
> +
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (res) {
>  		i2c->base = devm_ioremap_resource(&pdev->dev, res);
> @@ -504,11 +540,13 @@ static int ocores_i2c_probe(struct platform_device
> *pdev) goto err_clk;
> 
>  	init_waitqueue_head(&i2c->wait);
> -	ret = devm_request_irq(&pdev->dev, irq, ocores_isr, 0,
> -			       pdev->name, i2c);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Cannot claim IRQ\n");
> -		goto err_clk;
> +	if (!(i2c->flags & I2C_FLAG_NOIRQ)) {
> +		ret = devm_request_irq(&pdev->dev, irq, ocores_isr, 0,
> +				       pdev->name, i2c);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Cannot claim IRQ\n");
> +			goto err_clk;
> +		}
>  	}
> 
>  	/* hook up driver to tree */
Wolfram Sang Feb. 8, 2019, 12:13 p.m. | #2
> @Wolfram how seriously should we consider the possibility of having IRQ less 
> and atomic safe transfers?

You mean for late transfers? Consider it, I think we will find a way to
make these happen.
Andrew Lunn Feb. 8, 2019, 1:19 p.m. | #3
On Fri, Feb 08, 2019 at 10:52:13AM +0100, Federico Vaga wrote:
> Hi Andrew,
> 
> My patch for this (that is waiting since October 2018) has been reviewed by 
> Peter at least once, and we agreed to make it ready for a 
> master_xfer_irq_less() proposal. In case that proposal evolves in an actual 
> change, then the driver will be ready for it with minimal changes.

Hi Federico 

Maybe it is me being impatient, i'm mostly a netdev maintainer, where
things get reviewed in 3 days, not five months. However, it is now 5
months later, and master_xfer_irq_less() has not happened. Is it
really going to happen? Are you making it happen?  Do we want to add a
more invasive patch for something which might never happen?

> If you have improvements or comments can you do it on the patch (V2) I sent
> 
> https://patchwork.ozlabs.org/patch/990283/

In the end, i don't care if it is my patch or yours for polled IO. I
would just like to see one of them merged in the next week or two
before the next merge window opens. Please could you repost your patch
series so we can review it.

    Thanks
	Andrew
Federico Vaga Feb. 8, 2019, 2:29 p.m. | #4
On Friday, February 8, 2019 2:19:05 PM CET Andrew Lunn wrote:
> On Fri, Feb 08, 2019 at 10:52:13AM +0100, Federico Vaga wrote:
> > Hi Andrew,
> > 
> > My patch for this (that is waiting since October 2018) has been reviewed
> > by
> > Peter at least once, and we agreed to make it ready for a
> > master_xfer_irq_less() proposal. In case that proposal evolves in an
> > actual
> > change, then the driver will be ready for it with minimal changes.
> 
> Hi Federico
> 
> Maybe it is me being impatient, i'm mostly a netdev maintainer, where
> things get reviewed in 3 days, not five months. However, it is now 5
> months later, and master_xfer_irq_less() has not happened. Is it
> really going to happen?

I do not know

> Are you making it happen? 

No

> Do we want to add a more invasive patch for something which might never 
> happen?

I do not understand why you say so. The differences are not that big, I may 
have wrote more code and more comments but I would not say that it is 
invasive.
 
> > If you have improvements or comments can you do it on the patch (V2) I
> > sent
> > 
> > https://patchwork.ozlabs.org/patch/990283/
> 
> In the end, i don't care if it is my patch or yours for polled IO. I
> would just like to see one of them merged in the next week or two
> before the next merge window opens. Please could you repost your patch
> series so we can review it.

I do not mind what get integrated, as you, I want to use a polling interface 
without compiling my own out-of-tree module.

I will re-send V3 for the entire patch set.

> 
>     Thanks
> 	Andrew
Wolfram Sang Feb. 8, 2019, 6:40 p.m. | #5
> Maybe it is me being impatient, i'm mostly a netdev maintainer, where
> things get reviewed in 3 days, not five months.

Well, I2C is mostly a "fly-by" subsystem where people drop their driver
and move on. I don't think you can compare it with netdev in that
regard.

The only thing what helps here is exactly what you did. Take over a
bit of responsibility and encourage collaboration.

Thanks.

Patch

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index d2730efe6bec..9576e842e557 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -13,6 +13,7 @@ 
  */
 
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -40,10 +41,14 @@  struct ocores_i2c {
 	struct clk *clk;
 	int ip_clock_khz;
 	int bus_clock_khz;
+	int flags;
 	void (*setreg)(struct ocores_i2c *i2c, int reg, u8 value);
 	u8 (*getreg)(struct ocores_i2c *i2c, int reg);
 };
 
+/* flags */
+#define I2C_FLAG_NOIRQ		BIT(0)
+
 /* registers */
 #define OCI2C_PRELOW		0
 #define OCI2C_PREHIGH		1
@@ -224,6 +229,27 @@  static irqreturn_t ocores_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static void ocores_poll_irq(struct ocores_i2c *i2c)
+{
+	int timeout = 500;
+	u8 stat;
+
+	do {
+		/* delay 2-3 SCL (5-7usec for 400KHz) */
+		usleep_range(5, 7);
+		stat = oc_getreg(i2c, OCI2C_STATUS);
+	} while ((stat & OCI2C_STAT_TIP) && timeout--);
+
+
+	if (!(stat & OCI2C_STAT_TIP)) {
+		ocores_process(i2c);
+	} else {
+		i2c->state = STATE_ERROR;
+		dev_dbg(&i2c->adap.dev, "Timeout while polling %02x\n", stat);
+		oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
+	}
+}
+
 static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
 	struct ocores_i2c *i2c = i2c_get_adapdata(adap);
@@ -236,11 +262,18 @@  static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	oc_setreg(i2c, OCI2C_DATA, i2c_8bit_addr_from_msg(i2c->msg));
 	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
 
-	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
-			       (i2c->state == STATE_DONE), HZ))
+	if (i2c->flags & I2C_FLAG_NOIRQ) {
+		while (!((i2c->state == STATE_DONE) ||
+			 (i2c->state == STATE_ERROR)))
+			ocores_poll_irq(i2c);
 		return (i2c->state == STATE_DONE) ? num : -EIO;
-	else
-		return -ETIMEDOUT;
+	} else {
+		if (wait_event_timeout(i2c->wait,
+				       (i2c->state == STATE_ERROR) ||
+				       (i2c->state == STATE_DONE), HZ))
+			return (i2c->state == STATE_DONE) ? num : -EIO;
+	}
+	return -ETIMEDOUT;
 }
 
 static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
@@ -425,14 +458,17 @@  static int ocores_i2c_probe(struct platform_device *pdev)
 	int ret;
 	int i;
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		return irq;
-
 	i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
 	if (!i2c)
 		return -ENOMEM;
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq == -ENXIO)
+		i2c->flags |= I2C_FLAG_NOIRQ;
+	else
+		if (irq < 0)
+			return irq;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (res) {
 		i2c->base = devm_ioremap_resource(&pdev->dev, res);
@@ -504,11 +540,13 @@  static int ocores_i2c_probe(struct platform_device *pdev)
 		goto err_clk;
 
 	init_waitqueue_head(&i2c->wait);
-	ret = devm_request_irq(&pdev->dev, irq, ocores_isr, 0,
-			       pdev->name, i2c);
-	if (ret) {
-		dev_err(&pdev->dev, "Cannot claim IRQ\n");
-		goto err_clk;
+	if (!(i2c->flags & I2C_FLAG_NOIRQ)) {
+		ret = devm_request_irq(&pdev->dev, irq, ocores_isr, 0,
+				       pdev->name, i2c);
+		if (ret) {
+			dev_err(&pdev->dev, "Cannot claim IRQ\n");
+			goto err_clk;
+		}
 	}
 
 	/* hook up driver to tree */