Hi Laurent and Jacopo.
On Fri, 7 Oct 2022 at 09:11, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Jacopo,
>
> On Fri, Oct 07, 2022 at 09:29:59AM +0200, Jacopo Mondi wrote:
> > On Thu, Oct 06, 2022 at 07:33:45PM +0300, Laurent Pinchart wrote:
> > > 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...
>
> We have enough issues with sensor drivers implementing binning or
> skipping in different ways to not make it worse by implementing cropping
> in a creative way as wall :-)
>
> The first step is to fix the driver to implement the selection API. Then
> binning and skipping can be implemented on top, if anyone becomes
> interested in them.
The datasheet and register reference have a fair number of references
to SMIA standards. I wonder if the CCS driver can take over from this
driver entirely....
Dave
> > > > +
> > > > + 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
