Re: [PATCH v2 1/3] i2c: s3c24xx: fix read transfers in polling mode

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux for Synopsys ARC Processors]    
  • [Linux on Unisoc (RDA Micro) SoCs]     [Linux Actions SoC]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  •   Powered by Linux