LEDS: LED module for DA9052/53 PMIC v1
Subsystems (2)
PATCH 3/7 LEDS: LED module for DA9052/53 PMIC v1
LED Driver for Dialog Semiconductor DA9052/53 PMICs.
This patch is functionally tested on Samsung SMDKV6410.
Signed-off-by David Dajun Chen <[email protected]>
Signed-off-by Ashish Jangam <[email protected]>
--- drivers/leds/Kconfig | 8 ++ drivers/leds/Makefile | 1 + drivers/leds/leds-da9052.c | 214 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 223 insertions(+), 0 deletions(-) create mode 100644 drivers/leds/leds-da9052.c
drivers/leds/Kconfig +8 -0
@@ -258,6 +258,14 @@ config LEDS_DA903X This option enables support for on-chip LED drivers found on Dialog Semiconductor DA9030/DA9034 PMICs. +config LEDS_DA9052 + tristate "Dialog DA9052/DA9053 LEDS" + depends on LEDS_CLASS + depends on PMIC_DA9052 + help + This option enables support for on-chip LED drivers found + on Dialog Semiconductor DA9052-BC and DA9053-AA/Bx PMICs. + config LEDS_DAC124S085 tristate "LED Support for DAC124S085 SPI DAC" depends on LEDS_CLASS
drivers/leds/Makefile +1 -0
@@ -31,6 +31,7 @@ obj-$(CONFIG_LEDS_OT200) += leds-ot200.o obj-$(CONFIG_LEDS_FSG) += leds-fsg.o obj-$(CONFIG_LEDS_PCA955X) += leds-pca955x.o obj-$(CONFIG_LEDS_DA903X) += leds-da903x.o +obj-$(CONFIG_LEDS_DA9052) += leds-da9052.o obj-$(CONFIG_LEDS_WM831X_STATUS) += leds-wm831x-status.o obj-$(CONFIG_LEDS_WM8350) += leds-wm8350.o obj-$(CONFIG_LEDS_PWM) += leds-pwm.o
drivers/leds/leds-da9052.c new file +214 -0
214 lines not on this page · raw message
Quoted 4 lines
drivers/leds/leds-da9052.c new file +3
-0+ da9052 =dev_get_drvdata (pdev->dev.parent); + pdata = da9052->dev->platform_data; +if (pdata ==NULL ) {
drivers/leds/leds-da9052.c line 124
Quoted 10 lines
drivers/leds/leds-da9052.c new file +10
-0+dev_err (&pdev->dev,"No platform data\n" ); + error = -ENODEV ; + goto err_mem; + } + + pled = pdata->pled; +if (pled ==NULL ) { +dev_err (&pdev->dev,"Failed no platform data for LED\n" ); +return -ENOMEM ; + }
this should be ENODEV as well, there is no alloc
the message could be improved, e.g. "No platform data for LED"
Quoted 18 lines
drivers/leds/leds-da9052.c new file +18
-0+for (i =0 ; i < pled->num_leds; i++) { + led[i].cdev.name = pled->leds[i].name; + led[i].cdev.brightness_set = da9052_led_set; + led[i].cdev.brightness =LED_OFF ; + led[i].brightness =LED_OFF ; + led[i].led_index = pled->leds[i].flags; + led[i].da9052 =dev_get_drvdata (pdev->dev.parent); +INIT_WORK (&led[i].work, da9052_led_work); + + error =led_classdev_register (pdev->dev.parent, &led[i].cdev); +if (error) { +dev_err (&pdev->dev,"Failed to register led %d\n" , + led[i].led_index); + goto err_register; + } + + error =da9052_set_led_brightness (&led[i]); +if (error) {
drivers/leds/leds-da9052.c line 161
Quoted 10 lines
drivers/leds/leds-da9052.c new file +10
-0+dev_err (&pdev->dev,"Unable to init led %d\n" , + led[i].led_index); +continue ; + } + } + error =da9052_configure_leds (led); +if (error) { +dev_err (&pdev->dev,"Failed to configure GPIO Led,%d\n" , error); + goto err_register; + }
Led or led or LED?
Led,%d looks odd, maybe "LED%d"?
Signature · 2 lines
Peter Meerwald +43-664-2444418 (mobile)
Quoted 1 line · Ashish Jangam
LED Driver for Dialog Semiconductor DA9052/53 PMICs.
[...]
drivers/leds/leds-da9052.c line 51
Quoted 7 lines
drivers/leds/leds-da9052.c new file +7
-0+ +static int da9052_set_led_brightness (struct da9052_led *led) +{ +u8 val; +int error; + + val = (led->brightness *0x5f /LED_FULL ) &0x7f ;
If the maximum brightness value is 0x5f, just set it as the max_brightness field value, instead of scaling the value.
Quoted 9 lines
drivers/leds/leds-da9052.c new file +9
-0+ val |=DA9052_LED_CONT_DIM ; + + error =da9052_reg_write (led->da9052, led_reg[led->led_index], val); +if (error <0 ) +dev_err (led->da9052->dev,"Failed to set led brightness, %d\n" , + error); +return error; +} +
[...]
Review
Quoted 2 lines
(quoted diff) +2
-0+ +static int da9052_configure_leds(struct da9052_led *led)
Since this function doesn't confgiure one specific led I think it is better to just pass the pointer to the da9052 struct instead of a led.
drivers/leds/leds-da9052.c line 81
Quoted 3 lines
drivers/leds/leds-da9052.c new file +3
-0+{ +int error; +unsigned char register_value =DA9052_OPENDRAIN_OUTPUT |1 <<3 ;
1 << 3 is sort of a magic value, would be nice to have either a comment explaining what it does or have a define for it.
Quoted 26 lines
drivers/leds/leds-da9052.c new file +26
-0+ + error =da9052_reg_update (led->da9052,DA9052_GPIO_14_15_REG , +DA9052_MASK_LOWER_NIBBLE , + register_value); + +if (error <0 ) { +dev_err (led->da9052->dev,"Failed to write GPIO 14-15 reg, %d\n" , + error); +return error; + } + + error =da9052_reg_update (led->da9052,DA9052_GPIO_14_15_REG , +DA9052_MASK_UPPER_NIBBLE , + register_value <<DA9052_NIBBLE_SHIFT ); +if (error <0 ) +dev_err (led->da9052->dev,"Failed to write GPIO 14-15 reg, %d\n" , + error); + +return error; +} + +static int __devinit da9052_led_probe (struct platform_device *pdev) +{ +struct da9052_pdata *pdata; +struct da9052 *da9052; +struct led_platform_data *pled;
drivers/leds/leds-da9052.c line 117
Quoted 10 lines
drivers/leds/leds-da9052.c new file +10
-0+struct da9052_led *led =NULL ; +int error; +int i; + + da9052 =dev_get_drvdata (pdev->dev.parent); + pdata = da9052->dev->platform_data; +if (pdata ==NULL ) { +dev_err (&pdev->dev,"No platform data\n" ); + error = -ENODEV ; + goto err_mem;
Nothing has been allocated yet, so there shouldn't be anything to free either.
Quoted 46 lines
drivers/leds/leds-da9052.c new file +46
-0+ } + + pled = pdata->pled; +if (pled ==NULL ) { +dev_err (&pdev->dev,"Failed no platform data for LED\n" ); +return -ENOMEM ; + } + + led =devm_kzalloc (&pdev->dev, +sizeof (struct da9052_led ) * pled->num_leds, +GFP_KERNEL ); +if (led ==NULL ) { +dev_err (&pdev->dev,"Failed to alloc memory\n" ); +return -ENOMEM ; + } + +for (i =0 ; i < pled->num_leds; i++) { + led[i].cdev.name = pled->leds[i].name; + led[i].cdev.brightness_set = da9052_led_set; + led[i].cdev.brightness =LED_OFF ; + led[i].brightness =LED_OFF ; + led[i].led_index = pled->leds[i].flags; + led[i].da9052 =dev_get_drvdata (pdev->dev.parent); +INIT_WORK (&led[i].work, da9052_led_work); + + error =led_classdev_register (pdev->dev.parent, &led[i].cdev); +if (error) { +dev_err (&pdev->dev,"Failed to register led %d\n" , + led[i].led_index); + goto err_register; + } + + error =da9052_set_led_brightness (&led[i]); +if (error) { +dev_err (&pdev->dev,"Unable to init led %d\n" , + led[i].led_index); +continue ; + } + } + error =da9052_configure_leds (led); +if (error) { +dev_err (&pdev->dev,"Failed to configure GPIO Led,%d\n" , error); + goto err_register; + } + +platform_set_drvdata (pdev, led);
drivers/leds/leds-da9052.c line 173
Quoted 10 lines
drivers/leds/leds-da9052.c new file +10
-0+ +return 0 ; + +err_register: +for (i = i -1 ; i >=0 ; i--) { +led_classdev_unregister (&led[i].cdev); +cancel_work_sync (&led[i].work); + } +err_mem: +devm_kfree (&pdev->dev, led);
You shouldn't need this. If probe fails all devm resource will be freed.
Quoted 13 lines
drivers/leds/leds-da9052.c new file +13
-0+return error; +} + +static int __devexit da9052_led_remove(struct platform_device *pdev) +{ +struct da9052_led *led =platform_get_drvdata (pdev); +struct da9052_pdata *pdata; +struct da9052 *da9052; +struct led_platform_data *pled; +int i; + + da9052 =dev_get_drvdata (pdev->dev.parent); + pdata = da9052->dev->platform_data;
drivers/leds/leds-da9052.c line 196
Quoted 10 lines
drivers/leds/leds-da9052.c new file +10
-0+ pled = pdata->pled; + +for (i =0 ; i < pled->num_leds; i++) { + led[i].brightness =0 ; +da9052_set_led_brightness (&led[i]); +led_classdev_unregister (&led[i].cdev); +cancel_work_sync (&led[i].work); + } + +devm_kfree (&pdev->dev, led);
Neither this, that's the whole point of devm
Quoted 4 lines
drivers/leds/leds-da9052.c new file +4
-0+ +return 0 ; +} +
[...]