[PATCH v6 0/5] media: i2c: Add RDACM21 camera module

[PATCH v6 0/5] media: i2c: Add RDACM21 camera module

[Jacopo Mondi]

Hello, this v6 renames the 'maxim,initial-reverse-channel-mV' property
to 'maxim,reverse-channel-microvolt' to use standard suffix as suggested
by Rob.

v5: https://patchwork.linuxtv.org/project/linux-media/list/?series=3876
v4: https://patchwork.linuxtv.org/project/linux-media/list/?series=3847
v3: https://patchwork.linuxtv.org/project/linux-media/list/?series=3657
Background in v1 cover letter:
https://www.spinics.net/lists/linux-renesas-soc/msg52886.html

Jacopo Mondi (5):
  media: i2c: Add driver for RDACM21 camera module
  dt-bindings: media: max9286: Document
    'maxim,reverse-channel-microvolt'
  media: i2c: max9286: Break-out reverse channel setup
  media: i2c: max9286: Make channel amplitude programmable
  media: i2c: max9286: Configure reverse channel amplitude

 .../bindings/media/i2c/maxim,max9286.yaml     |  23 +
 MAINTAINERS                                   |  12 +
 drivers/media/i2c/Kconfig                     |  13 +
 drivers/media/i2c/Makefile                    |   2 +
 drivers/media/i2c/max9286.c                   |  61 +-
 drivers/media/i2c/rdacm21.c                   | 595 ++++++++++++++++++
 6 files changed, 693 insertions(+), 13 deletions(-)
 create mode 100644 drivers/media/i2c/rdacm21.c

--
2.29.2

[PATCH v6 1/5] media: i2c: Add driver for RDACM21 camera module

[Jacopo Mondi]

The RDACM21 is a GMSL camera supporting 1280x1080 resolution images
developed by IMI based on an Omnivision OV10640 sensor, an Omnivision
OV490 ISP and a Maxim MAX9271 GMSL serializer.

The driver uses the max9271 library module, to maximize code reuse with
other camera module drivers using the same serializer, such as rdacm20.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 MAINTAINERS                 |  12 +
 drivers/media/i2c/Kconfig   |  13 +
 drivers/media/i2c/Makefile  |   2 +
 drivers/media/i2c/rdacm21.c | 595 ++++++++++++++++++++++++++++++++++++
 4 files changed, 622 insertions(+)
 create mode 100644 drivers/media/i2c/rdacm21.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 4be038f0a59d..a011df5a14d7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14809,6 +14809,18 @@ F:	drivers/media/i2c/max9271.c
 F:	drivers/media/i2c/max9271.h
 F:	drivers/media/i2c/rdacm20.c
 
+RDACM21 Camera Sensor
+M:	Jacopo Mondi <jacopo+renesas@jmondi.org>
+M:	Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
+M:	Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
+M:	Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
+L:	linux-media@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/media/i2c/rdacm2x-gmsl.yaml
+F:	drivers/media/i2c/max9271.c
+F:	drivers/media/i2c/max9271.h
+F:	drivers/media/i2c/rdacm21.c
+
 RDC R-321X SoC
 M:	Florian Fainelli <florian@openwrt.org>
 S:	Maintained
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 2b9d81e4794a..d500edb8638b 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -1212,6 +1212,19 @@ config VIDEO_RDACM20
 	  This camera should be used in conjunction with a GMSL
 	  deserialiser such as the MAX9286.
 
+config VIDEO_RDACM21
+	tristate "IMI RDACM21 camera support"
+	depends on I2C
+	select V4L2_FWNODE
+	select VIDEO_V4L2_SUBDEV_API
+	select MEDIA_CONTROLLER
+	help
+	  This driver supports the IMI RDACM21 GMSL camera, used in
+	  ADAS systems.
+
+	  This camera should be used in conjunction with a GMSL
+	  deserialiser such as the MAX9286.
+
 config VIDEO_RJ54N1
 	tristate "Sharp RJ54N1CB0C sensor support"
 	depends on I2C && VIDEO_V4L2
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index a3149dce21bb..85b1edc62508 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -124,6 +124,8 @@ obj-$(CONFIG_VIDEO_IMX355)	+= imx355.o
 obj-$(CONFIG_VIDEO_MAX9286)	+= max9286.o
 rdacm20-camera_module-objs	:= rdacm20.o max9271.o
 obj-$(CONFIG_VIDEO_RDACM20)	+= rdacm20-camera_module.o
+rdacm21-camera_module-objs	:= rdacm21.o max9271.o
+obj-$(CONFIG_VIDEO_RDACM21)	+= rdacm21-camera_module.o
 obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
 
 obj-$(CONFIG_SDR_MAX2175) += max2175.o
diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
new file mode 100644
index 000000000000..5f9267e26258
--- /dev/null
+++ b/drivers/media/i2c/rdacm21.c
@@ -0,0 +1,595 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * IMI RDACM21 GMSL Camera Driver
+ *
+ * Copyright (C) 2017-2020 Jacopo Mondi
+ * Copyright (C) 2017-2019 Kieran Bingham
+ * Copyright (C) 2017-2019 Laurent Pinchart
+ * Copyright (C) 2017-2019 Niklas Söderlund
+ * Copyright (C) 2016 Renesas Electronics Corporation
+ * Copyright (C) 2015 Cogent Embedded, Inc.
+ */
+
+#include <linux/delay.h>
+#include <linux/fwnode.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/videodev2.h>
+
+#include <media/v4l2-async.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-subdev.h>
+#include "max9271.h"
+
+#define OV10640_ID_LOW			0xa6
+
+#define OV490_I2C_ADDRESS		0x24
+
+#define OV490_PAGE_HIGH_REG		0xfffd
+#define OV490_PAGE_LOW_REG		0xfffe
+
+#define OV490_DVP_CTRL3			0x80286009
+
+#define OV490_ODS_CTRL_FRAME_OUTPUT_EN	0x0c
+#define OV490_ODS_CTRL			0x8029d000
+
+#define OV490_ID_VAL			0x0490
+#define OV490_ID(_p, _v)		((((_p) & 0xff) << 8) | ((_v) & 0xff))
+#define OV490_PID			0x8080300a
+#define OV490_VER			0x8080300b
+
+#define OV490_ISP_HSIZE_LOW		0x80820060
+#define OV490_ISP_HSIZE_HIGH		0x80820061
+#define OV490_ISP_VSIZE_LOW		0x80820062
+#define OV490_ISP_VSIZE_HIGH		0x80820063
+
+#define OV10640_PIXEL_RATE		(55000000)
+
+struct rdacm21_device {
+	struct device			*dev;
+	struct max9271_device		*serializer;
+	struct i2c_client		*isp;
+	struct v4l2_subdev		sd;
+	struct media_pad		pad;
+	struct v4l2_mbus_framefmt	fmt;
+	struct v4l2_ctrl_handler	ctrls;
+	u32				addrs[32];
+	u16				last_page;
+};
+
+static inline struct rdacm21_device *sd_to_rdacm21(struct v4l2_subdev *sd)
+{
+	return container_of(sd, struct rdacm21_device, sd);
+}
+
+static inline struct rdacm21_device *i2c_to_rdacm21(struct i2c_client *client)
+{
+	return sd_to_rdacm21(i2c_get_clientdata(client));
+}
+
+static const struct ov490_reg {
+	u16 reg;
+	u8 val;
+} ov490_regs_wizard[] = {
+	{0xfffd, 0x80},
+	{0xfffe, 0x82},
+	{0x0071, 0x11},
+	{0x0075, 0x11},
+	{0xfffe, 0x29},
+	{0x6010, 0x01},
+	/*
+	 * OV490 EMB line disable in YUV and RAW data,
+	 * NOTE: EMB line is still used in ISP and sensor
+	 */
+	{0xe000, 0x14},
+	{0xfffe, 0x28},
+	{0x6000, 0x04},
+	{0x6004, 0x00},
+	/*
+	 * PCLK polarity - useless due to silicon bug.
+	 * Use 0x808000bb register instead.
+	 */
+	{0x6008, 0x00},
+	{0xfffe, 0x80},
+	{0x0091, 0x00},
+	/* bit[3]=0 - PCLK polarity workaround. */
+	{0x00bb, 0x1d},
+	/* Ov490 FSIN: app_fsin_from_fsync */
+	{0xfffe, 0x85},
+	{0x0008, 0x00},
+	{0x0009, 0x01},
+	/* FSIN0 source. */
+	{0x000A, 0x05},
+	{0x000B, 0x00},
+	/* FSIN0 delay. */
+	{0x0030, 0x02},
+	{0x0031, 0x00},
+	{0x0032, 0x00},
+	{0x0033, 0x00},
+	/* FSIN1 delay. */
+	{0x0038, 0x02},
+	{0x0039, 0x00},
+	{0x003A, 0x00},
+	{0x003B, 0x00},
+	/* FSIN0 length. */
+	{0x0070, 0x2C},
+	{0x0071, 0x01},
+	{0x0072, 0x00},
+	{0x0073, 0x00},
+	/* FSIN1 length. */
+	{0x0074, 0x64},
+	{0x0075, 0x00},
+	{0x0076, 0x00},
+	{0x0077, 0x00},
+	{0x0000, 0x14},
+	{0x0001, 0x00},
+	{0x0002, 0x00},
+	{0x0003, 0x00},
+	/*
+	 * Load fsin0,load fsin1,load other,
+	 * It will be cleared automatically.
+	 */
+	{0x0004, 0x32},
+	{0x0005, 0x00},
+	{0x0006, 0x00},
+	{0x0007, 0x00},
+	{0xfffe, 0x80},
+	/* Sensor FSIN. */
+	{0x0081, 0x00},
+	/* ov10640 FSIN enable */
+	{0xfffe, 0x19},
+	{0x5000, 0x00},
+	{0x5001, 0x30},
+	{0x5002, 0x8c},
+	{0x5003, 0xb2},
+	{0xfffe, 0x80},
+	{0x00c0, 0xc1},
+	/* ov10640 HFLIP=1 by default */
+	{0xfffe, 0x19},
+	{0x5000, 0x01},
+	{0x5001, 0x00},
+	{0xfffe, 0x80},
+	{0x00c0, 0xdc},
+};
+
+static int ov490_read(struct rdacm21_device *dev, u16 reg, u8 *val)
+{
+	u8 buf[2] = { reg >> 8, reg };
+	int ret;
+
+	ret = i2c_master_send(dev->isp, buf, 2);
+	if (ret == 2)
+		ret = i2c_master_recv(dev->isp, val, 1);
+
+	if (ret < 0) {
+		dev_dbg(dev->dev, "%s: register 0x%04x read failed (%d)\n",
+			__func__, reg, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int ov490_write(struct rdacm21_device *dev, u16 reg, u8 val)
+{
+	u8 buf[3] = { reg >> 8, reg, val };
+	int ret;
+
+	ret = i2c_master_send(dev->isp, buf, 3);
+	if (ret < 0) {
+		dev_err(dev->dev, "%s: register 0x%04x write failed (%d)\n",
+			__func__, reg, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int ov490_set_page(struct rdacm21_device *dev, u16 reg)
+{
+	bool page_new = false;
+	u8 page_high = reg >> 8;
+	u8 page_low = reg;
+	int ret;
+
+	if (page_high != (dev->last_page >> 8)) {
+		ret = ov490_write(dev, OV490_PAGE_HIGH_REG, page_high);
+		if (ret)
+			return ret;
+		page_new = true;
+	}
+
+	if (page_low != (u8)dev->last_page) {
+		ret = ov490_write(dev, OV490_PAGE_LOW_REG, page_low);
+		if (ret)
+			return ret;
+		page_new = true;
+	}
+
+	if (page_new) {
+		dev->last_page = reg;
+		usleep_range(100, 150);
+	}
+
+	return 0;
+}
+
+static int ov490_read_reg(struct rdacm21_device *dev, u32 reg, u8 *val)
+{
+	int ret;
+
+	ret = ov490_set_page(dev, reg >> 16);
+	if (ret)
+		return ret;
+
+	ret = ov490_read(dev, (u16)reg, val);
+	if (ret)
+		return ret;
+
+	dev_dbg(dev->dev, "%s: 0x%08x = 0x%02x\n", __func__, reg, *val);
+
+	return 0;
+}
+
+static int ov490_write_reg(struct rdacm21_device *dev, u32 reg, u8 val)
+{
+	int ret;
+
+	ret = ov490_set_page(dev, reg >> 16);
+	if (ret)
+		return ret;
+
+	ret = ov490_write(dev, (u16)reg, val);
+	if (ret)
+		return ret;
+
+	dev_dbg(dev->dev, "%s: 0x%08x = 0x%02x\n", __func__, reg, val);
+
+	return 0;
+}
+
+static int rdacm21_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct rdacm21_device *dev = sd_to_rdacm21(sd);
+
+	/*
+	 * Enable serial link now that the ISP provides a valid pixel clock
+	 * to start serializing video data on the GMSL link.
+	 */
+	return max9271_set_serial_link(dev->serializer, enable);
+}
+
+static int rdacm21_enum_mbus_code(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_pad_config *cfg,
+				  struct v4l2_subdev_mbus_code_enum *code)
+{
+	if (code->pad || code->index > 0)
+		return -EINVAL;
+
+	code->code = MEDIA_BUS_FMT_YUYV8_1X16;
+
+	return 0;
+}
+
+static int rdacm21_get_fmt(struct v4l2_subdev *sd,
+			   struct v4l2_subdev_pad_config *cfg,
+			   struct v4l2_subdev_format *format)
+{
+	struct v4l2_mbus_framefmt *mf = &format->format;
+	struct rdacm21_device *dev = sd_to_rdacm21(sd);
+
+	if (format->pad)
+		return -EINVAL;
+
+	mf->width		= dev->fmt.width;
+	mf->height		= dev->fmt.height;
+	mf->code		= MEDIA_BUS_FMT_YUYV8_1X16;
+	mf->colorspace		= V4L2_COLORSPACE_SRGB;
+	mf->field		= V4L2_FIELD_NONE;
+	mf->ycbcr_enc		= V4L2_YCBCR_ENC_601;
+	mf->quantization	= V4L2_QUANTIZATION_FULL_RANGE;
+	mf->xfer_func		= V4L2_XFER_FUNC_NONE;
+
+	return 0;
+}
+
+static struct v4l2_subdev_video_ops rdacm21_video_ops = {
+	.s_stream	= rdacm21_s_stream,
+};
+
+static const struct v4l2_subdev_pad_ops rdacm21_subdev_pad_ops = {
+	.enum_mbus_code = rdacm21_enum_mbus_code,
+	.get_fmt	= rdacm21_get_fmt,
+	.set_fmt	= rdacm21_get_fmt,
+};
+
+static struct v4l2_subdev_ops rdacm21_subdev_ops = {
+	.video		= &rdacm21_video_ops,
+	.pad		= &rdacm21_subdev_pad_ops,
+};
+
+static int ov490_initialize(struct rdacm21_device *dev)
+{
+	unsigned int ov490_pid_retry = 20;
+	unsigned int timeout;
+	u8 pid, ver, val;
+	unsigned int i;
+	int ret;
+
+	/* Read OV490 Id to test communications. */
+pid_retry:
+	ret = ov490_read_reg(dev, OV490_PID, &pid);
+	if (ret < 0) {
+		/* Give OV490 a few more cycles to exit from reset. */
+		if (ov490_pid_retry--) {
+			usleep_range(1000, 2000);
+			goto pid_retry;
+		}
+
+		dev_err(dev->dev, "OV490 PID read failed (%d)\n", ret);
+		return ret;
+	}
+
+	ret = ov490_read_reg(dev, OV490_VER, &ver);
+	if (ret < 0) {
+		dev_err(dev->dev, "OV490 VERSION read failed (%d)\n", ret);
+		return ret;
+	}
+
+	if (OV490_ID(pid, ver) != OV490_ID_VAL) {
+		dev_err(dev->dev, "OV490 ID mismatch: (0x%04x)\n",
+			OV490_ID(pid, ver));
+		return -ENODEV;
+	}
+
+	/* Wait for firmware boot by reading streamon status. */
+	for (timeout = 300; timeout > 0; timeout--) {
+		ov490_read_reg(dev, OV490_ODS_CTRL, &val);
+		if (val == OV490_ODS_CTRL_FRAME_OUTPUT_EN)
+			break;
+		mdelay(1);
+	}
+	if (!timeout) {
+		dev_err(dev->dev, "Timeout firmware boot wait\n");
+		return -ENODEV;
+	}
+	dev_dbg(dev->dev, "Firmware booted in %u msec\n", 300 - timeout);
+
+	/* Read OV10640 Id to test communications. */
+	ov490_write(dev, 0xfffd, 0x80);
+	ov490_write(dev, 0xfffe, 0x19);
+	usleep_range(100, 150);
+
+	ov490_write(dev, 0x5000, 0x01);
+	ov490_write(dev, 0x5001, 0x30);
+	ov490_write(dev, 0x5002, 0x0a);
+
+	ov490_write(dev, 0xfffe, 0x80);
+	usleep_range(100, 150);
+	ov490_write(dev, 0xc0, 0xc1);
+	ov490_write(dev, 0xfffe, 0x19);
+	usleep_range(1000, 1500);
+	ov490_read(dev, 0x5000, &val);
+	if (val != OV10640_ID_LOW) {
+		dev_err(dev->dev, "OV10640 ID mismatch: (0x%02x)\n", val);
+		return -ENODEV;
+	}
+
+	dev_dbg(dev->dev, "OV10640 ID = 0x%2x\n", val);
+
+	for (i = 0; i < ARRAY_SIZE(ov490_regs_wizard); ++i) {
+		ret = ov490_write(dev, ov490_regs_wizard[i].reg,
+				  ov490_regs_wizard[i].val);
+		if (ret < 0) {
+			dev_err(dev->dev,
+				"%s: register %u (0x%04x) write failed (%d)\n",
+				__func__, i, ov490_regs_wizard[i].reg, ret);
+
+			return -EIO;
+		}
+
+		usleep_range(100, 150);
+	}
+
+	/*
+	 * The ISP is programmed with the content of a serial flash memory.
+	 * Read the firmware configuration to reflect it through the V4L2 APIs.
+	 */
+	ov490_read_reg(dev, OV490_ISP_HSIZE_HIGH, &val);
+	dev->fmt.width = (val & 0xf) << 8;
+	ov490_read_reg(dev, OV490_ISP_HSIZE_LOW, &val);
+	dev->fmt.width |= (val & 0xff);
+
+	ov490_read_reg(dev, OV490_ISP_VSIZE_HIGH, &val);
+	dev->fmt.height = (val & 0xf) << 8;
+	ov490_read_reg(dev, OV490_ISP_VSIZE_LOW, &val);
+	dev->fmt.height |= val & 0xff;
+
+	/* Set bus width to 12 bits with [0:11] ordering. */
+	ov490_write_reg(dev, OV490_DVP_CTRL3, 0x10);
+
+	dev_info(dev->dev, "Identified RDACM21 camera module\n");
+
+	return 0;
+}
+
+static int rdacm21_initialize(struct rdacm21_device *dev)
+{
+	int ret;
+
+	/* Verify communication with the MAX9271: ping to wakeup. */
+	dev->serializer->client->addr = MAX9271_DEFAULT_ADDR;
+	i2c_smbus_read_byte(dev->serializer->client);
+
+	/* Serial link disabled during config as it needs a valid pixel clock. */
+	ret = max9271_set_serial_link(dev->serializer, false);
+	if (ret)
+		return ret;
+
+	/* Set GPO high to hold OV490 in reset during max9271 configuration. */
+	ret = max9271_set_gpios(dev->serializer, MAX9271_GPO);
+	if (ret)
+		return ret;
+
+	/* Configure I2C bus at 105Kbps speed and configure GMSL link. */
+	ret = max9271_configure_i2c(dev->serializer,
+				    MAX9271_I2CSLVSH_469NS_234NS |
+				    MAX9271_I2CSLVTO_1024US |
+				    MAX9271_I2CMSTBT_105KBPS);
+	if (ret)
+		return ret;
+
+	ret = max9271_configure_gmsl_link(dev->serializer);
+	if (ret)
+		return ret;
+
+	ret = max9271_set_address(dev->serializer, dev->addrs[0]);
+	if (ret)
+		return ret;
+	dev->serializer->client->addr = dev->addrs[0];
+
+	/*
+	 * Release OV490 from reset and program address translation
+	 * before performing OV490 configuration.
+	 */
+	ret = max9271_clear_gpios(dev->serializer, MAX9271_GPO);
+	if (ret)
+		return ret;
+
+	ret = max9271_set_translation(dev->serializer, dev->addrs[1],
+				      OV490_I2C_ADDRESS);
+	if (ret)
+		return ret;
+	dev->isp->addr = dev->addrs[1];
+
+	ret = ov490_initialize(dev);
+	if (ret)
+		return ret;
+
+	/*
+	 * Set reverse channel high threshold to increase noise immunity.
+	 *
+	 * This should be compensated by increasing the reverse channel
+	 * amplitude on the remote deserializer side.
+	 */
+	ret = max9271_set_high_threshold(dev->serializer, true);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int rdacm21_probe(struct i2c_client *client)
+{
+	struct rdacm21_device *dev;
+	struct fwnode_handle *ep;
+	int ret;
+
+	dev = devm_kzalloc(&client->dev, sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+	dev->dev = &client->dev;
+
+	dev->serializer = devm_kzalloc(&client->dev, sizeof(*dev->serializer),
+				       GFP_KERNEL);
+	if (!dev->serializer)
+		return -ENOMEM;
+
+	dev->serializer->client = client;
+
+	ret = of_property_read_u32_array(client->dev.of_node, "reg",
+					 dev->addrs, 2);
+	if (ret < 0) {
+		dev_err(dev->dev, "Invalid DT reg property: %d\n", ret);
+		return -EINVAL;
+	}
+
+	/* Create the dummy I2C client for the sensor. */
+	dev->isp = i2c_new_dummy_device(client->adapter, OV490_I2C_ADDRESS);
+	if (IS_ERR(dev->isp))
+		return PTR_ERR(dev->isp);
+
+	ret = rdacm21_initialize(dev);
+	if (ret < 0)
+		goto error;
+
+	/* Initialize and register the subdevice. */
+	v4l2_i2c_subdev_init(&dev->sd, client, &rdacm21_subdev_ops);
+	dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+	v4l2_ctrl_handler_init(&dev->ctrls, 1);
+	v4l2_ctrl_new_std(&dev->ctrls, NULL, V4L2_CID_PIXEL_RATE,
+			  OV10640_PIXEL_RATE, OV10640_PIXEL_RATE, 1,
+			  OV10640_PIXEL_RATE);
+	dev->sd.ctrl_handler = &dev->ctrls;
+
+	ret = dev->ctrls.error;
+	if (ret)
+		goto error_free_ctrls;
+
+	dev->pad.flags = MEDIA_PAD_FL_SOURCE;
+	dev->sd.entity.flags |= MEDIA_ENT_F_CAM_SENSOR;
+	ret = media_entity_pads_init(&dev->sd.entity, 1, &dev->pad);
+	if (ret < 0)
+		goto error_free_ctrls;
+
+	ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL);
+	if (!ep) {
+		dev_err(&client->dev,
+			"Unable to get endpoint in node %pOF\n",
+			client->dev.of_node);
+		ret = -ENOENT;
+		goto error_free_ctrls;
+	}
+	dev->sd.fwnode = ep;
+
+	ret = v4l2_async_register_subdev(&dev->sd);
+	if (ret)
+		goto error_put_node;
+
+	return 0;
+
+error_put_node:
+	fwnode_handle_put(dev->sd.fwnode);
+error_free_ctrls:
+	v4l2_ctrl_handler_free(&dev->ctrls);
+error:
+	i2c_unregister_device(dev->isp);
+
+	return ret;
+}
+
+static int rdacm21_remove(struct i2c_client *client)
+{
+	struct rdacm21_device *dev = i2c_to_rdacm21(client);
+
+	fwnode_handle_put(dev->sd.fwnode);
+	v4l2_async_unregister_subdev(&dev->sd);
+	v4l2_ctrl_handler_free(&dev->ctrls);
+	i2c_unregister_device(dev->isp);
+
+	return 0;
+}
+
+static const struct of_device_id rdacm21_of_ids[] = {
+	{ .compatible = "imi,rdacm21" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, rdacm21_of_ids);
+
+static struct i2c_driver rdacm21_i2c_driver = {
+	.driver	= {
+		.name	= "rdacm21",
+		.of_match_table = rdacm21_of_ids,
+	},
+	.probe_new	= rdacm21_probe,
+	.remove		= rdacm21_remove,
+};
+
+module_i2c_driver(rdacm21_i2c_driver);
+
+MODULE_DESCRIPTION("GMSL Camera driver for RDACM21");
+MODULE_AUTHOR("Jacopo Mondi, Kieran Bingham, Laurent Pinchart, Niklas Söderlund, Vladimir Barinov");
+MODULE_LICENSE("GPL v2");
-- 
2.29.2

Re: [PATCH v6 1/5] media: i2c: Add driver for RDACM21 camera module

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Dec 15, 2020 at 06:09:53PM +0100, Jacopo Mondi wrote:
> The RDACM21 is a GMSL camera supporting 1280x1080 resolution images
> developed by IMI based on an Omnivision OV10640 sensor, an Omnivision
> OV490 ISP and a Maxim MAX9271 GMSL serializer.
> 
> The driver uses the max9271 library module, to maximize code reuse with
> other camera module drivers using the same serializer, such as rdacm20.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  MAINTAINERS                 |  12 +
>  drivers/media/i2c/Kconfig   |  13 +
>  drivers/media/i2c/Makefile  |   2 +
>  drivers/media/i2c/rdacm21.c | 595 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 622 insertions(+)
>  create mode 100644 drivers/media/i2c/rdacm21.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4be038f0a59d..a011df5a14d7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14809,6 +14809,18 @@ F:	drivers/media/i2c/max9271.c
>  F:	drivers/media/i2c/max9271.h
>  F:	drivers/media/i2c/rdacm20.c
>  
> +RDACM21 Camera Sensor
> +M:	Jacopo Mondi <jacopo+renesas@jmondi.org>
> +M:	Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> +M:	Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> +M:	Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> +L:	linux-media@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/media/i2c/rdacm2x-gmsl.yaml
> +F:	drivers/media/i2c/max9271.c
> +F:	drivers/media/i2c/max9271.h
> +F:	drivers/media/i2c/rdacm21.c
> +
>  RDC R-321X SoC
>  M:	Florian Fainelli <florian@openwrt.org>
>  S:	Maintained
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 2b9d81e4794a..d500edb8638b 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -1212,6 +1212,19 @@ config VIDEO_RDACM20
>  	  This camera should be used in conjunction with a GMSL
>  	  deserialiser such as the MAX9286.
>  
> +config VIDEO_RDACM21
> +	tristate "IMI RDACM21 camera support"
> +	depends on I2C
> +	select V4L2_FWNODE
> +	select VIDEO_V4L2_SUBDEV_API
> +	select MEDIA_CONTROLLER
> +	help
> +	  This driver supports the IMI RDACM21 GMSL camera, used in
> +	  ADAS systems.
> +
> +	  This camera should be used in conjunction with a GMSL
> +	  deserialiser such as the MAX9286.
> +
>  config VIDEO_RJ54N1
>  	tristate "Sharp RJ54N1CB0C sensor support"
>  	depends on I2C && VIDEO_V4L2
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index a3149dce21bb..85b1edc62508 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -124,6 +124,8 @@ obj-$(CONFIG_VIDEO_IMX355)	+= imx355.o
>  obj-$(CONFIG_VIDEO_MAX9286)	+= max9286.o
>  rdacm20-camera_module-objs	:= rdacm20.o max9271.o
>  obj-$(CONFIG_VIDEO_RDACM20)	+= rdacm20-camera_module.o
> +rdacm21-camera_module-objs	:= rdacm21.o max9271.o
> +obj-$(CONFIG_VIDEO_RDACM21)	+= rdacm21-camera_module.o
>  obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
>  
>  obj-$(CONFIG_SDR_MAX2175) += max2175.o
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> new file mode 100644
> index 000000000000..5f9267e26258
> --- /dev/null
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -0,0 +1,595 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * IMI RDACM21 GMSL Camera Driver
> + *
> + * Copyright (C) 2017-2020 Jacopo Mondi
> + * Copyright (C) 2017-2019 Kieran Bingham
> + * Copyright (C) 2017-2019 Laurent Pinchart
> + * Copyright (C) 2017-2019 Niklas Söderlund
> + * Copyright (C) 2016 Renesas Electronics Corporation
> + * Copyright (C) 2015 Cogent Embedded, Inc.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/fwnode.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/videodev2.h>
> +
> +#include <media/v4l2-async.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-subdev.h>
> +#include "max9271.h"
> +
> +#define OV10640_ID_LOW			0xa6
> +
> +#define OV490_I2C_ADDRESS		0x24
> +
> +#define OV490_PAGE_HIGH_REG		0xfffd
> +#define OV490_PAGE_LOW_REG		0xfffe
> +
> +#define OV490_DVP_CTRL3			0x80286009
> +
> +#define OV490_ODS_CTRL_FRAME_OUTPUT_EN	0x0c
> +#define OV490_ODS_CTRL			0x8029d000
> +
> +#define OV490_ID_VAL			0x0490
> +#define OV490_ID(_p, _v)		((((_p) & 0xff) << 8) | ((_v) & 0xff))
> +#define OV490_PID			0x8080300a
> +#define OV490_VER			0x8080300b
> +
> +#define OV490_ISP_HSIZE_LOW		0x80820060
> +#define OV490_ISP_HSIZE_HIGH		0x80820061
> +#define OV490_ISP_VSIZE_LOW		0x80820062
> +#define OV490_ISP_VSIZE_HIGH		0x80820063
> +
> +#define OV10640_PIXEL_RATE		(55000000)

No need for parentheses.

At some point we should move the sensor and ISP-related code to a
separate driver, but that can wait.

> +
> +struct rdacm21_device {
> +	struct device			*dev;
> +	struct max9271_device		*serializer;
> +	struct i2c_client		*isp;
> +	struct v4l2_subdev		sd;
> +	struct media_pad		pad;
> +	struct v4l2_mbus_framefmt	fmt;
> +	struct v4l2_ctrl_handler	ctrls;
> +	u32				addrs[32];

Do we need 32 addresses ? 2 seem enough.

> +	u16				last_page;
> +};
> +
> +static inline struct rdacm21_device *sd_to_rdacm21(struct v4l2_subdev *sd)
> +{
> +	return container_of(sd, struct rdacm21_device, sd);
> +}
> +
> +static inline struct rdacm21_device *i2c_to_rdacm21(struct i2c_client *client)
> +{
> +	return sd_to_rdacm21(i2c_get_clientdata(client));
> +}

As this is used in the remove handler only you could inline it there, up
to you.

> +
> +static const struct ov490_reg {
> +	u16 reg;
> +	u8 val;
> +} ov490_regs_wizard[] = {
> +	{0xfffd, 0x80},
> +	{0xfffe, 0x82},
> +	{0x0071, 0x11},
> +	{0x0075, 0x11},
> +	{0xfffe, 0x29},
> +	{0x6010, 0x01},
> +	/*
> +	 * OV490 EMB line disable in YUV and RAW data,
> +	 * NOTE: EMB line is still used in ISP and sensor
> +	 */
> +	{0xe000, 0x14},
> +	{0xfffe, 0x28},
> +	{0x6000, 0x04},
> +	{0x6004, 0x00},
> +	/*
> +	 * PCLK polarity - useless due to silicon bug.
> +	 * Use 0x808000bb register instead.
> +	 */
> +	{0x6008, 0x00},
> +	{0xfffe, 0x80},
> +	{0x0091, 0x00},
> +	/* bit[3]=0 - PCLK polarity workaround. */
> +	{0x00bb, 0x1d},
> +	/* Ov490 FSIN: app_fsin_from_fsync */
> +	{0xfffe, 0x85},
> +	{0x0008, 0x00},
> +	{0x0009, 0x01},
> +	/* FSIN0 source. */
> +	{0x000A, 0x05},
> +	{0x000B, 0x00},
> +	/* FSIN0 delay. */
> +	{0x0030, 0x02},
> +	{0x0031, 0x00},
> +	{0x0032, 0x00},
> +	{0x0033, 0x00},
> +	/* FSIN1 delay. */
> +	{0x0038, 0x02},
> +	{0x0039, 0x00},
> +	{0x003A, 0x00},
> +	{0x003B, 0x00},
> +	/* FSIN0 length. */
> +	{0x0070, 0x2C},
> +	{0x0071, 0x01},
> +	{0x0072, 0x00},
> +	{0x0073, 0x00},
> +	/* FSIN1 length. */
> +	{0x0074, 0x64},
> +	{0x0075, 0x00},
> +	{0x0076, 0x00},
> +	{0x0077, 0x00},
> +	{0x0000, 0x14},
> +	{0x0001, 0x00},
> +	{0x0002, 0x00},
> +	{0x0003, 0x00},
> +	/*
> +	 * Load fsin0,load fsin1,load other,
> +	 * It will be cleared automatically.
> +	 */
> +	{0x0004, 0x32},
> +	{0x0005, 0x00},
> +	{0x0006, 0x00},
> +	{0x0007, 0x00},
> +	{0xfffe, 0x80},
> +	/* Sensor FSIN. */
> +	{0x0081, 0x00},
> +	/* ov10640 FSIN enable */
> +	{0xfffe, 0x19},
> +	{0x5000, 0x00},
> +	{0x5001, 0x30},
> +	{0x5002, 0x8c},
> +	{0x5003, 0xb2},
> +	{0xfffe, 0x80},
> +	{0x00c0, 0xc1},
> +	/* ov10640 HFLIP=1 by default */
> +	{0xfffe, 0x19},
> +	{0x5000, 0x01},
> +	{0x5001, 0x00},
> +	{0xfffe, 0x80},
> +	{0x00c0, 0xdc},
> +};
> +
> +static int ov490_read(struct rdacm21_device *dev, u16 reg, u8 *val)
> +{
> +	u8 buf[2] = { reg >> 8, reg };
> +	int ret;
> +
> +	ret = i2c_master_send(dev->isp, buf, 2);
> +	if (ret == 2)
> +		ret = i2c_master_recv(dev->isp, val, 1);
> +
> +	if (ret < 0) {
> +		dev_dbg(dev->dev, "%s: register 0x%04x read failed (%d)\n",
> +			__func__, reg, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ov490_write(struct rdacm21_device *dev, u16 reg, u8 val)
> +{
> +	u8 buf[3] = { reg >> 8, reg, val };
> +	int ret;
> +
> +	ret = i2c_master_send(dev->isp, buf, 3);
> +	if (ret < 0) {
> +		dev_err(dev->dev, "%s: register 0x%04x write failed (%d)\n",
> +			__func__, reg, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ov490_set_page(struct rdacm21_device *dev, u16 reg)

s/reg/page/

> +{
> +	bool page_new = false;
> +	u8 page_high = reg >> 8;
> +	u8 page_low = reg;
> +	int ret;
> +

If you add

	if (page == dev->last_page)
		return 0;

here, you can drop the page_new variable.

> +	if (page_high != (dev->last_page >> 8)) {
> +		ret = ov490_write(dev, OV490_PAGE_HIGH_REG, page_high);
> +		if (ret)
> +			return ret;
> +		page_new = true;
> +	}
> +
> +	if (page_low != (u8)dev->last_page) {
> +		ret = ov490_write(dev, OV490_PAGE_LOW_REG, page_low);
> +		if (ret)
> +			return ret;
> +		page_new = true;
> +	}
> +
> +	if (page_new) {
> +		dev->last_page = reg;
> +		usleep_range(100, 150);
> +	}
> +
> +	return 0;
> +}
> +
> +static int ov490_read_reg(struct rdacm21_device *dev, u32 reg, u8 *val)
> +{
> +	int ret;
> +
> +	ret = ov490_set_page(dev, reg >> 16);
> +	if (ret)
> +		return ret;
> +
> +	ret = ov490_read(dev, (u16)reg, val);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(dev->dev, "%s: 0x%08x = 0x%02x\n", __func__, reg, *val);
> +
> +	return 0;
> +}
> +
> +static int ov490_write_reg(struct rdacm21_device *dev, u32 reg, u8 val)
> +{
> +	int ret;
> +
> +	ret = ov490_set_page(dev, reg >> 16);
> +	if (ret)
> +		return ret;
> +
> +	ret = ov490_write(dev, (u16)reg, val);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(dev->dev, "%s: 0x%08x = 0x%02x\n", __func__, reg, val);
> +
> +	return 0;
> +}
> +
> +static int rdacm21_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct rdacm21_device *dev = sd_to_rdacm21(sd);
> +
> +	/*
> +	 * Enable serial link now that the ISP provides a valid pixel clock
> +	 * to start serializing video data on the GMSL link.
> +	 */
> +	return max9271_set_serial_link(dev->serializer, enable);
> +}
> +
> +static int rdacm21_enum_mbus_code(struct v4l2_subdev *sd,
> +				  struct v4l2_subdev_pad_config *cfg,
> +				  struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	if (code->pad || code->index > 0)
> +		return -EINVAL;
> +
> +	code->code = MEDIA_BUS_FMT_YUYV8_1X16;
> +
> +	return 0;
> +}
> +
> +static int rdacm21_get_fmt(struct v4l2_subdev *sd,
> +			   struct v4l2_subdev_pad_config *cfg,
> +			   struct v4l2_subdev_format *format)
> +{
> +	struct v4l2_mbus_framefmt *mf = &format->format;
> +	struct rdacm21_device *dev = sd_to_rdacm21(sd);
> +
> +	if (format->pad)
> +		return -EINVAL;
> +
> +	mf->width		= dev->fmt.width;
> +	mf->height		= dev->fmt.height;
> +	mf->code		= MEDIA_BUS_FMT_YUYV8_1X16;
> +	mf->colorspace		= V4L2_COLORSPACE_SRGB;
> +	mf->field		= V4L2_FIELD_NONE;
> +	mf->ycbcr_enc		= V4L2_YCBCR_ENC_601;
> +	mf->quantization	= V4L2_QUANTIZATION_FULL_RANGE;
> +	mf->xfer_func		= V4L2_XFER_FUNC_NONE;
> +
> +	return 0;
> +}
> +
> +static struct v4l2_subdev_video_ops rdacm21_video_ops = {

static const

> +	.s_stream	= rdacm21_s_stream,
> +};
> +
> +static const struct v4l2_subdev_pad_ops rdacm21_subdev_pad_ops = {
> +	.enum_mbus_code = rdacm21_enum_mbus_code,
> +	.get_fmt	= rdacm21_get_fmt,
> +	.set_fmt	= rdacm21_get_fmt,
> +};
> +
> +static struct v4l2_subdev_ops rdacm21_subdev_ops = {

static const

> +	.video		= &rdacm21_video_ops,
> +	.pad		= &rdacm21_subdev_pad_ops,
> +};
> +
> +static int ov490_initialize(struct rdacm21_device *dev)
> +{
> +	unsigned int ov490_pid_retry = 20;
> +	unsigned int timeout;
> +	u8 pid, ver, val;
> +	unsigned int i;
> +	int ret;
> +
> +	/* Read OV490 Id to test communications. */
> +pid_retry:

Could we use a for or while loop instead of a goto ?

> +	ret = ov490_read_reg(dev, OV490_PID, &pid);
> +	if (ret < 0) {
> +		/* Give OV490 a few more cycles to exit from reset. */
> +		if (ov490_pid_retry--) {
> +			usleep_range(1000, 2000);
> +			goto pid_retry;
> +		}
> +
> +		dev_err(dev->dev, "OV490 PID read failed (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	ret = ov490_read_reg(dev, OV490_VER, &ver);
> +	if (ret < 0) {
> +		dev_err(dev->dev, "OV490 VERSION read failed (%d)\n", ret);

There's already an error message printed in the read function, you can
drop this one.

> +		return ret;
> +	}
> +
> +	if (OV490_ID(pid, ver) != OV490_ID_VAL) {
> +		dev_err(dev->dev, "OV490 ID mismatch: (0x%04x)\n",

s/://

> +			OV490_ID(pid, ver));
> +		return -ENODEV;
> +	}
> +
> +	/* Wait for firmware boot by reading streamon status. */
> +	for (timeout = 300; timeout > 0; timeout--) {
> +		ov490_read_reg(dev, OV490_ODS_CTRL, &val);
> +		if (val == OV490_ODS_CTRL_FRAME_OUTPUT_EN)
> +			break;
> +		mdelay(1);

A sleep is better than a delay.

> +	}
> +	if (!timeout) {
> +		dev_err(dev->dev, "Timeout firmware boot wait\n");
> +		return -ENODEV;
> +	}
> +	dev_dbg(dev->dev, "Firmware booted in %u msec\n", 300 - timeout);

That won't be a very accurate time measurement.

> +
> +	/* Read OV10640 Id to test communications. */
> +	ov490_write(dev, 0xfffd, 0x80);
> +	ov490_write(dev, 0xfffe, 0x19);

The page is handled in ov490_write_reg(), can't we write

	ov490_write_reg(dev, 0x80195000, 0x01);
	ov490_write_reg(dev, 0x80195001, 0x30);
	ov490_write_reg(dev, 0x80195002, 0x0a);

to simplify the code ? Same below.

> +	usleep_range(100, 150);
> +
> +	ov490_write(dev, 0x5000, 0x01);
> +	ov490_write(dev, 0x5001, 0x30);
> +	ov490_write(dev, 0x5002, 0x0a);
> +
> +	ov490_write(dev, 0xfffe, 0x80);
> +	usleep_range(100, 150);
> +	ov490_write(dev, 0xc0, 0xc1);
> +	ov490_write(dev, 0xfffe, 0x19);
> +	usleep_range(1000, 1500);
> +	ov490_read(dev, 0x5000, &val);
> +	if (val != OV10640_ID_LOW) {
> +		dev_err(dev->dev, "OV10640 ID mismatch: (0x%02x)\n", val);
> +		return -ENODEV;
> +	}
> +
> +	dev_dbg(dev->dev, "OV10640 ID = 0x%2x\n", val);
> +
> +	for (i = 0; i < ARRAY_SIZE(ov490_regs_wizard); ++i) {
> +		ret = ov490_write(dev, ov490_regs_wizard[i].reg,
> +				  ov490_regs_wizard[i].val);
> +		if (ret < 0) {
> +			dev_err(dev->dev,
> +				"%s: register %u (0x%04x) write failed (%d)\n",
> +				__func__, i, ov490_regs_wizard[i].reg, ret);
> +
> +			return -EIO;
> +		}
> +
> +		usleep_range(100, 150);
> +	}
> +
> +	/*
> +	 * The ISP is programmed with the content of a serial flash memory.
> +	 * Read the firmware configuration to reflect it through the V4L2 APIs.
> +	 */
> +	ov490_read_reg(dev, OV490_ISP_HSIZE_HIGH, &val);
> +	dev->fmt.width = (val & 0xf) << 8;
> +	ov490_read_reg(dev, OV490_ISP_HSIZE_LOW, &val);
> +	dev->fmt.width |= (val & 0xff);
> +
> +	ov490_read_reg(dev, OV490_ISP_VSIZE_HIGH, &val);
> +	dev->fmt.height = (val & 0xf) << 8;
> +	ov490_read_reg(dev, OV490_ISP_VSIZE_LOW, &val);
> +	dev->fmt.height |= val & 0xff;
> +
> +	/* Set bus width to 12 bits with [0:11] ordering. */
> +	ov490_write_reg(dev, OV490_DVP_CTRL3, 0x10);
> +
> +	dev_info(dev->dev, "Identified RDACM21 camera module\n");
> +
> +	return 0;
> +}
> +
> +static int rdacm21_initialize(struct rdacm21_device *dev)
> +{
> +	int ret;
> +
> +	/* Verify communication with the MAX9271: ping to wakeup. */
> +	dev->serializer->client->addr = MAX9271_DEFAULT_ADDR;
> +	i2c_smbus_read_byte(dev->serializer->client);
> +
> +	/* Serial link disabled during config as it needs a valid pixel clock. */
> +	ret = max9271_set_serial_link(dev->serializer, false);
> +	if (ret)
> +		return ret;
> +
> +	/* Set GPO high to hold OV490 in reset during max9271 configuration. */
> +	ret = max9271_set_gpios(dev->serializer, MAX9271_GPO);
> +	if (ret)
> +		return ret;
> +
> +	/* Configure I2C bus at 105Kbps speed and configure GMSL link. */
> +	ret = max9271_configure_i2c(dev->serializer,
> +				    MAX9271_I2CSLVSH_469NS_234NS |
> +				    MAX9271_I2CSLVTO_1024US |
> +				    MAX9271_I2CMSTBT_105KBPS);
> +	if (ret)
> +		return ret;
> +
> +	ret = max9271_configure_gmsl_link(dev->serializer);
> +	if (ret)
> +		return ret;
> +
> +	ret = max9271_set_address(dev->serializer, dev->addrs[0]);
> +	if (ret)
> +		return ret;
> +	dev->serializer->client->addr = dev->addrs[0];
> +
> +	/*
> +	 * Release OV490 from reset and program address translation
> +	 * before performing OV490 configuration.
> +	 */
> +	ret = max9271_clear_gpios(dev->serializer, MAX9271_GPO);
> +	if (ret)
> +		return ret;
> +
> +	ret = max9271_set_translation(dev->serializer, dev->addrs[1],
> +				      OV490_I2C_ADDRESS);
> +	if (ret)
> +		return ret;
> +	dev->isp->addr = dev->addrs[1];
> +
> +	ret = ov490_initialize(dev);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Set reverse channel high threshold to increase noise immunity.
> +	 *
> +	 * This should be compensated by increasing the reverse channel
> +	 * amplitude on the remote deserializer side.
> +	 */
> +	ret = max9271_set_high_threshold(dev->serializer, true);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int rdacm21_probe(struct i2c_client *client)
> +{
> +	struct rdacm21_device *dev;
> +	struct fwnode_handle *ep;
> +	int ret;
> +
> +	dev = devm_kzalloc(&client->dev, sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
> +	dev->dev = &client->dev;
> +
> +	dev->serializer = devm_kzalloc(&client->dev, sizeof(*dev->serializer),
> +				       GFP_KERNEL);

Does this need to be allocated dynamically, can't the serializer field
be embedded ?

> +	if (!dev->serializer)
> +		return -ENOMEM;
> +
> +	dev->serializer->client = client;
> +
> +	ret = of_property_read_u32_array(client->dev.of_node, "reg",
> +					 dev->addrs, 2);
> +	if (ret < 0) {
> +		dev_err(dev->dev, "Invalid DT reg property: %d\n", ret);
> +		return -EINVAL;
> +	}
> +
> +	/* Create the dummy I2C client for the sensor. */
> +	dev->isp = i2c_new_dummy_device(client->adapter, OV490_I2C_ADDRESS);
> +	if (IS_ERR(dev->isp))
> +		return PTR_ERR(dev->isp);
> +
> +	ret = rdacm21_initialize(dev);
> +	if (ret < 0)
> +		goto error;
> +
> +	/* Initialize and register the subdevice. */
> +	v4l2_i2c_subdev_init(&dev->sd, client, &rdacm21_subdev_ops);
> +	dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +
> +	v4l2_ctrl_handler_init(&dev->ctrls, 1);
> +	v4l2_ctrl_new_std(&dev->ctrls, NULL, V4L2_CID_PIXEL_RATE,
> +			  OV10640_PIXEL_RATE, OV10640_PIXEL_RATE, 1,
> +			  OV10640_PIXEL_RATE);
> +	dev->sd.ctrl_handler = &dev->ctrls;
> +
> +	ret = dev->ctrls.error;
> +	if (ret)
> +		goto error_free_ctrls;
> +
> +	dev->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	dev->sd.entity.flags |= MEDIA_ENT_F_CAM_SENSOR;
> +	ret = media_entity_pads_init(&dev->sd.entity, 1, &dev->pad);
> +	if (ret < 0)
> +		goto error_free_ctrls;
> +
> +	ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL);
> +	if (!ep) {
> +		dev_err(&client->dev,
> +			"Unable to get endpoint in node %pOF\n",
> +			client->dev.of_node);
> +		ret = -ENOENT;
> +		goto error_free_ctrls;
> +	}
> +	dev->sd.fwnode = ep;
> +
> +	ret = v4l2_async_register_subdev(&dev->sd);
> +	if (ret)
> +		goto error_put_node;
> +
> +	return 0;
> +
> +error_put_node:
> +	fwnode_handle_put(dev->sd.fwnode);
> +error_free_ctrls:
> +	v4l2_ctrl_handler_free(&dev->ctrls);
> +error:
> +	i2c_unregister_device(dev->isp);
> +
> +	return ret;
> +}
> +
> +static int rdacm21_remove(struct i2c_client *client)
> +{
> +	struct rdacm21_device *dev = i2c_to_rdacm21(client);
> +
> +	fwnode_handle_put(dev->sd.fwnode);

Maybe this should go after unregistration of the subdev, to avoid a
use-after-free ? I'd move it to the end.

> +	v4l2_async_unregister_subdev(&dev->sd);
> +	v4l2_ctrl_handler_free(&dev->ctrls);
> +	i2c_unregister_device(dev->isp);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id rdacm21_of_ids[] = {
> +	{ .compatible = "imi,rdacm21" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, rdacm21_of_ids);
> +
> +static struct i2c_driver rdacm21_i2c_driver = {
> +	.driver	= {
> +		.name	= "rdacm21",
> +		.of_match_table = rdacm21_of_ids,
> +	},
> +	.probe_new	= rdacm21_probe,
> +	.remove		= rdacm21_remove,
> +};
> +
> +module_i2c_driver(rdacm21_i2c_driver);
> +
> +MODULE_DESCRIPTION("GMSL Camera driver for RDACM21");
> +MODULE_AUTHOR("Jacopo Mondi, Kieran Bingham, Laurent Pinchart, Niklas Söderlund, Vladimir Barinov");

Did I author any code in this driver ?

> +MODULE_LICENSE("GPL v2");

-- 
Regards,

Laurent Pinchart

[PATCH v6 2/5] dt-bindings: media: max9286: Document 'maxim,reverse-channel-microvolt'

[Jacopo Mondi]

Document the 'reverse-channel-microvolt' vendor property in the
bindings document of the max9286 driver.

The newly introduced property allows to specifying the initial
configuration of the GMSL reverse control channel to accommodate
remote serializers pre-programmed with the high threshold power
supply noise immunity enabled.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
v5->v6:
- Use standard unit suffix 'microvolt' for the custom property
- Drop '$ref' as according to 'example-schema.yaml':
  "Vendor specific properties having a standard unit suffix don't need a type."
---
 .../bindings/media/i2c/maxim,max9286.yaml     | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index 9ea827092fdd..b22ba3e0db4a 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -51,6 +51,26 @@ properties:
   '#gpio-cells':
     const: 2

+  maxim,reverse-channel-microvolt:
+    minimum: 30000
+    maximum: 200000
+    default: 170000
+    description: |
+      Initial amplitude of the reverse control channel, in micro volts.
+
+      The initial amplitude shall be adjusted to a value compatible with the
+      configuration of the connected remote serializer.
+
+      Some camera modules (for example RDACM20) include an on-board MCU that
+      pre-programs the embedded serializer with power supply noise immunity
+      (high-threshold) enabled. A typical value of the deserializer's reverse
+      channel amplitude to communicate with pre-programmed serializers is
+      170000 micro volts.
+
+      A typical value for the reverse channel amplitude to communicate with
+      a remote serializer whose high-threshold noise immunity is not enabled
+      is 100000 micro volts
+
   ports:
     type: object
     description: |
@@ -221,6 +241,7 @@ required:
   - ports
   - i2c-mux
   - gpio-controller
+  - maxim,reverse-channel-microvolt

 additionalProperties: false

@@ -243,6 +264,8 @@ examples:
         gpio-controller;
         #gpio-cells = <2>;

+        maxim,reverse-channel-microvolt = <170000>;
+
         ports {
           #address-cells = <1>;
           #size-cells = <0>;
--
2.29.2

Re: [PATCH v6 2/5] dt-bindings: media: max9286: Document 'maxim,reverse-channel-microvolt'

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Dec 15, 2020 at 06:09:54PM +0100, Jacopo Mondi wrote:
> Document the 'reverse-channel-microvolt' vendor property in the
> bindings document of the max9286 driver.
> 
> The newly introduced property allows to specifying the initial
> configuration of the GMSL reverse control channel to accommodate
> remote serializers pre-programmed with the high threshold power
> supply noise immunity enabled.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

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

> ---
> v5->v6:
> - Use standard unit suffix 'microvolt' for the custom property
> - Drop '$ref' as according to 'example-schema.yaml':
>   "Vendor specific properties having a standard unit suffix don't need a type."
> ---
>  .../bindings/media/i2c/maxim,max9286.yaml     | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> index 9ea827092fdd..b22ba3e0db4a 100644
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> @@ -51,6 +51,26 @@ properties:
>    '#gpio-cells':
>      const: 2
> 
> +  maxim,reverse-channel-microvolt:
> +    minimum: 30000
> +    maximum: 200000
> +    default: 170000
> +    description: |
> +      Initial amplitude of the reverse control channel, in micro volts.
> +
> +      The initial amplitude shall be adjusted to a value compatible with the
> +      configuration of the connected remote serializer.
> +
> +      Some camera modules (for example RDACM20) include an on-board MCU that
> +      pre-programs the embedded serializer with power supply noise immunity
> +      (high-threshold) enabled. A typical value of the deserializer's reverse
> +      channel amplitude to communicate with pre-programmed serializers is
> +      170000 micro volts.
> +
> +      A typical value for the reverse channel amplitude to communicate with
> +      a remote serializer whose high-threshold noise immunity is not enabled
> +      is 100000 micro volts
> +
>    ports:
>      type: object
>      description: |
> @@ -221,6 +241,7 @@ required:
>    - ports
>    - i2c-mux
>    - gpio-controller
> +  - maxim,reverse-channel-microvolt
> 
>  additionalProperties: false
> 
> @@ -243,6 +264,8 @@ examples:
>          gpio-controller;
>          #gpio-cells = <2>;
> 
> +        maxim,reverse-channel-microvolt = <170000>;
> +
>          ports {
>            #address-cells = <1>;
>            #size-cells = <0>;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 2/5] dt-bindings: media: max9286: Document 'maxim,reverse-channel-microvolt'

[Laurent Pinchart]

On Wed, Dec 16, 2020 at 07:05:34PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Tue, Dec 15, 2020 at 06:09:54PM +0100, Jacopo Mondi wrote:
> > Document the 'reverse-channel-microvolt' vendor property in the
> > bindings document of the max9286 driver.
> > 
> > The newly introduced property allows to specifying the initial
> > configuration of the GMSL reverse control channel to accommodate
> > remote serializers pre-programmed with the high threshold power
> > supply noise immunity enabled.
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > ---
> > v5->v6:
> > - Use standard unit suffix 'microvolt' for the custom property
> > - Drop '$ref' as according to 'example-schema.yaml':
> >   "Vendor specific properties having a standard unit suffix don't need a type."
> > ---
> >  .../bindings/media/i2c/maxim,max9286.yaml     | 23 +++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > index 9ea827092fdd..b22ba3e0db4a 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > @@ -51,6 +51,26 @@ properties:
> >    '#gpio-cells':
> >      const: 2
> > 
> > +  maxim,reverse-channel-microvolt:
> > +    minimum: 30000
> > +    maximum: 200000
> > +    default: 170000
> > +    description: |
> > +      Initial amplitude of the reverse control channel, in micro volts.
> > +
> > +      The initial amplitude shall be adjusted to a value compatible with the
> > +      configuration of the connected remote serializer.
> > +
> > +      Some camera modules (for example RDACM20) include an on-board MCU that
> > +      pre-programs the embedded serializer with power supply noise immunity
> > +      (high-threshold) enabled. A typical value of the deserializer's reverse
> > +      channel amplitude to communicate with pre-programmed serializers is
> > +      170000 micro volts.
> > +
> > +      A typical value for the reverse channel amplitude to communicate with
> > +      a remote serializer whose high-threshold noise immunity is not enabled
> > +      is 100000 micro volts
> > +
> >    ports:
> >      type: object
> >      description: |
> > @@ -221,6 +241,7 @@ required:
> >    - ports
> >    - i2c-mux
> >    - gpio-controller
> > +  - maxim,reverse-channel-microvolt

One comment though: You specify a default value above, which isn't very
useful when the property is required. Should we either drop the default
value, or make the property optional ?

> > 
> >  additionalProperties: false
> > 
> > @@ -243,6 +264,8 @@ examples:
> >          gpio-controller;
> >          #gpio-cells = <2>;
> > 
> > +        maxim,reverse-channel-microvolt = <170000>;
> > +
> >          ports {
> >            #address-cells = <1>;
> >            #size-cells = <0>;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 2/5] dt-bindings: media: max9286: Document 'maxim,reverse-channel-microvolt'

[Rob Herring]

On Wed, Dec 16, 2020 at 07:17:05PM +0200, Laurent Pinchart wrote:
> On Wed, Dec 16, 2020 at 07:05:34PM +0200, Laurent Pinchart wrote:
> > Hi Jacopo,
> > 
> > Thank you for the patch.
> > 
> > On Tue, Dec 15, 2020 at 06:09:54PM +0100, Jacopo Mondi wrote:
> > > Document the 'reverse-channel-microvolt' vendor property in the
> > > bindings document of the max9286 driver.
> > > 
> > > The newly introduced property allows to specifying the initial
> > > configuration of the GMSL reverse control channel to accommodate
> > > remote serializers pre-programmed with the high threshold power
> > > supply noise immunity enabled.
> > > 
> > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > > ---
> > > v5->v6:
> > > - Use standard unit suffix 'microvolt' for the custom property
> > > - Drop '$ref' as according to 'example-schema.yaml':
> > >   "Vendor specific properties having a standard unit suffix don't need a type."
> > > ---
> > >  .../bindings/media/i2c/maxim,max9286.yaml     | 23 +++++++++++++++++++
> > >  1 file changed, 23 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > index 9ea827092fdd..b22ba3e0db4a 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > @@ -51,6 +51,26 @@ properties:
> > >    '#gpio-cells':
> > >      const: 2
> > > 
> > > +  maxim,reverse-channel-microvolt:
> > > +    minimum: 30000
> > > +    maximum: 200000
> > > +    default: 170000
> > > +    description: |
> > > +      Initial amplitude of the reverse control channel, in micro volts.
> > > +
> > > +      The initial amplitude shall be adjusted to a value compatible with the
> > > +      configuration of the connected remote serializer.
> > > +
> > > +      Some camera modules (for example RDACM20) include an on-board MCU that
> > > +      pre-programs the embedded serializer with power supply noise immunity
> > > +      (high-threshold) enabled. A typical value of the deserializer's reverse
> > > +      channel amplitude to communicate with pre-programmed serializers is
> > > +      170000 micro volts.
> > > +
> > > +      A typical value for the reverse channel amplitude to communicate with
> > > +      a remote serializer whose high-threshold noise immunity is not enabled
> > > +      is 100000 micro volts
> > > +
> > >    ports:
> > >      type: object
> > >      description: |
> > > @@ -221,6 +241,7 @@ required:
> > >    - ports
> > >    - i2c-mux
> > >    - gpio-controller
> > > +  - maxim,reverse-channel-microvolt
> 
> One comment though: You specify a default value above, which isn't very
> useful when the property is required. Should we either drop the default
> value, or make the property optional ?

And generally added properties can't be required unless for some reason 
DT's without the property are broken.

With required dropped,

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

Rob

Re: [PATCH v6 2/5] dt-bindings: media: max9286: Document 'maxim,reverse-channel-microvolt'

[Jacopo Mondi]

Hi Rob, Laurent,

On Mon, Dec 21, 2020 at 11:58:27AM -0700, Rob Herring wrote:
> On Wed, Dec 16, 2020 at 07:17:05PM +0200, Laurent Pinchart wrote:
> > > > @@ -221,6 +241,7 @@ required:
> > > >    - ports
> > > >    - i2c-mux
> > > >    - gpio-controller
> > > > +  - maxim,reverse-channel-microvolt
> >
> > One comment though: You specify a default value above, which isn't very
> > useful when the property is required. Should we either drop the default
> > value, or make the property optional ?
>
> And generally added properties can't be required unless for some reason
> DT's without the property are broken.

My thinking was to make it required for new DTS and specify a default
for the old ones that do not have the property. I'll drop required and
keep the default value in next version.

Thanks
  j

>
> With required dropped,
>
> Reviewed-by: Rob Herring <robh@kernel.org>
>
> Rob

[PATCH v6 3/5] media: i2c: max9286: Break-out reverse channel setup

[Jacopo Mondi]

Break out the reverse channel setup configuration procedure to its own
function.

This change prepares for configuring the reverse channel conditionally
to the remote side high threshold configuration.

No functional changes intended.

Reviewed-by: Kieran Bingham <kieran.bingham+renesasa@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index c82c1493e099..1cfc8801c0b2 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -336,6 +336,22 @@ static void max9286_configure_i2c(struct max9286_priv *priv, bool localack)
 	usleep_range(3000, 5000);
 }
 
+static void max9286_reverse_channel_setup(struct max9286_priv *priv)
+{
+	/*
+	 * Reverse channel setup.
+	 *
+	 * - Enable custom reverse channel configuration (through register 0x3f)
+	 *   and set the first pulse length to 35 clock cycles.
+	 * - Increase the reverse channel amplitude to 170mV to accommodate the
+	 *   high threshold enabled by the serializer driver.
+	 */
+	max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35));
+	max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) | MAX9286_REV_AMP(70) |
+		      MAX9286_REV_AMP_X);
+	usleep_range(2000, 2500);
+}
+
 /*
  * max9286_check_video_links() - Make sure video links are detected and locked
  *
@@ -941,19 +957,7 @@ static int max9286_setup(struct max9286_priv *priv)
 	 * only. This should be disabled after the mux is initialised.
 	 */
 	max9286_configure_i2c(priv, true);
-
-	/*
-	 * Reverse channel setup.
-	 *
-	 * - Enable custom reverse channel configuration (through register 0x3f)
-	 *   and set the first pulse length to 35 clock cycles.
-	 * - Increase the reverse channel amplitude to 170mV to accommodate the
-	 *   high threshold enabled by the serializer driver.
-	 */
-	max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35));
-	max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) | MAX9286_REV_AMP(70) |
-		      MAX9286_REV_AMP_X);
-	usleep_range(2000, 2500);
+	max9286_reverse_channel_setup(priv);
 
 	/*
 	 * Enable GMSL links, mask unused ones and autodetect link
-- 
2.29.2

Re: [PATCH v6 3/5] media: i2c: max9286: Break-out reverse channel setup

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Dec 15, 2020 at 06:09:55PM +0100, Jacopo Mondi wrote:
> Break out the reverse channel setup configuration procedure to its own
> function.
> 
> This change prepares for configuring the reverse channel conditionally
> to the remote side high threshold configuration.
> 
> No functional changes intended.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesasa@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

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

> ---
>  drivers/media/i2c/max9286.c | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index c82c1493e099..1cfc8801c0b2 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -336,6 +336,22 @@ static void max9286_configure_i2c(struct max9286_priv *priv, bool localack)
>  	usleep_range(3000, 5000);
>  }
>  
> +static void max9286_reverse_channel_setup(struct max9286_priv *priv)
> +{
> +	/*
> +	 * Reverse channel setup.
> +	 *
> +	 * - Enable custom reverse channel configuration (through register 0x3f)
> +	 *   and set the first pulse length to 35 clock cycles.
> +	 * - Increase the reverse channel amplitude to 170mV to accommodate the
> +	 *   high threshold enabled by the serializer driver.
> +	 */
> +	max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35));
> +	max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) | MAX9286_REV_AMP(70) |
> +		      MAX9286_REV_AMP_X);
> +	usleep_range(2000, 2500);
> +}
> +
>  /*
>   * max9286_check_video_links() - Make sure video links are detected and locked
>   *
> @@ -941,19 +957,7 @@ static int max9286_setup(struct max9286_priv *priv)
>  	 * only. This should be disabled after the mux is initialised.
>  	 */
>  	max9286_configure_i2c(priv, true);
> -
> -	/*
> -	 * Reverse channel setup.
> -	 *
> -	 * - Enable custom reverse channel configuration (through register 0x3f)
> -	 *   and set the first pulse length to 35 clock cycles.
> -	 * - Increase the reverse channel amplitude to 170mV to accommodate the
> -	 *   high threshold enabled by the serializer driver.
> -	 */
> -	max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35));
> -	max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) | MAX9286_REV_AMP(70) |
> -		      MAX9286_REV_AMP_X);
> -	usleep_range(2000, 2500);
> +	max9286_reverse_channel_setup(priv);
>  
>  	/*
>  	 * Enable GMSL links, mask unused ones and autodetect link

-- 
Regards,

Laurent Pinchart

[PATCH v6 4/5] media: i2c: max9286: Make channel amplitude programmable

[Jacopo Mondi]

Instrument the function that configures the reverse channel with a
programmable amplitude value.

This change serves to prepare to adjust the reverse channel amplitude
depending on the remote end high-threshold configuration.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 1cfc8801c0b2..021309c6dd6f 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -336,19 +336,29 @@ static void max9286_configure_i2c(struct max9286_priv *priv, bool localack)
 	usleep_range(3000, 5000);
 }
 
-static void max9286_reverse_channel_setup(struct max9286_priv *priv)
+static void max9286_reverse_channel_setup(struct max9286_priv *priv,
+					  unsigned int chan_amplitude)
 {
+	/* Reverse channel transmission time: default to 1. */
+	u8 chan_config = MAX9286_REV_TRF(1);
+
 	/*
 	 * Reverse channel setup.
 	 *
 	 * - Enable custom reverse channel configuration (through register 0x3f)
 	 *   and set the first pulse length to 35 clock cycles.
-	 * - Increase the reverse channel amplitude to 170mV to accommodate the
-	 *   high threshold enabled by the serializer driver.
+	 * - Adjust reverse channel amplitude: values > 130 are programmed
+	 *   using the additional +100mV REV_AMP_X boost flag
 	 */
 	max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35));
