[PATCH] i2c: aspeed: added support for slave mode

[Joel Stanley]

Hello Brendan,

On Tue, Aug 2, 2016 at 2:03 PM, Brendan Higgins
<brendanhiggins at google.com> wrote:
> Added slave mode irq handler as well as reg_slave and unreg_slave
> functions to the i2c-aspeed's i2c_algorithm.

Thanks for the patch! I don't have any hardware to test on that uses
the Aspeed SoC as a slave. Have you had a chance to give this a spin?

This driver needs some cleanup and then submission upstream. I would
like to see it submitted in the next month or so in order to land it
in Linux 4.9. Would you be willing to take on part or all of this
work?

I've made a few comments below.

> Signed-off-by: Brendan Higgins <brendanhiggins at google.com>
> ---
>  drivers/i2c/busses/i2c-aspeed.c | 198 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 195 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index b8e8199..dc1ec40 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c

> @@ -529,10 +544,105 @@ static void ast_i2c_master_xfer_done(struct ast_i2c_bus *bus)
>                 complete(&bus->cmd_complete);
>  }
>
> -static irqreturn_t ast_i2c_bus_irq(int irq, void *dev_id)
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static bool ast_i2c_slave_irq(struct ast_i2c_bus *bus)
>  {

The master takes bus->cmd_lock in ast_i2c_master_irq. Should we be
doing the same when handling the slave?

Also, the master acks all IRQs when they come in. Will we miss a slave
irq if it comes in just after a master irq, as we enter
ast_i2c_master_irq?

> -       struct ast_i2c_bus *bus = dev_id;
> +       u32 command;
> +       u32 irq_status;
> +       u32 status_ack = 0;
> +       u8 value;
> +       enum i2c_slave_event event;
> +       struct i2c_client *slave = bus->slave;
> +
> +       if (!slave)
> +               return false;
> +       command = ast_i2c_read(bus, I2C_CMD_REG);
> +       irq_status = ast_i2c_read(bus, I2C_INTR_STS_REG);
> +       /*
> +        * Slave was requested, restart state machine.
> +        */

> @@ -674,9 +802,69 @@ static u32 ast_i2c_functionality(struct i2c_adapter *adap)
>         return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA;
>  }
>
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static int ast_i2c_reg_slave(struct i2c_client *client)
> +{
> +       struct ast_i2c_bus *bus;
> +       u32 addr_reg_val;
> +       u32 func_ctrl_reg_val;
> +
> +       if (!client->slave_cb)
> +               return -EINVAL;

This function is a callback that the i2c core uses:

 int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb)
 {
         int ret;

         if (!client || !slave_cb) {
                 WARN(1, "insufficent data\n");
                 return -EINVAL;

As you can see it checks that the callback is valid, so we don't need
to. Unless there's something I'm missing here?

> +       bus = client->adapter->algo_data;
> +       if (bus->slave)
> +               return -EINVAL;
> +
> +       /*
> +        * Set slave addr.
> +        */

Nit: I prefer /* foo */ for single line comments.

> +       addr_reg_val = ast_i2c_read(bus, I2C_DEV_ADDR_REG);
> +       addr_reg_val &= ~AST_I2CD_DEV_ADDR_MASK;
> +       addr_reg_val |= client->addr & AST_I2CD_DEV_ADDR_MASK;
> +       ast_i2c_write(bus, addr_reg_val, I2C_DEV_ADDR_REG);
> +
> +       /*
> +        * Switch from master mode to slave mode.
> +        */
> +       func_ctrl_reg_val = ast_i2c_read(bus, I2C_FUN_CTRL_REG);
> +       func_ctrl_reg_val &= ~AST_I2CD_MASTER_EN;
> +       func_ctrl_reg_val |= AST_I2CD_SLAVE_EN;
> +       ast_i2c_write(bus, func_ctrl_reg_val, I2C_FUN_CTRL_REG);
> +
> +       bus->slave = client;
> +       bus->slave_state = AST_I2C_SLAVE_STOP;
> +       return 0;
> +}
> +

>  static int ast_i2c_probe_bus(struct platform_device *pdev)
> @@ -717,6 +905,10 @@ static int ast_i2c_probe_bus(struct platform_device *pdev)
>                 return -ENXIO;
>         }
>
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +       bus->slave = NULL;
> +#endif

We've just kzalloc'd bus, so it will already be NULL.

Cheers,

Joel