Hi Laurent
On Thu, Oct 06, 2022 at 07:33:45PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Oct 05, 2022 at 09:06:04PM +0200, Jacopo Mondi wrote:
> > Implement the enum_frame_size pad operation.
> >
> > The sensor supports a continuous size range of resolutions.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
> > ---
> > drivers/media/i2c/ar0521.c | 19 +++++++++++++++++++
> > 1 file changed, 19 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> > index c7bdfc69b9be..89f3c01f18ce 100644
> > --- a/drivers/media/i2c/ar0521.c
> > +++ b/drivers/media/i2c/ar0521.c
> > @@ -798,6 +798,24 @@ static int ar0521_enum_mbus_code(struct v4l2_subdev *sd,
> > return 0;
> > }
> >
> > +static int ar0521_enum_frame_size(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *sd_state,
> > + struct v4l2_subdev_frame_size_enum *fse)
> > +{
> > + if (fse->index)
> > + return -EINVAL;
> > +
> > + if (fse->code != MEDIA_BUS_FMT_SGRBG8_1X8)
> > + return -EINVAL;
> > +
> > + fse->min_width = AR0521_WIDTH_MIN;
> > + fse->max_width = AR0521_WIDTH_MAX;
> > + fse->min_height = AR0521_HEIGHT_MIN;
> > + fse->max_height = AR0521_HEIGHT_MAX;
>
> This matches the driver implementation of .set_fmt(), but that's because
> the driver is *really* wrong :-( It uses the format to configure the
> crop rectangle, which is not right. This needs to be fixed.
>
As far as I understand it, the driver supports smaller resolutions by
cropping only, while the sensor would be actually capable of binning.
As the driver currently only crops, the analog rectangle always matches the
output size hence adding s_selection(CROP) just to replicate what
s_ftm() does feels a little dumb ?
I concur that ideally the driver should be capable of producing
smaller resolution by binning, and in that case being able to
configure the analog crop rectangle would be useful. But as long as it
doesn't...
> > +
> > + return 0;
> > +}
> > +
> > static int ar0521_pre_streamon(struct v4l2_subdev *sd, u32 flags)
> > {
> > struct ar0521_dev *sensor = to_ar0521_dev(sd);
> > @@ -864,6 +882,7 @@ static const struct v4l2_subdev_video_ops ar0521_video_ops = {
> >
> > static const struct v4l2_subdev_pad_ops ar0521_pad_ops = {
> > .enum_mbus_code = ar0521_enum_mbus_code,
> > + .enum_frame_size = ar0521_enum_frame_size,
> > .get_fmt = ar0521_get_fmt,
> > .set_fmt = ar0521_set_fmt,
> > };
>
> --
> Regards,
>
> Laurent Pinchart
