| Message ID | 1416403920-24561-4-git-send-email-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Hans de Goede |
| Headers | show |
On Wed, 2014-11-19 at 14:32 +0100, Hans de Goede wrote: > From: Luc Verhaegen <libv@skynet.be> > > Add simplefb support, note this depends on the kernel having support for > the clocks property which has recently been added to the simplefb devicetree > binding. > > Signed-off-by: Luc Verhaegen <libv@skynet.be> > [hdegoede@redhat.com: Use pre-populated simplefb node under /chosen as > disussed on the devicetree list] > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Acked-by: Ian Campbell <ijc@hellion.org.uk>. One non-blocking queries: > + /* Find a framebuffer node, with pipeline == "de_be0-lcd0-hdmi" */ > + offset = fdt_node_offset_by_compatible(blob, -1, > + "allwinner,simple-framebuffer"); > + while (offset >= 0) { > + ret = fdt_find_string(blob, offset, "allwinner,pipeline", > + "de_be0-lcd0-hdmi"); > + if (ret == 0) > + break; > + offset = fdt_node_offset_by_compatible(blob, offset, > + "allwinner,simple-framebuffer"); > + } Is this variant non-conformant with coding style?: int offset = -1; while ( (offset = fdt_node_offset_by_compatible(blob, offset, "allwinner,simple-framebuffer") ) { LOOP BODY } I expect it is because of the assignment within the while condition, which is a shame, since this is one case where it is IMHO leads to clearer code.
Hi, On 11/20/2014 10:22 AM, Ian Campbell wrote: > On Wed, 2014-11-19 at 14:32 +0100, Hans de Goede wrote: >> From: Luc Verhaegen <libv@skynet.be> >> >> Add simplefb support, note this depends on the kernel having support for >> the clocks property which has recently been added to the simplefb devicetree >> binding. >> >> Signed-off-by: Luc Verhaegen <libv@skynet.be> >> [hdegoede@redhat.com: Use pre-populated simplefb node under /chosen as >> disussed on the devicetree list] >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Acked-by: Ian Campbell <ijc@hellion.org.uk>. Thanks, any chance you could also review v2 of the CONFIG_USB_KEYBOARD patch ? > One non-blocking queries: > >> + /* Find a framebuffer node, with pipeline == "de_be0-lcd0-hdmi" */ >> + offset = fdt_node_offset_by_compatible(blob, -1, >> + "allwinner,simple-framebuffer"); >> + while (offset >= 0) { >> + ret = fdt_find_string(blob, offset, "allwinner,pipeline", >> + "de_be0-lcd0-hdmi"); >> + if (ret == 0) >> + break; >> + offset = fdt_node_offset_by_compatible(blob, offset, >> + "allwinner,simple-framebuffer"); >> + } > > Is this variant non-conformant with coding style?: > > int offset = -1; > while ( (offset = fdt_node_offset_by_compatible(blob, offset, > "allwinner,simple-framebuffer") ) { > LOOP BODY > } > > I expect it is because of the assignment within the while condition, > which is a shame, since this is one case where it is IMHO leads to > clearer code. AFAIK this indeed would go against the coding style, and TBH I'm also not sure if I agree it would be cleaner (mainly because indentation would become (even more) messy due to the 80 columns limit). Regards, Hans
Hi Hans, On 19 November 2014 13:32, Hans de Goede <hdegoede@redhat.com> wrote: > From: Luc Verhaegen <libv@skynet.be> > > Add simplefb support, note this depends on the kernel having support for > the clocks property which has recently been added to the simplefb devicetree > binding. > > Signed-off-by: Luc Verhaegen <libv@skynet.be> > [hdegoede@redhat.com: Use pre-populated simplefb node under /chosen as > disussed on the devicetree list] > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > arch/arm/include/asm/arch-sunxi/display.h | 4 +++ > board/sunxi/board.c | 11 ++++++++ > drivers/video/sunxi_display.c | 42 +++++++++++++++++++++++++++++++ > include/configs/sunxi-common.h | 8 ++++++ > 4 files changed, 65 insertions(+) > > diff --git a/arch/arm/include/asm/arch-sunxi/display.h b/arch/arm/include/asm/arch-sunxi/display.h > index 8d80ceb..c9d81ba 100644 > --- a/arch/arm/include/asm/arch-sunxi/display.h > +++ b/arch/arm/include/asm/arch-sunxi/display.h > @@ -195,4 +195,8 @@ struct sunxi_hdmi_reg { > #define SUNXI_HDMI_PLL_DBG0_PLL3 (0 << 21) > #define SUNXI_HDMI_PLL_DBG0_PLL7 (1 << 21) > > +#ifdef CONFIG_VIDEO_DT_SIMPLEFB > +int sunxi_simplefb_setup(void *blob); > +#endif Do you really need this #ifdef? > + > #endif /* _SUNXI_DISPLAY_H */ > diff --git a/board/sunxi/board.c b/board/sunxi/board.c > index e6ec5b8..d4530e8 100644 > --- a/board/sunxi/board.c > +++ b/board/sunxi/board.c > @@ -24,6 +24,7 @@ > #endif > #include <asm/arch/clock.h> > #include <asm/arch/cpu.h> > +#include <asm/arch/display.h> > #include <asm/arch/dram.h> > #include <asm/arch/gpio.h> > #include <asm/arch/mmc.h> > @@ -237,3 +238,13 @@ int misc_init_r(void) > return 0; > } > #endif > + > +#ifdef CONFIG_OF_BOARD_SETUP > +void > +ft_board_setup(void *blob, bd_t *bd) void on same line? Also can you make it return int. You could look at u-boot-fdt/next (I am waiting on resolving one patch before sending a pull). > +{ > +#ifdef CONFIG_VIDEO_DT_SIMPLEFB > + sunxi_simplefb_setup(blob); > +#endif > +} > +#endif /* CONFIG_OF_BOARD_SETUP */ > diff --git a/drivers/video/sunxi_display.c b/drivers/video/sunxi_display.c > index 3f46c31..2f3f5db 100644 > --- a/drivers/video/sunxi_display.c > +++ b/drivers/video/sunxi_display.c > @@ -13,6 +13,8 @@ > #include <asm/arch/display.h> > #include <asm/global_data.h> > #include <asm/io.h> > +#include <fdtdec.h> > +#include <fdt_support.h> > #include <linux/fb.h> > #include <video_fb.h> > > @@ -416,3 +418,43 @@ video_hw_init(void) > > return graphic_device; > } > + > +/* > + * Simplefb support. > + */ > +#if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_VIDEO_DT_SIMPLEFB) > +int > +sunxi_simplefb_setup(void *blob) int on same line > +{ > + static GraphicDevice *graphic_device = &sunxi_display.graphic_device; > + int offset, ret; > + > + if (!sunxi_display.enabled) > + return 0; > + > + /* Find a framebuffer node, with pipeline == "de_be0-lcd0-hdmi" */ > + offset = fdt_node_offset_by_compatible(blob, -1, > + "allwinner,simple-framebuffer"); > + while (offset >= 0) { > + ret = fdt_find_string(blob, offset, "allwinner,pipeline", > + "de_be0-lcd0-hdmi"); > + if (ret == 0) > + break; > + offset = fdt_node_offset_by_compatible(blob, offset, > + "allwinner,simple-framebuffer"); > + } Can you use do...while() to avoid repeating the 'offset =' line? > + if (offset < 0) { > + eprintf("Cannot setup simplefb: node not found\n"); > + return 0; /* Keep older kernels working */ > + } > + > + ret = fdt_setup_simplefb_node(blob, offset, gd->fb_base, > + graphic_device->winSizeX, graphic_device->winSizeY, > + graphic_device->winSizeX * graphic_device->gdfBytesPP, > + "x8r8g8b8"); > + if (ret) > + eprintf("Cannot setup simplefb: Error setting properties\n"); > + > + return ret; > +} > +#endif /* CONFIG_OF_BOARD_SETUP && CONFIG_VIDEO_DT_SIMPLEFB */ > diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h > index 900ef52..d5d907b 100644 > --- a/include/configs/sunxi-common.h > +++ b/include/configs/sunxi-common.h > @@ -204,6 +204,9 @@ > */ > #define CONFIG_SUNXI_FB_SIZE (8 << 20) > > +/* Do we want to initialize a simple FB? */ > +#define CONFIG_VIDEO_DT_SIMPLEFB > + > #define CONFIG_VIDEO_SUNXI > > #define CONFIG_CFB_CONSOLE > @@ -217,6 +220,11 @@ > > #define CONFIG_SYS_MEM_TOP_HIDE ((CONFIG_SUNXI_FB_SIZE + 0xFFF) & ~0xFFF) > > +/* To be able to hook simplefb into dt */ > +#ifdef CONFIG_VIDEO_DT_SIMPLEFB > +#define CONFIG_OF_BOARD_SETUP > +#endif > + > #endif /* CONFIG_VIDEO */ > > /* Ethernet support */ > -- > 2.1.0 > Regards, Simon
Hi, On 11/20/2014 06:40 PM, Simon Glass wrote: > Hi Hans, > > On 19 November 2014 13:32, Hans de Goede <hdegoede@redhat.com> wrote: >> From: Luc Verhaegen <libv@skynet.be> >> >> Add simplefb support, note this depends on the kernel having support for >> the clocks property which has recently been added to the simplefb devicetree >> binding. >> >> Signed-off-by: Luc Verhaegen <libv@skynet.be> >> [hdegoede@redhat.com: Use pre-populated simplefb node under /chosen as >> disussed on the devicetree list] >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> arch/arm/include/asm/arch-sunxi/display.h | 4 +++ >> board/sunxi/board.c | 11 ++++++++ >> drivers/video/sunxi_display.c | 42 +++++++++++++++++++++++++++++++ >> include/configs/sunxi-common.h | 8 ++++++ >> 4 files changed, 65 insertions(+) >> >> diff --git a/arch/arm/include/asm/arch-sunxi/display.h b/arch/arm/include/asm/arch-sunxi/display.h >> index 8d80ceb..c9d81ba 100644 >> --- a/arch/arm/include/asm/arch-sunxi/display.h >> +++ b/arch/arm/include/asm/arch-sunxi/display.h >> @@ -195,4 +195,8 @@ struct sunxi_hdmi_reg { >> #define SUNXI_HDMI_PLL_DBG0_PLL3 (0 << 21) >> #define SUNXI_HDMI_PLL_DBG0_PLL7 (1 << 21) >> >> +#ifdef CONFIG_VIDEO_DT_SIMPLEFB >> +int sunxi_simplefb_setup(void *blob); >> +#endif > > Do you really need this #ifdef? No, I'm not the original author of this patch and I inherited this from the original author, I'm not a big fan either, I'll drop it. > >> + >> #endif /* _SUNXI_DISPLAY_H */ >> diff --git a/board/sunxi/board.c b/board/sunxi/board.c >> index e6ec5b8..d4530e8 100644 >> --- a/board/sunxi/board.c >> +++ b/board/sunxi/board.c >> @@ -24,6 +24,7 @@ >> #endif >> #include <asm/arch/clock.h> >> #include <asm/arch/cpu.h> >> +#include <asm/arch/display.h> >> #include <asm/arch/dram.h> >> #include <asm/arch/gpio.h> >> #include <asm/arch/mmc.h> >> @@ -237,3 +238,13 @@ int misc_init_r(void) >> return 0; >> } >> #endif >> + >> +#ifdef CONFIG_OF_BOARD_SETUP >> +void >> +ft_board_setup(void *blob, bd_t *bd) > > void on same line? Another inherited thing, will fixup (everywhere). > Also can you make it return int. You could look at > u-boot-fdt/next (I am waiting on resolving one patch before sending a > pull). Ack, will also fix. > >> +{ >> +#ifdef CONFIG_VIDEO_DT_SIMPLEFB >> + sunxi_simplefb_setup(blob); >> +#endif >> +} >> +#endif /* CONFIG_OF_BOARD_SETUP */ >> diff --git a/drivers/video/sunxi_display.c b/drivers/video/sunxi_display.c >> index 3f46c31..2f3f5db 100644 >> --- a/drivers/video/sunxi_display.c >> +++ b/drivers/video/sunxi_display.c >> @@ -13,6 +13,8 @@ >> #include <asm/arch/display.h> >> #include <asm/global_data.h> >> #include <asm/io.h> >> +#include <fdtdec.h> >> +#include <fdt_support.h> >> #include <linux/fb.h> >> #include <video_fb.h> >> >> @@ -416,3 +418,43 @@ video_hw_init(void) >> >> return graphic_device; >> } >> + >> +/* >> + * Simplefb support. >> + */ >> +#if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_VIDEO_DT_SIMPLEFB) >> +int >> +sunxi_simplefb_setup(void *blob) > > int on same line > >> +{ >> + static GraphicDevice *graphic_device = &sunxi_display.graphic_device; >> + int offset, ret; >> + >> + if (!sunxi_display.enabled) >> + return 0; >> + >> + /* Find a framebuffer node, with pipeline == "de_be0-lcd0-hdmi" */ >> + offset = fdt_node_offset_by_compatible(blob, -1, >> + "allwinner,simple-framebuffer"); >> + while (offset >= 0) { >> + ret = fdt_find_string(blob, offset, "allwinner,pipeline", >> + "de_be0-lcd0-hdmi"); >> + if (ret == 0) >> + break; >> + offset = fdt_node_offset_by_compatible(blob, offset, >> + "allwinner,simple-framebuffer"); >> + } > > Can you use do...while() to avoid repeating the 'offset =' line? > >> + if (offset < 0) { >> + eprintf("Cannot setup simplefb: node not found\n"); >> + return 0; /* Keep older kernels working */ >> + } >> + >> + ret = fdt_setup_simplefb_node(blob, offset, gd->fb_base, >> + graphic_device->winSizeX, graphic_device->winSizeY, >> + graphic_device->winSizeX * graphic_device->gdfBytesPP, >> + "x8r8g8b8"); >> + if (ret) >> + eprintf("Cannot setup simplefb: Error setting properties\n"); >> + >> + return ret; >> +} >> +#endif /* CONFIG_OF_BOARD_SETUP && CONFIG_VIDEO_DT_SIMPLEFB */ >> diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h >> index 900ef52..d5d907b 100644 >> --- a/include/configs/sunxi-common.h >> +++ b/include/configs/sunxi-common.h >> @@ -204,6 +204,9 @@ >> */ >> #define CONFIG_SUNXI_FB_SIZE (8 << 20) >> >> +/* Do we want to initialize a simple FB? */ >> +#define CONFIG_VIDEO_DT_SIMPLEFB >> + >> #define CONFIG_VIDEO_SUNXI >> >> #define CONFIG_CFB_CONSOLE >> @@ -217,6 +220,11 @@ >> >> #define CONFIG_SYS_MEM_TOP_HIDE ((CONFIG_SUNXI_FB_SIZE + 0xFFF) & ~0xFFF) >> >> +/* To be able to hook simplefb into dt */ >> +#ifdef CONFIG_VIDEO_DT_SIMPLEFB >> +#define CONFIG_OF_BOARD_SETUP >> +#endif >> + >> #endif /* CONFIG_VIDEO */ >> >> /* Ethernet support */ >> -- >> 2.1.0 >> > > Regards, > Simon Regards, Hans
diff --git a/arch/arm/include/asm/arch-sunxi/display.h b/arch/arm/include/asm/arch-sunxi/display.h index 8d80ceb..c9d81ba 100644 --- a/arch/arm/include/asm/arch-sunxi/display.h +++ b/arch/arm/include/asm/arch-sunxi/display.h @@ -195,4 +195,8 @@ struct sunxi_hdmi_reg { #define SUNXI_HDMI_PLL_DBG0_PLL3 (0 << 21) #define SUNXI_HDMI_PLL_DBG0_PLL7 (1 << 21) +#ifdef CONFIG_VIDEO_DT_SIMPLEFB +int sunxi_simplefb_setup(void *blob); +#endif + #endif /* _SUNXI_DISPLAY_H */ diff --git a/board/sunxi/board.c b/board/sunxi/board.c index e6ec5b8..d4530e8 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -24,6 +24,7 @@ #endif #include <asm/arch/clock.h> #include <asm/arch/cpu.h> +#include <asm/arch/display.h> #include <asm/arch/dram.h> #include <asm/arch/gpio.h> #include <asm/arch/mmc.h> @@ -237,3 +238,13 @@ int misc_init_r(void) return 0; } #endif + +#ifdef CONFIG_OF_BOARD_SETUP +void +ft_board_setup(void *blob, bd_t *bd) +{ +#ifdef CONFIG_VIDEO_DT_SIMPLEFB + sunxi_simplefb_setup(blob); +#endif +} +#endif /* CONFIG_OF_BOARD_SETUP */ diff --git a/drivers/video/sunxi_display.c b/drivers/video/sunxi_display.c index 3f46c31..2f3f5db 100644 --- a/drivers/video/sunxi_display.c +++ b/drivers/video/sunxi_display.c @@ -13,6 +13,8 @@ #include <asm/arch/display.h> #include <asm/global_data.h> #include <asm/io.h> +#include <fdtdec.h> +#include <fdt_support.h> #include <linux/fb.h> #include <video_fb.h> @@ -416,3 +418,43 @@ video_hw_init(void) return graphic_device; } + +/* + * Simplefb support. + */ +#if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_VIDEO_DT_SIMPLEFB) +int +sunxi_simplefb_setup(void *blob) +{ + static GraphicDevice *graphic_device = &sunxi_display.graphic_device; + int offset, ret; + + if (!sunxi_display.enabled) + return 0; + + /* Find a framebuffer node, with pipeline == "de_be0-lcd0-hdmi" */ + offset = fdt_node_offset_by_compatible(blob, -1, + "allwinner,simple-framebuffer"); + while (offset >= 0) { + ret = fdt_find_string(blob, offset, "allwinner,pipeline", + "de_be0-lcd0-hdmi"); + if (ret == 0) + break; + offset = fdt_node_offset_by_compatible(blob, offset, + "allwinner,simple-framebuffer"); + } + if (offset < 0) { + eprintf("Cannot setup simplefb: node not found\n"); + return 0; /* Keep older kernels working */ + } + + ret = fdt_setup_simplefb_node(blob, offset, gd->fb_base, + graphic_device->winSizeX, graphic_device->winSizeY, + graphic_device->winSizeX * graphic_device->gdfBytesPP, + "x8r8g8b8"); + if (ret) + eprintf("Cannot setup simplefb: Error setting properties\n"); + + return ret; +} +#endif /* CONFIG_OF_BOARD_SETUP && CONFIG_VIDEO_DT_SIMPLEFB */ diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h index 900ef52..d5d907b 100644 --- a/include/configs/sunxi-common.h +++ b/include/configs/sunxi-common.h @@ -204,6 +204,9 @@ */ #define CONFIG_SUNXI_FB_SIZE (8 << 20) +/* Do we want to initialize a simple FB? */ +#define CONFIG_VIDEO_DT_SIMPLEFB + #define CONFIG_VIDEO_SUNXI #define CONFIG_CFB_CONSOLE @@ -217,6 +220,11 @@ #define CONFIG_SYS_MEM_TOP_HIDE ((CONFIG_SUNXI_FB_SIZE + 0xFFF) & ~0xFFF) +/* To be able to hook simplefb into dt */ +#ifdef CONFIG_VIDEO_DT_SIMPLEFB +#define CONFIG_OF_BOARD_SETUP +#endif + #endif /* CONFIG_VIDEO */ /* Ethernet support */