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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Alexander,

On Wed, Nov 23, 2022 at 10:04:58AM +0100, Alexander Stein wrote:
> 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.

There's error handling in this function, in the imx290_write() calls.
The ret variable should stay at least for that. As for making the
function void, I would rather fix the caller. I'll add a patch for that
in v2.

> > +	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;

-- 
Regards,

Laurent Pinchart