[PATCH 00/16] media: i2c: GMSL reliability improvements

[PATCH 00/16] media: i2c: GMSL reliability improvements

[Jacopo Mondi]

Hello,
  this series is based on the most recent media-master with the following
patch applied on top: "media: i2c: rdamc21: Fix warning on u8 cast"

The series contains multiple changes:

- patches from [1-8] contain enhancements to the existing camera module
  drivers. The first 7 patches apply to RDACM20 the same style comments
  received on RDACM21. Nothing controversial should be there.
  A cosmetic fix for the max9286 driver follows.

- patch [9/16] contains a fixup for the RDACM21 camera module that is required
  to avoid sporadic failures during the system initialization.

- From patches [10/16] a rework of the GMSL camera initialization procedure
  starts with 3 patches that prepare for the most substantial change on the
  series.

  The current initialization procedure for a GMSL chip looks like

	MAX9282				RDACM2x

	- probe()
	- init()
	- mux initialize()
		- probe camera 1	- probe()
					- init max9271
					- init image sensor/ISP
					- enable noise immunity
		...

	- camera 1 bound

		- probe camera n	- probe()
					- init max9271
					- init image sensor/ISP
					- enable noise immunity
	- camera n bound
	- all camera have probed
		- Increase channel amplitude

  This implies that all the initial configuration of the camera modules
  which requires several I2C transactions to configure the image sensor and
  the embedded ISP are run without noise immunity enabled.

  On a test of 50 boot cycle the failure rate for the RDACM21 camera module
  is around ~20% which is considerably bad.

  This series implements a different mechanism that allows to run the
  initialization of the camera module with noise immunity enabled, by splitting
  the operations between the usual probe() method and the .init() subdev
  core operation [1]

  The new procedure looks like

	MAX9282				RDACM2x

	- probe()
	- init()
	- mux initialize()
		- probe camera 1	- probe()
					- init max9271
					- enable noise immunity
	- camera 1 bound
	- increase channel amplitude
	- camera 1.init()
					- init image sensor/ISP
	- restore initial channel amplitude

		...
		- probe camera n	- probe()
					- init max9271
					- enable noise immunity
	- camera n bound
	- increase channel amplitude
	- camera n.init()
					- init image sensor/ISP
	- all camera have probed

  This allows to run the image sensor/ISP initialization with noise immunity
  enabled, as that's the part that requires the most I2C writes, being the
  components programmed with register-value tables.

  The same boot tests shows a failure percentage of 13%, considerably lower
  than the current version. It also allows to increase the I2C bit rate to the
  default 339 Kbps for which the setup/hold time are calibrated.

  Bouns points: this helps isolating the MAX9271 initialization and will make
  it easier making the max9271 a self-contained driver as suggested by Mauro.

  [1] All good and glorious BUT: all of this relies on the usage of a subdev
  operation that is considered deprecated. Is it an hard limitation ?

  GMSL is kind of different beast compared to usual subdevices, so it might
  make sense to make an exception in this case ?

Thanks
   j

Jacopo Mondi (16):
  media: i2c: rdacm20: Enable noise immunity
  media: i2c: rdacm20: Embedded 'serializer' field
  media: i2c: rdacm20: Replace goto with a loop
  media: i2c: rdacm20: Report camera module name
  media: i2c: rdacm20: Check return values
  media: i2c: rdacm20: Re-work ov10635 reset
  media: i2c: rdacm2x: Fix wake up delay
  media: i2c: max9286: Adjust parameters indent
  media: i2c: rdacm21: Re-work OV10640 initialization
  media: i2c: max9286: Rename reverse_channel_mv
  media: i2c: max9286: Cache channel amplitude
  media: i2c: max9286: Define high channel amplitude
  media: i2c: rdacm2x: Implement .init() subdev op
  media: i2c: max9286: Initialize remotes when bound
  media: i2c: max9286: Rework comments in .bound()
  media: i2c: gmsl: Use 339Kbps I2C bit-rate

 drivers/media/i2c/max9286.c |  67 ++++++++++------
 drivers/media/i2c/rdacm20.c | 153 +++++++++++++++++++-----------------
 drivers/media/i2c/rdacm21.c |  73 ++++++++++-------
 3 files changed, 167 insertions(+), 126 deletions(-)

--
2.30.0

[PATCH 01/16] media: i2c: rdacm20: Enable noise immunity

[Jacopo Mondi]

Enable the noise immunity threshold at the end of the rdacm20
initialization routine.

The rdcam20 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.

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)
-- 
2.30.0

Re: [PATCH 01/16] media: i2c: rdacm20: Enable noise immunity

[Kieran Bingham]

Hi Jacopo,

On 16/02/2021 17:41, Jacopo Mondi wrote:
> Enable the noise immunity threshold at the end of the rdacm20
> initialization routine.
> 
> The rdcam20 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.
> 

Oh, this is interesting, ... booting up without the delays would be ...
much nicer.

> 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);

Does this work 'out of the box' ? I.e. if this patch is applied, I
assume it is required to remove the regulator delays that I/we have in DT?

Likewise, does that note mean this patch must also be accompanied by the
update in max9286 somehow?

I guess we can't keep 'test bisectability' with this very easily so it
probably doesn't matter too much, the end result will be the interesting
part.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>



>  }
>  
>  static int rdacm20_probe(struct i2c_client *client)
> 

Re: [PATCH 01/16] media: i2c: rdacm20: Enable noise immunity

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Feb 17, 2021 at 12:55:19PM +0000, Kieran Bingham wrote:
> On 16/02/2021 17:41, Jacopo Mondi wrote:
> > Enable the noise immunity threshold at the end of the rdacm20
> > initialization routine.
> > 
> > The rdcam20 camera module has been so far tested with a startup

s/rdcam20/rdacm20/

> > 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.
> 
> Oh, this is interesting, ... booting up without the delays would be ...
> much nicer.

I second that, but I'm a bit worried. The MCU has caused us more pain
than gain, the best way to fix it may be with a desoldering station ;-)
Jokes aside, if we want to start initializing with the serializer before
the MCU completes its initialization, then we'll have a racy process,
with two I2C masters configuring the same device. I don't think anything
good can come out of that :-S

Taking into account the fact that on some platforms we'll want to
implement power management for the cameras, disabling power (possibly
individually) when the cameras are not in use, we'll have to handle the
race carefully, and I'm not sure there any other way than waiting for
the camera to be initialized with an initialization delay after power
up.

Based on this, I'm not concerned about this patch in particular, but
potentially about the series as a whole. I'll comment on individual
patches as applicable.

Regarding this patch, doies the MCU enable high threshold for the
reverse channel as part of its initialization procedure ? Do we have a
full list of what it configures in the MAX9271 ? If so, could we capture
it in a comment in the driver ? That would be very helpful as a
reference.

> > 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);
> 
> Does this work 'out of the box' ? I.e. if this patch is applied, I
> assume it is required to remove the regulator delays that I/we have in DT?
> 
> Likewise, does that note mean this patch must also be accompanied by the
> update in max9286 somehow?
> 
> I guess we can't keep 'test bisectability' with this very easily so it
> probably doesn't matter too much, the end result will be the interesting
> part.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> >  }
> >  
> >  static int rdacm20_probe(struct i2c_client *client)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 01/16] media: i2c: rdacm20: Enable noise immunity

[Jacopo Mondi]

Hi Kieran, Laurent,

On Mon, Feb 22, 2021 at 02:49:34AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Feb 17, 2021 at 12:55:19PM +0000, Kieran Bingham wrote:
> > On 16/02/2021 17:41, Jacopo Mondi wrote:
> > > Enable the noise immunity threshold at the end of the rdacm20
> > > initialization routine.
> > >
> > > The rdcam20 camera module has been so far tested with a startup
>
> s/rdcam20/rdacm20/
>
> > > 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.
> >
> > Oh, this is interesting, ... booting up without the delays would be ...
> > much nicer.
>
> I second that, but I'm a bit worried. The MCU has caused us more pain
> than gain, the best way to fix it may be with a desoldering station ;-)

I wish we could

> Jokes aside, if we want to start initializing with the serializer before
> the MCU completes its initialization, then we'll have a racy process,
> with two I2C masters configuring the same device. I don't think anything
> good can come out of that :-S
>
> Taking into account the fact that on some platforms we'll want to
> implement power management for the cameras, disabling power (possibly
> individually) when the cameras are not in use, we'll have to handle the
> race carefully, and I'm not sure there any other way than waiting for
> the camera to be initialized with an initialization delay after power
> up.

Currently I really cannot tell how long the intialization takes, and
we downstream inserted a 'long enough' 8 seconds delay to accommodate
that, which seems one of those solution that work as long as they
don't work anymore.

One thing to notices is that I tried to interface with the MCU to read
its most basic parameters, like the number of i2c messages sent to the
serializer before programming the image sensor and that's just a mere 3
messages. What I noticed was also that I was not able to talk to the MCU
for 1.5 seconds, which I'm not sure it's because of a startup delay of
because of cross-talks. If that was a startup delay, it would really be
convenient, as we could change the chip addresses immediately and have
the MCU programming being sent to a non-existing id.

>
> Based on this, I'm not concerned about this patch in particular, but
> potentially about the series as a whole. I'll comment on individual
> patches as applicable.
>
> Regarding this patch, doies the MCU enable high threshold for the
> reverse channel as part of its initialization procedure ? Do we have a

we always assumed so, as it was not required for the RDACM20 to start
with a de-serializer low power amplitude and increase it after the
camera has probed like we have to do for the un-programmed RDACM21
with the intiial startup delay (the custom maxim,reverse-channel-microvolt
property serves this purpose)

As a confirmation, if I remove the delay and probe the camera before
the MCU gets to program it, I need to enable the threshold manually
as otherwise I lose the ability to stream from cameras.

All in all, my best bet is that without the delay we get to probe and
re-program the serializer address before the MCU starts it programming
phase. All of this is without synchronization, without any known delay
being described in the camera manual, all based on empirical
deduction and repeated testing. I'll keep the opinion on GMSL as a
techology for myself here, but I think this solution is in-line with
the global quality level of the systems we're working with.

> full list of what it configures in the MAX9271 ? If so, could we capture

Not at the moment but I can investigate dumping that from the MCU as
I've been able to get its 'status' and a command to get its content
should be available

> it in a comment in the driver ? That would be very helpful as a
> reference.
>
> > > 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);
> >
> > Does this work 'out of the box' ? I.e. if this patch is applied, I
> > assume it is required to remove the regulator delays that I/we have in DT?

It doesn't hurt, as if this happen -after- the MCU has programmed the
chip, we're just re-enabling something that was enabled (remember
RDACM20 goes with maxim,reverse-channel-microvol=170 when the dealy
was inserted).

Without the dealy it could be operated as the RDACM21 (start low,
probe+enable threshold, set high).

> >
> > Likewise, does that note mean this patch must also be accompanied by the
> > update in max9286 somehow?
> >

Ee have a DT property to control this already, and the delay+channel
amplitude can be controlled from DTS entirely.

> > I guess we can't keep 'test bisectability' with this very easily so it
> > probably doesn't matter too much, the end result will be the interesting
> > part.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >
> > >  }
> > >
> > >  static int rdacm20_probe(struct i2c_client *client)
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 01/16] media: i2c: rdacm20: Enable noise immunity

[Laurent Pinchart]

Hi Jacopo,

On Mon, Feb 22, 2021 at 03:59:05PM +0100, Jacopo Mondi wrote:
> On Mon, Feb 22, 2021 at 02:49:34AM +0200, Laurent Pinchart wrote:
> > On Wed, Feb 17, 2021 at 12:55:19PM +0000, Kieran Bingham wrote:
> > > On 16/02/2021 17:41, Jacopo Mondi wrote:
> > > > Enable the noise immunity threshold at the end of the rdacm20
> > > > initialization routine.
> > > >
> > > > The rdcam20 camera module has been so far tested with a startup
> >
> > s/rdcam20/rdacm20/
> >
> > > > 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.
> > >
> > > Oh, this is interesting, ... booting up without the delays would be ...
> > > much nicer.
> >
> > I second that, but I'm a bit worried. The MCU has caused us more pain
> > than gain, the best way to fix it may be with a desoldering station ;-)
> 
> I wish we could
> 
> > Jokes aside, if we want to start initializing with the serializer before
> > the MCU completes its initialization, then we'll have a racy process,
> > with two I2C masters configuring the same device. I don't think anything
> > good can come out of that :-S
> >
> > Taking into account the fact that on some platforms we'll want to
> > implement power management for the cameras, disabling power (possibly
> > individually) when the cameras are not in use, we'll have to handle the
> > race carefully, and I'm not sure there any other way than waiting for
> > the camera to be initialized with an initialization delay after power
> > up.
> 
> Currently I really cannot tell how long the intialization takes, and
> we downstream inserted a 'long enough' 8 seconds delay to accommodate
> that, which seems one of those solution that work as long as they
> don't work anymore.
> 
> One thing to notices is that I tried to interface with the MCU to read
> its most basic parameters, like the number of i2c messages sent to the
> serializer before programming the image sensor and that's just a mere 3
> messages. What I noticed was also that I was not able to talk to the MCU
> for 1.5 seconds, which I'm not sure it's because of a startup delay of
> because of cross-talks. If that was a startup delay, it would really be
> convenient, as we could change the chip addresses immediately and have
> the MCU programming being sent to a non-existing id.

Scary :-)

> > Based on this, I'm not concerned about this patch in particular, but
> > potentially about the series as a whole. I'll comment on individual
> > patches as applicable.
> >
> > Regarding this patch, doies the MCU enable high threshold for the
> > reverse channel as part of its initialization procedure ? Do we have a
> 
> we always assumed so, as it was not required for the RDACM20 to start
> with a de-serializer low power amplitude and increase it after the
> camera has probed like we have to do for the un-programmed RDACM21
> with the intiial startup delay (the custom maxim,reverse-channel-microvolt
> property serves this purpose)
> 
> As a confirmation, if I remove the delay and probe the camera before
> the MCU gets to program it, I need to enable the threshold manually
> as otherwise I lose the ability to stream from cameras.
> 
> All in all, my best bet is that without the delay we get to probe and
> re-program the serializer address before the MCU starts it programming
> phase. All of this is without synchronization, without any known delay
> being described in the camera manual, all based on empirical
> deduction and repeated testing. I'll keep the opinion on GMSL as a
> techology for myself here, but I think this solution is in-line with
> the global quality level of the systems we're working with.

Your opinion may have transpired from that last sentence.

> > full list of what it configures in the MAX9271 ? If so, could we capture
> 
> Not at the moment but I can investigate dumping that from the MCU as
> I've been able to get its 'status' and a command to get its content
> should be available

Thanks, that would be very helpful.

> > it in a comment in the driver ? That would be very helpful as a
> > reference.
> >
> > > > 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);
> > >
> > > Does this work 'out of the box' ? I.e. if this patch is applied, I
> > > assume it is required to remove the regulator delays that I/we have in DT?
> 
> It doesn't hurt, as if this happen -after- the MCU has programmed the
> chip, we're just re-enabling something that was enabled (remember
> RDACM20 goes with maxim,reverse-channel-microvol=170 when the dealy
> was inserted).
> 
> Without the dealy it could be operated as the RDACM21 (start low,
> probe+enable threshold, set high).
> 
> > > Likewise, does that note mean this patch must also be accompanied by the
> > > update in max9286 somehow?
> 
> Ee have a DT property to control this already, and the delay+channel
> amplitude can be controlled from DTS entirely.
> 
> > > I guess we can't keep 'test bisectability' with this very easily so it
> > > probably doesn't matter too much, the end result will be the interesting
> > > part.
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > >
> > > >  }
> > > >
> > > >  static int rdacm20_probe(struct i2c_client *client)

-- 
Regards,

Laurent Pinchart

[PATCH 02/16] media: i2c: rdacm20: Embedded 'serializer' field

[Jacopo Mondi]

There's no reason to allocate dynamically the 'serializer' field in
the driver structure.

