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 8933D4195A0; Thu, 1 Oct 2026 08:36:36 +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=1790843801; cv=none; b=XKrcPjbL/bKjNVeVPBsmcjMmMWuS7uLgSzp++KrBqO7U3G2YiNGie9Yb4RHmw0PpUmKukpD7MrjHQDwxgtfXgOuDpvy4aos0TJOi0hUF9CcAO8wCFSdteUatf9FmPxc1ffRAgMim4wPEqmn++ZTlO/rzyhDSsop6v/BUV5zmSrs= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790843801; c=relaxed/simple; bh=1mcgHJ9P5dGTNcMIVKFpLUdJg3J8PUjUEjWuPAqt2ls=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=Z+kL83TR2x6/gHfzwm0Q2nVrb4X7HM6y31jCMwENxbGT2VT7jkL3EuGYO0Q5/nUcafArXcqL5vlzOKCJLSjcxKFb2oFTYSUIOThJOAuSam4b/S4iV2VPxPnKDHsxh6yfA997NUkfbgffKYDea0hkX3o2Gz24xiG/ocgZp6YikAU= 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=dQxA2vCq; 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="dQxA2vCq" Received: from pps.filterd (m0209328.ppops.net [127.0.0.1]) by mx08-001d1705.pphosted.com (8.18.1.11/8.18.1.11) with ESMTP id 6917GZ7g3799244; Thu, 1 Oct 2026 08:36:13 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=ioEPgeP 8H9iq+hL29PvdCiPBv1RwwYba9IkUteaojOI=; b=dQxA2vCqVpIbQGgOZdD9wGA RWHHfzTvxkXTTakhi39Rn+wg9CenWaas+Jw9M7f1VUjsLjzdjgujFRjBsEShMuoc iJezERQpJT5fwd0Y2EsVmkaIMOi1RK+mLl8pjVnuh+lEfkrjyakUW/RRdF8bDiHD TXX3b7NbAq405J9Jk/Tm9Tl+twndtILgU4hSvfAjd0gc7SC3MPgOBJDbI4of6Wiu lhpIJX8aMDsOFlIJ7VWrS9SjzN1+hGKgQYdjFdafHZKDlsT9Yqsv2Rc0yp8zYH0w oA/0EK4eaL5RTn39KK7u90VyUT1dvqMLl1u1PG6ybFBwaNFMGqoP6Fn8+iz62rw= = Received: from sgppsem01v.sg.gdce.sony.com.sg ([121.100.43.221]) by mx08-001d1705.pphosted.com (PPS) with ESMTPS id 4gx77gxj0h-1 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NO); Thu, 01 Oct 2026 08:36:12 +0000 (GMT) Received: from pps.filterd (SGPPSEM01v.sg.gdce.sony.com.sg [127.0.0.1]) by SGPPSEM01v.sg.gdce.sony.com.sg (8.18.1.11/8.18.1.11) with ESMTP id 6918F69B3465036; Thu, 1 Oct 2026 08:36:10 GMT Received: from sgppsba03v.s.gdce.sony.com.sg ([146.215.113.220]) by SGPPSEM01v.sg.gdce.sony.com.sg (PPS) with ESMTPS id 4gx74hn3nj-1 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Thu, 01 Oct 2026 08:36:10 +0000 (GMT) Received: from pps.filterd (SGPPSBA03v.s.gdce.sony.com.sg [127.0.0.1]) by SGPPSBA03v.s.gdce.sony.com.sg (8.18.1.11/8.18.1.11) with ESMTP id 691711LV3750002; Thu, 1 Oct 2026 08:36:10 GMT Received: from [43.3.252.132] ([146.215.113.251]) by SGPPSBA03v.s.gdce.sony.com.sg (PPS) with ESMTPS id 4gx74gt6w4-1 (version=TLSv1.3 cipher=TLS_AES_128_GCM_SHA256 bits=128 verify=NOT); Thu, 01 Oct 2026 08:36:10 +0000 (GMT) Message-ID: <9c7e136c-f092-4a6b-ab1d-cbafdea26706@sony.com> Date: Thu, 1 Oct 2026 17:36:08 +0900 Precedence: bulk X-Mailing-List: linux-kernel@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: Jai Luthra , devicetree@vger.kernel.org, hverkuil+cisco@kernel.org, laurent.pinchart@ideasonboard.com, linux-media@vger.kernel.org, mchehab@kernel.org, sakari.ailus@linux.intel.com Cc: robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, kieran.bingham@ideasonboard.com, Ryuichi.Tadano@sony.com, Kengo.Hayasaka@sony.com, Tim Bird , Kazumi.A.Sato@sony.com, Yuji.John.Takahashi@sony.com, linux-kernel@vger.kernel.org References: <20260828064843.65047-1-lachlan.michael@sony.com> <20260828064843.65047-3-lachlan.michael@sony.com> <179000809637.3701737.17026819145311474886@freya> Content-Language: en-US From: Lachlan Michael Organization: Sony Semiconductor Solutions Corporation In-Reply-To: <179000809637.3701737.17026819145311474886@freya> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Proofpoint-Reinject: loops=2 maxloops=12 X-Sony-BusinessRelay-GUID: aJ8mizSfFdCXomOKvZiOEwaonJWt5-ez 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-01_03,2026-09-21_02,2025-10-01_01 X-Sony-EdgeRelay-GUID: 8Woh32PWLEbb6XfLf6z2nQa1dQZtczNP 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-01_03,2026-09-21_02,2025-10-01_01 X-Proofpoint-GUID: AUgRh_vxZAteA4qeGQtp4UrXF4iUFTVP X-Proofpoint-ORIG-GUID: AUgRh_vxZAteA4qeGQtp4UrXF4iUFTVP X-Proofpoint-Spam-Info: AW1haW4tMjYxMDAxMDAzNCBTYWx0ZWRfX4dD/tAdOvF3C Sigghsy6J71FGlrNE7SMZoroxCm2Uzp+yyBoOs66HU05si9XYSAwErdcqUy9JD5so1ZrbWxM1Vg FaFqTNo5QrJijJvidqjUYy85+Mld38Ypo9XpzBY4/Jz1I2XjUhSy X-Authority-Analysis: v=2.4 cv=cdpHPXDM c=1 sm=1 tr=0 ts=6abe1b7d cx=c_pps a=5iEaAFJ3UIynSlflvg1n/g==:117 a=5iEaAFJ3UIynSlflvg1n/g==:17 a=IkcTkHD0fZMA:10 a=660iZSQnnn4A:10 a=VkNPw1HP01LnGYTKEx00:22 a=KAb5x4SsHD3PzxGk7EmX:22 a=jHItnBwsj8f3cr25iGAb:22 a=z6gsHLkEAAAA:8 a=pGLkceISAAAA:8 a=llUcCH6CVj5xXF-wNYYA:9 a=3ZKOabzyN94A:10 a=QEXdDO2ut3YA:10 X-Proofpoint-Spam-Details-Enc: AW1haW4tMjYxMDAxMDAzNCBTYWx0ZWRfX5KOByo1u4YXA NrFFvjlVUTha/Iw4ZsKuVKuDvCWRQT+bNmkgOG4joV/Soy62yV56VTzi8VKeCVedfcFbZQsih+C E/hxQw4x5RStl+CaakiRSaZGyCh1KcNObxPlz95CmC8coOmgYQlX7mMc974OcpNFEVF6nNhlcMx 7jnPu0aVQEJG1XQBBAUSyxPuWTI9CXxRUlwJntbrqf+Xut71Z4ruJmsorZpbiQ+jVfgj5x4Xr/u LPsIUWD/2ozBNBHjl6jrlQIgdzk6T/XqaoZprJslcFOVMqvDPwaaLD9zYRv7l1kVZZEUAGcowSZ h9CBnWm+MghVyhpvsH13ltyX4AwLzyNC+yGvPb4nCWYglgEej64baSm/amJ/hnNh9ClSgcI61aJ ZPD8pJv//l1Y3BnevHZx+mKVAAeEA/mh1NOEQSi+v86pPJyI3Oom5btjVTyWKwzcm7U6b1+ahTi gPqlzlDdklotiOhO0rg== X-Sony-Outbound-GUID: AUgRh_vxZAteA4qeGQtp4UrXF4iUFTVP 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-01_03,2026-09-21_02,2025-10-01_01 Dear Jai, On 9/22/2026 1:28 AM, Jai Luthra wrote: > Hi Lachlan, Thank you for your patch. And thanks for fixing my comments > from v2. I have a few more, some I missed the last time around, but some > are new. Quoting Lachlan Michael (2026-08-28 12: 18: 43) > The Sony > IMX908 is an 8. 39 megapixel > Hi Lachlan, > > Thank you for your patch. > > And thanks for fixing my comments from v2. I have a few more, some I missed > the last time around, but some are new. Appreciate the time you have spent to review it. Since Laurent made some additional comments, for those I will reply to Laurent's e-mail. > Quoting Lachlan Michael (2026-08-28 12:18:43) >> 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, >> +}; >> + > > The "Pixel Arrangement" section in the datasheet for IMX678 (Starvis 2, > same resolution) mentioned a horizontal margin of 8 and 9, and vertical > margin of 10 + 10 + 4 + 8 and 9, thus the total came out to be 3857x2201 > > Which also explained why the CFA readout stays R->G->G->B irrespective of > VFLIP and HFLIP, the extra pixel allowed shifting the readout start. I re-checked the IMX908 datasheet under the Pixel Arrangement and I confirmed the current width and height are correct. In both the vertical and horizontal directions, the final "Effective margin for error processing" is written as 9 (pixels) in the diagram, however in the comments it says "The last effective line and column are not read out." If we represent the not reading out as a -1 the numbers work out. - the horizontal width is: 0+8+3840+9(-1)+0 = 3856 - the vertical height is: 10+10+4+8+2160+9(-1) = 2200 >> +static const struct v4l2_rect imx908_active_area = { >> + .top = 0, >> + .left = 0, >> + .width = 3856, >> + .height = 2176, > > Shouldn't this have .top = 24 (10 + 10 + 4)? > > At least on IMX678, while the diagram shows the OB area at bottom, that's > where the scan starts from (N12 pin) when HREVERSE/VREVERSE are 0. > > It would be good to confirm if IMX908 is indeed different in its behaviour > from Starvis2 sensors. You are right. I will change this to .top = 24, >> +}; >> + >> +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); >> + >> + 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); >> + >> + 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); >> + >> + 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; >> + 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); > > Has the BLKLEVEL register handling changed from Starvis2 -> Starvis3? > > For IMX678 the default value recommended by datasheet were the same, 50 and > 200, but with a note that increasing register by 1 in 12-bit mode meant an > increase of 4 in the output. Thus the register always reads 50 (or 0x32) by > default. In the IMX908 register map (excel file), the clearly defined values are 50 for 10bit and 200 for 12bit. I have followed this. I also ran a simple experiment to check the values for a completely dark (black) RAW capture and the result was 3200 for both cases as expected. So I think the current code is correct. I do note older versions of the Software Reference Manual (e.g. v0.2) had the same value (032h) for both 10bit and 12bit, but this has been corrected in v1.0 to the following: Recommended setting 10-bit output: 032h (50d in LSB units) 12-bit output: 0C8h (200d in LSB units) 16-bit output: C80h (3200d in LSB units) >> + >> + 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); > > The above 3 lines are redundant for simple VBLANK control updates. V4L2 > control framework would ensure new values are within the defined min/max > range before the drivers .s_ctrl is triggered. Ok. >> + >> + 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); > > Same here. No reason the exposure->cur.val would be out of bounds when user > requests an update to VBLANK value. So this should be: > > u32 current_exposure = imx->ctrls.exposure->cur.val See my reply to Laurent's comment. >> + >> + 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; >> + > > Any reason the register values for HREVERSE/VREVERSE are inverted now w.r.t > the userspace HFLIP/VFLIP settings? > > Ah, and is this why imx908_active_area.top was 0 and not 24 ? > > What v2 did (writing ctrl->val directly to the registers) seemed okay to > me. It's usually a good idea to mention functional changes in the > changelog. If there is a valid reason for the swap, maybe also add a code > comment here explaining why. See reply to Laurent's additional comment. >> + 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; >> + code->code = imx908_mbus_codes[code->index]; >> + 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; >> + } > > I see program_window() only looks at crop size, so changing format > width/height to something else using S_FMT will not change anything on the > CSI-2 bus. > > For freely-configurable sensor drivers like these, I think the usual > pattern is to allow S_SELECTION to configure the window size, and S_FMT > always snaps format->width = crop->width (same for height). See reply to Laurent's additional comment. >> + >> + 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) >> +{ >> + 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: >> + sel->r = imx908_active_area; >> + return 0; > > nitpick: can be simplified as > > case V4L2_SEL_TGT_CROP_DEFAULT: > case V4L2_SEL_TGT_CROP_BOUNDS: > sel->r = imx908_active_area; > return 0; Ok. >> + 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); >> + >> + 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); >> +} >> + >> +static int imx908_runtime_suspend(struct device *dev) >> +{ >> + imx908_power_off(dev); >> + return 0; >> +} >> + > > You could use imx908_power_off and imx908_power_on directly as the > RUNTIME_PM_OPS below. See comment after Laurent's additional comment. >> +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 */ >> + 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; > > The above two lines could be dropped. See reply to Laurent's additional comment. >> + >> + 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]); >> + >> +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; >> + goto err_free; > > Instead of the goto you could directly return here: > > v4l2_ctrl_handler_free(hdl); > return ret; > Agreed. >> + } >> + >> + 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); >> + if (ret) >> + goto err_pm_put; >> + >> + pm_runtime_set_autosuspend_delay(imx->dev, 1000); >> + pm_runtime_use_autosuspend(imx->dev); >> + >> + 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); >> + >> + 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_disable(imx->dev); > > This should be dropped given devm_pm_runtime_enable() was used in probe(). Good catch, thank you. Dropped pm_runtime_disable() and simplified imx908_remove() by delegating the final suspend to pm_runtime_suspend(imx->dev), leaving runtime PM cleanup to devres. >> + if (!pm_runtime_status_suspended(imx->dev)) >> + imx908_power_off(imx->dev); >> + pm_runtime_set_suspended(imx->dev); >> +} >> + >> +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"); >> -- >> 2.47.3 >> > > Thanks, > Jai >