[v1] media: ov08x40: Add Signal Integration Adjustments for specific project

  From: Jason Chen <jason.z.chen@intel.com>

Due to certain MIPI hardware designs using overly long cables, there
is a loss of signal strength, resulting in failed signal integration.
The patch includes changes to adjust the driving strength in the register
settings for a specific project.

Signed-off-by: Jason Chen <jason.z.chen@intel.com>
---
 drivers/media/i2c/ov08x40.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
  

Quoting Chen, Jason Z (2023-10-20 05:09:08)
> From: Jason Chen <jason.z.chen@intel.com>
> 
> Due to certain MIPI hardware designs using overly long cables, there
> is a loss of signal strength, resulting in failed signal integration.
> The patch includes changes to adjust the driving strength in the register
> settings for a specific project.

For a specific project? Will this now affect 'all' projects using this
sensor? Will that be adverse in anyway?

Do 'short' cables still function with this patch?

> 
> Signed-off-by: Jason Chen <jason.z.chen@intel.com>
> ---
>  drivers/media/i2c/ov08x40.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov08x40.c b/drivers/media/i2c/ov08x40.c
> index 0b3b906ebef..4f13e23b325 100644
> --- a/drivers/media/i2c/ov08x40.c
> +++ b/drivers/media/i2c/ov08x40.c
> @@ -160,6 +160,18 @@ static const struct ov08x40_reg mipi_data_rate_800mbps[] = {
>         {0x6002, 0x2e},
>  };
>  
> +static const struct ov08x40_reg mipi_si_changed_regs[] = {
> +       {0x481b, 0x2c}, /* HS Exit: Data Tx TEOT - reducing 8ns */
> +       {0x4826, 0x42}, /* HS Entry: Data Tx TREOT - raising 8ns */
> +       {0x4829, 0x54}, /* HS Exit: Data Tx TREOT - reducing 8ns */
> +       {0x4885, 0x1f}, /* driving strength change */

Are these changes 'relative' to already written values?

> +};
> +
> +struct ov08x40_reg_list si_regs = {
> +       .regs = mipi_si_changed_regs,
> +       .num_of_regs = ARRAY_SIZE(mipi_si_changed_regs),
> +};
> +
>  static const struct ov08x40_reg mode_3856x2416_regs[] = {
>         {0x5000, 0x5d},
>         {0x5001, 0x20},
> @@ -2917,6 +2929,11 @@ static int ov08x40_start_streaming(struct ov08x40 *ov08x)
>                 return ret;
>         }
>  
> +       /* Apply SI change to current project */
> +       reg_list = &si_regs;
> +
> +       ov08x40_write_reg_list(ov08x, reg_list);
> +

This looks odd. Why isn't this just:

	ov08x40_write_reg_list(0v08x, &si_regs);


>         /* Apply default values of current mode */
>         reg_list = &ov08x->cur_mode->reg_list;
>         ret = ov08x40_write_reg_list(ov08x, reg_list);
> -- 
> 2.34.1
>
  

On Fri, Oct 20, 2023 at 02:08:59PM +0100, Kieran Bingham wrote:
> Quoting Chen, Jason Z (2023-10-20 05:09:08)
> > From: Jason Chen <jason.z.chen@intel.com>
> > 
> > Due to certain MIPI hardware designs using overly long cables, there
> > is a loss of signal strength, resulting in failed signal integration.
> > The patch includes changes to adjust the driving strength in the register
> > settings for a specific project.
> 
> For a specific project? Will this now affect 'all' projects using this
> sensor? Will that be adverse in anyway?
> 
> Do 'short' cables still function with this patch?
> 
> > Signed-off-by: Jason Chen <jason.z.chen@intel.com>
> > ---
> >  drivers/media/i2c/ov08x40.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/ov08x40.c b/drivers/media/i2c/ov08x40.c
> > index 0b3b906ebef..4f13e23b325 100644
> > --- a/drivers/media/i2c/ov08x40.c
> > +++ b/drivers/media/i2c/ov08x40.c
> > @@ -160,6 +160,18 @@ static const struct ov08x40_reg mipi_data_rate_800mbps[] = {
> >         {0x6002, 0x2e},
> >  };
> >  
> > +static const struct ov08x40_reg mipi_si_changed_regs[] = {
> > +       {0x481b, 0x2c}, /* HS Exit: Data Tx TEOT - reducing 8ns */
> > +       {0x4826, 0x42}, /* HS Entry: Data Tx TREOT - raising 8ns */
> > +       {0x4829, 0x54}, /* HS Exit: Data Tx TREOT - reducing 8ns */
> > +       {0x4885, 0x1f}, /* driving strength change */
> 
> Are these changes 'relative' to already written values?

Looks like it's time to address MIPI PHY timings properly, by
standardizing DT/ACPI device properties to convey timing information,
and implementing helpers to parse those properties and calculate timing
parameters for drivers.

> > +};
> > +
> > +struct ov08x40_reg_list si_regs = {
> > +       .regs = mipi_si_changed_regs,
> > +       .num_of_regs = ARRAY_SIZE(mipi_si_changed_regs),
> > +};
> > +
> >  static const struct ov08x40_reg mode_3856x2416_regs[] = {
> >         {0x5000, 0x5d},
> >         {0x5001, 0x20},
> > @@ -2917,6 +2929,11 @@ static int ov08x40_start_streaming(struct ov08x40 *ov08x)
> >                 return ret;
> >         }
> >  
> > +       /* Apply SI change to current project */
> > +       reg_list = &si_regs;
> > +
> > +       ov08x40_write_reg_list(ov08x, reg_list);
> > +
> 
> This looks odd. Why isn't this just:
> 
> 	ov08x40_write_reg_list(0v08x, &si_regs);
> 
> >         /* Apply default values of current mode */
> >         reg_list = &ov08x->cur_mode->reg_list;
> >         ret = ov08x40_write_reg_list(ov08x, reg_list);
  

Thanks for the review, Laurent and Kieran

> For a specific project? Will this now affect 'all' projects using this 
> sensor? Will that be adverse in anyway?

This was my initial concern with the patch very first, so I have added the quirk infrastructure to distinguish it using `dmi_check_system`.
However, the sensor vendor replies that if another platform uses the same clock frequency, it shouldn't pose a significant issue.

> Do 'short' cables still function with this patch?

Yes, the change only slightly modifies the HS Tx TEOT/TREOT part.
The value remains within the DPHY tolerance.

> Are these changes 'relative' to already written values?

It's mostly related to DPHY timing changes, as I mentioned in the comments.
Therefore, it's independent of other sensor's capabilitiles like exposure time.

> Looks like it's time to address MIPI PHY timings properly, by standardizing DT/ACPI device properties to convey timing information, and implementing helpers to parse those properties and calculate timing parameters for drivers.

This is a positive direction. However, I'd like to provide some context.
This project encountered issues like insufficient driving strength and inadequate HS preparing time. The sensor vendor only provided relative registers to assist in debugging such issues.
The detail are dependent on sensor vendor willingness to share this confidential information.
So I am not sure if other sensor vendors, like Sony, will provide such information for creating an universal API to access DPHY timing.

> This looks odd. Why isn't this just:

Thank you for this. I'll make the update.



-----Original Message-----
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> 
Sent: Tuesday, October 24, 2023 8:49 AM
To: Kieran Bingham <kieran.bingham@ideasonboard.com>
Cc: Chen, Jason Z <jason.z.chen@intel.com>; Yeh, Andy <andy.yeh@intel.com>; sakari.ailus@linux.intel.com; linux-media@vger.kernel.org; bingbu.cao@linux.intel.com; Zhang, Qingwu <qingwu.zhang@intel.com>
Subject: Re: [PATCH v1] media: ov08x40: Add Signal Integration Adjustments for specific project

On Fri, Oct 20, 2023 at 02:08:59PM +0100, Kieran Bingham wrote:
> Quoting Chen, Jason Z (2023-10-20 05:09:08)
> > From: Jason Chen <jason.z.chen@intel.com>
> > 
> > Due to certain MIPI hardware designs using overly long cables, there 
> > is a loss of signal strength, resulting in failed signal integration.
> > The patch includes changes to adjust the driving strength in the 
> > register settings for a specific project.
> 
> For a specific project? Will this now affect 'all' projects using this 
> sensor? Will that be adverse in anyway?
> 
> Do 'short' cables still function with this patch?
> 
> > Signed-off-by: Jason Chen <jason.z.chen@intel.com>
> > ---
> >  drivers/media/i2c/ov08x40.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/ov08x40.c 
> > b/drivers/media/i2c/ov08x40.c index 0b3b906ebef..4f13e23b325 100644
> > --- a/drivers/media/i2c/ov08x40.c
> > +++ b/drivers/media/i2c/ov08x40.c
> > @@ -160,6 +160,18 @@ static const struct ov08x40_reg mipi_data_rate_800mbps[] = {
> >         {0x6002, 0x2e},
> >  };
> >  
> > +static const struct ov08x40_reg mipi_si_changed_regs[] = {
> > +       {0x481b, 0x2c}, /* HS Exit: Data Tx TEOT - reducing 8ns */
> > +       {0x4826, 0x42}, /* HS Entry: Data Tx TREOT - raising 8ns */
> > +       {0x4829, 0x54}, /* HS Exit: Data Tx TREOT - reducing 8ns */
> > +       {0x4885, 0x1f}, /* driving strength change */
> 
> Are these changes 'relative' to already written values?

Looks like it's time to address MIPI PHY timings properly, by standardizing DT/ACPI device properties to convey timing information, and implementing helpers to parse those properties and calculate timing parameters for drivers.

> > +};
> > +
> > +struct ov08x40_reg_list si_regs = {
> > +       .regs = mipi_si_changed_regs,
> > +       .num_of_regs = ARRAY_SIZE(mipi_si_changed_regs), };
> > +
> >  static const struct ov08x40_reg mode_3856x2416_regs[] = {
> >         {0x5000, 0x5d},
> >         {0x5001, 0x20},
> > @@ -2917,6 +2929,11 @@ static int ov08x40_start_streaming(struct ov08x40 *ov08x)
> >                 return ret;
> >         }
> >  
> > +       /* Apply SI change to current project */
> > +       reg_list = &si_regs;
> > +
> > +       ov08x40_write_reg_list(ov08x, reg_list);
> > +
> 
> This looks odd. Why isn't this just:
> 
> 	ov08x40_write_reg_list(0v08x, &si_regs);
> 
> >         /* Apply default values of current mode */
> >         reg_list = &ov08x->cur_mode->reg_list;
> >         ret = ov08x40_write_reg_list(ov08x, reg_list);

--
Regards,

Laurent Pinchart