Embed the field and adjust all its users in the driver.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm20.c | 38 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index f7fd5ae955d0..4d9bac87cba8 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -312,7 +312,7 @@ static const struct ov10635_reg {
 
 struct rdacm20_device {
 	struct device			*dev;
-	struct max9271_device		*serializer;
+	struct max9271_device		serializer;
 	struct i2c_client		*sensor;
 	struct v4l2_subdev		sd;
 	struct media_pad		pad;
@@ -399,7 +399,7 @@ static int rdacm20_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct rdacm20_device *dev = sd_to_rdacm20(sd);
 
-	return max9271_set_serial_link(dev->serializer, enable);
+	return max9271_set_serial_link(&dev->serializer, enable);
 }
 
 static int rdacm20_enum_mbus_code(struct v4l2_subdev *sd,
@@ -456,11 +456,11 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
 	int ret;
 
 	/* Verify communication with the MAX9271: ping to wakeup. */
-	dev->serializer->client->addr = MAX9271_DEFAULT_ADDR;
-	i2c_smbus_read_byte(dev->serializer->client);
+	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
+	i2c_smbus_read_byte(dev->serializer.client);
 
 	/* Serial link disabled during config as it needs a valid pixel clock. */
-	ret = max9271_set_serial_link(dev->serializer, false);
+	ret = max9271_set_serial_link(&dev->serializer, false);
 	if (ret)
 		return ret;
 
@@ -468,35 +468,35 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
 	 *  Ensure that we have a good link configuration before attempting to
 	 *  identify the device.
 	 */
-	max9271_configure_i2c(dev->serializer, MAX9271_I2CSLVSH_469NS_234NS |
-					       MAX9271_I2CSLVTO_1024US |
-					       MAX9271_I2CMSTBT_105KBPS);
+	max9271_configure_i2c(&dev->serializer, MAX9271_I2CSLVSH_469NS_234NS |
+						MAX9271_I2CSLVTO_1024US |
+						MAX9271_I2CMSTBT_105KBPS);
 
-	max9271_configure_gmsl_link(dev->serializer);
+	max9271_configure_gmsl_link(&dev->serializer);
 
-	ret = max9271_verify_id(dev->serializer);
+	ret = max9271_verify_id(&dev->serializer);
 	if (ret < 0)
 		return ret;
 
-	ret = max9271_set_address(dev->serializer, dev->addrs[0]);
+	ret = max9271_set_address(&dev->serializer, dev->addrs[0]);
 	if (ret < 0)
 		return ret;
-	dev->serializer->client->addr = dev->addrs[0];
+	dev->serializer.client->addr = dev->addrs[0];
 
 	/*
 	 * Reset the sensor by cycling the OV10635 reset signal connected to the
 	 * MAX9271 GPIO1 and verify communication with the OV10635.
 	 */
-	ret = max9271_enable_gpios(dev->serializer, MAX9271_GPIO1OUT);
+	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
 	if (ret)
 		return ret;
 
-	ret = max9271_clear_gpios(dev->serializer, MAX9271_GPIO1OUT);
+	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
 	if (ret)
 		return ret;
 	usleep_range(10000, 15000);
 
-	ret = max9271_set_gpios(dev->serializer, MAX9271_GPIO1OUT);
+	ret = max9271_set_gpios(&dev->serializer, MAX9271_GPIO1OUT);
 	if (ret)
 		return ret;
 	usleep_range(10000, 15000);
@@ -560,13 +560,7 @@ static int rdacm20_probe(struct i2c_client *client)
 	if (!dev)
 		return -ENOMEM;
 	dev->dev = &client->dev;
-
-	dev->serializer = devm_kzalloc(&client->dev, sizeof(*dev->serializer),
-				       GFP_KERNEL);
-	if (!dev->serializer)
-		return -ENOMEM;
-
-	dev->serializer->client = client;
+	dev->serializer.client = client;
 
 	ret = of_property_read_u32_array(client->dev.of_node, "reg",
 					 dev->addrs, 2);
-- 
2.30.0

Re: [PATCH 02/16] media: i2c: rdacm20: Embedded 'serializer' field

[Kieran Bingham]

Hi Jacopo,

On 16/02/2021 17:41, Jacopo Mondi wrote:
> There's no reason to allocate dynamically the 'serializer' field in
> the driver structure.
> 
> Embed the field and adjust all its users in the driver.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/rdacm20.c | 38 ++++++++++++++++---------------------
>  1 file changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index f7fd5ae955d0..4d9bac87cba8 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -312,7 +312,7 @@ static const struct ov10635_reg {
>  
>  struct rdacm20_device {
>  	struct device			*dev;
> -	struct max9271_device		*serializer;
> +	struct max9271_device		serializer;
>  	struct i2c_client		*sensor;
>  	struct v4l2_subdev		sd;
>  	struct media_pad		pad;
> @@ -399,7 +399,7 @@ static int rdacm20_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct rdacm20_device *dev = sd_to_rdacm20(sd);
>  
> -	return max9271_set_serial_link(dev->serializer, enable);
> +	return max9271_set_serial_link(&dev->serializer, enable);
>  }
>  
>  static int rdacm20_enum_mbus_code(struct v4l2_subdev *sd,
> @@ -456,11 +456,11 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  	int ret;
>  
>  	/* Verify communication with the MAX9271: ping to wakeup. */
> -	dev->serializer->client->addr = MAX9271_DEFAULT_ADDR;
> -	i2c_smbus_read_byte(dev->serializer->client);
> +	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> +	i2c_smbus_read_byte(dev->serializer.client);
>  
>  	/* Serial link disabled during config as it needs a valid pixel clock. */
> -	ret = max9271_set_serial_link(dev->serializer, false);
> +	ret = max9271_set_serial_link(&dev->serializer, false);
>  	if (ret)
>  		return ret;
>  
> @@ -468,35 +468,35 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  	 *  Ensure that we have a good link configuration before attempting to
>  	 *  identify the device.
>  	 */
> -	max9271_configure_i2c(dev->serializer, MAX9271_I2CSLVSH_469NS_234NS |
> -					       MAX9271_I2CSLVTO_1024US |
> -					       MAX9271_I2CMSTBT_105KBPS);
> +	max9271_configure_i2c(&dev->serializer, MAX9271_I2CSLVSH_469NS_234NS |
> +						MAX9271_I2CSLVTO_1024US |
> +						MAX9271_I2CMSTBT_105KBPS);
>  
> -	max9271_configure_gmsl_link(dev->serializer);
> +	max9271_configure_gmsl_link(&dev->serializer);
>  
> -	ret = max9271_verify_id(dev->serializer);
> +	ret = max9271_verify_id(&dev->serializer);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = max9271_set_address(dev->serializer, dev->addrs[0]);
> +	ret = max9271_set_address(&dev->serializer, dev->addrs[0]);
>  	if (ret < 0)
>  		return ret;
> -	dev->serializer->client->addr = dev->addrs[0];
> +	dev->serializer.client->addr = dev->addrs[0];
>  
>  	/*
>  	 * Reset the sensor by cycling the OV10635 reset signal connected to the
>  	 * MAX9271 GPIO1 and verify communication with the OV10635.
>  	 */
> -	ret = max9271_enable_gpios(dev->serializer, MAX9271_GPIO1OUT);
> +	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
>  	if (ret)
>  		return ret;
>  
> -	ret = max9271_clear_gpios(dev->serializer, MAX9271_GPIO1OUT);
> +	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
>  	if (ret)
>  		return ret;
>  	usleep_range(10000, 15000);
>  
> -	ret = max9271_set_gpios(dev->serializer, MAX9271_GPIO1OUT);
> +	ret = max9271_set_gpios(&dev->serializer, MAX9271_GPIO1OUT);
>  	if (ret)
>  		return ret;
>  	usleep_range(10000, 15000);
> @@ -560,13 +560,7 @@ static int rdacm20_probe(struct i2c_client *client)
>  	if (!dev)
>  		return -ENOMEM;
>  	dev->dev = &client->dev;
> -
> -	dev->serializer = devm_kzalloc(&client->dev, sizeof(*dev->serializer),
> -				       GFP_KERNEL);
> -	if (!dev->serializer)
> -		return -ENOMEM;

Every allocation removed is a win.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


> -
> -	dev->serializer->client = client;
> +	dev->serializer.client = client;
>  
>  	ret = of_property_read_u32_array(client->dev.of_node, "reg",
>  					 dev->addrs, 2);
> 

Re: [PATCH 02/16] media: i2c: rdacm20: Embedded 'serializer' field

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Feb 16, 2021 at 06:41:32PM +0100, Jacopo Mondi wrote:
> There's no reason to allocate dynamically the 'serializer' field in
> the driver structure.
> 
> Embed the field and adjust all its users in the driver.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

This requires making the max9271_device structure definition available
to the rdacm20 and other drivers. Given how tightly coupled they are, I
don't think that's an issue, but let's keep in mind in the future that
the camera drivers should not, as a general rule, peek into the
max9271_device structure directly. It may be nice to add a
max9271_init() function that will initialize the client field, and move
the client->addr assignment to max9271_set_address().

Maybe you've already done so in the rest of the series, I'll find out
soon :-) For this patch,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/rdacm20.c | 38 ++++++++++++++++---------------------
>  1 file changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index f7fd5ae955d0..4d9bac87cba8 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -312,7 +312,7 @@ static const struct ov10635_reg {
>  
>  struct rdacm20_device {
>  	struct device			*dev;
> -	struct max9271_device		*serializer;
> +	struct max9271_device		serializer;
>  	struct i2c_client		*sensor;
>  	struct v4l2_subdev		sd;
>  	struct media_pad		pad;
> @@ -399,7 +399,7 @@ static int rdacm20_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct rdacm20_device *dev = sd_to_rdacm20(sd);
>  
> -	return max9271_set_serial_link(dev->serializer, enable);
> +	return max9271_set_serial_link(&dev->serializer, enable);
>  }
>  
>  static int rdacm20_enum_mbus_code(struct v4l2_subdev *sd,
> @@ -456,11 +456,11 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  	int ret;
>  
>  	/* Verify communication with the MAX9271: ping to wakeup. */
> -	dev->serializer->client->addr = MAX9271_DEFAULT_ADDR;
> -	i2c_smbus_read_byte(dev->serializer->client);
> +	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> +	i2c_smbus_read_byte(dev->serializer.client);
>  
>  	/* Serial link disabled during config as it needs a valid pixel clock. */
> -	ret = max9271_set_serial_link(dev->serializer, false);
> +	ret = max9271_set_serial_link(&dev->serializer, false);
>  	if (ret)
>  		return ret;
>  
> @@ -468,35 +468,35 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  	 *  Ensure that we have a good link configuration before attempting to
>  	 *  identify the device.
>  	 */
> -	max9271_configure_i2c(dev->serializer, MAX9271_I2CSLVSH_469NS_234NS |
> -					       MAX9271_I2CSLVTO_1024US |
> -					       MAX9271_I2CMSTBT_105KBPS);
> +	max9271_configure_i2c(&dev->serializer, MAX9271_I2CSLVSH_469NS_234NS |
> +						MAX9271_I2CSLVTO_1024US |
> +						MAX9271_I2CMSTBT_105KBPS);
>  
> -	max9271_configure_gmsl_link(dev->serializer);
> +	max9271_configure_gmsl_link(&dev->serializer);
>  
> -	ret = max9271_verify_id(dev->serializer);
> +	ret = max9271_verify_id(&dev->serializer);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = max9271_set_address(dev->serializer, dev->addrs[0]);
> +	ret = max9271_set_address(&dev->serializer, dev->addrs[0]);
>  	if (ret < 0)
>  		return ret;
> -	dev->serializer->client->addr = dev->addrs[0];
> +	dev->serializer.client->addr = dev->addrs[0];
>  
>  	/*
>  	 * Reset the sensor by cycling the OV10635 reset signal connected to the
>  	 * MAX9271 GPIO1 and verify communication with the OV10635.
>  	 */
> -	ret = max9271_enable_gpios(dev->serializer, MAX9271_GPIO1OUT);
> +	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
>  	if (ret)
>  		return ret;
>  
> -	ret = max9271_clear_gpios(dev->serializer, MAX9271_GPIO1OUT);
> +	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
>  	if (ret)
>  		return ret;
>  	usleep_range(10000, 15000);
>  
> -	ret = max9271_set_gpios(dev->serializer, MAX9271_GPIO1OUT);
> +	ret = max9271_set_gpios(&dev->serializer, MAX9271_GPIO1OUT);
>  	if (ret)
>  		return ret;
>  	usleep_range(10000, 15000);
> @@ -560,13 +560,7 @@ static int rdacm20_probe(struct i2c_client *client)
>  	if (!dev)
>  		return -ENOMEM;
>  	dev->dev = &client->dev;
> -
> -	dev->serializer = devm_kzalloc(&client->dev, sizeof(*dev->serializer),
> -				       GFP_KERNEL);
> -	if (!dev->serializer)
> -		return -ENOMEM;
> -
> -	dev->serializer->client = client;
> +	dev->serializer.client = client;
>  
>  	ret = of_property_read_u32_array(client->dev.of_node, "reg",
>  					 dev->addrs, 2);

-- 
Regards,

Laurent Pinchart

[PATCH 03/16] media: i2c: rdacm20: Replace goto with a loop

[Jacopo Mondi]

During the camera module initialization the image sensor PID is read to
verify it can correctly be identified. The current implementation is
rather confused and uses a loop implemented with a label and a goto.

Replace it with a more compact for() loop.

No functional changes intended.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm20.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 4d9bac87cba8..6504ed0bd3bc 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -59,6 +59,8 @@
  */
 #define OV10635_PIXEL_RATE		(44000000)
 
+#define OV10635_PID_TIMEOUT		3
+
 static const struct ov10635_reg {
 	u16	reg;
 	u8	val;
@@ -452,7 +454,7 @@ static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
 
 static int rdacm20_initialize(struct rdacm20_device *dev)
 {
-	unsigned int retry = 3;
+	unsigned int i;
 	int ret;
 
 	/* Verify communication with the MAX9271: ping to wakeup. */
@@ -501,23 +503,14 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
 		return ret;
 	usleep_range(10000, 15000);
 
-again:
-	ret = ov10635_read16(dev, OV10635_PID);
-	if (ret < 0) {
-		if (retry--)
-			goto again;
-
-		dev_err(dev->dev, "OV10635 ID read failed (%d)\n",
-			ret);
-		return -ENXIO;
+	for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
+		ret = ov10635_read16(dev, OV10635_PID);
+		if (ret == OV10635_VERSION)
+			break;
+		usleep_range(1000, 2000);
 	}
-
-	if (ret != OV10635_VERSION) {
-		if (retry--)
-			goto again;
-
-		dev_err(dev->dev, "OV10635 ID mismatch (0x%04x)\n",
-			ret);
+	if (i == OV10635_PID_TIMEOUT) {
+		dev_err(dev->dev, "OV10635 ID read failed (%d)\n", ret);
 		return -ENXIO;
 	}
 
-- 
2.30.0

Re: [PATCH 03/16] media: i2c: rdacm20: Replace goto with a loop

[Kieran Bingham]

Hi Jacopo,

On 16/02/2021 17:41, Jacopo Mondi wrote:
> During the camera module initialization the image sensor PID is read to
> verify it can correctly be identified. The current implementation is
> rather confused and uses a loop implemented with a label and a goto.
> 
> Replace it with a more compact for() loop.
> 
> No functional changes intended.

I think there is a functional change in here, but I almost like it.

Before, if the read was successful, it would check to see if the
OV10635_PID == OV10635_VERSION, and if not it would print that the read
was successful but a mismatch.

Now - it will retry again instead, and if at the end of the retries it
still fails then it's a failure.

This means we perhaps don't get told if the device id is not correct in
the same way, but it also means that if the VERSION was not correct
because of a read error (which I believe i've seen occur), it will retry.

Because there is a functional change you might want to update the
commit, but I still think this is a good change overall.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/rdacm20.c | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 4d9bac87cba8..6504ed0bd3bc 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -59,6 +59,8 @@
>   */
>  #define OV10635_PIXEL_RATE		(44000000)
>  
> +#define OV10635_PID_TIMEOUT		3
> +
>  static const struct ov10635_reg {
>  	u16	reg;
>  	u8	val;
> @@ -452,7 +454,7 @@ static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
>  
>  static int rdacm20_initialize(struct rdacm20_device *dev)
>  {
> -	unsigned int retry = 3;
> +	unsigned int i;
>  	int ret;
>  
>  	/* Verify communication with the MAX9271: ping to wakeup. */
> @@ -501,23 +503,14 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  		return ret;
>  	usleep_range(10000, 15000);
>  
> -again:
> -	ret = ov10635_read16(dev, OV10635_PID);
> -	if (ret < 0) {
> -		if (retry--)
> -			goto again;
> -
> -		dev_err(dev->dev, "OV10635 ID read failed (%d)\n",
> -			ret);
> -		return -ENXIO;
> +	for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
> +		ret = ov10635_read16(dev, OV10635_PID);
> +		if (ret == OV10635_VERSION)
> +			break;
> +		usleep_range(1000, 2000);
>  	}
> -
> -	if (ret != OV10635_VERSION) {
> -		if (retry--)
> -			goto again;
> -
> -		dev_err(dev->dev, "OV10635 ID mismatch (0x%04x)\n",
> -			ret);
> +	if (i == OV10635_PID_TIMEOUT) {
> +		dev_err(dev->dev, "OV10635 ID read failed (%d)\n", ret);
>  		return -ENXIO;
>  	}
>  
> 

Re: [PATCH 03/16] media: i2c: rdacm20: Replace goto with a loop

[Laurent Pinchart]

Hi Jacopo,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

On Wed, Feb 17, 2021 at 01:01:26PM +0000, Kieran Bingham wrote:
> On 16/02/2021 17:41, Jacopo Mondi wrote:
> > During the camera module initialization the image sensor PID is read to
> > verify it can correctly be identified. The current implementation is
> > rather confused and uses a loop implemented with a label and a goto.
> > 
> > Replace it with a more compact for() loop.
> > 
> > No functional changes intended.
> 
> I think there is a functional change in here, but I almost like it.
> 
> Before, if the read was successful, it would check to see if the
> OV10635_PID == OV10635_VERSION, and if not it would print that the read
> was successful but a mismatch.
> 
> Now - it will retry again instead, and if at the end of the retries it
> still fails then it's a failure.
> 
> This means we perhaps don't get told if the device id is not correct in
> the same way, but it also means that if the VERSION was not correct
> because of a read error (which I believe i've seen occur), it will retry.

I was going to ask about that, whether we can have a successful I2C read
operation that would return incorrect data. If we do, aren't we screwed
? If there's a non-negligible probability that reads will return
incorrect data without any way to know about it (for other registers
than the version register of course), then I would consider that writes
could fail the same way, and that would mean an unusable device,
wouldn't it ?

If, on the other hand, read failures can always (or nearly always,
ignoring space neutrinos and similar niceties) be detected, then I think
we should avoid the functional change.

> Because there is a functional change you might want to update the
> commit, but I still think this is a good change overall.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/rdacm20.c | 27 ++++++++++-----------------
> >  1 file changed, 10 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> > index 4d9bac87cba8..6504ed0bd3bc 100644
> > --- a/drivers/media/i2c/rdacm20.c
> > +++ b/drivers/media/i2c/rdacm20.c
> > @@ -59,6 +59,8 @@
> >   */
> >  #define OV10635_PIXEL_RATE		(44000000)
> >  
> > +#define OV10635_PID_TIMEOUT		3
> > +
> >  static const struct ov10635_reg {
> >  	u16	reg;
> >  	u8	val;
> > @@ -452,7 +454,7 @@ static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
> >  
> >  static int rdacm20_initialize(struct rdacm20_device *dev)
> >  {
> > -	unsigned int retry = 3;
> > +	unsigned int i;
> >  	int ret;
> >  
> >  	/* Verify communication with the MAX9271: ping to wakeup. */
> > @@ -501,23 +503,14 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
> >  		return ret;
> >  	usleep_range(10000, 15000);
> >  
> > -again:
> > -	ret = ov10635_read16(dev, OV10635_PID);
> > -	if (ret < 0) {
> > -		if (retry--)
> > -			goto again;
> > -
> > -		dev_err(dev->dev, "OV10635 ID read failed (%d)\n",
> > -			ret);
> > -		return -ENXIO;
> > +	for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
> > +		ret = ov10635_read16(dev, OV10635_PID);
> > +		if (ret == OV10635_VERSION)
> > +			break;
> > +		usleep_range(1000, 2000);
> >  	}
> > -
> > -	if (ret != OV10635_VERSION) {
> > -		if (retry--)
> > -			goto again;
> > -
> > -		dev_err(dev->dev, "OV10635 ID mismatch (0x%04x)\n",
> > -			ret);
> > +	if (i == OV10635_PID_TIMEOUT) {
> > +		dev_err(dev->dev, "OV10635 ID read failed (%d)\n", ret);
> >  		return -ENXIO;
> >  	}
> >  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 03/16] media: i2c: rdacm20: Replace goto with a loop

[Jacopo Mondi]

Hi,

On Mon, Feb 22, 2021 at 03:05:03AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> On Wed, Feb 17, 2021 at 01:01:26PM +0000, Kieran Bingham wrote:
> > On 16/02/2021 17:41, Jacopo Mondi wrote:
> > > During the camera module initialization the image sensor PID is read to
> > > verify it can correctly be identified. The current implementation is
> > > rather confused and uses a loop implemented with a label and a goto.
> > >
> > > Replace it with a more compact for() loop.
> > >
> > > No functional changes intended.
> >
> > I think there is a functional change in here, but I almost like it.
> >
> > Before, if the read was successful, it would check to see if the
> > OV10635_PID == OV10635_VERSION, and if not it would print that the read
> > was successful but a mismatch.
> >
> > Now - it will retry again instead, and if at the end of the retries it
> > still fails then it's a failure.
> >
> > This means we perhaps don't get told if the device id is not correct in
> > the same way, but it also means that if the VERSION was not correct
> > because of a read error (which I believe i've seen occur), it will retry.
>
> I was going to ask about that, whether we can have a successful I2C read
> operation that would return incorrect data. If we do, aren't we screwed
> ? If there's a non-negligible probability that reads will return
> incorrect data without any way to know about it (for other registers
> than the version register of course), then I would consider that writes
> could fail the same way, and that would mean an unusable device,
> wouldn't it ?
>
> If, on the other hand, read failures can always (or nearly always,
> ignoring space neutrinos and similar niceties) be detected, then I think
> we should avoid the functional change.
>
> > Because there is a functional change you might want to update the
> > commit, but I still think this is a good change overall.
> >

I'm not sure I got your concerns to be honest :/
yes before the code flow was like

        ret = ov10635_read();
        if (ret < 0) {

        }

        if (ret != PID) {

        }

But the condition ret != PID implied ret < 0 so I don't really get
what changes, apart from the fact that in the previous version we
could have had two different error messages for the same issue, and yes,
I saw ID mistmatch happening but the value of knowing the i2c read
didn't fail but the read data was garbage (usually it's 0x01 when it
fails iirc) is, well, questionable.

I'm sorry I didn't fully get this comment.


> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  drivers/media/i2c/rdacm20.c | 27 ++++++++++-----------------
> > >  1 file changed, 10 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> > > index 4d9bac87cba8..6504ed0bd3bc 100644
> > > --- a/drivers/media/i2c/rdacm20.c
> > > +++ b/drivers/media/i2c/rdacm20.c
> > > @@ -59,6 +59,8 @@
> > >   */
> > >  #define OV10635_PIXEL_RATE		(44000000)
> > >
> > > +#define OV10635_PID_TIMEOUT		3
> > > +
> > >  static const struct ov10635_reg {
> > >  	u16	reg;
> > >  	u8	val;
> > > @@ -452,7 +454,7 @@ static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
> > >
> > >  static int rdacm20_initialize(struct rdacm20_device *dev)
> > >  {
> > > -	unsigned int retry = 3;
> > > +	unsigned int i;
> > >  	int ret;
> > >
> > >  	/* Verify communication with the MAX9271: ping to wakeup. */
> > > @@ -501,23 +503,14 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
> > >  		return ret;
> > >  	usleep_range(10000, 15000);
> > >
> > > -again:
> > > -	ret = ov10635_read16(dev, OV10635_PID);
> > > -	if (ret < 0) {
> > > -		if (retry--)
> > > -			goto again;
> > > -
> > > -		dev_err(dev->dev, "OV10635 ID read failed (%d)\n",
> > > -			ret);
> > > -		return -ENXIO;
> > > +	for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
> > > +		ret = ov10635_read16(dev, OV10635_PID);
> > > +		if (ret == OV10635_VERSION)
> > > +			break;
> > > +		usleep_range(1000, 2000);
> > >  	}
> > > -
> > > -	if (ret != OV10635_VERSION) {
> > > -		if (retry--)
> > > -			goto again;
> > > -
> > > -		dev_err(dev->dev, "OV10635 ID mismatch (0x%04x)\n",
> > > -			ret);
> > > +	if (i == OV10635_PID_TIMEOUT) {
> > > +		dev_err(dev->dev, "OV10635 ID read failed (%d)\n", ret);
> > >  		return -ENXIO;
> > >  	}
> > >
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 03/16] media: i2c: rdacm20: Replace goto with a loop

[Laurent Pinchart]

Hi Jacopo,

On Mon, Feb 22, 2021 at 04:06:43PM +0100, Jacopo Mondi wrote:
> On Mon, Feb 22, 2021 at 03:05:03AM +0200, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > On Wed, Feb 17, 2021 at 01:01:26PM +0000, Kieran Bingham wrote:
> > > On 16/02/2021 17:41, Jacopo Mondi wrote:
> > > > During the camera module initialization the image sensor PID is read to
> > > > verify it can correctly be identified. The current implementation is
> > > > rather confused and uses a loop implemented with a label and a goto.
> > > >
> > > > Replace it with a more compact for() loop.
> > > >
> > > > No functional changes intended.
> > >
> > > I think there is a functional change in here, but I almost like it.
> > >
> > > Before, if the read was successful, it would check to see if the
> > > OV10635_PID == OV10635_VERSION, and if not it would print that the read
> > > was successful but a mismatch.
> > >
> > > Now - it will retry again instead, and if at the end of the retries it
> > > still fails then it's a failure.
> > >
> > > This means we perhaps don't get told if the device id is not correct in
> > > the same way, but it also means that if the VERSION was not correct
> > > because of a read error (which I believe i've seen occur), it will retry.
> >
> > I was going to ask about that, whether we can have a successful I2C read
> > operation that would return incorrect data. If we do, aren't we screwed
> > ? If there's a non-negligible probability that reads will return
> > incorrect data without any way to know about it (for other registers
> > than the version register of course), then I would consider that writes
> > could fail the same way, and that would mean an unusable device,
> > wouldn't it ?
> >
> > If, on the other hand, read failures can always (or nearly always,
> > ignoring space neutrinos and similar niceties) be detected, then I think
> > we should avoid the functional change.
> >
> > > Because there is a functional change you might want to update the
> > > commit, but I still think this is a good change overall.
> 
> I'm not sure I got your concerns to be honest :/
> yes before the code flow was like
> 
>         ret = ov10635_read();
>         if (ret < 0) {
> 
>         }
> 
>         if (ret != PID) {
> 
>         }
> 
> But the condition ret != PID implied ret < 0 so I don't really get
> what changes, apart from the fact that in the previous version we
> could have had two different error messages for the same issue, and yes,
> I saw ID mistmatch happening but the value of knowing the i2c read
> didn't fail but the read data was garbage (usually it's 0x01 when it
> fails iirc) is, well, questionable.

That's worrying :-S May we should add a warning message when the read
succeeds but the ID doesn't match, to at least have a way to track the
issue, and see if other changes get rid of this problem ?

> I'm sorry I didn't fully get this comment.

You're right, I had missed that the current code retried in case of a
version number mismatch. There's no functional change.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > >
> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > ---
> > > >  drivers/media/i2c/rdacm20.c | 27 ++++++++++-----------------
> > > >  1 file changed, 10 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> > > > index 4d9bac87cba8..6504ed0bd3bc 100644
> > > > --- a/drivers/media/i2c/rdacm20.c
> > > > +++ b/drivers/media/i2c/rdacm20.c
> > > > @@ -59,6 +59,8 @@
> > > >   */
> > > >  #define OV10635_PIXEL_RATE		(44000000)
> > > >
> > > > +#define OV10635_PID_TIMEOUT		3
> > > > +
> > > >  static const struct ov10635_reg {
> > > >  	u16	reg;
> > > >  	u8	val;
> > > > @@ -452,7 +454,7 @@ static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
> > > >
> > > >  static int rdacm20_initialize(struct rdacm20_device *dev)
> > > >  {
> > > > -	unsigned int retry = 3;
> > > > +	unsigned int i;
> > > >  	int ret;
> > > >
> > > >  	/* Verify communication with the MAX9271: ping to wakeup. */
> > > > @@ -501,23 +503,14 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
> > > >  		return ret;
> > > >  	usleep_range(10000, 15000);
> > > >
> > > > -again:
> > > > -	ret = ov10635_read16(dev, OV10635_PID);
> > > > -	if (ret < 0) {
> > > > -		if (retry--)
> > > > -			goto again;
> > > > -
> > > > -		dev_err(dev->dev, "OV10635 ID read failed (%d)\n",
> > > > -			ret);
> > > > -		return -ENXIO;
> > > > +	for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
> > > > +		ret = ov10635_read16(dev, OV10635_PID);
> > > > +		if (ret == OV10635_VERSION)
> > > > +			break;
> > > > +		usleep_range(1000, 2000);
> > > >  	}
> > > > -
> > > > -	if (ret != OV10635_VERSION) {
> > > > -		if (retry--)
> > > > -			goto again;
> > > > -
> > > > -		dev_err(dev->dev, "OV10635 ID mismatch (0x%04x)\n",
> > > > -			ret);
> > > > +	if (i == OV10635_PID_TIMEOUT) {
> > > > +		dev_err(dev->dev, "OV10635 ID read failed (%d)\n", ret);
> > > >  		return -ENXIO;
> > > >  	}
> > > >

-- 
Regards,

Laurent Pinchart

Re: [PATCH 03/16] media: i2c: rdacm20: Replace goto with a loop

[Kieran Bingham]

On 24/02/2021 20:27, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> On Mon, Feb 22, 2021 at 04:06:43PM +0100, Jacopo Mondi wrote:
>> On Mon, Feb 22, 2021 at 03:05:03AM +0200, Laurent Pinchart wrote:
>>> Hi Jacopo,
>>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>
>>> On Wed, Feb 17, 2021 at 01:01:26PM +0000, Kieran Bingham wrote:
>>>> On 16/02/2021 17:41, Jacopo Mondi wrote:
>>>>> During the camera module initialization the image sensor PID is read to
>>>>> verify it can correctly be identified. The current implementation is
>>>>> rather confused and uses a loop implemented with a label and a goto.
>>>>>
>>>>> Replace it with a more compact for() loop.
>>>>>
>>>>> No functional changes intended.
>>>>
>>>> I think there is a functional change in here, but I almost like it.
>>>>
>>>> Before, if the read was successful, it would check to see if the
>>>> OV10635_PID == OV10635_VERSION, and if not it would print that the read
>>>> was successful but a mismatch.
>>>>
>>>> Now - it will retry again instead, and if at the end of the retries it
>>>> still fails then it's a failure.
>>>>
>>>> This means we perhaps don't get told if the device id is not correct in
>>>> the same way, but it also means that if the VERSION was not correct
>>>> because of a read error (which I believe i've seen occur), it will retry.

So - to be clear here, I meant a 'read error', as in perhaps a
one-bit-flip or something else, not an error detected and propogated by
the I2C controllers.

I.e. ... something happening on the bus that gives a different result
but the 'read' was successful.... it's just that it returns a different
value than expected.

Given our noisy bus, not certain bus speeds, etc etc, I believe this can
happen.


>>>
>>> I was going to ask about that, whether we can have a successful I2C read
>>> operation that would return incorrect data. If we do, aren't we screwed
>>> ? If there's a non-negligible probability that reads will return
>>> incorrect data without any way to know about it (for other registers
>>> than the version register of course), then I would consider that writes
>>> could fail the same way, and that would mean an unusable device,
>>> wouldn't it ?
>>>
>>> If, on the other hand, read failures can always (or nearly always,
>>> ignoring space neutrinos and similar niceties) be detected, then I think
>>> we should avoid the functional change.
>>>
>>>> Because there is a functional change you might want to update the
>>>> commit, but I still think this is a good change overall.
>>
>> I'm not sure I got your concerns to be honest :/
>> yes before the code flow was like
>>
>>         ret = ov10635_read();
>>         if (ret < 0) {
>>
>>         }
>>
>>         if (ret != PID) {
>>

And so here you might have had a 'successful' read of the wrong value,
which means that ret > 0 but != PID.

>>         }
>>
>> But the condition ret != PID implied ret < 0 so I don't really get
>> what changes, apart from the fact that in the previous version we
>> could have had two different error messages for the same issue, and yes,
>> I saw ID mistmatch happening but the value of knowing the i2c read
>> didn't fail but the read data was garbage (usually it's 0x01 when it
>> fails iirc) is, well, questionable.
> 
> That's worrying :-S May we should add a warning message when the read
> succeeds but the ID doesn't match, to at least have a way to track the
> issue, and see if other changes get rid of this problem ?
> 

Ok, now I'm confused, that's what I was talking about!

Before we did do this, and now we don't. Ergo - functional change.


>> I'm sorry I didn't fully get this comment.
> 
> You're right, I had missed that the current code retried in case of a
> version number mismatch. There's no functional change.

I still think there's a functional change, but I'm not all too worried
about it.

As I said before, I think it's worth the retry in that event, which
didn't happen before, so my tag still holds.


> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>>>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>>>> ---
>>>>>  drivers/media/i2c/rdacm20.c | 27 ++++++++++-----------------
>>>>>  1 file changed, 10 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
>>>>> index 4d9bac87cba8..6504ed0bd3bc 100644
>>>>> --- a/drivers/media/i2c/rdacm20.c
>>>>> +++ b/drivers/media/i2c/rdacm20.c
>>>>> @@ -59,6 +59,8 @@
>>>>>   */
>>>>>  #define OV10635_PIXEL_RATE		(44000000)
>>>>>
>>>>> +#define OV10635_PID_TIMEOUT		3
>>>>> +
>>>>>  static const struct ov10635_reg {
>>>>>  	u16	reg;
>>>>>  	u8	val;
>>>>> @@ -452,7 +454,7 @@ static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
>>>>>
>>>>>  static int rdacm20_initialize(struct rdacm20_device *dev)
>>>>>  {
>>>>> -	unsigned int retry = 3;
>>>>> +	unsigned int i;
>>>>>  	int ret;
>>>>>
>>>>>  	/* Verify communication with the MAX9271: ping to wakeup. */
>>>>> @@ -501,23 +503,14 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>>>>>  		return ret;
>>>>>  	usleep_range(10000, 15000);
>>>>>
>>>>> -again:
>>>>> -	ret = ov10635_read16(dev, OV10635_PID);
>>>>> -	if (ret < 0) {
>>>>> -		if (retry--)
>>>>> -			goto again;
>>>>> -
>>>>> -		dev_err(dev->dev, "OV10635 ID read failed (%d)\n",
>>>>> -			ret);
>>>>> -		return -ENXIO;
>>>>> +	for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
>>>>> +		ret = ov10635_read16(dev, OV10635_PID);
>>>>> +		if (ret == OV10635_VERSION)
>>>>> +			break;
>>>>> +		usleep_range(1000, 2000);
>>>>>  	}
>>>>> -
>>>>> -	if (ret != OV10635_VERSION) {
>>>>> -		if (retry--)
>>>>> -			goto again;
>>>>> -
>>>>> -		dev_err(dev->dev, "OV10635 ID mismatch (0x%04x)\n",
>>>>> -			ret);
>>>>> +	if (i == OV10635_PID_TIMEOUT) {
>>>>> +		dev_err(dev->dev, "OV10635 ID read failed (%d)\n", ret);
>>>>>  		return -ENXIO;
>>>>>  	}
>>>>>
> 

[PATCH 04/16] media: i2c: rdacm20: Report camera module name

[Jacopo Mondi]

When the device is identified the driver currently reports the
names of the chips embedded in the camera module.

Report the name of the camera module itself instead.
Cosmetic change only.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm20.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 6504ed0bd3bc..56406d82b5ac 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -532,7 +532,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
 	if (ret)
 		return ret;
 
-	dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n");
+	dev_info(dev->dev, "Identified RDACM20 camera module\n");
 
 	/*
 	 * Set reverse channel high threshold to increase noise immunity.
-- 
2.30.0

Re: [PATCH 04/16] media: i2c: rdacm20: Report camera module name

[Kieran Bingham]

Hi Jacopo,

On 16/02/2021 17:41, Jacopo Mondi wrote:
> When the device is identified the driver currently reports the
> names of the chips embedded in the camera module.
> 
> Report the name of the camera module itself instead.
> Cosmetic change only.

Aha, there go all my scripts that rely on this string to know if the
camera was found ... but I can fix that ;-)

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/rdacm20.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 6504ed0bd3bc..56406d82b5ac 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -532,7 +532,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  	if (ret)
>  		return ret;
>  
> -	dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n");
> +	dev_info(dev->dev, "Identified RDACM20 camera module\n");
>  
>  	/*
>  	 * Set reverse channel high threshold to increase noise immunity.
> 

Re: [PATCH 04/16] media: i2c: rdacm20: Report camera module name

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Feb 16, 2021 at 06:41:34PM +0100, Jacopo Mondi wrote:
> When the device is identified the driver currently reports the
> names of the chips embedded in the camera module.
> 
> Report the name of the camera module itself instead.
> Cosmetic change only.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/rdacm20.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 6504ed0bd3bc..56406d82b5ac 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -532,7 +532,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  	if (ret)
>  		return ret;
>  
> -	dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n");
> +	dev_info(dev->dev, "Identified RDACM20 camera module\n");
>  
>  	/*
>  	 * Set reverse channel high threshold to increase noise immunity.

-- 
Regards,

Laurent Pinchart

[PATCH 05/16] media: i2c: rdacm20: Check return values

[Jacopo Mondi]

The camera module initialization routine does not check the return
value of a few functions. Fix that.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm20.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 56406d82b5ac..e982373908f2 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -470,11 +470,16 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
 	 *  Ensure that we have a good link configuration before attempting to
 	 *  identify the device.
 	 */
-	max9271_configure_i2c(&dev->serializer, MAX9271_I2CSLVSH_469NS_234NS |
-						MAX9271_I2CSLVTO_1024US |
-						MAX9271_I2CMSTBT_105KBPS);
+	ret = max9271_configure_i2c(&dev->serializer,
+				    MAX9271_I2CSLVSH_469NS_234NS |
+				    MAX9271_I2CSLVTO_1024US |
+				    MAX9271_I2CMSTBT_105KBPS);
+	if (ret)
+		return ret;
 
-	max9271_configure_gmsl_link(&dev->serializer);
+	ret = max9271_configure_gmsl_link(&dev->serializer);
+	if (ret)
+		return ret;
 
 	ret = max9271_verify_id(&dev->serializer);
 	if (ret < 0)
-- 
2.30.0

Re: [PATCH 05/16] media: i2c: rdacm20: Check return values

[Kieran Bingham]

Hi Jacopo,

On 16/02/2021 17:41, Jacopo Mondi wrote:
> The camera module initialization routine does not check the return
> value of a few functions. Fix that.
> 

Sounds quite valid to me.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/rdacm20.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 56406d82b5ac..e982373908f2 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -470,11 +470,16 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  	 *  Ensure that we have a good link configuration before attempting to
>  	 *  identify the device.
>  	 */
> -	max9271_configure_i2c(&dev->serializer, MAX9271_I2CSLVSH_469NS_234NS |
> -						MAX9271_I2CSLVTO_1024US |
> -						MAX9271_I2CMSTBT_105KBPS);
> +	ret = max9271_configure_i2c(&dev->serializer,
> +				    MAX9271_I2CSLVSH_469NS_234NS |
> +				    MAX9271_I2CSLVTO_1024US |
> +				    MAX9271_I2CMSTBT_105KBPS);
> +	if (ret)
> +		return ret;
>  
> -	max9271_configure_gmsl_link(&dev->serializer);
> +	ret = max9271_configure_gmsl_link(&dev->serializer);
> +	if (ret)
> +		return ret;
>  
>  	ret = max9271_verify_id(&dev->serializer);
>  	if (ret < 0)
> 

Re: [PATCH 05/16] media: i2c: rdacm20: Check return values

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Feb 16, 2021 at 06:41:35PM +0100, Jacopo Mondi wrote:
> The camera module initialization routine does not check the return
> value of a few functions. Fix that.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/rdacm20.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 56406d82b5ac..e982373908f2 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -470,11 +470,16 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  	 *  Ensure that we have a good link configuration before attempting to
>  	 *  identify the device.
>  	 */
> -	max9271_configure_i2c(&dev->serializer, MAX9271_I2CSLVSH_469NS_234NS |
> -						MAX9271_I2CSLVTO_1024US |
> -						MAX9271_I2CMSTBT_105KBPS);
> +	ret = max9271_configure_i2c(&dev->serializer,
> +				    MAX9271_I2CSLVSH_469NS_234NS |
> +				    MAX9271_I2CSLVTO_1024US |
> +				    MAX9271_I2CMSTBT_105KBPS);
> +	if (ret)
> +		return ret;
>  
> -	max9271_configure_gmsl_link(&dev->serializer);
> +	ret = max9271_configure_gmsl_link(&dev->serializer);
> +	if (ret)
> +		return ret;

This looks good, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

But it would be more useful if max9271_configure_gmsl_link() returned
errors when I2C writes fail :-)

>  
>  	ret = max9271_verify_id(&dev->serializer);
>  	if (ret < 0)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 05/16] media: i2c: rdacm20: Check return values

[Jacopo Mondi]

Hi

On Mon, Feb 22, 2021 at 03:09:26AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Feb 16, 2021 at 06:41:35PM +0100, Jacopo Mondi wrote:
> > The camera module initialization routine does not check the return
> > value of a few functions. Fix that.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/rdacm20.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> > index 56406d82b5ac..e982373908f2 100644
> > --- a/drivers/media/i2c/rdacm20.c
> > +++ b/drivers/media/i2c/rdacm20.c
> > @@ -470,11 +470,16 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
> >  	 *  Ensure that we have a good link configuration before attempting to
> >  	 *  identify the device.
> >  	 */
> > -	max9271_configure_i2c(&dev->serializer, MAX9271_I2CSLVSH_469NS_234NS |
> > -						MAX9271_I2CSLVTO_1024US |
> > -						MAX9271_I2CMSTBT_105KBPS);
> > +	ret = max9271_configure_i2c(&dev->serializer,
> > +				    MAX9271_I2CSLVSH_469NS_234NS |
> > +				    MAX9271_I2CSLVTO_1024US |
> > +				    MAX9271_I2CMSTBT_105KBPS);
> > +	if (ret)
> > +		return ret;
> >
> > -	max9271_configure_gmsl_link(&dev->serializer);
> > +	ret = max9271_configure_gmsl_link(&dev->serializer);
> > +	if (ret)
> > +		return ret;
>
> This looks good, so
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> But it would be more useful if max9271_configure_gmsl_link() returned
> errors when I2C writes fail :-)
>

Indeed, I'll add a patch to report back errors on failed i2c writes.

Thanks
  j

> >
> >  	ret = max9271_verify_id(&dev->serializer);
> >  	if (ret < 0)
>
> --
> Regards,
>
> Laurent Pinchart

[PATCH 06/16] media: i2c: rdacm20: Re-work ov10635 reset

[Jacopo Mondi]

The OV10635 image sensor embedded in the camera module is currently
reset after the MAX9271 initialization with two long delays that were
most probably not correctly characterized.

Re-work the image sensor reset procedure by holding the chip in reset
during the MAX9271 configuration, removing the long sleep delays and
only wait after the chip exits from reset for 350-500 microseconds
interval, which is larger than the minimum (2048 * (1 / XVCLK)) timeout
characterized in the chip manual.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm20.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index e982373908f2..ea30cc936531 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -477,6 +477,15 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
 	if (ret)
 		return ret;
 
+	/* Hold OV10635 in reset during max9271 configuration. */
+	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
+	if (ret)
+		return ret;
+
+	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
+	if (ret)
+		return ret;
+
 	ret = max9271_configure_gmsl_link(&dev->serializer);
 	if (ret)
 		return ret;
@@ -490,23 +499,11 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
 		return ret;
 	dev->serializer.client->addr = dev->addrs[0];
 
-	/*
-	 * Reset the sensor by cycling the OV10635 reset signal connected to the
-	 * MAX9271 GPIO1 and verify communication with the OV10635.
-	 */
-	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
-	if (ret)
-		return ret;
-
-	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
-	if (ret)
-		return ret;
-	usleep_range(10000, 15000);
-
+	/* Release ov10635 from reset and initialize it. */
 	ret = max9271_set_gpios(&dev->serializer, MAX9271_GPIO1OUT);
 	if (ret)
 		return ret;
-	usleep_range(10000, 15000);
+	usleep_range(350, 500);
 
 	for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
 		ret = ov10635_read16(dev, OV10635_PID);
-- 
2.30.0

Re: [PATCH 06/16] media: i2c: rdacm20: Re-work ov10635 reset

[Kieran Bingham]

On 16/02/2021 17:41, Jacopo Mondi wrote:
> The OV10635 image sensor embedded in the camera module is currently
> reset after the MAX9271 initialization with two long delays that were
> most probably not correctly characterized.
> 
> Re-work the image sensor reset procedure by holding the chip in reset
> during the MAX9271 configuration, removing the long sleep delays and
> only wait after the chip exits from reset for 350-500 microseconds
> interval, which is larger than the minimum (2048 * (1 / XVCLK)) timeout
> characterized in the chip manual.

Holding the OV10635 in reset earlier sounds good to me, but I don't know
beyond that what implications there would be. If it's working better
that's good though.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/rdacm20.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index e982373908f2..ea30cc936531 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -477,6 +477,15 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  	if (ret)
>  		return ret;
>  
> +	/* Hold OV10635 in reset during max9271 configuration. */
> +	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> +	if (ret)
> +		return ret;
> +
> +	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> +	if (ret)
> +		return ret;
> +
>  	ret = max9271_configure_gmsl_link(&dev->serializer);
>  	if (ret)
>  		return ret;
> @@ -490,23 +499,11 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  		return ret;
>  	dev->serializer.client->addr = dev->addrs[0];
>  
> -	/*
> -	 * Reset the sensor by cycling the OV10635 reset signal connected to the
> -	 * MAX9271 GPIO1 and verify communication with the OV10635.
> -	 */
> -	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> -	if (ret)
> -		return ret;
> -
> -	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> -	if (ret)
> -		return ret;
> -	usleep_range(10000, 15000);
> -
> +	/* Release ov10635 from reset and initialize it. */
>  	ret = max9271_set_gpios(&dev->serializer, MAX9271_GPIO1OUT);
>  	if (ret)
>  		return ret;
> -	usleep_range(10000, 15000);
> +	usleep_range(350, 500);
>  
>  	for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
>  		ret = ov10635_read16(dev, OV10635_PID);
> 

Re: [PATCH 06/16] media: i2c: rdacm20: Re-work ov10635 reset

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Feb 16, 2021 at 06:41:36PM +0100, Jacopo Mondi wrote:
> The OV10635 image sensor embedded in the camera module is currently
> reset after the MAX9271 initialization with two long delays that were
> most probably not correctly characterized.
> 
> Re-work the image sensor reset procedure by holding the chip in reset
> during the MAX9271 configuration, removing the long sleep delays and
> only wait after the chip exits from reset for 350-500 microseconds
> interval, which is larger than the minimum (2048 * (1 / XVCLK)) timeout
> characterized in the chip manual.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/rdacm20.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index e982373908f2..ea30cc936531 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -477,6 +477,15 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  	if (ret)
>  		return ret;
>  
> +	/* Hold OV10635 in reset during max9271 configuration. */
> +	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> +	if (ret)
> +		return ret;
> +
> +	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> +	if (ret)
> +		return ret;
> +

Unrelated to this patch, it could be nice to rename the GPIO-related
functions to use a similar naming scheme as the gpiod API.

>  	ret = max9271_configure_gmsl_link(&dev->serializer);
>  	if (ret)
>  		return ret;
> @@ -490,23 +499,11 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  		return ret;
>  	dev->serializer.client->addr = dev->addrs[0];
>  
> -	/*
> -	 * Reset the sensor by cycling the OV10635 reset signal connected to the
> -	 * MAX9271 GPIO1 and verify communication with the OV10635.
> -	 */
> -	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> -	if (ret)
> -		return ret;
> -
> -	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> -	if (ret)
> -		return ret;
> -	usleep_range(10000, 15000);

The OV10635 requires the reset signal to be asserted for at least 200µs.
Is this guaranteed by the different calls we have here after asserting
reset ? Maybe a comment to explain this could be useful ?

> -
> +	/* Release ov10635 from reset and initialize it. */
>  	ret = max9271_set_gpios(&dev->serializer, MAX9271_GPIO1OUT);
>  	if (ret)
>  		return ret;
> -	usleep_range(10000, 15000);

Maybe a comment here to state that the delay has to be at least 2048
XVCLK cycles would be useful ?

With these taken into account,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	usleep_range(350, 500);
>  
>  	for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
>  		ret = ov10635_read16(dev, OV10635_PID);

-- 
Regards,

Laurent Pinchart

[PATCH 07/16] media: i2c: rdacm2x: Fix wake up delay

[Jacopo Mondi]

The MAX9271 chip manual prescribes a delay of 5 milliseconds
after the chip exists from low power state.

Adjust the required delay in the rdacm21 camera module and add it
to the rdacm20 that currently doesn't implement one.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm20.c | 1 +
 drivers/media/i2c/rdacm21.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index ea30cc936531..39e4b4241870 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -460,6 +460,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
 	/* Verify communication with the MAX9271: ping to wakeup. */
 	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
 	i2c_smbus_read_byte(dev->serializer.client);
+	usleep_range(5000, 8000);
 
 	/* Serial link disabled during config as it needs a valid pixel clock. */
 	ret = max9271_set_serial_link(&dev->serializer, false);
diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 179d107f494c..b22a2ca5340b 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -453,7 +453,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
 	/* Verify communication with the MAX9271: ping to wakeup. */
 	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
 	i2c_smbus_read_byte(dev->serializer.client);
-	usleep_range(3000, 5000);
+	usleep_range(5000, 8000);
 
 	/* Enable reverse channel and disable the serial link. */
 	ret = max9271_set_serial_link(&dev->serializer, false);
-- 
2.30.0

Re: [PATCH 07/16] media: i2c: rdacm2x: Fix wake up delay

[Geert Uytterhoeven]

On Tue, Feb 16, 2021 at 6:41 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> The MAX9271 chip manual prescribes a delay of 5 milliseconds
> after the chip exists from low power state.

exits

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 07/16] media: i2c: rdacm2x: Fix wake up delay

[Kieran Bingham]

Hi Jacopo,


On 16/02/2021 17:41, Jacopo Mondi wrote:
> The MAX9271 chip manual prescribes a delay of 5 milliseconds
> after the chip exists from low power state.
> 
> Adjust the required delay in the rdacm21 camera module and add it
> to the rdacm20 that currently doesn't implement one.
> 

This sounds to me like it should be a common function in the max9271 module:

>         /* Verify communication with the MAX9271: ping to wakeup. */
>         dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
>         i2c_smbus_read_byte(dev->serializer.client);
>         usleep_range(5000, 8000);


Especially as that MAX9271_DEFAULT_ADDR should probably be handled
directly in the max9271.c file too, and the RDACM's shouldn't care about it.


If we end up moving the max9271 'library' into more of a module/device
then this would have to be done in it's 'probe' anyway, so it's likely
better handled down there...?

But ... it's not essential at this point in the series, so if you want
to keep this patch as is,

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
	> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/rdacm20.c | 1 +
>  drivers/media/i2c/rdacm21.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index ea30cc936531..39e4b4241870 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -460,6 +460,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  	/* Verify communication with the MAX9271: ping to wakeup. */
>  	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
>  	i2c_smbus_read_byte(dev->serializer.client);
> +	usleep_range(5000, 8000);
>  
>  	/* Serial link disabled during config as it needs a valid pixel clock. */
>  	ret = max9271_set_serial_link(&dev->serializer, false);
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index 179d107f494c..b22a2ca5340b 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -453,7 +453,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
>  	/* Verify communication with the MAX9271: ping to wakeup. */
>  	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
>  	i2c_smbus_read_byte(dev->serializer.client);
> -	usleep_range(3000, 5000);
> +	usleep_range(5000, 8000);
>  
>  	/* Enable reverse channel and disable the serial link. */
>  	ret = max9271_set_serial_link(&dev->serializer, false);
> 

Re: [PATCH 07/16] media: i2c: rdacm2x: Fix wake up delay

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Feb 17, 2021 at 01:33:01PM +0000, Kieran Bingham wrote:
> On 16/02/2021 17:41, Jacopo Mondi wrote:
> > The MAX9271 chip manual prescribes a delay of 5 milliseconds
> > after the chip exists from low power state.
> > 
> > Adjust the required delay in the rdacm21 camera module and add it
> > to the rdacm20 that currently doesn't implement one.
> 
> This sounds to me like it should be a common function in the max9271 module:
> 
> >         /* Verify communication with the MAX9271: ping to wakeup. */
> >         dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> >         i2c_smbus_read_byte(dev->serializer.client);
> >         usleep_range(5000, 8000);
> 
> Especially as that MAX9271_DEFAULT_ADDR should probably be handled
> directly in the max9271.c file too, and the RDACM's shouldn't care about it.

I think this is a good idea. With this addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> If we end up moving the max9271 'library' into more of a module/device
> then this would have to be done in it's 'probe' anyway, so it's likely
> better handled down there...?
> 
> But ... it's not essential at this point in the series, so if you want
> to keep this patch as is,
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 	> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/rdacm20.c | 1 +
> >  drivers/media/i2c/rdacm21.c | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> > index ea30cc936531..39e4b4241870 100644
> > --- a/drivers/media/i2c/rdacm20.c
> > +++ b/drivers/media/i2c/rdacm20.c
> > @@ -460,6 +460,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
> >  	/* Verify communication with the MAX9271: ping to wakeup. */
> >  	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> >  	i2c_smbus_read_byte(dev->serializer.client);
> > +	usleep_range(5000, 8000);
> >  
> >  	/* Serial link disabled during config as it needs a valid pixel clock. */
> >  	ret = max9271_set_serial_link(&dev->serializer, false);
> > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> > index 179d107f494c..b22a2ca5340b 100644
> > --- a/drivers/media/i2c/rdacm21.c
> > +++ b/drivers/media/i2c/rdacm21.c
> > @@ -453,7 +453,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
> >  	/* Verify communication with the MAX9271: ping to wakeup. */
> >  	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> >  	i2c_smbus_read_byte(dev->serializer.client);
> > -	usleep_range(3000, 5000);
> > +	usleep_range(5000, 8000);
> >  
> >  	/* Enable reverse channel and disable the serial link. */
> >  	ret = max9271_set_serial_link(&dev->serializer, false);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 07/16] media: i2c: rdacm2x: Fix wake up delay

[Jacopo Mondi]

Hi

On Mon, Feb 22, 2021 at 03:18:53AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Feb 17, 2021 at 01:33:01PM +0000, Kieran Bingham wrote:
> > On 16/02/2021 17:41, Jacopo Mondi wrote:
> > > The MAX9271 chip manual prescribes a delay of 5 milliseconds
> > > after the chip exists from low power state.
> > >
> > > Adjust the required delay in the rdacm21 camera module and add it
> > > to the rdacm20 that currently doesn't implement one.
> >
> > This sounds to me like it should be a common function in the max9271 module:
> >
> > >         /* Verify communication with the MAX9271: ping to wakeup. */
> > >         dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> > >         i2c_smbus_read_byte(dev->serializer.client);
> > >         usleep_range(5000, 8000);
> >
> > Especially as that MAX9271_DEFAULT_ADDR should probably be handled
> > directly in the max9271.c file too, and the RDACM's shouldn't care about it.
>
> I think this is a good idea. With this addressed,

The address reprogramming was exactly why I refrained from adding a
function to the max9271 library, as handling of the addresses there
would introduce a precendece order in the function calls, ie the newly
introduced function would require to be called first after a chip
reset and that's something I considered better handled by the camera
driver (even if it wouldn't be suprising a 'wake up' function to be
called first).

please also note that I will next try to make the max9271 a proper i2c
driver so this might be even less relevant that what it is right now

Thanks
   j
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > If we end up moving the max9271 'library' into more of a module/device
> > then this would have to be done in it's 'probe' anyway, so it's likely
> > better handled down there...?
> >
> > But ... it's not essential at this point in the series, so if you want
> > to keep this patch as is,
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > 	> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  drivers/media/i2c/rdacm20.c | 1 +
> > >  drivers/media/i2c/rdacm21.c | 2 +-
> > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> > > index ea30cc936531..39e4b4241870 100644
> > > --- a/drivers/media/i2c/rdacm20.c
> > > +++ b/drivers/media/i2c/rdacm20.c
> > > @@ -460,6 +460,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
> > >  	/* Verify communication with the MAX9271: ping to wakeup. */
> > >  	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> > >  	i2c_smbus_read_byte(dev->serializer.client);
> > > +	usleep_range(5000, 8000);
> > >
> > >  	/* Serial link disabled during config as it needs a valid pixel clock. */
> > >  	ret = max9271_set_serial_link(&dev->serializer, false);
> > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> > > index 179d107f494c..b22a2ca5340b 100644
> > > --- a/drivers/media/i2c/rdacm21.c
> > > +++ b/drivers/media/i2c/rdacm21.c
> > > @@ -453,7 +453,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
> > >  	/* Verify communication with the MAX9271: ping to wakeup. */
> > >  	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> > >  	i2c_smbus_read_byte(dev->serializer.client);
> > > -	usleep_range(3000, 5000);
> > > +	usleep_range(5000, 8000);
> > >
> > >  	/* Enable reverse channel and disable the serial link. */
> > >  	ret = max9271_set_serial_link(&dev->serializer, false);
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 07/16] media: i2c: rdacm2x: Fix wake up delay

[Laurent Pinchart]

Hi Jacopo,

On Mon, Feb 22, 2021 at 04:11:41PM +0100, Jacopo Mondi wrote:
> On Mon, Feb 22, 2021 at 03:18:53AM +0200, Laurent Pinchart wrote:
> > On Wed, Feb 17, 2021 at 01:33:01PM +0000, Kieran Bingham wrote:
> > > On 16/02/2021 17:41, Jacopo Mondi wrote:
> > > > The MAX9271 chip manual prescribes a delay of 5 milliseconds
> > > > after the chip exists from low power state.
> > > >
> > > > Adjust the required delay in the rdacm21 camera module and add it
> > > > to the rdacm20 that currently doesn't implement one.
> > >
> > > This sounds to me like it should be a common function in the max9271 module:
> > >
> > > >         /* Verify communication with the MAX9271: ping to wakeup. */
> > > >         dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> > > >         i2c_smbus_read_byte(dev->serializer.client);
> > > >         usleep_range(5000, 8000);
> > >
> > > Especially as that MAX9271_DEFAULT_ADDR should probably be handled
> > > directly in the max9271.c file too, and the RDACM's shouldn't care about it.
> >
> > I think this is a good idea. With this addressed,
> 
> The address reprogramming was exactly why I refrained from adding a
> function to the max9271 library, as handling of the addresses there
> would introduce a precendece order in the function calls, ie the newly
> introduced function would require to be called first after a chip
> reset and that's something I considered better handled by the camera
> driver (even if it wouldn't be suprising a 'wake up' function to be
> called first).

I'm not sure this would be an issue, it's fine for library code to set
requirements on how APIs have to be used, including which order
functions have to be called.

> please also note that I will next try to make the max9271 a proper i2c
> driver so this might be even less relevant that what it is right now

Let's see how that works out.

> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > > If we end up moving the max9271 'library' into more of a module/device
> > > then this would have to be done in it's 'probe' anyway, so it's likely
> > > better handled down there...?
> > >
> > > But ... it's not essential at this point in the series, so if you want
> > > to keep this patch as is,
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > 	> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > ---
> > > >  drivers/media/i2c/rdacm20.c | 1 +
> > > >  drivers/media/i2c/rdacm21.c | 2 +-
> > > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> > > > index ea30cc936531..39e4b4241870 100644
> > > > --- a/drivers/media/i2c/rdacm20.c
> > > > +++ b/drivers/media/i2c/rdacm20.c
> > > > @@ -460,6 +460,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
> > > >  	/* Verify communication with the MAX9271: ping to wakeup. */
> > > >  	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> > > >  	i2c_smbus_read_byte(dev->serializer.client);
> > > > +	usleep_range(5000, 8000);
> > > >
> > > >  	/* Serial link disabled during config as it needs a valid pixel clock. */
> > > >  	ret = max9271_set_serial_link(&dev->serializer, false);
> > > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> > > > index 179d107f494c..b22a2ca5340b 100644
> > > > --- a/drivers/media/i2c/rdacm21.c
> > > > +++ b/drivers/media/i2c/rdacm21.c
> > > > @@ -453,7 +453,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
> > > >  	/* Verify communication with the MAX9271: ping to wakeup. */
> > > >  	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> > > >  	i2c_smbus_read_byte(dev->serializer.client);
> > > > -	usleep_range(3000, 5000);
> > > > +	usleep_range(5000, 8000);
> > > >
> > > >  	/* Enable reverse channel and disable the serial link. */
> > > >  	ret = max9271_set_serial_link(&dev->serializer, false);

-- 
Regards,

Laurent Pinchart

[PATCH 08/16] media: i2c: max9286: Adjust parameters indent

[Jacopo Mondi]

The parameters to max9286_i2c_mux_configure() fits on the previous
line. Adjust it.

Cosmetic change only.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 6fd4d59fcc72..1d9951215868 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -287,9 +287,8 @@ static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan)
 
 	priv->mux_channel = chan;
 
-	max9286_i2c_mux_configure(priv,
-				  MAX9286_FWDCCEN(chan) |
-				  MAX9286_REVCCEN(chan));
+	max9286_i2c_mux_configure(priv, MAX9286_FWDCCEN(chan) |
+					MAX9286_REVCCEN(chan));
 
 	return 0;
 }
-- 
2.30.0

Re: [PATCH 08/16] media: i2c: max9286: Adjust parameters indent

[Kieran Bingham]

On 16/02/2021 17:41, Jacopo Mondi wrote:
> The parameters to max9286_i2c_mux_configure() fits on the previous
> line. Adjust it.
> 
> Cosmetic change only.

Cosmetic tag ;-)

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 6fd4d59fcc72..1d9951215868 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -287,9 +287,8 @@ static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan)
>  
>  	priv->mux_channel = chan;
>  
> -	max9286_i2c_mux_configure(priv,
> -				  MAX9286_FWDCCEN(chan) |
> -				  MAX9286_REVCCEN(chan));
> +	max9286_i2c_mux_configure(priv, MAX9286_FWDCCEN(chan) |
> +					MAX9286_REVCCEN(chan));
>  
>  	return 0;
>  }
> 

Re: [PATCH 08/16] media: i2c: max9286: Adjust parameters indent

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Feb 16, 2021 at 06:41:38PM +0100, Jacopo Mondi wrote:
> The parameters to max9286_i2c_mux_configure() fits on the previous
> line. Adjust it.
> 
> Cosmetic change only.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 6fd4d59fcc72..1d9951215868 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -287,9 +287,8 @@ static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan)
>  
>  	priv->mux_channel = chan;
>  
> -	max9286_i2c_mux_configure(priv,
> -				  MAX9286_FWDCCEN(chan) |
> -				  MAX9286_REVCCEN(chan));
> +	max9286_i2c_mux_configure(priv, MAX9286_FWDCCEN(chan) |
> +					MAX9286_REVCCEN(chan));

I feel obliged to say I would have written

	max9286_i2c_mux_configure(priv, MAX9286_FWDCCEN(chan) |
				  MAX9286_REVCCEN(chan));

:-)

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

[PATCH 09/16] media: i2c: rdacm21: Re-work OV10640 initialization

[Jacopo Mondi]

The OV10640 image sensor reset and powerdown on signals are controlled
by the embedded OV490 ISP. The current reset procedure does not respect
the 1 millisecond power-up delay and releases the reset signal before
the powerdown one.

Fix the OV10640 power up sequence by releasing the powerdown signal,
waiting the mandatory 1 millisecond power up delay and then releasing
the reset signal. The reset delay is not characterized in the chip
manual if not as "255 XVCLK + initialization". Wait for at least 3
milliseconds to guarantee the SCCB bus is available.

This commit fixes a sporadic start-up error triggered by a failure to
read the OV10640 chip ID:
rdacm21 8-0054: OV10640 ID mismatch: (0x01)

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm21.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index b22a2ca5340b..c420a6b96879 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -333,13 +333,15 @@ static int ov10640_initialize(struct rdacm21_device *dev)
 {
 	u8 val;
 
-	/* Power-up OV10640 by setting RESETB and PWDNB pins high. */
+	/* Power-up OV10640 by setting PWDNB and RESETB pins high. */
 	ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0);
 	ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
 	ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0);
 	ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0);
-	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
+
 	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0);
+	usleep_range(1500, 3000);
+	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
 	usleep_range(3000, 5000);
 
 	/* Read OV10640 ID to test communications. */
-- 
2.30.0

Re: [PATCH 09/16] media: i2c: rdacm21: Re-work OV10640 initialization

[Geert Uytterhoeven]

Hi Jacopo,

On Tue, Feb 16, 2021 at 6:41 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> The OV10640 image sensor reset and powerdown on signals are controlled

Drop the "on"?

> by the embedded OV490 ISP. The current reset procedure does not respect
> the 1 millisecond power-up delay and releases the reset signal before
> the powerdown one.
>
> Fix the OV10640 power up sequence by releasing the powerdown signal,
> waiting the mandatory 1 millisecond power up delay and then releasing

"powerdown" vs. "power-up" vs. "power up"?

> the reset signal. The reset delay is not characterized in the chip
> manual if not as "255 XVCLK + initialization". Wait for at least 3
> milliseconds to guarantee the SCCB bus is available.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 09/16] media: i2c: rdacm21: Re-work OV10640 initialization

[Kieran Bingham]

Hi Jacopo,

On 16/02/2021 17:41, Jacopo Mondi wrote:
> The OV10640 image sensor reset and powerdown on signals are controlled
> by the embedded OV490 ISP. The current reset procedure does not respect
> the 1 millisecond power-up delay and releases the reset signal before
> the powerdown one.
> 
> Fix the OV10640 power up sequence by releasing the powerdown signal,
> waiting the mandatory 1 millisecond power up delay and then releasing
> the reset signal. The reset delay is not characterized in the chip
> manual if not as "255 XVCLK + initialization". Wait for at least 3
> milliseconds to guarantee the SCCB bus is available.
> 
> This commit fixes a sporadic start-up error triggered by a failure to
> read the OV10640 chip ID:
> rdacm21 8-0054: OV10640 ID mismatch: (0x01)

Have you done a similar initialisation rework for the RDACM21 as you do
in [03/16] of this series for the RDACM20 (or was it already better
perhaps?)

Only interested because of noting that I think the 'mismatch' check is
now gone from the RDAMC20.


> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/rdacm21.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index b22a2ca5340b..c420a6b96879 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -333,13 +333,15 @@ static int ov10640_initialize(struct rdacm21_device *dev)
>  {
>  	u8 val;
>  
> -	/* Power-up OV10640 by setting RESETB and PWDNB pins high. */
> +	/* Power-up OV10640 by setting PWDNB and RESETB pins high. */
>  	ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0);
>  	ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
>  	ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0);
>  	ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0);
> -	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
> +
>  	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0);
> +	usleep_range(1500, 3000);
> +	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
>  	usleep_range(3000, 5000);
>  
>  	/* Read OV10640 ID to test communications. */
> 

Re: [PATCH 09/16] media: i2c: rdacm21: Re-work OV10640 initialization

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Feb 16, 2021 at 06:41:39PM +0100, Jacopo Mondi wrote:
> The OV10640 image sensor reset and powerdown on signals are controlled

s/ on//

> by the embedded OV490 ISP. The current reset procedure does not respect
> the 1 millisecond power-up delay and releases the reset signal before
> the powerdown one.
> 
> Fix the OV10640 power up sequence by releasing the powerdown signal,
> waiting the mandatory 1 millisecond power up delay and then releasing
> the reset signal. The reset delay is not characterized in the chip
> manual if not as "255 XVCLK + initialization". Wait for at least 3
> milliseconds to guarantee the SCCB bus is available.
> 
> This commit fixes a sporadic start-up error triggered by a failure to
> read the OV10640 chip ID:
> rdacm21 8-0054: OV10640 ID mismatch: (0x01)
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/rdacm21.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index b22a2ca5340b..c420a6b96879 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -333,13 +333,15 @@ static int ov10640_initialize(struct rdacm21_device *dev)
>  {
>  	u8 val;
>  
> -	/* Power-up OV10640 by setting RESETB and PWDNB pins high. */
> +	/* Power-up OV10640 by setting PWDNB and RESETB pins high. */
>  	ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0);
>  	ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
>  	ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0);
>  	ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0);
> -	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
> +
>  	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0);

