[PATCH v7 0/7] media: i2c: Add RDACM21 camera module

[PATCH v7 0/7] media: i2c: Add RDACM21 camera module

[Jacopo Mondi]

This iteration is only to ease review of the RDACM21 as it contains two
broken out patches marked as fixups

I've taken Laurent's comments on v6 in the driver, but once I've rebased on
the GPIO handling fixes applied to the MAX9271 driver, I've opened the pandora
can of properly resetting the OV490 ISP @_@

The two broken out patches serve to show the difference on top of the v6
version instead of having reviewers going through the changes  that might get
lost between the two versions.

With acks on the fixups patches, I'll squash in v8.

All the other patches are reviewed already, so I hope if nothing big is in
this version, v8 will be good for merging.

Thanks
  j

Jacopo Mondi (7):
  media: i2c: Add driver for RDACM21 camera module
  fixup! media: i2c: rdacm21: Fix GPIO handling
  fixup! media: i2c: rdacm21: Break-out ov10640 initialization
  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     |  22 +
 MAINTAINERS                                   |  12 +
 drivers/media/i2c/Kconfig                     |  13 +
 drivers/media/i2c/Makefile                    |   2 +
 drivers/media/i2c/max9286.c                   |  60 +-
 drivers/media/i2c/rdacm21.c                   | 623 ++++++++++++++++++
 6 files changed, 719 insertions(+), 13 deletions(-)
 create mode 100644 drivers/media/i2c/rdacm21.c

--
2.29.2

[PATCH v7 1/7] 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 | 574 ++++++++++++++++++++++++++++++++++++
 4 files changed, 601 insertions(+)
 create mode 100644 drivers/media/i2c/rdacm21.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 14adf87d90c7..1822d73ed615 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14967,6 +14967,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..f3841369768c
