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

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: <3e759da5-9bba-54ae-fe39-a7db2cbbb31c@ideasonboard.com>
Date:   Wed, 17 Feb 2021 13:33:01 +0000
From:   Kieran Bingham <kieran.bingham+renesas@...asonboard.com>
To:     Jacopo Mondi <jacopo+renesas@...ndi.org>,
        laurent.pinchart+renesas@...asonboard.com,
        niklas.soderlund+renesas@...natech.se, geert@...ux-m68k.org
Cc:     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 07/16] media: i2c: rdacm2x: Fix wake up delay

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@...asonboard.com>
	> Signed-off-by: Jacopo Mondi <jacopo+renesas@...ndi.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);
>