Shouldn't this be OV490_GPIO_OUTPUT_VALUE1 ?

> +	usleep_range(1500, 3000);
> +	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);

I'm a bit puzzled by why this patch would improve the ID read issue,
given that it sets GPIO0 to 1, then sets GPIO0 to 1, compared to
previously setting GPIO0 to 1 following by setting GPIO0 to 1 :-) Maybe
it's the additional delay ? In any case, it would probably be a good
idea to perform additional tests after fixing this.

>  	usleep_range(3000, 5000);
>  
>  	/* Read OV10640 ID to test communications. */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 09/16] media: i2c: rdacm21: Re-work OV10640 initialization

[Jacopo Mondi]

Hi

On Mon, Feb 22, 2021 at 03:27:25AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Feb 16, 2021 at 06:41:39PM +0100, Jacopo Mondi wrote:
> > The OV10640 image sensor reset and powerdown on signals are controlled
>
> s/ on//
>
> > by the embedded OV490 ISP. The current reset procedure does not respect
> > the 1 millisecond power-up delay and releases the reset signal before
> > the powerdown one.
> >
> > Fix the OV10640 power up sequence by releasing the powerdown signal,
> > waiting the mandatory 1 millisecond power up delay and then releasing
> > the reset signal. The reset delay is not characterized in the chip
> > manual if not as "255 XVCLK + initialization". Wait for at least 3
> > milliseconds to guarantee the SCCB bus is available.
> >
> > This commit fixes a sporadic start-up error triggered by a failure to
> > read the OV10640 chip ID:
> > rdacm21 8-0054: OV10640 ID mismatch: (0x01)
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/rdacm21.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> > index b22a2ca5340b..c420a6b96879 100644
> > --- a/drivers/media/i2c/rdacm21.c
> > +++ b/drivers/media/i2c/rdacm21.c
> > @@ -333,13 +333,15 @@ static int ov10640_initialize(struct rdacm21_device *dev)
> >  {
> >  	u8 val;
> >
> > -	/* Power-up OV10640 by setting RESETB and PWDNB pins high. */
> > +	/* Power-up OV10640 by setting PWDNB and RESETB pins high. */
> >  	ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0);
> >  	ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
> >  	ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0);
> >  	ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0);
> > -	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
> > +
> >  	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0);
>
> Shouldn't this be OV490_GPIO_OUTPUT_VALUE1 ?
>

