Re: [PATCHv2] commands: add of_display_timings

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

On Mon, Sep 29, 2014 at 02:50:11PM +0200, Teresa Gamez wrote:
> Hello Sascha,
> 
> Am Montag, den 29.09.2014, 08:05 +0200 schrieb Sascha Hauer:
> > On Fri, Sep 26, 2014 at 01:22:31PM +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.
> > > 
> > > Signed-off-by: Teresa Gámez <t.gamez@phytec.de>
> > > ---
> > > Changes v2:
> > > - removed cast
> > > - added free(ftd)
> > > - freed root
> > > 
> 
> ...
> > > +
> > > +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;
> > > +		case 's':
> > > +			selected = 1;
> > > +			break;
> > > +		case 'S':
> > > +			timingpath = xzalloc(strlen(optarg) + 1);
> > > +			strcpy(timingpath, optarg);
> > > +			break;
> > 
> > You're right, there's no place to free this string.
> > 
> > BTW we have xstrdup for duplicating strings.
> > 
> > 
> > > +		/*
> > > +		 * We set the external node to our internal node as
> > > +		 * long as the command runs. This makes life easier to
> > > +		 * use of tree functions. We backup the internal root node
> > > +		 * and set it back at the end.
> > > +		 */
> > > +		internal_root = of_get_root_node();
> > > +		of_set_root_node(NULL);
> > > +		of_set_root_node(root);
> > 
> > Since the only device tree function you are using is
> > for_each_node_by_name(), could you add a:
> > 
> > #define for_each_node_by_name_from(dn, root, name) \
> > 	for (dn = of_find_node_by_name(root, name); dn; \
> > 	     dn = of_find_node_by_name(dn, name))
> > 
> > to include/of.h and remove the temporary overwriting of the
> > internal device tree?
> 
> I'm also using of_parse_phandle() which calls of_find_node_by_phandle().
> And this is using the root_node again.
> 
> I can create *_from() functions for both of them as overwriting the root
> node is really not pretty..

Yes, I think it's better to create functions that can be passed a root
node. It's probably not the last time we need to modify an external
device tree. Overwriting the root node maybe has funny side effects in
the future.

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