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

[Brendan Higgins]

Hey Joel,

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

No problem. And yes, I have tried the patch on a AS2500 evaluation board.

> 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 would be totally willing to do the cleanup.

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

Fixed.

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

No, as the driver is written, slave mode and master mode are mutually exclusive
on the same bus so it would be impossible for this to happen. Even if both slave
and master mode were both enabled, each bus has its own registers and gets its
own IRQs. So the only time a master could steal a slave's IRQ would be if the
Aspeed's master and slave mode were enabled for the same bus and were both
active and in this case it would be talking to itself; I don't think there is
any way to make that work.

I added a comment to the effect of the first part (making master and slave mode
mutually exclusive).

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

Fixed.

> > +       bus = client->adapter->algo_data;
> > +       if (bus->slave)
> > +               return -EINVAL;
> > +
> > +       /*
> > +        * Set slave addr.
> > +        */
>
> Nit: I prefer /* foo */ for single line comments.

Fixed (in other places as well).

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

Fixed.

Cheers,

Brendan

P.S. My appologies if this comes out formatted weird on your end, I am not used
to git-send-email