Re: [PATCH 3/3] media: sun6i-csi: implement vidioc_enum_framesizes

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Paul,

On Wed, Feb 22, 2023 at 11:43:06AM +0100, Paul Kocialkowski wrote:
> On Sat 07 Jan 23, 01:35, Laurent Pinchart wrote:
> > On Fri, Jan 06, 2023 at 07:40:38PM +0000, adam@piggz.co.uk wrote:
> > > From: Adam Pigg <adam@piggz.co.uk>
> > > 
> > > Create sun6i_csi_capture_enum_framesizes which defines the min
> > > and max frame sizes
> > 
> > With the commit message updated (see review of 1/3),
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> I'm always a bit confused regarding how such an ioctl's behavior should depend
> on the attached subdev. Is it well-defined behavior that this here is only
> about the receiver part and has nothing to do with what the connected sensor?

That's right. For MC-based drivers, enumerating pixel formats and frame
sizes on a video node should return the formats and sizes intrinsically
supported by the video node (in most cases, that maps to the DMA engine
at the hardware level) without considering connected subdevs.

> > > Signed-off-by: Adam Pigg <adam@piggz.co.uk>
> > > ---
> > >  .../sunxi/sun6i-csi/sun6i_csi_capture.c       | 24 +++++++++++++++++++
> > >  1 file changed, 24 insertions(+)
> > > 
> > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> > > index 54beba4d2b2f..2be761e6b650 100644
> > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> > > @@ -739,6 +739,29 @@ static int sun6i_csi_capture_try_fmt(struct file *file, void *private,
> > >  	return 0;
> > >  }
> > >  
> > > +static int sun6i_csi_capture_enum_framesizes(struct file *file, void *fh,
> > > +					  struct v4l2_frmsizeenum *fsize)
> 
> A cosmetic/consistency suggestion would be to name this variable "frmsize" to
> reuse part of the name of the structure, which is what I've done in other places
> of the driver.
> 
> > > +{
> > > +	const struct sun6i_csi_capture_format *format;
> > > +
> > > +	if (fsize->index > 0)
> > > +		return -EINVAL;
> > > +
> > > +	format = sun6i_csi_capture_format_find(fsize->pixel_format);
> > > +	if (!format)
> > > +		return -EINVAL;
> > > +
> > > +	fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
> > > +	fsize->stepwise.min_width = SUN6I_CSI_CAPTURE_WIDTH_MIN;
> > > +	fsize->stepwise.max_width = SUN6I_CSI_CAPTURE_WIDTH_MAX;
> > > +	fsize->stepwise.min_height = SUN6I_CSI_CAPTURE_HEIGHT_MIN;
> > > +	fsize->stepwise.max_height = SUN6I_CSI_CAPTURE_HEIGHT_MAX;
> > > +	fsize->stepwise.step_width = 1;
> > > +	fsize->stepwise.step_height = 1;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int sun6i_csi_capture_enum_input(struct file *file, void *private,
> > >  					struct v4l2_input *input)
> > >  {
> > > @@ -775,6 +798,7 @@ static const struct v4l2_ioctl_ops sun6i_csi_capture_ioctl_ops = {
> > >  	.vidioc_g_fmt_vid_cap		= sun6i_csi_capture_g_fmt,
> > >  	.vidioc_s_fmt_vid_cap		= sun6i_csi_capture_s_fmt,
> > >  	.vidioc_try_fmt_vid_cap		= sun6i_csi_capture_try_fmt,
> > > +	.vidioc_enum_framesizes		= sun6i_csi_capture_enum_framesizes,
> > >  
> > >  	.vidioc_enum_input		= sun6i_csi_capture_enum_input,
> > >  	.vidioc_g_input			= sun6i_csi_capture_g_input,

-- 
Regards,

Laurent Pinchart