* [PATCH v2 10/11] leds: backlight trigger: Replace fb events with a dedicated function call
From: Thomas Zimmermann @ 2025-02-26 9:31 UTC (permalink / raw)
To: lee, pavel, danielt, jingoohan1, deller, simona
Cc: linux-leds, dri-devel, linux-fbdev, Thomas Zimmermann
In-Reply-To: <20250226093456.147402-1-tzimmermann@suse.de>
Remove support for fb events from the led backlight trigger. Provide
the helper ledtrig_backlight_blank() instead. Call it from fbdev to
inform the trigger of changes to a display's blank state.
Fbdev maintains a list of all installed notifiers. Instead of the fbdev
notifiers, maintain an internal list of led backlight triggers.
v2:
- maintain global list of led backlight triggers (Lee)
- avoid IS_REACHABLE() in source file (Lee)
- notify on changes to blank state instead of display state
- use lock guards
- initialize led list and list mutex
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/leds/trigger/ledtrig-backlight.c | 41 +++++++++---------------
drivers/video/fbdev/core/fbmem.c | 19 +++++------
include/linux/leds.h | 6 ++++
3 files changed, 32 insertions(+), 34 deletions(-)
diff --git a/drivers/leds/trigger/ledtrig-backlight.c b/drivers/leds/trigger/ledtrig-backlight.c
index 8e66d55a6c82..2d351de81927 100644
--- a/drivers/leds/trigger/ledtrig-backlight.c
+++ b/drivers/leds/trigger/ledtrig-backlight.c
@@ -10,7 +10,6 @@
#include <linux/kernel.h>
#include <linux/slab.h>
#include <linux/init.h>
-#include <linux/fb.h>
#include <linux/leds.h>
#include "../leds.h"
@@ -21,10 +20,14 @@ struct bl_trig_notifier {
struct led_classdev *led;
int brightness;
int old_status;
- struct notifier_block notifier;
unsigned invert;
+
+ struct list_head entry;
};
+static DEFINE_MUTEX(ledtrig_backlight_list_mutex);
+static LIST_HEAD(ledtrig_backlight_list);
+
static void ledtrig_backlight_notify_blank(struct bl_trig_notifier *n, int new_status)
{
struct led_classdev *led = n->led;
@@ -42,25 +45,15 @@ static void ledtrig_backlight_notify_blank(struct bl_trig_notifier *n, int new_s
n->old_status = new_status;
}
-static int fb_notifier_callback(struct notifier_block *p,
- unsigned long event, void *data)
+void ledtrig_backlight_blank(bool blank)
{
- struct bl_trig_notifier *n = container_of(p,
- struct bl_trig_notifier, notifier);
- struct fb_event *fb_event = data;
- int *blank;
- int new_status;
-
- /* If we aren't interested in this event, skip it immediately ... */
- if (event != FB_EVENT_BLANK)
- return 0;
-
- blank = fb_event->data;
- new_status = *blank ? BLANK : UNBLANK;
+ struct bl_trig_notifier *n;
+ int new_status = blank ? BLANK : UNBLANK;
- ledtrig_backlight_notify_blank(n, new_status);
+ guard(mutex)(&ledtrig_backlight_list_mutex);
- return 0;
+ list_for_each_entry(n, &ledtrig_backlight_list, entry)
+ ledtrig_backlight_notify_blank(n, new_status);
}
static ssize_t bl_trig_invert_show(struct device *dev,
@@ -106,8 +99,6 @@ ATTRIBUTE_GROUPS(bl_trig);
static int bl_trig_activate(struct led_classdev *led)
{
- int ret;
-
struct bl_trig_notifier *n;
n = kzalloc(sizeof(struct bl_trig_notifier), GFP_KERNEL);
@@ -118,11 +109,9 @@ static int bl_trig_activate(struct led_classdev *led)
n->led = led;
n->brightness = led->brightness;
n->old_status = UNBLANK;
- n->notifier.notifier_call = fb_notifier_callback;
- ret = fb_register_client(&n->notifier);
- if (ret)
- dev_err(led->dev, "unable to register backlight trigger\n");
+ guard(mutex)(&ledtrig_backlight_list_mutex);
+ list_add(&n->entry, &ledtrig_backlight_list);
return 0;
}
@@ -131,7 +120,9 @@ static void bl_trig_deactivate(struct led_classdev *led)
{
struct bl_trig_notifier *n = led_get_trigger_data(led);
- fb_unregister_client(&n->notifier);
+ guard(mutex)(&ledtrig_backlight_list_mutex);
+ list_del(&n->entry);
+
kfree(n);
}
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 001662c606d7..f089f72ec75a 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -16,6 +16,7 @@
#include <linux/fb.h>
#include <linux/fbcon.h>
#include <linux/lcd.h>
+#include <linux/leds.h>
#include <video/nomodeset.h>
@@ -369,11 +370,17 @@ static void fb_lcd_notify_blank(struct fb_info *info)
lcd_notify_blank_all(info->device, power);
}
+static void fb_ledtrig_backlight_notify_blank(struct fb_info *info)
+{
+ if (info->blank == FB_BLANK_UNBLANK)
+ ledtrig_backlight_blank(false);
+ else
+ ledtrig_backlight_blank(true);
+}
+
int fb_blank(struct fb_info *info, int blank)
{
int old_blank = info->blank;
- struct fb_event event;
- int data[2];
int ret;
if (!info->fbops->fb_blank)
@@ -382,11 +389,6 @@ int fb_blank(struct fb_info *info, int blank)
if (blank > FB_BLANK_POWERDOWN)
blank = FB_BLANK_POWERDOWN;
- data[0] = blank;
- data[1] = old_blank;
- event.info = info;
- event.data = data;
-
info->blank = blank;
ret = info->fbops->fb_blank(blank, info);
@@ -395,8 +397,7 @@ int fb_blank(struct fb_info *info, int blank)
fb_bl_notify_blank(info, old_blank);
fb_lcd_notify_blank(info);
-
- fb_notifier_call_chain(FB_EVENT_BLANK, &event);
+ fb_ledtrig_backlight_notify_blank(info);
return 0;
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 98f9719c924c..b3f0aa081064 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -640,6 +640,12 @@ static inline void ledtrig_flash_ctrl(bool on) {}
static inline void ledtrig_torch_ctrl(bool on) {}
#endif
+#if IS_REACHABLE(CONFIG_LEDS_TRIGGER_BACKLIGHT)
+void ledtrig_backlight_blank(bool blank);
+#else
+static inline void ledtrig_backlight_blank(bool blank) {}
+#endif
+
/*
* Generic LED platform data for describing LED names and default triggers.
*/
--
2.48.1
^ permalink raw reply related
* Re: [PATCH 1/3] dummycon: only build module if there are users
From: Arnd Bergmann @ 2025-02-26 11:46 UTC (permalink / raw)
To: Thomas Zimmermann, Arnd Bergmann, Greg Kroah-Hartman,
Helge Deller
Cc: linux-fbdev, dri-devel, linux-kernel
In-Reply-To: <29ecc7c4-2887-4989-a1d3-aa76b44c0387@suse.de>
On Wed, Feb 26, 2025, at 09:16, Thomas Zimmermann wrote:
> Am 26.02.25 um 08:55 schrieb Arnd Bergmann:
> Here's another general question. vgacon and fbcon only seem usable with
> CONFIG_VT=y. Wouldn't it make sense to have them depend on CONFIG_VT=y?
> dummycon could then be implemented as part of the vt code, maybe even
> become a vt-internal thing. The console code is complex, so I'm probably
> missing something here?
I think in theory one may have a system use fbcon purely to get the
boot logo, but not actually support VT. I had also assumed there might
be a way to use fbcon as the console (i.e. printk) but not register
the tty, but it looks like the console code still requires vt.
After I looked at the vt and conswitchp code some more, I wonder
if we could go the other way and instead of integrating it more
make the conswitchp logic optional: most of the complexity here
deals with switching between text console and fbcon dynamically,
but having any text console support is getting very rare (vga
on alpha/mips/x86-32, newport on mips-ip22, sti on parisc).
If we do this, the conswitchp code could be merged with dummycon
in drivers/video/console, with the simpler alternative just
calling into fbcon functions. I'm not sure if we can already drop
vgacon from normal x86-64 distro configs, i.e. if there are cases
that are not already covered by any of efi-earlycon, efifb,
vga16fb, vesafb/uvesafb or a PCI DRM driver.
Arnd
^ permalink raw reply
* Re: [PATCH 1/3] dummycon: only build module if there are users
From: Javier Martinez Canillas @ 2025-02-26 12:05 UTC (permalink / raw)
To: Arnd Bergmann, Thomas Zimmermann, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller
Cc: linux-fbdev, dri-devel, linux-kernel
In-Reply-To: <79d35e3b-9a0d-41a5-ab07-797bfa1e19cb@app.fastmail.com>
"Arnd Bergmann" <arnd@arndb.de> writes:
Hello Arnd,
> On Wed, Feb 26, 2025, at 09:16, Thomas Zimmermann wrote:
>> Am 26.02.25 um 08:55 schrieb Arnd Bergmann:
>> Here's another general question. vgacon and fbcon only seem usable with
>> CONFIG_VT=y. Wouldn't it make sense to have them depend on CONFIG_VT=y?
>> dummycon could then be implemented as part of the vt code, maybe even
>> become a vt-internal thing. The console code is complex, so I'm probably
>> missing something here?
>
> I think in theory one may have a system use fbcon purely to get the
> boot logo, but not actually support VT. I had also assumed there might
> be a way to use fbcon as the console (i.e. printk) but not register
> the tty, but it looks like the console code still requires vt.
>
> After I looked at the vt and conswitchp code some more, I wonder
> if we could go the other way and instead of integrating it more
> make the conswitchp logic optional: most of the complexity here
> deals with switching between text console and fbcon dynamically,
> but having any text console support is getting very rare (vga
> on alpha/mips/x86-32, newport on mips-ip22, sti on parisc).
>
> If we do this, the conswitchp code could be merged with dummycon
This sounds like a much better approach indeed.
> in drivers/video/console, with the simpler alternative just
> calling into fbcon functions. I'm not sure if we can already drop
> vgacon from normal x86-64 distro configs, i.e. if there are cases
> that are not already covered by any of efi-earlycon, efifb,
> vga16fb, vesafb/uvesafb or a PCI DRM driver.
>
I believe vgacon is still useful for x86 with legacy BIOS, because
vesafb (and simpledrm) only works if the user defines a mode using
the vga= kernel cmdline parameter.
> Arnd
>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
^ permalink raw reply
* Re: [PATCH 1/3] dummycon: only build module if there are users
From: Arnd Bergmann @ 2025-02-26 13:18 UTC (permalink / raw)
To: Javier Martinez Canillas, Thomas Zimmermann, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller
Cc: linux-fbdev, dri-devel, linux-kernel
In-Reply-To: <87mse8zzb0.fsf@minerva.mail-host-address-is-not-set>
On Wed, Feb 26, 2025, at 13:05, Javier Martinez Canillas wrote:
> "Arnd Bergmann" <arnd@arndb.de> writes:
>> in drivers/video/console, with the simpler alternative just
>> calling into fbcon functions. I'm not sure if we can already drop
>> vgacon from normal x86-64 distro configs, i.e. if there are cases
>> that are not already covered by any of efi-earlycon, efifb,
>> vga16fb, vesafb/uvesafb or a PCI DRM driver.
>>
>
> I believe vgacon is still useful for x86 with legacy BIOS, because
> vesafb (and simpledrm) only works if the user defines a mode using
> the vga= kernel cmdline parameter.
Right, at the minimum, a configuration without vgacon would
have to pick a graphical mode in arch/x86/boot/video*.c if one
wasn't already set by grub.
Looking through the git history of that code, it seems that there
are lots of corner cases with weird 32-bit hardware.
Anything from the past 20 years is probably reasonably safe,
but there still needs to be a way to configure vgacon back in
to be safe.
Arnd
^ permalink raw reply
* [PATCH RFC] backlight: pwm_bl: Read back PWM period from provider
From: Abel Vesa @ 2025-02-26 15:31 UTC (permalink / raw)
To: Uwe Kleine-König, Lee Jones, Daniel Thompson, Jingoo Han,
Helge Deller
Cc: Bjorn Andersson, Konrad Dybcio, Johan Hovold, Sebastian Reichel,
linux-pwm, dri-devel, linux-arm-msm, linux-fbdev, linux-kernel,
Abel Vesa
The current implementation assumes that the PWM provider will be able to
meet the requested period, but that is not always the case. Some PWM
providers have limited HW configuration capabilities and can only
provide a period that is somewhat close to the requested one. This
simply means that the duty cycle requested might either be above the
PWM's maximum value or the 100% duty cycle is never reached.
This could be easily fixed if the pwm_apply*() API family would allow
overriding the period within the PWM state that's used for providing the
duty cycle. But that is currently not the case.
So easiest fix here is to read back the period from the PWM provider via
the provider's ->get_state() op, if implemented, which should provide the
best matched period. Do this on probe after the first ->pwm_apply() op has
been done, which will allow the provider to determine the best match
period based on available configuration knobs. From there on, the
backlight will use the best matched period, since the driver's internal
PWM state is now synced up with the one from provider.
Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
drivers/video/backlight/pwm_bl.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 237d3d3f3bb1a6d713c5f6ec3198af772bf1268c..71a3e9cd8844095e85c01b194d7466978f1ca78e 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -525,6 +525,17 @@ static int pwm_backlight_probe(struct platform_device *pdev)
goto err_alloc;
}
+ /*
+ * The actual period might differ from the requested one due to HW
+ * limitations, so sync up the period with one determined by the
+ * provider driver.
+ */
+ ret = pwm_get_state_hw(pb->pwm, &pb->pwm->state);
+ if (ret && ret != -EOPNOTSUPP) {
+ dev_err(&pdev->dev, "failed to get PWM HW state");
+ goto err_alloc;
+ }
+
memset(&props, 0, sizeof(struct backlight_properties));
if (data->levels) {
---
base-commit: 8433c776e1eb1371f5cd40b5fd3a61f9c7b7f3ad
change-id: 20250226-pwm-bl-read-back-period-from-hw-08226cc2f920
Best regards,
--
Abel Vesa <abel.vesa@linaro.org>
^ permalink raw reply related
* Re: [PATCH RFC] backlight: pwm_bl: Read back PWM period from provider
From: Uwe Kleine-König @ 2025-02-26 16:34 UTC (permalink / raw)
To: Abel Vesa
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Helge Deller,
Bjorn Andersson, Konrad Dybcio, Johan Hovold, Sebastian Reichel,
linux-pwm, dri-devel, linux-arm-msm, linux-fbdev, linux-kernel
In-Reply-To: <20250226-pwm-bl-read-back-period-from-hw-v1-1-ccd1df656b23@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 2554 bytes --]
Hello,
On Wed, Feb 26, 2025 at 05:31:08PM +0200, Abel Vesa wrote:
> The current implementation assumes that the PWM provider will be able to
> meet the requested period, but that is not always the case. Some PWM
> providers have limited HW configuration capabilities and can only
> provide a period that is somewhat close to the requested one. This
> simply means that the duty cycle requested might either be above the
> PWM's maximum value or the 100% duty cycle is never reached.
If you request a state with 100% relative duty cycle you should get 100%
unless the hardware cannot do that. Which PWM hardware are you using?
Which requests are you actually doing that don't match your expectation?
> This could be easily fixed if the pwm_apply*() API family would allow
> overriding the period within the PWM state that's used for providing the
> duty cycle. But that is currently not the case.
I don't understand what you mean here.
> So easiest fix here is to read back the period from the PWM provider via
> the provider's ->get_state() op, if implemented, which should provide the
> best matched period. Do this on probe after the first ->pwm_apply() op has
> been done, which will allow the provider to determine the best match
> period based on available configuration knobs. From there on, the
> backlight will use the best matched period, since the driver's internal
> PWM state is now synced up with the one from provider.
> [...]
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index 237d3d3f3bb1a6d713c5f6ec3198af772bf1268c..71a3e9cd8844095e85c01b194d7466978f1ca78e 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -525,6 +525,17 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> goto err_alloc;
> }
>
> + /*
> + * The actual period might differ from the requested one due to HW
> + * limitations, so sync up the period with one determined by the
> + * provider driver.
> + */
> + ret = pwm_get_state_hw(pb->pwm, &pb->pwm->state);
As a consumer you're not supposed to write to &pb->pwm->state. That's a
layer violation. Please call pwm_get_state_hw() with a struct pwm_state
that you own and save the relevant parts in your driver data.
> + if (ret && ret != -EOPNOTSUPP) {
> + dev_err(&pdev->dev, "failed to get PWM HW state");
> + goto err_alloc;
> + }
> +
> memset(&props, 0, sizeof(struct backlight_properties));
>
> if (data->levels) {
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 1/3] dummycon: only build module if there are users
From: Helge Deller @ 2025-02-26 16:35 UTC (permalink / raw)
To: Arnd Bergmann, Thomas Zimmermann, Arnd Bergmann,
Greg Kroah-Hartman
Cc: linux-fbdev, dri-devel, linux-kernel
In-Reply-To: <79d35e3b-9a0d-41a5-ab07-797bfa1e19cb@app.fastmail.com>
On 2/26/25 12:46, Arnd Bergmann wrote:
> On Wed, Feb 26, 2025, at 09:16, Thomas Zimmermann wrote:
>> Am 26.02.25 um 08:55 schrieb Arnd Bergmann:
>> Here's another general question. vgacon and fbcon only seem usable with
>> CONFIG_VT=y. Wouldn't it make sense to have them depend on CONFIG_VT=y?
>> dummycon could then be implemented as part of the vt code, maybe even
>> become a vt-internal thing. The console code is complex, so I'm probably
>> missing something here?
>
> I think in theory one may have a system use fbcon purely to get the
> boot logo, but not actually support VT. I had also assumed there might
> be a way to use fbcon as the console (i.e. printk) but not register
> the tty, but it looks like the console code still requires vt.
>
> After I looked at the vt and conswitchp code some more, I wonder
> if we could go the other way and instead of integrating it more
> make the conswitchp logic optional: most of the complexity here
> deals with switching between text console and fbcon dynamically,
> but having any text console support is getting very rare (vga
> on alpha/mips/x86-32, newport on mips-ip22, sti on parisc).
Yes, it's rare. But on parisc, if no supported fbdev or drm
graphic card is found, it needs to stays on sticon (which always works).
Otherwise - if a card was found - the kernel switches dynamically to fbcon.
> If we do this, the conswitchp code could be merged with dummycon
> in drivers/video/console, with the simpler alternative just
> calling into fbcon functions.
As mentioned above, that should be optional then.
> I'm not sure if we can already drop
> vgacon from normal x86-64 distro configs, i.e. if there are cases
> that are not already covered by any of efi-earlycon, efifb,
> vga16fb, vesafb/uvesafb or a PCI DRM driver.
Helge
^ permalink raw reply
* Re: drm/fbdev-dma: regression
From: Nuno Gonçalves @ 2025-02-26 22:57 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: Thorsten Leemhuis, Linux kernel regressions list, dri-devel, LKML,
Linux Framebuffer
In-Reply-To: <844f1fa4-6f47-4386-878f-739d2819605e@suse.de>
Dear Thomas,
Could you check if the patch got lost in review?
I can confirm that mainline is still broken since this 2024/May regression.
Thanks,
Nuno
On Wed, Dec 11, 2024 at 9:07 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
>
> Am 09.12.24 um 14:56 schrieb Nuno Gonçalves:
> > On Mon, Dec 9, 2024 at 1:43 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >> Thanks you so much for testing. I'll prepare a real patch. Can I add
> >> your Reported-by and Tested-by tags?
> > Reported-by: Nuno Gonçalves <nunojpg@gmail.com>
> > Tested-by: Nuno Gonçalves <nunojpg@gmail.com>
>
> Thanks a lot. I've sent out the patch for review. Apologies if this took
> a bit longer than expected.
>
> Best regards
> Thomas
>
> >
> > Thanks,
> > Nuno
>
> --
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)
>
^ permalink raw reply
* Re: [PATCH RFC] backlight: pwm_bl: Read back PWM period from provider
From: Sebastian Reichel @ 2025-02-27 3:06 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Abel Vesa, Lee Jones, Daniel Thompson, Jingoo Han, Helge Deller,
Bjorn Andersson, Konrad Dybcio, Johan Hovold, linux-pwm,
dri-devel, linux-arm-msm, linux-fbdev, linux-kernel
In-Reply-To: <xltamao27utfrsax2pc6mh5tthanmrqszz4k7gyw3knqkm24xp@4tydmhfh6g4j>
[-- Attachment #1: Type: text/plain, Size: 3150 bytes --]
Hi,
On Wed, Feb 26, 2025 at 05:34:50PM +0100, Uwe Kleine-König wrote:
> On Wed, Feb 26, 2025 at 05:31:08PM +0200, Abel Vesa wrote:
> > The current implementation assumes that the PWM provider will be able to
> > meet the requested period, but that is not always the case. Some PWM
> > providers have limited HW configuration capabilities and can only
> > provide a period that is somewhat close to the requested one. This
> > simply means that the duty cycle requested might either be above the
> > PWM's maximum value or the 100% duty cycle is never reached.
>
> If you request a state with 100% relative duty cycle you should get 100%
> unless the hardware cannot do that. Which PWM hardware are you using?
> Which requests are you actually doing that don't match your expectation?
drivers/leds/rgb/leds-qcom-lpg.c (which probably should at least get
a MAINTAINERS entry to have you CC'd considering all the PWM bits in
it). See the following discussion (I point you to my message in the
middle of a thread, which has a summary and probably is a good
starting point):
https://lore.kernel.org/all/vc7irlp7nuy5yvkxwb5m7wy7j7jzgpg73zmajbmq2zjcd67pd2@cz2dcracta6w/
Greetings,
-- Sebastian
> > This could be easily fixed if the pwm_apply*() API family would allow
> > overriding the period within the PWM state that's used for providing the
> > duty cycle. But that is currently not the case.
>
> I don't understand what you mean here.
>
> > So easiest fix here is to read back the period from the PWM provider via
> > the provider's ->get_state() op, if implemented, which should provide the
> > best matched period. Do this on probe after the first ->pwm_apply() op has
> > been done, which will allow the provider to determine the best match
> > period based on available configuration knobs. From there on, the
> > backlight will use the best matched period, since the driver's internal
> > PWM state is now synced up with the one from provider.
> > [...]
> > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > index 237d3d3f3bb1a6d713c5f6ec3198af772bf1268c..71a3e9cd8844095e85c01b194d7466978f1ca78e 100644
> > --- a/drivers/video/backlight/pwm_bl.c
> > +++ b/drivers/video/backlight/pwm_bl.c
> > @@ -525,6 +525,17 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > goto err_alloc;
> > }
> >
> > + /*
> > + * The actual period might differ from the requested one due to HW
> > + * limitations, so sync up the period with one determined by the
> > + * provider driver.
> > + */
> > + ret = pwm_get_state_hw(pb->pwm, &pb->pwm->state);
>
> As a consumer you're not supposed to write to &pb->pwm->state. That's a
> layer violation. Please call pwm_get_state_hw() with a struct pwm_state
> that you own and save the relevant parts in your driver data.
>
> > + if (ret && ret != -EOPNOTSUPP) {
> > + dev_err(&pdev->dev, "failed to get PWM HW state");
> > + goto err_alloc;
> > + }
> > +
> > memset(&props, 0, sizeof(struct backlight_properties));
> >
> > if (data->levels) {
>
> Best regards
> Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: drm/fbdev-dma: regression
From: Thomas Zimmermann @ 2025-02-27 9:01 UTC (permalink / raw)
To: Nuno Gonçalves
Cc: Thorsten Leemhuis, Linux kernel regressions list, dri-devel, LKML,
Linux Framebuffer
In-Reply-To: <CAEXMXLQyOY=dXcYoSc9=LVVWb1BjXLd3_Lo3LRNor_STT+H+WQ@mail.gmail.com>
Hi
Am 26.02.25 um 23:57 schrieb Nuno Gonçalves:
> Dear Thomas,
>
> Could you check if the patch got lost in review?
>
> I can confirm that mainline is still broken since this 2024/May regression.
You're right. The patch got reviewed and then I forgot to merge it. It's
now on its way into the upstream kernel. Thanks for the note. Apologies
for the delay.
Best regards
Thomas
>
> Thanks,
> Nuno
>
> On Wed, Dec 11, 2024 at 9:07 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>>
>> Am 09.12.24 um 14:56 schrieb Nuno Gonçalves:
>>> On Mon, Dec 9, 2024 at 1:43 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>> Thanks you so much for testing. I'll prepare a real patch. Can I add
>>>> your Reported-by and Tested-by tags?
>>> Reported-by: Nuno Gonçalves <nunojpg@gmail.com>
>>> Tested-by: Nuno Gonçalves <nunojpg@gmail.com>
>> Thanks a lot. I've sent out the patch for review. Apologies if this took
>> a bit longer than expected.
>>
>> Best regards
>> Thomas
>>
>>> Thanks,
>>> Nuno
>> --
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Frankenstrasse 146, 90461 Nuernberg, Germany
>> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
>> HRB 36809 (AG Nuernberg)
>>
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
^ permalink raw reply
* Re: [PATCH 1/3] dummycon: only build module if there are users
From: Thomas Zimmermann @ 2025-02-27 9:25 UTC (permalink / raw)
To: Arnd Bergmann, Arnd Bergmann, Greg Kroah-Hartman, Helge Deller
Cc: linux-fbdev, dri-devel, linux-kernel
In-Reply-To: <79d35e3b-9a0d-41a5-ab07-797bfa1e19cb@app.fastmail.com>
Hi
Am 26.02.25 um 12:46 schrieb Arnd Bergmann:
> On Wed, Feb 26, 2025, at 09:16, Thomas Zimmermann wrote:
>> Am 26.02.25 um 08:55 schrieb Arnd Bergmann:
>> Here's another general question. vgacon and fbcon only seem usable with
>> CONFIG_VT=y. Wouldn't it make sense to have them depend on CONFIG_VT=y?
>> dummycon could then be implemented as part of the vt code, maybe even
>> become a vt-internal thing. The console code is complex, so I'm probably
>> missing something here?
> I think in theory one may have a system use fbcon purely to get the
> boot logo, but not actually support VT. I had also assumed there might
> be a way to use fbcon as the console (i.e. printk) but not register
> the tty, but it looks like the console code still requires vt.
>
> After I looked at the vt and conswitchp code some more, I wonder
> if we could go the other way and instead of integrating it more
> make the conswitchp logic optional: most of the complexity here
> deals with switching between text console and fbcon dynamically,
> but having any text console support is getting very rare (vga
> on alpha/mips/x86-32, newport on mips-ip22, sti on parisc).
>
> If we do this, the conswitchp code could be merged with dummycon
> in drivers/video/console, with the simpler alternative just
> calling into fbcon functions. I'm not sure if we can already drop
> vgacon from normal x86-64 distro configs, i.e. if there are cases
> that are not already covered by any of efi-earlycon, efifb,
> vga16fb, vesafb/uvesafb or a PCI DRM driver.
Thanks for the lengthy answer. I don't want to add work to your todo
list. For vgacon, I wouldn't remove it yet, as there still exisits
x86_64 systems with plain VGA support.
Best regards
Thomas
>
> Arnd
>
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
^ permalink raw reply
* Re: [PATCH v2 06/11] backlight: Replace fb events with a dedicated function call
From: kernel test robot @ 2025-02-27 11:42 UTC (permalink / raw)
To: Thomas Zimmermann, lee, pavel, danielt, jingoohan1, deller,
simona
Cc: oe-kbuild-all, linux-leds, dri-devel, linux-fbdev,
Thomas Zimmermann
In-Reply-To: <20250226093456.147402-7-tzimmermann@suse.de>
Hi Thomas,
kernel test robot noticed the following build errors:
[auto build test ERROR on lee-backlight/for-backlight-next]
[also build test ERROR on lee-leds/for-leds-next linus/master lee-backlight/for-backlight-fixes v6.14-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/fbdev-Rework-fb_blank/20250226-174013
base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git for-backlight-next
patch link: https://lore.kernel.org/r/20250226093456.147402-7-tzimmermann%40suse.de
patch subject: [PATCH v2 06/11] backlight: Replace fb events with a dedicated function call
config: arc-randconfig-002-20250227 (https://download.01.org/0day-ci/archive/20250227/202502271951.iaH6W6q1-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250227/202502271951.iaH6W6q1-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502271951.iaH6W6q1-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
In file included from drivers/video/cmdline.c:19:
>> include/linux/fb.h:767:6: warning: no previous prototype for 'fb_bl_notify_blank' [-Wmissing-prototypes]
767 | void fb_bl_notify_blank(struct fb_info *info, int old_blank)
| ^~~~~~~~~~~~~~~~~~
--
arc-elf-ld: drivers/video/cmdline.o: in function `fb_bl_notify_blank':
>> cmdline.c:(.text+0x9c): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/video/backlight/adp8860_bl.o: in function `fb_bl_notify_blank':
adp8860_bl.c:(.text+0xefc): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/video/backlight/as3711_bl.o: in function `fb_bl_notify_blank':
as3711_bl.c:(.text+0x85c): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/video/backlight/backlight.o: in function `fb_bl_notify_blank':
backlight.c:(.text+0xa34): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/video/backlight/da9052_bl.o: in function `fb_bl_notify_blank':
da9052_bl.c:(.text+0x20c): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/video/backlight/ktd253-backlight.o: in function `fb_bl_notify_blank':
ktd253-backlight.c:(.text+0x36c): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/video/backlight/lv5207lp.o: in function `fb_bl_notify_blank':
lv5207lp.c:(.text+0x224): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/video/backlight/max8925_bl.o: in function `fb_bl_notify_blank':
max8925_bl.c:(.text+0x3c8): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/video/backlight/mp3309c.o: in function `fb_bl_notify_blank':
mp3309c.c:(.text+0x730): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/video/backlight/pandora_bl.o: in function `fb_bl_notify_blank':
pandora_bl.c:(.text+0x294): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/video/backlight/pwm_bl.o: in function `fb_bl_notify_blank':
pwm_bl.c:(.text+0xd08): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/video/backlight/qcom-wled.o: in function `fb_bl_notify_blank':
qcom-wled.c:(.text+0x1f34): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/regulator/rpi-panel-attiny-regulator.o: in function `fb_bl_notify_blank':
rpi-panel-attiny-regulator.c:(.text+0x5f4): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/gpu/drm/drm_edid.o: in function `fb_bl_notify_blank':
include/linux/fb.h:768: multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/gpu/drm/drm_file.o: in function `fb_bl_notify_blank':
include/linux/fb.h:768: multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/gpu/drm/drm_modes.o: in function `fb_bl_notify_blank':
include/linux/fb.h:768: multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/gpu/drm/drm_panel.o: in function `fb_bl_notify_blank':
include/linux/fb.h:768: multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/gpu/drm/display/drm_dp_helper.o: in function `fb_bl_notify_blank':
include/linux/fb.h:768: multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.o: in function `fb_bl_notify_blank':
panel-asus-z00t-tm5p5-n35596.c:(.text+0x654): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/gpu/drm/panel/panel-jdi-lpm102a188a.o: in function `fb_bl_notify_blank':
panel-jdi-lpm102a188a.c:(.text+0x9c4): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/gpu/drm/panel/panel-lg-sw43408.o: in function `fb_bl_notify_blank':
panel-lg-sw43408.c:(.text+0x4ec): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/gpu/drm/panel/panel-novatek-nt35560.o: in function `fb_bl_notify_blank':
panel-novatek-nt35560.c:(.text+0x784): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/gpu/drm/panel/panel-raydium-rm69380.o: in function `fb_bl_notify_blank':
panel-raydium-rm69380.c:(.text+0x59c): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/gpu/drm/panel/panel-samsung-s6d7aa0.o: in function `fb_bl_notify_blank':
panel-samsung-s6d7aa0.c:(.text+0x92c): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/gpu/drm/panel/panel-samsung-sofef00.o: in function `fb_bl_notify_blank':
panel-samsung-sofef00.c:(.text+0x4f8): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.o: in function `fb_bl_notify_blank':
panel-sony-tulip-truly-nt35521.c:(.text+0xf38): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/gpu/drm/panel/panel-truly-nt35597.o: in function `fb_bl_notify_blank':
panel-truly-nt35597.c:(.text+0x7c8): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/gpu/drm/panel/panel-visionox-r66451.o: in function `fb_bl_notify_blank':
panel-visionox-r66451.c:(.text+0x704): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/gpu/drm/bridge/parade-ps8622.o: in function `fb_bl_notify_blank':
parade-ps8622.c:(.text+0x7e0): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/hid/hid-picolcd_core.o: in function `fb_bl_notify_blank':
hid-picolcd_core.c:(.text+0xca4): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/hid/hid-picolcd_backlight.o: in function `fb_bl_notify_blank':
include/linux/fb.h:768: multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/hid/hid-picolcd_debugfs.o: in function `fb_bl_notify_blank':
include/linux/fb.h:768: multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
arc-elf-ld: drivers/of/platform.o: in function `fb_bl_notify_blank':
platform.c:(.text+0xb10): multiple definition of `fb_bl_notify_blank'; drivers/video/aperture.o:include/linux/fb.h:768: first defined here
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v2 06/11] backlight: Replace fb events with a dedicated function call
From: kernel test robot @ 2025-02-27 12:46 UTC (permalink / raw)
To: Thomas Zimmermann, lee, pavel, danielt, jingoohan1, deller,
simona
Cc: llvm, oe-kbuild-all, linux-leds, dri-devel, linux-fbdev,
Thomas Zimmermann
In-Reply-To: <20250226093456.147402-7-tzimmermann@suse.de>
Hi Thomas,
kernel test robot noticed the following build errors:
[auto build test ERROR on lee-backlight/for-backlight-next]
[also build test ERROR on lee-leds/for-leds-next linus/master lee-backlight/for-backlight-fixes v6.14-rc4 next-20250227]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/fbdev-Rework-fb_blank/20250226-174013
base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git for-backlight-next
patch link: https://lore.kernel.org/r/20250226093456.147402-7-tzimmermann%40suse.de
patch subject: [PATCH v2 06/11] backlight: Replace fb events with a dedicated function call
config: arm-randconfig-002-20250227 (https://download.01.org/0day-ci/archive/20250227/202502272049.iWEcMOrk-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250227/202502272049.iWEcMOrk-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502272049.iWEcMOrk-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
In file included from arch/arm/mach-omap2/fb.c:16:
In file included from include/linux/omapfb.h:13:
In file included from include/uapi/linux/omapfb.h:28:
>> include/linux/fb.h:767:6: warning: no previous prototype for function 'fb_bl_notify_blank' [-Wmissing-prototypes]
767 | void fb_bl_notify_blank(struct fb_info *info, int old_blank)
| ^
include/linux/fb.h:767:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
767 | void fb_bl_notify_blank(struct fb_info *info, int old_blank)
| ^
| static
1 warning generated.
--
>> ld.lld: error: duplicate symbol: fb_bl_notify_blank
>>> defined at fb.h:768 (include/linux/fb.h:768)
>>> drivers/gpu/ipu-v3/ipu-common.o:(fb_bl_notify_blank)
>>> defined at fb.h:768 (include/linux/fb.h:768)
>>> drivers/gpu/ipu-v3/ipu-vdi.o:(.text+0x0)
--
>> ld.lld: error: duplicate symbol: fb_bl_notify_blank
>>> defined at fb.h:768 (include/linux/fb.h:768)
>>> drivers/gpu/ipu-v3/ipu-common.o:(fb_bl_notify_blank)
>>> defined at fb.h:768 (include/linux/fb.h:768)
>>> drivers/gpu/ipu-v3/ipu-smfc.o:(.text+0x0)
--
>> ld.lld: error: duplicate symbol: fb_bl_notify_blank
>>> defined at fb.h:768 (include/linux/fb.h:768)
>>> drivers/gpu/ipu-v3/ipu-common.o:(fb_bl_notify_blank)
>>> defined at fb.h:768 (include/linux/fb.h:768)
>>> drivers/gpu/ipu-v3/ipu-cpmem.o:(.text+0x0)
--
>> ld.lld: error: duplicate symbol: fb_bl_notify_blank
>>> defined at fb.h:768 (include/linux/fb.h:768)
>>> drivers/gpu/ipu-v3/ipu-common.o:(fb_bl_notify_blank)
>>> defined at fb.h:768 (include/linux/fb.h:768)
>>> drivers/gpu/ipu-v3/ipu-image-convert.o:(.text+0x0)
--
>> ld.lld: error: duplicate symbol: fb_bl_notify_blank
>>> defined at fb.h:768 (include/linux/fb.h:768)
>>> drivers/gpu/ipu-v3/ipu-common.o:(fb_bl_notify_blank)
>>> defined at fb.h:768 (include/linux/fb.h:768)
>>> drivers/gpu/ipu-v3/ipu-ic-csc.o:(.text+0x0)
--
>> ld.lld: error: duplicate symbol: fb_bl_notify_blank
>>> defined at fb.h:768 (include/linux/fb.h:768)
>>> drivers/gpu/ipu-v3/ipu-common.o:(fb_bl_notify_blank)
>>> defined at fb.h:768 (include/linux/fb.h:768)
>>> drivers/gpu/ipu-v3/ipu-ic.o:(.text+0x0)
--
>> ld.lld: error: duplicate symbol: fb_bl_notify_blank
>>> defined at fb.h:768 (include/linux/fb.h:768)
>>> drivers/gpu/ipu-v3/ipu-common.o:(fb_bl_notify_blank)
>>> defined at fb.h:768 (include/linux/fb.h:768)
>>> drivers/gpu/ipu-v3/ipu-dmfc.o:(.text+0x0)
--
>> ld.lld: error: duplicate symbol: fb_bl_notify_blank
>>> defined at fb.h:768 (include/linux/fb.h:768)
>>> drivers/gpu/ipu-v3/ipu-common.o:(fb_bl_notify_blank)
>>> defined at fb.h:768 (include/linux/fb.h:768)
>>> drivers/gpu/ipu-v3/ipu-dp.o:(.text+0x0)
--
>> ld.lld: error: duplicate symbol: fb_bl_notify_blank
>>> defined at fb.h:768 (include/linux/fb.h:768)
>>> drivers/gpu/ipu-v3/ipu-common.o:(fb_bl_notify_blank)
>>> defined at fb.h:768 (include/linux/fb.h:768)
>>> drivers/gpu/ipu-v3/ipu-di.o:(.text+0x0)
--
>> ld.lld: error: duplicate symbol: fb_bl_notify_blank
>>> defined at fb.h:768 (include/linux/fb.h:768)
>>> drivers/gpu/ipu-v3/ipu-common.o:(fb_bl_notify_blank)
>>> defined at fb.h:768 (include/linux/fb.h:768)
>>> drivers/gpu/ipu-v3/ipu-dc.o:(.text+0x0)
..
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH RFC] backlight: pwm_bl: Read back PWM period from provider
From: Abel Vesa @ 2025-02-27 13:07 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Helge Deller,
Bjorn Andersson, Konrad Dybcio, Johan Hovold, Sebastian Reichel,
linux-pwm, dri-devel, linux-arm-msm, linux-fbdev, linux-kernel
In-Reply-To: <xltamao27utfrsax2pc6mh5tthanmrqszz4k7gyw3knqkm24xp@4tydmhfh6g4j>
On 25-02-26 17:34:50, Uwe Kleine-König wrote:
> Hello,
>
> On Wed, Feb 26, 2025 at 05:31:08PM +0200, Abel Vesa wrote:
> > The current implementation assumes that the PWM provider will be able to
> > meet the requested period, but that is not always the case. Some PWM
> > providers have limited HW configuration capabilities and can only
> > provide a period that is somewhat close to the requested one. This
> > simply means that the duty cycle requested might either be above the
> > PWM's maximum value or the 100% duty cycle is never reached.
>
> If you request a state with 100% relative duty cycle you should get 100%
> unless the hardware cannot do that. Which PWM hardware are you using?
> Which requests are you actually doing that don't match your expectation?
The PWM hardware is Qualcomm PMK8550 PMIC. The way the duty cycle is
controlled is described in the following comment found in lpg_calc_freq
of the leds-qcom-lpg driver:
/*
* The PWM period is determined by:
*
* resolution * pre_div * 2^M
* period = --------------------------
* refclk
*
* Resolution = 2^9 bits for PWM or
* 2^{8, 9, 10, 11, 12, 13, 14, 15} bits for high resolution PWM
* pre_div = {1, 3, 5, 6} and
* M = [0..7].
*
* This allows for periods between 27uS and 384s for PWM channels and periods between
* 3uS and 24576s for high resolution PWMs.
* The PWM framework wants a period of equal or lower length than requested,
* reject anything below minimum period.
*/
So if we request a period of 5MHz, that will not ever be reached no matter what config
is used. Instead, the 4.26 MHz is selected as closest possible.
Now, the pwm_bl is not aware of this limitation and will request duty cycle values that
go above 4.26MHz.
>
> > This could be easily fixed if the pwm_apply*() API family would allow
> > overriding the period within the PWM state that's used for providing the
> > duty cycle. But that is currently not the case.
>
> I don't understand what you mean here.
What I was trying to say is that the PWM generic framework currently doesn't
allow overriding the PWM state's period with one provided by the consumer,
when calling pwm_apply_might_sleep().
Also, the pwm_get_state_hw() doesn't cache the state either.
This results in always having to call pwm_get_state_hw() before calling
pwm_apply_might_sleep().
On top of that, pwm_get_state_hw() doesn't default to the cached value if the
provider doesn't implement the ->get_state() op.
Please correct me if I'm wrong about these.
>
> > So easiest fix here is to read back the period from the PWM provider via
> > the provider's ->get_state() op, if implemented, which should provide the
> > best matched period. Do this on probe after the first ->pwm_apply() op has
> > been done, which will allow the provider to determine the best match
> > period based on available configuration knobs. From there on, the
> > backlight will use the best matched period, since the driver's internal
> > PWM state is now synced up with the one from provider.
> > [...]
> > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > index 237d3d3f3bb1a6d713c5f6ec3198af772bf1268c..71a3e9cd8844095e85c01b194d7466978f1ca78e 100644
> > --- a/drivers/video/backlight/pwm_bl.c
> > +++ b/drivers/video/backlight/pwm_bl.c
> > @@ -525,6 +525,17 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > goto err_alloc;
> > }
> >
> > + /*
> > + * The actual period might differ from the requested one due to HW
> > + * limitations, so sync up the period with one determined by the
> > + * provider driver.
> > + */
> > + ret = pwm_get_state_hw(pb->pwm, &pb->pwm->state);
>
> As a consumer you're not supposed to write to &pb->pwm->state. That's a
> layer violation. Please call pwm_get_state_hw() with a struct pwm_state
> that you own and save the relevant parts in your driver data.
Yep, that is indeed wrong. Maybe making the pwm opaque might be a good idea as well.
[1] Calling pwm_get_state_hw() would be wrong if the provider doesn't implement the ->get_state(),
as I mentioned above.
But are you suggesting we replace all calls to pwm_get_state() with
pwm_get_state_hw() in pwm_bl?
I can do that, but the concern from [1] still stands.
>
> > + if (ret && ret != -EOPNOTSUPP) {
> > + dev_err(&pdev->dev, "failed to get PWM HW state");
> > + goto err_alloc;
> > + }
> > +
> > memset(&props, 0, sizeof(struct backlight_properties));
> >
> > if (data->levels) {
>
> Best regards
> Uwe
Thanks for reviewing,
Abel
^ permalink raw reply
* Re: [PATCH v2 08/11] backlight: lcd: Replace fb events with a dedicated function call
From: kernel test robot @ 2025-02-27 15:07 UTC (permalink / raw)
To: Thomas Zimmermann, lee, pavel, danielt, jingoohan1, deller,
simona
Cc: oe-kbuild-all, linux-leds, dri-devel, linux-fbdev,
Thomas Zimmermann
In-Reply-To: <20250226093456.147402-9-tzimmermann@suse.de>
Hi Thomas,
kernel test robot noticed the following build errors:
[auto build test ERROR on lee-backlight/for-backlight-next]
[also build test ERROR on lee-leds/for-leds-next linus/master v6.14-rc4 next-20250227]
[cannot apply to lee-backlight/for-backlight-fixes]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/fbdev-Rework-fb_blank/20250226-174013
base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git for-backlight-next
patch link: https://lore.kernel.org/r/20250226093456.147402-9-tzimmermann%40suse.de
patch subject: [PATCH v2 08/11] backlight: lcd: Replace fb events with a dedicated function call
config: i386-buildonly-randconfig-002-20250227 (https://download.01.org/0day-ci/archive/20250227/202502272207.A1uo13Zf-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250227/202502272207.A1uo13Zf-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502272207.A1uo13Zf-lkp@intel.com/
All errors (new ones prefixed by >>, old ones prefixed by <<):
WARNING: modpost: missing MODULE_DESCRIPTION() in fs/exportfs/exportfs.o
>> ERROR: modpost: "lcd_notify_mode_change_all" [drivers/video/fbdev/core/fb.ko] undefined!
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH RFC] backlight: pwm_bl: Read back PWM period from provider
From: Uwe Kleine-König @ 2025-02-27 15:51 UTC (permalink / raw)
To: Abel Vesa
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Helge Deller,
Bjorn Andersson, Konrad Dybcio, Johan Hovold, Sebastian Reichel,
linux-pwm, dri-devel, linux-arm-msm, linux-fbdev, linux-kernel
In-Reply-To: <Z8BjiRjLin8jTE8j@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 5456 bytes --]
Hello Abel,
On Thu, Feb 27, 2025 at 03:07:21PM +0200, Abel Vesa wrote:
> On 25-02-26 17:34:50, Uwe Kleine-König wrote:
> > On Wed, Feb 26, 2025 at 05:31:08PM +0200, Abel Vesa wrote:
> > > The current implementation assumes that the PWM provider will be able to
> > > meet the requested period, but that is not always the case. Some PWM
> > > providers have limited HW configuration capabilities and can only
> > > provide a period that is somewhat close to the requested one. This
> > > simply means that the duty cycle requested might either be above the
> > > PWM's maximum value or the 100% duty cycle is never reached.
> >
> > If you request a state with 100% relative duty cycle you should get 100%
> > unless the hardware cannot do that. Which PWM hardware are you using?
> > Which requests are you actually doing that don't match your expectation?
>
> The PWM hardware is Qualcomm PMK8550 PMIC. The way the duty cycle is
> controlled is described in the following comment found in lpg_calc_freq
> of the leds-qcom-lpg driver:
>
> /*
> * The PWM period is determined by:
> *
> * resolution * pre_div * 2^M
> * period = --------------------------
> * refclk
> *
> * Resolution = 2^9 bits for PWM or
> * 2^{8, 9, 10, 11, 12, 13, 14, 15} bits for high resolution PWM
> * pre_div = {1, 3, 5, 6} and
> * M = [0..7].
> *
> * This allows for periods between 27uS and 384s for PWM channels and periods between
> * 3uS and 24576s for high resolution PWMs.
> * The PWM framework wants a period of equal or lower length than requested,
> * reject anything below minimum period.
> */
>
> So if we request a period of 5MHz, that will not ever be reached no matter what config
> is used. Instead, the 4.26 MHz is selected as closest possible.
The trace in the other mail thread suggest that you asked for a period
of 5 ms, not 5 MHz. And that results in a period of 4.26 ms.
> Now, the pwm_bl is not aware of this limitation and will request duty cycle values that
> go above 4.26MHz.
It requests .period = 5 ms + .duty_cycle = 5 ms. This is fine, and
according to the trace this results in both values becoming 4.26 ms in
real life. Seems fine to me.
> > > This could be easily fixed if the pwm_apply*() API family would allow
> > > overriding the period within the PWM state that's used for providing the
> > > duty cycle. But that is currently not the case.
> >
> > I don't understand what you mean here.
>
> What I was trying to say is that the PWM generic framework currently doesn't
> allow overriding the PWM state's period with one provided by the consumer,
> when calling pwm_apply_might_sleep().
Either I still don't understand what you want, or that is impossible or
useless. If you target .period = 5 ms and the hardware can only do 4.26
ms, why would you want to override period to 5 ms?
> Also, the pwm_get_state_hw() doesn't cache the state either.
*shrug*.
> This results in always having to call pwm_get_state_hw() before calling
> pwm_apply_might_sleep().
I cannot follow this conclusion. At least one of us two didn't
understand some detail yet.
> On top of that, pwm_get_state_hw() doesn't default to the cached value if the
> provider doesn't implement the ->get_state() op.
If it did that, the consumer wouldn't know if the request was
implemented exactly or if there is no way to read back the actual
configuration.
> Please correct me if I'm wrong about these.
>
> >
> > > So easiest fix here is to read back the period from the PWM provider via
> > > the provider's ->get_state() op, if implemented, which should provide the
> > > best matched period. Do this on probe after the first ->pwm_apply() op has
> > > been done, which will allow the provider to determine the best match
> > > period based on available configuration knobs. From there on, the
> > > backlight will use the best matched period, since the driver's internal
> > > PWM state is now synced up with the one from provider.
> > > [...]
> > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > index 237d3d3f3bb1a6d713c5f6ec3198af772bf1268c..71a3e9cd8844095e85c01b194d7466978f1ca78e 100644
> > > --- a/drivers/video/backlight/pwm_bl.c
> > > +++ b/drivers/video/backlight/pwm_bl.c
> > > @@ -525,6 +525,17 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > > goto err_alloc;
> > > }
> > >
> > > + /*
> > > + * The actual period might differ from the requested one due to HW
> > > + * limitations, so sync up the period with one determined by the
> > > + * provider driver.
> > > + */
> > > + ret = pwm_get_state_hw(pb->pwm, &pb->pwm->state);
> >
> > As a consumer you're not supposed to write to &pb->pwm->state. That's a
> > layer violation. Please call pwm_get_state_hw() with a struct pwm_state
> > that you own and save the relevant parts in your driver data.
>
> Yep, that is indeed wrong. Maybe making the pwm opaque might be a good idea as well.
>
> [1] Calling pwm_get_state_hw() would be wrong if the provider doesn't implement the ->get_state(),
> as I mentioned above.
>
> But are you suggesting we replace all calls to pwm_get_state() with
> pwm_get_state_hw() in pwm_bl?
No, I still didn't understand the problem you want to fix here. So I'm
not suggesting anything yet.
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH RFC] backlight: pwm_bl: Read back PWM period from provider
From: Abel Vesa @ 2025-02-27 16:50 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Helge Deller,
Bjorn Andersson, Konrad Dybcio, Johan Hovold, Sebastian Reichel,
linux-pwm, dri-devel, linux-arm-msm, linux-fbdev, linux-kernel
In-Reply-To: <rplq65h5k7kfu7anwhuh3w6lmwtm47lzeruofon4ilsxkhogjl@6k7nmeotjidd>
On 25-02-27 16:51:15, Uwe Kleine-König wrote:
> Hello Abel,
>
> On Thu, Feb 27, 2025 at 03:07:21PM +0200, Abel Vesa wrote:
> > On 25-02-26 17:34:50, Uwe Kleine-König wrote:
> > > On Wed, Feb 26, 2025 at 05:31:08PM +0200, Abel Vesa wrote:
> > > > The current implementation assumes that the PWM provider will be able to
> > > > meet the requested period, but that is not always the case. Some PWM
> > > > providers have limited HW configuration capabilities and can only
> > > > provide a period that is somewhat close to the requested one. This
> > > > simply means that the duty cycle requested might either be above the
> > > > PWM's maximum value or the 100% duty cycle is never reached.
> > >
> > > If you request a state with 100% relative duty cycle you should get 100%
> > > unless the hardware cannot do that. Which PWM hardware are you using?
> > > Which requests are you actually doing that don't match your expectation?
> >
> > The PWM hardware is Qualcomm PMK8550 PMIC. The way the duty cycle is
> > controlled is described in the following comment found in lpg_calc_freq
> > of the leds-qcom-lpg driver:
> >
> > /*
> > * The PWM period is determined by:
> > *
> > * resolution * pre_div * 2^M
> > * period = --------------------------
> > * refclk
> > *
> > * Resolution = 2^9 bits for PWM or
> > * 2^{8, 9, 10, 11, 12, 13, 14, 15} bits for high resolution PWM
> > * pre_div = {1, 3, 5, 6} and
> > * M = [0..7].
> > *
> > * This allows for periods between 27uS and 384s for PWM channels and periods between
> > * 3uS and 24576s for high resolution PWMs.
> > * The PWM framework wants a period of equal or lower length than requested,
> > * reject anything below minimum period.
> > */
> >
> > So if we request a period of 5MHz, that will not ever be reached no matter what config
> > is used. Instead, the 4.26 MHz is selected as closest possible.
>
> The trace in the other mail thread suggest that you asked for a period
> of 5 ms, not 5 MHz. And that results in a period of 4.26 ms.
OK. So unit is ms. Got it.
>
> > Now, the pwm_bl is not aware of this limitation and will request duty cycle values that
> > go above 4.26MHz.
>
> It requests .period = 5 ms + .duty_cycle = 5 ms. This is fine, and
> according to the trace this results in both values becoming 4.26 ms in
> real life. Seems fine to me.
Right, but as I keep trying to explain is that, the consumer keeps
asking for duty cycles that go over the 4.26ms, which is the period that
the provider decided it can do instead of 5ms.
>
> > > > This could be easily fixed if the pwm_apply*() API family would allow
> > > > overriding the period within the PWM state that's used for providing the
> > > > duty cycle. But that is currently not the case.
> > >
> > > I don't understand what you mean here.
> >
> > What I was trying to say is that the PWM generic framework currently doesn't
> > allow overriding the PWM state's period with one provided by the consumer,
> > when calling pwm_apply_might_sleep().
>
> Either I still don't understand what you want, or that is impossible or
> useless. If you target .period = 5 ms and the hardware can only do 4.26
> ms, why would you want to override period to 5 ms?
Meaning the consumer should become aware of the period the provider can
do before asking for a duty cycle.
If you look at the other mail thread, the trace there shows the
following sequence for every new backlight update request:
1. pwm_apply with consumer's period (5ms)
2. pwm_get reads the provider's period (4.25ms)
- which is what the provider is able to do instead of 5ms
3. pwm_apply (due to debug) which uses the state from 2.
4. pwm_get reads back exactly as 2.
So we can ignore 3 and 4 for now as they are there due to debug,
but the step 1 still requests a value over the 4.26ms (5ms),
which in the provider will translate to a pwm value higher than allowed
by the selected configuration.
>
> > Also, the pwm_get_state_hw() doesn't cache the state either.
>
> *shrug*.
>
> > This results in always having to call pwm_get_state_hw() before calling
> > pwm_apply_might_sleep().
>
> I cannot follow this conclusion. At least one of us two didn't
> understand some detail yet.
>
> > On top of that, pwm_get_state_hw() doesn't default to the cached value if the
> > provider doesn't implement the ->get_state() op.
>
> If it did that, the consumer wouldn't know if the request was
> implemented exactly or if there is no way to read back the actual
> configuration.
>
> > Please correct me if I'm wrong about these.
> >
> > >
> > > > So easiest fix here is to read back the period from the PWM provider via
> > > > the provider's ->get_state() op, if implemented, which should provide the
> > > > best matched period. Do this on probe after the first ->pwm_apply() op has
> > > > been done, which will allow the provider to determine the best match
> > > > period based on available configuration knobs. From there on, the
> > > > backlight will use the best matched period, since the driver's internal
> > > > PWM state is now synced up with the one from provider.
> > > > [...]
> > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > > index 237d3d3f3bb1a6d713c5f6ec3198af772bf1268c..71a3e9cd8844095e85c01b194d7466978f1ca78e 100644
> > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > @@ -525,6 +525,17 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > > > goto err_alloc;
> > > > }
> > > >
> > > > + /*
> > > > + * The actual period might differ from the requested one due to HW
> > > > + * limitations, so sync up the period with one determined by the
> > > > + * provider driver.
> > > > + */
> > > > + ret = pwm_get_state_hw(pb->pwm, &pb->pwm->state);
> > >
> > > As a consumer you're not supposed to write to &pb->pwm->state. That's a
> > > layer violation. Please call pwm_get_state_hw() with a struct pwm_state
> > > that you own and save the relevant parts in your driver data.
> >
> > Yep, that is indeed wrong. Maybe making the pwm opaque might be a good idea as well.
> >
> > [1] Calling pwm_get_state_hw() would be wrong if the provider doesn't implement the ->get_state(),
> > as I mentioned above.
> >
> > But are you suggesting we replace all calls to pwm_get_state() with
> > pwm_get_state_hw() in pwm_bl?
>
> No, I still didn't understand the problem you want to fix here. So I'm
> not suggesting anything yet.
>
> Best regards
> Uwe
^ permalink raw reply
* [PATCH v2] fbdev: pxafb: use devm_kmemdup*()
From: Raag Jadav @ 2025-02-28 7:10 UTC (permalink / raw)
To: deller, tzimmermann, andriy.shevchenko
Cc: linux-fbdev, linux-kernel, Raag Jadav
Convert to use devm_kmemdup() and devm_kmemdup_array() which are
more robust.
Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
This depends on changes available on immutable tag[1].
[1] https://lore.kernel.org/r/Z7xGpz3Q4Zj6YHx7@black.fi.intel.com
v2: Split patch series per subsystem
drivers/video/fbdev/pxafb.c | 23 +++++++++--------------
1 file changed, 9 insertions(+), 14 deletions(-)
diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c
index 4aa84853e31a..ee6da5084242 100644
--- a/drivers/video/fbdev/pxafb.c
+++ b/drivers/video/fbdev/pxafb.c
@@ -2233,32 +2233,27 @@ static int pxafb_probe(struct platform_device *dev)
{
struct pxafb_info *fbi;
struct pxafb_mach_info *inf, *pdata;
- int i, irq, ret;
+ int irq, ret;
dev_dbg(&dev->dev, "pxafb_probe\n");
ret = -ENOMEM;
pdata = dev_get_platdata(&dev->dev);
- inf = devm_kmalloc(&dev->dev, sizeof(*inf), GFP_KERNEL);
- if (!inf)
- goto failed;
-
if (pdata) {
- *inf = *pdata;
- inf->modes =
- devm_kmalloc_array(&dev->dev, pdata->num_modes,
- sizeof(inf->modes[0]), GFP_KERNEL);
+ inf = devm_kmemdup(&dev->dev, pdata, sizeof(*pdata), GFP_KERNEL);
+ if (!inf)
+ goto failed;
+
+ inf->modes = devm_kmemdup_array(&dev->dev, pdata->modes, pdata->num_modes,
+ sizeof(*pdata->modes), GFP_KERNEL);
if (!inf->modes)
goto failed;
- for (i = 0; i < inf->num_modes; i++)
- inf->modes[i] = pdata->modes[i];
} else {
inf = of_pxafb_of_mach_info(&dev->dev);
+ if (IS_ERR_OR_NULL(inf))
+ goto failed;
}
- if (IS_ERR_OR_NULL(inf))
- goto failed;
-
ret = pxafb_parse_options(&dev->dev, g_options, inf);
if (ret < 0)
goto failed;
base-commit: b8c38ccb2ca52b9a38cfeb9f89abab5d6e713221
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v3 2/2] mfd: lm3533: convert to use OF
From: Lee Jones @ 2025-02-28 8:59 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
Lars-Peter Clausen, Pavel Machek, Daniel Thompson, Jingoo Han,
Helge Deller, Andy Shevchenko, Uwe Kleine-König, devicetree,
linux-kernel, linux-iio, linux-leds, dri-devel, linux-fbdev
In-Reply-To: <20250224114815.146053-3-clamor95@gmail.com>
On Mon, 24 Feb 2025, Svyatoslav Ryhel wrote:
> Remove platform data and fully relay on OF and device tree
> parsing and binding devices.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
> drivers/iio/light/lm3533-als.c | 40 ++++---
> drivers/leds/leds-lm3533.c | 46 +++++---
> drivers/mfd/lm3533-core.c | 159 ++++++++--------------------
> drivers/video/backlight/lm3533_bl.c | 71 ++++++++++---
> include/linux/mfd/lm3533.h | 35 +-----
> 5 files changed, 164 insertions(+), 187 deletions(-)
>
> diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> index 99f0b903018c..cb52965e93c6 100644
> --- a/drivers/iio/light/lm3533-als.c
> +++ b/drivers/iio/light/lm3533-als.c
> @@ -16,9 +16,12 @@
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/mfd/core.h>
> +#include <linux/mod_devicetable.h>
> #include <linux/platform_device.h>
> +#include <linux/property.h>
> #include <linux/slab.h>
> #include <linux/uaccess.h>
> +#include <linux/units.h>
>
> #include <linux/mfd/lm3533.h>
>
> @@ -56,6 +59,9 @@ struct lm3533_als {
>
> atomic_t zone;
> struct mutex thresh_mutex;
> +
> + unsigned pwm_mode:1; /* PWM input mode (default analog) */
> + u8 r_select; /* 1 - 127 (ignored in PWM-mode) */
> };
>
>
> @@ -753,18 +759,17 @@ static int lm3533_als_set_resistor(struct lm3533_als *als, u8 val)
> return 0;
> }
>
> -static int lm3533_als_setup(struct lm3533_als *als,
> - const struct lm3533_als_platform_data *pdata)
> +static int lm3533_als_setup(struct lm3533_als *als)
> {
> int ret;
>
> - ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
> + ret = lm3533_als_set_input_mode(als, als->pwm_mode);
> if (ret)
> return ret;
>
> /* ALS input is always high impedance in PWM-mode. */
> - if (!pdata->pwm_mode) {
> - ret = lm3533_als_set_resistor(als, pdata->r_select);
> + if (!als->pwm_mode) {
> + ret = lm3533_als_set_resistor(als, als->r_select);
You're already passing 'als'.
Just teach lm3533_als_set_resistor that 'r_select' is now contained.
> if (ret)
> return ret;
> }
> @@ -828,22 +833,16 @@ static const struct iio_info lm3533_als_info = {
>
> static int lm3533_als_probe(struct platform_device *pdev)
> {
> - const struct lm3533_als_platform_data *pdata;
> struct lm3533 *lm3533;
> struct lm3533_als *als;
> struct iio_dev *indio_dev;
> + u32 val;
Value of what, potatoes?
> int ret;
>
> lm3533 = dev_get_drvdata(pdev->dev.parent);
> if (!lm3533)
> return -EINVAL;
>
> - pdata = dev_get_platdata(&pdev->dev);
> - if (!pdata) {
> - dev_err(&pdev->dev, "no platform data\n");
> - return -EINVAL;
> - }
> -
> indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*als));
> if (!indio_dev)
> return -ENOMEM;
> @@ -864,13 +863,21 @@ static int lm3533_als_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, indio_dev);
>
> + val = 200 * KILO; /* 200kOhm */
Better to #define magic numbers; DEFAULT_{DESCRIPTION}_OHMS
> + device_property_read_u32(&pdev->dev, "ti,resistor-value-ohm", &val);
> +
> + /* Convert resitance into R_ALS value with 2v / 10uA * R */
Because ...
> + als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * val);
> +
> + als->pwm_mode = device_property_read_bool(&pdev->dev, "ti,pwm-mode");
> +
> if (als->irq) {
> ret = lm3533_als_setup_irq(als, indio_dev);
> if (ret)
> return ret;
> }
>
> - ret = lm3533_als_setup(als, pdata);
> + ret = lm3533_als_setup(als);
> if (ret)
> goto err_free_irq;
>
> @@ -907,9 +914,16 @@ static void lm3533_als_remove(struct platform_device *pdev)
> free_irq(als->irq, indio_dev);
> }
>
> +static const struct of_device_id lm3533_als_match_table[] = {
> + { .compatible = "ti,lm3533-als" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, lm3533_als_match_table);
> +
> static struct platform_driver lm3533_als_driver = {
> .driver = {
> .name = "lm3533-als",
> + .of_match_table = lm3533_als_match_table,
> },
> .probe = lm3533_als_probe,
> .remove = lm3533_als_remove,
> diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
> index 45795f2a1042..6e661a2a540f 100644
> --- a/drivers/leds/leds-lm3533.c
> +++ b/drivers/leds/leds-lm3533.c
> @@ -10,8 +10,10 @@
> #include <linux/module.h>
> #include <linux/leds.h>
> #include <linux/mfd/core.h>
> +#include <linux/mod_devicetable.h>
> #include <linux/mutex.h>
> #include <linux/platform_device.h>
> +#include <linux/property.h>
> #include <linux/slab.h>
>
> #include <linux/mfd/lm3533.h>
> @@ -48,6 +50,9 @@ struct lm3533_led {
>
> struct mutex mutex;
> unsigned long flags;
> +
> + u16 max_current; /* 5000 - 29800 uA (800 uA step) */
> + u8 pwm; /* 0 - 0x3f */
This comment doesn't add anything.
> };
>
>
> @@ -632,23 +637,22 @@ static const struct attribute_group *lm3533_led_attribute_groups[] = {
> NULL
> };
>
> -static int lm3533_led_setup(struct lm3533_led *led,
> - struct lm3533_led_platform_data *pdata)
> +static int lm3533_led_setup(struct lm3533_led *led)
> {
> int ret;
>
> - ret = lm3533_ctrlbank_set_max_current(&led->cb, pdata->max_current);
> + ret = lm3533_ctrlbank_set_max_current(&led->cb, led->max_current);
Why not make max_current and attribute of lm3533_ctrlbank and drop the
redundant parameter?
> if (ret)
> return ret;
>
> - return lm3533_ctrlbank_set_pwm(&led->cb, pdata->pwm);
> + return lm3533_ctrlbank_set_pwm(&led->cb, led->pwm);
As above.
> }
>
> static int lm3533_led_probe(struct platform_device *pdev)
> {
> struct lm3533 *lm3533;
> - struct lm3533_led_platform_data *pdata;
> struct lm3533_led *led;
> + u32 val;
> int ret;
>
> dev_dbg(&pdev->dev, "%s\n", __func__);
> @@ -657,12 +661,6 @@ static int lm3533_led_probe(struct platform_device *pdev)
> if (!lm3533)
> return -EINVAL;
>
> - pdata = dev_get_platdata(&pdev->dev);
> - if (!pdata) {
> - dev_err(&pdev->dev, "no platform data\n");
> - return -EINVAL;
> - }
> -
> if (pdev->id < 0 || pdev->id >= LM3533_LVCTRLBANK_COUNT) {
> dev_err(&pdev->dev, "illegal LED id %d\n", pdev->id);
> return -EINVAL;
> @@ -673,8 +671,11 @@ static int lm3533_led_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> led->lm3533 = lm3533;
> - led->cdev.name = pdata->name;
> - led->cdev.default_trigger = pdata->default_trigger;
> + led->cdev.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-%d",
> + pdev->name, pdev->id);
> + led->cdev.default_trigger = "none";
> + device_property_read_string(&pdev->dev, "linux,default-trigger",
> + &led->cdev.default_trigger);
> led->cdev.brightness_set_blocking = lm3533_led_set;
> led->cdev.brightness_get = lm3533_led_get;
> led->cdev.blink_set = lm3533_led_blink_set;
> @@ -702,7 +703,17 @@ static int lm3533_led_probe(struct platform_device *pdev)
>
> led->cb.dev = led->cdev.dev;
>
> - ret = lm3533_led_setup(led, pdata);
> + /* 5000 - 29800 uA (800 uA step) */
> + val = 5000;
> + device_property_read_u32(&pdev->dev, "ti,max-current-microamp", &val);
> + led->max_current = val;
Why not just use 'led->max_current' from the offset and delete 'val'?
> +
> + /* 0 - 0x3f */
How does this improve readability? Why would use this info?
> + val = 0;
> + device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &val);
> + led->pwm = val;
> +
> + ret = lm3533_led_setup(led);
> if (ret)
> goto err_deregister;
>
> @@ -739,9 +750,16 @@ static void lm3533_led_shutdown(struct platform_device *pdev)
> lm3533_led_set(&led->cdev, LED_OFF); /* disable blink */
> }
>
> +static const struct of_device_id lm3533_led_match_table[] = {
> + { .compatible = "ti,lm3533-leds" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, lm3533_led_match_table);
> +
> static struct platform_driver lm3533_led_driver = {
> .driver = {
> .name = "lm3533-leds",
> + .of_match_table = lm3533_led_match_table,
> },
> .probe = lm3533_led_probe,
> .remove = lm3533_led_remove,
> diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
> index 0a2409d00b2e..e1b8fe136af9 100644
> --- a/drivers/mfd/lm3533-core.c
> +++ b/drivers/mfd/lm3533-core.c
> @@ -14,10 +14,13 @@
> #include <linux/gpio/consumer.h>
> #include <linux/i2c.h>
> #include <linux/mfd/core.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/property.h>
> #include <linux/regmap.h>
> #include <linux/seq_file.h>
> #include <linux/slab.h>
> #include <linux/uaccess.h>
> +#include <linux/units.h>
>
> #include <linux/mfd/lm3533.h>
>
> @@ -42,41 +45,41 @@
>
> #define LM3533_REG_MAX 0xb2
>
> -
> -static struct mfd_cell lm3533_als_devs[] = {
> +static struct mfd_cell lm3533_child_devices[] = {
Drop the child_ part.
> {
> .name = "lm3533-als",
> .id = -1,
> + .of_compatible = "ti,lm3533-als",
> },
> -};
> -
> -static struct mfd_cell lm3533_bl_devs[] = {
> {
> .name = "lm3533-backlight",
> .id = 0,
> + .of_compatible = "ti,lm3533-backlight",
> },
> {
> .name = "lm3533-backlight",
> .id = 1,
> + .of_compatible = "ti,lm3533-backlight",
> },
> -};
> -
> -static struct mfd_cell lm3533_led_devs[] = {
> {
> .name = "lm3533-leds",
> .id = 0,
Do you know if these are actually used for anything?
Any reason to not just use PLATFORM_DEVID_AUTO?
> + .of_compatible = "ti,lm3533-leds",
> },
> {
> .name = "lm3533-leds",
> .id = 1,
> + .of_compatible = "ti,lm3533-leds",
> },
> {
> .name = "lm3533-leds",
> .id = 2,
> + .of_compatible = "ti,lm3533-leds",
> },
> {
> .name = "lm3533-leds",
> .id = 3,
> + .of_compatible = "ti,lm3533-leds",
> },
> };
>
> @@ -376,136 +379,60 @@ static struct attribute_group lm3533_attribute_group = {
> .attrs = lm3533_attributes
> };
>
> -static int lm3533_device_als_init(struct lm3533 *lm3533)
> -{
> - struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
> - int ret;
> -
> - if (!pdata->als)
> - return 0;
> -
> - lm3533_als_devs[0].platform_data = pdata->als;
> - lm3533_als_devs[0].pdata_size = sizeof(*pdata->als);
> -
> - ret = mfd_add_devices(lm3533->dev, 0, lm3533_als_devs, 1, NULL,
> - 0, NULL);
> - if (ret) {
> - dev_err(lm3533->dev, "failed to add ALS device\n");
> - return ret;
> - }
> -
> - lm3533->have_als = 1;
> -
> - return 0;
> -}
> -
> -static int lm3533_device_bl_init(struct lm3533 *lm3533)
> -{
> - struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
> - int i;
> - int ret;
> -
> - if (!pdata->backlights || pdata->num_backlights == 0)
> - return 0;
> -
> - if (pdata->num_backlights > ARRAY_SIZE(lm3533_bl_devs))
> - pdata->num_backlights = ARRAY_SIZE(lm3533_bl_devs);
> -
> - for (i = 0; i < pdata->num_backlights; ++i) {
> - lm3533_bl_devs[i].platform_data = &pdata->backlights[i];
> - lm3533_bl_devs[i].pdata_size = sizeof(pdata->backlights[i]);
> - }
> -
> - ret = mfd_add_devices(lm3533->dev, 0, lm3533_bl_devs,
> - pdata->num_backlights, NULL, 0, NULL);
> - if (ret) {
> - dev_err(lm3533->dev, "failed to add backlight devices\n");
> - return ret;
> - }
> -
> - lm3533->have_backlights = 1;
> -
> - return 0;
> -}
> -
> -static int lm3533_device_led_init(struct lm3533 *lm3533)
> -{
> - struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
> - int i;
> - int ret;
> -
> - if (!pdata->leds || pdata->num_leds == 0)
> - return 0;
> -
> - if (pdata->num_leds > ARRAY_SIZE(lm3533_led_devs))
> - pdata->num_leds = ARRAY_SIZE(lm3533_led_devs);
> -
> - for (i = 0; i < pdata->num_leds; ++i) {
> - lm3533_led_devs[i].platform_data = &pdata->leds[i];
> - lm3533_led_devs[i].pdata_size = sizeof(pdata->leds[i]);
> - }
> -
> - ret = mfd_add_devices(lm3533->dev, 0, lm3533_led_devs,
> - pdata->num_leds, NULL, 0, NULL);
> - if (ret) {
> - dev_err(lm3533->dev, "failed to add LED devices\n");
> - return ret;
> - }
> -
> - lm3533->have_leds = 1;
> -
> - return 0;
> -}
> -
> -static int lm3533_device_setup(struct lm3533 *lm3533,
> - struct lm3533_platform_data *pdata)
> +static int lm3533_device_setup(struct lm3533 *lm3533)
> {
> int ret;
>
> - ret = lm3533_set_boost_freq(lm3533, pdata->boost_freq);
> + ret = lm3533_set_boost_freq(lm3533, lm3533->boost_freq);
Same comments as before.
Teach lm3533_set_boost_freq() that 'boost_freq' is contained in 'lm3533'.
> if (ret)
> return ret;
>
> - return lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
> + return lm3533_set_boost_ovp(lm3533, lm3533->boost_ovp);
> }
>
> static int lm3533_device_init(struct lm3533 *lm3533)
> {
> - struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
> + u32 val;
'uV' and 'hz' could be easier to follow.
> int ret;
>
> - dev_dbg(lm3533->dev, "%s\n", __func__);
> + lm3533->hwen = devm_gpiod_get(lm3533->dev, "enable", GPIOD_OUT_LOW);
> + if (IS_ERR(lm3533->hwen))
> + return dev_err_probe(lm3533->dev, PTR_ERR(lm3533->hwen),
> + "failed to request HWEN GPIO\n");
>
> - if (!pdata) {
> - dev_err(lm3533->dev, "no platform data\n");
> - return -EINVAL;
> - }
> + val = 16 * MICRO; /* 16V */
> + device_property_read_u32(lm3533->dev, "ti,boost-ovp-microvolt", &val);
>
> - lm3533->hwen = devm_gpiod_get(lm3533->dev, NULL, GPIOD_OUT_LOW);
> - if (IS_ERR(lm3533->hwen))
> - return dev_err_probe(lm3533->dev, PTR_ERR(lm3533->hwen), "failed to request HWEN GPIO\n");
> - gpiod_set_consumer_name(lm3533->hwen, "lm3533-hwen");
> + /* boost_ovp is defined in microvolts, convert to enum value */
> + lm3533->boost_ovp = val / (8 * MICRO) - 2;
Wait, what. Why?
Converting a useful microvolt value to an arbitrary enum sounds fragile.
> +
> + val = 500 * KILO; /* 500kHz */
> + device_property_read_u32(lm3533->dev, "ti,boost-freq-hz", &val);
> +
> + /* boost_freq is defined in Hz, convert to enum value */
> + lm3533->boost_freq = val / (500 * KILO) - 1;
>
> lm3533_enable(lm3533);
>
> - ret = lm3533_device_setup(lm3533, pdata);
> + ret = lm3533_device_setup(lm3533);
> if (ret)
> goto err_disable;
>
> - lm3533_device_als_init(lm3533);
> - lm3533_device_bl_init(lm3533);
> - lm3533_device_led_init(lm3533);
> + ret = devm_mfd_add_devices(lm3533->dev, 0, lm3533_child_devices,
> + ARRAY_SIZE(lm3533_child_devices), NULL, 0, NULL);
> + if (ret) {
> + dev_err(lm3533->dev, "failed to add MFD devices: %d\n", ret);
"child devices" or "sub-devices".
> + goto err_disable;
> + }
>
> ret = sysfs_create_group(&lm3533->dev->kobj, &lm3533_attribute_group);
> if (ret < 0) {
> dev_err(lm3533->dev, "failed to create sysfs attributes\n");
> - goto err_unregister;
> + goto err_disable;
> }
>
> return 0;
>
> -err_unregister:
> - mfd_remove_devices(lm3533->dev);
> err_disable:
> lm3533_disable(lm3533);
>
> @@ -517,8 +444,6 @@ static void lm3533_device_exit(struct lm3533 *lm3533)
> dev_dbg(lm3533->dev, "%s\n", __func__);
>
> sysfs_remove_group(&lm3533->dev->kobj, &lm3533_attribute_group);
> -
> - mfd_remove_devices(lm3533->dev);
> lm3533_disable(lm3533);
> }
>
> @@ -591,6 +516,9 @@ static int lm3533_i2c_probe(struct i2c_client *i2c)
> lm3533->dev = &i2c->dev;
> lm3533->irq = i2c->irq;
>
> + i2c->dev.coherent_dma_mask = 0;
> + i2c->dev.dma_mask = &i2c->dev.coherent_dma_mask;
Why are you manually doing this?
The core should take care of this for you:
drivers/mfd/mfd-core.c: pdev->dev.dma_mask = parent->dma_mask;
drivers/mfd/mfd-core.c: pdev->dev.coherent_dma_mask = parent->coherent_dma_mask;
> +
> return lm3533_device_init(lm3533);
> }
>
> @@ -603,6 +531,12 @@ static void lm3533_i2c_remove(struct i2c_client *i2c)
> lm3533_device_exit(lm3533);
> }
>
> +static const struct of_device_id lm3533_match_table[] = {
> + { .compatible = "ti,lm3533" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, lm3533_match_table);
> +
> static const struct i2c_device_id lm3533_i2c_ids[] = {
> { "lm3533" },
> { }
> @@ -612,6 +546,7 @@ MODULE_DEVICE_TABLE(i2c, lm3533_i2c_ids);
> static struct i2c_driver lm3533_i2c_driver = {
> .driver = {
> .name = "lm3533",
> + .of_match_table = lm3533_match_table,
> },
> .id_table = lm3533_i2c_ids,
> .probe = lm3533_i2c_probe,
> diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
> index babfd3ceec86..0827a5e98dbb 100644
> --- a/drivers/video/backlight/lm3533_bl.c
> +++ b/drivers/video/backlight/lm3533_bl.c
> @@ -9,7 +9,9 @@
>
> #include <linux/module.h>
> #include <linux/init.h>
> +#include <linux/mod_devicetable.h>
> #include <linux/platform_device.h>
> +#include <linux/property.h>
> #include <linux/backlight.h>
> #include <linux/slab.h>
>
> @@ -19,6 +21,7 @@
> #define LM3533_HVCTRLBANK_COUNT 2
> #define LM3533_BL_MAX_BRIGHTNESS 255
>
> +#define LM3533_REG_OUTPUT_CONF1 0x10
> #define LM3533_REG_CTRLBANK_AB_BCONF 0x1a
>
>
> @@ -27,6 +30,11 @@ struct lm3533_bl {
> struct lm3533_ctrlbank cb;
> struct backlight_device *bd;
> int id;
> +
> + u16 max_current; /* 5000 - 29800 uA (800 uA step) */
> + u8 pwm; /* 0 - 0x3f */
Remove or improve this comment.
> + bool linear;
> + bool hvled;
> };
>
>
> @@ -246,25 +254,40 @@ static struct attribute_group lm3533_bl_attribute_group = {
> .attrs = lm3533_bl_attributes
> };
>
> -static int lm3533_bl_setup(struct lm3533_bl *bl,
> - struct lm3533_bl_platform_data *pdata)
> +static int lm3533_bl_setup(struct lm3533_bl *bl)
> {
> + int id = lm3533_bl_get_ctrlbank_id(bl);
> int ret;
>
> - ret = lm3533_ctrlbank_set_max_current(&bl->cb, pdata->max_current);
> + if (bl->linear) {
> + ret = lm3533_update(bl->lm3533, LM3533_REG_CTRLBANK_AB_BCONF,
> + BIT(2 * id + 1), BIT(2 * id + 1));
These need to be defined as SHIFT values or whatever they are.
> + if (ret)
> + return ret;
> + }
> +
> + if (bl->hvled) {
> + ret = lm3533_update(bl->lm3533, LM3533_REG_OUTPUT_CONF1,
> + id | id << 1, BIT(0) | BIT(1));
> + if (ret)
> + return ret;
> + }
> +
> + ret = lm3533_ctrlbank_set_max_current(&bl->cb, bl->max_current);
> if (ret)
> return ret;
>
> - return lm3533_ctrlbank_set_pwm(&bl->cb, pdata->pwm);
> + return lm3533_ctrlbank_set_pwm(&bl->cb, bl->pwm);
> }
>
> static int lm3533_bl_probe(struct platform_device *pdev)
> {
> struct lm3533 *lm3533;
> - struct lm3533_bl_platform_data *pdata;
> struct lm3533_bl *bl;
> struct backlight_device *bd;
> struct backlight_properties props;
> + char *name;
> + u32 val;
As above.
> int ret;
>
> dev_dbg(&pdev->dev, "%s\n", __func__);
> @@ -273,12 +296,6 @@ static int lm3533_bl_probe(struct platform_device *pdev)
> if (!lm3533)
> return -EINVAL;
>
> - pdata = dev_get_platdata(&pdev->dev);
> - if (!pdata) {
> - dev_err(&pdev->dev, "no platform data\n");
> - return -EINVAL;
> - }
> -
> if (pdev->id < 0 || pdev->id >= LM3533_HVCTRLBANK_COUNT) {
> dev_err(&pdev->dev, "illegal backlight id %d\n", pdev->id);
> return -EINVAL;
> @@ -295,13 +312,15 @@ static int lm3533_bl_probe(struct platform_device *pdev)
> bl->cb.id = lm3533_bl_get_ctrlbank_id(bl);
> bl->cb.dev = NULL; /* until registered */
>
> + name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-%d", pdev->name, pdev->id);
Doesn't platform already provide enumerated names?
> +
> memset(&props, 0, sizeof(props));
> props.type = BACKLIGHT_RAW;
> props.max_brightness = LM3533_BL_MAX_BRIGHTNESS;
> - props.brightness = pdata->default_brightness;
> - bd = devm_backlight_device_register(&pdev->dev, pdata->name,
> - pdev->dev.parent, bl, &lm3533_bl_ops,
> - &props);
> + props.brightness = LM3533_BL_MAX_BRIGHTNESS;
> + device_property_read_u32(&pdev->dev, "default-brightness", &props.brightness);
> + bd = devm_backlight_device_register(&pdev->dev, name, pdev->dev.parent,
> + bl, &lm3533_bl_ops, &props);
> if (IS_ERR(bd)) {
> dev_err(&pdev->dev, "failed to register backlight device\n");
> return PTR_ERR(bd);
> @@ -320,7 +339,20 @@ static int lm3533_bl_probe(struct platform_device *pdev)
>
> backlight_update_status(bd);
>
> - ret = lm3533_bl_setup(bl, pdata);
> + /* 5000 - 29800 uA (800 uA step) */
> + val = 5000;
> + device_property_read_u32(&pdev->dev, "ti,max-current-microamp", &val);
> + bl->max_current = val;
> +
> + /* 0 - 0x3f */
> + val = 0;
> + device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &val);
> + bl->pwm = val;
> +
> + bl->linear = device_property_read_bool(&pdev->dev, "ti,linear-mapping-mode");
> + bl->hvled = device_property_read_bool(&pdev->dev, "ti,hardware-controlled");
> +
> + ret = lm3533_bl_setup(bl);
> if (ret)
> goto err_sysfs_remove;
>
> @@ -381,10 +413,17 @@ static void lm3533_bl_shutdown(struct platform_device *pdev)
> lm3533_ctrlbank_disable(&bl->cb);
> }
>
> +static const struct of_device_id lm3533_bl_match_table[] = {
> + { .compatible = "ti,lm3533-backlight" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, lm3533_bl_match_table);
> +
> static struct platform_driver lm3533_bl_driver = {
> .driver = {
> .name = "lm3533-backlight",
> .pm = &lm3533_bl_pm_ops,
> + .of_match_table = lm3533_bl_match_table,
> },
> .probe = lm3533_bl_probe,
> .remove = lm3533_bl_remove,
> diff --git a/include/linux/mfd/lm3533.h b/include/linux/mfd/lm3533.h
> index 69059a7a2ce5..3b28fc0970f6 100644
> --- a/include/linux/mfd/lm3533.h
> +++ b/include/linux/mfd/lm3533.h
> @@ -27,6 +27,9 @@ struct lm3533 {
> struct gpio_desc *hwen;
> int irq;
>
> + u32 boost_ovp;
> + u32 boost_freq;
> +
> unsigned have_als:1;
> unsigned have_backlights:1;
> unsigned have_leds:1;
> @@ -38,25 +41,6 @@ struct lm3533_ctrlbank {
> int id;
> };
>
> -struct lm3533_als_platform_data {
> - unsigned pwm_mode:1; /* PWM input mode (default analog) */
> - u8 r_select; /* 1 - 127 (ignored in PWM-mode) */
> -};
> -
> -struct lm3533_bl_platform_data {
> - char *name;
> - u16 max_current; /* 5000 - 29800 uA (800 uA step) */
> - u8 default_brightness; /* 0 - 255 */
> - u8 pwm; /* 0 - 0x3f */
> -};
> -
> -struct lm3533_led_platform_data {
> - char *name;
> - const char *default_trigger;
> - u16 max_current; /* 5000 - 29800 uA (800 uA step) */
> - u8 pwm; /* 0 - 0x3f */
> -};
> -
> enum lm3533_boost_freq {
> LM3533_BOOST_FREQ_500KHZ,
> LM3533_BOOST_FREQ_1000KHZ,
> @@ -69,19 +53,6 @@ enum lm3533_boost_ovp {
> LM3533_BOOST_OVP_40V,
> };
>
> -struct lm3533_platform_data {
> - enum lm3533_boost_ovp boost_ovp;
> - enum lm3533_boost_freq boost_freq;
> -
> - struct lm3533_als_platform_data *als;
> -
> - struct lm3533_bl_platform_data *backlights;
> - int num_backlights;
> -
> - struct lm3533_led_platform_data *leds;
> - int num_leds;
> -};
> -
> extern int lm3533_ctrlbank_enable(struct lm3533_ctrlbank *cb);
> extern int lm3533_ctrlbank_disable(struct lm3533_ctrlbank *cb);
>
> --
> 2.43.0
>
--
Lee Jones [李琼斯]
^ permalink raw reply
* Re: [PATCH v3 2/2] mfd: lm3533: convert to use OF
From: Svyatoslav Ryhel @ 2025-02-28 9:30 UTC (permalink / raw)
To: Lee Jones
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
Lars-Peter Clausen, Pavel Machek, Daniel Thompson, Jingoo Han,
Helge Deller, Andy Shevchenko, Uwe Kleine-König, devicetree,
linux-kernel, linux-iio, linux-leds, dri-devel, linux-fbdev
In-Reply-To: <20250228085927.GM824852@google.com>
пт, 28 лют. 2025 р. о 10:59 Lee Jones <lee@kernel.org> пише:
>
> On Mon, 24 Feb 2025, Svyatoslav Ryhel wrote:
>
> > Remove platform data and fully relay on OF and device tree
> > parsing and binding devices.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> > drivers/iio/light/lm3533-als.c | 40 ++++---
> > drivers/leds/leds-lm3533.c | 46 +++++---
> > drivers/mfd/lm3533-core.c | 159 ++++++++--------------------
> > drivers/video/backlight/lm3533_bl.c | 71 ++++++++++---
> > include/linux/mfd/lm3533.h | 35 +-----
> > 5 files changed, 164 insertions(+), 187 deletions(-)
> >
> > diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> > index 99f0b903018c..cb52965e93c6 100644
> > --- a/drivers/iio/light/lm3533-als.c
> > +++ b/drivers/iio/light/lm3533-als.c
> > @@ -16,9 +16,12 @@
> > #include <linux/module.h>
> > #include <linux/mutex.h>
> > #include <linux/mfd/core.h>
> > +#include <linux/mod_devicetable.h>
> > #include <linux/platform_device.h>
> > +#include <linux/property.h>
> > #include <linux/slab.h>
> > #include <linux/uaccess.h>
> > +#include <linux/units.h>
> >
> > #include <linux/mfd/lm3533.h>
> >
> > @@ -56,6 +59,9 @@ struct lm3533_als {
> >
> > atomic_t zone;
> > struct mutex thresh_mutex;
> > +
> > + unsigned pwm_mode:1; /* PWM input mode (default analog) */
> > + u8 r_select; /* 1 - 127 (ignored in PWM-mode) */
> > };
> >
> >
> > @@ -753,18 +759,17 @@ static int lm3533_als_set_resistor(struct lm3533_als *als, u8 val)
> > return 0;
> > }
> >
> > -static int lm3533_als_setup(struct lm3533_als *als,
> > - const struct lm3533_als_platform_data *pdata)
> > +static int lm3533_als_setup(struct lm3533_als *als)
> > {
> > int ret;
> >
> > - ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
> > + ret = lm3533_als_set_input_mode(als, als->pwm_mode);
> > if (ret)
> > return ret;
> >
> > /* ALS input is always high impedance in PWM-mode. */
> > - if (!pdata->pwm_mode) {
> > - ret = lm3533_als_set_resistor(als, pdata->r_select);
> > + if (!als->pwm_mode) {
> > + ret = lm3533_als_set_resistor(als, als->r_select);
>
> You're already passing 'als'.
>
> Just teach lm3533_als_set_resistor that 'r_select' is now contained.
>
This is not scope of this patchset. I was already accused in too much
changes which make it unreadable. This patchset is dedicated to
swapping platform data to use of the device tree. NOT improving
functions, NOT rewriting arbitrary mechanics. If you feed a need for
this change, then propose a followup. I need from this driver only one
thing, that it could work with device tree. But it seems that it is
better that it just rots in the garbage bin until removed cause no one
cared.
> > if (ret)
> > return ret;
> > }
> > @@ -828,22 +833,16 @@ static const struct iio_info lm3533_als_info = {
> >
> > static int lm3533_als_probe(struct platform_device *pdev)
> > {
> > - const struct lm3533_als_platform_data *pdata;
> > struct lm3533 *lm3533;
> > struct lm3533_als *als;
> > struct iio_dev *indio_dev;
> > + u32 val;
>
> Value of what, potatoes?
>
Oranges.
> > int ret;
> >
> > lm3533 = dev_get_drvdata(pdev->dev.parent);
> > if (!lm3533)
> > return -EINVAL;
> >
> > - pdata = dev_get_platdata(&pdev->dev);
> > - if (!pdata) {
> > - dev_err(&pdev->dev, "no platform data\n");
> > - return -EINVAL;
> > - }
> > -
> > indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*als));
> > if (!indio_dev)
> > return -ENOMEM;
> > @@ -864,13 +863,21 @@ static int lm3533_als_probe(struct platform_device *pdev)
> >
> > platform_set_drvdata(pdev, indio_dev);
> >
> > + val = 200 * KILO; /* 200kOhm */
>
> Better to #define magic numbers; DEFAULT_{DESCRIPTION}_OHMS
>
Why? that is not needed.
> > + device_property_read_u32(&pdev->dev, "ti,resistor-value-ohm", &val);
> > +
> > + /* Convert resitance into R_ALS value with 2v / 10uA * R */
>
> Because ...
>
BACAUSE the device DOES NOT understand human readable values, only 0s
and 1s, hence mOhms must be converted into value lm3533 chip can
understand.
> > + als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * val);
> > +
> > + als->pwm_mode = device_property_read_bool(&pdev->dev, "ti,pwm-mode");
> > +
> > if (als->irq) {
> > ret = lm3533_als_setup_irq(als, indio_dev);
> > if (ret)
> > return ret;
> > }
> >
> > - ret = lm3533_als_setup(als, pdata);
> > + ret = lm3533_als_setup(als);
> > if (ret)
> > goto err_free_irq;
> >
> > @@ -907,9 +914,16 @@ static void lm3533_als_remove(struct platform_device *pdev)
> > free_irq(als->irq, indio_dev);
> > }
> >
> > +static const struct of_device_id lm3533_als_match_table[] = {
> > + { .compatible = "ti,lm3533-als" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, lm3533_als_match_table);
> > +
> > static struct platform_driver lm3533_als_driver = {
> > .driver = {
> > .name = "lm3533-als",
> > + .of_match_table = lm3533_als_match_table,
> > },
> > .probe = lm3533_als_probe,
> > .remove = lm3533_als_remove,
> > diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
> > index 45795f2a1042..6e661a2a540f 100644
> > --- a/drivers/leds/leds-lm3533.c
> > +++ b/drivers/leds/leds-lm3533.c
> > @@ -10,8 +10,10 @@
> > #include <linux/module.h>
> > #include <linux/leds.h>
> > #include <linux/mfd/core.h>
> > +#include <linux/mod_devicetable.h>
> > #include <linux/mutex.h>
> > #include <linux/platform_device.h>
> > +#include <linux/property.h>
> > #include <linux/slab.h>
> >
> > #include <linux/mfd/lm3533.h>
> > @@ -48,6 +50,9 @@ struct lm3533_led {
> >
> > struct mutex mutex;
> > unsigned long flags;
> > +
> > + u16 max_current; /* 5000 - 29800 uA (800 uA step) */
> > + u8 pwm; /* 0 - 0x3f */
>
> This comment doesn't add anything.
>
This comment is from pdata. Both values were just transferred from there.
> > };
> >
> >
> > @@ -632,23 +637,22 @@ static const struct attribute_group *lm3533_led_attribute_groups[] = {
> > NULL
> > };
> >
> > -static int lm3533_led_setup(struct lm3533_led *led,
> > - struct lm3533_led_platform_data *pdata)
> > +static int lm3533_led_setup(struct lm3533_led *led)
> > {
> > int ret;
> >
> > - ret = lm3533_ctrlbank_set_max_current(&led->cb, pdata->max_current);
> > + ret = lm3533_ctrlbank_set_max_current(&led->cb, led->max_current);
>
> Why not make max_current and attribute of lm3533_ctrlbank and drop the
> redundant parameter?
>
> > if (ret)
> > return ret;
> >
> > - return lm3533_ctrlbank_set_pwm(&led->cb, pdata->pwm);
> > + return lm3533_ctrlbank_set_pwm(&led->cb, led->pwm);
>
> As above.
>
As above.
> > }
> >
> > static int lm3533_led_probe(struct platform_device *pdev)
> > {
> > struct lm3533 *lm3533;
> > - struct lm3533_led_platform_data *pdata;
> > struct lm3533_led *led;
> > + u32 val;
> > int ret;
> >
> > dev_dbg(&pdev->dev, "%s\n", __func__);
> > @@ -657,12 +661,6 @@ static int lm3533_led_probe(struct platform_device *pdev)
> > if (!lm3533)
> > return -EINVAL;
> >
> > - pdata = dev_get_platdata(&pdev->dev);
> > - if (!pdata) {
> > - dev_err(&pdev->dev, "no platform data\n");
> > - return -EINVAL;
> > - }
> > -
> > if (pdev->id < 0 || pdev->id >= LM3533_LVCTRLBANK_COUNT) {
> > dev_err(&pdev->dev, "illegal LED id %d\n", pdev->id);
> > return -EINVAL;
> > @@ -673,8 +671,11 @@ static int lm3533_led_probe(struct platform_device *pdev)
> > return -ENOMEM;
> >
> > led->lm3533 = lm3533;
> > - led->cdev.name = pdata->name;
> > - led->cdev.default_trigger = pdata->default_trigger;
> > + led->cdev.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-%d",
> > + pdev->name, pdev->id);
> > + led->cdev.default_trigger = "none";
> > + device_property_read_string(&pdev->dev, "linux,default-trigger",
> > + &led->cdev.default_trigger);
> > led->cdev.brightness_set_blocking = lm3533_led_set;
> > led->cdev.brightness_get = lm3533_led_get;
> > led->cdev.blink_set = lm3533_led_blink_set;
> > @@ -702,7 +703,17 @@ static int lm3533_led_probe(struct platform_device *pdev)
> >
> > led->cb.dev = led->cdev.dev;
> >
> > - ret = lm3533_led_setup(led, pdata);
> > + /* 5000 - 29800 uA (800 uA step) */
> > + val = 5000;
> > + device_property_read_u32(&pdev->dev, "ti,max-current-microamp", &val);
> > + led->max_current = val;
>
> Why not just use 'led->max_current' from the offset and delete 'val'?
>
BECAUSE, linux has no device_property_read_u32_default to pass default
value there. Additionally max_current is u16 and
device_property_read_u32 will not work for u16.
> > +
> > + /* 0 - 0x3f */
>
> How does this improve readability? Why would use this info?
>
> > + val = 0;
> > + device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &val);
> > + led->pwm = val;
> > +
> > + ret = lm3533_led_setup(led);
> > if (ret)
> > goto err_deregister;
> >
> > @@ -739,9 +750,16 @@ static void lm3533_led_shutdown(struct platform_device *pdev)
> > lm3533_led_set(&led->cdev, LED_OFF); /* disable blink */
> > }
> >
> > +static const struct of_device_id lm3533_led_match_table[] = {
> > + { .compatible = "ti,lm3533-leds" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, lm3533_led_match_table);
> > +
> > static struct platform_driver lm3533_led_driver = {
> > .driver = {
> > .name = "lm3533-leds",
> > + .of_match_table = lm3533_led_match_table,
> > },
> > .probe = lm3533_led_probe,
> > .remove = lm3533_led_remove,
> > diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
> > index 0a2409d00b2e..e1b8fe136af9 100644
> > --- a/drivers/mfd/lm3533-core.c
> > +++ b/drivers/mfd/lm3533-core.c
> > @@ -14,10 +14,13 @@
> > #include <linux/gpio/consumer.h>
> > #include <linux/i2c.h>
> > #include <linux/mfd/core.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/property.h>
> > #include <linux/regmap.h>
> > #include <linux/seq_file.h>
> > #include <linux/slab.h>
> > #include <linux/uaccess.h>
> > +#include <linux/units.h>
> >
> > #include <linux/mfd/lm3533.h>
> >
> > @@ -42,41 +45,41 @@
> >
> > #define LM3533_REG_MAX 0xb2
> >
> > -
> > -static struct mfd_cell lm3533_als_devs[] = {
> > +static struct mfd_cell lm3533_child_devices[] = {
>
> Drop the child_ part.
>
Why? It seems that you are nitpicking.
> > {
> > .name = "lm3533-als",
> > .id = -1,
> > + .of_compatible = "ti,lm3533-als",
> > },
> > -};
> > -
> > -static struct mfd_cell lm3533_bl_devs[] = {
> > {
> > .name = "lm3533-backlight",
> > .id = 0,
> > + .of_compatible = "ti,lm3533-backlight",
> > },
> > {
> > .name = "lm3533-backlight",
> > .id = 1,
> > + .of_compatible = "ti,lm3533-backlight",
> > },
> > -};
> > -
> > -static struct mfd_cell lm3533_led_devs[] = {
> > {
> > .name = "lm3533-leds",
> > .id = 0,
>
> Do you know if these are actually used for anything?
>
Yes, they are used to select correct control bank
> Any reason to not just use PLATFORM_DEVID_AUTO?
>
> > + .of_compatible = "ti,lm3533-leds",
> > },
> > {
> > .name = "lm3533-leds",
> > .id = 1,
> > + .of_compatible = "ti,lm3533-leds",
> > },
> > {
> > .name = "lm3533-leds",
> > .id = 2,
> > + .of_compatible = "ti,lm3533-leds",
> > },
> > {
> > .name = "lm3533-leds",
> > .id = 3,
> > + .of_compatible = "ti,lm3533-leds",
> > },
> > };
> >
> > @@ -376,136 +379,60 @@ static struct attribute_group lm3533_attribute_group = {
> > .attrs = lm3533_attributes
> > };
> >
> > -static int lm3533_device_als_init(struct lm3533 *lm3533)
> > -{
> > - struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
> > - int ret;
> > -
> > - if (!pdata->als)
> > - return 0;
> > -
> > - lm3533_als_devs[0].platform_data = pdata->als;
> > - lm3533_als_devs[0].pdata_size = sizeof(*pdata->als);
> > -
> > - ret = mfd_add_devices(lm3533->dev, 0, lm3533_als_devs, 1, NULL,
> > - 0, NULL);
> > - if (ret) {
> > - dev_err(lm3533->dev, "failed to add ALS device\n");
> > - return ret;
> > - }
> > -
> > - lm3533->have_als = 1;
> > -
> > - return 0;
> > -}
> > -
> > -static int lm3533_device_bl_init(struct lm3533 *lm3533)
> > -{
> > - struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
> > - int i;
> > - int ret;
> > -
> > - if (!pdata->backlights || pdata->num_backlights == 0)
> > - return 0;
> > -
> > - if (pdata->num_backlights > ARRAY_SIZE(lm3533_bl_devs))
> > - pdata->num_backlights = ARRAY_SIZE(lm3533_bl_devs);
> > -
> > - for (i = 0; i < pdata->num_backlights; ++i) {
> > - lm3533_bl_devs[i].platform_data = &pdata->backlights[i];
> > - lm3533_bl_devs[i].pdata_size = sizeof(pdata->backlights[i]);
> > - }
> > -
> > - ret = mfd_add_devices(lm3533->dev, 0, lm3533_bl_devs,
> > - pdata->num_backlights, NULL, 0, NULL);
> > - if (ret) {
> > - dev_err(lm3533->dev, "failed to add backlight devices\n");
> > - return ret;
> > - }
> > -
> > - lm3533->have_backlights = 1;
> > -
> > - return 0;
> > -}
> > -
> > -static int lm3533_device_led_init(struct lm3533 *lm3533)
> > -{
> > - struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
> > - int i;
> > - int ret;
> > -
> > - if (!pdata->leds || pdata->num_leds == 0)
> > - return 0;
> > -
> > - if (pdata->num_leds > ARRAY_SIZE(lm3533_led_devs))
> > - pdata->num_leds = ARRAY_SIZE(lm3533_led_devs);
> > -
> > - for (i = 0; i < pdata->num_leds; ++i) {
> > - lm3533_led_devs[i].platform_data = &pdata->leds[i];
> > - lm3533_led_devs[i].pdata_size = sizeof(pdata->leds[i]);
> > - }
> > -
> > - ret = mfd_add_devices(lm3533->dev, 0, lm3533_led_devs,
> > - pdata->num_leds, NULL, 0, NULL);
> > - if (ret) {
> > - dev_err(lm3533->dev, "failed to add LED devices\n");
> > - return ret;
> > - }
> > -
> > - lm3533->have_leds = 1;
> > -
> > - return 0;
> > -}
> > -
> > -static int lm3533_device_setup(struct lm3533 *lm3533,
> > - struct lm3533_platform_data *pdata)
> > +static int lm3533_device_setup(struct lm3533 *lm3533)
> > {
> > int ret;
> >
> > - ret = lm3533_set_boost_freq(lm3533, pdata->boost_freq);
> > + ret = lm3533_set_boost_freq(lm3533, lm3533->boost_freq);
>
> Same comments as before.
>
> Teach lm3533_set_boost_freq() that 'boost_freq' is contained in 'lm3533'.
>
Same answer as above.
> > if (ret)
> > return ret;
> >
> > - return lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
> > + return lm3533_set_boost_ovp(lm3533, lm3533->boost_ovp);
> > }
> >
> > static int lm3533_device_init(struct lm3533 *lm3533)
> > {
> > - struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
> > + u32 val;
>
> 'uV' and 'hz' could be easier to follow.
>
Add 2 temp values instead of one, nice.
> > int ret;
> >
> > - dev_dbg(lm3533->dev, "%s\n", __func__);
> > + lm3533->hwen = devm_gpiod_get(lm3533->dev, "enable", GPIOD_OUT_LOW);
> > + if (IS_ERR(lm3533->hwen))
> > + return dev_err_probe(lm3533->dev, PTR_ERR(lm3533->hwen),
> > + "failed to request HWEN GPIO\n");
> >
> > - if (!pdata) {
> > - dev_err(lm3533->dev, "no platform data\n");
> > - return -EINVAL;
> > - }
> > + val = 16 * MICRO; /* 16V */
> > + device_property_read_u32(lm3533->dev, "ti,boost-ovp-microvolt", &val);
> >
> > - lm3533->hwen = devm_gpiod_get(lm3533->dev, NULL, GPIOD_OUT_LOW);
> > - if (IS_ERR(lm3533->hwen))
> > - return dev_err_probe(lm3533->dev, PTR_ERR(lm3533->hwen), "failed to request HWEN GPIO\n");
> > - gpiod_set_consumer_name(lm3533->hwen, "lm3533-hwen");
> > + /* boost_ovp is defined in microvolts, convert to enum value */
> > + lm3533->boost_ovp = val / (8 * MICRO) - 2;
>
> Wait, what. Why?
>
> Converting a useful microvolt value to an arbitrary enum sounds fragile.
>
BACAUSE the device DOES NOT understand human readable values, only 0s
and 1s, hence uV and Hz must be converted into value lm3533 chip can
understand.
> > +
> > + val = 500 * KILO; /* 500kHz */
> > + device_property_read_u32(lm3533->dev, "ti,boost-freq-hz", &val);
> > +
> > + /* boost_freq is defined in Hz, convert to enum value */
> > + lm3533->boost_freq = val / (500 * KILO) - 1;
> >
> > lm3533_enable(lm3533);
> >
> > - ret = lm3533_device_setup(lm3533, pdata);
> > + ret = lm3533_device_setup(lm3533);
> > if (ret)
> > goto err_disable;
> >
> > - lm3533_device_als_init(lm3533);
> > - lm3533_device_bl_init(lm3533);
> > - lm3533_device_led_init(lm3533);
> > + ret = devm_mfd_add_devices(lm3533->dev, 0, lm3533_child_devices,
>
>
> > + ARRAY_SIZE(lm3533_child_devices), NULL, 0, NULL);
> > + if (ret) {
> > + dev_err(lm3533->dev, "failed to add MFD devices: %d\n", ret);
>
> "child devices" or "sub-devices".
>
Does this naming matters? I can call them any way I want to as long
this remains clear. Or you have created some twisted consensus in your
heads that mfd child devices MUST be called sub-devices?
> > + goto err_disable;
> > + }
> >
> > ret = sysfs_create_group(&lm3533->dev->kobj, &lm3533_attribute_group);
> > if (ret < 0) {
> > dev_err(lm3533->dev, "failed to create sysfs attributes\n");
> > - goto err_unregister;
> > + goto err_disable;
> > }
> >
> > return 0;
> >
> > -err_unregister:
> > - mfd_remove_devices(lm3533->dev);
> > err_disable:
> > lm3533_disable(lm3533);
> >
> > @@ -517,8 +444,6 @@ static void lm3533_device_exit(struct lm3533 *lm3533)
> > dev_dbg(lm3533->dev, "%s\n", __func__);
> >
> > sysfs_remove_group(&lm3533->dev->kobj, &lm3533_attribute_group);
> > -
> > - mfd_remove_devices(lm3533->dev);
> > lm3533_disable(lm3533);
> > }
> >
> > @@ -591,6 +516,9 @@ static int lm3533_i2c_probe(struct i2c_client *i2c)
> > lm3533->dev = &i2c->dev;
> > lm3533->irq = i2c->irq;
> >
> > + i2c->dev.coherent_dma_mask = 0;
> > + i2c->dev.dma_mask = &i2c->dev.coherent_dma_mask;
>
> Why are you manually doing this?
>
> The core should take care of this for you:
>
> drivers/mfd/mfd-core.c: pdev->dev.dma_mask = parent->dma_mask;
> drivers/mfd/mfd-core.c: pdev->dev.coherent_dma_mask = parent->coherent_dma_mask;
>
Parent uses DMA, but mfd and its children do not,
> > +
> > return lm3533_device_init(lm3533);
> > }
> >
> > @@ -603,6 +531,12 @@ static void lm3533_i2c_remove(struct i2c_client *i2c)
> > lm3533_device_exit(lm3533);
> > }
> >
> > +static const struct of_device_id lm3533_match_table[] = {
> > + { .compatible = "ti,lm3533" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, lm3533_match_table);
> > +
> > static const struct i2c_device_id lm3533_i2c_ids[] = {
> > { "lm3533" },
> > { }
> > @@ -612,6 +546,7 @@ MODULE_DEVICE_TABLE(i2c, lm3533_i2c_ids);
> > static struct i2c_driver lm3533_i2c_driver = {
> > .driver = {
> > .name = "lm3533",
> > + .of_match_table = lm3533_match_table,
> > },
> > .id_table = lm3533_i2c_ids,
> > .probe = lm3533_i2c_probe,
> > diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
> > index babfd3ceec86..0827a5e98dbb 100644
> > --- a/drivers/video/backlight/lm3533_bl.c
> > +++ b/drivers/video/backlight/lm3533_bl.c
> > @@ -9,7 +9,9 @@
> >
> > #include <linux/module.h>
> > #include <linux/init.h>
> > +#include <linux/mod_devicetable.h>
> > #include <linux/platform_device.h>
> > +#include <linux/property.h>
> > #include <linux/backlight.h>
> > #include <linux/slab.h>
> >
> > @@ -19,6 +21,7 @@
> > #define LM3533_HVCTRLBANK_COUNT 2
> > #define LM3533_BL_MAX_BRIGHTNESS 255
> >
> > +#define LM3533_REG_OUTPUT_CONF1 0x10
> > #define LM3533_REG_CTRLBANK_AB_BCONF 0x1a
> >
> >
> > @@ -27,6 +30,11 @@ struct lm3533_bl {
> > struct lm3533_ctrlbank cb;
> > struct backlight_device *bd;
> > int id;
> > +
> > + u16 max_current; /* 5000 - 29800 uA (800 uA step) */
> > + u8 pwm; /* 0 - 0x3f */
>
> Remove or improve this comment.
>
Comment was moved from the platform data along with the value.
> > + bool linear;
> > + bool hvled;
> > };
> >
> >
> > @@ -246,25 +254,40 @@ static struct attribute_group lm3533_bl_attribute_group = {
> > .attrs = lm3533_bl_attributes
> > };
> >
> > -static int lm3533_bl_setup(struct lm3533_bl *bl,
> > - struct lm3533_bl_platform_data *pdata)
> > +static int lm3533_bl_setup(struct lm3533_bl *bl)
> > {
> > + int id = lm3533_bl_get_ctrlbank_id(bl);
> > int ret;
> >
> > - ret = lm3533_ctrlbank_set_max_current(&bl->cb, pdata->max_current);
> > + if (bl->linear) {
> > + ret = lm3533_update(bl->lm3533, LM3533_REG_CTRLBANK_AB_BCONF,
> > + BIT(2 * id + 1), BIT(2 * id + 1));
>
> These need to be defined as SHIFT values or whatever they are.
>
Why? IMHO looks perfectly fine and readable.
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + if (bl->hvled) {
> > + ret = lm3533_update(bl->lm3533, LM3533_REG_OUTPUT_CONF1,
> > + id | id << 1, BIT(0) | BIT(1));
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + ret = lm3533_ctrlbank_set_max_current(&bl->cb, bl->max_current);
> > if (ret)
> > return ret;
> >
> > - return lm3533_ctrlbank_set_pwm(&bl->cb, pdata->pwm);
> > + return lm3533_ctrlbank_set_pwm(&bl->cb, bl->pwm);
> > }
> >
> > static int lm3533_bl_probe(struct platform_device *pdev)
> > {
> > struct lm3533 *lm3533;
> > - struct lm3533_bl_platform_data *pdata;
> > struct lm3533_bl *bl;
> > struct backlight_device *bd;
> > struct backlight_properties props;
> > + char *name;
> > + u32 val;
>
> As above.
>
As above.
> > int ret;
> >
> > dev_dbg(&pdev->dev, "%s\n", __func__);
> > @@ -273,12 +296,6 @@ static int lm3533_bl_probe(struct platform_device *pdev)
> > if (!lm3533)
> > return -EINVAL;
> >
> > - pdata = dev_get_platdata(&pdev->dev);
> > - if (!pdata) {
> > - dev_err(&pdev->dev, "no platform data\n");
> > - return -EINVAL;
> > - }
> > -
> > if (pdev->id < 0 || pdev->id >= LM3533_HVCTRLBANK_COUNT) {
> > dev_err(&pdev->dev, "illegal backlight id %d\n", pdev->id);
> > return -EINVAL;
> > @@ -295,13 +312,15 @@ static int lm3533_bl_probe(struct platform_device *pdev)
> > bl->cb.id = lm3533_bl_get_ctrlbank_id(bl);
> > bl->cb.dev = NULL; /* until registered */
> >
> > + name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-%d", pdev->name, pdev->id);
>
> Doesn't platform already provide enumerated names?
>
It does, but if you have 4 devices which use same driver there cannot
be 4 same names.
> > +
> > memset(&props, 0, sizeof(props));
> > props.type = BACKLIGHT_RAW;
> > props.max_brightness = LM3533_BL_MAX_BRIGHTNESS;
> > - props.brightness = pdata->default_brightness;
> > - bd = devm_backlight_device_register(&pdev->dev, pdata->name,
> > - pdev->dev.parent, bl, &lm3533_bl_ops,
> > - &props);
> > + props.brightness = LM3533_BL_MAX_BRIGHTNESS;
> > + device_property_read_u32(&pdev->dev, "default-brightness", &props.brightness);
> > + bd = devm_backlight_device_register(&pdev->dev, name, pdev->dev.parent,
> > + bl, &lm3533_bl_ops, &props);
> > if (IS_ERR(bd)) {
> > dev_err(&pdev->dev, "failed to register backlight device\n");
> > return PTR_ERR(bd);
> > @@ -320,7 +339,20 @@ static int lm3533_bl_probe(struct platform_device *pdev)
> >
> > backlight_update_status(bd);
> >
> > - ret = lm3533_bl_setup(bl, pdata);
> > + /* 5000 - 29800 uA (800 uA step) */
> > + val = 5000;
> > + device_property_read_u32(&pdev->dev, "ti,max-current-microamp", &val);
> > + bl->max_current = val;
> > +
> > + /* 0 - 0x3f */
> > + val = 0;
> > + device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &val);
> > + bl->pwm = val;
> > +
> > + bl->linear = device_property_read_bool(&pdev->dev, "ti,linear-mapping-mode");
> > + bl->hvled = device_property_read_bool(&pdev->dev, "ti,hardware-controlled");
> > +
> > + ret = lm3533_bl_setup(bl);
> > if (ret)
> > goto err_sysfs_remove;
> >
> > @@ -381,10 +413,17 @@ static void lm3533_bl_shutdown(struct platform_device *pdev)
> > lm3533_ctrlbank_disable(&bl->cb);
> > }
> >
> > +static const struct of_device_id lm3533_bl_match_table[] = {
> > + { .compatible = "ti,lm3533-backlight" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, lm3533_bl_match_table);
> > +
> > static struct platform_driver lm3533_bl_driver = {
> > .driver = {
> > .name = "lm3533-backlight",
> > .pm = &lm3533_bl_pm_ops,
> > + .of_match_table = lm3533_bl_match_table,
> > },
> > .probe = lm3533_bl_probe,
> > .remove = lm3533_bl_remove,
> > diff --git a/include/linux/mfd/lm3533.h b/include/linux/mfd/lm3533.h
> > index 69059a7a2ce5..3b28fc0970f6 100644
> > --- a/include/linux/mfd/lm3533.h
> > +++ b/include/linux/mfd/lm3533.h
> > @@ -27,6 +27,9 @@ struct lm3533 {
> > struct gpio_desc *hwen;
> > int irq;
> >
> > + u32 boost_ovp;
> > + u32 boost_freq;
> > +
> > unsigned have_als:1;
> > unsigned have_backlights:1;
> > unsigned have_leds:1;
> > @@ -38,25 +41,6 @@ struct lm3533_ctrlbank {
> > int id;
> > };
> >
> > -struct lm3533_als_platform_data {
> > - unsigned pwm_mode:1; /* PWM input mode (default analog) */
> > - u8 r_select; /* 1 - 127 (ignored in PWM-mode) */
> > -};
> > -
> > -struct lm3533_bl_platform_data {
> > - char *name;
> > - u16 max_current; /* 5000 - 29800 uA (800 uA step) */
> > - u8 default_brightness; /* 0 - 255 */
> > - u8 pwm; /* 0 - 0x3f */
> > -};
> > -
> > -struct lm3533_led_platform_data {
> > - char *name;
> > - const char *default_trigger;
> > - u16 max_current; /* 5000 - 29800 uA (800 uA step) */
> > - u8 pwm; /* 0 - 0x3f */
> > -};
> > -
> > enum lm3533_boost_freq {
> > LM3533_BOOST_FREQ_500KHZ,
> > LM3533_BOOST_FREQ_1000KHZ,
> > @@ -69,19 +53,6 @@ enum lm3533_boost_ovp {
> > LM3533_BOOST_OVP_40V,
> > };
> >
> > -struct lm3533_platform_data {
> > - enum lm3533_boost_ovp boost_ovp;
> > - enum lm3533_boost_freq boost_freq;
> > -
> > - struct lm3533_als_platform_data *als;
> > -
> > - struct lm3533_bl_platform_data *backlights;
> > - int num_backlights;
> > -
> > - struct lm3533_led_platform_data *leds;
> > - int num_leds;
> > -};
> > -
> > extern int lm3533_ctrlbank_enable(struct lm3533_ctrlbank *cb);
> > extern int lm3533_ctrlbank_disable(struct lm3533_ctrlbank *cb);
> >
> > --
> > 2.43.0
> >
>
> --
> Lee Jones [李琼斯]
^ permalink raw reply
* Re: [PATCH v2] fbdev: pxafb: use devm_kmemdup*()
From: Andy Shevchenko @ 2025-02-28 12:37 UTC (permalink / raw)
To: Raag Jadav; +Cc: deller, tzimmermann, linux-fbdev, linux-kernel
In-Reply-To: <20250228071009.150971-1-raag.jadav@intel.com>
On Fri, Feb 28, 2025 at 12:40:09PM +0530, Raag Jadav wrote:
> Convert to use devm_kmemdup() and devm_kmemdup_array() which are
> more robust.
FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 0/7] gpu: ipu-v3: Remove unused functions
From: Dr. David Alan Gilbert @ 2025-02-28 15:42 UTC (permalink / raw)
To: tzimmermann, airlied, simona
Cc: Dmitry Baryshkov, p.zabel, deller, shawnguo, s.hauer, kernel,
festevam, dri-devel, linux-fbdev, imx, linux-arm-kernel,
linux-kernel
In-Reply-To: <gugwtvw6qqknstlscr4hxfrvcgfa4gfwwgxdosr24mf7huk433@oh7axkbesrjs>
Hi All,
* Dmitry Baryshkov (dmitry.baryshkov@linaro.org) wrote:
> On Thu, Dec 26, 2024 at 02:27:45AM +0000, linux@treblig.org wrote:
> > From: "Dr. David Alan Gilbert" <linux@treblig.org>
> >
> > Hi,
> > This set removes a bunch of functions in ipu-v3 that
> > have been unused for a long time (since 2012-2017).
> >
> > No changes to functions are made, just full deletions.
> >
> > Build tested only.
> >
> > Signed-off-by: Dr. David Alan Gilbert <linux@treblig.org>
> >
>
>
> For the series:
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Could this be picked up for drm please?
Thanks,
Dave
> --
> With best wishes
> Dmitry
--
-----Open up your eyes, open up your mind, open up your code -------
/ Dr. David Alan Gilbert | Running GNU/Linux | Happy \
\ dave @ treblig.org | | In Hex /
\ _________________________|_____ http://www.treblig.org |_______/
^ permalink raw reply
* [PATCH] staging: sm750fb: Fix CamelCase variable naming
From: Gabriel Lima Luz @ 2025-02-28 16:23 UTC (permalink / raw)
To: linux-fbdev, linux-staging, linux-kernel, Sudip Mukherjee,
Teddy Wang, Greg Kroah-Hartman, ~lkcamp/patches
Adhere to Linux kernel coding style.
Reported by checkpatch:
CHECK: Avoid CamelCase
Signed-off-by: Gabriel Lima Luz <lima.gabriel.luz@gmail.com>
---
drivers/staging/sm750fb/ddk750_power.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/sm750fb/ddk750_power.h b/drivers/staging/sm750fb/ddk750_power.h
index 63c9e8b6ffb3..33e852fe6949 100644
--- a/drivers/staging/sm750fb/ddk750_power.h
+++ b/drivers/staging/sm750fb/ddk750_power.h
@@ -3,10 +3,10 @@
#define DDK750_POWER_H__
enum dpms {
- crtDPMS_ON = 0x0,
- crtDPMS_STANDBY = 0x1,
- crtDPMS_SUSPEND = 0x2,
- crtDPMS_OFF = 0x3,
+ crt_DPMS_ON = 0x0,
+ crt_DPMS_STANDBY = 0x1,
+ crt_DPMS_SUSPEND = 0x2,
+ crt_DPMS_OFF = 0x3,
};
#define set_DAC(off) { \
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] staging: sm750fb: Fix CamelCase variable naming
From: Dan Carpenter @ 2025-02-28 17:40 UTC (permalink / raw)
To: Gabriel Lima Luz
Cc: linux-fbdev, linux-staging, linux-kernel, Sudip Mukherjee,
Teddy Wang, Greg Kroah-Hartman, ~lkcamp/patches
In-Reply-To: <20250228162359.14029-1-lima.gabriel.luz@gmail.com>
On Fri, Feb 28, 2025 at 01:23:54PM -0300, Gabriel Lima Luz wrote:
> Adhere to Linux kernel coding style.
>
> Reported by checkpatch:
>
> CHECK: Avoid CamelCase
>
> Signed-off-by: Gabriel Lima Luz <lima.gabriel.luz@gmail.com>
> ---
> drivers/staging/sm750fb/ddk750_power.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/sm750fb/ddk750_power.h b/drivers/staging/sm750fb/ddk750_power.h
> index 63c9e8b6ffb3..33e852fe6949 100644
> --- a/drivers/staging/sm750fb/ddk750_power.h
> +++ b/drivers/staging/sm750fb/ddk750_power.h
> @@ -3,10 +3,10 @@
> #define DDK750_POWER_H__
>
> enum dpms {
> - crtDPMS_ON = 0x0,
> - crtDPMS_STANDBY = 0x1,
> - crtDPMS_SUSPEND = 0x2,
> - crtDPMS_OFF = 0x3,
> + crt_DPMS_ON = 0x0,
> + crt_DPMS_STANDBY = 0x1,
> + crt_DPMS_SUSPEND = 0x2,
> + crt_DPMS_OFF = 0x3,
> };
>
It seems these are not used. Just delete them.
regards,
dan carpenter
^ permalink raw reply
* Re: [syzbot] [fbdev?] KASAN: global-out-of-bounds Read in bit_putcs (3)
From: syzbot @ 2025-02-28 20:37 UTC (permalink / raw)
To: daniel, deller, dri-devel, linux-fbdev, linux-kernel, simona,
syzkaller-bugs
In-Reply-To: <000000000000e39b37061e898704@google.com>
syzbot has found a reproducer for the following issue on:
HEAD commit: 017f704fbfb1 Merge branch 'for-next/core' into for-kernelci
git tree: git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-kernelci
console output: https://syzkaller.appspot.com/x/log.txt?x=12128864580000
kernel config: https://syzkaller.appspot.com/x/.config?x=d6b7e15dc5b5e776
dashboard link: https://syzkaller.appspot.com/bug?extid=793cf822d213be1a74f2
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
userspace arch: arm64
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1643dba0580000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=16128864580000
Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/d34fb306468f/disk-017f704f.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/6f1126558a26/vmlinux-017f704f.xz
kernel image: https://storage.googleapis.com/syzbot-assets/6d9d912bee27/Image-017f704f.gz.xz
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+793cf822d213be1a74f2@syzkaller.appspotmail.com
do_el0_svc+0x48/0x58 arch/arm64/kernel/syscall.c:151
el0_svc+0x54/0x168 arch/arm64/kernel/entry-common.c:744
el0t_64_sync_handler+0x84/0x108 arch/arm64/kernel/entry-common.c:762
el0t_64_sync+0x198/0x19c arch/arm64/kernel/entry.S:600
==================================================================
BUG: KASAN: global-out-of-bounds in __fb_pad_aligned_buffer include/linux/fb.h:644 [inline]
BUG: KASAN: global-out-of-bounds in bit_putcs_aligned drivers/video/fbdev/core/bitblit.c:96 [inline]
BUG: KASAN: global-out-of-bounds in bit_putcs+0x9b8/0xe30 drivers/video/fbdev/core/bitblit.c:185
Read of size 1 at addr ffff80008be20810 by task syz-executor101/6448
CPU: 1 UID: 0 PID: 6448 Comm: syz-executor101 Not tainted 6.14.0-rc4-syzkaller-g017f704fbfb1 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 12/27/2024
Call trace:
show_stack+0x2c/0x3c arch/arm64/kernel/stacktrace.c:466 (C)
__dump_stack lib/dump_stack.c:94 [inline]
dump_stack_lvl+0xe4/0x150 lib/dump_stack.c:120
print_address_description mm/kasan/report.c:408 [inline]
print_report+0x198/0x550 mm/kasan/report.c:521
kasan_report+0xd8/0x138 mm/kasan/report.c:634
__asan_report_load1_noabort+0x20/0x2c mm/kasan/report_generic.c:378
__fb_pad_aligned_buffer include/linux/fb.h:644 [inline]
bit_putcs_aligned drivers/video/fbdev/core/bitblit.c:96 [inline]
bit_putcs+0x9b8/0xe30 drivers/video/fbdev/core/bitblit.c:185
fbcon_putcs+0x390/0x5a0 drivers/video/fbdev/core/fbcon.c:1308
do_update_region+0x1e8/0x3d0 drivers/tty/vt/vt.c:609
update_region+0x1e0/0x478 drivers/tty/vt/vt.c:633
vcs_write+0x9e8/0x1180 drivers/tty/vt/vc_screen.c:698
do_loop_readv_writev fs/read_write.c:843 [inline]
vfs_writev+0x494/0x92c fs/read_write.c:1052
do_writev+0x178/0x304 fs/read_write.c:1096
__do_sys_writev fs/read_write.c:1164 [inline]
__se_sys_writev fs/read_write.c:1161 [inline]
__arm64_sys_writev+0x80/0x94 fs/read_write.c:1161
__invoke_syscall arch/arm64/kernel/syscall.c:35 [inline]
invoke_syscall+0x98/0x2b8 arch/arm64/kernel/syscall.c:49
el0_svc_common+0x130/0x23c arch/arm64/kernel/syscall.c:132
do_el0_svc+0x48/0x58 arch/arm64/kernel/syscall.c:151
el0_svc+0x54/0x168 arch/arm64/kernel/entry-common.c:744
el0t_64_sync_handler+0x84/0x108 arch/arm64/kernel/entry-common.c:762
el0t_64_sync+0x198/0x19c arch/arm64/kernel/entry.S:600
The buggy address belongs to the variable:
fontdata_8x16+0x1010/0x1480
The buggy address belongs to the virtual mapping at
[ffff80008b810000, ffff80008f6b0000) created by:
declare_kernel_vmas+0x58/0xb8 arch/arm64/mm/mmu.c:771
The buggy address belongs to the physical page:
page: refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x1a1820
flags: 0x5ffc00000002000(reserved|node=0|zone=2|lastcpupid=0x7ff)
raw: 05ffc00000002000 fffffdffc5860808 fffffdffc5860808 0000000000000000
raw: 0000000000000000 0000000000000000 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff80008be20700: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffff80008be20780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>ffff80008be20800: 00 00 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
^
ffff80008be20880: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
ffff80008be20900: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
==================================================================
---
If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox