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
