[PATCH v2] i2c/pasemi: PASemi I2C controller IRQ enablement

[PATCH v2] i2c/pasemi: PASemi I2C controller IRQ enablement

[Arminder Singh]

Hello,

This is v2 of the PASemi I2C controller IRQ enablement patch.

This patch adds IRQ support to the PASemi I2C controller driver to 
increase the performace of I2C transactions on platforms with PASemi I2C 
controllers. While the patch is primarily intended for Apple silicon 
platforms, this patch should also help in enabling IRQ support for 
older PASemi hardware as well should the need arise.

This version of the patch has been tested on an M1 Ultra Mac Studio,
as well as an M1 MacBook Pro, and userspace launches successfully
while using the IRQ path for I2C transactions.

Tested-by: Arminder Singh <arminders208@outlook.com>
Signed-off-by: Arminder Singh <arminders208@outlook.com>
---
Changes from v1:
 - moved completion setup from pasemi_platform_i2c_probe to
   pasemi_i2c_common_probe to allow PASemi and Apple platforms to share
   common completion setup code in case PASemi hardware gets IRQ support
   added
 - initialized the status variable in pasemi_smb_waitready when going down
   the non-IRQ path
 - removed an unnecessary cast of dev_id in the IRQ handler
 - fixed alignment of struct member names in i2c-pasemi-core.h
   (addresses Christophe's feedback in the original submission)
 - IRQs are now disabled after the wait_for_completion_timeout call
   instead of inside the IRQ handler
   (prevents the IRQ from going off after the completion times out)
 - changed the request_irq call to a devm_request_irq call to obviate
   the need for a remove function and a free_irq call
   (thanks to Sven for pointing this out in the original submission)
 - added a reinit_completion call to pasemi_reset 
   as a failsafe to prevent missed interrupts from causing the completion
   to never complete (thanks to Arnd Bergmann for pointing this out)
 - removed the bitmask variable in favor of just using the value
   directly (it wasn't used anywhere else)

v1 linked here: https://lore.kernel.org/linux-i2c/MN2PR01MB535838492432C910F2381F929F6F9@MN2PR01MB5358.prod.exchangelabs.com/T/#m11b3504c2667517aad7521514c99ca0e07a9381f

Thanks for all the feedback on the previous submission, I'm sorry
I wasn't able to answer everyone's emails, was just pretty busy, I'll
make sure to be more responsive this time around! Also wasn't sure whether
the v1 changelog belonged before or after the '---' so I put it after
to keep the commit changelog short and concise.
(This is just one patch, didn't think it needed a cover letter)

 drivers/i2c/busses/i2c-pasemi-core.c     | 29 ++++++++++++++++++++----
 drivers/i2c/busses/i2c-pasemi-core.h     |  7 +++++-
 drivers/i2c/busses/i2c-pasemi-platform.c |  6 +++++
 3 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pasemi-core.c b/drivers/i2c/busses/i2c-pasemi-core.c
index 9028ffb58cc0..05af8f3575bc 100644
--- a/drivers/i2c/busses/i2c-pasemi-core.c
+++ b/drivers/i2c/busses/i2c-pasemi-core.c
@@ -21,6 +21,7 @@
 #define REG_MTXFIFO	0x00
 #define REG_MRXFIFO	0x04
 #define REG_SMSTA	0x14
+#define REG_IMASK   0x18
 #define REG_CTL		0x1c
 #define REG_REV		0x28
 
@@ -66,6 +67,7 @@ static void pasemi_reset(struct pasemi_smbus *smbus)
 		val |= CTL_EN;
 
 	reg_write(smbus, REG_CTL, val);
+	reinit_completion(&smbus->irq_completion);
 }
 
 static void pasemi_smb_clear(struct pasemi_smbus *smbus)
@@ -81,11 +83,18 @@ static int pasemi_smb_waitready(struct pasemi_smbus *smbus)
 	int timeout = 10;
 	unsigned int status;
 
-	status = reg_read(smbus, REG_SMSTA);
-
-	while (!(status & SMSTA_XEN) && timeout--) {
-		msleep(1);
+	if (smbus->use_irq) {
+		reinit_completion(&smbus->irq_completion);
+		reg_write(smbus, REG_IMASK, SMSTA_XEN | SMSTA_MTN);
+		wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(10));
+		reg_write(smbus, REG_IMASK, 0);
 		status = reg_read(smbus, REG_SMSTA);
+	} else {
+		status = reg_read(smbus, REG_SMSTA);
+		while (!(status & SMSTA_XEN) && timeout--) {
+			msleep(1);
+			status = reg_read(smbus, REG_SMSTA);
+		}
 	}
 
 	/* Got NACK? */
@@ -344,10 +353,14 @@ int pasemi_i2c_common_probe(struct pasemi_smbus *smbus)
 
 	/* set up the sysfs linkage to our parent device */
 	smbus->adapter.dev.parent = smbus->dev;
+	smbus->use_irq = 0;
+	init_completion(&smbus->irq_completion);
 
 	if (smbus->hw_rev != PASEMI_HW_REV_PCI)
 		smbus->hw_rev = reg_read(smbus, REG_REV);
 
+	reg_write(smbus, REG_IMASK, 0);
+
 	pasemi_reset(smbus);
 
 	error = devm_i2c_add_adapter(smbus->dev, &smbus->adapter);
@@ -356,3 +369,11 @@ int pasemi_i2c_common_probe(struct pasemi_smbus *smbus)
 
 	return 0;
 }
+
+irqreturn_t pasemi_irq_handler(int irq, void *dev_id)
+{
+	struct pasemi_smbus *smbus = dev_id;
+
+	complete(&smbus->irq_completion);
+	return IRQ_HANDLED;
+}
diff --git a/drivers/i2c/busses/i2c-pasemi-core.h b/drivers/i2c/busses/i2c-pasemi-core.h
index 4655124a37f3..ba6d6ccf9cdc 100644
--- a/drivers/i2c/busses/i2c-pasemi-core.h
+++ b/drivers/i2c/busses/i2c-pasemi-core.h
@@ -7,6 +7,7 @@
 #include <linux/i2c-smbus.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
+#include <linux/completion.h>
 
 #define PASEMI_HW_REV_PCI -1
 
@@ -15,7 +16,11 @@ struct pasemi_smbus {
 	struct i2c_adapter	 adapter;
 	void __iomem		*ioaddr;
 	unsigned int		 clk_div;
-	int			 hw_rev;
+	int			         hw_rev;
+	int                  use_irq;
+	struct completion    irq_completion;
 };
 
 int pasemi_i2c_common_probe(struct pasemi_smbus *smbus);
+
+irqreturn_t pasemi_irq_handler(int irq, void *dev_id);
diff --git a/drivers/i2c/busses/i2c-pasemi-platform.c b/drivers/i2c/busses/i2c-pasemi-platform.c
index 88a54aaf7e3c..e35945a91dbe 100644
--- a/drivers/i2c/busses/i2c-pasemi-platform.c
+++ b/drivers/i2c/busses/i2c-pasemi-platform.c
@@ -49,6 +49,7 @@ static int pasemi_platform_i2c_probe(struct platform_device *pdev)
 	struct pasemi_smbus *smbus;
 	u32 frequency;
 	int error;
+	int irq_num;
 
 	data = devm_kzalloc(dev, sizeof(struct pasemi_platform_i2c_data),
 			    GFP_KERNEL);
@@ -82,6 +83,11 @@ static int pasemi_platform_i2c_probe(struct platform_device *pdev)
 	if (error)
 		goto out_clk_disable;
 
+	irq_num = platform_get_irq(pdev, 0);
+	error = devm_request_irq(smbus->dev, irq_num, pasemi_irq_handler, 0, "pasemi_apple_i2c", (void *)smbus);
+
+	if (!error)
+		smbus->use_irq = 1;
 	platform_set_drvdata(pdev, data);
 
 	return 0;
-- 
2.34.1

Re: [PATCH v2] i2c/pasemi: PASemi I2C controller IRQ enablement

[Sven Peter]

Hi,

Looks almost good to me, just a few minor things:

On Sun, Oct 2, 2022, at 00:25, Arminder Singh wrote:
> Hello,
>
> This is v2 of the PASemi I2C controller IRQ enablement patch.

This shouldn't be inside the commit description.

>
> This patch adds IRQ support to the PASemi I2C controller driver to 
> increase the performace of I2C transactions on platforms with PASemi I2C 
> controllers. While the patch is primarily intended for Apple silicon 
> platforms, this patch should also help in enabling IRQ support for 
> older PASemi hardware as well should the need arise.

This is probably the only paragraph that should be the entire commit description.

>
> This version of the patch has been tested on an M1 Ultra Mac Studio,
> as well as an M1 MacBook Pro, and userspace launches successfully
> while using the IRQ path for I2C transactions.
>
> Tested-by: Arminder Singh <arminders208@outlook.com>

I think it's usually implied that you tested your own patches ;)

> Signed-off-by: Arminder Singh <arminders208@outlook.com>
> ---
> Changes from v1:
>  - moved completion setup from pasemi_platform_i2c_probe to
>    pasemi_i2c_common_probe to allow PASemi and Apple platforms to share
>    common completion setup code in case PASemi hardware gets IRQ support
>    added
>  - initialized the status variable in pasemi_smb_waitready when going down
>    the non-IRQ path
>  - removed an unnecessary cast of dev_id in the IRQ handler
>  - fixed alignment of struct member names in i2c-pasemi-core.h
>    (addresses Christophe's feedback in the original submission)
>  - IRQs are now disabled after the wait_for_completion_timeout call
>    instead of inside the IRQ handler
>    (prevents the IRQ from going off after the completion times out)
>  - changed the request_irq call to a devm_request_irq call to obviate
>    the need for a remove function and a free_irq call
>    (thanks to Sven for pointing this out in the original submission)
>  - added a reinit_completion call to pasemi_reset 
>    as a failsafe to prevent missed interrupts from causing the completion
>    to never complete (thanks to Arnd Bergmann for pointing this out)
>  - removed the bitmask variable in favor of just using the value
>    directly (it wasn't used anywhere else)
>
> v1 linked here: 
> https://lore.kernel.org/linux-i2c/MN2PR01MB535838492432C910F2381F929F6F9@MN2PR01MB5358.prod.exchangelabs.com/T/#m11b3504c2667517aad7521514c99ca0e07a9381f
>
> Thanks for all the feedback on the previous submission, I'm sorry
> I wasn't able to answer everyone's emails, was just pretty busy, I'll
> make sure to be more responsive this time around! Also wasn't sure whether
> the v1 changelog belonged before or after the '---' so I put it after
> to keep the commit changelog short and concise.
> (This is just one patch, didn't think it needed a cover letter)
>
>  drivers/i2c/busses/i2c-pasemi-core.c     | 29 ++++++++++++++++++++----
>  drivers/i2c/busses/i2c-pasemi-core.h     |  7 +++++-
>  drivers/i2c/busses/i2c-pasemi-platform.c |  6 +++++
>  3 files changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-pasemi-core.c 
> b/drivers/i2c/busses/i2c-pasemi-core.c
> index 9028ffb58cc0..05af8f3575bc 100644
> --- a/drivers/i2c/busses/i2c-pasemi-core.c
> +++ b/drivers/i2c/busses/i2c-pasemi-core.c
> @@ -21,6 +21,7 @@
>  #define REG_MTXFIFO	0x00
>  #define REG_MRXFIFO	0x04
>  #define REG_SMSTA	0x14
> +#define REG_IMASK   0x18

This doesn't seem to be aligned correctly, this file seems to use a tab
to separate the register name and the offset and you used spaces here.

>  #define REG_CTL		0x1c
>  #define REG_REV		0x28
> 
> @@ -66,6 +67,7 @@ static void pasemi_reset(struct pasemi_smbus *smbus)
>  		val |= CTL_EN;
> 
>  	reg_write(smbus, REG_CTL, val);
> +	reinit_completion(&smbus->irq_completion);
>  }
> 
>  static void pasemi_smb_clear(struct pasemi_smbus *smbus)
> @@ -81,11 +83,18 @@ static int pasemi_smb_waitready(struct pasemi_smbus *smbus)
>  	int timeout = 10;
>  	unsigned int status;
> 
> -	status = reg_read(smbus, REG_SMSTA);
> -
> -	while (!(status & SMSTA_XEN) && timeout--) {
> -		msleep(1);
> +	if (smbus->use_irq) {
> +		reinit_completion(&smbus->irq_completion);
> +		reg_write(smbus, REG_IMASK, SMSTA_XEN | SMSTA_MTN);
> +		wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(10));
> +		reg_write(smbus, REG_IMASK, 0);
>  		status = reg_read(smbus, REG_SMSTA);
> +	} else {
> +		status = reg_read(smbus, REG_SMSTA);
> +		while (!(status & SMSTA_XEN) && timeout--) {
> +			msleep(1);
> +			status = reg_read(smbus, REG_SMSTA);
> +		}
>  	}
> 
>  	/* Got NACK? */
> @@ -344,10 +353,14 @@ int pasemi_i2c_common_probe(struct pasemi_smbus *smbus)
> 
>  	/* set up the sysfs linkage to our parent device */
>  	smbus->adapter.dev.parent = smbus->dev;
> +	smbus->use_irq = 0;
> +	init_completion(&smbus->irq_completion);
> 
>  	if (smbus->hw_rev != PASEMI_HW_REV_PCI)
>  		smbus->hw_rev = reg_read(smbus, REG_REV);
> 
> +	reg_write(smbus, REG_IMASK, 0);
> +
>  	pasemi_reset(smbus);
> 
>  	error = devm_i2c_add_adapter(smbus->dev, &smbus->adapter);
> @@ -356,3 +369,11 @@ int pasemi_i2c_common_probe(struct pasemi_smbus *smbus)
> 
>  	return 0;
>  }
> +
> +irqreturn_t pasemi_irq_handler(int irq, void *dev_id)
> +{
> +	struct pasemi_smbus *smbus = dev_id;
> +
> +	complete(&smbus->irq_completion);
> +	return IRQ_HANDLED;
> +}
> diff --git a/drivers/i2c/busses/i2c-pasemi-core.h 
> b/drivers/i2c/busses/i2c-pasemi-core.h
> index 4655124a37f3..ba6d6ccf9cdc 100644
> --- a/drivers/i2c/busses/i2c-pasemi-core.h
> +++ b/drivers/i2c/busses/i2c-pasemi-core.h
> @@ -7,6 +7,7 @@
>  #include <linux/i2c-smbus.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
> +#include <linux/completion.h>
> 
>  #define PASEMI_HW_REV_PCI -1
> 
> @@ -15,7 +16,11 @@ struct pasemi_smbus {
>  	struct i2c_adapter	 adapter;
>  	void __iomem		*ioaddr;
>  	unsigned int		 clk_div;
> -	int			 hw_rev;
> +	int			         hw_rev;
> +	int                  use_irq;
> +	struct completion    irq_completion;

This doesn't seem to be aligned correctly and the hw_rev line
doesn't have to be changed.

>  };
> 
>  int pasemi_i2c_common_probe(struct pasemi_smbus *smbus);
> +
> +irqreturn_t pasemi_irq_handler(int irq, void *dev_id);
> diff --git a/drivers/i2c/busses/i2c-pasemi-platform.c 
> b/drivers/i2c/busses/i2c-pasemi-platform.c
> index 88a54aaf7e3c..e35945a91dbe 100644
> --- a/drivers/i2c/busses/i2c-pasemi-platform.c
> +++ b/drivers/i2c/busses/i2c-pasemi-platform.c
> @@ -49,6 +49,7 @@ static int pasemi_platform_i2c_probe(struct 
> platform_device *pdev)
>  	struct pasemi_smbus *smbus;
>  	u32 frequency;
>  	int error;
> +	int irq_num;
> 
>  	data = devm_kzalloc(dev, sizeof(struct pasemi_platform_i2c_data),
>  			    GFP_KERNEL);
> @@ -82,6 +83,11 @@ static int pasemi_platform_i2c_probe(struct 
> platform_device *pdev)
>  	if (error)
>  		goto out_clk_disable;
> 
> +	irq_num = platform_get_irq(pdev, 0);
> +	error = devm_request_irq(smbus->dev, irq_num, pasemi_irq_handler, 0, 
> "pasemi_apple_i2c", (void *)smbus);
> +
> +	if (!error)
> +		smbus->use_irq = 1;
>  	platform_set_drvdata(pdev, data);
> 
>  	return 0;
> -- 
> 2.34.1


Best,

Sven

Re: [PATCH v2] i2c/pasemi: PASemi I2C controller IRQ enablement

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 856 bytes --]

Hi,

some comments from me. Thanks for the patch and review!

> > This version of the patch has been tested on an M1 Ultra Mac Studio,
> > as well as an M1 MacBook Pro, and userspace launches successfully
> > while using the IRQ path for I2C transactions.
> >
> > Tested-by: Arminder Singh <arminders208@outlook.com>
> 
> I think it's usually implied that you tested your own patches ;)

Yes, the tag is superfluous. The paragraph before is nice, though, to
learn which testing has been applied.

> > make sure to be more responsive this time around! Also wasn't sure whether
> > the v1 changelog belonged before or after the '---' so I put it after
> > to keep the commit changelog short and concise.
> > (This is just one patch, didn't think it needed a cover letter)

Both assumptions are correct.

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] i2c-pasemi: PASemi I2C controller IRQ enablement

[Arminder Singh]

Hi,

>  #define REG_MTXFIFO	0x00
>  #define REG_MRXFIFO	0x04
>  #define REG_SMSTA	0x14
> +#define REG_IMASK   0x18

> This doesn't seem to be aligned correctly, this file seems to use a tab
> to separate the register name and the offset and you used spaces here.

> @@ -15,7 +16,11 @@ struct pasemi_smbus {
>  	struct i2c_adapter	 adapter;
>  	void __iomem		*ioaddr;
>  	unsigned int		 clk_div;
> -	int			 hw_rev;
> +	int			         hw_rev;
> +	int                  use_irq;
> +	struct completion    irq_completion;

> This doesn't seem to be aligned correctly and the hw_rev line
> doesn't have to be changed.

I'm sorry for the alignment issues in the patch, I genuinely didn't notice
them as from the perspective of my primary editor (Visual Studio Code)
the entries were aligned. I just saw them when opening the files in
nano.

Does fixing the alignment issues and the commit description justify a v3
of the patch or should the minor fixes go out as a "resend"? Just not sure
in this particular case as the fixes seem to be very minor ones.

Thanks,
Arminder

Re: [PATCH v2] i2c-pasemi: PASemi I2C controller IRQ enablement

[Sven Peter]

Hi,


On Sun, Oct 2, 2022, at 16:07, Arminder Singh wrote:
> Hi,
>
>>  #define REG_MTXFIFO	0x00
>>  #define REG_MRXFIFO	0x04
>>  #define REG_SMSTA	0x14
>> +#define REG_IMASK   0x18
>
>> This doesn't seem to be aligned correctly, this file seems to use a tab
>> to separate the register name and the offset and you used spaces here.
>
>> @@ -15,7 +16,11 @@ struct pasemi_smbus {
>>  	struct i2c_adapter	 adapter;
>>  	void __iomem		*ioaddr;
>>  	unsigned int		 clk_div;
>> -	int			 hw_rev;
>> +	int			         hw_rev;
>> +	int                  use_irq;
>> +	struct completion    irq_completion;
>
>> This doesn't seem to be aligned correctly and the hw_rev line
>> doesn't have to be changed.
>
> I'm sorry for the alignment issues in the patch, I genuinely didn't notice
> them as from the perspective of my primary editor (Visual Studio Code)
> the entries were aligned. I just saw them when opening the files in
> nano.

No worries, it's just a small nit and quickly fixed after all! :)

>
> Does fixing the alignment issues and the commit description justify a v3
> of the patch or should the minor fixes go out as a "resend"? Just not sure
> in this particular case as the fixes seem to be very minor ones.

I'd send a v3. I've only used resend when e.g. my previous mail provider
messed up and silently converted all my outgoing mails to HTML.


Best,

Sven

Re: [PATCH v2] i2c/pasemi: PASemi I2C controller IRQ enablement

[Christian Zigotzky]

On 02 October 2022 at 00:25 am, Arminder Singh wrote:
> Hello,
>
> This is v2 of the PASemi I2C controller IRQ enablement patch.
>
> This patch adds IRQ support to the PASemi I2C controller driver to
> increase the performace of I2C transactions on platforms with PASemi I2C
> controllers. While the patch is primarily intended for Apple silicon
> platforms, this patch should also help in enabling IRQ support for
> older PASemi hardware as well should the need arise.
>
> This version of the patch has been tested on an M1 Ultra Mac Studio,
> as well as an M1 MacBook Pro, and userspace launches successfully
> while using the IRQ path for I2C transactions.
>
> Tested-by: Arminder Singh <arminders208@outlook.com>
> Signed-off-by: Arminder Singh <arminders208@outlook.com>
> ---
> Changes from v1:
>   - moved completion setup from pasemi_platform_i2c_probe to
>     pasemi_i2c_common_probe to allow PASemi and Apple platforms to share
>     common completion setup code in case PASemi hardware gets IRQ support
>     added
>   - initialized the status variable in pasemi_smb_waitready when going down
>     the non-IRQ path
>   - removed an unnecessary cast of dev_id in the IRQ handler
>   - fixed alignment of struct member names in i2c-pasemi-core.h
>     (addresses Christophe's feedback in the original submission)
>   - IRQs are now disabled after the wait_for_completion_timeout call
>     instead of inside the IRQ handler
>     (prevents the IRQ from going off after the completion times out)
>   - changed the request_irq call to a devm_request_irq call to obviate
>     the need for a remove function and a free_irq call
>     (thanks to Sven for pointing this out in the original submission)
>   - added a reinit_completion call to pasemi_reset
>     as a failsafe to prevent missed interrupts from causing the completion
>     to never complete (thanks to Arnd Bergmann for pointing this out)
>   - removed the bitmask variable in favor of just using the value
>     directly (it wasn't used anywhere else)
>
> v1 linked here: https://lore.kernel.org/linux-i2c/MN2PR01MB535838492432C910F2381F929F6F9@MN2PR01MB5358.prod.exchangelabs.com/T/#m11b3504c2667517aad7521514c99ca0e07a9381f
>
> Thanks for all the feedback on the previous submission, I'm sorry
> I wasn't able to answer everyone's emails, was just pretty busy, I'll
> make sure to be more responsive this time around! Also wasn't sure whether
> the v1 changelog belonged before or after the '---' so I put it after
> to keep the commit changelog short and concise.
> (This is just one patch, didn't think it needed a cover letter)
>
>   drivers/i2c/busses/i2c-pasemi-core.c     | 29 ++++++++++++++++++++----
>   drivers/i2c/busses/i2c-pasemi-core.h     |  7 +++++-
>   drivers/i2c/busses/i2c-pasemi-platform.c |  6 +++++
>   3 files changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-pasemi-core.c b/drivers/i2c/busses/i2c-pasemi-core.c
> index 9028ffb58cc0..05af8f3575bc 100644
> --- a/drivers/i2c/busses/i2c-pasemi-core.c
> +++ b/drivers/i2c/busses/i2c-pasemi-core.c
> @@ -21,6 +21,7 @@
>   #define REG_MTXFIFO	0x00
>   #define REG_MRXFIFO	0x04
>   #define REG_SMSTA	0x14
> +#define REG_IMASK   0x18
>   #define REG_CTL		0x1c
>   #define REG_REV		0x28
>   
> @@ -66,6 +67,7 @@ static void pasemi_reset(struct pasemi_smbus *smbus)
>   		val |= CTL_EN;
>   
>   	reg_write(smbus, REG_CTL, val);
> +	reinit_completion(&smbus->irq_completion);
>   }
>   
>   static void pasemi_smb_clear(struct pasemi_smbus *smbus)
> @@ -81,11 +83,18 @@ static int pasemi_smb_waitready(struct pasemi_smbus *smbus)
>   	int timeout = 10;
>   	unsigned int status;
>   
> -	status = reg_read(smbus, REG_SMSTA);
> -
> -	while (!(status & SMSTA_XEN) && timeout--) {
> -		msleep(1);
> +	if (smbus->use_irq) {
> +		reinit_completion(&smbus->irq_completion);
> +		reg_write(smbus, REG_IMASK, SMSTA_XEN | SMSTA_MTN);
> +		wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(10));
> +		reg_write(smbus, REG_IMASK, 0);
>   		status = reg_read(smbus, REG_SMSTA);
> +	} else {
> +		status = reg_read(smbus, REG_SMSTA);
> +		while (!(status & SMSTA_XEN) && timeout--) {
> +			msleep(1);
> +			status = reg_read(smbus, REG_SMSTA);
> +		}
>   	}
>   
>   	/* Got NACK? */
> @@ -344,10 +353,14 @@ int pasemi_i2c_common_probe(struct pasemi_smbus *smbus)
>   
>   	/* set up the sysfs linkage to our parent device */
>   	smbus->adapter.dev.parent = smbus->dev;
> +	smbus->use_irq = 0;
> +	init_completion(&smbus->irq_completion);
>   
>   	if (smbus->hw_rev != PASEMI_HW_REV_PCI)
>   		smbus->hw_rev = reg_read(smbus, REG_REV);
>   
> +	reg_write(smbus, REG_IMASK, 0);
> +
>   	pasemi_reset(smbus);
>   
>   	error = devm_i2c_add_adapter(smbus->dev, &smbus->adapter);
> @@ -356,3 +369,11 @@ int pasemi_i2c_common_probe(struct pasemi_smbus *smbus)
>   
>   	return 0;
>   }
> +
> +irqreturn_t pasemi_irq_handler(int irq, void *dev_id)
> +{
> +	struct pasemi_smbus *smbus = dev_id;
> +
> +	complete(&smbus->irq_completion);
> +	return IRQ_HANDLED;
> +}
> diff --git a/drivers/i2c/busses/i2c-pasemi-core.h b/drivers/i2c/busses/i2c-pasemi-core.h
> index 4655124a37f3..ba6d6ccf9cdc 100644
> --- a/drivers/i2c/busses/i2c-pasemi-core.h
> +++ b/drivers/i2c/busses/i2c-pasemi-core.h
> @@ -7,6 +7,7 @@
>   #include <linux/i2c-smbus.h>
>   #include <linux/io.h>
>   #include <linux/kernel.h>
> +#include <linux/completion.h>
>   
>   #define PASEMI_HW_REV_PCI -1
>   
> @@ -15,7 +16,11 @@ struct pasemi_smbus {
>   	struct i2c_adapter	 adapter;
>   	void __iomem		*ioaddr;
>   	unsigned int		 clk_div;
> -	int			 hw_rev;
> +	int			         hw_rev;
> +	int                  use_irq;
> +	struct completion    irq_completion;
>   };
>   
>   int pasemi_i2c_common_probe(struct pasemi_smbus *smbus);
> +
> +irqreturn_t pasemi_irq_handler(int irq, void *dev_id);
> diff --git a/drivers/i2c/busses/i2c-pasemi-platform.c b/drivers/i2c/busses/i2c-pasemi-platform.c
> index 88a54aaf7e3c..e35945a91dbe 100644
> --- a/drivers/i2c/busses/i2c-pasemi-platform.c
> +++ b/drivers/i2c/busses/i2c-pasemi-platform.c
> @@ -49,6 +49,7 @@ static int pasemi_platform_i2c_probe(struct platform_device *pdev)
>   	struct pasemi_smbus *smbus;
>   	u32 frequency;
>   	int error;
> +	int irq_num;
>   
>   	data = devm_kzalloc(dev, sizeof(struct pasemi_platform_i2c_data),
>   			    GFP_KERNEL);
> @@ -82,6 +83,11 @@ static int pasemi_platform_i2c_probe(struct platform_device *pdev)
>   	if (error)
>   		goto out_clk_disable;
>   
> +	irq_num = platform_get_irq(pdev, 0);
> +	error = devm_request_irq(smbus->dev, irq_num, pasemi_irq_handler, 0, "pasemi_apple_i2c", (void *)smbus);
> +
> +	if (!error)
> +		smbus->use_irq = 1;
>   	platform_set_drvdata(pdev, data);
>   
>   	return 0;

Tested-by: Christian Zigotzky <chzigotzky at xenosoft.de>

on an A-EON AmigaOne X1000 with a PASemi PWRficient PA6T-1682 processor.

Re: [PATCH v2] i2c-pasemi: PASemi I2C controller IRQ enablement

[Michael Ellerman]

"Sven Peter" <sven@svenpeter.dev> writes:
> On Sun, Oct 2, 2022, at 16:07, Arminder Singh wrote:
>> Hi,
>>
>>>  #define REG_MTXFIFO	0x00
>>>  #define REG_MRXFIFO	0x04
>>>  #define REG_SMSTA	0x14
>>> +#define REG_IMASK   0x18
>>
>>> This doesn't seem to be aligned correctly, this file seems to use a tab
>>> to separate the register name and the offset and you used spaces here.
>>
>>> @@ -15,7 +16,11 @@ struct pasemi_smbus {
>>>  	struct i2c_adapter	 adapter;
>>>  	void __iomem		*ioaddr;
>>>  	unsigned int		 clk_div;
>>> -	int			 hw_rev;
>>> +	int			         hw_rev;
>>> +	int                  use_irq;
>>> +	struct completion    irq_completion;
>>
>>> This doesn't seem to be aligned correctly and the hw_rev line
>>> doesn't have to be changed.
>>
>> I'm sorry for the alignment issues in the patch, I genuinely didn't notice
>> them as from the perspective of my primary editor (Visual Studio Code)
>> the entries were aligned. I just saw them when opening the files in
>> nano.
>
> No worries, it's just a small nit and quickly fixed after all! :)
>
>>
>> Does fixing the alignment issues and the commit description justify a v3
>> of the patch or should the minor fixes go out as a "resend"? Just not sure
>> in this particular case as the fixes seem to be very minor ones.
>
> I'd send a v3. I've only used resend when e.g. my previous mail provider
> messed up and silently converted all my outgoing mails to HTML.

If you've modified the patches then it's not a "resend":

  https://www.kernel.org/doc/html/latest/process/submitting-patches.html#don-t-get-discouraged-or-impatient

So yeah send a v3 in this case.

cheers

Re: [PATCH v2] i2c-pasemi: PASemi I2C controller IRQ enablement

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 348 bytes --]


> Does fixing the alignment issues and the commit description justify a v3
> of the patch or should the minor fixes go out as a "resend"? Just not sure
> in this particular case as the fixes seem to be very minor ones.

Yes, please send a v3. Since you are only fixing whitespace issues, you
can add the Tested-by tags given here for v3 already.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]