-	max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) | MAX9286_REV_AMP(70) |
-		      MAX9286_REV_AMP_X);
+
+	if (chan_amplitude > 100) {
+		/* It is not possible to express values (100 < x < 130) */
+		chan_amplitude = chan_amplitude < 130
+			       ? 30 : chan_amplitude - 100;
+		chan_config |= MAX9286_REV_AMP_X;
+	}
+	max9286_write(priv, 0x3b, chan_config | MAX9286_REV_AMP(chan_amplitude));
 	usleep_range(2000, 2500);
 }
 
@@ -957,7 +967,7 @@ static int max9286_setup(struct max9286_priv *priv)
 	 * only. This should be disabled after the mux is initialised.
 	 */
 	max9286_configure_i2c(priv, true);
-	max9286_reverse_channel_setup(priv);
+	max9286_reverse_channel_setup(priv, 170);
 
 	/*
 	 * Enable GMSL links, mask unused ones and autodetect link
-- 
2.29.2

Re: [PATCH v6 4/5] media: i2c: max9286: Make channel amplitude programmable

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Dec 15, 2020 at 06:09:56PM +0100, Jacopo Mondi wrote:
> Instrument the function that configures the reverse channel with a
> programmable amplitude value.
> 
> This change serves to prepare to adjust the reverse channel amplitude
> depending on the remote end high-threshold configuration.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 1cfc8801c0b2..021309c6dd6f 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -336,19 +336,29 @@ static void max9286_configure_i2c(struct max9286_priv *priv, bool localack)
>  	usleep_range(3000, 5000);
>  }
>  
> -static void max9286_reverse_channel_setup(struct max9286_priv *priv)
> +static void max9286_reverse_channel_setup(struct max9286_priv *priv,
> +					  unsigned int chan_amplitude)
>  {
> +	/* Reverse channel transmission time: default to 1. */
> +	u8 chan_config = MAX9286_REV_TRF(1);
> +
>  	/*
>  	 * Reverse channel setup.
>  	 *
>  	 * - Enable custom reverse channel configuration (through register 0x3f)
>  	 *   and set the first pulse length to 35 clock cycles.
> -	 * - Increase the reverse channel amplitude to 170mV to accommodate the
> -	 *   high threshold enabled by the serializer driver.
> +	 * - Adjust reverse channel amplitude: values > 130 are programmed
> +	 *   using the additional +100mV REV_AMP_X boost flag
>  	 */
>  	max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35));
> -	max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) | MAX9286_REV_AMP(70) |
> -		      MAX9286_REV_AMP_X);
> +
> +	if (chan_amplitude > 100) {
> +		/* It is not possible to express values (100 < x < 130) */
> +		chan_amplitude = chan_amplitude < 130
> +			       ? 30 : chan_amplitude - 100;

This could also be written

		chan_amplitude = min(30, chan_amplitude - 100);

With or without the change,

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

> +		chan_config |= MAX9286_REV_AMP_X;
> +	}
> +	max9286_write(priv, 0x3b, chan_config | MAX9286_REV_AMP(chan_amplitude));
>  	usleep_range(2000, 2500);
>  }
>  
> @@ -957,7 +967,7 @@ static int max9286_setup(struct max9286_priv *priv)
>  	 * only. This should be disabled after the mux is initialised.
>  	 */
>  	max9286_configure_i2c(priv, true);
> -	max9286_reverse_channel_setup(priv);
> +	max9286_reverse_channel_setup(priv, 170);
>  
>  	/*
>  	 * Enable GMSL links, mask unused ones and autodetect link

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 4/5] media: i2c: max9286: Make channel amplitude programmable

[Laurent Pinchart]

On Wed, Dec 16, 2020 at 07:14:25PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Tue, Dec 15, 2020 at 06:09:56PM +0100, Jacopo Mondi wrote:
> > Instrument the function that configures the reverse channel with a
> > programmable amplitude value.
> > 
> > This change serves to prepare to adjust the reverse channel amplitude
> > depending on the remote end high-threshold configuration.
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/max9286.c | 22 ++++++++++++++++------
> >  1 file changed, 16 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 1cfc8801c0b2..021309c6dd6f 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -336,19 +336,29 @@ static void max9286_configure_i2c(struct max9286_priv *priv, bool localack)
> >  	usleep_range(3000, 5000);
> >  }
> >  
> > -static void max9286_reverse_channel_setup(struct max9286_priv *priv)
> > +static void max9286_reverse_channel_setup(struct max9286_priv *priv,
> > +					  unsigned int chan_amplitude)
> >  {
> > +	/* Reverse channel transmission time: default to 1. */
> > +	u8 chan_config = MAX9286_REV_TRF(1);
> > +
> >  	/*
> >  	 * Reverse channel setup.
> >  	 *
> >  	 * - Enable custom reverse channel configuration (through register 0x3f)
> >  	 *   and set the first pulse length to 35 clock cycles.
> > -	 * - Increase the reverse channel amplitude to 170mV to accommodate the
> > -	 *   high threshold enabled by the serializer driver.
> > +	 * - Adjust reverse channel amplitude: values > 130 are programmed
> > +	 *   using the additional +100mV REV_AMP_X boost flag
> >  	 */
> >  	max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35));
> > -	max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) | MAX9286_REV_AMP(70) |
> > -		      MAX9286_REV_AMP_X);
> > +
> > +	if (chan_amplitude > 100) {
> > +		/* It is not possible to express values (100 < x < 130) */
> > +		chan_amplitude = chan_amplitude < 130
> > +			       ? 30 : chan_amplitude - 100;
> 
> This could also be written
> 
> 		chan_amplitude = min(30, chan_amplitude - 100);

s/min/max/ of course.

> 
> With or without the change,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +		chan_config |= MAX9286_REV_AMP_X;
> > +	}
> > +	max9286_write(priv, 0x3b, chan_config | MAX9286_REV_AMP(chan_amplitude));
> >  	usleep_range(2000, 2500);
> >  }
> >  
> > @@ -957,7 +967,7 @@ static int max9286_setup(struct max9286_priv *priv)
> >  	 * only. This should be disabled after the mux is initialised.
> >  	 */
> >  	max9286_configure_i2c(priv, true);
> > -	max9286_reverse_channel_setup(priv);
> > +	max9286_reverse_channel_setup(priv, 170);
> >  
> >  	/*
> >  	 * Enable GMSL links, mask unused ones and autodetect link

-- 
Regards,

Laurent Pinchart

[PATCH v6 5/5] media: i2c: max9286: Configure reverse channel amplitude

[Jacopo Mondi]

Adjust the initial reverse channel amplitude parsing from
firmware interface the 'maxim,reverse-channel-microvolt'
property.

This change is required for both rdacm20 and rdacm21 camera
modules to be correctly probed when used in combination with
the max9286 deserializer.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 021309c6dd6f..9b40a4890c4d 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -163,6 +163,8 @@ struct max9286_priv {
 	unsigned int mux_channel;
 	bool mux_open;
 
+	u32 reverse_channel_mv;
+
 	struct v4l2_ctrl_handler ctrls;
 	struct v4l2_ctrl *pixelrate;
 
@@ -557,10 +559,14 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
 	 * All enabled sources have probed and enabled their reverse control
 	 * channels:
 	 *
+	 * - Increase the reverse channel amplitude to compensate for the
+	 *   remote ends high threshold, if not done already
 	 * - Verify all configuration links are properly detected
 	 * - Disable auto-ack as communication on the control channel are now
 	 *   stable.
 	 */