--- /dev/null
+++ b/drivers/media/i2c/rdacm21.c
@@ -0,0 +1,574 @@
+// 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 MAX9271_RESET_CYCLES		10
+
+#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_PID_TIMEOUT		20
+#define OV490_OUTPUT_EN_TIMEOUT		300
+
+#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_ID_LOW			0xa6
+#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[2];
+	u16				last_page;
+};
+
+static inline struct rdacm21_device *sd_to_rdacm21(struct v4l2_subdev *sd)
+{
+	return container_of(sd, struct rdacm21_device, sd);
+}
+
+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 page)
+{
+	u8 page_high = page >> 8;
+	u8 page_low = page;
+	int ret;
+
+	if (page == dev->last_page)
+		return 0;
+
+	if (page_high != (dev->last_page >> 8)) {
+		ret = ov490_write(dev, OV490_PAGE_HIGH_REG, page_high);
+		if (ret)
+			return ret;
+	}
+
+	if (page_low != (u8)dev->last_page) {
+		ret = ov490_write(dev, OV490_PAGE_LOW_REG, page_low);
+		if (ret)
+			return ret;
+	}
+
+	dev->last_page = page;
+	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 const 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 const struct v4l2_subdev_ops rdacm21_subdev_ops = {
+	.video		= &rdacm21_video_ops,
+	.pad		= &rdacm21_subdev_pad_ops,
+};
+
+static int ov490_initialize(struct rdacm21_device *dev)
+{
+	u8 pid, ver, val;
+	unsigned int i;
+	int ret;
+
+	/*
+	 * Read OV490 Id to test communications. Give it up to 40msec to
+	 * exit from reset.
+	 */
+	for (i = 0; i < OV490_PID_TIMEOUT; ++i) {
+		ret = ov490_read_reg(dev, OV490_PID, &pid);
+		if (ret == 0)
+			break;
+		usleep_range(1000, 2000);
+	}
+	if (i == OV490_PID_TIMEOUT) {
+		dev_err(dev->dev, "OV490 PID read failed (%d)\n", ret);
+		return ret;
+	}
+
+	ret = ov490_read_reg(dev, OV490_VER, &ver);
+	if (ret < 0)
+		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 (i = 0; i < OV490_OUTPUT_EN_TIMEOUT; ++i) {
+		ov490_read_reg(dev, OV490_ODS_CTRL, &val);
+		if (val == OV490_ODS_CTRL_FRAME_OUTPUT_EN)
+			break;
+		usleep_range(1000, 2000);
+	}
+	if (i == OV490_OUTPUT_EN_TIMEOUT) {
+		dev_err(dev->dev, "Timeout waiting for firmware boot\n");
+		return -ENODEV;
+	}
+
+	/* Read OV10640 Id to test communications. */
+	ov490_write_reg(dev, 0x80195000, 0x01);
+	ov490_write_reg(dev, 0x80195001, 0x30);
+	ov490_write_reg(dev, 0x80195002, 0x0a);
+	ov490_write_reg(dev, 0x808000c0, 0xc1);
+
+	ov490_read_reg(dev, 0x80195000, &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);
+	usleep_range(3000, 5000);
+
+	/* Enable reverse channel and disable the serial link. */
+	ret = max9271_set_serial_link(&dev->serializer, false);
+	if (ret)
+		return ret;
+
+	/* Configure I2C bus at 105Kbps speed and configure GMSL. */
+	ret = max9271_configure_i2c(&dev->serializer,
+				    MAX9271_I2CSLVSH_469NS_234NS |
+				    MAX9271_I2CSLVTO_1024US |
+				    MAX9271_I2CMSTBT_105KBPS);
+	if (ret)
+		return ret;
+
+	ret = max9271_verify_id(&dev->serializer);
+	if (ret)
+		return ret;
+
+	ret = max9271_configure_gmsl_link(&dev->serializer);
+	if (ret)
+		return ret;
+
+	/* Set GPO high to hold OV490 in reset. */
+	ret = max9271_set_gpios(&dev->serializer, MAX9271_GPO);
+	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.
+	 */
+	return max9271_set_high_threshold(&dev->serializer, true);
+}
+
+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.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 = sd_to_rdacm21(i2c_get_clientdata(client));
+
+	v4l2_async_unregister_subdev(&dev->sd);
+	v4l2_ctrl_handler_free(&dev->ctrls);
+	i2c_unregister_device(dev->isp);
+	fwnode_handle_put(dev->sd.fwnode);
+
+	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");
+MODULE_LICENSE("GPL v2");
-- 
2.29.2

Re: [PATCH v7 1/7] media: i2c: Add driver for RDACM21 camera module

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Jan 13, 2021 at 07:54:59PM +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>

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

> ---
>  MAINTAINERS                 |  12 +
>  drivers/media/i2c/Kconfig   |  13 +
>  drivers/media/i2c/Makefile  |   2 +
>  drivers/media/i2c/rdacm21.c | 574 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 601 insertions(+)
>  create mode 100644 drivers/media/i2c/rdacm21.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 14adf87d90c7..1822d73ed615 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14967,6 +14967,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..f3841369768c
> --- /dev/null
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -0,0 +1,574 @@
> +// 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 MAX9271_RESET_CYCLES		10
> +
> +#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_PID_TIMEOUT		20
> +#define OV490_OUTPUT_EN_TIMEOUT		300
> +
> +#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_ID_LOW			0xa6
> +#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[2];
> +	u16				last_page;
> +};
> +
> +static inline struct rdacm21_device *sd_to_rdacm21(struct v4l2_subdev *sd)
> +{
> +	return container_of(sd, struct rdacm21_device, sd);
> +}
> +
> +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 page)
> +{
> +	u8 page_high = page >> 8;
> +	u8 page_low = page;
> +	int ret;
> +
> +	if (page == dev->last_page)
> +		return 0;
> +
> +	if (page_high != (dev->last_page >> 8)) {
> +		ret = ov490_write(dev, OV490_PAGE_HIGH_REG, page_high);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (page_low != (u8)dev->last_page) {
> +		ret = ov490_write(dev, OV490_PAGE_LOW_REG, page_low);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	dev->last_page = page;
> +	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 const 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 const struct v4l2_subdev_ops rdacm21_subdev_ops = {
> +	.video		= &rdacm21_video_ops,
> +	.pad		= &rdacm21_subdev_pad_ops,
> +};
> +
> +static int ov490_initialize(struct rdacm21_device *dev)
> +{
> +	u8 pid, ver, val;
> +	unsigned int i;
> +	int ret;
> +
> +	/*
> +	 * Read OV490 Id to test communications. Give it up to 40msec to
> +	 * exit from reset.
> +	 */
> +	for (i = 0; i < OV490_PID_TIMEOUT; ++i) {
> +		ret = ov490_read_reg(dev, OV490_PID, &pid);
> +		if (ret == 0)
> +			break;
> +		usleep_range(1000, 2000);
> +	}
> +	if (i == OV490_PID_TIMEOUT) {
> +		dev_err(dev->dev, "OV490 PID read failed (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	ret = ov490_read_reg(dev, OV490_VER, &ver);
> +	if (ret < 0)
> +		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 (i = 0; i < OV490_OUTPUT_EN_TIMEOUT; ++i) {
> +		ov490_read_reg(dev, OV490_ODS_CTRL, &val);
> +		if (val == OV490_ODS_CTRL_FRAME_OUTPUT_EN)
> +			break;
> +		usleep_range(1000, 2000);
> +	}
> +	if (i == OV490_OUTPUT_EN_TIMEOUT) {
> +		dev_err(dev->dev, "Timeout waiting for firmware boot\n");
> +		return -ENODEV;
> +	}
> +
> +	/* Read OV10640 Id to test communications. */
> +	ov490_write_reg(dev, 0x80195000, 0x01);
> +	ov490_write_reg(dev, 0x80195001, 0x30);
> +	ov490_write_reg(dev, 0x80195002, 0x0a);
> +	ov490_write_reg(dev, 0x808000c0, 0xc1);
> +
> +	ov490_read_reg(dev, 0x80195000, &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);
> +	usleep_range(3000, 5000);
> +
> +	/* Enable reverse channel and disable the serial link. */
> +	ret = max9271_set_serial_link(&dev->serializer, false);
> +	if (ret)
> +		return ret;
> +
> +	/* Configure I2C bus at 105Kbps speed and configure GMSL. */
> +	ret = max9271_configure_i2c(&dev->serializer,
> +				    MAX9271_I2CSLVSH_469NS_234NS |
> +				    MAX9271_I2CSLVTO_1024US |
> +				    MAX9271_I2CMSTBT_105KBPS);
> +	if (ret)
> +		return ret;
> +
> +	ret = max9271_verify_id(&dev->serializer);
> +	if (ret)
> +		return ret;
> +
> +	ret = max9271_configure_gmsl_link(&dev->serializer);
> +	if (ret)
> +		return ret;
> +
> +	/* Set GPO high to hold OV490 in reset. */
> +	ret = max9271_set_gpios(&dev->serializer, MAX9271_GPO);
> +	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.
> +	 */
> +	return max9271_set_high_threshold(&dev->serializer, true);
> +}
> +
> +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.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 = sd_to_rdacm21(i2c_get_clientdata(client));
> +
> +	v4l2_async_unregister_subdev(&dev->sd);
> +	v4l2_ctrl_handler_free(&dev->ctrls);
> +	i2c_unregister_device(dev->isp);
> +	fwnode_handle_put(dev->sd.fwnode);
> +
> +	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");
> +MODULE_LICENSE("GPL v2");

-- 
Regards,

Laurent Pinchart

[PATCH v7 2/7] fixup! media: i2c: rdacm21: Fix GPIO handling

[Jacopo Mondi]

The MAX9271 GPIO line connected to the OV490 RESETB line is
GPIO1, not GPO. As the GPIO1 line is not enabled by default, first
enable it then control the OV490 reset during the MAX9271 configuration
procedure.

Before this change the embedded OV490 ISP was not actually reset.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm21.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index f3841369768c..0428e3209463 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -425,27 +425,23 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
 	if (ret)
 		return ret;
 
-	ret = max9271_configure_gmsl_link(&dev->serializer);
+	/* Enable GPIO1 and hold OV490 in reset during max9271 configuration. */
+	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
 	if (ret)
 		return ret;
 
-	/* Set GPO high to hold OV490 in reset. */
-	ret = max9271_set_gpios(&dev->serializer, MAX9271_GPO);
+	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
 	if (ret)
 		return ret;
 
-	ret = max9271_set_address(&dev->serializer, dev->addrs[0]);
+	ret = max9271_configure_gmsl_link(&dev->serializer);
 	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);
+	ret = max9271_set_address(&dev->serializer, dev->addrs[0]);
 	if (ret)
 		return ret;
+	dev->serializer.client->addr = dev->addrs[0];
 
 	ret = max9271_set_translation(&dev->serializer, dev->addrs[1],
 				      OV490_I2C_ADDRESS);
@@ -453,6 +449,12 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
 		return ret;
 	dev->isp->addr = dev->addrs[1];
 
+	/* Release OV490 from reset and initialize it. */
+	ret = max9271_set_gpios(&dev->serializer, MAX9271_GPIO1OUT);
+	if (ret)
+		return ret;
+	usleep_range(3000, 5000);
+
 	ret = ov490_initialize(dev);
 	if (ret)
 		return ret;
-- 
2.29.2

Re: [PATCH v7 2/7] fixup! media: i2c: rdacm21: Fix GPIO handling

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Jan 13, 2021 at 07:55:00PM +0100, Jacopo Mondi wrote:
> The MAX9271 GPIO line connected to the OV490 RESETB line is
> GPIO1, not GPO. As the GPIO1 line is not enabled by default, first
> enable it then control the OV490 reset during the MAX9271 configuration
> procedure.
> 
> Before this change the embedded OV490 ISP was not actually reset.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

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

> ---
>  drivers/media/i2c/rdacm21.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index f3841369768c..0428e3209463 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -425,27 +425,23 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
>  	if (ret)
>  		return ret;
>  
> -	ret = max9271_configure_gmsl_link(&dev->serializer);
> +	/* Enable GPIO1 and hold OV490 in reset during max9271 configuration. */
> +	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
>  	if (ret)
>  		return ret;
>  
> -	/* Set GPO high to hold OV490 in reset. */
> -	ret = max9271_set_gpios(&dev->serializer, MAX9271_GPO);
> +	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
>  	if (ret)
>  		return ret;
>  
> -	ret = max9271_set_address(&dev->serializer, dev->addrs[0]);
> +	ret = max9271_configure_gmsl_link(&dev->serializer);
>  	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);
> +	ret = max9271_set_address(&dev->serializer, dev->addrs[0]);
>  	if (ret)
>  		return ret;
> +	dev->serializer.client->addr = dev->addrs[0];
>  
>  	ret = max9271_set_translation(&dev->serializer, dev->addrs[1],
>  				      OV490_I2C_ADDRESS);
> @@ -453,6 +449,12 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
>  		return ret;
>  	dev->isp->addr = dev->addrs[1];
>  
> +	/* Release OV490 from reset and initialize it. */
> +	ret = max9271_set_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> +	if (ret)
> +		return ret;
> +	usleep_range(3000, 5000);
> +
>  	ret = ov490_initialize(dev);
>  	if (ret)
>  		return ret;

-- 
Regards,

Laurent Pinchart

[PATCH v7 3/7] fixup! media: i2c: rdacm21: Break-out ov10640 initialization

[Jacopo Mondi]

The embedded OV490 ISP chip provides a secondary SCCB interface and
two GPIO lines to control the connected OV10640 image sensor.

Break out the OV10640 initialization from the OV490 initialization and
explicitely control the powerdown and reset GPIOs. After the image
sensor has been hard reset, implement a more clear handling of the
secondary SCCB interface to read the image sensor chip ID.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm21.c | 75 ++++++++++++++++++++++++++++++-------
 1 file changed, 61 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 0428e3209463..944009687de5 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -30,11 +30,24 @@
 #define OV490_PAGE_HIGH_REG		0xfffd
 #define OV490_PAGE_LOW_REG		0xfffe
 
+/*
+ * The SCCB slave handling is undocumented; the registers naming scheme is
+ * totally arbitrary.
+ */
+#define OV490_SCCB_SLAVE_WRITE		0x00
+#define OV490_SCCB_SLAVE_READ		0x01
+#define OV490_SCCB_SLAVE0_DIR		0x80195000
+#define OV490_SCCB_SLAVE0_ADDR_HIGH	0x80195001
+#define OV490_SCCB_SLAVE0_ADDR_LOW	0x80195002
+
 #define OV490_DVP_CTRL3			0x80286009
 
 #define OV490_ODS_CTRL_FRAME_OUTPUT_EN	0x0c
 #define OV490_ODS_CTRL			0x8029d000
 
+#define OV490_HOST_CMD			0x808000c0
+#define OV490_HOST_CMD_TRIGGER		0xc1
+
 #define OV490_ID_VAL			0x0490
 #define OV490_ID(_p, _v)		((((_p) & 0xff) << 8) | ((_v) & 0xff))
 #define OV490_PID			0x8080300a
@@ -42,12 +55,22 @@
 #define OV490_PID_TIMEOUT		20
 #define OV490_OUTPUT_EN_TIMEOUT		300
 
+#define OV490_GPIO0_RESETB		0x01
+#define OV490_SPWDN0			0x01
+#define OV490_GPIO_SEL0			0x80800050
+#define OV490_GPIO_SEL1			0x80800051
+#define OV490_GPIO_DIRECTION0		0x80800054
+#define OV490_GPIO_DIRECTION1		0x80800055
+#define OV490_GPIO_OUTPUT_VALUE0	0x80800058
+#define OV490_GPIO_OUTPUT_VALUE1	0x80800059
+
 #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_ID_LOW			0xa6
+#define OV10640_ID_HIGH			0xa6
+#define OV10640_CHIP_ID			0x300a
 #define OV10640_PIXEL_RATE		55000000
 
 struct rdacm21_device {
@@ -306,6 +329,39 @@ static const struct v4l2_subdev_ops rdacm21_subdev_ops = {
 	.pad		= &rdacm21_subdev_pad_ops,
 };
 
+static int ov10640_initialize(struct rdacm21_device *dev)
+{
+	u8 val;
+
+	/* Power-up OV10640 by setting RESETB and PWDNB pins high. */
+	ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0_RESETB);
+	ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
+	ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0_RESETB);
+	ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0);
+	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0_RESETB);
+	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0);
+	usleep_range(3000, 5000);
+
+	/* Read OV10640 ID to test communications. */
+	ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR, OV490_SCCB_SLAVE_READ);
+	ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH, OV10640_CHIP_ID >> 8);
+	ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, (u8)OV10640_CHIP_ID);
+
+	/* Trigger SCCB slave transaction and give it some time to complete. */
+	ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
+	usleep_range(1000, 1500);
+
+	ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, &val);
+	if (val != OV10640_ID_HIGH) {
+		dev_err(dev->dev, "OV10640 ID mismatch: (0x%02x)\n", val);
+		return -ENODEV;
+	}
+
+	dev_dbg(dev->dev, "OV10640 ID = 0x%2x\n", val);
+
+	return 0;
+}
+
 static int ov490_initialize(struct rdacm21_device *dev)
 {
 	u8 pid, ver, val;
@@ -349,20 +405,11 @@ static int ov490_initialize(struct rdacm21_device *dev)
 		return -ENODEV;
 	}
 
