Re: [PATCH 25/30] fbdev/core: Move framebuffer and backlight helpers into separate files

From: Sam Ravnborg <sam@ravnborg.org>
To: Thomas Zimmermann <tzimmermann@suse.de>
Cc: daniel@ffwll.ch, javierm@redhat.com, deller@gmx.de,
	geert+renesas@glider.be, lee@kernel.org,
	daniel.thompson@linaro.org, jingoohan1@gmail.com,
	linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linux-sh@vger.kernel.org, linux-omap@vger.kernel.org,
	linux-staging@lists.linux.dev
Subject: Re: [PATCH 25/30] fbdev/core: Move framebuffer and backlight helpers into separate files
Date: Wed, 7 Jun 2023 21:38:22 +0200	[thread overview]
Message-ID: <20230607193822.GB670717@ravnborg.org> (raw)
In-Reply-To: <20230605144812.15241-26-tzimmermann@suse.de>

Hi Thomas,

On Mon, Jun 05, 2023 at 04:48:07PM +0200, Thomas Zimmermann wrote:
> Move framebuffer and backlight helpers into separate files. Leave
> fbsysfs.c to sysfs-related code. No functional changes.
> 
> The framebuffer helpers are not in fbmem.c because they are under
> GPL-2.0-or-later copyright, while fbmem.c is GPL-2.0.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Some nits that you decide what to do with.
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

I do not get why they are moved out in separate files.
I guess the picture will materialize later.

	Sam

> ---
>  drivers/video/fbdev/core/Makefile       |   4 +-
>  drivers/video/fbdev/core/fb_backlight.c |  32 +++++++
>  drivers/video/fbdev/core/fb_info.c      |  76 ++++++++++++++++
>  drivers/video/fbdev/core/fbsysfs.c      | 110 +-----------------------
>  4 files changed, 112 insertions(+), 110 deletions(-)
>  create mode 100644 drivers/video/fbdev/core/fb_backlight.c
>  create mode 100644 drivers/video/fbdev/core/fb_info.c
> 
> diff --git a/drivers/video/fbdev/core/Makefile b/drivers/video/fbdev/core/Makefile
> index 8f0060160ffb..eee3295bc225 100644
> --- a/drivers/video/fbdev/core/Makefile
> +++ b/drivers/video/fbdev/core/Makefile
> @@ -1,7 +1,9 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_FB_NOTIFY)           += fb_notify.o
>  obj-$(CONFIG_FB)                  += fb.o
> -fb-y                              := fbmem.o fbmon.o fbcmap.o fbsysfs.o \
> +fb-y                              := fb_backlight.o \
> +                                     fb_info.o \
> +                                     fbmem.o fbmon.o fbcmap.o fbsysfs.o \
>                                       modedb.o fbcvt.o fb_cmdline.o fb_io_fops.o
There is "+=" that can be used in Makefile, but people prefer '\' -
sigh!

>  fb-$(CONFIG_FB_DEFERRED_IO)       += fb_defio.o
>  
> diff --git a/drivers/video/fbdev/core/fb_backlight.c b/drivers/video/fbdev/core/fb_backlight.c
> new file mode 100644
> index 000000000000..feffe6c68039
> --- /dev/null
> +++ b/drivers/video/fbdev/core/fb_backlight.c
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
Hmm, can we change the license from 2.0 to 2.0-or-later without any
concern? I hope so.

> +
> +#include <linux/export.h>
> +#include <linux/fb.h>
#include <linux/mutex.h> - to avoid relying on indirect includes?

