* [PATCH 0/2] media: i2c: imx290: imx327 support @ 2023-02-03 10:24 Alexander Stein 2023-02-03 10:24 ` [PATCH 1/2] media: dt-bindings: media: i2c: Add imx327 version to IMX327 bindings Alexander Stein 2023-02-03 10:24 ` [PATCH 2/2] media: i2c: imx290: Add support for imx327 variant Alexander Stein 0 siblings, 2 replies; 7+ messages in thread From: Alexander Stein @ 2023-02-03 10:24 UTC (permalink / raw) To: Manivannan Sadhasivam, Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski, Shawn Guo, Sascha Hauer, Fabio Estevam, Laurent Pinchart, Dave Stevenson Cc: Alexander Stein, Pengutronix Kernel Team, linux-media, devicetree Hi, this series adds support imx327 to existing imx290 driver. The differences in specs is that imx327 lacks support for 8 lanes LVDS (LVDS not used in driver anyway) and 120 FPS (currently only 60 FPS supported). Although just magic numbers, imx327 has a specific set of init register to be written. This series is based on [1] and [2] which adds imx290-mono support. Best regards, Alexander [1] https://lore.kernel.org/linux-media/20230131190700.3476796-2-dave.stevenson@raspberrypi.com/ [2] https://lore.kernel.org/linux-media/20230131192016.3476937-3-dave.stevenson@raspberrypi.com/ Alexander Stein (2): media: dt-bindings: media: i2c: Add imx327 version to IMX327 bindings media: i2c: imx290: Add support for imx327 variant .../bindings/media/i2c/sony,imx290.yaml | 1 + drivers/media/i2c/imx290.c | 88 ++++++++++++++++--- 2 files changed, 78 insertions(+), 11 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] media: dt-bindings: media: i2c: Add imx327 version to IMX327 bindings 2023-02-03 10:24 [PATCH 0/2] media: i2c: imx290: imx327 support Alexander Stein @ 2023-02-03 10:24 ` Alexander Stein 2023-02-03 10:28 ` Krzysztof Kozlowski 2023-02-03 10:24 ` [PATCH 2/2] media: i2c: imx290: Add support for imx327 variant Alexander Stein 1 sibling, 1 reply; 7+ messages in thread From: Alexander Stein @ 2023-02-03 10:24 UTC (permalink / raw) To: Manivannan Sadhasivam, Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski, Shawn Guo, Sascha Hauer, Fabio Estevam, Laurent Pinchart, Dave Stevenson Cc: Alexander Stein, Pengutronix Kernel Team, linux-media, devicetree The imx290 driver can be used for both imx290 and imx327 as they have a similar register set and configuration. imx327 lacks 8 lanes LVDS and 120 FPS support. Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> --- Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml index 29ca4052591f..13de5b061343 100644 --- a/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml @@ -23,6 +23,7 @@ properties: enum: - sony,imx290 - sony,imx290-mono + - sony,imx327 reg: maxItems: 1 -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] media: dt-bindings: media: i2c: Add imx327 version to IMX327 bindings 2023-02-03 10:24 ` [PATCH 1/2] media: dt-bindings: media: i2c: Add imx327 version to IMX327 bindings Alexander Stein @ 2023-02-03 10:28 ` Krzysztof Kozlowski 0 siblings, 0 replies; 7+ messages in thread From: Krzysztof Kozlowski @ 2023-02-03 10:28 UTC (permalink / raw) To: Alexander Stein, Manivannan Sadhasivam, Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski, Shawn Guo, Sascha Hauer, Fabio Estevam, Laurent Pinchart, Dave Stevenson Cc: Pengutronix Kernel Team, linux-media, devicetree On 03/02/2023 11:24, Alexander Stein wrote: > The imx290 driver can be used for both imx290 and imx327 as they have a > similar register set and configuration. imx327 lacks 8 lanes LVDS and > 120 FPS support. > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] media: i2c: imx290: Add support for imx327 variant 2023-02-03 10:24 [PATCH 0/2] media: i2c: imx290: imx327 support Alexander Stein 2023-02-03 10:24 ` [PATCH 1/2] media: dt-bindings: media: i2c: Add imx327 version to IMX327 bindings Alexander Stein @ 2023-02-03 10:24 ` Alexander Stein 2023-02-03 10:55 ` Dave Stevenson 1 sibling, 1 reply; 7+ messages in thread From: Alexander Stein @ 2023-02-03 10:24 UTC (permalink / raw) To: Manivannan Sadhasivam, Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski, Shawn Guo, Sascha Hauer, Fabio Estevam, Laurent Pinchart, Dave Stevenson Cc: Alexander Stein, Pengutronix Kernel Team, linux-media, devicetree Both sensors are quite similar. Their specs only differ regarding LVDS and parallel output but are identical regarding MIPI-CSI-2 interface. But they use a different init setting of hard-coded values, taken from the datasheet. Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> --- drivers/media/i2c/imx290.c | 88 +++++++++++++++++++++++++++++++++----- 1 file changed, 77 insertions(+), 11 deletions(-) diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c index e642e1df520d..337252b2ec15 100644 --- a/drivers/media/i2c/imx290.c +++ b/drivers/media/i2c/imx290.c @@ -164,6 +164,36 @@ #define CLK_74_25 1 #define NUM_CLK 2 +enum imx290_model { + IMX290, + IMX290_MONO, + IMX327, +}; + +struct imx290_device_data { + enum imx290_model model; + const char *name; + u8 mono; +}; + +static const struct imx290_device_data imx290_models[] = { + [IMX290] = { + .model = IMX290, + .name = "imx290", + .mono = 0, + }, + [IMX290_MONO] = { + .model = IMX290_MONO, + .name = "imx290-mono", + .mono = 1, + }, + [IMX327] = { + .model = IMX327, + .name = "imx327", + .mono = 0, + }, +}; + struct imx290_regval { u32 reg; u32 val; @@ -210,9 +240,9 @@ struct imx290 { struct device *dev; struct clk *xclk; struct regmap *regmap; + const struct imx290_device_data *devdata; u32 xclk_freq; u8 nlanes; - u8 mono; struct v4l2_subdev sd; struct media_pad pad; @@ -240,7 +270,7 @@ static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd) * Modes and formats */ -static const struct imx290_regval imx290_global_init_settings[] = { +static const struct imx290_regval imx290_global_init_settings_290[] = { { IMX290_WINWV_OB, 12 }, { IMX290_WINPH, 0 }, { IMX290_WINPV, 0 }, @@ -292,6 +322,23 @@ static const struct imx290_regval imx290_global_init_settings[] = { { IMX290_REG_8BIT(0x33b3), 0x04 }, }; +static const struct imx290_regval imx290_global_init_settings_327[] = { + { IMX290_WINWV_OB, 12 }, + { IMX290_WINPH, 0 }, + { IMX290_WINPV, 0 }, + { IMX290_WINWH, 1948 }, + { IMX290_WINWV, 1097 }, + { IMX290_XSOUTSEL, IMX290_XSOUTSEL_XVSOUTSEL_VSYNC | + IMX290_XSOUTSEL_XHSOUTSEL_HSYNC }, + { IMX290_REG_8BIT(0x3011), 0x0A }, + { IMX290_REG_8BIT(0x3012), 0x64 }, + { IMX290_REG_8BIT(0x3013), 0x00 }, + { IMX290_REG_8BIT(0x309e), 0x4A }, + { IMX290_REG_8BIT(0x309f), 0x4A }, + { IMX290_REG_8BIT(0x3128), 0x04 }, + { IMX290_REG_8BIT(0x313b), 0x41 }, +}; + static const struct imx290_regval imx290_37_125mhz_clock[] = { { IMX290_EXTCK_FREQ, 0x2520 }, { IMX290_INCKSEL7, 0x49 }, @@ -558,7 +605,7 @@ imx290_format_info(const struct imx290 *imx290, u32 code) for (i = 0; i < ARRAY_SIZE(imx290_formats); ++i) { const struct imx290_format_info *info = &imx290_formats[i]; - if (info->code[imx290->mono] == code) + if (info->code[imx290->devdata->mono] == code) return info; } @@ -957,11 +1004,27 @@ static int imx290_start_streaming(struct imx290 *imx290, struct v4l2_subdev_state *state) { const struct v4l2_mbus_framefmt *format; + const struct imx290_regval *regs; + unsigned int reg_num; int ret; + switch (imx290->devdata->model) { + case IMX290: + case IMX290_MONO: + regs = imx290_global_init_settings_290; + reg_num = ARRAY_SIZE(imx290_global_init_settings_290); + break; + case IMX327: + regs = imx290_global_init_settings_327; + reg_num = ARRAY_SIZE(imx290_global_init_settings_327); + break; + default: + dev_err(imx290->dev, "Invalid model: %u\n", imx290->devdata->model); + return -EINVAL; + } + /* Set init register settings */ - ret = imx290_set_register_array(imx290, imx290_global_init_settings, - ARRAY_SIZE(imx290_global_init_settings)); + ret = imx290_set_register_array(imx290, regs, reg_num); if (ret < 0) { dev_err(imx290->dev, "Could not set init registers\n"); return ret; @@ -1072,7 +1135,7 @@ static int imx290_enum_mbus_code(struct v4l2_subdev *sd, if (code->index >= ARRAY_SIZE(imx290_formats)) return -EINVAL; - code->code = imx290_formats[code->index].code[imx290->mono]; + code->code = imx290_formats[code->index].code[imx290->devdata->mono]; return 0; } @@ -1114,7 +1177,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd, fmt->format.height = mode->height; if (!imx290_format_info(imx290, fmt->format.code)) - fmt->format.code = imx290_formats[0].code[imx290->mono]; + fmt->format.code = imx290_formats[0].code[imx290->devdata->mono]; fmt->format.field = V4L2_FIELD_NONE; fmt->format.colorspace = V4L2_COLORSPACE_RAW; @@ -1422,8 +1485,9 @@ static s64 imx290_check_link_freqs(const struct imx290 *imx290, } static const struct of_device_id imx290_of_match[] = { - { .compatible = "sony,imx290" }, - { .compatible = "sony,imx290-mono", .data = (void *)1 }, + { .compatible = "sony,imx290", .data = &imx290_models[IMX290] }, + { .compatible = "sony,imx290-mono", .data = &imx290_models[IMX290_MONO] }, + { .compatible = "sony,imx327", .data = &imx290_models[IMX327] }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, imx290_of_match); @@ -1441,8 +1505,7 @@ static int imx290_parse_dt(struct imx290 *imx290) s64 fq; match = i2c_of_match_device(imx290_of_match, client); - if (match) - imx290->mono = match->data ? 1 : 0; + imx290->devdata = match->data; endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(imx290->dev), NULL); if (!endpoint) { @@ -1561,6 +1624,9 @@ static int imx290_probe(struct i2c_client *client) if (ret) goto err_pm; + v4l2_i2c_subdev_set_name(&imx290->sd, client, + imx290->devdata->name, NULL); + /* * Finally, register the V4L2 subdev. This must be done after * initializing everything as the subdev can be used immediately after -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] media: i2c: imx290: Add support for imx327 variant 2023-02-03 10:24 ` [PATCH 2/2] media: i2c: imx290: Add support for imx327 variant Alexander Stein @ 2023-02-03 10:55 ` Dave Stevenson 2023-02-03 11:10 ` Alexander Stein 0 siblings, 1 reply; 7+ messages in thread From: Dave Stevenson @ 2023-02-03 10:55 UTC (permalink / raw) To: Alexander Stein Cc: Manivannan Sadhasivam, Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski, Shawn Guo, Sascha Hauer, Fabio Estevam, Laurent Pinchart, Pengutronix Kernel Team, linux-media, devicetree Hi Alexander On Fri, 3 Feb 2023 at 10:24, Alexander Stein <alexander.stein@ew.tq-group.com> wrote: > > Both sensors are quite similar. Their specs only differ regarding LVDS > and parallel output but are identical regarding MIPI-CSI-2 interface. > But they use a different init setting of hard-coded values, taken from > the datasheet. > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > --- > drivers/media/i2c/imx290.c | 88 +++++++++++++++++++++++++++++++++----- > 1 file changed, 77 insertions(+), 11 deletions(-) > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > index e642e1df520d..337252b2ec15 100644 > --- a/drivers/media/i2c/imx290.c > +++ b/drivers/media/i2c/imx290.c > @@ -164,6 +164,36 @@ > #define CLK_74_25 1 > #define NUM_CLK 2 > > +enum imx290_model { > + IMX290, > + IMX290_MONO, > + IMX327, > +}; > + > +struct imx290_device_data { > + enum imx290_model model; > + const char *name; > + u8 mono; > +}; > + > +static const struct imx290_device_data imx290_models[] = { > + [IMX290] = { > + .model = IMX290, > + .name = "imx290", > + .mono = 0, > + }, > + [IMX290_MONO] = { > + .model = IMX290_MONO, > + .name = "imx290-mono", > + .mono = 1, > + }, > + [IMX327] = { > + .model = IMX327, > + .name = "imx327", > + .mono = 0, > + }, > +}; > + > struct imx290_regval { > u32 reg; > u32 val; > @@ -210,9 +240,9 @@ struct imx290 { > struct device *dev; > struct clk *xclk; > struct regmap *regmap; > + const struct imx290_device_data *devdata; > u32 xclk_freq; > u8 nlanes; > - u8 mono; > > struct v4l2_subdev sd; > struct media_pad pad; > @@ -240,7 +270,7 @@ static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd) > * Modes and formats > */ > > -static const struct imx290_regval imx290_global_init_settings[] = { > +static const struct imx290_regval imx290_global_init_settings_290[] = { > { IMX290_WINWV_OB, 12 }, > { IMX290_WINPH, 0 }, > { IMX290_WINPV, 0 }, > @@ -292,6 +322,23 @@ static const struct imx290_regval imx290_global_init_settings[] = { > { IMX290_REG_8BIT(0x33b3), 0x04 }, > }; > > +static const struct imx290_regval imx290_global_init_settings_327[] = { > + { IMX290_WINWV_OB, 12 }, > + { IMX290_WINPH, 0 }, > + { IMX290_WINPV, 0 }, > + { IMX290_WINWH, 1948 }, > + { IMX290_WINWV, 1097 }, > + { IMX290_XSOUTSEL, IMX290_XSOUTSEL_XVSOUTSEL_VSYNC | > + IMX290_XSOUTSEL_XHSOUTSEL_HSYNC }, > + { IMX290_REG_8BIT(0x3011), 0x0A }, What datasheet are you working from? Mine (2019/03/25) has a correction listed at v0.3 of: Register 3011h setting 0Ah -> 02h > + { IMX290_REG_8BIT(0x3012), 0x64 }, > + { IMX290_REG_8BIT(0x3013), 0x00 }, > + { IMX290_REG_8BIT(0x309e), 0x4A }, > + { IMX290_REG_8BIT(0x309f), 0x4A }, 309e/f undocumented in my datasheet beyond "default value 5Ah, set to "4Ah"". Not documented in imx290 or imx462 datasheets either. I'll read it back from IMX290 and IMX462 when I get to the office and see if 0x4a is the default anyway, in which case it can be generic. > + { IMX290_REG_8BIT(0x3128), 0x04 }, Correction v0.3 - register address 3128h deleted. > + { IMX290_REG_8BIT(0x313b), 0x41 }, Correction v0.3 - Register address 313Bh setting 41h -> 61h. I'll check the defaults on imx290 and imx462, because there is no harm in adding those register writes if they happen to be the defaults. There is also a fair amount of duplication between imx290_global_init_settings_290 and imx290_global_init_settings_327 - it'd be nice to reduce it down to the minimum set of diffs. > +}; > + > static const struct imx290_regval imx290_37_125mhz_clock[] = { > { IMX290_EXTCK_FREQ, 0x2520 }, > { IMX290_INCKSEL7, 0x49 }, > @@ -558,7 +605,7 @@ imx290_format_info(const struct imx290 *imx290, u32 code) > for (i = 0; i < ARRAY_SIZE(imx290_formats); ++i) { > const struct imx290_format_info *info = &imx290_formats[i]; > > - if (info->code[imx290->mono] == code) > + if (info->code[imx290->devdata->mono] == code) > return info; > } > > @@ -957,11 +1004,27 @@ static int imx290_start_streaming(struct imx290 *imx290, > struct v4l2_subdev_state *state) > { > const struct v4l2_mbus_framefmt *format; > + const struct imx290_regval *regs; > + unsigned int reg_num; > int ret; > > + switch (imx290->devdata->model) { > + case IMX290: > + case IMX290_MONO: > + regs = imx290_global_init_settings_290; > + reg_num = ARRAY_SIZE(imx290_global_init_settings_290); > + break; > + case IMX327: > + regs = imx290_global_init_settings_327; > + reg_num = ARRAY_SIZE(imx290_global_init_settings_327); > + break; > + default: > + dev_err(imx290->dev, "Invalid model: %u\n", imx290->devdata->model); > + return -EINVAL; > + } > + > /* Set init register settings */ > - ret = imx290_set_register_array(imx290, imx290_global_init_settings, > - ARRAY_SIZE(imx290_global_init_settings)); > + ret = imx290_set_register_array(imx290, regs, reg_num); > if (ret < 0) { > dev_err(imx290->dev, "Could not set init registers\n"); > return ret; > @@ -1072,7 +1135,7 @@ static int imx290_enum_mbus_code(struct v4l2_subdev *sd, > if (code->index >= ARRAY_SIZE(imx290_formats)) > return -EINVAL; > > - code->code = imx290_formats[code->index].code[imx290->mono]; > + code->code = imx290_formats[code->index].code[imx290->devdata->mono]; > > return 0; > } > @@ -1114,7 +1177,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd, > fmt->format.height = mode->height; > > if (!imx290_format_info(imx290, fmt->format.code)) > - fmt->format.code = imx290_formats[0].code[imx290->mono]; > + fmt->format.code = imx290_formats[0].code[imx290->devdata->mono]; > > fmt->format.field = V4L2_FIELD_NONE; > fmt->format.colorspace = V4L2_COLORSPACE_RAW; > @@ -1422,8 +1485,9 @@ static s64 imx290_check_link_freqs(const struct imx290 *imx290, > } > > static const struct of_device_id imx290_of_match[] = { > - { .compatible = "sony,imx290" }, > - { .compatible = "sony,imx290-mono", .data = (void *)1 }, > + { .compatible = "sony,imx290", .data = &imx290_models[IMX290] }, > + { .compatible = "sony,imx290-mono", .data = &imx290_models[IMX290_MONO] }, > + { .compatible = "sony,imx327", .data = &imx290_models[IMX327] }, Based on Laurent's requests my parent to this set will be switching to imx290 (as legacy), imx290lqr and imx290llr as the compatible strings. imx327 ought to follow the same pattern. Dave > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, imx290_of_match); > @@ -1441,8 +1505,7 @@ static int imx290_parse_dt(struct imx290 *imx290) > s64 fq; > > match = i2c_of_match_device(imx290_of_match, client); > - if (match) > - imx290->mono = match->data ? 1 : 0; > + imx290->devdata = match->data; > > endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(imx290->dev), NULL); > if (!endpoint) { > @@ -1561,6 +1624,9 @@ static int imx290_probe(struct i2c_client *client) > if (ret) > goto err_pm; > > + v4l2_i2c_subdev_set_name(&imx290->sd, client, > + imx290->devdata->name, NULL); > + > /* > * Finally, register the V4L2 subdev. This must be done after > * initializing everything as the subdev can be used immediately after > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] media: i2c: imx290: Add support for imx327 variant 2023-02-03 10:55 ` Dave Stevenson @ 2023-02-03 11:10 ` Alexander Stein 2023-02-03 11:38 ` Dave Stevenson 0 siblings, 1 reply; 7+ messages in thread From: Alexander Stein @ 2023-02-03 11:10 UTC (permalink / raw) To: Dave Stevenson Cc: Manivannan Sadhasivam, Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski, Shawn Guo, Sascha Hauer, Fabio Estevam, Laurent Pinchart, Pengutronix Kernel Team, linux-media, devicetree Hi Dave, Am Freitag, 3. Februar 2023, 11:55:55 CET schrieb Dave Stevenson: > Hi Alexander > > On Fri, 3 Feb 2023 at 10:24, Alexander Stein > > <alexander.stein@ew.tq-group.com> wrote: > > Both sensors are quite similar. Their specs only differ regarding LVDS > > and parallel output but are identical regarding MIPI-CSI-2 interface. > > But they use a different init setting of hard-coded values, taken from > > the datasheet. > > > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > --- > > > > drivers/media/i2c/imx290.c | 88 +++++++++++++++++++++++++++++++++----- > > 1 file changed, 77 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > > index e642e1df520d..337252b2ec15 100644 > > --- a/drivers/media/i2c/imx290.c > > +++ b/drivers/media/i2c/imx290.c > > @@ -164,6 +164,36 @@ > > > > #define CLK_74_25 1 > > #define NUM_CLK 2 > > > > +enum imx290_model { > > + IMX290, > > + IMX290_MONO, > > + IMX327, > > +}; > > + > > +struct imx290_device_data { > > + enum imx290_model model; > > + const char *name; > > + u8 mono; > > +}; > > + > > +static const struct imx290_device_data imx290_models[] = { > > + [IMX290] = { > > + .model = IMX290, > > + .name = "imx290", > > + .mono = 0, > > + }, > > + [IMX290_MONO] = { > > + .model = IMX290_MONO, > > + .name = "imx290-mono", > > + .mono = 1, > > + }, > > + [IMX327] = { > > + .model = IMX327, > > + .name = "imx327", > > + .mono = 0, > > + }, > > +}; > > + > > > > struct imx290_regval { > > > > u32 reg; > > u32 val; > > > > @@ -210,9 +240,9 @@ struct imx290 { > > > > struct device *dev; > > struct clk *xclk; > > struct regmap *regmap; > > > > + const struct imx290_device_data *devdata; > > > > u32 xclk_freq; > > u8 nlanes; > > > > - u8 mono; > > > > struct v4l2_subdev sd; > > struct media_pad pad; > > > > @@ -240,7 +270,7 @@ static inline struct imx290 *to_imx290(struct > > v4l2_subdev *_sd)> > > * Modes and formats > > */ > > > > -static const struct imx290_regval imx290_global_init_settings[] = { > > +static const struct imx290_regval imx290_global_init_settings_290[] = { > > > > { IMX290_WINWV_OB, 12 }, > > { IMX290_WINPH, 0 }, > > { IMX290_WINPV, 0 }, > > > > @@ -292,6 +322,23 @@ static const struct imx290_regval > > imx290_global_init_settings[] = {> > > { IMX290_REG_8BIT(0x33b3), 0x04 }, > > > > }; > > > > +static const struct imx290_regval imx290_global_init_settings_327[] = { > > + { IMX290_WINWV_OB, 12 }, > > + { IMX290_WINPH, 0 }, > > + { IMX290_WINPV, 0 }, > > + { IMX290_WINWH, 1948 }, > > + { IMX290_WINWV, 1097 }, > > + { IMX290_XSOUTSEL, IMX290_XSOUTSEL_XVSOUTSEL_VSYNC | > > + IMX290_XSOUTSEL_XHSOUTSEL_HSYNC }, > > + { IMX290_REG_8BIT(0x3011), 0x0A }, > > What datasheet are you working from? Mine (2019/03/25) has a > correction listed at v0.3 of: > Register 3011h setting 0Ah -> 02h I've got a v0.2 (2017/05/25). As you have v0.3 I am not really surprised you have different/additional values. > > + { IMX290_REG_8BIT(0x3012), 0x64 }, > > + { IMX290_REG_8BIT(0x3013), 0x00 }, > > + { IMX290_REG_8BIT(0x309e), 0x4A }, > > + { IMX290_REG_8BIT(0x309f), 0x4A }, > > 309e/f undocumented in my datasheet beyond "default value 5Ah, set to > "4Ah"". Not documented in imx290 or imx462 datasheets either. I'll read it > back from IMX290 and IMX462 when I get to the office and see if 0x4a is the > default anyway, in which case it can be generic. Exactly, they are not documented at all, just marked red indicating it's different from reset default. > > + { IMX290_REG_8BIT(0x3128), 0x04 }, > > Correction v0.3 - register address 3128h deleted. > > > + { IMX290_REG_8BIT(0x313b), 0x41 }, > > Correction v0.3 - Register address 313Bh setting 41h -> 61h. > > > I'll check the defaults on imx290 and imx462, because there is no harm > in adding those register writes if they happen to be the defaults. > There is also a fair amount of duplication between > imx290_global_init_settings_290 and imx290_global_init_settings_327 - > it'd be nice to reduce it down to the minimum set of diffs. Thanks, I'll update to the values you provided and split the common settings. > > +}; > > + > > > > static const struct imx290_regval imx290_37_125mhz_clock[] = { > > > > { IMX290_EXTCK_FREQ, 0x2520 }, > > { IMX290_INCKSEL7, 0x49 }, > > > > @@ -558,7 +605,7 @@ imx290_format_info(const struct imx290 *imx290, u32 > > code)> > > for (i = 0; i < ARRAY_SIZE(imx290_formats); ++i) { > > > > const struct imx290_format_info *info = > > &imx290_formats[i]; > > > > - if (info->code[imx290->mono] == code) > > + if (info->code[imx290->devdata->mono] == code) > > > > return info; > > > > } > > > > @@ -957,11 +1004,27 @@ static int imx290_start_streaming(struct imx290 > > *imx290,> > > struct v4l2_subdev_state *state) > > > > { > > > > const struct v4l2_mbus_framefmt *format; > > > > + const struct imx290_regval *regs; > > + unsigned int reg_num; > > > > int ret; > > > > + switch (imx290->devdata->model) { > > + case IMX290: > > + case IMX290_MONO: > > + regs = imx290_global_init_settings_290; > > + reg_num = ARRAY_SIZE(imx290_global_init_settings_290); > > + break; > > + case IMX327: > > + regs = imx290_global_init_settings_327; > > + reg_num = ARRAY_SIZE(imx290_global_init_settings_327); > > + break; > > + default: > > + dev_err(imx290->dev, "Invalid model: %u\n", > > imx290->devdata->model); + return -EINVAL; > > + } > > + > > > > /* Set init register settings */ > > > > - ret = imx290_set_register_array(imx290, > > imx290_global_init_settings, - > > ARRAY_SIZE(imx290_global_init_settings)); + ret = > > imx290_set_register_array(imx290, regs, reg_num); > > > > if (ret < 0) { > > > > dev_err(imx290->dev, "Could not set init registers\n"); > > return ret; > > > > @@ -1072,7 +1135,7 @@ static int imx290_enum_mbus_code(struct v4l2_subdev > > *sd,> > > if (code->index >= ARRAY_SIZE(imx290_formats)) > > > > return -EINVAL; > > > > - code->code = imx290_formats[code->index].code[imx290->mono]; > > + code->code = > > imx290_formats[code->index].code[imx290->devdata->mono];> > > return 0; > > > > } > > > > @@ -1114,7 +1177,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd, > > > > fmt->format.height = mode->height; > > > > if (!imx290_format_info(imx290, fmt->format.code)) > > > > - fmt->format.code = imx290_formats[0].code[imx290->mono]; > > + fmt->format.code = > > imx290_formats[0].code[imx290->devdata->mono];> > > fmt->format.field = V4L2_FIELD_NONE; > > fmt->format.colorspace = V4L2_COLORSPACE_RAW; > > > > @@ -1422,8 +1485,9 @@ static s64 imx290_check_link_freqs(const struct > > imx290 *imx290,> > > } > > > > static const struct of_device_id imx290_of_match[] = { > > > > - { .compatible = "sony,imx290" }, > > - { .compatible = "sony,imx290-mono", .data = (void *)1 }, > > + { .compatible = "sony,imx290", .data = &imx290_models[IMX290] }, > > + { .compatible = "sony,imx290-mono", .data = > > &imx290_models[IMX290_MONO] }, + { .compatible = "sony,imx327", > > .data = &imx290_models[IMX327] }, > Based on Laurent's requests my parent to this set will be switching to > imx290 (as legacy), imx290lqr and imx290llr as the compatible strings. > imx327 ought to follow the same pattern. Okay thanks. I'll update to imx327lqr as well. Put me on CC so I'll notice the update & conflict. Best regards, Alexander > Dave > > > { /* sentinel */ } > > > > }; > > MODULE_DEVICE_TABLE(of, imx290_of_match); > > > > @@ -1441,8 +1505,7 @@ static int imx290_parse_dt(struct imx290 *imx290) > > > > s64 fq; > > > > match = i2c_of_match_device(imx290_of_match, client); > > > > - if (match) > > - imx290->mono = match->data ? 1 : 0; > > + imx290->devdata = match->data; > > > > endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(imx290->dev), > > NULL); > > if (!endpoint) { > > > > @@ -1561,6 +1624,9 @@ static int imx290_probe(struct i2c_client *client) > > > > if (ret) > > > > goto err_pm; > > > > + v4l2_i2c_subdev_set_name(&imx290->sd, client, > > + imx290->devdata->name, NULL); > > + > > > > /* > > > > * Finally, register the V4L2 subdev. This must be done after > > * initializing everything as the subdev can be used immediately > > after > > > > -- > > 2.34.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] media: i2c: imx290: Add support for imx327 variant 2023-02-03 11:10 ` Alexander Stein @ 2023-02-03 11:38 ` Dave Stevenson 0 siblings, 0 replies; 7+ messages in thread From: Dave Stevenson @ 2023-02-03 11:38 UTC (permalink / raw) To: Alexander Stein Cc: Manivannan Sadhasivam, Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski, Shawn Guo, Sascha Hauer, Fabio Estevam, Laurent Pinchart, Pengutronix Kernel Team, linux-media, devicetree On Fri, 3 Feb 2023 at 11:10, Alexander Stein <alexander.stein@ew.tq-group.com> wrote: > > Hi Dave, > > Am Freitag, 3. Februar 2023, 11:55:55 CET schrieb Dave Stevenson: > > Hi Alexander > > > > On Fri, 3 Feb 2023 at 10:24, Alexander Stein > > > > <alexander.stein@ew.tq-group.com> wrote: > > > Both sensors are quite similar. Their specs only differ regarding LVDS > > > and parallel output but are identical regarding MIPI-CSI-2 interface. > > > But they use a different init setting of hard-coded values, taken from > > > the datasheet. > > > > > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > > --- > > > > > > drivers/media/i2c/imx290.c | 88 +++++++++++++++++++++++++++++++++----- > > > 1 file changed, 77 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > > > index e642e1df520d..337252b2ec15 100644 > > > --- a/drivers/media/i2c/imx290.c > > > +++ b/drivers/media/i2c/imx290.c > > > @@ -164,6 +164,36 @@ > > > > > > #define CLK_74_25 1 > > > #define NUM_CLK 2 > > > > > > +enum imx290_model { > > > + IMX290, > > > + IMX290_MONO, > > > + IMX327, > > > +}; > > > + > > > +struct imx290_device_data { > > > + enum imx290_model model; > > > + const char *name; > > > + u8 mono; > > > +}; > > > + > > > +static const struct imx290_device_data imx290_models[] = { > > > + [IMX290] = { > > > + .model = IMX290, > > > + .name = "imx290", > > > + .mono = 0, > > > + }, > > > + [IMX290_MONO] = { > > > + .model = IMX290_MONO, > > > + .name = "imx290-mono", > > > + .mono = 1, > > > + }, > > > + [IMX327] = { > > > + .model = IMX327, > > > + .name = "imx327", > > > + .mono = 0, > > > + }, > > > +}; > > > + > > > > > > struct imx290_regval { > > > > > > u32 reg; > > > u32 val; > > > > > > @@ -210,9 +240,9 @@ struct imx290 { > > > > > > struct device *dev; > > > struct clk *xclk; > > > struct regmap *regmap; > > > > > > + const struct imx290_device_data *devdata; > > > > > > u32 xclk_freq; > > > u8 nlanes; > > > > > > - u8 mono; > > > > > > struct v4l2_subdev sd; > > > struct media_pad pad; > > > > > > @@ -240,7 +270,7 @@ static inline struct imx290 *to_imx290(struct > > > v4l2_subdev *_sd)> > > > * Modes and formats > > > */ > > > > > > -static const struct imx290_regval imx290_global_init_settings[] = { > > > +static const struct imx290_regval imx290_global_init_settings_290[] = { > > > > > > { IMX290_WINWV_OB, 12 }, > > > { IMX290_WINPH, 0 }, > > > { IMX290_WINPV, 0 }, > > > > > > @@ -292,6 +322,23 @@ static const struct imx290_regval > > > imx290_global_init_settings[] = {> > > > { IMX290_REG_8BIT(0x33b3), 0x04 }, > > > > > > }; > > > > > > +static const struct imx290_regval imx290_global_init_settings_327[] = { > > > + { IMX290_WINWV_OB, 12 }, > > > + { IMX290_WINPH, 0 }, > > > + { IMX290_WINPV, 0 }, > > > + { IMX290_WINWH, 1948 }, > > > + { IMX290_WINWV, 1097 }, > > > + { IMX290_XSOUTSEL, IMX290_XSOUTSEL_XVSOUTSEL_VSYNC | > > > + IMX290_XSOUTSEL_XHSOUTSEL_HSYNC }, > > > + { IMX290_REG_8BIT(0x3011), 0x0A }, > > > > What datasheet are you working from? Mine (2019/03/25) has a > > correction listed at v0.3 of: > > Register 3011h setting 0Ah -> 02h > > I've got a v0.2 (2017/05/25). As you have v0.3 I am not really surprised you > have different/additional values. Revision history appears to be 0.1 2017/02/23 0.2 2017/05/25 0.3 2017/09/04 E17Z06 2017/12/18 E17Z06A81 2018/02/01 E1706B93 2019/03/25 so you are a way behind. > > > > + { IMX290_REG_8BIT(0x3012), 0x64 }, > > > + { IMX290_REG_8BIT(0x3013), 0x00 }, > > > + { IMX290_REG_8BIT(0x309e), 0x4A }, > > > + { IMX290_REG_8BIT(0x309f), 0x4A }, > > > > 309e/f undocumented in my datasheet beyond "default value 5Ah, set to > > "4Ah"". Not documented in imx290 or imx462 datasheets either. I'll read it > > back from IMX290 and IMX462 when I get to the office and see if 0x4a is the > > default anyway, in which case it can be generic. I can't read back from my IMX290LLR as Vision Components use a secondary MCU to stop you reading registers. IMX462LQR default 0x5a 0x5a :-( > Exactly, they are not documented at all, just marked red indicating it's > different from reset default. > > > > + { IMX290_REG_8BIT(0x3128), 0x04 }, > > > > Correction v0.3 - register address 3128h deleted. > > > > > + { IMX290_REG_8BIT(0x313b), 0x41 }, > > > > Correction v0.3 - Register address 313Bh setting 41h -> 61h. IMX462LQR default 0x51 > > > > > > I'll check the defaults on imx290 and imx462, because there is no harm > > in adding those register writes if they happen to be the defaults. > > There is also a fair amount of duplication between > > imx290_global_init_settings_290 and imx290_global_init_settings_327 - > > it'd be nice to reduce it down to the minimum set of diffs. > > Thanks, I'll update to the values you provided and split the common settings. So annoyingly that does mean we need a sensor specific table, but I think it is only 3 registers (0x309e, 0x309ef, and 0x313b). None of those need to be set for IMX290 or 462. Dave > > > +}; > > > + > > > > > > static const struct imx290_regval imx290_37_125mhz_clock[] = { > > > > > > { IMX290_EXTCK_FREQ, 0x2520 }, > > > { IMX290_INCKSEL7, 0x49 }, > > > > > > @@ -558,7 +605,7 @@ imx290_format_info(const struct imx290 *imx290, u32 > > > code)> > > > for (i = 0; i < ARRAY_SIZE(imx290_formats); ++i) { > > > > > > const struct imx290_format_info *info = > > > &imx290_formats[i]; > > > > > > - if (info->code[imx290->mono] == code) > > > + if (info->code[imx290->devdata->mono] == code) > > > > > > return info; > > > > > > } > > > > > > @@ -957,11 +1004,27 @@ static int imx290_start_streaming(struct imx290 > > > *imx290,> > > > struct v4l2_subdev_state *state) > > > > > > { > > > > > > const struct v4l2_mbus_framefmt *format; > > > > > > + const struct imx290_regval *regs; > > > + unsigned int reg_num; > > > > > > int ret; > > > > > > + switch (imx290->devdata->model) { > > > + case IMX290: > > > + case IMX290_MONO: > > > + regs = imx290_global_init_settings_290; > > > + reg_num = ARRAY_SIZE(imx290_global_init_settings_290); > > > + break; > > > + case IMX327: > > > + regs = imx290_global_init_settings_327; > > > + reg_num = ARRAY_SIZE(imx290_global_init_settings_327); > > > + break; > > > + default: > > > + dev_err(imx290->dev, "Invalid model: %u\n", > > > imx290->devdata->model); + return -EINVAL; > > > + } > > > + > > > > > > /* Set init register settings */ > > > > > > - ret = imx290_set_register_array(imx290, > > > imx290_global_init_settings, - > > > ARRAY_SIZE(imx290_global_init_settings)); + ret = > > > imx290_set_register_array(imx290, regs, reg_num); > > > > > > if (ret < 0) { > > > > > > dev_err(imx290->dev, "Could not set init registers\n"); > > > return ret; > > > > > > @@ -1072,7 +1135,7 @@ static int imx290_enum_mbus_code(struct v4l2_subdev > > > *sd,> > > > if (code->index >= ARRAY_SIZE(imx290_formats)) > > > > > > return -EINVAL; > > > > > > - code->code = imx290_formats[code->index].code[imx290->mono]; > > > + code->code = > > > imx290_formats[code->index].code[imx290->devdata->mono];> > > > return 0; > > > > > > } > > > > > > @@ -1114,7 +1177,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd, > > > > > > fmt->format.height = mode->height; > > > > > > if (!imx290_format_info(imx290, fmt->format.code)) > > > > > > - fmt->format.code = imx290_formats[0].code[imx290->mono]; > > > + fmt->format.code = > > > imx290_formats[0].code[imx290->devdata->mono];> > > > fmt->format.field = V4L2_FIELD_NONE; > > > fmt->format.colorspace = V4L2_COLORSPACE_RAW; > > > > > > @@ -1422,8 +1485,9 @@ static s64 imx290_check_link_freqs(const struct > > > imx290 *imx290,> > > > } > > > > > > static const struct of_device_id imx290_of_match[] = { > > > > > > - { .compatible = "sony,imx290" }, > > > - { .compatible = "sony,imx290-mono", .data = (void *)1 }, > > > + { .compatible = "sony,imx290", .data = &imx290_models[IMX290] }, > > > + { .compatible = "sony,imx290-mono", .data = > > > &imx290_models[IMX290_MONO] }, + { .compatible = "sony,imx327", > > > .data = &imx290_models[IMX327] }, > > Based on Laurent's requests my parent to this set will be switching to > > imx290 (as legacy), imx290lqr and imx290llr as the compatible strings. > > imx327 ought to follow the same pattern. > > Okay thanks. I'll update to imx327lqr as well. Put me on CC so I'll notice the > update & conflict. > > Best regards, > Alexander > > > Dave > > > > > { /* sentinel */ } > > > > > > }; > > > MODULE_DEVICE_TABLE(of, imx290_of_match); > > > > > > @@ -1441,8 +1505,7 @@ static int imx290_parse_dt(struct imx290 *imx290) > > > > > > s64 fq; > > > > > > match = i2c_of_match_device(imx290_of_match, client); > > > > > > - if (match) > > > - imx290->mono = match->data ? 1 : 0; > > > + imx290->devdata = match->data; > > > > > > endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(imx290->dev), > > > NULL); > > > if (!endpoint) { > > > > > > @@ -1561,6 +1624,9 @@ static int imx290_probe(struct i2c_client *client) > > > > > > if (ret) > > > > > > goto err_pm; > > > > > > + v4l2_i2c_subdev_set_name(&imx290->sd, client, > > > + imx290->devdata->name, NULL); > > > + > > > > > > /* > > > > > > * Finally, register the V4L2 subdev. This must be done after > > > * initializing everything as the subdev can be used immediately > > > after > > > > > > -- > > > 2.34.1 > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-02-03 11:38 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-02-03 10:24 [PATCH 0/2] media: i2c: imx290: imx327 support Alexander Stein 2023-02-03 10:24 ` [PATCH 1/2] media: dt-bindings: media: i2c: Add imx327 version to IMX327 bindings Alexander Stein 2023-02-03 10:28 ` Krzysztof Kozlowski 2023-02-03 10:24 ` [PATCH 2/2] media: i2c: imx290: Add support for imx327 variant Alexander Stein 2023-02-03 10:55 ` Dave Stevenson 2023-02-03 11:10 ` Alexander Stein 2023-02-03 11:38 ` Dave Stevenson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).