-	/* Read OV10640 Id to test communications. */
-	ov490_write_reg(dev, 0x80195000, 0x01);
-	ov490_write_reg(dev, 0x80195001, 0x30);
-	ov490_write_reg(dev, 0x80195002, 0x0a);
-	ov490_write_reg(dev, 0x808000c0, 0xc1);
-
-	ov490_read_reg(dev, 0x80195000, &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);
+	ret = ov10640_initialize(dev);
+	if (ret)
+		return ret;
 
+	/* Program OV490 with register-value table. */
 	for (i = 0; i < ARRAY_SIZE(ov490_regs_wizard); ++i) {
 		ret = ov490_write(dev, ov490_regs_wizard[i].reg,
 				  ov490_regs_wizard[i].val);
-- 
2.29.2

Re: [PATCH v7 3/7] fixup! media: i2c: rdacm21: Break-out ov10640 initialization

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Jan 13, 2021 at 07:55:01PM +0100, Jacopo Mondi wrote:
> The embedded OV490 ISP chip provides a secondary SCCB interface and
> two GPIO lines to control the connected OV10640 image sensor.
> 
> Break out the OV10640 initialization from the OV490 initialization and
> explicitely control the powerdown and reset GPIOs. After the image

s/explicitely/explicitly/

> sensor has been hard reset, implement a more clear handling of the
> secondary SCCB interface to read the image sensor chip ID.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/rdacm21.c | 75 ++++++++++++++++++++++++++++++-------
>  1 file changed, 61 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index 0428e3209463..944009687de5 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -30,11 +30,24 @@
>  #define OV490_PAGE_HIGH_REG		0xfffd
>  #define OV490_PAGE_LOW_REG		0xfffe
>  
> +/*
> + * The SCCB slave handling is undocumented; the registers naming scheme is
> + * totally arbitrary.
> + */
> +#define OV490_SCCB_SLAVE_WRITE		0x00
> +#define OV490_SCCB_SLAVE_READ		0x01
> +#define OV490_SCCB_SLAVE0_DIR		0x80195000
> +#define OV490_SCCB_SLAVE0_ADDR_HIGH	0x80195001
> +#define OV490_SCCB_SLAVE0_ADDR_LOW	0x80195002
> +
>  #define OV490_DVP_CTRL3			0x80286009
>  
>  #define OV490_ODS_CTRL_FRAME_OUTPUT_EN	0x0c
>  #define OV490_ODS_CTRL			0x8029d000
>  
> +#define OV490_HOST_CMD			0x808000c0
> +#define OV490_HOST_CMD_TRIGGER		0xc1
> +
>  #define OV490_ID_VAL			0x0490
>  #define OV490_ID(_p, _v)		((((_p) & 0xff) << 8) | ((_v) & 0xff))
>  #define OV490_PID			0x8080300a
> @@ -42,12 +55,22 @@
>  #define OV490_PID_TIMEOUT		20
>  #define OV490_OUTPUT_EN_TIMEOUT		300
>  
> +#define OV490_GPIO0_RESETB		0x01

Shouldn't this be named just OV490_GPIO0 ? The fact that it's connected
to the RESETB signal of the OV10640 is board-specific, not an OV490
intrinsic property.

BIT(0) ?

> +#define OV490_SPWDN0			0x01

Same here.

> +#define OV490_GPIO_SEL0			0x80800050
> +#define OV490_GPIO_SEL1			0x80800051
> +#define OV490_GPIO_DIRECTION0		0x80800054
> +#define OV490_GPIO_DIRECTION1		0x80800055
> +#define OV490_GPIO_OUTPUT_VALUE0	0x80800058
> +#define OV490_GPIO_OUTPUT_VALUE1	0x80800059
> +
>  #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_ID_LOW			0xa6
> +#define OV10640_ID_HIGH			0xa6
> +#define OV10640_CHIP_ID			0x300a
>  #define OV10640_PIXEL_RATE		55000000
>  
>  struct rdacm21_device {
> @@ -306,6 +329,39 @@ static const struct v4l2_subdev_ops rdacm21_subdev_ops = {
>  	.pad		= &rdacm21_subdev_pad_ops,
>  };
>  
> +static int ov10640_initialize(struct rdacm21_device *dev)
> +{
> +	u8 val;
> +
> +	/* Power-up OV10640 by setting RESETB and PWDNB pins high. */
> +	ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0_RESETB);
> +	ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
> +	ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0_RESETB);
> +	ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0);
> +	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0_RESETB);
> +	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0);
> +	usleep_range(3000, 5000);

