[Bug 31122] New: Cannot control backlight intensity on Powerbook

[Bug 31122] New: Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

           Summary: Cannot control backlight intensity on Powerbook
           Product: xorg
           Version: unspecified
          Platform: PowerPC
        OS/Version: Linux (All)
            Status: NEW
          Severity: enhancement
          Priority: medium
         Component: Driver/nouveau
        AssignedTo: nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
        ReportedBy: jk-KVH6KZ/iFWQ@public.gmane.org
         QAContact: xorg-team-go0+a7rfsptAfugRpC6u6w@public.gmane.org


The backlight intensity cannot be controlled using the special Powerbook
function keys and neither through Gnome's brightness applet.

I don't know how, but the function keys work fine when running the nv driver
with Debian stock Linux kernel 2.6.32-5-powerpc (which uses nvidiafb, in case
that is important).  The Gnome applet, though, doesn't work either; Gnome,
however, shows a nice overlay indicating that the brightness changed when the
fn-keys are pressed.

I ran radeontool as described in bug #29685 and took dumps of the registers
while regulating the brightness under nv, in case thats helpful:

  fn-key brightness level 0 (no backlight):
  0x0060081c: 0x86066050
  0x000010f0: 0x00000535

  fn-key brightness levels 1-8:
  0x0060081c: 0x86066051 or 0x86066055 (bit 0x4 flips randomly)
  0x000010f0: 0x82450535, 0x82900535, 0x82db0535, 0x83290535,
              0x83740535, 0x83bf0535, 0x840d0535, 0x84550535
              for levels 1-8, respectively

  dpms standby at max. brightness (xset dpms force standby):
  0x0060081c: 0x86066050
  0x000010f0: 0x04550535

I played a bit with radeontool, and bits 0x0fff0000 of 0x10f0 seem continous. 
The values can actually be set a bit higher (and lower) than level 8 (or level
1). The maximum brightness here is reached at ca. 0x84c00535.

The display will also be in maximum brightness when bit 31 of 0x10f0 is not set
(as long as bit 1 of 0x60081c is set, otherwise the backlight turns off).

The machine is a PowerBook6,2 12", with an

  nVidia Corporation NV34M [GeForce FX Go5200] [10de:0329] (rev a1).

It is running Debian xserver-xorg-video-nouveau 1:0.0.16+git20100825+390f1c8-1
and vanilla Linux 2.6.36.

I also filed the related bug #31120 about the backlight not working after
hibernation.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

