Re: [PATCH 1/5] fbdev/au1100fb: Use FB blanking constants

[Helge Deller <deller@gmx.de>]

On 10/19/26 09:24, Thomas Zimmermann wrote:
> Hi
> 
> Am 18.10.26 um 15:52 schrieb Helge Deller:
>> Hi Thomas,
>>
>> On 10/9/26 18:05, Thomas Zimmermann wrote:
>>> Fbdev and its drivers use FB_BLANK constants for controlling the
>>> display's blanking state. Semantics differ to similar VESA blanking
>>> constants.
>>>
>>> Replace the VESA constants in au1100fb with FB_BLANK.
>>
>> I understand that this series drops the VESA constants.
>> That seems logical and correct, but....
>>
>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>>   drivers/video/fbdev/au1100fb.c | 18 +++++++++---------
>>>   1 file changed, 9 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/video/fbdev/au1100fb.c b/drivers/video/fbdev/au1100fb.c
>>> index 39f86ffb1ae3..6c375f389fbd 100644
>>> --- a/drivers/video/fbdev/au1100fb.c
>>> +++ b/drivers/video/fbdev/au1100fb.c
>>> @@ -424,15 +424,15 @@ static int au1100fb_fb_blank(int blank_mode, struct fb_info *fbi)
>>>         switch (blank_mode) {
>>>   -    case VESA_NO_BLANKING:
>>> +    case FB_BLANK_UNBLANK:
>>>           /* Turn on panel */
>>>           fbdev->regs->lcd_control |= LCD_CONTROL_GO;
>>>           wmb(); /* drain writebuffer */
>>>           break;
>>> -
>>> -    case VESA_VSYNC_SUSPEND:
>>> -    case VESA_HSYNC_SUSPEND:
>>> -    case VESA_POWERDOWN:
>>> +    case FB_BLANK_NORMAL:
>>> +    case FB_BLANK_VSYNC_SUSPEND:
>>> +    case FB_BLANK_HSYNC_SUSPEND:
>>> +    case FB_BLANK_POWERDOWN:
>> I don't understand (this is not the fault of this/your patch),
>> how this enum in fb.h works:
>>
>> enum {
>>         /* screen: unblanked, hsync: on,  vsync: on */
>>         FB_BLANK_UNBLANK       = VESA_NO_BLANKING,
>>
>>         /* screen: blanked,   hsync: on,  vsync: on */
>>         FB_BLANK_NORMAL        = VESA_NO_BLANKING + 1,
>>
>>         /* screen: blanked,   hsync: on,  vsync: off */
>>         FB_BLANK_VSYNC_SUSPEND = VESA_VSYNC_SUSPEND + 1,
>>
>>         /* screen: blanked,   hsync: off, vsync: on */
>>         FB_BLANK_HSYNC_SUSPEND = VESA_HSYNC_SUSPEND + 1,
>>
>>         /* screen: blanked,   hsync: off, vsync: off */
>>         FB_BLANK_POWERDOWN     = VESA_POWERDOWN + 1
>>
>>
>> In vesa.h we have:
>> /* VESA Blanking Levels */
>> enum vesa_blank_mode {
>>         VESA_NO_BLANKING        = 0,
>> #define VESA_NO_BLANKING        VESA_NO_BLANKING
>>         VESA_VSYNC_SUSPEND      = 1,
>> #define VESA_VSYNC_SUSPEND      VESA_VSYNC_SUSPEND
>>         VESA_HSYNC_SUSPEND      = 2,
>> #define VESA_HSYNC_SUSPEND      VESA_HSYNC_SUSPEND
>>         VESA_POWERDOWN          = VESA_VSYNC_SUSPEND | VESA_HSYNC_SUSPEND,
>> #define VESA_POWERDOWN          VESA_POWERDOWN
>>         VESA_BLANK_MAX          = VESA_POWERDOWN,
>>
>>
>> for example this one:
>>         /* screen: blanked,   hsync: on,  vsync: off */
>>         FB_BLANK_VSYNC_SUSPEND = VESA_VSYNC_SUSPEND + 1,
>> which is effectively:
>>         FB_BLANK_VSYNC_SUSPEND = VESA_HSYNC_SUSPEND,
>> which seems wrong.
>>
>> Am I missing something?
> 
> No worries, it's super-confusing.
> 
> - FB_BLANK_ describes CRTC state plus monitor state. VESA_ only describes monitor state and assumes that the CRTC is off.
> 
> - FB_BLANK_UNBLANK means "monitor on and send pixels".  There is no VESA equivalent for this mode, because all VESA constants assume that the stream if pixel data has been disabled.
> 
> - FB_BLANK_NORMAL means "monitor on, but do not send pixels". This corresponds to VESA_BLANK_NO_BLANKING, but has a different integer value.
> 
> - The FB_BLANK_*_SUSPEND and _POWERDOWN constants correspond to the VESA constants of similar name. But they are all off by one.
> 
> Therefore some of the drivers (au1100fb) use VESA_NO_BLANKING when
> they mean FB_BLANK_UNBLANK. Others (xilinxfb) get it wrong and use
> VESA_POWERDOWN when they should use FB_BLANK_POWERDOWN.

Ok, thanks for this summary.

I've applied this series now to fbdev git tree.

Thanks!
Helge