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

[Paul Kocialkowski <paul.kocialkowski@bootlin.com>]

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

Hi,

On Sat 07 Jan 23, 01:35, Laurent Pinchart wrote:
> Hi Adam,
> 
> Thank you for the patch.
> 
> 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?

> > 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.

Cheers,

Paul

> > +{
> > +	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

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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