> +
> +#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
> +/*
> + * This function generates a linear backlight curve
> + *
> + *     0: off
> + *   1-7: min
> + * 8-127: linear from min to max
> + */
> +void fb_bl_default_curve(struct fb_info *fb_info, u8 off, u8 min, u8 max)
> +{
> +	unsigned int i, flat, count, range = (max - min);
> +
> +	mutex_lock(&fb_info->bl_curve_mutex);
> +
> +	fb_info->bl_curve[0] = off;
> +
> +	for (flat = 1; flat < (FB_BACKLIGHT_LEVELS / 16); ++flat)
> +		fb_info->bl_curve[flat] = min;
> +
> +	count = FB_BACKLIGHT_LEVELS * 15 / 16;
> +	for (i = 0; i < count; ++i)
> +		fb_info->bl_curve[flat + i] = min + (range * (i + 1) / count);
> +
> +	mutex_unlock(&fb_info->bl_curve_mutex);
> +}
> +EXPORT_SYMBOL_GPL(fb_bl_default_curve);
> +#endif
> diff --git a/drivers/video/fbdev/core/fb_info.c b/drivers/video/fbdev/core/fb_info.c
> new file mode 100644
> index 000000000000..fb5b75009ee7
> --- /dev/null
> +++ b/drivers/video/fbdev/core/fb_info.c
> @@ -0,0 +1,76 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <linux/export.h>
> +#include <linux/fb.h>
Same as above, consider including mutex.h
Also slab.h

