Re: [sparse stuff] bug in lp3944_led_set_brightness()

From: Antonio Ospite
Date: Wed Oct 05 2011 - 16:27:15 EST

On Wed, 5 Oct 2011 12:32:48 +0300
Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:

> Hi Antonio,
> I was going through some Sparse warnings and this one seems pretty
> valid.
> drivers/leds/leds-lp3944.c +292 23:
> warning: mixing different enum types
> 284 static void lp3944_led_set_brightness(struct led_classdev *led_cdev,
> 285 enum led_brightness brightness)
> 286 {
> 287 struct lp3944_led_data *led = ldev_to_led(led_cdev);
> 288
> 289 dev_dbg(&led->client->dev, "%s: %s, %d\n",
> 290 __func__, led_cdev->name, brightness);
> 291
> 292 led->status = brightness;
> ^^^^^^^^^^^^^^^^^^^^^^^^
> 293 schedule_work(&led->work);
> 294 }
> led->status should be values between 0-3. brightness is 0, 127 and
> 255.

Hi Dan, thanks for reporting that.

AFAICS this is not a problem _in_practice_ as lp3944_led_set_brightness
() is called by the leds framework with brightness values already
limited by max_brightness, so it is always 0 or 1 in our case. The
sparse warning is not pretty tho, so I guess we can safely get rid of it
anyways with:

led->status = !!brightness;

keeping track of LP3944_LED_STATUS_OFF and LP3944_LED_STATUS_ON only in
lp3944_led_set_brightness() is OK, as the handling of DIM (blinking)
mode[s] is in lp3944_led_set_blink().

I am following up with a patch in that direction tomorrow, after a test
on the actual hardware.

This is the rationale behind the current code if it is of any interest:
in the development phase of the driver I was abusing brightness = {2,3}
to trigger the hardware blinking function, but then I set the
max_brightness to 1 and so this abusing mechanism became inactive and I
must have forgotten about it.

Thanks again.


Antonio Ospite

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: pgp00000.pgp
Description: PGP signature