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