Re: [PATCH v2 2/6] video: logo: allow the boot logo to come from the device tree

[Helge Deller <deller@kernel.org>]

* Max Pedraza <maximpedraza@gmail.com>:
> Add CONFIG_LOGO_DT_CLUT224, which makes fb_find_logo() look for a node
> compatible with "linux,boot-logo-clut224" under /chosen before falling
> back to the logos built into the kernel image.
> 
> The image is validated before it is used: the palette must have at most
> 224 entries, the pixel data length must match the geometry, and every
> pixel must reference an entry that exists. A malformed node is reported
> and ignored rather than drawn, so a bad device tree cannot take the
> display down with it.
> 
> The image is copied out of the device tree so that the 32 entry offset
> the frame buffer layer reserves for the console can be applied to the
> pixels, and the copy is released from fb_logo_late_init() alongside the
> built-in logos.
> 
> The node lives under /chosen because a logo is configuration handed over
> by firmware rather than a description of the hardware, which is also
> where simple-framebuffer nodes live for the same reason.
> 
> Signed-off-by: Max Pedraza <maximpedraza@gmail.com>
> ---
>  drivers/video/logo/Kconfig |  12 +++
>  drivers/video/logo/logo.c  | 160 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 172 insertions(+)
> 
> diff --git a/drivers/video/logo/Kconfig b/drivers/video/logo/Kconfig
> index cda15b958..215afa7ef 100644
> --- a/drivers/video/logo/Kconfig
> +++ b/drivers/video/logo/Kconfig
> @@ -76,4 +76,16 @@ config LOGO_LINUX_CLUT224_FILE
>  
>  	    magick source_image -compress none -colors 224 destination.ppm
>  
> +config LOGO_DT_CLUT224
> +	bool "224-color logo supplied by the device tree"
> +	depends on OF
> +	help
> +	  Look for a boot logo in the device tree, in a node compatible with
> +	  "linux,boot-logo-clut224" under /chosen, instead of using one of
> +	  the logos built into the kernel image. This allows a single kernel
> +	  image to be used by several products that only differ in branding.
> +
> +	  If no such node is present, or it is disabled, the built-in logo
> +	  selected above is used, so saying Y here is safe.
> +
>  endif # LOGO
> diff --git a/drivers/video/logo/logo.c b/drivers/video/logo/logo.c
> index 91535f884..7f8b04ecf 100644
> --- a/drivers/video/logo/logo.c
> +++ b/drivers/video/logo/logo.c
> @@ -11,6 +11,9 @@
>   */
>  
>  #include <linux/linux_logo.h>
> +#include <linux/of.h>
> +#include <linux/sizes.h>
> +#include <linux/slab.h>
>  #include <linux/stddef.h>
>  #include <linux/module.h>
>  
> @@ -22,6 +25,155 @@ static bool nologo;
>  module_param(nologo, bool, 0);
>  MODULE_PARM_DESC(nologo, "Disables startup logo");
>  
> +#ifdef CONFIG_LOGO_DT_CLUT224


With the #ifdef above, your logo code will only be compiled when
people enable CONFIG_LOGO_DT_CLUT224, and as such coding errors
(maybe even introduced by other patches) will only show up randomly.

I usually prefer if people use the IS_ENABLED(CONFIG_XXX) macro instead
and put it at specific entry places, so that while the compiler can do
compile-time checking the code, it can optimize it away too, when the
option isn't enabled.

As an *example*, see my patch below (on top you your code). It compiles cleanly
for me and does the compile-time checking as well.

While respinning your other patches, maybe you can check if something similiar
can be used there too (but only if it makes sense there!).

Helge


diff --git a/drivers/video/logo/logo.c b/drivers/video/logo/logo.c
index 66bcb37e78d5..f68ded458d2e 100644
--- a/drivers/video/logo/logo.c
+++ b/drivers/video/logo/logo.c
@@ -27,7 +27,8 @@ static bool nologo;
 module_param(nologo, bool, 0);
 MODULE_PARM_DESC(nologo, "Disables startup logo");
 
-#ifdef CONFIG_LOGO_DT_CLUT224
+
+/* LOGO in devicetree: */
 
 #define LOGO_DT_COMPATIBLE	"linux,boot-logo-clut224"
 #define LOGO_DT_MAX_CLUT	224
@@ -229,6 +230,9 @@ static const struct linux_logo *logo_dt_find(void)
 	struct device_node *np;
 	int ret;
 
+	if (!IS_ENABLED(CONFIG_LOGO_DT_CLUT224))
+		return NULL;
+
 	if (probed)
 		return logo_dt_data ? &logo_dt_clut224 : NULL;
 
@@ -252,6 +256,9 @@ static const struct linux_logo *logo_dt_find(void)
 
 static void logo_dt_free(void)
 {
+	if (!IS_ENABLED(CONFIG_LOGO_DT_CLUT224))
+		return;
+
 	logo_dt_clut224.clut = NULL;
 	logo_dt_clut224.data = NULL;
 
@@ -262,17 +269,6 @@ static void logo_dt_free(void)
 	logo_dt_data = NULL;
 }
 
-#else /* !CONFIG_LOGO_DT_CLUT224 */
-
-static inline const struct linux_logo *logo_dt_find(void)
-{
-	return NULL;
-}
-
-static inline void logo_dt_free(void) { }
-
-#endif /* CONFIG_LOGO_DT_CLUT224 */
-
 /*
  * Logos are located in the initdata, and will be freed in kernel_init.
  * Use late_init to mark the logos as freed to prevent any further use.