So the OV490 firmware doesn't handle this ?

> +
> +	/* Read OV10640 ID to test communications. */
> +	ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR, OV490_SCCB_SLAVE_READ);
> +	ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH, OV10640_CHIP_ID >> 8);
> +	ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, (u8)OV10640_CHIP_ID);
> +
> +	/* Trigger SCCB slave transaction and give it some time to complete. */
> +	ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
> +	usleep_range(1000, 1500);
> +
> +	ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, &val);
> +	if (val != OV10640_ID_HIGH) {
> +		dev_err(dev->dev, "OV10640 ID mismatch: (0x%02x)\n", val);
> +		return -ENODEV;
> +	}

Would it make sense to create an ov490_sensor_read() helper ?

> +
> +	dev_dbg(dev->dev, "OV10640 ID = 0x%2x\n", val);
> +
> +	return 0;
> +}
> +
>  static int ov490_initialize(struct rdacm21_device *dev)
>  {
>  	u8 pid, ver, val;
> @@ -349,20 +405,11 @@ static int ov490_initialize(struct rdacm21_device *dev)
>  		return -ENODEV;
>  	}
>  
> -	/* Read OV10640 Id to test communications. */
> -	ov490_write_reg(dev, 0x80195000, 0x01);
> -	ov490_write_reg(dev, 0x80195001, 0x30);
> -	ov490_write_reg(dev, 0x80195002, 0x0a);
> -	ov490_write_reg(dev, 0x808000c0, 0xc1);
> -
> -	ov490_read_reg(dev, 0x80195000, &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);
> +	ret = ov10640_initialize(dev);
> +	if (ret)
> +		return ret;
>  
> +	/* Program OV490 with register-value table. */
>  	for (i = 0; i < ARRAY_SIZE(ov490_regs_wizard); ++i) {
>  		ret = ov490_write(dev, ov490_regs_wizard[i].reg,
>  				  ov490_regs_wizard[i].val);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v7 3/7] fixup! media: i2c: rdacm21: Break-out ov10640 initialization

