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
>
