Re: [PATCH 3/3] media: imx214: Add new control with V4L2_CID_PIXEL_SIZE

[Jacopo Mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 3788 bytes --]

Hi Ricardo,

On Wed, Aug 21, 2019 at 06:31:05PM +0200, Ricardo Ribalda Delgado wrote:
> Hi Jacopo
>
>
> On Wed, Aug 21, 2019 at 6:14 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hi Ricardo,
> >
> > On Mon, Aug 19, 2019 at 02:17:20PM +0200, Ricardo Ribalda Delgado wrote:
> > > According to the product brief, the unit cell size is 1120 nanometers^2.
> >
> > Should this information come from DT ?
>
> I do not think so. You cannot change this value and it needs to be
> defined also in sensors/cameras that might not have a DT, like a usb
> webcam.

You're probably right. I got this thinking because the camera
location/orientation are two read only parameters that come from DT,
but their value depends on the design of the device where the camera
is installed on, so they're configurable, while this and other
physical properties are not, and it doesn't make much sense to have
them in DT.

Thanks
   j

>
> It would be like adding to the DT the min/max exposure time...
>
> But of course we can discuss it ;)
>
> Best regards
>
> >
> > I'm asking as I've a series in review that adds an helper that
> > collectes dt properties and register controls for them. It currently
> > only supports the newly proposed camera location control, but there
> > might be others like the rotation, for which we already have a DT
> > property.
> >
> > https://patchwork.kernel.org/project/linux-media/list/?series=160901
> >
> > This new one is indeed an HW property of the sensor, I wonder if
> > having it in the firmware interface would make any sense or not...
> >
> > Thanks
> >   j
> >
> > >
> > > https://www.sony-semicon.co.jp/products_en/IS/sensor1/img/products/ProductBrief_IMX214_20150428.pdf
> > >
> > > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> > > ---
> > >  drivers/media/i2c/imx214.c | 23 +++++++++++++++++++++++
> > >  1 file changed, 23 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > > index 159a3a604f0e..b2f6bd2d8d7d 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 *pixel_size;
> > >
> > >       struct regulator_bulk_data      supplies[IMX214_NUM_SUPPLIES];
> > >
> > > @@ -941,6 +942,26 @@ static int __maybe_unused imx214_resume(struct device *dev)
> > >       return ret;
> > >  }
> > >
> > > +static void pixel_size_init(const struct v4l2_ctrl *ctrl, u32 idx,
> > > +                  union v4l2_ctrl_ptr ptr)
> > > +{
> > > +     ptr.p_pixel_size->width = 1120;
> > > +     ptr.p_pixel_size->height = 1120;
> > > +}
> > > +
> > > +static const struct v4l2_ctrl_type_ops pixel_size_ops = {
> > > +     .init = pixel_size_init,
> > > +};
> > > +
> > > +static struct v4l2_ctrl *new_pixel_size_ctrl(struct v4l2_ctrl_handler *handler)
> > > +{
> > > +     static struct v4l2_ctrl_config ctrl = {
> > > +             .id = V4L2_CID_PIXEL_SIZE,
> > > +             .type_ops = &pixel_size_ops,
> > > +     };
> > > +
> > > +     return v4l2_ctrl_new_custom(handler, &ctrl, NULL);
> > > +}
> > >  static int imx214_probe(struct i2c_client *client)
> > >  {
> > >       struct device *dev = &client->dev;
> > > @@ -1029,6 +1050,8 @@ static int imx214_probe(struct i2c_client *client)
> > >                                            V4L2_CID_EXPOSURE,
> > >                                            0, 3184, 1, 0x0c70);
> > >
> > > +     imx214->pixel_size = new_pixel_size_ctrl(&imx214->ctrls);
> > > +
> > >       ret = imx214->ctrls.error;
> > >       if (ret) {
> > >               dev_err(&client->dev, "%s control init failed (%d)\n",
> > > --
> > > 2.23.0.rc1
> > >

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]