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

[Andy Shevchenko]

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?

> +/* 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
?

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

> +       __be32 data_be = cpu_to_be32(*data);

cpu_to_be32p()  IIUC?

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

> +
> +       /* 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?

> +}

-- 
With Best Regards,
Andy Shevchenko