* [PATCH v6 4/7] drivers/i2c: Add abort and hardware reset procedures
From: Eddie James @ 2017-11-16 19:53 UTC (permalink / raw)
To: linux-kernel
Cc: linux-i2c, devicetree, wsa, robh+dt, joel, eajames,
Edward A. James
In-Reply-To: <1510862032-12394-1-git-send-email-eajames@linux.vnet.ibm.com>
From: "Edward A. James" <eajames@us.ibm.com>
Add abort procedure for failed transfers. Add engine and bus reset
procedures to recover from as many faults as possible.
Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
drivers/i2c/busses/i2c-fsi.c | 186 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 186 insertions(+)
diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index 4ad5d68..61609bce 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -9,14 +9,18 @@
* 2 of the License, or (at your option) any later version.
*/
+#include <linux/delay.h>
#include <linux/device.h>
#include <linux/errno.h>
#include <linux/fsi.h>
#include <linux/i2c.h>
+#include <linux/jiffies.h>
#include <linux/kernel.h>
#include <linux/list.h>
#include <linux/module.h>
#include <linux/of.h>
+#include <linux/sched.h>
+#include <linux/spinlock.h>
#define FSI_ENGID_I2C 0x7
@@ -129,10 +133,18 @@
#define I2C_ESTAT_SELF_BUSY 0x00000040
#define I2C_ESTAT_VERSION 0x0000001f
+#define I2C_PORT_BUSY_RESET 0x80000000
+
+#define I2C_LOCAL_WAIT_TIMEOUT 2 /* jiffies */
+
+/* choose timeout length from legacy driver; it's well tested */
+#define I2C_ABORT_TIMEOUT msecs_to_jiffies(100)
+
struct fsi_i2c_master {
struct fsi_device *fsi;
u8 fifo_size;
struct list_head ports;
+ spinlock_t reset_lock;
};
struct fsi_i2c_port {
@@ -224,6 +236,179 @@ static int fsi_i2c_set_port(struct fsi_i2c_port *port)
return rc;
}
+static int fsi_i2c_reset_bus(struct fsi_i2c_master *i2c)
+{
+ int i, rc;
+ u32 mode, stat, ext, dummy = 0;
+
+ rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_MODE, &mode);
+ if (rc)
+ return rc;
+
+ mode |= I2C_MODE_DIAG;
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_MODE, &mode);
+ if (rc)
+ return rc;
+
+ for (i = 0; i < 9; ++i) {
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_SCL, &dummy);
+ if (rc)
+ return rc;
+
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_SET_SCL, &dummy);
+ if (rc)
+ return rc;
+ }
+
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_SCL, &dummy);
+ if (rc)
+ return rc;
+
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_SDA, &dummy);
+ if (rc)
+ return rc;
+
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_SET_SCL, &dummy);
+ if (rc)
+ return rc;
+
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_SET_SDA, &dummy);
+ if (rc)
+ return rc;
+
+ mode &= ~I2C_MODE_DIAG;
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_MODE, &mode);
+ if (rc)
+ return rc;
+
+ rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_STAT, &stat);
+ if (rc)
+ return rc;
+
+ /* check for hardware fault */
+ if (!(stat & I2C_STAT_SCL_IN) || !(stat & I2C_STAT_SDA_IN)) {
+ rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_ESTAT, &ext);
+ if (rc)
+ return rc;
+
+ dev_err(&i2c->fsi->dev, "bus stuck status[%08X] ext[%08X]\n",
+ stat, ext);
+ }
+
+ return 0;
+}
+
+static int fsi_i2c_reset(struct fsi_i2c_master *i2c, u16 port)
+{
+ int rc;
+ u32 mode, stat, dummy = 0;
+ unsigned long flags;
+
+ /* disable pre-emption; bus won't get left in a bad state for long */
+ spin_lock_irqsave(&i2c->reset_lock, flags);
+
+ /* reset engine */
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_I2C, &dummy);
+ if (rc)
+ goto done;
+
+ /* re-init engine */
+ rc = fsi_i2c_dev_init(i2c);
+ if (rc)
+ goto done;
+
+ rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_MODE, &mode);
+ if (rc)
+ goto done;
+
+ /* set port; default after reset is 0 */
+ if (port) {
+ mode = SETFIELD(I2C_MODE_PORT, mode, port);
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_MODE, &mode);
+ if (rc)
+ goto done;
+ }
+
+ /* reset busy register; hw workaround */
+ dummy = I2C_PORT_BUSY_RESET;
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_PORT_BUSY, &dummy);
+ if (rc)
+ goto done;
+
+ /* force bus reset */
+ rc = fsi_i2c_reset_bus(i2c);
+ if (rc)
+ goto done;
+
+ /* reset errors */
+ dummy = 0;
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_ERR, &dummy);
+ if (rc)
+ goto done;
+
+ /* wait for command complete; time from legacy driver */
+ udelay(1000);
+
+ rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_STAT, &stat);
+ if (rc)
+ goto done;
+
+ if (stat & I2C_STAT_CMD_COMP)
+ goto done;
+
+ /* failed to get command complete; reset engine again */
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_I2C, &dummy);
+ if (rc)
+ goto done;
+
+ /* re-init engine again */
+ rc = fsi_i2c_dev_init(i2c);
+
+done:
+ spin_unlock_irqrestore(&i2c->reset_lock, flags);
+ return rc;
+}
+
+static int fsi_i2c_abort(struct fsi_i2c_port *port, u32 status)
+{
+ int rc;
+ unsigned long start;
+ u32 cmd = I2C_CMD_WITH_STOP;
+ struct fsi_device *fsi = port->master->fsi;
+
+ rc = fsi_i2c_reset(port->master, port->port);
+ if (rc)
+ return rc;
+
+ /* skip final stop command for these errors */
+ if (status & (I2C_STAT_PARITY | I2C_STAT_LOST_ARB | I2C_STAT_STOP_ERR))
+ return 0;
+
+ /* write stop command */
+ rc = fsi_i2c_write_reg(fsi, I2C_FSI_CMD, &cmd);
+ if (rc)
+ return rc;
+
+ /* wait until we see command complete in the master */
+ start = jiffies;
+
+ do {
+ rc = fsi_i2c_read_reg(fsi, I2C_FSI_STAT, &status);
+ if (rc)
+ return rc;
+
+ if (status & I2C_STAT_CMD_COMP)
+ return 0;
+
+ set_current_state(TASK_INTERRUPTIBLE);
+ if (schedule_timeout(I2C_LOCAL_WAIT_TIMEOUT) > 0)
+ return -EINTR;
+
+ } while (time_after(start + I2C_ABORT_TIMEOUT, jiffies));
+
+ return -ETIME;
+}
+
static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
int num)
{
@@ -260,6 +445,7 @@ static int fsi_i2c_probe(struct device *dev)
if (!i2c)
return -ENOMEM;
+ spin_lock_init(&i2c->reset_lock);
i2c->fsi = to_fsi_dev(dev);
INIT_LIST_HEAD(&i2c->ports);
--
1.8.3.1
^ permalink raw reply related
* [PATCH v6 3/7] drivers/i2c: Add port structure to FSI algorithm
From: Eddie James @ 2017-11-16 19:53 UTC (permalink / raw)
To: linux-kernel
Cc: linux-i2c, devicetree, wsa, robh+dt, joel, eajames,
Edward A. James
In-Reply-To: <1510862032-12394-1-git-send-email-eajames@linux.vnet.ibm.com>
From: "Edward A. James" <eajames@us.ibm.com>
Add and initialize I2C adapters for each port on the FSI-attached I2C
master. Ports for each master are defined in the devicetree.
Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
drivers/i2c/busses/i2c-fsi.c | 96 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 96 insertions(+)
diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index 79475f8..4ad5d68 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -14,7 +14,9 @@
#include <linux/fsi.h>
#include <linux/i2c.h>
#include <linux/kernel.h>
+#include <linux/list.h>
#include <linux/module.h>
+#include <linux/of.h>
#define FSI_ENGID_I2C 0x7
@@ -130,6 +132,14 @@
struct fsi_i2c_master {
struct fsi_device *fsi;
u8 fifo_size;
+ struct list_head ports;
+};
+
+struct fsi_i2c_port {
+ struct list_head list;
+ struct i2c_adapter adapter;
+ struct fsi_i2c_master *master;
+ u16 port;
};
static int fsi_i2c_read_reg(struct fsi_device *fsi, unsigned int reg,
@@ -186,9 +196,44 @@ static int fsi_i2c_dev_init(struct fsi_i2c_master *i2c)
return rc;
}
+static int fsi_i2c_set_port(struct fsi_i2c_port *port)
+{
+ int rc;
+ struct fsi_device *fsi = port->master->fsi;
+ u32 mode, dummy = 0;
+ u16 old_port;
+
+ rc = fsi_i2c_read_reg(fsi, I2C_FSI_MODE, &mode);
+ if (rc)
+ return rc;
+
+ old_port = GETFIELD(I2C_MODE_PORT, mode);
+
+ if (old_port != port->port) {
+ mode = SETFIELD(I2C_MODE_PORT, mode, port->port);
+ rc = fsi_i2c_write_reg(fsi, I2C_FSI_MODE, &mode);
+ if (rc)
+ return rc;
+
+ /* reset engine when port is changed */
+ rc = fsi_i2c_write_reg(fsi, I2C_FSI_RESET_ERR, &dummy);
+ if (rc)
+ return rc;
+ }
+
+ return rc;
+}
+
static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
int num)
{
+ int rc;
+ struct fsi_i2c_port *port = adap->algo_data;
+
+ rc = fsi_i2c_set_port(port);
+ if (rc)
+ return rc;
+
return -EOPNOTSUPP;
}
@@ -206,23 +251,73 @@ static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
static int fsi_i2c_probe(struct device *dev)
{
struct fsi_i2c_master *i2c;
+ struct fsi_i2c_port *port;
+ struct device_node *np;
int rc;
+ u32 port_no;
i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
if (!i2c)
return -ENOMEM;
i2c->fsi = to_fsi_dev(dev);
+ INIT_LIST_HEAD(&i2c->ports);
rc = fsi_i2c_dev_init(i2c);
if (rc)
return rc;
+ /* Add adapter for each i2c port of the master. */
+ for_each_available_child_of_node(dev->of_node, np) {
+ rc = of_property_read_u32(np, "reg", &port_no);
+ if (rc || port_no > USHRT_MAX)
+ continue;
+
+ port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
+ if (!port)
+ break;
+
+ port->master = i2c;
+ port->port = port_no;
+
+ port->adapter.owner = THIS_MODULE;
+ port->adapter.dev.of_node = np;
+ port->adapter.dev.parent = dev;
+ port->adapter.algo = &fsi_i2c_algorithm;
+ port->adapter.algo_data = port;
+
+ snprintf(port->adapter.name, sizeof(port->adapter.name),
+ "i2c_bus-%u", port_no);
+
+ rc = i2c_add_adapter(&port->adapter);
+ if (rc < 0) {
+ dev_err(dev, "Failed to register adapter: %d\n", rc);
+ devm_kfree(dev, port);
+ continue;
+ }
+
+ list_add(&port->list, &i2c->ports);
+ }
+
dev_set_drvdata(dev, i2c);
return 0;
}
+static int fsi_i2c_remove(struct device *dev)
+{
+ struct fsi_i2c_master *i2c = dev_get_drvdata(dev);
+ struct fsi_i2c_port *port;
+
+ if (!list_empty(&i2c->ports)) {
+ list_for_each_entry(port, &i2c->ports, list) {
+ i2c_del_adapter(&port->adapter);
+ }
+ }
+
+ return 0;
+}
+
static const struct fsi_device_id fsi_i2c_ids[] = {
{ FSI_ENGID_I2C, FSI_VERSION_ANY },
{ 0 }
@@ -234,6 +329,7 @@ static int fsi_i2c_probe(struct device *dev)
.name = "i2c-fsi",
.bus = &fsi_bus_type,
.probe = fsi_i2c_probe,
+ .remove = fsi_i2c_remove,
},
};
--
1.8.3.1
^ permalink raw reply related
* [PATCH v6 2/7] drivers/i2c: Add FSI-attached I2C master algorithm
From: Eddie James @ 2017-11-16 19:53 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, joel-U3u1mxZcP9KHXe+LvDLADg,
eajames-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8, Edward A. James
In-Reply-To: <1510862032-12394-1-git-send-email-eajames-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
From: "Edward A. James" <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
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.
Signed-off-by: Edward A. James <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
drivers/i2c/busses/Kconfig | 11 ++
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-fsi.c | 244 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 256 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-fsi.c
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 009345d..4db3f44 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1331,4 +1331,15 @@ config I2C_ZX2967
This driver can also be built as a module. If so, the module will be
called i2c-zx2967.
+config I2C_FSI
+ tristate "FSI I2C driver"
+ depends on FSI
+ help
+ Driver for FSI bus attached I2C masters. These are I2C masters that
+ are connected to the system over a FSI bus, instead of the more
+ common PCI or MMIO interface.
+
+ This driver can also be built as a module. If so, the module will be
+ called as i2c-fsi.
+
endmenu
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 2ce8576..f0ddead 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -136,5 +136,6 @@ obj-$(CONFIG_I2C_PCA_ISA) += i2c-pca-isa.o
obj-$(CONFIG_I2C_SIBYTE) += i2c-sibyte.o
obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o
obj-$(CONFIG_SCx200_ACB) += scx200_acb.o
+obj-$(CONFIG_I2C_FSI) += i2c-fsi.o
ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
new file mode 100644
index 0000000..79475f8
--- /dev/null
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -0,0 +1,244 @@
+/*
+ * Copyright 2017 IBM Corporation
+ *
+ * Eddie James <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
+ *
+ * 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.
+ */
+
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/fsi.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#define FSI_ENGID_I2C 0x7
+
+/* Find left shift from first set bit in m */
+#define MASK_TO_LSH(m) (__builtin_ffsll(m) - 1ULL)
+
+/* 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)))
+
+#define I2C_DEFAULT_CLK_DIV 6
+
+/* i2c registers */
+#define I2C_FSI_FIFO 0x00
+#define I2C_FSI_CMD 0x04
+#define I2C_FSI_MODE 0x08
+#define I2C_FSI_WATER_MARK 0x0C
+#define I2C_FSI_INT_MASK 0x10
+#define I2C_FSI_INT_COND 0x14
+#define I2C_FSI_OR_INT_MASK 0x14
+#define I2C_FSI_INTS 0x18
+#define I2C_FSI_AND_INT_MASK 0x18
+#define I2C_FSI_STAT 0x1C
+#define I2C_FSI_RESET_I2C 0x1C
+#define I2C_FSI_ESTAT 0x20
+#define I2C_FSI_RESET_ERR 0x20
+#define I2C_FSI_RESID_LEN 0x24
+#define I2C_FSI_SET_SCL 0x24
+#define I2C_FSI_PORT_BUSY 0x28
+#define I2C_FSI_RESET_SCL 0x2C
+#define I2C_FSI_SET_SDA 0x30
+#define I2C_FSI_RESET_SDA 0x34
+
+/* cmd register */
+#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
+#define I2C_CMD_ADDR 0x00fe0000
+#define I2C_CMD_READ 0x00010000
+#define I2C_CMD_LEN 0x0000ffff
+
+/* mode register */
+#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
+
+/* watermark register */
+#define I2C_WATERMARK_HI 0x0000f000
+#define I2C_WATERMARK_LO 0x000000f0
+
+#define I2C_FIFO_HI_LVL 4
+#define I2C_FIFO_LO_LVL 4
+
+/* interrupt register */
+#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
+
+#define I2C_INT_ENABLE 0x0000ff80
+#define I2C_INT_ERR 0x0000fcc0
+
+/* status register */
+#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
+#define I2C_STAT_FIFO_COUNT 0x000000ff
+
+#define I2C_STAT_ERR 0xfc800000
+#define I2C_STAT_ANY_RESP 0xff800000
+
+/* extended status register */
+#define I2C_ESTAT_FIFO_SZ 0xff000000
+#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
+#define I2C_ESTAT_VERSION 0x0000001f
+
+struct fsi_i2c_master {
+ struct fsi_device *fsi;
+ u8 fifo_size;
+};
+
+static int fsi_i2c_read_reg(struct fsi_device *fsi, unsigned int reg,
+ u32 *data)
+{
+ int rc;
+ __be32 data_be;
+
+ rc = fsi_device_read(fsi, reg, &data_be, sizeof(data_be));
+ if (rc)
+ return rc;
+
+ *data = be32_to_cpu(data_be);
+
+ return 0;
+}
+
+static int fsi_i2c_write_reg(struct fsi_device *fsi, unsigned int reg,
+ u32 *data)
+{
+ __be32 data_be = cpu_to_be32(*data);
+
+ return fsi_device_write(fsi, reg, &data_be, sizeof(data_be));
+}
+
+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;
+
+ /* since we use polling, disable interrupts */
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_INT_MASK, &interrupt);
+ if (rc)
+ return rc;
+
+ mode = SETFIELD(I2C_MODE_CLKDIV, mode, I2C_DEFAULT_CLK_DIV);
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_MODE, &mode);
+ if (rc)
+ return rc;
+
+ rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_ESTAT, &extended_status);
+ if (rc)
+ return rc;
+
+ i2c->fifo_size = GETFIELD(I2C_ESTAT_FIFO_SZ, extended_status);
+ watermark = SETFIELD(I2C_WATERMARK_HI, watermark,
+ i2c->fifo_size - I2C_FIFO_HI_LVL);
+ watermark = SETFIELD(I2C_WATERMARK_LO, watermark,
+ I2C_FIFO_LO_LVL);
+
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_WATER_MARK, &watermark);
+
+ return rc;
+}
+
+static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
+ int num)
+{
+ return -EOPNOTSUPP;
+}
+
+static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_PROTOCOL_MANGLING | I2C_FUNC_10BIT_ADDR
+ | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA;
+}
+
+static const struct i2c_algorithm fsi_i2c_algorithm = {
+ .master_xfer = fsi_i2c_xfer,
+ .functionality = fsi_i2c_functionality,
+};
+
+static int fsi_i2c_probe(struct device *dev)
+{
+ struct fsi_i2c_master *i2c;
+ int rc;
+
+ i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
+ if (!i2c)
+ return -ENOMEM;
+
+ i2c->fsi = to_fsi_dev(dev);
+
+ rc = fsi_i2c_dev_init(i2c);
+ if (rc)
+ return rc;
+
+ dev_set_drvdata(dev, i2c);
+
+ return 0;
+}
+
+static const struct fsi_device_id fsi_i2c_ids[] = {
+ { FSI_ENGID_I2C, FSI_VERSION_ANY },
+ { 0 }
+};
+
+static struct fsi_driver fsi_i2c_driver = {
+ .id_table = fsi_i2c_ids,
+ .drv = {
+ .name = "i2c-fsi",
+ .bus = &fsi_bus_type,
+ .probe = fsi_i2c_probe,
+ },
+};
+
+module_fsi_driver(fsi_i2c_driver);
+
+MODULE_AUTHOR("Eddie James <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>");
+MODULE_DESCRIPTION("FSI attached I2C master");
+MODULE_LICENSE("GPL");
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v6 1/7] dt-bindings: i2c: Add FSI-attached I2C master dt binding documentation
From: Eddie James @ 2017-11-16 19:53 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, joel-U3u1mxZcP9KHXe+LvDLADg,
eajames-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8, Edward A. James
In-Reply-To: <1510862032-12394-1-git-send-email-eajames-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
From: "Edward A. James" <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Document the bindings.
Signed-off-by: Edward A. James <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
Documentation/devicetree/bindings/i2c/i2c-fsi.txt | 40 +++++++++++++++++++++++
1 file changed, 40 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-fsi.txt
diff --git a/Documentation/devicetree/bindings/i2c/i2c-fsi.txt b/Documentation/devicetree/bindings/i2c/i2c-fsi.txt
new file mode 100644
index 0000000..b1be2ce
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-fsi.txt
@@ -0,0 +1,40 @@
+Device-tree bindings for FSI-attached I2C master and busses
+-----------------------------------------------------------
+
+Required properties:
+ - compatible = "ibm,i2c-fsi";
+ - reg = < address size >; : The FSI CFAM address and address
+ space size.
+ - #address-cells = <1>; : Number of address cells in child
+ nodes.
+ - #size-cells = <0>; : Number of size cells in child nodes.
+ - child nodes : Nodes to describe busses off the I2C
+ master.
+
+Child node required properties:
+ - reg = < port number > : The port number on the I2C master.
+
+Child node optional properties:
+ - child nodes : Nodes to describe devices on the I2C
+ bus.
+
+Examples:
+
+ i2c@1800 {
+ compatible = "ibm,i2c-fsi";
+ reg = < 0x1800 0x400 >;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ i2c-bus@0 {
+ reg = <0>;
+ };
+
+ i2c-bus@1 {
+ reg = <1>;
+
+ eeprom@50 {
+ compatible = "vendor,dev-name";
+ };
+ };
+ };
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v6 0/7] drivers/i2c: Add FSI-attached I2C master algorithm
From: Eddie James @ 2017-11-16 19:53 UTC (permalink / raw)
To: linux-kernel
Cc: linux-i2c, devicetree, wsa, robh+dt, joel, eajames,
Edward A. James
From: "Edward A. James" <eajames@us.ibm.com>
This series adds an algorithm for an I2C master physically located on an FSI
slave device. The I2C master has multiple ports, each of which may be connected
to an I2C slave. Access to the I2C master registers is achieved over FSI bus.
Due to the multi-port nature of the I2C master, the driver instantiates a new
I2C adapter for each port connected to a slave. The connected ports should be
defined in the device tree under the I2C master device.
Changes since v5
- Fix reset functionality and do a reset after every transfer failure
Edward A. James (7):
dt-bindings: i2c: Add FSI-attached I2C master dt binding documentation
drivers/i2c: Add FSI-attached I2C master algorithm
drivers/i2c: Add port structure to FSI algorithm
drivers/i2c: Add abort and hardware reset procedures
drivers/i2c: Add transfer implementation for FSI algorithm
drivers/i2c: Add I2C master locking to FSI algorithm
drivers/i2c: Add bus recovery for FSI algorithm
Documentation/devicetree/bindings/i2c/i2c-fsi.txt | 40 ++
drivers/i2c/busses/Kconfig | 11 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-fsi.c | 793 ++++++++++++++++++++++
4 files changed, 845 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-fsi.txt
create mode 100644 drivers/i2c/busses/i2c-fsi.c
--
1.8.3.1
^ permalink raw reply
* Re: [PATCH v2 1/4] omapdrm: fix compatible string for td028ttec1
From: H. Nikolaus Schaller @ 2017-11-16 18:56 UTC (permalink / raw)
To: Andrew F. Davis
Cc: Mark Rutland, devicetree, Julia Lawall, letux-kernel,
Bartlomiej Zolnierkiewicz, David Airlie, dri-devel, Russell King,
Rob Herring, linux-kernel, Tony Lindgren, linux-fbdev,
Tomi Valkeinen, Thierry Reding, Laurent Pinchart,
Benoît Cousson, kernel, Sean Paul, linux-omap,
linux-arm-kernel
In-Reply-To: <2e67402b-4a32-9da9-3b06-e271c23d2422@ti.com>
Hi Andrew,
> Am 16.11.2017 um 19:32 schrieb Andrew F. Davis <afd@ti.com>:
>
> On 11/16/2017 12:18 PM, H. Nikolaus Schaller wrote:
>>
>>> Am 16.11.2017 um 18:08 schrieb Andrew F. Davis <afd@ti.com>:
>>>
>>> On 11/16/2017 10:10 AM, H. Nikolaus Schaller wrote:
>>>> Hi Andrew,
>>>>
>>>>> Am 16.11.2017 um 16:53 schrieb Andrew F. Davis <afd@ti.com>:
>>>>>
>>>>> On 11/16/2017 07:43 AM, H. Nikolaus Schaller wrote:
>>>>>>
>>>>>>> Am 16.11.2017 um 13:32 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:
>>>>>>>
>>>>>>> On 16/11/17 10:50, H. Nikolaus Schaller wrote:
>>>>>>>> The vendor name was "toppoly" but other panels and the vendor list
>>>>>>>> have defined it as "tpo". So let's fix it in driver and bindings.
>>>>>>>>
>>>>>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>>>>>> ---
>>>>>>>
>>>>>>>
>>>>>>>> -MODULE_ALIAS("spi:toppoly,td028ttec1");
>>>>>>>> +MODULE_ALIAS("spi:tpo,td028ttec1");
>>>>>>>
>>>>>>> Doesn't this mean that the module won't load if you have old bindings?
>>>>>>
>>>>>> Hm.
>>>>>>
>>>>>> Well, I think it can load but doesn't automatically from DT strings which might
>>>>>> be unexpected.
>>>>>>
>>>>>>> Can't we have two module aliases?
>>>>>>
>>>>>> I think we can. Just a random example:
>>>>>> https://elixir.free-electrons.com/linux/latest/source/drivers/w1/slaves/w1_therm.c#L754
>>>>>>
>>>>>> So we should keep both.
>>>>>
>>>>> Even better would be to drop both MODULE_ALIAS and let the
>>>>> MODULE_DEVICE_TABLE macro define them for your from the SPI id table.
>>>>
>>>> Why would that be better?
>>>>
>>>
>>> MODULE_ALIAS is ugly, you already have a table (usually) of device names
>>> that are supported by the driver, the module aliases should be generated
>>> from that table. This also keeps supported device list in one place.
>>>
>>>> As far as I see it will need more code and changes than adding one line of
>>>> MODULE_ALIAS.
>>>>
>>>>> Although it doesn't look like this driver has an SPI id table, you
>>>>> should probably add one, I be interested to see if this module is always
>>>>> being matched through the "spi" or the "of" alias..
>>>>
>>>> Could you please propose how that code should look like, so that I can test?
>>>>
>>>
>>> Sure,
>>>
>>> start with
>>> $ udevadm monitor
>>> and see what string the kernel is looking for when trying to find a
>>> module for this device.
>>
>> Well, the module is loaded automatically from DT at boot time well before
>> I can start udevadm. So that is the most tricky part to setup the system
>> to suppress this...
>>
>>>
>>> If it is only ever looking for "of:toppoly,td028ttec1", then you can
>>> drop the MODULE_ALIAS and be done as it was never getting used anyway.
>>
>> Since it is an SPI client, I am sure it looks for "spi:something.
>>
>>>
>>> What I expect though is "spi:toppoly,td028ttec1", in which case you
>>> should add
>>>
>>> static const struct spi_device_id td028ttec1_ids[] = {
>>> { "toppoly,td028ttec1", 0 },
>>> { "tpo,td028ttec1", 0},
>>> { /* sentinel */ }
>>> };
>>> MODULE_DEVICE_TABLE(spi, td028ttec1_ids);
>>
>> We already have a static const struct of_device_id td028ttec1_of_match[]
>> table with the same information.
>>
>> So we still have two places to keep in sync.
>>
>> Or can we remove the td028ttec1_of_match[]? AFAIK not.
>>
>>>
>>> link to it in the td028ttec1_spi_driver struct:
>>> .id_table = td028ttec1_ids,
>>>
>>> Then test again to see that the module still loads with the new and old
>>> DT string.
>>
>> In total I am not really convinced that adding 7 lines of code is better
>> than one (the "tpo," alias) that is tested and works...
>>
>> And it looks like a lot of unplanned code testing for me which takes more
>> than 5 minutes :)
>>
>> So I'd prefer to leave that exercise of fixing the MODULE_ALIAS/DEVICE_TABLE
>> to someone else...
>>
>
> That's fine, someday I'll probably get some script to do this for all
> the drivers that still have MODULE_ALIAS and an existing table.
That would be cool!
On a second thought, I think there is a quick experiment for this driver
not needing to monitor events.
1st attempt: remove ALIASES => if it still loads it would be fine
2nd attempt: add your id table => if it loads again, it is fine
if not, let's keep ALIASES.
Maybe I can try tomorrow.
BR and thanks,
Nikolaus
>
>> BR and thanks,
>> Nikolaus
>>
>>>
>>> Andrew
>>>
>>>> BR and thanks,
>>>> Nikolaus Schaller
>>>>
>>>>>
>>>>>>
>>>>>> Should I submit a new version?
>>>>>>
>>>>>> BR,
>>>>>> Nikolaus
>>>>>>
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
^ permalink raw reply
* Re: [PATCH 1/2] of: unittest: let dtc generate __local_fixups__
From: Rob Herring @ 2017-11-16 18:42 UTC (permalink / raw)
To: Frank Rowand
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <fb992198-d55c-0d06-3f19-0444d0c292d3-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Thu, Nov 16, 2017 at 12:34 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 11/16/17 13:05, Rob Herring wrote:
>> Remove the manually added __local_fixups__ because dtc can now generate
>> them. This also fixes a new warning in the process:
>>
>> drivers/of/unittest-data/testcases.dtb: Warning (interrupts_extended_property): Could not get phandle node for /__local_fixups__/testcase-data/interrupts/interrupts-extended0:interrupts-extended(cell 3)
>>
>> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> ---
>> drivers/of/unittest-data/testcases.dts | 56 ++--------------------------------
>> 1 file changed, 2 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/of/unittest-data/testcases.dts b/drivers/of/unittest-data/testcases.dts
>> index 12f7c3d649c8..ac616ad18ea6 100644
>> --- a/drivers/of/unittest-data/testcases.dts
>> +++ b/drivers/of/unittest-data/testcases.dts
>> @@ -1,4 +1,6 @@
>> /dts-v1/;
>> +/plugin/;
>> +
>> / {
>> testcase-data {
>> changeset {
>> @@ -23,57 +25,3 @@
>> * The format of this data may be subject to change. For the time being consider
>> * this a kernel-internal data format.
>> */
>> -/ { __local_fixups__ {
>> - testcase-data {
>> - phandle-tests {
>> - consumer-a {
>
> < snip >
>
> You should also delete the comment immediately above that describes the removed lines.
Ah yes, I meant to do that.
Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/2] of: unittest: let dtc generate __local_fixups__
From: Frank Rowand @ 2017-11-16 18:34 UTC (permalink / raw)
To: Rob Herring, devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20171116180545.12322-1-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
On 11/16/17 13:05, Rob Herring wrote:
> Remove the manually added __local_fixups__ because dtc can now generate
> them. This also fixes a new warning in the process:
>
> drivers/of/unittest-data/testcases.dtb: Warning (interrupts_extended_property): Could not get phandle node for /__local_fixups__/testcase-data/interrupts/interrupts-extended0:interrupts-extended(cell 3)
>
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
> drivers/of/unittest-data/testcases.dts | 56 ++--------------------------------
> 1 file changed, 2 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/of/unittest-data/testcases.dts b/drivers/of/unittest-data/testcases.dts
> index 12f7c3d649c8..ac616ad18ea6 100644
> --- a/drivers/of/unittest-data/testcases.dts
> +++ b/drivers/of/unittest-data/testcases.dts
> @@ -1,4 +1,6 @@
> /dts-v1/;
> +/plugin/;
> +
> / {
> testcase-data {
> changeset {
> @@ -23,57 +25,3 @@
> * The format of this data may be subject to change. For the time being consider
> * this a kernel-internal data format.
> */
> -/ { __local_fixups__ {
> - testcase-data {
> - phandle-tests {
> - consumer-a {
< snip >
You should also delete the comment immediately above that describes the removed lines.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 0/8] USB: add device-tree support for interfaces
From: Rob Herring @ 2017-11-16 18:33 UTC (permalink / raw)
To: Johan Hovold
Cc: Greg Kroah-Hartman, Mark Rutland, Arnd Bergmann, Alan Stern,
Peter Chen, Linux USB List,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Rafał Miłecki, Florian Fainelli
In-Reply-To: <20171116161249.GJ11226@localhost>
On Thu, Nov 16, 2017 at 10:12 AM, Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Nov 16, 2017 at 08:43:21AM -0600, Rob Herring wrote:
>> On Thu, Nov 09, 2017 at 06:07:15PM +0100, Johan Hovold wrote:
>> > This series adds support for representing USB interfaces in device tree
>> > by implementing support for "interface nodes" and "combined nodes" from
>> > the OF specification.
>> >
>> > This is needed to be able to describe non-discoverable properties of
>> > permanently attached USB devices and their interfaces such as any
>> > i2c-clients connected to a USB-i2c bridge (e.g. the dln2 mfd) or,
>> > eventually, devices connected to usb-serial converters (to be used with
>> > serdev).
>>
>> In the original OF binding, the firmware dynamically generated the tree
>> for the active configuration AIUI. That doesn't really fit for the
>> (mostly) static FDT usage and why we stopped at the device level. So how
>> do we handle multiple configs? Or can we assume that if say the I2C bus
>> is used, then there's only one config and interface that can use it?
>
> Multiple configuration can be used to implement different sets of
> functionality. A hypothetical device could have one i2c controller in
> one configuration and two in another. Most devices will only have one
> configuration though.
Right, but ultimately the device has the physical interface (pins) and
we could have multiple USB interfaces pointing to the single physical
interface. How do we represent that without duplicating the DT data in
both interfaces? I guess we can do a phandle to the I2C bus as we do
sometimes.
> A USB interface implements some functionality of the device (and this is
> what Linux USB drivers bind to). So even for single-configuration
> devices, you need to be able to say which i2c controller (bus) you are
> describing.
Are you saying the mapping of USB interface to physical interface
cannot be implied by the specific device? Say, we had something like
this:
usb-device {
i2c-bus@0 {};
i2c-bus@1 {};
};
Where 0 and 1 are based on the pin out of the device. There's no way
for the driver to figure out the mapping of USB interfaces to i2c bus?
How does the user writing the DT do it?
> [ And as a simplification, the combined nodes can be used for most cases
> were we only have one configuration with a single interface. ]
>
> Note that a new set of interfaces (in the kernel device model) is
> created when a new USB device configuration is selected. These new
> interfaces will be associated with any matching device-tree interface
> nodes and that these would be distinct from any nodes that matches
> another configuration.
>
> So I don't think there's any problem with dealing with the rare cases of
> multi-configuration devices.
Perhaps it is rare enough that we don't worry about the above case.
I'm not saying we shouldn't follow the OF spec here, but I also think
our usecases have changed a bit in 20 years so we could want to do
something different.
The other part of this is how do we make this usecase work on non-DT
systems or DT systems where the USB topology is not fully described
because you're just hotplugging the USB device.
Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 1/4] omapdrm: fix compatible string for td028ttec1
From: Andrew F. Davis @ 2017-11-16 18:32 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Mark Rutland, devicetree, Julia Lawall, letux-kernel,
Bartlomiej Zolnierkiewicz, David Airlie, dri-devel, Russell King,
Rob Herring, linux-kernel, Tony Lindgren, linux-fbdev,
Tomi Valkeinen, Thierry Reding, Laurent Pinchart,
Benoît Cousson, kernel, Sean Paul, linux-omap,
linux-arm-kernel
In-Reply-To: <689D4636-A169-4C4A-89C2-7039C78E853F@goldelico.com>
On 11/16/2017 12:18 PM, H. Nikolaus Schaller wrote:
>
>> Am 16.11.2017 um 18:08 schrieb Andrew F. Davis <afd@ti.com>:
>>
>> On 11/16/2017 10:10 AM, H. Nikolaus Schaller wrote:
>>> Hi Andrew,
>>>
>>>> Am 16.11.2017 um 16:53 schrieb Andrew F. Davis <afd@ti.com>:
>>>>
>>>> On 11/16/2017 07:43 AM, H. Nikolaus Schaller wrote:
>>>>>
>>>>>> Am 16.11.2017 um 13:32 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:
>>>>>>
>>>>>> On 16/11/17 10:50, H. Nikolaus Schaller wrote:
>>>>>>> The vendor name was "toppoly" but other panels and the vendor list
>>>>>>> have defined it as "tpo". So let's fix it in driver and bindings.
>>>>>>>
>>>>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>>>>> ---
>>>>>>
>>>>>>
>>>>>>> -MODULE_ALIAS("spi:toppoly,td028ttec1");
>>>>>>> +MODULE_ALIAS("spi:tpo,td028ttec1");
>>>>>>
>>>>>> Doesn't this mean that the module won't load if you have old bindings?
>>>>>
>>>>> Hm.
>>>>>
>>>>> Well, I think it can load but doesn't automatically from DT strings which might
>>>>> be unexpected.
>>>>>
>>>>>> Can't we have two module aliases?
>>>>>
>>>>> I think we can. Just a random example:
>>>>> https://elixir.free-electrons.com/linux/latest/source/drivers/w1/slaves/w1_therm.c#L754
>>>>>
>>>>> So we should keep both.
>>>>
>>>> Even better would be to drop both MODULE_ALIAS and let the
>>>> MODULE_DEVICE_TABLE macro define them for your from the SPI id table.
>>>
>>> Why would that be better?
>>>
>>
>> MODULE_ALIAS is ugly, you already have a table (usually) of device names
>> that are supported by the driver, the module aliases should be generated
>> from that table. This also keeps supported device list in one place.
>>
>>> As far as I see it will need more code and changes than adding one line of
>>> MODULE_ALIAS.
>>>
>>>> Although it doesn't look like this driver has an SPI id table, you
>>>> should probably add one, I be interested to see if this module is always
>>>> being matched through the "spi" or the "of" alias..
>>>
>>> Could you please propose how that code should look like, so that I can test?
>>>
>>
>> Sure,
>>
>> start with
>> $ udevadm monitor
>> and see what string the kernel is looking for when trying to find a
>> module for this device.
>
> Well, the module is loaded automatically from DT at boot time well before
> I can start udevadm. So that is the most tricky part to setup the system
> to suppress this...
>
>>
>> If it is only ever looking for "of:toppoly,td028ttec1", then you can
>> drop the MODULE_ALIAS and be done as it was never getting used anyway.
>
> Since it is an SPI client, I am sure it looks for "spi:something.
>
>>
>> What I expect though is "spi:toppoly,td028ttec1", in which case you
>> should add
>>
>> static const struct spi_device_id td028ttec1_ids[] = {
>> { "toppoly,td028ttec1", 0 },
>> { "tpo,td028ttec1", 0},
>> { /* sentinel */ }
>> };
>> MODULE_DEVICE_TABLE(spi, td028ttec1_ids);
>
> We already have a static const struct of_device_id td028ttec1_of_match[]
> table with the same information.
>
> So we still have two places to keep in sync.
>
> Or can we remove the td028ttec1_of_match[]? AFAIK not.
>
>>
>> link to it in the td028ttec1_spi_driver struct:
>> .id_table = td028ttec1_ids,
>>
>> Then test again to see that the module still loads with the new and old
>> DT string.
>
> In total I am not really convinced that adding 7 lines of code is better
> than one (the "tpo," alias) that is tested and works...
>
> And it looks like a lot of unplanned code testing for me which takes more
> than 5 minutes :)
>
> So I'd prefer to leave that exercise of fixing the MODULE_ALIAS/DEVICE_TABLE
> to someone else...
>
That's fine, someday I'll probably get some script to do this for all
the drivers that still have MODULE_ALIAS and an existing table.
> BR and thanks,
> Nikolaus
>
>>
>> Andrew
>>
>>> BR and thanks,
>>> Nikolaus Schaller
>>>
>>>>
>>>>>
>>>>> Should I submit a new version?
>>>>>
>>>>> BR,
>>>>> Nikolaus
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* [PATCH v2 4/4] ARM: dts: r8a7745: add VIN dt support
From: Fabrizio Castro @ 2017-11-16 18:22 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Russell King
Cc: Fabrizio Castro, Simon Horman, Magnus Damm, linux-renesas-soc,
devicetree, linux-arm-kernel, Geert Uytterhoeven, Chris Paterson,
Biju Das
In-Reply-To: <1510856571-30281-1-git-send-email-fabrizio.castro@bp.renesas.com>
Add VIN[01] support to SoC dt. Also, add aliases.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Biju Das <biju.das@bp.renesas.com>
---
This patch depends on the following patch:
* ARM: dts: r8a7745: Add CAN[01] SoC support
https://patchwork.kernel.org/patch/10046867/
v1->v2:
* new patch to keep dt-bindings changes and all the related DT changes in the
same series
arch/arm/boot/dts/r8a7745.dtsi | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/arch/arm/boot/dts/r8a7745.dtsi b/arch/arm/boot/dts/r8a7745.dtsi
index 529e8c6..0fa7861 100644
--- a/arch/arm/boot/dts/r8a7745.dtsi
+++ b/arch/arm/boot/dts/r8a7745.dtsi
@@ -31,6 +31,8 @@
spi1 = &msiof0;
spi2 = &msiof1;
spi3 = &msiof2;
+ vin0 = &vin0;
+ vin1 = &vin1;
};
cpus {
@@ -837,6 +839,28 @@
status = "disabled";
};
+ vin0: video@e6ef0000 {
+ compatible = "renesas,vin-r8a7745",
+ "renesas,rcar-gen2-vin";
+ reg = <0 0xe6ef0000 0 0x1000>;
+ interrupts = <GIC_SPI 188 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cpg CPG_MOD 811>;
+ power-domains = <&sysc R8A7745_PD_ALWAYS_ON>;
+ resets = <&cpg 811>;
+ status = "disabled";
+ };
+
+ vin1: video@e6ef1000 {
+ compatible = "renesas,vin-r8a7745",
+ "renesas,rcar-gen2-vin";
+ reg = <0 0xe6ef1000 0 0x1000>;
+ interrupts = <GIC_SPI 189 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cpg CPG_MOD 810>;
+ power-domains = <&sysc R8A7745_PD_ALWAYS_ON>;
+ resets = <&cpg 810>;
+ status = "disabled";
+ };
+
du: display@feb00000 {
compatible = "renesas,du-r8a7745";
reg = <0 0xfeb00000 0 0x40000>;
--
2.7.4
^ permalink raw reply related
* [PATCH v2 3/4] ARM: dts: r8a7743: add VIN dt support
From: Fabrizio Castro @ 2017-11-16 18:22 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Russell King
Cc: Fabrizio Castro, Simon Horman, Magnus Damm, linux-renesas-soc,
devicetree, linux-arm-kernel, Geert Uytterhoeven, Chris Paterson,
Biju Das
In-Reply-To: <1510856571-30281-1-git-send-email-fabrizio.castro@bp.renesas.com>
Add VIN[012] support to SoC dt. Also, add aliases.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Biju Das <biju.das@bp.renesas.com>
---
This patch depends on the following patches:
* ARM: dts: r8a7743: Add default PCIe bus clock
https://patchwork.kernel.org/patch/10056485/
* ARM: dts: r8a7743: Add PCIe Controller device node
https://patchwork.kernel.org/patch/10056487/
v1->v2:
* no difference
arch/arm/boot/dts/r8a7743.dtsi | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/arch/arm/boot/dts/r8a7743.dtsi b/arch/arm/boot/dts/r8a7743.dtsi
index 9e26c40..c09c667 100644
--- a/arch/arm/boot/dts/r8a7743.dtsi
+++ b/arch/arm/boot/dts/r8a7743.dtsi
@@ -32,6 +32,9 @@
spi1 = &msiof0;
spi2 = &msiof1;
spi3 = &msiof2;
+ vin0 = &vin0;
+ vin1 = &vin1;
+ vin2 = &vin2;
};
cpus {
@@ -1037,6 +1040,39 @@
};
};
+ vin0: video@e6ef0000 {
+ compatible = "renesas,vin-r8a7743",
+ "renesas,rcar-gen2-vin";
+ reg = <0 0xe6ef0000 0 0x1000>;
+ interrupts = <GIC_SPI 188 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cpg CPG_MOD 811>;
+ power-domains = <&sysc R8A7743_PD_ALWAYS_ON>;
+ resets = <&cpg 811>;
+ status = "disabled";
+ };
+
+ vin1: video@e6ef1000 {
+ compatible = "renesas,vin-r8a7743",
+ "renesas,rcar-gen2-vin";
+ reg = <0 0xe6ef1000 0 0x1000>;
+ interrupts = <GIC_SPI 189 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cpg CPG_MOD 810>;
+ power-domains = <&sysc R8A7743_PD_ALWAYS_ON>;
+ resets = <&cpg 810>;
+ status = "disabled";
+ };
+
+ vin2: video@e6ef2000 {
+ compatible = "renesas,vin-r8a7743",
+ "renesas,rcar-gen2-vin";
+ reg = <0 0xe6ef2000 0 0x1000>;
+ interrupts = <GIC_SPI 190 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cpg CPG_MOD 809>;
+ power-domains = <&sysc R8A7743_PD_ALWAYS_ON>;
+ resets = <&cpg 809>;
+ status = "disabled";
+ };
+
du: display@feb00000 {
compatible = "renesas,du-r8a7743";
reg = <0 0xfeb00000 0 0x40000>,
--
2.7.4
^ permalink raw reply related
* [PATCH v2 2/4] dt-bindings: media: rcar_vin: add device tree support for r8a774[35]
From: Fabrizio Castro @ 2017-11-16 18:22 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Mark Rutland, Rob Herring
Cc: Fabrizio Castro, Niklas Söderlund,
linux-media-u79uwXL29TY76Z2rM5mHXA,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Simon Horman,
Geert Uytterhoeven, Chris Paterson, Biju Das
In-Reply-To: <1510856571-30281-1-git-send-email-fabrizio.castro-kTT6dE0pTRh9uiUsa/gSgQ@public.gmane.org>
Add compatible strings for r8a7743 and r8a7745. No driver change
is needed as "renesas,rcar-gen2-vin" will activate the right code.
However, it is good practice to document compatible strings for the
specific SoC as this allows SoC specific changes to the driver if
needed, in addition to document SoC support and therefore allow
checkpatch.pl to validate compatible string values.
Signed-off-by: Fabrizio Castro <fabrizio.castro-kTT6dE0pTRh9uiUsa/gSgQ@public.gmane.org>
Reviewed-by: Biju Das <biju.das-kTT6dE0pTRh9uiUsa/gSgQ@public.gmane.org>
---
v1->v2:
* Fixed double "change" in changelog
Documentation/devicetree/bindings/media/rcar_vin.txt | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/rcar_vin.txt
index 98931f5..ff9697e 100644
--- a/Documentation/devicetree/bindings/media/rcar_vin.txt
+++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
@@ -6,6 +6,8 @@ family of devices. The current blocks are always slaves and suppot one input
channel which can be either RGB, YUYV or BT656.
- compatible: Must be one or more of the following
+ - "renesas,vin-r8a7743" for the R8A7743 device
+ - "renesas,vin-r8a7745" for the R8A7745 device
- "renesas,vin-r8a7778" for the R8A7778 device
- "renesas,vin-r8a7779" for the R8A7779 device
- "renesas,vin-r8a7790" for the R8A7790 device
@@ -14,7 +16,8 @@ channel which can be either RGB, YUYV or BT656.
- "renesas,vin-r8a7793" for the R8A7793 device
- "renesas,vin-r8a7794" for the R8A7794 device
- "renesas,vin-r8a7795" for the R8A7795 device
- - "renesas,rcar-gen2-vin" for a generic R-Car Gen2 compatible device.
+ - "renesas,rcar-gen2-vin" for a generic R-Car Gen2 or RZ/G1 compatible
+ device.
- "renesas,rcar-gen3-vin" for a generic R-Car Gen3 compatible device.
When compatible with the generic version nodes must list the
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v2 1/4] dt-bindings: media: rcar_vin: Reverse SoC part number list
From: Fabrizio Castro @ 2017-11-16 18:22 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Mark Rutland, Rob Herring
Cc: Fabrizio Castro, Niklas Söderlund, linux-media,
linux-renesas-soc, devicetree, Simon Horman, Geert Uytterhoeven,
Chris Paterson, Biju Das
In-Reply-To: <1510856571-30281-1-git-send-email-fabrizio.castro@bp.renesas.com>
Change the sorting of the part numbers from descending to ascending to
match with other documentation.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Biju Das <biju.das@bp.renesas.com>
---
v1->v2:
* new patch triggered by Geert's comment, see the below link for details:
https://www.mail-archive.com/linux-media@vger.kernel.org/msg121992.html
Documentation/devicetree/bindings/media/rcar_vin.txt | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/rcar_vin.txt
index 6e4ef8c..98931f5 100644
--- a/Documentation/devicetree/bindings/media/rcar_vin.txt
+++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
@@ -6,14 +6,14 @@ family of devices. The current blocks are always slaves and suppot one input
channel which can be either RGB, YUYV or BT656.
- compatible: Must be one or more of the following
- - "renesas,vin-r8a7795" for the R8A7795 device
- - "renesas,vin-r8a7794" for the R8A7794 device
- - "renesas,vin-r8a7793" for the R8A7793 device
- - "renesas,vin-r8a7792" for the R8A7792 device
- - "renesas,vin-r8a7791" for the R8A7791 device
- - "renesas,vin-r8a7790" for the R8A7790 device
- - "renesas,vin-r8a7779" for the R8A7779 device
- "renesas,vin-r8a7778" for the R8A7778 device
+ - "renesas,vin-r8a7779" for the R8A7779 device
+ - "renesas,vin-r8a7790" for the R8A7790 device
+ - "renesas,vin-r8a7791" for the R8A7791 device
+ - "renesas,vin-r8a7792" for the R8A7792 device
+ - "renesas,vin-r8a7793" for the R8A7793 device
+ - "renesas,vin-r8a7794" for the R8A7794 device
+ - "renesas,vin-r8a7795" for the R8A7795 device
- "renesas,rcar-gen2-vin" for a generic R-Car Gen2 compatible device.
- "renesas,rcar-gen3-vin" for a generic R-Car Gen3 compatible device.
--
2.7.4
^ permalink raw reply related
* [PATCH v2 0/4] Add VIN support to r8a774[35]
From: Fabrizio Castro @ 2017-11-16 18:22 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Mark Rutland, Rob Herring, Russell King
Cc: Fabrizio Castro, Niklas Söderlund, linux-media,
linux-renesas-soc, devicetree, linux-arm-kernel, Simon Horman,
Magnus Damm, Geert Uytterhoeven, Chris Paterson, Biju Das
Hello,
this series documents VIN related dt-bindings for r8a774[35], adds VIN[012]
nodes to the r8a7743 SoC dtsi and adds VIN[01] nodes to the r8a7745 SoC dtsi.
Best regards,
Fabrizio Castro (4):
dt-bindings: media: rcar_vin: Reverse SoC part number list
dt-bindings: media: rcar_vin: add device tree support for r8a774[35]
ARM: dts: r8a7743: add VIN dt support
ARM: dts: r8a7745: add VIN dt support
.../devicetree/bindings/media/rcar_vin.txt | 19 +++++++-----
arch/arm/boot/dts/r8a7743.dtsi | 36 ++++++++++++++++++++++
arch/arm/boot/dts/r8a7745.dtsi | 24 +++++++++++++++
3 files changed, 71 insertions(+), 8 deletions(-)
--
2.7.4
^ permalink raw reply
* RE: [PATCH 1/2] dt-bindings: media: rcar_vin: add device tree support for r8a774[35]
From: Fabrizio Castro @ 2017-11-16 18:22 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Mauro Carvalho Chehab, Mark Rutland, Rob Herring,
Niklas Söderlund, Linux Media Mailing List, Linux-Renesas,
devicetree@vger.kernel.org, Simon Horman, Chris Paterson,
Biju Das
In-Reply-To: <CAMuHMdVLkhdb0nKMd8OzRK7=wnh_Dxww932Vq2P_OiL=a29V4w@mail.gmail.com>
Hi Geert,
> >>
> >> Please keep the list sorted by SoC part number.
> >
> > It is sorted, just in descending order. Do you want me to re-order the full list in ascending order?
>
> That may be a good idea, given the current order is non-standard and
> counter-intuitive.
sure, dropping this series and sending a v2 then.
Cheers,
Fab
>
> Thanks!
>
> Gr{oetje,eeting}s,
>
> Geert
Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
^ permalink raw reply
* Re: [PATCH v2 1/4] omapdrm: fix compatible string for td028ttec1
From: H. Nikolaus Schaller @ 2017-11-16 18:18 UTC (permalink / raw)
To: Andrew F. Davis
Cc: Mark Rutland, devicetree, Julia Lawall, letux-kernel,
Bartlomiej Zolnierkiewicz, David Airlie, dri-devel, Russell King,
Rob Herring, linux-kernel, Tony Lindgren, linux-fbdev,
Tomi Valkeinen, Thierry Reding, Laurent Pinchart,
Benoît Cousson, kernel, Sean Paul, linux-omap,
linux-arm-kernel
In-Reply-To: <a6b4c02d-a5ff-b0e6-ca59-afaf1148fe2a@ti.com>
> Am 16.11.2017 um 18:08 schrieb Andrew F. Davis <afd@ti.com>:
>
> On 11/16/2017 10:10 AM, H. Nikolaus Schaller wrote:
>> Hi Andrew,
>>
>>> Am 16.11.2017 um 16:53 schrieb Andrew F. Davis <afd@ti.com>:
>>>
>>> On 11/16/2017 07:43 AM, H. Nikolaus Schaller wrote:
>>>>
>>>>> Am 16.11.2017 um 13:32 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:
>>>>>
>>>>> On 16/11/17 10:50, H. Nikolaus Schaller wrote:
>>>>>> The vendor name was "toppoly" but other panels and the vendor list
>>>>>> have defined it as "tpo". So let's fix it in driver and bindings.
>>>>>>
>>>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>>>> ---
>>>>>
>>>>>
>>>>>> -MODULE_ALIAS("spi:toppoly,td028ttec1");
>>>>>> +MODULE_ALIAS("spi:tpo,td028ttec1");
>>>>>
>>>>> Doesn't this mean that the module won't load if you have old bindings?
>>>>
>>>> Hm.
>>>>
>>>> Well, I think it can load but doesn't automatically from DT strings which might
>>>> be unexpected.
>>>>
>>>>> Can't we have two module aliases?
>>>>
>>>> I think we can. Just a random example:
>>>> https://elixir.free-electrons.com/linux/latest/source/drivers/w1/slaves/w1_therm.c#L754
>>>>
>>>> So we should keep both.
>>>
>>> Even better would be to drop both MODULE_ALIAS and let the
>>> MODULE_DEVICE_TABLE macro define them for your from the SPI id table.
>>
>> Why would that be better?
>>
>
> MODULE_ALIAS is ugly, you already have a table (usually) of device names
> that are supported by the driver, the module aliases should be generated
> from that table. This also keeps supported device list in one place.
>
>> As far as I see it will need more code and changes than adding one line of
>> MODULE_ALIAS.
>>
>>> Although it doesn't look like this driver has an SPI id table, you
>>> should probably add one, I be interested to see if this module is always
>>> being matched through the "spi" or the "of" alias..
>>
>> Could you please propose how that code should look like, so that I can test?
>>
>
> Sure,
>
> start with
> $ udevadm monitor
> and see what string the kernel is looking for when trying to find a
> module for this device.
Well, the module is loaded automatically from DT at boot time well before
I can start udevadm. So that is the most tricky part to setup the system
to suppress this...
>
> If it is only ever looking for "of:toppoly,td028ttec1", then you can
> drop the MODULE_ALIAS and be done as it was never getting used anyway.
Since it is an SPI client, I am sure it looks for "spi:something.
>
> What I expect though is "spi:toppoly,td028ttec1", in which case you
> should add
>
> static const struct spi_device_id td028ttec1_ids[] = {
> { "toppoly,td028ttec1", 0 },
> { "tpo,td028ttec1", 0},
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(spi, td028ttec1_ids);
We already have a static const struct of_device_id td028ttec1_of_match[]
table with the same information.
So we still have two places to keep in sync.
Or can we remove the td028ttec1_of_match[]? AFAIK not.
>
> link to it in the td028ttec1_spi_driver struct:
> .id_table = td028ttec1_ids,
>
> Then test again to see that the module still loads with the new and old
> DT string.
In total I am not really convinced that adding 7 lines of code is better
than one (the "tpo," alias) that is tested and works...
And it looks like a lot of unplanned code testing for me which takes more
than 5 minutes :)
So I'd prefer to leave that exercise of fixing the MODULE_ALIAS/DEVICE_TABLE
to someone else...
BR and thanks,
Nikolaus
>
> Andrew
>
>> BR and thanks,
>> Nikolaus Schaller
>>
>>>
>>>>
>>>> Should I submit a new version?
>>>>
>>>> BR,
>>>> Nikolaus
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
^ permalink raw reply
* [PATCH 2/2] of: unittest: disable interrupts_property warning
From: Rob Herring @ 2017-11-16 18:05 UTC (permalink / raw)
To: devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Frank Rowand
In-Reply-To: <20171116180545.12322-1-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
The testcases.dts has purposely bad data which now generates a dtc warning:
drivers/of/unittest-data/testcases.dtb: Warning (interrupts_property): interrupts size is (4), expected multiple of 8 in /testcase-data/testcase-device2
Disable this warning for now. The proper solution is to split the unit
tests into good and bad data.
Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
drivers/of/unittest-data/Makefile | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
index 2d135fba94c1..84b3bc44c0b4 100644
--- a/drivers/of/unittest-data/Makefile
+++ b/drivers/of/unittest-data/Makefile
@@ -1,3 +1,4 @@
+DTC_FLAGS_testcases := -Wno-interrupts_property
obj-y += testcases.dtb.o
targets += testcases.dtb testcases.dtb.S
--
2.14.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH 1/2] of: unittest: let dtc generate __local_fixups__
From: Rob Herring @ 2017-11-16 18:05 UTC (permalink / raw)
To: devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Frank Rowand
Remove the manually added __local_fixups__ because dtc can now generate
them. This also fixes a new warning in the process:
drivers/of/unittest-data/testcases.dtb: Warning (interrupts_extended_property): Could not get phandle node for /__local_fixups__/testcase-data/interrupts/interrupts-extended0:interrupts-extended(cell 3)
Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
drivers/of/unittest-data/testcases.dts | 56 ++--------------------------------
1 file changed, 2 insertions(+), 54 deletions(-)
diff --git a/drivers/of/unittest-data/testcases.dts b/drivers/of/unittest-data/testcases.dts
index 12f7c3d649c8..ac616ad18ea6 100644
--- a/drivers/of/unittest-data/testcases.dts
+++ b/drivers/of/unittest-data/testcases.dts
@@ -1,4 +1,6 @@
/dts-v1/;
+/plugin/;
+
/ {
testcase-data {
changeset {
@@ -23,57 +25,3 @@
* The format of this data may be subject to change. For the time being consider
* this a kernel-internal data format.
*/
-/ { __local_fixups__ {
- testcase-data {
- phandle-tests {
- consumer-a {
- phandle-list = <0x00000000 0x00000008
- 0x00000018 0x00000028
- 0x00000034 0x00000038>;
- phandle-list-bad-args = <0x00000000 0x0000000c>;
- };
- };
- interrupts {
- intmap0 {
- interrupt-map = <0x00000004 0x00000010
- 0x00000024 0x00000034>;
- };
- intmap1 {
- interrupt-map = <0x0000000c>;
- };
- interrupts0 {
- interrupt-parent = <0x00000000>;
- };
- interrupts1 {
- interrupt-parent = <0x00000000>;
- };
- interrupts-extended0 {
- interrupts-extended = <0x00000000 0x00000008
- 0x00000018 0x00000024
- 0x0000002c 0x00000034
- 0x0000003c>;
- };
- };
- testcase-device1 {
- interrupt-parent = <0x00000000>;
- };
- testcase-device2 {
- interrupt-parent = <0x00000000>;
- };
- overlay2 {
- fragment@0 {
- target = <0x00000000>;
- };
- };
- overlay3 {
- fragment@0 {
- target = <0x00000000>;
- };
- };
- overlay4 {
- fragment@0 {
- target = <0x00000000>;
- };
- };
- };
-}; };
--
2.14.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH v7 04/13] slimbus: core: Add slim controllers support
From: Srinivas Kandagatla @ 2017-11-16 17:29 UTC (permalink / raw)
To: Vinod Koul
Cc: mark.rutland, alsa-devel, michael.opdenacker, poeschel,
andreas.noever, arnd, treding, devicetree, james.hogan,
pawel.moll, linux-arm-msm, sharon.dvir1, robh+dt, sdharia, alan,
bp, mathieu.poirier, gregkh, linux-kernel, broonie, daniel,
jkosina, joe, davem
In-Reply-To: <20171116164233.GB3187@localhost>
thanks for the comments.
On 16/11/17 16:42, Vinod Koul wrote:
> On Wed, Nov 15, 2017 at 02:10:34PM +0000, srinivas.kandagatla@linaro.org wrote:
>
>> +static void slim_dev_release(struct device *dev)
>> +{
>> + struct slim_device *sbdev = to_slim_device(dev);
>> +
>> + put_device(sbdev->ctrl->dev);
>
> which device would that be?
This is controller device
>
>> +static int slim_add_device(struct slim_controller *ctrl,
>> + struct slim_device *sbdev,
>> + struct device_node *node)
>> +{
>> + sbdev->dev.bus = &slimbus_bus;
>> + sbdev->dev.parent = ctrl->dev;
>> + sbdev->dev.release = slim_dev_release;
>> + sbdev->dev.driver = NULL;
>> + sbdev->ctrl = ctrl;
>> +
>> + dev_set_name(&sbdev->dev, "%x:%x:%x:%x",
>> + sbdev->e_addr.manf_id,
>> + sbdev->e_addr.prod_code,
>> + sbdev->e_addr.dev_index,
>> + sbdev->e_addr.instance);
>> +
>> + get_device(ctrl->dev);
>
> is this controller device and you ensuring it doesnt go away while you have
> slaves on it?
Yes.
>
>> +static struct slim_device *slim_alloc_device(struct slim_controller *ctrl,
>> + struct slim_eaddr *eaddr,
>> + struct device_node *node)
>> +{
>> + struct slim_device *sbdev;
>> + int ret;
>> +
>> + sbdev = kzalloc(sizeof(struct slim_device), GFP_KERNEL);
>
> Usual kernel way of doing is kzalloc(*sbdev)
>
I agree will fix it in next version.
>> +void slim_report_absent(struct slim_device *sbdev)
>> +{
>> + struct slim_controller *ctrl = sbdev->ctrl;
>> +
>> + if (!ctrl)
>> + return;
>> +
>> + /* invalidate logical addresses */
>> + mutex_lock(&ctrl->lock);
>> + sbdev->is_laddr_valid = false;
>> + mutex_unlock(&ctrl->lock);
>> +
>> + ida_simple_remove(&ctrl->laddr_ida, sbdev->laddr);
>> + slim_device_update_status(sbdev, SLIM_DEVICE_STATUS_DOWN);
>> +}
>> +EXPORT_SYMBOL_GPL(slim_report_absent);
>
> Do you have APIs for report present too, if so why not add te status in
> argument as you may have common handling
Yes, We do have api for reporting too, I will give it a try to combine
both.
>
>> +static int slim_device_alloc_laddr(struct slim_device *sbdev,
>> + u8 *laddr, bool report_present)
>> +{
>> + struct slim_controller *ctrl = sbdev->ctrl;
>> + int ret;
>> +
>> + mutex_lock(&ctrl->lock);
>> + if (ctrl->get_laddr) {
>> + ret = ctrl->get_laddr(ctrl, &sbdev->e_addr, laddr);
>> + if (ret < 0)
>> + goto err;
>> + } else if (report_present) {
>> + ret = ida_simple_get(&ctrl->laddr_ida,
>> + 0, SLIM_LA_MANAGER - 1, GFP_KERNEL);
>> + if (ret < 0)
>> + goto err;
>> +
>> + *laddr = ret;
>> + } else {
>> + ret = -EINVAL;
>> + goto err;
>> + }
>> +
>> + if (ctrl->set_laddr) {
>> + ret = ctrl->set_laddr(ctrl, &sbdev->e_addr, *laddr);
>> + if (ret) {
>> + ret = -EINVAL;
>> + goto err;
>> + }
>> + }
>> +
>> + sbdev->laddr = *laddr;
>
> if you have this in sbdev, then why have this as an arg also?
Yes makes sens, laddr argument in this function is redundant, it can be
removed totally.
>
>> + sbdev->is_laddr_valid = true;
>
> shouldn't non-zero value signify that?
0 is also a valid laddr.
>
^ permalink raw reply
* Re: [PATCH v2 1/4] omapdrm: fix compatible string for td028ttec1
From: Andrew F. Davis @ 2017-11-16 17:08 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Mark Rutland, devicetree, Julia Lawall, letux-kernel,
Bartlomiej Zolnierkiewicz, David Airlie, dri-devel, Russell King,
Rob Herring, linux-kernel, Tony Lindgren, linux-fbdev,
Tomi Valkeinen, Thierry Reding, Laurent Pinchart,
Benoît Cousson, kernel, Sean Paul, linux-omap,
linux-arm-kernel
In-Reply-To: <908C52B9-83E2-49B3-B5B4-D8F27A96ECD6@goldelico.com>
On 11/16/2017 10:10 AM, H. Nikolaus Schaller wrote:
> Hi Andrew,
>
>> Am 16.11.2017 um 16:53 schrieb Andrew F. Davis <afd@ti.com>:
>>
>> On 11/16/2017 07:43 AM, H. Nikolaus Schaller wrote:
>>>
>>>> Am 16.11.2017 um 13:32 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:
>>>>
>>>> On 16/11/17 10:50, H. Nikolaus Schaller wrote:
>>>>> The vendor name was "toppoly" but other panels and the vendor list
>>>>> have defined it as "tpo". So let's fix it in driver and bindings.
>>>>>
>>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>>> ---
>>>>
>>>>
>>>>> -MODULE_ALIAS("spi:toppoly,td028ttec1");
>>>>> +MODULE_ALIAS("spi:tpo,td028ttec1");
>>>>
>>>> Doesn't this mean that the module won't load if you have old bindings?
>>>
>>> Hm.
>>>
>>> Well, I think it can load but doesn't automatically from DT strings which might
>>> be unexpected.
>>>
>>>> Can't we have two module aliases?
>>>
>>> I think we can. Just a random example:
>>> https://elixir.free-electrons.com/linux/latest/source/drivers/w1/slaves/w1_therm.c#L754
>>>
>>> So we should keep both.
>>
>> Even better would be to drop both MODULE_ALIAS and let the
>> MODULE_DEVICE_TABLE macro define them for your from the SPI id table.
>
> Why would that be better?
>
MODULE_ALIAS is ugly, you already have a table (usually) of device names
that are supported by the driver, the module aliases should be generated
from that table. This also keeps supported device list in one place.
> As far as I see it will need more code and changes than adding one line of
> MODULE_ALIAS.
>
>> Although it doesn't look like this driver has an SPI id table, you
>> should probably add one, I be interested to see if this module is always
>> being matched through the "spi" or the "of" alias..
>
> Could you please propose how that code should look like, so that I can test?
>
Sure,
start with
$ udevadm monitor
and see what string the kernel is looking for when trying to find a
module for this device.
If it is only ever looking for "of:toppoly,td028ttec1", then you can
drop the MODULE_ALIAS and be done as it was never getting used anyway.
What I expect though is "spi:toppoly,td028ttec1", in which case you
should add
static const struct spi_device_id td028ttec1_ids[] = {
{ "toppoly,td028ttec1", 0 },
{ "tpo,td028ttec1", 0},
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(spi, td028ttec1_ids);
link to it in the td028ttec1_spi_driver struct:
.id_table = td028ttec1_ids,
Then test again to see that the module still loads with the new and old
DT string.
Andrew
> BR and thanks,
> Nikolaus Schaller
>
>>
>>>
>>> Should I submit a new version?
>>>
>>> BR,
>>> Nikolaus
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH V1 1/4] qcom: spmi-wled: Add support for qcom wled driver
From: Bjorn Andersson @ 2017-11-16 16:55 UTC (permalink / raw)
To: Kiran Gunda
Cc: linux-arm-msm, Lee Jones, Daniel Thompson, Jingoo Han,
Richard Purdie, Jacek Anaszewski, Pavel Machek, Rob Herring,
Mark Rutland, Bartlomiej Zolnierkiewicz, linux-leds, devicetree,
linux-kernel, linux-fbdev, linux-arm-msm-owner
In-Reply-To: <1510834717-21765-2-git-send-email-kgunda@codeaurora.org>
On Thu 16 Nov 04:18 PST 2017, Kiran Gunda wrote:
> WLED driver provides the interface to the display driver to
> adjust the brightness of the display backlight.
>
Hi Kiran,
This driver has a lot in common with the already upstream pm8941-wled.c,
because it's just a new revision of the same block.
Please extend the existing driver rather than providing a new one
(and yes, renaming the file is okay).
Regards,
Bjorn
^ permalink raw reply
* Re: Decompiled vs *tmp extended device tree
From: Rob Herring @ 2017-11-16 16:52 UTC (permalink / raw)
To: Alan Martinovic; +Cc: devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CAOT_U5bV1cWsXa-d7A5sbd0e+UuhD6NmwsmyjrQcuWceqtus_A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Thu, Nov 16, 2017 at 01:12:55PM +0100, Alan Martinovic wrote:
> Hi,
> I'm trying to modify a device tree to enable support of the second UART.
> The actual device tree which is used for the example:
>
> https://github.com/getsenic/senic-os-linux/blob/senic/4.13/arch/arm/boot/dts/sun8i-h3-senic-hub.dts
>
> The device tree includes and overrides some additional files:
>
> #include "sun8i-h3.dtsi"
> #include "sunxi-common-regulators.dtsi"
> #include <dt-bindings/gpio/gpio.h>
> #include <dt-bindings/input/input.h>
> #include <dt-bindings/pinctrl/sun4i-a10.h>
>
> I would like to know to what does the extended device tree look like,
> and use that as my base, because I don't really have the complete picture
> by just looking at the sun8i-h3-senic-hub.dts
>
> In this example I'm interested in where are "uart0_pins_a" defined.
> sun8i-h3-senic-hub.dts references them but has no definitions,
> so I'm assuming the definition is somewhere in the included files.
>
> To help me out with this task I discovered the device tree decompiler.
> Am expecting that the decompiled dts contains all the data that
> end up in the dtb blob, and should, among other, contain all the information
> available in sun8i-h3-senic-hub.dts.
>
> scripts/dtc/dtc -I dtb -O dts -o expanded-devtree.dts
> arch/arm/boot/dts/sun8i-h3-senic-hub.dtb
>
> But the decompiled extended device tree also seems to have no
> traces of "uart0_pins_a" at all :
>
> grep uart0_pins_a expanded-devtree.dts
labels are purely a source level construct (though overlays changes
that some) and are lost after compiling. You have to look for 'uart0@0'.
>
> I've also discovered the hidden .tmp files:
>
> ⟫ ls arch/arm/boot/dts/.sun8i-h3-*
> arch/arm/boot/dts/.sun8i-h3-senic-hub.dtb.cmd
> arch/arm/boot/dts/.sun8i-h3-senic-hub.dtb.d.dtc.tmp
> arch/arm/boot/dts/.sun8i-h3-senic-hub.dtb.d.pre.tmp
> arch/arm/boot/dts/.sun8i-h3-senic-hub.dtb.dts.tmp
>
> and in one of them there is a definition to "uart0_pins_a":
>
> ⟫ grep uart0_pins_a arch/arm/boot/dts/.sun8i-h3-*
> arch/arm/boot/dts/.sun8i-h3-senic-hub.dtb.dts.tmp: uart0_pins_a: uart0@0 {
> arch/arm/boot/dts/.sun8i-h3-senic-hub.dtb.dts.tmp: pinctrl-0 =
> <&uart0_pins_a>;
>
> My question is, which one is a more accurate representation
> of the final device tree that I should use as a basis?
Compiling dts to dts is mainly useful to get rid of all the C
preprocessor and includes to see what the final nodes look like. You do
lose some things though.
> Why isn't it correct to expect that everything from sun8i-h3-senic-hub.dts
> will end up in the final device tree blob?
That's true for every compiler. After compiling, preprocessor symbols
are gone, functions may be inlined, etc.
Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCHv4 1/3] ARMv8: dts: ls1046a: add the property of IB and OB
From: Lorenzo Pieralisi @ 2017-11-16 16:49 UTC (permalink / raw)
To: M.h. Lian, robh+dt
Cc: Leo Li, Kishon Vijay Abraham I, Xiaowei Bao, mark.rutland@arm.com,
catalin.marinas@arm.com, will.deacon@arm.com, bhelgaas@google.com,
shawnguo@kernel.org, Madalin-cristian Bucur, Sumit Garg, Y.b. Lu,
Andy Tang, jingoohan1@gmail.com, pbrobinson@gmail.com,
songxiaowei@hisilicon.com, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <VI1PR04MB161516288A0FF2F507DAA9BEE82B0@VI1PR04MB1615.eurprd04.prod.outlook.com>
On Mon, Nov 13, 2017 at 02:35:48AM +0000, M.h. Lian wrote:
[...]
> > > On Friday 10 November 2017 09:18 AM, Bao Xiaowei wrote:
> > > > Add the property of inbound and outbound windows number for ep driver.
> > > >
> > > > Signed-off-by: Bao Xiaowei <xiaowei.bao@nxp.com>
> > > > Acked-by: Minghuan Lian <minghuan.Lian@nxp.com>
> > > > ---
> > > > v2:
> > > > - no change
> > > > v3:
> > > > - modify the commit message
> > > > v4:
> > > > - no change
> > > >
> > > > arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 6 ++++++
> > > > 1 file changed, 6 insertions(+)
> > >
> > > $subject should start with something like
> > > arm64: dts: ls1046a: **
Indeed.
> > > > diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
> > > > b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
> > > > index 06b5e12d04d8..f8332669663c 100644
> > > > --- a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
> > > > +++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
> > > > @@ -674,6 +674,8 @@
> > > > device_type = "pci";
> > > > dma-coherent;
> > > > num-lanes = <4>;
> > > > + num-ib-windows = <6>;
> > > > + num-ob-windows = <6>;
> > >
> > > EP specific properties shouldn't be added in RC dt node. Ideally you
> > > should have a separate dt node for RC and EP.
> >
> > It is a single PCIe controller which can be configured to either RC
> > mode or EP mode. Wouldn't it conflict with the device tree
> > principles to have two device tree nodes for the same PCIe
> > controller? And obviously the two modes cannot be used at the same
> > time so we cannot have two drivers both probe on the same hardware.
> >
> [Minghuan Lian] There is only one PCIe dts node in the dts file. PCIe
> dts node describes the PCIe controller's hardware properties and does
> not have work mode. The new properties "num-ib-windows " and
> "num-ob-windows" are used to describe the inbound/outbound window
> number included in the PCIe hardware. These windows are used in both
> RC and EP mode. We can change work mode when resetting via RCW(reset
> configuration word).
I am not happy about this (that's what I am asking Rob to chime in
please on DT side).
1) I do not think it is allowed to have two DT nodes in a dts with same unit
address (ie same reg property)
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/dra7.dtsi?h=v4.14
2) In the Synopsis Designware PCIe interface bindings we have some
properties that are for RC mode and some for EP mode but there is
no way from a *binding* perspective to detect in what mode the
controller is:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/designware-pcie.txt?h=v4.14
3) You can't use properties that in the bindings above are declared EP
only for RC mode, we define bindings to respect their rules.
4) I think that a) a compatible should be added to the designware-pcie
bindings to define endpoint mode and b) the same should be done for
the ls1046a bindings. If the RC is programmed in EP mode DT firmware
should be able to provide the information to an operating system, it
is actually a _different_ component but on this I need DT people to
chime in to define the best way forward.
I cannot review/merge this code until the points above are clarified.
Thanks,
Lorenzo
^ permalink raw reply
* Re: [PATCH v7 04/13] slimbus: core: Add slim controllers support
From: Vinod Koul @ 2017-11-16 16:42 UTC (permalink / raw)
To: srinivas.kandagatla
Cc: mark.rutland, alsa-devel, michael.opdenacker, poeschel,
andreas.noever, arnd, treding, devicetree, james.hogan,
pawel.moll, linux-arm-msm, sharon.dvir1, robh+dt, sdharia, alan,
bp, mathieu.poirier, gregkh, linux-kernel, broonie, daniel,
jkosina, joe, davem
In-Reply-To: <20171115141043.29202-5-srinivas.kandagatla@linaro.org>
On Wed, Nov 15, 2017 at 02:10:34PM +0000, srinivas.kandagatla@linaro.org wrote:
> +static void slim_dev_release(struct device *dev)
> +{
> + struct slim_device *sbdev = to_slim_device(dev);
> +
> + put_device(sbdev->ctrl->dev);
which device would that be?
> +static int slim_add_device(struct slim_controller *ctrl,
> + struct slim_device *sbdev,
> + struct device_node *node)
> +{
> + sbdev->dev.bus = &slimbus_bus;
> + sbdev->dev.parent = ctrl->dev;
> + sbdev->dev.release = slim_dev_release;
> + sbdev->dev.driver = NULL;
> + sbdev->ctrl = ctrl;
> +
> + dev_set_name(&sbdev->dev, "%x:%x:%x:%x",
> + sbdev->e_addr.manf_id,
> + sbdev->e_addr.prod_code,
> + sbdev->e_addr.dev_index,
> + sbdev->e_addr.instance);
> +
> + get_device(ctrl->dev);
is this controller device and you ensuring it doesnt go away while you have
slaves on it?
> +static struct slim_device *slim_alloc_device(struct slim_controller *ctrl,
> + struct slim_eaddr *eaddr,
> + struct device_node *node)
> +{
> + struct slim_device *sbdev;
> + int ret;
> +
> + sbdev = kzalloc(sizeof(struct slim_device), GFP_KERNEL);
Usual kernel way of doing is kzalloc(*sbdev)
> +void slim_report_absent(struct slim_device *sbdev)
> +{
> + struct slim_controller *ctrl = sbdev->ctrl;
> +
> + if (!ctrl)
> + return;
> +
> + /* invalidate logical addresses */
> + mutex_lock(&ctrl->lock);
> + sbdev->is_laddr_valid = false;
> + mutex_unlock(&ctrl->lock);
> +
> + ida_simple_remove(&ctrl->laddr_ida, sbdev->laddr);
> + slim_device_update_status(sbdev, SLIM_DEVICE_STATUS_DOWN);
> +}
> +EXPORT_SYMBOL_GPL(slim_report_absent);
Do you have APIs for report present too, if so why not add te status in
argument as you may have common handling
> +static int slim_device_alloc_laddr(struct slim_device *sbdev,
> + u8 *laddr, bool report_present)
> +{
> + struct slim_controller *ctrl = sbdev->ctrl;
> + int ret;
> +
> + mutex_lock(&ctrl->lock);
> + if (ctrl->get_laddr) {
> + ret = ctrl->get_laddr(ctrl, &sbdev->e_addr, laddr);
> + if (ret < 0)
> + goto err;
> + } else if (report_present) {
> + ret = ida_simple_get(&ctrl->laddr_ida,
> + 0, SLIM_LA_MANAGER - 1, GFP_KERNEL);
> + if (ret < 0)
> + goto err;
> +
> + *laddr = ret;
> + } else {
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + if (ctrl->set_laddr) {
> + ret = ctrl->set_laddr(ctrl, &sbdev->e_addr, *laddr);
> + if (ret) {
> + ret = -EINVAL;
> + goto err;
> + }
> + }
> +
> + sbdev->laddr = *laddr;
if you have this in sbdev, then why have this as an arg also?
> + sbdev->is_laddr_valid = true;
shouldn't non-zero value signify that?
--
~Vinod
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox