[RFC v2 0/6] serdev multiplexing support

[RFC v2 0/6] serdev multiplexing support

[Ulrich Hecht]

Hi!

This is a new attempt to add multiplexer support to serdev. It is now based
on the mux subsystem, making it more generic than the previous iteration
("[RFC 0/4] serdev GPIO-based multiplexing support").

Thanks to reviewers for their comments. This revision incorporates the
changes suggested as far as they are still applicable, which mostly applies
to those concerning the MAX9260 i2c adapter driver.

New patches have been added that fix a small issue in the mux include files
("mux: include compiler.h from mux/consumer.h"), and implement deferred
probing of serdev controllers ("serial: core: support deferring serdev
controller registration"), hopefully correctly.

This series depends on the "pinctrl: sh-pfc: r8a7792: Add SCIF1 pin groups"
patch as well as v15 of the mux subsystem series ("[PATCH v15 00/13] mux
controller abstraction and iio/i2c muxes").

CU
Uli


Ulrich Hecht (6):
  mux: include compiler.h from mux/consumer.h
  serdev: add method to set parity
  serdev: add multiplexer support
  serial: core: support deferring serdev controller registration
  max9260: add driver for i2c over GMSL passthrough
  ARM: dts: blanche: add SCIF1 and MAX9260 deserializer

 arch/arm/boot/dts/r8a7792-blanche.dts |  52 ++++++
 drivers/media/i2c/Kconfig             |   6 +
 drivers/media/i2c/Makefile            |   1 +
 drivers/media/i2c/max9260.c           | 288 ++++++++++++++++++++++++++++++++++
 drivers/tty/serdev/Kconfig            |   3 +
 drivers/tty/serdev/Makefile           |   1 +
 drivers/tty/serdev/core.c             |  26 ++-
 drivers/tty/serdev/mux.c              |  66 ++++++++
 drivers/tty/serdev/serdev-ttyport.c   |  17 ++
 drivers/tty/serial/serial_core.c      |   4 +
 include/linux/mux/consumer.h          |   2 +
 include/linux/serdev.h                |  20 ++-
 12 files changed, 482 insertions(+), 4 deletions(-)
 create mode 100644 drivers/media/i2c/max9260.c
 create mode 100644 drivers/tty/serdev/mux.c

-- 
2.7.4

[RFC v2 1/6] mux: include compiler.h from mux/consumer.h

[Ulrich Hecht]

Required for __must_check.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
---
 include/linux/mux/consumer.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
index 5577e1b..ea96d4c 100644
--- a/include/linux/mux/consumer.h
+++ b/include/linux/mux/consumer.h
@@ -13,6 +13,8 @@
 #ifndef _LINUX_MUX_CONSUMER_H
 #define _LINUX_MUX_CONSUMER_H
 
+#include <linux/compiler.h>
+
 struct device;
 struct mux_control;
 
-- 
2.7.4

[RFC v2 2/6] serdev: add method to set parity

[Ulrich Hecht]

Adds serdev_device_set_parity() and an implementation for ttyport.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
---
 drivers/tty/serdev/core.c           | 12 ++++++++++++
 drivers/tty/serdev/serdev-ttyport.c | 17 +++++++++++++++++
 include/linux/serdev.h              |  4 ++++
 3 files changed, 33 insertions(+)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index f71b473..1fbaa4c 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -242,6 +242,18 @@ int serdev_device_set_tiocm(struct serdev_device *serdev, int set, int clear)
 }
 EXPORT_SYMBOL_GPL(serdev_device_set_tiocm);
 
+int serdev_device_set_parity(struct serdev_device *serdev,
+			     bool enable, bool odd)
+{
+	struct serdev_controller *ctrl = serdev->ctrl;
+
+	if (!ctrl || !ctrl->ops->set_parity)
+		return -ENOTSUPP;
+
+	return ctrl->ops->set_parity(ctrl, enable, odd);
+}
+EXPORT_SYMBOL_GPL(serdev_device_set_parity);
+
 static int serdev_drv_probe(struct device *dev)
 {
 	const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index 302018d..9114956 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -195,6 +195,22 @@ static int ttyport_set_tiocm(struct serdev_controller *ctrl, unsigned int set, u
 	return tty->driver->ops->tiocmset(tty, set, clear);
 }
 
+static int ttyport_set_parity(struct serdev_controller *ctrl,
+			      bool enable, bool odd)
+{
+	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+	struct tty_struct *tty = serport->tty;
+	struct ktermios ktermios = tty->termios;
+
+	ktermios.c_cflag &= ~(PARENB | PARODD);
+	if (enable)
+		ktermios.c_cflag |= PARENB;
+	if (odd)
+		ktermios.c_cflag |= PARODD;
+
+	return tty_set_termios(tty, &ktermios);
+}
+
 static const struct serdev_controller_ops ctrl_ops = {
 	.write_buf = ttyport_write_buf,
 	.write_flush = ttyport_write_flush,
@@ -206,6 +222,7 @@ static const struct serdev_controller_ops ctrl_ops = {
 	.wait_until_sent = ttyport_wait_until_sent,
 	.get_tiocm = ttyport_get_tiocm,
 	.set_tiocm = ttyport_set_tiocm,
+	.set_parity = ttyport_set_parity,
 };
 
 struct device *serdev_tty_port_register(struct tty_port *port,
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index e69402d..8b67fcd 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -90,6 +90,7 @@ struct serdev_controller_ops {
 	void (*wait_until_sent)(struct serdev_controller *, long);
 	int (*get_tiocm)(struct serdev_controller *);
 	int (*set_tiocm)(struct serdev_controller *, unsigned int, unsigned int);
+	int (*set_parity)(struct serdev_controller *, bool, bool);
 };
 
 /**
@@ -298,6 +299,9 @@ static inline int serdev_device_set_rts(struct serdev_device *serdev, bool enabl
 		return serdev_device_set_tiocm(serdev, 0, TIOCM_RTS);
 }
 
+int serdev_device_set_parity(struct serdev_device *serdev,
+			     bool enable, bool odd);
+
 /*
  * serdev hooks into TTY core
  */
-- 
2.7.4

[RFC v2 3/6] serdev: add multiplexer support

[Ulrich Hecht]

Adds an interface for slave device multiplexing using the mux subsystem.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
---
 drivers/tty/serdev/Kconfig  |  3 +++
 drivers/tty/serdev/Makefile |  1 +
 drivers/tty/serdev/core.c   | 14 +++++++++-
 drivers/tty/serdev/mux.c    | 66 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/serdev.h      | 16 ++++++++---
 5 files changed, 96 insertions(+), 4 deletions(-)
 create mode 100644 drivers/tty/serdev/mux.c

diff --git a/drivers/tty/serdev/Kconfig b/drivers/tty/serdev/Kconfig
index cdc6b82..9096b71 100644
--- a/drivers/tty/serdev/Kconfig
+++ b/drivers/tty/serdev/Kconfig
@@ -13,4 +13,7 @@ config SERIAL_DEV_CTRL_TTYPORT
 	depends on TTY
 	depends on SERIAL_DEV_BUS != m
 
+config SERIAL_DEV_MUX
+	bool "Serial device multiplexer support"
+
 endif
diff --git a/drivers/tty/serdev/Makefile b/drivers/tty/serdev/Makefile
index 0cbdb94..d713381 100644
--- a/drivers/tty/serdev/Makefile
+++ b/drivers/tty/serdev/Makefile
@@ -1,5 +1,6 @@
 serdev-objs := core.o
 
 obj-$(CONFIG_SERIAL_DEV_BUS) += serdev.o
+obj-$(CONFIG_SERIAL_DEV_MUX) += mux.o
 
 obj-$(CONFIG_SERIAL_DEV_CTRL_TTYPORT) += serdev-ttyport.o
diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index 1fbaa4c..92c5bfa 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -305,7 +305,8 @@ struct serdev_device *serdev_device_alloc(struct serdev_controller *ctrl)
 		return NULL;
 
 	serdev->ctrl = ctrl;
-	ctrl->serdev = serdev;
+	if (!ctrl->serdev)
+		ctrl->serdev = serdev;
 	device_initialize(&serdev->dev);
 	serdev->dev.parent = &ctrl->dev;
 	serdev->dev.bus = &serdev_bus_type;
@@ -368,6 +369,8 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
 	struct serdev_device *serdev = NULL;
 	int err;
 	bool found = false;
+	int nr = 0;
+	u32 reg;
 
 	for_each_available_child_of_node(ctrl->dev.of_node, node) {
 		if (!of_get_property(node, "compatible", NULL))
@@ -380,6 +383,10 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
 			continue;
 
 		serdev->dev.of_node = node;
+		serdev->nr = nr++;
+
+		if (!of_property_read_u32(node, "reg", &reg))
+			serdev->mux_addr = reg;
 
 		err = serdev_device_add(serdev);
 		if (err) {
@@ -414,6 +421,11 @@ int serdev_controller_add(struct serdev_controller *ctrl)
 	if (ret)
 		return ret;
 
+#ifdef CONFIG_SERIAL_DEV_MUX
+	if (of_serdev_register_mux(ctrl) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+#endif
+
 	ret = of_serdev_register_devices(ctrl);
 	if (ret)
 		goto out_dev_del;
diff --git a/drivers/tty/serdev/mux.c b/drivers/tty/serdev/mux.c
new file mode 100644
index 0000000..fc9405b
--- /dev/null
+++ b/drivers/tty/serdev/mux.c
@@ -0,0 +1,66 @@
+/*
+ * Copyright (C) 2017 Ulrich Hecht
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/mux/consumer.h>
+#include <linux/mux/driver.h>
+#include <linux/of_gpio.h>
+#include <linux/serdev.h>
+#include <linux/slab.h>
+
+int serdev_device_mux_select(struct serdev_device *serdev)
+{
+	struct mux_control *mux = serdev->ctrl->mux;
+	int ret;
+
+	if (!mux)
+		return -EINVAL;
+
+	ret = mux_control_select(mux, serdev->mux_addr);
+	serdev->ctrl->mux_do_not_deselect = ret < 0;
+
+	serdev->ctrl->serdev = serdev;
+
+	return ret;
+}
+
+int serdev_device_mux_deselect(struct serdev_device *serdev)
+{
+	struct mux_control *mux = serdev->ctrl->mux;
+
+	if (!mux)
+		return -EINVAL;
+
+	if (!serdev->ctrl->mux_do_not_deselect)
+		return mux_control_deselect(mux);
+	else
+		return 0;
+}
+
+int of_serdev_register_mux(struct serdev_controller *ctrl)
+{
+	struct mux_control *mux;
+	struct device *dev = &ctrl->dev;
+
+	mux = devm_mux_control_get(dev, NULL);
+
+	if (IS_ERR(mux)) {
+		if (PTR_ERR(mux) != -EPROBE_DEFER)
+			dev_err(dev, "failed to get control mux\n");
+		return PTR_ERR(mux);
+	}
+
+	ctrl->mux = mux;
+
+	return 0;
+}
+
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index 8b67fcd..26d5994 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -17,6 +17,7 @@
 #include <linux/device.h>
 #include <linux/termios.h>
 #include <linux/delay.h>
+#include <linux/rtmutex.h>
 
 struct serdev_controller;
 struct serdev_device;
@@ -51,6 +52,7 @@ struct serdev_device {
 	const struct serdev_device_ops *ops;
 	struct completion write_comp;
 	struct mutex write_lock;
+	int mux_addr;
 };
 
 static inline struct serdev_device *to_serdev_device(struct device *d)
@@ -76,6 +78,8 @@ static inline struct serdev_device_driver *to_serdev_device_driver(struct device
 	return container_of(d, struct serdev_device_driver, driver);
 }
 
+int of_serdev_register_mux(struct serdev_controller *ctrl);
+
 /*
  * serdev controller structures
  */
@@ -97,14 +101,18 @@ struct serdev_controller_ops {
  * struct serdev_controller - interface to the serdev controller
  * @dev:	Driver model representation of the device.
  * @nr:		number identifier for this controller/bus.
- * @serdev:	Pointer to slave device for this controller.
+ * @serdev:	Pointer to active slave device for this controller.
  * @ops:	Controller operations.
+ * @mux:	Slave multiplexer control.
+ * @mux_do_not_deselect: Set if slave selection failed.
  */
 struct serdev_controller {
 	struct device		dev;
 	unsigned int		nr;
 	struct serdev_device	*serdev;
 	const struct serdev_controller_ops *ops;
+	struct mux_control	*mux;
+	int			mux_do_not_deselect;
 };
 
 static inline struct serdev_controller *to_serdev_controller(struct device *d)
@@ -172,7 +180,7 @@ static inline void serdev_controller_write_wakeup(struct serdev_controller *ctrl
 {
 	struct serdev_device *serdev = ctrl->serdev;
 
-	if (!serdev || !serdev->ops->write_wakeup)
+	if (!serdev || !serdev->ops || !serdev->ops->write_wakeup)
 		return;
 
 	serdev->ops->write_wakeup(serdev);
@@ -184,7 +192,7 @@ static inline int serdev_controller_receive_buf(struct serdev_controller *ctrl,
 {
 	struct serdev_device *serdev = ctrl->serdev;
 
-	if (!serdev || !serdev->ops->receive_buf)
+	if (!serdev || !serdev->ops || !serdev->ops->receive_buf)
 		return -EINVAL;
 
 	return serdev->ops->receive_buf(serdev, data, count);
@@ -204,6 +212,8 @@ void serdev_device_write_wakeup(struct serdev_device *);
 int serdev_device_write(struct serdev_device *, const unsigned char *, size_t, unsigned long);
 void serdev_device_write_flush(struct serdev_device *);
 int serdev_device_write_room(struct serdev_device *);
+int serdev_device_mux_select(struct serdev_device *);
+int serdev_device_mux_deselect(struct serdev_device *);
 
 /*
  * serdev device driver functions
-- 
2.7.4

[RFC v2 4/6] serial: core: support deferring serdev controller registration

[Ulrich Hecht]

serdev controllers may depend on other devices (such as multiplexers)
and thus require deferred probing support.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
---
 drivers/tty/serial/serial_core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index f534a40..30a8997 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2785,6 +2785,10 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 			uport->line, uport->dev, port, uport->tty_groups);
 	if (likely(!IS_ERR(tty_dev))) {
 		device_set_wakeup_capable(tty_dev, 1);
+	} else if (PTR_ERR(tty_dev) == -EPROBE_DEFER) {
+		ret = -EPROBE_DEFER;
+		state->uart_port = NULL;
+		goto out;
 	} else {
 		dev_err(uport->dev, "Cannot register tty device on line %d\n",
 		       uport->line);
-- 
2.7.4

[RFC v2 5/6] max9260: add driver for i2c over GMSL passthrough

[Ulrich Hecht]

This driver implements tunnelling of i2c requests over GMSL via a
MAX9260 deserializer. It provides an i2c adapter that can be used
to reach devices on the far side of the link.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
---
 drivers/media/i2c/Kconfig   |   6 +
 drivers/media/i2c/Makefile  |   1 +
 drivers/media/i2c/max9260.c | 288 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 295 insertions(+)
 create mode 100644 drivers/media/i2c/max9260.c

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 7641667..a405d67 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -423,6 +423,12 @@ config VIDEO_VPX3220
 	  To compile this driver as a module, choose M here: the
 	  module will be called vpx3220.
 
+config VIDEO_MAX9260
+	tristate "Maxim MAX9260 GMSL deserializer support"
+	depends on I2C
+	---help---
+	  This driver supports the Maxim MAX9260 GMSL deserializer.
+
 comment "Video and audio decoders"
 
 config VIDEO_SAA717X
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 30e856c..3b6f6f2 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
 obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
 obj-$(CONFIG_VIDEO_OV9650) += ov9650.o
 obj-$(CONFIG_VIDEO_OV13858) += ov13858.o
+obj-$(CONFIG_VIDEO_MAX9260) += max9260.o
 obj-$(CONFIG_VIDEO_MT9M032) += mt9m032.o
 obj-$(CONFIG_VIDEO_MT9M111) += mt9m111.o
 obj-$(CONFIG_VIDEO_MT9P031) += mt9p031.o
diff --git a/drivers/media/i2c/max9260.c b/drivers/media/i2c/max9260.c
new file mode 100644
index 0000000..ca34e67
--- /dev/null
+++ b/drivers/media/i2c/max9260.c
@@ -0,0 +1,288 @@
+/*
+ * Maxim MAX9260 GMSL Deserializer Driver
+ *
+ * Copyright (C) 2017 Ulrich Hecht
+ *
+ * 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/i2c.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/serdev.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+
+#define SYNC	0x79
+#define ACK	0xc3
+
+#define RX_FINISHED		0
+#define RX_FRAME_ERROR		1
+#define RX_EXPECT_ACK		2
+#define RX_EXPECT_ACK_DATA	3
+#define RX_EXPECT_DATA		4
+
+struct max9260_device {
+	struct serdev_device *serdev;
+	u8 *rx_buf;
+	int rx_len;
+	int rx_state;
+	wait_queue_head_t rx_wq;
+	struct i2c_adapter adap;
+};
+
+static void wait_for_transaction(struct max9260_device *dev)
+{
+	wait_event_interruptible_timeout(dev->rx_wq,
+		dev->rx_state <= RX_FRAME_ERROR,
+		HZ/2);
+}
+
+static void max9260_transact(struct max9260_device *dev,
+			     int expect,
+			     u8 *request, int len)
+{
+	serdev_device_mux_select(dev->serdev);
+
+	serdev_device_set_baudrate(dev->serdev, 115200);
+	serdev_device_set_parity(dev->serdev, 1, 0);
+
+	dev->rx_state = expect;
+	serdev_device_write_buf(dev->serdev, request, len);
+
+	wait_for_transaction(dev);
+
+	serdev_device_mux_deselect(dev->serdev);
+}
+
+static int max9260_read_reg(struct max9260_device *dev, int reg)
+{
+	u8 request[] = { 0x79, 0x91, reg, 1 };
+	u8 rx;
+
+	dev->rx_len = 1;
+	dev->rx_buf = &rx;
+
+	max9260_transact(dev, RX_EXPECT_ACK_DATA, request, 4);
+
+	if (dev->rx_state == RX_FINISHED)
+		return rx;
+
+	return -EIO;
+}
+
+static int max9260_setup(struct max9260_device *dev)
+{
+	int ret;
+
+	ret = max9260_read_reg(dev, 0x1e);
+
+	if (ret != 0x02) {
+		dev_err(&dev->serdev->dev,
+			"device does not identify as MAX9260\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void max9260_uart_write_wakeup(struct serdev_device *serdev)
+{
+}
+
+static int max9260_uart_receive_buf(struct serdev_device *serdev,
+				    const u8 *data, size_t count)
+{
+	struct max9260_device *dev = serdev_device_get_drvdata(serdev);
+	int accepted;
+
+	switch (dev->rx_state) {
+	case RX_FINISHED:
+		dev_dbg(&dev->serdev->dev, "excess data ignored\n");
+		return count;
+
+	case RX_EXPECT_ACK:
+	case RX_EXPECT_ACK_DATA:
+		if (data[0] != ACK) {
+			dev_dbg(&dev->serdev->dev, "frame error");
+			dev->rx_state = RX_FRAME_ERROR;
+			wake_up_interruptible(&dev->rx_wq);
+			return 1;
+		}
+
+		if (dev->rx_state == RX_EXPECT_ACK_DATA) {
+			dev->rx_state = RX_EXPECT_DATA;
+		} else {
+			dev->rx_state = RX_FINISHED;
+			wake_up_interruptible(&dev->rx_wq);
+		}
+		return 1;
+
+	case RX_EXPECT_DATA:
+		accepted = dev->rx_len < count ? dev->rx_len : count;
+
+		memcpy(dev->rx_buf, data, accepted);
+
+		dev->rx_len -= accepted;
+		dev->rx_buf += accepted;
+
+		if (!dev->rx_len) {
+			dev->rx_state = RX_FINISHED;
+			wake_up_interruptible(&dev->rx_wq);
+		}
+
+		return accepted;
+
+	case RX_FRAME_ERROR:
+		dev_dbg(&dev->serdev->dev, "%d bytes ignored\n", count);
+		return count;
+
+	}
+	return 0;
+}
+
+struct serdev_device_ops max9260_serdev_client_ops = {
+	.receive_buf = max9260_uart_receive_buf,
+	.write_wakeup = max9260_uart_write_wakeup,
+};
+
+static u32 max9260_i2c_func(struct i2c_adapter *adapter)
+{
+	return I2C_FUNC_SMBUS_BYTE|I2C_FUNC_SMBUS_BYTE_DATA;
+}
+
+static s32 max9260_smbus_xfer(struct i2c_adapter *adap, u16 addr,
+	unsigned short flags, char read_write, u8 command, int size,
+	union i2c_smbus_data *data)
+{
+	u8 request[] = { SYNC,
+			 (addr << 1) + (read_write == I2C_SMBUS_READ),
+			 command, 0, 0 };
+	struct max9260_device *dev = i2c_get_adapdata(adap);
+
+	switch (size) {
+	case I2C_SMBUS_BYTE:
+		if (read_write == I2C_SMBUS_WRITE) {
+			max9260_transact(dev, RX_EXPECT_ACK, request, 4);
+			dev_dbg(&adap->dev,
+				"smbus byte - addr 0x%02x, wrote 0x%02x.\n",
+				addr, command);
+		} else {
+			/* TBD */
+			return -EOPNOTSUPP;
+		}
+		break;
+
+	case I2C_SMBUS_BYTE_DATA:
+		request[3] = 1;
+		if (read_write == I2C_SMBUS_WRITE) {
+			request[4] = data->byte;
+			max9260_transact(dev, RX_EXPECT_ACK, request, 5);
+			dev_dbg(&adap->dev,
+				"smbus byte data - addr 0x%02x, wrote 0x%02x at 0x%02x.\n",
+				addr, data->byte, command);
+		} else {
+			dev->rx_len = 1;
+			dev->rx_buf = &data->byte;
+			max9260_transact(dev, RX_EXPECT_ACK_DATA, request, 4);
+			dev_dbg(&adap->dev,
+				"smbus byte data - addr 0x%02x, read  0x%02x at 0x%02x.\n",
+				addr, data->byte, command);
+		}
+		break;
+	default:
+		dev_dbg(&adap->dev,
+			"Unsupported I2C/SMBus command %d\n", size);
+		return -EOPNOTSUPP;
+	}
+
+	if (dev->rx_state != RX_FINISHED) {
+		dev_dbg(&adap->dev, "xfer timed out\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static const struct i2c_algorithm max9260_i2c_algorithm = {
+	.functionality	= max9260_i2c_func,
+	.smbus_xfer	= max9260_smbus_xfer,
+};
+
+static int max9260_probe(struct serdev_device *serdev)
+{
+	struct max9260_device *dev;
+	struct i2c_adapter *adap;
+	int ret;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	init_waitqueue_head(&dev->rx_wq);
+
+	dev->serdev = serdev;
+	serdev_device_open(serdev);
+	serdev_device_set_drvdata(serdev, dev);
+
+	serdev_device_set_client_ops(serdev, &max9260_serdev_client_ops);
+
+	ret = max9260_setup(dev);
+	if (ret < 0)
+		goto err_free;
+
+	adap = &dev->adap;
+	i2c_set_adapdata(adap, dev);
+
+	adap->owner = THIS_MODULE;
+	adap->algo = &max9260_i2c_algorithm;
+	adap->dev.parent = &serdev->dev;
+	adap->retries = 5;
+	strlcpy(adap->name, dev_name(&serdev->dev), sizeof(adap->name));
+
+	ret = i2c_add_adapter(adap);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+
+err_free:
+	kfree(dev);
+	return ret;
+}
+
+static void max9260_remove(struct serdev_device *serdev)
+{
+	struct max9260_device *dev = serdev_device_get_drvdata(serdev);
+
+	serdev_device_close(dev->serdev);
+
+	kfree(dev);
+}
+
+static const struct of_device_id max9260_dt_ids[] = {
+	{ .compatible = "maxim,max9260" },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, max9260_dt_ids);
+
+static struct serdev_device_driver max9260_driver = {
+	.probe = max9260_probe,
+	.remove = max9260_remove,
+	.driver = {
+		.name = "max9260",
+		.of_match_table = of_match_ptr(max9260_dt_ids),
+	},
+};
+
+module_serdev_device_driver(max9260_driver);
+
+MODULE_DESCRIPTION("Maxim MAX9260 GMSL Deserializer Driver");
+MODULE_AUTHOR("Ulrich Hecht");
+MODULE_LICENSE("GPL");
-- 
2.7.4

[RFC v2 6/6] ARM: dts: blanche: add SCIF1 and MAX9260 deserializer

[Ulrich Hecht]

Adds serial port SCIF1 and the MAX9260 deserializers connected to it.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
---
 arch/arm/boot/dts/r8a7792-blanche.dts | 52 +++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/arch/arm/boot/dts/r8a7792-blanche.dts b/arch/arm/boot/dts/r8a7792-blanche.dts
index 9b67dca..2ae9a87 100644
--- a/arch/arm/boot/dts/r8a7792-blanche.dts
+++ b/arch/arm/boot/dts/r8a7792-blanche.dts
@@ -21,6 +21,7 @@
 	aliases {
 		serial0 = &scif0;
 		serial1 = &scif3;
+		serial2 = &scif1;
 	};
 
 	chosen {
@@ -186,6 +187,16 @@
 		gpio = <&gpio11 12 GPIO_ACTIVE_HIGH>;
 		enable-active-high;
 	};
+
+	mux: mux-controller {
+		compatible = "gpio-mux";
+		#mux-control-cells = <0>;
+
+		mux-gpios = <&gpio5 12 GPIO_ACTIVE_HIGH>,
+			    <&gpio5 13 GPIO_ACTIVE_HIGH>,
+			    <&gpio5 14 GPIO_ACTIVE_HIGH>,
+			    <&gpio5 15 GPIO_ACTIVE_HIGH>;
+	};
 };
 
 &extal_clk {
@@ -202,6 +213,11 @@
 		function = "scif0";
 	};
 
+	scif1_pins: scif1 {
+		groups = "scif1_data";
+		function = "scif1";
+	};
+
 	scif3_pins: scif3 {
 		groups = "scif3_data";
 		function = "scif3";
@@ -246,6 +262,42 @@
 	status = "okay";
 };
 
+&scif1 {
+	pinctrl-0 = <&scif1_pins>;
+	pinctrl-names = "default";
+
+	status = "okay";
+
+	mux-controls = <&mux>;
+
+	#address-cells = <1>;
+	#size-cells = <0>;
+	gmsl-deserializer@0 {
+		compatible = "maxim,max9260";
+		reg = <0x8>;
+	};
+	gmsl-deserializer@1 {
+		compatible = "maxim,max9260";
+		reg = <0x9>;
+	};
+	gmsl-deserializer@2 {
+		compatible = "maxim,max9260";
+		reg = <0xa>;
+	};
+	gmsl-deserializer@3 {
+		compatible = "maxim,max9260";
+		reg = <0xb>;
+	};
+	gmsl-deserializer@4 {
+		compatible = "maxim,max9260";
+		reg = <0x4>;
+	};
+	gmsl-deserializer@5 {
+		compatible = "maxim,max9260";
+		reg = <0x5>;
+	};
+};
+
 &scif3 {
 	pinctrl-0 = <&scif3_pins>;
 	pinctrl-names = "default";
-- 
2.7.4

Re: [RFC v2 5/6] max9260: add driver for i2c over GMSL passthrough

[Geert Uytterhoeven]

Hi Ulrich,

On Mon, Jul 17, 2017 at 5:24 PM, Ulrich Hecht
<ulrich.hecht+renesas@gmail.com> wrote:
> This driver implements tunnelling of i2c requests over GMSL via a
> MAX9260 deserializer. It provides an i2c adapter that can be used
> to reach devices on the far side of the link.
>
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/media/i2c/max9260.c
> @@ -0,0 +1,288 @@

> +static void max9260_transact(struct max9260_device *dev,
> +                            int expect,
> +                            u8 *request, int len)

const u8 *request (or const void *?)

> +static int max9260_read_reg(struct max9260_device *dev, int reg)
> +{
> +       u8 request[] = { 0x79, 0x91, reg, 1 };

static const

I don't know if this buffer is ever copied. If not, perhaps it should not be
on the stack anyway because some serial drivers may not support DMA
from stack buffers?
This applies to all buffers below.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC v2 6/6] ARM: dts: blanche: add SCIF1 and MAX9260 deserializer

[Geert Uytterhoeven]

Hi Uli,

On Mon, Jul 17, 2017 at 5:24 PM, Ulrich Hecht
<ulrich.hecht+renesas@gmail.com> wrote:
> Adds serial port SCIF1 and the MAX9260 deserializers connected to it.
>
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>

Thanks for your patch!

>  arch/arm/boot/dts/r8a7792-blanche.dts | 52 +++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>
> diff --git a/arch/arm/boot/dts/r8a7792-blanche.dts b/arch/arm/boot/dts/r8a7792-blanche.dts
> index 9b67dca..2ae9a87 100644
> --- a/arch/arm/boot/dts/r8a7792-blanche.dts
> +++ b/arch/arm/boot/dts/r8a7792-blanche.dts

>         status = "okay";
>  };
>
> +&scif1 {
> +       pinctrl-0 = <&scif1_pins>;
> +       pinctrl-names = "default";
> +
> +       status = "okay";
> +
> +       mux-controls = <&mux>;
> +
> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +       gmsl-deserializer@0 {
> +               compatible = "maxim,max9260";
> +               reg = <0x8>;

unit address and reg property don't match.

(try "make dtbs W=1")

Gr{oetje,eeting}s,

                        Geert

Re: [RFC v2 6/6] ARM: dts: blanche: add SCIF1 and MAX9260 deserializer

[Geert Uytterhoeven]

Hi Uli,

On Mon, Jul 17, 2017 at 5:24 PM, Ulrich Hecht
<ulrich.hecht+renesas@gmail.com> wrote:
> Adds serial port SCIF1 and the MAX9260 deserializers connected to it.
>
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>

Thanks for your patch!

>  arch/arm/boot/dts/r8a7792-blanche.dts | 52 +++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>
> diff --git a/arch/arm/boot/dts/r8a7792-blanche.dts b/arch/arm/boot/dts/r8a7792-blanche.dts
> index 9b67dca..2ae9a87 100644
> --- a/arch/arm/boot/dts/r8a7792-blanche.dts
> +++ b/arch/arm/boot/dts/r8a7792-blanche.dts

>         status = "okay";
>  };
>
> +&scif1 {
> +       pinctrl-0 = <&scif1_pins>;
> +       pinctrl-names = "default";
> +
> +       status = "okay";
> +
> +       mux-controls = <&mux>;
> +
> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +       gmsl-deserializer@0 {
> +               compatible = "maxim,max9260";
> +               reg = <0x8>;

unit address and reg property don't match.

(try "make dtbs W=1")

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC v2 3/6] serdev: add multiplexer support

[Rob Herring]

On Mon, Jul 17, 2017 at 10:24 AM, Ulrich Hecht
<ulrich.hecht+renesas@gmail.com> wrote:
> Adds an interface for slave device multiplexing using the mux subsystem.
>
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
>  drivers/tty/serdev/Kconfig  |  3 +++
>  drivers/tty/serdev/Makefile |  1 +
>  drivers/tty/serdev/core.c   | 14 +++++++++-
>  drivers/tty/serdev/mux.c    | 66 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/serdev.h      | 16 ++++++++---
>  5 files changed, 96 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/tty/serdev/mux.c
>
> diff --git a/drivers/tty/serdev/Kconfig b/drivers/tty/serdev/Kconfig
> index cdc6b82..9096b71 100644
> --- a/drivers/tty/serdev/Kconfig
> +++ b/drivers/tty/serdev/Kconfig
> @@ -13,4 +13,7 @@ config SERIAL_DEV_CTRL_TTYPORT
>         depends on TTY
>         depends on SERIAL_DEV_BUS != m
>
> +config SERIAL_DEV_MUX
> +       bool "Serial device multiplexer support"

I think this breaks for modules.

I'm inclined to just keep mux support in the core.

> +
>  endif
> diff --git a/drivers/tty/serdev/Makefile b/drivers/tty/serdev/Makefile
> index 0cbdb94..d713381 100644
> --- a/drivers/tty/serdev/Makefile
> +++ b/drivers/tty/serdev/Makefile
> @@ -1,5 +1,6 @@
>  serdev-objs := core.o
>
>  obj-$(CONFIG_SERIAL_DEV_BUS) += serdev.o
> +obj-$(CONFIG_SERIAL_DEV_MUX) += mux.o
>
>  obj-$(CONFIG_SERIAL_DEV_CTRL_TTYPORT) += serdev-ttyport.o
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index 1fbaa4c..92c5bfa 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -305,7 +305,8 @@ struct serdev_device *serdev_device_alloc(struct serdev_controller *ctrl)
>                 return NULL;
>
>         serdev->ctrl = ctrl;
> -       ctrl->serdev = serdev;
> +       if (!ctrl->serdev)
> +               ctrl->serdev = serdev;
>         device_initialize(&serdev->dev);
>         serdev->dev.parent = &ctrl->dev;
>         serdev->dev.bus = &serdev_bus_type;
> @@ -368,6 +369,8 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
>         struct serdev_device *serdev = NULL;
>         int err;
>         bool found = false;
> +       int nr = 0;
> +       u32 reg;
>
>         for_each_available_child_of_node(ctrl->dev.of_node, node) {
>                 if (!of_get_property(node, "compatible", NULL))
> @@ -380,6 +383,10 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
>                         continue;
>
>                 serdev->dev.of_node = node;
> +               serdev->nr = nr++;
> +
> +               if (!of_property_read_u32(node, "reg", &reg))
> +                       serdev->mux_addr = reg;

Perhaps nr should just be assigned reg value.

>
>                 err = serdev_device_add(serdev);
>                 if (err) {
> @@ -414,6 +421,11 @@ int serdev_controller_add(struct serdev_controller *ctrl)
>         if (ret)
>                 return ret;
>
> +#ifdef CONFIG_SERIAL_DEV_MUX
> +       if (of_serdev_register_mux(ctrl) == -EPROBE_DEFER)
> +               return -EPROBE_DEFER;
> +#endif
> +
>         ret = of_serdev_register_devices(ctrl);
>         if (ret)
>                 goto out_dev_del;
> diff --git a/drivers/tty/serdev/mux.c b/drivers/tty/serdev/mux.c
> new file mode 100644
> index 0000000..fc9405b
> --- /dev/null
> +++ b/drivers/tty/serdev/mux.c
> @@ -0,0 +1,66 @@
> +/*
> + * Copyright (C) 2017 Ulrich Hecht
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/mux/consumer.h>
> +#include <linux/mux/driver.h>
> +#include <linux/of_gpio.h>
> +#include <linux/serdev.h>
> +#include <linux/slab.h>
> +
> +int serdev_device_mux_select(struct serdev_device *serdev)
> +{
> +       struct mux_control *mux = serdev->ctrl->mux;
> +       int ret;
> +
> +       if (!mux)
> +               return -EINVAL;
> +
> +       ret = mux_control_select(mux, serdev->mux_addr);
> +       serdev->ctrl->mux_do_not_deselect = ret < 0;
> +
> +       serdev->ctrl->serdev = serdev;
> +
> +       return ret;
> +}
> +
> +int serdev_device_mux_deselect(struct serdev_device *serdev)
> +{
> +       struct mux_control *mux = serdev->ctrl->mux;
> +
> +       if (!mux)
> +               return -EINVAL;
> +
> +       if (!serdev->ctrl->mux_do_not_deselect)
> +               return mux_control_deselect(mux);
> +       else
> +               return 0;
> +}
> +
> +int of_serdev_register_mux(struct serdev_controller *ctrl)
> +{
> +       struct mux_control *mux;
> +       struct device *dev = &ctrl->dev;
> +
> +       mux = devm_mux_control_get(dev, NULL);
> +
> +       if (IS_ERR(mux)) {
> +               if (PTR_ERR(mux) != -EPROBE_DEFER)
> +                       dev_err(dev, "failed to get control mux\n");
> +               return PTR_ERR(mux);
> +       }
> +
> +       ctrl->mux = mux;
> +
> +       return 0;
> +}
> +
> diff --git a/include/linux/serdev.h b/include/linux/serdev.h
> index 8b67fcd..26d5994 100644
> --- a/include/linux/serdev.h
> +++ b/include/linux/serdev.h
> @@ -17,6 +17,7 @@
>  #include <linux/device.h>
>  #include <linux/termios.h>
>  #include <linux/delay.h>
> +#include <linux/rtmutex.h>
>
>  struct serdev_controller;
>  struct serdev_device;
> @@ -51,6 +52,7 @@ struct serdev_device {
>         const struct serdev_device_ops *ops;
>         struct completion write_comp;
>         struct mutex write_lock;
> +       int mux_addr;
>  };
>
>  static inline struct serdev_device *to_serdev_device(struct device *d)
> @@ -76,6 +78,8 @@ static inline struct serdev_device_driver *to_serdev_device_driver(struct device
>         return container_of(d, struct serdev_device_driver, driver);
>  }
>
> +int of_serdev_register_mux(struct serdev_controller *ctrl);
> +
>  /*
>   * serdev controller structures
>   */
> @@ -97,14 +101,18 @@ struct serdev_controller_ops {
>   * struct serdev_controller - interface to the serdev controller
>   * @dev:       Driver model representation of the device.
>   * @nr:                number identifier for this controller/bus.
> - * @serdev:    Pointer to slave device for this controller.
> + * @serdev:    Pointer to active slave device for this controller.
>   * @ops:       Controller operations.
> + * @mux:       Slave multiplexer control.
> + * @mux_do_not_deselect: Set if slave selection failed.
>   */
>  struct serdev_controller {
>         struct device           dev;
>         unsigned int            nr;
>         struct serdev_device    *serdev;
>         const struct serdev_controller_ops *ops;
> +       struct mux_control      *mux;
> +       int                     mux_do_not_deselect;
>  };
>
>  static inline struct serdev_controller *to_serdev_controller(struct device *d)
> @@ -172,7 +180,7 @@ static inline void serdev_controller_write_wakeup(struct serdev_controller *ctrl
>  {
>         struct serdev_device *serdev = ctrl->serdev;
>
> -       if (!serdev || !serdev->ops->write_wakeup)
> +       if (!serdev || !serdev->ops || !serdev->ops->write_wakeup)

When is ops ever NULL? Should be a separate change AFIACT.

>                 return;
>
>         serdev->ops->write_wakeup(serdev);
> @@ -184,7 +192,7 @@ static inline int serdev_controller_receive_buf(struct serdev_controller *ctrl,
>  {
>         struct serdev_device *serdev = ctrl->serdev;
>
> -       if (!serdev || !serdev->ops->receive_buf)
> +       if (!serdev || !serdev->ops || !serdev->ops->receive_buf)
>                 return -EINVAL;
>
>         return serdev->ops->receive_buf(serdev, data, count);
> @@ -204,6 +212,8 @@ void serdev_device_write_wakeup(struct serdev_device *);
>  int serdev_device_write(struct serdev_device *, const unsigned char *, size_t, unsigned long);
>  void serdev_device_write_flush(struct serdev_device *);
>  int serdev_device_write_room(struct serdev_device *);
> +int serdev_device_mux_select(struct serdev_device *);
> +int serdev_device_mux_deselect(struct serdev_device *);
>
>  /*
>   * serdev device driver functions
> --
> 2.7.4
>

Re: [RFC v2 2/6] serdev: add method to set parity

[Rob Herring]

On Mon, Jul 17, 2017 at 10:24 AM, Ulrich Hecht
<ulrich.hecht+renesas@gmail.com> wrote:
> Adds serdev_device_set_parity() and an implementation for ttyport.
>
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
>  drivers/tty/serdev/core.c           | 12 ++++++++++++
>  drivers/tty/serdev/serdev-ttyport.c | 17 +++++++++++++++++
>  include/linux/serdev.h              |  4 ++++
>  3 files changed, 33 insertions(+)

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [RFC v2 0/6] serdev multiplexing support

[Rob Herring]

On Mon, Jul 17, 2017 at 10:24 AM, Ulrich Hecht
<ulrich.hecht+renesas@gmail.com> wrote:
> Hi!
>
> This is a new attempt to add multiplexer support to serdev. It is now based
> on the mux subsystem, making it more generic than the previous iteration
> ("[RFC 0/4] serdev GPIO-based multiplexing support").
>
> Thanks to reviewers for their comments. This revision incorporates the
> changes suggested as far as they are still applicable, which mostly applies
> to those concerning the MAX9260 i2c adapter driver.
>
> New patches have been added that fix a small issue in the mux include files
> ("mux: include compiler.h from mux/consumer.h"), and implement deferred
> probing of serdev controllers ("serial: core: support deferring serdev
> controller registration"), hopefully correctly.
>
> This series depends on the "pinctrl: sh-pfc: r8a7792: Add SCIF1 pin groups"
> patch as well as v15 of the mux subsystem series ("[PATCH v15 00/13] mux
> controller abstraction and iio/i2c muxes").

Please also document how muxed devices are represented in DT in
serial/slave-device.txt.

Rob

Re: [RFC v2 4/6] serial: core: support deferring serdev controller registration

[Rob Herring]

On Mon, Jul 17, 2017 at 10:24 AM, Ulrich Hecht
<ulrich.hecht+renesas@gmail.com> wrote:
> serdev controllers may depend on other devices (such as multiplexers)
> and thus require deferred probing support.

I wonder if instead of deferring, we could just delay binding the
devices. There's really no reason to defer the controller. This is a
small addition, but deferring involves a lot of undoing and redoing of
initialization.

Rob

Re: [RFC v2 3/6] serdev: add multiplexer support

[Peter Rosin]

On 2017-07-17 17:24, Ulrich Hecht wrote:
> Adds an interface for slave device multiplexing using the mux subsystem.
> 
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
>  drivers/tty/serdev/Kconfig  |  3 +++
>  drivers/tty/serdev/Makefile |  1 +
>  drivers/tty/serdev/core.c   | 14 +++++++++-
>  drivers/tty/serdev/mux.c    | 66 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/serdev.h      | 16 ++++++++---
>  5 files changed, 96 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/tty/serdev/mux.c
> 
> diff --git a/drivers/tty/serdev/Kconfig b/drivers/tty/serdev/Kconfig
> index cdc6b82..9096b71 100644
> --- a/drivers/tty/serdev/Kconfig
> +++ b/drivers/tty/serdev/Kconfig
> @@ -13,4 +13,7 @@ config SERIAL_DEV_CTRL_TTYPORT
>  	depends on TTY
>  	depends on SERIAL_DEV_BUS != m
>  
> +config SERIAL_DEV_MUX
> +	bool "Serial device multiplexer support"

Here, you should

	select MULTIPLEXER

And, instead of adding a config option, you might want to consider
making the mux optional instead. See

https://lkml.org/lkml/2017/7/14/655

> +
>  endif
> diff --git a/drivers/tty/serdev/Makefile b/drivers/tty/serdev/Makefile
> index 0cbdb94..d713381 100644
> --- a/drivers/tty/serdev/Makefile
> +++ b/drivers/tty/serdev/Makefile
> @@ -1,5 +1,6 @@
>  serdev-objs := core.o
>  
>  obj-$(CONFIG_SERIAL_DEV_BUS) += serdev.o
> +obj-$(CONFIG_SERIAL_DEV_MUX) += mux.o
>  
>  obj-$(CONFIG_SERIAL_DEV_CTRL_TTYPORT) += serdev-ttyport.o
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index 1fbaa4c..92c5bfa 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -305,7 +305,8 @@ struct serdev_device *serdev_device_alloc(struct serdev_controller *ctrl)
>  		return NULL;
>  
>  	serdev->ctrl = ctrl;
> -	ctrl->serdev = serdev;
> +	if (!ctrl->serdev)
> +		ctrl->serdev = serdev;
>  	device_initialize(&serdev->dev);
>  	serdev->dev.parent = &ctrl->dev;
>  	serdev->dev.bus = &serdev_bus_type;
> @@ -368,6 +369,8 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
>  	struct serdev_device *serdev = NULL;
>  	int err;
>  	bool found = false;
> +	int nr = 0;
> +	u32 reg;
>  
>  	for_each_available_child_of_node(ctrl->dev.of_node, node) {
>  		if (!of_get_property(node, "compatible", NULL))
> @@ -380,6 +383,10 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
>  			continue;
>  
>  		serdev->dev.of_node = node;
> +		serdev->nr = nr++;
> +
> +		if (!of_property_read_u32(node, "reg", &reg))
> +			serdev->mux_addr = reg;
>  
>  		err = serdev_device_add(serdev);
>  		if (err) {
> @@ -414,6 +421,11 @@ int serdev_controller_add(struct serdev_controller *ctrl)
>  	if (ret)
>  		return ret;
>  
> +#ifdef CONFIG_SERIAL_DEV_MUX
> +	if (of_serdev_register_mux(ctrl) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +#endif
> +
>  	ret = of_serdev_register_devices(ctrl);
>  	if (ret)
>  		goto out_dev_del;
> diff --git a/drivers/tty/serdev/mux.c b/drivers/tty/serdev/mux.c
> new file mode 100644
> index 0000000..fc9405b
> --- /dev/null
> +++ b/drivers/tty/serdev/mux.c
> @@ -0,0 +1,66 @@
> +/*
> + * Copyright (C) 2017 Ulrich Hecht
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/mux/consumer.h>
> +#include <linux/mux/driver.h>
> +#include <linux/of_gpio.h>
> +#include <linux/serdev.h>
> +#include <linux/slab.h>
> +
> +int serdev_device_mux_select(struct serdev_device *serdev)
> +{
> +	struct mux_control *mux = serdev->ctrl->mux;
> +	int ret;
> +
> +	if (!mux)
> +		return -EINVAL;
> +
> +	ret = mux_control_select(mux, serdev->mux_addr);
> +	serdev->ctrl->mux_do_not_deselect = ret < 0;
> +
> +	serdev->ctrl->serdev = serdev;
> +
> +	return ret;
> +}
> +
> +int serdev_device_mux_deselect(struct serdev_device *serdev)
> +{
> +	struct mux_control *mux = serdev->ctrl->mux;
> +
> +	if (!mux)
> +		return -EINVAL;
> +
> +	if (!serdev->ctrl->mux_do_not_deselect)
> +		return mux_control_deselect(mux);
> +	else
> +		return 0;
> +}
> +
> +int of_serdev_register_mux(struct serdev_controller *ctrl)
> +{
> +	struct mux_control *mux;
> +	struct device *dev = &ctrl->dev;
> +
> +	mux = devm_mux_control_get(dev, NULL);
> +
> +	if (IS_ERR(mux)) {
> +		if (PTR_ERR(mux) != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get control mux\n");
> +		return PTR_ERR(mux);
> +	}
> +
> +	ctrl->mux = mux;
> +
> +	return 0;
> +}
> +
> diff --git a/include/linux/serdev.h b/include/linux/serdev.h
> index 8b67fcd..26d5994 100644
> --- a/include/linux/serdev.h
> +++ b/include/linux/serdev.h
> @@ -17,6 +17,7 @@
>  #include <linux/device.h>
>  #include <linux/termios.h>
>  #include <linux/delay.h>
> +#include <linux/rtmutex.h>
>  
>  struct serdev_controller;
>  struct serdev_device;
> @@ -51,6 +52,7 @@ struct serdev_device {
>  	const struct serdev_device_ops *ops;
>  	struct completion write_comp;
>  	struct mutex write_lock;
> +	int mux_addr;
>  };
>  
>  static inline struct serdev_device *to_serdev_device(struct device *d)
> @@ -76,6 +78,8 @@ static inline struct serdev_device_driver *to_serdev_device_driver(struct device
>  	return container_of(d, struct serdev_device_driver, driver);
>  }
>  
> +int of_serdev_register_mux(struct serdev_controller *ctrl);
> +
>  /*
>   * serdev controller structures
>   */
> @@ -97,14 +101,18 @@ struct serdev_controller_ops {
>   * struct serdev_controller - interface to the serdev controller
>   * @dev:	Driver model representation of the device.
>   * @nr:		number identifier for this controller/bus.
> - * @serdev:	Pointer to slave device for this controller.
> + * @serdev:	Pointer to active slave device for this controller.
>   * @ops:	Controller operations.
> + * @mux:	Slave multiplexer control.
> + * @mux_do_not_deselect: Set if slave selection failed.
>   */
>  struct serdev_controller {
>  	struct device		dev;
>  	unsigned int		nr;
>  	struct serdev_device	*serdev;
>  	const struct serdev_controller_ops *ops;
> +	struct mux_control	*mux;
> +	int			mux_do_not_deselect;
>  };
>  
>  static inline struct serdev_controller *to_serdev_controller(struct device *d)
> @@ -172,7 +180,7 @@ static inline void serdev_controller_write_wakeup(struct serdev_controller *ctrl
>  {
>  	struct serdev_device *serdev = ctrl->serdev;
>  
> -	if (!serdev || !serdev->ops->write_wakeup)
> +	if (!serdev || !serdev->ops || !serdev->ops->write_wakeup)
>  		return;
>  
>  	serdev->ops->write_wakeup(serdev);
> @@ -184,7 +192,7 @@ static inline int serdev_controller_receive_buf(struct serdev_controller *ctrl,
>  {
>  	struct serdev_device *serdev = ctrl->serdev;
>  
> -	if (!serdev || !serdev->ops->receive_buf)
> +	if (!serdev || !serdev->ops || !serdev->ops->receive_buf)
>  		return -EINVAL;
>  
>  	return serdev->ops->receive_buf(serdev, data, count);
> @@ -204,6 +212,8 @@ void serdev_device_write_wakeup(struct serdev_device *);
>  int serdev_device_write(struct serdev_device *, const unsigned char *, size_t, unsigned long);
>  void serdev_device_write_flush(struct serdev_device *);
>  int serdev_device_write_room(struct serdev_device *);
> +int serdev_device_mux_select(struct serdev_device *);
> +int serdev_device_mux_deselect(struct serdev_device *);
>  
>  /*
>   * serdev device driver functions
> 

Re: [RFC v2 5/6] max9260: add driver for i2c over GMSL passthrough

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1413 bytes --]

Hi Uli,

> +struct max9260_device {
> +	struct serdev_device *serdev;
> +	u8 *rx_buf;
> +	int rx_len;
> +	int rx_state;
> +	wait_queue_head_t rx_wq;
> +	struct i2c_adapter adap;
> +};
> +
> +static void wait_for_transaction(struct max9260_device *dev)

max9260_ prefix as well?

> +{
> +	wait_event_interruptible_timeout(dev->rx_wq,
> +		dev->rx_state <= RX_FRAME_ERROR,
> +		HZ/2);

I'd suggest to drop the interruptible. It can be done but it is usually
not trivial to abort the operation gracefully when a signal comes in.

Also, timeout is superfluous since you don't get the return value?

> +static int max9260_setup(struct max9260_device *dev)
> +{
> +	int ret;
> +
> +	ret = max9260_read_reg(dev, 0x1e);
> +
> +	if (ret != 0x02) {
> +		dev_err(&dev->serdev->dev,
> +			"device does not identify as MAX9260\n");
> +		return -EINVAL;

I think -ENODEV is the proper errno for a not-found device. Also, the
error message could probably go.

> +	}
> +
> +	return 0;
> +}
> +
> +static void max9260_uart_write_wakeup(struct serdev_device *serdev)
> +{

Maybe a FIXME comment for this empty function?

> +static u32 max9260_i2c_func(struct i2c_adapter *adapter)
> +{
> +	return I2C_FUNC_SMBUS_BYTE|I2C_FUNC_SMBUS_BYTE_DATA;

Spaces around operators.

> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;

You don't like devm_* it seems ;)

Rest looks good to me!

Regards,

   Wolfram

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC v2 1/6] mux: include compiler.h from mux/consumer.h

[Peter Rosin]

Hi Ulrich,

This patch looks good to me. Let me know if I should feed this
through the mux subsystem or if it will take some other route.

In case someone else ends up taking it:
Acked-by: Peter Rosin <peda@axentia.se>

Cheers,
Peter

On 2017-07-17 17:24, Ulrich Hecht wrote:
> Required for __must_check.
> 
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
>  include/linux/mux/consumer.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
> index 5577e1b..ea96d4c 100644
> --- a/include/linux/mux/consumer.h
> +++ b/include/linux/mux/consumer.h
> @@ -13,6 +13,8 @@
>  #ifndef _LINUX_MUX_CONSUMER_H
>  #define _LINUX_MUX_CONSUMER_H
>  
> +#include <linux/compiler.h>
> +
>  struct device;
>  struct mux_control;
>  
> 

Re: [RFC v2 1/6] mux: include compiler.h from mux/consumer.h

[Ulrich Hecht]

On Mon, Jul 31, 2017 at 11:02 AM, Peter Rosin <peda@axentia.se> wrote:
> Hi Ulrich,
>
> This patch looks good to me. Let me know if I should feed this
> through the mux subsystem or if it will take some other route.

Please pick it up, thanks.

> In case someone else ends up taking it:
> Acked-by: Peter Rosin <peda@axentia.se>

CU
Uli

Re: [RFC v2 2/6] serdev: add method to set parity

[Laurent Pinchart]

Hi Ulrich,

Thank you for the patch.

On Monday 17 Jul 2017 17:24:32 Ulrich Hecht wrote:
> Adds serdev_device_set_parity() and an implementation for ttyport.
> 
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
>  drivers/tty/serdev/core.c           | 12 ++++++++++++
>  drivers/tty/serdev/serdev-ttyport.c | 17 +++++++++++++++++
>  include/linux/serdev.h              |  4 ++++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index f71b473..1fbaa4c 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -242,6 +242,18 @@ int serdev_device_set_tiocm(struct serdev_device
> *serdev, int set, int clear) }
>  EXPORT_SYMBOL_GPL(serdev_device_set_tiocm);
> 
> +int serdev_device_set_parity(struct serdev_device *serdev,
> +			     bool enable, bool odd)

Code calling this function will be difficult to read:

	serdev_device_set_partity(dev, true, false);

Without looking up the function definition, the last two arguments are not 
very explicit. How about replacing them with a disabled, odd, even enum ?

> +{
> +	struct serdev_controller *ctrl = serdev->ctrl;
> +
> +	if (!ctrl || !ctrl->ops->set_parity)
> +		return -ENOTSUPP;
> +
> +	return ctrl->ops->set_parity(ctrl, enable, odd);
> +}
> +EXPORT_SYMBOL_GPL(serdev_device_set_parity);
> +
>  static int serdev_drv_probe(struct device *dev)
>  {
>  	const struct serdev_device_driver *sdrv =
> to_serdev_device_driver(dev->driver); diff --git
> a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
> index 302018d..9114956 100644
> --- a/drivers/tty/serdev/serdev-ttyport.c
> +++ b/drivers/tty/serdev/serdev-ttyport.c
> @@ -195,6 +195,22 @@ static int ttyport_set_tiocm(struct serdev_controller
> *ctrl, unsigned int set, u return tty->driver->ops->tiocmset(tty, set,
> clear);
>  }
> 
> +static int ttyport_set_parity(struct serdev_controller *ctrl,
> +			      bool enable, bool odd)
> +{
> +	struct serport *serport = serdev_controller_get_drvdata(ctrl);
> +	struct tty_struct *tty = serport->tty;
> +	struct ktermios ktermios = tty->termios;
> +
> +	ktermios.c_cflag &= ~(PARENB | PARODD);
> +	if (enable)
> +		ktermios.c_cflag |= PARENB;
> +	if (odd)
> +		ktermios.c_cflag |= PARODD;
> +
> +	return tty_set_termios(tty, &ktermios);
> +}
> +
>  static const struct serdev_controller_ops ctrl_ops = {
>  	.write_buf = ttyport_write_buf,
>  	.write_flush = ttyport_write_flush,
> @@ -206,6 +222,7 @@ static const struct serdev_controller_ops ctrl_ops = {
>  	.wait_until_sent = ttyport_wait_until_sent,
>  	.get_tiocm = ttyport_get_tiocm,
>  	.set_tiocm = ttyport_set_tiocm,
> +	.set_parity = ttyport_set_parity,
>  };
> 
>  struct device *serdev_tty_port_register(struct tty_port *port,
> diff --git a/include/linux/serdev.h b/include/linux/serdev.h
> index e69402d..8b67fcd 100644
> --- a/include/linux/serdev.h
> +++ b/include/linux/serdev.h
> @@ -90,6 +90,7 @@ struct serdev_controller_ops {
>  	void (*wait_until_sent)(struct serdev_controller *, long);
>  	int (*get_tiocm)(struct serdev_controller *);
>  	int (*set_tiocm)(struct serdev_controller *, unsigned int, unsigned 
int);
> +	int (*set_parity)(struct serdev_controller *, bool, bool);
>  };
> 
>  /**
> @@ -298,6 +299,9 @@ static inline int serdev_device_set_rts(struct
> serdev_device *serdev, bool enabl return serdev_device_set_tiocm(serdev, 0,
> TIOCM_RTS);
>  }
> 
> +int serdev_device_set_parity(struct serdev_device *serdev,
> +			     bool enable, bool odd);
> +
>  /*
>   * serdev hooks into TTY core
>   */

-- 
Regards,

Laurent Pinchart

Re: [RFC v2 5/6] max9260: add driver for i2c over GMSL passthrough

[Laurent Pinchart]

Hi Ulrich,

Thank you for the patch.

On Monday 17 Jul 2017 17:24:35 Ulrich Hecht wrote:
> This driver implements tunnelling of i2c requests over GMSL via a
> MAX9260 deserializer. It provides an i2c adapter that can be used
> to reach devices on the far side of the link.
> 
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
>  drivers/media/i2c/Kconfig   |   6 +
>  drivers/media/i2c/Makefile  |   1 +
>  drivers/media/i2c/max9260.c | 288 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 295 insertions(+)
>  create mode 100644 drivers/media/i2c/max9260.c

[snip]

> diff --git a/drivers/media/i2c/max9260.c b/drivers/media/i2c/max9260.c
> new file mode 100644
> index 0000000..ca34e67
> --- /dev/null
> +++ b/drivers/media/i2c/max9260.c
> @@ -0,0 +1,288 @@
> +/*
> + * Maxim MAX9260 GMSL Deserializer Driver
> + *
> + * Copyright (C) 2017 Ulrich Hecht
> + *
> + * 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/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/serdev.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +
> +#define SYNC	0x79
> +#define ACK	0xc3
> +
> +#define RX_FINISHED		0
> +#define RX_FRAME_ERROR		1
> +#define RX_EXPECT_ACK		2
> +#define RX_EXPECT_ACK_DATA	3
> +#define RX_EXPECT_DATA		4

Should you make this an enum and update the type of max9260_device::rx_state 
accordingly ?

> +
> +struct max9260_device {
> +	struct serdev_device *serdev;
> +	u8 *rx_buf;
> +	int rx_len;

rx_len can't be negative, you can make it an unsigned int.

> +	int rx_state;
> +	wait_queue_head_t rx_wq;
> +	struct i2c_adapter adap;
> +};
> +
> +static void wait_for_transaction(struct max9260_device *dev)
> +{
> +	wait_event_interruptible_timeout(dev->rx_wq,
> +		dev->rx_state <= RX_FRAME_ERROR,
> +		HZ/2);
> +}
> +
> +static void max9260_transact(struct max9260_device *dev,
> +			     int expect,
> +			     u8 *request, int len)
> +{
> +	serdev_device_mux_select(dev->serdev);
> +
> +	serdev_device_set_baudrate(dev->serdev, 115200);
> +	serdev_device_set_parity(dev->serdev, 1, 0);
> +
> +	dev->rx_state = expect;
> +	serdev_device_write_buf(dev->serdev, request, len);
> +
> +	wait_for_transaction(dev);
> +
> +	serdev_device_mux_deselect(dev->serdev);

You will end up selecting and deselecting the multiplexer around every I2C 
transaction, which can be quite inefficient when initializing devices on the 
other side of the link that require hundreds of I2C writes.

Fixing this could be considered as a future optimization, but it will likely 
involve changes in the serdev mux API, so I think it should be considered now 
already, even if not fully implemented in this driver.

> +}
> +
> +static int max9260_read_reg(struct max9260_device *dev, int reg)
> +{
> +	u8 request[] = { 0x79, 0x91, reg, 1 };
> +	u8 rx;
> +
> +	dev->rx_len = 1;
> +	dev->rx_buf = &rx;
> +
> +	max9260_transact(dev, RX_EXPECT_ACK_DATA, request, 4);

s/4/ARRAY_SIZE(request)/

Passing part of the transaction through function arguments and part through 
the max9260_device structure seems weird. I think you should pass both the 
output and input buffers and lengths as arguments, and store the information 
you need in max9260_device within max9260_transact().

> +	if (dev->rx_state == RX_FINISHED)
> +		return rx;
> +
> +	return -EIO;
> +}
> +
> +static int max9260_setup(struct max9260_device *dev)
> +{
> +	int ret;
> +
> +	ret = max9260_read_reg(dev, 0x1e);

No magic values, please use macros.

> +
> +	if (ret != 0x02) {

Ditto here and in multiple locations below.

> +		dev_err(&dev->serdev->dev,
> +			"device does not identify as MAX9260\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void max9260_uart_write_wakeup(struct serdev_device *serdev)
> +{
> +}
> +
> +static int max9260_uart_receive_buf(struct serdev_device *serdev,
> +				    const u8 *data, size_t count)
> +{
> +	struct max9260_device *dev = serdev_device_get_drvdata(serdev);
> +	int accepted;

This can't be negative, you can make it an unsigned int.

> +
> +	switch (dev->rx_state) {
> +	case RX_FINISHED:
> +		dev_dbg(&dev->serdev->dev, "excess data ignored\n");
> +		return count;
> +
> +	case RX_EXPECT_ACK:
> +	case RX_EXPECT_ACK_DATA:
> +		if (data[0] != ACK) {
> +			dev_dbg(&dev->serdev->dev, "frame error");
> +			dev->rx_state = RX_FRAME_ERROR;
> +			wake_up_interruptible(&dev->rx_wq);
> +			return 1;
> +		}
> +
> +		if (dev->rx_state == RX_EXPECT_ACK_DATA) {
> +			dev->rx_state = RX_EXPECT_DATA;
> +		} else {
> +			dev->rx_state = RX_FINISHED;
> +			wake_up_interruptible(&dev->rx_wq);
> +		}
> +		return 1;
> +
> +	case RX_EXPECT_DATA:
> +		accepted = dev->rx_len < count ? dev->rx_len : count;

accepted = min(dev->rx_len, count);

> +
> +		memcpy(dev->rx_buf, data, accepted);
> +
> +		dev->rx_len -= accepted;
> +		dev->rx_buf += accepted;
> +
> +		if (!dev->rx_len) {
> +			dev->rx_state = RX_FINISHED;
> +			wake_up_interruptible(&dev->rx_wq);
> +		}
> +
> +		return accepted;
> +
> +	case RX_FRAME_ERROR:
> +		dev_dbg(&dev->serdev->dev, "%d bytes ignored\n", count);
> +		return count;
> +
> +	}
> +	return 0;
> +}
> +
> +struct serdev_device_ops max9260_serdev_client_ops = {

static const ?

> +	.receive_buf = max9260_uart_receive_buf,
> +	.write_wakeup = max9260_uart_write_wakeup,
> +};
> +
> +static u32 max9260_i2c_func(struct i2c_adapter *adapter)
> +{
> +	return I2C_FUNC_SMBUS_BYTE|I2C_FUNC_SMBUS_BYTE_DATA;
> +}
> +
> +static s32 max9260_smbus_xfer(struct i2c_adapter *adap, u16 addr,
> +	unsigned short flags, char read_write, u8 command, int size,
> +	union i2c_smbus_data *data)
> +{
> +	u8 request[] = { SYNC,
> +			 (addr << 1) + (read_write == I2C_SMBUS_READ),
> +			 command, 0, 0 };
> +	struct max9260_device *dev = i2c_get_adapdata(adap);
> +
> +	switch (size) {
> +	case I2C_SMBUS_BYTE:
> +		if (read_write == I2C_SMBUS_WRITE) {
> +			max9260_transact(dev, RX_EXPECT_ACK, request, 4);
> +			dev_dbg(&adap->dev,
> +				"smbus byte - addr 0x%02x, wrote 0x%02x.\n",
> +				addr, command);
> +		} else {
> +			/* TBD */
> +			return -EOPNOTSUPP;
> +		}
> +		break;
> +
> +	case I2C_SMBUS_BYTE_DATA:
> +		request[3] = 1;
> +		if (read_write == I2C_SMBUS_WRITE) {
> +			request[4] = data->byte;
> +			max9260_transact(dev, RX_EXPECT_ACK, request, 5);
> +			dev_dbg(&adap->dev,
> +				"smbus byte data - addr 0x%02x, wrote 0x%02x 
at 0x%02x.\n",
> +				addr, data->byte, command);
> +		} else {
> +			dev->rx_len = 1;
> +			dev->rx_buf = &data->byte;
> +			max9260_transact(dev, RX_EXPECT_ACK_DATA, request, 4);
> +			dev_dbg(&adap->dev,
> +				"smbus byte data - addr 0x%02x, read  0x%02x 
at 0x%02x.\n",
> +				addr, data->byte, command);
> +		}
> +		break;

Missing blank line.

> +	default:
> +		dev_dbg(&adap->dev,
> +			"Unsupported I2C/SMBus command %d\n", size);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (dev->rx_state != RX_FINISHED) {
> +		dev_dbg(&adap->dev, "xfer timed out\n");
> +		return -EIO;

The comment states timed out but the return value corresponds to an I/O error. 
Which one should it be ?

> +	}
> +
> +	return 0;
> +}
> +
> +static const struct i2c_algorithm max9260_i2c_algorithm = {
> +	.functionality	= max9260_i2c_func,
> +	.smbus_xfer	= max9260_smbus_xfer,
> +};
> +
> +static int max9260_probe(struct serdev_device *serdev)
> +{
> +	struct max9260_device *dev;
> +	struct i2c_adapter *adap;
> +	int ret;
> +
> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	init_waitqueue_head(&dev->rx_wq);
> +
> +	dev->serdev = serdev;
> +	serdev_device_open(serdev);
> +	serdev_device_set_drvdata(serdev, dev);
> +
> +	serdev_device_set_client_ops(serdev, &max9260_serdev_client_ops);
> +
> +	ret = max9260_setup(dev);
> +	if (ret < 0)
> +		goto err_free;
> +
> +	adap = &dev->adap;
> +	i2c_set_adapdata(adap, dev);
> +
> +	adap->owner = THIS_MODULE;
> +	adap->algo = &max9260_i2c_algorithm;
> +	adap->dev.parent = &serdev->dev;
> +	adap->retries = 5;
> +	strlcpy(adap->name, dev_name(&serdev->dev), sizeof(adap->name));
> +
> +	ret = i2c_add_adapter(adap);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +
> +err_free:

No need for a serdev_device_close() ?

> +	kfree(dev);
> +	return ret;
> +}
> +
> +static void max9260_remove(struct serdev_device *serdev)
> +{
> +	struct max9260_device *dev = serdev_device_get_drvdata(serdev);
> +
> +	serdev_device_close(dev->serdev);
> +
> +	kfree(dev);
> +}
> +
> +static const struct of_device_id max9260_dt_ids[] = {
> +	{ .compatible = "maxim,max9260" },
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(of, max9260_dt_ids);
> +
> +static struct serdev_device_driver max9260_driver = {
> +	.probe = max9260_probe,
> +	.remove = max9260_remove,
> +	.driver = {
> +		.name = "max9260",
> +		.of_match_table = of_match_ptr(max9260_dt_ids),
> +	},
> +};
> +
> +module_serdev_device_driver(max9260_driver);
> +
> +MODULE_DESCRIPTION("Maxim MAX9260 GMSL Deserializer Driver");
> +MODULE_AUTHOR("Ulrich Hecht");
> +MODULE_LICENSE("GPL");

-- 
Regards,

Laurent Pinchart

Re: [RFC v2 6/6] ARM: dts: blanche: add SCIF1 and MAX9260 deserializer

[Laurent Pinchart]

Hi Ulrich,

Thank you for the patch.

On Monday 17 Jul 2017 17:24:36 Ulrich Hecht wrote:
> Adds serial port SCIF1 and the MAX9260 deserializers connected to it.
> 
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
>  arch/arm/boot/dts/r8a7792-blanche.dts | 52 +++++++++++++++++++++++++++++++

You're probably aware of this already, but you need to document the DT 
bindings :-)

>  1 file changed, 52 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/r8a7792-blanche.dts
> b/arch/arm/boot/dts/r8a7792-blanche.dts index 9b67dca..2ae9a87 100644
> --- a/arch/arm/boot/dts/r8a7792-blanche.dts
> +++ b/arch/arm/boot/dts/r8a7792-blanche.dts
> @@ -21,6 +21,7 @@
>  	aliases {
>  		serial0 = &scif0;
>  		serial1 = &scif3;
> +		serial2 = &scif1;
>  	};
> 
>  	chosen {
> @@ -186,6 +187,16 @@
>  		gpio = <&gpio11 12 GPIO_ACTIVE_HIGH>;
>  		enable-active-high;
>  	};
> +
> +	mux: mux-controller {
> +		compatible = "gpio-mux";
> +		#mux-control-cells = <0>;
> +
> +		mux-gpios = <&gpio5 12 GPIO_ACTIVE_HIGH>,
> +			    <&gpio5 13 GPIO_ACTIVE_HIGH>,
> +			    <&gpio5 14 GPIO_ACTIVE_HIGH>,
> +			    <&gpio5 15 GPIO_ACTIVE_HIGH>;
> +	};
>  };
> 
>  &extal_clk {
> @@ -202,6 +213,11 @@
>  		function = "scif0";
>  	};
> 
> +	scif1_pins: scif1 {
> +		groups = "scif1_data";
> +		function = "scif1";
> +	};
> +
>  	scif3_pins: scif3 {
>  		groups = "scif3_data";
>  		function = "scif3";
> @@ -246,6 +262,42 @@
>  	status = "okay";
>  };
> 
> +&scif1 {
> +	pinctrl-0 = <&scif1_pins>;
> +	pinctrl-names = "default";
> +
> +	status = "okay";
> +
> +	mux-controls = <&mux>;
> +
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	gmsl-deserializer@0 {
> +		compatible = "maxim,max9260";
> +		reg = <0x8>;
> +	};
> +	gmsl-deserializer@1 {
> +		compatible = "maxim,max9260";
> +		reg = <0x9>;
> +	};
> +	gmsl-deserializer@2 {
> +		compatible = "maxim,max9260";
> +		reg = <0xa>;
> +	};
> +	gmsl-deserializer@3 {
> +		compatible = "maxim,max9260";
> +		reg = <0xb>;
> +	};
> +	gmsl-deserializer@4 {
> +		compatible = "maxim,max9260";
> +		reg = <0x4>;
> +	};
> +	gmsl-deserializer@5 {
> +		compatible = "maxim,max9260";
> +		reg = <0x5>;
> +	};
> +};
> +
>  &scif3 {
>  	pinctrl-0 = <&scif3_pins>;
>  	pinctrl-names = "default";

-- 
Regards,

Laurent Pinchart

Re: [RFC v2 1/6] mux: include compiler.h from mux/consumer.h

[Laurent Pinchart]

Hi Ulrich,

Thank you for the patch.

On Monday 17 Jul 2017 17:24:31 Ulrich Hecht wrote:
> Required for __must_check.
> 
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  include/linux/mux/consumer.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
> index 5577e1b..ea96d4c 100644
> --- a/include/linux/mux/consumer.h
> +++ b/include/linux/mux/consumer.h
> @@ -13,6 +13,8 @@
>  #ifndef _LINUX_MUX_CONSUMER_H
>  #define _LINUX_MUX_CONSUMER_H
> 
> +#include <linux/compiler.h>
> +
>  struct device;
>  struct mux_control;

-- 
Regards,

Laurent Pinchart

Re: [RFC v2 5/6] max9260: add driver for i2c over GMSL passthrough

[Ulrich Hecht]

On Mon, Jul 31, 2017 at 1:13 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> +static int max9260_probe(struct serdev_device *serdev)
>> +{
>> +     struct max9260_device *dev;
>> +     struct i2c_adapter *adap;
>> +     int ret;
>> +
>> +     dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>> +     if (!dev)
>> +             return -ENOMEM;
>> +
>> +     init_waitqueue_head(&dev->rx_wq);
>> +
>> +     dev->serdev = serdev;
>> +     serdev_device_open(serdev);
>> +     serdev_device_set_drvdata(serdev, dev);
>> +
>> +     serdev_device_set_client_ops(serdev, &max9260_serdev_client_ops);
>> +
>> +     ret = max9260_setup(dev);
>> +     if (ret < 0)
>> +             goto err_free;
>> +
>> +     adap = &dev->adap;
>> +     i2c_set_adapdata(adap, dev);
>> +
>> +     adap->owner = THIS_MODULE;
>> +     adap->algo = &max9260_i2c_algorithm;
>> +     adap->dev.parent = &serdev->dev;
>> +     adap->retries = 5;
>> +     strlcpy(adap->name, dev_name(&serdev->dev), sizeof(adap->name));
>> +
>> +     ret = i2c_add_adapter(adap);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     return 0;
>> +
>> +err_free:
>
> No need for a serdev_device_close() ?

serdev_device_{open,close}() actually operate on the controller, so
that would pull the rug from under the other devices connected to that
controller.

CU
Uli

Re: [RFC v2 3/6] serdev: add multiplexer support

[Ulrich Hecht]

On Wed, Jul 19, 2017 at 1:06 AM, Rob Herring <robh@kernel.org> wrote:
>>  static inline struct serdev_controller *to_serdev_controller(struct device *d)
>> @@ -172,7 +180,7 @@ static inline void serdev_controller_write_wakeup(struct serdev_controller *ctrl
>>  {
>>         struct serdev_device *serdev = ctrl->serdev;
>>
>> -       if (!serdev || !serdev->ops->write_wakeup)
>> +       if (!serdev || !serdev->ops || !serdev->ops->write_wakeup)
>
> When is ops ever NULL? Should be a separate change AFIACT.

I dumped it; IIRC that happened once during development due to a bug elsewhere.

CU
Uli

Re: [RFC v2 5/6] max9260: add driver for i2c over GMSL passthrough

[Ulrich Hecht]

On Wed, Jul 19, 2017 at 5:00 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> +{
>> +     wait_event_interruptible_timeout(dev->rx_wq,
>> +             dev->rx_state <= RX_FRAME_ERROR,
>> +             HZ/2);
>
> I'd suggest to drop the interruptible. It can be done but it is usually
> not trivial to abort the operation gracefully when a signal comes in.
>
> Also, timeout is superfluous since you don't get the return value?

Be that as it may, I still want a timeout; wouldn't wait_event() block forever?

CU
Uli

Re: [RFC v2 5/6] max9260: add driver for i2c over GMSL passthrough

[Laurent Pinchart]

Hi Ulrich,

On Wednesday 16 Aug 2017 15:23:27 Ulrich Hecht wrote:
> On Mon, Jul 31, 2017 at 1:13 PM, Laurent Pinchart wrote:
> >> +static int max9260_probe(struct serdev_device *serdev)
> >> +{
> >> +     struct max9260_device *dev;
> >> +     struct i2c_adapter *adap;
> >> +     int ret;
> >> +
> >> +     dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> >> +     if (!dev)
> >> +             return -ENOMEM;
> >> +
> >> +     init_waitqueue_head(&dev->rx_wq);
> >> +
> >> +     dev->serdev = serdev;
> >> +     serdev_device_open(serdev);
> >> +     serdev_device_set_drvdata(serdev, dev);
> >> +
> >> +     serdev_device_set_client_ops(serdev, &max9260_serdev_client_ops);
> >> +
> >> +     ret = max9260_setup(dev);
> >> +     if (ret < 0)
> >> +             goto err_free;
> >> +
> >> +     adap = &dev->adap;
> >> +     i2c_set_adapdata(adap, dev);
> >> +
> >> +     adap->owner = THIS_MODULE;
> >> +     adap->algo = &max9260_i2c_algorithm;
> >> +     adap->dev.parent = &serdev->dev;
> >> +     adap->retries = 5;
> >> +     strlcpy(adap->name, dev_name(&serdev->dev), sizeof(adap->name));
> >> +
> >> +     ret = i2c_add_adapter(adap);
> >> +     if (ret < 0)
> >> +             return ret;
> >> +
> >> +     return 0;
> >> +
> >> +err_free:
> > No need for a serdev_device_close() ?
> 
> serdev_device_{open,close}() actually operate on the controller, so
> that would pull the rug from under the other devices connected to that
> controller.

My point is that you call open in the .probe() handler and close in the 
.remove() handler. If open needs to be balanced by close, it seems to me that 
you should also call close in the error path.

-- 
Regards,

Laurent Pinchart

Re: [RFC v2 5/6] max9260: add driver for i2c over GMSL passthrough

[Laurent Pinchart]

Hi Ulrich,

On Wednesday 16 Aug 2017 15:23:52 Ulrich Hecht wrote:
> On Wed, Jul 19, 2017 at 5:00 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> >> +{
> >> +     wait_event_interruptible_timeout(dev->rx_wq,
> >> +             dev->rx_state <= RX_FRAME_ERROR,
> >> +             HZ/2);
> > 
> > I'd suggest to drop the interruptible. It can be done but it is usually
> > not trivial to abort the operation gracefully when a signal comes in.
> > 
> > Also, timeout is superfluous since you don't get the return value?
> 
> Be that as it may, I still want a timeout; wouldn't wait_event() block
> forever?

I think that Wolfram's point is that you should report the timeout error up to 
the upper layers.

-- 
Regards,

Laurent Pinchart