Re: [PATCH 2/4] media: i2c: imx214: Move controls init to separate function

[André Apitzsch]

Am Dienstag, dem 24.10.2023 um 09:22 +0200 schrieb Jacopo Mondi:
> Hi Andre'
> 
> On Mon, Oct 23, 2023 at 11:47:51PM +0200, André Apitzsch wrote:
> > Code refinement, no functional changes.
> > 
> > Signed-off-by: André Apitzsch <git@xxxxxxxxxxx>
> > ---
> >  drivers/media/i2c/imx214.c | 111 ++++++++++++++++++++++++++-------
> > ------------
> >  1 file changed, 64 insertions(+), 47 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx214.c
> > b/drivers/media/i2c/imx214.c
> > index 9218c149d4c8..554fc4733128 100644
> > --- a/drivers/media/i2c/imx214.c
> > +++ b/drivers/media/i2c/imx214.c
> > @@ -695,6 +695,68 @@ static const struct v4l2_ctrl_ops
> > imx214_ctrl_ops = {
> >  	.s_ctrl = imx214_set_ctrl,
> >  };
> > 
> > +static int imx214_ctrls_init(struct imx214 *imx214)
> > +{
> > +	static const s64 link_freq[] = {
> > +		IMX214_DEFAULT_LINK_FREQ
> > +	};
> > +	static const struct v4l2_area unit_size = {
> > +		.width = 1120,
> > +		.height = 1120,
> > +	};
> > +	struct v4l2_ctrl_handler *ctrl_hdlr;
> > +	int ret;
> > +
> > +	ctrl_hdlr = &imx214->ctrls;
> > +	ret = v4l2_ctrl_handler_init(&imx214->ctrls, 3);
> 
> I know it was already like this, but you could take occasion to
> pre-allocate enough control slots. I count 4 here, plus the 2 parsed
> from system firware in the next patch.
> 
> You can change this here and mention it in the commit message or with
> a separate patch on top. Up to you!
> 
I will add it to the next patch ("Read orientation and rotation from
system firmware"). As it should be increased there anyway. Hope that's
fine.

> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	imx214->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, NULL,
> > +					      
> > V4L2_CID_PIXEL_RATE, 0,
> > +					      
> > IMX214_DEFAULT_PIXEL_RATE, 1,
> > +					      
> > IMX214_DEFAULT_PIXEL_RATE);
> > +
> > +	imx214->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> > NULL,
> > +						  
> > V4L2_CID_LINK_FREQ,
> > +						  
> > ARRAY_SIZE(link_freq) - 1,
> > +						   0, link_freq);
> > +	if (imx214->link_freq)
> > +		imx214->link_freq->flags |=
> > V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > +	/*
> > +	 * WARNING!
> > +	 * Values obtained reverse engineering blobs and/or
> > devices.
> > +	 * Ranges and functionality might be wrong.
> > +	 *
> > +	 * Sony, please release some register set documentation
> > for the
> > +	 * device.
> > +	 *
> > +	 * Yours sincerely, Ricardo.
> > +	 */
> > +	imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr,
> > &imx214_ctrl_ops,
> > +					     V4L2_CID_EXPOSURE,
> > +					     IMX214_EXPOSURE_MIN,
> > +					     IMX214_EXPOSURE_MAX,
> > +					     IMX214_EXPOSURE_STEP,
> > +					    
> > IMX214_EXPOSURE_DEFAULT);
> > +
> > +	imx214->unit_size = v4l2_ctrl_new_std_compound(ctrl_hdlr,
> > +				NULL,
> > +				V4L2_CID_UNIT_CELL_SIZE,
> > +				v4l2_ctrl_ptr_create((void
> > *)&unit_size));
> > +
> > +	ret = ctrl_hdlr->error;
> > +	if (ret) {
> > +		v4l2_ctrl_handler_free(ctrl_hdlr);
> > +		return dev_err_probe(imx214->dev, ret, "failed to
> > add controls\n");
> 
> dev_err_probe won't help I think, or could ctrl_hdr->error be
> -EPROBE_DEFER ? Not a big deal though!

dev_err_probe() is used by imx415 (the latest added imx* driver).
That's why I used it, too.

André
> 
> All minor comments, with these addressed
> Reviewed-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> 
> Thanks
>   j
>