Subject: Re: [PATCH v2] media: imx: mipi csi-2: Don't fail if initial state
 times-out
To:     Fabio Estevam <festevam@gmail.com>, hverkuil@xs4all.nl
Cc:     p.zabel@pengutronix.de, linux-imx@nxp.com,
        linux-media@vger.kernel.org, kernel@pengutronix.de,
        shawnguo@kernel.org, mchehab@kernel.org, ezequiel@collabora.com
References: <20190627221359.18960-1-festevam@gmail.com>
From:   Steve Longerbeam <slongerbeam@gmail.com>
Message-ID: <9bd5768b-63ba-93ff-00ea-e288b04386e8@gmail.com>
Date:   Thu, 27 Jun 2019 15:17:29 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.1
MIME-Version: 1.0
In-Reply-To: <20190627221359.18960-1-festevam@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Sender: linux-media-owner@vger.kernel.org
Precedence: bulk


On 6/27/19 3:13 PM, Fabio Estevam wrote:
> From: Ezequiel Garcia <ezequiel@collabora.com>
>
> Not all sensors will be able to guarantee a proper initial state.
> This may be either because the driver is not properly written,
> or (probably unlikely) because the hardware won't support it.
>
> While the right solution in the former case is to fix the sensor
> driver, the real world not always allows right solutions, due to lack
> of available documentation and support on these sensors.
>
> Let's relax this requirement, and allow the driver to support stream start,
> even if the sensor initial sequence wasn't the expected.
>
> Also improve the warning message to better explain the problem and provide
> a hint that the sensor driver needs to be fixed.
>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Signed-off-by: Fabio Estevam <festevam@gmail.com>
> ---
> Changes since v1:
> - Changed the warning message to better explain the problem and provide
> a hint that the sensor driver needs to be fixed. (Phillip)
> - Keep printing the phy_state information (Phillip)
> - Do not change csi2_dphy_wait_clock_lane() (Phillip/Steve)
>
>   drivers/staging/media/imx/imx6-mipi-csi2.c | 14 ++++++--------
>   1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
> index f29e28df36ed..240f992ad2ef 100644
> --- a/drivers/staging/media/imx/imx6-mipi-csi2.c
> +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
> @@ -243,7 +243,7 @@ static int __maybe_unused csi2_dphy_wait_ulp(struct csi2_dev *csi2)
>   }
>   
>   /* Waits for low-power LP-11 state on data and clock lanes. */
> -static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
> +static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
>   {
>   	u32 mask, reg;
>   	int ret;
> @@ -254,11 +254,11 @@ static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
>   	ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
>   				 (reg & mask) == mask, 0, 500000);
>   	if (ret) {
> -		v4l2_err(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg);
> -		return ret;
> +		v4l2_warn(&csi2->sd, "Timeout waiting for LP-11 state on all active lanes.\n");
> +		v4l2_warn(&csi2->sd, "This is most likely caused by a bug in the sensor driver.\n");
> +		v4l2_warn(&csi2->sd, "Capture might fail or contain visual artifacts.\n");

I think this should be reduced, something like:

"LP-11 wait timeout, likely a sensor driver bug, expect capture failures"

Steve

> +		v4l2_warn(&csi2->sd, "phy_state = 0x%08x\n", reg);
>   	}
> -
> -	return 0;
>   }
>   
>   /* Wait for active clock on the clock lane. */
> @@ -316,9 +316,7 @@ static int csi2_start(struct csi2_dev *csi2)
>   	csi2_enable(csi2, true);
>   
>   	/* Step 5 */
> -	ret = csi2_dphy_wait_stopstate(csi2);
> -	if (ret)
> -		goto err_assert_reset;
> +	csi2_dphy_wait_stopstate(csi2);
>   
>   	/* Step 6 */
>   	ret = v4l2_subdev_call(csi2->src_sd, video, s_stream, 1);