Re: [PATCH 24/28] media: ti-vpe: cal: add mbus_code support to cal_mc_enum_fmt_vid_cap

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

On 18/04/2021 16:17, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Mon, Apr 12, 2021 at 02:34:53PM +0300, Tomi Valkeinen wrote:
>> Commit e5b6b07a1b45dd9d19bec1fa1d60750b0fcf2fb0 ("media: v4l2: Extend
> 
> You can abbreviate that to 12 characters if desired.
> 
>> VIDIOC_ENUM_FMT to support MC-centric devices") added support to
>> enumerate formats based in mbus-code.
>>
>> Add this feature to cal driver.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/media/platform/ti-vpe/cal-video.c | 21 ++++++++++++++++++---
>>   1 file changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/ti-vpe/cal-video.c b/drivers/media/platform/ti-vpe/cal-video.c
>> index ea9b13c16a06..1d9c0fce4b03 100644
>> --- a/drivers/media/platform/ti-vpe/cal-video.c
>> +++ b/drivers/media/platform/ti-vpe/cal-video.c
>> @@ -437,13 +437,28 @@ static const struct v4l2_ioctl_ops cal_ioctl_video_ops = {
>>   static int cal_mc_enum_fmt_vid_cap(struct file *file, void  *priv,
>>   				   struct v4l2_fmtdesc *f)
>>   {
>> +	unsigned int i;
>> +	unsigned int idx;
>> +
>>   	if (f->index >= cal_num_formats)
>>   		return -EINVAL;
>>   
>> -	f->pixelformat = cal_formats[f->index].fourcc;
>> -	f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> 
> As a shortcut, you could have
> 
> 	f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> 
> 	if (!f->mbus_code) {
> 		f->pixelformat = cal_formats[f->index].fourcc;
> 		return 0;
> 	}
> 
>> +	idx = 0;
>>   
>> -	return 0;
>> +	for (i = 0; i < cal_num_formats; ++i) {
>> +		if (f->mbus_code && cal_formats[i].code != f->mbus_code)
> 
> And drop the first condition here, as well as f->type below. Up to you.

True, but in a patch that adds metadata support (not posted) I add also 
metadata formats into the cal_formats array, and I need to iterate and 
skip the possible metadata-formats even for !f->mbus_code case.

I'm not sure if that approach is good or not (I also tried separate 
metadata format array but that just caused mess elsewhere), but for the 
time being I'd rather keep this as it is to easily allow adding the 
metadata formats.

  Tomi