[U-Boot] [PATCH 2/3] fdt: add fdt_add_display_timings(..) and friends

[Simon Glass]

HI Christian,

On 15 September 2014 07:06, Christian Gmeiner <christian.gmeiner at 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 at 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