Hi Andi,
On 27.10.2023 15:39, Andi Shyti wrote:
> On Wed, Oct 25, 2023 at 02:17:23PM +0200, Marek Szyprowski wrote:
>> To properly handle read transfers in polling mode, no waiting for the ACK
>> state is needed as it will never come. Just wait a bit to ensure start
>> state is on the bus and continue processing next bytes.
>>
>> Fixes: 117053f77a5a ("i2c: s3c2410: Add polling mode support")
>> Signed-off-by: Marek Szyprowski<m.szyprowski@xxxxxxxxxxx>
>> ---
>> drivers/i2c/busses/i2c-s3c2410.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
>> index 127eb3805fac..f9dcb1112a61 100644
>> --- a/drivers/i2c/busses/i2c-s3c2410.c
>> +++ b/drivers/i2c/busses/i2c-s3c2410.c
>> @@ -216,8 +216,13 @@ static bool is_ack(struct s3c24xx_i2c *i2c)
>> int tries;
>>
>> for (tries = 50; tries; --tries) {
>> - if (readl(i2c->regs + S3C2410_IICCON)
>> - & S3C2410_IICCON_IRQPEND) {
>> + unsigned long tmp = readl(i2c->regs + S3C2410_IICCON);
>> +
>> + if (!(tmp & S3C2410_IICCON_ACKEN)) {
>> + usleep_range(100, 200);
>> + return true;
> What is the real issue here? Is the value of S3C2410_IICCON_ACKEN
> enabling/disabling irq's?
It is not about the enabling/disabling interrupts, but controlling the
bus state. This bit is named as 'Acknowledge generation / I2C-bus
acknowledge enable bit' in Exynos reference manual:
In Tx mode, the I2CSDA is idle in the ACK time.
In Rx mode, the I2CSDA is low in the ACK time.
So it is a part of proper controlling the bus state, not the reported
interrupts, although the S3C2410_IICCON_ACKEN name is a bit misleading
in this case.
> Besides, if we use polling mode, shouldn't we disable the acks
> already in probe (even though they are disabled by default),
> never enable them before starting the message and avoid checking
> here everytime?
I assume that this polling mode is a special case, so there is no point
in optimizing it much. It is used only by the i2c core for some special
transfers to the PMIC during system reboot/shutdown or by the s3c24xx
i2c controller embedded in SoC for controlling some PHYs. Till now only
the second case was actually used. There were only a few single writes
done this way, so noone even noticed that the other types of transfers
(multi message or read) were broken... I found all those issues by
enabling polling mode unconditionally and fixing it to make all my test
systems working again.
>> + }
>> + if (tmp & S3C2410_IICCON_IRQPEND) {
>> if (!(readl(i2c->regs + S3C2410_IICSTAT)
>> & S3C2410_IICSTAT_LASTBIT))
>> return true;
>> --
>> 2.34.1
>>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
