[PATCH v2 0/3] Add atomic transfers to s3c24xx i2c driver

[PATCH v2 0/3] Add atomic transfers to s3c24xx i2c driver

[Marek Szyprowski]

Dear All,

This patchset adds support for atomic transfers, which has been added to
the i2c core recently by the commit 63b96983a5dd ("i2c: core: introduce
callbacks for atomic transfers") to hide warnings observed during system
reboot and power-off. Almost everything needed for that was already in
the driver as so called polling mode. Unfortunately, that polling mode
has been tested only with single message, write transfers so far and it
turned out that it doesn't work well with read and multi-message
transfers, so first it had to be fixed.

Best regards,
Marek Szyprowski


Changelog:
v2:
- updated and extended commit messages


Patch summary:

Marek Szyprowski (3):
  i2c: s3c24xx: fix read transfers in polling mode
  i2c: s3c24xx: fix transferring more than one message in polling mode
  i2c: s3c24xx: add support for atomic transfers

 drivers/i2c/busses/i2c-s3c2410.c | 55 +++++++++++++++++++++-----------
 1 file changed, 36 insertions(+), 19 deletions(-)

-- 
2.34.1

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

[Marek Szyprowski]

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@samsung.com>
---
 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;
+		}
+		if (tmp & S3C2410_IICCON_IRQPEND) {
 			if (!(readl(i2c->regs + S3C2410_IICSTAT)
 				& S3C2410_IICSTAT_LASTBIT))
 				return true;
-- 
2.34.1

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

[Chanho Park]

> -----Original Message-----
> From: Marek Szyprowski <m.szyprowski@samsung.com>
> Sent: Wednesday, October 25, 2023 9:17 PM
> To: linux-samsung-soc@vger.kernel.org; linux-i2c@vger.kernel.org
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>; Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org>; Alim Akhtar <alim.akhtar@samsung.com>;
> Andi Shyti <andi.shyti@kernel.org>; Wolfram Sang <wsa@kernel.org>
> Subject: [PATCH v2 1/3] i2c: s3c24xx: fix read transfers in polling mode
> 
> 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@samsung.com>

Reviewed-by: Chanho Park <chanho61.park@samsung.com>

> ---
>  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;
> +		}
> +		if (tmp & S3C2410_IICCON_IRQPEND) {
>  			if (!(readl(i2c->regs + S3C2410_IICSTAT)
>  				& S3C2410_IICSTAT_LASTBIT))
>  				return true;
> --
> 2.34.1

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

[Marek Szyprowski]

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@samsung.com>
>> ---
>>   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

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

[Andi Shyti]

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@samsung.com>
> >> ---
> >>   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

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

[Marek Szyprowski]

On 02.11.2023 01:49, Andi Shyti wrote:
> 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@samsung.com>
>>>> ---
>>>>    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?

It must be cleared by the hardware when read mode is set. Frankly 
speaking I didn't dig much into the driver internals and respective i2c 
bus states. I just logged all the call paths and driver states from the 
driver operating with interrupts enabled and fixed the polling mode to 
match what I have captured with interrupts enabled. After that changes, 
all my test boards finally booted properly with polling mode enabled 
unconditionally. This ACK related logic is a bit strange, but I don't 
really want to change driver logic and risk other regressions, so my 
changes were limited only to the code reachable during polling mode.

>>> 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.

Well, it is now with this patchset. It is just a matter of core to 
select it.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

[대인기/Tizen Platform Lab(SR)/삼성전자]

Hi,

> -----Original Message-----
> From: Marek Szyprowski <m.szyprowski@samsung.com>
> Sent: Wednesday, October 25, 2023 9:17 PM
> To: linux-samsung-soc@vger.kernel.org; linux-i2c@vger.kernel.org
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>; Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org>; Alim Akhtar <alim.akhtar@samsung.com>;
> Andi Shyti <andi.shyti@kernel.org>; Wolfram Sang <wsa@kernel.org>
> Subject: [PATCH v2 1/3] i2c: s3c24xx: fix read transfers in polling mode
> 
> 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@samsung.com>
> ---
>  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);

