* [PATCH v2 0/2] media: i2c: imx290: imx327 support
@ 2023-02-06 13:17 Alexander Stein
2023-02-06 13:17 ` [PATCH v2 1/2] media: dt-bindings: media: i2c: Add imx327 version to IMX327 bindings Alexander Stein
2023-02-06 13:17 ` [PATCH v2 2/2] media: i2c: imx290: Add support for imx327 variant Alexander Stein
0 siblings, 2 replies; 9+ messages in thread
From: Alexander Stein @ 2023-02-06 13:17 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 is the next version for supporting imx327 sensor.
Changes in v2:
* Switched compatible to sony,imx327lqr
* Rebased on top of Dave's updated series
* Split some register writes into common and device specific lists
[1] https://lore.kernel.org/linux-media/20230203191644.947643-1-dave.stevenson@raspberrypi.com/
[2] https://lore.kernel.org/linux-media/20230203191811.947697-1-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 | 58 ++++++++++++++++++-
2 files changed, 56 insertions(+), 3 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/2] media: dt-bindings: media: i2c: Add imx327 version to IMX327 bindings
2023-02-06 13:17 [PATCH v2 0/2] media: i2c: imx290: imx327 support Alexander Stein
@ 2023-02-06 13:17 ` Alexander Stein
2023-02-07 1:27 ` Laurent Pinchart
2023-02-06 13:17 ` [PATCH v2 2/2] media: i2c: imx290: Add support for imx327 variant Alexander Stein
1 sibling, 1 reply; 9+ messages in thread
From: Alexander Stein @ 2023-02-06 13:17 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, Krzysztof Kozlowski
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>
---
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 dacecb0cd9aa..5afe508011ec 100644
--- a/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
@@ -29,6 +29,7 @@ properties:
- sony,imx290
- sony,imx290lqr
- sony,imx290llr
+ - sony,imx327lqr
reg:
maxItems: 1
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] media: i2c: imx290: Add support for imx327 variant
2023-02-06 13:17 [PATCH v2 0/2] media: i2c: imx290: imx327 support Alexander Stein
2023-02-06 13:17 ` [PATCH v2 1/2] media: dt-bindings: media: i2c: Add imx327 version to IMX327 bindings Alexander Stein
@ 2023-02-06 13:17 ` Alexander Stein
2023-02-06 14:40 ` Dave Stevenson
2023-02-07 2:00 ` Laurent Pinchart
1 sibling, 2 replies; 9+ messages in thread
From: Alexander Stein @ 2023-02-06 13:17 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>
---
Note: The call to v4l2_i2c_subdev_set_name will change the device name
shown to userspace. So now 'imx290lqr' will be shown instead of 'imx290'.
drivers/media/i2c/imx290.c | 58 ++++++++++++++++++++++++++++++++++++--
1 file changed, 55 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 1cfdd700bca5..0bfbce8853e6 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -173,10 +173,13 @@ enum imx290_colour_variant {
enum imx290_model {
IMX290_MODEL_IMX290LQR,
IMX290_MODEL_IMX290LLR,
+ IMX290_MODEL_IMX327LQR,
};
struct imx290_model_info {
enum imx290_colour_variant colour_variant;
+ enum imx290_model model;
+ const char *name;
};
enum imx290_clk_freq {
@@ -272,10 +275,14 @@ static const struct imx290_regval imx290_global_init_settings[] = {
{ IMX290_WINWV, 1097 },
{ IMX290_XSOUTSEL, IMX290_XSOUTSEL_XVSOUTSEL_VSYNC |
IMX290_XSOUTSEL_XHSOUTSEL_HSYNC },
- { IMX290_REG_8BIT(0x300f), 0x00 },
- { IMX290_REG_8BIT(0x3010), 0x21 },
+ { IMX290_REG_8BIT(0x3011), 0x02 },
{ IMX290_REG_8BIT(0x3012), 0x64 },
{ IMX290_REG_8BIT(0x3013), 0x00 },
+};
+
+static const struct imx290_regval imx290_global_init_settings_290[] = {
+ { IMX290_REG_8BIT(0x300f), 0x00 },
+ { IMX290_REG_8BIT(0x3010), 0x21 },
{ IMX290_REG_8BIT(0x3016), 0x09 },
{ IMX290_REG_8BIT(0x3070), 0x02 },
{ IMX290_REG_8BIT(0x3071), 0x11 },
@@ -328,6 +335,12 @@ static const struct imx290_regval xclk_regs[][IMX290_NUM_CLK_REGS] = {
},
};
+static const struct imx290_regval imx290_global_init_settings_327[] = {
+ { IMX290_REG_8BIT(0x309e), 0x4A },
+ { IMX290_REG_8BIT(0x309f), 0x4A },
+ { IMX290_REG_8BIT(0x313b), 0x61 },
+};
+
static const struct imx290_regval imx290_1080p_settings[] = {
/* mode settings */
{ IMX290_WINWV_OB, 12 },
@@ -999,9 +1012,11 @@ 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;
- /* Set init register settings */
+ /* Set common init register settings */
ret = imx290_set_register_array(imx290, imx290_global_init_settings,
ARRAY_SIZE(imx290_global_init_settings));
if (ret < 0) {
@@ -1009,6 +1024,28 @@ static int imx290_start_streaming(struct imx290 *imx290,
return ret;
}
+ switch (imx290->model->model) {
+ case IMX290_MODEL_IMX290LQR:
+ case IMX290_MODEL_IMX290LLR:
+ regs = imx290_global_init_settings_290;
+ reg_num = ARRAY_SIZE(imx290_global_init_settings_290);
+ break;
+ case IMX290_MODEL_IMX327LQR:
+ 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->model->model);
+ return -EINVAL;
+ }
+
+ /* Set init register 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;
+ }
+
/* Set clock parameters based on mode and xclk */
ret = imx290_set_clock(imx290);
if (ret < 0) {
@@ -1479,9 +1516,18 @@ static s64 imx290_check_link_freqs(const struct imx290 *imx290,
static const struct imx290_model_info imx290_models[] = {
[IMX290_MODEL_IMX290LQR] = {
.colour_variant = IMX290_VARIANT_COLOUR,
+ .model = IMX290_MODEL_IMX290LQR,
+ .name = "imx290lqr",
},
[IMX290_MODEL_IMX290LLR] = {
.colour_variant = IMX290_VARIANT_MONO,
+ .model = IMX290_MODEL_IMX290LLR,
+ .name = "imx290llr",
+ },
+ [IMX290_MODEL_IMX327LQR] = {
+ .colour_variant = IMX290_VARIANT_COLOUR,
+ .model = IMX290_MODEL_IMX327LQR,
+ .name = "imx327lqr",
},
};
@@ -1496,6 +1542,9 @@ static const struct of_device_id imx290_of_match[] = {
}, {
.compatible = "sony,imx290llr",
.data = &imx290_models[IMX290_MODEL_IMX290LLR],
+ }, {
+ .compatible = "sony,imx327lqr",
+ .data = &imx290_models[IMX290_MODEL_IMX327LQR],
},
{ /* sentinel */ },
};
@@ -1630,6 +1679,9 @@ static int imx290_probe(struct i2c_client *client)
if (ret)
goto err_pm;
+ v4l2_i2c_subdev_set_name(&imx290->sd, client,
+ imx290->model->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] 9+ messages in thread
* Re: [PATCH v2 2/2] media: i2c: imx290: Add support for imx327 variant
2023-02-06 13:17 ` [PATCH v2 2/2] media: i2c: imx290: Add support for imx327 variant Alexander Stein
@ 2023-02-06 14:40 ` Dave Stevenson
2023-02-07 1:32 ` Laurent Pinchart
2023-02-07 2:00 ` Laurent Pinchart
1 sibling, 1 reply; 9+ messages in thread
From: Dave Stevenson @ 2023-02-06 14:40 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
Thanks for the patch.
On Mon, 6 Feb 2023 at 13:17, 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>
> ---
> Note: The call to v4l2_i2c_subdev_set_name will change the device name
> shown to userspace. So now 'imx290lqr' will be shown instead of 'imx290'.
This is going to cause grief as we already have a Pi libcamera
pipeline handler and tuning that relies on the entity name being
"imx290", so changing that is going to cause issues.
From userspace, the difference between lqr and llr is already reported
via the different colour formats supported (RGGB10 & RGGB12 vs Y10 &
Y12), so there is no need to provide the full part number. If there is
a need to distinguish imx327 vs imx290 in userspace, then I'd propose
just using the base model identifier.
> drivers/media/i2c/imx290.c | 58 ++++++++++++++++++++++++++++++++++++--
> 1 file changed, 55 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 1cfdd700bca5..0bfbce8853e6 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -173,10 +173,13 @@ enum imx290_colour_variant {
> enum imx290_model {
> IMX290_MODEL_IMX290LQR,
> IMX290_MODEL_IMX290LLR,
> + IMX290_MODEL_IMX327LQR,
> };
>
> struct imx290_model_info {
> enum imx290_colour_variant colour_variant;
> + enum imx290_model model;
> + const char *name;
> };
>
> enum imx290_clk_freq {
> @@ -272,10 +275,14 @@ static const struct imx290_regval imx290_global_init_settings[] = {
> { IMX290_WINWV, 1097 },
> { IMX290_XSOUTSEL, IMX290_XSOUTSEL_XVSOUTSEL_VSYNC |
> IMX290_XSOUTSEL_XHSOUTSEL_HSYNC },
> - { IMX290_REG_8BIT(0x300f), 0x00 },
> - { IMX290_REG_8BIT(0x3010), 0x21 },
> + { IMX290_REG_8BIT(0x3011), 0x02 },
> { IMX290_REG_8BIT(0x3012), 0x64 },
> { IMX290_REG_8BIT(0x3013), 0x00 },
> +};
> +
> +static const struct imx290_regval imx290_global_init_settings_290[] = {
> + { IMX290_REG_8BIT(0x300f), 0x00 },
> + { IMX290_REG_8BIT(0x3010), 0x21 },
> { IMX290_REG_8BIT(0x3016), 0x09 },
> { IMX290_REG_8BIT(0x3070), 0x02 },
> { IMX290_REG_8BIT(0x3071), 0x11 },
> @@ -328,6 +335,12 @@ static const struct imx290_regval xclk_regs[][IMX290_NUM_CLK_REGS] = {
> },
> };
>
> +static const struct imx290_regval imx290_global_init_settings_327[] = {
> + { IMX290_REG_8BIT(0x309e), 0x4A },
> + { IMX290_REG_8BIT(0x309f), 0x4A },
> + { IMX290_REG_8BIT(0x313b), 0x61 },
> +};
> +
> static const struct imx290_regval imx290_1080p_settings[] = {
> /* mode settings */
> { IMX290_WINWV_OB, 12 },
> @@ -999,9 +1012,11 @@ 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;
>
> - /* Set init register settings */
> + /* Set common init register settings */
> ret = imx290_set_register_array(imx290, imx290_global_init_settings,
> ARRAY_SIZE(imx290_global_init_settings));
> if (ret < 0) {
> @@ -1009,6 +1024,28 @@ static int imx290_start_streaming(struct imx290 *imx290,
> return ret;
> }
>
> + switch (imx290->model->model) {
> + case IMX290_MODEL_IMX290LQR:
> + case IMX290_MODEL_IMX290LLR:
> + regs = imx290_global_init_settings_290;
> + reg_num = ARRAY_SIZE(imx290_global_init_settings_290);
> + break;
> + case IMX290_MODEL_IMX327LQR:
> + 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->model->model);
> + return -EINVAL;
> + }
switch/case here, or add a pointer to struct imx290_model_info?
Keeping all the configuration for the different models in struct
imx290_model_info has an appeal to me.
Dave
> +
> + /* Set init register 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;
> + }
> +
> /* Set clock parameters based on mode and xclk */
> ret = imx290_set_clock(imx290);
> if (ret < 0) {
> @@ -1479,9 +1516,18 @@ static s64 imx290_check_link_freqs(const struct imx290 *imx290,
> static const struct imx290_model_info imx290_models[] = {
> [IMX290_MODEL_IMX290LQR] = {
> .colour_variant = IMX290_VARIANT_COLOUR,
> + .model = IMX290_MODEL_IMX290LQR,
> + .name = "imx290lqr",
> },
> [IMX290_MODEL_IMX290LLR] = {
> .colour_variant = IMX290_VARIANT_MONO,
> + .model = IMX290_MODEL_IMX290LLR,
> + .name = "imx290llr",
> + },
> + [IMX290_MODEL_IMX327LQR] = {
> + .colour_variant = IMX290_VARIANT_COLOUR,
> + .model = IMX290_MODEL_IMX327LQR,
> + .name = "imx327lqr",
> },
> };
>
> @@ -1496,6 +1542,9 @@ static const struct of_device_id imx290_of_match[] = {
> }, {
> .compatible = "sony,imx290llr",
> .data = &imx290_models[IMX290_MODEL_IMX290LLR],
> + }, {
> + .compatible = "sony,imx327lqr",
> + .data = &imx290_models[IMX290_MODEL_IMX327LQR],
> },
> { /* sentinel */ },
> };
> @@ -1630,6 +1679,9 @@ static int imx290_probe(struct i2c_client *client)
> if (ret)
> goto err_pm;
>
> + v4l2_i2c_subdev_set_name(&imx290->sd, client,
> + imx290->model->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] 9+ messages in thread
* Re: [PATCH v2 1/2] media: dt-bindings: media: i2c: Add imx327 version to IMX327 bindings
2023-02-06 13:17 ` [PATCH v2 1/2] media: dt-bindings: media: i2c: Add imx327 version to IMX327 bindings Alexander Stein
@ 2023-02-07 1:27 ` Laurent Pinchart
0 siblings, 0 replies; 9+ messages in thread
From: Laurent Pinchart @ 2023-02-07 1:27 UTC (permalink / raw)
To: Alexander Stein
Cc: Manivannan Sadhasivam, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Shawn Guo, Sascha Hauer, Fabio Estevam,
Dave Stevenson, Pengutronix Kernel Team, linux-media, devicetree,
Krzysztof Kozlowski
Hi Alexander,
Thank you for the patch.
On Mon, Feb 06, 2023 at 02:17:30PM +0100, 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>
> ---
> 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 dacecb0cd9aa..5afe508011ec 100644
> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
> @@ -29,6 +29,7 @@ properties:
> - sony,imx290
> - sony,imx290lqr
> - sony,imx290llr
> + - sony,imx327lqr
Looks good, but may need to be rebased on top of v3 of Dave's series.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> reg:
> maxItems: 1
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] media: i2c: imx290: Add support for imx327 variant
2023-02-06 14:40 ` Dave Stevenson
@ 2023-02-07 1:32 ` Laurent Pinchart
2023-02-07 12:40 ` Dave Stevenson
0 siblings, 1 reply; 9+ messages in thread
From: Laurent Pinchart @ 2023-02-07 1:32 UTC (permalink / raw)
To: Dave Stevenson
Cc: Alexander Stein, Manivannan Sadhasivam, Mauro Carvalho Chehab,
Rob Herring, Krzysztof Kozlowski, Shawn Guo, Sascha Hauer,
Fabio Estevam, Pengutronix Kernel Team, linux-media, devicetree
Hello,
On Mon, Feb 06, 2023 at 02:40:56PM +0000, Dave Stevenson wrote:
> On Mon, 6 Feb 2023 at 13:17, Alexander Stein 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>
> > ---
> > Note: The call to v4l2_i2c_subdev_set_name will change the device name
> > shown to userspace. So now 'imx290lqr' will be shown instead of 'imx290'.
>
> This is going to cause grief as we already have a Pi libcamera
> pipeline handler and tuning that relies on the entity name being
> "imx290", so changing that is going to cause issues.
>
> From userspace, the difference between lqr and llr is already reported
> via the different colour formats supported (RGGB10 & RGGB12 vs Y10 &
> Y12), so there is no need to provide the full part number. If there is
> a need to distinguish imx327 vs imx290 in userspace, then I'd propose
> just using the base model identifier.
Agreed.
> > drivers/media/i2c/imx290.c | 58 ++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 55 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 1cfdd700bca5..0bfbce8853e6 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -173,10 +173,13 @@ enum imx290_colour_variant {
> > enum imx290_model {
> > IMX290_MODEL_IMX290LQR,
> > IMX290_MODEL_IMX290LLR,
> > + IMX290_MODEL_IMX327LQR,
> > };
> >
> > struct imx290_model_info {
> > enum imx290_colour_variant colour_variant;
> > + enum imx290_model model;
> > + const char *name;
> > };
> >
> > enum imx290_clk_freq {
> > @@ -272,10 +275,14 @@ static const struct imx290_regval imx290_global_init_settings[] = {
> > { IMX290_WINWV, 1097 },
> > { IMX290_XSOUTSEL, IMX290_XSOUTSEL_XVSOUTSEL_VSYNC |
> > IMX290_XSOUTSEL_XHSOUTSEL_HSYNC },
> > - { IMX290_REG_8BIT(0x300f), 0x00 },
> > - { IMX290_REG_8BIT(0x3010), 0x21 },
> > + { IMX290_REG_8BIT(0x3011), 0x02 },
This change should be mentioned in the commit message.
> > { IMX290_REG_8BIT(0x3012), 0x64 },
> > { IMX290_REG_8BIT(0x3013), 0x00 },
> > +};
> > +
> > +static const struct imx290_regval imx290_global_init_settings_290[] = {
> > + { IMX290_REG_8BIT(0x300f), 0x00 },
> > + { IMX290_REG_8BIT(0x3010), 0x21 },
> > { IMX290_REG_8BIT(0x3016), 0x09 },
> > { IMX290_REG_8BIT(0x3070), 0x02 },
> > { IMX290_REG_8BIT(0x3071), 0x11 },
> > @@ -328,6 +335,12 @@ static const struct imx290_regval xclk_regs[][IMX290_NUM_CLK_REGS] = {
> > },
> > };
> >
> > +static const struct imx290_regval imx290_global_init_settings_327[] = {
> > + { IMX290_REG_8BIT(0x309e), 0x4A },
> > + { IMX290_REG_8BIT(0x309f), 0x4A },
> > + { IMX290_REG_8BIT(0x313b), 0x61 },
Lowercase hex constants pleasea.
> > +};
> > +
> > static const struct imx290_regval imx290_1080p_settings[] = {
> > /* mode settings */
> > { IMX290_WINWV_OB, 12 },
> > @@ -999,9 +1012,11 @@ 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;
> >
> > - /* Set init register settings */
> > + /* Set common init register settings */
> > ret = imx290_set_register_array(imx290, imx290_global_init_settings,
> > ARRAY_SIZE(imx290_global_init_settings));
> > if (ret < 0) {
> > @@ -1009,6 +1024,28 @@ static int imx290_start_streaming(struct imx290 *imx290,
> > return ret;
> > }
> >
> > + switch (imx290->model->model) {
> > + case IMX290_MODEL_IMX290LQR:
> > + case IMX290_MODEL_IMX290LLR:
> > + regs = imx290_global_init_settings_290;
> > + reg_num = ARRAY_SIZE(imx290_global_init_settings_290);
> > + break;
> > + case IMX290_MODEL_IMX327LQR:
> > + 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->model->model);
This should never happen, so you can drop the message.
> > + return -EINVAL;
> > + }
>
> switch/case here, or add a pointer to struct imx290_model_info?
Do you mean a pointer to the model-specific init regs array? I like the
idea. The size would need to be added too (unless we switch to
terminating those arrays with a sentinel).
> Keeping all the configuration for the different models in struct
> imx290_model_info has an appeal to me.
>
> > +
> > + /* Set init register settings */
/* Set device-specific init register 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;
> > + }
> > +
> > /* Set clock parameters based on mode and xclk */
> > ret = imx290_set_clock(imx290);
> > if (ret < 0) {
> > @@ -1479,9 +1516,18 @@ static s64 imx290_check_link_freqs(const struct imx290 *imx290,
> > static const struct imx290_model_info imx290_models[] = {
> > [IMX290_MODEL_IMX290LQR] = {
> > .colour_variant = IMX290_VARIANT_COLOUR,
> > + .model = IMX290_MODEL_IMX290LQR,
> > + .name = "imx290lqr",
> > },
> > [IMX290_MODEL_IMX290LLR] = {
> > .colour_variant = IMX290_VARIANT_MONO,
> > + .model = IMX290_MODEL_IMX290LLR,
> > + .name = "imx290llr",
> > + },
> > + [IMX290_MODEL_IMX327LQR] = {
> > + .colour_variant = IMX290_VARIANT_COLOUR,
> > + .model = IMX290_MODEL_IMX327LQR,
> > + .name = "imx327lqr",
> > },
> > };
> >
> > @@ -1496,6 +1542,9 @@ static const struct of_device_id imx290_of_match[] = {
> > }, {
> > .compatible = "sony,imx290llr",
> > .data = &imx290_models[IMX290_MODEL_IMX290LLR],
> > + }, {
> > + .compatible = "sony,imx327lqr",
> > + .data = &imx290_models[IMX290_MODEL_IMX327LQR],
> > },
> > { /* sentinel */ },
> > };
> > @@ -1630,6 +1679,9 @@ static int imx290_probe(struct i2c_client *client)
> > if (ret)
> > goto err_pm;
> >
> > + v4l2_i2c_subdev_set_name(&imx290->sd, client,
> > + imx290->model->name, NULL);
> > +
> > /*
> > * Finally, register the V4L2 subdev. This must be done after
> > * initializing everything as the subdev can be used immediately after
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] media: i2c: imx290: Add support for imx327 variant
2023-02-06 13:17 ` [PATCH v2 2/2] media: i2c: imx290: Add support for imx327 variant Alexander Stein
2023-02-06 14:40 ` Dave Stevenson
@ 2023-02-07 2:00 ` Laurent Pinchart
1 sibling, 0 replies; 9+ messages in thread
From: Laurent Pinchart @ 2023-02-07 2:00 UTC (permalink / raw)
To: Alexander Stein
Cc: Manivannan Sadhasivam, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Shawn Guo, Sascha Hauer, Fabio Estevam,
Dave Stevenson, Pengutronix Kernel Team, linux-media, devicetree
Hi Alexander,
Another comment.
On Mon, Feb 06, 2023 at 02:17:31PM +0100, Alexander Stein 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>
> ---
> Note: The call to v4l2_i2c_subdev_set_name will change the device name
> shown to userspace. So now 'imx290lqr' will be shown instead of 'imx290'.
>
> drivers/media/i2c/imx290.c | 58 ++++++++++++++++++++++++++++++++++++--
> 1 file changed, 55 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 1cfdd700bca5..0bfbce8853e6 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -173,10 +173,13 @@ enum imx290_colour_variant {
> enum imx290_model {
> IMX290_MODEL_IMX290LQR,
> IMX290_MODEL_IMX290LLR,
> + IMX290_MODEL_IMX327LQR,
> };
>
> struct imx290_model_info {
> enum imx290_colour_variant colour_variant;
> + enum imx290_model model;
> + const char *name;
> };
>
> enum imx290_clk_freq {
> @@ -272,10 +275,14 @@ static const struct imx290_regval imx290_global_init_settings[] = {
> { IMX290_WINWV, 1097 },
> { IMX290_XSOUTSEL, IMX290_XSOUTSEL_XVSOUTSEL_VSYNC |
> IMX290_XSOUTSEL_XHSOUTSEL_HSYNC },
> - { IMX290_REG_8BIT(0x300f), 0x00 },
> - { IMX290_REG_8BIT(0x3010), 0x21 },
> + { IMX290_REG_8BIT(0x3011), 0x02 },
> { IMX290_REG_8BIT(0x3012), 0x64 },
> { IMX290_REG_8BIT(0x3013), 0x00 },
> +};
> +
> +static const struct imx290_regval imx290_global_init_settings_290[] = {
> + { IMX290_REG_8BIT(0x300f), 0x00 },
> + { IMX290_REG_8BIT(0x3010), 0x21 },
> { IMX290_REG_8BIT(0x3016), 0x09 },
> { IMX290_REG_8BIT(0x3070), 0x02 },
> { IMX290_REG_8BIT(0x3071), 0x11 },
> @@ -328,6 +335,12 @@ static const struct imx290_regval xclk_regs[][IMX290_NUM_CLK_REGS] = {
> },
> };
>
> +static const struct imx290_regval imx290_global_init_settings_327[] = {
> + { IMX290_REG_8BIT(0x309e), 0x4A },
> + { IMX290_REG_8BIT(0x309f), 0x4A },
> + { IMX290_REG_8BIT(0x313b), 0x61 },
> +};
I wonder what the impact of those different init sequences is, as I've
run my IMX327 with this driver without your changes for quite some time
:-)
> +
> static const struct imx290_regval imx290_1080p_settings[] = {
> /* mode settings */
> { IMX290_WINWV_OB, 12 },
> @@ -999,9 +1012,11 @@ 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;
>
> - /* Set init register settings */
> + /* Set common init register settings */
> ret = imx290_set_register_array(imx290, imx290_global_init_settings,
> ARRAY_SIZE(imx290_global_init_settings));
> if (ret < 0) {
> @@ -1009,6 +1024,28 @@ static int imx290_start_streaming(struct imx290 *imx290,
> return ret;
> }
>
> + switch (imx290->model->model) {
> + case IMX290_MODEL_IMX290LQR:
> + case IMX290_MODEL_IMX290LLR:
> + regs = imx290_global_init_settings_290;
> + reg_num = ARRAY_SIZE(imx290_global_init_settings_290);
> + break;
> + case IMX290_MODEL_IMX327LQR:
> + 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->model->model);
> + return -EINVAL;
> + }
> +
> + /* Set init register 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;
> + }
> +
> /* Set clock parameters based on mode and xclk */
> ret = imx290_set_clock(imx290);
> if (ret < 0) {
> @@ -1479,9 +1516,18 @@ static s64 imx290_check_link_freqs(const struct imx290 *imx290,
> static const struct imx290_model_info imx290_models[] = {
> [IMX290_MODEL_IMX290LQR] = {
> .colour_variant = IMX290_VARIANT_COLOUR,
> + .model = IMX290_MODEL_IMX290LQR,
> + .name = "imx290lqr",
> },
> [IMX290_MODEL_IMX290LLR] = {
> .colour_variant = IMX290_VARIANT_MONO,
> + .model = IMX290_MODEL_IMX290LLR,
> + .name = "imx290llr",
> + },
> + [IMX290_MODEL_IMX327LQR] = {
> + .colour_variant = IMX290_VARIANT_COLOUR,
> + .model = IMX290_MODEL_IMX327LQR,
> + .name = "imx327lqr",
> },
> };
>
> @@ -1496,6 +1542,9 @@ static const struct of_device_id imx290_of_match[] = {
> }, {
> .compatible = "sony,imx290llr",
> .data = &imx290_models[IMX290_MODEL_IMX290LLR],
> + }, {
> + .compatible = "sony,imx327lqr",
> + .data = &imx290_models[IMX290_MODEL_IMX327LQR],
> },
> { /* sentinel */ },
> };
> @@ -1630,6 +1679,9 @@ static int imx290_probe(struct i2c_client *client)
> if (ret)
> goto err_pm;
>
> + v4l2_i2c_subdev_set_name(&imx290->sd, client,
> + imx290->model->name, NULL);
> +
> /*
> * Finally, register the V4L2 subdev. This must be done after
> * initializing everything as the subdev can be used immediately after
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] media: i2c: imx290: Add support for imx327 variant
2023-02-07 1:32 ` Laurent Pinchart
@ 2023-02-07 12:40 ` Dave Stevenson
2023-02-07 15:17 ` Laurent Pinchart
0 siblings, 1 reply; 9+ messages in thread
From: Dave Stevenson @ 2023-02-07 12:40 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Alexander Stein, Manivannan Sadhasivam, Mauro Carvalho Chehab,
Rob Herring, Krzysztof Kozlowski, Shawn Guo, Sascha Hauer,
Fabio Estevam, Pengutronix Kernel Team, linux-media, devicetree
Hi Laurent
On Tue, 7 Feb 2023 at 01:32, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hello,
>
> On Mon, Feb 06, 2023 at 02:40:56PM +0000, Dave Stevenson wrote:
> > On Mon, 6 Feb 2023 at 13:17, Alexander Stein 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>
> > > ---
> > > Note: The call to v4l2_i2c_subdev_set_name will change the device name
> > > shown to userspace. So now 'imx290lqr' will be shown instead of 'imx290'.
> >
> > This is going to cause grief as we already have a Pi libcamera
> > pipeline handler and tuning that relies on the entity name being
> > "imx290", so changing that is going to cause issues.
> >
> > From userspace, the difference between lqr and llr is already reported
> > via the different colour formats supported (RGGB10 & RGGB12 vs Y10 &
> > Y12), so there is no need to provide the full part number. If there is
> > a need to distinguish imx327 vs imx290 in userspace, then I'd propose
> > just using the base model identifier.
>
> Agreed.
>
> > > drivers/media/i2c/imx290.c | 58 ++++++++++++++++++++++++++++++++++++--
> > > 1 file changed, 55 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index 1cfdd700bca5..0bfbce8853e6 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -173,10 +173,13 @@ enum imx290_colour_variant {
> > > enum imx290_model {
> > > IMX290_MODEL_IMX290LQR,
> > > IMX290_MODEL_IMX290LLR,
> > > + IMX290_MODEL_IMX327LQR,
> > > };
> > >
> > > struct imx290_model_info {
> > > enum imx290_colour_variant colour_variant;
> > > + enum imx290_model model;
> > > + const char *name;
> > > };
> > >
> > > enum imx290_clk_freq {
> > > @@ -272,10 +275,14 @@ static const struct imx290_regval imx290_global_init_settings[] = {
> > > { IMX290_WINWV, 1097 },
> > > { IMX290_XSOUTSEL, IMX290_XSOUTSEL_XVSOUTSEL_VSYNC |
> > > IMX290_XSOUTSEL_XHSOUTSEL_HSYNC },
> > > - { IMX290_REG_8BIT(0x300f), 0x00 },
> > > - { IMX290_REG_8BIT(0x3010), 0x21 },
> > > + { IMX290_REG_8BIT(0x3011), 0x02 },
>
> This change should be mentioned in the commit message.
imx290 datasheet says 3011h should be "Fixed to 00h", which is also the default.
imx327 datasheet says "Set to 02h", which differs from the default.
(Updated in v3 from 0Ah to 02h)
So this should be in imx290_global_init_settings_327, not global_init_settings.
> > > { IMX290_REG_8BIT(0x3012), 0x64 },
> > > { IMX290_REG_8BIT(0x3013), 0x00 },
> > > +};
> > > +
> > > +static const struct imx290_regval imx290_global_init_settings_290[] = {
> > > + { IMX290_REG_8BIT(0x300f), 0x00 },
> > > + { IMX290_REG_8BIT(0x3010), 0x21 },
> > > { IMX290_REG_8BIT(0x3016), 0x09 },
> > > { IMX290_REG_8BIT(0x3070), 0x02 },
> > > { IMX290_REG_8BIT(0x3071), 0x11 },
> > > @@ -328,6 +335,12 @@ static const struct imx290_regval xclk_regs[][IMX290_NUM_CLK_REGS] = {
> > > },
> > > };
> > >
> > > +static const struct imx290_regval imx290_global_init_settings_327[] = {
> > > + { IMX290_REG_8BIT(0x309e), 0x4A },
> > > + { IMX290_REG_8BIT(0x309f), 0x4A },
> > > + { IMX290_REG_8BIT(0x313b), 0x61 },
>
> Lowercase hex constants pleasea.
>
> > > +};
> > > +
> > > static const struct imx290_regval imx290_1080p_settings[] = {
> > > /* mode settings */
> > > { IMX290_WINWV_OB, 12 },
> > > @@ -999,9 +1012,11 @@ 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;
> > >
> > > - /* Set init register settings */
> > > + /* Set common init register settings */
> > > ret = imx290_set_register_array(imx290, imx290_global_init_settings,
> > > ARRAY_SIZE(imx290_global_init_settings));
> > > if (ret < 0) {
> > > @@ -1009,6 +1024,28 @@ static int imx290_start_streaming(struct imx290 *imx290,
> > > return ret;
> > > }
> > >
> > > + switch (imx290->model->model) {
> > > + case IMX290_MODEL_IMX290LQR:
> > > + case IMX290_MODEL_IMX290LLR:
> > > + regs = imx290_global_init_settings_290;
> > > + reg_num = ARRAY_SIZE(imx290_global_init_settings_290);
> > > + break;
> > > + case IMX290_MODEL_IMX327LQR:
> > > + 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->model->model);
>
> This should never happen, so you can drop the message.
>
> > > + return -EINVAL;
> > > + }
> >
> > switch/case here, or add a pointer to struct imx290_model_info?
>
> Do you mean a pointer to the model-specific init regs array? I like the
> idea. The size would need to be added too (unless we switch to
> terminating those arrays with a sentinel).
Yes, I meant along the lines of:
struct imx290_model_info {
enum imx290_colour_variant colour_variant;
enum imx290_model model;
const char *name;
const struct imx290_regval *init_regs;
unsigned int num_init_regs;
};
static const struct imx290_model_info imx290_models[] = {
[IMX290_MODEL_IMX290LQR] = {
.colour_variant = IMX290_VARIANT_COLOUR,
.model = IMX290_MODEL_IMX290LQR,
.name = "imx290lqr",
.init_regs = imx290_global_init_settings_290,
.num_init_regs = ARRAY_SIZE(imx290_global_init_settings_290),
},
...
if (imx290->model->init_regs) {
ret = imx290_set_register_array(imx290, imx290->model->init_regs,
imx290->model->num_init_regs);
if (ret < 0) {
dev_err(imx290->dev, "Could not set init registers\n");
return ret;
}
}
(sorry for the mess with indentation)
As both need model specific init regs we might be able to skip the "if
(imx290->model->init_regs)" check - I tend to think better safe than
sorry.
Noticed in passing, we have a comment [1] around creating the control
for V4L2_CID_ANALOGUE_GAIN that the IMX327 and IMX462 have a max
analogue gain of 29.4dB (value 98) vs IMX290 30dB (value 100), and
ignoring that until support for the other sensors is added. Seeing as
you're adding that support, it would be nice to fix that up as part of
this series.
Dave
[1] https://git.linuxtv.org/sailus/media_tree.git/tree/drivers/media/i2c/imx290.c#n673
> > Keeping all the configuration for the different models in struct
> > imx290_model_info has an appeal to me.
> >
> > > +
> > > + /* Set init register settings */
>
> /* Set device-specific init register settings */
>
> > > + ret = imx290_set_register_array(imx290, regs, reg_num);
> > > + if (ret < 0) {
> > > + dev_err(imx290->dev, "Could not set init registers\n");
I've just noticed this is exactly the same error message as for the
common registers. It'd be nice to keep them unique.
> > > + return ret;
> > > + }
> > > +
> > > /* Set clock parameters based on mode and xclk */
> > > ret = imx290_set_clock(imx290);
> > > if (ret < 0) {
> > > @@ -1479,9 +1516,18 @@ static s64 imx290_check_link_freqs(const struct imx290 *imx290,
> > > static const struct imx290_model_info imx290_models[] = {
> > > [IMX290_MODEL_IMX290LQR] = {
> > > .colour_variant = IMX290_VARIANT_COLOUR,
> > > + .model = IMX290_MODEL_IMX290LQR,
> > > + .name = "imx290lqr",
> > > },
> > > [IMX290_MODEL_IMX290LLR] = {
> > > .colour_variant = IMX290_VARIANT_MONO,
> > > + .model = IMX290_MODEL_IMX290LLR,
> > > + .name = "imx290llr",
> > > + },
> > > + [IMX290_MODEL_IMX327LQR] = {
> > > + .colour_variant = IMX290_VARIANT_COLOUR,
> > > + .model = IMX290_MODEL_IMX327LQR,
> > > + .name = "imx327lqr",
> > > },
> > > };
> > >
> > > @@ -1496,6 +1542,9 @@ static const struct of_device_id imx290_of_match[] = {
> > > }, {
> > > .compatible = "sony,imx290llr",
> > > .data = &imx290_models[IMX290_MODEL_IMX290LLR],
> > > + }, {
> > > + .compatible = "sony,imx327lqr",
> > > + .data = &imx290_models[IMX290_MODEL_IMX327LQR],
> > > },
> > > { /* sentinel */ },
> > > };
> > > @@ -1630,6 +1679,9 @@ static int imx290_probe(struct i2c_client *client)
> > > if (ret)
> > > goto err_pm;
> > >
> > > + v4l2_i2c_subdev_set_name(&imx290->sd, client,
> > > + imx290->model->name, NULL);
> > > +
> > > /*
> > > * Finally, register the V4L2 subdev. This must be done after
> > > * initializing everything as the subdev can be used immediately after
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] media: i2c: imx290: Add support for imx327 variant
2023-02-07 12:40 ` Dave Stevenson
@ 2023-02-07 15:17 ` Laurent Pinchart
0 siblings, 0 replies; 9+ messages in thread
From: Laurent Pinchart @ 2023-02-07 15:17 UTC (permalink / raw)
To: Dave Stevenson
Cc: Alexander Stein, Manivannan Sadhasivam, Mauro Carvalho Chehab,
Rob Herring, Krzysztof Kozlowski, Shawn Guo, Sascha Hauer,
Fabio Estevam, Pengutronix Kernel Team, linux-media, devicetree
Hi Dave,
On Tue, Feb 07, 2023 at 12:40:16PM +0000, Dave Stevenson wrote:
> On Tue, 7 Feb 2023 at 01:32, Laurent Pinchart wrote:
> > On Mon, Feb 06, 2023 at 02:40:56PM +0000, Dave Stevenson wrote:
> > > On Mon, 6 Feb 2023 at 13:17, Alexander Stein 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>
> > > > ---
> > > > Note: The call to v4l2_i2c_subdev_set_name will change the device name
> > > > shown to userspace. So now 'imx290lqr' will be shown instead of 'imx290'.
> > >
> > > This is going to cause grief as we already have a Pi libcamera
> > > pipeline handler and tuning that relies on the entity name being
> > > "imx290", so changing that is going to cause issues.
> > >
> > > From userspace, the difference between lqr and llr is already reported
> > > via the different colour formats supported (RGGB10 & RGGB12 vs Y10 &
> > > Y12), so there is no need to provide the full part number. If there is
> > > a need to distinguish imx327 vs imx290 in userspace, then I'd propose
> > > just using the base model identifier.
> >
> > Agreed.
> >
> > > > drivers/media/i2c/imx290.c | 58 ++++++++++++++++++++++++++++++++++++--
> > > > 1 file changed, 55 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > > index 1cfdd700bca5..0bfbce8853e6 100644
> > > > --- a/drivers/media/i2c/imx290.c
> > > > +++ b/drivers/media/i2c/imx290.c
> > > > @@ -173,10 +173,13 @@ enum imx290_colour_variant {
> > > > enum imx290_model {
> > > > IMX290_MODEL_IMX290LQR,
> > > > IMX290_MODEL_IMX290LLR,
> > > > + IMX290_MODEL_IMX327LQR,
> > > > };
> > > >
> > > > struct imx290_model_info {
> > > > enum imx290_colour_variant colour_variant;
> > > > + enum imx290_model model;
> > > > + const char *name;
> > > > };
> > > >
> > > > enum imx290_clk_freq {
> > > > @@ -272,10 +275,14 @@ static const struct imx290_regval imx290_global_init_settings[] = {
> > > > { IMX290_WINWV, 1097 },
> > > > { IMX290_XSOUTSEL, IMX290_XSOUTSEL_XVSOUTSEL_VSYNC |
> > > > IMX290_XSOUTSEL_XHSOUTSEL_HSYNC },
> > > > - { IMX290_REG_8BIT(0x300f), 0x00 },
> > > > - { IMX290_REG_8BIT(0x3010), 0x21 },
> > > > + { IMX290_REG_8BIT(0x3011), 0x02 },
> >
> > This change should be mentioned in the commit message.
>
> imx290 datasheet says 3011h should be "Fixed to 00h", which is also the default.
> imx327 datasheet says "Set to 02h", which differs from the default.
> (Updated in v3 from 0Ah to 02h)
>
> So this should be in imx290_global_init_settings_327, not global_init_settings.
>
> > > > { IMX290_REG_8BIT(0x3012), 0x64 },
> > > > { IMX290_REG_8BIT(0x3013), 0x00 },
> > > > +};
> > > > +
> > > > +static const struct imx290_regval imx290_global_init_settings_290[] = {
> > > > + { IMX290_REG_8BIT(0x300f), 0x00 },
> > > > + { IMX290_REG_8BIT(0x3010), 0x21 },
> > > > { IMX290_REG_8BIT(0x3016), 0x09 },
> > > > { IMX290_REG_8BIT(0x3070), 0x02 },
> > > > { IMX290_REG_8BIT(0x3071), 0x11 },
> > > > @@ -328,6 +335,12 @@ static const struct imx290_regval xclk_regs[][IMX290_NUM_CLK_REGS] = {
> > > > },
> > > > };
> > > >
> > > > +static const struct imx290_regval imx290_global_init_settings_327[] = {
> > > > + { IMX290_REG_8BIT(0x309e), 0x4A },
> > > > + { IMX290_REG_8BIT(0x309f), 0x4A },
> > > > + { IMX290_REG_8BIT(0x313b), 0x61 },
> >
> > Lowercase hex constants pleasea.
> >
> > > > +};
> > > > +
> > > > static const struct imx290_regval imx290_1080p_settings[] = {
> > > > /* mode settings */
> > > > { IMX290_WINWV_OB, 12 },
> > > > @@ -999,9 +1012,11 @@ 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;
> > > >
> > > > - /* Set init register settings */
> > > > + /* Set common init register settings */
> > > > ret = imx290_set_register_array(imx290, imx290_global_init_settings,
> > > > ARRAY_SIZE(imx290_global_init_settings));
> > > > if (ret < 0) {
> > > > @@ -1009,6 +1024,28 @@ static int imx290_start_streaming(struct imx290 *imx290,
> > > > return ret;
> > > > }
> > > >
> > > > + switch (imx290->model->model) {
> > > > + case IMX290_MODEL_IMX290LQR:
> > > > + case IMX290_MODEL_IMX290LLR:
> > > > + regs = imx290_global_init_settings_290;
> > > > + reg_num = ARRAY_SIZE(imx290_global_init_settings_290);
> > > > + break;
> > > > + case IMX290_MODEL_IMX327LQR:
> > > > + 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->model->model);
> >
> > This should never happen, so you can drop the message.
> >
> > > > + return -EINVAL;
> > > > + }
> > >
> > > switch/case here, or add a pointer to struct imx290_model_info?
> >
> > Do you mean a pointer to the model-specific init regs array? I like the
> > idea. The size would need to be added too (unless we switch to
> > terminating those arrays with a sentinel).
>
> Yes, I meant along the lines of:
>
> struct imx290_model_info {
> enum imx290_colour_variant colour_variant;
> enum imx290_model model;
> const char *name;
> const struct imx290_regval *init_regs;
> unsigned int num_init_regs;
> };
>
> static const struct imx290_model_info imx290_models[] = {
> [IMX290_MODEL_IMX290LQR] = {
> .colour_variant = IMX290_VARIANT_COLOUR,
> .model = IMX290_MODEL_IMX290LQR,
> .name = "imx290lqr",
> .init_regs = imx290_global_init_settings_290,
> .num_init_regs = ARRAY_SIZE(imx290_global_init_settings_290),
> },
> ...
>
> if (imx290->model->init_regs) {
> ret = imx290_set_register_array(imx290, imx290->model->init_regs,
> imx290->model->num_init_regs);
> if (ret < 0) {
> dev_err(imx290->dev, "Could not set init registers\n");
> return ret;
> }
> }
Looks good to me.
> (sorry for the mess with indentation)
> As both need model specific init regs we might be able to skip the "if
> (imx290->model->init_regs)" check - I tend to think better safe than
> sorry.
>
> Noticed in passing, we have a comment [1] around creating the control
> for V4L2_CID_ANALOGUE_GAIN that the IMX327 and IMX462 have a max
> analogue gain of 29.4dB (value 98) vs IMX290 30dB (value 100), and
> ignoring that until support for the other sensors is added. Seeing as
> you're adding that support, it would be nice to fix that up as part of
> this series.
Good idea :-)
> [1] https://git.linuxtv.org/sailus/media_tree.git/tree/drivers/media/i2c/imx290.c#n673
>
> > > Keeping all the configuration for the different models in struct
> > > imx290_model_info has an appeal to me.
> > >
> > > > +
> > > > + /* Set init register settings */
> >
> > /* Set device-specific init register settings */
> >
> > > > + ret = imx290_set_register_array(imx290, regs, reg_num);
> > > > + if (ret < 0) {
> > > > + dev_err(imx290->dev, "Could not set init registers\n");
>
> I've just noticed this is exactly the same error message as for the
> common registers. It'd be nice to keep them unique.
>
> > > > + return ret;
> > > > + }
> > > > +
> > > > /* Set clock parameters based on mode and xclk */
> > > > ret = imx290_set_clock(imx290);
> > > > if (ret < 0) {
> > > > @@ -1479,9 +1516,18 @@ static s64 imx290_check_link_freqs(const struct imx290 *imx290,
> > > > static const struct imx290_model_info imx290_models[] = {
> > > > [IMX290_MODEL_IMX290LQR] = {
> > > > .colour_variant = IMX290_VARIANT_COLOUR,
> > > > + .model = IMX290_MODEL_IMX290LQR,
> > > > + .name = "imx290lqr",
> > > > },
> > > > [IMX290_MODEL_IMX290LLR] = {
> > > > .colour_variant = IMX290_VARIANT_MONO,
> > > > + .model = IMX290_MODEL_IMX290LLR,
> > > > + .name = "imx290llr",
> > > > + },
> > > > + [IMX290_MODEL_IMX327LQR] = {
> > > > + .colour_variant = IMX290_VARIANT_COLOUR,
> > > > + .model = IMX290_MODEL_IMX327LQR,
> > > > + .name = "imx327lqr",
> > > > },
> > > > };
> > > >
> > > > @@ -1496,6 +1542,9 @@ static const struct of_device_id imx290_of_match[] = {
> > > > }, {
> > > > .compatible = "sony,imx290llr",
> > > > .data = &imx290_models[IMX290_MODEL_IMX290LLR],
> > > > + }, {
> > > > + .compatible = "sony,imx327lqr",
> > > > + .data = &imx290_models[IMX290_MODEL_IMX327LQR],
> > > > },
> > > > { /* sentinel */ },
> > > > };
> > > > @@ -1630,6 +1679,9 @@ static int imx290_probe(struct i2c_client *client)
> > > > if (ret)
> > > > goto err_pm;
> > > >
> > > > + v4l2_i2c_subdev_set_name(&imx290->sd, client,
> > > > + imx290->model->name, NULL);
> > > > +
> > > > /*
> > > > * Finally, register the V4L2 subdev. This must be done after
> > > > * initializing everything as the subdev can be used immediately after
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-02-07 15:17 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-06 13:17 [PATCH v2 0/2] media: i2c: imx290: imx327 support Alexander Stein
2023-02-06 13:17 ` [PATCH v2 1/2] media: dt-bindings: media: i2c: Add imx327 version to IMX327 bindings Alexander Stein
2023-02-07 1:27 ` Laurent Pinchart
2023-02-06 13:17 ` [PATCH v2 2/2] media: i2c: imx290: Add support for imx327 variant Alexander Stein
2023-02-06 14:40 ` Dave Stevenson
2023-02-07 1:32 ` Laurent Pinchart
2023-02-07 12:40 ` Dave Stevenson
2023-02-07 15:17 ` Laurent Pinchart
2023-02-07 2:00 ` Laurent Pinchart
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.