Hi Laurent,
On Wed, Feb 20, 2019 at 02:51:19PM +0200, Laurent Pinchart wrote:
> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> yavta.c | 154 ++++++++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 115 insertions(+), 39 deletions(-)
>
> diff --git a/yavta.c b/yavta.c
> index eb50d592736f..6428c22f88d7 100644
> --- a/yavta.c
> +++ b/yavta.c
> @@ -529,6 +529,7 @@ static int get_control(struct device *dev,
> struct v4l2_ext_control *ctrl)
> {
> struct v4l2_ext_controls ctrls;
> + struct v4l2_control old;
> int ret;
>
> memset(&ctrls, 0, sizeof(ctrls));
> @@ -540,34 +541,32 @@ static int get_control(struct device *dev,
>
> ctrl->id = query->id;
>
> - if (query->type == V4L2_CTRL_TYPE_STRING) {
> - ctrl->string = malloc(query->maximum + 1);
> - if (ctrl->string == NULL)
> + if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
This breaks string controls for kernels that don't have
V4L2_CTRL_FLAG_HAS_PAYLOAD. As you still support kernels that have no
V4L2_CTRL_FLAG_NEXT_CTRL, how about checking for string type specifically?
> + ctrl->size = query->elems * query->elem_size;
> + ctrl->ptr = malloc(ctrl->size);
> + if (ctrl->ptr == NULL)
> return -ENOMEM;
> -
> - ctrl->size = query->maximum + 1;
> }
>
> ret = ioctl(dev->fd, VIDIOC_G_EXT_CTRLS, &ctrls);
> if (ret != -1)
> return 0;
>
> - if (query->type != V4L2_CTRL_TYPE_INTEGER64 &&
> - query->type != V4L2_CTRL_TYPE_STRING &&
> - (errno == EINVAL || errno == ENOTTY)) {
> - struct v4l2_control old;
> + if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD)
> + free(ctrl->ptr);
>
> - old.id = query->id;
> - ret = ioctl(dev->fd, VIDIOC_G_CTRL, &old);
> - if (ret != -1) {
> - ctrl->value = old.value;
> - return 0;
> - }
> - }
> + if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD ||
> + query->type == V4L2_CTRL_TYPE_INTEGER64 ||
> + (errno != EINVAL && errno != ENOTTY))
> + return -errno;
>
> - printf("unable to get control 0x%8.8x: %s (%d).\n",
> - query->id, strerror(errno), errno);
> - return -1;
> + old.id = query->id;
> + ret = ioctl(dev->fd, VIDIOC_G_CTRL, &old);
> + if (ret < 0)
> + return -errno;
> +
> + ctrl->value = old.value;
> + return 0;
> }
>
> static void set_control(struct device *dev, unsigned int id,
> @@ -1170,7 +1169,7 @@ static int video_for_each_control(struct device *dev,
> #else
> id = 0;
> while (1) {
> - id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> + id |= V4L2_CTRL_FLAG_NEXT_CTRL | V4L2_CTRL_FLAG_NEXT_COMPOUND;
> #endif
>
> ret = query_control(dev, id, &query);
> @@ -1215,13 +1214,76 @@ static void video_query_menu(struct device *dev,
> };
> }
>
> +static void video_print_control_array(const struct v4l2_query_ext_ctrl *query,
> + struct v4l2_ext_control *ctrl)
> +{
> + unsigned int i;
> +
> + printf("{");
A space would be nice after the opening brace, also before the closing one.
> +
> + for (i = 0; i < query->elems; ++i) {
> + switch (query->type) {
> + case V4L2_CTRL_TYPE_U8:
> + printf("%u", ctrl->p_u8[i]);
> + break;
> + case V4L2_CTRL_TYPE_U16:
> + printf("%u", ctrl->p_u16[i]);
> + break;
> + case V4L2_CTRL_TYPE_U32:
> + printf("%u", ctrl->p_u32[i]);
> + break;
> + }
> +
> + if (i != query->elems - 1)
> + printf(", ");
> + }
> +
> + printf("}");
> +}
> +
> +static void video_print_control_value(const struct v4l2_query_ext_ctrl *query,
> + struct v4l2_ext_control *ctrl)
> +{
> + if (query->nr_of_dims == 0) {
> + switch (query->type) {
> + case V4L2_CTRL_TYPE_INTEGER:
> + case V4L2_CTRL_TYPE_BOOLEAN:
> + case V4L2_CTRL_TYPE_MENU:
> + case V4L2_CTRL_TYPE_INTEGER_MENU:
> + printf("%d", ctrl->value);
> + break;
> + case V4L2_CTRL_TYPE_BITMASK:
> + printf("0x%08x", ctrl->value);
A cast to unsigned here?
> + break;
> + case V4L2_CTRL_TYPE_INTEGER64:
> + printf("%lld", ctrl->value64);
> + break;
> + case V4L2_CTRL_TYPE_STRING:
> + printf("%s", ctrl->string);
> + break;
> + }
> +
> + return;
> + }
> +
> + switch (query->type) {
> + case V4L2_CTRL_TYPE_U8:
> + case V4L2_CTRL_TYPE_U16:
> + case V4L2_CTRL_TYPE_U32:
> + video_print_control_array(query, ctrl);
> + break;
> + default:
> + printf("unsupported");
How about printing the unsupported type?
> + break;
> + }
> +}
> +
> static int video_print_control(struct device *dev,
> const struct v4l2_query_ext_ctrl *query,
> bool full)
> {
> struct v4l2_ext_control ctrl;
> - char sval[24];
> - char *current = sval;
> + unsigned int i;
> int ret;
>
> if (query->flags & V4L2_CTRL_FLAG_DISABLED)
> @@ -1232,25 +1294,39 @@ static int video_print_control(struct device *dev,
> return 0;
> }
>
> - ret = get_control(dev, query, &ctrl);
> - if (ret < 0)
> - strcpy(sval, "n/a");
> - else if (query->type == V4L2_CTRL_TYPE_INTEGER64)
> - sprintf(sval, "%lld", ctrl.value64);
> - 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 %lld max %lld step %lld default %lld current %s.\n",
> + if (full) {
> + printf("control 0x%08x `%s' min %lld max %lld step %lld default %lld ",
> 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);
> + query->step, query->default_value);
> + if (query->nr_of_dims) {
> + for (i = 0; i < query->nr_of_dims; ++i)
> + printf("[%u]", query->dims[i]);
> + printf(" ");
> + }
> + } else {
> + printf("control 0x%08x ", query->id);
> + }
>
> - if (query->type == V4L2_CTRL_TYPE_STRING)
> - free(ctrl.string);
> + if (query->type == V4L2_CTRL_TYPE_BUTTON) {
> + /* Button controls have no current value. */
> + printf("\n");
> + return 1;
> + }
> +
> + printf("current ");
> +
> + ret = get_control(dev, query, &ctrl);
> + if (ret < 0) {
> + printf("n/a\n");
> + printf("unable to get control 0x%8.8x: %s (%d).\n",
> + query->id, strerror(errno), errno);
> + } else {
> + video_print_control_value(query, &ctrl);
> + printf("\n");
> + }
> +
> + if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD)
> + free(ctrl.ptr);
>
> if (!full)
> return 1;
--
Regards,
Sakari Ailus
