Re: [PATCH] rcar_vin: propagate querystd() error upstream

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

Hi Sergei,

On Tuesday 25 August 2015 00:58:07 Sergei Shtylyov wrote:
> On 08/21/2015 12:51 AM, Laurent Pinchart wrote:
> >> rcar_vin_set_fmt() defaults to  PAL when the subdevice's querystd()
> >> method call fails (e.g. due to I2C error). This doesn't work very well
> >> when a camera being used  outputs NTSC which has different order of
> >> fields and resolution. Let us stop pretending and return the actual
> >> error (which would prevent video capture on at least Renesas
> >> Henninger/Porter board where I2C seems particularly buggy).
> >> 
> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >> 
> >> ---
> >> The patch is against the 'media_tree.git' repo's 'fixes' branch.
> >> 
> >>   drivers/media/platform/soc_camera/rcar_vin.c |    2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> Index: media_tree/drivers/media/platform/soc_camera/rcar_vin.c
> >> ===================================================================
> >> --- media_tree.orig/drivers/media/platform/soc_camera/rcar_vin.c
> >> +++ media_tree/drivers/media/platform/soc_camera/rcar_vin.c
> >> @@ -1592,7 +1592,7 @@ static int rcar_vin_set_fmt(struct soc_c
> >> 
> >>   		/* Query for standard if not explicitly mentioned _TB/_BT */
> >>   		ret = v4l2_subdev_call(sd, video, querystd, &std);
> >>   		if (ret < 0)
> >> -			std = V4L2_STD_625_50;
> >> +			return ret;
> > 
> > What if the subdev doesn't implement querystd ? That's the case of camera
> > sensors for instance.
> 
> Indeed.
> 
> > In that case we should default to V4L2_FIELD_NONE.
> 
> Hmm, even if the set_fmt() method is called with V4L2_FIELD_INTERLACED
> already, like in this case?

Yes. If the device doesn't support interlacing then the field passed to 
set_fmt() should be set to V4L2_FIELD_NONE. The V4L2 API requires drivers to 
fix unsupported parameters passed to the VIDIOC_S_FMT ioctl instead of 
returning an error like commonly done in such situation.

> >>   		field = std & V4L2_STD_625_50 ? V4L2_FIELD_INTERLACED_TB :
> >>   						V4L2_FIELD_INTERLACED_BT;

-- 
Regards,

Laurent Pinchart