Quoting Sakari Ailus (2023-10-10 07:14:09)
> Hi Kieran,
>
> On Tue, Oct 10, 2023 at 01:51:24AM +0100, Kieran Bingham wrote:
> > Support reporting of the Sensor Native and Active pixel array areas
> > through the Selection API.
> >
> > The implementation reports a single target crop only for the mode that
> > is presently exposed by the driver.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
>
> Shouldn't you use the same callback for .set_selection? I guess this is
> somewhat grey area but doing so would be in line with how V4L2 API works in
> general.
Hrm ... I didn't think it was needed as it's not possible to /set/
anything.
I expect to change this once I add support for setting crops later
though. It was going to be something I'd add when it is used.
Only the 'get_selection' call is necessary to make this camera operate
on both i.MX8MP and RPi5 platforms with libcamera, so that's what I've
done so far. My goal of this series was to bring the existing driver up
to a point that it can be used, before I start making new feature
additions.
--
Kieran
>
> Cc Laurent.
>
> > ---
> > drivers/media/i2c/imx335.c | 44 ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 44 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> > index bf12b9b69fce..026777eb362e 100644
> > --- a/drivers/media/i2c/imx335.c
> > +++ b/drivers/media/i2c/imx335.c
> > @@ -55,6 +55,14 @@
> > #define IMX335_REG_MIN 0x00
> > #define IMX335_REG_MAX 0xfffff
> >
> > +/* IMX335 native and active pixel array size. */
> > +#define IMX335_NATIVE_WIDTH 2616U
> > +#define IMX335_NATIVE_HEIGHT 1964U
> > +#define IMX335_PIXEL_ARRAY_LEFT 12U
> > +#define IMX335_PIXEL_ARRAY_TOP 12U
> > +#define IMX335_PIXEL_ARRAY_WIDTH 2592U
> > +#define IMX335_PIXEL_ARRAY_HEIGHT 1944U
> > +
> > /**
> > * struct imx335_reg - imx335 sensor register
> > * @address: Register address
> > @@ -651,6 +659,41 @@ static int imx335_init_pad_cfg(struct v4l2_subdev *sd,
> > return imx335_set_pad_format(sd, sd_state, &fmt);
> > }
> >
> > +/**
> > + * imx335_get_selection() - Selection API
> > + * @sd: pointer to imx335 V4L2 sub-device structure
> > + * @sd_state: V4L2 sub-device configuration
> > + * @sel: V4L2 selection info
> > + *
> > + * Return: 0 if successful, error code otherwise.
> > + */
> > +static int imx335_get_selection(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *sd_state,
> > + struct v4l2_subdev_selection *sel)
> > +{
> > + switch (sel->target) {
> > + case V4L2_SEL_TGT_NATIVE_SIZE:
> > + sel->r.top = 0;
> > + sel->r.left = 0;
> > + sel->r.width = IMX335_NATIVE_WIDTH;
> > + sel->r.height = IMX335_NATIVE_HEIGHT;
> > +
> > + return 0;
> > +
> > + case V4L2_SEL_TGT_CROP:
> > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > + sel->r.top = IMX335_PIXEL_ARRAY_TOP;
> > + sel->r.left = IMX335_PIXEL_ARRAY_LEFT;
> > + sel->r.width = IMX335_PIXEL_ARRAY_WIDTH;
> > + sel->r.height = IMX335_PIXEL_ARRAY_HEIGHT;
> > +
> > + return 0;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > /**
> > * imx335_start_streaming() - Start sensor stream
> > * @imx335: pointer to imx335 device
> > @@ -864,6 +907,7 @@ static const struct v4l2_subdev_pad_ops imx335_pad_ops = {
> > .init_cfg = imx335_init_pad_cfg,
> > .enum_mbus_code = imx335_enum_mbus_code,
> > .enum_frame_size = imx335_enum_frame_size,
> > + .get_selection = imx335_get_selection,
> > .get_fmt = imx335_get_pad_format,
> > .set_fmt = imx335_set_pad_format,
> > };
>
> --
> Regards,
>
> Sakari Ailus