> +
> +/**
> + * framebuffer_alloc - creates a new frame buffer info structure
> + *
> + * @size: size of driver private data, can be zero
> + * @dev: pointer to the device for this fb, this can be NULL
> + *
> + * Creates a new frame buffer info structure. Also reserves @size bytes
> + * for driver private data (info->par). info->par (if any) will be
> + * aligned to sizeof(long).
> + *
> + * Returns the new structure, or NULL if an error occurred.
> + *
> + */
> +struct fb_info *framebuffer_alloc(size_t size, struct device *dev)
> +{
> +#define BYTES_PER_LONG (BITS_PER_LONG/8)
> +#define PADDING (BYTES_PER_LONG - (sizeof(struct fb_info) % BYTES_PER_LONG))
> +	int fb_info_size = sizeof(struct fb_info);
> +	struct fb_info *info;
> +	char *p;
> +
> +	if (size)
> +		fb_info_size += PADDING;
> +
> +	p = kzalloc(fb_info_size + size, GFP_KERNEL);
> +
> +	if (!p)
> +		return NULL;
> +
> +	info = (struct fb_info *) p;
> +
> +	if (size)
> +		info->par = p + fb_info_size;
> +
> +	info->device = dev;
> +	info->fbcon_rotate_hint = -1;
> +
> +#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
> +	mutex_init(&info->bl_curve_mutex);
> +#endif
> +
> +	return info;
> +#undef PADDING
> +#undef BYTES_PER_LONG
> +}
> +EXPORT_SYMBOL(framebuffer_alloc);
> +
> +/**
> + * framebuffer_release - marks the structure available for freeing
> + *
> + * @info: frame buffer info structure
> + *
> + * Drop the reference count of the device embedded in the
> + * framebuffer info structure.
> + *
> + */
> +void framebuffer_release(struct fb_info *info)
> +{
> +	if (!info)
> +		return;
> +
> +	if (WARN_ON(refcount_read(&info->count)))
> +		return;
> +
> +#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
> +	mutex_destroy(&info->bl_curve_mutex);
> +#endif
> +
> +	kfree(info);
> +}
> +EXPORT_SYMBOL(framebuffer_release);
> diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
> index 0c33c4adcd79..849073f1ca06 100644
> --- a/drivers/video/fbdev/core/fbsysfs.c
> +++ b/drivers/video/fbdev/core/fbsysfs.c
> @@ -5,93 +5,12 @@
>   * Copyright (c) 2004 James Simmons <jsimmons@infradead.org>
>   */
>  
> -/*
> - * Note:  currently there's only stubs for framebuffer_alloc and
> - * framebuffer_release here.  The reson for that is that until all drivers
> - * are converted to use it a sysfsification will open OOPSable races.
> - */
> -
> -#include <linux/kernel.h>
> -#include <linux/slab.h>
> +#include <linux/console.h>
>  #include <linux/fb.h>
>  #include <linux/fbcon.h>
> -#include <linux/console.h>
> -#include <linux/module.h>
>  
>  #define FB_SYSFS_FLAG_ATTR 1
>  
> -/**
> - * framebuffer_alloc - creates a new frame buffer info structure
> - *
> - * @size: size of driver private data, can be zero
> - * @dev: pointer to the device for this fb, this can be NULL
> - *
> - * Creates a new frame buffer info structure. Also reserves @size bytes
> - * for driver private data (info->par). info->par (if any) will be
> - * aligned to sizeof(long).
> - *
> - * Returns the new structure, or NULL if an error occurred.
> - *
> - */
> -struct fb_info *framebuffer_alloc(size_t size, struct device *dev)
> -{
> -#define BYTES_PER_LONG (BITS_PER_LONG/8)
> -#define PADDING (BYTES_PER_LONG - (sizeof(struct fb_info) % BYTES_PER_LONG))
> -	int fb_info_size = sizeof(struct fb_info);
> -	struct fb_info *info;
> -	char *p;
> -
> -	if (size)
> -		fb_info_size += PADDING;
> -
> -	p = kzalloc(fb_info_size + size, GFP_KERNEL);
> -
> -	if (!p)
> -		return NULL;
> -
> -	info = (struct fb_info *) p;
> -
> -	if (size)
> -		info->par = p + fb_info_size;
> -
> -	info->device = dev;
> -	info->fbcon_rotate_hint = -1;
> -
> -#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
> -	mutex_init(&info->bl_curve_mutex);
> -#endif
> -
> -	return info;
> -#undef PADDING
> -#undef BYTES_PER_LONG
> -}
> -EXPORT_SYMBOL(framebuffer_alloc);
> -
> -/**
> - * framebuffer_release - marks the structure available for freeing
> - *
> - * @info: frame buffer info structure
> - *
> - * Drop the reference count of the device embedded in the
> - * framebuffer info structure.
> - *
> - */
> -void framebuffer_release(struct fb_info *info)
> -{
> -	if (!info)
> -		return;
> -
> -	if (WARN_ON(refcount_read(&info->count)))
> -		return;
> -
> -#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
> -	mutex_destroy(&info->bl_curve_mutex);
> -#endif
> -
> -	kfree(info);
> -}
> -EXPORT_SYMBOL(framebuffer_release);
> -
>  static int activate(struct fb_info *fb_info, struct fb_var_screeninfo *var)
>  {
>  	int err;
> @@ -551,30 +470,3 @@ void fb_cleanup_device(struct fb_info *fb_info)
>  		fb_info->class_flag &= ~FB_SYSFS_FLAG_ATTR;
>  	}
>  }
> -
> -#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
> -/* This function generates a linear backlight curve
> - *
> - *     0: off
> - *   1-7: min
> - * 8-127: linear from min to max
> - */
> -void fb_bl_default_curve(struct fb_info *fb_info, u8 off, u8 min, u8 max)
> -{
> -	unsigned int i, flat, count, range = (max - min);
> -
> -	mutex_lock(&fb_info->bl_curve_mutex);
> -
> -	fb_info->bl_curve[0] = off;
> -
> -	for (flat = 1; flat < (FB_BACKLIGHT_LEVELS / 16); ++flat)
> -		fb_info->bl_curve[flat] = min;
> -
> -	count = FB_BACKLIGHT_LEVELS * 15 / 16;
> -	for (i = 0; i < count; ++i)
> -		fb_info->bl_curve[flat + i] = min + (range * (i + 1) / count);
> -
> -	mutex_unlock(&fb_info->bl_curve_mutex);
> -}
> -EXPORT_SYMBOL_GPL(fb_bl_default_curve);
> -#endif
> -- 
> 2.40.1