Re: [PATCH v7 2/7] drivers/i2c: Add FSI-attached I2C master algorithm

[Eddie James]

On 05/29/2018 06:42 PM, Andy Shevchenko wrote:
> On Wed, May 30, 2018 at 1:24 AM, Eddie James <eajames@xxxxxxxxxxxxxxxxxx> wrote:
> > From: "Edward A. James" <eajames@xxxxxxxxxx>
> >
> > Add register definitions for FSI-attached I2C master and functions to
> > access those registers over FSI. Add an FSI driver so that our I2C bus
> > is probed up during an FSI scan.
> This looks like reinventing a wheel in some ways.
>
> See my comments below.
>
> > +/*
> > + * Copyright 2017 IBM Corporation
> > + *
> > + * Eddie James <eajames@xxxxxxxxxx>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + */
> We are using SPDX identifiers. Can you?

Sure.

> > +/* Find left shift from first set bit in m */
> > +#define MASK_TO_LSH(m)         (__builtin_ffsll(m) - 1ULL)
> Oh. What about GENMASK()?
>
> > +/* Extract field m from v */
> > +#define GETFIELD(m, v)         (((v) & (m)) >> MASK_TO_LSH(m))
> > +
> > +/* Set field m of v to val */
> > +#define SETFIELD(m, v, val)    \
> > +       (((v) & ~(m)) | ((((typeof(v))(val)) << MASK_TO_LSH(m)) & (m)))
> Oh, what about https://elixir.bootlin.com/linux/latest/source/include/linux/bitfield.h
> ?

Good idea, thanks.

> > +#define I2C_CMD_WITH_START     0x80000000
> > +#define I2C_CMD_WITH_ADDR      0x40000000
> > +#define I2C_CMD_RD_CONT                0x20000000
> > +#define I2C_CMD_WITH_STOP      0x10000000
> > +#define I2C_CMD_FORCELAUNCH    0x08000000
> BIT() ?
>
> > +#define I2C_CMD_ADDR           0x00fe0000
> > +#define I2C_CMD_READ           0x00010000
> GENMASK()? Though precisely here it might be good to leave explicit values.
>
> > +#define I2C_CMD_LEN            0x0000ffff
> > +#define I2C_MODE_CLKDIV                0xffff0000
> > +#define I2C_MODE_PORT          0x0000fc00
> > +#define I2C_MODE_ENHANCED      0x00000008
> > +#define I2C_MODE_DIAG          0x00000004
> > +#define I2C_MODE_PACE_ALLOW    0x00000002
> > +#define I2C_MODE_WRAP          0x00000001
> What are they? Masks? Bit fields? Just plain numbers?
>
> > +#define I2C_WATERMARK_HI       0x0000f000
> > +#define I2C_WATERMARK_LO       0x000000f0
> GENMASK() ?
>
> > +#define I2C_INT_INV_CMD                0x00008000
> > +#define I2C_INT_PARITY         0x00004000
> > +#define I2C_INT_BE_OVERRUN     0x00002000
> > +#define I2C_INT_BE_ACCESS      0x00001000
> > +#define I2C_INT_LOST_ARB       0x00000800
> > +#define I2C_INT_NACK           0x00000400
> > +#define I2C_INT_DAT_REQ                0x00000200
> > +#define I2C_INT_CMD_COMP       0x00000100
> > +#define I2C_INT_STOP_ERR       0x00000080
> > +#define I2C_INT_BUSY           0x00000040
> > +#define I2C_INT_IDLE           0x00000020
> BIT()
>
> > +#define I2C_INT_ENABLE         0x0000ff80
> > +#define I2C_INT_ERR            0x0000fcc0
> > +#define I2C_STAT_INV_CMD       0x80000000
> > +#define I2C_STAT_PARITY                0x40000000
> > +#define I2C_STAT_BE_OVERRUN    0x20000000
> > +#define I2C_STAT_BE_ACCESS     0x10000000
> > +#define I2C_STAT_LOST_ARB      0x08000000
> > +#define I2C_STAT_NACK          0x04000000
> > +#define I2C_STAT_DAT_REQ       0x02000000
> > +#define I2C_STAT_CMD_COMP      0x01000000
> > +#define I2C_STAT_STOP_ERR      0x00800000
> > +#define I2C_STAT_MAX_PORT      0x000f0000
> > +#define I2C_STAT_ANY_INT       0x00008000
> > +#define I2C_STAT_SCL_IN                0x00000800
> > +#define I2C_STAT_SDA_IN                0x00000400
> > +#define I2C_STAT_PORT_BUSY     0x00000200
> > +#define I2C_STAT_SELF_BUSY     0x00000100
> BIT()
>
> > +#define I2C_STAT_FIFO_COUNT    0x000000ff
> GENMASK()
>
> > +
> > +#define I2C_STAT_ERR           0xfc800000
> > +#define I2C_STAT_ANY_RESP      0xff800000
> > +#define I2C_ESTAT_FIFO_SZ      0xff000000
> GENMASK()
>
> > +#define I2C_ESTAT_SCL_IN_SY    0x00008000
> > +#define I2C_ESTAT_SDA_IN_SY    0x00004000
> > +#define I2C_ESTAT_S_SCL                0x00002000
> > +#define I2C_ESTAT_S_SDA                0x00001000
> > +#define I2C_ESTAT_M_SCL                0x00000800
> > +#define I2C_ESTAT_M_SDA                0x00000400
> > +#define I2C_ESTAT_HI_WATER     0x00000200
> > +#define I2C_ESTAT_LO_WATER     0x00000100
> > +#define I2C_ESTAT_PORT_BUSY    0x00000080
> > +#define I2C_ESTAT_SELF_BUSY    0x00000040
> BIT()
>
> > +#define I2C_ESTAT_VERSION      0x0000001f
> GENMASK()
>
> > +       __be32 data_be;
> No need to have a suffix. If anything can go wrong we have a tool,
> it's called sparse. It will catch out inappropriate use of __bitwise
> types.

I already have a variable called data...

> > +       __be32 data_be = cpu_to_be32(*data);
> cpu_to_be32p()  IIUC?

Sure.

> > +static int fsi_i2c_dev_init(struct fsi_i2c_master *i2c)
> > +{
> > +       int rc;
> > +       u32 mode = I2C_MODE_ENHANCED, extended_status, watermark = 0;
> > +       u32 interrupt = 0;
> Redundant assignment.

No, I need to set the interrupt register to 0, so I must set this.

> > +
> > +       /* since we use polling, disable interrupts */
> > +       rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_INT_MASK, &interrupt);
> > +       if (rc)
> > +               return rc;
> > +       return rc;
> Would be non-zero?

No, fsi_i2c_write_reg returns non-zero on error, zero on success. That 
is what I want.

Thanks,
Eddie

> > +}