| Message ID | 20201116135305.81319-1-jacopo+renesas@jmondi.org |
|---|---|
| Headers | show |
| Series | media: i2c: Add RDACM21 camera module | expand |
Hi Kieran, On Mon, Nov 16, 2020 at 02:47:49PM +0000, Kieran Bingham wrote: > On 16/11/2020 13:52, Jacopo Mondi wrote: > > The RDACM21 is a GMSL camera supporting 1280x1080 resolution images > > developed by IMI based on an Omnivision OV10640 sensor, an Omnivision > > OV490 ISP and a Maxim MAX9271 GMSL serializer. > > > > The driver uses the max9271 library module, to maximize code reuse with > > other camera module drivers using the same serializer, such as rdacm20. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > MAINTAINERS | 12 + > > drivers/media/i2c/Kconfig | 13 + > > drivers/media/i2c/Makefile | 2 + > > drivers/media/i2c/rdacm21.c | 595 ++++++++++++++++++++++++++++++++++++ > > 4 files changed, 622 insertions(+) > > create mode 100644 drivers/media/i2c/rdacm21.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 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..5f9267e26258 > > --- /dev/null > > +++ b/drivers/media/i2c/rdacm21.c > > @@ -0,0 +1,595 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * IMI RDACM21 GMSL Camera Driver > > + * > > + * Copyright (C) 2017-2020 Jacopo Mondi > > + * Copyright (C) 2017-2019 Kieran Bingham > > + * Copyright (C) 2017-2019 Laurent Pinchart > > + * Copyright (C) 2017-2019 Niklas Söderlund > > + * Copyright (C) 2016 Renesas Electronics Corporation > > + * Copyright (C) 2015 Cogent Embedded, Inc. > > + */ > > + > > +#include <linux/delay.h> > > +#include <linux/fwnode.h> > > +#include <linux/init.h> > > +#include <linux/i2c.h> > > +#include <linux/module.h> > > +#include <linux/slab.h> > > +#include <linux/videodev2.h> > > + > > +#include <media/v4l2-async.h> > > +#include <media/v4l2-ctrls.h> > > +#include <media/v4l2-subdev.h> > > +#include "max9271.h" > > + > > +#define OV10640_ID_LOW 0xa6 > > + > > +#define OV490_I2C_ADDRESS 0x24 > > + > > +#define OV490_PAGE_HIGH_REG 0xfffd > > +#define OV490_PAGE_LOW_REG 0xfffe > > + > > +#define OV490_DVP_CTRL3 0x80286009 > > + > > +#define OV490_ODS_CTRL_FRAME_OUTPUT_EN 0x0c > > +#define OV490_ODS_CTRL 0x8029d000 > > + > > +#define OV490_ID_VAL 0x0490 > > +#define OV490_ID(_p, _v) ((((_p) & 0xff) << 8) | ((_v) & 0xff)) > > +#define OV490_PID 0x8080300a > > +#define OV490_VER 0x8080300b > > + > > +#define OV490_ISP_HSIZE_LOW 0x80820060 > > +#define OV490_ISP_HSIZE_HIGH 0x80820061 > > +#define OV490_ISP_VSIZE_LOW 0x80820062 > > +#define OV490_ISP_VSIZE_HIGH 0x80820063 > > + > > +#define OV10640_PIXEL_RATE (55000000) > > + > > +struct rdacm21_device { > > + struct device *dev; > > + struct max9271_device *serializer; > > + struct i2c_client *isp; > > + struct v4l2_subdev sd; > > + struct media_pad pad; > > + struct v4l2_mbus_framefmt fmt; > > + struct v4l2_ctrl_handler ctrls; > > + u32 addrs[32]; > > + u16 last_page; > > +}; > > + > > +static inline struct rdacm21_device *sd_to_rdacm21(struct v4l2_subdev *sd) > > +{ > > + return container_of(sd, struct rdacm21_device, sd); > > +} > > + > > +static inline struct rdacm21_device *i2c_to_rdacm21(struct i2c_client *client) > > +{ > > + return sd_to_rdacm21(i2c_get_clientdata(client)); > > +} > > + > > +static const struct ov490_reg { > > + u16 reg; > > + u8 val; > > +} ov490_regs_wizard[] = { > > + {0xfffd, 0x80}, > > + {0xfffe, 0x82}, > > + {0x0071, 0x11}, > > + {0x0075, 0x11}, > > + {0xfffe, 0x29}, > > + {0x6010, 0x01}, > > + /* > > + * OV490 EMB line disable in YUV and RAW data, > > + * NOTE: EMB line is still used in ISP and sensor > > + */ > > + {0xe000, 0x14}, > > + {0xfffe, 0x28}, > > + {0x6000, 0x04}, > > + {0x6004, 0x00}, > > + /* > > + * PCLK polarity - useless due to silicon bug. > > + * Use 0x808000bb register instead. > > + */ > > + {0x6008, 0x00}, > > + {0xfffe, 0x80}, > > + {0x0091, 0x00}, > > + /* bit[3]=0 - PCLK polarity workaround. */ > > + {0x00bb, 0x1d}, > > + /* Ov490 FSIN: app_fsin_from_fsync */ > > + {0xfffe, 0x85}, > > + {0x0008, 0x00}, > > + {0x0009, 0x01}, > > + /* FSIN0 source. */ > > + {0x000A, 0x05}, > > + {0x000B, 0x00}, > > + /* FSIN0 delay. */ > > + {0x0030, 0x02}, > > + {0x0031, 0x00}, > > + {0x0032, 0x00}, > > + {0x0033, 0x00}, > > + /* FSIN1 delay. */ > > + {0x0038, 0x02}, > > + {0x0039, 0x00}, > > + {0x003A, 0x00}, > > + {0x003B, 0x00}, > > + /* FSIN0 length. */ > > + {0x0070, 0x2C}, > > + {0x0071, 0x01}, > > + {0x0072, 0x00}, > > + {0x0073, 0x00}, > > + /* FSIN1 length. */ > > + {0x0074, 0x64}, > > + {0x0075, 0x00}, > > + {0x0076, 0x00}, > > + {0x0077, 0x00}, > > + {0x0000, 0x14}, > > + {0x0001, 0x00}, > > + {0x0002, 0x00}, > > + {0x0003, 0x00}, > > + /* > > + * Load fsin0,load fsin1,load other, > > + * It will be cleared automatically. > > + */ > > + {0x0004, 0x32}, > > + {0x0005, 0x00}, > > + {0x0006, 0x00}, > > + {0x0007, 0x00}, > > + {0xfffe, 0x80}, > > + /* Sensor FSIN. */ > > + {0x0081, 0x00}, > > + /* ov10640 FSIN enable */ > > + {0xfffe, 0x19}, > > + {0x5000, 0x00}, > > + {0x5001, 0x30}, > > + {0x5002, 0x8c}, > > + {0x5003, 0xb2}, > > + {0xfffe, 0x80}, > > + {0x00c0, 0xc1}, > > + /* ov10640 HFLIP=1 by default */ > > + {0xfffe, 0x19}, > > + {0x5000, 0x01}, > > + {0x5001, 0x00}, > > + {0xfffe, 0x80}, > > + {0x00c0, 0xdc}, > > +}; > > + > > +static int ov490_read(struct rdacm21_device *dev, u16 reg, u8 *val) > > +{ > > + u8 buf[2] = { reg >> 8, reg }; > > + int ret; > > + > > + ret = i2c_master_send(dev->isp, buf, 2); > > + if (ret == 2) > > + ret = i2c_master_recv(dev->isp, val, 1); > > + > > + if (ret < 0) { > > + dev_dbg(dev->dev, "%s: register 0x%04x read failed (%d)\n", > > + __func__, reg, ret); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int ov490_write(struct rdacm21_device *dev, u16 reg, u8 val) > > +{ > > + u8 buf[3] = { reg >> 8, reg, val }; > > + int ret; > > + > > + ret = i2c_master_send(dev->isp, buf, 3); > > + if (ret < 0) { > > + dev_err(dev->dev, "%s: register 0x%04x write failed (%d)\n", > > + __func__, reg, ret); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int ov490_set_page(struct rdacm21_device *dev, u16 reg) > > +{ > > + bool page_new = false; > > + u8 page_high = reg >> 8; > > + u8 page_low = reg; > > + int ret; > > + > > + if (page_high != (dev->last_page >> 8)) { > > + ret = ov490_write(dev, OV490_PAGE_HIGH_REG, page_high); > > + if (ret) > > + return ret; > > + page_new = true; > > + } > > + > > + if (page_low != (u8)dev->last_page) { > > + ret = ov490_write(dev, OV490_PAGE_LOW_REG, page_low); > > + if (ret) > > + return ret; > > + page_new = true; > > + } > > + > > + if (page_new) { > > + dev->last_page = reg; > > + usleep_range(100, 150); > > + } > > + > > + return 0; > > +} > > + > > +static int ov490_read_reg(struct rdacm21_device *dev, u32 reg, u8 *val) > > +{ > > + int ret; > > + > > + ret = ov490_set_page(dev, reg >> 16); > > + if (ret) > > + return ret; > > + > > + ret = ov490_read(dev, (u16)reg, val); > > + if (ret) > > + return ret; > > + > > + dev_dbg(dev->dev, "%s: 0x%08x = 0x%02x\n", __func__, reg, *val); > > + > > + return 0; > > +} > > + > > +static int ov490_write_reg(struct rdacm21_device *dev, u32 reg, u8 val) > > +{ > > + int ret; > > + > > + ret = ov490_set_page(dev, reg >> 16); > > + if (ret) > > + return ret; > > + > > + ret = ov490_write(dev, (u16)reg, val); > > + if (ret) > > + return ret; > > + > > + dev_dbg(dev->dev, "%s: 0x%08x = 0x%02x\n", __func__, reg, val); > > + > > + return 0; > > +} > > + > > +static int rdacm21_s_stream(struct v4l2_subdev *sd, int enable) > > +{ > > + struct rdacm21_device *dev = sd_to_rdacm21(sd); > > + > > + /* > > + * Enable serial link now that the ISP provides a valid pixel clock > > + * to start serializing video data on the GMSL link. > > + */ > > + return max9271_set_serial_link(dev->serializer, enable); > > +} > > + > > +static int rdacm21_enum_mbus_code(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_mbus_code_enum *code) > > +{ > > + if (code->pad || code->index > 0) > > + return -EINVAL; > > + > > + code->code = MEDIA_BUS_FMT_YUYV8_1X16; > > + > > + return 0; > > +} > > + > > +static int rdacm21_get_fmt(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *format) > > +{ > > + struct v4l2_mbus_framefmt *mf = &format->format; > > + struct rdacm21_device *dev = sd_to_rdacm21(sd); > > + > > + if (format->pad) > > + return -EINVAL; > > + > > + mf->width = dev->fmt.width; > > + mf->height = dev->fmt.height; > > + mf->code = MEDIA_BUS_FMT_YUYV8_1X16; > > + mf->colorspace = V4L2_COLORSPACE_SRGB; > > + mf->field = V4L2_FIELD_NONE; > > + mf->ycbcr_enc = V4L2_YCBCR_ENC_601; > > + mf->quantization = V4L2_QUANTIZATION_FULL_RANGE; > > + mf->xfer_func = V4L2_XFER_FUNC_NONE; > > + > > + return 0; > > +} > > + > > +static struct v4l2_subdev_video_ops rdacm21_video_ops = { > > + .s_stream = rdacm21_s_stream, > > +}; > > + > > +static const struct v4l2_subdev_pad_ops rdacm21_subdev_pad_ops = { > > + .enum_mbus_code = rdacm21_enum_mbus_code, > > + .get_fmt = rdacm21_get_fmt, > > + .set_fmt = rdacm21_get_fmt, > > +}; > > + > > +static struct v4l2_subdev_ops rdacm21_subdev_ops = { > > + .video = &rdacm21_video_ops, > > + .pad = &rdacm21_subdev_pad_ops, > > +}; > > + > > +static int ov490_initialize(struct rdacm21_device *dev) > > +{ > > + unsigned int ov490_pid_retry = 20; > > + unsigned int timeout; > > + u8 pid, ver, val; > > + unsigned int i; > > + int ret; > > + > > + /* Read OV490 Id to test communications. */ > > +pid_retry: > > + ret = ov490_read_reg(dev, OV490_PID, &pid); > > + if (ret < 0) { > > + /* Give OV490 a few more cycles to exit from reset. */ > > + if (ov490_pid_retry--) { > > + usleep_range(1000, 2000); > > + goto pid_retry; > > + } > > + > > + dev_err(dev->dev, "OV490 PID read failed (%d)\n", ret); > > + return ret; > > + } > > + > > + ret = ov490_read_reg(dev, OV490_VER, &ver); > > + if (ret < 0) { > > + dev_err(dev->dev, "OV490 VERSION read failed (%d)\n", ret); > > + return ret; > > + } > > + > > + if (OV490_ID(pid, ver) != OV490_ID_VAL) { > > + dev_err(dev->dev, "OV490 ID mismatch: (0x%04x)\n", > > + OV490_ID(pid, ver)); > > + return -ENODEV; > > + } > > + > > + /* Wait for firmware boot by reading streamon status. */ > > + for (timeout = 300; timeout > 0; timeout--) { > > + ov490_read_reg(dev, OV490_ODS_CTRL, &val); > > + if (val == OV490_ODS_CTRL_FRAME_OUTPUT_EN) > > + break; > > + mdelay(1); > > + } > > + if (!timeout) { > > + dev_err(dev->dev, "Timeout firmware boot wait\n"); > > + return -ENODEV; > > + } > > + dev_dbg(dev->dev, "Firmware booted in %u msec\n", 300 - timeout); > > + > > + /* Read OV10640 Id to test communications. */ > > + ov490_write(dev, 0xfffd, 0x80); > > + ov490_write(dev, 0xfffe, 0x19); > > + usleep_range(100, 150); > > Is that setting a page? Or doing something else? > > > + > > + 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); > > And here? > > Can this not go through the new helpers? > > If the 'read_reg', 'write_reg', helpers are not appropriate, at least > setting the pages could be done with set_page() ... but I'd be surprised > if those couldn't be handled with ov490_write_reg() ? As I've said in previous reply, I prefer the comunication with ov10640 to be kept explicit as long as we don't have more users and I don't better understand what register 0x5000 is for. I can go through set_page() but the I have to pass 0x8019 which kind of defeat the purpose of defining registers by name I prefer to keep this as it is. > > > > > + usleep_range(1000, 1500); > > + ov490_read(dev, 0x5000, &val); > > + if (val != OV10640_ID_LOW) { > > + dev_err(dev->dev, "OV10640 ID mismatch: (0x%02x)\n", val); > > + return -ENODEV; > > + } > > + > > + dev_dbg(dev->dev, "OV10640 ID = 0x%2x\n", val); > > + > > + for (i = 0; i < ARRAY_SIZE(ov490_regs_wizard); ++i) { > > + ret = ov490_write(dev, ov490_regs_wizard[i].reg, > > + ov490_regs_wizard[i].val); > > + if (ret < 0) { > > + dev_err(dev->dev, > > + "%s: register %u (0x%04x) write failed (%d)\n", > > + __func__, i, ov490_regs_wizard[i].reg, ret); > > + > > + return -EIO; > > + } > > + > > + usleep_range(100, 150); > > + } > > + > > + /* > > + * The ISP is programmed with the content of a serial flash memory. > > + * Read the firmware configuration to reflect it through the V4L2 APIs. > > + */ > > + ov490_read_reg(dev, OV490_ISP_HSIZE_HIGH, &val); > > + dev->fmt.width = (val & 0xf) << 8; > > + ov490_read_reg(dev, OV490_ISP_HSIZE_LOW, &val); > > + dev->fmt.width |= (val & 0xff); > > + > > + ov490_read_reg(dev, OV490_ISP_VSIZE_HIGH, &val); > > + dev->fmt.height = (val & 0xf) << 8; > > + ov490_read_reg(dev, OV490_ISP_VSIZE_LOW, &val); > > + dev->fmt.height |= val & 0xff; > > + > > > Hrm, these look like "ov490_read_reg_long()", but given they're the only > occurrences of a "long read" I think we could leave it as is. > > > > + /* Set bus width to 12 bits with [0:11] ordering. */ > > + ov490_write_reg(dev, OV490_DVP_CTRL3, 0x10); > > + > > + dev_info(dev->dev, "Identified RDACM21 camera module\n"); > > + > > + return 0; > > +} > > + > > +static int rdacm21_initialize(struct rdacm21_device *dev) > > +{ > > + int ret; > > + > > + /* Verify communication with the MAX9271: ping to wakeup. */ > > + dev->serializer->client->addr = MAX9271_DEFAULT_ADDR; > > + i2c_smbus_read_byte(dev->serializer->client); > > + > > + /* Serial link disabled during config as it needs a valid pixel clock. */ > > + ret = max9271_set_serial_link(dev->serializer, false); > > + if (ret) > > + return ret; > > + > > + /* Set GPO high to hold OV490 in reset during max9271 configuration. */ > > + ret = max9271_set_gpios(dev->serializer, MAX9271_GPO); > > + if (ret) > > + return ret; > > + > > + /* Configure I2C bus at 105Kbps speed and configure GMSL link. */ > > + ret = max9271_configure_i2c(dev->serializer, > > + MAX9271_I2CSLVSH_469NS_234NS | > > + MAX9271_I2CSLVTO_1024US | > > + MAX9271_I2CMSTBT_105KBPS); > > + if (ret) > > + return ret; > > + > > + ret = max9271_configure_gmsl_link(dev->serializer); > > + if (ret) > > + return ret; > > + > > + ret = max9271_set_address(dev->serializer, dev->addrs[0]); > > + if (ret) > > + return ret; > > + dev->serializer->client->addr = dev->addrs[0]; > > + > > + /* > > + * Release OV490 from reset and program address translation > > + * before performing OV490 configuration. > > + */ > > + ret = max9271_clear_gpios(dev->serializer, MAX9271_GPO); > > + if (ret) > > + return ret; > > + > > + ret = max9271_set_translation(dev->serializer, dev->addrs[1], > > + OV490_I2C_ADDRESS); > > + if (ret) > > + return ret; > > + dev->isp->addr = dev->addrs[1]; > > + > > + ret = ov490_initialize(dev); > > + if (ret) > > + return ret; > > + > > + /* > > + * Set reverse channel high threshold to increase noise immunity. > > + * > > + * This should be compensated by increasing the reverse channel > > + * amplitude on the remote deserializer side. > > + */ > > + ret = max9271_set_high_threshold(dev->serializer, true); > > + if (ret) > > + return ret; > > + > > + return 0; > > +} > > + > > +static int rdacm21_probe(struct i2c_client *client) > > +{ > > + struct rdacm21_device *dev; > > + struct fwnode_handle *ep; > > + int ret; > > + > > + dev = devm_kzalloc(&client->dev, sizeof(*dev), GFP_KERNEL); > > + if (!dev) > > + return -ENOMEM; > > + dev->dev = &client->dev; > > + > > + dev->serializer = devm_kzalloc(&client->dev, sizeof(*dev->serializer), > > + GFP_KERNEL); > > + if (!dev->serializer) > > + return -ENOMEM; > > + > > + dev->serializer->client = client; > > + > > + ret = of_property_read_u32_array(client->dev.of_node, "reg", > > + dev->addrs, 2); > > + if (ret < 0) { > > + dev_err(dev->dev, "Invalid DT reg property: %d\n", ret); > > + return -EINVAL; > > + } > > + > > + /* Create the dummy I2C client for the sensor. */ > > + dev->isp = i2c_new_dummy_device(client->adapter, OV490_I2C_ADDRESS); > > + if (IS_ERR(dev->isp)) > > + return PTR_ERR(dev->isp); > > + > > + ret = rdacm21_initialize(dev); > > + if (ret < 0) > > + goto error; > > + > > + /* Initialize and register the subdevice. */ > > + v4l2_i2c_subdev_init(&dev->sd, client, &rdacm21_subdev_ops); > > + dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > > + > > + v4l2_ctrl_handler_init(&dev->ctrls, 1); > > + v4l2_ctrl_new_std(&dev->ctrls, NULL, V4L2_CID_PIXEL_RATE, > > + OV10640_PIXEL_RATE, OV10640_PIXEL_RATE, 1, > > + OV10640_PIXEL_RATE); > > + dev->sd.ctrl_handler = &dev->ctrls; > > + > > + ret = dev->ctrls.error; > > + if (ret) > > + goto error_free_ctrls; > > + > > + dev->pad.flags = MEDIA_PAD_FL_SOURCE; > > + dev->sd.entity.flags |= MEDIA_ENT_F_CAM_SENSOR; > > + ret = media_entity_pads_init(&dev->sd.entity, 1, &dev->pad); > > + if (ret < 0) > > + goto error_free_ctrls; > > + > > + ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL); > > + if (!ep) { > > + dev_err(&client->dev, > > + "Unable to get endpoint in node %pOF\n", > > + client->dev.of_node); > > + ret = -ENOENT; > > + goto error_free_ctrls; > > + } > > + dev->sd.fwnode = ep; > > + > > + ret = v4l2_async_register_subdev(&dev->sd); > > + if (ret) > > + goto error_put_node; > > + > > + return 0; > > + > > +error_put_node: > > + fwnode_handle_put(dev->sd.fwnode); > > +error_free_ctrls: > > + v4l2_ctrl_handler_free(&dev->ctrls); > > +error: > > + i2c_unregister_device(dev->isp); > > + > > + return ret; > > +} > > + > > +static int rdacm21_remove(struct i2c_client *client) > > +{ > > + struct rdacm21_device *dev = i2c_to_rdacm21(client); > > + > > + fwnode_handle_put(dev->sd.fwnode); > > + v4l2_async_unregister_subdev(&dev->sd); > > + v4l2_ctrl_handler_free(&dev->ctrls); > > + i2c_unregister_device(dev->isp); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id rdacm21_of_ids[] = { > > + { .compatible = "imi,rdacm21" }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(of, rdacm21_of_ids); > > + > > +static struct i2c_driver rdacm21_i2c_driver = { > > + .driver = { > > + .name = "rdacm21", > > + .of_match_table = rdacm21_of_ids, > > + }, > > + .probe_new = rdacm21_probe, > > + .remove = rdacm21_remove, > > +}; > > + > > +module_i2c_driver(rdacm21_i2c_driver); > > + > > +MODULE_DESCRIPTION("GMSL Camera driver for RDACM21"); > > +MODULE_AUTHOR("Jacopo Mondi, Kieran Bingham, Laurent Pinchart, Niklas Söderlund, Vladimir Barinov"); > > +MODULE_LICENSE("GPL v2"); > > >
On 17/11/2020 13:36, Jacopo Mondi wrote: > Hi Kieran, > > On Mon, Nov 16, 2020 at 02:47:49PM +0000, Kieran Bingham wrote: >> On 16/11/2020 13:52, Jacopo Mondi wrote: >>> The RDACM21 is a GMSL camera supporting 1280x1080 resolution images >>> developed by IMI based on an Omnivision OV10640 sensor, an Omnivision >>> OV490 ISP and a Maxim MAX9271 GMSL serializer. >>> >>> The driver uses the max9271 library module, to maximize code reuse with >>> other camera module drivers using the same serializer, such as rdacm20. >>> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>> --- >>> MAINTAINERS | 12 + >>> drivers/media/i2c/Kconfig | 13 + >>> drivers/media/i2c/Makefile | 2 + >>> drivers/media/i2c/rdacm21.c | 595 ++++++++++++++++++++++++++++++++++++ >>> 4 files changed, 622 insertions(+) >>> create mode 100644 drivers/media/i2c/rdacm21.c >>> >>> diff --git a/MAINTAINERS b/MAINTAINERS >>> index 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..5f9267e26258 >>> --- /dev/null >>> +++ b/drivers/media/i2c/rdacm21.c >>> @@ -0,0 +1,595 @@ >>> +// SPDX-License-Identifier: GPL-2.0+ >>> +/* >>> + * IMI RDACM21 GMSL Camera Driver >>> + * >>> + * Copyright (C) 2017-2020 Jacopo Mondi >>> + * Copyright (C) 2017-2019 Kieran Bingham >>> + * Copyright (C) 2017-2019 Laurent Pinchart >>> + * Copyright (C) 2017-2019 Niklas Söderlund >>> + * Copyright (C) 2016 Renesas Electronics Corporation >>> + * Copyright (C) 2015 Cogent Embedded, Inc. >>> + */ >>> + >>> +#include <linux/delay.h> >>> +#include <linux/fwnode.h> >>> +#include <linux/init.h> >>> +#include <linux/i2c.h> >>> +#include <linux/module.h> >>> +#include <linux/slab.h> >>> +#include <linux/videodev2.h> >>> + >>> +#include <media/v4l2-async.h> >>> +#include <media/v4l2-ctrls.h> >>> +#include <media/v4l2-subdev.h> >>> +#include "max9271.h" >>> + >>> +#define OV10640_ID_LOW 0xa6 Should we keep the two OV10640 defines together? >>> + >>> +#define OV490_I2C_ADDRESS 0x24 >>> + >>> +#define OV490_PAGE_HIGH_REG 0xfffd >>> +#define OV490_PAGE_LOW_REG 0xfffe >>> + >>> +#define OV490_DVP_CTRL3 0x80286009 >>> + >>> +#define OV490_ODS_CTRL_FRAME_OUTPUT_EN 0x0c >>> +#define OV490_ODS_CTRL 0x8029d000 >>> + >>> +#define OV490_ID_VAL 0x0490 >>> +#define OV490_ID(_p, _v) ((((_p) & 0xff) << 8) | ((_v) & 0xff)) >>> +#define OV490_PID 0x8080300a >>> +#define OV490_VER 0x8080300b >>> + >>> +#define OV490_ISP_HSIZE_LOW 0x80820060 >>> +#define OV490_ISP_HSIZE_HIGH 0x80820061 >>> +#define OV490_ISP_VSIZE_LOW 0x80820062 >>> +#define OV490_ISP_VSIZE_HIGH 0x80820063 >>> + >>> +#define OV10640_PIXEL_RATE (55000000) >>> + >>> +struct rdacm21_device { >>> + struct device *dev; >>> + struct max9271_device *serializer; >>> + struct i2c_client *isp; >>> + struct v4l2_subdev sd; >>> + struct media_pad pad; >>> + struct v4l2_mbus_framefmt fmt; >>> + struct v4l2_ctrl_handler ctrls; >>> + u32 addrs[32]; >>> + u16 last_page; >>> +}; >>> + >>> +static inline struct rdacm21_device *sd_to_rdacm21(struct v4l2_subdev *sd) >>> +{ >>> + return container_of(sd, struct rdacm21_device, sd); >>> +} >>> + >>> +static inline struct rdacm21_device *i2c_to_rdacm21(struct i2c_client *client) >>> +{ >>> + return sd_to_rdacm21(i2c_get_clientdata(client)); >>> +} >>> + >>> +static const struct ov490_reg { >>> + u16 reg; >>> + u8 val; >>> +} ov490_regs_wizard[] = { >>> + {0xfffd, 0x80}, >>> + {0xfffe, 0x82}, >>> + {0x0071, 0x11}, >>> + {0x0075, 0x11}, >>> + {0xfffe, 0x29}, >>> + {0x6010, 0x01}, >>> + /* >>> + * OV490 EMB line disable in YUV and RAW data, >>> + * NOTE: EMB line is still used in ISP and sensor >>> + */ >>> + {0xe000, 0x14}, >>> + {0xfffe, 0x28}, >>> + {0x6000, 0x04}, >>> + {0x6004, 0x00}, >>> + /* >>> + * PCLK polarity - useless due to silicon bug. >>> + * Use 0x808000bb register instead. >>> + */ >>> + {0x6008, 0x00}, >>> + {0xfffe, 0x80}, >>> + {0x0091, 0x00}, >>> + /* bit[3]=0 - PCLK polarity workaround. */ >>> + {0x00bb, 0x1d}, >>> + /* Ov490 FSIN: app_fsin_from_fsync */ >>> + {0xfffe, 0x85}, >>> + {0x0008, 0x00}, >>> + {0x0009, 0x01}, >>> + /* FSIN0 source. */ >>> + {0x000A, 0x05}, >>> + {0x000B, 0x00}, >>> + /* FSIN0 delay. */ >>> + {0x0030, 0x02}, >>> + {0x0031, 0x00}, >>> + {0x0032, 0x00}, >>> + {0x0033, 0x00}, >>> + /* FSIN1 delay. */ >>> + {0x0038, 0x02}, >>> + {0x0039, 0x00}, >>> + {0x003A, 0x00}, >>> + {0x003B, 0x00}, >>> + /* FSIN0 length. */ >>> + {0x0070, 0x2C}, >>> + {0x0071, 0x01}, >>> + {0x0072, 0x00}, >>> + {0x0073, 0x00}, >>> + /* FSIN1 length. */ >>> + {0x0074, 0x64}, >>> + {0x0075, 0x00}, >>> + {0x0076, 0x00}, >>> + {0x0077, 0x00}, >>> + {0x0000, 0x14}, >>> + {0x0001, 0x00}, >>> + {0x0002, 0x00}, >>> + {0x0003, 0x00}, >>> + /* >>> + * Load fsin0,load fsin1,load other, >>> + * It will be cleared automatically. >>> + */ >>> + {0x0004, 0x32}, >>> + {0x0005, 0x00}, >>> + {0x0006, 0x00}, >>> + {0x0007, 0x00}, >>> + {0xfffe, 0x80}, >>> + /* Sensor FSIN. */ >>> + {0x0081, 0x00}, >>> + /* ov10640 FSIN enable */ >>> + {0xfffe, 0x19}, >>> + {0x5000, 0x00}, >>> + {0x5001, 0x30}, >>> + {0x5002, 0x8c}, >>> + {0x5003, 0xb2}, >>> + {0xfffe, 0x80}, >>> + {0x00c0, 0xc1}, >>> + /* ov10640 HFLIP=1 by default */ >>> + {0xfffe, 0x19}, >>> + {0x5000, 0x01}, >>> + {0x5001, 0x00}, >>> + {0xfffe, 0x80}, >>> + {0x00c0, 0xdc}, >>> +}; >>> + >>> +static int ov490_read(struct rdacm21_device *dev, u16 reg, u8 *val) >>> +{ >>> + u8 buf[2] = { reg >> 8, reg }; >>> + int ret; >>> + >>> + ret = i2c_master_send(dev->isp, buf, 2); >>> + if (ret == 2) >>> + ret = i2c_master_recv(dev->isp, val, 1); >>> + >>> + if (ret < 0) { >>> + dev_dbg(dev->dev, "%s: register 0x%04x read failed (%d)\n", >>> + __func__, reg, ret); >>> + return ret; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static int ov490_write(struct rdacm21_device *dev, u16 reg, u8 val) >>> +{ >>> + u8 buf[3] = { reg >> 8, reg, val }; >>> + int ret; >>> + >>> + ret = i2c_master_send(dev->isp, buf, 3); >>> + if (ret < 0) { >>> + dev_err(dev->dev, "%s: register 0x%04x write failed (%d)\n", >>> + __func__, reg, ret); >>> + return ret; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static int ov490_set_page(struct rdacm21_device *dev, u16 reg) >>> +{ >>> + bool page_new = false; >>> + u8 page_high = reg >> 8; >>> + u8 page_low = reg; >>> + int ret; >>> + >>> + if (page_high != (dev->last_page >> 8)) { >>> + ret = ov490_write(dev, OV490_PAGE_HIGH_REG, page_high); >>> + if (ret) >>> + return ret; >>> + page_new = true; >>> + } >>> + >>> + if (page_low != (u8)dev->last_page) { >>> + ret = ov490_write(dev, OV490_PAGE_LOW_REG, page_low); >>> + if (ret) >>> + return ret; >>> + page_new = true; >>> + } >>> + >>> + if (page_new) { >>> + dev->last_page = reg; >>> + usleep_range(100, 150); >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static int ov490_read_reg(struct rdacm21_device *dev, u32 reg, u8 *val) >>> +{ >>> + int ret; >>> + >>> + ret = ov490_set_page(dev, reg >> 16); >>> + if (ret) >>> + return ret; >>> + >>> + ret = ov490_read(dev, (u16)reg, val); >>> + if (ret) >>> + return ret; >>> + >>> + dev_dbg(dev->dev, "%s: 0x%08x = 0x%02x\n", __func__, reg, *val); >>> + >>> + return 0; >>> +} >>> + >>> +static int ov490_write_reg(struct rdacm21_device *dev, u32 reg, u8 val) >>> +{ >>> + int ret; >>> + >>> + ret = ov490_set_page(dev, reg >> 16); >>> + if (ret) >>> + return ret; >>> + >>> + ret = ov490_write(dev, (u16)reg, val); >>> + if (ret) >>> + return ret; >>> + >>> + dev_dbg(dev->dev, "%s: 0x%08x = 0x%02x\n", __func__, reg, val); >>> + >>> + return 0; >>> +} >>> + >>> +static int rdacm21_s_stream(struct v4l2_subdev *sd, int enable) >>> +{ >>> + struct rdacm21_device *dev = sd_to_rdacm21(sd); >>> + >>> + /* >>> + * Enable serial link now that the ISP provides a valid pixel clock >>> + * to start serializing video data on the GMSL link. >>> + */ >>> + return max9271_set_serial_link(dev->serializer, enable); >>> +} >>> + >>> +static int rdacm21_enum_mbus_code(struct v4l2_subdev *sd, >>> + struct v4l2_subdev_pad_config *cfg, >>> + struct v4l2_subdev_mbus_code_enum *code) >>> +{ >>> + if (code->pad || code->index > 0) >>> + return -EINVAL; >>> + >>> + code->code = MEDIA_BUS_FMT_YUYV8_1X16; >>> + >>> + return 0; >>> +} >>> + >>> +static int rdacm21_get_fmt(struct v4l2_subdev *sd, >>> + struct v4l2_subdev_pad_config *cfg, >>> + struct v4l2_subdev_format *format) >>> +{ >>> + struct v4l2_mbus_framefmt *mf = &format->format; >>> + struct rdacm21_device *dev = sd_to_rdacm21(sd); >>> + >>> + if (format->pad) >>> + return -EINVAL; >>> + >>> + mf->width = dev->fmt.width; >>> + mf->height = dev->fmt.height; >>> + mf->code = MEDIA_BUS_FMT_YUYV8_1X16; >>> + mf->colorspace = V4L2_COLORSPACE_SRGB; >>> + mf->field = V4L2_FIELD_NONE; >>> + mf->ycbcr_enc = V4L2_YCBCR_ENC_601; >>> + mf->quantization = V4L2_QUANTIZATION_FULL_RANGE; >>> + mf->xfer_func = V4L2_XFER_FUNC_NONE; >>> + >>> + return 0; >>> +} >>> + >>> +static struct v4l2_subdev_video_ops rdacm21_video_ops = { >>> + .s_stream = rdacm21_s_stream, >>> +}; >>> + >>> +static const struct v4l2_subdev_pad_ops rdacm21_subdev_pad_ops = { >>> + .enum_mbus_code = rdacm21_enum_mbus_code, >>> + .get_fmt = rdacm21_get_fmt, >>> + .set_fmt = rdacm21_get_fmt, >>> +}; >>> + >>> +static struct v4l2_subdev_ops rdacm21_subdev_ops = { >>> + .video = &rdacm21_video_ops, >>> + .pad = &rdacm21_subdev_pad_ops, >>> +}; >>> + >>> +static int ov490_initialize(struct rdacm21_device *dev) >>> +{ >>> + unsigned int ov490_pid_retry = 20; >>> + unsigned int timeout; >>> + u8 pid, ver, val; >>> + unsigned int i; >>> + int ret; >>> + >>> + /* Read OV490 Id to test communications. */ >>> +pid_retry: >>> + ret = ov490_read_reg(dev, OV490_PID, &pid); >>> + if (ret < 0) { >>> + /* Give OV490 a few more cycles to exit from reset. */ >>> + if (ov490_pid_retry--) { >>> + usleep_range(1000, 2000); >>> + goto pid_retry; >>> + } >>> + >>> + dev_err(dev->dev, "OV490 PID read failed (%d)\n", ret); >>> + return ret; >>> + } >>> + >>> + ret = ov490_read_reg(dev, OV490_VER, &ver); >>> + if (ret < 0) { >>> + dev_err(dev->dev, "OV490 VERSION read failed (%d)\n", ret); >>> + return ret; >>> + } >>> + >>> + if (OV490_ID(pid, ver) != OV490_ID_VAL) { >>> + dev_err(dev->dev, "OV490 ID mismatch: (0x%04x)\n", >>> + OV490_ID(pid, ver)); >>> + return -ENODEV; >>> + } >>> + >>> + /* Wait for firmware boot by reading streamon status. */ >>> + for (timeout = 300; timeout > 0; timeout--) { >>> + ov490_read_reg(dev, OV490_ODS_CTRL, &val); >>> + if (val == OV490_ODS_CTRL_FRAME_OUTPUT_EN) >>> + break; >>> + mdelay(1); >>> + } >>> + if (!timeout) { >>> + dev_err(dev->dev, "Timeout firmware boot wait\n"); >>> + return -ENODEV; >>> + } >>> + dev_dbg(dev->dev, "Firmware booted in %u msec\n", 300 - timeout); >>> + >>> + /* Read OV10640 Id to test communications. */ >>> + ov490_write(dev, 0xfffd, 0x80); >>> + ov490_write(dev, 0xfffe, 0x19); >>> + usleep_range(100, 150); >> >> Is that setting a page? Or doing something else? >> >>> + >>> + 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); >> And here? >> >> Can this not go through the new helpers? >> >> If the 'read_reg', 'write_reg', helpers are not appropriate, at least >> setting the pages could be done with set_page() ... but I'd be surprised >> if those couldn't be handled with ov490_write_reg() ? > > As I've said in previous reply, I prefer the comunication with ov10640 > to be kept explicit as long as we don't have more users and I don't > better understand what register 0x5000 is for. > Ok, I understand, but we 'know' it's the register page for communicating with the Sensor though? Do we have any documentation on this at all? > I can go through set_page() but the I have to pass 0x8019 which kind > of defeat the purpose of defining registers by name > > I prefer to keep this as it is. The issue I would have is you now have an invalid lastPage setting in dev. So you're potentially letting yourself access registers on an incorrect page, next time you use ov490_write_reg(). You could quite easily define: /* * The OV10640 is interacted with through * registers in page 0x8019 using registers from 0x5000 to 0x5003, * However the definition of these is currently unknown. */ #define SENSOR_REGISTER_0 0x80195000 #define SENSOR_REGISTER_1 0x80195001 #define SENSOR_REGISTER_2 0x80195002 To me that would be better than leaving some raw code inline or maybe a short wrapper to make it even clearer that this talks to /another component/: #define SENSOR_PAGE 0x8019 sensor_write(dev, reg, val) { reg |= (SENSOR_PAGE << 16); return ov490_write_reg(dev, reg, val); } .. sensor_write(dev, 0x5000, 0x01); sensor_write(dev, 0x5001, 0x30); sensor_write(dev, 0x5002, 0x0a); ... >>> + 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); >>> + Even if you want to keep the register writes raw, we know this code block is handling the OV10640, so I'd be at least tempted to move it all to a int ov10640_initialise(dev); but it's your code, and you seem quite against this, so I won't push anymore. >>> + 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); >>> + } You should almost certainly mark dev->lastPage as something invalid here anyway (0xFFFF?). Otherwise the hardware page will be set to whatever was last used in the ov490_regs_wizard table, and the ov490_read_reg() won't necessarily update if it was on the same page last usage. The incorrect lastPage is potentially a more critical issue though so with that (if confirmed): Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >>> + >>> + /* >>> + * The ISP is programmed with the content of a serial flash memory. >>> + * Read the firmware configuration to reflect it through the V4L2 APIs. >>> + */ >>> + ov490_read_reg(dev, OV490_ISP_HSIZE_HIGH, &val); >>> + dev->fmt.width = (val & 0xf) << 8; >>> + ov490_read_reg(dev, OV490_ISP_HSIZE_LOW, &val); >>> + dev->fmt.width |= (val & 0xff); >>> + >>> + ov490_read_reg(dev, OV490_ISP_VSIZE_HIGH, &val); >>> + dev->fmt.height = (val & 0xf) << 8; >>> + ov490_read_reg(dev, OV490_ISP_VSIZE_LOW, &val); >>> + dev->fmt.height |= val & 0xff; >>> + >> >> >> Hrm, these look like "ov490_read_reg_long()", but given they're the only >> occurrences of a "long read" I think we could leave it as is. >> >> >>> + /* Set bus width to 12 bits with [0:11] ordering. */ >>> + ov490_write_reg(dev, OV490_DVP_CTRL3, 0x10); >>> + >>> + dev_info(dev->dev, "Identified RDACM21 camera module\n"); >>> + >>> + return 0; >>> +} >>> + >>> +static int rdacm21_initialize(struct rdacm21_device *dev) >>> +{ >>> + int ret; >>> + >>> + /* Verify communication with the MAX9271: ping to wakeup. */ >>> + dev->serializer->client->addr = MAX9271_DEFAULT_ADDR; >>> + i2c_smbus_read_byte(dev->serializer->client); >>> + >>> + /* Serial link disabled during config as it needs a valid pixel clock. */ >>> + ret = max9271_set_serial_link(dev->serializer, false); >>> + if (ret) >>> + return ret; >>> + >>> + /* Set GPO high to hold OV490 in reset during max9271 configuration. */ >>> + ret = max9271_set_gpios(dev->serializer, MAX9271_GPO); >>> + if (ret) >>> + return ret; >>> + >>> + /* Configure I2C bus at 105Kbps speed and configure GMSL link. */ >>> + ret = max9271_configure_i2c(dev->serializer, >>> + MAX9271_I2CSLVSH_469NS_234NS | >>> + MAX9271_I2CSLVTO_1024US | >>> + MAX9271_I2CMSTBT_105KBPS); >>> + if (ret) >>> + return ret; >>> + >>> + ret = max9271_configure_gmsl_link(dev->serializer); >>> + if (ret) >>> + return ret; >>> + >>> + ret = max9271_set_address(dev->serializer, dev->addrs[0]); >>> + if (ret) >>> + return ret; >>> + dev->serializer->client->addr = dev->addrs[0]; >>> + >>> + /* >>> + * Release OV490 from reset and program address translation >>> + * before performing OV490 configuration. >>> + */ >>> + ret = max9271_clear_gpios(dev->serializer, MAX9271_GPO); >>> + if (ret) >>> + return ret; >>> + >>> + ret = max9271_set_translation(dev->serializer, dev->addrs[1], >>> + OV490_I2C_ADDRESS); >>> + if (ret) >>> + return ret; >>> + dev->isp->addr = dev->addrs[1]; >>> + >>> + ret = ov490_initialize(dev); >>> + if (ret) >>> + return ret; >>> + >>> + /* >>> + * Set reverse channel high threshold to increase noise immunity. >>> + * >>> + * This should be compensated by increasing the reverse channel >>> + * amplitude on the remote deserializer side. >>> + */ >>> + ret = max9271_set_high_threshold(dev->serializer, true); >>> + if (ret) >>> + return ret; >>> + >>> + return 0; >>> +} >>> + >>> +static int rdacm21_probe(struct i2c_client *client) >>> +{ >>> + struct rdacm21_device *dev; >>> + struct fwnode_handle *ep; >>> + int ret; >>> + >>> + dev = devm_kzalloc(&client->dev, sizeof(*dev), GFP_KERNEL); >>> + if (!dev) >>> + return -ENOMEM; >>> + dev->dev = &client->dev; >>> + >>> + dev->serializer = devm_kzalloc(&client->dev, sizeof(*dev->serializer), >>> + GFP_KERNEL); >>> + if (!dev->serializer) >>> + return -ENOMEM; >>> + >>> + dev->serializer->client = client; >>> + >>> + ret = of_property_read_u32_array(client->dev.of_node, "reg", >>> + dev->addrs, 2); >>> + if (ret < 0) { >>> + dev_err(dev->dev, "Invalid DT reg property: %d\n", ret); >>> + return -EINVAL; >>> + } >>> + >>> + /* Create the dummy I2C client for the sensor. */ >>> + dev->isp = i2c_new_dummy_device(client->adapter, OV490_I2C_ADDRESS); >>> + if (IS_ERR(dev->isp)) >>> + return PTR_ERR(dev->isp); >>> + >>> + ret = rdacm21_initialize(dev); >>> + if (ret < 0) >>> + goto error; >>> + >>> + /* Initialize and register the subdevice. */ >>> + v4l2_i2c_subdev_init(&dev->sd, client, &rdacm21_subdev_ops); >>> + dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; >>> + >>> + v4l2_ctrl_handler_init(&dev->ctrls, 1); >>> + v4l2_ctrl_new_std(&dev->ctrls, NULL, V4L2_CID_PIXEL_RATE, >>> + OV10640_PIXEL_RATE, OV10640_PIXEL_RATE, 1, >>> + OV10640_PIXEL_RATE); >>> + dev->sd.ctrl_handler = &dev->ctrls; >>> + >>> + ret = dev->ctrls.error; >>> + if (ret) >>> + goto error_free_ctrls; >>> + >>> + dev->pad.flags = MEDIA_PAD_FL_SOURCE; >>> + dev->sd.entity.flags |= MEDIA_ENT_F_CAM_SENSOR; >>> + ret = media_entity_pads_init(&dev->sd.entity, 1, &dev->pad); >>> + if (ret < 0) >>> + goto error_free_ctrls; >>> + >>> + ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL); >>> + if (!ep) { >>> + dev_err(&client->dev, >>> + "Unable to get endpoint in node %pOF\n", >>> + client->dev.of_node); >>> + ret = -ENOENT; >>> + goto error_free_ctrls; >>> + } >>> + dev->sd.fwnode = ep; >>> + >>> + ret = v4l2_async_register_subdev(&dev->sd); >>> + if (ret) >>> + goto error_put_node; >>> + >>> + return 0; >>> + >>> +error_put_node: >>> + fwnode_handle_put(dev->sd.fwnode); >>> +error_free_ctrls: >>> + v4l2_ctrl_handler_free(&dev->ctrls); >>> +error: >>> + i2c_unregister_device(dev->isp); >>> + >>> + return ret; >>> +} >>> + >>> +static int rdacm21_remove(struct i2c_client *client) >>> +{ >>> + struct rdacm21_device *dev = i2c_to_rdacm21(client); >>> + >>> + fwnode_handle_put(dev->sd.fwnode); >>> + v4l2_async_unregister_subdev(&dev->sd); >>> + v4l2_ctrl_handler_free(&dev->ctrls); >>> + i2c_unregister_device(dev->isp); >>> + >>> + return 0; >>> +} >>> + >>> +static const struct of_device_id rdacm21_of_ids[] = { >>> + { .compatible = "imi,rdacm21" }, >>> + { } >>> +}; >>> +MODULE_DEVICE_TABLE(of, rdacm21_of_ids); >>> + >>> +static struct i2c_driver rdacm21_i2c_driver = { >>> + .driver = { >>> + .name = "rdacm21", >>> + .of_match_table = rdacm21_of_ids, >>> + }, >>> + .probe_new = rdacm21_probe, >>> + .remove = rdacm21_remove, >>> +}; >>> + >>> +module_i2c_driver(rdacm21_i2c_driver); >>> + >>> +MODULE_DESCRIPTION("GMSL Camera driver for RDACM21"); >>> +MODULE_AUTHOR("Jacopo Mondi, Kieran Bingham, Laurent Pinchart, Niklas Söderlund, Vladimir Barinov"); >>> +MODULE_LICENSE("GPL v2"); >>> >>
Hi Jacopo, On 16/11/2020 13:52, Jacopo Mondi wrote: > Hello, > this v5 mostly changes how the rdacm21 driver handles writes to the > OV490 chip by handling the 'high' part of the sensor register through an > helper function as suggested by Kieran and Laurent. > > The diff is available at: > https://paste.debian.net/1172700/ > > Minor fixes in patches subjects and in bindings as reported by Sergei and > Kieran. > > Bindings have now been sent to DT people now that they've been reviewed > by Kieran. > > v4: https://patchwork.linuxtv.org/project/linux-media/list/?series=3847 > v3: https://patchwork.linuxtv.org/project/linux-media/list/?series=3657 > > Background in v1 cover letter: > https://www.spinics.net/lists/linux-renesas-soc/msg52886.html > > 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 patches are > meant for ease your testing. Can you give this series a spin ? After some rabbit-holes :-D: A 5-camera capture on Salvator-X - https://pasteboard.co/JAW0PSr.png For this series, on both Eagle-V3M and Salvator-X Tested-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > Series based on latest renesas-drivers tag: renesas-drivers-2020-11-10-v5.10-rc3 > > 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 | 595 ++++++++++++++++++ > 8 files changed, 1087 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 >
Hi Kieran On Wed, Nov 18, 2020 at 01:34:54PM +0000, Kieran Bingham wrote: > Hi Jacopo, > > On 16/11/2020 13:52, Jacopo Mondi wrote: > > Hello, > > this v5 mostly changes how the rdacm21 driver handles writes to the > > OV490 chip by handling the 'high' part of the sensor register through an > > helper function as suggested by Kieran and Laurent. > > > > The diff is available at: > > https://paste.debian.net/1172700/ > > > > Minor fixes in patches subjects and in bindings as reported by Sergei and > > Kieran. > > > > Bindings have now been sent to DT people now that they've been reviewed > > by Kieran. > > > > v4: https://patchwork.linuxtv.org/project/linux-media/list/?series=3847 > > v3: https://patchwork.linuxtv.org/project/linux-media/list/?series=3657 > > > > Background in v1 cover letter: > > https://www.spinics.net/lists/linux-renesas-soc/msg52886.html > > > > 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 patches are > > meant for ease your testing. Can you give this series a spin ? > > > After some rabbit-holes :-D: > > A 5-camera capture on Salvator-X - https://pasteboard.co/JAW0PSr.png > For this series, on both Eagle-V3M and Salvator-X > > Tested-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> Thanks! > > > > > Series based on latest renesas-drivers tag: renesas-drivers-2020-11-10-v5.10-rc3 > > > > 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 | 595 ++++++++++++++++++ > > 8 files changed, 1087 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 > > >
Hi Kieran, On Tue, Nov 17, 2020 at 03:31:37PM +0000, Kieran Bingham wrote: > On 17/11/2020 13:36, Jacopo Mondi wrote: > > Hi Kieran, > > > > On Mon, Nov 16, 2020 at 02:47:49PM +0000, Kieran Bingham wrote: > >> On 16/11/2020 13:52, Jacopo Mondi wrote: > >>> The RDACM21 is a GMSL camera supporting 1280x1080 resolution images > >>> developed by IMI based on an Omnivision OV10640 sensor, an Omnivision > >>> OV490 ISP and a Maxim MAX9271 GMSL serializer. > >>> > >>> The driver uses the max9271 library module, to maximize code reuse with > >>> other camera module drivers using the same serializer, such as rdacm20. > >>> > >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > >>> --- > >>> MAINTAINERS | 12 + > >>> drivers/media/i2c/Kconfig | 13 + > >>> drivers/media/i2c/Makefile | 2 + > >>> drivers/media/i2c/rdacm21.c | 595 ++++++++++++++++++++++++++++++++++++ > >>> 4 files changed, 622 insertions(+) > >>> create mode 100644 drivers/media/i2c/rdacm21.c > >>> > >>> diff --git a/MAINTAINERS b/MAINTAINERS > >>> index 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..5f9267e26258 > >>> --- /dev/null > >>> +++ b/drivers/media/i2c/rdacm21.c > >>> @@ -0,0 +1,595 @@ > >>> +// SPDX-License-Identifier: GPL-2.0+ > >>> +/* > >>> + * IMI RDACM21 GMSL Camera Driver > >>> + * > >>> + * Copyright (C) 2017-2020 Jacopo Mondi > >>> + * Copyright (C) 2017-2019 Kieran Bingham > >>> + * Copyright (C) 2017-2019 Laurent Pinchart > >>> + * Copyright (C) 2017-2019 Niklas Söderlund > >>> + * Copyright (C) 2016 Renesas Electronics Corporation > >>> + * Copyright (C) 2015 Cogent Embedded, Inc. > >>> + */ > >>> + > >>> +#include <linux/delay.h> > >>> +#include <linux/fwnode.h> > >>> +#include <linux/init.h> > >>> +#include <linux/i2c.h> > >>> +#include <linux/module.h> > >>> +#include <linux/slab.h> > >>> +#include <linux/videodev2.h> > >>> + > >>> +#include <media/v4l2-async.h> > >>> +#include <media/v4l2-ctrls.h> > >>> +#include <media/v4l2-subdev.h> > >>> +#include "max9271.h" > >>> + > >>> +#define OV10640_ID_LOW 0xa6 > > Should we keep the two OV10640 defines together? > > >>> + > >>> +#define OV490_I2C_ADDRESS 0x24 > >>> + > >>> +#define OV490_PAGE_HIGH_REG 0xfffd > >>> +#define OV490_PAGE_LOW_REG 0xfffe > >>> + > >>> +#define OV490_DVP_CTRL3 0x80286009 > >>> + > >>> +#define OV490_ODS_CTRL_FRAME_OUTPUT_EN 0x0c > >>> +#define OV490_ODS_CTRL 0x8029d000 > >>> + > >>> +#define OV490_ID_VAL 0x0490 > >>> +#define OV490_ID(_p, _v) ((((_p) & 0xff) << 8) | ((_v) & 0xff)) > >>> +#define OV490_PID 0x8080300a > >>> +#define OV490_VER 0x8080300b > >>> + > >>> +#define OV490_ISP_HSIZE_LOW 0x80820060 > >>> +#define OV490_ISP_HSIZE_HIGH 0x80820061 > >>> +#define OV490_ISP_VSIZE_LOW 0x80820062 > >>> +#define OV490_ISP_VSIZE_HIGH 0x80820063 > >>> + > >>> +#define OV10640_PIXEL_RATE (55000000) > >>> + > >>> +struct rdacm21_device { > >>> + struct device *dev; > >>> + struct max9271_device *serializer; > >>> + struct i2c_client *isp; > >>> + struct v4l2_subdev sd; > >>> + struct media_pad pad; > >>> + struct v4l2_mbus_framefmt fmt; > >>> + struct v4l2_ctrl_handler ctrls; > >>> + u32 addrs[32]; > >>> + u16 last_page; > >>> +}; > >>> + > >>> +static inline struct rdacm21_device *sd_to_rdacm21(struct v4l2_subdev *sd) > >>> +{ > >>> + return container_of(sd, struct rdacm21_device, sd); > >>> +} > >>> + > >>> +static inline struct rdacm21_device *i2c_to_rdacm21(struct i2c_client *client) > >>> +{ > >>> + return sd_to_rdacm21(i2c_get_clientdata(client)); > >>> +} > >>> + > >>> +static const struct ov490_reg { > >>> + u16 reg; > >>> + u8 val; > >>> +} ov490_regs_wizard[] = { > >>> + {0xfffd, 0x80}, > >>> + {0xfffe, 0x82}, > >>> + {0x0071, 0x11}, > >>> + {0x0075, 0x11}, > >>> + {0xfffe, 0x29}, > >>> + {0x6010, 0x01}, > >>> + /* > >>> + * OV490 EMB line disable in YUV and RAW data, > >>> + * NOTE: EMB line is still used in ISP and sensor > >>> + */ > >>> + {0xe000, 0x14}, > >>> + {0xfffe, 0x28}, > >>> + {0x6000, 0x04}, > >>> + {0x6004, 0x00}, > >>> + /* > >>> + * PCLK polarity - useless due to silicon bug. > >>> + * Use 0x808000bb register instead. > >>> + */ > >>> + {0x6008, 0x00}, > >>> + {0xfffe, 0x80}, > >>> + {0x0091, 0x00}, > >>> + /* bit[3]=0 - PCLK polarity workaround. */ > >>> + {0x00bb, 0x1d}, > >>> + /* Ov490 FSIN: app_fsin_from_fsync */ > >>> + {0xfffe, 0x85}, > >>> + {0x0008, 0x00}, > >>> + {0x0009, 0x01}, > >>> + /* FSIN0 source. */ > >>> + {0x000A, 0x05}, > >>> + {0x000B, 0x00}, > >>> + /* FSIN0 delay. */ > >>> + {0x0030, 0x02}, > >>> + {0x0031, 0x00}, > >>> + {0x0032, 0x00}, > >>> + {0x0033, 0x00}, > >>> + /* FSIN1 delay. */ > >>> + {0x0038, 0x02}, > >>> + {0x0039, 0x00}, > >>> + {0x003A, 0x00}, > >>> + {0x003B, 0x00}, > >>> + /* FSIN0 length. */ > >>> + {0x0070, 0x2C}, > >>> + {0x0071, 0x01}, > >>> + {0x0072, 0x00}, > >>> + {0x0073, 0x00}, > >>> + /* FSIN1 length. */ > >>> + {0x0074, 0x64}, > >>> + {0x0075, 0x00}, > >>> + {0x0076, 0x00}, > >>> + {0x0077, 0x00}, > >>> + {0x0000, 0x14}, > >>> + {0x0001, 0x00}, > >>> + {0x0002, 0x00}, > >>> + {0x0003, 0x00}, > >>> + /* > >>> + * Load fsin0,load fsin1,load other, > >>> + * It will be cleared automatically. > >>> + */ > >>> + {0x0004, 0x32}, > >>> + {0x0005, 0x00}, > >>> + {0x0006, 0x00}, > >>> + {0x0007, 0x00}, > >>> + {0xfffe, 0x80}, > >>> + /* Sensor FSIN. */ > >>> + {0x0081, 0x00}, > >>> + /* ov10640 FSIN enable */ > >>> + {0xfffe, 0x19}, > >>> + {0x5000, 0x00}, > >>> + {0x5001, 0x30}, > >>> + {0x5002, 0x8c}, > >>> + {0x5003, 0xb2}, > >>> + {0xfffe, 0x80}, > >>> + {0x00c0, 0xc1}, > >>> + /* ov10640 HFLIP=1 by default */ > >>> + {0xfffe, 0x19}, > >>> + {0x5000, 0x01}, > >>> + {0x5001, 0x00}, > >>> + {0xfffe, 0x80}, > >>> + {0x00c0, 0xdc}, > >>> +}; > >>> + > >>> +static int ov490_read(struct rdacm21_device *dev, u16 reg, u8 *val) > >>> +{ > >>> + u8 buf[2] = { reg >> 8, reg }; > >>> + int ret; > >>> + > >>> + ret = i2c_master_send(dev->isp, buf, 2); > >>> + if (ret == 2) > >>> + ret = i2c_master_recv(dev->isp, val, 1); > >>> + > >>> + if (ret < 0) { > >>> + dev_dbg(dev->dev, "%s: register 0x%04x read failed (%d)\n", > >>> + __func__, reg, ret); > >>> + return ret; > >>> + } > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int ov490_write(struct rdacm21_device *dev, u16 reg, u8 val) > >>> +{ > >>> + u8 buf[3] = { reg >> 8, reg, val }; > >>> + int ret; > >>> + > >>> + ret = i2c_master_send(dev->isp, buf, 3); > >>> + if (ret < 0) { > >>> + dev_err(dev->dev, "%s: register 0x%04x write failed (%d)\n", > >>> + __func__, reg, ret); > >>> + return ret; > >>> + } > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int ov490_set_page(struct rdacm21_device *dev, u16 reg) > >>> +{ > >>> + bool page_new = false; > >>> + u8 page_high = reg >> 8; > >>> + u8 page_low = reg; > >>> + int ret; > >>> + > >>> + if (page_high != (dev->last_page >> 8)) { > >>> + ret = ov490_write(dev, OV490_PAGE_HIGH_REG, page_high); > >>> + if (ret) > >>> + return ret; > >>> + page_new = true; > >>> + } > >>> + > >>> + if (page_low != (u8)dev->last_page) { > >>> + ret = ov490_write(dev, OV490_PAGE_LOW_REG, page_low); > >>> + if (ret) > >>> + return ret; > >>> + page_new = true; > >>> + } > >>> + > >>> + if (page_new) { > >>> + dev->last_page = reg; > >>> + usleep_range(100, 150); > >>> + } > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int ov490_read_reg(struct rdacm21_device *dev, u32 reg, u8 *val) > >>> +{ > >>> + int ret; > >>> + > >>> + ret = ov490_set_page(dev, reg >> 16); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + ret = ov490_read(dev, (u16)reg, val); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + dev_dbg(dev->dev, "%s: 0x%08x = 0x%02x\n", __func__, reg, *val); > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int ov490_write_reg(struct rdacm21_device *dev, u32 reg, u8 val) > >>> +{ > >>> + int ret; > >>> + > >>> + ret = ov490_set_page(dev, reg >> 16); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + ret = ov490_write(dev, (u16)reg, val); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + dev_dbg(dev->dev, "%s: 0x%08x = 0x%02x\n", __func__, reg, val); > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int rdacm21_s_stream(struct v4l2_subdev *sd, int enable) > >>> +{ > >>> + struct rdacm21_device *dev = sd_to_rdacm21(sd); > >>> + > >>> + /* > >>> + * Enable serial link now that the ISP provides a valid pixel clock > >>> + * to start serializing video data on the GMSL link. > >>> + */ > >>> + return max9271_set_serial_link(dev->serializer, enable); > >>> +} > >>> + > >>> +static int rdacm21_enum_mbus_code(struct v4l2_subdev *sd, > >>> + struct v4l2_subdev_pad_config *cfg, > >>> + struct v4l2_subdev_mbus_code_enum *code) > >>> +{ > >>> + if (code->pad || code->index > 0) > >>> + return -EINVAL; > >>> + > >>> + code->code = MEDIA_BUS_FMT_YUYV8_1X16; > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int rdacm21_get_fmt(struct v4l2_subdev *sd, > >>> + struct v4l2_subdev_pad_config *cfg, > >>> + struct v4l2_subdev_format *format) > >>> +{ > >>> + struct v4l2_mbus_framefmt *mf = &format->format; > >>> + struct rdacm21_device *dev = sd_to_rdacm21(sd); > >>> + > >>> + if (format->pad) > >>> + return -EINVAL; > >>> + > >>> + mf->width = dev->fmt.width; > >>> + mf->height = dev->fmt.height; > >>> + mf->code = MEDIA_BUS_FMT_YUYV8_1X16; > >>> + mf->colorspace = V4L2_COLORSPACE_SRGB; > >>> + mf->field = V4L2_FIELD_NONE; > >>> + mf->ycbcr_enc = V4L2_YCBCR_ENC_601; > >>> + mf->quantization = V4L2_QUANTIZATION_FULL_RANGE; > >>> + mf->xfer_func = V4L2_XFER_FUNC_NONE; > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static struct v4l2_subdev_video_ops rdacm21_video_ops = { > >>> + .s_stream = rdacm21_s_stream, > >>> +}; > >>> + > >>> +static const struct v4l2_subdev_pad_ops rdacm21_subdev_pad_ops = { > >>> + .enum_mbus_code = rdacm21_enum_mbus_code, > >>> + .get_fmt = rdacm21_get_fmt, > >>> + .set_fmt = rdacm21_get_fmt, > >>> +}; > >>> + > >>> +static struct v4l2_subdev_ops rdacm21_subdev_ops = { > >>> + .video = &rdacm21_video_ops, > >>> + .pad = &rdacm21_subdev_pad_ops, > >>> +}; > >>> + > >>> +static int ov490_initialize(struct rdacm21_device *dev) > >>> +{ > >>> + unsigned int ov490_pid_retry = 20; > >>> + unsigned int timeout; > >>> + u8 pid, ver, val; > >>> + unsigned int i; > >>> + int ret; > >>> + > >>> + /* Read OV490 Id to test communications. */ > >>> +pid_retry: > >>> + ret = ov490_read_reg(dev, OV490_PID, &pid); > >>> + if (ret < 0) { > >>> + /* Give OV490 a few more cycles to exit from reset. */ > >>> + if (ov490_pid_retry--) { > >>> + usleep_range(1000, 2000); > >>> + goto pid_retry; > >>> + } > >>> + > >>> + dev_err(dev->dev, "OV490 PID read failed (%d)\n", ret); > >>> + return ret; > >>> + } > >>> + > >>> + ret = ov490_read_reg(dev, OV490_VER, &ver); > >>> + if (ret < 0) { > >>> + dev_err(dev->dev, "OV490 VERSION read failed (%d)\n", ret); > >>> + return ret; > >>> + } > >>> + > >>> + if (OV490_ID(pid, ver) != OV490_ID_VAL) { > >>> + dev_err(dev->dev, "OV490 ID mismatch: (0x%04x)\n", > >>> + OV490_ID(pid, ver)); > >>> + return -ENODEV; > >>> + } > >>> + > >>> + /* Wait for firmware boot by reading streamon status. */ > >>> + for (timeout = 300; timeout > 0; timeout--) { > >>> + ov490_read_reg(dev, OV490_ODS_CTRL, &val); > >>> + if (val == OV490_ODS_CTRL_FRAME_OUTPUT_EN) > >>> + break; > >>> + mdelay(1); > >>> + } > >>> + if (!timeout) { > >>> + dev_err(dev->dev, "Timeout firmware boot wait\n"); > >>> + return -ENODEV; > >>> + } > >>> + dev_dbg(dev->dev, "Firmware booted in %u msec\n", 300 - timeout); > >>> + > >>> + /* Read OV10640 Id to test communications. */ > >>> + ov490_write(dev, 0xfffd, 0x80); > >>> + ov490_write(dev, 0xfffe, 0x19); > >>> + usleep_range(100, 150); > >> > >> Is that setting a page? Or doing something else? > >> > >>> + > >>> + 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); > >> And here? > >> > >> Can this not go through the new helpers? > >> > >> If the 'read_reg', 'write_reg', helpers are not appropriate, at least > >> setting the pages could be done with set_page() ... but I'd be surprised > >> if those couldn't be handled with ov490_write_reg() ? > > > > As I've said in previous reply, I prefer the comunication with ov10640 > > to be kept explicit as long as we don't have more users and I don't > > better understand what register 0x5000 is for. > > > > Ok, I understand, but we 'know' it's the register page for communicating > with the Sensor though? > > Do we have any documentation on this at all? > > > > I can go through set_page() but the I have to pass 0x8019 which kind > > of defeat the purpose of defining registers by name > > > > I prefer to keep this as it is. > > > The issue I would have is you now have an invalid lastPage setting in > dev. So you're potentially letting yourself access registers on an > incorrect page, next time you use ov490_write_reg(). Correct, that's a very good point > > > You could quite easily define: > > /* > * The OV10640 is interacted with through > * registers in page 0x8019 using registers from 0x5000 to 0x5003, > * However the definition of these is currently unknown. > */ > #define SENSOR_REGISTER_0 0x80195000 > #define SENSOR_REGISTER_1 0x80195001 > #define SENSOR_REGISTER_2 0x80195002 > > To me that would be better than leaving some raw code inline > or maybe a short wrapper to make it even clearer that this talks to > /another component/: > > #define SENSOR_PAGE 0x8019 > > sensor_write(dev, reg, val) { > reg |= (SENSOR_PAGE << 16); > return ov490_write_reg(dev, reg, val); > } > > .. > sensor_write(dev, 0x5000, 0x01); As long as I don't have more users and I don't know what 0x5000 is for I don't want to make a function and hardcode this. > sensor_write(dev, 0x5001, 0x30); > sensor_write(dev, 0x5002, 0x0a); > ... > > > > >>> + 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); > >>> + > > Even if you want to keep the register writes raw, we know this code > block is handling the OV10640, so I'd be at least tempted to move it all > to a > > int ov10640_initialise(dev); > > but it's your code, and you seem quite against this, so I won't push > anymore. It's just it feels adding functions for 5 lines of code called once it's not that strictly necessary > > > > >>> + 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); > >>> + } > > You should almost certainly mark dev->lastPage as something invalid here > anyway (0xFFFF?). > > Otherwise the hardware page will be set to whatever was last used in the > ov490_regs_wizard table, and the ov490_read_reg() won't necessarily > update if it was on the same page last usage. > > > The incorrect lastPage is potentially a more critical issue though so > with that (if confirmed): Yes indeed, I'll make sure page changes go through the function in place to write 'regular' registers. > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > Thanks j > > > >>> + > >>> + /* > >>> + * The ISP is programmed with the content of a serial flash memory. > >>> + * Read the firmware configuration to reflect it through the V4L2 APIs. > >>> + */ > >>> + ov490_read_reg(dev, OV490_ISP_HSIZE_HIGH, &val); > >>> + dev->fmt.width = (val & 0xf) << 8; > >>> + ov490_read_reg(dev, OV490_ISP_HSIZE_LOW, &val); > >>> + dev->fmt.width |= (val & 0xff); > >>> + > >>> + ov490_read_reg(dev, OV490_ISP_VSIZE_HIGH, &val); > >>> + dev->fmt.height = (val & 0xf) << 8; > >>> + ov490_read_reg(dev, OV490_ISP_VSIZE_LOW, &val); > >>> + dev->fmt.height |= val & 0xff; > >>> + > >> > >> > >> Hrm, these look like "ov490_read_reg_long()", but given they're the only > >> occurrences of a "long read" I think we could leave it as is. > >> > >> > >>> + /* Set bus width to 12 bits with [0:11] ordering. */ > >>> + ov490_write_reg(dev, OV490_DVP_CTRL3, 0x10); > >>> + > >>> + dev_info(dev->dev, "Identified RDACM21 camera module\n"); > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int rdacm21_initialize(struct rdacm21_device *dev) > >>> +{ > >>> + int ret; > >>> + > >>> + /* Verify communication with the MAX9271: ping to wakeup. */ > >>> + dev->serializer->client->addr = MAX9271_DEFAULT_ADDR; > >>> + i2c_smbus_read_byte(dev->serializer->client); > >>> + > >>> + /* Serial link disabled during config as it needs a valid pixel clock. */ > >>> + ret = max9271_set_serial_link(dev->serializer, false); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + /* Set GPO high to hold OV490 in reset during max9271 configuration. */ > >>> + ret = max9271_set_gpios(dev->serializer, MAX9271_GPO); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + /* Configure I2C bus at 105Kbps speed and configure GMSL link. */ > >>> + ret = max9271_configure_i2c(dev->serializer, > >>> + MAX9271_I2CSLVSH_469NS_234NS | > >>> + MAX9271_I2CSLVTO_1024US | > >>> + MAX9271_I2CMSTBT_105KBPS); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + ret = max9271_configure_gmsl_link(dev->serializer); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + ret = max9271_set_address(dev->serializer, dev->addrs[0]); > >>> + if (ret) > >>> + return ret; > >>> + dev->serializer->client->addr = dev->addrs[0]; > >>> + > >>> + /* > >>> + * Release OV490 from reset and program address translation > >>> + * before performing OV490 configuration. > >>> + */ > >>> + ret = max9271_clear_gpios(dev->serializer, MAX9271_GPO); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + ret = max9271_set_translation(dev->serializer, dev->addrs[1], > >>> + OV490_I2C_ADDRESS); > >>> + if (ret) > >>> + return ret; > >>> + dev->isp->addr = dev->addrs[1]; > >>> + > >>> + ret = ov490_initialize(dev); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + /* > >>> + * Set reverse channel high threshold to increase noise immunity. > >>> + * > >>> + * This should be compensated by increasing the reverse channel > >>> + * amplitude on the remote deserializer side. > >>> + */ > >>> + ret = max9271_set_high_threshold(dev->serializer, true); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int rdacm21_probe(struct i2c_client *client) > >>> +{ > >>> + struct rdacm21_device *dev; > >>> + struct fwnode_handle *ep; > >>> + int ret; > >>> + > >>> + dev = devm_kzalloc(&client->dev, sizeof(*dev), GFP_KERNEL); > >>> + if (!dev) > >>> + return -ENOMEM; > >>> + dev->dev = &client->dev; > >>> + > >>> + dev->serializer = devm_kzalloc(&client->dev, sizeof(*dev->serializer), > >>> + GFP_KERNEL); > >>> + if (!dev->serializer) > >>> + return -ENOMEM; > >>> + > >>> + dev->serializer->client = client; > >>> + > >>> + ret = of_property_read_u32_array(client->dev.of_node, "reg", > >>> + dev->addrs, 2); > >>> + if (ret < 0) { > >>> + dev_err(dev->dev, "Invalid DT reg property: %d\n", ret); > >>> + return -EINVAL; > >>> + } > >>> + > >>> + /* Create the dummy I2C client for the sensor. */ > >>> + dev->isp = i2c_new_dummy_device(client->adapter, OV490_I2C_ADDRESS); > >>> + if (IS_ERR(dev->isp)) > >>> + return PTR_ERR(dev->isp); > >>> + > >>> + ret = rdacm21_initialize(dev); > >>> + if (ret < 0) > >>> + goto error; > >>> + > >>> + /* Initialize and register the subdevice. */ > >>> + v4l2_i2c_subdev_init(&dev->sd, client, &rdacm21_subdev_ops); > >>> + dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > >>> + > >>> + v4l2_ctrl_handler_init(&dev->ctrls, 1); > >>> + v4l2_ctrl_new_std(&dev->ctrls, NULL, V4L2_CID_PIXEL_RATE, > >>> + OV10640_PIXEL_RATE, OV10640_PIXEL_RATE, 1, > >>> + OV10640_PIXEL_RATE); > >>> + dev->sd.ctrl_handler = &dev->ctrls; > >>> + > >>> + ret = dev->ctrls.error; > >>> + if (ret) > >>> + goto error_free_ctrls; > >>> + > >>> + dev->pad.flags = MEDIA_PAD_FL_SOURCE; > >>> + dev->sd.entity.flags |= MEDIA_ENT_F_CAM_SENSOR; > >>> + ret = media_entity_pads_init(&dev->sd.entity, 1, &dev->pad); > >>> + if (ret < 0) > >>> + goto error_free_ctrls; > >>> + > >>> + ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL); > >>> + if (!ep) { > >>> + dev_err(&client->dev, > >>> + "Unable to get endpoint in node %pOF\n", > >>> + client->dev.of_node); > >>> + ret = -ENOENT; > >>> + goto error_free_ctrls; > >>> + } > >>> + dev->sd.fwnode = ep; > >>> + > >>> + ret = v4l2_async_register_subdev(&dev->sd); > >>> + if (ret) > >>> + goto error_put_node; > >>> + > >>> + return 0; > >>> + > >>> +error_put_node: > >>> + fwnode_handle_put(dev->sd.fwnode); > >>> +error_free_ctrls: > >>> + v4l2_ctrl_handler_free(&dev->ctrls); > >>> +error: > >>> + i2c_unregister_device(dev->isp); > >>> + > >>> + return ret; > >>> +} > >>> + > >>> +static int rdacm21_remove(struct i2c_client *client) > >>> +{ > >>> + struct rdacm21_device *dev = i2c_to_rdacm21(client); > >>> + > >>> + fwnode_handle_put(dev->sd.fwnode); > >>> + v4l2_async_unregister_subdev(&dev->sd); > >>> + v4l2_ctrl_handler_free(&dev->ctrls); > >>> + i2c_unregister_device(dev->isp); > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static const struct of_device_id rdacm21_of_ids[] = { > >>> + { .compatible = "imi,rdacm21" }, > >>> + { } > >>> +}; > >>> +MODULE_DEVICE_TABLE(of, rdacm21_of_ids); > >>> + > >>> +static struct i2c_driver rdacm21_i2c_driver = { > >>> + .driver = { > >>> + .name = "rdacm21", > >>> + .of_match_table = rdacm21_of_ids, > >>> + }, > >>> + .probe_new = rdacm21_probe, > >>> + .remove = rdacm21_remove, > >>> +}; > >>> + > >>> +module_i2c_driver(rdacm21_i2c_driver); > >>> + > >>> +MODULE_DESCRIPTION("GMSL Camera driver for RDACM21"); > >>> +MODULE_AUTHOR("Jacopo Mondi, Kieran Bingham, Laurent Pinchart, Niklas Söderlund, Vladimir Barinov"); > >>> +MODULE_LICENSE("GPL v2"); > >>> > >> >
Hello, this v5 mostly changes how the rdacm21 driver handles writes to the OV490 chip by handling the 'high' part of the sensor register through an helper function as suggested by Kieran and Laurent. The diff is available at: https://paste.debian.net/1172700/ Minor fixes in patches subjects and in bindings as reported by Sergei and Kieran. Bindings have now been sent to DT people now that they've been reviewed by Kieran. v4: https://patchwork.linuxtv.org/project/linux-media/list/?series=3847 v3: https://patchwork.linuxtv.org/project/linux-media/list/?series=3657 Background in v1 cover letter: https://www.spinics.net/lists/linux-renesas-soc/msg52886.html 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 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 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 | 595 ++++++++++++++++++ 8 files changed, 1087 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