Re: [PATCH 25/30] fbdev/core: Move framebuffer and backlight helpers into separate files
Hi Sam,
thanks for reviewing.
Am 07.06.23 um 21:38 schrieb Sam Ravnborg:
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
Some nits that you decide what to do with.
Reviewed-by: Sam Ravnborg
I do not get why they are moved out in separate files.
I guess the picture will materialize later.
In patch 30/30, sysfs support will be built conditionally. Doing this in
the Makefile is so much nicer than having an ifdef conditional in the
source files.
I'd also argue that the backlight and framebuffer functions don't belong
next to the sysfs code.
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 ..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.
No change here. The backlight function comes from fbsysfs.c, which is
also GPL-2.0-or-later.
+
+#include
+#include
#include - to avoid relying on indirect includes?
I can do that.
+
+#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(_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(_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 ..fb5b75009ee7
--- /dev/null
+++ b/drivers/video/fbdev/core/fb_info.c
@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include
+#include
Same as above, consider including mutex.h
Also slab.h
Ok.
Best regards
Thomas
+
+/**
+ * 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(>bl_curve_mutex);
+#endif
+
+ return info;
Re: [PATCH 25/30] fbdev/core: Move framebuffer and backlight helpers into separate files
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
Some nits that you decide what to do with.
Reviewed-by: Sam Ravnborg
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 ..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
> +#include
#include - 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(_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(_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 ..fb5b75009ee7
> --- /dev/null
> +++ b/drivers/video/fbdev/core/fb_info.c
> @@ -0,0 +1,76 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include
> +#include
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(>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
[PATCH 25/30] fbdev/core: Move framebuffer and backlight helpers into separate files
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
---
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
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 ..feffe6c68039
--- /dev/null
+++ b/drivers/video/fbdev/core/fb_backlight.c
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include
+#include
+
+#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(_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(_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 ..fb5b75009ee7
--- /dev/null
+++ b/drivers/video/fbdev/core/fb_info.c
@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include
+#include
+
+/**
+ * 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(>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(>count)))
+ return;
+
+#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
+ mutex_destroy(>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
*/
-/*
- * 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
