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

[Sascha Hauer <s.hauer@pengutronix.de>]

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.

> +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.

> +		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.

> +			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


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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