+	if (priv->reverse_channel_mv < 170)
+		max9286_reverse_channel_setup(priv, 170);
 	max9286_check_config_link(priv, priv->source_mask);
 
 	/*
@@ -967,7 +973,7 @@ static int max9286_setup(struct max9286_priv *priv)
 	 * only. This should be disabled after the mux is initialised.
 	 */
 	max9286_configure_i2c(priv, true);
-	max9286_reverse_channel_setup(priv, 170);
+	max9286_reverse_channel_setup(priv, priv->reverse_channel_mv);
 
 	/*
 	 * Enable GMSL links, mask unused ones and autodetect link
@@ -1131,6 +1137,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 	struct device_node *i2c_mux;
 	struct device_node *node = NULL;
 	unsigned int i2c_mux_mask = 0;
+	u32 reverse_channel_microvolt;
 
 	/* Balance the of_node_put() performed by of_find_node_by_name(). */
 	of_node_get(dev->of_node);
@@ -1221,6 +1228,20 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 	}
 	of_node_put(node);
 
+	/*
+	 * Parse the initial value of the reverse channel amplitude from
+	 * the firmware interface and convert it to millivolts.
+	 *
+	 * Default it to 170mV for backward compatibility with DTBs that do not
+	 * provide the property.
+	 */
+	if (of_property_read_u32(dev->of_node,
+				 "maxim,reverse-channel-microvolt",
+				 &reverse_channel_microvolt))
+		priv->reverse_channel_mv = 170;
+	else
+		priv->reverse_channel_mv = reverse_channel_microvolt / 1000U;
+
 	priv->route_mask = priv->source_mask;
 
 	return 0;
