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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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


+}




[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux