[PATCH] i2c: samsung: add support for Exynos7 HS-I2C

[Kaustabh Chakraborty]

On 2025-10-27 05:15, Heiko Schocher wrote:
> Hello Kaustabh,
> 
> On 17.10.25 17:23, Kaustabh Chakraborty wrote:
>> Exynos7 (and later) HS-I2C blocks have special interrupts regarding
>> various data transfer states (see HSI2C_INT_I2C_TRANS_EN). Add support
>> for enabling and handling these interrupt bits.
>> 
>> Add the corresponding compatible, 'samsung,exynos7-hsi2c'. In order to
>> differentiate between the multiple device variants, an enum is
>> introduced which is used where difference in implementations exist.
>> 
>> Signed-off-by: Kaustabh Chakraborty <kauschluss at disroot.org>
>> ---
>>   drivers/i2c/exynos_hs_i2c.c | 105 ++++++++++++++++++++++++++++++++++++--------
>>   drivers/i2c/s3c24x0_i2c.h   |   7 +++
>>   2 files changed, 94 insertions(+), 18 deletions(-)
>> 
>> diff --git a/drivers/i2c/exynos_hs_i2c.c b/drivers/i2c/exynos_hs_i2c.c
>> index fa0d1c8f64ab593942a4ebe52f7b028366ce489c..67b497b228b151a2b23befc7fa62b34bbd07f388 100644
>> --- a/drivers/i2c/exynos_hs_i2c.c
>> +++ b/drivers/i2c/exynos_hs_i2c.c
>> @@ -54,6 +54,17 @@ DECLARE_GLOBAL_DATA_PTR;
>>   				 HSI2C_RX_OVERRUN_EN  |\
>>   				 HSI2C_INT_TRAILING_EN)
>>   +#define HSI2C_INT_TRANS_DONE		(1u << 7)
>> +#define HSI2C_INT_TRANS_ABORT		(1u << 8)
>> +#define HSI2C_INT_NO_DEV_ACK		(1u << 9)
>> +#define HSI2C_INT_NO_DEV		(1u << 10)
>> +#define HSI2C_INT_TIMEOUT_AUTO		(1u << 11)
>> +#define HSI2C_INT_I2C_TRANS_EN		(HSI2C_INT_TRANS_DONE  |\
>> +					 HSI2C_INT_TRANS_ABORT |\
>> +					 HSI2C_INT_NO_DEV_ACK  |\
>> +					 HSI2C_INT_NO_DEV      |\
>> +					 HSI2C_INT_TIMEOUT_AUTO)
>> +
>>   /* I2C_CONF Register bits */
>>   #define HSI2C_AUTO_MODE			(1u << 31)
>>   #define HSI2C_10BIT_ADDR_MODE		(1u << 30)
>> @@ -99,24 +110,32 @@ DECLARE_GLOBAL_DATA_PTR;
>>    * bit to be set, which indicates copletion of a transaction.
>>    *
>>    * @param i2c: pointer to the appropriate register bank
>> + * @param variant: hardware variant of the i2c device
>>    *
>>    * @return: I2C_OK in case of successful completion, I2C_NOK_TIMEOUT in case
>>    *          the status bits do not get set in time, or an approrpiate error
>>    *          value in case of transfer errors.
>>    */
>> -static int hsi2c_wait_for_trx(struct exynos5_hsi2c *i2c)
>> +static int hsi2c_wait_for_trx(struct exynos5_hsi2c *i2c,
>> +			      enum s3c24x0_i2c_variant variant)
>>   {
>>   	int i = HSI2C_TIMEOUT_US;
>>     	while (i-- > 0) {
>>   		u32 int_status = readl(&i2c->usi_int_stat);
>> +		u32 trans_status;
>>   -		if (int_status & HSI2C_INT_I2C_EN) {
>> -			u32 trans_status = readl(&i2c->usi_trans_status);
>> +		switch (variant) {
>> +		case VARIANT_HSI2C_EXYNOS5:
>> +			trans_status = readl(&i2c->usi_trans_status);
>>     			/* Deassert pending interrupt. */
>>   			writel(int_status, &i2c->usi_int_stat);
>>   +			if (!(int_status & HSI2C_INT_I2C_EN)) {
>> +				udelay(1);
>> +				continue;
>> +			}
>>   			if (trans_status & HSI2C_NO_DEV_ACK) {
>>   				debug("%s: no ACK from device\n", __func__);
>>   				return I2C_NACK;
>> @@ -134,8 +153,32 @@ static int hsi2c_wait_for_trx(struct exynos5_hsi2c *i2c)
>>   				return I2C_NOK_TOUT;
>>   			}
>>   			return I2C_OK;
>> +		case VARIANT_HSI2C_EXYNOS7:
>> +			if (int_status & HSI2C_INT_TRANS_DONE)
>> +				return I2C_OK;
>> +
>> +			if (int_status & HSI2C_INT_TRANS_ABORT) {
>> +				debug("%s: arbitration lost\n", __func__);
>> +				return I2C_NOK_LA;
>> +			}
>> +			if (int_status & HSI2C_NO_DEV_ACK) {
>> +				debug("%s: no ACK from device\n", __func__);
>> +				return I2C_NACK;
>> +			}
>> +			if (int_status & HSI2C_NO_DEV) {
>> +				debug("%s: no device\n", __func__);
>> +				return I2C_NOK;
>> +			}
>> +			if (int_status & HSI2C_TIMEOUT_AUTO) {
>> +				debug("%s: device timed out\n", __func__);
>> +				return I2C_NOK_TOUT;
>> +			}
>> +			udelay(1);
>> +			continue;
>> +		default:
>> +			debug("%s: unknown variant\n", __func__);
>> +			return I2C_NOK;
>>   		}
>> -		udelay(1);
>>   	}
>>   	debug("%s: transaction timeout!\n", __func__);
>>   	return I2C_NOK_TOUT;
> 
> May I oversee something .. but is for the new variant the i2c->usi_int_stat
> register ever cleared? I do not see a write for it in this case... or
> is it there cleared on read?

Yes, it should be cleared. That is not intended.

> 
> In linux driver:/drivers/i2c/busses/i2c-exynos5.c
> 
>  501 static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
> [...]
>  512         int_status = readl(i2c->regs + HSI2C_INT_STATUS);
>  513         writel(int_status, i2c->regs + HSI2C_INT_STATUS);
> 
> independent from the variant... may it is valid to do this in our
> driver too?

I believe this should be implemented. I did not observe any issues
with this method though, maybe it is cleared on read? Even if
that's the case, I reckon this should be the case for all variants
and follow the Linux driver's approach as its more battle-tested.

> 
> Some Testing on VARIANT_HSI2C_EXYNOS5 would be nice ... do you have
> such a hardware?

No, unfortunately.

> 
> Beside of this, change looks good to me.
> 
> Reviewed-by: Heiko Schocher <hs at nabladev.com>

I will apply this at the next revision which will clear the register
for all variants.

> 
> bye,
> Heiko
> 
> -- 
> Nabla Software Engineering
> HRB 40522 Augsburg
> Phone: +49 821 45592596
> E-Mail: office at nabladev.com
> Geschäftsführer : Stefano Babic