Ouch, yes it should

> > +	usleep_range(1500, 3000);
> > +	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
>
> I'm a bit puzzled by why this patch would improve the ID read issue,
> given that it sets GPIO0 to 1, then sets GPIO0 to 1, compared to
> previously setting GPIO0 to 1 following by setting GPIO0 to 1 :-) Maybe

:) it doesn't make things worse at least!

> it's the additional delay ? In any case, it would probably be a good
> idea to perform additional tests after fixing this.

I think the additional delay plays indeed a role, as it's a
requirement in the datasheet that was not respected, but now I'm dead
scared to fix this and find out I've opened another can of worms..

>
> >  	usleep_range(3000, 5000);
> >
> >  	/* Read OV10640 ID to test communications. */
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 09/16] media: i2c: rdacm21: Re-work OV10640 initialization

[Laurent Pinchart]

Hi Jacopo,

On Mon, Feb 22, 2021 at 04:19:13PM +0100, Jacopo Mondi wrote:
> On Mon, Feb 22, 2021 at 03:27:25AM +0200, Laurent Pinchart wrote:
> > On Tue, Feb 16, 2021 at 06:41:39PM +0100, Jacopo Mondi wrote:
> > > The OV10640 image sensor reset and powerdown on signals are controlled
> >
> > s/ on//
> >
> > > by the embedded OV490 ISP. The current reset procedure does not respect
> > > the 1 millisecond power-up delay and releases the reset signal before
> > > the powerdown one.
> > >
> > > Fix the OV10640 power up sequence by releasing the powerdown signal,
> > > waiting the mandatory 1 millisecond power up delay and then releasing
> > > the reset signal. The reset delay is not characterized in the chip
> > > manual if not as "255 XVCLK + initialization". Wait for at least 3
> > > milliseconds to guarantee the SCCB bus is available.
> > >
> > > This commit fixes a sporadic start-up error triggered by a failure to
> > > read the OV10640 chip ID:
> > > rdacm21 8-0054: OV10640 ID mismatch: (0x01)
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  drivers/media/i2c/rdacm21.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> > > index b22a2ca5340b..c420a6b96879 100644
> > > --- a/drivers/media/i2c/rdacm21.c
> > > +++ b/drivers/media/i2c/rdacm21.c
> > > @@ -333,13 +333,15 @@ static int ov10640_initialize(struct rdacm21_device *dev)
> > >  {
> > >  	u8 val;
> > >
> > > -	/* Power-up OV10640 by setting RESETB and PWDNB pins high. */
> > > +	/* Power-up OV10640 by setting PWDNB and RESETB pins high. */
> > >  	ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0);
> > >  	ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
> > >  	ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0);
> > >  	ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0);
> > > -	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
> > > +
> > >  	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0);
> >
> > Shouldn't this be OV490_GPIO_OUTPUT_VALUE1 ?
> 
> Ouch, yes it should
> 
> > > +	usleep_range(1500, 3000);
> > > +	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
> >
> > I'm a bit puzzled by why this patch would improve the ID read issue,
> > given that it sets GPIO0 to 1, then sets GPIO0 to 1, compared to
> > previously setting GPIO0 to 1 following by setting GPIO0 to 1 :-) Maybe
> 
> :) it doesn't make things worse at least!
> 
> > it's the additional delay ? In any case, it would probably be a good
> > idea to perform additional tests after fixing this.
> 
> I think the additional delay plays indeed a role, as it's a
> requirement in the datasheet that was not respected, but now I'm dead
> scared to fix this and find out I've opened another can of worms..

