* [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
* [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 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
* 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).