--- Comment #1 from cfr <margali-FcTltIntn4Fg9hUCZPvPmw@public.gmane.org> 2011-08-29 17:23:17 PDT ---
This still applies to Debian's 3.0.0-1-powerpc kernel (when the kernel command
line prohibits nvidiafb to force nouveau to be used) as does the bug concerning
it not being switched back on after hibernation (#31122).

Is there any news of a fix or work around for this?

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

--- Comment #2 from Emil Velikov <emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2011-08-31 12:37:20 PDT ---
Hi cfr

Thank you for trying a recent kernel, but can you please provide a bit more
information as per our bugs [1] policy

Note that by default if nouveau detects another backlight interface it will not
register

"ACPI backlight interface available, not registering our own"

Thus the brightness control will be left on the hands of the existing interface
- ACPI in my case


Thanks

[1] http://nouveau.freedesktop.org/wiki/Bugs

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

--- Comment #3 from cfr <reescf-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2011-11-24 18:00:21 PST ---
The problem is that there is no device. The system doesn't recognise the
existence of the backlight in some sense.

I realise you'd want dmesg output from the 3.1 kernel but I can't provide that
right now because it panics on boot. I'll try to get it later.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

[-- Attachment #1.1: Type: text/plain, Size: 1532 bytes --]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

--- Comment #4 from Danny <moondrake-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> ---
The powerpc platform has become so old that most current developers do no
longer care (and most maintainers have no clue about it...).

There is no ACPI on powerpc, so that will not help you.

About 3-4 years ago nouveau was working reasonable on powerbooks, since then it
has gotten more love from hard-working (paid) RH devs, who's priorities are
different than from some of the original group. Not to discredit their work,
they come a long way since the original, but much of the newer work breaks the
weird powerpc platform.

The harddisk of my own model also broke so I also stopped testing things on
powerpc. 

Backlight and dpms on powerpc is done via some registers on the card. You can
write directly to them (via radeontool) or make a special driver for it (a
patch was posted on lkml, but here is some more recent info about it:
http://mac.linux.be/phpBB3/viewtopic.php?f=11&t=365).

I remember abstracting the similar code in a separate kernel module which still
sits on my powerbook harddisk (it also contained patches against the then
current nouveau to make the machine wake from sleep and restore the screen.
Perhaps I will get time someday to see how much of that can be salvaged and
used with modern kernels, but still there probably only 5 people left that are
interested, it has not much priority...).

-- 
You are receiving this mail because:
You are the assignee for the bug.

[-- Attachment #1.2: Type: text/html, Size: 2338 bytes --]

[-- Attachment #2: Type: text/plain, Size: 181 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

[-- Attachment #1.1: Type: text/plain, Size: 959 bytes --]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

--- Comment #5 from Ilia Mirkin <imirkin-FrUbXkNCsVf2fBVCVOL8/A@public.gmane.org> ---
What do the Fn+X keys do? I assume they just act as a keypress? (And I'm
guessing that the nv driver hooks into that somehow... although the only
reference to 10f0 that I see is in turning the thing on/off, perhaps it just
leaves something untouched that nouveau touches and messes up.)

This should be fairly easy to implement... You know what register to control,
what the min/max are. I'm guessing it's just 10f0 that matters -- the other
register controls whether the CRTC is on/off.

It should be easy to stick something into nouveau_backlight.c that deals with
your sitation. Instead of keying off of card family, you'd key off of
dev->pci_device, like dfp.c:nv04_dfp_update_backlight does. You should give it
a shot if you're interested.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[-- Attachment #1.2: Type: text/html, Size: 1704 bytes --]

[-- Attachment #2: Type: text/plain, Size: 181 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

[-- Attachment #1.1: Type: text/plain, Size: 562 bytes --]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

Tobias Klausmann <tobias.klausmann-AqjdNwhu20eELgA04lAiVw@public.gmane.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |NEEDINFO

--- Comment #6 from Tobias Klausmann <tobias.klausmann-AqjdNwhu20eELgA04lAiVw@public.gmane.org> ---
No comment after all, did the last comment resolve this?

-- 
You are receiving this mail because:
You are the assignee for the bug.

[-- Attachment #1.2: Type: text/html, Size: 1831 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

[-- Attachment #1.1: Type: text/plain, Size: 339 bytes --]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

--- Comment #7 from A. Chaud <nvbugz-ZdXa/xnzD07wXD0AkxfrAg@public.gmane.org> ---
Created attachment 125547
  --> https://bugs.freedesktop.org/attachment.cgi?id=125547&action=edit
adds backlight control

-- 
You are receiving this mail because:
You are the assignee for the bug.

[-- Attachment #1.2: Type: text/html, Size: 1326 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

[-- Attachment #1.1: Type: text/plain, Size: 339 bytes --]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

--- Comment #8 from A. Chaud <nvbugz-ZdXa/xnzD07wXD0AkxfrAg@public.gmane.org> ---
Created attachment 125548
  --> https://bugs.freedesktop.org/attachment.cgi?id=125548&action=edit
adds backlight control

-- 
You are receiving this mail because:
You are the assignee for the bug.

[-- Attachment #1.2: Type: text/html, Size: 1326 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

[-- Attachment #1.1: Type: text/plain, Size: 531 bytes --]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

--- Comment #9 from A. Chaud <nvbugz-ZdXa/xnzD07wXD0AkxfrAg@public.gmane.org> ---
I created a patch for this and it works for me under debian testing on
4.6.4-1-powerpc. I also added a workaround for another bug 31120. So this patch
fixes backlight control and backlight after hibernate.  

I never submitted a patch before. Hopefully this one is ok.  I welcome all
comments and suggestions.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[-- Attachment #1.2: Type: text/html, Size: 1461 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

[-- Attachment #1.1: Type: text/plain, Size: 819 bytes --]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

Martin Peres <martin.peres-GANU6spQydw@public.gmane.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEEDINFO                    |NEW

--- Comment #10 from Martin Peres <martin.peres-GANU6spQydw@public.gmane.org> ---
Nice to see a patch :)

However, this patch exposes a backlight even when there is no panel connected.
Could you come up with an heuristic to detect if there is or not a backlight?

The normal thing to do is to check in the GPIO table of the bios (check it out
using envytools's nvbios and nvagetbios).

I can assist a little for this.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[-- Attachment #1.2: Type: text/html, Size: 2084 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

[-- Attachment #1.1: Type: text/plain, Size: 924 bytes --]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

--- Comment #11 from A. Chaud <nvbugz-ZdXa/xnzD07wXD0AkxfrAg@public.gmane.org> ---
nvagetbios wasn't able to extract the bios.  From what I gathered after
searching, openfirmware handles vbios differently (ie: no checksums, smaller
vbios, etc).  I found a nv34 openfirmware bios someone else dumped and nvbios
is unable to decode the GPIO section.

However, if the aim is ensuring the device has a backlight, I believe it's
already done.  In nouveau_backlight_init, it first checks to ensure there's a
display on either LVDS or eDP.  Can we not reasonably assume that if the gpu
has a LVDS or eDP panel attached that it also has a backlight?  My init
function is a slightly altered version of the existing nv40_backlight_init
function which doesn't have a backlight check either.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[-- Attachment #1.2: Type: text/html, Size: 1671 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

[-- Attachment #1.1: Type: text/plain, Size: 1689 bytes --]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

--- Comment #12 from Ilia Mirkin <imirkin-FrUbXkNCsVf2fBVCVOL8/A@public.gmane.org> ---
You can get vbios once nouveau is loaded from debugfs. It should correspond
1:1 to the of blob as well.

On Aug 5, 2016 11:37, <bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ@public.gmane.org> wrote:

> *Comment # 11 <https://bugs.freedesktop.org/show_bug.cgi?id=31122#c11> on
> bug 31122 <https://bugs.freedesktop.org/show_bug.cgi?id=31122> from A.
> Chaud <nvbugz-ZdXa/xnzD07wXD0AkxfrAg@public.gmane.org> *
>
> nvagetbios wasn't able to extract the bios.  From what I gathered after
> searching, openfirmware handles vbios differently (ie: no checksums, smaller
> vbios, etc).  I found a nv34 openfirmware bios someone else dumped and nvbios
> is unable to decode the GPIO section.
>
> However, if the aim is ensuring the device has a backlight, I believe it's
> already done.  In nouveau_backlight_init, it first checks to ensure there's a
> display on either LVDS or eDP.  Can we not reasonably assume that if the gpu
> has a LVDS or eDP panel attached that it also has a backlight?  My init
> function is a slightly altered version of the existing nv40_backlight_init
> function which doesn't have a backlight check either.
>
> ------------------------------
> You are receiving this mail because:
>
>    - You are the assignee for the bug.
>
>
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
>
>

-- 
You are receiving this mail because:
You are the assignee for the bug.

[-- Attachment #1.2: Type: text/html, Size: 3378 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

[-- Attachment #1.1: Type: text/plain, Size: 984 bytes --]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

--- Comment #13 from A. Chaud <nvbugz-ZdXa/xnzD07wXD0AkxfrAg@public.gmane.org> ---
Created attachment 125562
  --> https://bugs.freedesktop.org/attachment.cgi?id=125562&action=edit
backlight control w/ gpio check

Thanks for that tip. It was helpful.  

The rom in debugfs worked in nvbios and this patch has a gpio check.  It seems
to work but I've no idea whether or not this is correctly implemented.  

However, I still believe this is unnecessary as the main init function already
checks for LVDS and eDP.

I copied the GPIO table below for reference:

GPIO table at 0x0ac3 version 1.0
GPIO 0: line 1 tag 0x08 [HPD_1] IN
GPIO 1: line 2 tag 0x00 [PANEL_BACKLIGHT_ON] OUT param 1
GPIO 2: line 3 tag 0x01 [PANEL_POWER] OUT
GPIO 3: line 4 tag 0x0c [TVDAC_0] OUT
GPIO 4: line 6 tag 0x13 [???] IN
GPIO 5: line 9 tag 0x0a [???] OUT

-- 
You are receiving this mail because:
You are the assignee for the bug.

[-- Attachment #1.2: Type: text/html, Size: 1970 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

[-- Attachment #1.1: Type: text/plain, Size: 2368 bytes --]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

--- Comment #14 from Karol Herbst <freedesktop-lIBOoy2+GI7scQ4cX5LuPg@public.gmane.org> ---
Comment on attachment 125562
  --> https://bugs.freedesktop.org/attachment.cgi?id=125562
backlight control w/ gpio check

Review of attachment 125562:
-----------------------------------------------------------------

Please read the Linux kernel coding style:
https://www.kernel.org/doc/Documentation/CodingStyle
and adjust your Patch to that (especially the tabs except spaces part).

Also there are a few things I really dislike:

 1. way too coarse backlight level control.
    Please consider exposing the full range and remove all the curve handling
code.
    Userspace should handle this _not_ the kernel. It makes the code much
cleaner and easier to read.

 2. it only works for powerpc. Is there any reason to make it ppc only?

more things inline.

::: nouveau_backlight.c.orig
@@ +49,5 @@
> +        struct nvif_object *device = &drm->device.object;
> +        int val = (nvif_rd32(device, NV30_PMC_BACKLIGHT) &
> +                                   NV30_PMC_BACKLIGHT_MASK) >> 16;
> +        int i;
> +        for(i=0;i<sizeof(NV30_BL_CURVE)/sizeof(NV30_BL_CURVE[0]);i++)

spaces

@@ +54,5 @@
> +        {
> +                if(NV30_BL_CURVE[i] == val) return i;
> +        }
> +
> +        return sizeof(NV30_BL_CURVE)/sizeof(NV30_BL_CURVE[0]);

spaces

@@ +66,5 @@
> +        struct nvif_object *device = &drm->device.object;
> +        int val = bd->props.brightness;
> +
> +        nvif_wr32(device, NV30_PMC_BACKLIGHT,
> +                 (NV30_BL_CURVE[val] << 16) + 0x80000535);

what does the 0x535 part stands for? Is it to protect against turning off the
display by accident? In that case, remove it.

@@ +87,5 @@
> +        struct backlight_device *bd;
> +	
> +	struct dcb_gpio_func func;
> +	struct nvkm_gpio *gpio = nvxx_gpio(&drm->device);
> +	int ret = nvkm_gpio_find(gpio, 0,0x00,0xff, &func);

spaces

@@ +97,5 @@
> +                return 0;
> +
> +        memset(&props, 0, sizeof(struct backlight_properties));
> +        props.type = BACKLIGHT_RAW;
> +        props.max_brightness = sizeof(NV30_BL_CURVE)/sizeof(NV30_BL_CURVE[0])-1;

spaces

-- 
You are receiving this mail because:
You are the assignee for the bug.

[-- Attachment #1.2: Type: text/html, Size: 4104 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

[-- Attachment #1.1: Type: text/plain, Size: 1054 bytes --]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

A. Chaud <nvbugz-ZdXa/xnzD07wXD0AkxfrAg@public.gmane.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #125562|0                           |1
        is obsolete|                            |

--- Comment #15 from A. Chaud <nvbugz-ZdXa/xnzD07wXD0AkxfrAg@public.gmane.org> ---
Created attachment 125642
  --> https://bugs.freedesktop.org/attachment.cgi?id=125642&action=edit
backlight control w/ gpio check

Thank you for the comments.

This version removes the powerpc ifdef and exposes the full range of brightness
values.  I also fixed the formatting.  Let me know if I missed any of it.

Regarding the 0x535.  It's a workaround for bug 31120.  The lower half of the
register gets set to 0x7fff upon waking from suspend which disables the
backlight. This changes it back to a working value.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[-- Attachment #1.2: Type: text/html, Size: 2688 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

[-- Attachment #1.1: Type: text/plain, Size: 783 bytes --]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

--- Comment #16 from Ilia Mirkin <imirkin-FrUbXkNCsVf2fBVCVOL8/A@public.gmane.org> ---
(In reply to A. Chaud from comment #15)
> Regarding the 0x535.  It's a workaround for bug 31120.  The lower half of
> the register gets set to 0x7fff upon waking from suspend which disables the
> backlight. This changes it back to a working value.

There should be a way to save a value on suspend and restore that saved value
on resume.

BTW, I didn't check whether you're still doing this in the latest patch, but a
few things I remember noticing:

(a) foo | 0x80000000, not +
(b) use ARRAY_SIZE, although I think you got rid of that array anyways

-- 
You are receiving this mail because:
You are the assignee for the bug.

[-- Attachment #1.2: Type: text/html, Size: 1775 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

[-- Attachment #1.1: Type: text/plain, Size: 1919 bytes --]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

--- Comment #17 from Karol Herbst <karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> ---
Comment on attachment 125642
  --> https://bugs.freedesktop.org/attachment.cgi?id=125642
backlight control w/ gpio check

Review of attachment 125642:
-----------------------------------------------------------------

This looks much better already. There are still a few things:

the kernel uses tabs 8 character big. Don't use whitespaces. Other remaining
issues inlined.

With everything taking care off, you get my rb-by karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

::: nouveau_backlight.c.orig
@@ +46,5 @@
> +{
> +        struct nouveau_drm *drm = bl_get_data(bd);
> +        struct nvif_object *device = &drm->device.object;
> +        int val = (nvif_rd32(device, NV30_PMC_BACKLIGHT) &
> +                                   NV30_PMC_BACKLIGHT_MASK) >> 16;

indentation, either try to fit it in one line or break the nvif_rd32 command
like this:

nvif_rd32(arg1,
          arg2);

@@ +64,5 @@
> +        int level;
> +
> +        if (val == 0)
> +                level = 0x00;
> +

remove empty line here

@@ +69,5 @@
> +        else
> +                level = val + NV30_BL_MIN_LEVEL - 1;
> +
> +        nvif_wr32(device, NV30_PMC_BACKLIGHT,
> +                 (level << 16) + 0x80000535);  /* workaround for disabled backlight after waking from suspend */

indentation same as above. Also beware the 80 character limit.

@@ +101,5 @@
> +                return 0;
> +
> +        memset(&props, 0, sizeof(struct backlight_properties));
> +        props.type = BACKLIGHT_RAW;
> +        props.max_brightness = NV30_BL_MAX_LEVEL - NV30_BL_MIN_LEVEL;  /* includes an additional 0x00 level (backlight off) */

80 character limit

-- 
You are receiving this mail because:
You are the assignee for the bug.

[-- Attachment #1.2: Type: text/html, Size: 3516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

[-- Attachment #1.1: Type: text/plain, Size: 827 bytes --]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

--- Comment #18 from Karol Herbst <karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> ---
Comment on attachment 125642
  --> https://bugs.freedesktop.org/attachment.cgi?id=125642
backlight control w/ gpio check

Review of attachment 125642:
-----------------------------------------------------------------

::: nouveau_backlight.c.orig
@@ +69,5 @@
> +        else
> +                level = val + NV30_BL_MIN_LEVEL - 1;
> +
> +        nvif_wr32(device, NV30_PMC_BACKLIGHT,
> +                 (level << 16) + 0x80000535);  /* workaround for disabled backlight after waking from suspend */

also like imirkin said: use | instead of +

((level << 16) + 0000535) | 0x80000000

-- 
You are receiving this mail because:
You are the assignee for the bug.

[-- Attachment #1.2: Type: text/html, Size: 2211 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

[-- Attachment #1.1: Type: text/plain, Size: 267 bytes --]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

--- Comment #19 from Karol Herbst <karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> ---
I meant of course reviewed-by, not rb-by

-- 
You are receiving this mail because:
You are the assignee for the bug.

[-- Attachment #1.2: Type: text/html, Size: 1017 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

[-- Attachment #1.1: Type: text/plain, Size: 1075 bytes --]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

A. Chaud <nvbugz-ZdXa/xnzD07wXD0AkxfrAg@public.gmane.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #125547|0                           |1
        is obsolete|                            |
 Attachment #125561|0                           |1
        is obsolete|                            |
 Attachment #125642|0                           |1
        is obsolete|                            |

--- Comment #20 from A. Chaud <nvbugz-ZdXa/xnzD07wXD0AkxfrAg@public.gmane.org> ---
Created attachment 125817
  --> https://bugs.freedesktop.org/attachment.cgi?id=125817&action=edit
backlight control w/ gpio check

I've fixed the formatting.  Hopefully this new version is now fully compliant.

However I've one question.  Is that GPIO check necessary considering the main
init function already checks for LVDS and eDP?

-- 
You are receiving this mail because:
You are the assignee for the bug.

[-- Attachment #1.2: Type: text/html, Size: 2749 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

[-- Attachment #1.1: Type: text/plain, Size: 2364 bytes --]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

--- Comment #21 from Karol Herbst <karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> ---
Comment on attachment 125817
  --> https://bugs.freedesktop.org/attachment.cgi?id=125817
backlight control w/ gpio check

Review of attachment 125817:
-----------------------------------------------------------------

yeah, it is much better now, just a few lines are oddish, comments inlined.

With everything fixed you can add this to your patch:

Reviewed-by: Karol Herbst <karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Also please send it to the ML after you are done with the stuff:
https://lists.freedesktop.org/mailman/listinfo/nouveau

::: /root/nouveau_backlight.c.orig
@@ +47,5 @@
> +	struct nouveau_drm *drm = bl_get_data(bd);
> +	struct nvif_object *device = &drm->device.object;
> +	int val = (nvif_rd32(device,
> +			     NV30_PMC_BACKLIGHT) &
> +                             NV30_PMC_BACKLIGHT_MASK) >> 16;

this line is a bit odd (also contains spaces).
Maybe it would be cleaner if you move the rd out of the calculation and do
something like that (which looks much cleaner imho):

int val = nvif_rd32(device, NV30_PMC_BACKLIGHT);
val = (val & NV30_PMC_BACKLIGHT_MASK) >> 16;

@@ +68,5 @@
> +		level = val + NV30_BL_MIN_LEVEL - 1;
> +	nvif_wr32(device,
> +		  NV30_PMC_BACKLIGHT,
> +		  ((level << 16) + 0x535) | 0x80000000);
> +	/* workaround for disabled backlight after waking from suspend */

please move the comment before the code it refers to

@@ +99,5 @@
> +
> +	memset(&props, 0, sizeof(struct backlight_properties));
> +	props.type = BACKLIGHT_RAW;
> +	props.max_brightness = NV30_BL_MAX_LEVEL - NV30_BL_MIN_LEVEL;  
> +	/* includes an additional 0x00 level (backlight off) */

please move the comment before the code it refers to

@@ +104,5 @@
> +
> +	bd = backlight_device_register("nv_backlight",
> +				       connector->kdev,
> +				       drm,
> +                                       &nv30_bl_ops,

spaces in this line

@@ +307,5 @@
>  			continue;
>  
>  		switch (device->info.family) {
> +                case NV_DEVICE_INFO_V0_RANKINE:
> +                        return nv30_backlight_init(connector);

both lines have spaces

-- 
You are receiving this mail because:
You are the assignee for the bug.

[-- Attachment #1.2: Type: text/html, Size: 4125 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ]

[-- Attachment #1.1: Type: text/plain, Size: 870 bytes --]

https://bugs.freedesktop.org/show_bug.cgi?id=31122

Martin Peres <martin.peres-GANU6spQydw@public.gmane.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |MOVED
             Status|NEW                         |RESOLVED

--- Comment #22 from Martin Peres <martin.peres-GANU6spQydw@public.gmane.org> ---
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been
closed from further activity.

You can subscribe and participate further through the new bug through this link
to our GitLab instance:
https://gitlab.freedesktop.org/xorg/driver/xf86-video-nouveau/issues/10.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[-- Attachment #1.2: Type: text/html, Size: 2407 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau