Re: [PATCH] i2c: pasemi: Add IRQ support for Apple Silicon

[Christophe Leroy]

Le 20/08/2022 à 21:45, Arminder Singh a écrit :
> 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.

The above text should go after you signed-off-by, after the --- , so 
that it get's ignored by 'git am' when applying.

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

That as well shouldn't be part of the commit message.

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

Used only inside the if (smbus->use_irq), bitmask should be defined only 
in that scope.

> 
> -       status = reg_read(smbus, REG_SMSTA);

This should go in the else block.

> -
> -       while (!(status & SMSTA_XEN) && timeout--) {
> -               msleep(1);
> +       if (smbus->use_irq) {
> +               reinit_completion(&smbus->irq_completion);
> +               reg_write(smbus, REG_IMASK, bitmask);
> +               wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(10));
>                  status = reg_read(smbus, REG_SMSTA);
> +       } else {

status is uninitialised when entering the while loop.

> +               while (!(status & SMSTA_XEN) && timeout--) {
> +                       msleep(1);
> +                       status = reg_read(smbus, REG_SMSTA);
> +               }
>          }
> 
> +

Why a second blank line here ?

>          /* 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++) {

This shouldn't be part of this patch, it is unrelated.

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

dev_id is void*, the cast is not needed.

> +
> +       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;

Keep same alignment for member name.

>   };
> 
>   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);
> +       irq_num = platform_get_irq(pdev, 0);
> +       error = request_irq(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
>