With some luck it will be the opposite, and fixing this will make the
startup sequence much more reliable. Of course luck has been quite
scarce so far for anything even remotely related to GMSL, so I won't
hold my breath.

> > >  	usleep_range(3000, 5000);
> > >
> > >  	/* Read OV10640 ID to test communications. */

-- 
Regards,

Laurent Pinchart

[PATCH 10/16] media: i2c: max9286: Rename reverse_channel_mv

[Jacopo Mondi]

Rename the reverse_channel_mv variable to init_rev_chan_mv as
the next patches will cache the reverse channel amplitude in
a new driver variable.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 1d9951215868..1f14cd817fbf 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -163,7 +163,7 @@ struct max9286_priv {
 	unsigned int mux_channel;
 	bool mux_open;
 
-	u32 reverse_channel_mv;
+	u32 init_rev_chan_mv;
 
 	struct v4l2_ctrl_handler ctrls;
 	struct v4l2_ctrl *pixelrate;
@@ -563,7 +563,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
 	 * - Disable auto-ack as communication on the control channel are now
 	 *   stable.
 	 */
-	if (priv->reverse_channel_mv < 170)
+	if (priv->init_rev_chan_mv < 170)
 		max9286_reverse_channel_setup(priv, 170);
 	max9286_check_config_link(priv, priv->source_mask);
 
@@ -971,7 +971,7 @@ static int max9286_setup(struct max9286_priv *priv)
 	 * only. This should be disabled after the mux is initialised.
 	 */
 	max9286_configure_i2c(priv, true);
-	max9286_reverse_channel_setup(priv, priv->reverse_channel_mv);
+	max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);
 
 	/*
 	 * Enable GMSL links, mask unused ones and autodetect link
@@ -1236,9 +1236,9 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 	if (of_property_read_u32(dev->of_node,
 				 "maxim,reverse-channel-microvolt",
 				 &reverse_channel_microvolt))
-		priv->reverse_channel_mv = 170;
+		priv->init_rev_chan_mv = 170;
 	else
-		priv->reverse_channel_mv = reverse_channel_microvolt / 1000U;
+		priv->init_rev_chan_mv = reverse_channel_microvolt / 1000U;
 
 	priv->route_mask = priv->source_mask;
 
-- 
2.30.0

Re: [PATCH 10/16] media: i2c: max9286: Rename reverse_channel_mv

[Geert Uytterhoeven]

On Tue, Feb 16, 2021 at 6:41 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> Rename the reverse_channel_mv variable to init_rev_chan_mv as
> the next patches will cache the reverse channel amplitude in

patch?

> a new driver variable.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 10/16] media: i2c: max9286: Rename reverse_channel_mv

[Kieran Bingham]

Hi Jacopo,

On 16/02/2021 17:41, Jacopo Mondi wrote:
> Rename the reverse_channel_mv variable to init_rev_chan_mv as
> the next patches will cache the reverse channel amplitude in
> a new driver variable.
> 

I've been trying to figure out if we really do need two variables to
store this now, but I can't see an easy way to factor out the
initialisation value, and I like the idea of caching the current stored
value.

So

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 1d9951215868..1f14cd817fbf 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -163,7 +163,7 @@ struct max9286_priv {
>  	unsigned int mux_channel;
>  	bool mux_open;
>  
> -	u32 reverse_channel_mv;
> +	u32 init_rev_chan_mv;
>  
>  	struct v4l2_ctrl_handler ctrls;
>  	struct v4l2_ctrl *pixelrate;
> @@ -563,7 +563,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
>  	 * - Disable auto-ack as communication on the control channel are now
>  	 *   stable.
>  	 */
> -	if (priv->reverse_channel_mv < 170)
> +	if (priv->init_rev_chan_mv < 170)
>  		max9286_reverse_channel_setup(priv, 170);
>  	max9286_check_config_link(priv, priv->source_mask);
>  
> @@ -971,7 +971,7 @@ static int max9286_setup(struct max9286_priv *priv)
>  	 * only. This should be disabled after the mux is initialised.
>  	 */
>  	max9286_configure_i2c(priv, true);
> -	max9286_reverse_channel_setup(priv, priv->reverse_channel_mv);
> +	max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);
>  
>  	/*
>  	 * Enable GMSL links, mask unused ones and autodetect link
> @@ -1236,9 +1236,9 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  	if (of_property_read_u32(dev->of_node,
>  				 "maxim,reverse-channel-microvolt",
>  				 &reverse_channel_microvolt))
> -		priv->reverse_channel_mv = 170;
> +		priv->init_rev_chan_mv = 170;
>  	else
> -		priv->reverse_channel_mv = reverse_channel_microvolt / 1000U;
> +		priv->init_rev_chan_mv = reverse_channel_microvolt / 1000U;
>  
>  	priv->route_mask = priv->source_mask;
>  
> 

Re: [PATCH 10/16] media: i2c: max9286: Rename reverse_channel_mv

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Feb 16, 2021 at 06:41:40PM +0100, Jacopo Mondi wrote:
> Rename the reverse_channel_mv variable to init_rev_chan_mv as
> the next patches will cache the reverse channel amplitude in
> a new driver variable.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 1d9951215868..1f14cd817fbf 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -163,7 +163,7 @@ struct max9286_priv {
>  	unsigned int mux_channel;
>  	bool mux_open;
>  
> -	u32 reverse_channel_mv;
> +	u32 init_rev_chan_mv;

Maybe it could be time to add some kerneldoc to this structure, or just
a comment for this field, to explain what it stores ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  	struct v4l2_ctrl_handler ctrls;
>  	struct v4l2_ctrl *pixelrate;
> @@ -563,7 +563,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
>  	 * - Disable auto-ack as communication on the control channel are now
>  	 *   stable.
>  	 */
> -	if (priv->reverse_channel_mv < 170)
> +	if (priv->init_rev_chan_mv < 170)
>  		max9286_reverse_channel_setup(priv, 170);
>  	max9286_check_config_link(priv, priv->source_mask);
>  
> @@ -971,7 +971,7 @@ static int max9286_setup(struct max9286_priv *priv)
>  	 * only. This should be disabled after the mux is initialised.
>  	 */
>  	max9286_configure_i2c(priv, true);
> -	max9286_reverse_channel_setup(priv, priv->reverse_channel_mv);
> +	max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);
>  
>  	/*
>  	 * Enable GMSL links, mask unused ones and autodetect link
> @@ -1236,9 +1236,9 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  	if (of_property_read_u32(dev->of_node,
>  				 "maxim,reverse-channel-microvolt",
>  				 &reverse_channel_microvolt))
> -		priv->reverse_channel_mv = 170;
> +		priv->init_rev_chan_mv = 170;
>  	else
> -		priv->reverse_channel_mv = reverse_channel_microvolt / 1000U;
> +		priv->init_rev_chan_mv = reverse_channel_microvolt / 1000U;
>  
>  	priv->route_mask = priv->source_mask;
>  

-- 
Regards,

Laurent Pinchart

[PATCH 11/16] media: i2c: max9286: Cache channel amplitude

[Jacopo Mondi]

Cache the current channel amplitude in a driver variable
to skip updating it if the new requested value is the same
as the currently configured one.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 1f14cd817fbf..4afb5ca06448 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -164,6 +164,7 @@ struct max9286_priv {
 	bool mux_open;
 
 	u32 init_rev_chan_mv;
+	u32 rev_chan_mv;
 
 	struct v4l2_ctrl_handler ctrls;
 	struct v4l2_ctrl *pixelrate;
@@ -340,8 +341,15 @@ static void max9286_configure_i2c(struct max9286_priv *priv, bool localack)
 static void max9286_reverse_channel_setup(struct max9286_priv *priv,
 					  unsigned int chan_amplitude)
 {
+	u8 chan_config;
+
+	if (priv->rev_chan_mv == chan_amplitude)
+		return;
+
+	priv->rev_chan_mv = chan_amplitude;
+
 	/* Reverse channel transmission time: default to 1. */
