<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#c14">Comment # 14</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:freedesktop@karolherbst.de" title="Karol Herbst <freedesktop@karolherbst.de>"> <span class="fn">Karol Herbst</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=125562" name="attach_125562" title="backlight control w/ gpio check">attachment 125562</a> <a href="attachment.cgi?id=125562&action=edit" title="backlight control w/ gpio check">[details]</a></span> <a href='page.cgi?id=splinter.html&bug=31122&attachment=125562'>[review]</a>
backlight control w/ gpio check
Review of <span class=""><a href="attachment.cgi?id=125562" name="attach_125562" title="backlight control w/ gpio check">attachment 125562</a> <a href="attachment.cgi?id=125562&action=edit" title="backlight control w/ gpio check">[details]</a></span> <a href='page.cgi?id=splinter.html&bug=31122&attachment=125562'>[review]</a>:
-----------------------------------------------------------------
Please read the Linux kernel coding style:
<a href="https://www.kernel.org/doc/Documentation/CodingStyle">https://www.kernel.org/doc/Documentation/CodingStyle</a>
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 @@
<span class="quote">> + 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++)</span >
spaces
@@ +54,5 @@
<span class="quote">> + {
> + if(NV30_BL_CURVE[i] == val) return i;
> + }
> +
> + return sizeof(NV30_BL_CURVE)/sizeof(NV30_BL_CURVE[0]);</span >
spaces
@@ +66,5 @@
<span class="quote">> + struct nvif_object *device = &drm->device.object;
> + int val = bd->props.brightness;
> +
> + nvif_wr32(device, NV30_PMC_BACKLIGHT,
> + (NV30_BL_CURVE[val] << 16) + 0x80000535);</span >
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 @@
<span class="quote">> + 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);</span >
spaces
@@ +97,5 @@
<span class="quote">> + 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;</span >
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>