Hi Simon > On 15 September 2014 07:06, Christian Gmeiner <christian.gmei...@gmail.com> > wrote: >> >> This new function is used to set all display-timings >> properties based on fb_videomode. >> >> display-timings { >> timing0 { >> clock-frequency = <25000000>; >> hactive = <640>; >> vactive = <480>; >> hback-porch = <48>; >> hfront-porch = <16>; >> vback-porch = <31>; >> vfront-porch = <12>; >> hsync-len = <96>; >> vsync-len = <2>; >> }; >> }; >> >> Signed-off-by: Christian Gmeiner <christian.gmei...@gmail.com> >
The ot1200 base patch has landed in u-boot-imx and I will work to get this EDID stuff mainline too. > > Thanks for the patch. There are a few style violations and I have a few > minor comments below. > > $ ./tools/patman/patman -c1 -n > Cleaned 1 patches > 0 errors, 4 warnings, 0 checks for > 0001-fdt-add-fdt_add_display_timings-.-and-friends.patch: > warning: common/fdt_support.c,1400: line over 80 characters > warning: common/fdt_support.c,1406: braces {} are not necessary for single > statement blocks > warning: common/fdt_support.c,1412: braces {} are not necessary for single > statement blocks > warning: include/fdt_support.h,96: line over 80 characters > > checkpatch.pl found 0 error(s), 4 warning(s), 0 checks(s) > Will fix them in the next patch series about this topic. > >> >> --- >> common/fdt_support.c | 56 >> +++++++++++++++++++++++++++++++++++++++++++++++++++ >> include/fdt_support.h | 5 +++++ >> 2 files changed, 61 insertions(+) >> >> diff --git a/common/fdt_support.c b/common/fdt_support.c >> index 784a570..72004a3 100644 >> --- a/common/fdt_support.c >> +++ b/common/fdt_support.c >> @@ -11,6 +11,7 @@ >> #include <stdio_dev.h> >> #include <linux/ctype.h> >> #include <linux/types.h> >> +#include <linux/fb.h> >> #include <asm/global_data.h> >> #include <libfdt.h> >> #include <fdt_support.h> >> @@ -1373,6 +1374,61 @@ err_size: >> #endif >> >> /* >> +* fdt_find_display_timings: finde node containing display-timings >> +* >> +* @fdt: fdt to device tree >> +* @compat: compatiable string to match >> +* @parent: parent node containing display-timings > > > or -ve error code FDT_ERROR_... > ok >> >> +*/ >> +int fdt_find_display_timings(void *fdt, const char *compat, const char >> *parent) >> +{ >> + int coff = fdt_node_offset_by_compatible(fdt, -1, compat); >> + int poff = fdt_subnode_offset(fdt, coff, parent); >> + int noff = fdt_subnode_offset(fdt, poff, "display-timings"); >> + > > > Can we return an error when we see one? Here it will return a somewhat > meaningless error if (say) the first call finds no node. > I can return the return code of the three functions. Something like: int coff, poff, noff; coff = fdt_node_offset_by_compatible(..); if (coff < 0) return coff; poff = fdt_subnode_offset(..); if (poff < 0) .... Would that be better? >> >> + return noff; >> +} >> + >> +/* >> +* fdt_update_display_timings: update display-timings properties >> +* >> +* @fdt: fdt to device tree >> +* @compat: compatiable string to match > > > compatible > ok >> >> +* @parent: parent node containing display-timings > > > @parent: parent node containing display-timings subnode > yes... thats better. >> >> +* @mode: ptr to fb_videomode > > > Well we know that from the code. Perhaps "display timings to add to the > device tree" > sounds good to me. >> >> +*/ > > > This function is exported so the comment should go in the header file. > okay. >> >> +int fdt_update_display_timings(void *fdt, const char *compat, const char >> *parent, >> + struct fb_videomode *mode) >> +{ >> + int noff = fdt_find_display_timings(fdt, compat, parent); >> + >> + /* check if display-timings subnode does exist */ >> + if (noff == -FDT_ERR_NOTFOUND) { > > > if (noff < 0) > > would be better ok > >> >> + return noff; >> + } >> + >> + /* check if timing0 subnode exists */ >> + noff = fdt_subnode_offset(fdt, noff, "timing0"); >> + if (noff == -FDT_ERR_NOTFOUND) { > > > same here ok > >> >> + return noff; >> + } >> + >> + /* set all needed properties */ >> + fdt_setprop_u32(fdt, noff, "clock-frequency", >> + PICOS2KHZ(mode->pixclock) * 1000); >> + fdt_setprop_u32(fdt, noff, "hactive", mode->xres); >> + fdt_setprop_u32(fdt, noff, "vactive", mode->yres); >> + fdt_setprop_u32(fdt, noff, "hback-porch", mode->left_margin); >> + fdt_setprop_u32(fdt, noff, "hfront-porch", mode->right_margin); >> + fdt_setprop_u32(fdt, noff, "vback-porch", mode->upper_margin); >> + fdt_setprop_u32(fdt, noff, "vfront-porch", mode->lower_margin); >> + fdt_setprop_u32(fdt, noff, "hsync-len", mode->hsync_len); >> + fdt_setprop_u32(fdt, noff, "vsync-len", mode->vsync_len); > > > Should you have error checking here? We might run out of space. Sounds like a good idea - will change the current code. > >> >> + >> + return 0; >> +} >> + >> +/* >> * Verify the physical address of device tree node for a given alias >> * >> * This function locates the device tree node of a given alias, and then >> diff --git a/include/fdt_support.h b/include/fdt_support.h >> index 1bda686..4222ab4 100644 >> --- a/include/fdt_support.h >> +++ b/include/fdt_support.h >> @@ -91,6 +91,11 @@ int fdt_set_phandle(void *fdt, int nodeoffset, uint32_t >> phandle); >> unsigned int fdt_create_phandle(void *fdt, int nodeoffset); >> int fdt_add_edid(void *blob, const char *compat, unsigned char *buf); >> >> +struct fb_videomode; >> +int fdt_find_display_timings(void *fdt, const char *compat, const char >> *parent); >> +int fdt_update_display_timings(void *fdt, const char *compat, const char >> *parent, >> + struct fb_videomode *mode); >> + >> int fdt_verify_alias_address(void *fdt, int anode, const char *alias, >> u64 addr); >> u64 fdt_get_base_address(void *fdt, int node); >> -- > > greets -- Christian Gmeiner, MSc https://soundcloud.com/christian-gmeiner _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot