On Tue, Oct 31, 2023 at 02:49:04PM +0100, Marek Szyprowski wrote:
> 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.
Yes, correct, but I still don't understand this sequence in the
message_start:
- enable ACKEN in IICCON (enable_ack())
- read IICCON (iiccon = readl(...))
- write what you read in IICCON (writel(iiccon, ...))
- if ACKEN is disabled in IICCON (from your patch)
Where is supposed ACKEN to be disabled?
> > 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.
Yeah, I understand your point here.
It would be nice to have a pure polling mode supported though.
Thanks,
Andi
