* Re: [PATCH 00/12] Recover sysfb after DRM probe failure
From: Thomas Zimmermann @ 2026-01-15 14:54 UTC (permalink / raw)
To: Christian König, Zack Rusin
Cc: dri-devel, Alex Deucher, amd-gfx, Ard Biesheuvel, Ce Sun,
Chia-I Wu, Danilo Krummrich, Dave Airlie, Deepak Rawat,
Dmitry Osipenko, Gerd Hoffmann, Gurchetan Singh, Hans de Goede,
Hawking Zhang, Helge Deller, intel-gfx, intel-xe, Jani Nikula,
Javier Martinez Canillas, Jocelyn Falempe, Joonas Lahtinen,
Lijo Lazar, linux-efi, linux-fbdev, linux-hyperv, linux-kernel,
Lucas De Marchi, Lyude Paul, Maarten Lankhorst,
Mario Limonciello (AMD), Mario Limonciello, Maxime Ripard,
nouveau, Rodrigo Vivi, Simona Vetter, spice-devel,
Thomas Hellström, Timur Kristóf, Tvrtko Ursulin,
virtualization, Vitaly Prosyak
In-Reply-To: <9058636d-cc18-4c8f-92cf-782fd8f771af@amd.com>
Hi
Am 15.01.26 um 15:39 schrieb Christian König:
> Sorry to being late, but I only now realized what you are doing here.
>
> On 1/15/26 12:02, Thomas Zimmermann wrote:
>> Hi,
>>
>> apologies for the delay. I wanted to reply and then forgot about it.
>>
>> Am 10.01.26 um 05:52 schrieb Zack Rusin:
>>> On Fri, Jan 9, 2026 at 5:34 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>> Hi
>>>>
>>>> Am 29.12.25 um 22:58 schrieb Zack Rusin:
>>>>> Almost a rite of passage for every DRM developer and most Linux users
>>>>> is upgrading your DRM driver/updating boot flags/changing some config
>>>>> and having DRM driver fail at probe resulting in a blank screen.
>>>>>
>>>>> Currently there's no way to recover from DRM driver probe failure. PCI
>>>>> DRM driver explicitly throw out the existing sysfb to get exclusive
>>>>> access to PCI resources so if the probe fails the system is left without
>>>>> a functioning display driver.
>>>>>
>>>>> Add code to sysfb to recever system framebuffer when DRM driver's probe
>>>>> fails. This means that a DRM driver that fails to load reloads the system
>>>>> framebuffer driver.
>>>>>
>>>>> This works best with simpledrm. Without it Xorg won't recover because
>>>>> it still tries to load the vendor specific driver which ends up usually
>>>>> not working at all. With simpledrm the system recovers really nicely
>>>>> ending up with a working console and not a blank screen.
>>>>>
>>>>> There's a caveat in that some hardware might require some special magic
>>>>> register write to recover EFI display. I'd appreciate it a lot if
>>>>> maintainers could introduce a temporary failure in their drivers
>>>>> probe to validate that the sysfb recovers and they get a working console.
>>>>> The easiest way to double check it is by adding:
>>>>> /* XXX: Temporary failure to test sysfb restore - REMOVE BEFORE COMMIT */
>>>>> dev_info(&pdev->dev, "Testing sysfb restore: forcing probe failure\n");
>>>>> ret = -EINVAL;
>>>>> goto out_error;
>>>>> or such right after the devm_aperture_remove_conflicting_pci_devices .
>>>> Recovering the display like that is guess work and will at best work
>>>> with simple discrete devices where the framebuffer is always located in
>>>> a confined graphics aperture.
>>>>
>>>> But the problem you're trying to solve is a real one.
>>>>
>>>> What we'd want to do instead is to take the initial hardware state into
>>>> account when we do the initial mode-setting operation.
>>>>
>>>> The first step is to move each driver's remove_conflicting_devices call
>>>> to the latest possible location in the probe function. We usually do it
>>>> first, because that's easy. But on most hardware, it could happen much
>>>> later.
>>> Well, some drivers (vbox, vmwgfx, bochs and currus-qemu) do it because
>>> they request pci regions which is going to fail otherwise. Because
>>> grabbining the pci resources is in general the very first thing that
>>> those drivers need to do to setup anything, we
>>> remove_conflicting_devices first or at least very early.
>> To my knowledge, requesting resources is more about correctness than a hard requirement to use an I/O or memory range. Has this changed?
> Nope that is not correct.
>
> At least for AMD GPUs remove_conflicting_devices() really early is necessary because otherwise some operations just result in a spontaneous system reboot.
Here I was only talking about avoiding calls to request_resource() and
similar interfaces.
>
> For example resizing the PCIe BAR giving access to VRAM or disabling VGA emulation (which AFAIK is used for EFI as well) is only possible when the VGA or EFI framebuffer driver is kicked out first.
Yeah, that's what I expected.
>
> And disabling VGA emulation is among the absolutely first steps you do to take over the scanout config.
Assuming the driver (or driver author) is careful, is it possible to
only read state from AMD hardware at such an early time?
We usually do remove_conflicting_devices() as the first thing in most
driver's probe function. As a first step, it would be helpful to
postpone itto a later point.
>
> So I absolutely clearly have to reject the amdgpu patch in this series, that will break tons of use cases.
Don't worry, we're still in the early ideation phase.
Best regards
Thomas
>
> Regards,
> Christian.
>
>>> I also don't think it's possible or even desirable by some drivers to
>>> reuse the initial state, good example here is vmwgfx where by default
>>> some people will setup their vm's with e.g. 8mb ram, when the vmwgfx
>>> loads we allow scanning out from system memory, so you can set your vm
>>> up with 8mb of vram but still use 4k resolutions when the driver
>>> loads, this way the suspend size of the vm is very predictable (tiny
>>> vram plus whatever ram was setup) while still allowing a lot of
>>> flexibility.
>> If there's no initial state to switch from, the first modeset can fail while leaving the display unusable. There's no way around that. Going back to the old state is not an option unless the driver has been written to support this.
>>
>> The case of vmwgfx is special, but does not effect the overall problem. For vmwgfx, it would be best to import that initial state and support a transparent modeset from vram to system memory (and back) at least during this initial state.
>>
>>
>>> In general I think however this is planned it's two or three separate series:
>>> 1) infrastructure to reload the sysfb driver (what this series is)
>>> 2) making sure that drivers that do want to recover cleanly actually
>>> clean out all the state on exit properly,
>>> 3) abstracting at least some of that cleanup in some driver independent way
>> That's really not going to work. For example, in the current series, you invoke devm_aperture_remove_conflicting_pci_devices_done() after drm_mode_reset(), drm_dev_register() and drm_client_setup(). Each of these calls can modify hardware state. In the case of _register() and _setup(), the DRM clients can perform a modeset, which destroys the initial hardware state. Patch 1 of this series removes the sysfb device/driver entirely. That should be a no-go as it significantly complicates recovery. For example, if the native drivers failed from an allocation failure, the sysfb device/driver is not likely to come back either. As the very first thing, the series should state which failures is is going to resolve, - failed hardware init, - invalid initial modesetting, - runtime errors (such ENOMEM, failed firmware loading), - others? And then specify how a recovery to sysfb could look in each supported scenario. In terms of implementation, make any transition between drivers
>> gradually. The native driver needs to acquire the hardware resource (framebuffer and I/O apertures) without unloading the sysfb driver. Luckily there's struct drm_device.unplug, which does that. [1] Flipping this field disables hardware access for DRM drivers. All sysfb drivers support this. To get the sysfb drivers ready, I suggest dedicated helpers for each drivers aperture. The aperture helpers can use these callback to flip the DRM driver off and on again. For example, efidrm could do this as a minimum: int efidrm_aperture_suspend() { dev->unplug = true; remove_resource(/*framebuffer aperture*/) return 0 } int efidrm_aperture_resume() { insert_resource(/*framebuffer aperture*/) dev->unplug = false; return 0 } struct aperture_funcs efidrm_aperture_funcs { .suspend = efidrm_aperture_suspend, .resume = efidrm_aperture_resume, } Pass this struct when efidrm acquires the framebuffer aperture, so that the aperture helpers can control the behavior of efidrm. With this, a multi-
>> step takeover from sysfb to native driver can be tried. It's still a massive effort that requires an audit of each driver's probing logic. There's no copy-paste pattern AFAICT. I suggest to pick one simple driver first and make a prototype. Let me also say that I DO like the general idea you're proposing. But if it was easy, we would likely have done it already. Best regards Thomas
>>> z
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)
^ permalink raw reply
* Re: [PATCH 00/12] Recover sysfb after DRM probe failure
From: Christian König @ 2026-01-15 14:39 UTC (permalink / raw)
To: Thomas Zimmermann, Zack Rusin
Cc: dri-devel, Alex Deucher, amd-gfx, Ard Biesheuvel, Ce Sun,
Chia-I Wu, Danilo Krummrich, Dave Airlie, Deepak Rawat,
Dmitry Osipenko, Gerd Hoffmann, Gurchetan Singh, Hans de Goede,
Hawking Zhang, Helge Deller, intel-gfx, intel-xe, Jani Nikula,
Javier Martinez Canillas, Jocelyn Falempe, Joonas Lahtinen,
Lijo Lazar, linux-efi, linux-fbdev, linux-hyperv, linux-kernel,
Lucas De Marchi, Lyude Paul, Maarten Lankhorst,
Mario Limonciello (AMD), Mario Limonciello, Maxime Ripard,
nouveau, Rodrigo Vivi, Simona Vetter, spice-devel,
Thomas Hellström, Timur Kristóf, Tvrtko Ursulin,
virtualization, Vitaly Prosyak
In-Reply-To: <97993761-5884-4ada-b345-9fb64819e02a@suse.de>
Sorry to being late, but I only now realized what you are doing here.
On 1/15/26 12:02, Thomas Zimmermann wrote:
> Hi,
>
> apologies for the delay. I wanted to reply and then forgot about it.
>
> Am 10.01.26 um 05:52 schrieb Zack Rusin:
>> On Fri, Jan 9, 2026 at 5:34 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>> Hi
>>>
>>> Am 29.12.25 um 22:58 schrieb Zack Rusin:
>>>> Almost a rite of passage for every DRM developer and most Linux users
>>>> is upgrading your DRM driver/updating boot flags/changing some config
>>>> and having DRM driver fail at probe resulting in a blank screen.
>>>>
>>>> Currently there's no way to recover from DRM driver probe failure. PCI
>>>> DRM driver explicitly throw out the existing sysfb to get exclusive
>>>> access to PCI resources so if the probe fails the system is left without
>>>> a functioning display driver.
>>>>
>>>> Add code to sysfb to recever system framebuffer when DRM driver's probe
>>>> fails. This means that a DRM driver that fails to load reloads the system
>>>> framebuffer driver.
>>>>
>>>> This works best with simpledrm. Without it Xorg won't recover because
>>>> it still tries to load the vendor specific driver which ends up usually
>>>> not working at all. With simpledrm the system recovers really nicely
>>>> ending up with a working console and not a blank screen.
>>>>
>>>> There's a caveat in that some hardware might require some special magic
>>>> register write to recover EFI display. I'd appreciate it a lot if
>>>> maintainers could introduce a temporary failure in their drivers
>>>> probe to validate that the sysfb recovers and they get a working console.
>>>> The easiest way to double check it is by adding:
>>>> /* XXX: Temporary failure to test sysfb restore - REMOVE BEFORE COMMIT */
>>>> dev_info(&pdev->dev, "Testing sysfb restore: forcing probe failure\n");
>>>> ret = -EINVAL;
>>>> goto out_error;
>>>> or such right after the devm_aperture_remove_conflicting_pci_devices .
>>> Recovering the display like that is guess work and will at best work
>>> with simple discrete devices where the framebuffer is always located in
>>> a confined graphics aperture.
>>>
>>> But the problem you're trying to solve is a real one.
>>>
>>> What we'd want to do instead is to take the initial hardware state into
>>> account when we do the initial mode-setting operation.
>>>
>>> The first step is to move each driver's remove_conflicting_devices call
>>> to the latest possible location in the probe function. We usually do it
>>> first, because that's easy. But on most hardware, it could happen much
>>> later.
>> Well, some drivers (vbox, vmwgfx, bochs and currus-qemu) do it because
>> they request pci regions which is going to fail otherwise. Because
>> grabbining the pci resources is in general the very first thing that
>> those drivers need to do to setup anything, we
>> remove_conflicting_devices first or at least very early.
>
> To my knowledge, requesting resources is more about correctness than a hard requirement to use an I/O or memory range. Has this changed?
Nope that is not correct.
At least for AMD GPUs remove_conflicting_devices() really early is necessary because otherwise some operations just result in a spontaneous system reboot.
For example resizing the PCIe BAR giving access to VRAM or disabling VGA emulation (which AFAIK is used for EFI as well) is only possible when the VGA or EFI framebuffer driver is kicked out first.
And disabling VGA emulation is among the absolutely first steps you do to take over the scanout config.
So I absolutely clearly have to reject the amdgpu patch in this series, that will break tons of use cases.
Regards,
Christian.
>> I also don't think it's possible or even desirable by some drivers to
>> reuse the initial state, good example here is vmwgfx where by default
>> some people will setup their vm's with e.g. 8mb ram, when the vmwgfx
>> loads we allow scanning out from system memory, so you can set your vm
>> up with 8mb of vram but still use 4k resolutions when the driver
>> loads, this way the suspend size of the vm is very predictable (tiny
>> vram plus whatever ram was setup) while still allowing a lot of
>> flexibility.
>
> If there's no initial state to switch from, the first modeset can fail while leaving the display unusable. There's no way around that. Going back to the old state is not an option unless the driver has been written to support this.
>
> The case of vmwgfx is special, but does not effect the overall problem. For vmwgfx, it would be best to import that initial state and support a transparent modeset from vram to system memory (and back) at least during this initial state.
>
>
>>
>> In general I think however this is planned it's two or three separate series:
>> 1) infrastructure to reload the sysfb driver (what this series is)
>> 2) making sure that drivers that do want to recover cleanly actually
>> clean out all the state on exit properly,
>> 3) abstracting at least some of that cleanup in some driver independent way
>
> That's really not going to work. For example, in the current series, you invoke devm_aperture_remove_conflicting_pci_devices_done() after drm_mode_reset(), drm_dev_register() and drm_client_setup(). Each of these calls can modify hardware state. In the case of _register() and _setup(), the DRM clients can perform a modeset, which destroys the initial hardware state. Patch 1 of this series removes the sysfb device/driver entirely. That should be a no-go as it significantly complicates recovery. For example, if the native drivers failed from an allocation failure, the sysfb device/driver is not likely to come back either. As the very first thing, the series should state which failures is is going to resolve, - failed hardware init, - invalid initial modesetting, - runtime errors (such ENOMEM, failed firmware loading), - others? And then specify how a recovery to sysfb could look in each supported scenario. In terms of implementation, make any transition between drivers
> gradually. The native driver needs to acquire the hardware resource (framebuffer and I/O apertures) without unloading the sysfb driver. Luckily there's struct drm_device.unplug, which does that. [1] Flipping this field disables hardware access for DRM drivers. All sysfb drivers support this. To get the sysfb drivers ready, I suggest dedicated helpers for each drivers aperture. The aperture helpers can use these callback to flip the DRM driver off and on again. For example, efidrm could do this as a minimum: int efidrm_aperture_suspend() { dev->unplug = true; remove_resource(/*framebuffer aperture*/) return 0 } int efidrm_aperture_resume() { insert_resource(/*framebuffer aperture*/) dev->unplug = false; return 0 } struct aperture_funcs efidrm_aperture_funcs { .suspend = efidrm_aperture_suspend, .resume = efidrm_aperture_resume, } Pass this struct when efidrm acquires the framebuffer aperture, so that the aperture helpers can control the behavior of efidrm. With this, a multi-
> step takeover from sysfb to native driver can be tried. It's still a massive effort that requires an audit of each driver's probing logic. There's no copy-paste pattern AFAICT. I suggest to pick one simple driver first and make a prototype. Let me also say that I DO like the general idea you're proposing. But if it was easy, we would likely have done it already. Best regards Thomas
>>
>> z
>
^ permalink raw reply
* [syzbot] Monthly fbdev report (Jan 2026)
From: syzbot @ 2026-01-15 13:09 UTC (permalink / raw)
To: deller, dri-devel, linux-fbdev, linux-kernel, syzkaller-bugs
Hello fbdev maintainers/developers,
This is a 31-day syzbot report for the fbdev subsystem.
All related reports/information can be found at:
https://syzkaller.appspot.com/upstream/s/fbdev
During the period, 0 new issues were detected and 0 were fixed.
In total, 6 issues are still open and 29 have already been fixed.
Some of the still happening issues:
Ref Crashes Repro Title
<1> 1171 Yes KASAN: slab-out-of-bounds Read in fbcon_prepare_logo
https://syzkaller.appspot.com/bug?extid=0c815b25cdb3678e7083
<2> 751 Yes KASAN: vmalloc-out-of-bounds Write in imageblit (6)
https://syzkaller.appspot.com/bug?extid=5a40432dfe8f86ee657a
<3> 140 No KASAN: vmalloc-out-of-bounds Write in fillrect
https://syzkaller.appspot.com/bug?extid=7a63ce155648954e749b
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
To disable reminders for individual bugs, reply with the following command:
#syz set <Ref> no-reminders
To change bug's subsystems, reply with:
#syz set <Ref> subsystems: new-subsystem
You may send multiple commands in a single email message.
^ permalink raw reply
* Re: [PATCH v3] staging: sm750fb: Convert sw_i2c_read_sda to return bool
From: Karthikey Kadati @ 2026-01-15 12:50 UTC (permalink / raw)
To: Greg KH
Cc: sudipm.mukherjee, teddy.wang, linux-fbdev, linux-staging,
linux-kernel
In-Reply-To: <2026011521-regally-lunchroom-5602@gregkh>
You are right. I overlooked that the return value is used in bitwise
shift operations used to construct the data byte.
Since `sw_i2c_read_sda()` returns a raw data bit (0 or 1) intended for
shifting, `bool` is semantically incorrect here.
I will drop this patch.
Thanks,
Karthikey
On Thu, 15 Jan 2026 at 17:02, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jan 14, 2026 at 10:47:48PM +0530, Karthikey Kadati wrote:
> > The sw_i2c_read_sda() function currently returns unsigned char (1 or 0).
> > Standardize it to return bool (true or false) to match kernel standards.
> >
> > Signed-off-by: Karthikey Kadati <karthikey3608@gmail.com>
> > ---
> > v3:
> > - Add version history (Reported by kernel test robot).
> > v2:
> > - Fix invalid "Unix Antigravity" Signed-off-by.
> > - Submit as standalone patch (detached from unrelated series).
> >
> > drivers/staging/sm750fb/ddk750_swi2c.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/sm750fb/ddk750_swi2c.c b/drivers/staging/sm750fb/ddk750_swi2c.c
> > index 0ef8d4ff2..9d48673d3 100644
> > --- a/drivers/staging/sm750fb/ddk750_swi2c.c
> > +++ b/drivers/staging/sm750fb/ddk750_swi2c.c
> > @@ -180,7 +180,7 @@ static void sw_i2c_sda(unsigned char value)
> > * Return Value:
> > * The SDA data bit sent by the Slave
> > */
> > -static unsigned char sw_i2c_read_sda(void)
> > +static bool sw_i2c_read_sda(void)
>
> So how does this call:
> data |= (sw_i2c_read_sda() << i);
> work with a boolean?
>
> confused,
>
> greg k-h
^ permalink raw reply
* Re: [PATCH v3] staging: sm750fb: Convert sw_i2c_read_sda to return bool
From: Greg KH @ 2026-01-15 11:32 UTC (permalink / raw)
To: Karthikey Kadati
Cc: sudipm.mukherjee, teddy.wang, linux-fbdev, linux-staging,
linux-kernel
In-Reply-To: <20260114171748.34767-1-karthikey3608@gmail.com>
On Wed, Jan 14, 2026 at 10:47:48PM +0530, Karthikey Kadati wrote:
> The sw_i2c_read_sda() function currently returns unsigned char (1 or 0).
> Standardize it to return bool (true or false) to match kernel standards.
>
> Signed-off-by: Karthikey Kadati <karthikey3608@gmail.com>
> ---
> v3:
> - Add version history (Reported by kernel test robot).
> v2:
> - Fix invalid "Unix Antigravity" Signed-off-by.
> - Submit as standalone patch (detached from unrelated series).
>
> drivers/staging/sm750fb/ddk750_swi2c.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/sm750fb/ddk750_swi2c.c b/drivers/staging/sm750fb/ddk750_swi2c.c
> index 0ef8d4ff2..9d48673d3 100644
> --- a/drivers/staging/sm750fb/ddk750_swi2c.c
> +++ b/drivers/staging/sm750fb/ddk750_swi2c.c
> @@ -180,7 +180,7 @@ static void sw_i2c_sda(unsigned char value)
> * Return Value:
> * The SDA data bit sent by the Slave
> */
> -static unsigned char sw_i2c_read_sda(void)
> +static bool sw_i2c_read_sda(void)
So how does this call:
data |= (sw_i2c_read_sda() << i);
work with a boolean?
confused,
greg k-h
^ permalink raw reply
* Re: [PATCH 00/12] Recover sysfb after DRM probe failure
From: Thomas Zimmermann @ 2026-01-15 11:02 UTC (permalink / raw)
To: Zack Rusin
Cc: dri-devel, Alex Deucher, amd-gfx, Ard Biesheuvel, Ce Sun,
Chia-I Wu, Christian König, Danilo Krummrich, Dave Airlie,
Deepak Rawat, Dmitry Osipenko, Gerd Hoffmann, Gurchetan Singh,
Hans de Goede, Hawking Zhang, Helge Deller, intel-gfx, intel-xe,
Jani Nikula, Javier Martinez Canillas, Jocelyn Falempe,
Joonas Lahtinen, Lijo Lazar, linux-efi, linux-fbdev, linux-hyperv,
linux-kernel, Lucas De Marchi, Lyude Paul, Maarten Lankhorst,
Mario Limonciello (AMD), Mario Limonciello, Maxime Ripard,
nouveau, Rodrigo Vivi, Simona Vetter, spice-devel,
Thomas Hellström, Timur Kristóf, Tvrtko Ursulin,
virtualization, Vitaly Prosyak
In-Reply-To: <CABQX2QNQU4XZ1rJFqnJeMkz8WP=t9atj0BqXHbDQab7ZnAyJxg@mail.gmail.com>
Hi,
apologies for the delay. I wanted to reply and then forgot about it.
Am 10.01.26 um 05:52 schrieb Zack Rusin:
> On Fri, Jan 9, 2026 at 5:34 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 29.12.25 um 22:58 schrieb Zack Rusin:
>>> Almost a rite of passage for every DRM developer and most Linux users
>>> is upgrading your DRM driver/updating boot flags/changing some config
>>> and having DRM driver fail at probe resulting in a blank screen.
>>>
>>> Currently there's no way to recover from DRM driver probe failure. PCI
>>> DRM driver explicitly throw out the existing sysfb to get exclusive
>>> access to PCI resources so if the probe fails the system is left without
>>> a functioning display driver.
>>>
>>> Add code to sysfb to recever system framebuffer when DRM driver's probe
>>> fails. This means that a DRM driver that fails to load reloads the system
>>> framebuffer driver.
>>>
>>> This works best with simpledrm. Without it Xorg won't recover because
>>> it still tries to load the vendor specific driver which ends up usually
>>> not working at all. With simpledrm the system recovers really nicely
>>> ending up with a working console and not a blank screen.
>>>
>>> There's a caveat in that some hardware might require some special magic
>>> register write to recover EFI display. I'd appreciate it a lot if
>>> maintainers could introduce a temporary failure in their drivers
>>> probe to validate that the sysfb recovers and they get a working console.
>>> The easiest way to double check it is by adding:
>>> /* XXX: Temporary failure to test sysfb restore - REMOVE BEFORE COMMIT */
>>> dev_info(&pdev->dev, "Testing sysfb restore: forcing probe failure\n");
>>> ret = -EINVAL;
>>> goto out_error;
>>> or such right after the devm_aperture_remove_conflicting_pci_devices .
>> Recovering the display like that is guess work and will at best work
>> with simple discrete devices where the framebuffer is always located in
>> a confined graphics aperture.
>>
>> But the problem you're trying to solve is a real one.
>>
>> What we'd want to do instead is to take the initial hardware state into
>> account when we do the initial mode-setting operation.
>>
>> The first step is to move each driver's remove_conflicting_devices call
>> to the latest possible location in the probe function. We usually do it
>> first, because that's easy. But on most hardware, it could happen much
>> later.
> Well, some drivers (vbox, vmwgfx, bochs and currus-qemu) do it because
> they request pci regions which is going to fail otherwise. Because
> grabbining the pci resources is in general the very first thing that
> those drivers need to do to setup anything, we
> remove_conflicting_devices first or at least very early.
To my knowledge, requesting resources is more about correctness than a
hard requirement to use an I/O or memory range. Has this changed?
>
> I also don't think it's possible or even desirable by some drivers to
> reuse the initial state, good example here is vmwgfx where by default
> some people will setup their vm's with e.g. 8mb ram, when the vmwgfx
> loads we allow scanning out from system memory, so you can set your vm
> up with 8mb of vram but still use 4k resolutions when the driver
> loads, this way the suspend size of the vm is very predictable (tiny
> vram plus whatever ram was setup) while still allowing a lot of
> flexibility.
If there's no initial state to switch from, the first modeset can fail
while leaving the display unusable. There's no way around that. Going
back to the old state is not an option unless the driver has been
written to support this.
The case of vmwgfx is special, but does not effect the overall problem.
For vmwgfx, it would be best to import that initial state and support a
transparent modeset from vram to system memory (and back) at least
during this initial state.
>
> In general I think however this is planned it's two or three separate series:
> 1) infrastructure to reload the sysfb driver (what this series is)
> 2) making sure that drivers that do want to recover cleanly actually
> clean out all the state on exit properly,
> 3) abstracting at least some of that cleanup in some driver independent way
That's really not going to work. For example, in the current series, you
invoke devm_aperture_remove_conflicting_pci_devices_done() after
drm_mode_reset(), drm_dev_register() and drm_client_setup(). Each of
these calls can modify hardware state. In the case of _register() and
_setup(), the DRM clients can perform a modeset, which destroys the
initial hardware state. Patch 1 of this series removes the sysfb
device/driver entirely. That should be a no-go as it significantly
complicates recovery. For example, if the native drivers failed from an
allocation failure, the sysfb device/driver is not likely to come back
either. As the very first thing, the series should state which failures
is is going to resolve, - failed hardware init, - invalid initial
modesetting, - runtime errors (such ENOMEM, failed firmware loading), -
others? And then specify how a recovery to sysfb could look in each
supported scenario. In terms of implementation, make any transition
between drivers gradually. The native driver needs to acquire the
hardware resource (framebuffer and I/O apertures) without unloading the
sysfb driver. Luckily there's struct drm_device.unplug, which does that.
[1] Flipping this field disables hardware access for DRM drivers. All
sysfb drivers support this. To get the sysfb drivers ready, I suggest
dedicated helpers for each drivers aperture. The aperture helpers can
use these callback to flip the DRM driver off and on again. For example,
efidrm could do this as a minimum: int efidrm_aperture_suspend() {
dev->unplug = true; remove_resource(/*framebuffer aperture*/) return 0 }
int efidrm_aperture_resume() { insert_resource(/*framebuffer aperture*/)
dev->unplug = false; return 0 } struct aperture_funcs
efidrm_aperture_funcs { .suspend = efidrm_aperture_suspend, .resume =
efidrm_aperture_resume, } Pass this struct when efidrm acquires the
framebuffer aperture, so that the aperture helpers can control the
behavior of efidrm. With this, a multi-step takeover from sysfb to
native driver can be tried. It's still a massive effort that requires an
audit of each driver's probing logic. There's no copy-paste pattern
AFAICT. I suggest to pick one simple driver first and make a prototype.
Let me also say that I DO like the general idea you're proposing. But if
it was easy, we would likely have done it already. Best regards Thomas
>
> z
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)
^ permalink raw reply
* Re: [PATCH] staging: fbtft: replace udelay with usleep_range
From: Greg Kroah-Hartman @ 2026-01-15 8:59 UTC (permalink / raw)
To: WooYoung Jeon
Cc: Andy Shevchenko, dri-devel, linux-fbdev, linux-staging,
linux-kernel
In-Reply-To: <20260115084019.28574-1-chococookieman1@gmail.com>
On Thu, Jan 15, 2026 at 05:40:19PM +0900, WooYoung Jeon wrote:
> In the fb_ra8875 driver, udelay(100) is used for delay which
> causes busy-waiting. Replacing it with usleep_range(100, 120)
> allows the CPU to sleep during the delay, improving system
> resource efficiency.
>
> This change was suggested by checkpatch.pl.
>
> Signed-off-by: WooYoung Jeon <chococookieman1@gmail.com>
> ---
> drivers/staging/fbtft/fb_ra8875.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/fbtft/fb_ra8875.c b/drivers/staging/fbtft/fb_ra8875.c
> index 0ab1de664..92c9e4e03 100644
> --- a/drivers/staging/fbtft/fb_ra8875.c
> +++ b/drivers/staging/fbtft/fb_ra8875.c
> @@ -210,7 +210,7 @@ static void write_reg8_bus8(struct fbtft_par *par, int len, ...)
> }
> len--;
>
> - udelay(100);
> + usleep_range(100, 120);
>
> if (len) {
> buf = (u8 *)par->buf;
> --
> 2.43.0
>
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- You sent a patch that has been sent multiple times in the past and is
identical to ones that have been rejected. Please always look at the
mailing list traffic to determine if you are duplicating other
people's work.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
^ permalink raw reply
* Re: [PATCH] staging: fbtft: replace udelay with usleep_range
From: Andy Shevchenko @ 2026-01-15 8:47 UTC (permalink / raw)
To: WooYoung Jeon
Cc: Andy Shevchenko, Greg Kroah-Hartman, dri-devel, linux-fbdev,
linux-staging, linux-kernel
In-Reply-To: <20260115084019.28574-1-chococookieman1@gmail.com>
On Thu, Jan 15, 2026 at 05:40:19PM +0900, WooYoung Jeon wrote:
> In the fb_ra8875 driver, udelay(100) is used for delay which
> causes busy-waiting. Replacing it with usleep_range(100, 120)
> allows the CPU to sleep during the delay, improving system
> resource efficiency.
>
> This change was suggested by checkpatch.pl.
Without HW test it's no go. See the previous attempts to "fix" the same place
over and over. (https://lore.kernel.org/ is at your service to dive into mail
archives)
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH] staging: fbtft: replace udelay with usleep_range
From: WooYoung Jeon @ 2026-01-15 8:40 UTC (permalink / raw)
To: Andy Shevchenko, Greg Kroah-Hartman
Cc: dri-devel, linux-fbdev, linux-staging, linux-kernel,
WooYoung Jeon
In the fb_ra8875 driver, udelay(100) is used for delay which
causes busy-waiting. Replacing it with usleep_range(100, 120)
allows the CPU to sleep during the delay, improving system
resource efficiency.
This change was suggested by checkpatch.pl.
Signed-off-by: WooYoung Jeon <chococookieman1@gmail.com>
---
drivers/staging/fbtft/fb_ra8875.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/fbtft/fb_ra8875.c b/drivers/staging/fbtft/fb_ra8875.c
index 0ab1de664..92c9e4e03 100644
--- a/drivers/staging/fbtft/fb_ra8875.c
+++ b/drivers/staging/fbtft/fb_ra8875.c
@@ -210,7 +210,7 @@ static void write_reg8_bus8(struct fbtft_par *par, int len, ...)
}
len--;
- udelay(100);
+ usleep_range(100, 120);
if (len) {
buf = (u8 *)par->buf;
--
2.43.0
^ permalink raw reply related
* [PATCH v2 01/12] firmware: google: framebuffer: Do not unregister platform device
From: Thomas Zimmermann @ 2026-01-15 7:57 UTC (permalink / raw)
To: tzungbi, briannorris, jwerner, javierm, samuel, maarten.lankhorst,
mripard, airlied, simona
Cc: chrome-platform, dri-devel, Thomas Zimmermann, Hans de Goede,
linux-fbdev, stable
In-Reply-To: <20260115082128.12460-1-tzimmermann@suse.de>
The native driver takes over the framebuffer aperture by removing the
system- framebuffer platform device. Afterwards the pointer in drvdata
is dangling. Remove the entire logic around drvdata and let the kernel's
aperture helpers handle this. The platform device depends on the native
hardware device instead of the coreboot device anyway.
When commit 851b4c14532d ("firmware: coreboot: Add coreboot framebuffer
driver") added the coreboot framebuffer code, the kernel did not support
device-based aperture management. Instead native driviers only removed
the conflicting fbdev device. At that point, unregistering the framebuffer
device most likely worked correctly. It was definitely broken after
commit d9702b2a2171 ("fbdev/simplefb: Do not use struct
fb_info.apertures"). So take this commit for the Fixes tag. Earlier
releases might work depending on the native hardware driver.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: d9702b2a2171 ("fbdev/simplefb: Do not use struct fb_info.apertures")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Hans de Goede <hansg@kernel.org>
Cc: linux-fbdev@vger.kernel.org
Cc: <stable@vger.kernel.org> # v6.3+
---
drivers/firmware/google/framebuffer-coreboot.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
index c68c9f56370f..4e9177105992 100644
--- a/drivers/firmware/google/framebuffer-coreboot.c
+++ b/drivers/firmware/google/framebuffer-coreboot.c
@@ -81,19 +81,10 @@ static int framebuffer_probe(struct coreboot_device *dev)
sizeof(pdata));
if (IS_ERR(pdev))
pr_warn("coreboot: could not register framebuffer\n");
- else
- dev_set_drvdata(&dev->dev, pdev);
return PTR_ERR_OR_ZERO(pdev);
}
-static void framebuffer_remove(struct coreboot_device *dev)
-{
- struct platform_device *pdev = dev_get_drvdata(&dev->dev);
-
- platform_device_unregister(pdev);
-}
-
static const struct coreboot_device_id framebuffer_ids[] = {
{ .tag = CB_TAG_FRAMEBUFFER },
{ /* sentinel */ }
@@ -102,7 +93,6 @@ MODULE_DEVICE_TABLE(coreboot, framebuffer_ids);
static struct coreboot_driver framebuffer_driver = {
.probe = framebuffer_probe,
- .remove = framebuffer_remove,
.drv = {
.name = "framebuffer",
},
--
2.52.0
^ permalink raw reply related
* Re: [PATCH v6] staging: fbtft: Use fbdev logging helpers when FB_DEVICE is disabled
From: Thomas Zimmermann @ 2026-01-15 7:55 UTC (permalink / raw)
To: Chintan Patel, linux-fbdev, linux-staging, linux-omap
Cc: linux-kernel, dri-devel, andy, deller, gregkh, kernel test robot
In-Reply-To: <20260113045909.336931-1-chintanlike@gmail.com>
Hi
Am 13.01.26 um 05:59 schrieb Chintan Patel:
> Replace direct accesses to info->dev with fb_dbg() and fb_info()
> helpers to avoid build failures when CONFIG_FB_DEVICE=n.
>
> Fixes: a06d03f9f238 ("staging: fbtft: Make FB_DEVICE dependency optional")
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202601110740.Y9XK5HtN-lkp@intel.com
> Signed-off-by: Chintan Patel <chintanlike@gmail.com>
>
> Changes in v6:
> - Switch debug/info logging to fb_dbg() and fb_info()(suggested by Thomas Zimmermann)
> - Drop dev_of_fbinfo() usage in favor of framebuffer helpers that implicitly
> handle the debug/info context.
> - Drop __func__ usage per review feedback(suggested by greg k-h)
> - Add Fixes tag for a06d03f9f238 ("staging: fbtft: Make FB_DEVICE dependency optional")
> (suggested by Andy Shevchenko)
>
> Changes in v5:
> - Initial attempt to replace info->dev accesses using
> dev_of_fbinfo() helper
> ---
> drivers/staging/fbtft/fbtft-core.c | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
> index 8a5ccc8ae0a1..1b3b62950205 100644
> --- a/drivers/staging/fbtft/fbtft-core.c
> +++ b/drivers/staging/fbtft/fbtft-core.c
> @@ -365,9 +365,9 @@ static int fbtft_fb_setcolreg(unsigned int regno, unsigned int red,
> unsigned int val;
> int ret = 1;
>
> - dev_dbg(info->dev,
> - "%s(regno=%u, red=0x%X, green=0x%X, blue=0x%X, trans=0x%X)\n",
> - __func__, regno, red, green, blue, transp);
> + fb_dbg(info,
> + "regno=%u, red=0x%X, green=0x%X, blue=0x%X, trans=0x%X\n",
> + regno, red, green, blue, transp);
>
> switch (info->fix.visual) {
> case FB_VISUAL_TRUECOLOR:
> @@ -391,8 +391,7 @@ static int fbtft_fb_blank(int blank, struct fb_info *info)
> struct fbtft_par *par = info->par;
> int ret = -EINVAL;
>
> - dev_dbg(info->dev, "%s(blank=%d)\n",
> - __func__, blank);
> + fb_dbg(info, "blank=%d\n", blank);
>
> if (!par->fbtftops.blank)
> return ret;
> @@ -793,11 +792,11 @@ int fbtft_register_framebuffer(struct fb_info *fb_info)
> if (spi)
> sprintf(text2, ", spi%d.%d at %d MHz", spi->controller->bus_num,
> spi_get_chipselect(spi, 0), spi->max_speed_hz / 1000000);
> - dev_info(fb_info->dev,
> - "%s frame buffer, %dx%d, %d KiB video memory%s, fps=%lu%s\n",
> - fb_info->fix.id, fb_info->var.xres, fb_info->var.yres,
> - fb_info->fix.smem_len >> 10, text1,
> - HZ / fb_info->fbdefio->delay, text2);
> + fb_info(fb_info,
> + "%s frame buffer, %dx%d, %d KiB video memory%s, fps=%lu%s\n",
> + fb_info->fix.id, fb_info->var.xres, fb_info->var.yres,
> + fb_info->fix.smem_len >> 10, text1,
> + HZ / fb_info->fbdefio->delay, text2);
As discussed before, this should become fb_dbg(). Drivers should not
print status reports unless they do not work as expected.
Best regards
Thomas
>
> /* Turn on backlight if available */
> if (fb_info->bl_dev) {
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)
^ permalink raw reply
* Re: [PATCH v6] staging: fbtft: Use fbdev logging helpers when FB_DEVICE is disabled
From: Chintan Patel @ 2026-01-15 4:05 UTC (permalink / raw)
To: Thomas Zimmermann, Greg KH
Cc: linux-fbdev, linux-staging, linux-omap, linux-kernel, dri-devel,
andy, deller, kernel test robot
In-Reply-To: <a2d5cc20-5160-4294-bda1-3d5b645ec787@suse.de>
On 1/14/26 03:38, Thomas Zimmermann wrote:
> Hi
>
> Am 13.01.26 um 07:16 schrieb Greg KH:
>> On Mon, Jan 12, 2026 at 08:59:09PM -0800, Chintan Patel wrote:
>>> Replace direct accesses to info->dev with fb_dbg() and fb_info()
>>> helpers to avoid build failures when CONFIG_FB_DEVICE=n.
>> Why is there a fb_* specific logging helper? dev_info() and dev_dbg()
>> should be used instead.
>
> Fbdev is entirely inconsistent about its logging. There's dev_*(),
> there's pr_*(), and even printk(). The problem with dev_*() logging is
> that devices are not always available. The HW device can be NULL and
> might not be all that useful in practice. The Fbdev software device is
> often not even compiled in nowadays. (This patch is about that problem.)
> Hence the next best option is to make fb_*() logging helpers that
> address these problems. They are based on pr_*() and print the
> framebuffer index, which should always be available after
> register_framebuffer().
>
>>
Thanks Andy and Thomas.
I’ll update the commit message to clearly describe the underlying issue.
I’ll also split the changes as suggested in 2 patches and send v7:
1) a patch focused purely on fixing the compilation issue by avoiding
info->dev dereferences (using fb_dbg() where logging remains), and
2) a follow-up cleanup that removes or downgrades the framebuffer
registration message to debug level.
I’ll rework the series accordingly and resend.
Thanks for the guidance.
^ permalink raw reply
* [PATCH v3] staging: sm750fb: Convert sw_i2c_read_sda to return bool
From: Karthikey Kadati @ 2026-01-14 17:17 UTC (permalink / raw)
To: sudipm.mukherjee, teddy.wang, gregkh
Cc: linux-fbdev, linux-staging, linux-kernel, karthikey3608
The sw_i2c_read_sda() function currently returns unsigned char (1 or 0).
Standardize it to return bool (true or false) to match kernel standards.
Signed-off-by: Karthikey Kadati <karthikey3608@gmail.com>
---
v3:
- Add version history (Reported by kernel test robot).
v2:
- Fix invalid "Unix Antigravity" Signed-off-by.
- Submit as standalone patch (detached from unrelated series).
drivers/staging/sm750fb/ddk750_swi2c.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/sm750fb/ddk750_swi2c.c b/drivers/staging/sm750fb/ddk750_swi2c.c
index 0ef8d4ff2..9d48673d3 100644
--- a/drivers/staging/sm750fb/ddk750_swi2c.c
+++ b/drivers/staging/sm750fb/ddk750_swi2c.c
@@ -180,7 +180,7 @@ static void sw_i2c_sda(unsigned char value)
* Return Value:
* The SDA data bit sent by the Slave
*/
-static unsigned char sw_i2c_read_sda(void)
+static bool sw_i2c_read_sda(void)
{
unsigned long gpio_dir;
unsigned long gpio_data;
@@ -196,9 +196,9 @@ static unsigned char sw_i2c_read_sda(void)
/* Now read the SDA line */
gpio_data = peek32(sw_i2c_data_gpio_data_reg);
if (gpio_data & (1 << sw_i2c_data_gpio))
- return 1;
+ return true;
else
- return 0;
+ return false;
}
/*
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v6 2/4] backlight: add max25014atg backlight
From: Daniel Thompson @ 2026-01-14 16:29 UTC (permalink / raw)
To: Maud Spierings
Cc: Daniel Thompson, Lee Jones, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Liam Girdwood, Mark Brown, dri-devel, linux-leds, devicetree,
linux-kernel, linux-fbdev, imx, linux-arm-kernel
In-Reply-To: <fc5aad54-08fe-453e-a3cf-621414c8a060@gocontroll.com>
On Fri, Jan 09, 2026 at 09:55:18AM +0100, Maud Spierings wrote:
> Do you have any comments about:
>
> > +static void max25014_remove(struct i2c_client *cl)
> > +{
> > + struct max25014 *maxim = i2c_get_clientdata(cl);
> > +
> > + maxim->bl->props.brightness = 0;
> > + max25014_update_status(maxim->bl);
> > + gpiod_set_value_cansleep(maxim->enable, 0);
> > + regulator_disable(maxim->vin);
> > +}
>
> I'm feeling like the setting of the brightness + update status maybe should
> be a call to backlight_device_set_brightness() or maybe it shouldn't really
> be there at all?
Using backlight_device_set_brightness() makes sense (although there is
still a window where userspace could come back in and turn the backlight
on again). And, if both the GPIO and regulator were optional then it is
sensible to set the brightness to zero before removing the driver.
Daniel.
^ permalink raw reply
* Re: [PATCH v1 2/2] backlight: gpio: add support for multiple GPIOs for backlight control
From: Daniel Thompson @ 2026-01-14 16:03 UTC (permalink / raw)
To: tessolveupstream
Cc: lee, danielt, jingoohan1, deller, pavel, robh, krzk+dt, conor+dt,
dri-devel, linux-fbdev, linux-leds, devicetree, linux-kernel
In-Reply-To: <1fedb7d7-3a30-4f0f-961f-09613f2a95d0@gmail.com>
On Tue, Jan 13, 2026 at 12:47:26PM +0530, tessolveupstream@gmail.com wrote:
>
>
> On 05-01-2026 15:39, Daniel Thompson wrote:
> > On Mon, Jan 05, 2026 at 02:21:20PM +0530, Sudarshan Shetty wrote:
> >> Extend the gpio-backlight driver to handle multiple GPIOs instead of a
> >> single one. This allows panels that require driving several enable pins
> >> to be controlled by the backlight framework.
> >>
> >> Signed-off-by: Sudarshan Shetty <tessolveupstream@gmail.com>
> >> ---
> >> drivers/video/backlight/gpio_backlight.c | 61 +++++++++++++++++-------
> >> 1 file changed, 45 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
> >> index 728a546904b0..037e1c111e48 100644
> >> --- a/drivers/video/backlight/gpio_backlight.c
> >> +++ b/drivers/video/backlight/gpio_backlight.c
> >> @@ -17,14 +17,18 @@
> >>
> >> struct gpio_backlight {
> >> struct device *dev;
> >> - struct gpio_desc *gpiod;
> >> + struct gpio_desc **gpiods;
> >> + unsigned int num_gpios;
> >
> > Why not use struct gpio_descs for this?
> >
> > Once you do that, then most of the gbl->num_gpios loops can be replaced with
> > calls to the array based accessors.
> >
>
> Based on your feedback, I have updated the implementation to use
> struct gpio_descs and array-based accessors, as recommended like
> below:
>
> git diff drivers/video/backlight/gpio_backlight.c
> diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
> index 037e1c111e48..e99d7a9dc670 100644
> --- a/drivers/video/backlight/gpio_backlight.c
> +++ b/drivers/video/backlight/gpio_backlight.c
> @@ -14,22 +14,37 @@
> #include <linux/platform_device.h>
> #include <linux/property.h>
> #include <linux/slab.h>
> +#include <linux/bitmap.h>
>
> struct gpio_backlight {
> struct device *dev;
> - struct gpio_desc **gpiods;
> + struct gpio_descs *gpiods;
> unsigned int num_gpios;
> };
>
> static int gpio_backlight_update_status(struct backlight_device *bl)
> {
> struct gpio_backlight *gbl = bl_get_data(bl);
> - unsigned int i;
> + unsigned int n = gbl->num_gpios;
> int br = backlight_get_brightness(bl);
> + unsigned long *value_bitmap;
> + int words = BITS_TO_LONGS(n);
> +
> + value_bitmap = kcalloc(words, sizeof(unsigned long), GFP_KERNEL);
Not sure you need a kcalloc() here. If you want to support more than 32
GPIOs then you can pre-allocate space with a devm_kcalloc() in the probe
method rather than reallocate every time it is used.
To be honest I don't really mind putting a hard limit on the maximum
gpl->num_gpios (so you can just use a local variable) and having no
allocation at all.
> Could you please share your thoughts on whether this approach
> aligns with your expectations?
Looks like it is going in the right direction, yes.
Daniel.
^ permalink raw reply
* Re: [PATCH v1 1/2] dt-bindings: backlight: gpio-backlight: allow multiple GPIOs
From: Daniel Thompson @ 2026-01-14 15:53 UTC (permalink / raw)
To: tessolveupstream
Cc: lee, danielt, jingoohan1, deller, pavel, robh, krzk+dt, conor+dt,
dri-devel, linux-fbdev, linux-leds, devicetree, linux-kernel
In-Reply-To: <c182df66-8503-49cf-8d1d-7da17214b843@gmail.com>
On Tue, Jan 13, 2026 at 10:15:53AM +0530, tessolveupstream@gmail.com wrote:
>
>
> On 05-01-2026 15:25, Daniel Thompson wrote:
> > On Mon, Jan 05, 2026 at 02:21:19PM +0530, Sudarshan Shetty wrote:
> >> Update the gpio-backlight binding to support configurations that require
> >> more than one GPIO for enabling/disabling the backlight.
> >>
> >> Signed-off-by: Sudarshan Shetty <tessolveupstream@gmail.com>
> >> ---
> >> .../bindings/leds/backlight/gpio-backlight.yaml | 12 +++++++++++-
> >> 1 file changed, 11 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/leds/backlight/gpio-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/gpio-backlight.yaml
> >> index 584030b6b0b9..1483ce4a3480 100644
> >> --- a/Documentation/devicetree/bindings/leds/backlight/gpio-backlight.yaml
> >> +++ b/Documentation/devicetree/bindings/leds/backlight/gpio-backlight.yaml
> >> @@ -17,7 +17,8 @@ properties:
> >>
> >> gpios:
> >> description: The gpio that is used for enabling/disabling the backlight.
> >> - maxItems: 1
> >> + minItems: 1
> >> + maxItems: 2
> >
> > Why 2?
> >
>
> In the current design, the LVDS panel has a single backlight that
> is controlled by two GPIOs. Initially, It described as two separate
> backlight devices using the same gpio-backlight driver, since the
> existing driver supports only one GPIO per instance.
>
> So the maintainer suggested to extend the gpio-backlight driver
> and bindings to support multiple GPIOs.
> https://lore.kernel.org/all/q63bdon55app4gb2il5e7skyc6z2amcnaiqbqlhen7arkxphtb@3jejbelji2ti/
Right. So, once we support multiple GPIOs then why limit it to 2?
Daniel.
^ permalink raw reply
* printk's threaded legacy console + fbcon => schedule where it should not
From: Sebastian Andrzej Siewior @ 2026-01-14 14:59 UTC (permalink / raw)
To: linux-kernel, linux-serial, linux-fbdev
Cc: Petr Mladek, Steven Rostedt, John Ogness, Sergey Senozhatsky,
Greg Kroah-Hartman, Jiri Slaby, Simona Vetter, Helge Deller
Hi,
legacy_kthread_func() does console_lock() which means
console_may_schedule is 1.
The other path is from vprintk_emit() where we have
if (ft.legacy_direct) {
preempt_disable();
if (console_trylock_spinning())
console_unlock();
preempt_enable();
}
so all printing happens from console_unlock() where
console_may_schedule is 0. This is a small difference. With the legacy
console enabled I get:
| BUG: sleeping function called from invalid context at kernel/printk/printk.c:3377
| in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 15, name: pr/legacy
| preempt_count: 1, expected: 0
| RCU nest depth: 0, expected: 0
| 3 locks held by pr/legacy/15:
| #0: ffffffffa8aebac0 (console_lock){+.+.}-{0:0}, at: legacy_kthread_func+0x6c/0x130
| #1: ffffffffa8aebb18 (console_srcu){....}-{0:0}, at: console_flush_one_record+0x7e/0x4d0
| #2: ffffffffa8c49818 (printing_lock){+.+.}-{3:3}, at: vt_console_print+0x55/0x490
| Preemption disabled at:
| [<0000000000000000>] 0x0
| CPU: 7 UID: 0 PID: 15 Comm: pr/legacy Not tainted 6.19.0-rc5+ #19 PREEMPT(lazy)
| Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./Z68 Pro3-M, BIOS P2.30 06/29/2012
| Call Trace:
| <TASK>
| dump_stack_lvl+0x68/0x90
| __might_resched.cold+0xf0/0x12b
| console_conditional_schedule+0x27/0x30
| fbcon_redraw+0xa0/0x240
| fbcon_scroll+0x164/0x1c0
| con_scroll+0xfa/0x200
| lf+0xa5/0xb0
| vt_console_print+0x313/0x490
| console_flush_one_record+0x2a0/0x4d0
| legacy_kthread_func+0x83/0x130
| kthread+0x118/0x250
| ret_from_fork+0x309/0x3b0
| ret_from_fork_asm+0x1a/0x30
| </TASK>
because vt_console_print() acquires a spin_lock for synchronisation
against another caller while console_conditional_schedule() would like
to schedule.
Most callers of console_unlock() do trylock except for few such as
__pr_flush() which are affected by this the same way as the legacy
printing thread. But we don't have much pr_flush() so this is hidden.
Is there a strict need for fbcon_scroll() to schedule in fbcon_redraw()?
From a quick look it looks that intense callers such the printk flush do
cond_resched() on their own and tty does it, too
| fbcon_scroll+0x164/0x1c0
| con_scroll+0xfa/0x200
| lf+0xa5/0xb0
| do_con_write+0xc68/0x2630
| con_write+0xf/0x40
| do_output_char+0x180/0x1e0
| n_tty_write+0x1ba/0x580
| file_tty_write.isra.0+0x17e/0x2c0
the cond_resched() is in file_tty_write()/ iterate_tty_write().
Therefore I would suggest to simply
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 59b4b5e126ba1..53daf7614b1af 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3236,7 +3236,6 @@ static int do_con_write(struct tty_struct *tty, const u8 *buf, int count)
goto rescan_last_byte;
}
con_flush(vc, &draw);
- console_conditional_schedule();
notify_update(vc);
return n;
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 7be9e865325d9..36dd9d4a46ae0 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -1607,12 +1607,10 @@ static void fbcon_redraw_move(struct vc_data *vc, struct fbcon_display *p,
start = s;
}
}
- console_conditional_schedule();
s++;
} while (s < le);
if (s > start)
fbcon_putcs(vc, start, s - start, dy, x);
- console_conditional_schedule();
dy++;
}
}
@@ -1648,14 +1646,12 @@ static void fbcon_redraw_blit(struct vc_data *vc, struct fb_info *info,
}
scr_writew(c, d);
- console_conditional_schedule();
s++;
d++;
} while (s < le);
if (s > start)
par->bitops->bmove(vc, info, line + ycount, x, line, x, 1,
s - start);
- console_conditional_schedule();
if (ycount > 0)
line++;
else {
@@ -1703,13 +1699,11 @@ static void fbcon_redraw(struct vc_data *vc, int line, int count, int offset)
}
}
scr_writew(c, d);
- console_conditional_schedule();
s++;
d++;
} while (s < le);
if (s > start)
fbcon_putcs(vc, start, s - start, line, x);
- console_conditional_schedule();
if (offset > 0)
line++;
else {
diff --git a/include/linux/console.h b/include/linux/console.h
index fc9f5c5c1b04c..ec506d3501965 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -697,7 +697,6 @@ extern int unregister_console(struct console *);
extern void console_lock(void);
extern int console_trylock(void);
extern void console_unlock(void);
-extern void console_conditional_schedule(void);
extern void console_unblank(void);
extern void console_flush_on_panic(enum con_flush_mode mode);
extern struct tty_driver *console_device(int *);
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 1d765ad242b82..52b1fefdff4e0 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3362,22 +3362,6 @@ void console_unlock(void)
}
EXPORT_SYMBOL(console_unlock);
-/**
- * console_conditional_schedule - yield the CPU if required
- *
- * If the console code is currently allowed to sleep, and
- * if this CPU should yield the CPU to another task, do
- * so here.
- *
- * Must be called within console_lock();.
- */
-void __sched console_conditional_schedule(void)
-{
- if (console_may_schedule)
- cond_resched();
-}
-EXPORT_SYMBOL(console_conditional_schedule);
-
void console_unblank(void)
{
bool found_unblank = false;
Sebastian
^ permalink raw reply related
* Re: [PATCH v6] staging: fbtft: Use fbdev logging helpers when FB_DEVICE is disabled
From: Thomas Zimmermann @ 2026-01-14 11:38 UTC (permalink / raw)
To: Greg KH, Chintan Patel
Cc: linux-fbdev, linux-staging, linux-omap, linux-kernel, dri-devel,
andy, deller, kernel test robot
In-Reply-To: <2026011341-chomp-protegee-6be5@gregkh>
Hi
Am 13.01.26 um 07:16 schrieb Greg KH:
> On Mon, Jan 12, 2026 at 08:59:09PM -0800, Chintan Patel wrote:
>> Replace direct accesses to info->dev with fb_dbg() and fb_info()
>> helpers to avoid build failures when CONFIG_FB_DEVICE=n.
> Why is there a fb_* specific logging helper? dev_info() and dev_dbg()
> should be used instead.
Fbdev is entirely inconsistent about its logging. There's dev_*(),
there's pr_*(), and even printk(). The problem with dev_*() logging is
that devices are not always available. The HW device can be NULL and
might not be all that useful in practice. The Fbdev software device is
often not even compiled in nowadays. (This patch is about that problem.)
Hence the next best option is to make fb_*() logging helpers that
address these problems. They are based on pr_*() and print the
framebuffer index, which should always be available after
register_framebuffer().
>
>> Fixes: a06d03f9f238 ("staging: fbtft: Make FB_DEVICE dependency optional")
> Is this really a bug?
>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202601110740.Y9XK5HtN-lkp@intel.com
>> Signed-off-by: Chintan Patel <chintanlike@gmail.com>
>>
>> Changes in v6:
>> - Switch debug/info logging to fb_dbg() and fb_info()(suggested by Thomas Zimmermann)
>> - Drop dev_of_fbinfo() usage in favor of framebuffer helpers that implicitly
>> handle the debug/info context.
>> - Drop __func__ usage per review feedback(suggested by greg k-h)
>> - Add Fixes tag for a06d03f9f238 ("staging: fbtft: Make FB_DEVICE dependency optional")
>> (suggested by Andy Shevchenko)
>>
>> Changes in v5:
>> - Initial attempt to replace info->dev accesses using
>> dev_of_fbinfo() helper
>> ---
> The changelog stuff goes below the --- line.
>
>> drivers/staging/fbtft/fbtft-core.c | 19 +++++++++----------
>> 1 file changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
>> index 8a5ccc8ae0a1..1b3b62950205 100644
>> --- a/drivers/staging/fbtft/fbtft-core.c
>> +++ b/drivers/staging/fbtft/fbtft-core.c
>> @@ -365,9 +365,9 @@ static int fbtft_fb_setcolreg(unsigned int regno, unsigned int red,
>> unsigned int val;
>> int ret = 1;
>>
>> - dev_dbg(info->dev,
>> - "%s(regno=%u, red=0x%X, green=0x%X, blue=0x%X, trans=0x%X)\n",
>> - __func__, regno, red, green, blue, transp);
>> + fb_dbg(info,
>> + "regno=%u, red=0x%X, green=0x%X, blue=0x%X, trans=0x%X\n",
>> + regno, red, green, blue, transp);
> I dont understand what is wrong with the existing dev_dbg() line (with
> the exception that __func__ should not be in it.
>
>>
>> switch (info->fix.visual) {
>> case FB_VISUAL_TRUECOLOR:
>> @@ -391,8 +391,7 @@ static int fbtft_fb_blank(int blank, struct fb_info *info)
>> struct fbtft_par *par = info->par;
>> int ret = -EINVAL;
>>
>> - dev_dbg(info->dev, "%s(blank=%d)\n",
>> - __func__, blank);
>> + fb_dbg(info, "blank=%d\n", blank);
> Same here, what's wrong with dev_dbg()?
>
>
>>
>> if (!par->fbtftops.blank)
>> return ret;
>> @@ -793,11 +792,11 @@ int fbtft_register_framebuffer(struct fb_info *fb_info)
>> if (spi)
>> sprintf(text2, ", spi%d.%d at %d MHz", spi->controller->bus_num,
>> spi_get_chipselect(spi, 0), spi->max_speed_hz / 1000000);
>> - dev_info(fb_info->dev,
>> - "%s frame buffer, %dx%d, %d KiB video memory%s, fps=%lu%s\n",
>> - fb_info->fix.id, fb_info->var.xres, fb_info->var.yres,
>> - fb_info->fix.smem_len >> 10, text1,
>> - HZ / fb_info->fbdefio->delay, text2);
>> + fb_info(fb_info,
>> + "%s frame buffer, %dx%d, %d KiB video memory%s, fps=%lu%s\n",
>> + fb_info->fix.id, fb_info->var.xres, fb_info->var.yres,
>> + fb_info->fix.smem_len >> 10, text1,
>> + HZ / fb_info->fbdefio->delay, text2);
> When drivers work properly, they are quiet. Why is this needed at all
> except as a debug message?
Agreed. If there's anything useful in this output, it should be printed
with _dbg(), but not _info().
Best regards
Thomas
>
> thanks,
>
> greg k-h
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)
^ permalink raw reply
* Re: [PATCH v6] staging: fbtft: Use fbdev logging helpers when FB_DEVICE is disabled
From: Andy Shevchenko @ 2026-01-14 7:15 UTC (permalink / raw)
To: Chintan Patel
Cc: Greg KH, linux-fbdev, linux-staging, linux-omap, linux-kernel,
dri-devel, tzimmermann, andy, deller, kernel test robot
In-Reply-To: <0a90bd0a-cb74-43a3-a50b-4c83bc086556@gmail.com>
On Tue, Jan 13, 2026 at 08:47:54PM -0800, Chintan Patel wrote:
> On 1/12/26 22:16, Greg KH wrote:
> > On Mon, Jan 12, 2026 at 08:59:09PM -0800, Chintan Patel wrote:
> > > Replace direct accesses to info->dev with fb_dbg() and fb_info()
> > > helpers to avoid build failures when CONFIG_FB_DEVICE=n.
> >
> > Why is there a fb_* specific logging helper? dev_info() and dev_dbg()
> > should be used instead.
>
> You’re correct that dev_dbg()/dev_info() are the standard logging APIs.
>
> The reason I switched to fb_dbg()/fb_info() is not stylistic: direct
> dereferences of info->dev / fb_info->dev are invalid when
> CONFIG_FB_DEVICE=n, which causes compile-time errors.
>
> fb_dbg() and fb_info() are framebuffer-specific helpers that handle
> this case correctly, allowing logging without touching info->dev.
>
> > > Fixes: a06d03f9f238 ("staging: fbtft: Make FB_DEVICE dependency optional")
> >
> > Is this really a bug?
>
> The build failure occurs when CONFIG_FB_DEVICE=n, where direct
> dereferences of info->dev / fb_info->dev are not valid. This was reported by
> the kernel test robot.
>
> That said, I’m fine dropping the Fixes tag if you don’t consider this a
> regression.
I believe the point Greg made is that: If it's a bug, state it more clearly in
the commit message. The summary of the above sounds to me like a good enough
justification to leave Fixes tag as is.
...
> Same reason: dereferencing info->dev is invalid when CONFIG_FB_DEVICE=n.
> fb_dbg() handles this correctly without needing info->dev.
Similar comment here, make it more clearly, e.g. by adding more details in the
commit message, like explaining that there is no such a field to access when it
goes under some circumstances.
...
> > > + fb_info(fb_info,
> > > + "%s frame buffer, %dx%d, %d KiB video memory%s, fps=%lu%s\n",
> > > + fb_info->fix.id, fb_info->var.xres, fb_info->var.yres,
> > > + fb_info->fix.smem_len >> 10, text1,
> > > + HZ / fb_info->fbdefio->delay, text2);
> >
> > When drivers work properly, they are quiet. Why is this needed at all
> > except as a debug message?
>
> Agreed. The informational message during framebuffer registration is not
> necessary. I will either remove it entirely or convert it to a debug-only
> message.
>
> I’ll rework the patch accordingly and resend.
If you go this direction, I would do it in two stages (first is a direct
fix for a compilation issue and second one is switching to dbg level, each
with the respective commit message), but I leave it up to you and Greg.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3] staging: fbtft: Change udelay() to fsleep()
From: Greg Kroah-Hartman @ 2026-01-14 6:55 UTC (permalink / raw)
To: Gideon Adjei
Cc: Andy Shevchenko, Abdun Nihaal, Dan Carpenter, Jianglei Nie,
Matthew Wilcox, dri-devel, linux-fbdev, linux-staging,
linux-kernel
In-Reply-To: <20260113221722.5157-1-gideonadjei.dev@gmail.com>
On Tue, Jan 13, 2026 at 02:17:22PM -0800, Gideon Adjei wrote:
> Replace udelay() calls >= 100us with fsleep() to avoid busy-waiting.
>
> The delays are used in init_display() callbacks. These callbacks are
> invoked by fbtft_probe_common() during the driver's probe path. The
> probe path runs in process context which already uses sleeping APIs.
> This makes fsleep() safe to use in these situations.
>
> Signed-off-by: Gideon Adjei <gideonadjei.dev@gmail.com>
> ---
> drivers/staging/fbtft/fb_tinylcd.c | 2 +-
> drivers/staging/fbtft/fb_upd161704.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/fbtft/fb_tinylcd.c b/drivers/staging/fbtft/fb_tinylcd.c
> index 9469248f2c50..3fb15df31592 100644
> --- a/drivers/staging/fbtft/fb_tinylcd.c
> +++ b/drivers/staging/fbtft/fb_tinylcd.c
> @@ -41,7 +41,7 @@ static int init_display(struct fbtft_par *par)
> 0x00, 0x35, 0x33, 0x00, 0x00, 0x00);
> write_reg(par, MIPI_DCS_SET_PIXEL_FORMAT, 0x55);
> write_reg(par, MIPI_DCS_EXIT_SLEEP_MODE);
> - udelay(250);
> + fsleep(250);
Without the hardware and specifications to test this with, we can't take
this type of patch. Please see the mailing list archives for the
multitude of times it has been rejected in the past.
sorry, but checkpatch can be ignored here.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v6] staging: fbtft: Use fbdev logging helpers when FB_DEVICE is disabled
From: Chintan Patel @ 2026-01-14 4:47 UTC (permalink / raw)
To: Greg KH
Cc: linux-fbdev, linux-staging, linux-omap, linux-kernel, dri-devel,
tzimmermann, andy, deller, kernel test robot
In-Reply-To: <2026011341-chomp-protegee-6be5@gregkh>
On 1/12/26 22:16, Greg KH wrote:
> On Mon, Jan 12, 2026 at 08:59:09PM -0800, Chintan Patel wrote:
>> Replace direct accesses to info->dev with fb_dbg() and fb_info()
>> helpers to avoid build failures when CONFIG_FB_DEVICE=n.
>
> Why is there a fb_* specific logging helper? dev_info() and dev_dbg()
> should be used instead.
You’re correct that dev_dbg()/dev_info() are the standard logging APIs.
The reason I switched to fb_dbg()/fb_info() is not stylistic: direct
dereferences of info->dev / fb_info->dev are invalid when
CONFIG_FB_DEVICE=n, which causes compile-time errors.
fb_dbg() and fb_info() are framebuffer-specific helpers that handle
this case correctly, allowing logging without touching info->dev.
>
>> Fixes: a06d03f9f238 ("staging: fbtft: Make FB_DEVICE dependency optional")
>
> Is this really a bug?
The build failure occurs when CONFIG_FB_DEVICE=n, where direct
dereferences of info->dev / fb_info->dev are not valid. This was
reported by the kernel test robot.
That said, I’m fine dropping the Fixes tag if you don’t consider this a
regression.
>> Reported-by: kernel test robot <lkp@intel.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202601110740.Y9XK5HtN-lkp@intel.com
>> Signed-off-by: Chintan Patel <chintanlike@gmail.com>
>>
>> Changes in v6:
>> - Switch debug/info logging to fb_dbg() and fb_info()(suggested by Thomas Zimmermann)
>> - Drop dev_of_fbinfo() usage in favor of framebuffer helpers that implicitly
>> handle the debug/info context.
>> - Drop __func__ usage per review feedback(suggested by greg k-h)
>> - Add Fixes tag for a06d03f9f238 ("staging: fbtft: Make FB_DEVICE dependency optional")
>> (suggested by Andy Shevchenko)
>>
>> Changes in v5:
>> - Initial attempt to replace info->dev accesses using
>> dev_of_fbinfo() helper
>> ---
>
> The changelog stuff goes below the --- line.
Ack.
>
>> drivers/staging/fbtft/fbtft-core.c | 19 +++++++++----------
>> 1 file changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
>> index 8a5ccc8ae0a1..1b3b62950205 100644
>> --- a/drivers/staging/fbtft/fbtft-core.c
>> +++ b/drivers/staging/fbtft/fbtft-core.c
>> @@ -365,9 +365,9 @@ static int fbtft_fb_setcolreg(unsigned int regno, unsigned int red,
>> unsigned int val;
>> int ret = 1;
>>
>> - dev_dbg(info->dev,
>> - "%s(regno=%u, red=0x%X, green=0x%X, blue=0x%X, trans=0x%X)\n",
>> - __func__, regno, red, green, blue, transp);
>> + fb_dbg(info,
>> + "regno=%u, red=0x%X, green=0x%X, blue=0x%X, trans=0x%X\n",
>> + regno, red, green, blue, transp);
>
> I dont understand what is wrong with the existing dev_dbg() line (with
> the exception that __func__ should not be in it.
>
>>
>> switch (info->fix.visual) {
>> case FB_VISUAL_TRUECOLOR:
>> @@ -391,8 +391,7 @@ static int fbtft_fb_blank(int blank, struct fb_info *info)
>> struct fbtft_par *par = info->par;
>> int ret = -EINVAL;
>>
>> - dev_dbg(info->dev, "%s(blank=%d)\n",
>> - __func__, blank);
>> + fb_dbg(info, "blank=%d\n", blank);
>
> Same here, what's wrong with dev_dbg()?
>
Same reason: dereferencing info->dev is invalid when CONFIG_FB_DEVICE=n.
fb_dbg() handles this correctly without needing info->dev.
>>
>> if (!par->fbtftops.blank)
>> return ret;
>> @@ -793,11 +792,11 @@ int fbtft_register_framebuffer(struct fb_info *fb_info)
>> if (spi)
>> sprintf(text2, ", spi%d.%d at %d MHz", spi->controller->bus_num,
>> spi_get_chipselect(spi, 0), spi->max_speed_hz / 1000000);
>> - dev_info(fb_info->dev,
>> - "%s frame buffer, %dx%d, %d KiB video memory%s, fps=%lu%s\n",
>> - fb_info->fix.id, fb_info->var.xres, fb_info->var.yres,
>> - fb_info->fix.smem_len >> 10, text1,
>> - HZ / fb_info->fbdefio->delay, text2);
>> + fb_info(fb_info,
>> + "%s frame buffer, %dx%d, %d KiB video memory%s, fps=%lu%s\n",
>> + fb_info->fix.id, fb_info->var.xres, fb_info->var.yres,
>> + fb_info->fix.smem_len >> 10, text1,
>> + HZ / fb_info->fbdefio->delay, text2);
>
> When drivers work properly, they are quiet. Why is this needed at all
> except as a debug message?
Agreed. The informational message during framebuffer registration is not
necessary. I will either remove it entirely or convert it to a
debug-only message.
I’ll rework the patch accordingly and resend.
Thanks again for the guidance.
^ permalink raw reply
* Re: [PATCH v3] staging: fbtft: Change udelay() to fsleep()
From: Gideon Adjei @ 2026-01-14 0:30 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Greg Kroah-Hartman, Abdun Nihaal, Dan Carpenter, Jianglei Nie,
Matthew Wilcox, dri-devel, linux-fbdev, linux-staging,
linux-kernel
In-Reply-To: <CAHp75Ved+zhACJ9CYLYnkzW6Z52fmZT0VY+0NWKYk6O38HCkdg@mail.gmail.com>
On Wed, Jan 14, 2026 at 12:33:42AM +0200, Andy Shevchenko wrote:
>No need to send v4 because of this, just reply with the changelogs for
>v1->v2 and v2->v3.
>
Changelog:
v1->v2:
- Added explanation to why usleep_range() is safe in this context to
commit message.
v2->v3:
- Switched from usleep_range() to fsleep().
- Corrected a minor grammar error in commit message.
No functional changes to the code itself.
>Also note, it's assumed that even for such a simple patch the time
>between versions is at least 24h.
Thank you for the reminder regarding the expected interval between
revions. I will ensure this observed going forward.
Signed-off-by: Gideon Adjei <gideonadjei.dev@gmail.com>
^ permalink raw reply
* Re: [PATCH v3] staging: fbtft: Change udelay() to fsleep()
From: Andy Shevchenko @ 2026-01-13 22:33 UTC (permalink / raw)
To: Gideon Adjei
Cc: Andy Shevchenko, Greg Kroah-Hartman, Abdun Nihaal, Dan Carpenter,
Jianglei Nie, Matthew Wilcox, dri-devel, linux-fbdev,
linux-staging, linux-kernel
In-Reply-To: <20260113221722.5157-1-gideonadjei.dev@gmail.com>
On Wed, Jan 14, 2026 at 12:17 AM Gideon Adjei <gideonadjei.dev@gmail.com> wrote:
>
> Replace udelay() calls >= 100us with fsleep() to avoid busy-waiting.
>
> The delays are used in init_display() callbacks. These callbacks are
> invoked by fbtft_probe_common() during the driver's probe path. The
> probe path runs in process context which already uses sleeping APIs.
> This makes fsleep() safe to use in these situations.
You forgot to add a changelog...
> Signed-off-by: Gideon Adjei <gideonadjei.dev@gmail.com>
> ---
...somewhere here.
No need to send v4 because of this, just reply with the changelogs for
v1->v2 and v2->v3.
Also note, it's assumed that even for such a simple patch the time
between versions is at least 24h.
> drivers/staging/fbtft/fb_tinylcd.c | 2 +-
> drivers/staging/fbtft/fb_upd161704.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH v3] staging: fbtft: Change udelay() to fsleep()
From: Gideon Adjei @ 2026-01-13 22:17 UTC (permalink / raw)
To: Andy Shevchenko, Greg Kroah-Hartman
Cc: Abdun Nihaal, Dan Carpenter, Jianglei Nie, Matthew Wilcox,
dri-devel, linux-fbdev, linux-staging, linux-kernel, Gideon Adjei
Replace udelay() calls >= 100us with fsleep() to avoid busy-waiting.
The delays are used in init_display() callbacks. These callbacks are
invoked by fbtft_probe_common() during the driver's probe path. The
probe path runs in process context which already uses sleeping APIs.
This makes fsleep() safe to use in these situations.
Signed-off-by: Gideon Adjei <gideonadjei.dev@gmail.com>
---
drivers/staging/fbtft/fb_tinylcd.c | 2 +-
drivers/staging/fbtft/fb_upd161704.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/fbtft/fb_tinylcd.c b/drivers/staging/fbtft/fb_tinylcd.c
index 9469248f2c50..3fb15df31592 100644
--- a/drivers/staging/fbtft/fb_tinylcd.c
+++ b/drivers/staging/fbtft/fb_tinylcd.c
@@ -41,7 +41,7 @@ static int init_display(struct fbtft_par *par)
0x00, 0x35, 0x33, 0x00, 0x00, 0x00);
write_reg(par, MIPI_DCS_SET_PIXEL_FORMAT, 0x55);
write_reg(par, MIPI_DCS_EXIT_SLEEP_MODE);
- udelay(250);
+ fsleep(250);
write_reg(par, MIPI_DCS_SET_DISPLAY_ON);
return 0;
diff --git a/drivers/staging/fbtft/fb_upd161704.c b/drivers/staging/fbtft/fb_upd161704.c
index c680160d6380..7fe2b556e17c 100644
--- a/drivers/staging/fbtft/fb_upd161704.c
+++ b/drivers/staging/fbtft/fb_upd161704.c
@@ -32,7 +32,7 @@ static int init_display(struct fbtft_par *par)
/* oscillator start */
write_reg(par, 0x003A, 0x0001); /*Oscillator 0: stop, 1: operation */
- udelay(100);
+ fsleep(100);
/* y-setting */
write_reg(par, 0x0024, 0x007B); /* amplitude setting */
@@ -60,7 +60,7 @@ static int init_display(struct fbtft_par *par)
/* Power supply setting */
write_reg(par, 0x0019, 0x0000); /* DC/DC output setting */
- udelay(200);
+ fsleep(200);
write_reg(par, 0x001A, 0x1000); /* DC/DC frequency setting */
write_reg(par, 0x001B, 0x0023); /* DC/DC rising setting */
write_reg(par, 0x001C, 0x0C01); /* Regulator voltage setting */
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v2] staging: fbtft: Change udelay() to usleep_range()
From: Andy Shevchenko @ 2026-01-13 21:13 UTC (permalink / raw)
To: Gideon Adjei
Cc: Andy Shevchenko, Greg Kroah-Hartman, Abdun Nihaal, Dan Carpenter,
Jianglei Nie, Matthew Wilcox, dri-devel, linux-fbdev,
linux-staging, linux-kernel
In-Reply-To: <20260113210559.3020-1-gideonadjei.dev@gmail.com>
On Tue, Jan 13, 2026 at 11:06 PM Gideon Adjei <gideonadjei.dev@gmail.com> wrote:
>
> Replace udelay() calls >= 100us with usleep_range() to avoid busy-waiting.
>
> The delays are used in init_display() callbacks. These callbacks are
> invoked by fbtft_probe_common() during the driver's probe path. the
> probe path runs in process context which already uses sleeping APIs.
> This makes usleep_range() safe to use in these situations.
Nice, now can we switch to modern API, i.e. fsleep()?
--
With Best Regards,
Andy Shevchenko
^ 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