Re: [PATCH v3 1/3] i2c: imx: Fix reset of I2SR_IAL flag

[Uwe Kleine-König]

On Tue, Oct 06, 2020 at 12:06:36PM +0000, David Laight wrote:
> From: Christian Eggers
> > Sent: 06 October 2020 11:52
> > 
> > According to the "VFxxx Controller Reference Manual" (and the comment
> > block starting at line 97), Vybrid requires writing a one for clearing
> > an interrupt flag. Syncing the method for clearing I2SR_IIF in
> > i2c_imx_isr().
> > 
> > Signed-off-by: Christian Eggers <ceggers@xxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> >  drivers/i2c/busses/i2c-imx.c | 20 +++++++++++++++-----
> >  1 file changed, 15 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> > index 0ab5381aa012..745f4071155a 100644
> > --- a/drivers/i2c/busses/i2c-imx.c
> > +++ b/drivers/i2c/busses/i2c-imx.c
> > @@ -412,6 +412,19 @@ static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx)
> >  	dma->chan_using = NULL;
> >  }
> > 
> > +static void i2c_imx_clear_irq(struct imx_i2c_struct *i2c_imx, unsigned int bits)
> > +{
> > +	unsigned int temp;
> > +
> > +	/*
> > +	 * i2sr_clr_opcode is the value to clear all interrupts.
> > +	 * Here we want to clear only <bits>, so we write
> > +	 * ~i2sr_clr_opcode with just <bits> toggled.
> > +	 */
> > +	temp = ~i2c_imx->hwdata->i2sr_clr_opcode ^ bits;
> > +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
> > +}
> 
> That looks either wrong or maybe just overcomplicated.
> Why isn't:
> 	imx_i2c_write_reg(bits, i2c_imx, IMX_I2C_I2SR);
> enough?

Your question suggests you either didn't read the comment or the comment
is not good enough. Maybe once you understood the complication (see
Christian's mail) you could suggest a better wording? Maybe we have to
mention that this handles both W1C and W0C.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature