>-----Original Message-----
>From: [email protected]
[mailto:linux-kernel-commits-
>[email protected]] On Behalf Of Mike Frysinger
>Sent: Tuesday, March 17, 2009 1:58 PM
>To: [email protected]
>Cc: [email protected]
>Subject: Re: [Linux-kernel-commits] [6185]
trunk/drivers/video/backlight:[#4375] ADP5520
>Multifunction LCD Backlight and Keypad Input Device
>
>On Tue, Mar 17, 2009 at 08:38, <[email protected]> wrote:
>> --- trunk/drivers/video/backlight/Kconfig 2009-03-17 12:24:38 UTC
(rev 6184)
>> +++ trunk/drivers/video/backlight/Kconfig 2009-03-17 12:38:22 UTC
(rev 6185)
>> @@ -214,3 +214,10 @@
>> +config BACKLIGHT_ADP5520
>> + tristate "Backlight Driver for ADP5520/ADP5501 using WLED"
>> + depends on BACKLIGHT_CLASS_DEVICE && PMIC_ADP5520
>> + help
>> + If you have a LCD backlight connected to the BST/BL_SNK output
of
>> + ADP5520 or ADP5501, say Y here to enable this driver.
>
>could you add info as to what the module will be called if selected as
such ?
Sure
>
>> --- trunk/drivers/video/backlight/adp5520_bl.c
>> +++ trunk/drivers/video/backlight/adp5520_bl.c
> @@ -0,0 +1,368 @@
>> + * Copyright 2009 Analog Devices Inc.
>> + *
>> + *
>
>only need one empty comment line here ...
>
>> +static int adp5520_bl_set(struct backlight_device *bl, int
brightness)
>> +{
>> + ...
>> + } else {
>> + ret |= adp5520_write(master, DAYLIGHT_MAX, brightness);
>> + }
>
>dont need these braces
I thought - in case the if path required braces the else path should
have them too?
>
>> + if (data->current_brightness && brightness == 0)
>> + ret |= adp5520_set_bits(master,
>> + MODE_STATUS, DIM_EN);
>> +
>> + if (data->current_brightness == 0 && brightness)
>
>else if ?
That's better...
>
>> + if (ret)
>> + return ret;
>> +
>> + data->current_brightness = brightness;
>> + return 0;
>
>maybe better structured as
>if (!ret)
> data->current_brightness = brightness;
>return ret;
Does this really matter?
>
>> +static int adp5520_bl_setup(struct backlight_device *bl)
>
>only called by _probe, so this should be __devinit
>
>> +static int adp5520_bl_probe(struct platform_device *pdev)
>
>__devinit
Sure
>
>> + if (data->pdata->en_ambl_sens)
>> + ret = sysfs_create_group(&bl->dev.kobj,
>> + &adp5520_bl_attr_group);
>> +
>> + platform_set_drvdata(pdev, bl);
>> + ret |= adp5520_bl_setup(bl);
>> + backlight_update_status(bl);
>> +
>> + return ret;
>
>can sysfs_create_group() fail ? if it does, do we care ?
It can - but I don't care. Actually never seen this happen.
The driver is still functional.
>
>> +static int adp5520_bl_remove(struct platform_device *pdev)
>
>__devexit
sure
>
>> +{
>> + struct backlight_device *bl = platform_get_drvdata(pdev);
>> + struct adp5520_bl *data = bl_get_data(bl);
>> + int ret;
>> +
>> + ret = adp5520_clr_bits(data->master, MODE_STATUS, BL_EN);
>> +
>> + if (data->pdata->en_ambl_sens)
>> + sysfs_remove_group(&bl->dev.kobj,
>> + &adp5520_bl_attr_group);
>> +
>> + backlight_device_unregister(bl);
>> + kfree(data);
>> +
>> + return ret;
>> +}
>
>if the ret is non-zero, i'm guessing the means the instance wont be
>removed and so the remove function will be called again. if that
>happens, the freeing here will happen doubly and thus crash right ?
Maybe - I better always return 0
-Michael
>-mike
>_______________________________________________
>Linux-kernel-commits mailing list
>[email protected]
>http://blackfin.uclinux.org/mailman/listinfo/linux-kernel-commits
_______________________________________________
Linux-kernel-commits mailing list
[email protected]
http://blackfin.uclinux.org/mailman/listinfo/linux-kernel-commits