-- 
2.29.2

Re: [PATCH v6 5/5] media: i2c: max9286: Configure reverse channel amplitude

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Dec 15, 2020 at 06:09:57PM +0100, Jacopo Mondi wrote:
> Adjust the initial reverse channel amplitude parsing from
> firmware interface the 'maxim,reverse-channel-microvolt'
> property.
> 
> This change is required for both rdacm20 and rdacm21 camera
> modules to be correctly probed when used in combination with
> the max9286 deserializer.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 021309c6dd6f..9b40a4890c4d 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -163,6 +163,8 @@ struct max9286_priv {
>  	unsigned int mux_channel;
>  	bool mux_open;
>  
> +	u32 reverse_channel_mv;
> +
>  	struct v4l2_ctrl_handler ctrls;
>  	struct v4l2_ctrl *pixelrate;
>  
> @@ -557,10 +559,14 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
>  	 * All enabled sources have probed and enabled their reverse control
>  	 * channels:
>  	 *
> +	 * - Increase the reverse channel amplitude to compensate for the
> +	 *   remote ends high threshold, if not done already
>  	 * - Verify all configuration links are properly detected
>  	 * - Disable auto-ack as communication on the control channel are now
>  	 *   stable.
>  	 */
> +	if (priv->reverse_channel_mv < 170)
> +		max9286_reverse_channel_setup(priv, 170);

I'm beginning to wonder if there will be a need in the future to not
increase the reverse channel amplitude (keeping the threshold low on the
remote side). An increased amplitude increases power consumption, and if
the environment isn't noisy, a low amplitude would work. The device tree
would then need to specify both the initial amplitude required by the
remote side, and the desired amplitude after initialization. What do you
think ? Is it overkill ? We don't have to implement this now, so

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

but if this feature could be required later, we may want to take into
account in the naming of the new DT property to reflect the fact that it
is the initial value.

>  	max9286_check_config_link(priv, priv->source_mask);
>  
>  	/*
> @@ -967,7 +973,7 @@ static int max9286_setup(struct max9286_priv *priv)
>  	 * only. This should be disabled after the mux is initialised.
>  	 */
>  	max9286_configure_i2c(priv, true);
> -	max9286_reverse_channel_setup(priv, 170);
> +	max9286_reverse_channel_setup(priv, priv->reverse_channel_mv);
>  
>  	/*
>  	 * Enable GMSL links, mask unused ones and autodetect link
> @@ -1131,6 +1137,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  	struct device_node *i2c_mux;
>  	struct device_node *node = NULL;
>  	unsigned int i2c_mux_mask = 0;
> +	u32 reverse_channel_microvolt;
>  
>  	/* Balance the of_node_put() performed by of_find_node_by_name(). */
>  	of_node_get(dev->of_node);
> @@ -1221,6 +1228,20 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  	}
>  	of_node_put(node);
>  
> +	/*
> +	 * Parse the initial value of the reverse channel amplitude from
> +	 * the firmware interface and convert it to millivolts.
> +	 *
> +	 * Default it to 170mV for backward compatibility with DTBs that do not
> +	 * provide the property.
> +	 */
> +	if (of_property_read_u32(dev->of_node,
> +				 "maxim,reverse-channel-microvolt",
> +				 &reverse_channel_microvolt))
> +		priv->reverse_channel_mv = 170;
> +	else
> +		priv->reverse_channel_mv = reverse_channel_microvolt / 1000U;
> +
>  	priv->route_mask = priv->source_mask;
>  
>  	return 0;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 5/5] media: i2c: max9286: Configure reverse channel amplitude

[Jacopo Mondi]

Hi Laurent,

On Wed, Dec 16, 2020 at 07:22:17PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Dec 15, 2020 at 06:09:57PM +0100, Jacopo Mondi wrote:
> > Adjust the initial reverse channel amplitude parsing from
> > firmware interface the 'maxim,reverse-channel-microvolt'
> > property.
> >
> > This change is required for both rdacm20 and rdacm21 camera
> > modules to be correctly probed when used in combination with
> > the max9286 deserializer.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/max9286.c | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 021309c6dd6f..9b40a4890c4d 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -163,6 +163,8 @@ struct max9286_priv {
> >  	unsigned int mux_channel;
> >  	bool mux_open;
> >
> > +	u32 reverse_channel_mv;
> > +
> >  	struct v4l2_ctrl_handler ctrls;
> >  	struct v4l2_ctrl *pixelrate;
> >
> > @@ -557,10 +559,14 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
> >  	 * All enabled sources have probed and enabled their reverse control
> >  	 * channels:
> >  	 *
> > +	 * - Increase the reverse channel amplitude to compensate for the
> > +	 *   remote ends high threshold, if not done already
> >  	 * - Verify all configuration links are properly detected
> >  	 * - Disable auto-ack as communication on the control channel are now
> >  	 *   stable.
> >  	 */
> > +	if (priv->reverse_channel_mv < 170)
> > +		max9286_reverse_channel_setup(priv, 170);
>
> I'm beginning to wonder if there will be a need in the future to not
> increase the reverse channel amplitude (keeping the threshold low on the
> remote side). An increased amplitude increases power consumption, and if
> the environment isn't noisy, a low amplitude would work. The device tree
> would then need to specify both the initial amplitude required by the
> remote side, and the desired amplitude after initialization. What do you
> think ? Is it overkill ? We don't have to implement this now, so
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> but if this feature could be required later, we may want to take into
> account in the naming of the new DT property to reflect the fact that it
> is the initial value.

I had the same thought when I initially proposed
"maxim,initial-reverse-channel-mV"

Having to use the standard unit suffix that would have become
"maxim,initial-reverse-channel-microvolt"
which is extremely long.

I can't tell if there will be any need to adjust the amplitude later.
In any case, I would not rely on a DTS property to do so, as once we
have probed the remote we have a subdev where to call
'get_mbus_config()' on, and from there we can report the high threshold
status of the serializer and adjust the deser amplitude accordingly.

The property documentation clearly says the there specified amplitude
is 'initial' many times, so I don't think it is strictly necessary to
report it in the name too.

Would this work for you ?

>
> >  	max9286_check_config_link(priv, priv->source_mask);
> >
> >  	/*
> > @@ -967,7 +973,7 @@ static int max9286_setup(struct max9286_priv *priv)
> >  	 * only. This should be disabled after the mux is initialised.
> >  	 */
> >  	max9286_configure_i2c(priv, true);
> > -	max9286_reverse_channel_setup(priv, 170);
> > +	max9286_reverse_channel_setup(priv, priv->reverse_channel_mv);
> >
> >  	/*
> >  	 * Enable GMSL links, mask unused ones and autodetect link
> > @@ -1131,6 +1137,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> >  	struct device_node *i2c_mux;
> >  	struct device_node *node = NULL;
> >  	unsigned int i2c_mux_mask = 0;
> > +	u32 reverse_channel_microvolt;
> >
> >  	/* Balance the of_node_put() performed by of_find_node_by_name(). */
> >  	of_node_get(dev->of_node);
> > @@ -1221,6 +1228,20 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> >  	}
> >  	of_node_put(node);
> >
> > +	/*
> > +	 * Parse the initial value of the reverse channel amplitude from
> > +	 * the firmware interface and convert it to millivolts.
> > +	 *
> > +	 * Default it to 170mV for backward compatibility with DTBs that do not
> > +	 * provide the property.
> > +	 */
> > +	if (of_property_read_u32(dev->of_node,
> > +				 "maxim,reverse-channel-microvolt",
> > +				 &reverse_channel_microvolt))
> > +		priv->reverse_channel_mv = 170;
> > +	else
> > +		priv->reverse_channel_mv = reverse_channel_microvolt / 1000U;
> > +
> >  	priv->route_mask = priv->source_mask;
> >
> >  	return 0;
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v6 5/5] media: i2c: max9286: Configure reverse channel amplitude

[Laurent Pinchart]

Hi Jacopo,

