RE: Hook collie frontlight into sysfs

[Bompart Cedric]

Hi Pavel,

During our test with Richard, we've been thinking can you implement the
full range of brightness intensity values? For example for the others
Zaurus, I think the range is between 0 and 63. So the userspace can
adjust a wider range of levels for the backlight. Another thing, I
didn't see anything different visually between the value 3 and 4.

Regards,
Ced.



-----Original Message-----
From: Richard Purdie [mailto:rpurdie@xxxxxxxxx] 
Sent: 31 March 2006 00:59
To: Pavel Machek
Cc: lenz@xxxxxxxxxxx; kernel list; patches@xxxxxxxxxxxxxxxx; Bompart
Cedric
Subject: Re: Hook collie frontlight into sysfs

On Thu, 2006-03-30 at 13:46 +0200, Pavel Machek wrote:
> Hook backlight handling into backlight subsystem, so that backlight is
> controllable using /sys . I had to shuffle code around a bit in order
> to avoid prototypes.

Hi Pavel,

There are a few issues with this. Firstly,

 CC      arch/arm/common/locomo.o
arch/arm/common/locomo.c: In function `locomo_frontlight_set':
arch/arm/common/locomo.c:1061: error: `LOCOMO_ALC_EN' undeclared (first
use in this function)
arch/arm/common/locomo.c:1061: error: (Each undeclared identifier is
reported only once
arch/arm/common/locomo.c:1061: error: for each function it appears in.)
make[1]: *** [arch/arm/common/locomo.o] Error 1

> diff --git a/drivers/video/backlight/locomolcd.c
b/drivers/video/backlight/locomolcd.c
> index 60831bb..a95cd25 100644
> --- a/drivers/video/backlight/locomolcd.c
> +++ b/drivers/video/backlight/locomolcd.c
> @@ -105,6 +106,38 @@ void locomolcd_power(int on)
>  }
>  EXPORT_SYMBOL(locomolcd_power);
>  
> +
> +static int current_intensity;
> +
> +static int set_intensity(struct backlight_device *bd, int intensity)
> +{
> +	switch (intensity) {
> +	/* AC and non-AC are handled differently, but produce same
results in sharp code? */
> +	case 0: locomo_frontlight_set(locomolcd_dev, 0, 0, 161); break;
> +	case 1: locomo_frontlight_set(locomolcd_dev, 117, 0, 161);
break;
> +	case 2: locomo_frontlight_set(locomolcd_dev, 163, 0, 148);
break;
> +	case 3: locomo_frontlight_set(locomolcd_dev, 194, 0, 161);
break;
> +	case 4: locomo_frontlight_set(locomolcd_dev, 194, 1, 161);
break;
> +
> +	default:
> +		locomo_frontlight_set(locomolcd_dev, intensity, 0, 148);
break;
> +	}
> +	current_intensity = intensity;
> +	return 0;
> +}

That default statement gives cause for concern. Should it not return
-EINVAL for intensities outside of 0-4?

> +static int get_intensity(struct backlight_device *bd)
> +{
> +	return current_intensity;
> +}
> +
> +static struct backlight_properties locomobl_data = {
> +	.owner		= THIS_MODULE,
> +	.get_brightness = get_intensity,
> +	.set_brightness = set_intensity,
> +	.max_brightness = 5,

Maximum brightness is 4 above?

> +};
> +
>  static int poodle_lcd_probe(struct locomo_dev *dev)
>  {
>  	unsigned long flags;
> @@ -147,8 +183,13 @@ static int __init poodle_lcd_init(void)
>  
>  #ifdef CONFIG_SA1100_COLLIE
>  	sa1100fb_lcd_power = locomolcd_power;
> +
> +	backlight_device_register("collie-bl", NULL, &locomobl_data);
>  #endif
>  	return 0;
>  }

Could this be changed to:

#ifdef CONFIG_SA1100_COLLIE
	sa1100fb_lcd_power = locomolcd_power;
#endif
	backlight_device_register("locomo-bl", NULL, &locomobl_data);

 	return 0;

This means that it will also present a backlight interface on poodle. In
fact I've already helped a poodle user test this and it works!

Also note that there are some backlight interface changes sitting in -mm
(see the linux-fbdev-devel mailing list) which this patch isn't covered
by. If this patch gets merged first, I'll make sure it gets adapted to
the new interface though (which actually brings some benefits like power
attribute implementation for free).

Cheers,

Richard





-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/