[U-Boot] [PATCH 2/3] fdt: add fdt_add_display_timings(..) and friends
Christian Gmeiner
christian.gmeiner at gmail.com
Tue Oct 7 11:18:26 CEST 2014
Hi Simon
> 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>
>
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
More information about the U-Boot
mailing list