Trivial question, but is there any hardware specification related to sleeping for 100-200 microseconds? If any then it would be nice to use const variable instead and add some description about why sleeping here is needed.

Thanks,
Inki Dae

> +			return true;
> +		}
> +		if (tmp & S3C2410_IICCON_IRQPEND) {
>  			if (!(readl(i2c->regs + S3C2410_IICSTAT)
>  				& S3C2410_IICSTAT_LASTBIT))
>  				return true;
> --
> 2.34.1

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

[Marek Szyprowski]

Hi Inki,

On 02.11.2023 02:31, 대인기/Tizen Platform Lab(SR)/삼성전자 wrote:
>
>> -----Original Message-----
>> From: Marek Szyprowski<m.szyprowski@samsung.com>
>> Sent: Wednesday, October 25, 2023 9:17 PM
>> To:linux-samsung-soc@vger.kernel.org;linux-i2c@vger.kernel.org
>> Cc: Marek Szyprowski<m.szyprowski@samsung.com>; Krzysztof Kozlowski
>> <krzysztof.kozlowski@linaro.org>; Alim Akhtar<alim.akhtar@samsung.com>;
>> Andi Shyti<andi.shyti@kernel.org>; Wolfram Sang<wsa@kernel.org>
>> Subject: [PATCH v2 1/3] i2c: s3c24xx: fix read transfers in polling mode
>>
>> 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@samsung.com>
>> ---
>>   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);
> Trivial question, but is there any hardware specification related to sleeping for 100-200 microseconds? If any then it would be nice to use const variable instead and add some description about why sleeping here is needed.


Well, this is a bit magic value I got from my experiments. There is some 
delay needed there to let hardware to clear that bit and the values I 
proposed worked. If You don't like that, I can reuse the delay value 
that is already present in that loop: usleep_range(1000, 2000).


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

[Andi Shyti]

Hi Marek,

> >>   	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);
> > Trivial question, but is there any hardware specification related to sleeping for 100-200 microseconds? If any then it would be nice to use const variable instead and add some description about why sleeping here is needed.
> 
> 
> Well, this is a bit magic value I got from my experiments. There is some 
> delay needed there to let hardware to clear that bit and the values I 
> proposed worked. If You don't like that, I can reuse the delay value 
> that is already present in that loop: usleep_range(1000, 2000).

I also wanted to comment on this... please, then, add a comment
saying how you got to this range.

Andi

[PATCH v2 2/3] i2c: s3c24xx: fix transferring more than one message in polling mode

[Marek Szyprowski]

To properly handle ACK on the bus when transferring more than one
message in polling mode, move the polling handling loop from
s3c24xx_i2c_message_start() to s3c24xx_i2c_doxfer(). This way
i2c_s3c_irq_nextbyte() is always executed till the end, properly
acknowledging the IRQ bits and no recursive calls to
i2c_s3c_irq_nextbyte() are made.

While touching this, also fix finishing transfers in polling mode by
using common code path and always waiting for the bus to become idle
and disabled.

Fixes: 117053f77a5a ("i2c: s3c2410: Add polling mode support")
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/i2c/busses/i2c-s3c2410.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index f9dcb1112a61..8da85cb42980 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -275,16 +275,6 @@ static void s3c24xx_i2c_message_start(struct s3c24xx_i2c *i2c,
 
 	stat |= S3C2410_IICSTAT_START;
 	writel(stat, i2c->regs + S3C2410_IICSTAT);
-
-	if (i2c->quirks & QUIRK_POLL) {
-		while ((i2c->msg_num != 0) && is_ack(i2c)) {
-			i2c_s3c_irq_nextbyte(i2c, stat);
-			stat = readl(i2c->regs + S3C2410_IICSTAT);
-
-			if (stat & S3C2410_IICSTAT_ARBITR)
-				dev_err(i2c->dev, "deal with arbitration loss\n");
-		}
-	}
 }
 
 static inline void s3c24xx_i2c_stop(struct s3c24xx_i2c *i2c, int ret)
