Message ID | 20240403150355.189229-20-git@luigi311.com (mailing list archive) |
---|---|
State | Superseded |
Headers |
Received: from am.mirrors.kernel.org ([147.75.80.249]) by linuxtv.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from <linux-media+bounces-8539-patchwork=linuxtv.org@vger.kernel.org>) id 1rs2Gu-00045F-2w for patchwork@linuxtv.org; Wed, 03 Apr 2024 15:11:29 +0000 Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id 1AA6D1F23690 for <patchwork@linuxtv.org>; Wed, 3 Apr 2024 15:11:26 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 513F814E2D1; Wed, 3 Apr 2024 15:09:01 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=luigi311.com header.i=@luigi311.com header.b="o7l8zwQ6" X-Original-To: linux-media@vger.kernel.org Received: from mail-108-mta118.mxroute.com (mail-108-mta118.mxroute.com [136.175.108.118]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0910214D6F0 for <linux-media@vger.kernel.org>; Wed, 3 Apr 2024 15:08:58 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=136.175.108.118 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712156941; cv=none; b=VP8WHHvSRU2b8spfUPMOIKYNL/XBvTaOk9gFCxqxIBIYLAOxG8V3zwrqEZ5hOv37YwN5a7p8RBG2wIvW0QLJD4vkZ/dDYhEhkg9PM1oirjMvw3K93r95nGr3E6Z6Fi808hbLuVH5fpH+2iYKcZ6iyQsa58BlgyJECDVfS8KIb0U= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712156941; c=relaxed/simple; bh=SZKceW7XhiaFkOM1tmAN0wp1PM47pjw3iJvgmSOm5o0=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=ESZ+0njo3hgxnVtemNzZfrZEJuDHRSP5Wa8+aEr6dv3fz3hiu1PKSXX57xSv8K9I9bislAPZPPb1Ep8iCckh76vNFFNOSbbS1vMUaHDPf62ocwSoZuY67TXCa41UVT0sK9xPYaFQH1QFP9OhU3mvMDE9/4ddz/XVYU83VZJXsT4= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=luigi311.com; spf=pass smtp.mailfrom=luigi311.com; dkim=pass (2048-bit key) header.d=luigi311.com header.i=@luigi311.com header.b=o7l8zwQ6; arc=none smtp.client-ip=136.175.108.118 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=luigi311.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=luigi311.com Received: from filter006.mxroute.com ([136.175.111.2] filter006.mxroute.com) (Authenticated sender: mN4UYu2MZsgR) by mail-108-mta118.mxroute.com (ZoneMTA) with ESMTPSA id 18ea4809fcc0003bea.011 for <linux-media@vger.kernel.org> (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384); Wed, 03 Apr 2024 15:07:56 +0000 X-Zone-Loop: 57b7d282aaddff868a4ca1c24b6e871e84be91c23a30 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=luigi311.com; s=x; h=Content-Transfer-Encoding:MIME-Version:References: In-Reply-To:Message-ID:Date:Subject:Cc:To:From:Sender:Reply-To:Content-Type: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=89UYSwemjH72oCJkImoj3GYITxzLE07DmWICqE+PSTA=; b=o7l8zwQ6frYkmj0OJGZtK64NUa h7YAI85vmWCxIqCkNZwSlU48WtNJ10P3xewp+Q2rppA9GinKIwNxsMlGiEbxOFDuUzZHyiEK50+rf DsA4rie9qVKkAfCqnIKEduBqzYB4Aq7TCrxe+NUS82k0nbb0oXu7f1UOCmtQplw1f6k7XmYNoovjL tcAgUflDknJevPd0PXBSBZ9Uz6lSyMD6px3QHzHiAOeGe4cc3+lGEWC0YfK0TvWtfXDBDLY8KFn3f qv+sJHTA4o1i0mxukWtzS8zVKyaC55Bnb94BNPc7I/HCEZjF1XyTxembR4KkQk3rSD77nrTvCX0ih 5md5ucdg==; From: git@luigi311.com To: linux-media@vger.kernel.org Cc: dave.stevenson@raspberrypi.com, 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, sakari.ailus@linux.intel.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, Luis Garcia <git@luigi311.com> Subject: [PATCH v3 19/25] media: i2c: imx258: Change register settings for variants of the sensor Date: Wed, 3 Apr 2024 09:03:48 -0600 Message-ID: <20240403150355.189229-20-git@luigi311.com> In-Reply-To: <20240403150355.189229-1-git@luigi311.com> References: <20240403150355.189229-1-git@luigi311.com> Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: <linux-media.vger.kernel.org> List-Subscribe: <mailto:linux-media+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-media+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Authenticated-Id: personal@luigi311.com X-LSpam-Score: -2.3 (--) X-LSpam-Report: No, score=-2.3 required=5.0 tests=ARC_SIGNED=0.001,ARC_VALID=-0.1,BAYES_00=-1.9,DKIM_INVALID=0.1,DKIM_SIGNED=0.1,DMARC_MISSING=0.001,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,RCVD_IN_DNSWL_NONE=-0.0001,SPF_HELO_NONE=0.001,SPF_PASS=-0.001 autolearn=unavailable autolearn_force=no |
Series |
imx258 improvement series
|
|
Commit Message
Luis Garcia
April 3, 2024, 3:03 p.m. UTC
From: Dave Stevenson <dave.stevenson@raspberrypi.com> 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 <dave.stevenson@raspberrypi.com> Signed-off-by: Luis Garcia <git@luigi311.com> --- drivers/media/i2c/imx258.c | 48 ++++++++++++++++++++++++++++++++++---- 1 file changed, 44 insertions(+), 4 deletions(-)
Comments
Hi Luis, Dave, On Wed, Apr 03, 2024 at 09:03:48AM -0600, git@luigi311.com wrote: > From: Dave Stevenson <dave.stevenson@raspberrypi.com> > > 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 <dave.stevenson@raspberrypi.com> > Signed-off-by: Luis Garcia <git@luigi311.com> > --- > 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 <linux/delay.h> > #include <linux/i2c.h> > #include <linux/module.h> > +#include <linux/of_device.h> > #include <linux/pm_runtime.h> > #include <linux/regulator/consumer.h> > #include <media/v4l2-ctrls.h> > @@ -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. > + 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);
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 <dave.stevenson@raspberrypi.com> >> >> 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 <dave.stevenson@raspberrypi.com> >> Signed-off-by: Luis Garcia <git@luigi311.com> >> --- >> 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 <linux/delay.h> >> #include <linux/i2c.h> >> #include <linux/module.h> >> +#include <linux/of_device.h> >> #include <linux/pm_runtime.h> >> #include <linux/regulator/consumer.h> >> #include <media/v4l2-ctrls.h> >> @@ -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? >> + 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); >
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 <dave.stevenson@raspberrypi.com> > >> > >> 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 <dave.stevenson@raspberrypi.com> > >> Signed-off-by: Luis Garcia <git@luigi311.com> > >> --- > >> 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 <linux/delay.h> > >> #include <linux/i2c.h> > >> #include <linux/module.h> > >> +#include <linux/of_device.h> > >> #include <linux/pm_runtime.h> > >> #include <linux/regulator/consumer.h> > >> #include <media/v4l2-ctrls.h> > >> @@ -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. > > >> + 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); > > >
Hi Sakari On Fri, 5 Apr 2024 at 11:59, Sakari Ailus <sakari.ailus@linux.intel.com> 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 <dave.stevenson@raspberrypi.com> > > >> > > >> 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 <dave.stevenson@raspberrypi.com> > > >> Signed-off-by: Luis Garcia <git@luigi311.com> > > >> --- > > >> 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 <linux/delay.h> > > >> #include <linux/i2c.h> > > >> #include <linux/module.h> > > >> +#include <linux/of_device.h> > > >> #include <linux/pm_runtime.h> > > >> #include <linux/regulator/consumer.h> > > >> #include <media/v4l2-ctrls.h> > > >> @@ -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] 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); > > > > > > > -- > Regards, > > Sakari Ailus
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 <sakari.ailus@linux.intel.com> 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 <dave.stevenson@raspberrypi.com> > > > >> > > > >> 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 <dave.stevenson@raspberrypi.com> > > > >> Signed-off-by: Luis Garcia <git@luigi311.com> > > > >> --- > > > >> 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 <linux/delay.h> > > > >> #include <linux/i2c.h> > > > >> #include <linux/module.h> > > > >> +#include <linux/of_device.h> > > > >> #include <linux/pm_runtime.h> > > > >> #include <linux/regulator/consumer.h> > > > >> #include <media/v4l2-ctrls.h> > > > >> @@ -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); > > > > > > >
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 <linux/delay.h> #include <linux/i2c.h> #include <linux/module.h> +#include <linux/of_device.h> #include <linux/pm_runtime.h> #include <linux/regulator/consumer.h> #include <media/v4l2-ctrls.h> @@ -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); + 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);