[Jacopo Mondi]

Hi Laurent,

On Thu, Jan 14, 2021 at 01:23:25AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Jan 13, 2021 at 07:55:01PM +0100, Jacopo Mondi wrote:
> > The embedded OV490 ISP chip provides a secondary SCCB interface and
> > two GPIO lines to control the connected OV10640 image sensor.
> >
> > Break out the OV10640 initialization from the OV490 initialization and
> > explicitely control the powerdown and reset GPIOs. After the image
>
> s/explicitely/explicitly/
>
> > sensor has been hard reset, implement a more clear handling of the
> > secondary SCCB interface to read the image sensor chip ID.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/rdacm21.c | 75 ++++++++++++++++++++++++++++++-------
> >  1 file changed, 61 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> > index 0428e3209463..944009687de5 100644
> > --- a/drivers/media/i2c/rdacm21.c
> > +++ b/drivers/media/i2c/rdacm21.c
> > @@ -30,11 +30,24 @@
> >  #define OV490_PAGE_HIGH_REG		0xfffd
> >  #define OV490_PAGE_LOW_REG		0xfffe
> >
> > +/*
> > + * The SCCB slave handling is undocumented; the registers naming scheme is
> > + * totally arbitrary.
> > + */
> > +#define OV490_SCCB_SLAVE_WRITE		0x00
> > +#define OV490_SCCB_SLAVE_READ		0x01
> > +#define OV490_SCCB_SLAVE0_DIR		0x80195000
> > +#define OV490_SCCB_SLAVE0_ADDR_HIGH	0x80195001
> > +#define OV490_SCCB_SLAVE0_ADDR_LOW	0x80195002
> > +
> >  #define OV490_DVP_CTRL3			0x80286009
> >
> >  #define OV490_ODS_CTRL_FRAME_OUTPUT_EN	0x0c
> >  #define OV490_ODS_CTRL			0x8029d000
> >
> > +#define OV490_HOST_CMD			0x808000c0
> > +#define OV490_HOST_CMD_TRIGGER		0xc1
> > +
> >  #define OV490_ID_VAL			0x0490
> >  #define OV490_ID(_p, _v)		((((_p) & 0xff) << 8) | ((_v) & 0xff))
> >  #define OV490_PID			0x8080300a
> > @@ -42,12 +55,22 @@
> >  #define OV490_PID_TIMEOUT		20
> >  #define OV490_OUTPUT_EN_TIMEOUT		300
> >
> > +#define OV490_GPIO0_RESETB		0x01
>
> Shouldn't this be named just OV490_GPIO0 ? The fact that it's connected
> to the RESETB signal of the OV10640 is board-specific, not an OV490
> intrinsic property.
>
> BIT(0) ?
>
> > +#define OV490_SPWDN0			0x01
>
> Same here.
>