On Mon, Jan 11, 2021 at 11:43:11AM +0100, Jacopo Mondi wrote:
> On Wed, Dec 16, 2020 at 07:22:17PM +0200, Laurent Pinchart wrote:
> > On Tue, Dec 15, 2020 at 06:09:57PM +0100, Jacopo Mondi wrote:
> > > Adjust the initial reverse channel amplitude parsing from
> > > firmware interface the 'maxim,reverse-channel-microvolt'
> > > property.
> > >
> > > This change is required for both rdacm20 and rdacm21 camera
> > > modules to be correctly probed when used in combination with
> > > the max9286 deserializer.
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  drivers/media/i2c/max9286.c | 23 ++++++++++++++++++++++-
> > >  1 file changed, 22 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > index 021309c6dd6f..9b40a4890c4d 100644
> > > --- a/drivers/media/i2c/max9286.c
> > > +++ b/drivers/media/i2c/max9286.c
> > > @@ -163,6 +163,8 @@ struct max9286_priv {
> > >  	unsigned int mux_channel;
> > >  	bool mux_open;
> > >
> > > +	u32 reverse_channel_mv;
> > > +
> > >  	struct v4l2_ctrl_handler ctrls;
> > >  	struct v4l2_ctrl *pixelrate;
> > >
> > > @@ -557,10 +559,14 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
> > >  	 * All enabled sources have probed and enabled their reverse control
> > >  	 * channels:
> > >  	 *
> > > +	 * - Increase the reverse channel amplitude to compensate for the
> > > +	 *   remote ends high threshold, if not done already
> > >  	 * - Verify all configuration links are properly detected
> > >  	 * - Disable auto-ack as communication on the control channel are now
> > >  	 *   stable.
> > >  	 */
> > > +	if (priv->reverse_channel_mv < 170)
> > > +		max9286_reverse_channel_setup(priv, 170);
> >
> > I'm beginning to wonder if there will be a need in the future to not
> > increase the reverse channel amplitude (keeping the threshold low on the
> > remote side). An increased amplitude increases power consumption, and if
> > the environment isn't noisy, a low amplitude would work. The device tree
> > would then need to specify both the initial amplitude required by the
> > remote side, and the desired amplitude after initialization. What do you
> > think ? Is it overkill ? We don't have to implement this now, so
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > but if this feature could be required later, we may want to take into
> > account in the naming of the new DT property to reflect the fact that it
> > is the initial value.
> 
> I had the same thought when I initially proposed
> "maxim,initial-reverse-channel-mV"
> 
> Having to use the standard unit suffix that would have become
> "maxim,initial-reverse-channel-microvolt"
> which is extremely long.
> 
> I can't tell if there will be any need to adjust the amplitude later.
> In any case, I would not rely on a DTS property to do so, as once we
> have probed the remote we have a subdev where to call
> 'get_mbus_config()' on, and from there we can report the high threshold
> status of the serializer and adjust the deser amplitude accordingly.

I don't think that's the point. The threshold of the serializer is
something we can configure at runtime. What voltage level to use after
initialization time is a system property as it depends on noise
immunity, so we'll have to specify it in DT.

> The property documentation clearly says the there specified amplitude
> is 'initial' many times, so I don't think it is strictly necessary to
> report it in the name too.
> 
> Would this work for you ?

I don't mind either way.

> > >  	max9286_check_config_link(priv, priv->source_mask);
> > >
> > >  	/*
> > > @@ -967,7 +973,7 @@ static int max9286_setup(struct max9286_priv *priv)
> > >  	 * only. This should be disabled after the mux is initialised.
> > >  	 */
> > >  	max9286_configure_i2c(priv, true);
> > > -	max9286_reverse_channel_setup(priv, 170);
> > > +	max9286_reverse_channel_setup(priv, priv->reverse_channel_mv);
> > >
> > >  	/*
> > >  	 * Enable GMSL links, mask unused ones and autodetect link
> > > @@ -1131,6 +1137,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> > >  	struct device_node *i2c_mux;
> > >  	struct device_node *node = NULL;
> > >  	unsigned int i2c_mux_mask = 0;
> > > +	u32 reverse_channel_microvolt;
> > >
> > >  	/* Balance the of_node_put() performed by of_find_node_by_name(). */
> > >  	of_node_get(dev->of_node);
> > > @@ -1221,6 +1228,20 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> > >  	}
> > >  	of_node_put(node);
> > >
> > > +	/*
> > > +	 * Parse the initial value of the reverse channel amplitude from
> > > +	 * the firmware interface and convert it to millivolts.
> > > +	 *
> > > +	 * Default it to 170mV for backward compatibility with DTBs that do not
> > > +	 * provide the property.
> > > +	 */
> > > +	if (of_property_read_u32(dev->of_node,
> > > +				 "maxim,reverse-channel-microvolt",
> > > +				 &reverse_channel_microvolt))
> > > +		priv->reverse_channel_mv = 170;
> > > +	else
> > > +		priv->reverse_channel_mv = reverse_channel_microvolt / 1000U;
> > > +
> > >  	priv->route_mask = priv->source_mask;
> > >
> > >  	return 0;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 5/5] media: i2c: max9286: Configure reverse channel amplitude

[Jacopo Mondi]

Hi Laurent,

On Mon, Jan 11, 2021 at 12:58:59PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Jan 11, 2021 at 11:43:11AM +0100, Jacopo Mondi wrote:
> > On Wed, Dec 16, 2020 at 07:22:17PM +0200, Laurent Pinchart wrote:
> > > On Tue, Dec 15, 2020 at 06:09:57PM +0100, Jacopo Mondi wrote:
> > > > Adjust the initial reverse channel amplitude parsing from
> > > > firmware interface the 'maxim,reverse-channel-microvolt'
> > > > property.
> > > >
> > > > This change is required for both rdacm20 and rdacm21 camera
> > > > modules to be correctly probed when used in combination with
> > > > the max9286 deserializer.
> > > >
> > > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > ---
> > > >  drivers/media/i2c/max9286.c | 23 ++++++++++++++++++++++-
> > > >  1 file changed, 22 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > > index 021309c6dd6f..9b40a4890c4d 100644
> > > > --- a/drivers/media/i2c/max9286.c
> > > > +++ b/drivers/media/i2c/max9286.c
> > > > @@ -163,6 +163,8 @@ struct max9286_priv {
> > > >  	unsigned int mux_channel;
> > > >  	bool mux_open;
> > > >
> > > > +	u32 reverse_channel_mv;
> > > > +
> > > >  	struct v4l2_ctrl_handler ctrls;
> > > >  	struct v4l2_ctrl *pixelrate;
> > > >
> > > > @@ -557,10 +559,14 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
> > > >  	 * All enabled sources have probed and enabled their reverse control
> > > >  	 * channels:
> > > >  	 *
> > > > +	 * - Increase the reverse channel amplitude to compensate for the
> > > > +	 *   remote ends high threshold, if not done already
> > > >  	 * - Verify all configuration links are properly detected
> > > >  	 * - Disable auto-ack as communication on the control channel are now
> > > >  	 *   stable.
> > > >  	 */
> > > > +	if (priv->reverse_channel_mv < 170)
> > > > +		max9286_reverse_channel_setup(priv, 170);
> > >
> > > I'm beginning to wonder if there will be a need in the future to not
> > > increase the reverse channel amplitude (keeping the threshold low on the
> > > remote side). An increased amplitude increases power consumption, and if
> > > the environment isn't noisy, a low amplitude would work. The device tree
> > > would then need to specify both the initial amplitude required by the
> > > remote side, and the desired amplitude after initialization. What do you
> > > think ? Is it overkill ? We don't have to implement this now, so
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > > but if this feature could be required later, we may want to take into
> > > account in the naming of the new DT property to reflect the fact that it
> > > is the initial value.
> >
> > I had the same thought when I initially proposed
> > "maxim,initial-reverse-channel-mV"
> >
> > Having to use the standard unit suffix that would have become
> > "maxim,initial-reverse-channel-microvolt"
> > which is extremely long.
> >
> > I can't tell if there will be any need to adjust the amplitude later.
> > In any case, I would not rely on a DTS property to do so, as once we
> > have probed the remote we have a subdev where to call
> > 'get_mbus_config()' on, and from there we can report the high threshold
> > status of the serializer and adjust the deser amplitude accordingly.
>
> I don't think that's the point. The threshold of the serializer is
> something we can configure at runtime. What voltage level to use after

How so ? I mean, we can add an API for this, but currently it's
configured at probe time and that's it. Its configuration might as
well come from a DT property like we do on the deserializer here but I
fail to see why it's different. Both settings depends on the required
noise immunity of th system.

> initialization time is a system property as it depends on noise
> immunity, so we'll have to specify it in DT.

I don't see it differently than what happens on the serializer. We can
add an API if we want to, but it's configured at probe time (initial
value) and later can be adjusted in reponse to the serializer
configuration setting.

I feel like we're on different pages :/

>
> > The property documentation clearly says the there specified amplitude
> > is 'initial' many times, so I don't think it is strictly necessary to
> > report it in the name too.
> >
> > Would this work for you ?
>
> I don't mind either way.
>
> > > >  	max9286_check_config_link(priv, priv->source_mask);
> > > >
> > > >  	/*
> > > > @@ -967,7 +973,7 @@ static int max9286_setup(struct max9286_priv *priv)
> > > >  	 * only. This should be disabled after the mux is initialised.
> > > >  	 */
> > > >  	max9286_configure_i2c(priv, true);
> > > > -	max9286_reverse_channel_setup(priv, 170);
> > > > +	max9286_reverse_channel_setup(priv, priv->reverse_channel_mv);
> > > >
> > > >  	/*
> > > >  	 * Enable GMSL links, mask unused ones and autodetect link
> > > > @@ -1131,6 +1137,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> > > >  	struct device_node *i2c_mux;
> > > >  	struct device_node *node = NULL;
> > > >  	unsigned int i2c_mux_mask = 0;
> > > > +	u32 reverse_channel_microvolt;
> > > >
> > > >  	/* Balance the of_node_put() performed by of_find_node_by_name(). */
> > > >  	of_node_get(dev->of_node);
> > > > @@ -1221,6 +1228,20 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> > > >  	}
> > > >  	of_node_put(node);
> > > >
> > > > +	/*
> > > > +	 * Parse the initial value of the reverse channel amplitude from
> > > > +	 * the firmware interface and convert it to millivolts.
> > > > +	 *
> > > > +	 * Default it to 170mV for backward compatibility with DTBs that do not
> > > > +	 * provide the property.
> > > > +	 */
> > > > +	if (of_property_read_u32(dev->of_node,
> > > > +				 "maxim,reverse-channel-microvolt",
> > > > +				 &reverse_channel_microvolt))
> > > > +		priv->reverse_channel_mv = 170;
> > > > +	else
> > > > +		priv->reverse_channel_mv = reverse_channel_microvolt / 1000U;
> > > > +
> > > >  	priv->route_mask = priv->source_mask;
> > > >
> > > >  	return 0;
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v6 5/5] media: i2c: max9286: Configure reverse channel amplitude

[Laurent Pinchart]

Hi Jacopo,

On Mon, Jan 11, 2021 at 12:20:23PM +0100, Jacopo Mondi wrote:
> On Mon, Jan 11, 2021 at 12:58:59PM +0200, Laurent Pinchart wrote:
> > On Mon, Jan 11, 2021 at 11:43:11AM +0100, Jacopo Mondi wrote:
> >> On Wed, Dec 16, 2020 at 07:22:17PM +0200, Laurent Pinchart wrote:
> >>> On Tue, Dec 15, 2020 at 06:09:57PM +0100, Jacopo Mondi wrote:
> >>>> Adjust the initial reverse channel amplitude parsing from
> >>>> firmware interface the 'maxim,reverse-channel-microvolt'
> >>>> property.
> >>>>
> >>>> This change is required for both rdacm20 and rdacm21 camera
> >>>> modules to be correctly probed when used in combination with
> >>>> the max9286 deserializer.
> >>>>
> >>>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>>> ---
> >>>>  drivers/media/i2c/max9286.c | 23 ++++++++++++++++++++++-
> >>>>  1 file changed, 22 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> >>>> index 021309c6dd6f..9b40a4890c4d 100644
> >>>> --- a/drivers/media/i2c/max9286.c
> >>>> +++ b/drivers/media/i2c/max9286.c
> >>>> @@ -163,6 +163,8 @@ struct max9286_priv {
> >>>>  	unsigned int mux_channel;
> >>>>  	bool mux_open;
> >>>>
> >>>> +	u32 reverse_channel_mv;
> >>>> +
> >>>>  	struct v4l2_ctrl_handler ctrls;
> >>>>  	struct v4l2_ctrl *pixelrate;
> >>>>
> >>>> @@ -557,10 +559,14 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
> >>>>  	 * All enabled sources have probed and enabled their reverse control
> >>>>  	 * channels:
> >>>>  	 *
> >>>> +	 * - Increase the reverse channel amplitude to compensate for the
> >>>> +	 *   remote ends high threshold, if not done already
> >>>>  	 * - Verify all configuration links are properly detected
> >>>>  	 * - Disable auto-ack as communication on the control channel are now
> >>>>  	 *   stable.
> >>>>  	 */
> >>>> +	if (priv->reverse_channel_mv < 170)
> >>>> +		max9286_reverse_channel_setup(priv, 170);
> >>>
> >>> I'm beginning to wonder if there will be a need in the future to not
> >>> increase the reverse channel amplitude (keeping the threshold low on the
> >>> remote side). An increased amplitude increases power consumption, and if
> >>> the environment isn't noisy, a low amplitude would work. The device tree
> >>> would then need to specify both the initial amplitude required by the
> >>> remote side, and the desired amplitude after initialization. What do you
> >>> think ? Is it overkill ? We don't have to implement this now, so
> >>>
> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>
> >>> but if this feature could be required later, we may want to take into
> >>> account in the naming of the new DT property to reflect the fact that it
> >>> is the initial value.
> >>
> >> I had the same thought when I initially proposed
> >> "maxim,initial-reverse-channel-mV"
> >>
> >> Having to use the standard unit suffix that would have become
> >> "maxim,initial-reverse-channel-microvolt"
> >> which is extremely long.
> >>
> >> I can't tell if there will be any need to adjust the amplitude later.
> >> In any case, I would not rely on a DTS property to do so, as once we
> >> have probed the remote we have a subdev where to call
> >> 'get_mbus_config()' on, and from there we can report the high threshold
> >> status of the serializer and adjust the deser amplitude accordingly.
> >
> > I don't think that's the point. The threshold of the serializer is
> > something we can configure at runtime. What voltage level to use after
> 
> How so ? I mean, we can add an API for this, but currently it's
> configured at probe time and that's it. Its configuration might as
> well come from a DT property like we do on the deserializer here but I
> fail to see why it's different. Both settings depends on the required
> noise immunity of th system.

The voltage level configuration need to match between the tserializer
(transmitter) and the deserializer (receiver). The serializer is
configured with a voltage level, and the deserializer needs to be
configured with a corresponding threshold.

The voltage level of the serializer is configurable on the camera side
when the system is powered up. The RDACM20 has a microcontroller which
can configure the serializer, and other cameras may have similar
mechanisms. As the deserializer can't query the information from the
serializer (communication is unreliable if the threshold has an
incorrect value), we need a DT property to tell the deserializer what
threshold is initially used by the camera when it gets powered up.

This only covers initialization. A camera could boot up with a low
voltage level, but we may want to increase the voltage level (and thus
the threshold on the deserializer side) to increase noise immunity. Or,
if the system environment isn't noisy, we may want to keep a low voltage
level, or even decrease it if the camera boots up with a high voltage
level. This runtime voltage level depends on the system design and its
susceptibility to noise, and is thus a system property. Should we want
to make it configurable, it should be specified in DT, and it's separate
from the initial voltage level that is used to establish communication.

> > initialization time is a system property as it depends on noise
> > immunity, so we'll have to specify it in DT.
> 
> I don't see it differently than what happens on the serializer. We can
> add an API if we want to, but it's configured at probe time (initial
> value) and later can be adjusted in reponse to the serializer
> configuration setting.
> 
> I feel like we're on different pages :/
> 
> >> The property documentation clearly says the there specified amplitude
> >> is 'initial' many times, so I don't think it is strictly necessary to
> >> report it in the name too.
> >>
> >> Would this work for you ?
> >
> > I don't mind either way.
> >
> >>>>  	max9286_check_config_link(priv, priv->source_mask);
> >>>>
> >>>>  	/*
> >>>> @@ -967,7 +973,7 @@ static int max9286_setup(struct max9286_priv *priv)
> >>>>  	 * only. This should be disabled after the mux is initialised.
> >>>>  	 */
> >>>>  	max9286_configure_i2c(priv, true);
> >>>> -	max9286_reverse_channel_setup(priv, 170);
> >>>> +	max9286_reverse_channel_setup(priv, priv->reverse_channel_mv);
> >>>>
> >>>>  	/*
> >>>>  	 * Enable GMSL links, mask unused ones and autodetect link
> >>>> @@ -1131,6 +1137,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> >>>>  	struct device_node *i2c_mux;
> >>>>  	struct device_node *node = NULL;
> >>>>  	unsigned int i2c_mux_mask = 0;
> >>>> +	u32 reverse_channel_microvolt;
> >>>>
> >>>>  	/* Balance the of_node_put() performed by of_find_node_by_name(). */
> >>>>  	of_node_get(dev->of_node);
> >>>> @@ -1221,6 +1228,20 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> >>>>  	}
> >>>>  	of_node_put(node);
> >>>>
> >>>> +	/*
> >>>> +	 * Parse the initial value of the reverse channel amplitude from
> >>>> +	 * the firmware interface and convert it to millivolts.
> >>>> +	 *
> >>>> +	 * Default it to 170mV for backward compatibility with DTBs that do not
> >>>> +	 * provide the property.
> >>>> +	 */
> >>>> +	if (of_property_read_u32(dev->of_node,
> >>>> +				 "maxim,reverse-channel-microvolt",
> >>>> +				 &reverse_channel_microvolt))
> >>>> +		priv->reverse_channel_mv = 170;
> >>>> +	else
> >>>> +		priv->reverse_channel_mv = reverse_channel_microvolt / 1000U;
> >>>> +
> >>>>  	priv->route_mask = priv->source_mask;
> >>>>
> >>>>  	return 0;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 5/5] media: i2c: max9286: Configure reverse channel amplitude

