[Nouveau] [Bug 31122] Cannot control backlight intensity on Powerbook

[bugzilla-daemon at freedesktop.org]

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

--- Comment #21 from Karol Herbst <karolherbst at gmail.com> ---
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 at gmail.com>

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/nouveau/attachments/20160818/ad6cd0cd/attachment.html>