Re: [PATCH v6 7/7] media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE

[Ricardo Ribalda Delgado]

Hi Jacopo

Thanks for the review

On Wed, Sep 25, 2019 at 11:24 AM Jacopo Mondi <jacopo@xxxxxxxxxx> wrote:
>
> Hi Ricardo,
>
> On Fri, Sep 20, 2019 at 03:51:37PM +0200, Ricardo Ribalda Delgado wrote:
> > From: Ricardo Ribalda Delgado <ribalda@xxxxxxxxxx>
> >
> > According to the product brief, the unit cell size is 1120 nanometers^2.
> >
> > https://www.sony-semicon.co.jp/products_en/IS/sensor1/img/products/ProductBrief_IMX214_20150428.pdf
> >
> > Signed-off-by: Ricardo Ribalda Delgado <ribalda@xxxxxxxxxx>
> > ---
> >  drivers/media/i2c/imx214.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > index 159a3a604f0e..57562e20c4ca 100644
> > --- a/drivers/media/i2c/imx214.c
> > +++ b/drivers/media/i2c/imx214.c
> > @@ -47,6 +47,7 @@ struct imx214 {
> >       struct v4l2_ctrl *pixel_rate;
> >       struct v4l2_ctrl *link_freq;
> >       struct v4l2_ctrl *exposure;
> > +     struct v4l2_ctrl *unit_size;
> >
> >       struct regulator_bulk_data      supplies[IMX214_NUM_SUPPLIES];
> >
> > @@ -948,6 +949,13 @@ static int imx214_probe(struct i2c_client *client)
> >       static const s64 link_freq[] = {
> >               IMX214_DEFAULT_LINK_FREQ,
> >       };
> > +     struct v4l2_area unit_size = {
> > +             .width = 1120,
> > +             .height = 1120,
> > +     };
> > +     union v4l2_ctrl_ptr p_def = {
> > +             .p_area = &unit_size,
> > +     };
> >       int ret;
> >
> >       ret = imx214_parse_fwnode(dev);
> > @@ -1029,6 +1037,10 @@ static int imx214_probe(struct i2c_client *client)
> >                                            V4L2_CID_EXPOSURE,
> >                                            0, 3184, 1, 0x0c70);
> >
> > +     imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls,
> > +                                                    NULL,
> > +                                                    V4L2_CID_UNIT_CELL_SIZE,
> > +                                                    p_def);
> >       ret = imx214->ctrls.error;
> >       if (ret) {
> >               dev_err(&client->dev, "%s control init failed (%d)\n",
>
> Would something like this scale? I'm a bit bothered by the need of
> declaring v4l2_ctrl_ptr structure just to set a field there in
> drivers.

I have implemented the interface that Hans proposed on
https://www.mail-archive.com/linux-media@xxxxxxxxxxxxxxx/msg149901.html

I thik Hans prefers this way to avoid castings, which can end in a lot
of "here be dragoons" :)


>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 57562e20c4ca..a00d8fa481c2 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -953,9 +953,6 @@ static int imx214_probe(struct i2c_client *client)
>                 .width = 1120,
>                 .height = 1120,
>         };
> -       union v4l2_ctrl_ptr p_def = {
> -               .p_area = &unit_size,
> -       };
>         int ret;
>
>         ret = imx214_parse_fwnode(dev);
> @@ -1040,7 +1037,7 @@ static int imx214_probe(struct i2c_client *client)
>         imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls,
>                                                        NULL,
>                                                        V4L2_CID_UNIT_CELL_SIZE,
> -                                                      p_def);
> +                                                      &unit_size);
>         ret = imx214->ctrls.error;
>         if (ret) {
>                 dev_err(&client->dev, "%s control init failed (%d)\n",
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index f626f9983408..4a2648bee6f5 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -2681,18 +2681,26 @@ EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
>  /* Helper function for standard compound controls */
>  struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
>                                 const struct v4l2_ctrl_ops *ops, u32 id,
> -                               const union v4l2_ctrl_ptr p_def)
> +                               void *defval)
>  {
>         const char *name;
>         enum v4l2_ctrl_type type;
>         u32 flags;
>         s64 min, max, step, def;
> +       union v4l2_ctrl_ptr p_def;
>
>         v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
>         if (type < V4L2_CTRL_COMPOUND_TYPES) {
>                 handler_set_err(hdl, -EINVAL);
>                 return NULL;
>         }
> +
> +       switch ((u32)type) {
> +       case V4L2_CTRL_TYPE_AREA:
> +               p_def.p_area = defval;
> +               break;
> +       }
> +
>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
>                              min, max, step, def, NULL, 0,
>                              flags, NULL, NULL, p_def, NULL);
>
> However, due to my limitied understanding of the control framework, I
> cannot tell how many cases the newly introduced switch should
> handle...
>
> > --
> > 2.23.0
> >



-- 
Ricardo Ribalda