lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <YDL/npLVS7vk3TV7@pendragon.ideasonboard.com>
Date:   Mon, 22 Feb 2021 02:49:34 +0200
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Jacopo Mondi <jacopo+renesas@...ndi.org>
Cc:     Kieran Bingham <kieran.bingham+renesas@...asonboard.com>,
        niklas.soderlund+renesas@...natech.se, geert@...ux-m68k.org,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-media@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/16] media: i2c: rdacm20: Enable noise immunity

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@...ndi.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@...asonboard.com>
> 
> >  }
> >  
> >  static int rdacm20_probe(struct i2c_client *client)

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