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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx wrote: > > > >> From: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> > > > >> > > > >> 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@xxxxxxxxxxxxxxx> > > > >> Signed-off-by: Luis Garcia <git@xxxxxxxxxxxx> > > > >> --- > > > >> 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); > > > > > > > -- Kind regards, Sakari Ailus