-	u8 chan_config = MAX9286_REV_TRF(1);
+	chan_config = MAX9286_REV_TRF(1);
 
 	/*
 	 * Reverse channel setup.
@@ -563,8 +571,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
 	 * - Disable auto-ack as communication on the control channel are now
 	 *   stable.
 	 */
-	if (priv->init_rev_chan_mv < 170)
-		max9286_reverse_channel_setup(priv, 170);
+	max9286_reverse_channel_setup(priv, 170);
 	max9286_check_config_link(priv, priv->source_mask);
 
 	/*
-- 
2.30.0

Re: [PATCH 11/16] media: i2c: max9286: Cache channel amplitude

[Geert Uytterhoeven]

On Tue, Feb 16, 2021 at 6:41 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> Cache the current channel amplitude in a driver variable
> to skip updating it if the new requested value is the same

newly

> as the currently configured one.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 11/16] media: i2c: max9286: Cache channel amplitude

[Kieran Bingham]

On 16/02/2021 17:41, Jacopo Mondi wrote:
> Cache the current channel amplitude in a driver variable
> to skip updating it if the new requested value is the same
> as the currently configured one.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

I like this model better than deciding outside of the call :-)

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


> ---
>  drivers/media/i2c/max9286.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 1f14cd817fbf..4afb5ca06448 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -164,6 +164,7 @@ struct max9286_priv {
>  	bool mux_open;
>  
>  	u32 init_rev_chan_mv;
> +	u32 rev_chan_mv;
>  
>  	struct v4l2_ctrl_handler ctrls;
>  	struct v4l2_ctrl *pixelrate;
> @@ -340,8 +341,15 @@ static void max9286_configure_i2c(struct max9286_priv *priv, bool localack)
>  static void max9286_reverse_channel_setup(struct max9286_priv *priv,
>  					  unsigned int chan_amplitude)
>  {
> +	u8 chan_config;
> +
> +	if (priv->rev_chan_mv == chan_amplitude)
> +		return;
> +
> +	priv->rev_chan_mv = chan_amplitude;
> +
>  	/* Reverse channel transmission time: default to 1. */
> -	u8 chan_config = MAX9286_REV_TRF(1);
> +	chan_config = MAX9286_REV_TRF(1);
>  
>  	/*
>  	 * Reverse channel setup.
> @@ -563,8 +571,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
>  	 * - Disable auto-ack as communication on the control channel are now
>  	 *   stable.
>  	 */
> -	if (priv->init_rev_chan_mv < 170)
> -		max9286_reverse_channel_setup(priv, 170);
> +	max9286_reverse_channel_setup(priv, 170);
>  	max9286_check_config_link(priv, priv->source_mask);
>  
>  	/*
> 

Re: [PATCH 11/16] media: i2c: max9286: Cache channel amplitude

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Feb 16, 2021 at 06:41:41PM +0100, Jacopo Mondi wrote:
> Cache the current channel amplitude in a driver variable
> to skip updating it if the new requested value is the same
> as the currently configured one.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/max9286.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 1f14cd817fbf..4afb5ca06448 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -164,6 +164,7 @@ struct max9286_priv {
>  	bool mux_open;
>  
>  	u32 init_rev_chan_mv;
> +	u32 rev_chan_mv;
>  
>  	struct v4l2_ctrl_handler ctrls;
>  	struct v4l2_ctrl *pixelrate;
> @@ -340,8 +341,15 @@ static void max9286_configure_i2c(struct max9286_priv *priv, bool localack)
>  static void max9286_reverse_channel_setup(struct max9286_priv *priv,
>  					  unsigned int chan_amplitude)
>  {
> +	u8 chan_config;
> +
> +	if (priv->rev_chan_mv == chan_amplitude)
> +		return;
> +
> +	priv->rev_chan_mv = chan_amplitude;
> +
>  	/* Reverse channel transmission time: default to 1. */
> -	u8 chan_config = MAX9286_REV_TRF(1);
> +	chan_config = MAX9286_REV_TRF(1);
>  
>  	/*
>  	 * Reverse channel setup.
> @@ -563,8 +571,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
>  	 * - Disable auto-ack as communication on the control channel are now
>  	 *   stable.
>  	 */
> -	if (priv->init_rev_chan_mv < 170)
> -		max9286_reverse_channel_setup(priv, 170);
> +	max9286_reverse_channel_setup(priv, 170);
>  	max9286_check_config_link(priv, priv->source_mask);
>  
>  	/*

-- 
Regards,

Laurent Pinchart

[PATCH 12/16] media: i2c: max9286: Define high channel amplitude

[Jacopo Mondi]

Provide a macro to define the reverse channel amplitude to
be used to compensate the remote serializer noise immunity.

While at it, update a comment.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 4afb5ca06448..7913b5f2249e 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -113,6 +113,7 @@
 #define MAX9286_REV_TRF(n)		((n) << 4)
 #define MAX9286_REV_AMP(n)		((((n) - 30) / 10) << 1) /* in mV */
 #define MAX9286_REV_AMP_X		BIT(0)
+#define MAX9286_REV_AMP_HIGH		170
 /* Register 0x3f */
 #define MAX9286_EN_REV_CFG		BIT(6)
 #define MAX9286_REV_FLEN(n)		((n) - 20)
@@ -566,12 +567,12 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
 	 * channels:
 	 *
 	 * - Increase the reverse channel amplitude to compensate for the
-	 *   remote ends high threshold, if not done already
+	 *   remote ends high threshold
 	 * - Verify all configuration links are properly detected
 	 * - Disable auto-ack as communication on the control channel are now
 	 *   stable.
 	 */
-	max9286_reverse_channel_setup(priv, 170);
+	max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
 	max9286_check_config_link(priv, priv->source_mask);
 
 	/*
-- 
2.30.0

Re: [PATCH 12/16] media: i2c: max9286: Define high channel amplitude

[Kieran Bingham]

Hi Jacopo,

On 16/02/2021 17:41, Jacopo Mondi wrote:
> Provide a macro to define the reverse channel amplitude to
> be used to compensate the remote serializer noise immunity.
> 
> While at it, update a comment.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 4afb5ca06448..7913b5f2249e 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -113,6 +113,7 @@
>  #define MAX9286_REV_TRF(n)		((n) << 4)
>  #define MAX9286_REV_AMP(n)		((((n) - 30) / 10) << 1) /* in mV */
>  #define MAX9286_REV_AMP_X		BIT(0)
> +#define MAX9286_REV_AMP_HIGH		170
>  /* Register 0x3f */
>  #define MAX9286_EN_REV_CFG		BIT(6)
>  #define MAX9286_REV_FLEN(n)		((n) - 20)
> @@ -566,12 +567,12 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
>  	 * channels:
>  	 *
>  	 * - Increase the reverse channel amplitude to compensate for the
> -	 *   remote ends high threshold, if not done already
> +	 *   remote ends high threshold
>  	 * - Verify all configuration links are properly detected
>  	 * - Disable auto-ack as communication on the control channel are now
>  	 *   stable.
>  	 */
> -	max9286_reverse_channel_setup(priv, 170);
> +	max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
>  	max9286_check_config_link(priv, priv->source_mask);
>  
>  	/*
> 

Re: [PATCH 12/16] media: i2c: max9286: Define high channel amplitude

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Feb 16, 2021 at 06:41:42PM +0100, Jacopo Mondi wrote:
> Provide a macro to define the reverse channel amplitude to
> be used to compensate the remote serializer noise immunity.
> 
> While at it, update a comment.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 4afb5ca06448..7913b5f2249e 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -113,6 +113,7 @@
>  #define MAX9286_REV_TRF(n)		((n) << 4)
>  #define MAX9286_REV_AMP(n)		((((n) - 30) / 10) << 1) /* in mV */
>  #define MAX9286_REV_AMP_X		BIT(0)
> +#define MAX9286_REV_AMP_HIGH		170
>  /* Register 0x3f */
>  #define MAX9286_EN_REV_CFG		BIT(6)
>  #define MAX9286_REV_FLEN(n)		((n) - 20)
> @@ -566,12 +567,12 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
>  	 * channels:
>  	 *
>  	 * - Increase the reverse channel amplitude to compensate for the
> -	 *   remote ends high threshold, if not done already
> +	 *   remote ends high threshold

