Re: [v4l-utils PATCH v1.2 2/2] media-ctl: Print information related to a single entity

[Sakari Ailus <sakari.ailus@linux.intel.com>]

On 09/15/16 01:05, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Wednesday 14 Sep 2016 17:29:39 Sakari Ailus wrote:
>> Add a possibility to printing all information related to a given entity by
>> using both -p and -e options. This may be handy sometimes if only a single
>> entity is of interest and there are many entities.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>> Fixed the commit message as well.
>>
>>  utils/media-ctl/media-ctl.c | 33 +++++++++++++++------------------
>>  utils/media-ctl/options.c   |  2 ++
>>  2 files changed, 17 insertions(+), 18 deletions(-)
>>
>> diff --git a/utils/media-ctl/media-ctl.c b/utils/media-ctl/media-ctl.c
>> index 0499008..109cc11 100644
>> --- a/utils/media-ctl/media-ctl.c
>> +++ b/utils/media-ctl/media-ctl.c
>> @@ -504,19 +504,11 @@ static void media_print_topology_text(struct
>> media_device *media) media, media_get_entity(media, i));
>>  }
>>
>> -void media_print_topology(struct media_device *media, int dot)
>> -{
>> -	if (dot)
>> -		media_print_topology_dot(media);
>> -	else
>> -		media_print_topology_text(media);
>> -}
>> -
>>  int main(int argc, char **argv)
>>  {
>>  	struct media_device *media;
>> +	struct media_entity *entity = NULL;
>>  	int ret = -1;
>> -	const char *devname;
>>
>>  	if (parse_cmdline(argc, argv))
>>  		return EXIT_FAILURE;
>> @@ -562,17 +554,11 @@ int main(int argc, char **argv)
>>  	}
>>
>>  	if (media_opts.entity) {
>> -		struct media_entity *entity;
>> -
>>  		entity = media_get_entity_by_name(media, media_opts.entity);
>>  		if (entity == NULL) {
>>  			printf("Entity '%s' not found\n", media_opts.entity);
>>  			goto out;
>>  		}
>> -
>> -		devname = media_entity_get_devname(entity);
>> -		if (devname)
>> -			printf("%s\n", devname);
>>  	}
>>
>>  	if (media_opts.fmt_pad) {
>> @@ -611,9 +597,20 @@ int main(int argc, char **argv)
>>  		}
>>  	}
>>
>> -	if (media_opts.print || media_opts.print_dot) {
>> -		media_print_topology(media, media_opts.print_dot);
>> -		printf("\n");
>> +	if (media_opts.print_dot) {
>> +		media_print_topology_dot(media);
>> +	} else if (media_opts.print) {
>> +		if (entity) {
>> +			media_print_topology_text_entity(media, entity);
>> +		} else {
>> +			media_print_topology_text(media);
>> +		}
> 
> You could remove the curly braces here.

Will fix.

> 
>> +	} else if (entity) {
>> +		const char *devname;
>> +
>> +		devname = media_entity_get_devname(entity);
>> +		if (devname)
>> +			printf("%s\n", devname);
>>  	}
>>
>>  	if (media_opts.reset) {
>> diff --git a/utils/media-ctl/options.c b/utils/media-ctl/options.c
>> index a288a1b..304a86c 100644
>> --- a/utils/media-ctl/options.c
>> +++ b/utils/media-ctl/options.c
>> @@ -52,6 +52,8 @@ static void usage(const char *argv0)
>>  	printf("-l, --links links	Comma-separated list of link 
> descriptors to
>> setup\n"); printf("    --known-mbus-fmts	List known media bus formats 
> and
>> their numeric values\n"); printf("-p, --print-topology	Print the 
> device
>> topology\n");
>> +	printf("			If entity name is specified using -e 
> option, information\n");
>> +	printf("			related to that entity only is 
> printed.\n");
> 
> Nitpicking, was anything wrong with
> 
> printf("-p, --print-topology    Print the device topology. If an entity\n");
> printf("                        is specified through the -e option, print\n");
> printf("                        information for that entity only.\n);

Not necessarily, but I missed that part of your reply. I'll replace it.

> 
> ? I think the help text looks more natural when using articles :-)

The message was still clear, wasn't it? :-D

> 
>>  	printf("    --print-dot		Print the device topology as a dot 
> graph\n");
>>  	printf("-r, --reset		Reset all links to inactive\n");
>>  	printf("-v, --verbose		Be verbose\n");
> 


-- 
Cheers,

Sakari Ailus
sakari.ailus@linux.intel.com