Date:   Wed, 30 May 2018 10:40:11 -0500
From:   Eddie James <eajames@...ux.vnet.ibm.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     linux-i2c <linux-i2c@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Wolfram Sang <wsa@...-dreams.de>,
        Rob Herring <robh+dt@...nel.org>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Joel Stanley <joel@....id.au>,
        Mark Rutland <mark.rutland@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Edward A. James" <eajames@...ibm.com>
Subject: Re: [PATCH v7 2/7] drivers/i2c: Add FSI-attached I2C master algorithm



On 05/29/2018 06:42 PM, Andy Shevchenko wrote:
> On Wed, May 30, 2018 at 1:24 AM, Eddie James <eajames@...ux.vnet.ibm.com> wrote:
>> From: "Edward A. James" <eajames@...ibm.com>
>>
>> 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@...ibm.com>
>> + *
>> + * 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

>
>> +}

Powered by blists - more mailing lists