Thread (14 messages) 14 messages, 4 authors, 2024-12-11

Re: [PATCH 1/3] fbdev: Fix recursive dependencies wrt BACKLIGHT_CLASS_DEVICE

From: Helge Deller <deller@gmx.de>
Date: 2024-12-11 00:04:50
Also in: dri-devel, linux-fbdev, linux-staging

On 12/11/24 00:53, Helge Deller wrote:
quoted hunk ↗ jump to hunk
On 12/11/24 00:37, Helge Deller wrote:
quoted
On 12/10/24 16:41, Thomas Zimmermann wrote:
quoted
Hi


Am 10.12.24 um 15:34 schrieb Helge Deller:
quoted
On 12/10/24 15:29, Helge Deller wrote:
quoted
On 12/10/24 15:09, Thomas Zimmermann wrote:
quoted
diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig
index 77ab44362f16..577e91ff7bf6 100644
--- a/drivers/staging/fbtft/Kconfig
+++ b/drivers/staging/fbtft/Kconfig
@@ -3,6 +3,7 @@ menuconfig FB_TFT
      tristate "Support for small TFT LCD display modules"
      depends on FB && SPI
      depends on FB_DEVICE
+    depends on BACKLIGHT_DEVICE_CLASS
Typo. Should be BACKLIGHT_CLASS_DEVICE...
Ah, thanks. I'll better check the rest of the series for similar mistakes.
quoted
Beside the typo:
In this case, doesn't it make sense to "select BACKLIGHT_DEVICE_CLASS" instead?
That causes the dependency error mentioned in the commit message. This time it's just for fbtft instead of shmobilefb.
quoted
If people want the fbtft, backlight support should be enabled too.
As a user-visible option, it should not be auto-selected
unnecessarily.
Right, it should not be auto-selected.
Unless if fbtft really needs it enabled to function.
IMHO all fb/drm drivers have higher priority than some low-level
background backlight controller code.
quoted
The DRM panel drivers already depend on the backlight
instead of selecting it. It's the correct approach.
Sounds wrong IMHO.
quoted
As I mentioned
in the cover letter, the few remaining driver that select it should
probably be updated.
That dependency sounds weird, but maybe I simply misunderstand your logic...?

As a Linux end user I usually know which graphic cards are in my machine
and which ones I want to enable.
But as a normal user I think I shouldn't be expected to know
that I first need to enable the "backlight class device"
so that I'm then able to afterwards enable the fbtft (or any other drm/fb driver).

Am I wrong?
Looking closer on this...
You propose:
--- a/drivers/staging/fbtft/Kconfig
+++ b/drivers/staging/fbtft/Kconfig
@@ -3,6 +3,7 @@ menuconfig FB_TFT
      tristate "Support for small TFT LCD display modules"
      depends on FB && SPI
      depends on FB_DEVICE
+    depends on BACKLIGHT_DEVICE_CLASS
      depends on GPIOLIB || COMPILE_TEST
      select FB_BACKLIGHT
So, it will depend on BACKLIGHT_DEVICE_CLASS.
But there is "select FB_BACKLIGHT" as well, which is:
config FB_BACKLIGHT
         tristate
         depends on FB
         select BACKLIGHT_CLASS_DEVICE

So, you end up with selecting and depending on BACKLIGHT_CLASS_DEVICE ?
Ok. Ignore this ^^^ . I now understand your cover letter.

Looking at your DRM tiny drivers and the i915/gma500 DRM drivers,
there is a "select BACKLIGHT_CLASS_DEVICE" in those.

So, isn't the right approach then something like:
--- a/drivers/staging/fbtft/Kconfig
        tristate "Support for small TFT LCD display modules"
        depends on FB && SPI
        depends on FB_DEVICE
   +    select BACKLIGHT_DEVICE_CLASS
        depends on GPIOLIB || COMPILE_TEST
        select FB_BACKLIGHT
config FB_BACKLIGHT
           tristate
           depends on FB
   +       depends on BACKLIGHT_CLASS_DEVICE

?
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help