| Message ID | 20190208010507.12411-5-andrew@lunn.ch |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c-ocores: Add IO mapped polled support | expand |
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 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.
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
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
> 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.
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 */
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(-)