Hi David,
On Tuesday, 6 October 2020, 14:06:36 CEST, David Laight wrote:
From: Christian Eggers
+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.
Yes, it looks so.
Why isn't: imx_i2c_write_reg(bits, i2c_imx, IMX_I2C_I2SR); enough?
i.MX requires W1C and Vybrid requires W0C in order to clear status bits.
More usually you just write back the read value of such 'write 1 to clear' status registers and then act on all the set bits.
This pattern has been suggested by Uwe Klein-Koenig. It works because write access to read-only register bits is ignored, independent whether 0 or 1 is written. W0C is quite unusual, but I didn't design the hardware... The pattern ensures that not accidentally more status bits are cleared than desired.
That ensures you clear all interrupts that were pending.
I think that Uwe's intention was not clearing bits which are not handled at this place. Otherwise events may get lost.
If you need to avoid writes of bits that aren't in the 'clear all interrupts' value then you just need: bits &= i2c_imx->hwdata->i2sr_clr_opcode; prior to the write.
I think this wouldn't fit the W0C case for Vybrid.
Best regards Christian