Re: [PATCH v3] adv7180: add more subdev video ops

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

Hello.

On 21-05-2013 13:35, Hans Verkuil wrote:

>> From: Vladimir Barinov <vladimir.barinov@cogentembedded.com>

>> Add subdev video ops for ADV7180 video decoder.  This makes decoder usable on
>> the soc-camera drivers.

>> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

>> ---
>> This patch is against the 'media_tree.git' repo.

>> Changes from version 2:
>> - set the field format depending on video standard in try_mbus_fmt() method;
>> - removed querystd() method calls from try_mbus_fmt() and cropcap() methods;
>> - removed g_crop() method.

>>   drivers/media/i2c/adv7180.c |   86 ++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 86 insertions(+)
>>
>> Index: media_tree/drivers/media/i2c/adv7180.c
>> =================================>> --- media_tree.orig/drivers/media/i2c/adv7180.c
>> +++ media_tree/drivers/media/i2c/adv7180.c


>> +
>> +static int adv7180_try_mbus_fmt(struct v4l2_subdev *sd,
>> +				struct v4l2_mbus_framefmt *fmt)
>> +{
>> +	struct adv7180_state *state = to_state(sd);
>> +
>> +	fmt->code = V4L2_MBUS_FMT_YUYV8_2X8;
>> +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
>> +	fmt->field = state->curr_norm & V4L2_STD_525_60 ?
>> +		     V4L2_FIELD_INTERLACED_BT : V4L2_FIELD_INTERLACED_TB;

> Just noticed this: use V4L2_FIELD_INTERLACED as that does the right thing.
> No need to split in _BT and _TB.

    Hm, testers have reported that _BT vs _TB do make a difference. I'll 
try to look into how V4L2 handles interlacing for different standards.

>> +	fmt->width = 720;
>> +	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
>> +
>> +	return 0;
>> +}

> Actually, all this code can be simplified substantially: the try/g/s_mbus_fmt
> functions are really all identical since the data returned is only dependent
> on the current standard. So this means you can use just a single function for
> all three ops, and you can do away with adding struct v4l2_mbus_framefmt to
> adv7180_state.

    Hm, I wonder how "get" and "set" methods can be identical... I'll 
look into this some more.

> Regards,

> 	Hans

WBR, Sergei