Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 17 Jun 2020 21:08:55 +0000
From:   Tomasz Figa <tfiga@chromium.org>
To:     Helen Koike <helen.koike@collabora.com>
Cc:     linux-media@vger.kernel.org, mchehab@kernel.org,
        dafna.hirschfeld@collabora.com, linux-kernel@vger.kernel.org,
        tfiga@google.com, hans.verkuil@cisco.com, kernel@collabora.com,
        Wojciech Zabolotny <wzab01@gmail.com>
Subject: Re: [PATCH] media: staging: rkisp1: isp: check return value from
 phy_*
Message-ID: <20200617210855.GA81308@chromium.org>
References: <20200617182229.164675-1-helen.koike@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200617182229.164675-1-helen.koike@collabora.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Helen,

On Wed, Jun 17, 2020 at 03:22:29PM -0300, Helen Koike wrote:
> When starting streaming, do not ignore return value from phy_set_mode(),
> phy_configure() and phy_power_on().
> If it fails, return error to the user.
> 
> Fixes: d65dd85281fb ("media: staging: rkisp1: add Rockchip ISP1 base driver")
> 
> Reported-by: Wojciech Zabolotny <wzab01@gmail.com>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> 
> ---
> 
>  drivers/staging/media/rkisp1/rkisp1-isp.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 

Thank you for the patch. Please see my comments inline.

> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index dc2b59a0160a8..531047fc34a01 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -892,6 +892,7 @@ static int rkisp1_mipi_csi2_start(struct rkisp1_isp *isp,
>  	union phy_configure_opts opts;
>  	struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;
>  	s64 pixel_clock;
> +	int ret;
>  
>  	if (!sensor->pixel_rate_ctrl) {
>  		dev_warn(sensor->sd->dev, "No pixel rate control in subdev\n");
> @@ -906,9 +907,24 @@ static int rkisp1_mipi_csi2_start(struct rkisp1_isp *isp,
>  
>  	phy_mipi_dphy_get_default_config(pixel_clock, isp->sink_fmt->bus_width,
>  					 sensor->lanes, cfg);
> -	phy_set_mode(sensor->dphy, PHY_MODE_MIPI_DPHY);
> -	phy_configure(sensor->dphy, &opts);
> -	phy_power_on(sensor->dphy);
> +
> +	ret = phy_set_mode(sensor->dphy, PHY_MODE_MIPI_DPHY);
> +	if (ret) {

nit: I don't seem to be able to find any documentation for this API and
it's not clear if it's guaranteed that the API doesn't return positive
values. It would probably be safer to check for ret < 0.

> +		dev_err(sensor->sd->dev, "Fail setting MIPI DPHY mode\n");
> +		return -EINVAL;

Should we just return ret?

> +	}
> +
> +	ret = phy_configure(sensor->dphy, &opts);
> +	if (ret && ret != -EOPNOTSUPP) {

Why are we okay with -EOPNOTSUPP?

> +		dev_err(sensor->sd->dev, "Fail configuring MIPI DPHY\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = phy_power_on(sensor->dphy);
> +	if (ret) {
> +		dev_err(sensor->sd->dev, "Fail powering on MIPI DPHY\n");
> +		return -EINVAL;

Ditto.

Best regards,
Tomasz