Correct, I'll fix...

> > +#define OV490_GPIO_SEL0			0x80800050
> > +#define OV490_GPIO_SEL1			0x80800051
> > +#define OV490_GPIO_DIRECTION0		0x80800054
> > +#define OV490_GPIO_DIRECTION1		0x80800055
> > +#define OV490_GPIO_OUTPUT_VALUE0	0x80800058
> > +#define OV490_GPIO_OUTPUT_VALUE1	0x80800059
> > +
> >  #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_ID_LOW			0xa6
> > +#define OV10640_ID_HIGH			0xa6
> > +#define OV10640_CHIP_ID			0x300a
> >  #define OV10640_PIXEL_RATE		55000000
> >
> >  struct rdacm21_device {
> > @@ -306,6 +329,39 @@ static const struct v4l2_subdev_ops rdacm21_subdev_ops = {
> >  	.pad		= &rdacm21_subdev_pad_ops,
> >  };
> >
> > +static int ov10640_initialize(struct rdacm21_device *dev)
> > +{
> > +	u8 val;
> > +
> > +	/* Power-up OV10640 by setting RESETB and PWDNB pins high. */
> > +	ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0_RESETB);
> > +	ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
> > +	ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0_RESETB);
> > +	ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0);
> > +	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0_RESETB);
> > +	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0);
> > +	usleep_range(3000, 5000);
>
> So the OV490 firmware doesn't handle this ?
>

Do you mean the delay or the reset of the ov10640 ?

I need the delay here otherwise reading the ov10640 id fails below.
Same for the reset :)

About reset, it seems it does not... The ov490 settings are loaded
from an EEPROM but I don't know what it content is, maybe it's just
about the imaging-related settings ?

> > +
> > +	/* Read OV10640 ID to test communications. */
> > +	ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR, OV490_SCCB_SLAVE_READ);
> > +	ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH, OV10640_CHIP_ID >> 8);
> > +	ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, (u8)OV10640_CHIP_ID);
> > +
> > +	/* Trigger SCCB slave transaction and give it some time to complete. */
> > +	ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
> > +	usleep_range(1000, 1500);
> > +
> > +	ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, &val);
> > +	if (val != OV10640_ID_HIGH) {
> > +		dev_err(dev->dev, "OV10640 ID mismatch: (0x%02x)\n", val);
> > +		return -ENODEV;
> > +	}
>
> Would it make sense to create an ov490_sensor_read() helper ?
>

While developing this I went and also tested reading other registers
of the ov10640 and I had contradictory results. I'm mostly wondering
if OV490_HOST_CMD_TRIGGER is correct in all cases, as I have some BSP
code that uses a different value.

