Re: [PATCH v7 2/7] fixup! media: i2c: rdacm21: Fix GPIO handling

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: kieran.bingham+renesas@ideasonboard.com,
	laurent.pinchart+renesas@ideasonboard.com,
	niklas.soderlund+renesas@ragnatech.se, geert@linux-m68k.org,
	linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
	linux-kernel@vger.kernel.org, Hyun Kwon <hyunk@xilinx.com>,
	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	sergei.shtylyov@gmail.com
Subject: Re: [PATCH v7 2/7] fixup! media: i2c: rdacm21: Fix GPIO handling
Date: Thu, 14 Jan 2021 01:09:42 +0200	[thread overview]
Message-ID: <X/99thQ5z/lJGrjJ@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20210113185506.119808-3-jacopo+renesas@jmondi.org>

Hi Jacopo,

Thank you for the patch.

On Wed, Jan 13, 2021 at 07:55:00PM +0100, Jacopo Mondi wrote:
> The MAX9271 GPIO line connected to the OV490 RESETB line is
> GPIO1, not GPO. As the GPIO1 line is not enabled by default, first
> enable it then control the OV490 reset during the MAX9271 configuration
> procedure.
> 
> Before this change the embedded OV490 ISP was not actually reset.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

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

> ---
>  drivers/media/i2c/rdacm21.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index f3841369768c..0428e3209463 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -425,27 +425,23 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
>  	if (ret)
>  		return ret;
>  
> -	ret = max9271_configure_gmsl_link(&dev->serializer);
> +	/* Enable GPIO1 and hold OV490 in reset during max9271 configuration. */
> +	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
>  	if (ret)
>  		return ret;
>  
> -	/* Set GPO high to hold OV490 in reset. */
> -	ret = max9271_set_gpios(&dev->serializer, MAX9271_GPO);
> +	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
>  	if (ret)
>  		return ret;
>  
> -	ret = max9271_set_address(&dev->serializer, dev->addrs[0]);
> +	ret = max9271_configure_gmsl_link(&dev->serializer);
>  	if (ret)
>  		return ret;
> -	dev->serializer.client->addr = dev->addrs[0];
>  
> -	/*
> -	 * Release OV490 from reset and program address translation
> -	 * before performing OV490 configuration.
> -	 */
> -	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPO);
> +	ret = max9271_set_address(&dev->serializer, dev->addrs[0]);
>  	if (ret)
>  		return ret;
> +	dev->serializer.client->addr = dev->addrs[0];
>  
>  	ret = max9271_set_translation(&dev->serializer, dev->addrs[1],
>  				      OV490_I2C_ADDRESS);
> @@ -453,6 +449,12 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
>  		return ret;
>  	dev->isp->addr = dev->addrs[1];
>  
> +	/* Release OV490 from reset and initialize it. */
> +	ret = max9271_set_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> +	if (ret)
> +		return ret;
> +	usleep_range(3000, 5000);
> +
>  	ret = ov490_initialize(dev);
>  	if (ret)
>  		return ret;

-- 
Regards,

Laurent Pinchart