I would have done this in the previous patch, but it doesn't matter
much.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	 * - Verify all configuration links are properly detected
>  	 * - Disable auto-ack as communication on the control channel are now
>  	 *   stable.
>  	 */
> -	max9286_reverse_channel_setup(priv, 170);
> +	max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
>  	max9286_check_config_link(priv, priv->source_mask);
>  
>  	/*

-- 
Regards,

Laurent Pinchart

[PATCH 13/16] media: i2c: rdacm2x: Implement .init() subdev op

[Jacopo Mondi]

The current probe() procedure of the RDACM20 and RDACM20 performs
initialization of the serializer image sensors and increases the noise
immunity threshold as last operation, which is then compensated by the
remote deserializer by increasing the reverse channel signal amplitude
once all remotes have bound.

The probe routine is then run without noise immunity activated which
in noisy environment conditions makes the probe sequence less reliable as
the chips configuration requires a relevant amount of i2c transactions.

Break chip initialization in two:
- At probe time only configure the serializer's reverse channel with
  noise immunity activated, to reduce the number of transactions
  performed without noise immunity protection
- Move the chips initialization to the .init() core subdev operation to
  be invoked by the deserializer after the camera has probed and it has
  increased the reverse channel amplitude

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm20.c | 65 ++++++++++++++++++++++---------------
 drivers/media/i2c/rdacm21.c | 65 ++++++++++++++++++++++---------------
 2 files changed, 78 insertions(+), 52 deletions(-)

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 39e4b4241870..0632ef98eea7 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -437,36 +437,12 @@ static int rdacm20_get_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static const struct v4l2_subdev_video_ops rdacm20_video_ops = {
-	.s_stream	= rdacm20_s_stream,
-};
-
-static const struct v4l2_subdev_pad_ops rdacm20_subdev_pad_ops = {
-	.enum_mbus_code = rdacm20_enum_mbus_code,
-	.get_fmt	= rdacm20_get_fmt,
-	.set_fmt	= rdacm20_get_fmt,
-};
-
-static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
-	.video		= &rdacm20_video_ops,
-	.pad		= &rdacm20_subdev_pad_ops,
-};
-
-static int rdacm20_initialize(struct rdacm20_device *dev)
+static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
 {
+	struct rdacm20_device *dev = sd_to_rdacm20(sd);
 	unsigned int i;
 	int ret;
 
-	/* Verify communication with the MAX9271: ping to wakeup. */
-	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
-	i2c_smbus_read_byte(dev->serializer.client);
-	usleep_range(5000, 8000);
-
-	/* Serial link disabled during config as it needs a valid pixel clock. */
-	ret = max9271_set_serial_link(&dev->serializer, false);
-	if (ret)
-		return ret;
-
 	/*
 	 *  Ensure that we have a good link configuration before attempting to
 	 *  identify the device.
@@ -537,6 +513,43 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
 
 	dev_info(dev->dev, "Identified RDACM20 camera module\n");
 
+	return 0;
+}
+
+static const struct v4l2_subdev_core_ops rdacm20_core_ops = {
+	.init           = rdacm20_init,
+};
+
+static const struct v4l2_subdev_video_ops rdacm20_video_ops = {
+	.s_stream	= rdacm20_s_stream,
+};
+
+static const struct v4l2_subdev_pad_ops rdacm20_subdev_pad_ops = {
+	.enum_mbus_code = rdacm20_enum_mbus_code,
+	.get_fmt	= rdacm20_get_fmt,
+	.set_fmt	= rdacm20_get_fmt,
+};
+
+static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
+	.core		= &rdacm20_core_ops,
+	.video		= &rdacm20_video_ops,
+	.pad		= &rdacm20_subdev_pad_ops,
+};
+
+static int rdacm20_initialize(struct rdacm20_device *dev)
+{
+	int ret;
+
+	/* Verify communication with the MAX9271: ping to wakeup. */
+	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
+	i2c_smbus_read_byte(dev->serializer.client);
+	usleep_range(5000, 8000);
+
+	/* Serial link disabled during config as it needs a valid pixel clock. */
+	ret = max9271_set_serial_link(&dev->serializer, false);
+	if (ret)
+		return ret;
+
 	/*
 	 * Set reverse channel high threshold to increase noise immunity.
 	 *
diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index c420a6b96879..80b6f16f87a8 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -314,21 +314,6 @@ static int rdacm21_get_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static const struct v4l2_subdev_video_ops rdacm21_video_ops = {
-	.s_stream	= rdacm21_s_stream,
-};
-
-static const struct v4l2_subdev_pad_ops rdacm21_subdev_pad_ops = {
-	.enum_mbus_code = rdacm21_enum_mbus_code,
-	.get_fmt	= rdacm21_get_fmt,
-	.set_fmt	= rdacm21_get_fmt,
-};
-
-static const struct v4l2_subdev_ops rdacm21_subdev_ops = {
-	.video		= &rdacm21_video_ops,
-	.pad		= &rdacm21_subdev_pad_ops,
-};
-
 static int ov10640_initialize(struct rdacm21_device *dev)
 {
 	u8 val;
@@ -448,20 +433,11 @@ static int ov490_initialize(struct rdacm21_device *dev)
 	return 0;
 }
 
-static int rdacm21_initialize(struct rdacm21_device *dev)
+static int rdacm21_init(struct v4l2_subdev *sd, unsigned int val)
 {
+	struct rdacm21_device *dev = sd_to_rdacm21(sd);
 	int ret;
 
-	/* Verify communication with the MAX9271: ping to wakeup. */
-	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
-	i2c_smbus_read_byte(dev->serializer.client);
-	usleep_range(5000, 8000);
-
-	/* Enable reverse channel and disable the serial link. */
-	ret = max9271_set_serial_link(&dev->serializer, false);
-	if (ret)
-		return ret;
-
 	/* Configure I2C bus at 105Kbps speed and configure GMSL. */
 	ret = max9271_configure_i2c(&dev->serializer,
 				    MAX9271_I2CSLVSH_469NS_234NS |
@@ -508,6 +484,43 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
 	if (ret)
 		return ret;
 
+	return 0;
+}
+
+static const struct v4l2_subdev_core_ops rdacm21_core_ops = {
+	.init		= rdacm21_init,
+};
+
+static const struct v4l2_subdev_video_ops rdacm21_video_ops = {
+	.s_stream	= rdacm21_s_stream,
+};
+
+static const struct v4l2_subdev_pad_ops rdacm21_subdev_pad_ops = {
+	.enum_mbus_code = rdacm21_enum_mbus_code,
+	.get_fmt	= rdacm21_get_fmt,
+	.set_fmt	= rdacm21_get_fmt,
+};
+
+static const struct v4l2_subdev_ops rdacm21_subdev_ops = {
+	.core		= &rdacm21_core_ops,
+	.video		= &rdacm21_video_ops,
+	.pad		= &rdacm21_subdev_pad_ops,
+};
+
+static int rdacm21_initialize(struct rdacm21_device *dev)
+{
+	int ret;
+
+	/* Verify communication with the MAX9271: ping to wakeup. */
+	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
+	i2c_smbus_read_byte(dev->serializer.client);
+	usleep_range(5000, 8000);
+
+	/* Enable reverse channel and disable the serial link. */
+	ret = max9271_set_serial_link(&dev->serializer, false);
+	if (ret)
+		return ret;
+
 	/*
 	 * Set reverse channel high threshold to increase noise immunity.
 	 *
-- 
2.30.0

Re: [PATCH 13/16] media: i2c: rdacm2x: Implement .init() subdev op

[Geert Uytterhoeven]

On Tue, Feb 16, 2021 at 6:41 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> The current probe() procedure of the RDACM20 and RDACM20 performs
> initialization of the serializer image sensors and increases the noise
> immunity threshold as last operation, which is then compensated by the
> remote deserializer by increasing the reverse channel signal amplitude
> once all remotes have bound.
>
> The probe routine is then run without noise immunity activated which
> in noisy environment conditions makes the probe sequence less reliable as
> the chips configuration requires a relevant amount of i2c transactions.
>
> Break chip initialization in two:
> - At probe time only configure the serializer's reverse channel with
>   noise immunity activated, to reduce the number of transactions

deactivated?

>   performed without noise immunity protection
> - Move the chips initialization to the .init() core subdev operation to
>   be invoked by the deserializer after the camera has probed and it has
>   increased the reverse channel amplitude
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 13/16] media: i2c: rdacm2x: Implement .init() subdev op

[Kieran Bingham]

Hi Jacopo,

On 16/02/2021 17:41, Jacopo Mondi wrote:
> The current probe() procedure of the RDACM20 and RDACM20 performs

and RDACM21?

> initialization of the serializer image sensors and increases the noise

Of the serializer 'and' image sensors ?
or perhaps just

s/serializer/serializer,/ ?

> immunity threshold as last operation, which is then compensated by the

as a last operation
or
as a final operation


> remote deserializer by increasing the reverse channel signal amplitude
> once all remotes have bound.
> 
> The probe routine is then run without noise immunity activated which
> in noisy environment conditions makes the probe sequence less reliable as
> the chips configuration requires a relevant amount of i2c transactions.

s/relevant/relatively high/ ?

> 
> Break chip initialization in two:
> - At probe time only configure the serializer's reverse channel with
>   noise immunity activated, to reduce the number of transactions
>   performed without noise immunity protection
> - Move the chips initialization to the .init() core subdev operation to
>   be invoked by the deserializer after the camera has probed and it has
>   increased the reverse channel amplitude

Is this the op you said was deprecated?


Functionally in this code, it seems fine, but as mentioned on the next
patch, I suspect it might need squashing to make sure it stays functional...

I'm not fully sure of the implications of this patch, but your tests
have reportede that this series is helping a lot with reliability so I
don't want to block it.

The code changes themselves look ok, with the following thougts:

 - If this op/methodology is deprecated it might be harder to get
   acceptance?

 - now we have _init and _initialise - should that be made more
   distinct?

 - Seeing the duplication of the MAX9271_DEFAULT_ADDR / ping again
   really makes me want to see that wrapped in the max9271.c ;-)

 - Likely needs squashed with relevant changes in max9286?

But even with those thoughts, I don't think this is necessarily wrong so:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/rdacm20.c | 65 ++++++++++++++++++++++---------------
>  drivers/media/i2c/rdacm21.c | 65 ++++++++++++++++++++++---------------
>  2 files changed, 78 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 39e4b4241870..0632ef98eea7 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -437,36 +437,12 @@ static int rdacm20_get_fmt(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> -static const struct v4l2_subdev_video_ops rdacm20_video_ops = {
> -	.s_stream	= rdacm20_s_stream,
> -};
> -
> -static const struct v4l2_subdev_pad_ops rdacm20_subdev_pad_ops = {
> -	.enum_mbus_code = rdacm20_enum_mbus_code,
> -	.get_fmt	= rdacm20_get_fmt,
> -	.set_fmt	= rdacm20_get_fmt,
> -};
> -
> -static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
> -	.video		= &rdacm20_video_ops,
> -	.pad		= &rdacm20_subdev_pad_ops,
> -};
> -
> -static int rdacm20_initialize(struct rdacm20_device *dev)
> +static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
>  {
> +	struct rdacm20_device *dev = sd_to_rdacm20(sd);
>  	unsigned int i;
>  	int ret;
>  
> -	/* Verify communication with the MAX9271: ping to wakeup. */
> -	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> -	i2c_smbus_read_byte(dev->serializer.client);
> -	usleep_range(5000, 8000);
> -
> -	/* Serial link disabled during config as it needs a valid pixel clock. */
> -	ret = max9271_set_serial_link(&dev->serializer, false);
> -	if (ret)
> -		return ret;
> -
>  	/*
>  	 *  Ensure that we have a good link configuration before attempting to
>  	 *  identify the device.
> @@ -537,6 +513,43 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  
>  	dev_info(dev->dev, "Identified RDACM20 camera module\n");
>  
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_core_ops rdacm20_core_ops = {
> +	.init           = rdacm20_init,
> +};
> +
> +static const struct v4l2_subdev_video_ops rdacm20_video_ops = {
> +	.s_stream	= rdacm20_s_stream,
> +};
> +
> +static const struct v4l2_subdev_pad_ops rdacm20_subdev_pad_ops = {
> +	.enum_mbus_code = rdacm20_enum_mbus_code,
> +	.get_fmt	= rdacm20_get_fmt,
> +	.set_fmt	= rdacm20_get_fmt,
> +};
> +
> +static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
> +	.core		= &rdacm20_core_ops,
> +	.video		= &rdacm20_video_ops,
> +	.pad		= &rdacm20_subdev_pad_ops,
> +};
> +
> +static int rdacm20_initialize(struct rdacm20_device *dev)
> +{
> +	int ret;
> +
> +	/* Verify communication with the MAX9271: ping to wakeup. */
> +	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> +	i2c_smbus_read_byte(dev->serializer.client);
> +	usleep_range(5000, 8000);
> +
> +	/* Serial link disabled during config as it needs a valid pixel clock. */
> +	ret = max9271_set_serial_link(&dev->serializer, false);
> +	if (ret)
> +		return ret;
> +
>  	/*
>  	 * Set reverse channel high threshold to increase noise immunity.
>  	 *
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index c420a6b96879..80b6f16f87a8 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -314,21 +314,6 @@ static int rdacm21_get_fmt(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> -static const struct v4l2_subdev_video_ops rdacm21_video_ops = {
> -	.s_stream	= rdacm21_s_stream,
> -};
> -
> -static const struct v4l2_subdev_pad_ops rdacm21_subdev_pad_ops = {
> -	.enum_mbus_code = rdacm21_enum_mbus_code,
> -	.get_fmt	= rdacm21_get_fmt,
> -	.set_fmt	= rdacm21_get_fmt,
> -};
> -
> -static const struct v4l2_subdev_ops rdacm21_subdev_ops = {
> -	.video		= &rdacm21_video_ops,
> -	.pad		= &rdacm21_subdev_pad_ops,
> -};
> -
>  static int ov10640_initialize(struct rdacm21_device *dev)
>  {
>  	u8 val;
> @@ -448,20 +433,11 @@ static int ov490_initialize(struct rdacm21_device *dev)
>  	return 0;
>  }
>  
> -static int rdacm21_initialize(struct rdacm21_device *dev)
> +static int rdacm21_init(struct v4l2_subdev *sd, unsigned int val)
>  {
> +	struct rdacm21_device *dev = sd_to_rdacm21(sd);
>  	int ret;
>  
> -	/* Verify communication with the MAX9271: ping to wakeup. */
> -	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> -	i2c_smbus_read_byte(dev->serializer.client);
> -	usleep_range(5000, 8000);
> -
> -	/* Enable reverse channel and disable the serial link. */
> -	ret = max9271_set_serial_link(&dev->serializer, false);
> -	if (ret)
> -		return ret;
> -
>  	/* Configure I2C bus at 105Kbps speed and configure GMSL. */
>  	ret = max9271_configure_i2c(&dev->serializer,
>  				    MAX9271_I2CSLVSH_469NS_234NS |
> @@ -508,6 +484,43 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
>  	if (ret)
>  		return ret;
>  
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_core_ops rdacm21_core_ops = {
> +	.init		= rdacm21_init,
> +};
> +
> +static const struct v4l2_subdev_video_ops rdacm21_video_ops = {
> +	.s_stream	= rdacm21_s_stream,
> +};
> +
> +static const struct v4l2_subdev_pad_ops rdacm21_subdev_pad_ops = {
> +	.enum_mbus_code = rdacm21_enum_mbus_code,
> +	.get_fmt	= rdacm21_get_fmt,
> +	.set_fmt	= rdacm21_get_fmt,
> +};
> +
> +static const struct v4l2_subdev_ops rdacm21_subdev_ops = {
> +	.core		= &rdacm21_core_ops,
> +	.video		= &rdacm21_video_ops,
> +	.pad		= &rdacm21_subdev_pad_ops,
> +};
> +
> +static int rdacm21_initialize(struct rdacm21_device *dev)
> +{
> +	int ret;
> +
> +	/* Verify communication with the MAX9271: ping to wakeup. */
> +	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> +	i2c_smbus_read_byte(dev->serializer.client);
> +	usleep_range(5000, 8000);
> +
> +	/* Enable reverse channel and disable the serial link. */
> +	ret = max9271_set_serial_link(&dev->serializer, false);
> +	if (ret)
> +		return ret;
> +
>  	/*
>  	 * Set reverse channel high threshold to increase noise immunity.
>  	 *
> 

Re: [PATCH 13/16] media: i2c: rdacm2x: Implement .init() subdev op

[Laurent Pinchart]

Hi Jacopo,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

On Thu, Feb 18, 2021 at 04:13:14PM +0000, Kieran Bingham wrote:
> On 16/02/2021 17:41, Jacopo Mondi wrote:
> > The current probe() procedure of the RDACM20 and RDACM20 performs
> 
> and RDACM21?
> 
> > initialization of the serializer image sensors and increases the noise
> 
> Of the serializer 'and' image sensors ?
> or perhaps just
> 
> s/serializer/serializer,/ ?
> 
> > immunity threshold as last operation, which is then compensated by the
> 
> as a last operation
> or
> as a final operation
> 
> 
> > remote deserializer by increasing the reverse channel signal amplitude
> > once all remotes have bound.
> > 
> > The probe routine is then run without noise immunity activated which

Maybe s/then/thus/ ? I initially interpreted this as meaning the probe
routine is run after the operations listed above.

> > in noisy environment conditions makes the probe sequence less reliable as
> > the chips configuration requires a relevant amount of i2c transactions.
> 
> s/relevant/relatively high/ ?
> 
> > Break chip initialization in two:
> > - At probe time only configure the serializer's reverse channel with
> >   noise immunity activated, to reduce the number of transactions
> >   performed without noise immunity protection
> > - Move the chips initialization to the .init() core subdev operation to
> >   be invoked by the deserializer after the camera has probed and it has
> >   increased the reverse channel amplitude
> 
> Is this the op you said was deprecated?
> 
> 
> Functionally in this code, it seems fine, but as mentioned on the next
> patch, I suspect it might need squashing to make sure it stays functional...
> 
> I'm not fully sure of the implications of this patch, but your tests
> have reportede that this series is helping a lot with reliability so I
> don't want to block it.
> 
> The code changes themselves look ok, with the following thougts:
> 
>  - If this op/methodology is deprecated it might be harder to get
>    acceptance?

I think we need to propose de-deprecating the operation, which should
include a clear explanation of the valid use cases.

>  - now we have _init and _initialise - should that be made more
>    distinct?
> 
>  - Seeing the duplication of the MAX9271_DEFAULT_ADDR / ping again
>    really makes me want to see that wrapped in the max9271.c ;-)
> 
>  - Likely needs squashed with relevant changes in max9286?

I share the bisectability concerns.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> But even with those thoughts, I don't think this is necessarily wrong so:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/rdacm20.c | 65 ++++++++++++++++++++++---------------
> >  drivers/media/i2c/rdacm21.c | 65 ++++++++++++++++++++++---------------
> >  2 files changed, 78 insertions(+), 52 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> > index 39e4b4241870..0632ef98eea7 100644
> > --- a/drivers/media/i2c/rdacm20.c
> > +++ b/drivers/media/i2c/rdacm20.c
> > @@ -437,36 +437,12 @@ static int rdacm20_get_fmt(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >  
> > -static const struct v4l2_subdev_video_ops rdacm20_video_ops = {
> > -	.s_stream	= rdacm20_s_stream,
> > -};
> > -
> > -static const struct v4l2_subdev_pad_ops rdacm20_subdev_pad_ops = {
> > -	.enum_mbus_code = rdacm20_enum_mbus_code,
> > -	.get_fmt	= rdacm20_get_fmt,
> > -	.set_fmt	= rdacm20_get_fmt,
> > -};
> > -
> > -static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
> > -	.video		= &rdacm20_video_ops,
> > -	.pad		= &rdacm20_subdev_pad_ops,
> > -};
> > -
> > -static int rdacm20_initialize(struct rdacm20_device *dev)
> > +static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
> >  {
> > +	struct rdacm20_device *dev = sd_to_rdacm20(sd);
> >  	unsigned int i;
> >  	int ret;
> >  
> > -	/* Verify communication with the MAX9271: ping to wakeup. */
> > -	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> > -	i2c_smbus_read_byte(dev->serializer.client);
> > -	usleep_range(5000, 8000);
> > -
> > -	/* Serial link disabled during config as it needs a valid pixel clock. */
> > -	ret = max9271_set_serial_link(&dev->serializer, false);
> > -	if (ret)
> > -		return ret;
> > -
> >  	/*
> >  	 *  Ensure that we have a good link configuration before attempting to
> >  	 *  identify the device.
> > @@ -537,6 +513,43 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
> >  
> >  	dev_info(dev->dev, "Identified RDACM20 camera module\n");
> >  
> > +	return 0;
> > +}
> > +
> > +static const struct v4l2_subdev_core_ops rdacm20_core_ops = {
> > +	.init           = rdacm20_init,
> > +};
> > +
> > +static const struct v4l2_subdev_video_ops rdacm20_video_ops = {
> > +	.s_stream	= rdacm20_s_stream,
> > +};
> > +
> > +static const struct v4l2_subdev_pad_ops rdacm20_subdev_pad_ops = {
> > +	.enum_mbus_code = rdacm20_enum_mbus_code,
> > +	.get_fmt	= rdacm20_get_fmt,
> > +	.set_fmt	= rdacm20_get_fmt,
> > +};
> > +
> > +static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
> > +	.core		= &rdacm20_core_ops,
> > +	.video		= &rdacm20_video_ops,
> > +	.pad		= &rdacm20_subdev_pad_ops,
> > +};
> > +
> > +static int rdacm20_initialize(struct rdacm20_device *dev)
> > +{
> > +	int ret;
> > +
> > +	/* Verify communication with the MAX9271: ping to wakeup. */
> > +	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> > +	i2c_smbus_read_byte(dev->serializer.client);
> > +	usleep_range(5000, 8000);
> > +
> > +	/* Serial link disabled during config as it needs a valid pixel clock. */
> > +	ret = max9271_set_serial_link(&dev->serializer, false);
> > +	if (ret)
> > +		return ret;
> > +
> >  	/*
> >  	 * Set reverse channel high threshold to increase noise immunity.
> >  	 *
> > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> > index c420a6b96879..80b6f16f87a8 100644
> > --- a/drivers/media/i2c/rdacm21.c
> > +++ b/drivers/media/i2c/rdacm21.c
> > @@ -314,21 +314,6 @@ static int rdacm21_get_fmt(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >  
> > -static const struct v4l2_subdev_video_ops rdacm21_video_ops = {
> > -	.s_stream	= rdacm21_s_stream,
> > -};
> > -
> > -static const struct v4l2_subdev_pad_ops rdacm21_subdev_pad_ops = {
> > -	.enum_mbus_code = rdacm21_enum_mbus_code,
> > -	.get_fmt	= rdacm21_get_fmt,
> > -	.set_fmt	= rdacm21_get_fmt,
> > -};
> > -
> > -static const struct v4l2_subdev_ops rdacm21_subdev_ops = {
> > -	.video		= &rdacm21_video_ops,
> > -	.pad		= &rdacm21_subdev_pad_ops,
> > -};
> > -
> >  static int ov10640_initialize(struct rdacm21_device *dev)
> >  {
> >  	u8 val;
> > @@ -448,20 +433,11 @@ static int ov490_initialize(struct rdacm21_device *dev)
> >  	return 0;
> >  }
> >  
> > -static int rdacm21_initialize(struct rdacm21_device *dev)
> > +static int rdacm21_init(struct v4l2_subdev *sd, unsigned int val)
> >  {
> > +	struct rdacm21_device *dev = sd_to_rdacm21(sd);
> >  	int ret;
> >  
> > -	/* Verify communication with the MAX9271: ping to wakeup. */
> > -	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> > -	i2c_smbus_read_byte(dev->serializer.client);
> > -	usleep_range(5000, 8000);
> > -
> > -	/* Enable reverse channel and disable the serial link. */
> > -	ret = max9271_set_serial_link(&dev->serializer, false);
> > -	if (ret)
> > -		return ret;
> > -
> >  	/* Configure I2C bus at 105Kbps speed and configure GMSL. */
> >  	ret = max9271_configure_i2c(&dev->serializer,
> >  				    MAX9271_I2CSLVSH_469NS_234NS |
> > @@ -508,6 +484,43 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
> >  	if (ret)
> >  		return ret;
> >  
> > +	return 0;
> > +}
> > +
> > +static const struct v4l2_subdev_core_ops rdacm21_core_ops = {
> > +	.init		= rdacm21_init,
> > +};
> > +
> > +static const struct v4l2_subdev_video_ops rdacm21_video_ops = {
> > +	.s_stream	= rdacm21_s_stream,
> > +};
> > +
> > +static const struct v4l2_subdev_pad_ops rdacm21_subdev_pad_ops = {
> > +	.enum_mbus_code = rdacm21_enum_mbus_code,
> > +	.get_fmt	= rdacm21_get_fmt,
> > +	.set_fmt	= rdacm21_get_fmt,
> > +};
> > +
> > +static const struct v4l2_subdev_ops rdacm21_subdev_ops = {
> > +	.core		= &rdacm21_core_ops,
> > +	.video		= &rdacm21_video_ops,
> > +	.pad		= &rdacm21_subdev_pad_ops,
> > +};
> > +
> > +static int rdacm21_initialize(struct rdacm21_device *dev)
> > +{
> > +	int ret;
> > +
> > +	/* Verify communication with the MAX9271: ping to wakeup. */
> > +	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> > +	i2c_smbus_read_byte(dev->serializer.client);
> > +	usleep_range(5000, 8000);
> > +
> > +	/* Enable reverse channel and disable the serial link. */
> > +	ret = max9271_set_serial_link(&dev->serializer, false);
> > +	if (ret)
> > +		return ret;
> > +
> >  	/*
> >  	 * Set reverse channel high threshold to increase noise immunity.
> >  	 *

-- 
Regards,

Laurent Pinchart

[PATCH 14/16] media: i2c: max9286: Initialize remotes when bound

[Jacopo Mondi]

With the introduction of the .init() core subdev operation in the
max9271 GMSL serializer, the max9286 deserializer needs to explicitly
initialize the remote devices by calling the .init() subdev operation on
each probed camera.

Call the .init() subdev operation at remote bound time and toggle
the reverse channel amplitude to compensate for the remote ends
noise immunity threshold.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 7913b5f2249e..c41284de89b6 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -554,25 +554,39 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
 	dev_dbg(&priv->client->dev, "Bound %s pad: %u on index %u\n",
 		subdev->name, src_pad, index);
 
+	/*
+	 * Initialize the remote camera. Increase the channel amplitude
+	 * to compensate for the remote noise immunity threshold.
+	 */
+	max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
+	ret = v4l2_subdev_call(subdev, core, init, 0);
+	if (ret) {
+		dev_err(&priv->client->dev,
+			"Failed to initialize camera device %u\n", index);
+		return ret;
+	}
+
 	/*
 	 * We can only register v4l2_async_notifiers, which do not provide a
 	 * means to register a complete callback. bound_sources allows us to
 	 * identify when all remote serializers have completed their probe.
 	 */
-	if (priv->bound_sources != priv->source_mask)
+	if (priv->bound_sources != priv->source_mask) {
+		/*
+		 * If not all remotes have probed yet, restore the initial
+		 * reverse channel amplitude to allow the next camera to probe.
+		 */
+		max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);
 		return 0;
+	}
 
 	/*
 	 * All enabled sources have probed and enabled their reverse control
 	 * channels:
-	 *
-	 * - Increase the reverse channel amplitude to compensate for the
-	 *   remote ends high threshold
 	 * - Verify all configuration links are properly detected
 	 * - Disable auto-ack as communication on the control channel are now
 	 *   stable.
 	 */
-	max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
 	max9286_check_config_link(priv, priv->source_mask);
 
 	/*
-- 
2.30.0

Re: [PATCH 14/16] media: i2c: max9286: Initialize remotes when bound

[Geert Uytterhoeven]

On Tue, Feb 16, 2021 at 6:41 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> With the introduction of the .init() core subdev operation in the
> max9271 GMSL serializer, the max9286 deserializer needs to explicitly
> initialize the remote devices by calling the .init() subdev operation on
> each probed camera.
>
> Call the .init() subdev operation at remote bound time and toggle
> the reverse channel amplitude to compensate for the remote ends
> noise immunity threshold.

remote end's?
remote ends' ... thresholds?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 14/16] media: i2c: max9286: Initialize remotes when bound

[Kieran Bingham]

On 16/02/2021 17:41, Jacopo Mondi wrote:
> With the introduction of the .init() core subdev operation in the
> max9271 GMSL serializer, the max9286 deserializer needs to explicitly
> initialize the remote devices by calling the .init() subdev operation on
> each probed camera.
> 
> Call the .init() subdev operation at remote bound time and toggle
> the reverse channel amplitude to compensate for the remote ends
> noise immunity threshold.
> 

Should this patch be merged with the previous one to keep the RDACM2x
and max9286 usage aligned?

I expect it won't compile fail, but it would fail a test (if bisecting
was testing the capture).

Seems to look ok, given the previous patch as a dependency:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 7913b5f2249e..c41284de89b6 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -554,25 +554,39 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
>  	dev_dbg(&priv->client->dev, "Bound %s pad: %u on index %u\n",
>  		subdev->name, src_pad, index);
>  
> +	/*
> +	 * Initialize the remote camera. Increase the channel amplitude
> +	 * to compensate for the remote noise immunity threshold.
> +	 */
> +	max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
> +	ret = v4l2_subdev_call(subdev, core, init, 0);
> +	if (ret) {
> +		dev_err(&priv->client->dev,
> +			"Failed to initialize camera device %u\n", index);
> +		return ret;
> +	}
> +
>  	/*
>  	 * We can only register v4l2_async_notifiers, which do not provide a
>  	 * means to register a complete callback. bound_sources allows us to
>  	 * identify when all remote serializers have completed their probe.
>  	 */
> -	if (priv->bound_sources != priv->source_mask)
> +	if (priv->bound_sources != priv->source_mask) {
> +		/*
> +		 * If not all remotes have probed yet, restore the initial
> +		 * reverse channel amplitude to allow the next camera to probe.
> +		 */
> +		max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);

Oh - wow, now I see why we definitely need to store both the initial and
the current value.



>  		return 0;
> +	}
>  
>  	/*
>  	 * All enabled sources have probed and enabled their reverse control
>  	 * channels:
> -	 *
> -	 * - Increase the reverse channel amplitude to compensate for the
> -	 *   remote ends high threshold
>  	 * - Verify all configuration links are properly detected
>  	 * - Disable auto-ack as communication on the control channel are now
>  	 *   stable.
>  	 */
> -	max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
>  	max9286_check_config_link(priv, priv->source_mask);
>  
>  	/*
> 

Re: [PATCH 14/16] media: i2c: max9286: Initialize remotes when bound

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Feb 16, 2021 at 06:41:44PM +0100, Jacopo Mondi wrote:
> With the introduction of the .init() core subdev operation in the
> max9271 GMSL serializer, the max9286 deserializer needs to explicitly
> initialize the remote devices by calling the .init() subdev operation on
> each probed camera.
> 
> Call the .init() subdev operation at remote bound time and toggle
> the reverse channel amplitude to compensate for the remote ends
> noise immunity threshold.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 7913b5f2249e..c41284de89b6 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -554,25 +554,39 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
>  	dev_dbg(&priv->client->dev, "Bound %s pad: %u on index %u\n",
>  		subdev->name, src_pad, index);
>  
> +	/*
> +	 * Initialize the remote camera. Increase the channel amplitude
> +	 * to compensate for the remote noise immunity threshold.
> +	 */
> +	max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
> +	ret = v4l2_subdev_call(subdev, core, init, 0);
> +	if (ret) {
> +		dev_err(&priv->client->dev,
> +			"Failed to initialize camera device %u\n", index);
> +		return ret;
> +	}
> +
>  	/*
>  	 * We can only register v4l2_async_notifiers, which do not provide a
>  	 * means to register a complete callback. bound_sources allows us to
>  	 * identify when all remote serializers have completed their probe.
>  	 */
> -	if (priv->bound_sources != priv->source_mask)
> +	if (priv->bound_sources != priv->source_mask) {
> +		/*
> +		 * If not all remotes have probed yet, restore the initial
> +		 * reverse channel amplitude to allow the next camera to probe.
> +		 */
> +		max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);
>  		return 0;
> +	}

Instead of going back and forth, would it make sense to incerase the
channel amplitude here, and call the init() subdev operation in a loop
over all cameras ?

Otherwise the patch looks good to me, but I agree with Kieran that it
should probably be squashed with 13/16.

>  
>  	/*
>  	 * All enabled sources have probed and enabled their reverse control
>  	 * channels:
> -	 *
> -	 * - Increase the reverse channel amplitude to compensate for the
> -	 *   remote ends high threshold
>  	 * - Verify all configuration links are properly detected
>  	 * - Disable auto-ack as communication on the control channel are now
>  	 *   stable.
>  	 */
> -	max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
>  	max9286_check_config_link(priv, priv->source_mask);
>  
>  	/*

-- 
Regards,

Laurent Pinchart

[PATCH 15/16] media: i2c: max9286: Rework comments in .bound()

[Jacopo Mondi]

Re-phrase a comment in .bound() callback to make it clear we register
a subdev notifier and remove a redundant comment about disabling i2c
auto-ack.

No functional changes intended.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index c41284de89b6..aa01d5bb79ef 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -567,9 +567,9 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
 	}
 
 	/*
-	 * We can only register v4l2_async_notifiers, which do not provide a
-	 * means to register a complete callback. bound_sources allows us to
-	 * identify when all remote serializers have completed their probe.
+	 * As we register a subdev notifiers we won't get a .complete() callback
+	 * here, so we have to use bound_sources to identify when all remote
+	 * serializers have probed.
 	 */
 	if (priv->bound_sources != priv->source_mask) {
 		/*
@@ -583,16 +583,12 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
 	/*
 	 * All enabled sources have probed and enabled their reverse control
 	 * channels:
+	 * - The reverse channel amplitude stays high
 	 * - Verify all configuration links are properly detected
-	 * - Disable auto-ack as communication on the control channel are now
-	 *   stable.
+	 * - Disable auto-ack as communications on the control channel are now
+	 *   stable
 	 */
 	max9286_check_config_link(priv, priv->source_mask);
-
-	/*
-	 * Re-configure I2C with local acknowledge disabled after cameras have
-	 * probed.
-	 */
 	max9286_configure_i2c(priv, false);
 
 	return max9286_set_pixelrate(priv);
-- 
2.30.0

Re: [PATCH 15/16] media: i2c: max9286: Rework comments in .bound()

[Kieran Bingham]

On 16/02/2021 17:41, Jacopo Mondi wrote:
> Re-phrase a comment in .bound() callback to make it clear we register
> a subdev notifier and remove a redundant comment about disabling i2c
> auto-ack.
> 
> No functional changes intended.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index c41284de89b6..aa01d5bb79ef 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -567,9 +567,9 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
>  	}
>  
>  	/*
> -	 * We can only register v4l2_async_notifiers, which do not provide a
> -	 * means to register a complete callback. bound_sources allows us to
> -	 * identify when all remote serializers have completed their probe.
> +	 * As we register a subdev notifiers we won't get a .complete() callback
> +	 * here, so we have to use bound_sources to identify when all remote
> +	 * serializers have probed.

s/subdev notifiers/subdev notifier/ ?

... I'm not sure that helps me ? :S

>  	 */
>  	if (priv->bound_sources != priv->source_mask) {
>  		/*
> @@ -583,16 +583,12 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
>  	/*
>  	 * All enabled sources have probed and enabled their reverse control
>  	 * channels:
> +	 * - The reverse channel amplitude stays high

Isn't this part of the previous patch?


>  	 * - Verify all configuration links are properly detected
> -	 * - Disable auto-ack as communication on the control channel are now
> -	 *   stable.
> +	 * - Disable auto-ack as communications on the control channel are now
> +	 *   stable
>  	 */
>  	max9286_check_config_link(priv, priv->source_mask);
> -
> -	/*
> -	 * Re-configure I2C with local acknowledge disabled after cameras have
> -	 * probed.
> -	 */
>  	max9286_configure_i2c(priv, false);
>  
>  	return max9286_set_pixelrate(priv);
> 

[PATCH 16/16] media: i2c: gmsl: Use 339Kbps I2C bit-rate

[Jacopo Mondi]

With the camera modules initialization routines now running with
the noise immunity threshold enabled, it is possible to restore
the bit rate of the I2C transactions transported on the GMSL control
channel to 339 Kbps.

The 339 Kbps bit rate represents the default setting for the serializer
and the deserializer chips, and the setup/hold time and slave timeout
time in use are calibrate to support that rate.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 2 +-
 drivers/media/i2c/rdacm20.c | 2 +-
 drivers/media/i2c/rdacm21.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index aa01d5bb79ef..0b620f2f8c41 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -330,7 +330,7 @@ static int max9286_i2c_mux_init(struct max9286_priv *priv)
 static void max9286_configure_i2c(struct max9286_priv *priv, bool localack)
 {
 	u8 config = MAX9286_I2CSLVSH_469NS_234NS | MAX9286_I2CSLVTO_1024US |
-		    MAX9286_I2CMSTBT_105KBPS;
+		    MAX9286_I2CMSTBT_339KBPS;
 
 	if (localack)
 		config |= MAX9286_I2CLOCACK;
diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 0632ef98eea7..d45e8b0e52a0 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -450,7 +450,7 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
 	ret = max9271_configure_i2c(&dev->serializer,
 				    MAX9271_I2CSLVSH_469NS_234NS |
 				    MAX9271_I2CSLVTO_1024US |
-				    MAX9271_I2CMSTBT_105KBPS);
+				    MAX9271_I2CMSTBT_339KBPS);
 	if (ret)
 		return ret;
 
diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 80b6f16f87a8..552985026458 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -442,7 +442,7 @@ static int rdacm21_init(struct v4l2_subdev *sd, unsigned int val)
 	ret = max9271_configure_i2c(&dev->serializer,
 				    MAX9271_I2CSLVSH_469NS_234NS |
 				    MAX9271_I2CSLVTO_1024US |
-				    MAX9271_I2CMSTBT_105KBPS);
+				    MAX9271_I2CMSTBT_339KBPS);
 	if (ret)
 		return ret;
 
-- 
2.30.0

Re: [PATCH 16/16] media: i2c: gmsl: Use 339Kbps I2C bit-rate

[Geert Uytterhoeven]

On Tue, Feb 16, 2021 at 6:41 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> With the camera modules initialization routines now running with
> the noise immunity threshold enabled, it is possible to restore
> the bit rate of the I2C transactions transported on the GMSL control
> channel to 339 Kbps.
>
> The 339 Kbps bit rate represents the default setting for the serializer
> and the deserializer chips, and the setup/hold time and slave timeout
> time in use are calibrate to support that rate.

calibrated

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 16/16] media: i2c: gmsl: Use 339Kbps I2C bit-rate

