HI Christian,
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>
>
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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
