Received: by 2002:a05:6a10:16a7:0:0:0:0 with SMTP id gp39csp3946103pxb; Tue, 17 Nov 2020 07:35:35 -0800 (PST) X-Google-Smtp-Source: ABdhPJz1QvmWp0vc00B+37j56vZ5Q18ygg/fOvNbYtCK8WsQxnUXXX1vCoQz5C4cZnvzlHt6sV9s X-Received: by 2002:a17:906:374a:: with SMTP id e10mr10454012ejc.246.1605627335624; Tue, 17 Nov 2020 07:35:35 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1605627335; cv=none; d=google.com; s=arc-20160816; b=AZswNSG+ai5MoKboFkQA+aUepmytreBqEZt0j//fs2N4H2y9GapiQxAyZMd4A2vCsD /1Yd9KldeWtyY78YQX75CClSUwh9tDipe/HZfBdb8cXVS1yf6aJGztIW85aStyr32/9R VGtGuRmEokHq0tntFBKFoPR+5P/Q1Ekb47+5ndI9JmDCO7CRRnqdPKiUx7zpIhQcsvQ4 mpuwbvo+aMUgm9UJPqA4FfhbHA5e8v37PDT2IaWqzWCa6ecnqyhTN2zp8zQvH/QzE1+3 3Ddem1DSnx93EHPFIBKrFjeLKIT66QK1YbevZcT43JB7DhCxddcK+pDCZwhH6pf+fZmq 2mmg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:content-language :in-reply-to:mime-version:user-agent:date:message-id:organization :from:references:cc:to:subject:reply-to:dkim-signature; bh=I+hoFx3uEnQMqCk7vZPNTL4t4VcLJ7S6AdXdcIqScR8=; b=z91vxSxtAxyox/mkuYzn6fvE4c6E/l2nk2SI7+btuM08sBE+TUCgTWfKHEDBN6WSGA gnkJDeAiXdAo7cw4kWO1dHJsz//Z/hS06u+szs2yoeTBhH7EksPz+AKVB/dERaG5tWhl D6ANLhsty8rNH02usSl954Emn2YgOewnHHv/5y7nW2OPascDz89deBWz+sMWDXFSxFLo j6FavuFM68CCft8pmovfP86mgjdbkC9ZAORsaN01wr+fBo+QNfc/RXsFvkG7ptIS0krl jG8hnfmooEqy0RyrBsfdhmtIxj3ybJtRxBOs6Z0RtxUr9Z0XuOtkOoImnhQSaKZu7qFs KKCw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass (test mode) header.i=@ideasonboard.com header.s=mail header.b=lW8Vw7EO; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id n23si14196659edy.328.2020.11.17.07.35.10; Tue, 17 Nov 2020 07:35:35 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass (test mode) header.i=@ideasonboard.com header.s=mail header.b=lW8Vw7EO; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728505AbgKQPbo (ORCPT + 99 others); Tue, 17 Nov 2020 10:31:44 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51354 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726849AbgKQPbn (ORCPT ); Tue, 17 Nov 2020 10:31:43 -0500 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 76B33C0613CF; Tue, 17 Nov 2020 07:31:43 -0800 (PST) Received: from [192.168.0.20] (cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 989002A3; Tue, 17 Nov 2020 16:31:40 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1605627101; bh=YCS9NLB00O0TZAbYDpPYCITIFLd5VvQQ2IR36eALT7w=; h=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From; b=lW8Vw7EOMJlgOdyAQgtFBQaXy4sFIWrH0V8L4sjw7ErznbPXvMHdclXnHBFSM8205 pUdrkUKWJeWcPuph6yyFC5VrvpI8jXrUMfeOJuoUP5PxV2JJoTlJt4/0g0lVMQ2Ftw oSIOAaweCvkCLba4o0m4EuARnoZbigoj5Wd9ClvI= Reply-To: kieran.bingham+renesas@ideasonboard.com Subject: Re: [PATCH v5 1/8] media: i2c: Add driver for RDACM21 camera module To: Jacopo Mondi Cc: Jacopo Mondi , laurent.pinchart+renesas@ideasonboard.com, niklas.soderlund+renesas@ragnatech.se, geert@linux-m68k.org, linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org, linux-kernel@vger.kernel.org, Hyun Kwon , Manivannan Sadhasivam , sergei.shtylyov@gmail.com References: <20201116135305.81319-1-jacopo+renesas@jmondi.org> <20201116135305.81319-2-jacopo+renesas@jmondi.org> <20201117133634.s62i32hcwzikmhzb@uno.localdomain> From: Kieran Bingham Organization: Ideas on Board Message-ID: <50b1f540-9289-b82b-e261-f549603556f7@ideasonboard.com> Date: Tue, 17 Nov 2020 15:31:37 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20201117133634.s62i32hcwzikmhzb@uno.localdomain> Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>> --- >>> 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 >>> +M: Kieran Bingham >>> +M: Laurent Pinchart >>> +M: Niklas Söderlund >>> +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 >>> 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 >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#include >>> +#include >>> +#include >>> +#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 >>> + >>> + /* >>> + * 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"); >>> >>