Re: [PATCH v1 15/15] media: i2c: imx290: Simplify imx290_set_data_lanes()

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hello Laurent,

thanks for working on this.

Am Dienstag, 22. November 2022, 23:32:50 CET schrieb Laurent Pinchart:
> There's no need to check for an incorrect number of data lanes in
> imx290_set_data_lanes() as the value is validated at probe() time. Drop
> the check.
> 
> The PHY_LANE_NUM and CSI_LANE_MODE registers are programmed with a value
> equal to the number of lanes minus one. Compute it instead of handling
> it in the switch/case.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/i2c/imx290.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 4dfa090f918d..369db35a7afd 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -512,28 +512,21 @@ static int imx290_set_register_array(struct imx290
> *imx290,
> 
>  static int imx290_set_data_lanes(struct imx290 *imx290)
>  {
> -	int ret = 0, laneval, frsel;
> +	int ret = 0;

You can remove ret and make function returning void as well as there 
(currently) is no error handling at all. The current single caller also 
ignores the return value.

Regards,
Alexander

> +	u32 frsel;
> 
>  	switch (imx290->nlanes) {
>  	case 2:
> -		laneval = 0x01;
> +	default:
>  		frsel = 0x02;
>  		break;
>  	case 4:
> -		laneval = 0x03;
>  		frsel = 0x01;
>  		break;
> -	default:
> -		/*
> -		 * We should never hit this since the data lane count is
> -		 * validated in probe itself
> -		 */
> -		dev_err(imx290->dev, "Lane configuration not 
supported\n");
> -		return -EINVAL;
>  	}
> 
> -	imx290_write(imx290, IMX290_PHY_LANE_NUM, laneval, &ret);
> -	imx290_write(imx290, IMX290_CSI_LANE_MODE, laneval, &ret);
> +	imx290_write(imx290, IMX290_PHY_LANE_NUM, imx290->nlanes - 1, &ret);
> +	imx290_write(imx290, IMX290_CSI_LANE_MODE, imx290->nlanes - 1, 
&ret);
>  	imx290_write(imx290, IMX290_FR_FDG_SEL, frsel, &ret);
> 
>  	return ret;