Message ID | 1557147240-29551-1-git-send-email-sagar.kadam@sifive.com |
---|---|
Headers | show |
Series | Add dt bindings to support I2C on sifive devices and a fix for polling mode i2c transfers. | expand |
> /* > * 'process_lock' exists because ocores_process() and ocores_process_timeout() > @@ -239,8 +240,13 @@ static irqreturn_t ocores_isr(int irq, void *dev_id) > struct ocores_i2c *i2c = dev_id; > u8 stat = oc_getreg(i2c, OCI2C_STATUS); > > - if (!(stat & OCI2C_STAT_IF)) > + if (i2c->flags && SIFIVE_FLAG_POLL) { Do you really want && here? > + if (stat & OCI2C_STAT_IF) > + if (!(stat & OCI2C_STAT_BUSY)) > + return IRQ_NONE; > + } else if (!(stat & OCI2C_STAT_IF)) { > return IRQ_NONE; > + } > > ocores_process(i2c, stat); > > @@ -356,6 +362,11 @@ static void ocores_process_polling(struct ocores_i2c *i2c) > ret = ocores_isr(-1, i2c); > if (ret == IRQ_NONE) > break; /* all messages have been transferred */ > + else { > + if (i2c->flags && SIFIVE_FLAG_POLL) And here? > + if (i2c->state == STATE_DONE) > + break; > + } > } > } > > @@ -406,7 +417,7 @@ static int ocores_xfer(struct i2c_adapter *adap, > { > struct ocores_i2c *i2c = i2c_get_adapdata(adap); > > - if (i2c->flags & OCORES_FLAG_POLL) > + if ((i2c->flags & OCORES_FLAG_POLL) || (i2c->flags & SIFIVE_FLAG_POLL)) You can combine this if ((i2c->flags & (OCORES_FLAG_POLL | SIFIVE_FLAG_POLL)) > return ocores_xfer_polling(adap, msgs, num); > return ocores_xfer_core(i2c, msgs, num, false); > } > @@ -597,6 +608,7 @@ static int ocores_i2c_probe(struct platform_device *pdev) > { > struct ocores_i2c *i2c; > struct ocores_i2c_platform_data *pdata; > + const struct of_device_id *match; > struct resource *res; > int irq; > int ret; > @@ -678,13 +690,21 @@ static int ocores_i2c_probe(struct platform_device *pdev) > > irq = platform_get_irq(pdev, 0); > if (irq == -ENXIO) { > - i2c->flags |= OCORES_FLAG_POLL; > + /* > + * Set a SIFIVE_FLAG_POLL to enable workaround for FU540 > + * in polling mode interface of i2c-ocore driver. > + */ > + match = of_match_node(ocores_i2c_match, pdev->dev.of_node); > + if (match && (long)match->data == TYPE_SIFIVE_REV0) > + i2c->flags |= SIFIVE_FLAG_POLL; > + else > + i2c->flags |= OCORES_FLAG_POLL; Please take a look at the whole code, and consider if it is better to set both SIFIVE_FLAG_POLL and OCORES_FLAG_POLL. Maybe rename SIFIVE_FLAG_POLL to OCORES_FLAG_BROKEN_IRQ_BIT? Thanks Andrew
On Mon, May 6, 2019 at 6:59 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > /* > > * 'process_lock' exists because ocores_process() and ocores_process_timeout() > > @@ -239,8 +240,13 @@ static irqreturn_t ocores_isr(int irq, void *dev_id) > > struct ocores_i2c *i2c = dev_id; > > u8 stat = oc_getreg(i2c, OCI2C_STATUS); > > > > - if (!(stat & OCI2C_STAT_IF)) > > + if (i2c->flags && SIFIVE_FLAG_POLL) { > > Do you really want && here? > > > + if (stat & OCI2C_STAT_IF) > > + if (!(stat & OCI2C_STAT_BUSY)) > > + return IRQ_NONE; > > + } else if (!(stat & OCI2C_STAT_IF)) { > > return IRQ_NONE; > > + } > > > > ocores_process(i2c, stat); > > > > @@ -356,6 +362,11 @@ static void ocores_process_polling(struct ocores_i2c *i2c) > > ret = ocores_isr(-1, i2c); > > if (ret == IRQ_NONE) > > break; /* all messages have been transferred */ > > + else { > > + if (i2c->flags && SIFIVE_FLAG_POLL) > > And here? > > > + if (i2c->state == STATE_DONE) > > + break; > > + } > > } > > } > > > > @@ -406,7 +417,7 @@ static int ocores_xfer(struct i2c_adapter *adap, > > { > > struct ocores_i2c *i2c = i2c_get_adapdata(adap); > > > > - if (i2c->flags & OCORES_FLAG_POLL) > > + if ((i2c->flags & OCORES_FLAG_POLL) || (i2c->flags & SIFIVE_FLAG_POLL)) > > You can combine this Thanks for your suggestion's Andrew. Yes, I will optimize this. > > if ((i2c->flags & (OCORES_FLAG_POLL | SIFIVE_FLAG_POLL)) > > > return ocores_xfer_polling(adap, msgs, num); > > return ocores_xfer_core(i2c, msgs, num, false); > > } > > @@ -597,6 +608,7 @@ static int ocores_i2c_probe(struct platform_device *pdev) > > { > > struct ocores_i2c *i2c; > > struct ocores_i2c_platform_data *pdata; > > + const struct of_device_id *match; > > struct resource *res; > > int irq; > > int ret; > > @@ -678,13 +690,21 @@ static int ocores_i2c_probe(struct platform_device *pdev) > > > > irq = platform_get_irq(pdev, 0); > > if (irq == -ENXIO) { > > - i2c->flags |= OCORES_FLAG_POLL; > > + /* > > + * Set a SIFIVE_FLAG_POLL to enable workaround for FU540 > > + * in polling mode interface of i2c-ocore driver. > > + */ > > + match = of_match_node(ocores_i2c_match, pdev->dev.of_node); > > + if (match && (long)match->data == TYPE_SIFIVE_REV0) > > + i2c->flags |= SIFIVE_FLAG_POLL; > > + else > > + i2c->flags |= OCORES_FLAG_POLL; > > Please take a look at the whole code, and consider if it is better to > set both SIFIVE_FLAG_POLL and OCORES_FLAG_POLL. Maybe rename > SIFIVE_FLAG_POLL to OCORES_FLAG_BROKEN_IRQ_BIT? > The intent of this patch is to add a workaround for hardware errratum of FU540 a SiFive Device, hence I had named the flag accordingly. Yes, OCORES_FLAG_BROKEN_IRQ_BIT is a better and generic term, I will rename and resubmit this patch -Thanks Sagar > Thanks > Andrew