Re: [PATCHv1 -next] BACKLIGHT: Backlight module for DA9052 PMIC driver

[Thiago Farina]

On Wed, May 4, 2011 at 6:17 AM, Ashish Jangam
<Ashish.Jangam@xxxxxxxxxxxxxxx> wrote:
> Hi Richard,
>
> Backlight Driver for Dialog Semiconductor DA9052 PMICs.
>
> Changes made since last submission:
> . read and write moved to MFD
>
> Signed-off-by: Dajun Chen <dchen@xxxxxxxxxxx>
> ---
> diff -Naur linux-next-20110421.orig/drivers/video/backlight/da9052_bl.c linux-next-20110421/drivers/video/backlight/da9052_bl.c
> --- linux-next-20110421.orig/drivers/video/backlight/da9052_bl.c    Â1970-01-01 05:00:00.000000000 +0500
> +++ linux-next-20110421/drivers/video/backlight/da9052_bl.c   2011-04-26 10:41:20.000000000 +0500
> @@ -0,0 +1,215 @@
> +/*
> + * Backlight Driver for Dialog DA9052 PMICs
> + *
> + * Copyright(c) 2011 Dialog Semiconductor Ltd.
> + *
> + * Author: Dajun Chen <dchen@xxxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/fb.h>
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
Please, could you sort these includes in alphabetical order?

> +
> +#include <linux/mfd/da9052/da9052.h>
> +#include <linux/mfd/da9052/reg.h>
> +
> +#define DA9052_MAX_BRIGHTNESS Â Â Â Â Â0xFF
> +
> +enum {
> + Â Â Â DA9052_WLEDS_OFF,
> + Â Â Â DA9052_WLEDS_ON,
> + Â Â Â };
There is some unnecessary spaces here. I think }; should be vertical
aligned with 'e'.

Please, could you think all the other cases below?

> +
> +u8 wled_bank[] = {
> + Â Â Â Â Â Â Â Â Â Â Â DA9052_LED1_CONF_REG,
> + Â Â Â Â Â Â Â Â Â Â Â DA9052_LED2_CONF_REG,
> + Â Â Â Â Â Â Â Â Â Â Â DA9052_LED3_CONF_REG,
> + Â Â Â Â Â Â Â };
> +
static?

> +struct da9052_bl {
> + Â Â Â struct da9052 *da9052;
> + Â Â Â uint brightness;
> + Â Â Â uint state;
> + Â Â Â uint led_reg;
> + Â Â Â };
> +
static ?

> +static int da9052_adjust_wled_brightness(struct da9052_bl *wleds)
> +{
> + Â Â Â u8 boost_en, i_sink;
> + Â Â Â int ret = 0;
> +
> + Â Â Â boost_en = 0x3F;
> +    i_sink  = 0xFF;
> + Â Â Â if (wleds->state == DA9052_WLEDS_OFF) {
> + Â Â Â Â Â Â Â boost_en = 0x00;
> +        i_sink  = 0x00;
> + Â Â Â }
> +
> + Â Â Â ret = da9052_reg_write(wleds->da9052, DA9052_BOOST_REG, boost_en);
> + Â Â Â if (ret < 0) {
> + Â Â Â Â Â Â Â dev_err(wleds->da9052->dev,
> + Â Â Â Â Â Â Â Â Â Â Â "Failed to write boost reg, %d\n", ret);
> + Â Â Â Â Â Â Â return ret;
> + Â Â Â }
> +
> + Â Â Â ret = da9052_reg_write(wleds->da9052, DA9052_LED_CONT_REG, i_sink);
> + Â Â Â if (ret < 0) {
> + Â Â Â Â Â Â Â dev_err(wleds->da9052->dev,
> + Â Â Â Â Â Â Â Â Â Â Â "Failed to write led cont reg, %d\n", ret);
> + Â Â Â Â Â Â Â return ret;
> + Â Â Â }
> +
> + Â Â Â ret = da9052_reg_write(wleds->da9052, wled_bank[wleds->led_reg],
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â 0x0);
> + Â Â Â if (ret < 0) {
> + Â Â Â Â Â Â Â dev_err(wleds->da9052->dev,
> + Â Â Â Â Â Â Â Â Â Â Â "Failed to write led conf reg, %d", ret);
> + Â Â Â Â Â Â Â return ret;
> + Â Â Â }
> +
> + Â Â Â if (wleds->brightness) {
> + Â Â Â Â Â Â Â msleep(10);
> + Â Â Â Â Â Â Â ret = da9052_reg_write(wleds->da9052,
> + Â Â Â Â Â Â Â wled_bank[wleds->led_reg], wleds->brightness);
> + Â Â Â Â Â Â Â if (ret < 0)
> + Â Â Â Â Â Â Â Â Â Â Â dev_err(wleds->da9052->dev,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â "Failed to write led conf reg, %d", ret);
> + Â Â Â }
> +
> + Â Â Â return ret;
> +
> +}
> +
> +static int da9052_backlight_update_status(struct backlight_device *bl)
> +{
> + Â Â Â int brightness = bl->props.brightness;
> + Â Â Â struct da9052_bl *wleds = bl_get_data(bl);
> +
> + Â Â Â wleds->brightness = brightness;
> + Â Â Â wleds->state = DA9052_WLEDS_ON;
> +
> + Â Â Â return da9052_adjust_wled_brightness(wleds);
> +}
> +
> +static int da9052_backlight_get_brightness(struct backlight_device *bl)
> +{
> + Â Â Â struct da9052_bl *wleds = bl_get_data(bl);
> + Â Â Â return wleds->brightness;
> +}
> +
> +struct backlight_ops da9052_backlight_ops = {
> + Â Â Â .update_status Â= da9052_backlight_update_status,
> + Â Â Â .get_brightness = da9052_backlight_get_brightness,
> +};
> +
static?

> +static int da9052_backlight_probe(struct platform_device *pdev)
> +{
> + Â Â Â struct backlight_device *bl;
> + Â Â Â struct backlight_properties props;
> + Â Â Â int ret = 0;
> + Â Â Â static int led_reg_num;
> + Â Â Â struct da9052_bl *wleds;
> +
remove this extra blank line. One empty line is fine to separate the
declarations from the rest.

> +
> + Â Â Â wleds = kzalloc(sizeof(struct da9052_bl), GFP_KERNEL);
> +
please, could you remove this blank line too?

> + Â Â Â if (!wleds)
> + Â Â Â Â Â Â Â return -ENOMEM;
> +
> + Â Â Â wleds->da9052 Â = dev_get_drvdata(pdev->dev.parent);
> + Â Â Â wleds->brightness = 0;
> + Â Â Â wleds->led_reg = led_reg_num++;
> + Â Â Â wleds->state = DA9052_WLEDS_OFF;
> +
> + Â Â Â bl = backlight_device_register(pdev->name, wleds->da9052->dev,
> + Â Â Â Â Â Â Â Â Â Â Â wleds, &da9052_backlight_ops, &props);
> +
> + Â Â Â if (IS_ERR(bl)) {
> + Â Â Â Â Â Â Â dev_err(&pdev->dev, "Failed to register backlight\n");
> + Â Â Â Â Â Â Â return PTR_ERR(bl);
> + Â Â Â }
> +
> + Â Â Â bl->props.max_brightness = DA9052_MAX_BRIGHTNESS;
> + Â Â Â bl->props.brightness = 0;
> + Â Â Â platform_set_drvdata(pdev, bl);
> +
> + Â Â Â ret = da9052_adjust_wled_brightness(wleds);
> + Â Â Â if (ret)
> + Â Â Â Â Â Â Â return ret;
> +
> + Â Â Â return 0;
> +}
> +
> +static int da9052_backlight_remove(struct platform_device *pdev)
> +{
> + Â Â Â struct backlight_device *bl = platform_get_drvdata(pdev);
> + Â Â Â struct da9052_bl *wleds = bl_get_data(bl);
> +
> + Â Â Â wleds->brightness = 0;
> + Â Â Â wleds->state = DA9052_WLEDS_OFF;
> +
> + Â Â Â da9052_adjust_wled_brightness(wleds);
> + Â Â Â backlight_device_unregister(bl);
> +
> + Â Â Â return 0;
> +}
> +
> +static struct platform_driver da9052_wled1_driver = {
> +    .driver.name  Â= "da9052-WLED1",
> +    .driver.owner  = THIS_MODULE,
> +    .probe         Â= da9052_backlight_probe,
> +    .remove         = da9052_backlight_remove,
> +};
> +
> +static struct platform_driver da9052_wled2_driver = {
> +    .driver.name  Â= "da9052-WLED2",
> +    .driver.owner  = THIS_MODULE,
> +    .probe         Â= da9052_backlight_probe,
> +    .remove         = da9052_backlight_remove,
> +};
> +static struct platform_driver da9052_wled3_driver = {
> +    .driver.name  Â= "da9052-WLED3",
> +    .driver.owner  = THIS_MODULE,
> +    .probe         Â= da9052_backlight_probe,
> +    .remove         = da9052_backlight_remove,
> +};
> +
> +static int __init da9052_backlight_init(void)
> +{
> + Â Â Â s32 ret;
Add a blank line here?

> + Â Â Â ret = platform_driver_register(&da9052_wled1_driver);
> + Â Â Â if (ret)
> + Â Â Â Â Â Â Â return ret;
> + Â Â Â ret = platform_driver_register(&da9052_wled2_driver);
> + Â Â Â if (ret)
> + Â Â Â Â Â Â Â return ret;
> +
> + Â Â Â ret = platform_driver_register(&da9052_wled3_driver);
> + Â Â Â if (ret)
> + Â Â Â Â Â Â Â return ret;
> +
> + Â Â Â return 0;
> +}
> +module_init(da9052_backlight_init);
> +
> +static void __exit da9052_backlight_exit(void)
> +{
> + Â Â Â platform_driver_unregister(&da9052_wled1_driver);
> + Â Â Â platform_driver_unregister(&da9052_wled2_driver);
> + Â Â Â platform_driver_unregister(&da9052_wled3_driver);
> +}
> +module_exit(da9052_backlight_exit);
> +
> +MODULE_AUTHOR("Dajun Chen <dchen@xxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Backlight driver for Dialog DA9052 PMIC");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:da9052-backlight");
> +
> +
> diff -Naur linux-next-20110421.orig/drivers/video/backlight/Kconfig linux-next-20110421/drivers/video/backlight/Kconfig
> --- linux-next-20110421.orig/drivers/video/backlight/Kconfig  Â2011-04-26 09:33:59.000000000 +0500
> +++ linux-next-20110421/drivers/video/backlight/Kconfig 2011-04-26 10:47:08.000000000 +0500
> @@ -237,6 +237,12 @@
> Â Â Â Â ÂIf you have a LCD backlight connected to the WLED output of DA9030
> Â Â Â Â Âor DA9034 WLED output, say Y here to enable this driver.
>
> +config BACKLIGHT_DA9052
> + Â Â Â tristate "Dialog DA9052 WLED"
> + Â Â Â depends on PMIC_DA9052
> + Â Â Â help
> + Â Â Â Â Enable the DA9052 Backlight Driver
> +
> Âconfig BACKLIGHT_MAX8925
> Â Â Â Âtristate "Backlight driver for MAX8925"
> Â Â Â Âdepends on MFD_MAX8925
> diff -Naur linux-next-20110421.orig/drivers/video/backlight/Makefile linux-next-20110421/drivers/video/backlight/Makefile
> --- linux-next-20110421.orig/drivers/video/backlight/Makefile  2011-04-26 09:33:59.000000000 +0500
> +++ linux-next-20110421/drivers/video/backlight/Makefile    Â2011-04-26 10:45:49.000000000 +0500
> @@ -26,6 +26,7 @@
> Âobj-$(CONFIG_BACKLIGHT_CARILLO_RANCH) += cr_bllcd.o
> Âobj-$(CONFIG_BACKLIGHT_PWM) Â Â Â Â Â Â+= pwm_bl.o
> Âobj-$(CONFIG_BACKLIGHT_DA903X) += da903x_bl.o
> +obj-$(CONFIG_BACKLIGHT_DA9052) += da9052_bl.o
> Âobj-$(CONFIG_BACKLIGHT_MAX8925) Â Â Â Â+= max8925_bl.o
> Âobj-$(CONFIG_BACKLIGHT_APPLE) Â+= apple_bl.o
> Âobj-$(CONFIG_BACKLIGHT_TOSA) Â Â Â Â Â += tosa_bl.o
>
> Regards,
> Ashish
>
>
>
--
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/