[Jacopo Mondi]

Hi Laurent,

On Tue, Jan 12, 2021 at 07:03:42AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Jan 11, 2021 at 12:20:23PM +0100, Jacopo Mondi wrote:
> > On Mon, Jan 11, 2021 at 12:58:59PM +0200, Laurent Pinchart wrote:
> > > On Mon, Jan 11, 2021 at 11:43:11AM +0100, Jacopo Mondi wrote:
> > >> On Wed, Dec 16, 2020 at 07:22:17PM +0200, Laurent Pinchart wrote:
> > >>> On Tue, Dec 15, 2020 at 06:09:57PM +0100, Jacopo Mondi wrote:
> > >>>> Adjust the initial reverse channel amplitude parsing from
> > >>>> firmware interface the 'maxim,reverse-channel-microvolt'
> > >>>> property.
> > >>>>
> > >>>> This change is required for both rdacm20 and rdacm21 camera
> > >>>> modules to be correctly probed when used in combination with
> > >>>> the max9286 deserializer.
> > >>>>
> > >>>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > >>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > >>>> ---
> > >>>>  drivers/media/i2c/max9286.c | 23 ++++++++++++++++++++++-
> > >>>>  1 file changed, 22 insertions(+), 1 deletion(-)
> > >>>>
> > >>>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > >>>> index 021309c6dd6f..9b40a4890c4d 100644
> > >>>> --- a/drivers/media/i2c/max9286.c
> > >>>> +++ b/drivers/media/i2c/max9286.c
> > >>>> @@ -163,6 +163,8 @@ struct max9286_priv {
> > >>>>  	unsigned int mux_channel;
> > >>>>  	bool mux_open;
> > >>>>
> > >>>> +	u32 reverse_channel_mv;
> > >>>> +
> > >>>>  	struct v4l2_ctrl_handler ctrls;
> > >>>>  	struct v4l2_ctrl *pixelrate;
> > >>>>
> > >>>> @@ -557,10 +559,14 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
> > >>>>  	 * All enabled sources have probed and enabled their reverse control
> > >>>>  	 * channels:
> > >>>>  	 *
> > >>>> +	 * - Increase the reverse channel amplitude to compensate for the
> > >>>> +	 *   remote ends high threshold, if not done already
> > >>>>  	 * - Verify all configuration links are properly detected
> > >>>>  	 * - Disable auto-ack as communication on the control channel are now
> > >>>>  	 *   stable.
> > >>>>  	 */
> > >>>> +	if (priv->reverse_channel_mv < 170)
> > >>>> +		max9286_reverse_channel_setup(priv, 170);
> > >>>
> > >>> I'm beginning to wonder if there will be a need in the future to not
> > >>> increase the reverse channel amplitude (keeping the threshold low on the
> > >>> remote side). An increased amplitude increases power consumption, and if
> > >>> the environment isn't noisy, a low amplitude would work. The device tree
> > >>> would then need to specify both the initial amplitude required by the
> > >>> remote side, and the desired amplitude after initialization. What do you
> > >>> think ? Is it overkill ? We don't have to implement this now, so
> > >>>
> > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >>>
> > >>> but if this feature could be required later, we may want to take into
> > >>> account in the naming of the new DT property to reflect the fact that it
> > >>> is the initial value.
> > >>
> > >> I had the same thought when I initially proposed
> > >> "maxim,initial-reverse-channel-mV"
> > >>
> > >> Having to use the standard unit suffix that would have become
> > >> "maxim,initial-reverse-channel-microvolt"
> > >> which is extremely long.
> > >>
> > >> I can't tell if there will be any need to adjust the amplitude later.
> > >> In any case, I would not rely on a DTS property to do so, as once we
> > >> have probed the remote we have a subdev where to call
> > >> 'get_mbus_config()' on, and from there we can report the high threshold
> > >> status of the serializer and adjust the deser amplitude accordingly.
> > >
> > > I don't think that's the point. The threshold of the serializer is
> > > something we can configure at runtime. What voltage level to use after
> >
> > How so ? I mean, we can add an API for this, but currently it's
> > configured at probe time and that's it. Its configuration might as
> > well come from a DT property like we do on the deserializer here but I
> > fail to see why it's different. Both settings depends on the required
> > noise immunity of th system.
>
> The voltage level configuration need to match between the tserializer
> (transmitter) and the deserializer (receiver). The serializer is
> configured with a voltage level, and the deserializer needs to be
> configured with a corresponding threshold.
>

If I'm not mistaken it's actually the other way around, at least for
the chips we're dealing with.

The serializer (MAX9271) has an "Reverse Channel Receiver High
Threshold Enable" bit (register 0x08[0]) undocumented in the chip
manual but described in the "MAX9286 Programming Guide 2 10.pdf"
document in the "Important Registers" section.

The deserializer (MAX9286) has instead a configurable setting for the reverse
channel signal amplitude, which is what we are controlling in this
series.

The deserializer reverse channel amplitude has to match the remote
side 'high threshold enable' setting. If it is enabled the amplitude
has to be increased to be able to probe the remote side. If it's not
a lower amplitude has to be used to make comunication reliable.

As you said, some models (RDACM20) might be pre-programmed with the
'high threshold enable' bit set, and so the deserializer reverse
channel amplitude has to be adjusted accordingly to be able to
comunicate on the reverse channel.

> The voltage level of the serializer is configurable on the camera side
> when the system is powered up. The RDACM20 has a microcontroller which
> can configure the serializer, and other cameras may have similar
> mechanisms. As the deserializer can't query the information from the
> serializer (communication is unreliable if the threshold has an
> incorrect value), we need a DT property to tell the deserializer what
> threshold is initially used by the camera when it gets powered up.
>

That's what this series does, yes.

> This only covers initialization. A camera could boot up with a low
> voltage level, but we may want to increase the voltage level (and thus
> the threshold on the deserializer side) to increase noise immunity. Or,
> if the system environment isn't noisy, we may want to keep a low voltage
> level, or even decrease it if the camera boots up with a high voltage
> level. This runtime voltage level depends on the system design and its
> susceptibility to noise, and is thus a system property. Should we want
> to make it configurable, it should be specified in DT, and it's separate
> from the initial voltage level that is used to establish communication.
>

And that's what I meant. Assuming we handle initialization correctly
with this series, the serializers 'high threshold' configuration
-after- initialization can be specified with a DT property on the
-serializer- side. Then, to adjust the deserializer reverse channel
amplitude, once we the remote has probed and we have a subdevice
registered for it, we can query the 'high threshold' configuration
using get_mbus_config() (or another API if we think it's better) and
adjust the deserializer accordingly.

All in all:
- yes, I think there might be a need to control the noise immunity
  settings after initialization
- I think it should be done on the serializer side, possibly with a DT
  property, possibly something like a boolean 'maxim,high-threshold-enable'
- the deserializer can query that information with a kAPI like
  get_mbus_config() after the remote has probed
- Because of that there is no need for an additional deserializer property

Hope this makes sense

Re: [PATCH v6 5/5] media: i2c: max9286: Configure reverse channel amplitude

[Geert Uytterhoeven]

Hi Jacopo,

On Tue, Jan 12, 2021 at 10:07 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
> On Tue, Jan 12, 2021 at 07:03:42AM +0200, Laurent Pinchart wrote:
> > On Mon, Jan 11, 2021 at 12:20:23PM +0100, Jacopo Mondi wrote:
> > > On Mon, Jan 11, 2021 at 12:58:59PM +0200, Laurent Pinchart wrote:
> > > > On Mon, Jan 11, 2021 at 11:43:11AM +0100, Jacopo Mondi wrote:
> > > >> On Wed, Dec 16, 2020 at 07:22:17PM +0200, Laurent Pinchart wrote:
> > > >>> On Tue, Dec 15, 2020 at 06:09:57PM +0100, Jacopo Mondi wrote:
> > > >>>> Adjust the initial reverse channel amplitude parsing from
> > > >>>> firmware interface the 'maxim,reverse-channel-microvolt'
> > > >>>> property.
> > > >>>>
> > > >>>> This change is required for both rdacm20 and rdacm21 camera
> > > >>>> modules to be correctly probed when used in combination with
> > > >>>> the max9286 deserializer.
> > > >>>>
> > > >>>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > >>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > >>>> ---
> > > >>>>  drivers/media/i2c/max9286.c | 23 ++++++++++++++++++++++-
> > > >>>>  1 file changed, 22 insertions(+), 1 deletion(-)
> > > >>>>
> > > >>>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > >>>> index 021309c6dd6f..9b40a4890c4d 100644
> > > >>>> --- a/drivers/media/i2c/max9286.c
> > > >>>> +++ b/drivers/media/i2c/max9286.c
> > > >>>> @@ -163,6 +163,8 @@ struct max9286_priv {
> > > >>>>        unsigned int mux_channel;
> > > >>>>        bool mux_open;
> > > >>>>
> > > >>>> +      u32 reverse_channel_mv;
> > > >>>> +
> > > >>>>        struct v4l2_ctrl_handler ctrls;
> > > >>>>        struct v4l2_ctrl *pixelrate;
> > > >>>>
> > > >>>> @@ -557,10 +559,14 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
> > > >>>>         * All enabled sources have probed and enabled their reverse control
> > > >>>>         * channels:
> > > >>>>         *
> > > >>>> +       * - Increase the reverse channel amplitude to compensate for the
> > > >>>> +       *   remote ends high threshold, if not done already
> > > >>>>         * - Verify all configuration links are properly detected
> > > >>>>         * - Disable auto-ack as communication on the control channel are now
> > > >>>>         *   stable.
> > > >>>>         */
> > > >>>> +      if (priv->reverse_channel_mv < 170)
> > > >>>> +              max9286_reverse_channel_setup(priv, 170);
> > > >>>
> > > >>> I'm beginning to wonder if there will be a need in the future to not
> > > >>> increase the reverse channel amplitude (keeping the threshold low on the
> > > >>> remote side). An increased amplitude increases power consumption, and if
> > > >>> the environment isn't noisy, a low amplitude would work. The device tree
> > > >>> would then need to specify both the initial amplitude required by the
> > > >>> remote side, and the desired amplitude after initialization. What do you
> > > >>> think ? Is it overkill ? We don't have to implement this now, so
> > > >>>
> > > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > >>>
> > > >>> but if this feature could be required later, we may want to take into
> > > >>> account in the naming of the new DT property to reflect the fact that it
> > > >>> is the initial value.
> > > >>
> > > >> I had the same thought when I initially proposed
> > > >> "maxim,initial-reverse-channel-mV"
> > > >>
> > > >> Having to use the standard unit suffix that would have become
> > > >> "maxim,initial-reverse-channel-microvolt"
> > > >> which is extremely long.
> > > >>
> > > >> I can't tell if there will be any need to adjust the amplitude later.
> > > >> In any case, I would not rely on a DTS property to do so, as once we
> > > >> have probed the remote we have a subdev where to call
> > > >> 'get_mbus_config()' on, and from there we can report the high threshold
> > > >> status of the serializer and adjust the deser amplitude accordingly.
> > > >
> > > > I don't think that's the point. The threshold of the serializer is
> > > > something we can configure at runtime. What voltage level to use after
> > >
> > > How so ? I mean, we can add an API for this, but currently it's
> > > configured at probe time and that's it. Its configuration might as
> > > well come from a DT property like we do on the deserializer here but I
> > > fail to see why it's different. Both settings depends on the required
> > > noise immunity of th system.
> >
> > The voltage level configuration need to match between the tserializer
> > (transmitter) and the deserializer (receiver). The serializer is
> > configured with a voltage level, and the deserializer needs to be
> > configured with a corresponding threshold.
> >
>
> If I'm not mistaken it's actually the other way around, at least for
> the chips we're dealing with.
>
> The serializer (MAX9271) has an "Reverse Channel Receiver High
> Threshold Enable" bit (register 0x08[0]) undocumented in the chip
> manual but described in the "MAX9286 Programming Guide 2 10.pdf"
> document in the "Important Registers" section.
>
> The deserializer (MAX9286) has instead a configurable setting for the reverse
> channel signal amplitude, which is what we are controlling in this
> series.
>
> The deserializer reverse channel amplitude has to match the remote
> side 'high threshold enable' setting. If it is enabled the amplitude
> has to be increased to be able to probe the remote side. If it's not
> a lower amplitude has to be used to make comunication reliable.
>
> As you said, some models (RDACM20) might be pre-programmed with the
> 'high threshold enable' bit set, and so the deserializer reverse
> channel amplitude has to be adjusted accordingly to be able to
> comunicate on the reverse channel.
>
> > The voltage level of the serializer is configurable on the camera side
> > when the system is powered up. The RDACM20 has a microcontroller which
> > can configure the serializer, and other cameras may have similar
> > mechanisms. As the deserializer can't query the information from the
> > serializer (communication is unreliable if the threshold has an
> > incorrect value), we need a DT property to tell the deserializer what
> > threshold is initially used by the camera when it gets powered up.
> >
>
> That's what this series does, yes.
>
> > This only covers initialization. A camera could boot up with a low
> > voltage level, but we may want to increase the voltage level (and thus
> > the threshold on the deserializer side) to increase noise immunity. Or,
> > if the system environment isn't noisy, we may want to keep a low voltage
> > level, or even decrease it if the camera boots up with a high voltage
> > level. This runtime voltage level depends on the system design and its
> > susceptibility to noise, and is thus a system property. Should we want
> > to make it configurable, it should be specified in DT, and it's separate
> > from the initial voltage level that is used to establish communication.
> >
>
> And that's what I meant. Assuming we handle initialization correctly
> with this series, the serializers 'high threshold' configuration
> -after- initialization can be specified with a DT property on the
> -serializer- side. Then, to adjust the deserializer reverse channel
> amplitude, once we the remote has probed and we have a subdevice
> registered for it, we can query the 'high threshold' configuration
> using get_mbus_config() (or another API if we think it's better) and
> adjust the deserializer accordingly.
>
> All in all:
> - yes, I think there might be a need to control the noise immunity
>   settings after initialization
> - I think it should be done on the serializer side, possibly with a DT
>   property, possibly something like a boolean 'maxim,high-threshold-enable'

Can the needed voltage level be calibrated at runtime, cfr. DDR
link training?

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: [PATCH v6 5/5] media: i2c: max9286: Configure reverse channel amplitude

[Jacopo Mondi]

Hi Geert

On Tue, Jan 12, 2021 at 10:10:39AM +0100, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Tue, Jan 12, 2021 at 10:07 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
> > On Tue, Jan 12, 2021 at 07:03:42AM +0200, Laurent Pinchart wrote:
> > > On Mon, Jan 11, 2021 at 12:20:23PM +0100, Jacopo Mondi wrote:
> > > > On Mon, Jan 11, 2021 at 12:58:59PM +0200, Laurent Pinchart wrote:
> > > > > On Mon, Jan 11, 2021 at 11:43:11AM +0100, Jacopo Mondi wrote:
> > > > >> On Wed, Dec 16, 2020 at 07:22:17PM +0200, Laurent Pinchart wrote:
> > > > >>> On Tue, Dec 15, 2020 at 06:09:57PM +0100, Jacopo Mondi wrote:
> > > > >>>> Adjust the initial reverse channel amplitude parsing from
> > > > >>>> firmware interface the 'maxim,reverse-channel-microvolt'
> > > > >>>> property.
> > > > >>>>
> > > > >>>> This change is required for both rdacm20 and rdacm21 camera
> > > > >>>> modules to be correctly probed when used in combination with
> > > > >>>> the max9286 deserializer.
> > > > >>>>
> > > > >>>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > > >>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > >>>> ---
> > > > >>>>  drivers/media/i2c/max9286.c | 23 ++++++++++++++++++++++-
> > > > >>>>  1 file changed, 22 insertions(+), 1 deletion(-)
> > > > >>>>
> > > > >>>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > > >>>> index 021309c6dd6f..9b40a4890c4d 100644
> > > > >>>> --- a/drivers/media/i2c/max9286.c
> > > > >>>> +++ b/drivers/media/i2c/max9286.c
> > > > >>>> @@ -163,6 +163,8 @@ struct max9286_priv {
> > > > >>>>        unsigned int mux_channel;
> > > > >>>>        bool mux_open;
> > > > >>>>
> > > > >>>> +      u32 reverse_channel_mv;
> > > > >>>> +
> > > > >>>>        struct v4l2_ctrl_handler ctrls;
> > > > >>>>        struct v4l2_ctrl *pixelrate;
> > > > >>>>
> > > > >>>> @@ -557,10 +559,14 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
> > > > >>>>         * All enabled sources have probed and enabled their reverse control
> > > > >>>>         * channels:
> > > > >>>>         *
> > > > >>>> +       * - Increase the reverse channel amplitude to compensate for the
> > > > >>>> +       *   remote ends high threshold, if not done already
> > > > >>>>         * - Verify all configuration links are properly detected
> > > > >>>>         * - Disable auto-ack as communication on the control channel are now
> > > > >>>>         *   stable.
> > > > >>>>         */
> > > > >>>> +      if (priv->reverse_channel_mv < 170)
> > > > >>>> +              max9286_reverse_channel_setup(priv, 170);
> > > > >>>
> > > > >>> I'm beginning to wonder if there will be a need in the future to not
> > > > >>> increase the reverse channel amplitude (keeping the threshold low on the
> > > > >>> remote side). An increased amplitude increases power consumption, and if
> > > > >>> the environment isn't noisy, a low amplitude would work. The device tree
> > > > >>> would then need to specify both the initial amplitude required by the
> > > > >>> remote side, and the desired amplitude after initialization. What do you
> > > > >>> think ? Is it overkill ? We don't have to implement this now, so
> > > > >>>
> > > > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > >>>
> > > > >>> but if this feature could be required later, we may want to take into
> > > > >>> account in the naming of the new DT property to reflect the fact that it
> > > > >>> is the initial value.
> > > > >>
> > > > >> I had the same thought when I initially proposed
> > > > >> "maxim,initial-reverse-channel-mV"
> > > > >>
> > > > >> Having to use the standard unit suffix that would have become
> > > > >> "maxim,initial-reverse-channel-microvolt"
> > > > >> which is extremely long.
> > > > >>
> > > > >> I can't tell if there will be any need to adjust the amplitude later.
> > > > >> In any case, I would not rely on a DTS property to do so, as once we
> > > > >> have probed the remote we have a subdev where to call
> > > > >> 'get_mbus_config()' on, and from there we can report the high threshold
> > > > >> status of the serializer and adjust the deser amplitude accordingly.
> > > > >
> > > > > I don't think that's the point. The threshold of the serializer is
> > > > > something we can configure at runtime. What voltage level to use after
> > > >
> > > > How so ? I mean, we can add an API for this, but currently it's
> > > > configured at probe time and that's it. Its configuration might as
> > > > well come from a DT property like we do on the deserializer here but I
> > > > fail to see why it's different. Both settings depends on the required
> > > > noise immunity of th system.
> > >
> > > The voltage level configuration need to match between the tserializer
> > > (transmitter) and the deserializer (receiver). The serializer is
> > > configured with a voltage level, and the deserializer needs to be
> > > configured with a corresponding threshold.
> > >
> >
> > If I'm not mistaken it's actually the other way around, at least for
> > the chips we're dealing with.
> >
> > The serializer (MAX9271) has an "Reverse Channel Receiver High
> > Threshold Enable" bit (register 0x08[0]) undocumented in the chip
> > manual but described in the "MAX9286 Programming Guide 2 10.pdf"
> > document in the "Important Registers" section.
> >
> > The deserializer (MAX9286) has instead a configurable setting for the reverse
> > channel signal amplitude, which is what we are controlling in this
> > series.
> >
> > The deserializer reverse channel amplitude has to match the remote
> > side 'high threshold enable' setting. If it is enabled the amplitude
> > has to be increased to be able to probe the remote side. If it's not
> > a lower amplitude has to be used to make comunication reliable.
> >
> > As you said, some models (RDACM20) might be pre-programmed with the
> > 'high threshold enable' bit set, and so the deserializer reverse
> > channel amplitude has to be adjusted accordingly to be able to
> > comunicate on the reverse channel.
> >
> > > The voltage level of the serializer is configurable on the camera side
> > > when the system is powered up. The RDACM20 has a microcontroller which
> > > can configure the serializer, and other cameras may have similar
> > > mechanisms. As the deserializer can't query the information from the
> > > serializer (communication is unreliable if the threshold has an
> > > incorrect value), we need a DT property to tell the deserializer what
> > > threshold is initially used by the camera when it gets powered up.
> > >
> >
> > That's what this series does, yes.
> >
> > > This only covers initialization. A camera could boot up with a low
> > > voltage level, but we may want to increase the voltage level (and thus
> > > the threshold on the deserializer side) to increase noise immunity. Or,
> > > if the system environment isn't noisy, we may want to keep a low voltage
> > > level, or even decrease it if the camera boots up with a high voltage
> > > level. This runtime voltage level depends on the system design and its
> > > susceptibility to noise, and is thus a system property. Should we want
> > > to make it configurable, it should be specified in DT, and it's separate
> > > from the initial voltage level that is used to establish communication.
> > >
> >
> > And that's what I meant. Assuming we handle initialization correctly
> > with this series, the serializers 'high threshold' configuration
> > -after- initialization can be specified with a DT property on the
> > -serializer- side. Then, to adjust the deserializer reverse channel
> > amplitude, once we the remote has probed and we have a subdevice
> > registered for it, we can query the 'high threshold' configuration
> > using get_mbus_config() (or another API if we think it's better) and
> > adjust the deserializer accordingly.
> >
> > All in all:
> > - yes, I think there might be a need to control the noise immunity
> >   settings after initialization
> > - I think it should be done on the serializer side, possibly with a DT
> >   property, possibly something like a boolean 'maxim,high-threshold-enable'
>
> Can the needed voltage level be calibrated at runtime, cfr. DDR
> link training?

I'm not familiar with the mechanism you mention here, but being the
voltages levels a tuning parameters meant to maximize the reliability
of a communication channel which might show failure that cannot be
recovered easily [1] I fear it hardly can be run-time
calibrated but it's rather a decision that system integrators have to
make, depending on their usage environment and possibly a lot of
reliability testing.

Thanks
   j


[1] To give a simple example, if the deserializer detects it cannot
communicate with the remote side (and that's also hard to guess, as
the start-up phase requires auto-ack of i2c messages transmitted on
the reverse channel) how could it 'suggest' to the remote side that it
should enable its noise immunity threshold ?

>
> 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: [PATCH v6 5/5] media: i2c: max9286: Configure reverse channel amplitude

[Laurent Pinchart]

Hi Jacopo,

On Tue, Jan 12, 2021 at 10:08:05AM +0100, Jacopo Mondi wrote:
> On Tue, Jan 12, 2021 at 07:03:42AM +0200, Laurent Pinchart wrote:
> > On Mon, Jan 11, 2021 at 12:20:23PM +0100, Jacopo Mondi wrote:
> > > On Mon, Jan 11, 2021 at 12:58:59PM +0200, Laurent Pinchart wrote:
> > > > On Mon, Jan 11, 2021 at 11:43:11AM +0100, Jacopo Mondi wrote:
> > > >> On Wed, Dec 16, 2020 at 07:22:17PM +0200, Laurent Pinchart wrote:
> > > >>> On Tue, Dec 15, 2020 at 06:09:57PM +0100, Jacopo Mondi wrote:
> > > >>>> Adjust the initial reverse channel amplitude parsing from
> > > >>>> firmware interface the 'maxim,reverse-channel-microvolt'
> > > >>>> property.
> > > >>>>
> > > >>>> This change is required for both rdacm20 and rdacm21 camera
> > > >>>> modules to be correctly probed when used in combination with
> > > >>>> the max9286 deserializer.
> > > >>>>
> > > >>>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > >>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > >>>> ---
> > > >>>>  drivers/media/i2c/max9286.c | 23 ++++++++++++++++++++++-
> > > >>>>  1 file changed, 22 insertions(+), 1 deletion(-)
> > > >>>>
> > > >>>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > >>>> index 021309c6dd6f..9b40a4890c4d 100644
> > > >>>> --- a/drivers/media/i2c/max9286.c
> > > >>>> +++ b/drivers/media/i2c/max9286.c
> > > >>>> @@ -163,6 +163,8 @@ struct max9286_priv {
> > > >>>>  	unsigned int mux_channel;
> > > >>>>  	bool mux_open;
> > > >>>>
> > > >>>> +	u32 reverse_channel_mv;
> > > >>>> +
> > > >>>>  	struct v4l2_ctrl_handler ctrls;
> > > >>>>  	struct v4l2_ctrl *pixelrate;
> > > >>>>
> > > >>>> @@ -557,10 +559,14 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
> > > >>>>  	 * All enabled sources have probed and enabled their reverse control
> > > >>>>  	 * channels:
> > > >>>>  	 *
> > > >>>> +	 * - Increase the reverse channel amplitude to compensate for the
> > > >>>> +	 *   remote ends high threshold, if not done already
> > > >>>>  	 * - Verify all configuration links are properly detected
> > > >>>>  	 * - Disable auto-ack as communication on the control channel are now
> > > >>>>  	 *   stable.
> > > >>>>  	 */
> > > >>>> +	if (priv->reverse_channel_mv < 170)
> > > >>>> +		max9286_reverse_channel_setup(priv, 170);
> > > >>>
> > > >>> I'm beginning to wonder if there will be a need in the future to not
> > > >>> increase the reverse channel amplitude (keeping the threshold low on the
> > > >>> remote side). An increased amplitude increases power consumption, and if
> > > >>> the environment isn't noisy, a low amplitude would work. The device tree
> > > >>> would then need to specify both the initial amplitude required by the
> > > >>> remote side, and the desired amplitude after initialization. What do you
> > > >>> think ? Is it overkill ? We don't have to implement this now, so
> > > >>>
> > > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > >>>
> > > >>> but if this feature could be required later, we may want to take into
> > > >>> account in the naming of the new DT property to reflect the fact that it
> > > >>> is the initial value.
> > > >>
> > > >> I had the same thought when I initially proposed
> > > >> "maxim,initial-reverse-channel-mV"
> > > >>
> > > >> Having to use the standard unit suffix that would have become
> > > >> "maxim,initial-reverse-channel-microvolt"
> > > >> which is extremely long.
> > > >>
> > > >> I can't tell if there will be any need to adjust the amplitude later.
> > > >> In any case, I would not rely on a DTS property to do so, as once we
> > > >> have probed the remote we have a subdev where to call
> > > >> 'get_mbus_config()' on, and from there we can report the high threshold
> > > >> status of the serializer and adjust the deser amplitude accordingly.
> > > >
> > > > I don't think that's the point. The threshold of the serializer is
> > > > something we can configure at runtime. What voltage level to use after
> > >
> > > How so ? I mean, we can add an API for this, but currently it's
> > > configured at probe time and that's it. Its configuration might as
> > > well come from a DT property like we do on the deserializer here but I
> > > fail to see why it's different. Both settings depends on the required
> > > noise immunity of th system.
> >
> > The voltage level configuration need to match between the tserializer
> > (transmitter) and the deserializer (receiver). The serializer is
> > configured with a voltage level, and the deserializer needs to be
> > configured with a corresponding threshold.
> 
> If I'm not mistaken it's actually the other way around, at least for
> the chips we're dealing with.

Yes, I mixed up the direction, sorry about that.

> The serializer (MAX9271) has an "Reverse Channel Receiver High
> Threshold Enable" bit (register 0x08[0]) undocumented in the chip
> manual but described in the "MAX9286 Programming Guide 2 10.pdf"
> document in the "Important Registers" section.
> 
> The deserializer (MAX9286) has instead a configurable setting for the reverse
> channel signal amplitude, which is what we are controlling in this
> series.
> 
> The deserializer reverse channel amplitude has to match the remote
> side 'high threshold enable' setting. If it is enabled the amplitude
> has to be increased to be able to probe the remote side. If it's not
> a lower amplitude has to be used to make comunication reliable.
> 
> As you said, some models (RDACM20) might be pre-programmed with the
> 'high threshold enable' bit set, and so the deserializer reverse
> channel amplitude has to be adjusted accordingly to be able to
> comunicate on the reverse channel.
>
> > The voltage level of the serializer is configurable on the camera side
> > when the system is powered up. The RDACM20 has a microcontroller which
> > can configure the serializer, and other cameras may have similar
> > mechanisms. As the deserializer can't query the information from the
> > serializer (communication is unreliable if the threshold has an
> > incorrect value), we need a DT property to tell the deserializer what
> > threshold is initially used by the camera when it gets powered up.
> 
> That's what this series does, yes.
> 
> > This only covers initialization. A camera could boot up with a low
> > voltage level, but we may want to increase the voltage level (and thus
> > the threshold on the deserializer side) to increase noise immunity. Or,
> > if the system environment isn't noisy, we may want to keep a low voltage
> > level, or even decrease it if the camera boots up with a high voltage
> > level. This runtime voltage level depends on the system design and its
> > susceptibility to noise, and is thus a system property. Should we want
> > to make it configurable, it should be specified in DT, and it's separate
> > from the initial voltage level that is used to establish communication.
> 
> And that's what I meant. Assuming we handle initialization correctly
> with this series, the serializers 'high threshold' configuration
> -after- initialization can be specified with a DT property on the
> -serializer- side. Then, to adjust the deserializer reverse channel
> amplitude, once we the remote has probed and we have a subdevice
> registered for it, we can query the 'high threshold' configuration
> using get_mbus_config() (or another API if we think it's better) and
> adjust the deserializer accordingly.
> 
> All in all:
> - yes, I think there might be a need to control the noise immunity
>   settings after initialization
> - I think it should be done on the serializer side, possibly with a DT
>   property, possibly something like a boolean 'maxim,high-threshold-enable'
> - the deserializer can query that information with a kAPI like
>   get_mbus_config() after the remote has probed
> - Because of that there is no need for an additional deserializer property
> 
> Hope this makes sense

Now I get what you meant. Sorry for missing the point.

While it would be technically feasible to query the property from the
serializer at runtime, there's the additional issue that the
deserializer has a single reverse channel amplitude setting for all the
channels. We would need to ensure that the property is set to the same
value in all camera DT nodes. Wouldn't it be best to then set it once
only, in the deserializer node ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 5/5] media: i2c: max9286: Configure reverse channel amplitude

[Jacopo Mondi]

Hi Laurent,

On Thu, Jan 14, 2021 at 07:53:36AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> >
> > All in all:
> > - yes, I think there might be a need to control the noise immunity
> >   settings after initialization
> > - I think it should be done on the serializer side, possibly with a DT
> >   property, possibly something like a boolean 'maxim,high-threshold-enable'
> > - the deserializer can query that information with a kAPI like
> >   get_mbus_config() after the remote has probed
> > - Because of that there is no need for an additional deserializer property
> >
> > Hope this makes sense
>
> Now I get what you meant. Sorry for missing the point.
>
> While it would be technically feasible to query the property from the
> serializer at runtime, there's the additional issue that the
> deserializer has a single reverse channel amplitude setting for all the
> channels. We would need to ensure that the property is set to the same
> value in all camera DT nodes. Wouldn't it be best to then set it once
> only, in the deserializer node ?
>

To be honest I wouldn't mind a run-time error, or a fallback like "the
first one to probe is the authoritative one, the rest have to follow".
And don't forget we would need a serializer property anyway to tell
the chip if it has to enable its noise immunity threshold or not.

But anyway, the here introduced new property already requires
knwoledge on the deserializer about which camera is connected on the
other side. It's not so bad, as if cameras are described in a .dtsi or
.dtbo the deserializer property can be overridden. We can do the same
for an additional property.

ie. a deserializer-serializer 'maxim,high-threshold-enable' property

RDACM20: pre-programmed high threshold enable

-------------- rdacm20.dtsi -------------------
&gmsl {
        maxim,reverse-channel-microvolt = <170000>;

        i2c-mux {
                i2c@0 {
                        camera@51 {
                                ....

                        }

                }

        }
};
-------------------------------------------------

RDACM21: no pre-programmed high-threshold, high threshold enabled
after camera probe

-------------- rdacm21.dtsi -------------------
&gmsl {
        maxim,reverse-channel-microvolt = <100000>;
        maxim,high-threshold-enable;

        i2c-mux {
                i2c@0 {
                        camera@51 {
                                maxim,high-threshold-enable;
                                ....

                        }

                }

        }
};
-------------------------------------------------

RDACM21: no high-threshold enabled at all

-------------- rdacm21.dtsi -------------------
&gmsl {
        maxim,reverse-channel-microvolt = <100000>;

        i2c-mux {
                i2c@0 {
                        camera@51 {
                                ....

                        }

                }

        }
};
-------------------------------------------------

For the serializer it's a boolean, for the deser we might need to
specify a voltage, so it might become an uint32
'maxim,high-threshold-microvolt' there.

-------------- rdacm21.dtsi -------------------
&gmsl {
        maxim,reverse-channel-microvolt = <100000>;
        maxim,high-threshold-microvolt = <170000>;

        i2c-mux {
                i2c@0 {
                        camera@51 {
                                maxim,high-threshold-enable;
                                ....

                        }

                }

        }
};
-------------------------------------------------

> --
> Regards,
>
> Laurent Pinchart