| Message ID | 1410786366-26494-2-git-send-email-christian.gmeiner@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
HI Christian, On 15 September 2014 07:06, Christian Gmeiner <christian.gmeiner@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.gmeiner@gmail.com> > 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) > --- > 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_... > +*/ > +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. > + return noff; > +} > + > +/* > +* fdt_update_display_timings: update display-timings properties > +* > +* @fdt: fdt to device tree > +* @compat: compatiable string to match > compatible > +* @parent: parent node containing display-timings > @parent: parent node containing display-timings subnode > +* @mode: ptr to fb_videomode > Well we know that from the code. Perhaps "display timings to add to the device tree" > +*/ > This function is exported so the comment should go in the header file. > +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 > + return noff; > + } > + > + /* check if timing0 subnode exists */ > + noff = fdt_subnode_offset(fdt, noff, "timing0"); > + if (noff == -FDT_ERR_NOTFOUND) { > same here > + 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. > + > + 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); > -- > Regards, Simon
Hi Simon > On 15 September 2014 07:06, Christian Gmeiner <christian.gmeiner@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.gmeiner@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
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 +*/ +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"); + + return noff; +} + +/* +* fdt_update_display_timings: update display-timings properties +* +* @fdt: fdt to device tree +* @compat: compatiable string to match +* @parent: parent node containing display-timings +* @mode: ptr to fb_videomode +*/ +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) { + return noff; + } + + /* check if timing0 subnode exists */ + noff = fdt_subnode_offset(fdt, noff, "timing0"); + if (noff == -FDT_ERR_NOTFOUND) { + 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); + + 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);
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.gmeiner@gmail.com> --- common/fdt_support.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/fdt_support.h | 5 +++++ 2 files changed, 61 insertions(+)