* [PATCH v4 0/8] media: i2c: Add RDACM21 camera module
@ 2020-11-12 16:27 Jacopo Mondi
2020-11-12 16:27 ` [PATCH v4 1/8] media: i2c: Add driver for " Jacopo Mondi
` (7 more replies)
0 siblings, 8 replies; 18+ messages in thread
From: Jacopo Mondi @ 2020-11-12 16:27 UTC (permalink / raw)
To: kieran.bingham+renesas, laurent.pinchart+renesas,
niklas.soderlund+renesas, geert
Cc: Jacopo Mondi, linux-media, linux-renesas-soc, linux-kernel,
Hyun Kwon, Manivannan Sadhasivam
Hello, this v4 follows discussion on how to better handle the initial
reverse channel amplitude and incorporate Geert's and Kieran suggestion
to use the value in millivolts instead of relying on a boolean property.
v4:
- Replace 'maxim-high-threshold' with 'maxim,initial-reverse-channel-mV'
- Increse delay when reading OV490 chip ID to give it a bit more time to
exit reset
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
I have tested on Eagle V3M with 4 RDACM21, but the whole point of
this series is to retain compatibility with RDACM20.
For this reason I have included 2 patches on top, not intended for merge
that re-propose DTS support for the MAXIM max9286 expansion board connected
to Salvator-X and add the newly introduced property to the DTS file.
Kieran, I know you have a working setup with RDACM20, the final two patches are
meant for ease your testing. Can you give this series a spin ?
Series based on latest renesas-drivers tag: renesas-drivers-2020-11-10-v5.10-rc3
If I get a confirmation this setup works on Salvator-X, I'll submit the new
property for inclusion to devicetree people, which I have left out at the
moment.
Thanks
j
Jacopo Mondi (7):
media: i2c: Add driver for RDACM21 camera module
dt-bindings: media: max9286: Document
'maxim,,initial-reverse-channel-mV"
media: i2c: max9286: Break-out reverse channel setup
media: i2c: max9286: Make channel amplitude programmable
media: i2c: max9286: Configure reverse channel amplitude
[DNI] arm64: dts: renesas: salvator-x-max9286: Specify channel
amplitude
[DNI] arm64: dts: renesas: eagle: Specify channel amplitude
Laurent Pinchart (1):
arm64: dts: renesas: salvator-x: Add MAX9286 expansion board
.../bindings/media/i2c/maxim,max9286.yaml | 23 +
MAINTAINERS | 12 +
.../arm64/boot/dts/renesas/r8a77970-eagle.dts | 1 +
.../boot/dts/renesas/salvator-x-max9286.dtsi | 396 +++++++++++++
drivers/media/i2c/Kconfig | 13 +
drivers/media/i2c/Makefile | 2 +
drivers/media/i2c/max9286.c | 58 +-
drivers/media/i2c/rdacm21.c | 539 ++++++++++++++++++
8 files changed, 1031 insertions(+), 13 deletions(-)
create mode 100644 arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi
create mode 100644 drivers/media/i2c/rdacm21.c
--
2.29.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 1/8] media: i2c: Add driver for RDACM21 camera module
2020-11-12 16:27 [PATCH v4 0/8] media: i2c: Add RDACM21 camera module Jacopo Mondi
@ 2020-11-12 16:27 ` Jacopo Mondi
2020-11-12 22:31 ` Kieran Bingham
2020-11-12 16:27 ` [PATCH v4 2/8] dt-bindings: media: max9286: Document 'maxim,,initial-reverse-channel-mV" Jacopo Mondi
` (6 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Jacopo Mondi @ 2020-11-12 16:27 UTC (permalink / raw)
To: kieran.bingham+renesas, laurent.pinchart+renesas,
niklas.soderlund+renesas, geert
Cc: Jacopo Mondi, linux-media, linux-renesas-soc, linux-kernel,
Hyun Kwon, Manivannan Sadhasivam
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 | 539 ++++++++++++++++++++++++++++++++++++
4 files changed, 566 insertions(+)
create mode 100644 drivers/media/i2c/rdacm21.c
diff --git a/MAINTAINERS b/MAINTAINERS
index cef6b6090d76..9a5026fd6788 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14750,6 +14750,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 c64326ca331c..64f4316d11ad 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -1185,6 +1185,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 f0a77473979d..f3641b58929d 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -122,6 +122,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..81529f17b562
--- /dev/null
+++ b/drivers/media/i2c/rdacm21.c
@@ -0,0 +1,539 @@
+// 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_I2C_ADDRESS 0x30
+#define OV10640_ID_LOW 0xa6
+
+#define OV490_I2C_ADDRESS 0x24
+
+#define OV490_ISP_HSIZE_LOW 0x60
+#define OV490_ISP_HSIZE_HIGH 0x61
+#define OV490_ISP_VSIZE_LOW 0x62
+#define OV490_ISP_VSIZE_HIGH 0x63
+
+#define OV490_PID 0x300a
+#define OV490_VER 0x300b
+#define OV490_ID_VAL 0x0490
+#define OV490_ID(_p, _v) ((((_p) & 0xff) << 8) | ((_v) & 0xff))
+
+#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];
+};
+
+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 & 0xff };
+ 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 & 0xff, val };
+ int ret;
+
+ dev_dbg(dev->dev, "%s: 0x%04x = 0x%02x\n", __func__, reg, val);
+
+ 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 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_write(dev, 0xfffd, 0x80);
+ ret = ov490_write(dev, 0xfffe, 0x80);
+ usleep_range(100, 150);
+
+ ret = ov490_read(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(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. */
+ ov490_write(dev, 0xfffd, 0x80);
+ ov490_write(dev, 0xfffe, 0x29);
+ usleep_range(100, 150);
+ for (timeout = 300; timeout > 0; timeout--) {
+ ov490_read(dev, 0xd000, &val);
+ if (val == 0x0c)
+ 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_write(dev, 0xfffd, 0x80);
+ ov490_write(dev, 0xfffe, 0x82);
+ usleep_range(100, 150);
+ ov490_read(dev, OV490_ISP_HSIZE_HIGH, &val);
+ dev->fmt.width = (val & 0xf) << 8;
+ ov490_read(dev, OV490_ISP_HSIZE_LOW, &val);
+ dev->fmt.width |= (val & 0xff);
+
+ ov490_read(dev, OV490_ISP_VSIZE_HIGH, &val);
+ dev->fmt.height = (val & 0xf) << 8;
+ ov490_read(dev, OV490_ISP_VSIZE_LOW, &val);
+ dev->fmt.height |= val & 0xff;
+
+ /* Set bus width to 12 bits [0:11] */
+ ov490_write(dev, 0xfffd, 0x80);
+ ov490_write(dev, 0xfffe, 0x28);
+ usleep_range(100, 150);
+ ov490_write(dev, 0x6009, 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.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 2/8] dt-bindings: media: max9286: Document 'maxim,,initial-reverse-channel-mV"
2020-11-12 16:27 [PATCH v4 0/8] media: i2c: Add RDACM21 camera module Jacopo Mondi
2020-11-12 16:27 ` [PATCH v4 1/8] media: i2c: Add driver for " Jacopo Mondi
@ 2020-11-12 16:27 ` Jacopo Mondi
2020-11-12 22:34 ` Kieran Bingham
2020-11-12 16:27 ` [PATCH v4 3/8] media: i2c: max9286: Break-out reverse channel setup Jacopo Mondi
` (5 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Jacopo Mondi @ 2020-11-12 16:27 UTC (permalink / raw)
To: kieran.bingham+renesas, laurent.pinchart+renesas,
niklas.soderlund+renesas, geert
Cc: Jacopo Mondi, linux-media, linux-renesas-soc, linux-kernel,
Hyun Kwon, Manivannan Sadhasivam
Document the 'initial-reverse-channel-mV' vendor property in the
bindings document of the max9286 driver.
The newly introduced property allows to specify the initial
configuration of the GMSL reverse control channel to accommodate
remote serializers pre-programmed with the high threshold power
supply noise immunity enabled.
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
.../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..c506a0261325 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,initial-reverse-channel-mV:
+ $ref: '/schemas/types.yaml#/definitions/uint32'
+ minimum: 30
+ maximum: 200
+ default: 170
+ description: |
+ Initial amplitude of the reverse control channel, in millivolts.
+
+ The initial amplitude shall be adjusted to a value compatible with the
+ configuration of the connected remote serializer.
+
+ Some camera modules (in 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 170mV.
+
+ A typical value for the reverse channel amplitude to communicate with
+ a remote serializer whose high-threshold noise immunity is not enabled
+ is 100mV.
+
ports:
type: object
description: |
@@ -221,6 +241,7 @@ required:
- ports
- i2c-mux
- gpio-controller
+ - maxim,initial-reverse-channel-mV
additionalProperties: false
@@ -243,6 +264,8 @@ examples:
gpio-controller;
#gpio-cells = <2>;
+ maxim,initial-reverse-channel-mV = <170>;
+
ports {
#address-cells = <1>;
#size-cells = <0>;
--
2.29.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 3/8] media: i2c: max9286: Break-out reverse channel setup
2020-11-12 16:27 [PATCH v4 0/8] media: i2c: Add RDACM21 camera module Jacopo Mondi
2020-11-12 16:27 ` [PATCH v4 1/8] media: i2c: Add driver for " Jacopo Mondi
2020-11-12 16:27 ` [PATCH v4 2/8] dt-bindings: media: max9286: Document 'maxim,,initial-reverse-channel-mV" Jacopo Mondi
@ 2020-11-12 16:27 ` Jacopo Mondi
2020-11-12 16:27 ` [PATCH v4 4/8] media: i2c: max9286: Make channel amplitude programmable Jacopo Mondi
` (4 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Jacopo Mondi @ 2020-11-12 16:27 UTC (permalink / raw)
To: kieran.bingham+renesas, laurent.pinchart+renesas,
niklas.soderlund+renesas, geert
Cc: Jacopo Mondi, linux-media, linux-renesas-soc, linux-kernel,
Hyun Kwon, Manivannan Sadhasivam, Kieran Bingham
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 d969ac21a058..526b6e557dfb 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.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 4/8] media: i2c: max9286: Make channel amplitude programmable
2020-11-12 16:27 [PATCH v4 0/8] media: i2c: Add RDACM21 camera module Jacopo Mondi
` (2 preceding siblings ...)
2020-11-12 16:27 ` [PATCH v4 3/8] media: i2c: max9286: Break-out reverse channel setup Jacopo Mondi
@ 2020-11-12 16:27 ` Jacopo Mondi
2020-11-12 16:27 ` [PATCH v4 5/8] media: i2c: max9286: Configure reverse channel amplitude Jacopo Mondi
` (3 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Jacopo Mondi @ 2020-11-12 16:27 UTC (permalink / raw)
To: kieran.bingham+renesas, laurent.pinchart+renesas,
niklas.soderlund+renesas, geert
Cc: Jacopo Mondi, linux-media, linux-renesas-soc, linux-kernel,
Hyun Kwon, Manivannan Sadhasivam
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 526b6e557dfb..31e27d0f34f1 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.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 5/8] media: i2c: max9286: Configure reverse channel amplitude
2020-11-12 16:27 [PATCH v4 0/8] media: i2c: Add RDACM21 camera module Jacopo Mondi
` (3 preceding siblings ...)
2020-11-12 16:27 ` [PATCH v4 4/8] media: i2c: max9286: Make channel amplitude programmable Jacopo Mondi
@ 2020-11-12 16:27 ` Jacopo Mondi
2020-11-12 22:38 ` Kieran Bingham
2020-11-12 16:27 ` [PATCH v4 6/8] arm64: dts: renesas: salvator-x: Add MAX9286 expansion board Jacopo Mondi
` (2 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Jacopo Mondi @ 2020-11-12 16:27 UTC (permalink / raw)
To: kieran.bingham+renesas, laurent.pinchart+renesas,
niklas.soderlund+renesas, geert
Cc: Jacopo Mondi, linux-media, linux-renesas-soc, linux-kernel,
Hyun Kwon, Manivannan Sadhasivam
Adjust the initial reverse channel amplitude parsing from
firmware interface the 'maxim,initial-reverse-channel-mV'
property.
This change is required for both rdacm20 and rdacm21 camera
modules to be correctly probed when used in combination with
the max9286 deserializer.
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
drivers/media/i2c/max9286.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 31e27d0f34f1..11ba047f3793 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
@@ -1235,6 +1241,18 @@ 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.
+ *
+ * Default it to 170mV for backward compatibility with DTB that do not
+ * provide the property.
+ */
+ if (of_property_read_u32(dev->of_node,
+ "maxim,initial-reverse-channel-mV",
+ &priv->reverse_channel_mV))
+ priv->reverse_channel_mV = 170;
+
priv->route_mask = priv->source_mask;
return 0;
--
2.29.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 6/8] arm64: dts: renesas: salvator-x: Add MAX9286 expansion board
2020-11-12 16:27 [PATCH v4 0/8] media: i2c: Add RDACM21 camera module Jacopo Mondi
` (4 preceding siblings ...)
2020-11-12 16:27 ` [PATCH v4 5/8] media: i2c: max9286: Configure reverse channel amplitude Jacopo Mondi
@ 2020-11-12 16:27 ` Jacopo Mondi
2020-11-12 16:27 ` [PATCH v4 7/8] [DNI] arm64: dts: renesas: salvator-x-max9286: Specify channel amplitude Jacopo Mondi
2020-11-12 16:27 ` [PATCH v4 8/8] [DNI] arm64: dts: renesas: eagle: " Jacopo Mondi
7 siblings, 0 replies; 18+ messages in thread
From: Jacopo Mondi @ 2020-11-12 16:27 UTC (permalink / raw)
To: kieran.bingham+renesas, laurent.pinchart+renesas,
niklas.soderlund+renesas, geert
Cc: Jacopo Mondi, linux-media, linux-renesas-soc, linux-kernel,
Hyun Kwon, Manivannan Sadhasivam
From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Add a .dtsi fragment to describe the MAX9286-based expansion board for
the Renesas Salvator-X board.
The MAX9286 expansion board has eight RDACM20 cameras connected to it.
They can be individually controlled by enabling or disabling the macro
defines.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
v2:
- Use SPDX headers
- Remove link from ADV748x TXA (HDMI)
- Use 0x31-0x38, and 0x41-0x48 for the 8 cameras. 0x30 and 0x40 are the
base addresses for the OV10635 and MAX9271 (0x50 for the MCU)
- Provide RDACM20 MCU I2C address reservations. (0x51-0x58)
v3:
- Fix gmsl-serializer@ i2c node addressing
v6:
- Make i2c-mux child node and update to be conformant to new bindings.
v7:
- Separate register arguments
---
.../boot/dts/renesas/salvator-x-max9286.dtsi | 394 ++++++++++++++++++
1 file changed, 394 insertions(+)
create mode 100644 arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi
diff --git a/arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi b/arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi
new file mode 100644
index 000000000000..6f4798859542
--- /dev/null
+++ b/arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi
@@ -0,0 +1,394 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Device Tree Source for the Salvator-X MAX9286 expansion board
+ *
+ * Copyright (C) 2017 Ideas on Board <laurent.pinchart@ideasonboard.com>
+ */
+
+#include <dt-bindings/gpio/gpio.h>
+
+/*
+ * MAX9286 A
+ */
+#define MAXIM_CAMERA0 "imi,rdacm20"
+#define MAXIM_CAMERA1 "imi,rdacm20"
+#define MAXIM_CAMERA2 "imi,rdacm20"
+#define MAXIM_CAMERA3 "imi,rdacm20"
+
+/*
+ * MAX9286 B
+ */
+#define MAXIM_CAMERA4 "imi,rdacm20"
+#define MAXIM_CAMERA5 "imi,rdacm20"
+#define MAXIM_CAMERA6 "imi,rdacm20"
+#define MAXIM_CAMERA7 "imi,rdacm20"
+
+/ {
+/*
+ * Powered MCU IMI cameras need delay between power-on and R-Car access
+ * to avoid I2C bus conflicts since the R-Car I2C does not support I2C
+ * multi-master. The I2C bus conflict would result in R-Car I2C IP stall.
+ */
+#define IMI_MCU_V0_DELAY 8000000 /* delay for powered MCU firmware v0 */
+#define IMI_MCU_V1_DELAY 3000000 /* delay for powered MCU firmware v1 */
+#define IMI_MCU_NO_DELAY 0 /* delay for unpowered MCU */
+#define IMI_MCU_DELAY IMI_MCU_V0_DELAY
+
+ poc_12v: regulator-vcc-poc-12v {
+ compatible = "regulator-fixed";
+
+ regulator-name = "Camera PoC 12V";
+ regulator-min-microvolt = <12000000>;
+ regulator-max-microvolt = <12000000>;
+ startup-delay-us = <(250000 + IMI_MCU_DELAY)>;
+
+ gpio = <&gpio6 30 GPIO_ACTIVE_HIGH>;
+ enable-active-high;
+ };
+};
+
+&vin0 {
+ status = "okay";
+};
+
+&vin1 {
+ status = "okay";
+};
+
+&vin2 {
+ status = "okay";
+};
+
+&vin3 {
+ status = "okay";
+};
+
+&vin4 {
+ status = "okay";
+};
+
+&vin5 {
+ status = "okay";
+};
+
+&vin6 {
+ status = "okay";
+};
+
+&vin7 {
+ status = "okay";
+};
+
+/* Disconnect the csi40 endpoint from the ADV748x TXA (HDMI) */
+&adv7482_txa {
+ /delete-property/ remote-endpoint;
+ status = "disabled";
+};
+
+&csi40 {
+ status = "okay";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+
+ csi40_in: endpoint {
+ clock-lanes = <0>;
+ data-lanes = <1 2 3 4>;
+ remote-endpoint = <&max9286_out0>;
+ };
+ };
+ };
+};
+
+&csi41 {
+ status = "okay";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+
+ csi41_in: endpoint {
+ clock-lanes = <0>;
+ data-lanes = <1 2 3 4>;
+ remote-endpoint = <&max9286_out1>;
+ };
+ };
+ };
+};
+
+&i2c4 {
+ gmsl-deserializer@4c {
+ compatible = "maxim,max9286";
+ reg = <0x4c>;
+ poc-supply = <&poc_12v>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ max9286_in0: endpoint {
+#ifdef MAXIM_CAMERA0
+ remote-endpoint = <&rdacm20_out0>;
+#endif
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+ max9286_in1: endpoint {
+#ifdef MAXIM_CAMERA1
+ remote-endpoint = <&rdacm20_out1>;
+#endif
+ };
+ };
+
+ port@2 {
+ reg = <2>;
+ max9286_in2: endpoint {
+#ifdef MAXIM_CAMERA2
+ remote-endpoint = <&rdacm20_out2>;
+#endif
+ };
+ };
+
+ port@3 {
+ reg = <3>;
+ max9286_in3: endpoint {
+#ifdef MAXIM_CAMERA3
+ remote-endpoint = <&rdacm20_out3>;
+#endif
+ };
+ };
+
+ port@4 {
+ reg = <4>;
+ max9286_out0: endpoint {
+ clock-lanes = <0>;
+ data-lanes = <1 2 3 4>;
+ remote-endpoint = <&csi40_in>;
+ };
+ };
+ };
+
+ i2c-mux {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ i2c@0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0>;
+
+#ifdef MAXIM_CAMERA0
+ camera@31 {
+ compatible = MAXIM_CAMERA0;
+ reg = <0x31>, <0x41>, <0x51>;
+
+ port {
+ rdacm20_out0: endpoint {
+ remote-endpoint = <&max9286_in0>;
+ };
+ };
+
+ };
+#endif
+ };
+
+ i2c@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+
+#ifdef MAXIM_CAMERA1
+ camera@32 {
+ compatible = MAXIM_CAMERA1;
+ reg = <0x32>, <0x42>, <0x52>;
+ port {
+ rdacm20_out1: endpoint {
+ remote-endpoint = <&max9286_in1>;
+ };
+ };
+ };
+#endif
+ };
+
+ i2c@2 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <2>;
+
+#ifdef MAXIM_CAMERA2
+ camera@33 {
+ compatible = MAXIM_CAMERA2;
+ reg = <0x33>, <0x43>, <0x53>;
+ port {
+ rdacm20_out2: endpoint {
+ remote-endpoint = <&max9286_in2>;
+ };
+ };
+ };
+#endif
+ };
+
+ i2c@3 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <3>;
+
+#ifdef MAXIM_CAMERA3
+ camera@34 {
+ compatible = MAXIM_CAMERA3;
+ reg = <0x34>, <0x44>, <0x54>;
+ port {
+ rdacm20_out3: endpoint {
+ remote-endpoint = <&max9286_in3>;
+ };
+ };
+ };
+#endif
+ };
+ };
+ };
+
+ gmsl-deserializer@6c {
+ compatible = "maxim,max9286";
+ reg = <0x6c>;
+ poc-supply = <&poc_12v>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ max9286_in4: endpoint {
+#ifdef MAXIM_CAMERA4
+ remote-endpoint = <&rdacm20_out4>;
+#endif
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+ max9286_in5: endpoint {
+#ifdef MAXIM_CAMERA5
+ remote-endpoint = <&rdacm20_out5>;
+#endif
+ };
+ };
+
+ port@2 {
+ reg = <2>;
+ max9286_in6: endpoint {
+#ifdef MAXIM_CAMERA6
+ remote-endpoint = <&rdacm20_out6>;
+#endif
+ };
+ };
+
+ port@3 {
+ reg = <3>;
+ max9286_in7: endpoint {
+#ifdef MAXIM_CAMERA7
+ remote-endpoint = <&rdacm20_out7>;
+#endif
+ };
+ };
+
+ port@4 {
+ reg = <4>;
+ max9286_out1: endpoint {
+ clock-lanes = <0>;
+ data-lanes = <1 2 3 4>;
+ remote-endpoint = <&csi41_in>;
+ };
+ };
+ };
+
+ i2c-mux {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ i2c@0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0>;
+
+#ifdef MAXIM_CAMERA4
+ camera@35 {
+ compatible = MAXIM_CAMERA4;
+ reg = <0x35>, <0x45>, <0x55>;
+ port {
+ rdacm20_out4: endpoint {
+ remote-endpoint = <&max9286_in4>;
+ };
+ };
+ };
+#endif
+ };
+
+ i2c@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+
+#ifdef MAXIM_CAMERA5
+ camera@36 {
+ compatible = MAXIM_CAMERA5;
+ reg = <0x36>, <0x46>, <0x56>;
+ port {
+ rdacm20_out5: endpoint {
+ remote-endpoint = <&max9286_in5>;
+ };
+ };
+ };
+#endif
+ };
+
+ i2c@2 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <2>;
+
+#ifdef MAXIM_CAMERA6
+ camera@37 {
+ compatible = MAXIM_CAMERA6;
+ reg = <0x37>, <0x47>, <0x57>;
+ port {
+ rdacm20_out6: endpoint {
+ remote-endpoint = <&max9286_in6>;
+ };
+ };
+ };
+#endif
+ };
+
+ i2c@3 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <3>;
+
+#ifdef MAXIM_CAMERA7
+ camera@38 {
+ compatible = MAXIM_CAMERA7;
+ reg = <0x38>, <0x48>, <0x58>;
+ port {
+ rdacm20_out7: endpoint {
+ remote-endpoint = <&max9286_in7>;
+ };
+ };
+ };
+#endif
+ };
+ };
+ };
+};
--
2.29.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 7/8] [DNI] arm64: dts: renesas: salvator-x-max9286: Specify channel amplitude
2020-11-12 16:27 [PATCH v4 0/8] media: i2c: Add RDACM21 camera module Jacopo Mondi
` (5 preceding siblings ...)
2020-11-12 16:27 ` [PATCH v4 6/8] arm64: dts: renesas: salvator-x: Add MAX9286 expansion board Jacopo Mondi
@ 2020-11-12 16:27 ` Jacopo Mondi
2020-11-12 16:27 ` [PATCH v4 8/8] [DNI] arm64: dts: renesas: eagle: " Jacopo Mondi
7 siblings, 0 replies; 18+ messages in thread
From: Jacopo Mondi @ 2020-11-12 16:27 UTC (permalink / raw)
To: kieran.bingham+renesas, laurent.pinchart+renesas,
niklas.soderlund+renesas, geert
Cc: Jacopo Mondi, linux-media, linux-renesas-soc, linux-kernel,
Hyun Kwon, Manivannan Sadhasivam
Use the newly introduced 'maxim,maxim,initial-reverse-channel-mV'
property to specify the initial reverse channel amplitude when
the remote serializers are pre-programmed with noise immunity threshold
enabled.
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi b/arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi
index 6f4798859542..f14a133b7302 100644
--- a/arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi
+++ b/arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi
@@ -128,6 +128,7 @@ gmsl-deserializer@4c {
compatible = "maxim,max9286";
reg = <0x4c>;
poc-supply = <&poc_12v>;
+ maxim,initial-reverse-channel-mV = <170>;
ports {
#address-cells = <1>;
@@ -263,6 +264,7 @@ gmsl-deserializer@6c {
compatible = "maxim,max9286";
reg = <0x6c>;
poc-supply = <&poc_12v>;
+ maxim,initial-reverse-channel-mV = <170>;
ports {
#address-cells = <1>;
--
2.29.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 8/8] [DNI] arm64: dts: renesas: eagle: Specify channel amplitude
2020-11-12 16:27 [PATCH v4 0/8] media: i2c: Add RDACM21 camera module Jacopo Mondi
` (6 preceding siblings ...)
2020-11-12 16:27 ` [PATCH v4 7/8] [DNI] arm64: dts: renesas: salvator-x-max9286: Specify channel amplitude Jacopo Mondi
@ 2020-11-12 16:27 ` Jacopo Mondi
2020-11-12 20:52 ` Sergei Shtylyov
7 siblings, 1 reply; 18+ messages in thread
From: Jacopo Mondi @ 2020-11-12 16:27 UTC (permalink / raw)
To: kieran.bingham+renesas, laurent.pinchart+renesas,
niklas.soderlund+renesas, geert
Cc: Jacopo Mondi, linux-media, linux-renesas-soc, linux-kernel,
Hyun Kwon, Manivannan Sadhasivam
Use the newly introduced 'maxim,maxim,initial-reverse-channel-mV'
property to specify the initial reverse channel amplitude when the
remote serializers are not pre-programmed with noise immunity threshold
enabled.
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
index 45b947d44cee..75296865104c 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
@@ -308,6 +308,7 @@ gmsl: gmsl-deserializer@48 {
/* eagle-pca9654-max9286-pwdn */
enable-gpios = <&io_expander 0 GPIO_ACTIVE_HIGH>;
+ maxim,initial-reverse-channel-mV = <100>;
/*
* Workaround: Hog the CAMVDD line high as we can't establish a
--
2.29.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v4 8/8] [DNI] arm64: dts: renesas: eagle: Specify channel amplitude
2020-11-12 16:27 ` [PATCH v4 8/8] [DNI] arm64: dts: renesas: eagle: " Jacopo Mondi
@ 2020-11-12 20:52 ` Sergei Shtylyov
0 siblings, 0 replies; 18+ messages in thread
From: Sergei Shtylyov @ 2020-11-12 20:52 UTC (permalink / raw)
To: Jacopo Mondi, kieran.bingham+renesas, laurent.pinchart+renesas,
niklas.soderlund+renesas, geert
Cc: linux-media, linux-renesas-soc, linux-kernel, Hyun Kwon,
Manivannan Sadhasivam
On 11/12/20 7:27 PM, Jacopo Mondi wrote:
> Use the newly introduced 'maxim,maxim,initial-reverse-channel-mV'
"maxim," repeated twice.
> property to specify the initial reverse channel amplitude when the
> remote serializers are not pre-programmed with noise immunity threshold
> enabled.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> index 45b947d44cee..75296865104c 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> @@ -308,6 +308,7 @@ gmsl: gmsl-deserializer@48 {
>
> /* eagle-pca9654-max9286-pwdn */
> enable-gpios = <&io_expander 0 GPIO_ACTIVE_HIGH>;
> + maxim,initial-reverse-channel-mV = <100>;
>
> /*
> * Workaround: Hog the CAMVDD line high as we can't establish a
MBR, Sergei
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/8] media: i2c: Add driver for RDACM21 camera module
2020-11-12 16:27 ` [PATCH v4 1/8] media: i2c: Add driver for " Jacopo Mondi
@ 2020-11-12 22:31 ` Kieran Bingham
2020-11-14 14:04 ` Jacopo Mondi
0 siblings, 1 reply; 18+ messages in thread
From: Kieran Bingham @ 2020-11-12 22:31 UTC (permalink / raw)
To: Jacopo Mondi, laurent.pinchart+renesas, niklas.soderlund+renesas, geert
Cc: linux-media, linux-renesas-soc, linux-kernel, Hyun Kwon,
Manivannan Sadhasivam
Hi Jacopo,
On 12/11/2020 16:27, 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 | 539 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 566 insertions(+)
> create mode 100644 drivers/media/i2c/rdacm21.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index cef6b6090d76..9a5026fd6788 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14750,6 +14750,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 c64326ca331c..64f4316d11ad 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -1185,6 +1185,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 f0a77473979d..f3641b58929d 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -122,6 +122,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..81529f17b562
> --- /dev/null
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -0,0 +1,539 @@
> +// 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_I2C_ADDRESS 0x30
> +#define OV10640_ID_LOW 0xa6
> +
> +#define OV490_I2C_ADDRESS 0x24
> +
> +#define OV490_ISP_HSIZE_LOW 0x60
> +#define OV490_ISP_HSIZE_HIGH 0x61
> +#define OV490_ISP_VSIZE_LOW 0x62
> +#define OV490_ISP_VSIZE_HIGH 0x63
> +
> +#define OV490_PID 0x300a
> +#define OV490_VER 0x300b
> +#define OV490_ID_VAL 0x0490
> +#define OV490_ID(_p, _v) ((((_p) & 0xff) << 8) | ((_v) & 0xff))
> +
> +#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];
> +};
> +
> +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 & 0xff };
> + 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 & 0xff, val };
> + int ret;
> +
> + dev_dbg(dev->dev, "%s: 0x%04x = 0x%02x\n", __func__, reg, val);
> +
> + 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 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_write(dev, 0xfffd, 0x80);
> + ret = ov490_write(dev, 0xfffe, 0x80);
> + usleep_range(100, 150);
> +
> + ret = ov490_read(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(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. */
> + ov490_write(dev, 0xfffd, 0x80);
> + ov490_write(dev, 0xfffe, 0x29);
> + usleep_range(100, 150);
> + for (timeout = 300; timeout > 0; timeout--) {
> + ov490_read(dev, 0xd000, &val);
> + if (val == 0x0c)
What is 0x0c here? Is it something we can better describe in a #define?
> + 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_write(dev, 0xfffd, 0x80);
> + ov490_write(dev, 0xfffe, 0x82);
> + usleep_range(100, 150);
> + ov490_read(dev, OV490_ISP_HSIZE_HIGH, &val);
> + dev->fmt.width = (val & 0xf) << 8;
> + ov490_read(dev, OV490_ISP_HSIZE_LOW, &val);
> + dev->fmt.width |= (val & 0xff);
> +
> + ov490_read(dev, OV490_ISP_VSIZE_HIGH, &val);
> + dev->fmt.height = (val & 0xf) << 8;
> + ov490_read(dev, OV490_ISP_VSIZE_LOW, &val);
> + dev->fmt.height |= val & 0xff;
> +
> + /* Set bus width to 12 bits [0:11] */
> + ov490_write(dev, 0xfffd, 0x80);
> + ov490_write(dev, 0xfffe, 0x28);
> + usleep_range(100, 150);
> + ov490_write(dev, 0x6009, 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");
I think by this point you could chop MODULE_AUTHOR for this one down to
just you ;-)
A fairly arbitrary, and cursory
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
I'll be aiming to test this (series) as soon as I can too.
--
Kieran
> +MODULE_LICENSE("GPL v2");
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 2/8] dt-bindings: media: max9286: Document 'maxim,,initial-reverse-channel-mV"
2020-11-12 16:27 ` [PATCH v4 2/8] dt-bindings: media: max9286: Document 'maxim,,initial-reverse-channel-mV" Jacopo Mondi
@ 2020-11-12 22:34 ` Kieran Bingham
0 siblings, 0 replies; 18+ messages in thread
From: Kieran Bingham @ 2020-11-12 22:34 UTC (permalink / raw)
To: Jacopo Mondi, laurent.pinchart+renesas, niklas.soderlund+renesas, geert
Cc: linux-media, linux-renesas-soc, linux-kernel, Hyun Kwon,
Manivannan Sadhasivam
Hi Jacopo,
in $SUBJECT, there's a double ',' between maxim,,initial and it swaps
from a single quote to a double quote which you might want to fix too.
On 12/11/2020 16:27, Jacopo Mondi wrote:
> Document the 'initial-reverse-channel-mV' vendor property in the
> bindings document of the max9286 driver.
>
> The newly introduced property allows to specify the initial
s/to specify/specifying/
> configuration of the GMSL reverse control channel to accommodate
> remote serializers pre-programmed with the high threshold power
> supply noise immunity enabled.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> .../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..c506a0261325 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,initial-reverse-channel-mV:
> + $ref: '/schemas/types.yaml#/definitions/uint32'
> + minimum: 30
> + maximum: 200
> + default: 170
> + description: |
> + Initial amplitude of the reverse control channel, in millivolts.
> +
> + The initial amplitude shall be adjusted to a value compatible with the
> + configuration of the connected remote serializer.
> +
> + Some camera modules (in example RDACM20) include an on-board MCU that
s/in example/for example/
> + 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 170mV.
> +
> + A typical value for the reverse channel amplitude to communicate with
> + a remote serializer whose high-threshold noise immunity is not enabled
> + is 100mV.
> +
> ports:
> type: object
> description: |
> @@ -221,6 +241,7 @@ required:
> - ports
> - i2c-mux
> - gpio-controller
> + - maxim,initial-reverse-channel-mV
>
> additionalProperties: false
>
> @@ -243,6 +264,8 @@ examples:
> gpio-controller;
> #gpio-cells = <2>;
>
> + maxim,initial-reverse-channel-mV = <170>;
> +
Sounds good to me.
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 5/8] media: i2c: max9286: Configure reverse channel amplitude
2020-11-12 16:27 ` [PATCH v4 5/8] media: i2c: max9286: Configure reverse channel amplitude Jacopo Mondi
@ 2020-11-12 22:38 ` Kieran Bingham
0 siblings, 0 replies; 18+ messages in thread
From: Kieran Bingham @ 2020-11-12 22:38 UTC (permalink / raw)
To: Jacopo Mondi, laurent.pinchart+renesas, niklas.soderlund+renesas, geert
Cc: linux-media, linux-renesas-soc, linux-kernel, Hyun Kwon,
Manivannan Sadhasivam
Hi Jacopo,
On 12/11/2020 16:27, Jacopo Mondi wrote:
> Adjust the initial reverse channel amplitude parsing from
> firmware interface the 'maxim,initial-reverse-channel-mV'
> property.
>
> This change is required for both rdacm20 and rdacm21 camera
> modules to be correctly probed when used in combination with
> the max9286 deserializer.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> drivers/media/i2c/max9286.c | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 31e27d0f34f1..11ba047f3793 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
> @@ -1235,6 +1241,18 @@ 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.
> + *
> + * Default it to 170mV for backward compatibility with DTB that do not
> + * provide the property.
> + */
> + if (of_property_read_u32(dev->of_node,
> + "maxim,initial-reverse-channel-mV",
> + &priv->reverse_channel_mV))
> + priv->reverse_channel_mV = 170;
> +
> priv->route_mask = priv->source_mask;
>
> return 0;
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/8] media: i2c: Add driver for RDACM21 camera module
2020-11-12 22:31 ` Kieran Bingham
@ 2020-11-14 14:04 ` Jacopo Mondi
2020-11-16 9:08 ` Laurent Pinchart
0 siblings, 1 reply; 18+ messages in thread
From: Jacopo Mondi @ 2020-11-14 14:04 UTC (permalink / raw)
To: Kieran Bingham
Cc: Jacopo Mondi, laurent.pinchart+renesas, niklas.soderlund+renesas,
geert, linux-media, linux-renesas-soc, linux-kernel, Hyun Kwon,
Manivannan Sadhasivam
Hi Kieran,
On Thu, Nov 12, 2020 at 10:31:05PM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
[snip]
> > + /* Wait for firmware boot by reading streamon status. */
> > + ov490_write(dev, 0xfffd, 0x80);
> > + ov490_write(dev, 0xfffe, 0x29);
> > + usleep_range(100, 150);
> > + for (timeout = 300; timeout > 0; timeout--) {
> > + ov490_read(dev, 0xd000, &val);
> > + if (val == 0x0c)
>
> What is 0x0c here? Is it something we can better describe in a #define?
>
The 0x0c value itself means "frame output enable" + "whole frame
output enable". I don't think it has much value to define it,
otherwise we would need to define also the register 8029d000
Also, the ov490 is programmed loading the content of a SPI Flash chip,
I guess it's just known that "output enabled" is required to have
stream operations properly working.
> > + 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_write(dev, 0xfffd, 0x80);
> > + ov490_write(dev, 0xfffe, 0x82);
> > + usleep_range(100, 150);
> > + ov490_read(dev, OV490_ISP_HSIZE_HIGH, &val);
> > + dev->fmt.width = (val & 0xf) << 8;
> > + ov490_read(dev, OV490_ISP_HSIZE_LOW, &val);
> > + dev->fmt.width |= (val & 0xff);
> > +
> > + ov490_read(dev, OV490_ISP_VSIZE_HIGH, &val);
> > + dev->fmt.height = (val & 0xf) << 8;
> > + ov490_read(dev, OV490_ISP_VSIZE_LOW, &val);
> > + dev->fmt.height |= val & 0xff;
> > +
> > + /* Set bus width to 12 bits [0:11] */
> > + ov490_write(dev, 0xfffd, 0x80);
> > + ov490_write(dev, 0xfffe, 0x28);
> > + usleep_range(100, 150);
> > + ov490_write(dev, 0x6009, 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");
>
> I think by this point you could chop MODULE_AUTHOR for this one down to
> just you ;-)
>
>
> A fairly arbitrary, and cursory
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> I'll be aiming to test this (series) as soon as I can too.
Thanks, let me know if I should submit for proper inclusion!
Thanks
j
>
> --
> Kieran
>
>
> > +MODULE_LICENSE("GPL v2");
> >
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/8] media: i2c: Add driver for RDACM21 camera module
2020-11-14 14:04 ` Jacopo Mondi
@ 2020-11-16 9:08 ` Laurent Pinchart
2020-11-16 10:03 ` Jacopo Mondi
0 siblings, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2020-11-16 9:08 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Kieran Bingham, Jacopo Mondi, laurent.pinchart+renesas,
niklas.soderlund+renesas, geert, linux-media, linux-renesas-soc,
linux-kernel, Hyun Kwon, Manivannan Sadhasivam
Hi Jacopo,
On Sat, Nov 14, 2020 at 03:04:57PM +0100, Jacopo Mondi wrote:
> On Thu, Nov 12, 2020 at 10:31:05PM +0000, Kieran Bingham wrote:
> > Hi Jacopo,
>
> [snip]
>
> > > + /* Wait for firmware boot by reading streamon status. */
> > > + ov490_write(dev, 0xfffd, 0x80);
> > > + ov490_write(dev, 0xfffe, 0x29);
> > > + usleep_range(100, 150);
> > > + for (timeout = 300; timeout > 0; timeout--) {
> > > + ov490_read(dev, 0xd000, &val);
> > > + if (val == 0x0c)
> >
> > What is 0x0c here? Is it something we can better describe in a #define?
> >
>
> The 0x0c value itself means "frame output enable" + "whole frame
> output enable". I don't think it has much value to define it,
> otherwise we would need to define also the register 8029d000
Shouldn't we have macros for *all* register addresses and fields ?
> Also, the ov490 is programmed loading the content of a SPI Flash chip,
> I guess it's just known that "output enabled" is required to have
> stream operations properly working.
>
> > > + 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_write(dev, 0xfffd, 0x80);
> > > + ov490_write(dev, 0xfffe, 0x82);
> > > + usleep_range(100, 150);
> > > + ov490_read(dev, OV490_ISP_HSIZE_HIGH, &val);
> > > + dev->fmt.width = (val & 0xf) << 8;
> > > + ov490_read(dev, OV490_ISP_HSIZE_LOW, &val);
> > > + dev->fmt.width |= (val & 0xff);
> > > +
> > > + ov490_read(dev, OV490_ISP_VSIZE_HIGH, &val);
> > > + dev->fmt.height = (val & 0xf) << 8;
> > > + ov490_read(dev, OV490_ISP_VSIZE_LOW, &val);
> > > + dev->fmt.height |= val & 0xff;
> > > +
> > > + /* Set bus width to 12 bits [0:11] */
> > > + ov490_write(dev, 0xfffd, 0x80);
> > > + ov490_write(dev, 0xfffe, 0x28);
> > > + usleep_range(100, 150);
> > > + ov490_write(dev, 0x6009, 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");
> >
> > I think by this point you could chop MODULE_AUTHOR for this one down to
> > just you ;-)
> >
> >
> > A fairly arbitrary, and cursory
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >
> > I'll be aiming to test this (series) as soon as I can too.
>
> Thanks, let me know if I should submit for proper inclusion!
>
> > > +MODULE_LICENSE("GPL v2");
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/8] media: i2c: Add driver for RDACM21 camera module
2020-11-16 9:08 ` Laurent Pinchart
@ 2020-11-16 10:03 ` Jacopo Mondi
2020-11-16 10:20 ` Kieran Bingham
0 siblings, 1 reply; 18+ messages in thread
From: Jacopo Mondi @ 2020-11-16 10:03 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Kieran Bingham, Jacopo Mondi, laurent.pinchart+renesas,
niklas.soderlund+renesas, geert, linux-media, linux-renesas-soc,
linux-kernel, Hyun Kwon, Manivannan Sadhasivam
Hi Laurent,
On Mon, Nov 16, 2020 at 11:08:33AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Sat, Nov 14, 2020 at 03:04:57PM +0100, Jacopo Mondi wrote:
> > On Thu, Nov 12, 2020 at 10:31:05PM +0000, Kieran Bingham wrote:
> > > Hi Jacopo,
> >
> > [snip]
> >
> > > > + /* Wait for firmware boot by reading streamon status. */
> > > > + ov490_write(dev, 0xfffd, 0x80);
> > > > + ov490_write(dev, 0xfffe, 0x29);
> > > > + usleep_range(100, 150);
> > > > + for (timeout = 300; timeout > 0; timeout--) {
> > > > + ov490_read(dev, 0xd000, &val);
> > > > + if (val == 0x0c)
> > >
> > > What is 0x0c here? Is it something we can better describe in a #define?
> > >
> >
> > The 0x0c value itself means "frame output enable" + "whole frame
> > output enable". I don't think it has much value to define it,
> > otherwise we would need to define also the register 8029d000
>
> Shouldn't we have macros for *all* register addresses and fields ?
>
I'm not sure it's worth it, we have a single register-value table, and
the way ov490 is programmed, as you can see is to specify the high
bytes of the 32-bits register to write in the special 'page' registers
0xfffd, 0xfffe (which I've not found documented)
ov490_write(dev, 0xfffd, 0x80);
ov490_write(dev, 0xfffe, 0x29);
ov490_read(dev, 0xd000, &val);
This, to my understanding reads register 0x8029d000
We would need three macros, maybe a
PAGE_HIGH(reg) (u8)(reg >> 24)
PAGE_LOW(reg) (u8)(reg >> 16)
REG_LOW(reg) (u16)(reg)
To that's a lot of churn for no gain imho, the code isn't much more
clear
> > Also, the ov490 is programmed loading the content of a SPI Flash chip,
> > I guess it's just known that "output enabled" is required to have
> > stream operations properly working.
> >
> > > > + 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);
Not to add that I don't have register 0x80195000 in the documentation I've
access to (the master SCCB control page is at address 0x8090xxxx
> > > > +
> > > > + ov490_write(dev, 0x5000, 0x01);
> > > > + ov490_write(dev, 0x5001, 0x30);
> > > > + ov490_write(dev, 0x5002, 0x0a)
> > > > + ov490_write(dev, 0xfffe, 0x80);
This sequence in example, reads the 0x300a register of the slave
(ov10640) by programming registers
0x80195000 0x1
0x80195001 0x30
0x80195002 0x0a
> > > > + usleep_range(100, 150);
> > > > + ov490_write(dev, 0xc0, 0xc1);
Triggering a transaction writing 0xc1 to 0x808000c0 (0xc1
undocumented)
> > > > + ov490_write(dev, 0xfffe, 0x19);
> > > > + usleep_range(1000, 1500);
> > > > + ov490_read(dev, 0x5000, &val);
and reading back the transaction result at address 0x80195000
I got these parts from
https://github.com/CogentEmbedded/meta-rcar/blob/v2.12.0/meta-rcar-gen3/recipes-kernel/linux/linux-renesas/0040-H3-MAX9286-TI964-support-add-10635-10640-cameras.patch#L3732
and that's why I kept Vladimir's authorship in MODULE_AUTHORS()
> > > > + 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_write(dev, 0xfffd, 0x80);
> > > > + ov490_write(dev, 0xfffe, 0x82);
> > > > + usleep_range(100, 150);
> > > > + ov490_read(dev, OV490_ISP_HSIZE_HIGH, &val);
> > > > + dev->fmt.width = (val & 0xf) << 8;
> > > > + ov490_read(dev, OV490_ISP_HSIZE_LOW, &val);
> > > > + dev->fmt.width |= (val & 0xff);
> > > > +
> > > > + ov490_read(dev, OV490_ISP_VSIZE_HIGH, &val);
> > > > + dev->fmt.height = (val & 0xf) << 8;
> > > > + ov490_read(dev, OV490_ISP_VSIZE_LOW, &val);
> > > > + dev->fmt.height |= val & 0xff;
> > > > +
> > > > + /* Set bus width to 12 bits [0:11] */
> > > > + ov490_write(dev, 0xfffd, 0x80);
> > > > + ov490_write(dev, 0xfffe, 0x28);
> > > > + usleep_range(100, 150);
> > > > + ov490_write(dev, 0x6009, 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");
> > >
> > > I think by this point you could chop MODULE_AUTHOR for this one down to
> > > just you ;-)
> > >
> > >
> > > A fairly arbitrary, and cursory
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > >
> > > I'll be aiming to test this (series) as soon as I can too.
> >
> > Thanks, let me know if I should submit for proper inclusion!
> >
> > > > +MODULE_LICENSE("GPL v2");
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/8] media: i2c: Add driver for RDACM21 camera module
2020-11-16 10:03 ` Jacopo Mondi
@ 2020-11-16 10:20 ` Kieran Bingham
2020-11-16 11:33 ` Jacopo Mondi
0 siblings, 1 reply; 18+ messages in thread
From: Kieran Bingham @ 2020-11-16 10:20 UTC (permalink / raw)
To: Jacopo Mondi, Laurent Pinchart
Cc: Jacopo Mondi, laurent.pinchart+renesas, niklas.soderlund+renesas,
geert, linux-media, linux-renesas-soc, linux-kernel, Hyun Kwon,
Manivannan Sadhasivam
On 16/11/2020 10:03, Jacopo Mondi wrote:
> Hi Laurent,
>
> On Mon, Nov 16, 2020 at 11:08:33AM +0200, Laurent Pinchart wrote:
>> Hi Jacopo,
>>
>> On Sat, Nov 14, 2020 at 03:04:57PM +0100, Jacopo Mondi wrote:
>>> On Thu, Nov 12, 2020 at 10:31:05PM +0000, Kieran Bingham wrote:
>>>> Hi Jacopo,
>>>
>>> [snip]
>>>
>>>>> + /* Wait for firmware boot by reading streamon status. */
>>>>> + ov490_write(dev, 0xfffd, 0x80);
>>>>> + ov490_write(dev, 0xfffe, 0x29);
>>>>> + usleep_range(100, 150);
>>>>> + for (timeout = 300; timeout > 0; timeout--) {
>>>>> + ov490_read(dev, 0xd000, &val);
>>>>> + if (val == 0x0c)
>>>>
>>>> What is 0x0c here? Is it something we can better describe in a #define?
>>>>
>>>
>>> The 0x0c value itself means "frame output enable" + "whole frame
>>> output enable". I don't think it has much value to define it,
>>> otherwise we would need to define also the register 8029d000
>>
>> Shouldn't we have macros for *all* register addresses and fields ?
>>
>
> I'm not sure it's worth it, we have a single register-value table, and
> the way ov490 is programmed, as you can see is to specify the high
> bytes of the 32-bits register to write in the special 'page' registers
> 0xfffd, 0xfffe (which I've not found documented)
>
> ov490_write(dev, 0xfffd, 0x80);
> ov490_write(dev, 0xfffe, 0x29);
> ov490_read(dev, 0xd000, &val);
>
> This, to my understanding reads register 0x8029d000
>
> We would need three macros, maybe a
> PAGE_HIGH(reg) (u8)(reg >> 24)
> PAGE_LOW(reg) (u8)(reg >> 16)
> REG_LOW(reg) (u16)(reg)
>
> To that's a lot of churn for no gain imho, the code isn't much more
> clear
To me it means we need a couple of helper functions.
ov490_write_long()
ov490_read_long().
(Or other names if appropriate)
Wouldn't that make the code clearer, and more maintainable? And then
allow more correct register definitions to be created?
>>> Also, the ov490 is programmed loading the content of a SPI Flash chip,
>>> I guess it's just known that "output enabled" is required to have
>>> stream operations properly working.
>>>
>>>>> + 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);
The other one was a write to 0x80, 0x29, why is this now to 0x19?
>>>>> + usleep_range(100, 150);
>
> Not to add that I don't have register 0x80195000 in the documentation I've
Aha, so writing to 0xfffd, 0xfffe, 'sets' the top page. I see.
And I presume there must always be a usleep_range(100, 150) after any
page change?
Really sounds like a helper function to me, which keeps track of the
current page, and updates the page when needed.
> access to (the master SCCB control page is at address 0x8090xxxx
>
>>>>> +
>>>>> + ov490_write(dev, 0x5000, 0x01);
>>>>> + ov490_write(dev, 0x5001, 0x30);
>>>>> + ov490_write(dev, 0x5002, 0x0a)
>>>>> + ov490_write(dev, 0xfffe, 0x80);
>
> This sequence in example, reads the 0x300a register of the slave
> (ov10640) by programming registers
> 0x80195000 0x1
> 0x80195001 0x30
> 0x80195002 0x0a
>
>>>>> + usleep_range(100, 150);
>>>>> + ov490_write(dev, 0xc0, 0xc1);
>
> Triggering a transaction writing 0xc1 to 0x808000c0 (0xc1
> undocumented)
Do we do many reads of the connected sensor? or is this the only one.
If you know that's what's happening, I'd be tempted to say we should
wrap that up in a function to make it clearer too.
>>>>> + ov490_write(dev, 0xfffe, 0x19);
>>>>> + usleep_range(1000, 1500);
>>>>> + ov490_read(dev, 0x5000, &val);
>
> and reading back the transaction result at address 0x80195000
>
> I got these parts from
> https://github.com/CogentEmbedded/meta-rcar/blob/v2.12.0/meta-rcar-gen3/recipes-kernel/linux/linux-renesas/0040-H3-MAX9286-TI964-support-add-10635-10640-cameras.patch#L3732
>
> and that's why I kept Vladimir's authorship in MODULE_AUTHORS()
That's fine, but I think we can do better than dragging that code in
directly.
>>>>> + 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_write(dev, 0xfffd, 0x80);
>>>>> + ov490_write(dev, 0xfffe, 0x82);
>>>>> + usleep_range(100, 150);
>>>>> + ov490_read(dev, OV490_ISP_HSIZE_HIGH, &val);
>>>>> + dev->fmt.width = (val & 0xf) << 8;
>>>>> + ov490_read(dev, OV490_ISP_HSIZE_LOW, &val);
>>>>> + dev->fmt.width |= (val & 0xff);
>>>>> +
>>>>> + ov490_read(dev, OV490_ISP_VSIZE_HIGH, &val);
>>>>> + dev->fmt.height = (val & 0xf) << 8;
>>>>> + ov490_read(dev, OV490_ISP_VSIZE_LOW, &val);
>>>>> + dev->fmt.height |= val & 0xff;
>>>>> +
>>>>> + /* Set bus width to 12 bits [0:11] */
>>>>> + ov490_write(dev, 0xfffd, 0x80);
>>>>> + ov490_write(dev, 0xfffe, 0x28);
>>>>> + usleep_range(100, 150);
>>>>> + ov490_write(dev, 0x6009, 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");
>>>>
>>>> I think by this point you could chop MODULE_AUTHOR for this one down to
>>>> just you ;-)
>>>>
>>>>
>>>> A fairly arbitrary, and cursory
>>>>
>>>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>>>
>>>> I'll be aiming to test this (series) as soon as I can too.
>>>
>>> Thanks, let me know if I should submit for proper inclusion!
>>>
>>>>> +MODULE_LICENSE("GPL v2");
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/8] media: i2c: Add driver for RDACM21 camera module
2020-11-16 10:20 ` Kieran Bingham
@ 2020-11-16 11:33 ` Jacopo Mondi
0 siblings, 0 replies; 18+ messages in thread
From: Jacopo Mondi @ 2020-11-16 11:33 UTC (permalink / raw)
To: Kieran Bingham
Cc: Laurent Pinchart, Jacopo Mondi, laurent.pinchart+renesas,
niklas.soderlund+renesas, geert, linux-media, linux-renesas-soc,
linux-kernel, Hyun Kwon, Manivannan Sadhasivam
Hi Kieran,
On Mon, Nov 16, 2020 at 10:20:47AM +0000, Kieran Bingham wrote:
> On 16/11/2020 10:03, Jacopo Mondi wrote:
> > Hi Laurent,
> >
> > On Mon, Nov 16, 2020 at 11:08:33AM +0200, Laurent Pinchart wrote:
> >> Hi Jacopo,
> >>
> >> On Sat, Nov 14, 2020 at 03:04:57PM +0100, Jacopo Mondi wrote:
> >>> On Thu, Nov 12, 2020 at 10:31:05PM +0000, Kieran Bingham wrote:
> >>>> Hi Jacopo,
> >>>
> >>> [snip]
> >>>
> >>>>> + /* Wait for firmware boot by reading streamon status. */
> >>>>> + ov490_write(dev, 0xfffd, 0x80);
> >>>>> + ov490_write(dev, 0xfffe, 0x29);
> >>>>> + usleep_range(100, 150);
> >>>>> + for (timeout = 300; timeout > 0; timeout--) {
> >>>>> + ov490_read(dev, 0xd000, &val);
> >>>>> + if (val == 0x0c)
> >>>>
> >>>> What is 0x0c here? Is it something we can better describe in a #define?
> >>>>
> >>>
> >>> The 0x0c value itself means "frame output enable" + "whole frame
> >>> output enable". I don't think it has much value to define it,
> >>> otherwise we would need to define also the register 8029d000
> >>
> >> Shouldn't we have macros for *all* register addresses and fields ?
> >>
> >
> > I'm not sure it's worth it, we have a single register-value table, and
> > the way ov490 is programmed, as you can see is to specify the high
> > bytes of the 32-bits register to write in the special 'page' registers
> > 0xfffd, 0xfffe (which I've not found documented)
> >
> > ov490_write(dev, 0xfffd, 0x80);
> > ov490_write(dev, 0xfffe, 0x29);
> > ov490_read(dev, 0xd000, &val);
> >
> > This, to my understanding reads register 0x8029d000
> >
> > We would need three macros, maybe a
> > PAGE_HIGH(reg) (u8)(reg >> 24)
> > PAGE_LOW(reg) (u8)(reg >> 16)
> > REG_LOW(reg) (u16)(reg)
> >
> > To that's a lot of churn for no gain imho, the code isn't much more
> > clear
>
> To me it means we need a couple of helper functions.
>
> ov490_write_long()
> ov490_read_long().
>
> (Or other names if appropriate)
>
> Wouldn't that make the code clearer, and more maintainable? And then
> allow more correct register definitions to be created?
>
>
>
> >>> Also, the ov490 is programmed loading the content of a SPI Flash chip,
> >>> I guess it's just known that "output enabled" is required to have
> >>> stream operations properly working.
> >>>
> >>>>> + 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);
>
> The other one was a write to 0x80, 0x29, why is this now to 0x19?
>
>
> >>>>> + usleep_range(100, 150);
> >
> > Not to add that I don't have register 0x80195000 in the documentation I've
>
> Aha, so writing to 0xfffd, 0xfffe, 'sets' the top page. I see.
>
> And I presume there must always be a usleep_range(100, 150) after any
> page change?
>
> Really sounds like a helper function to me, which keeps track of the
> current page, and updates the page when needed.
Ok, I'll make an helper to select the register's and handle the top
page internally.
I would leave communications with ov10640 out, as long as we don't
have other users and I better understand what's register 5000 for.
>
>
>
> > access to (the master SCCB control page is at address 0x8090xxxx
> >
> >>>>> +
> >>>>> + ov490_write(dev, 0x5000, 0x01);
> >>>>> + ov490_write(dev, 0x5001, 0x30);
> >>>>> + ov490_write(dev, 0x5002, 0x0a)
> >>>>> + ov490_write(dev, 0xfffe, 0x80);
> >
> > This sequence in example, reads the 0x300a register of the slave
> > (ov10640) by programming registers
> > 0x80195000 0x1
> > 0x80195001 0x30
> > 0x80195002 0x0a
> >
> >>>>> + usleep_range(100, 150);
> >>>>> + ov490_write(dev, 0xc0, 0xc1);
> >
> > Triggering a transaction writing 0xc1 to 0x808000c0 (0xc1
> > undocumented)
>
>
> Do we do many reads of the connected sensor? or is this the only one.
> If you know that's what's happening, I'd be tempted to say we should
> wrap that up in a function to make it clearer too.
>
>
>
> >>>>> + ov490_write(dev, 0xfffe, 0x19);
> >>>>> + usleep_range(1000, 1500);
> >>>>> + ov490_read(dev, 0x5000, &val);
> >
> > and reading back the transaction result at address 0x80195000
> >
> > I got these parts from
> > https://github.com/CogentEmbedded/meta-rcar/blob/v2.12.0/meta-rcar-gen3/recipes-kernel/linux/linux-renesas/0040-H3-MAX9286-TI964-support-add-10635-10640-cameras.patch#L3732
> >
> > and that's why I kept Vladimir's authorship in MODULE_AUTHORS()
>
> That's fine, but I think we can do better than dragging that code in
> directly.
>
>
>
> >>>>> + 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_write(dev, 0xfffd, 0x80);
> >>>>> + ov490_write(dev, 0xfffe, 0x82);
> >>>>> + usleep_range(100, 150);
> >>>>> + ov490_read(dev, OV490_ISP_HSIZE_HIGH, &val);
> >>>>> + dev->fmt.width = (val & 0xf) << 8;
> >>>>> + ov490_read(dev, OV490_ISP_HSIZE_LOW, &val);
> >>>>> + dev->fmt.width |= (val & 0xff);
> >>>>> +
> >>>>> + ov490_read(dev, OV490_ISP_VSIZE_HIGH, &val);
> >>>>> + dev->fmt.height = (val & 0xf) << 8;
> >>>>> + ov490_read(dev, OV490_ISP_VSIZE_LOW, &val);
> >>>>> + dev->fmt.height |= val & 0xff;
> >>>>> +
> >>>>> + /* Set bus width to 12 bits [0:11] */
> >>>>> + ov490_write(dev, 0xfffd, 0x80);
> >>>>> + ov490_write(dev, 0xfffe, 0x28);
> >>>>> + usleep_range(100, 150);
> >>>>> + ov490_write(dev, 0x6009, 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");
> >>>>
> >>>> I think by this point you could chop MODULE_AUTHOR for this one down to
> >>>> just you ;-)
> >>>>
> >>>>
> >>>> A fairly arbitrary, and cursory
> >>>>
> >>>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >>>>
> >>>> I'll be aiming to test this (series) as soon as I can too.
> >>>
> >>> Thanks, let me know if I should submit for proper inclusion!
> >>>
> >>>>> +MODULE_LICENSE("GPL v2");
> >>
> >> --
> >> Regards,
> >>
> >> Laurent Pinchart
>
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-11-16 12:38 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-12 16:27 [PATCH v4 0/8] media: i2c: Add RDACM21 camera module Jacopo Mondi
2020-11-12 16:27 ` [PATCH v4 1/8] media: i2c: Add driver for " Jacopo Mondi
2020-11-12 22:31 ` Kieran Bingham
2020-11-14 14:04 ` Jacopo Mondi
2020-11-16 9:08 ` Laurent Pinchart
2020-11-16 10:03 ` Jacopo Mondi
2020-11-16 10:20 ` Kieran Bingham
2020-11-16 11:33 ` Jacopo Mondi
2020-11-12 16:27 ` [PATCH v4 2/8] dt-bindings: media: max9286: Document 'maxim,,initial-reverse-channel-mV" Jacopo Mondi
2020-11-12 22:34 ` Kieran Bingham
2020-11-12 16:27 ` [PATCH v4 3/8] media: i2c: max9286: Break-out reverse channel setup Jacopo Mondi
2020-11-12 16:27 ` [PATCH v4 4/8] media: i2c: max9286: Make channel amplitude programmable Jacopo Mondi
2020-11-12 16:27 ` [PATCH v4 5/8] media: i2c: max9286: Configure reverse channel amplitude Jacopo Mondi
2020-11-12 22:38 ` Kieran Bingham
2020-11-12 16:27 ` [PATCH v4 6/8] arm64: dts: renesas: salvator-x: Add MAX9286 expansion board Jacopo Mondi
2020-11-12 16:27 ` [PATCH v4 7/8] [DNI] arm64: dts: renesas: salvator-x-max9286: Specify channel amplitude Jacopo Mondi
2020-11-12 16:27 ` [PATCH v4 8/8] [DNI] arm64: dts: renesas: eagle: " Jacopo Mondi
2020-11-12 20:52 ` Sergei Shtylyov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).