<html>
    <head>
      <base href="https://bugs.freedesktop.org/">
    </head>
    <body>
      <p>
        <div>
            <b><a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Cannot control backlight intensity on Powerbook"
   href="https://bugs.freedesktop.org/show_bug.cgi?id=31122#c21">Comment # 21</a>
              on <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Cannot control backlight intensity on Powerbook"
   href="https://bugs.freedesktop.org/show_bug.cgi?id=31122">bug 31122</a>
              from <span class="vcard"><a class="email" href="mailto:karolherbst@gmail.com" title="Karol Herbst <karolherbst@gmail.com>"> <span class="fn">Karol Herbst</span></a>
</span></b>
        <pre>Comment on <span class=""><a href="attachment.cgi?id=125817" name="attach_125817" title="backlight control w/ gpio check">attachment 125817</a> <a href="attachment.cgi?id=125817&action=edit" title="backlight control w/ gpio check">[details]</a></span> <a href='page.cgi?id=splinter.html&bug=31122&attachment=125817'>[review]</a>
backlight control w/ gpio check

Review of <span class=""><a href="attachment.cgi?id=125817" name="attach_125817" title="backlight control w/ gpio check">attachment 125817</a> <a href="attachment.cgi?id=125817&action=edit" title="backlight control w/ gpio check">[details]</a></span> <a href='page.cgi?id=splinter.html&bug=31122&attachment=125817'>[review]</a>:
-----------------------------------------------------------------

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 <<a href="mailto:karolherbst@gmail.com">karolherbst@gmail.com</a>>

Also please send it to the ML after you are done with the stuff:
<a href="https://lists.freedesktop.org/mailman/listinfo/nouveau">https://lists.freedesktop.org/mailman/listinfo/nouveau</a>

::: /root/nouveau_backlight.c.orig
@@ +47,5 @@
<span class="quote">> +      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;</span >

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 @@
<span class="quote">> +              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 */</span >

please move the comment before the code it refers to

@@ +99,5 @@
<span class="quote">> +
> +  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) */</span >

please move the comment before the code it refers to

@@ +104,5 @@
<span class="quote">> +
> +  bd = backlight_device_register("nv_backlight",
> +                                 connector->kdev,
> +                                 drm,
> +                                       &nv30_bl_ops,</span >

spaces in this line

@@ +307,5 @@
<span class="quote">>                        continue;
>  
>            switch (device->info.family) {
> +                case NV_DEVICE_INFO_V0_RANKINE:
> +                        return nv30_backlight_init(connector);</span >

both lines have spaces</pre>
        </div>
      </p>


      <hr>
      <span>You are receiving this mail because:</span>

      <ul>
          <li>You are the assignee for the bug.</li>
      </ul>
    </body>
</html>