Can we wait for an helper until we have more users ?

Thanks
  j

> > +
> > +	dev_dbg(dev->dev, "OV10640 ID = 0x%2x\n", val);
> > +
> > +	return 0;
> > +}
> > +
> >  static int ov490_initialize(struct rdacm21_device *dev)
> >  {
> >  	u8 pid, ver, val;
> > @@ -349,20 +405,11 @@ static int ov490_initialize(struct rdacm21_device *dev)
> >  		return -ENODEV;
> >  	}
> >
> > -	/* Read OV10640 Id to test communications. */
> > -	ov490_write_reg(dev, 0x80195000, 0x01);
> > -	ov490_write_reg(dev, 0x80195001, 0x30);
> > -	ov490_write_reg(dev, 0x80195002, 0x0a);
> > -	ov490_write_reg(dev, 0x808000c0, 0xc1);
> > -
> > -	ov490_read_reg(dev, 0x80195000, &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);
> > +	ret = ov10640_initialize(dev);
> > +	if (ret)
> > +		return ret;
> >
> > +	/* Program OV490 with register-value table. */
> >  	for (i = 0; i < ARRAY_SIZE(ov490_regs_wizard); ++i) {
> >  		ret = ov490_write(dev, ov490_regs_wizard[i].reg,
> >  				  ov490_regs_wizard[i].val);
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v7 3/7] fixup! media: i2c: rdacm21: Break-out ov10640 initialization

[Laurent Pinchart]

Hi Jacopo,

On Thu, Jan 14, 2021 at 08:52:28AM +0100, Jacopo Mondi wrote:
> On Thu, Jan 14, 2021 at 01:23:25AM +0200, Laurent Pinchart wrote:
> > On Wed, Jan 13, 2021 at 07:55:01PM +0100, Jacopo Mondi wrote:
> > > The embedded OV490 ISP chip provides a secondary SCCB interface and
> > > two GPIO lines to control the connected OV10640 image sensor.
> > >
> > > Break out the OV10640 initialization from the OV490 initialization and
> > > explicitely control the powerdown and reset GPIOs. After the image
> >
> > s/explicitely/explicitly/
> >
> > > sensor has been hard reset, implement a more clear handling of the
> > > secondary SCCB interface to read the image sensor chip ID.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  drivers/media/i2c/rdacm21.c | 75 ++++++++++++++++++++++++++++++-------
> > >  1 file changed, 61 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> > > index 0428e3209463..944009687de5 100644
> > > --- a/drivers/media/i2c/rdacm21.c
> > > +++ b/drivers/media/i2c/rdacm21.c
> > > @@ -30,11 +30,24 @@
> > >  #define OV490_PAGE_HIGH_REG		0xfffd
> > >  #define OV490_PAGE_LOW_REG		0xfffe
> > >
> > > +/*
> > > + * The SCCB slave handling is undocumented; the registers naming scheme is
> > > + * totally arbitrary.
> > > + */
> > > +#define OV490_SCCB_SLAVE_WRITE		0x00
> > > +#define OV490_SCCB_SLAVE_READ		0x01
> > > +#define OV490_SCCB_SLAVE0_DIR		0x80195000
> > > +#define OV490_SCCB_SLAVE0_ADDR_HIGH	0x80195001
> > > +#define OV490_SCCB_SLAVE0_ADDR_LOW	0x80195002
> > > +
> > >  #define OV490_DVP_CTRL3			0x80286009
> > >
> > >  #define OV490_ODS_CTRL_FRAME_OUTPUT_EN	0x0c
> > >  #define OV490_ODS_CTRL			0x8029d000
> > >
> > > +#define OV490_HOST_CMD			0x808000c0
> > > +#define OV490_HOST_CMD_TRIGGER		0xc1
> > > +
> > >  #define OV490_ID_VAL			0x0490
> > >  #define OV490_ID(_p, _v)		((((_p) & 0xff) << 8) | ((_v) & 0xff))
> > >  #define OV490_PID			0x8080300a
> > > @@ -42,12 +55,22 @@
> > >  #define OV490_PID_TIMEOUT		20
> > >  #define OV490_OUTPUT_EN_TIMEOUT		300
> > >
> > > +#define OV490_GPIO0_RESETB		0x01
> >
> > Shouldn't this be named just OV490_GPIO0 ? The fact that it's connected
> > to the RESETB signal of the OV10640 is board-specific, not an OV490
> > intrinsic property.
> >
> > BIT(0) ?
> >
> > > +#define OV490_SPWDN0			0x01
> >
> > Same here.
> >
> 
> Correct, I'll fix...
> 
> > > +#define OV490_GPIO_SEL0			0x80800050
> > > +#define OV490_GPIO_SEL1			0x80800051
> > > +#define OV490_GPIO_DIRECTION0		0x80800054
> > > +#define OV490_GPIO_DIRECTION1		0x80800055
> > > +#define OV490_GPIO_OUTPUT_VALUE0	0x80800058
> > > +#define OV490_GPIO_OUTPUT_VALUE1	0x80800059
> > > +
> > >  #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_ID_LOW			0xa6
> > > +#define OV10640_ID_HIGH			0xa6
> > > +#define OV10640_CHIP_ID			0x300a
> > >  #define OV10640_PIXEL_RATE		55000000
> > >
> > >  struct rdacm21_device {
> > > @@ -306,6 +329,39 @@ static const struct v4l2_subdev_ops rdacm21_subdev_ops = {
> > >  	.pad		= &rdacm21_subdev_pad_ops,
> > >  };
> > >
> > > +static int ov10640_initialize(struct rdacm21_device *dev)
> > > +{
> > > +	u8 val;
> > > +
> > > +	/* Power-up OV10640 by setting RESETB and PWDNB pins high. */
> > > +	ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0_RESETB);
> > > +	ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
> > > +	ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0_RESETB);
> > > +	ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0);
> > > +	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0_RESETB);
> > > +	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0);
> > > +	usleep_range(3000, 5000);
> >
> > So the OV490 firmware doesn't handle this ?
> 
> Do you mean the delay or the reset of the ov10640 ?
> 
> I need the delay here otherwise reading the ov10640 id fails below.
> Same for the reset :)
> 
> About reset, it seems it does not... The ov490 settings are loaded
> from an EEPROM but I don't know what it content is, maybe it's just
> about the imaging-related settings ?

