Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>]

On Mon, Dec 06, 2010 at 01:38:14AM -0500, Matt Turner wrote:
> From: Maciej W. Rozycki <macro-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
> 
> This is a rewrite of large parts of the driver mainly so that it uses
> SMBus interrupts to offload the CPU from busy-waiting on status inputs.
> As a part of the overhaul of the init and exit calls, all accesses to the
> hardware got converted to use accessory functions via an ioremap() cookie.
> 
> Minimally rebased by Matt Turner.
> 
> Tested-by: Matt Turner <mattst88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Matt Turner <mattst88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> This patch was originally sent in May 2008 [1], but appears to have been lost.
> 
> I believe this patch depends on
> [PATCH 1/3] RTC: SMBus support for the M41T80, etc. driver
> 
> Please review the change in return values (search for ENXIO). I wasn't entirely
> sure how this code should look. (The code the original patch was against just
> returned -1 on error. See 102b59c6d6d30fb6560177fd1ae8a34c4c163897). Please
> apply if acceptable.
> 
> [1] http://lists.lm-sensors.org/pipermail/i2c/2008-May/003638.html
> 
>  drivers/i2c/busses/i2c-sibyte.c |  278 +++++++++++++++++++++++++++++----------
>  1 files changed, 208 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sibyte.c b/drivers/i2c/busses/i2c-sibyte.c
> index 0fe505d..283747c 100644
> --- a/drivers/i2c/busses/i2c-sibyte.c
> +++ b/drivers/i2c/busses/i2c-sibyte.c
> @@ -2,6 +2,7 @@
>   * Copyright (C) 2004 Steven J. Hill
>   * Copyright (C) 2001,2002,2003 Broadcom Corporation
>   * Copyright (C) 1995-2000 Simon G. Vogl
> + * Copyright (C) 2008 Maciej W. Rozycki
>   *
>   * This program is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU General Public License
> @@ -18,104 +19,159 @@
>   * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
>   */
> 
> +#include <linux/errno.h>
> +#include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/i2c.h>
> +#include <linux/param.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +#include <linux/wait.h>
>  #include <linux/io.h>
> +#include <asm/sibyte/sb1250_int.h>
>  #include <asm/sibyte/sb1250_regs.h>
>  #include <asm/sibyte/sb1250_smbus.h>
> 
> 
>  struct i2c_algo_sibyte_data {
> -       void *data;             /* private data */
> -       int   bus;              /* which bus */
> -       void *reg_base;         /* CSR base */
> +       wait_queue_head_t       wait;           /* IRQ queue */
> +       void __iomem            *csr;           /* mapped CSR handle */
> +       phys_t                  base;           /* physical CSR base */
> +       char                    *name;          /* IRQ handler name */
> +       spinlock_t              lock;           /* atomiser */
> +       int                     irq;            /* IRQ line */
> +       int                     status;         /* IRQ status */
>  };
> 
> -/* ----- global defines ----------------------------------------------- */
> -#define SMB_CSR(a,r) ((long)(a->reg_base + r))
> 
> +static irqreturn_t i2c_sibyte_interrupt(int irq, void *dev_id)
> +{
> +       struct i2c_adapter *i2c_adap = dev_id;
> +       struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
> +       void __iomem *csr = adap->csr;
> +       u8 status;
> +
> +       /*
> +        * Ugh, no way to detect the finish interrupt,
> +        * but if busy it is obviously not one.
> +        */
> +       status = __raw_readq(csr + R_SMB_STATUS);
> +       if ((status & (M_SMB_ERROR | M_SMB_BUSY)) == M_SMB_BUSY)
> +               return IRQ_NONE;
> 
> -static int smbus_xfer(struct i2c_adapter *i2c_adap, u16 addr,
> -                     unsigned short flags, char read_write,
> -                     u8 command, int size, union i2c_smbus_data * data)
> +       /*
> +        * Clear the error interrupt (write 1 to clear);
> +        * the finish interrupt was cleared by the read above.
> +        */
> +       __raw_writeq(status, csr + R_SMB_STATUS);
> +
> +       /* Post the status. */
> +       spin_lock_irq(&adap->lock);
> +       adap->status = status & (M_SMB_ERROR_TYPE | M_SMB_ERROR | M_SMB_BUSY);
> +       wake_up(&adap->wait);
> +       spin_unlock_irq(&adap->lock);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static s32 i2c_sibyte_smbus_xfer(struct i2c_adapter *i2c_adap, u16 addr,
> +                                unsigned short cflags,
> +                                char read_write, u8 command, int size,
> +                                union i2c_smbus_data *data)
>  {
>         struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
> +       void __iomem *csr = adap->csr;
> +       unsigned long flags;
>         int data_bytes = 0;
>         int error;
> 
> -       while (csr_in32(SMB_CSR(adap, R_SMB_STATUS)) & M_SMB_BUSY)
> -               ;
> +       spin_lock_irqsave(&adap->lock, flags);
> +
> +       if (adap->status < 0) {
> +               error = -EIO;
> +               goto out_unlock;
> +       }

If a previous operation timed out, subsequent operations will fail forever. 
Is this a good idea ? Maybe it is - just asking.

> 
>         switch (size) {
>         case I2C_SMBUS_QUICK:
> -               csr_out32((V_SMB_ADDR(addr) |
> -                          (read_write == I2C_SMBUS_READ ? M_SMB_QDATA : 0) |
> -                          V_SMB_TT_QUICKCMD), SMB_CSR(adap, R_SMB_START));
> +               __raw_writeq(V_SMB_ADDR(addr) |
> +                            (read_write == I2C_SMBUS_READ ? M_SMB_QDATA : 0) |
> +                            V_SMB_TT_QUICKCMD,
> +                            csr + R_SMB_START);
>                 break;
>         case I2C_SMBUS_BYTE:
>                 if (read_write == I2C_SMBUS_READ) {
> -                       csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_RD1BYTE),
> -                                 SMB_CSR(adap, R_SMB_START));
> +                       __raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_RD1BYTE,
> +                                    csr + R_SMB_START);
>                         data_bytes = 1;
>                 } else {
> -                       csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
> -                       csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR1BYTE),
> -                                 SMB_CSR(adap, R_SMB_START));
> +                       __raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
> +                       __raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR1BYTE,
> +                                    csr + R_SMB_START);
>                 }
>                 break;
>         case I2C_SMBUS_BYTE_DATA:
> -               csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
> +               __raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
>                 if (read_write == I2C_SMBUS_READ) {
> -                       csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD1BYTE),
> -                                 SMB_CSR(adap, R_SMB_START));
> +                       __raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD1BYTE,
> +                                    csr + R_SMB_START);
>                         data_bytes = 1;
>                 } else {
> -                       csr_out32(V_SMB_LB(data->byte),
> -                                 SMB_CSR(adap, R_SMB_DATA));
> -                       csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE),
> -                                 SMB_CSR(adap, R_SMB_START));
> +                       __raw_writeq(V_SMB_LB(data->byte), csr + R_SMB_DATA);
> +                       __raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE,
> +                                    csr + R_SMB_START);
>                 }
>                 break;
>         case I2C_SMBUS_WORD_DATA:
> -               csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
> +               __raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
>                 if (read_write == I2C_SMBUS_READ) {
> -                       csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD2BYTE),
> -                                 SMB_CSR(adap, R_SMB_START));
> +                       __raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD2BYTE,
> +                                    csr + R_SMB_START);
>                         data_bytes = 2;
>                 } else {
> -                       csr_out32(V_SMB_LB(data->word & 0xff),
> -                                 SMB_CSR(adap, R_SMB_DATA));
> -                       csr_out32(V_SMB_MB(data->word >> 8),
> -                                 SMB_CSR(adap, R_SMB_DATA));
> -                       csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE),
> -                                 SMB_CSR(adap, R_SMB_START));
> +                       __raw_writeq(V_SMB_LB(data->word & 0xff),
> +                                    csr + R_SMB_DATA);
> +                       __raw_writeq(V_SMB_MB(data->word >> 8),
> +                                    csr + R_SMB_DATA);
> +                       __raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE,
> +                                    csr + R_SMB_START);
>                 }
>                 break;
>         default:
> -               return -EOPNOTSUPP;
> +               error = -EOPNOTSUPP;
> +               goto out_unlock;
>         }
> +       mmiowb();
> +       __raw_readq(csr + R_SMB_START);
> +       adap->status = -1;
> +
> +       spin_unlock_irqrestore(&adap->lock, flags);
> +
> +       wait_event_timeout(adap->wait, (adap->status >= 0), HZ);
> 
> -       while (csr_in32(SMB_CSR(adap, R_SMB_STATUS)) & M_SMB_BUSY)
> -               ;
> +       spin_lock_irqsave(&adap->lock, flags);
> 
> -       error = csr_in32(SMB_CSR(adap, R_SMB_STATUS));
> -       if (error & M_SMB_ERROR) {
> -               /* Clear error bit by writing a 1 */
> -               csr_out32(M_SMB_ERROR, SMB_CSR(adap, R_SMB_STATUS));
> -               return (error & M_SMB_ERROR_TYPE) ? -EIO : -ENXIO;
> +       if (adap->status < 0 || (adap->status & (M_SMB_ERROR | M_SMB_BUSY))) {
> +               error = -EIO;
> +               goto out_unlock;
>         }

The idea was to return -ENXIO if there was no ACK from a device (per Documentation/i2c/fault-codes).
With your change, this distinction gets lost. I think you should retain the original semantics,
ie return -ENXIO if status > 0 && ((status & (M_SMB_ERROR | M_SMB_ERROR_TYPE) == M_SMB_ERROR).

> 
>         if (data_bytes == 1)
> -               data->byte = csr_in32(SMB_CSR(adap, R_SMB_DATA)) & 0xff;
> +               data->byte = __raw_readq(csr + R_SMB_DATA) & 0xff;
>         if (data_bytes == 2)
> -               data->word = csr_in32(SMB_CSR(adap, R_SMB_DATA)) & 0xffff;
> +               data->word = __raw_readq(csr + R_SMB_DATA) & 0xffff;
> 
> -       return 0;
> +       error = 0;
> +
> +out_unlock:
> +       spin_unlock_irqrestore(&adap->lock, flags);
> +
> +       return error;
>  }
> 
> -static u32 bit_func(struct i2c_adapter *adap)
> +static u32 i2c_sibyte_bit_func(struct i2c_adapter *adap)
>  {
>         return (I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
>                 I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA);
> @@ -125,8 +181,8 @@ static u32 bit_func(struct i2c_adapter *adap)
>  /* -----exported algorithm data: ------------------------------------- */
> 
>  static const struct i2c_algorithm i2c_sibyte_algo = {
> -       .smbus_xfer     = smbus_xfer,
> -       .functionality  = bit_func,
> +       .smbus_xfer     = i2c_sibyte_smbus_xfer,
> +       .functionality  = i2c_sibyte_bit_func,
>  };
> 
>  /*
> @@ -135,37 +191,108 @@ static const struct i2c_algorithm i2c_sibyte_algo = {
>  static int __init i2c_sibyte_add_bus(struct i2c_adapter *i2c_adap, int speed)
>  {
>         struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
> +       void __iomem *csr;
> +       int err;
> 
> -       /* Register new adapter to i2c module... */
> -       i2c_adap->algo = &i2c_sibyte_algo;
> +       adap->status = 0;
> +       init_waitqueue_head(&adap->wait);
> +       spin_lock_init(&adap->lock);
> +
> +       csr = ioremap(adap->base, R_SMB_PEC + SMB_REGISTER_SPACING);
> +       if (!csr) {
> +               err = -ENOMEM;
> +               goto out;
> +       }
> +       adap->csr = csr;
> 
>         /* Set the requested frequency. */
> -       csr_out32(speed, SMB_CSR(adap,R_SMB_FREQ));
> -       csr_out32(0, SMB_CSR(adap,R_SMB_CONTROL));
> +       __raw_writeq(speed, csr + R_SMB_FREQ);
> +
> +       /* Clear any pending error interrupt. */
> +       __raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
> +       /* Disable interrupts. */
> +       __raw_writeq(0, csr + R_SMB_CONTROL);
> +       mmiowb();
> +       __raw_readq(csr + R_SMB_CONTROL);
> +
> +       err = request_irq(adap->irq, i2c_sibyte_interrupt, IRQF_SHARED,
> +                         adap->name, i2c_adap);
> +       if (err < 0)
> +               goto out_unmap;
> +
> +       /* Enable finish and error interrupts. */
> +       __raw_writeq(M_SMB_FINISH_INTR | M_SMB_ERR_INTR, csr + R_SMB_CONTROL);
> +
> +       /* Register new adapter to i2c module... */
> +       err = i2c_add_numbered_adapter(i2c_adap);
> +       if (err < 0)
> +               goto out_unirq;
> 
> -       return i2c_add_numbered_adapter(i2c_adap);
> +       return 0;
> +
> +out_unirq:
> +       /* Disable interrupts. */
> +       __raw_writeq(0, csr + R_SMB_CONTROL);
> +       mmiowb();
> +       __raw_readq(csr + R_SMB_CONTROL);
> +
> +       free_irq(adap->irq, i2c_adap);
> +
> +       /* Clear any pending error interrupt. */
> +       __raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
> +out_unmap:
> +       iounmap(csr);
> +out:
> +       return err;
>  }
> 
> +static void i2c_sibyte_remove_bus(struct i2c_adapter *i2c_adap)
> +{
> +       struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
> +       void __iomem *csr = adap->csr;
> +
> +       i2c_del_adapter(i2c_adap);
> +
> +       /* Disable interrupts. */
> +       __raw_writeq(0, csr + R_SMB_CONTROL);
> +       mmiowb();
> +       __raw_readq(csr + R_SMB_CONTROL);
> +
> +       free_irq(adap->irq, i2c_adap);
> +
> +       /* Clear any pending error interrupt. */
> +       __raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
> +
> +       iounmap(csr);
> +}
> 
> -static struct i2c_algo_sibyte_data sibyte_board_data[2] = {
> -       { NULL, 0, (void *) (CKSEG1+A_SMB_BASE(0)) },
> -       { NULL, 1, (void *) (CKSEG1+A_SMB_BASE(1)) }
> +static struct i2c_algo_sibyte_data i2c_sibyte_board_data[2] = {
> +       {
> +               .name   = "sb1250-smbus-0",
> +               .base   = A_SMB_0,
> +               .irq    = K_INT_SMB_0,
> +       },
> +       {
> +               .name   = "sb1250-smbus-1",
> +               .base   = A_SMB_1,
> +               .irq    = K_INT_SMB_1,
> +       }
>  };
> 
> -static struct i2c_adapter sibyte_board_adapter[2] = {
> +static struct i2c_adapter i2c_sibyte_board_adapter[2] = {
>         {
>                 .owner          = THIS_MODULE,
>                 .class          = I2C_CLASS_HWMON | I2C_CLASS_SPD,
> -               .algo           = NULL,
> -               .algo_data      = &sibyte_board_data[0],
> +               .algo           = &i2c_sibyte_algo,
> +               .algo_data      = &i2c_sibyte_board_data[0],
>                 .nr             = 0,
>                 .name           = "SiByte SMBus 0",
>         },
>         {
>                 .owner          = THIS_MODULE,
>                 .class          = I2C_CLASS_HWMON | I2C_CLASS_SPD,
> -               .algo           = NULL,
> -               .algo_data      = &sibyte_board_data[1],
> +               .algo           = &i2c_sibyte_algo,
> +               .algo_data      = &i2c_sibyte_board_data[1],
>                 .nr             = 1,
>                 .name           = "SiByte SMBus 1",
>         },
> @@ -173,21 +300,32 @@ static struct i2c_adapter sibyte_board_adapter[2] = {
> 
>  static int __init i2c_sibyte_init(void)
>  {
> +       int err;
> +
>         pr_info("i2c-sibyte: i2c SMBus adapter module for SiByte board\n");
> -       if (i2c_sibyte_add_bus(&sibyte_board_adapter[0], K_SMB_FREQ_100KHZ) < 0)
> -               return -ENODEV;
> -       if (i2c_sibyte_add_bus(&sibyte_board_adapter[1],
> -                              K_SMB_FREQ_400KHZ) < 0) {
> -               i2c_del_adapter(&sibyte_board_adapter[0]);
> -               return -ENODEV;
> -       }
> +
> +       err = i2c_sibyte_add_bus(&i2c_sibyte_board_adapter[0],
> +                                K_SMB_FREQ_100KHZ);
> +       if (err < 0)
> +               goto out;
> +
> +       err = i2c_sibyte_add_bus(&i2c_sibyte_board_adapter[1],
> +                                K_SMB_FREQ_400KHZ);
> +       if (err < 0)
> +               goto out_remove;
> +
>         return 0;
> +
> +out_remove:
> +       i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[0]);
> +out:
> +       return err;
>  }
> 
>  static void __exit i2c_sibyte_exit(void)
>  {
> -       i2c_del_adapter(&sibyte_board_adapter[0]);
> -       i2c_del_adapter(&sibyte_board_adapter[1]);
> +       i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[1]);
> +       i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[0]);
>  }
> 
>  module_init(i2c_sibyte_init);
> --
> 1.7.2.2
> 
>