[Kieran Bingham]

On 16/02/2021 17:41, Jacopo Mondi wrote:
> With the camera modules initialization routines now running with
> the noise immunity threshold enabled, it is possible to restore
> the bit rate of the I2C transactions transported on the GMSL control
> channel to 339 Kbps.
> 
> The 339 Kbps bit rate represents the default setting for the serializer
> and the deserializer chips, and the setup/hold time and slave timeout
> time in use are calibrate to support that rate.

s/calibrate/calibrated/

Does that mean the setup/hold time and timeouts should be adjusted based
on the i2c speed? (which we have not been doing?)

With all of your other reliability improvements, does *this* change
alone have any difference or impact on reliability?

I.e. if we go slower (stay at current speed) - would we be more reliable?

Is there a reliability improvement by making this speed faster?

I presume we don't have a way to convey the i2c bus speed between the
max9286 and the max9271 currently? Seems a bit like a bus parameter....





> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 2 +-
>  drivers/media/i2c/rdacm20.c | 2 +-
>  drivers/media/i2c/rdacm21.c | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index aa01d5bb79ef..0b620f2f8c41 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -330,7 +330,7 @@ static int max9286_i2c_mux_init(struct max9286_priv *priv)
>  static void max9286_configure_i2c(struct max9286_priv *priv, bool localack)
>  {
>  	u8 config = MAX9286_I2CSLVSH_469NS_234NS | MAX9286_I2CSLVTO_1024US |
> -		    MAX9286_I2CMSTBT_105KBPS;
> +		    MAX9286_I2CMSTBT_339KBPS;
>  
>  	if (localack)
>  		config |= MAX9286_I2CLOCACK;
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 0632ef98eea7..d45e8b0e52a0 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -450,7 +450,7 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
>  	ret = max9271_configure_i2c(&dev->serializer,
>  				    MAX9271_I2CSLVSH_469NS_234NS |
>  				    MAX9271_I2CSLVTO_1024US |
> -				    MAX9271_I2CMSTBT_105KBPS);
> +				    MAX9271_I2CMSTBT_339KBPS);
>  	if (ret)
>  		return ret;
>  
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index 80b6f16f87a8..552985026458 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -442,7 +442,7 @@ static int rdacm21_init(struct v4l2_subdev *sd, unsigned int val)
>  	ret = max9271_configure_i2c(&dev->serializer,
>  				    MAX9271_I2CSLVSH_469NS_234NS |
>  				    MAX9271_I2CSLVTO_1024US |
> -				    MAX9271_I2CMSTBT_105KBPS);
> +				    MAX9271_I2CMSTBT_339KBPS);
>  	if (ret)
>  		return ret;
>  
> 

Re: [PATCH 16/16] media: i2c: gmsl: Use 339Kbps I2C bit-rate

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Feb 18, 2021 at 04:07:30PM +0000, Kieran Bingham wrote:
> On 16/02/2021 17:41, Jacopo Mondi wrote:
> > With the camera modules initialization routines now running with
> > the noise immunity threshold enabled, it is possible to restore
> > the bit rate of the I2C transactions transported on the GMSL control
> > channel to 339 Kbps.
> > 
> > The 339 Kbps bit rate represents the default setting for the serializer
> > and the deserializer chips, and the setup/hold time and slave timeout
> > time in use are calibrate to support that rate.
> 
> s/calibrate/calibrated/
> 
> Does that mean the setup/hold time and timeouts should be adjusted based
> on the i2c speed? (which we have not been doing?)
> 
> With all of your other reliability improvements, does *this* change
> alone have any difference or impact on reliability?
> 
> I.e. if we go slower (stay at current speed) - would we be more reliable?
> 
> Is there a reliability improvement by making this speed faster?
> 
> I presume we don't have a way to convey the i2c bus speed between the
> max9286 and the max9271 currently? Seems a bit like a bus parameter....

There's a max bus speed DT parameter that we can use. And I think we
should, as, in theory at least, there's no guarantee that all the
devices behind the serializer can operate at 400kbps.

> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/max9286.c | 2 +-
> >  drivers/media/i2c/rdacm20.c | 2 +-
> >  drivers/media/i2c/rdacm21.c | 2 +-
> >  3 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index aa01d5bb79ef..0b620f2f8c41 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -330,7 +330,7 @@ static int max9286_i2c_mux_init(struct max9286_priv *priv)
> >  static void max9286_configure_i2c(struct max9286_priv *priv, bool localack)
> >  {
> >  	u8 config = MAX9286_I2CSLVSH_469NS_234NS | MAX9286_I2CSLVTO_1024US |
> > -		    MAX9286_I2CMSTBT_105KBPS;
> > +		    MAX9286_I2CMSTBT_339KBPS;
> >  
> >  	if (localack)
> >  		config |= MAX9286_I2CLOCACK;
> > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> > index 0632ef98eea7..d45e8b0e52a0 100644
> > --- a/drivers/media/i2c/rdacm20.c
> > +++ b/drivers/media/i2c/rdacm20.c
> > @@ -450,7 +450,7 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
> >  	ret = max9271_configure_i2c(&dev->serializer,
> >  				    MAX9271_I2CSLVSH_469NS_234NS |
> >  				    MAX9271_I2CSLVTO_1024US |
> > -				    MAX9271_I2CMSTBT_105KBPS);
> > +				    MAX9271_I2CMSTBT_339KBPS);
> >  	if (ret)
> >  		return ret;
> >  
> > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> > index 80b6f16f87a8..552985026458 100644
> > --- a/drivers/media/i2c/rdacm21.c
> > +++ b/drivers/media/i2c/rdacm21.c
> > @@ -442,7 +442,7 @@ static int rdacm21_init(struct v4l2_subdev *sd, unsigned int val)
> >  	ret = max9271_configure_i2c(&dev->serializer,
> >  				    MAX9271_I2CSLVSH_469NS_234NS |
> >  				    MAX9271_I2CSLVTO_1024US |
> > -				    MAX9271_I2CMSTBT_105KBPS);
> > +				    MAX9271_I2CMSTBT_339KBPS);
> >  	if (ret)
> >  		return ret;
> >  

-- 
Regards,

Laurent Pinchart