I meant the configuration of the GPIOs and the reset of the sensor, yes.
I was just curious, as I was expecting the firmware to handle all the
platform-specific data.

> > > +
> > > +	/* Read OV10640 ID to test communications. */
> > > +	ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR, OV490_SCCB_SLAVE_READ);
> > > +	ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH, OV10640_CHIP_ID >> 8);
> > > +	ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, (u8)OV10640_CHIP_ID);
> > > +
> > > +	/* Trigger SCCB slave transaction and give it some time to complete. */
> > > +	ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
> > > +	usleep_range(1000, 1500);
> > > +
> > > +	ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, &val);
> > > +	if (val != OV10640_ID_HIGH) {
> > > +		dev_err(dev->dev, "OV10640 ID mismatch: (0x%02x)\n", val);
> > > +		return -ENODEV;
> > > +	}
> >
> > Would it make sense to create an ov490_sensor_read() helper ?
> 
> While developing this I went and also tested reading other registers
> of the ov10640 and I had contradictory results. I'm mostly wondering
> if OV490_HOST_CMD_TRIGGER is correct in all cases, as I have some BSP
> code that uses a different value.
> 
> Can we wait for an helper until we have more users ?

I don't mind.

> > > +
> > > +	dev_dbg(dev->dev, "OV10640 ID = 0x%2x\n", val);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int ov490_initialize(struct rdacm21_device *dev)
> > >  {
> > >  	u8 pid, ver, val;
> > > @@ -349,20 +405,11 @@ static int ov490_initialize(struct rdacm21_device *dev)
> > >  		return -ENODEV;
> > >  	}
> > >
> > > -	/* Read OV10640 Id to test communications. */
> > > -	ov490_write_reg(dev, 0x80195000, 0x01);
> > > -	ov490_write_reg(dev, 0x80195001, 0x30);
> > > -	ov490_write_reg(dev, 0x80195002, 0x0a);
> > > -	ov490_write_reg(dev, 0x808000c0, 0xc1);
> > > -
> > > -	ov490_read_reg(dev, 0x80195000, &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);
> > > +	ret = ov10640_initialize(dev);
> > > +	if (ret)
> > > +		return ret;
> > >
> > > +	/* Program OV490 with register-value table. */
> > >  	for (i = 0; i < ARRAY_SIZE(ov490_regs_wizard); ++i) {
> > >  		ret = ov490_write(dev, ov490_regs_wizard[i].reg,
> > >  				  ov490_regs_wizard[i].val);

-- 
Regards,

Laurent Pinchart

[PATCH v7 4/7] 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: Rob Herring <robh@kernel.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../bindings/media/i2c/maxim,max9286.yaml     | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index 68ee8c7d9e79..1406236e37ef 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -50,6 +50,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: |
@@ -242,6 +262,8 @@ examples:
         gpio-controller;
         #gpio-cells = <2>;
 
+        maxim,reverse-channel-microvolt = <170000>;
+
         ports {
           #address-cells = <1>;
           #size-cells = <0>;
-- 
2.29.2

Re: [PATCH v7 4/7] dt-bindings: media: max9286: Document 'maxim,reverse-channel-microvolt'

[Sergei Shtylyov]

Hello!

On 13.01.2021 21:55, Jacopo Mondi wrote:

> Document the 'reverse-channel-microvolt' vendor property in the

    Where is "maxim,"?

> bindings document of the max9286 driver.
> 
> The newly introduced property allows to specifying the initial

    Specify?

> configuration of the GMSL reverse control channel to accommodate
> remote serializers pre-programmed with the high threshold power
> supply noise immunity enabled.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
[...]

MBR, Sergei

[PATCH v7 5/7] 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: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
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

[PATCH v7 6/7] 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: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 1cfc8801c0b2..ba84a2d7e29b 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -336,19 +336,28 @@ 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 = max(30U, 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 +966,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

[PATCH v7 7/7] 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: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
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 ba84a2d7e29b..46c4e7b3c40b 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;
 
@@ -556,10 +558,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);
 
 	/*
@@ -966,7 +972,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
@@ -1130,6 +1136,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);
@@ -1220,6 +1227,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