Re: [PATCH 2/2] commands: add of_display_timings

[Teresa Gamez <t.gamez@phytec.de>]

Hello Sascha,

Am Donnerstag, den 25.09.2014, 08:07 +0200 schrieb Sascha Hauer:
> Hi Teresa,
> 
> On Wed, Sep 24, 2014 at 11:33:11AM +0200, Teresa Gámez wrote:
> > A lot of boards use display-timings nodes to define the timings of one or more
> > displays. This command makes it possible to list and select displays which are
> > defined in a device tree.
> 
> Nice idea.
> 
> > +static int of_display_timing(struct device_node *root, void *timingpath)
> > +{
> > +	int ret = 0;
> > +	struct device_node *display;
> > +
> > +	display = of_find_node_by_path_from(root, (char *) timingpath);
> 
> Unnecessary cast.

Removed it.
> 
> > +static int do_of_display_timings(int argc, char *argv[])
> > +{
> > +	int opt;
> > +	int list = 0;
> > +	int selected = 0;
> > +	int ret = 0;
> > +	struct device_node *root = NULL;
> > +	struct device_node *internal_root = NULL;
> > +	struct device_node *display = NULL;
> > +	struct device_node *timings = NULL;
> > +	char *timingpath = NULL;
> > +	char *dtbfile = NULL;
> > +
> > +	while ((opt = getopt(argc, argv, "sS:lf:")) > 0) {
> > +		switch (opt) {
> > +		case 'l':
> > +			list = 1;
> > +			break;
> > +		case 'f':
> > +			dtbfile = optarg;
> > +			break;
> 
> Do we really need this option? It can only be used for showing display
> timings in an external dtb, not for manipulating it. If I plan to use
> the external tree I can still do a oftree -f / oftree -l dtb to exchange
> the internal tree.

Well, it just gives the same feature like of_dump has. It is not that 
necessary, but it makes it more handy. 

> 
> > +		case 's':
> > +			selected = 1;
> > +			break;
> > +		case 'S':
> > +			timingpath = xzalloc(strlen(optarg) + 1);
> > +			strcpy(timingpath, optarg);
> 
> You never free timingpath. But why are you making a copy of the string?
> This shouldn't be necessary.

I need to save the string for later usage in the fixup function. This is
called after the command has finished. I think the optarg pointer to
argv is not longer vaild then.

The fixup can be called multiple times (canceled boot, of_dump -F,..).
I don't see a save place to free timingpath.

Teresa

> 
> > +			break;
> > +		default:
> > +			return COMMAND_ERROR_USAGE;
> > +		}
> > +	}
> > +
> > +	/* Check if external dtb given */
> > +	if (dtbfile) {
> > +		void *fdt;
> > +		size_t size;
> > +
> > +		fdt = read_file(dtbfile, &size);
> > +		if (!fdt) {
> > +			pr_err("unable to read %s: %s\n", dtbfile,
> > +				strerror(errno));
> > +			return -errno;
> > +		}
> > +
> > +		if (file_detect_type(fdt, size) != filetype_oftree) {
> > +			pr_err("%s is not a oftree file.\n", dtbfile);
> > +			return -EINVAL;
> 
> Here you lose memory.
> 
> > +		}
> > +
> > +		root = of_unflatten_dtb(fdt);
> 
> This must be freed after use with of_delete_node().
> 
> Sascha
> 
> 



_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox