From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx07-001d1705.pphosted.com (mx07-001d1705.pphosted.com [185.132.183.11]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 8A7D13FC5A1; Thu, 15 Oct 2026 09:21:26 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.132.183.11 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1792056091; cv=none; b=NIk8ob+u8gtArGbpoFVgNqn0qswdcZMKiksurNNAllgKl6Q0sU34rn2126TidKem9UKKxjWpOTd9zJBe2eldRcMY3HGxKiS6UDg9bZcUBI2zK6ZPLFLaiEcQSP3CE5BbmihjmDgr4t0DK9P5bGxaHU0ceqvYkF30oRIMstYyqgo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1792056091; c=relaxed/simple; bh=MIhcj9OmX6xx9EWiY1SHrosqzeo4OhfqcGZBNR39UzU=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=UPpCwHK3e8r8yAjFUt2T7/iJTy04xx68g00CHVkwEA3UV5iojBOgYd6WuH1hxoPM/CQ1hW/iW8byCVOeXeLCOC0zDv25CFR1R1oB6AzPfv1F/tZme8gO1DgPEqouu2IQCOBadwxtxhibhnsp4n1IPoB9FT8XAS789+Zwl+ogtJ8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=sony.com; spf=pass smtp.mailfrom=sony.com; dkim=pass (2048-bit key) header.d=sony.com header.i=@sony.com header.b=k+djng7T; arc=none smtp.client-ip=185.132.183.11 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=sony.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=sony.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=sony.com header.i=@sony.com header.b="k+djng7T" Received: from pps.filterd (m0209324.ppops.net [127.0.0.1]) by mx08-001d1705.pphosted.com (8.18.1.11/8.18.1.11) with ESMTP id 69ENQDQk3502554; Thu, 15 Oct 2026 08:56:38 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sony.com; h=cc :content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to; s=p1; bh=dD2fpxu hNNn8o7sRFgv8RuNCr5KC8U97L0ZRdm4ebI4=; b=k+djng7TlhqunHEz9yX2fyI OQtkmnC4Im3h0LV5Epod7zeLGsIOre2ni2TVllSedZtTul+d+BjoVqs3Tts0c7pE 4ZcTybYwBGibS1Xk2x9Er7beZVLgfG+/FMs/SZ/C+Ib7pNtMoDcN5EmRCyteoBWF ptb6TG5l/JEjMTuZ8lJInRKMZRXyM650VEqr6bs41DgPOeZirlM48l24GajP0AdF R3QjmnKVL8L0W7Z/wAZy8S+2WDLP8NyKnyAbKW2VDG1P+ZzXgziO/Yv4Zo5jIxxP fo5nUWyelEXFLW7P3O9L0sCQGvBhX7e7/hFgb/ASHspo3JMjQSiVuENkaHZLYHw= = Received: from sgppsea02v.sg.gdce.sony.com.sg ([121.100.43.226]) by mx08-001d1705.pphosted.com (PPS) with ESMTPS id 4h7dd4p3au-1 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NO); Thu, 15 Oct 2026 08:56:38 +0000 (GMT) Received: from pps.filterd (SGPPSEA02v.sg.gdce.sony.com.sg [127.0.0.1]) by SGPPSEA02v.sg.gdce.sony.com.sg (8.18.1.11/8.18.1.11) with ESMTP id 69F8E6Qx3717003; Thu, 15 Oct 2026 08:56:36 GMT Received: from sgppsba02v.s.gdce.sony.com.sg ([146.215.113.219]) by SGPPSEA02v.sg.gdce.sony.com.sg (PPS) with ESMTPS id 4h7eej4fkd-1 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Thu, 15 Oct 2026 08:56:36 +0000 (GMT) Received: from pps.filterd (SGPPSBA02v.s.gdce.sony.com.sg [127.0.0.1]) by SGPPSBA02v.s.gdce.sony.com.sg (8.18.1.11/8.18.1.11) with ESMTP id 69F7hLm21733938; Thu, 15 Oct 2026 08:56:35 GMT Received: from [43.3.250.71] ([146.215.113.251]) by SGPPSBA02v.s.gdce.sony.com.sg (PPS) with ESMTPS id 4h7eejtyst-1 (version=TLSv1.3 cipher=TLS_AES_128_GCM_SHA256 bits=128 verify=NOT); Thu, 15 Oct 2026 08:56:35 +0000 (GMT) Message-ID: <413f5e92-2f45-4f2b-af92-90843e76b0b2@sony.com> Date: Thu, 15 Oct 2026 17:56:33 +0900 Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 2/2] media: i2c: Add Sony IMX908 image sensor driver To: Laurent Pinchart , Sakari Ailus Cc: mchehab@kernel.org, hverkuil+cisco@kernel.org, linux-media@vger.kernel.org, devicetree@vger.kernel.org, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, kieran.bingham@ideasonboard.com, jai.luthra@ideasonboard.com, Ryuichi.Tadano@sony.com, Kengo.Hayasaka@sony.com, Kazumi.A.Sato@sony.com, Yuji.John.Takahashi@sony.com, linux-kernel@vger.kernel.org, Tim Bird References: <20260828064843.65047-1-lachlan.michael@sony.com> <20260828064843.65047-3-lachlan.michael@sony.com> <20261001133138.GM944070@killaraus.ideasonboard.com> <20261001134139.GA1347947@killaraus.ideasonboard.com> Content-Language: en-US From: Lachlan Michael Organization: Sony Semiconductor Solutions Corporation In-Reply-To: <20261001134139.GA1347947@killaraus.ideasonboard.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Proofpoint-Reinject: loops=2 maxloops=12 X-Sony-BusinessRelay-GUID: 3NTQfOhGV0duwws6YIi-D0nT8ChWAkB3 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.293,Aquarius:18.0.1176,Hydra:6.1.134,FMLib:17.12.100.49 definitions=2026-10-13_01,2026-10-14_01,2025-10-01_01 X-Sony-EdgeRelay-GUID: XgQNMRoUGD_DlC5BU76UuyJJYZxJzJ-D X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.293,Aquarius:18.0.1176,Hydra:6.1.134,FMLib:17.12.100.49 definitions=2026-10-13_01,2026-10-14_01,2025-10-01_01 X-Authority-Analysis: v=2.4 cv=Z7F5j3RA c=1 sm=1 tr=0 ts=6ad09546 cx=c_pps a=6mgWUpd4vGE96FGw1JXD8A==:117 a=6mgWUpd4vGE96FGw1JXD8A==:17 a=IkcTkHD0fZMA:10 a=660iZSQnnn4A:10 a=VkNPw1HP01LnGYTKEx00:22 a=KAb5x4SsHD3PzxGk7EmX:22 a=I_Y4LlEPCAdqsIK1J1Zo:22 a=xq3W2uTSAAAA:8 a=z6gsHLkEAAAA:8 a=pGLkceISAAAA:8 a=MGLFNxe6PGFUVSrxihMA:9 a=QEXdDO2ut3YA:10 a=P5L7wpMTXyg1GfFA3Gwx:22 X-Proofpoint-Spam-Info: AW1haW4tMjYxMDE1MDAzNSBTYWx0ZWRfX0ysVPtb8XgT2 0Y3sr1bAGnmxiiPdINJbX0gO3Duu2MoiSBWWDdULELdq5gXo2e92KGxDJOuILDa0sos4o2yEbYJ tPTJcjJRrsOUwjj+QTmQXuOYxNy0ehmdh3z6Zgvb3z0V0Fz7jQ5e X-Proofpoint-ORIG-GUID: XKoNW6COmE30ZVM9Wc3LI-0Siuuy6RNs X-Proofpoint-Spam-Details-Enc: AW1haW4tMjYxMDE1MDAzNSBTYWx0ZWRfXxeCpY0Z4BJSG H6r+6b3+cO+ZAitH6EFbBl1cWYhLStI4RI2bBJsO7UGRTn5x5h5+qDCfkoOPnKK6RfqiduAzPUk Ih7xj1loSOk7hkvTi1T9K61M46vwLHFPWVNtS17uTN1j4PcpdP1gV0lI8o3qxUHatzLstrcgmwL 2u8oy3uWqApirF7Ttbq+B92rU26ygC+mMYeeKNnw8QAfRQc7Ksxw3DHc7kXIOdYqKSe6/+ArTSL MPgOouUZQ91ky7IVyvFb1EFcs/bmSy01nDnX3JAUPbExEBEQjU9tzLiWmlXWj2qTlQKZ7JoDelX 3rSN6oSIe70I1oaTnNdznQy00n6UHJDzb6nH6IQe5jFE8ldkvLTq6dAImHOWHTMpT/JmVZgD+k7 4DAF4CIARUKsdqDgD2cWcLJk34GfyUJb/+X6/areTPwS6gCX1JcEruw1J72qRGEQT1RlYeG2OZt eejmUUNY+dO5QbFtokA== X-Proofpoint-GUID: XKoNW6COmE30ZVM9Wc3LI-0Siuuy6RNs X-Sony-Outbound-GUID: XKoNW6COmE30ZVM9Wc3LI-0Siuuy6RNs X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.293,Aquarius:18.0.1176,Hydra:6.1.134,FMLib:17.12.100.49 definitions=2026-10-13_01,2026-10-14_01,2025-10-01_01 Dear Laurent, Thank-you for all the reviews. On 10/1/2026 10:41 PM, Laurent Pinchart wrote: > On Thu, Oct 01, 2026 at 04:31:40PM +0300, Laurent Pinchart wrote: >> On Thu, Oct 01, 2026 at 03:24:54PM +0300, Sakari Ailus wrote: >>> Hi Lachlan, >>> >>> Thanks for the update. A few more comments below... >>> >>> On Fri, Aug 28, 2026 at 03:48:43PM +0900, Lachlan Michael wrote: >>>> The Sony IMX908 is an 8.39 megapixel (3856x2176) CMOS image sensor >>>> with a MIPI CSI-2 output interface, configurable as either 2 or 4 >>>> data lanes. >>>> >>>> Add a V4L2 sub-device driver for the sensor. The driver supports >>>> RAW10 and RAW12 output formats, exposure and analogue gain controls, >>>> horizontal and vertical flipping, horizontal and vertical blanking >>>> controls, window cropping and test pattern generation. >>>> >>>> HDR modes and RAW16 output are not currently supported. >>>> >>>> Signed-off-by: Lachlan Michael >>>> --- >>>> Changes in v3: >>>> - Correct link-frequency indexing mapping to match the sensor >>>> DATARATE_SEL values and the frequency ordering >>>> - Changed the pixel_rate to 16-pixel-per-clocks based on datasheet >>>> mode values >>>> - Clean up the constant names, register definitions and comments in >>>> response to review feedback. >>>> - Remove various (uneeded) checks in code based on feedback >>>> - Remove cached vmax/hmax and recalculated where necessary >>>> - Drop pixel_rate and test_pattern pointers from struct >>>> - Remove read-only controls set_ctrl switch >>>> - Remove event handlers >>>> - Remove uneeded delays at startup >>>> - Simplify runtime_resume/suspend functions >>>> - Remove the 4k recording-area in favour of larger active area >>>> - Use the same constraints for VMAX for crop and all pixel mode to >>>> simplify the driver logic by removing branching >>>> - Rework HMAX based on the fact that the whole line is always read >>>> even in crop mode. HMAX varies in all modes now, but not with >>>> horizontal crop. >>>> - Minor edits and bugfixes >>>> --- >>>> MAINTAINERS | 1 + >>>> drivers/media/i2c/Kconfig | 11 + >>>> drivers/media/i2c/Makefile | 1 + >>>> drivers/media/i2c/imx908.c | 1367 ++++++++++++++++++++++++++++++++++++ >>>> 4 files changed, 1380 insertions(+) >>>> create mode 100644 drivers/media/i2c/imx908.c >>>> >>>> diff --git a/MAINTAINERS b/MAINTAINERS >>>> index bb7adcfd4767..a17c31234ec7 100644 >>>> --- a/MAINTAINERS >>>> +++ b/MAINTAINERS >>>> @@ -25542,6 +25542,7 @@ SONY IMX908 SENSOR DRIVER >>>> M: Lachlan Michael >>>> S: Maintained >>>> F: Documentation/devicetree/bindings/media/i2c/sony,imx908.yaml >>>> +F: drivers/media/i2c/imx908.c >>>> >>>> SONY MEMORYSTICK SUBSYSTEM >>>> M: Maxim Levitsky >>>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig >>>> index 5c52007f9cbe..cfa1359d8c7d 100644 >>>> --- a/drivers/media/i2c/Kconfig >>>> +++ b/drivers/media/i2c/Kconfig >>>> @@ -310,6 +310,17 @@ config VIDEO_IMX678 >>>> To compile this driver as a module, choose M here: the >>>> module will be called imx678. >>>> >>>> +config VIDEO_IMX908 >>>> + tristate "Sony IMX908 sensor support" >>>> + depends on GPIOLIB >>>> + select V4L2_CCI_I2C >>>> + help >>>> + This is a Video4Linux2 sensor driver for the Sony >>>> + IMX908 CMOS image sensor. >>>> + >>>> + To compile this driver as a module, choose M here: the >>>> + module will be called imx908. >>>> + >>>> config VIDEO_MAX9271_LIB >>>> tristate >>>> >>>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile >>>> index d04bd5724552..d91ed1bc6427 100644 >>>> --- a/drivers/media/i2c/Makefile >>>> +++ b/drivers/media/i2c/Makefile >>>> @@ -63,6 +63,7 @@ obj-$(CONFIG_VIDEO_IMX412) += imx412.o >>>> obj-$(CONFIG_VIDEO_IMX415) += imx415.o >>>> obj-$(CONFIG_VIDEO_IMX678) += imx678.o >>>> obj-$(CONFIG_VIDEO_IMX471) += imx471.o >>>> +obj-$(CONFIG_VIDEO_IMX908) += imx908.o >>>> obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o >>>> obj-$(CONFIG_VIDEO_ISL7998X) += isl7998x.o >>>> obj-$(CONFIG_VIDEO_KS0127) += ks0127.o >>>> diff --git a/drivers/media/i2c/imx908.c b/drivers/media/i2c/imx908.c >>>> new file mode 100644 >>>> index 000000000000..ca85a5cd058a >>>> --- /dev/null >>>> +++ b/drivers/media/i2c/imx908.c >>>> @@ -0,0 +1,1367 @@ >>>> +// SPDX-License-Identifier: GPL-2.0 >>>> +/* >>>> + * V4L2 driver for Sony IMX908 >>>> + * >>>> + * Diagonal 6.42 mm (Type 1/2.8) CMOS image sensor with 8.39 M pixels. >>>> + * >>>> + * Copyright 2026 Sony Semiconductor Solutions Corporation >>>> + * >>>> + */ >>>> + >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include /* DIV_ROUND_UP */ >>>> +#include /* DIV64_U64_ROUND_UP */ >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> + >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> + >>>> +/* ---- IMX908 Registers --------- */ >>>> +#define IMX908_REG_STANDBY CCI_REG8(0x3000) >>>> +#define IMX908_STANDBY_EN BIT(0) /* Standby mode */ >>>> +#define IMX908_STANDBY_CANCEL 0x00 /* Operating mode */ >>>> +#define IMX908_REG_XMSTA CCI_REG8(0x3002) /* [0] Init 1h */ >>>> +#define IMX908_XMSTA_START 0x00 >>>> +#define IMX908_XMSTA_STOP 0x01 >>>> +#define IMX908_REG_XMASTER CCI_REG8(0x3003) /* [0] Init 0h */ >>>> +#define IMX908_CONTROLLER_MODE 0x00 >>>> +#define IMX908_REG_INCK_SEL CCI_REG8(0x3014) /* 0:74.25 1:37.125 2:72 3:27 4:24 */ >>>> +#define IMX908_REG_DATARATE_SEL CCI_REG8(0x3015) >>>> + >>>> +/* ---- Crop */ >>>> +#define IMX908_REG_WINMODE CCI_REG8(0x3018) /* [3:0] */ >>>> +#define IMX908_WINMODE_ALLPIX 0x00 /* All-pixel readout mode */ >>>> +#define IMX908_WINMODE_CROP 0x04 /* Window cropping mode */ >>>> + >>>> +/* ---- Flip */ >>>> +#define IMX908_REG_HREVERSE CCI_REG8(0x3020) >>>> +#define IMX908_HREVERSE_NORMAL 0x00 >>>> +#define IMX908_HREVERSE_INV 0x01 >>>> +#define IMX908_REG_VREVERSE CCI_REG8(0x3021) >>>> +#define IMX908_VREVERSE_NORMAL 0x00 >>>> +#define IMX908_VREVERSE_INV 0x01 >>>> + >>>> +/* ---- Internal Analog to Digital conversion bit width */ >>>> +#define IMX908_REG_ADBIT CCI_REG8(0x3022) >>>> +#define IMX908_ADBIT_10BIT 0x00 /* 10-bit */ >>>> +#define IMX908_ADBIT_12BIT 0x01 /* 11-bit + digital dither */ >>>> + >>>> +#define IMX908_REG_MDBIT CCI_REG8(0x3023) >>>> +#define IMX908_MDBIT_RAW10 0x00 >>>> +#define IMX908_MDBIT_RAW12 0x01 >>>> + >>>> +/* ---- VMAX Frame length in lines */ >>>> +#define IMX908_REG_VMAX CCI_REG24_LE(0x3028) /* [19:0] */ >>>> +#define IMX908_VMAX_MAX 0xfffff >>>> +#define IMX908_VMAX_DEFAULT 2250 >>>> +/* >>>> + * VMAX restrictions, documented for window cropping mode: >>>> + * VMAX >= PIX_VWIDTH + 70 >>>> + * VMAX >= 1206 >>>> + */ >>>> +#define IMX908_VMAX_MARGIN 70 >>>> +#define IMX908_VMAX_MIN 1206 >>>> + >>>> +/* ---- HMAX Line length in clocks */ >>>> +#define IMX908_REG_HMAX CCI_REG16_LE(0x302c) /* [15:0] */ >>>> +#define IMX908_HMAX_MAX 0xffff >>>> +#define IMX908_HMAX_DEFAULT 1100 >>>> + >>>> +/* ---- Cropping related */ >>>> +#define IMX908_REG_PIX_HST CCI_REG16_LE(0x303c) /* [12:0] Init 0h */ >>>> +#define IMX908_REG_PIX_HWIDTH CCI_REG16_LE(0x303e) /* [12:0] Init 0F10h */ >>>> +#define IMX908_REG_LANEMODE CCI_REG8(0x3040) >>>> +#define IMX908_LANEMODE_2LANE 0x01 >>>> +#define IMX908_LANEMODE_4LANE 0x03 >>>> +#define IMX908_REG_PIX_VST CCI_REG16_LE(0x3044) /* [12:0] Init 0h */ >>>> +#define IMX908_REG_PIX_VWIDTH CCI_REG16_LE(0x3046) /* [12:0] Init 0884h */ >>>> +#define IMX908_CROP_ALIGN_VSTART 4 >>>> +#define IMX908_CROP_ALIGN_VWIDTH 4 >>>> +#define IMX908_CROP_ALIGN_HSTART 4 >>>> +#define IMX908_CROP_ALIGN_HWIDTH 16 >>>> +#define IMX908_CROP_MIN_HEIGHT 1136 >>>> +#define IMX908_CROP_MIN_WIDTH 1040 >>>> + >>>> +/* ---- Shutter */ >>>> +#define IMX908_REG_SHR0 CCI_REG24_LE(0x3050) /* [19:0] */ >>>> +#define IMX908_MIN_SHR0 7 >>>> + >>>> +/* >>>> + * Analogue gain control >>>> + * Range is from 0 to 100 (0dB - 30dB) with 0.3dB step size >>>> + * Values from 101 to 240 are valid but correspond to additional digital gain >>>> + * (0.3dB - 42dB) so don't expose it to userspace >>>> + */ >>>> +#define IMX908_REG_GAIN CCI_REG16_LE(0x3070) /* [10:0] */ >>>> +#define IMX908_ANA_GAIN_MAX 100 >>>> +#define IMX908_ANA_GAIN_MIN 0 >>>> +#define IMX908_ANA_GAIN_STEP 1 >>>> +#define IMX908_ANA_GAIN_DEFAULT 0 >>>> + >>>> +/* ---- XVS/XHS output mode */ >>>> +#define IMX908_XXS_DRV CCI_REG8(0x30a6) /* [1:0] XVS_DRV [3:2] XHS_DRV */ >>>> + >>>> +#define IMX908_REG_BLKLEVEL CCI_REG16_LE(0x30dc) /* [15:0] black level */ >>>> + >>>> +/* ---- Test Pattern Generator (TPG) registers */ >>>> +#define IMX908_REG_TPG_EN CCI_REG8(0x30e0) >>>> +#define IMX908_TPG_EN_BIT BIT(0) >>>> +#define IMX908_REG_TPG_PATSEL CCI_REG8(0x30e2) /* [4:0] pattern idx */ >>>> +#define IMX908_REG_TESTCLKEN CCI_REG8(0x4900) >>>> +#define IMX908_TESTCLKEN_BIT BIT(3) >>>> + >>>> +#define IMX908_REG_TYPE_ID CCI_REG16_LE(0x4c0c) >>>> +#define IMX908_CHIP_ID 0x038c >>>> + >>>> +/* ---- IMX908 Common Register List ---- */ >>>> +static const struct cci_reg_sequence imx908_common_regs[] = { >>>> + { IMX908_XXS_DRV, IMX908_CONTROLLER_MODE }, >>>> + { CCI_REG8(0x039c), 0x03 }, >>>> + { CCI_REG8(0x3416), 0x20 }, >>>> + { CCI_REG8(0x3417), 0x00 }, >>>> + { CCI_REG8(0x3456), 0xf4 }, >>>> + { CCI_REG8(0x345d), 0x01 }, >>>> + { CCI_REG8(0x3460), 0x00 }, >>>> + { CCI_REG8(0x3461), 0x0b }, >>>> + { CCI_REG8(0x3471), 0x00 }, >>>> + { CCI_REG8(0x3472), 0x23 }, >>>> + { CCI_REG8(0x347b), 0x02 }, >>>> + { CCI_REG8(0x3481), 0x01 }, >>>> + { CCI_REG8(0x380c), 0x00 }, >>>> + { CCI_REG8(0x380f), 0x0c }, >>>> + { CCI_REG8(0x381c), 0x11 }, >>>> + { CCI_REG8(0x3820), 0x22 }, >>>> + { CCI_REG8(0x3824), 0x33 }, >>>> + { CCI_REG8(0x3828), 0x22 }, >>>> + { CCI_REG8(0x382c), 0x33 }, >>>> + { CCI_REG8(0x3830), 0x33 }, >>>> + { CCI_REG8(0x3838), 0x05 }, >>>> + { CCI_REG8(0x383c), 0x07 }, >>>> + { CCI_REG8(0x3840), 0x06 }, >>>> + { CCI_REG8(0x3848), 0x17 }, >>>> + { CCI_REG8(0x384c), 0x0b }, >>>> + { CCI_REG8(0x3850), 0x10 }, >>>> + { CCI_REG8(0x3854), 0x12 }, >>>> + { CCI_REG8(0x385c), 0x20 }, >>>> + { CCI_REG8(0x38c4), 0x64 }, >>>> + { CCI_REG8(0x38c5), 0x64 }, >>>> + { CCI_REG8(0x38c6), 0x64 }, >>>> + { CCI_REG8(0x3c4a), 0x15 }, >>>> + { CCI_REG8(0x3c4c), 0x13 }, >>>> + { CCI_REG8(0x3c4d), 0x13 }, >>>> + { CCI_REG8(0x3c4e), 0x13 }, >>>> + { CCI_REG8(0x3c50), 0x77 }, >>>> + { CCI_REG8(0x3c51), 0x07 }, >>>> + { CCI_REG8(0x3db4), 0x00 }, >>>> + { CCI_REG8(0x4419), 0x06 }, >>>> + { CCI_REG8(0x441c), 0x00 }, >>>> + { CCI_REG8(0x4426), 0x00 }, >>>> + { CCI_REG8(0x4538), 0x20 }, >>>> + { CCI_REG8(0x4539), 0x19 }, >>>> + { CCI_REG8(0x453a), 0x19 }, >>>> + { CCI_REG8(0x453b), 0x19 }, >>>> + { CCI_REG8(0x453c), 0x19 }, >>>> + { CCI_REG8(0x453d), 0x19 }, >>>> + { CCI_REG8(0x453e), 0x19 }, >>>> + { CCI_REG8(0x453f), 0x19 }, >>>> + { CCI_REG8(0x4540), 0x19 }, >>>> + { CCI_REG8(0x4544), 0x11 }, >>>> + { CCI_REG8(0x4545), 0x11 }, >>>> + { CCI_REG8(0x4546), 0x11 }, >>>> + { CCI_REG8(0x463c), 0x20 }, >>>> + { CCI_REG8(0x465e), 0xcf }, >>>> + { CCI_REG8(0x4684), 0x20 }, >>>> + { CCI_REG8(0x46a6), 0xcf }, >>>> + { CCI_REG8(0x46e2), 0xf3 }, >>>> +}; >>>> + >>>> +static const u32 imx908_bit_depth_regs[] = { >>>> + CCI_REG8(0x3d78), >>>> + CCI_REG8(0x3d79), >>>> + CCI_REG8(0x3d80), >>>> + CCI_REG8(0x3d81), >>>> + CCI_REG8(0x3d88), >>>> + CCI_REG8(0x3d89), >>>> + CCI_REG8(0x3d90), >>>> + CCI_REG8(0x3d91), >>>> +}; >>>> + >>>> +#define IMX908_EXPOSURE_MIN 1 >>>> +#define IMX908_EXPOSURE_STEP 1 >>>> + >>>> +/* ---- Internal image-data interface clock */ >>>> +#define IMX908_VTPXCK_HZ 74250000ULL >>>> + >>>> +/* Fixed pixel rate: 16 pixels per video timing pixel clock cycle */ >>>> +#define IMX908_PIX_PER_CLK 16 >>>> +#define IMX908_PIXEL_RATE (IMX908_VTPXCK_HZ * IMX908_PIX_PER_CLK) >>>> + >>>> +/* ---- Subdev Pads */ >>>> +#define IMX908_SOURCE_PAD 0 >>>> + >>>> +#define IMX908_DEFAULT_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10 >>>> + >>>> +/* >>>> + * IMX908 total area includes active area height plus >>>> + * 4 pixels effective pixel ignored area >>>> + * 10 pixels vertical direction effective OB >>>> + * 10 pixels OB side ignored area >>>> + */ >>>> +static const struct v4l2_rect imx908_total_area = { >>>> + .top = 0, >>>> + .left = 0, >>>> + .width = 3856, >>>> + .height = 2200, >>>> +}; >>>> + >>>> +static const struct v4l2_rect imx908_active_area = { >>>> + .top = 0, >>>> + .left = 0, >>>> + .width = 3856, >>>> + .height = 2176, >>>> +}; >>>> + >>>> +static const char *const imx908_supply_names[] = { >>>> + "avdd", /* Analog (3.3V) supply */ >>>> + "dvdd", /* Digital Core (1.1V) supply */ >>>> + "ovdd", /* IF (1.8V) supply */ >>>> +}; >>>> + >>>> +static const u32 imx908_mbus_codes[] = { >>>> + MEDIA_BUS_FMT_SRGGB10_1X10, /* RAW10 */ >>>> + MEDIA_BUS_FMT_SRGGB12_1X12, /* RAW12 */ >>>> +}; >>>> + >>>> +/* >>>> + * Link frequency indices, ordered so that the index is also the >>>> + * DATARATE_SEL register value. Keep in descending frequency order. >>>> + */ >>>> +enum { >>>> + IMX908_LINK_FREQ_1188MHZ = 0x00, >>>> + IMX908_LINK_FREQ_1039MHZ = 0x01, >>>> + IMX908_LINK_FREQ_891MHZ = 0x02, >>>> + IMX908_LINK_FREQ_720MHZ = 0x03, >>>> + IMX908_LINK_FREQ_594MHZ = 0x04, >>>> + IMX908_LINK_FREQ_445MHZ = 0x05, >>>> + IMX908_LINK_FREQ_360MHZ = 0x06, >>>> + IMX908_LINK_FREQ_297MHZ = 0x07, >>>> +}; >>>> + >>>> +static const s64 imx908_link_freqs[] = { >>>> + [IMX908_LINK_FREQ_1188MHZ] = 1188000000LL, >>>> + [IMX908_LINK_FREQ_1039MHZ] = 1039500000LL, >>>> + [IMX908_LINK_FREQ_891MHZ] = 891000000LL, >>>> + [IMX908_LINK_FREQ_720MHZ] = 720000000LL, >>>> + [IMX908_LINK_FREQ_594MHZ] = 594000000LL, >>>> + [IMX908_LINK_FREQ_445MHZ] = 445500000LL, >>>> + [IMX908_LINK_FREQ_360MHZ] = 360000000LL, >>>> + [IMX908_LINK_FREQ_297MHZ] = 297000000LL, >>>> +}; >>>> + >>>> +/* Allowed clock values in Hz */ >>>> +static const u32 imx908_inck_table[] = { >>>> + 74250000, >>>> + 37125000, >>>> + 72000000, >>>> + 27000000, >>>> + 24000000, >>>> +}; >>>> + >>>> +struct imx908 { >>>> + struct v4l2_subdev sd; >>>> + struct media_pad pad; >>>> + struct device *dev; >>>> + >>>> + struct regmap *cci; >>>> + >>>> + struct clk *xclk; >>>> + struct gpio_desc *reset_gpio; >>>> + struct regulator_bulk_data supplies[ARRAY_SIZE(imx908_supply_names)]; >>>> + >>>> + u8 inck_sel; >>>> + >>>> + u8 num_lanes; >>>> + unsigned long link_freq_bitmap; >>>> + unsigned int link_freq_idx; >>>> + >>>> + struct { >>>> + struct v4l2_ctrl_handler handler; >>>> + >>>> + struct v4l2_ctrl *exposure; >>>> + struct v4l2_ctrl *vblank; >>>> + struct v4l2_ctrl *hblank; >>>> + } ctrls; >>>> +}; >>>> + >>>> +static inline struct imx908 *to_imx908(struct v4l2_subdev *_sd) >>>> +{ >>>> + return container_of(_sd, struct imx908, sd); >>>> +} >>>> + >>>> +/* ----------------------- Basic Helper Functions --------------------------- */ >>>> +static bool imx908_mbus_code_supported(struct imx908 *imx, u32 code) >>>> +{ >>>> + for (unsigned int i = 0; i < ARRAY_SIZE(imx908_mbus_codes); i++) { >>>> + if (imx908_mbus_codes[i] == code) >>>> + return true; >>>> + } >>>> + >>>> + return false; >>>> +} >>>> + >>>> +static u16 imx908_calc_hmax(u32 width, u32 hblank) >>>> +{ >>>> + u32 hmax = DIV_ROUND_UP(width + hblank, IMX908_PIX_PER_CLK); >>>> + >>>> + return min_t(u32, hmax, IMX908_HMAX_MAX); >>>> +} >>>> + >>>> +/* Horizontal cropping is digital, so the full width is always read out */ >>>> +static u16 imx908_calc_min_hmax(struct imx908 *imx, u8 bpp) >>>> +{ >>>> + u64 link_hz = imx908_link_freqs[imx->link_freq_idx]; >>>> + u64 num = (u64)imx908_active_area.width * bpp * IMX908_VTPXCK_HZ; >>>> + u64 den = (u64)imx->num_lanes * link_hz * 2; /* DDR */ >>>> + >>>> + /* >>>> + * den can exceed 32 bits (e.g. 4 lanes * 720 MHz * 2 = 5.76 GHz), so >>>> + * DIV_ROUND_UP_ULL / do_div would truncate the divisor to u32. Use a >>>> + * full 64/64 division. >>>> + */ >>>> + return DIV64_U64_ROUND_UP(num, den); >>>> +} >>>> + >>>> +static u32 imx908_calc_min_vblank(const struct v4l2_rect *crop) >>>> +{ >>>> + u32 min_vmax = max_t(u32, crop->height + IMX908_VMAX_MARGIN, >>>> + IMX908_VMAX_MIN); >>> >>> Alignment. >>> >>>> + >>>> + return min_vmax - crop->height; >>>> +} >>>> + >>>> +static int imx908_set_exposure_lines(struct imx908 *imx, u32 exposure_lines) >>>> +{ >>>> + const struct v4l2_mbus_framefmt *format; >>>> + struct v4l2_subdev_state *state; >>>> + u32 vmax, max_lines; >>>> + >>>> + state = v4l2_subdev_get_locked_active_state(&imx->sd); >>>> + format = v4l2_subdev_state_get_format(state, IMX908_SOURCE_PAD); >>>> + >>>> + vmax = format->height + imx->ctrls.vblank->val; >>>> + max_lines = vmax - IMX908_MIN_SHR0; >>>> + >>>> + /* Guards SHR0 underflow when VBLANK+EXPOSURE are set together */ >>>> + exposure_lines = clamp_t(u32, exposure_lines, 1, max_lines); >>>> + >>>> + u32 shr0 = vmax - exposure_lines; >>>> + >>>> + return cci_write(imx->cci, IMX908_REG_SHR0, shr0, NULL); >>>> +} >>>> + >>>> +static u8 imx908_bits_per_pixel(u32 code) >>>> +{ >>>> + switch (code) { >>>> + case MEDIA_BUS_FMT_SRGGB12_1X12: >>>> + return 12; >>>> + case MEDIA_BUS_FMT_SRGGB10_1X10: >>>> + default: >>>> + return 10; >>>> + } >>>> +} >>>> + >>>> +static u32 imx908_hmax_to_hblank(u16 hmax, u32 width) >>>> +{ >>>> + return hmax * IMX908_PIX_PER_CLK - width; >>>> +} >>>> + >>>> +static int imx908_update_hblank_limits(struct imx908 *imx, u32 old_width, >>>> + u32 width, u8 bpp) >>>> +{ >>>> + u16 min_hmax = imx908_calc_min_hmax(imx, bpp); >>>> + u32 max_hblank = imx908_hmax_to_hblank(IMX908_HMAX_MAX, width); >>>> + u32 min_hblank = imx908_hmax_to_hblank(min_hmax, width); >>>> + /* Re-derive the line period at the old width to preserve line time */ >>>> + u16 hmax = imx908_calc_hmax(old_width, imx->ctrls.hblank->val); >>>> + u32 hblank = imx908_hmax_to_hblank(hmax, width); >>>> + int ret; >>>> + >>>> + hblank = clamp_t(u32, hblank, min_hblank, max_hblank); >>>> + >>>> + ret = __v4l2_ctrl_modify_range(imx->ctrls.hblank, min_hblank, >>>> + max_hblank, IMX908_PIX_PER_CLK, hblank); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + if (imx->ctrls.hblank->val != hblank) >>>> + __v4l2_ctrl_s_ctrl(imx->ctrls.hblank, hblank); >>> >>> __v4l2_ctrl_s_ctrl() can fail, please return the error here. >>> >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int imx908_update_vblank_limits(struct imx908 *imx, >>>> + const struct v4l2_rect *crop, >>>> + u32 old_height) >>>> +{ >>>> + u32 min_vblank = imx908_calc_min_vblank(crop); >>>> + u32 max_vblank = IMX908_VMAX_MAX - crop->height; >>>> + /* Preserve frame length across height changes when possible. */ >>>> + u32 vmax = old_height + imx->ctrls.vblank->val; >>>> + u32 vblank = clamp_t(u32, vmax - crop->height, >>>> + min_vblank, max_vblank); >>>> + int ret; >>>> + >>>> + ret = __v4l2_ctrl_modify_range(imx->ctrls.vblank, min_vblank, >>>> + max_vblank, 1, vblank); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + if (imx->ctrls.vblank->val != vblank) >>>> + __v4l2_ctrl_s_ctrl(imx->ctrls.vblank, vblank); >>> >>> Ditto. >>> >>> You should omit the check btw.; this is something the control framework >>> does. Same in the above function. >>> >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +/* ------------------ Pattern Generator (TPG) ----------------------- */ >>>> +static const char * const imx908_tpg_menu[] = { >>>> + "Disabled", >>>> + "All 000h", >>>> + "All FFFh", >>>> + "All 555h", >>>> + "All AAAh", >>>> + "555h / AAAh Toggle", >>>> + "AAAh / 555h Toggle", >>>> + "000h / 555h Toggle", >>>> + "555h / 000h Toggle", >>>> + "000h / FFFh Toggle", >>>> + "FFFh / 000h Toggle", >>>> + "Horiz Color Bars", >>>> + "Vert Color Bars", >>>> +}; >>>> + >>>> +static int imx908_update_test_pattern(struct imx908 *imx, u32 index) >>>> +{ >>>> + int ret = 0; >>>> + >>>> + if (index == 0) { >>>> + /* Picture (not TPG) output setting */ >>>> + cci_update_bits(imx->cci, IMX908_REG_TPG_EN, IMX908_TPG_EN_BIT, >>>> + 0x00, &ret); >>>> + >>>> + /* Disable TESTCLKEN (write-only register) */ >>>> + cci_write(imx->cci, IMX908_REG_TESTCLKEN, 0x00, &ret); >>>> + >>>> + return ret; >>>> + } >>>> + >>>> + cci_write(imx->cci, IMX908_REG_TESTCLKEN, IMX908_TESTCLKEN_BIT, &ret); >>>> + cci_write(imx->cci, IMX908_REG_TPG_PATSEL, index - 1, &ret); >>>> + cci_update_bits(imx->cci, IMX908_REG_TPG_EN, IMX908_TPG_EN_BIT, >>>> + IMX908_TPG_EN_BIT, &ret); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int imx908_update_framing_limits(struct imx908 *imx, >>>> + struct v4l2_subdev_state *state, >>>> + u32 old_width, >>>> + u32 old_height) >>>> +{ >>>> + const struct v4l2_mbus_framefmt *fmt; >>>> + const struct v4l2_rect *crop; >>>> + int ret; >>>> + >>>> + fmt = v4l2_subdev_state_get_format(state, IMX908_SOURCE_PAD); >>>> + crop = v4l2_subdev_state_get_crop(state, IMX908_SOURCE_PAD); >>>> + u8 bpp = imx908_bits_per_pixel(fmt->code); >>>> + >>>> + ret = imx908_update_hblank_limits(imx, old_width, fmt->width, bpp); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = imx908_update_vblank_limits(imx, crop, old_height); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + u32 vmax = fmt->height + clamp_t(u32, imx->ctrls.vblank->val, >>>> + imx908_calc_min_vblank(crop), >>>> + IMX908_VMAX_MAX - fmt->height); >>>> + >>>> + return __v4l2_ctrl_modify_range(imx->ctrls.exposure, >>>> + IMX908_EXPOSURE_MIN, >>>> + vmax - IMX908_MIN_SHR0, >>>> + IMX908_EXPOSURE_STEP, >>>> + imx->ctrls.exposure->default_value); >>>> +} >>>> + >>>> +/* ----------------------- HW Programming ---------------------- */ >>>> +static int imx908_set_mode(struct imx908 *imx, >>>> + const struct v4l2_mbus_framefmt *fmt) >>>> +{ >>>> + u8 adbit, mdbit, value; >>> >>> As value is used for a set of registers, I'd call it accordingly, e.g. >>> bit_depth_value. >>> >>>> + u16 blklevel; >>>> + int ret = 0; >>>> + >>>> + switch (fmt->code) { >>>> + case MEDIA_BUS_FMT_SRGGB10_1X10: >>>> + adbit = IMX908_ADBIT_10BIT; >>>> + mdbit = IMX908_MDBIT_RAW10; >>>> + blklevel = 50; /* datasheet recommended value for 10-bit */ >>>> + value = 0x0c; /* register map value for 10-bit */ >>>> + break; >>>> + case MEDIA_BUS_FMT_SRGGB12_1X12: >>>> + default: >>>> + adbit = IMX908_ADBIT_12BIT; >>>> + mdbit = IMX908_MDBIT_RAW12; >>>> + blklevel = 200; /* datasheet recommended value for 12-bit */ >>>> + value = 0x05; /* register map value for 12-bit */ >>>> + break; >>>> + } >>>> + >>>> + cci_write(imx->cci, IMX908_REG_XMASTER, IMX908_CONTROLLER_MODE, &ret); >>>> + cci_write(imx->cci, IMX908_REG_ADBIT, adbit, &ret); >>>> + cci_write(imx->cci, IMX908_REG_MDBIT, mdbit, &ret); >>>> + >>>> + for (unsigned int i = 0; i < ARRAY_SIZE(imx908_bit_depth_regs); ++i) >>>> + cci_write(imx->cci, imx908_bit_depth_regs[i], value, &ret); >>>> + >>>> + cci_write(imx->cci, IMX908_REG_INCK_SEL, imx->inck_sel, &ret); >>>> + cci_write(imx->cci, IMX908_REG_DATARATE_SEL, imx->link_freq_idx, &ret); >>>> + cci_write(imx->cci, IMX908_REG_LANEMODE, imx->num_lanes == 2 ? >>>> + IMX908_LANEMODE_2LANE : IMX908_LANEMODE_4LANE, &ret); >>>> + cci_write(imx->cci, IMX908_REG_BLKLEVEL, blklevel, &ret); >>>> + >>>> + if (ret) >>>> + dev_err(imx->dev, " register write failed: %d\n", ret); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int imx908_program_window(struct imx908 *imx, >>>> + const struct v4l2_rect *crop) >>>> +{ >>>> + bool all_pixel_mode = v4l2_rect_equal(crop, &imx908_active_area); >>>> + int ret = 0; >>>> + >>>> + cci_write(imx->cci, IMX908_REG_WINMODE, >>>> + all_pixel_mode ? IMX908_WINMODE_ALLPIX : IMX908_WINMODE_CROP, >>>> + &ret); >>>> + >>>> + if (!all_pixel_mode) { >>>> + cci_write(imx->cci, IMX908_REG_PIX_HST, crop->left, &ret); >>>> + cci_write(imx->cci, IMX908_REG_PIX_HWIDTH, crop->width, &ret); >>>> + cci_write(imx->cci, IMX908_REG_PIX_VST, crop->top, &ret); >>>> + cci_write(imx->cci, IMX908_REG_PIX_VWIDTH, crop->height, &ret); >>>> + } >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +/* ----------------------- Start/Stop streaming ---------------------- */ >>>> +static int imx908_start_streaming(struct imx908 *imx, >>>> + struct v4l2_subdev_state *sd_state) >>>> +{ >>>> + const struct v4l2_mbus_framefmt *format; >>>> + const struct v4l2_rect *crop; >>>> + int ret = 0; >>>> + >>>> + crop = v4l2_subdev_state_get_crop(sd_state, IMX908_SOURCE_PAD); >>>> + format = v4l2_subdev_state_get_format(sd_state, IMX908_SOURCE_PAD); >>>> + >>>> + cci_multi_reg_write(imx->cci, imx908_common_regs, >>>> + ARRAY_SIZE(imx908_common_regs), &ret); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = imx908_set_mode(imx, format); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = imx908_program_window(imx, crop); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = __v4l2_ctrl_handler_setup(imx->sd.ctrl_handler); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + cci_write(imx->cci, IMX908_REG_STANDBY, IMX908_STANDBY_CANCEL, &ret); >>>> + fsleep(24 * USEC_PER_MSEC); /* >=24 ms stabilization after standby cancel */ >>>> + >>>> + cci_write(imx->cci, IMX908_REG_XMSTA, IMX908_XMSTA_START, &ret); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int imx908_stop_streaming(struct imx908 *imx) >>>> +{ >>>> + int ret = 0; >>>> + >>>> + cci_write(imx->cci, IMX908_REG_STANDBY, IMX908_STANDBY_EN, &ret); >>>> + cci_write(imx->cci, IMX908_REG_XMSTA, IMX908_XMSTA_STOP, &ret); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +/* --------------------------- V4L2 controls ------------------------------ */ >>>> +static int imx908_set_ctrl(struct v4l2_ctrl *ctrl) >>>> +{ >>>> + struct imx908 *imx = container_of(ctrl->handler, struct imx908, >>>> + ctrls.handler); >>>> + const struct v4l2_mbus_framefmt *format; >>>> + struct v4l2_subdev_state *state; >>>> + int ret = 0; >>>> + >>>> + state = v4l2_subdev_get_locked_active_state(&imx->sd); >>>> + format = v4l2_subdev_state_get_format(state, IMX908_SOURCE_PAD); >>>> + >>>> + /* Update exposure control limits even if the sensor is not streaming */ >>>> + if (ctrl->id == V4L2_CID_VBLANK) { >>>> + const struct v4l2_rect *crop; >>>> + >>>> + crop = v4l2_subdev_state_get_crop(state, IMX908_SOURCE_PAD); >>>> + >>>> + u32 min_vblank = imx908_calc_min_vblank(crop); >>>> + u32 max_vblank = IMX908_VMAX_MAX - format->height; >>>> + u32 vblank = clamp_t(u32, ctrl->val, min_vblank, max_vblank); >>>> + >>>> + u32 vmax = format->height + vblank; >>>> + u32 max_exposure = vmax - IMX908_MIN_SHR0; >>>> + u32 current_exposure = clamp_t(u32, imx->ctrls.exposure->cur.val, >>>> + IMX908_EXPOSURE_MIN, max_exposure); >>>> + >>>> + ret = __v4l2_ctrl_modify_range(imx->ctrls.exposure, >>>> + IMX908_EXPOSURE_MIN, >>>> + max_exposure, >>>> + IMX908_EXPOSURE_STEP, >>>> + current_exposure); >>>> + if (ret) >>>> + return ret; >>>> + } >>>> + >>>> + /* Hardware writes only when powered; cached ctrls applied on resume */ >>>> + if (!pm_runtime_get_if_active(imx->dev)) >>>> + return 0; >>>> + >>>> + switch (ctrl->id) { >>>> + case V4L2_CID_EXPOSURE: >>>> + ret = imx908_set_exposure_lines(imx, ctrl->val); >>>> + break; >>>> + >>>> + case V4L2_CID_ANALOGUE_GAIN: >>>> + cci_write(imx->cci, IMX908_REG_GAIN, ctrl->val, &ret); >>>> + break; >>>> + >>>> + case V4L2_CID_VBLANK: { >>>> + u32 vmax = format->height + ctrl->val; >>>> + >>>> + ret = cci_write(imx->cci, IMX908_REG_VMAX, vmax, NULL); >>>> + /* SHR0 derived from VMAX, re-apply exposure after changes */ >>>> + if (!ret) >>>> + ret = imx908_set_exposure_lines(imx, >>>> + imx->ctrls.exposure->val); >>>> + break; >>>> + } >>>> + >>>> + case V4L2_CID_HBLANK: { >>>> + u16 hmax = imx908_calc_hmax(format->width, ctrl->val); >>>> + >>>> + cci_write(imx->cci, IMX908_REG_HMAX, hmax, &ret); >>>> + break; >>>> + } >>>> + >>>> + case V4L2_CID_TEST_PATTERN: >>>> + ret = imx908_update_test_pattern(imx, ctrl->val); >>>> + break; >>>> + >>>> + case V4L2_CID_HFLIP: >>>> + cci_write(imx->cci, IMX908_REG_HREVERSE, >>>> + ctrl->val ? IMX908_HREVERSE_INV >>>> + : IMX908_HREVERSE_NORMAL, &ret); >>>> + break; >>>> + >>>> + case V4L2_CID_VFLIP: >>>> + cci_write(imx->cci, IMX908_REG_VREVERSE, >>>> + ctrl->val ? IMX908_VREVERSE_INV >>>> + : IMX908_VREVERSE_NORMAL, &ret); >>>> + break; >>>> + >>>> + default: >>>> + dev_warn(imx->dev, >>>> + "ctrl(id:0x%x,val:0x%x) is not handled\n", >>>> + ctrl->id, ctrl->val); >>>> + break; >>>> + } >>>> + >>>> + pm_runtime_put(imx->dev); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static const struct v4l2_ctrl_ops imx908_ctrl_ops = { >>>> + .s_ctrl = imx908_set_ctrl, >>>> +}; >>>> + >>>> +/* ------------------------ Pad/video ops ------------------ */ >>>> +static int imx908_enum_mbus_code(struct v4l2_subdev *sd, >>>> + struct v4l2_subdev_state *sd_state, >>>> + struct v4l2_subdev_mbus_code_enum *code) >>>> +{ >>>> + if (code->index >= ARRAY_SIZE(imx908_mbus_codes)) >>>> + return -EINVAL; >>> >>> Maybe a newline here? >>> >>>> + code->code = imx908_mbus_codes[code->index]; >>> >>> And here. >>> >>>> + return 0; >>>> +} >>>> + >>>> +static int imx908_enum_frame_size(struct v4l2_subdev *sd, >>>> + struct v4l2_subdev_state *sd_state, >>>> + struct v4l2_subdev_frame_size_enum *fse) >>>> +{ >>>> + struct imx908 *imx = to_imx908(sd); >>>> + const struct v4l2_rect *crop; >>>> + >>>> + if (fse->index > 0) >>>> + return -EINVAL; >>>> + >>>> + if (!imx908_mbus_code_supported(imx, fse->code)) >>>> + return -EINVAL; >>>> + >>>> + crop = v4l2_subdev_state_get_crop(sd_state, IMX908_SOURCE_PAD); >>>> + >>>> + fse->min_width = crop->width; >>>> + fse->max_width = crop->width; >>>> + fse->min_height = crop->height; >>>> + fse->max_height = crop->height; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int imx908_set_pad_format(struct v4l2_subdev *sd, >>>> + struct v4l2_subdev_state *sd_state, >>>> + struct v4l2_subdev_format *fmt) >>>> +{ >>>> + struct imx908 *imx = to_imx908(sd); >>>> + struct v4l2_mbus_framefmt *format; >>>> + >>>> + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE && >>>> + v4l2_subdev_is_streaming(sd)) >>>> + return -EBUSY; >>>> + >>>> + format = v4l2_subdev_state_get_format(sd_state, fmt->pad); >>>> + >>>> + if (imx908_mbus_code_supported(imx, fmt->format.code)) >>>> + format->code = fmt->format.code; >>>> + else >>>> + format->code = IMX908_DEFAULT_MBUS_CODE; >>>> + >>>> + /* Fully define metadata */ >>>> + format->field = V4L2_FIELD_NONE; >>>> + format->colorspace = V4L2_COLORSPACE_RAW; >>>> + format->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; >>>> + format->quantization = V4L2_QUANTIZATION_FULL_RANGE; >>>> + format->xfer_func = V4L2_XFER_FUNC_NONE; >>>> + >>>> + /* Limits track the real controls only for the ACTIVE state, not TRY */ >>>> + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) { >>>> + /* set_pad_format() never changes width, so old == new here */ >>>> + u32 old_width = format->width; >>>> + u32 old_height = format->height; >>>> + int ret = imx908_update_framing_limits(imx, sd_state, >>>> + old_width, old_height); >>>> + >>>> + if (ret) >>>> + return ret; >>>> + } >>>> + >>>> + fmt->format = *format; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int imx908_set_selection(struct v4l2_subdev *sd, >>>> + struct v4l2_subdev_state *sd_state, >>>> + struct v4l2_subdev_selection *sel) >>> >>> We don't really have cropping behaviour documented before the common raw >>> sensor model. I'd just postpone this until we have the common raw sensor >>> model support >>> merged. >> >> There's real progress on merging the raw camera sensor model, with Jai >> finalizing the implementation in libcamera. We could drop crop support >> for the initial version of the IMX908 driver or wait for the raw camera >> sensor model to be merged. The former is probably better to avoid >> delays. >> >> Lachlan, sorry about this last-minute request. Hopefully it will be >> quite simple to drop the .set_selection() handler and hardcode full >> resolution. imx908_update_framing_limits() will then have no user and >> could be dropped too. We'll add it back one kernel version later by >> adding crop support based on the raw camera sensor model. > > Actually imx908_update_framing_limits() is still needed in > imx908_set_pad_format() account for the format and bpp change, so you > can just drop imx908_set_selection() and hardcode the width and height. I removed the framing-limits wrapper in v4, but retained its bit-depth-dependent HBLANK update directly in imx908_set_pad_format(). With dimensions fixed, VBLANK limits remain constant, and exposure limits are still updated when VBLANK changes. >>>> +{ >>>> + struct imx908 *imx = to_imx908(sd); >>>> + struct v4l2_mbus_framefmt *format; >>>> + struct v4l2_rect *r = &sel->r; >>>> + struct v4l2_rect *crop; >>>> + >>>> + if (sel->target != V4L2_SEL_TGT_CROP) >>>> + return -EINVAL; >>>> + >>>> + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE && >>>> + v4l2_subdev_is_streaming(sd)) >>>> + return -EBUSY; >>>> + >>>> + crop = v4l2_subdev_state_get_crop(sd_state, sel->pad); >>>> + format = v4l2_subdev_state_get_format(sd_state, sel->pad); >>>> + >>>> + /* >>>> + * Round the crop rectangle size down to the hardware alignment >>>> + * constraints, then clamp it to the supported size range. >>>> + */ >>>> + r->width = clamp_t(s32, ALIGN_DOWN(r->width, IMX908_CROP_ALIGN_HWIDTH), >>>> + IMX908_CROP_MIN_WIDTH, imx908_active_area.width); >>>> + r->height = clamp_t(s32, ALIGN_DOWN(r->height, IMX908_CROP_ALIGN_VWIDTH), >>>> + IMX908_CROP_MIN_HEIGHT, imx908_active_area.height); >>>> + >>>> + /* >>>> + * Round and clamp the crop position similarly, with the maximum value >>>> + * chosen so that the crop rectangle remains fully inside the active >>>> + * pixel array. >>>> + */ >>>> + r->left = clamp_t(s32, ALIGN_DOWN(r->left, IMX908_CROP_ALIGN_HSTART), 0, >>>> + imx908_active_area.width - r->width); >>>> + r->top = clamp_t(s32, ALIGN_DOWN(r->top, IMX908_CROP_ALIGN_VSTART), 0, >>>> + imx908_active_area.height - r->height); >>>> + >>>> + u32 old_width = crop->width; >>>> + u32 old_height = crop->height; >>>> + >>>> + *crop = *r; >>>> + >>>> + /* IMX908 has no binning, so the output size matches the crop 1:1 */ >>>> + format->width = crop->width; >>>> + format->height = crop->height; >>>> + >>>> + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) { >>>> + int ret = imx908_update_framing_limits(imx, sd_state, >>>> + old_width, old_height); >>>> + >>>> + if (ret) >>>> + return ret; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int imx908_get_selection(struct v4l2_subdev *sd, >>>> + struct v4l2_subdev_state *sd_state, >>>> + struct v4l2_subdev_selection *sel) >>>> +{ >>>> + switch (sel->target) { >>>> + case V4L2_SEL_TGT_CROP: >>>> + sel->r = *v4l2_subdev_state_get_crop(sd_state, >>>> + IMX908_SOURCE_PAD); >>>> + return 0; >>>> + >>>> + case V4L2_SEL_TGT_NATIVE_SIZE: >>>> + sel->r = imx908_total_area; >>>> + return 0; >>>> + >>>> + case V4L2_SEL_TGT_CROP_DEFAULT: >>>> + sel->r = imx908_active_area; >>>> + return 0; >>>> + >>>> + case V4L2_SEL_TGT_CROP_BOUNDS: >>> >>> You can combine with with the above case. >>> >>>> + sel->r = imx908_active_area; >>>> + return 0; >>>> + >>>> + default: >>>> + return -EINVAL; >>>> + } >>>> +} >>>> + >>>> +static int imx908_enable_streams(struct v4l2_subdev *sd, >>>> + struct v4l2_subdev_state *sd_state, >>>> + u32 pad, >>>> + u64 streams_mask) >>>> +{ >>>> + struct imx908 *imx = to_imx908(sd); >>>> + int ret; >>>> + >>>> + ret = pm_runtime_resume_and_get(imx->dev); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = imx908_start_streaming(imx, sd_state); >>>> + if (ret) { >>>> + pm_runtime_put_autosuspend(imx->dev); >>>> + return ret; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int imx908_disable_streams(struct v4l2_subdev *sd, >>>> + struct v4l2_subdev_state *sd_state, >>>> + u32 pad, >>>> + u64 streams_mask) >>>> +{ >>>> + struct imx908 *imx = to_imx908(sd); >>>> + int ret; >>>> + >>>> + ret = imx908_stop_streaming(imx); >>> >>> imx908_stop_streaming() is used in a single location only. I think you >>> should move the code here. Same for imx908_enable_streams() and >>> imx906_start_streaming(), too. >> >> For imx908_start_streaming() it could make error handling a tiny bit >> more complex. This is the kind of detail I'd leave to the appreciation >> of the driver author. >> >>>> + >>>> + pm_runtime_put_autosuspend(imx->dev); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int imx908_init_state(struct v4l2_subdev *sd, >>>> + struct v4l2_subdev_state *sd_state) >>>> +{ >>>> + struct v4l2_subdev_selection sel = { >>>> + .which = V4L2_SUBDEV_FORMAT_TRY, >>>> + .pad = IMX908_SOURCE_PAD, >>>> + .target = V4L2_SEL_TGT_CROP, >>>> + .r = imx908_active_area, >>>> + }; >>>> + struct v4l2_subdev_format fmt = { >>>> + .which = V4L2_SUBDEV_FORMAT_TRY, >>>> + .pad = IMX908_SOURCE_PAD, >>>> + .format = { >>>> + .code = IMX908_DEFAULT_MBUS_CODE, >>>> + .width = imx908_active_area.width, >>>> + .height = imx908_active_area.height, >>>> + }, >>>> + }; >>>> + >>>> + imx908_set_selection(sd, sd_state, &sel); >>>> + imx908_set_pad_format(sd, sd_state, &fmt); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static const struct v4l2_subdev_video_ops imx908_video_ops = { >>>> + .s_stream = v4l2_subdev_s_stream_helper, >>>> +}; >>>> + >>>> +static const struct v4l2_subdev_pad_ops imx908_pad_ops = { >>>> + .enum_mbus_code = imx908_enum_mbus_code, >>>> + .enum_frame_size = imx908_enum_frame_size, >>>> + .get_fmt = v4l2_subdev_get_fmt, >>>> + .set_fmt = imx908_set_pad_format, >>>> + .get_selection = imx908_get_selection, >>>> + .set_selection = imx908_set_selection, >>>> + .enable_streams = imx908_enable_streams, >>>> + .disable_streams = imx908_disable_streams, >>>> +}; >>>> + >>>> +static const struct v4l2_subdev_internal_ops imx908_internal_ops = { >>>> + .init_state = imx908_init_state, >>>> +}; >>>> + >>>> +static const struct v4l2_subdev_ops imx908_subdev_ops = { >>>> + .video = &imx908_video_ops, >>>> + .pad = &imx908_pad_ops, >>>> +}; >>>> + >>>> +/* ----------------------- Power management ---------------------- */ >>>> +static int imx908_power_on(struct device *dev) >>>> +{ >>>> + struct v4l2_subdev *sd = dev_get_drvdata(dev); >>>> + struct imx908 *imx = to_imx908(sd); >>>> + int ret; >>>> + >>>> + ret = regulator_bulk_enable(ARRAY_SIZE(imx908_supply_names), >>>> + imx->supplies); >>>> + if (ret) { >>>> + dev_err(imx->dev, "failed to enable regulators\n"); >>>> + return ret; >>>> + } >>>> + /* XCLR already asserted by GPIOD_OUT_HIGH */ >>>> + udelay(1); /* >= 500ns T_low */ >>>> + gpiod_set_value_cansleep(imx->reset_gpio, 0); /* Sensor start */ >>>> + >>>> + ret = clk_prepare_enable(imx->xclk); >>>> + if (ret) { >>>> + dev_err(imx->dev, "failed to enable xclk: %d\n", ret); >>>> + goto err_reset; >>>> + } >>>> + >>>> + fsleep(20); /* T_1 >= 20us before first SDA/SCL */ >>>> + >>>> + return 0; >>>> + >>>> +err_reset: >>>> + gpiod_set_value_cansleep(imx->reset_gpio, 1); /* assert reset */ >>>> + regulator_bulk_disable(ARRAY_SIZE(imx908_supply_names), imx->supplies); >>>> + return ret; >>>> +} >>>> + >>>> +static void imx908_power_off(struct device *dev) >>>> +{ >>>> + struct v4l2_subdev *sd = dev_get_drvdata(dev); >>>> + struct imx908 *imx = to_imx908(sd); >>>> + >>>> + clk_disable_unprepare(imx->xclk); >>>> + gpiod_set_value_cansleep(imx->reset_gpio, 1); >>>> + regulator_bulk_disable(ARRAY_SIZE(imx908_supply_names), imx->supplies); >>>> +} >>>> + >>>> +static int imx908_runtime_resume(struct device *dev) >>>> +{ >>>> + return imx908_power_on(dev); >>> >>> Please move the code from imx908_power_on() here; similarly for >>> imx908_runtime_suspend() and imx908_power_off(). >>> >>>> +} >>>> + >>>> +static int imx908_runtime_suspend(struct device *dev) >>>> +{ >>>> + imx908_power_off(dev); >>>> + return 0; >>>> +} >>>> + >>>> +static DEFINE_RUNTIME_DEV_PM_OPS(imx908_pm_ops, >>>> + imx908_runtime_suspend, >>>> + imx908_runtime_resume, NULL); >>>> + >>>> +/* ------------------------------- Probe/remove --------------------------- */ >>>> + >>>> +static int imx908_get_inck_sel(struct imx908 *imx, u32 xclk_freq) >>>> +{ >>>> + for (unsigned int i = 0; i < ARRAY_SIZE(imx908_inck_table); i++) { >>>> + if (imx908_inck_table[i] == xclk_freq) { >>>> + imx->inck_sel = i; >>>> + return 0; >>>> + } >>>> + } >>>> + >>>> + return dev_err_probe(imx->dev, -EINVAL, "unsupported xclk %u Hz\n", >>>> + xclk_freq); >>>> +} >>>> + >>>> +static int imx908_get_regulators(struct imx908 *imx) >>>> +{ >>>> + for (unsigned int i = 0; i < ARRAY_SIZE(imx908_supply_names); i++) >>>> + imx->supplies[i].supply = imx908_supply_names[i]; >>>> + >>>> + return devm_regulator_bulk_get(imx->dev, >>>> + ARRAY_SIZE(imx908_supply_names), >>>> + imx->supplies); >>>> +} >>>> + >>>> +static int imx908_parse_fwnode(struct imx908 *imx) >>>> +{ >>>> + struct v4l2_fwnode_endpoint bus_cfg = { >>>> + .bus_type = V4L2_MBUS_CSI2_DPHY >>>> + }; >>>> + struct fwnode_handle *ep; >>>> + int ret = 0; >>>> + >>>> + ep = fwnode_graph_get_next_endpoint(dev_fwnode(imx->dev), NULL); >>>> + >>>> + /* Only data-lanes and link-frequencies are used from the endpoint */ >>> >>> This is trivially visible below, please drop the comment. >>> >>>> + ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg); >>>> + fwnode_handle_put(ep); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + imx->num_lanes = bus_cfg.bus.mipi_csi2.num_data_lanes; >>>> + >>>> + if (imx->num_lanes != 2 && imx->num_lanes != 4) { >>>> + dev_err(imx->dev, >>>> + "only 2 or 4 CSI-2 data lanes are supported (got %u)\n", >>>> + imx->num_lanes); >>>> + ret = -EINVAL; >>>> + goto out_free; >>>> + } >>>> + >>>> + ret = v4l2_link_freq_to_bitmap(imx->dev, >>>> + bus_cfg.link_frequencies, >>>> + bus_cfg.nr_of_link_frequencies, >>>> + imx908_link_freqs, >>>> + ARRAY_SIZE(imx908_link_freqs), >>>> + &imx->link_freq_bitmap); >>>> + if (ret) >>>> + goto out_free; >>>> + >>>> + imx->link_freq_idx = __ffs(imx->link_freq_bitmap); >>>> + dev_dbg(imx->dev, "using %u lanes at link freq %llu Hz\n", >>>> + imx->num_lanes, imx908_link_freqs[imx->link_freq_idx]); >>> >>> This information is already printed by v4l2-fwnode framework, please drop. >>> >>>> + >>>> +out_free: >>>> + v4l2_fwnode_endpoint_free(&bus_cfg); >>>> + return ret; >>>> +} >>>> + >>>> +static int imx908_init_controls(struct imx908 *imx) >>>> +{ >>>> + struct v4l2_ctrl_handler *hdl = &imx->ctrls.handler; >>>> + struct v4l2_fwnode_device_properties props; >>>> + struct v4l2_ctrl *ctrl; >>>> + int ret; >>>> + >>>> + /* Read rotation and orientation properties from the firmware node */ >>>> + ret = v4l2_fwnode_device_parse(imx->dev, &props); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = v4l2_ctrl_handler_init(hdl, 11); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + /* PIXEL_RATE is always read-only, so it needs no s_ctrl() handling */ >>>> + v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE, >>>> + IMX908_PIXEL_RATE, IMX908_PIXEL_RATE, 1, >>>> + IMX908_PIXEL_RATE); >>>> + >>>> + /* LINK_FREQ is informational only and has no s_ctrl() handling */ >>>> + ctrl = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ, >>>> + ARRAY_SIZE(imx908_link_freqs) - 1, >>>> + imx->link_freq_idx, imx908_link_freqs); >>>> + if (ctrl) >>>> + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; >>>> + >>>> + u32 min_vblank = imx908_calc_min_vblank(&imx908_active_area); >>>> + u32 max_vblank = IMX908_VMAX_MAX - imx908_active_area.height; >>>> + /* Default to the datasheet 30 fps operating point */ >>>> + u32 def_vblank = IMX908_VMAX_DEFAULT - imx908_active_area.height; >>>> + >>>> + imx->ctrls.vblank = v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops, >>>> + V4L2_CID_VBLANK, min_vblank, >>>> + max_vblank, 1, def_vblank); >>>> + >>>> + u8 bpp = imx908_bits_per_pixel(IMX908_DEFAULT_MBUS_CODE); >>>> + u16 min_hmax = imx908_calc_min_hmax(imx, bpp); >>>> + u32 min_hblank = imx908_hmax_to_hblank(min_hmax, >>>> + imx908_active_area.width); >>>> + u32 max_hblank = imx908_hmax_to_hblank(IMX908_HMAX_MAX, >>>> + imx908_active_area.width); >>>> + u32 hblank = imx908_hmax_to_hblank(IMX908_HMAX_DEFAULT, >>>> + imx908_active_area.width); >>>> + >>>> + /* Default HMAX can be infeasible at low link freqs; clamp into range */ >>>> + hblank = clamp_t(u32, hblank, min_hblank, max_hblank); >>>> + >>>> + imx->ctrls.hblank = v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops, >>>> + V4L2_CID_HBLANK, min_hblank, >>>> + max_hblank, IMX908_PIX_PER_CLK, >>>> + hblank); >>>> + u32 max_exp = IMX908_VMAX_DEFAULT - IMX908_MIN_SHR0; >>>> + >>>> + imx->ctrls.exposure = v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops, >>>> + V4L2_CID_EXPOSURE, >>>> + IMX908_EXPOSURE_MIN, >>>> + max_exp, >>>> + IMX908_EXPOSURE_STEP, >>>> + max_exp / 2); >>>> + >>>> + v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops, V4L2_CID_ANALOGUE_GAIN, >>>> + IMX908_ANA_GAIN_MIN, IMX908_ANA_GAIN_MAX, >>>> + IMX908_ANA_GAIN_STEP, IMX908_ANA_GAIN_DEFAULT); >>>> + >>>> + /* Set test pattern. Menu (13 entries: Disabled + 12 patterns) */ >>>> + v4l2_ctrl_new_std_menu_items(hdl, &imx908_ctrl_ops, >>>> + V4L2_CID_TEST_PATTERN, >>>> + ARRAY_SIZE(imx908_tpg_menu) - 1, 0, 0, >>>> + imx908_tpg_menu); >>>> + >>>> + v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops, V4L2_CID_HFLIP, 0, 1, 1, 0); >>>> + v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops, V4L2_CID_VFLIP, 0, 1, 1, 0); >>>> + >>>> + v4l2_ctrl_new_fwnode_properties(hdl, &imx908_ctrl_ops, &props); >>>> + if (hdl->error) { >>>> + ret = hdl->error; >>> >>> No need to assign ret, just return hdl->error below. >> >> Or better, >> >> if (hdl->error) >> return v4l2_ctrl_handler_free(hdl); >> >> here and drop the error label. Commit 04f541cef2db ("media: v4l2-ctrls: >> Return the handler's error in v4l2_ctrl_handler_free()") recently made >> that possible. >> >>>> + goto err_free; >>>> + } >>>> + >>>> + imx->sd.ctrl_handler = hdl; >>>> + >>>> + return 0; >>>> + >>>> +err_free: >>>> + v4l2_ctrl_handler_free(hdl); >>>> + return ret; >>>> +} >>>> + >>>> +static int imx908_identify_model(struct imx908 *imx) >>>> +{ >>>> + u16 chip_id; >>>> + u64 val; >>>> + int ret; >>>> + >>>> + /* >>>> + * The TYPE_ID registers are not accessible after power-up while >>>> + * the device remains in standby. Exit standby and wait for the >>>> + * required stabilization period before reading the chip ID. >>>> + */ >>>> + ret = cci_write(imx->cci, IMX908_REG_STANDBY, IMX908_STANDBY_CANCEL, NULL); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + fsleep(24 * USEC_PER_MSEC); /* >=24 ms stabilization after standby cancel */ >>>> + >>>> + ret = cci_read(imx->cci, IMX908_REG_TYPE_ID, &val, NULL); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + chip_id = val; >>>> + dev_dbg(imx->dev, "IMX908 chip ID: 0x%04x\n", chip_id); >>>> + >>>> + if (chip_id != IMX908_CHIP_ID) { >>>> + dev_err(imx->dev, "unexpected chip ID 0x%04x (expected 0x%04x)\n", >>>> + chip_id, IMX908_CHIP_ID); >>>> + return -ENXIO; >>>> + } >>>> + >>>> + /* Set to standby mode to save power */ >>>> + ret = cci_write(imx->cci, IMX908_REG_STANDBY, IMX908_STANDBY_EN, NULL); >>>> + if (ret) { >>>> + dev_err(imx->dev, "failed to enter standby state: %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int imx908_probe(struct i2c_client *client) >>>> +{ >>>> + struct imx908 *imx; >>>> + int ret; >>>> + >>>> + imx = devm_kzalloc(&client->dev, sizeof(*imx), GFP_KERNEL); >>>> + if (!imx) >>>> + return -ENOMEM; >>>> + imx->dev = &client->dev; >>>> + >>>> + v4l2_i2c_subdev_init(&imx->sd, client, &imx908_subdev_ops); >>>> + imx->sd.internal_ops = &imx908_internal_ops; >>>> + >>>> + imx->cci = devm_cci_regmap_init_i2c(client, 16); >>>> + if (IS_ERR(imx->cci)) >>>> + return dev_err_probe(imx->dev, PTR_ERR(imx->cci), >>>> + "CCI regmap init failed\n"); >>>> + >>>> + imx->xclk = devm_v4l2_sensor_clk_get(imx->dev, NULL); >>>> + if (IS_ERR(imx->xclk)) >>>> + return dev_err_probe(imx->dev, PTR_ERR(imx->xclk), >>>> + "failed to get clock\n"); >>>> + >>>> + ret = imx908_get_inck_sel(imx, clk_get_rate(imx->xclk)); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + imx->reset_gpio = devm_gpiod_get_optional(imx->dev, "reset", >>>> + GPIOD_OUT_HIGH); >>>> + >>>> + if (IS_ERR(imx->reset_gpio)) >>>> + return dev_err_probe(imx->dev, PTR_ERR(imx->reset_gpio), >>>> + "failed to get reset gpio\n"); >>>> + >>>> + ret = imx908_get_regulators(imx); >>>> + if (ret) >>>> + return dev_err_probe(imx->dev, ret, "failed to get regulators\n"); >>>> + >>>> + ret = imx908_parse_fwnode(imx); >>>> + if (ret) >>>> + return dev_err_probe(imx->dev, ret, "device tree parse failed\n"); >>>> + >>>> + ret = imx908_power_on(imx->dev); >>>> + if (ret) >>>> + return dev_err_probe(imx->dev, ret, "power-on failed\n"); >>>> + >>>> + ret = imx908_identify_model(imx); >>>> + if (ret) { >>>> + dev_err(imx->dev, "failed to identify model: %d\n", ret); >>>> + goto err_power_off; >>>> + } >>>> + >>>> + /* Device is powered; keep it resumed across registration, release at end */ >>>> + pm_runtime_set_active(imx->dev); >>>> + pm_runtime_get_noresume(imx->dev); >>>> + ret = devm_pm_runtime_enable(imx->dev); >>> >>> What will happen on error here? PM runtime will be disabled after probe() >>> exits on error, but you'd need to set the state to suspended after that. >>> I'm not sure you can correctly use this as things stand currently. This >>> also conflicts currently with remove() below disabling Runtime PM >>> explicitly. >> >> I'm not sure to understand what's requested here. Are you saying there's >> a potential issue that can't be fixed currently ? Or should this code be >> modified ? In the latter case, could you please tell what should be done >> ? >> >>>> + if (ret) >>>> + goto err_pm_put; >>>> + >>>> + pm_runtime_set_autosuspend_delay(imx->dev, 1000); >>>> + pm_runtime_use_autosuspend(imx->dev); >>> >>> I'd move this after registering the sensor sub-device. >>> >>>> + >>>> + ret = imx908_init_controls(imx); >>>> + if (ret) >>>> + goto err_pm_put; >>>> + >>>> + imx->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; >>>> + imx->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; >>>> + imx->pad.flags = MEDIA_PAD_FL_SOURCE; >>>> + ret = media_entity_pads_init(&imx->sd.entity, 1, &imx->pad); >>>> + if (ret) >>>> + goto err_hdl; >>>> + >>>> + /* Share the ctrl handler lock so s_ctrl can access the locked state */ >>>> + imx->sd.state_lock = imx->ctrls.handler.lock; >>>> + >>>> + ret = v4l2_subdev_init_finalize(&imx->sd); >>>> + if (ret) >>>> + goto err_entity; >>>> + >>>> + ret = v4l2_async_register_subdev_sensor(&imx->sd); >>>> + if (ret) >>>> + goto err_subdev; >>>> + >>>> + /* Balance get_noresume; allow the sensor to suspend after probe */ >>>> + pm_runtime_put_autosuspend(imx->dev); >>> >>> Please use pm_runtime_idle() here instead as I requested in review of v2. >>> >>>> + >>>> + return 0; >>>> + >>>> +err_subdev: >>>> + v4l2_subdev_cleanup(&imx->sd); >>>> + >>>> +err_entity: >>>> + media_entity_cleanup(&imx->sd.entity); >>>> + >>>> +err_hdl: >>>> + v4l2_ctrl_handler_free(imx->sd.ctrl_handler); >>>> + >>>> +err_pm_put: >>>> + pm_runtime_put_noidle(imx->dev); >>>> + >>>> +err_power_off: >>>> + imx908_power_off(imx->dev); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static void imx908_remove(struct i2c_client *client) >>>> +{ >>>> + struct v4l2_subdev *sd = i2c_get_clientdata(client); >>>> + struct imx908 *imx = to_imx908(sd); >>>> + >>>> + v4l2_async_unregister_subdev(sd); >>>> + v4l2_subdev_cleanup(sd); >>>> + media_entity_cleanup(&sd->entity); >>>> + v4l2_ctrl_handler_free(sd->ctrl_handler); >>>> + >>> pm_runtime_dont_use_autosuspend(); >>> >>> ? >>> >>>> + pm_runtime_disable(imx->dev); >>>> + if (!pm_runtime_status_suspended(imx->dev)) >>>> + imx908_power_off(imx->dev); >>>> + pm_runtime_set_suspended(imx->dev); >>> >>> This needs to be conditional to status being suspended, too. >>> >>>> +} >>>> + >>>> +static const struct of_device_id imx908_of_ids[] = { >>>> + { .compatible = "sony,imx908" }, >>>> + { /* sentinel */ } >>>> +}; >>>> +MODULE_DEVICE_TABLE(of, imx908_of_ids); >>>> + >>>> +static struct i2c_driver imx908_i2c_driver = { >>>> + .driver = { >>>> + .name = "imx908", >>>> + .pm = pm_ptr(&imx908_pm_ops), >>>> + .of_match_table = imx908_of_ids, >>>> + }, >>>> + .probe = imx908_probe, >>>> + .remove = imx908_remove, >>>> +}; >>>> +module_i2c_driver(imx908_i2c_driver); >>>> + >>>> +MODULE_AUTHOR("Lachlan Michael "); >>>> +MODULE_DESCRIPTION("Sony IMX908 Sensor Driver"); >>>> +MODULE_LICENSE("GPL"); >