[PATCH 1/3] media: i2c: isl7998x: Enable VACTIVE and HDELAY shadow registers

[PATCH 1/3] media: i2c: isl7998x: Enable VACTIVE and HDELAY shadow registers

[Marek Vasut]

The VACTIVE and HDELAY shadow registers must be enabled for per channel
NTSC/PAL detection to work, enable them.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Michael Tretter <m.tretter@pengutronix.de>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: linux-media@vger.kernel.org
---
 drivers/media/i2c/isl7998x.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/isl7998x.c b/drivers/media/i2c/isl7998x.c
index c7089035bbc10..5f8f11f7a2c1c 100644
--- a/drivers/media/i2c/isl7998x.c
+++ b/drivers/media/i2c/isl7998x.c
@@ -312,13 +312,13 @@
 
 static const struct reg_sequence isl7998x_init_seq_1[] = {
 	{ ISL7998X_REG_P0_SHORT_DIAG_IRQ_EN, 0xff },
-	{ ISL7998X_REG_PX_DEC_SDT(0x1), 0x00 },
+	{ ISL7998X_REG_PX_DEC_SDT(0x1), 0x07 },
 	{ ISL7998X_REG_PX_DEC_SHORT_DET_CTL_1(0x1), 0x03 },
-	{ ISL7998X_REG_PX_DEC_SDT(0x2), 0x00 },
+	{ ISL7998X_REG_PX_DEC_SDT(0x2), 0x07 },
 	{ ISL7998X_REG_PX_DEC_SHORT_DET_CTL_1(0x2), 0x03 },
-	{ ISL7998X_REG_PX_DEC_SDT(0x3), 0x00 },
+	{ ISL7998X_REG_PX_DEC_SDT(0x3), 0x07 },
 	{ ISL7998X_REG_PX_DEC_SHORT_DET_CTL_1(0x3), 0x03 },
-	{ ISL7998X_REG_PX_DEC_SDT(0x4), 0x00 },
+	{ ISL7998X_REG_PX_DEC_SDT(0x4), 0x07 },
 	{ ISL7998X_REG_PX_DEC_SHORT_DET_CTL_1(0x4), 0x03 },
 	{ ISL7998X_REG_P5_LI_ENGINE_CTL, 0x00 },
 	{ ISL7998X_REG_P0_SW_RESET_CTL, 0x1f, 10 },
-- 
2.43.0

[PATCH 2/3] media: i2c: isl7998x: Use 0xf page to program all four pages

[Marek Vasut]

Use page 0xf to program the same value of SDT and short detection
settings into all four pages 1..4 . This reduces the I2C traffic
slightly.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Michael Tretter <m.tretter@pengutronix.de>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: linux-media@vger.kernel.org
---
 drivers/media/i2c/isl7998x.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/isl7998x.c b/drivers/media/i2c/isl7998x.c
index 5f8f11f7a2c1c..7d2799e498520 100644
--- a/drivers/media/i2c/isl7998x.c
+++ b/drivers/media/i2c/isl7998x.c
@@ -312,14 +312,8 @@
 
 static const struct reg_sequence isl7998x_init_seq_1[] = {
 	{ ISL7998X_REG_P0_SHORT_DIAG_IRQ_EN, 0xff },
-	{ ISL7998X_REG_PX_DEC_SDT(0x1), 0x07 },
-	{ ISL7998X_REG_PX_DEC_SHORT_DET_CTL_1(0x1), 0x03 },
-	{ ISL7998X_REG_PX_DEC_SDT(0x2), 0x07 },
-	{ ISL7998X_REG_PX_DEC_SHORT_DET_CTL_1(0x2), 0x03 },
-	{ ISL7998X_REG_PX_DEC_SDT(0x3), 0x07 },
-	{ ISL7998X_REG_PX_DEC_SHORT_DET_CTL_1(0x3), 0x03 },
-	{ ISL7998X_REG_PX_DEC_SDT(0x4), 0x07 },
-	{ ISL7998X_REG_PX_DEC_SHORT_DET_CTL_1(0x4), 0x03 },
+	{ ISL7998X_REG_PX_DEC_SDT(0xf), 0x07 },
+	{ ISL7998X_REG_PX_DEC_SHORT_DET_CTL_1(0xf), 0x03 },
 	{ ISL7998X_REG_P5_LI_ENGINE_CTL, 0x00 },
 	{ ISL7998X_REG_P0_SW_RESET_CTL, 0x1f, 10 },
 	{ ISL7998X_REG_P0_IO_BUFFER_CTL, 0x00 },
-- 
2.43.0

[PATCH 3/3] media: i2c: isl7998x: Implement get_mbus_config

[Marek Vasut]

This is used e.g. by imx6-mipi-csi2.c to determine the CSI2 lane count.
Implement the callback in isl7998x driver so it can be used with i.MX6
CSI2 receiver.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Michael Tretter <m.tretter@pengutronix.de>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: linux-media@vger.kernel.org
---
 drivers/media/i2c/isl7998x.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/media/i2c/isl7998x.c b/drivers/media/i2c/isl7998x.c
index 7d2799e498520..560f4845b5e62 100644
--- a/drivers/media/i2c/isl7998x.c
+++ b/drivers/media/i2c/isl7998x.c
@@ -1045,6 +1045,17 @@ static int isl7998x_set_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int isl7998x_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad,
+				    struct v4l2_mbus_config *cfg)
+{
+	struct isl7998x *isl7998x = sd_to_isl7998x(sd);
+
+	cfg->type = V4L2_MBUS_CSI2_DPHY;
+	cfg->bus.mipi_csi2.num_data_lanes = isl7998x->nr_mipi_lanes;
+
+	return 0;
+}
+
 static int isl7998x_set_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct isl7998x *isl7998x = container_of(ctrl->handler,
@@ -1104,6 +1115,7 @@ static const struct v4l2_subdev_pad_ops isl7998x_subdev_pad_ops = {
 	.enum_frame_size	= isl7998x_enum_frame_size,
 	.get_fmt		= isl7998x_get_fmt,
 	.set_fmt		= isl7998x_set_fmt,
+	.get_mbus_config	= isl7998x_get_mbus_config,
 };
 
 static const struct v4l2_subdev_ops isl7998x_subdev_ops = {
-- 
2.43.0

Re: [PATCH 3/3] media: i2c: isl7998x: Implement get_mbus_config

[Francesco Dolcini]

Hello Marek,

On Sat, Jul 13, 2024 at 05:41:56PM +0200, Marek Vasut wrote:
> This is used e.g. by imx6-mipi-csi2.c to determine the CSI2 lane count.
> Implement the callback in isl7998x driver so it can be used with i.MX6
> CSI2 receiver.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

We had a similar need and we did send a similar patch for ov5640 [1],
however it was rejected. I did not follow-up if anything changed that
makes this patch not required, or maybe I am misunderstanding all of it?
Any comment?

Francesco

[1] https://lore.kernel.org/all/20230306063649.7387-1-marcel@ziswiler.com/

Re: [PATCH 3/3] media: i2c: isl7998x: Implement get_mbus_config

[Marek Vasut]

On 7/14/24 4:33 PM, Francesco Dolcini wrote:
> Hello Marek,

Hi,

> On Sat, Jul 13, 2024 at 05:41:56PM +0200, Marek Vasut wrote:
>> This is used e.g. by imx6-mipi-csi2.c to determine the CSI2 lane count.
>> Implement the callback in isl7998x driver so it can be used with i.MX6
>> CSI2 receiver.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
> 
> We had a similar need and we did send a similar patch for ov5640 [1],
> however it was rejected. I did not follow-up if anything changed that
> makes this patch not required, or maybe I am misunderstanding all of it?
> Any comment?

I am not sure what to make of that discussion, there was a lot of 
discussion, but no conclusion or hint how to move forward . So, what now?

> Francesco
> 
> [1] https://lore.kernel.org/all/20230306063649.7387-1-marcel@ziswiler.com/

Re: [PATCH 3/3] media: i2c: isl7998x: Implement get_mbus_config

[Francesco Dolcini]

On Sun, Jul 14, 2024 at 05:37:10PM +0200, Marek Vasut wrote:
> On 7/14/24 4:33 PM, Francesco Dolcini wrote:
> > On Sat, Jul 13, 2024 at 05:41:56PM +0200, Marek Vasut wrote:
> > > This is used e.g. by imx6-mipi-csi2.c to determine the CSI2 lane count.
> > > Implement the callback in isl7998x driver so it can be used with i.MX6
> > > CSI2 receiver.
> > > 
> > > Signed-off-by: Marek Vasut <marex@denx.de>
> > 
> > We had a similar need and we did send a similar patch for ov5640 [1],
> > however it was rejected. I did not follow-up if anything changed that
> > makes this patch not required, or maybe I am misunderstanding all of it?
> > Any comment?
> 
> I am not sure what to make of that discussion, there was a lot of
> discussion, but no conclusion or hint how to move forward . So, what now?

That discusion was very confusing to me, but given I know little on the
topic I was hoping you could understand better than me ... 

BTW, I am not even sure this patch is about the same issue.

Francesco

Re: [PATCH 1/3] media: i2c: isl7998x: Enable VACTIVE and HDELAY shadow registers

[Marek Vasut]

On 7/13/24 5:41 PM, Marek Vasut wrote:
> The VACTIVE and HDELAY shadow registers must be enabled for per channel
> NTSC/PAL detection to work, enable them.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: Michael Tretter <m.tretter@pengutronix.de>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: linux-media@vger.kernel.org
> ---
>   drivers/media/i2c/isl7998x.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/isl7998x.c b/drivers/media/i2c/isl7998x.c
> index c7089035bbc10..5f8f11f7a2c1c 100644
> --- a/drivers/media/i2c/isl7998x.c
> +++ b/drivers/media/i2c/isl7998x.c
> @@ -312,13 +312,13 @@
>   
>   static const struct reg_sequence isl7998x_init_seq_1[] = {
>   	{ ISL7998X_REG_P0_SHORT_DIAG_IRQ_EN, 0xff },
> -	{ ISL7998X_REG_PX_DEC_SDT(0x1), 0x00 },
> +	{ ISL7998X_REG_PX_DEC_SDT(0x1), 0x07 },
>   	{ ISL7998X_REG_PX_DEC_SHORT_DET_CTL_1(0x1), 0x03 },
> -	{ ISL7998X_REG_PX_DEC_SDT(0x2), 0x00 },
> +	{ ISL7998X_REG_PX_DEC_SDT(0x2), 0x07 },
>   	{ ISL7998X_REG_PX_DEC_SHORT_DET_CTL_1(0x2), 0x03 },
> -	{ ISL7998X_REG_PX_DEC_SDT(0x3), 0x00 },
> +	{ ISL7998X_REG_PX_DEC_SDT(0x3), 0x07 },
>   	{ ISL7998X_REG_PX_DEC_SHORT_DET_CTL_1(0x3), 0x03 },
> -	{ ISL7998X_REG_PX_DEC_SDT(0x4), 0x00 },
> +	{ ISL7998X_REG_PX_DEC_SDT(0x4), 0x07 },
>   	{ ISL7998X_REG_PX_DEC_SHORT_DET_CTL_1(0x4), 0x03 },
>   	{ ISL7998X_REG_P5_LI_ENGINE_CTL, 0x00 },
>   	{ ISL7998X_REG_P0_SW_RESET_CTL, 0x1f, 10 },

Any news on these patches ?

Re: [PATCH 1/3] media: i2c: isl7998x: Enable VACTIVE and HDELAY shadow registers

[Michael Tretter]

On Sat, 13 Jul 2024 17:41:54 +0200, Marek Vasut wrote:
> The VACTIVE and HDELAY shadow registers must be enabled for per channel
> NTSC/PAL detection to work, enable them.

I think the description is wrong as the change doesn't affect the
VACTIVE and HDELAY registers. The change sets the standard selection
register to auto detection to always trigger a standard detection during
initialization instead of defaulting to NTSC. querystd explicitly
triggers this behavior.

If the driver always runs standard detection during init, the driver
should also wait for the detection to be finished and update
isl7998x->norm accordingly, because otherwise the standard will be reset
to NTSC.

Maybe we can also get rid of this part from the init sequence and run
the standard detection during the initialization in code?

Michael

> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: Michael Tretter <m.tretter@pengutronix.de>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: linux-media@vger.kernel.org
> ---
>  drivers/media/i2c/isl7998x.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/isl7998x.c b/drivers/media/i2c/isl7998x.c
> index c7089035bbc10..5f8f11f7a2c1c 100644
> --- a/drivers/media/i2c/isl7998x.c
> +++ b/drivers/media/i2c/isl7998x.c
> @@ -312,13 +312,13 @@
>  
>  static const struct reg_sequence isl7998x_init_seq_1[] = {
>  	{ ISL7998X_REG_P0_SHORT_DIAG_IRQ_EN, 0xff },
> -	{ ISL7998X_REG_PX_DEC_SDT(0x1), 0x00 },
> +	{ ISL7998X_REG_PX_DEC_SDT(0x1), 0x07 },
>  	{ ISL7998X_REG_PX_DEC_SHORT_DET_CTL_1(0x1), 0x03 },
> -	{ ISL7998X_REG_PX_DEC_SDT(0x2), 0x00 },
> +	{ ISL7998X_REG_PX_DEC_SDT(0x2), 0x07 },
>  	{ ISL7998X_REG_PX_DEC_SHORT_DET_CTL_1(0x2), 0x03 },
> -	{ ISL7998X_REG_PX_DEC_SDT(0x3), 0x00 },
> +	{ ISL7998X_REG_PX_DEC_SDT(0x3), 0x07 },
>  	{ ISL7998X_REG_PX_DEC_SHORT_DET_CTL_1(0x3), 0x03 },
> -	{ ISL7998X_REG_PX_DEC_SDT(0x4), 0x00 },
> +	{ ISL7998X_REG_PX_DEC_SDT(0x4), 0x07 },
>  	{ ISL7998X_REG_PX_DEC_SHORT_DET_CTL_1(0x4), 0x03 },
>  	{ ISL7998X_REG_P5_LI_ENGINE_CTL, 0x00 },
>  	{ ISL7998X_REG_P0_SW_RESET_CTL, 0x1f, 10 },
> -- 
> 2.43.0
> 
> 

Re: [PATCH 1/3] media: i2c: isl7998x: Enable VACTIVE and HDELAY shadow registers

[Marek Vasut]

On 10/1/24 4:56 PM, Michael Tretter wrote:
> On Sat, 13 Jul 2024 17:41:54 +0200, Marek Vasut wrote:
>> The VACTIVE and HDELAY shadow registers must be enabled for per channel
>> NTSC/PAL detection to work, enable them.
> 
> I think the description is wrong as the change doesn't affect the
> VACTIVE and HDELAY registers. The change sets the standard selection
> register to auto detection to always trigger a standard detection during
> initialization instead of defaulting to NTSC. querystd explicitly
> triggers this behavior.

It does, commit message fixed.

> If the driver always runs standard detection during init, the driver
> should also wait for the detection to be finished and update
> isl7998x->norm accordingly, because otherwise the standard will be reset
> to NTSC.
> 
> Maybe we can also get rid of this part from the init sequence and run
> the standard detection during the initialization in code?
Maybe the init sequence is OK as-is and this and the follow up patches 
should be discarded ? Users can run QUERYSTD to trigger the standard 
detection when needed ?