diff mbox series

[v2,02/18] media: i2c: rdacm20: Enable noise immunity

Message ID 20210315131512.133720-3-jacopo+renesas@jmondi.org (mailing list archive)
State New, archived
Headers show
Series media: gmsl: Reliability improvement | expand

Commit Message

Jacopo Mondi March 15, 2021, 1:14 p.m. UTC
Enable the noise immunity threshold at the end of the rdacm20
initialization routine.

The rdacm20 camera module has been so far tested with a startup
delay that allowed the embedded MCU to program the serializer. If
the initialization routine is run before the MCU programs the
serializer and the image sensor and their addresses gets changed
by the rdacm20 driver it is required to manually enable the noise
immunity threshold to make the communication on the control channel
more reliable.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm20.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart March 15, 2021, 9:37 p.m. UTC | #1
Hi Jacopo,

Thank you for the patch.

On Mon, Mar 15, 2021 at 02:14:56PM +0100, Jacopo Mondi wrote:
> Enable the noise immunity threshold at the end of the rdacm20
> initialization routine.
> 
> The rdacm20 camera module has been so far tested with a startup
> delay that allowed the embedded MCU to program the serializer. If
> the initialization routine is run before the MCU programs the
> serializer and the image sensor and their addresses gets changed
> by the rdacm20 driver it is required to manually enable the noise
> immunity threshold to make the communication on the control channel
> more reliable.

I'm still worried by the race with the MCU. Any update on dumping the
MCU configuration to check what it initializes ?

> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/rdacm20.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 90eb73f0e6e9..f7fd5ae955d0 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -541,7 +541,13 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  
>  	dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n");
>  
> -	return 0;
> +	/*
> +	 * Set reverse channel high threshold to increase noise immunity.
> +	 *
> +	 * This should be compensated by increasing the reverse channel
> +	 * amplitude on the remote deserializer side.
> +	 */
> +	return max9271_set_high_threshold(&dev->serializer, true);
>  }
>  
>  static int rdacm20_probe(struct i2c_client *client)
Jacopo Mondi March 16, 2021, 12:56 p.m. UTC | #2
Hi Laurent,

On Mon, Mar 15, 2021 at 11:37:26PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Mar 15, 2021 at 02:14:56PM +0100, Jacopo Mondi wrote:
> > Enable the noise immunity threshold at the end of the rdacm20
> > initialization routine.
> >
> > The rdacm20 camera module has been so far tested with a startup
> > delay that allowed the embedded MCU to program the serializer. If
> > the initialization routine is run before the MCU programs the
> > serializer and the image sensor and their addresses gets changed
> > by the rdacm20 driver it is required to manually enable the noise
> > immunity threshold to make the communication on the control channel
> > more reliable.
>
> I'm still worried by the race with the MCU. Any update on dumping the
> MCU configuration to check what it initializes ?
>

Not yet, you're right ...

I mainly focused on testing with rdacm21, what if I strip the rdacm20
changes out from this series ? I will have to keep the init()
operation introduction to maintain compatibility with max9286 changes,
and in case of no regressions, we can keep the 8 seconds delay in the
.dtsi. However it will break upstream support on Eagle for rdacm20 as
we don't have a regulator where to insert the startup delay there, and
a downstream patch that waits for 8 seconds in the deserializer driver
should be used instead...

> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/rdacm20.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> > index 90eb73f0e6e9..f7fd5ae955d0 100644
> > --- a/drivers/media/i2c/rdacm20.c
> > +++ b/drivers/media/i2c/rdacm20.c
> > @@ -541,7 +541,13 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
> >
> >  	dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n");
> >
> > -	return 0;
> > +	/*
> > +	 * Set reverse channel high threshold to increase noise immunity.
> > +	 *
> > +	 * This should be compensated by increasing the reverse channel
> > +	 * amplitude on the remote deserializer side.
> > +	 */
> > +	return max9271_set_high_threshold(&dev->serializer, true);
> >  }
> >
> >  static int rdacm20_probe(struct i2c_client *client)
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart March 16, 2021, 7:24 p.m. UTC | #3
Hi Jacopo,

On Tue, Mar 16, 2021 at 01:56:07PM +0100, Jacopo Mondi wrote:
> On Mon, Mar 15, 2021 at 11:37:26PM +0200, Laurent Pinchart wrote:
> > On Mon, Mar 15, 2021 at 02:14:56PM +0100, Jacopo Mondi wrote:
> > > Enable the noise immunity threshold at the end of the rdacm20
> > > initialization routine.
> > >
> > > The rdacm20 camera module has been so far tested with a startup
> > > delay that allowed the embedded MCU to program the serializer. If
> > > the initialization routine is run before the MCU programs the
> > > serializer and the image sensor and their addresses gets changed
> > > by the rdacm20 driver it is required to manually enable the noise
> > > immunity threshold to make the communication on the control channel
> > > more reliable.
> >
> > I'm still worried by the race with the MCU. Any update on dumping the
> > MCU configuration to check what it initializes ?
> 
> Not yet, you're right ...
> 
> I mainly focused on testing with rdacm21, what if I strip the rdacm20
> changes out from this series ? I will have to keep the init()
> operation introduction to maintain compatibility with max9286 changes,
> and in case of no regressions, we can keep the 8 seconds delay in the
> .dtsi. However it will break upstream support on Eagle for rdacm20 as
> we don't have a regulator where to insert the startup delay there, and
> a downstream patch that waits for 8 seconds in the deserializer driver
> should be used instead...

I don't think the rdacm20 changes need to wait. Even this one could be
merged as-is, as long as we consider it to be a temporary workaround and
don't build anything on top that would make it more difficult to address
the issue properly (a TODO comment in the code could help).

> > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  drivers/media/i2c/rdacm20.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> > > index 90eb73f0e6e9..f7fd5ae955d0 100644
> > > --- a/drivers/media/i2c/rdacm20.c
> > > +++ b/drivers/media/i2c/rdacm20.c
> > > @@ -541,7 +541,13 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
> > >
> > >  	dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n");
> > >
> > > -	return 0;
> > > +	/*
> > > +	 * Set reverse channel high threshold to increase noise immunity.
> > > +	 *
> > > +	 * This should be compensated by increasing the reverse channel
> > > +	 * amplitude on the remote deserializer side.
> > > +	 */
> > > +	return max9271_set_high_threshold(&dev->serializer, true);
> > >  }
> > >
> > >  static int rdacm20_probe(struct i2c_client *client)
diff mbox series

Patch

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 90eb73f0e6e9..f7fd5ae955d0 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -541,7 +541,13 @@  static int rdacm20_initialize(struct rdacm20_device *dev)
 
 	dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n");
 
-	return 0;
+	/*
+	 * Set reverse channel high threshold to increase noise immunity.
+	 *
+	 * This should be compensated by increasing the reverse channel
+	 * amplitude on the remote deserializer side.
+	 */
+	return max9271_set_high_threshold(&dev->serializer, true);
 }
 
 static int rdacm20_probe(struct i2c_client *client)