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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