@@ -691,7 +681,7 @@ static void s3c24xx_i2c_wait_idle(struct s3c24xx_i2c *i2c)
 static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
 			      struct i2c_msg *msgs, int num)
 {
-	unsigned long timeout;
+	unsigned long timeout = 0;
 	int ret;
 
 	ret = s3c24xx_i2c_set_master(i2c);
@@ -711,16 +701,21 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
 	s3c24xx_i2c_message_start(i2c, msgs);
 
 	if (i2c->quirks & QUIRK_POLL) {
-		ret = i2c->msg_idx;
+		while ((i2c->msg_num != 0) && is_ack(i2c)) {
+			unsigned long stat = readl(i2c->regs + S3C2410_IICSTAT);
 
-		if (ret != num)
-			dev_dbg(i2c->dev, "incomplete xfer (%d)\n", ret);
+			i2c_s3c_irq_nextbyte(i2c, stat);
 
-		goto out;
+			stat = readl(i2c->regs + S3C2410_IICSTAT);
+			if (stat & S3C2410_IICSTAT_ARBITR)
+				dev_err(i2c->dev, "deal with arbitration loss\n");
+		}
+		goto skip_waiting;
 	}
 
 	timeout = wait_event_timeout(i2c->wait, i2c->msg_num == 0, HZ * 5);
 
+ skip_waiting:
 	ret = i2c->msg_idx;
 
 	/*
-- 
2.34.1

RE: [PATCH v2 2/3] i2c: s3c24xx: fix transferring more than one message in polling mode

[Chanho Park]

> -----Original Message-----
> From: Marek Szyprowski <m.szyprowski@samsung.com>
> Sent: Wednesday, October 25, 2023 9:17 PM
> To: linux-samsung-soc@vger.kernel.org; linux-i2c@vger.kernel.org
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>; Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org>; Alim Akhtar <alim.akhtar@samsung.com>;
> Andi Shyti <andi.shyti@kernel.org>; Wolfram Sang <wsa@kernel.org>
> Subject: [PATCH v2 2/3] i2c: s3c24xx: fix transferring more than one
> message in polling mode
> 
> To properly handle ACK on the bus when transferring more than one
> message in polling mode, move the polling handling loop from
> s3c24xx_i2c_message_start() to s3c24xx_i2c_doxfer(). This way
> i2c_s3c_irq_nextbyte() is always executed till the end, properly
> acknowledging the IRQ bits and no recursive calls to
> i2c_s3c_irq_nextbyte() are made.
> 
> While touching this, also fix finishing transfers in polling mode by
> using common code path and always waiting for the bus to become idle
> and disabled.
> 
> Fixes: 117053f77a5a ("i2c: s3c2410: Add polling mode support")
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

Reviewed-by: Chanho Park <chanho61.park@samsung.com>

> ---
>  drivers/i2c/busses/i2c-s3c2410.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-
> s3c2410.c
> index f9dcb1112a61..8da85cb42980 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -275,16 +275,6 @@ static void s3c24xx_i2c_message_start(struct
> s3c24xx_i2c *i2c,
> 
>  	stat |= S3C2410_IICSTAT_START;
>  	writel(stat, i2c->regs + S3C2410_IICSTAT);
> -
> -	if (i2c->quirks & QUIRK_POLL) {
> -		while ((i2c->msg_num != 0) && is_ack(i2c)) {
> -			i2c_s3c_irq_nextbyte(i2c, stat);
> -			stat = readl(i2c->regs + S3C2410_IICSTAT);
> -
> -			if (stat & S3C2410_IICSTAT_ARBITR)
> -				dev_err(i2c->dev, "deal with arbitration
> loss\n");
> -		}
> -	}
>  }
> 
>  static inline void s3c24xx_i2c_stop(struct s3c24xx_i2c *i2c, int ret)
> @@ -691,7 +681,7 @@ static void s3c24xx_i2c_wait_idle(struct s3c24xx_i2c
> *i2c)
>  static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
>  			      struct i2c_msg *msgs, int num)
>  {
> -	unsigned long timeout;
> +	unsigned long timeout = 0;
>  	int ret;
> 
>  	ret = s3c24xx_i2c_set_master(i2c);
> @@ -711,16 +701,21 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c
> *i2c,
>  	s3c24xx_i2c_message_start(i2c, msgs);
> 
>  	if (i2c->quirks & QUIRK_POLL) {
> -		ret = i2c->msg_idx;
> +		while ((i2c->msg_num != 0) && is_ack(i2c)) {
> +			unsigned long stat = readl(i2c->regs +
> S3C2410_IICSTAT);
> 
> -		if (ret != num)
> -			dev_dbg(i2c->dev, "incomplete xfer (%d)\n", ret);
> +			i2c_s3c_irq_nextbyte(i2c, stat);
> 
> -		goto out;
> +			stat = readl(i2c->regs + S3C2410_IICSTAT);
> +			if (stat & S3C2410_IICSTAT_ARBITR)
> +				dev_err(i2c->dev, "deal with arbitration
> loss\n");
> +		}
> +		goto skip_waiting;
>  	}
> 
>  	timeout = wait_event_timeout(i2c->wait, i2c->msg_num == 0, HZ * 5);
> 
> + skip_waiting:
>  	ret = i2c->msg_idx;
> 
>  	/*
> --
> 2.34.1

[PATCH v2 3/3] i2c: s3c24xx: add support for atomic transfers

[Marek Szyprowski]

Add support for atomic transfers using polling mode with interrupts
intentionally disabled to get rid of the warning introduced by commit
63b96983a5dd ("i2c: core: introduce callbacks for atomic transfers")
during system reboot and power-off.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/i2c/busses/i2c-s3c2410.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 8da85cb42980..586b82b30bdf 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -76,6 +76,7 @@
 #define QUIRK_HDMIPHY		(1 << 1)
 #define QUIRK_NO_GPIO		(1 << 2)
 #define QUIRK_POLL		(1 << 3)
+#define QUIRK_ATOMIC		(1 << 4)
 
 /* Max time to wait for bus to become idle after a xfer (in us) */
 #define S3C2410_IDLE_TIMEOUT	5000
@@ -174,7 +175,7 @@ static inline void s3c24xx_i2c_master_complete(struct s3c24xx_i2c *i2c, int ret)
 	if (ret)
 		i2c->msg_idx = ret;
 
-	if (!(i2c->quirks & QUIRK_POLL))
+	if (!(i2c->quirks & (QUIRK_POLL | QUIRK_ATOMIC)))
 		wake_up(&i2c->wait);
 }
 
@@ -700,7 +701,7 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
 	s3c24xx_i2c_enable_irq(i2c);
 	s3c24xx_i2c_message_start(i2c, msgs);
 
-	if (i2c->quirks & QUIRK_POLL) {
+	if (i2c->quirks & (QUIRK_POLL | QUIRK_ATOMIC)) {
 		while ((i2c->msg_num != 0) && is_ack(i2c)) {
 			unsigned long stat = readl(i2c->regs + S3C2410_IICSTAT);
 
@@ -774,6 +775,21 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
 	return -EREMOTEIO;
 }
 
+static int s3c24xx_i2c_xfer_atomic(struct i2c_adapter *adap,
+				   struct i2c_msg *msgs, int num)
+{
+	struct s3c24xx_i2c *i2c = (struct s3c24xx_i2c *)adap->algo_data;
+	int ret;
+
+	disable_irq(i2c->irq);
+	i2c->quirks |= QUIRK_ATOMIC;
+	ret = s3c24xx_i2c_xfer(adap, msgs, num);
+	i2c->quirks &= ~QUIRK_ATOMIC;
+	enable_irq(i2c->irq);
+
+	return ret;
+}
+
 /* declare our i2c functionality */
 static u32 s3c24xx_i2c_func(struct i2c_adapter *adap)
 {
@@ -784,6 +800,7 @@ static u32 s3c24xx_i2c_func(struct i2c_adapter *adap)
 /* i2c bus registration info */
 static const struct i2c_algorithm s3c24xx_i2c_algorithm = {
 	.master_xfer		= s3c24xx_i2c_xfer,
+	.master_xfer_atomic     = s3c24xx_i2c_xfer_atomic,
 	.functionality		= s3c24xx_i2c_func,
 };
 
-- 
2.34.1

RE: [PATCH v2 3/3] i2c: s3c24xx: add support for atomic transfers

[Chanho Park]

> -----Original Message-----
> From: Marek Szyprowski <m.szyprowski@samsung.com>
> Sent: Wednesday, October 25, 2023 9:17 PM
> To: linux-samsung-soc@vger.kernel.org; linux-i2c@vger.kernel.org
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>; Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org>; Alim Akhtar <alim.akhtar@samsung.com>;
> Andi Shyti <andi.shyti@kernel.org>; Wolfram Sang <wsa@kernel.org>
> Subject: [PATCH v2 3/3] i2c: s3c24xx: add support for atomic transfers
> 
> 
> Add support for atomic transfers using polling mode with interrupts
> intentionally disabled to get rid of the warning introduced by commit
> 63b96983a5dd ("i2c: core: introduce callbacks for atomic transfers")
> during system reboot and power-off.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

Reviewed-by: Chanho Park <chanho61.park@samsung.com>

> ---
>  drivers/i2c/busses/i2c-s3c2410.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-
> s3c2410.c
> index 8da85cb42980..586b82b30bdf 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -76,6 +76,7 @@
>  #define QUIRK_HDMIPHY		(1 << 1)
>  #define QUIRK_NO_GPIO		(1 << 2)
>  #define QUIRK_POLL		(1 << 3)
> +#define QUIRK_ATOMIC		(1 << 4)
> 
>  /* Max time to wait for bus to become idle after a xfer (in us) */
>  #define S3C2410_IDLE_TIMEOUT	5000
> @@ -174,7 +175,7 @@ static inline void s3c24xx_i2c_master_complete(struct
> s3c24xx_i2c *i2c, int ret)
>  	if (ret)
>  		i2c->msg_idx = ret;
> 
> -	if (!(i2c->quirks & QUIRK_POLL))
> +	if (!(i2c->quirks & (QUIRK_POLL | QUIRK_ATOMIC)))
>  		wake_up(&i2c->wait);
>  }
> 
> @@ -700,7 +701,7 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
>  	s3c24xx_i2c_enable_irq(i2c);
>  	s3c24xx_i2c_message_start(i2c, msgs);
> 
> -	if (i2c->quirks & QUIRK_POLL) {
> +	if (i2c->quirks & (QUIRK_POLL | QUIRK_ATOMIC)) {
>  		while ((i2c->msg_num != 0) && is_ack(i2c)) {
>  			unsigned long stat = readl(i2c->regs +
> S3C2410_IICSTAT);
> 
> @@ -774,6 +775,21 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
>  	return -EREMOTEIO;
>  }
> 
> +static int s3c24xx_i2c_xfer_atomic(struct i2c_adapter *adap,
> +				   struct i2c_msg *msgs, int num)
> +{
> +	struct s3c24xx_i2c *i2c = (struct s3c24xx_i2c *)adap->algo_data;
> +	int ret;
> +
> +	disable_irq(i2c->irq);
> +	i2c->quirks |= QUIRK_ATOMIC;
> +	ret = s3c24xx_i2c_xfer(adap, msgs, num);
> +	i2c->quirks &= ~QUIRK_ATOMIC;
> +	enable_irq(i2c->irq);
> +
> +	return ret;
> +}
> +
>  /* declare our i2c functionality */
>  static u32 s3c24xx_i2c_func(struct i2c_adapter *adap)
>  {
> @@ -784,6 +800,7 @@ static u32 s3c24xx_i2c_func(struct i2c_adapter *adap)
>  /* i2c bus registration info */
>  static const struct i2c_algorithm s3c24xx_i2c_algorithm = {
>  	.master_xfer		= s3c24xx_i2c_xfer,
> +	.master_xfer_atomic     = s3c24xx_i2c_xfer_atomic,
>  	.functionality		= s3c24xx_i2c_func,
>  };
> 
> --
> 2.34.1