Re: [PATCH yavta 1/7] yavta: Refactor video_list_controls()

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

Hi Sakari,

On Wed, Feb 20, 2019 at 03:21:57PM +0200, Sakari Ailus wrote:
> On Wed, Feb 20, 2019 at 02:51:17PM +0200, Laurent Pinchart wrote:
> > Separate iteration over controls from printing, in order to reuse the
> > iteration to implement control reset.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  yavta.c | 133 ++++++++++++++++++++++++++++++++++----------------------
> >  1 file changed, 82 insertions(+), 51 deletions(-)
> > 
> > diff --git a/yavta.c b/yavta.c
> > index 2d3b2d096f7d..98bc09810ff1 100644
> > --- a/yavta.c
> > +++ b/yavta.c
> > @@ -484,9 +484,12 @@ static int query_control(struct device *dev, unsigned int id,
> >  	query->id = id;
> >  
> >  	ret = ioctl(dev->fd, VIDIOC_QUERYCTRL, query);
> > -	if (ret < 0 && errno != EINVAL)
> > -		printf("unable to query control 0x%8.8x: %s (%d).\n",
> > -		       id, strerror(errno), errno);
> > +	if (ret < 0) {
> > +		ret = -errno;
> > +		if (ret != -EINVAL)
> > +			printf("unable to query control 0x%8.8x: %s (%d).\n",
> > +			       id, strerror(errno), errno);
> > +	}
> >  
> >  	return ret;
> >  }
> > @@ -1120,7 +1123,45 @@ static int video_enable(struct device *dev, int enable)
> >  	return 0;
> >  }
> >  
> > -static void video_query_menu(struct device *dev, struct v4l2_queryctrl *query,
> > +static int video_for_each_control(struct device *dev,
> > +				  int(*callback)(struct device *dev, const struct v4l2_queryctrl *query))
> 
> This is over 80 characters per line. How about wrapping? Also int on the
> above line is desperate for some breathing room before the opening
> parenthesis.

One option would be to turn the callback into a typedef.

I'm thinking about doing some refactoring in yavta, possibly splitting
it in multiple source files, as it's getting quite big. Control handling
is a candidate to be moved to a separate file. What do you think ?

I'm also wondering whether I should enumerate controls when opening the
device and caching them, to operate on the cache later on.

> > +{
> > +	struct v4l2_queryctrl query;
> > +	unsigned int nctrls = 0;
> > +	unsigned int id;
> > +	int ret;
> > +
> > +#ifndef V4L2_CTRL_FLAG_NEXT_CTRL
> 
> This was added back in 2012. Do you think it's still worth checking for it?
> 
> Not related to this patch though, just a general remark.

Please see patch 7/7 :-)

> > +	unsigned int i;
> > +
> > +	for (i = V4L2_CID_BASE; i <= V4L2_CID_LASTP1; ++i) {
> > +		id = i;
> > +#else
> > +	id = 0;
> > +	while (1) {
> > +		id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> > +#endif
> > +
> > +		ret = query_control(dev, id, &query);
> > +		if (ret == -EINVAL)
> > +			break;
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		id = query.id;
> > +
> > +		ret = callback(dev, &query);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		if (ret)
> > +			nctrls++;
> > +	}
> > +
> > +	return nctrls;
> > +}
> > +
> > +static void video_query_menu(struct device *dev, const struct v4l2_queryctrl *query,
> >  			     unsigned int value)
> >  {
> >  	struct v4l2_querymenu menu;
> > @@ -1142,83 +1183,68 @@ static void video_query_menu(struct device *dev, struct v4l2_queryctrl *query,
> >  	};
> >  }
> >  
> > -static int video_print_control(struct device *dev, unsigned int id, bool full)
> > +static int video_print_control(struct device *dev,
> > +			       const struct v4l2_queryctrl *query, bool full)
> >  {
> >  	struct v4l2_ext_control ctrl;
> > -	struct v4l2_queryctrl query;
> >  	char sval[24];
> >  	char *current = sval;
> >  	int ret;
> >  
> > -	ret = query_control(dev, id, &query);
> > -	if (ret < 0)
> > -		return ret;
> > +	if (query->flags & V4L2_CTRL_FLAG_DISABLED)
> > +		return 0;
> >  
> > -	if (query.flags & V4L2_CTRL_FLAG_DISABLED)
> > -		return query.id;
> > -
> > -	if (query.type == V4L2_CTRL_TYPE_CTRL_CLASS) {
> > -		printf("--- %s (class 0x%08x) ---\n", query.name, query.id);
> > -		return query.id;
> > +	if (query->type == V4L2_CTRL_TYPE_CTRL_CLASS) {
> > +		printf("--- %s (class 0x%08x) ---\n", query->name, query->id);
> > +		return 0;
> >  	}
> >  
> > -	ret = get_control(dev, &query, &ctrl);
> > +	ret = get_control(dev, query, &ctrl);
> >  	if (ret < 0)
> >  		strcpy(sval, "n/a");
> > -	else if (query.type == V4L2_CTRL_TYPE_INTEGER64)
> > +	else if (query->type == V4L2_CTRL_TYPE_INTEGER64)
> >  		sprintf(sval, "%lld", ctrl.value64);
> > -	else if (query.type == V4L2_CTRL_TYPE_STRING)
> > +	else if (query->type == V4L2_CTRL_TYPE_STRING)
> >  		current = ctrl.string;
> >  	else
> >  		sprintf(sval, "%d", ctrl.value);
> >  
> >  	if (full)
> >  		printf("control 0x%08x `%s' min %d max %d step %d default %d current %s.\n",
> > -			query.id, query.name, query.minimum, query.maximum,
> > -			query.step, query.default_value, current);
> > +			query->id, query->name, query->minimum, query->maximum,
> > +			query->step, query->default_value, current);
> >  	else
> > -		printf("control 0x%08x current %s.\n", query.id, current);
> > +		printf("control 0x%08x current %s.\n", query->id, current);
> >  
> > -	if (query.type == V4L2_CTRL_TYPE_STRING)
> > +	if (query->type == V4L2_CTRL_TYPE_STRING)
> >  		free(ctrl.string);
> >  
> >  	if (!full)
> > -		return query.id;
> > +		return 1;
> >  
> > -	if (query.type == V4L2_CTRL_TYPE_MENU ||
> > -	    query.type == V4L2_CTRL_TYPE_INTEGER_MENU)
> > -		video_query_menu(dev, &query, ctrl.value);
> > +	if (query->type == V4L2_CTRL_TYPE_MENU ||
> > +	    query->type == V4L2_CTRL_TYPE_INTEGER_MENU)
> > +		video_query_menu(dev, query, ctrl.value);
> >  
> > -	return query.id;
> > +	return 1;
> > +}
> > +
> > +static int __video_print_control(struct device *dev,
> > +				 const struct v4l2_queryctrl *query)
> > +{
> > +	return video_print_control(dev, query, true);
> >  }
> >  
> >  static void video_list_controls(struct device *dev)
> >  {
> > -	unsigned int nctrls = 0;
> > -	unsigned int id;
> >  	int ret;
> >  
> > -#ifndef V4L2_CTRL_FLAG_NEXT_CTRL
> > -	unsigned int i;
> > +	ret = video_for_each_control(dev, __video_print_control);
> > +	if (ret < 0)
> > +		return;
> >  
> > -	for (i = V4L2_CID_BASE; i <= V4L2_CID_LASTP1; ++i) {
> > -		id = i;
> > -#else
> > -	id = 0;
> > -	while (1) {
> > -		id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> > -#endif
> > -
> > -		ret = video_print_control(dev, id, true);
> > -		if (ret < 0)
> > -			break;
> > -
> > -		id = ret;
> > -		nctrls++;
> > -	}
> > -
> > -	if (nctrls)
> > -		printf("%u control%s found.\n", nctrls, nctrls > 1 ? "s" : "");
> > +	if (ret)
> > +		printf("%u control%s found.\n", ret, ret > 1 ? "s" : "");
> >  	else
> >  		printf("No control found.\n");
> >  }
> > @@ -2184,8 +2210,13 @@ int main(int argc, char *argv[])
> >  	if (do_log_status)
> >  		video_log_status(&dev);
> >  
> > -	if (do_get_control)
> > -		video_print_control(&dev, ctrl_name, false);
> > +	if (do_get_control) {
> > +		struct v4l2_queryctrl query;
> > +
> > +		ret = query_control(&dev, ctrl_name, &query);
> > +		if (ret == 0)
> > +			video_print_control(&dev, &query, false);
> > +	}
> >  
> >  	if (do_set_control)
> >  		set_control(&dev, ctrl_name, ctrl_value);

-- 
Regards,

Laurent Pinchart