From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 0CC64CD11C2 for ; Wed, 10 Apr 2024 08:44:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=+xnwx+MDlh2IqlUf3hLXjgJaQc2r3bWTDwDQmGJ6D1Y=; b=fyZC7O6bBMHoJw fT1uSqBQrDmU8wmjT/5tkAQlgBgc9yzCBs70YsPTg2KfmwqA2zSTgyS4ak8DFdmjbAt/rKU8tUZDP zb34whn2ffMj0TIFnYbIqtw/5P9CtMoUt1sPBqiD8p7lJWFMv6xeFDlFn7cdtlxWXUh+6QpVcC+tJ ArhG7cDZ5XG0xo8lz6JM+1gNhBSjyyJaZgNNZdj8pHc7nF+oiBvRSOUONScIuwsmmTdRYclJBfzCu Kh51JxWJhIV4Sp7qTze8x34djYByb6cn1NkeWKzPps/nEMKq8qWqvhxaunxhyHDzsHta4EdRqucOE Bwh0ov9Ed1yWghLtCmjg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1ruTZC-00000005wFO-43Xz; Wed, 10 Apr 2024 08:44:26 +0000 Received: from mgamail.intel.com ([192.198.163.14]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1ruTZ9-00000005wBm-2Hq7 for linux-arm-kernel@lists.infradead.org; Wed, 10 Apr 2024 08:44:25 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1712738664; x=1744274664; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=22j8uEO0liUSIyAblZj5xYhfKJtbo1A0yPEkeRp6MxQ=; b=POfrLBLKY6XyQoS0BPeeillbcIJhHDWvoMfsPKmU59emH29YbvUr8ObA uSL+asd8wEBvHmnGA8fRFkh90VKGmmRfFAAh0ONNvld+Bfq5bnnZfRx36 zJef9GTC3HGSzaWCHV2vJwvKIhU5/ps74tyGQ+RYTQfEwqXpLjGqNYKvJ Yd1dRiRDLr4i+KFLiGNaclKw04uNMQHGqkxsjpV+LvbkHNj/ElnkjxMb6 8U6cvDfI7rLL3PWA0m0SICooFhPak1JEiiAW/zZmpAxS5Gk0a3Awq8eLQ Sk/1lcB5YyvvXSvTJGBrja4nz4gyIzoS0L+XjSdmibPWRVTsjuA8y4oit Q==; X-CSE-ConnectionGUID: tGg49BOCTyGtQFYmut9mcg== X-CSE-MsgGUID: HdjW/IfjT8mAfyibg9UQIA== X-IronPort-AV: E=McAfee;i="6600,9927,11039"; a="8312391" X-IronPort-AV: E=Sophos;i="6.07,190,1708416000"; d="scan'208";a="8312391" Received: from orviesa009.jf.intel.com ([10.64.159.149]) by fmvoesa108.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Apr 2024 01:44:20 -0700 X-CSE-ConnectionGUID: ZCEpQddvRGO1fCBVA3e7SA== X-CSE-MsgGUID: +JF+LgLfSBCF7PIBaR62nw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.07,190,1708416000"; d="scan'208";a="20442869" Received: from turnipsi.fi.intel.com (HELO kekkonen.fi.intel.com) ([10.237.72.44]) by orviesa009-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Apr 2024 01:44:16 -0700 Received: from kekkonen.localdomain (localhost [127.0.0.1]) by kekkonen.fi.intel.com (Postfix) with SMTP id 3DE6711FC46; Wed, 10 Apr 2024 11:44:13 +0300 (EEST) Date: Wed, 10 Apr 2024 08:44:13 +0000 From: Sakari Ailus To: Dave Stevenson Cc: Luigi311 , linux-media@vger.kernel.org, jacopo.mondi@ideasonboard.com, mchehab@kernel.org, robh@kernel.org, krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, shawnguo@kernel.org, s.hauer@pengutronix.de, kernel@pengutronix.de, festevam@gmail.com, devicetree@vger.kernel.org, imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, pavel@ucw.cz, phone-devel@vger.kernel.org Subject: Re: [PATCH v3 19/25] media: i2c: imx258: Change register settings for variants of the sensor Message-ID: References: <20240403150355.189229-1-git@luigi311.com> <20240403150355.189229-20-git@luigi311.com> <998efafa-699b-4226-91d4-2ebba85d63ec@luigi311.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240410_014423_695931_6D053769 X-CRM114-Status: GOOD ( 49.47 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Dave, On Fri, Apr 05, 2024 at 02:16:31PM +0100, Dave Stevenson wrote: > Hi Sakari > > On Fri, 5 Apr 2024 at 11:59, Sakari Ailus wrote: > > > > Hi Luis, Dave, > > > > On Thu, Apr 04, 2024 at 04:44:05PM -0600, Luigi311 wrote: > > > On 4/3/24 10:18, Sakari Ailus wrote: > > > > Hi Luis, Dave, > > > > > > > > On Wed, Apr 03, 2024 at 09:03:48AM -0600, git@luigi311.com wrote: > > > >> From: Dave Stevenson > > > >> > > > >> Sony have advised that there are variants of the IMX258 sensor which > > > >> require slightly different register configuration to the mainline > > > >> imx258 driver defaults. > > > >> > > > >> There is no available run-time detection for the variant, so add > > > >> configuration via the DT compatible string. > > > >> > > > >> The Vision Components imx258 module supports PDAF, so add the > > > >> register differences for that variant > > > >> > > > >> Signed-off-by: Dave Stevenson > > > >> Signed-off-by: Luis Garcia > > > >> --- > > > >> drivers/media/i2c/imx258.c | 48 ++++++++++++++++++++++++++++++++++---- > > > >> 1 file changed, 44 insertions(+), 4 deletions(-) > > > >> > > > >> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c > > > >> index 775d957c9b87..fa48da212037 100644 > > > >> --- a/drivers/media/i2c/imx258.c > > > >> +++ b/drivers/media/i2c/imx258.c > > > >> @@ -6,6 +6,7 @@ > > > >> #include > > > >> #include > > > >> #include > > > >> +#include > > > >> #include > > > >> #include > > > >> #include > > > >> @@ -321,8 +322,6 @@ static const struct imx258_reg mipi_642mbps_24mhz_4l[] = { > > > >> > > > >> static const struct imx258_reg mode_common_regs[] = { > > > >> { 0x3051, 0x00 }, > > > >> - { 0x3052, 0x00 }, > > > >> - { 0x4E21, 0x14 }, > > > >> { 0x6B11, 0xCF }, > > > >> { 0x7FF0, 0x08 }, > > > >> { 0x7FF1, 0x0F }, > > > >> @@ -345,7 +344,6 @@ static const struct imx258_reg mode_common_regs[] = { > > > >> { 0x7FA8, 0x03 }, > > > >> { 0x7FA9, 0xFE }, > > > >> { 0x7B24, 0x81 }, > > > >> - { 0x7B25, 0x00 }, > > > >> { 0x6564, 0x07 }, > > > >> { 0x6B0D, 0x41 }, > > > >> { 0x653D, 0x04 }, > > > >> @@ -460,6 +458,33 @@ static const struct imx258_reg mode_1048_780_regs[] = { > > > >> { 0x034F, 0x0C }, > > > >> }; > > > >> > > > >> +struct imx258_variant_cfg { > > > >> + const struct imx258_reg *regs; > > > >> + unsigned int num_regs; > > > >> +}; > > > >> + > > > >> +static const struct imx258_reg imx258_cfg_regs[] = { > > > >> + { 0x3052, 0x00 }, > > > >> + { 0x4E21, 0x14 }, > > > >> + { 0x7B25, 0x00 }, > > > >> +}; > > > >> + > > > >> +static const struct imx258_variant_cfg imx258_cfg = { > > > >> + .regs = imx258_cfg_regs, > > > >> + .num_regs = ARRAY_SIZE(imx258_cfg_regs), > > > >> +}; > > > >> + > > > >> +static const struct imx258_reg imx258_pdaf_cfg_regs[] = { > > > >> + { 0x3052, 0x01 }, > > > >> + { 0x4E21, 0x10 }, > > > >> + { 0x7B25, 0x01 }, > > > >> +}; > > > >> + > > > >> +static const struct imx258_variant_cfg imx258_pdaf_cfg = { > > > >> + .regs = imx258_pdaf_cfg_regs, > > > >> + .num_regs = ARRAY_SIZE(imx258_pdaf_cfg_regs), > > > >> +}; > > > >> + > > > >> static const char * const imx258_test_pattern_menu[] = { > > > >> "Disabled", > > > >> "Solid Colour", > > > >> @@ -637,6 +662,8 @@ struct imx258 { > > > >> struct v4l2_subdev sd; > > > >> struct media_pad pad; > > > >> > > > >> + const struct imx258_variant_cfg *variant_cfg; > > > >> + > > > >> struct v4l2_ctrl_handler ctrl_handler; > > > >> /* V4L2 Controls */ > > > >> struct v4l2_ctrl *link_freq; > > > >> @@ -1104,6 +1131,14 @@ static int imx258_start_streaming(struct imx258 *imx258) > > > >> return ret; > > > >> } > > > >> > > > >> + ret = imx258_write_regs(imx258, imx258->variant_cfg->regs, > > > >> + imx258->variant_cfg->num_regs); > > > >> + if (ret) { > > > >> + dev_err(&client->dev, "%s failed to set variant config\n", > > > >> + __func__); > > > >> + return ret; > > > >> + } > > > >> + > > > >> ret = imx258_write_reg(imx258, IMX258_CLK_BLANK_STOP, > > > >> IMX258_REG_VALUE_08BIT, > > > >> imx258->csi2_flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK ? > > > >> @@ -1492,6 +1527,10 @@ static int imx258_probe(struct i2c_client *client) > > > >> > > > >> imx258->csi2_flags = ep.bus.mipi_csi2.flags; > > > >> > > > >> + imx258->variant_cfg = of_device_get_match_data(&client->dev); > > > > > > > > You'll also need to keep this working for ACPI based systems. I.e. in > > > > practice remove "of_" prefix here and add the non-PDAF variant data to the > > > > relevant ACPI ID list. > > > > > > > > > > Removing of_ is easy enough and looking at all the other commits that make > > > this change in other drivers I dont see anything else being done besides > > > adding in the .data section that is down below for both imx258 and pdaf > > > versions. Is that what you are referencing or is there some other place > > > to add variant data to ACPI ID list? > > > > Speaking of which---are you absolutely certain there are two variants of > > this sensor? Many sensors that have a different pixel pattern (PDAF pixels > > or a non-Bayer pattern) can produce Bayer data when condigured so. The fact > > that you have differing register configuration for the PDAF and non-PDAF > > cases suggests this may well be the case. > > I had a discussion with our contact at Sony over the configuration, > and Soho Enterprises who made the module I have also consulted with > Sony (their main person is ex Sony himself). > > There is a spec version field in the OTP which reflects the pixel > pattern. It has defined options of: > - HDR pattern > - Binning pattern > - mono > - non-PDAF > - HDR HDD > > Sony can't release information on how to read that information from > the sensor OTP as it is contractually locked by contracts with Intel. > Whilst information obtained via other routes means I have checked it > on my module as HDR pattern whilst the Nautilus platform has the > non-PDAF variant, I'm not going to spoil our relationship with Sony by > releasing that. > > It's possible that the Nautilus sensor will work happily with the > settings required for the PDAF variant, but I have no way of testing > that, and the registers in question are undocumented. Changing them > blindly isn't going to make any friends, and I doubt existing platform > users wish to rerun all their image quality tests on the sensor to > validate the change. > > Unless Intel wish to release the information on reading the OTP, we > have no way of telling the variants apart but need different register > configurations. If there is a better way of handling that situation > than compatible strings, then I'm open to suggestions. > > There's a short thread on libcamera-devel from back in 2022 where I > was looking into this [1] Oops! I guess we'll need these two for now at least. We don't really have support for PDAF anyway (I'd expect this to be documented for a driver, for instance) so I presume currently for PDAF-variants the configuration is about "correcting" the PDAF pixels? The problem seems to be worse on ACPI systems as there's a single HID only. > > Dave > > [1] https://lists.libcamera.org/pipermail/libcamera-devel/2022-June/031449.html > > > > > > > >> + if (!imx258->variant_cfg) > > > >> + imx258->variant_cfg = &imx258_cfg; > > > >> + > > > >> /* Initialize subdev */ > > > >> v4l2_i2c_subdev_init(&imx258->sd, client, &imx258_subdev_ops); > > > >> > > > >> @@ -1579,7 +1618,8 @@ MODULE_DEVICE_TABLE(acpi, imx258_acpi_ids); > > > >> #endif > > > >> > > > >> static const struct of_device_id imx258_dt_ids[] = { > > > >> - { .compatible = "sony,imx258" }, > > > >> + { .compatible = "sony,imx258", .data = &imx258_cfg }, > > > >> + { .compatible = "sony,imx258-pdaf", .data = &imx258_pdaf_cfg }, > > > >> { /* sentinel */ } > > > >> }; > > > >> MODULE_DEVICE_TABLE(of, imx258_dt_ids); > > > > > > > -- Kind regards, Sakari Ailus _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel