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.