Re: [PATCH] i2c: pasemi: Add IRQ support for Apple Silicon
From: Sven Peter
Date: Sun Aug 21 2022 - 05:57:45 EST
Hi,
Thanks for the patch! Some additional comments:
On Sat, Aug 20, 2022, at 21:45, Arminder Singh wrote:
> This is the first time I'm interacting with the Linux mailing lists, so
> please don't eviscerate me *too much* if I get the formatting wrong.
> Of course I'm always willing to take criticism and improve my formatting
> in the future.
>
> This patch adds support for IRQs to the PASemi I2C controller driver.
> This will allow for faster performing I2C transactions on Apple Silicon
> hardware, as previously, the driver was forced to poll the SMSTA register
> for a set amount of time.
>
> With this patchset the driver on Apple silicon hardware will instead wait
> for an interrupt which will signal the completion of the I2C transaction.
> The timeout value for this completion will be the same as the current
> amount of time the I2C driver polls for.
>
> This will result in some performance improvement since the driver will be
> waiting for less time than it does right now on Apple Silicon hardware.
>
> The patch right now will only enable IRQs for Apple Silicon I2C chips,
> and only if it's able to successfully request the IRQ from the kernel.
>
> === Testing ===
>
> This patch has been tested on both the mainline Linux kernel tree and
> the Asahi branch (https://github.com/AsahiLinux/linux.git) on both an
> M1 and M2 MacBook Air, and it compiles successfully as both a module and
> built-in to the kernel itself. The patch in both trees successfully boots
> to userspace without any hitch.
>
> I do not have PASemi hardware on hand unfortunately, so I'm unable to test
> the impact of this patch on old PASemi hardware. This is also why I've
> elected to do the IRQ request and enablement on the Apple platform driver
> and not in the common file, as I'm not sure if PASemi hardware supports
> IRQs.
>
> I also fixed a quick checkpatch warning on line 303. "i ++" is now "i++".
>
> Any and all critiques of the patch would be well appreciated.
>
>
>
>
> Signed-off-by: Arminder Singh <arminders208@xxxxxxxxxxx>
> ---
> drivers/i2c/busses/i2c-pasemi-core.c | 29 ++++++++++++++++++++----
> drivers/i2c/busses/i2c-pasemi-core.h | 5 ++++
> drivers/i2c/busses/i2c-pasemi-platform.c | 8 +++++++
> 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..375aa9528233 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
>
> @@ -80,14 +81,21 @@ static int pasemi_smb_waitready(struct pasemi_smbus *smbus)
> {
> int timeout = 10;
> unsigned int status;
> + unsigned int bitmask = SMSTA_XEN | SMSTA_MTN;
>
> - 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, bitmask);
s/bitmask/SMSTA_XEN | SMSTA_MTN/ and then you can just drop the bitmask
variable which isn't used anywhere else.
> + wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(10));
> status = reg_read(smbus, REG_SMSTA);
If the irq hasn't fired and wait_for_completion_timeout timed out the irq
is still enabled here. I'd put a reg_write(smbus, REG_IMASK, 0); here
to be safe.
> + } else {
You also need status = reg_read(smbus, REG_SMSTA); here.
> + while (!(status & SMSTA_XEN) && timeout--) {
> + msleep(1);
> + status = reg_read(smbus, REG_SMSTA);
> + }
> }
>
> +
> /* Got NACK? */
> if (status & SMSTA_MTN)
> return -ENXIO;
> @@ -300,7 +308,7 @@ static int pasemi_smb_xfer(struct i2c_adapter *adapter,
> case I2C_SMBUS_BLOCK_DATA:
> case I2C_SMBUS_BLOCK_PROC_CALL:
> data->block[0] = len;
> - for (i = 1; i <= len; i ++) {
> + for (i = 1; i <= len; i++) {
> rd = RXFIFO_RD(smbus);
> if (rd & MRXFIFO_EMPTY) {
> err = -ENODATA;
> @@ -348,6 +356,8 @@ int pasemi_i2c_common_probe(struct pasemi_smbus *smbus)
> 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 +366,12 @@ 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 = (struct pasemi_smbus *)dev_id;
> +
> + reg_write(smbus, REG_IMASK, 0);
> + 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..045e4a9a3d13 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
>
> @@ -16,6 +17,10 @@ struct pasemi_smbus {
> void __iomem *ioaddr;
> unsigned int clk_div;
> 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..ee1c84e7734b 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,13 @@ static int pasemi_platform_i2c_probe(struct
> platform_device *pdev)
> if (error)
> goto out_clk_disable;
>
> + smbus->use_irq = 0;
> + init_completion(&smbus->irq_completion);
I'd move this into the common probe function. If someone eventually wants
to add irq support to the PASemi boards it'll be required there as well.
> + irq_num = platform_get_irq(pdev, 0);
> + error = request_irq(irq_num, pasemi_irq_handler, 0,
> "pasemi_apple_i2c", (void *)smbus);
If you use request_irq here you'll have to add a remove function and
call free_irq there. I'd just use devm_request_irq which takes care of
that automatically.
> +
> + if (!error)
> + smbus->use_irq = 1;
> platform_set_drvdata(pdev, data);
>
> return 0;
> --
> 2.34.1
Best,
Sven