* [PATCH] backlight: add an AS3711 PMIC backlight driver
@ 2013-01-08 17:54 Guennadi Liakhovetski
2013-01-25 5:20 ` Jingoo Han
0 siblings, 1 reply; 5+ messages in thread
From: Guennadi Liakhovetski @ 2013-01-08 17:54 UTC (permalink / raw)
To: linux-fbdev
This is an initial commit of a backlight driver, using step-up DCDC power
supplies on AS3711 PMIC. Only one mode has actually been tested, several
further modes have been implemented "dry," but disabled to avoid accidental
hardware damage. Anyone wishing to use any of those modes will have to
modify the driver.
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
Tested on sh73a0-based kzm9g board. As the commit message says, only one
mode has been tested and is enabled. That mode copies the sample code from
the manufacturer. Deviations from that code proved to be fatal for the
hardware...
drivers/video/backlight/Kconfig | 7 +
drivers/video/backlight/Makefile | 1 +
drivers/video/backlight/as3711_bl.c | 379 +++++++++++++++++++++++++++++++++++
3 files changed, 387 insertions(+), 0 deletions(-)
create mode 100644 drivers/video/backlight/as3711_bl.c
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 765a945..6ef0ede 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -390,6 +390,13 @@ config BACKLIGHT_TPS65217
If you have a Texas Instruments TPS65217 say Y to enable the
backlight driver.
+config BACKLIGHT_AS3711
+ tristate "AS3711 Backlight"
+ depends on BACKLIGHT_CLASS_DEVICE && MFD_AS3711
+ help
+ If you have an Austrian Microsystems AS3711 say Y to enable the
+ backlight driver.
+
endif # BACKLIGHT_CLASS_DEVICE
endif # BACKLIGHT_LCD_SUPPORT
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index e7ce729..d3e188f 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -45,3 +45,4 @@ obj-$(CONFIG_BACKLIGHT_PCF50633) += pcf50633-backlight.o
obj-$(CONFIG_BACKLIGHT_AAT2870) += aat2870_bl.o
obj-$(CONFIG_BACKLIGHT_OT200) += ot200_bl.o
obj-$(CONFIG_BACKLIGHT_TPS65217) += tps65217_bl.o
+obj-$(CONFIG_BACKLIGHT_AS3711) += as3711_bl.o
diff --git a/drivers/video/backlight/as3711_bl.c b/drivers/video/backlight/as3711_bl.c
new file mode 100644
index 0000000..c6bc65d
--- /dev/null
+++ b/drivers/video/backlight/as3711_bl.c
@@ -0,0 +1,379 @@
+/*
+ * AS3711 PMIC backlight driver, using DCDC Step Up Converters
+ *
+ * Copyright (C) 2012 Renesas Electronics Corporation
+ * Author: Guennadi Liakhovetski, <g.liakhovetski@gmx.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the version 2 of the GNU General Public License as
+ * published by the Free Software Foundation
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/fb.h>
+#include <linux/kernel.h>
+#include <linux/mfd/as3711.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+enum as3711_bl_type {
+ AS3711_BL_SU1,
+ AS3711_BL_SU2,
+};
+
+struct as3711_bl_data {
+ bool powered;
+ const char *fb_name;
+ struct device *fb_dev;
+ enum as3711_bl_type type;
+ int brightness;
+ struct backlight_device *bl;
+};
+
+struct as3711_bl_supply {
+ struct as3711_bl_data su1;
+ struct as3711_bl_data su2;
+ const struct as3711_bl_pdata *pdata;
+ struct as3711 *as3711;
+};
+
+static struct as3711_bl_supply *to_supply(struct as3711_bl_data *su)
+{
+ switch (su->type) {
+ case AS3711_BL_SU1:
+ return container_of(su, struct as3711_bl_supply, su1);
+ case AS3711_BL_SU2:
+ return container_of(su, struct as3711_bl_supply, su2);
+ }
+ return NULL;
+}
+
+static int as3711_set_brightness_auto_i(struct as3711_bl_data *data,
+ unsigned int brightness)
+{
+ struct as3711_bl_supply *supply = to_supply(data);
+ struct as3711 *as3711 = supply->as3711;
+ const struct as3711_bl_pdata *pdata = supply->pdata;
+ int ret = 0;
+
+ /* Only all equal current values are supported */
+ if (pdata->su2_auto_curr1)
+ ret = regmap_write(as3711->regmap, AS3711_CURR1_VALUE,
+ brightness);
+ if (!ret && pdata->su2_auto_curr2)
+ ret = regmap_write(as3711->regmap, AS3711_CURR2_VALUE,
+ brightness);
+ if (!ret && pdata->su2_auto_curr3)
+ ret = regmap_write(as3711->regmap, AS3711_CURR3_VALUE,
+ brightness);
+
+ return ret;
+}
+
+static int as3711_set_brightness_v(struct as3711 *as3711,
+ unsigned int brightness,
+ unsigned int reg)
+{
+ if (brightness > 31)
+ return -EINVAL;
+
+ return regmap_update_bits(as3711->regmap, reg, 0xf0,
+ brightness << 4);
+}
+
+static int as3711_bl_su2_reset(struct as3711_bl_supply *supply)
+{
+ struct as3711 *as3711 = supply->as3711;
+ int ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_5,
+ 3, supply->pdata->su2_fbprot);
+ if (!ret)
+ ret = regmap_update_bits(as3711->regmap,
+ AS3711_STEPUP_CONTROL_2, 1, 0);
+ if (!ret)
+ ret = regmap_update_bits(as3711->regmap,
+ AS3711_STEPUP_CONTROL_2, 1, 1);
+ return ret;
+}
+
+/*
+ * Someone with less fragile or less expensive hardware could try to simplify
+ * the brightness adjustment procedure.
+ */
+static int as3711_bl_update_status(struct backlight_device *bl)
+{
+ struct as3711_bl_data *data = bl_get_data(bl);
+ struct as3711_bl_supply *supply = to_supply(data);
+ struct as3711 *as3711 = supply->as3711;
+ int brightness = bl->props.brightness;
+ int ret = 0;
+
+ dev_dbg(&bl->dev, "%s(): brightness %u, pwr %x, blank %x, state %x\n",
+ __func__, bl->props.brightness, bl->props.power,
+ bl->props.fb_blank, bl->props.state);
+
+ if (bl->props.power != FB_BLANK_UNBLANK ||
+ bl->props.fb_blank != FB_BLANK_UNBLANK ||
+ bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
+ brightness = 0;
+
+ if (data->type = AS3711_BL_SU1) {
+ ret = as3711_set_brightness_v(as3711, brightness,
+ AS3711_STEPUP_CONTROL_1);
+ } else {
+ const struct as3711_bl_pdata *pdata = supply->pdata;
+
+ switch (pdata->su2_feedback) {
+ case AS3711_SU2_VOLTAGE:
+ ret = as3711_set_brightness_v(as3711, brightness,
+ AS3711_STEPUP_CONTROL_2);
+ break;
+ case AS3711_SU2_CURR_AUTO:
+ ret = as3711_set_brightness_auto_i(data, brightness / 4);
+ if (ret < 0)
+ return ret;
+ if (brightness) {
+ ret = as3711_bl_su2_reset(supply);
+ if (ret < 0)
+ return ret;
+ udelay(500);
+ ret = as3711_set_brightness_auto_i(data, brightness);
+ } else {
+ ret = regmap_update_bits(as3711->regmap,
+ AS3711_STEPUP_CONTROL_2, 1, 0);
+ }
+ break;
+ /* Manual one current feedback pin below */
+ case AS3711_SU2_CURR1:
+ ret = regmap_write(as3711->regmap, AS3711_CURR1_VALUE,
+ brightness);
+ break;
+ case AS3711_SU2_CURR2:
+ ret = regmap_write(as3711->regmap, AS3711_CURR2_VALUE,
+ brightness);
+ break;
+ case AS3711_SU2_CURR3:
+ ret = regmap_write(as3711->regmap, AS3711_CURR3_VALUE,
+ brightness);
+ break;
+ default:
+ ret = -EINVAL;
+ }
+ }
+ if (!ret)
+ data->brightness = brightness;
+
+ return ret;
+}
+
+static int as3711_bl_get_brightness(struct backlight_device *bl)
+{
+ struct as3711_bl_data *data = bl_get_data(bl);
+
+ return data->brightness;
+}
+
+static const struct backlight_ops as3711_bl_ops = {
+ .update_status = as3711_bl_update_status,
+ .get_brightness = as3711_bl_get_brightness,
+};
+
+static int as3711_bl_init_su2(struct as3711_bl_supply *supply)
+{
+ struct as3711 *as3711 = supply->as3711;
+ const struct as3711_bl_pdata *pdata = supply->pdata;
+ u8 ctl = 0;
+ int ret;
+
+ dev_dbg(as3711->dev, "%s(): use %u\n", __func__, pdata->su2_feedback);
+
+ /* Turn SU2 off */
+ ret = regmap_write(as3711->regmap, AS3711_STEPUP_CONTROL_2, 0);
+ if (ret < 0)
+ return ret;
+
+ switch (pdata->su2_feedback) {
+ case AS3711_SU2_VOLTAGE:
+ ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 0);
+ break;
+ case AS3711_SU2_CURR1:
+ ctl = 1;
+ ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 1);
+ break;
+ case AS3711_SU2_CURR2:
+ ctl = 4;
+ ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 2);
+ break;
+ case AS3711_SU2_CURR3:
+ ctl = 0x10;
+ ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 3);
+ break;
+ case AS3711_SU2_CURR_AUTO:
+ if (pdata->su2_auto_curr1)
+ ctl = 2;
+ if (pdata->su2_auto_curr2)
+ ctl |= 8;
+ if (pdata->su2_auto_curr3)
+ ctl |= 0x20;
+ ret = 0;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (!ret)
+ ret = regmap_write(as3711->regmap, AS3711_CURR_CONTROL, ctl);
+
+ return ret;
+}
+
+static int as3711_bl_register(struct platform_device *pdev,
+ unsigned int max_brightness, struct as3711_bl_data *su)
+{
+ struct backlight_properties props = {.type = BACKLIGHT_RAW,};
+ struct backlight_device *bl;
+
+ /* max tuning I = 31uA for voltage- and 38250uA for current-feedback */
+ props.max_brightness = max_brightness;
+
+ bl = backlight_device_register(su->type = AS3711_BL_SU1 ?
+ "as3711-su1" : "as3711-su2",
+ &pdev->dev, su,
+ &as3711_bl_ops, &props);
+ if (IS_ERR(bl)) {
+ dev_err(&pdev->dev, "failed to register backlight\n");
+ return PTR_ERR(bl);
+ }
+
+ bl->props.brightness = props.max_brightness;
+
+ backlight_update_status(bl);
+
+ su->bl = bl;
+
+ return 0;
+}
+
+static int as3711_backlight_probe(struct platform_device *pdev)
+{
+ struct as3711_bl_pdata *pdata = dev_get_platdata(&pdev->dev);
+ struct as3711 *as3711 = dev_get_drvdata(pdev->dev.parent);
+ struct as3711_bl_supply *supply;
+ struct as3711_bl_data *su;
+ unsigned int max_brightness;
+ int ret;
+
+ if (!pdata || (!pdata->su1_fb && !pdata->su2_fb)) {
+ dev_err(&pdev->dev, "No platform data, exiting...\n");
+ return -ENODEV;
+ }
+
+ /*
+ * Due to possible hardware damage I chose to block all modes,
+ * unsupported on my hardware. Anyone, wishing to use any of those modes
+ * will have to first review the code, then activate and test it.
+ */
+ if (pdata->su1_fb ||
+ pdata->su2_fbprot != AS3711_SU2_GPIO4 ||
+ pdata->su2_feedback != AS3711_SU2_CURR_AUTO) {
+ dev_warn(&pdev->dev,
+ "Attention! An untested mode has been chosen!\n"
+ "Please, review the code, enable, test, and report success:-)\n");
+ return -EINVAL;
+ }
+
+ supply = devm_kzalloc(&pdev->dev, sizeof(*supply), GFP_KERNEL);
+ if (!supply)
+ return -ENOMEM;
+
+ supply->as3711 = as3711;
+ supply->pdata = pdata;
+
+ if (pdata->su1_fb) {
+ su = &supply->su1;
+ su->fb_name = pdata->su1_fb;
+ su->type = AS3711_BL_SU1;
+
+ max_brightness = min(pdata->su1_max_uA, 31);
+ ret = as3711_bl_register(pdev, max_brightness, su);
+ if (ret < 0)
+ return ret;
+ }
+
+ if (pdata->su2_fb) {
+ su = &supply->su2;
+ su->fb_name = pdata->su2_fb;
+ su->type = AS3711_BL_SU2;
+
+ switch (pdata->su2_fbprot) {
+ case AS3711_SU2_GPIO2:
+ case AS3711_SU2_GPIO3:
+ case AS3711_SU2_GPIO4:
+ case AS3711_SU2_LX_SD4:
+ break;
+ default:
+ ret = -EINVAL;
+ goto esu2;
+ }
+
+ switch (pdata->su2_feedback) {
+ case AS3711_SU2_VOLTAGE:
+ max_brightness = min(pdata->su2_max_uA, 31);
+ break;
+ case AS3711_SU2_CURR1:
+ case AS3711_SU2_CURR2:
+ case AS3711_SU2_CURR3:
+ case AS3711_SU2_CURR_AUTO:
+ max_brightness = min(pdata->su2_max_uA / 150, 255);
+ break;
+ default:
+ ret = -EINVAL;
+ goto esu2;
+ }
+
+ ret = as3711_bl_init_su2(supply);
+ if (ret < 0)
+ return ret;
+
+ ret = as3711_bl_register(pdev, max_brightness, su);
+ if (ret < 0)
+ goto esu2;
+ }
+
+ platform_set_drvdata(pdev, supply);
+
+ return 0;
+
+esu2:
+ backlight_device_unregister(supply->su1.bl);
+ return ret;
+}
+
+static int as3711_backlight_remove(struct platform_device *pdev)
+{
+ struct as3711_bl_supply *supply = platform_get_drvdata(pdev);
+
+ backlight_device_unregister(supply->su1.bl);
+ backlight_device_unregister(supply->su2.bl);
+
+ return 0;
+}
+
+static struct platform_driver as3711_backlight_driver = {
+ .driver = {
+ .name = "as3711-backlight",
+ .owner = THIS_MODULE,
+ },
+ .probe = as3711_backlight_probe,
+ .remove = as3711_backlight_remove,
+};
+
+module_platform_driver(as3711_backlight_driver);
+
+MODULE_DESCRIPTION("Backlight Driver for AS3711 PMICs");
+MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@gmx.de");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:as3711-backlight");
--
1.7.2.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] backlight: add an AS3711 PMIC backlight driver
2013-01-08 17:54 [PATCH] backlight: add an AS3711 PMIC backlight driver Guennadi Liakhovetski
@ 2013-01-25 5:20 ` Jingoo Han
2013-01-25 7:48 ` Guennadi Liakhovetski
0 siblings, 1 reply; 5+ messages in thread
From: Jingoo Han @ 2013-01-25 5:20 UTC (permalink / raw)
To: 'Guennadi Liakhovetski'
Cc: 'Andrew Morton', linux-kernel, linux-fbdev, linux-sh,
'Magnus Damm', 'Richard Purdie'
On Wednesday, January 09, 2013 2:55 AM, Guennadi Liakhovetski wrote
>
> This is an initial commit of a backlight driver, using step-up DCDC power
> supplies on AS3711 PMIC. Only one mode has actually been tested, several
> further modes have been implemented "dry," but disabled to avoid accidental
> hardware damage. Anyone wishing to use any of those modes will have to
> modify the driver.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
CC'ed Andrew Morton.
Hi Guennadi Liakhovetski,
I have reviewed this patch with AS3711 datasheet.
I cannot find any problems. It looks good.
However, some hardcoded numbers need to be changed
to the bit definitions.
Acked-by: Jingoo Han <jg1.han@samsung.com>
Best regards,
Jingoo Han
> ---
>
> Tested on sh73a0-based kzm9g board. As the commit message says, only one
> mode has been tested and is enabled. That mode copies the sample code from
> the manufacturer. Deviations from that code proved to be fatal for the
> hardware...
>
> drivers/video/backlight/Kconfig | 7 +
> drivers/video/backlight/Makefile | 1 +
> drivers/video/backlight/as3711_bl.c | 379 +++++++++++++++++++++++++++++++++++
> 3 files changed, 387 insertions(+), 0 deletions(-)
> create mode 100644 drivers/video/backlight/as3711_bl.c
>
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 765a945..6ef0ede 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -390,6 +390,13 @@ config BACKLIGHT_TPS65217
> If you have a Texas Instruments TPS65217 say Y to enable the
> backlight driver.
>
> +config BACKLIGHT_AS3711
> + tristate "AS3711 Backlight"
> + depends on BACKLIGHT_CLASS_DEVICE && MFD_AS3711
> + help
> + If you have an Austrian Microsystems AS3711 say Y to enable the
> + backlight driver.
> +
> endif # BACKLIGHT_CLASS_DEVICE
>
> endif # BACKLIGHT_LCD_SUPPORT
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index e7ce729..d3e188f 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -45,3 +45,4 @@ obj-$(CONFIG_BACKLIGHT_PCF50633) += pcf50633-backlight.o
> obj-$(CONFIG_BACKLIGHT_AAT2870) += aat2870_bl.o
> obj-$(CONFIG_BACKLIGHT_OT200) += ot200_bl.o
> obj-$(CONFIG_BACKLIGHT_TPS65217) += tps65217_bl.o
> +obj-$(CONFIG_BACKLIGHT_AS3711) += as3711_bl.o
> diff --git a/drivers/video/backlight/as3711_bl.c b/drivers/video/backlight/as3711_bl.c
> new file mode 100644
> index 0000000..c6bc65d
> --- /dev/null
> +++ b/drivers/video/backlight/as3711_bl.c
> @@ -0,0 +1,379 @@
> +/*
> + * AS3711 PMIC backlight driver, using DCDC Step Up Converters
> + *
> + * Copyright (C) 2012 Renesas Electronics Corporation
> + * Author: Guennadi Liakhovetski, <g.liakhovetski@gmx.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the version 2 of the GNU General Public License as
> + * published by the Free Software Foundation
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/fb.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/as3711.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +enum as3711_bl_type {
> + AS3711_BL_SU1,
> + AS3711_BL_SU2,
> +};
> +
> +struct as3711_bl_data {
> + bool powered;
> + const char *fb_name;
> + struct device *fb_dev;
> + enum as3711_bl_type type;
> + int brightness;
> + struct backlight_device *bl;
> +};
> +
> +struct as3711_bl_supply {
> + struct as3711_bl_data su1;
> + struct as3711_bl_data su2;
> + const struct as3711_bl_pdata *pdata;
> + struct as3711 *as3711;
> +};
> +
> +static struct as3711_bl_supply *to_supply(struct as3711_bl_data *su)
> +{
> + switch (su->type) {
> + case AS3711_BL_SU1:
> + return container_of(su, struct as3711_bl_supply, su1);
> + case AS3711_BL_SU2:
> + return container_of(su, struct as3711_bl_supply, su2);
> + }
> + return NULL;
> +}
> +
> +static int as3711_set_brightness_auto_i(struct as3711_bl_data *data,
> + unsigned int brightness)
> +{
> + struct as3711_bl_supply *supply = to_supply(data);
> + struct as3711 *as3711 = supply->as3711;
> + const struct as3711_bl_pdata *pdata = supply->pdata;
> + int ret = 0;
> +
> + /* Only all equal current values are supported */
> + if (pdata->su2_auto_curr1)
> + ret = regmap_write(as3711->regmap, AS3711_CURR1_VALUE,
> + brightness);
> + if (!ret && pdata->su2_auto_curr2)
> + ret = regmap_write(as3711->regmap, AS3711_CURR2_VALUE,
> + brightness);
> + if (!ret && pdata->su2_auto_curr3)
> + ret = regmap_write(as3711->regmap, AS3711_CURR3_VALUE,
> + brightness);
> +
> + return ret;
> +}
> +
> +static int as3711_set_brightness_v(struct as3711 *as3711,
> + unsigned int brightness,
> + unsigned int reg)
> +{
> + if (brightness > 31)
> + return -EINVAL;
> +
> + return regmap_update_bits(as3711->regmap, reg, 0xf0,
> + brightness << 4);
> +}
> +
> +static int as3711_bl_su2_reset(struct as3711_bl_supply *supply)
> +{
> + struct as3711 *as3711 = supply->as3711;
> + int ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_5,
> + 3, supply->pdata->su2_fbprot);
> + if (!ret)
> + ret = regmap_update_bits(as3711->regmap,
> + AS3711_STEPUP_CONTROL_2, 1, 0);
> + if (!ret)
> + ret = regmap_update_bits(as3711->regmap,
> + AS3711_STEPUP_CONTROL_2, 1, 1);
> + return ret;
> +}
> +
> +/*
> + * Someone with less fragile or less expensive hardware could try to simplify
> + * the brightness adjustment procedure.
> + */
> +static int as3711_bl_update_status(struct backlight_device *bl)
> +{
> + struct as3711_bl_data *data = bl_get_data(bl);
> + struct as3711_bl_supply *supply = to_supply(data);
> + struct as3711 *as3711 = supply->as3711;
> + int brightness = bl->props.brightness;
> + int ret = 0;
> +
> + dev_dbg(&bl->dev, "%s(): brightness %u, pwr %x, blank %x, state %x\n",
> + __func__, bl->props.brightness, bl->props.power,
> + bl->props.fb_blank, bl->props.state);
> +
> + if (bl->props.power != FB_BLANK_UNBLANK ||
> + bl->props.fb_blank != FB_BLANK_UNBLANK ||
> + bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
> + brightness = 0;
> +
> + if (data->type = AS3711_BL_SU1) {
> + ret = as3711_set_brightness_v(as3711, brightness,
> + AS3711_STEPUP_CONTROL_1);
> + } else {
> + const struct as3711_bl_pdata *pdata = supply->pdata;
> +
> + switch (pdata->su2_feedback) {
> + case AS3711_SU2_VOLTAGE:
> + ret = as3711_set_brightness_v(as3711, brightness,
> + AS3711_STEPUP_CONTROL_2);
> + break;
> + case AS3711_SU2_CURR_AUTO:
> + ret = as3711_set_brightness_auto_i(data, brightness / 4);
> + if (ret < 0)
> + return ret;
> + if (brightness) {
> + ret = as3711_bl_su2_reset(supply);
> + if (ret < 0)
> + return ret;
> + udelay(500);
> + ret = as3711_set_brightness_auto_i(data, brightness);
> + } else {
> + ret = regmap_update_bits(as3711->regmap,
> + AS3711_STEPUP_CONTROL_2, 1, 0);
> + }
> + break;
> + /* Manual one current feedback pin below */
> + case AS3711_SU2_CURR1:
> + ret = regmap_write(as3711->regmap, AS3711_CURR1_VALUE,
> + brightness);
> + break;
> + case AS3711_SU2_CURR2:
> + ret = regmap_write(as3711->regmap, AS3711_CURR2_VALUE,
> + brightness);
> + break;
> + case AS3711_SU2_CURR3:
> + ret = regmap_write(as3711->regmap, AS3711_CURR3_VALUE,
> + brightness);
> + break;
> + default:
> + ret = -EINVAL;
> + }
> + }
> + if (!ret)
> + data->brightness = brightness;
> +
> + return ret;
> +}
> +
> +static int as3711_bl_get_brightness(struct backlight_device *bl)
> +{
> + struct as3711_bl_data *data = bl_get_data(bl);
> +
> + return data->brightness;
> +}
> +
> +static const struct backlight_ops as3711_bl_ops = {
> + .update_status = as3711_bl_update_status,
> + .get_brightness = as3711_bl_get_brightness,
> +};
> +
> +static int as3711_bl_init_su2(struct as3711_bl_supply *supply)
> +{
> + struct as3711 *as3711 = supply->as3711;
> + const struct as3711_bl_pdata *pdata = supply->pdata;
> + u8 ctl = 0;
> + int ret;
> +
> + dev_dbg(as3711->dev, "%s(): use %u\n", __func__, pdata->su2_feedback);
> +
> + /* Turn SU2 off */
> + ret = regmap_write(as3711->regmap, AS3711_STEPUP_CONTROL_2, 0);
> + if (ret < 0)
> + return ret;
> +
> + switch (pdata->su2_feedback) {
> + case AS3711_SU2_VOLTAGE:
> + ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 0);
> + break;
> + case AS3711_SU2_CURR1:
> + ctl = 1;
> + ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 1);
> + break;
> + case AS3711_SU2_CURR2:
> + ctl = 4;
> + ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 2);
> + break;
> + case AS3711_SU2_CURR3:
> + ctl = 0x10;
> + ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 3);
> + break;
> + case AS3711_SU2_CURR_AUTO:
> + if (pdata->su2_auto_curr1)
> + ctl = 2;
> + if (pdata->su2_auto_curr2)
> + ctl |= 8;
> + if (pdata->su2_auto_curr3)
> + ctl |= 0x20;
> + ret = 0;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (!ret)
> + ret = regmap_write(as3711->regmap, AS3711_CURR_CONTROL, ctl);
> +
> + return ret;
> +}
> +
> +static int as3711_bl_register(struct platform_device *pdev,
> + unsigned int max_brightness, struct as3711_bl_data *su)
> +{
> + struct backlight_properties props = {.type = BACKLIGHT_RAW,};
> + struct backlight_device *bl;
> +
> + /* max tuning I = 31uA for voltage- and 38250uA for current-feedback */
> + props.max_brightness = max_brightness;
> +
> + bl = backlight_device_register(su->type = AS3711_BL_SU1 ?
> + "as3711-su1" : "as3711-su2",
> + &pdev->dev, su,
> + &as3711_bl_ops, &props);
> + if (IS_ERR(bl)) {
> + dev_err(&pdev->dev, "failed to register backlight\n");
> + return PTR_ERR(bl);
> + }
> +
> + bl->props.brightness = props.max_brightness;
> +
> + backlight_update_status(bl);
> +
> + su->bl = bl;
> +
> + return 0;
> +}
> +
> +static int as3711_backlight_probe(struct platform_device *pdev)
> +{
> + struct as3711_bl_pdata *pdata = dev_get_platdata(&pdev->dev);
> + struct as3711 *as3711 = dev_get_drvdata(pdev->dev.parent);
> + struct as3711_bl_supply *supply;
> + struct as3711_bl_data *su;
> + unsigned int max_brightness;
> + int ret;
> +
> + if (!pdata || (!pdata->su1_fb && !pdata->su2_fb)) {
> + dev_err(&pdev->dev, "No platform data, exiting...\n");
> + return -ENODEV;
> + }
> +
> + /*
> + * Due to possible hardware damage I chose to block all modes,
> + * unsupported on my hardware. Anyone, wishing to use any of those modes
> + * will have to first review the code, then activate and test it.
> + */
> + if (pdata->su1_fb ||
> + pdata->su2_fbprot != AS3711_SU2_GPIO4 ||
> + pdata->su2_feedback != AS3711_SU2_CURR_AUTO) {
> + dev_warn(&pdev->dev,
> + "Attention! An untested mode has been chosen!\n"
> + "Please, review the code, enable, test, and report success:-)\n");
> + return -EINVAL;
> + }
> +
> + supply = devm_kzalloc(&pdev->dev, sizeof(*supply), GFP_KERNEL);
> + if (!supply)
> + return -ENOMEM;
> +
> + supply->as3711 = as3711;
> + supply->pdata = pdata;
> +
> + if (pdata->su1_fb) {
> + su = &supply->su1;
> + su->fb_name = pdata->su1_fb;
> + su->type = AS3711_BL_SU1;
> +
> + max_brightness = min(pdata->su1_max_uA, 31);
> + ret = as3711_bl_register(pdev, max_brightness, su);
> + if (ret < 0)
> + return ret;
> + }
> +
> + if (pdata->su2_fb) {
> + su = &supply->su2;
> + su->fb_name = pdata->su2_fb;
> + su->type = AS3711_BL_SU2;
> +
> + switch (pdata->su2_fbprot) {
> + case AS3711_SU2_GPIO2:
> + case AS3711_SU2_GPIO3:
> + case AS3711_SU2_GPIO4:
> + case AS3711_SU2_LX_SD4:
> + break;
> + default:
> + ret = -EINVAL;
> + goto esu2;
> + }
> +
> + switch (pdata->su2_feedback) {
> + case AS3711_SU2_VOLTAGE:
> + max_brightness = min(pdata->su2_max_uA, 31);
> + break;
> + case AS3711_SU2_CURR1:
> + case AS3711_SU2_CURR2:
> + case AS3711_SU2_CURR3:
> + case AS3711_SU2_CURR_AUTO:
> + max_brightness = min(pdata->su2_max_uA / 150, 255);
> + break;
> + default:
> + ret = -EINVAL;
> + goto esu2;
> + }
> +
> + ret = as3711_bl_init_su2(supply);
> + if (ret < 0)
> + return ret;
> +
> + ret = as3711_bl_register(pdev, max_brightness, su);
> + if (ret < 0)
> + goto esu2;
> + }
> +
> + platform_set_drvdata(pdev, supply);
> +
> + return 0;
> +
> +esu2:
> + backlight_device_unregister(supply->su1.bl);
> + return ret;
> +}
> +
> +static int as3711_backlight_remove(struct platform_device *pdev)
> +{
> + struct as3711_bl_supply *supply = platform_get_drvdata(pdev);
> +
> + backlight_device_unregister(supply->su1.bl);
> + backlight_device_unregister(supply->su2.bl);
> +
> + return 0;
> +}
> +
> +static struct platform_driver as3711_backlight_driver = {
> + .driver = {
> + .name = "as3711-backlight",
> + .owner = THIS_MODULE,
> + },
> + .probe = as3711_backlight_probe,
> + .remove = as3711_backlight_remove,
> +};
> +
> +module_platform_driver(as3711_backlight_driver);
> +
> +MODULE_DESCRIPTION("Backlight Driver for AS3711 PMICs");
> +MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@gmx.de");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:as3711-backlight");
> --
> 1.7.2.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] backlight: add an AS3711 PMIC backlight driver
2013-01-25 5:20 ` Jingoo Han
@ 2013-01-25 7:48 ` Guennadi Liakhovetski
2013-01-28 0:53 ` Jingoo Han
0 siblings, 1 reply; 5+ messages in thread
From: Guennadi Liakhovetski @ 2013-01-25 7:48 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Andrew Morton', linux-kernel, linux-fbdev, linux-sh,
'Magnus Damm', 'Richard Purdie'
Hi Jingoo Han
On Fri, 25 Jan 2013, Jingoo Han wrote:
> On Wednesday, January 09, 2013 2:55 AM, Guennadi Liakhovetski wrote
> >
> > This is an initial commit of a backlight driver, using step-up DCDC power
> > supplies on AS3711 PMIC. Only one mode has actually been tested, several
> > further modes have been implemented "dry," but disabled to avoid accidental
> > hardware damage. Anyone wishing to use any of those modes will have to
> > modify the driver.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>
> CC'ed Andrew Morton.
>
> Hi Guennadi Liakhovetski,
>
> I have reviewed this patch with AS3711 datasheet.
> I cannot find any problems. It looks good.
Thanks for the review.
> However, some hardcoded numbers need to be changed
> to the bit definitions.
Which specific hardcoded values do you mean? A while ago in a discussion
on one of kernel mailing lists a conclusion has been made, that macros do
not always improve code readability. E.g. in a statement like this
+ case AS3711_SU2_CURR_AUTO:
+ if (pdata->su2_auto_curr1)
+ ctl = 2;
+ if (pdata->su2_auto_curr2)
+ ctl |= 8;
+ if (pdata->su2_auto_curr3)
+ ctl |= 0x20;
making it
+ case AS3711_SU2_CURR_AUTO:
+ if (pdata->su2_auto_curr1)
+ ctl = SU2_AUTO_CURR1;
would hardly make it more readable. IMHO it is already pretty clear, that
bit 1 enables the current-1 sink. As long as these fields are only used at
one location in the driver (and they are), using a macro and defining it
elsewhere only makes it harder to see actual values. Speaking of this, a
comment like
/*
* Select, which current sinks shall be used for automatic
* feedback selection
*/
would help much more than any macro names. But as it stands, I think the
current version is also possible to understand :-) If desired, however,
comments can be added in an incremental patch.
Thanks
Guennadi
> Acked-by: Jingoo Han <jg1.han@samsung.com>
>
>
> Best regards,
> Jingoo Han
>
> > ---
> >
> > Tested on sh73a0-based kzm9g board. As the commit message says, only one
> > mode has been tested and is enabled. That mode copies the sample code from
> > the manufacturer. Deviations from that code proved to be fatal for the
> > hardware...
> >
> > drivers/video/backlight/Kconfig | 7 +
> > drivers/video/backlight/Makefile | 1 +
> > drivers/video/backlight/as3711_bl.c | 379 +++++++++++++++++++++++++++++++++++
> > 3 files changed, 387 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/video/backlight/as3711_bl.c
> >
> > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> > index 765a945..6ef0ede 100644
> > --- a/drivers/video/backlight/Kconfig
> > +++ b/drivers/video/backlight/Kconfig
> > @@ -390,6 +390,13 @@ config BACKLIGHT_TPS65217
> > If you have a Texas Instruments TPS65217 say Y to enable the
> > backlight driver.
> >
> > +config BACKLIGHT_AS3711
> > + tristate "AS3711 Backlight"
> > + depends on BACKLIGHT_CLASS_DEVICE && MFD_AS3711
> > + help
> > + If you have an Austrian Microsystems AS3711 say Y to enable the
> > + backlight driver.
> > +
> > endif # BACKLIGHT_CLASS_DEVICE
> >
> > endif # BACKLIGHT_LCD_SUPPORT
> > diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> > index e7ce729..d3e188f 100644
> > --- a/drivers/video/backlight/Makefile
> > +++ b/drivers/video/backlight/Makefile
> > @@ -45,3 +45,4 @@ obj-$(CONFIG_BACKLIGHT_PCF50633) += pcf50633-backlight.o
> > obj-$(CONFIG_BACKLIGHT_AAT2870) += aat2870_bl.o
> > obj-$(CONFIG_BACKLIGHT_OT200) += ot200_bl.o
> > obj-$(CONFIG_BACKLIGHT_TPS65217) += tps65217_bl.o
> > +obj-$(CONFIG_BACKLIGHT_AS3711) += as3711_bl.o
> > diff --git a/drivers/video/backlight/as3711_bl.c b/drivers/video/backlight/as3711_bl.c
> > new file mode 100644
> > index 0000000..c6bc65d
> > --- /dev/null
> > +++ b/drivers/video/backlight/as3711_bl.c
> > @@ -0,0 +1,379 @@
> > +/*
> > + * AS3711 PMIC backlight driver, using DCDC Step Up Converters
> > + *
> > + * Copyright (C) 2012 Renesas Electronics Corporation
> > + * Author: Guennadi Liakhovetski, <g.liakhovetski@gmx.de>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the version 2 of the GNU General Public License as
> > + * published by the Free Software Foundation
> > + */
> > +
> > +#include <linux/backlight.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/fb.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mfd/as3711.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +
> > +enum as3711_bl_type {
> > + AS3711_BL_SU1,
> > + AS3711_BL_SU2,
> > +};
> > +
> > +struct as3711_bl_data {
> > + bool powered;
> > + const char *fb_name;
> > + struct device *fb_dev;
> > + enum as3711_bl_type type;
> > + int brightness;
> > + struct backlight_device *bl;
> > +};
> > +
> > +struct as3711_bl_supply {
> > + struct as3711_bl_data su1;
> > + struct as3711_bl_data su2;
> > + const struct as3711_bl_pdata *pdata;
> > + struct as3711 *as3711;
> > +};
> > +
> > +static struct as3711_bl_supply *to_supply(struct as3711_bl_data *su)
> > +{
> > + switch (su->type) {
> > + case AS3711_BL_SU1:
> > + return container_of(su, struct as3711_bl_supply, su1);
> > + case AS3711_BL_SU2:
> > + return container_of(su, struct as3711_bl_supply, su2);
> > + }
> > + return NULL;
> > +}
> > +
> > +static int as3711_set_brightness_auto_i(struct as3711_bl_data *data,
> > + unsigned int brightness)
> > +{
> > + struct as3711_bl_supply *supply = to_supply(data);
> > + struct as3711 *as3711 = supply->as3711;
> > + const struct as3711_bl_pdata *pdata = supply->pdata;
> > + int ret = 0;
> > +
> > + /* Only all equal current values are supported */
> > + if (pdata->su2_auto_curr1)
> > + ret = regmap_write(as3711->regmap, AS3711_CURR1_VALUE,
> > + brightness);
> > + if (!ret && pdata->su2_auto_curr2)
> > + ret = regmap_write(as3711->regmap, AS3711_CURR2_VALUE,
> > + brightness);
> > + if (!ret && pdata->su2_auto_curr3)
> > + ret = regmap_write(as3711->regmap, AS3711_CURR3_VALUE,
> > + brightness);
> > +
> > + return ret;
> > +}
> > +
> > +static int as3711_set_brightness_v(struct as3711 *as3711,
> > + unsigned int brightness,
> > + unsigned int reg)
> > +{
> > + if (brightness > 31)
> > + return -EINVAL;
> > +
> > + return regmap_update_bits(as3711->regmap, reg, 0xf0,
> > + brightness << 4);
> > +}
> > +
> > +static int as3711_bl_su2_reset(struct as3711_bl_supply *supply)
> > +{
> > + struct as3711 *as3711 = supply->as3711;
> > + int ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_5,
> > + 3, supply->pdata->su2_fbprot);
> > + if (!ret)
> > + ret = regmap_update_bits(as3711->regmap,
> > + AS3711_STEPUP_CONTROL_2, 1, 0);
> > + if (!ret)
> > + ret = regmap_update_bits(as3711->regmap,
> > + AS3711_STEPUP_CONTROL_2, 1, 1);
> > + return ret;
> > +}
> > +
> > +/*
> > + * Someone with less fragile or less expensive hardware could try to simplify
> > + * the brightness adjustment procedure.
> > + */
> > +static int as3711_bl_update_status(struct backlight_device *bl)
> > +{
> > + struct as3711_bl_data *data = bl_get_data(bl);
> > + struct as3711_bl_supply *supply = to_supply(data);
> > + struct as3711 *as3711 = supply->as3711;
> > + int brightness = bl->props.brightness;
> > + int ret = 0;
> > +
> > + dev_dbg(&bl->dev, "%s(): brightness %u, pwr %x, blank %x, state %x\n",
> > + __func__, bl->props.brightness, bl->props.power,
> > + bl->props.fb_blank, bl->props.state);
> > +
> > + if (bl->props.power != FB_BLANK_UNBLANK ||
> > + bl->props.fb_blank != FB_BLANK_UNBLANK ||
> > + bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
> > + brightness = 0;
> > +
> > + if (data->type = AS3711_BL_SU1) {
> > + ret = as3711_set_brightness_v(as3711, brightness,
> > + AS3711_STEPUP_CONTROL_1);
> > + } else {
> > + const struct as3711_bl_pdata *pdata = supply->pdata;
> > +
> > + switch (pdata->su2_feedback) {
> > + case AS3711_SU2_VOLTAGE:
> > + ret = as3711_set_brightness_v(as3711, brightness,
> > + AS3711_STEPUP_CONTROL_2);
> > + break;
> > + case AS3711_SU2_CURR_AUTO:
> > + ret = as3711_set_brightness_auto_i(data, brightness / 4);
> > + if (ret < 0)
> > + return ret;
> > + if (brightness) {
> > + ret = as3711_bl_su2_reset(supply);
> > + if (ret < 0)
> > + return ret;
> > + udelay(500);
> > + ret = as3711_set_brightness_auto_i(data, brightness);
> > + } else {
> > + ret = regmap_update_bits(as3711->regmap,
> > + AS3711_STEPUP_CONTROL_2, 1, 0);
> > + }
> > + break;
> > + /* Manual one current feedback pin below */
> > + case AS3711_SU2_CURR1:
> > + ret = regmap_write(as3711->regmap, AS3711_CURR1_VALUE,
> > + brightness);
> > + break;
> > + case AS3711_SU2_CURR2:
> > + ret = regmap_write(as3711->regmap, AS3711_CURR2_VALUE,
> > + brightness);
> > + break;
> > + case AS3711_SU2_CURR3:
> > + ret = regmap_write(as3711->regmap, AS3711_CURR3_VALUE,
> > + brightness);
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + }
> > + }
> > + if (!ret)
> > + data->brightness = brightness;
> > +
> > + return ret;
> > +}
> > +
> > +static int as3711_bl_get_brightness(struct backlight_device *bl)
> > +{
> > + struct as3711_bl_data *data = bl_get_data(bl);
> > +
> > + return data->brightness;
> > +}
> > +
> > +static const struct backlight_ops as3711_bl_ops = {
> > + .update_status = as3711_bl_update_status,
> > + .get_brightness = as3711_bl_get_brightness,
> > +};
> > +
> > +static int as3711_bl_init_su2(struct as3711_bl_supply *supply)
> > +{
> > + struct as3711 *as3711 = supply->as3711;
> > + const struct as3711_bl_pdata *pdata = supply->pdata;
> > + u8 ctl = 0;
> > + int ret;
> > +
> > + dev_dbg(as3711->dev, "%s(): use %u\n", __func__, pdata->su2_feedback);
> > +
> > + /* Turn SU2 off */
> > + ret = regmap_write(as3711->regmap, AS3711_STEPUP_CONTROL_2, 0);
> > + if (ret < 0)
> > + return ret;
> > +
> > + switch (pdata->su2_feedback) {
> > + case AS3711_SU2_VOLTAGE:
> > + ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 0);
> > + break;
> > + case AS3711_SU2_CURR1:
> > + ctl = 1;
> > + ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 1);
> > + break;
> > + case AS3711_SU2_CURR2:
> > + ctl = 4;
> > + ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 2);
> > + break;
> > + case AS3711_SU2_CURR3:
> > + ctl = 0x10;
> > + ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 3);
> > + break;
> > + case AS3711_SU2_CURR_AUTO:
> > + if (pdata->su2_auto_curr1)
> > + ctl = 2;
> > + if (pdata->su2_auto_curr2)
> > + ctl |= 8;
> > + if (pdata->su2_auto_curr3)
> > + ctl |= 0x20;
> > + ret = 0;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + if (!ret)
> > + ret = regmap_write(as3711->regmap, AS3711_CURR_CONTROL, ctl);
> > +
> > + return ret;
> > +}
> > +
> > +static int as3711_bl_register(struct platform_device *pdev,
> > + unsigned int max_brightness, struct as3711_bl_data *su)
> > +{
> > + struct backlight_properties props = {.type = BACKLIGHT_RAW,};
> > + struct backlight_device *bl;
> > +
> > + /* max tuning I = 31uA for voltage- and 38250uA for current-feedback */
> > + props.max_brightness = max_brightness;
> > +
> > + bl = backlight_device_register(su->type = AS3711_BL_SU1 ?
> > + "as3711-su1" : "as3711-su2",
> > + &pdev->dev, su,
> > + &as3711_bl_ops, &props);
> > + if (IS_ERR(bl)) {
> > + dev_err(&pdev->dev, "failed to register backlight\n");
> > + return PTR_ERR(bl);
> > + }
> > +
> > + bl->props.brightness = props.max_brightness;
> > +
> > + backlight_update_status(bl);
> > +
> > + su->bl = bl;
> > +
> > + return 0;
> > +}
> > +
> > +static int as3711_backlight_probe(struct platform_device *pdev)
> > +{
> > + struct as3711_bl_pdata *pdata = dev_get_platdata(&pdev->dev);
> > + struct as3711 *as3711 = dev_get_drvdata(pdev->dev.parent);
> > + struct as3711_bl_supply *supply;
> > + struct as3711_bl_data *su;
> > + unsigned int max_brightness;
> > + int ret;
> > +
> > + if (!pdata || (!pdata->su1_fb && !pdata->su2_fb)) {
> > + dev_err(&pdev->dev, "No platform data, exiting...\n");
> > + return -ENODEV;
> > + }
> > +
> > + /*
> > + * Due to possible hardware damage I chose to block all modes,
> > + * unsupported on my hardware. Anyone, wishing to use any of those modes
> > + * will have to first review the code, then activate and test it.
> > + */
> > + if (pdata->su1_fb ||
> > + pdata->su2_fbprot != AS3711_SU2_GPIO4 ||
> > + pdata->su2_feedback != AS3711_SU2_CURR_AUTO) {
> > + dev_warn(&pdev->dev,
> > + "Attention! An untested mode has been chosen!\n"
> > + "Please, review the code, enable, test, and report success:-)\n");
> > + return -EINVAL;
> > + }
> > +
> > + supply = devm_kzalloc(&pdev->dev, sizeof(*supply), GFP_KERNEL);
> > + if (!supply)
> > + return -ENOMEM;
> > +
> > + supply->as3711 = as3711;
> > + supply->pdata = pdata;
> > +
> > + if (pdata->su1_fb) {
> > + su = &supply->su1;
> > + su->fb_name = pdata->su1_fb;
> > + su->type = AS3711_BL_SU1;
> > +
> > + max_brightness = min(pdata->su1_max_uA, 31);
> > + ret = as3711_bl_register(pdev, max_brightness, su);
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + if (pdata->su2_fb) {
> > + su = &supply->su2;
> > + su->fb_name = pdata->su2_fb;
> > + su->type = AS3711_BL_SU2;
> > +
> > + switch (pdata->su2_fbprot) {
> > + case AS3711_SU2_GPIO2:
> > + case AS3711_SU2_GPIO3:
> > + case AS3711_SU2_GPIO4:
> > + case AS3711_SU2_LX_SD4:
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + goto esu2;
> > + }
> > +
> > + switch (pdata->su2_feedback) {
> > + case AS3711_SU2_VOLTAGE:
> > + max_brightness = min(pdata->su2_max_uA, 31);
> > + break;
> > + case AS3711_SU2_CURR1:
> > + case AS3711_SU2_CURR2:
> > + case AS3711_SU2_CURR3:
> > + case AS3711_SU2_CURR_AUTO:
> > + max_brightness = min(pdata->su2_max_uA / 150, 255);
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + goto esu2;
> > + }
> > +
> > + ret = as3711_bl_init_su2(supply);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = as3711_bl_register(pdev, max_brightness, su);
> > + if (ret < 0)
> > + goto esu2;
> > + }
> > +
> > + platform_set_drvdata(pdev, supply);
> > +
> > + return 0;
> > +
> > +esu2:
> > + backlight_device_unregister(supply->su1.bl);
> > + return ret;
> > +}
> > +
> > +static int as3711_backlight_remove(struct platform_device *pdev)
> > +{
> > + struct as3711_bl_supply *supply = platform_get_drvdata(pdev);
> > +
> > + backlight_device_unregister(supply->su1.bl);
> > + backlight_device_unregister(supply->su2.bl);
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver as3711_backlight_driver = {
> > + .driver = {
> > + .name = "as3711-backlight",
> > + .owner = THIS_MODULE,
> > + },
> > + .probe = as3711_backlight_probe,
> > + .remove = as3711_backlight_remove,
> > +};
> > +
> > +module_platform_driver(as3711_backlight_driver);
> > +
> > +MODULE_DESCRIPTION("Backlight Driver for AS3711 PMICs");
> > +MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@gmx.de");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:as3711-backlight");
> > --
> > 1.7.2.5
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] backlight: add an AS3711 PMIC backlight driver
2013-01-25 7:48 ` Guennadi Liakhovetski
@ 2013-01-28 0:53 ` Jingoo Han
2013-02-05 9:08 ` Guennadi Liakhovetski
0 siblings, 1 reply; 5+ messages in thread
From: Jingoo Han @ 2013-01-28 0:53 UTC (permalink / raw)
To: 'Guennadi Liakhovetski'
Cc: 'Andrew Morton', linux-kernel, linux-fbdev, linux-sh,
'Magnus Damm', 'Richard Purdie'
On Friday, January 25, 2013 4:49 PM, Guennadi Liakhovetski wrote
>
> Hi Jingoo Han
>
> On Fri, 25 Jan 2013, Jingoo Han wrote:
>
> > On Wednesday, January 09, 2013 2:55 AM, Guennadi Liakhovetski wrote
> > >
> > > This is an initial commit of a backlight driver, using step-up DCDC power
> > > supplies on AS3711 PMIC. Only one mode has actually been tested, several
> > > further modes have been implemented "dry," but disabled to avoid accidental
> > > hardware damage. Anyone wishing to use any of those modes will have to
> > > modify the driver.
> > >
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >
> > CC'ed Andrew Morton.
> >
> > Hi Guennadi Liakhovetski,
> >
> > I have reviewed this patch with AS3711 datasheet.
> > I cannot find any problems. It looks good.
>
> Thanks for the review.
>
> > However, some hardcoded numbers need to be changed
> > to the bit definitions.
>
> Which specific hardcoded values do you mean? A while ago in a discussion
> on one of kernel mailing lists a conclusion has been made, that macros do
> not always improve code readability. E.g. in a statement like this
>
> + case AS3711_SU2_CURR_AUTO:
> + if (pdata->su2_auto_curr1)
> + ctl = 2;
> + if (pdata->su2_auto_curr2)
> + ctl |= 8;
> + if (pdata->su2_auto_curr3)
> + ctl |= 0x20;
>
> making it
>
> + case AS3711_SU2_CURR_AUTO:
> + if (pdata->su2_auto_curr1)
> + ctl = SU2_AUTO_CURR1;
>
> would hardly make it more readable. IMHO it is already pretty clear, that
> bit 1 enables the current-1 sink. As long as these fields are only used at
> one location in the driver (and they are), using a macro and defining it
> elsewhere only makes it harder to see actual values. Speaking of this, a
> comment like
I don't think so. Some people feel that it is not clear a bit.
Of course, I know what you mean.
Also, your comment is reasonable.
However, personally, I prefer the latter.
Because, I think that the meaning of bits is more important than
actual bits. In the latter case, we can notice the meaning of bits
more easily.
Best regards,
Jingoo Han
>
> /*
> * Select, which current sinks shall be used for automatic
> * feedback selection
> */
>
> would help much more than any macro names. But as it stands, I think the
> current version is also possible to understand :-) If desired, however,
> comments can be added in an incremental patch
>
> Thanks
> Guennadi
>
> > Acked-by: Jingoo Han <jg1.han@samsung.com>
> >
> >
> > Best regards,
> > Jingoo Han
> >
> > > ---
> > >
> > > Tested on sh73a0-based kzm9g board. As the commit message says, only one
> > > mode has been tested and is enabled. That mode copies the sample code from
> > > the manufacturer. Deviations from that code proved to be fatal for the
> > > hardware...
> > >
> > > drivers/video/backlight/Kconfig | 7 +
> > > drivers/video/backlight/Makefile | 1 +
> > > drivers/video/backlight/as3711_bl.c | 379 +++++++++++++++++++++++++++++++++++
> > > 3 files changed, 387 insertions(+), 0 deletions(-)
> > > create mode 100644 drivers/video/backlight/as3711_bl.c
> > >
> > > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> > > index 765a945..6ef0ede 100644
> > > --- a/drivers/video/backlight/Kconfig
> > > +++ b/drivers/video/backlight/Kconfig
> > > @@ -390,6 +390,13 @@ config BACKLIGHT_TPS65217
> > > If you have a Texas Instruments TPS65217 say Y to enable the
> > > backlight driver.
> > >
> > > +config BACKLIGHT_AS3711
> > > + tristate "AS3711 Backlight"
> > > + depends on BACKLIGHT_CLASS_DEVICE && MFD_AS3711
> > > + help
> > > + If you have an Austrian Microsystems AS3711 say Y to enable the
> > > + backlight driver.
> > > +
> > > endif # BACKLIGHT_CLASS_DEVICE
> > >
> > > endif # BACKLIGHT_LCD_SUPPORT
> > > diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> > > index e7ce729..d3e188f 100644
> > > --- a/drivers/video/backlight/Makefile
> > > +++ b/drivers/video/backlight/Makefile
> > > @@ -45,3 +45,4 @@ obj-$(CONFIG_BACKLIGHT_PCF50633) += pcf50633-backlight.o
> > > obj-$(CONFIG_BACKLIGHT_AAT2870) += aat2870_bl.o
> > > obj-$(CONFIG_BACKLIGHT_OT200) += ot200_bl.o
> > > obj-$(CONFIG_BACKLIGHT_TPS65217) += tps65217_bl.o
> > > +obj-$(CONFIG_BACKLIGHT_AS3711) += as3711_bl.o
> > > diff --git a/drivers/video/backlight/as3711_bl.c b/drivers/video/backlight/as3711_bl.c
> > > new file mode 100644
> > > index 0000000..c6bc65d
> > > --- /dev/null
> > > +++ b/drivers/video/backlight/as3711_bl.c
> > > @@ -0,0 +1,379 @@
> > > +/*
> > > + * AS3711 PMIC backlight driver, using DCDC Step Up Converters
> > > + *
> > > + * Copyright (C) 2012 Renesas Electronics Corporation
> > > + * Author: Guennadi Liakhovetski, <g.liakhovetski@gmx.de>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the version 2 of the GNU General Public License as
> > > + * published by the Free Software Foundation
> > > + */
> > > +
> > > +#include <linux/backlight.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/device.h>
> > > +#include <linux/err.h>
> > > +#include <linux/fb.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/mfd/as3711.h>
> > > +#include <linux/module.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/slab.h>
> > > +
> > > +enum as3711_bl_type {
> > > + AS3711_BL_SU1,
> > > + AS3711_BL_SU2,
> > > +};
> > > +
> > > +struct as3711_bl_data {
> > > + bool powered;
> > > + const char *fb_name;
> > > + struct device *fb_dev;
> > > + enum as3711_bl_type type;
> > > + int brightness;
> > > + struct backlight_device *bl;
> > > +};
> > > +
> > > +struct as3711_bl_supply {
> > > + struct as3711_bl_data su1;
> > > + struct as3711_bl_data su2;
> > > + const struct as3711_bl_pdata *pdata;
> > > + struct as3711 *as3711;
> > > +};
> > > +
> > > +static struct as3711_bl_supply *to_supply(struct as3711_bl_data *su)
> > > +{
> > > + switch (su->type) {
> > > + case AS3711_BL_SU1:
> > > + return container_of(su, struct as3711_bl_supply, su1);
> > > + case AS3711_BL_SU2:
> > > + return container_of(su, struct as3711_bl_supply, su2);
> > > + }
> > > + return NULL;
> > > +}
> > > +
> > > +static int as3711_set_brightness_auto_i(struct as3711_bl_data *data,
> > > + unsigned int brightness)
> > > +{
> > > + struct as3711_bl_supply *supply = to_supply(data);
> > > + struct as3711 *as3711 = supply->as3711;
> > > + const struct as3711_bl_pdata *pdata = supply->pdata;
> > > + int ret = 0;
> > > +
> > > + /* Only all equal current values are supported */
> > > + if (pdata->su2_auto_curr1)
> > > + ret = regmap_write(as3711->regmap, AS3711_CURR1_VALUE,
> > > + brightness);
> > > + if (!ret && pdata->su2_auto_curr2)
> > > + ret = regmap_write(as3711->regmap, AS3711_CURR2_VALUE,
> > > + brightness);
> > > + if (!ret && pdata->su2_auto_curr3)
> > > + ret = regmap_write(as3711->regmap, AS3711_CURR3_VALUE,
> > > + brightness);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int as3711_set_brightness_v(struct as3711 *as3711,
> > > + unsigned int brightness,
> > > + unsigned int reg)
> > > +{
> > > + if (brightness > 31)
> > > + return -EINVAL;
> > > +
> > > + return regmap_update_bits(as3711->regmap, reg, 0xf0,
> > > + brightness << 4);
> > > +}
> > > +
> > > +static int as3711_bl_su2_reset(struct as3711_bl_supply *supply)
> > > +{
> > > + struct as3711 *as3711 = supply->as3711;
> > > + int ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_5,
> > > + 3, supply->pdata->su2_fbprot);
> > > + if (!ret)
> > > + ret = regmap_update_bits(as3711->regmap,
> > > + AS3711_STEPUP_CONTROL_2, 1, 0);
> > > + if (!ret)
> > > + ret = regmap_update_bits(as3711->regmap,
> > > + AS3711_STEPUP_CONTROL_2, 1, 1);
> > > + return ret;
> > > +}
> > > +
> > > +/*
> > > + * Someone with less fragile or less expensive hardware could try to simplify
> > > + * the brightness adjustment procedure.
> > > + */
> > > +static int as3711_bl_update_status(struct backlight_device *bl)
> > > +{
> > > + struct as3711_bl_data *data = bl_get_data(bl);
> > > + struct as3711_bl_supply *supply = to_supply(data);
> > > + struct as3711 *as3711 = supply->as3711;
> > > + int brightness = bl->props.brightness;
> > > + int ret = 0;
> > > +
> > > + dev_dbg(&bl->dev, "%s(): brightness %u, pwr %x, blank %x, state %x\n",
> > > + __func__, bl->props.brightness, bl->props.power,
> > > + bl->props.fb_blank, bl->props.state);
> > > +
> > > + if (bl->props.power != FB_BLANK_UNBLANK ||
> > > + bl->props.fb_blank != FB_BLANK_UNBLANK ||
> > > + bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
> > > + brightness = 0;
> > > +
> > > + if (data->type = AS3711_BL_SU1) {
> > > + ret = as3711_set_brightness_v(as3711, brightness,
> > > + AS3711_STEPUP_CONTROL_1);
> > > + } else {
> > > + const struct as3711_bl_pdata *pdata = supply->pdata;
> > > +
> > > + switch (pdata->su2_feedback) {
> > > + case AS3711_SU2_VOLTAGE:
> > > + ret = as3711_set_brightness_v(as3711, brightness,
> > > + AS3711_STEPUP_CONTROL_2);
> > > + break;
> > > + case AS3711_SU2_CURR_AUTO:
> > > + ret = as3711_set_brightness_auto_i(data, brightness / 4);
> > > + if (ret < 0)
> > > + return ret;
> > > + if (brightness) {
> > > + ret = as3711_bl_su2_reset(supply);
> > > + if (ret < 0)
> > > + return ret;
> > > + udelay(500);
> > > + ret = as3711_set_brightness_auto_i(data, brightness);
> > > + } else {
> > > + ret = regmap_update_bits(as3711->regmap,
> > > + AS3711_STEPUP_CONTROL_2, 1, 0);
> > > + }
> > > + break;
> > > + /* Manual one current feedback pin below */
> > > + case AS3711_SU2_CURR1:
> > > + ret = regmap_write(as3711->regmap, AS3711_CURR1_VALUE,
> > > + brightness);
> > > + break;
> > > + case AS3711_SU2_CURR2:
> > > + ret = regmap_write(as3711->regmap, AS3711_CURR2_VALUE,
> > > + brightness);
> > > + break;
> > > + case AS3711_SU2_CURR3:
> > > + ret = regmap_write(as3711->regmap, AS3711_CURR3_VALUE,
> > > + brightness);
> > > + break;
> > > + default:
> > > + ret = -EINVAL;
> > > + }
> > > + }
> > > + if (!ret)
> > > + data->brightness = brightness;
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int as3711_bl_get_brightness(struct backlight_device *bl)
> > > +{
> > > + struct as3711_bl_data *data = bl_get_data(bl);
> > > +
> > > + return data->brightness;
> > > +}
> > > +
> > > +static const struct backlight_ops as3711_bl_ops = {
> > > + .update_status = as3711_bl_update_status,
> > > + .get_brightness = as3711_bl_get_brightness,
> > > +};
> > > +
> > > +static int as3711_bl_init_su2(struct as3711_bl_supply *supply)
> > > +{
> > > + struct as3711 *as3711 = supply->as3711;
> > > + const struct as3711_bl_pdata *pdata = supply->pdata;
> > > + u8 ctl = 0;
> > > + int ret;
> > > +
> > > + dev_dbg(as3711->dev, "%s(): use %u\n", __func__, pdata->su2_feedback);
> > > +
> > > + /* Turn SU2 off */
> > > + ret = regmap_write(as3711->regmap, AS3711_STEPUP_CONTROL_2, 0);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + switch (pdata->su2_feedback) {
> > > + case AS3711_SU2_VOLTAGE:
> > > + ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 0);
> > > + break;
> > > + case AS3711_SU2_CURR1:
> > > + ctl = 1;
> > > + ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 1);
> > > + break;
> > > + case AS3711_SU2_CURR2:
> > > + ctl = 4;
> > > + ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 2);
> > > + break;
> > > + case AS3711_SU2_CURR3:
> > > + ctl = 0x10;
> > > + ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 3);
> > > + break;
> > > + case AS3711_SU2_CURR_AUTO:
> > > + if (pdata->su2_auto_curr1)
> > > + ctl = 2;
> > > + if (pdata->su2_auto_curr2)
> > > + ctl |= 8;
> > > + if (pdata->su2_auto_curr3)
> > > + ctl |= 0x20;
> > > + ret = 0;
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (!ret)
> > > + ret = regmap_write(as3711->regmap, AS3711_CURR_CONTROL, ctl);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int as3711_bl_register(struct platform_device *pdev,
> > > + unsigned int max_brightness, struct as3711_bl_data *su)
> > > +{
> > > + struct backlight_properties props = {.type = BACKLIGHT_RAW,};
> > > + struct backlight_device *bl;
> > > +
> > > + /* max tuning I = 31uA for voltage- and 38250uA for current-feedback */
> > > + props.max_brightness = max_brightness;
> > > +
> > > + bl = backlight_device_register(su->type = AS3711_BL_SU1 ?
> > > + "as3711-su1" : "as3711-su2",
> > > + &pdev->dev, su,
> > > + &as3711_bl_ops, &props);
> > > + if (IS_ERR(bl)) {
> > > + dev_err(&pdev->dev, "failed to register backlight\n");
> > > + return PTR_ERR(bl);
> > > + }
> > > +
> > > + bl->props.brightness = props.max_brightness;
> > > +
> > > + backlight_update_status(bl);
> > > +
> > > + su->bl = bl;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int as3711_backlight_probe(struct platform_device *pdev)
> > > +{
> > > + struct as3711_bl_pdata *pdata = dev_get_platdata(&pdev->dev);
> > > + struct as3711 *as3711 = dev_get_drvdata(pdev->dev.parent);
> > > + struct as3711_bl_supply *supply;
> > > + struct as3711_bl_data *su;
> > > + unsigned int max_brightness;
> > > + int ret;
> > > +
> > > + if (!pdata || (!pdata->su1_fb && !pdata->su2_fb)) {
> > > + dev_err(&pdev->dev, "No platform data, exiting...\n");
> > > + return -ENODEV;
> > > + }
> > > +
> > > + /*
> > > + * Due to possible hardware damage I chose to block all modes,
> > > + * unsupported on my hardware. Anyone, wishing to use any of those modes
> > > + * will have to first review the code, then activate and test it.
> > > + */
> > > + if (pdata->su1_fb ||
> > > + pdata->su2_fbprot != AS3711_SU2_GPIO4 ||
> > > + pdata->su2_feedback != AS3711_SU2_CURR_AUTO) {
> > > + dev_warn(&pdev->dev,
> > > + "Attention! An untested mode has been chosen!\n"
> > > + "Please, review the code, enable, test, and report success:-)\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + supply = devm_kzalloc(&pdev->dev, sizeof(*supply), GFP_KERNEL);
> > > + if (!supply)
> > > + return -ENOMEM;
> > > +
> > > + supply->as3711 = as3711;
> > > + supply->pdata = pdata;
> > > +
> > > + if (pdata->su1_fb) {
> > > + su = &supply->su1;
> > > + su->fb_name = pdata->su1_fb;
> > > + su->type = AS3711_BL_SU1;
> > > +
> > > + max_brightness = min(pdata->su1_max_uA, 31);
> > > + ret = as3711_bl_register(pdev, max_brightness, su);
> > > + if (ret < 0)
> > > + return ret;
> > > + }
> > > +
> > > + if (pdata->su2_fb) {
> > > + su = &supply->su2;
> > > + su->fb_name = pdata->su2_fb;
> > > + su->type = AS3711_BL_SU2;
> > > +
> > > + switch (pdata->su2_fbprot) {
> > > + case AS3711_SU2_GPIO2:
> > > + case AS3711_SU2_GPIO3:
> > > + case AS3711_SU2_GPIO4:
> > > + case AS3711_SU2_LX_SD4:
> > > + break;
> > > + default:
> > > + ret = -EINVAL;
> > > + goto esu2;
> > > + }
> > > +
> > > + switch (pdata->su2_feedback) {
> > > + case AS3711_SU2_VOLTAGE:
> > > + max_brightness = min(pdata->su2_max_uA, 31);
> > > + break;
> > > + case AS3711_SU2_CURR1:
> > > + case AS3711_SU2_CURR2:
> > > + case AS3711_SU2_CURR3:
> > > + case AS3711_SU2_CURR_AUTO:
> > > + max_brightness = min(pdata->su2_max_uA / 150, 255);
> > > + break;
> > > + default:
> > > + ret = -EINVAL;
> > > + goto esu2;
> > > + }
> > > +
> > > + ret = as3711_bl_init_su2(supply);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + ret = as3711_bl_register(pdev, max_brightness, su);
> > > + if (ret < 0)
> > > + goto esu2;
> > > + }
> > > +
> > > + platform_set_drvdata(pdev, supply);
> > > +
> > > + return 0;
> > > +
> > > +esu2:
> > > + backlight_device_unregister(supply->su1.bl);
> > > + return ret;
> > > +}
> > > +
> > > +static int as3711_backlight_remove(struct platform_device *pdev)
> > > +{
> > > + struct as3711_bl_supply *supply = platform_get_drvdata(pdev);
> > > +
> > > + backlight_device_unregister(supply->su1.bl);
> > > + backlight_device_unregister(supply->su2.bl);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static struct platform_driver as3711_backlight_driver = {
> > > + .driver = {
> > > + .name = "as3711-backlight",
> > > + .owner = THIS_MODULE,
> > > + },
> > > + .probe = as3711_backlight_probe,
> > > + .remove = as3711_backlight_remove,
> > > +};
> > > +
> > > +module_platform_driver(as3711_backlight_driver);
> > > +
> > > +MODULE_DESCRIPTION("Backlight Driver for AS3711 PMICs");
> > > +MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@gmx.de");
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_ALIAS("platform:as3711-backlight");
> > > --
> > > 1.7.2.5
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] backlight: add an AS3711 PMIC backlight driver
2013-01-28 0:53 ` Jingoo Han
@ 2013-02-05 9:08 ` Guennadi Liakhovetski
0 siblings, 0 replies; 5+ messages in thread
From: Guennadi Liakhovetski @ 2013-02-05 9:08 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Andrew Morton', linux-kernel, linux-fbdev, linux-sh,
'Magnus Damm', 'Richard Purdie'
Hi Richard
Could you tell us your opinion on this:
On Mon, 28 Jan 2013, Jingoo Han wrote:
> On Friday, January 25, 2013 4:49 PM, Guennadi Liakhovetski wrote
> >
> > Hi Jingoo Han
> >
> > On Fri, 25 Jan 2013, Jingoo Han wrote:
> >
> > > On Wednesday, January 09, 2013 2:55 AM, Guennadi Liakhovetski wrote
> > > >
> > > > This is an initial commit of a backlight driver, using step-up DCDC power
> > > > supplies on AS3711 PMIC. Only one mode has actually been tested, several
> > > > further modes have been implemented "dry," but disabled to avoid accidental
> > > > hardware damage. Anyone wishing to use any of those modes will have to
> > > > modify the driver.
> > > >
> > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > >
> > > CC'ed Andrew Morton.
> > >
> > > Hi Guennadi Liakhovetski,
> > >
> > > I have reviewed this patch with AS3711 datasheet.
> > > I cannot find any problems. It looks good.
> >
> > Thanks for the review.
> >
> > > However, some hardcoded numbers need to be changed
> > > to the bit definitions.
> >
> > Which specific hardcoded values do you mean? A while ago in a discussion
> > on one of kernel mailing lists a conclusion has been made, that macros do
> > not always improve code readability. E.g. in a statement like this
> >
> > + case AS3711_SU2_CURR_AUTO:
> > + if (pdata->su2_auto_curr1)
> > + ctl = 2;
> > + if (pdata->su2_auto_curr2)
> > + ctl |= 8;
> > + if (pdata->su2_auto_curr3)
> > + ctl |= 0x20;
> >
> > making it
> >
> > + case AS3711_SU2_CURR_AUTO:
> > + if (pdata->su2_auto_curr1)
> > + ctl = SU2_AUTO_CURR1;
> >
> > would hardly make it more readable. IMHO it is already pretty clear, that
> > bit 1 enables the current-1 sink. As long as these fields are only used at
> > one location in the driver (and they are), using a macro and defining it
> > elsewhere only makes it harder to see actual values. Speaking of this, a
> > comment like
>
> I don't think so. Some people feel that it is not clear a bit.
> Of course, I know what you mean.
> Also, your comment is reasonable.
> However, personally, I prefer the latter.
> Because, I think that the meaning of bits is more important than
> actual bits. In the latter case, we can notice the meaning of bits
> more easily.
Do you also find preferable to use symbolic names for every single bit,
occurring in a driver, or you agree, that excessive use of such macros can
only needlessly clutter the code?
Thanks
Guennadi
> Best regards,
> Jingoo Han
>
> >
> > /*
> > * Select, which current sinks shall be used for automatic
> > * feedback selection
> > */
> >
> > would help much more than any macro names. But as it stands, I think the
> > current version is also possible to understand :-) If desired, however,
> > comments can be added in an incremental patch
>
> >
> > Thanks
> > Guennadi
> >
> > > Acked-by: Jingoo Han <jg1.han@samsung.com>
> > >
> > >
> > > Best regards,
> > > Jingoo Han
> > >
> > > > ---
> > > >
> > > > Tested on sh73a0-based kzm9g board. As the commit message says, only one
> > > > mode has been tested and is enabled. That mode copies the sample code from
> > > > the manufacturer. Deviations from that code proved to be fatal for the
> > > > hardware...
> > > >
> > > > drivers/video/backlight/Kconfig | 7 +
> > > > drivers/video/backlight/Makefile | 1 +
> > > > drivers/video/backlight/as3711_bl.c | 379 +++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 387 insertions(+), 0 deletions(-)
> > > > create mode 100644 drivers/video/backlight/as3711_bl.c
> > > >
> > > > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> > > > index 765a945..6ef0ede 100644
> > > > --- a/drivers/video/backlight/Kconfig
> > > > +++ b/drivers/video/backlight/Kconfig
> > > > @@ -390,6 +390,13 @@ config BACKLIGHT_TPS65217
> > > > If you have a Texas Instruments TPS65217 say Y to enable the
> > > > backlight driver.
> > > >
> > > > +config BACKLIGHT_AS3711
> > > > + tristate "AS3711 Backlight"
> > > > + depends on BACKLIGHT_CLASS_DEVICE && MFD_AS3711
> > > > + help
> > > > + If you have an Austrian Microsystems AS3711 say Y to enable the
> > > > + backlight driver.
> > > > +
> > > > endif # BACKLIGHT_CLASS_DEVICE
> > > >
> > > > endif # BACKLIGHT_LCD_SUPPORT
> > > > diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> > > > index e7ce729..d3e188f 100644
> > > > --- a/drivers/video/backlight/Makefile
> > > > +++ b/drivers/video/backlight/Makefile
> > > > @@ -45,3 +45,4 @@ obj-$(CONFIG_BACKLIGHT_PCF50633) += pcf50633-backlight.o
> > > > obj-$(CONFIG_BACKLIGHT_AAT2870) += aat2870_bl.o
> > > > obj-$(CONFIG_BACKLIGHT_OT200) += ot200_bl.o
> > > > obj-$(CONFIG_BACKLIGHT_TPS65217) += tps65217_bl.o
> > > > +obj-$(CONFIG_BACKLIGHT_AS3711) += as3711_bl.o
> > > > diff --git a/drivers/video/backlight/as3711_bl.c b/drivers/video/backlight/as3711_bl.c
> > > > new file mode 100644
> > > > index 0000000..c6bc65d
> > > > --- /dev/null
> > > > +++ b/drivers/video/backlight/as3711_bl.c
> > > > @@ -0,0 +1,379 @@
> > > > +/*
> > > > + * AS3711 PMIC backlight driver, using DCDC Step Up Converters
> > > > + *
> > > > + * Copyright (C) 2012 Renesas Electronics Corporation
> > > > + * Author: Guennadi Liakhovetski, <g.liakhovetski@gmx.de>
> > > > + *
> > > > + * This program is free software; you can redistribute it and/or modify
> > > > + * it under the terms of the version 2 of the GNU General Public License as
> > > > + * published by the Free Software Foundation
> > > > + */
> > > > +
> > > > +#include <linux/backlight.h>
> > > > +#include <linux/delay.h>
> > > > +#include <linux/device.h>
> > > > +#include <linux/err.h>
> > > > +#include <linux/fb.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/mfd/as3711.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/regmap.h>
> > > > +#include <linux/slab.h>
> > > > +
> > > > +enum as3711_bl_type {
> > > > + AS3711_BL_SU1,
> > > > + AS3711_BL_SU2,
> > > > +};
> > > > +
> > > > +struct as3711_bl_data {
> > > > + bool powered;
> > > > + const char *fb_name;
> > > > + struct device *fb_dev;
> > > > + enum as3711_bl_type type;
> > > > + int brightness;
> > > > + struct backlight_device *bl;
> > > > +};
> > > > +
> > > > +struct as3711_bl_supply {
> > > > + struct as3711_bl_data su1;
> > > > + struct as3711_bl_data su2;
> > > > + const struct as3711_bl_pdata *pdata;
> > > > + struct as3711 *as3711;
> > > > +};
> > > > +
> > > > +static struct as3711_bl_supply *to_supply(struct as3711_bl_data *su)
> > > > +{
> > > > + switch (su->type) {
> > > > + case AS3711_BL_SU1:
> > > > + return container_of(su, struct as3711_bl_supply, su1);
> > > > + case AS3711_BL_SU2:
> > > > + return container_of(su, struct as3711_bl_supply, su2);
> > > > + }
> > > > + return NULL;
> > > > +}
> > > > +
> > > > +static int as3711_set_brightness_auto_i(struct as3711_bl_data *data,
> > > > + unsigned int brightness)
> > > > +{
> > > > + struct as3711_bl_supply *supply = to_supply(data);
> > > > + struct as3711 *as3711 = supply->as3711;
> > > > + const struct as3711_bl_pdata *pdata = supply->pdata;
> > > > + int ret = 0;
> > > > +
> > > > + /* Only all equal current values are supported */
> > > > + if (pdata->su2_auto_curr1)
> > > > + ret = regmap_write(as3711->regmap, AS3711_CURR1_VALUE,
> > > > + brightness);
> > > > + if (!ret && pdata->su2_auto_curr2)
> > > > + ret = regmap_write(as3711->regmap, AS3711_CURR2_VALUE,
> > > > + brightness);
> > > > + if (!ret && pdata->su2_auto_curr3)
> > > > + ret = regmap_write(as3711->regmap, AS3711_CURR3_VALUE,
> > > > + brightness);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int as3711_set_brightness_v(struct as3711 *as3711,
> > > > + unsigned int brightness,
> > > > + unsigned int reg)
> > > > +{
> > > > + if (brightness > 31)
> > > > + return -EINVAL;
> > > > +
> > > > + return regmap_update_bits(as3711->regmap, reg, 0xf0,
> > > > + brightness << 4);
> > > > +}
> > > > +
> > > > +static int as3711_bl_su2_reset(struct as3711_bl_supply *supply)
> > > > +{
> > > > + struct as3711 *as3711 = supply->as3711;
> > > > + int ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_5,
> > > > + 3, supply->pdata->su2_fbprot);
> > > > + if (!ret)
> > > > + ret = regmap_update_bits(as3711->regmap,
> > > > + AS3711_STEPUP_CONTROL_2, 1, 0);
> > > > + if (!ret)
> > > > + ret = regmap_update_bits(as3711->regmap,
> > > > + AS3711_STEPUP_CONTROL_2, 1, 1);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Someone with less fragile or less expensive hardware could try to simplify
> > > > + * the brightness adjustment procedure.
> > > > + */
> > > > +static int as3711_bl_update_status(struct backlight_device *bl)
> > > > +{
> > > > + struct as3711_bl_data *data = bl_get_data(bl);
> > > > + struct as3711_bl_supply *supply = to_supply(data);
> > > > + struct as3711 *as3711 = supply->as3711;
> > > > + int brightness = bl->props.brightness;
> > > > + int ret = 0;
> > > > +
> > > > + dev_dbg(&bl->dev, "%s(): brightness %u, pwr %x, blank %x, state %x\n",
> > > > + __func__, bl->props.brightness, bl->props.power,
> > > > + bl->props.fb_blank, bl->props.state);
> > > > +
> > > > + if (bl->props.power != FB_BLANK_UNBLANK ||
> > > > + bl->props.fb_blank != FB_BLANK_UNBLANK ||
> > > > + bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
> > > > + brightness = 0;
> > > > +
> > > > + if (data->type = AS3711_BL_SU1) {
> > > > + ret = as3711_set_brightness_v(as3711, brightness,
> > > > + AS3711_STEPUP_CONTROL_1);
> > > > + } else {
> > > > + const struct as3711_bl_pdata *pdata = supply->pdata;
> > > > +
> > > > + switch (pdata->su2_feedback) {
> > > > + case AS3711_SU2_VOLTAGE:
> > > > + ret = as3711_set_brightness_v(as3711, brightness,
> > > > + AS3711_STEPUP_CONTROL_2);
> > > > + break;
> > > > + case AS3711_SU2_CURR_AUTO:
> > > > + ret = as3711_set_brightness_auto_i(data, brightness / 4);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > + if (brightness) {
> > > > + ret = as3711_bl_su2_reset(supply);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > + udelay(500);
> > > > + ret = as3711_set_brightness_auto_i(data, brightness);
> > > > + } else {
> > > > + ret = regmap_update_bits(as3711->regmap,
> > > > + AS3711_STEPUP_CONTROL_2, 1, 0);
> > > > + }
> > > > + break;
> > > > + /* Manual one current feedback pin below */
> > > > + case AS3711_SU2_CURR1:
> > > > + ret = regmap_write(as3711->regmap, AS3711_CURR1_VALUE,
> > > > + brightness);
> > > > + break;
> > > > + case AS3711_SU2_CURR2:
> > > > + ret = regmap_write(as3711->regmap, AS3711_CURR2_VALUE,
> > > > + brightness);
> > > > + break;
> > > > + case AS3711_SU2_CURR3:
> > > > + ret = regmap_write(as3711->regmap, AS3711_CURR3_VALUE,
> > > > + brightness);
> > > > + break;
> > > > + default:
> > > > + ret = -EINVAL;
> > > > + }
> > > > + }
> > > > + if (!ret)
> > > > + data->brightness = brightness;
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int as3711_bl_get_brightness(struct backlight_device *bl)
> > > > +{
> > > > + struct as3711_bl_data *data = bl_get_data(bl);
> > > > +
> > > > + return data->brightness;
> > > > +}
> > > > +
> > > > +static const struct backlight_ops as3711_bl_ops = {
> > > > + .update_status = as3711_bl_update_status,
> > > > + .get_brightness = as3711_bl_get_brightness,
> > > > +};
> > > > +
> > > > +static int as3711_bl_init_su2(struct as3711_bl_supply *supply)
> > > > +{
> > > > + struct as3711 *as3711 = supply->as3711;
> > > > + const struct as3711_bl_pdata *pdata = supply->pdata;
> > > > + u8 ctl = 0;
> > > > + int ret;
> > > > +
> > > > + dev_dbg(as3711->dev, "%s(): use %u\n", __func__, pdata->su2_feedback);
> > > > +
> > > > + /* Turn SU2 off */
> > > > + ret = regmap_write(as3711->regmap, AS3711_STEPUP_CONTROL_2, 0);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + switch (pdata->su2_feedback) {
> > > > + case AS3711_SU2_VOLTAGE:
> > > > + ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 0);
> > > > + break;
> > > > + case AS3711_SU2_CURR1:
> > > > + ctl = 1;
> > > > + ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 1);
> > > > + break;
> > > > + case AS3711_SU2_CURR2:
> > > > + ctl = 4;
> > > > + ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 2);
> > > > + break;
> > > > + case AS3711_SU2_CURR3:
> > > > + ctl = 0x10;
> > > > + ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 3);
> > > > + break;
> > > > + case AS3711_SU2_CURR_AUTO:
> > > > + if (pdata->su2_auto_curr1)
> > > > + ctl = 2;
> > > > + if (pdata->su2_auto_curr2)
> > > > + ctl |= 8;
> > > > + if (pdata->su2_auto_curr3)
> > > > + ctl |= 0x20;
> > > > + ret = 0;
> > > > + break;
> > > > + default:
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + if (!ret)
> > > > + ret = regmap_write(as3711->regmap, AS3711_CURR_CONTROL, ctl);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int as3711_bl_register(struct platform_device *pdev,
> > > > + unsigned int max_brightness, struct as3711_bl_data *su)
> > > > +{
> > > > + struct backlight_properties props = {.type = BACKLIGHT_RAW,};
> > > > + struct backlight_device *bl;
> > > > +
> > > > + /* max tuning I = 31uA for voltage- and 38250uA for current-feedback */
> > > > + props.max_brightness = max_brightness;
> > > > +
> > > > + bl = backlight_device_register(su->type = AS3711_BL_SU1 ?
> > > > + "as3711-su1" : "as3711-su2",
> > > > + &pdev->dev, su,
> > > > + &as3711_bl_ops, &props);
> > > > + if (IS_ERR(bl)) {
> > > > + dev_err(&pdev->dev, "failed to register backlight\n");
> > > > + return PTR_ERR(bl);
> > > > + }
> > > > +
> > > > + bl->props.brightness = props.max_brightness;
> > > > +
> > > > + backlight_update_status(bl);
> > > > +
> > > > + su->bl = bl;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int as3711_backlight_probe(struct platform_device *pdev)
> > > > +{
> > > > + struct as3711_bl_pdata *pdata = dev_get_platdata(&pdev->dev);
> > > > + struct as3711 *as3711 = dev_get_drvdata(pdev->dev.parent);
> > > > + struct as3711_bl_supply *supply;
> > > > + struct as3711_bl_data *su;
> > > > + unsigned int max_brightness;
> > > > + int ret;
> > > > +
> > > > + if (!pdata || (!pdata->su1_fb && !pdata->su2_fb)) {
> > > > + dev_err(&pdev->dev, "No platform data, exiting...\n");
> > > > + return -ENODEV;
> > > > + }
> > > > +
> > > > + /*
> > > > + * Due to possible hardware damage I chose to block all modes,
> > > > + * unsupported on my hardware. Anyone, wishing to use any of those modes
> > > > + * will have to first review the code, then activate and test it.
> > > > + */
> > > > + if (pdata->su1_fb ||
> > > > + pdata->su2_fbprot != AS3711_SU2_GPIO4 ||
> > > > + pdata->su2_feedback != AS3711_SU2_CURR_AUTO) {
> > > > + dev_warn(&pdev->dev,
> > > > + "Attention! An untested mode has been chosen!\n"
> > > > + "Please, review the code, enable, test, and report success:-)\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + supply = devm_kzalloc(&pdev->dev, sizeof(*supply), GFP_KERNEL);
> > > > + if (!supply)
> > > > + return -ENOMEM;
> > > > +
> > > > + supply->as3711 = as3711;
> > > > + supply->pdata = pdata;
> > > > +
> > > > + if (pdata->su1_fb) {
> > > > + su = &supply->su1;
> > > > + su->fb_name = pdata->su1_fb;
> > > > + su->type = AS3711_BL_SU1;
> > > > +
> > > > + max_brightness = min(pdata->su1_max_uA, 31);
> > > > + ret = as3711_bl_register(pdev, max_brightness, su);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > + }
> > > > +
> > > > + if (pdata->su2_fb) {
> > > > + su = &supply->su2;
> > > > + su->fb_name = pdata->su2_fb;
> > > > + su->type = AS3711_BL_SU2;
> > > > +
> > > > + switch (pdata->su2_fbprot) {
> > > > + case AS3711_SU2_GPIO2:
> > > > + case AS3711_SU2_GPIO3:
> > > > + case AS3711_SU2_GPIO4:
> > > > + case AS3711_SU2_LX_SD4:
> > > > + break;
> > > > + default:
> > > > + ret = -EINVAL;
> > > > + goto esu2;
> > > > + }
> > > > +
> > > > + switch (pdata->su2_feedback) {
> > > > + case AS3711_SU2_VOLTAGE:
> > > > + max_brightness = min(pdata->su2_max_uA, 31);
> > > > + break;
> > > > + case AS3711_SU2_CURR1:
> > > > + case AS3711_SU2_CURR2:
> > > > + case AS3711_SU2_CURR3:
> > > > + case AS3711_SU2_CURR_AUTO:
> > > > + max_brightness = min(pdata->su2_max_uA / 150, 255);
> > > > + break;
> > > > + default:
> > > > + ret = -EINVAL;
> > > > + goto esu2;
> > > > + }
> > > > +
> > > > + ret = as3711_bl_init_su2(supply);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + ret = as3711_bl_register(pdev, max_brightness, su);
> > > > + if (ret < 0)
> > > > + goto esu2;
> > > > + }
> > > > +
> > > > + platform_set_drvdata(pdev, supply);
> > > > +
> > > > + return 0;
> > > > +
> > > > +esu2:
> > > > + backlight_device_unregister(supply->su1.bl);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int as3711_backlight_remove(struct platform_device *pdev)
> > > > +{
> > > > + struct as3711_bl_supply *supply = platform_get_drvdata(pdev);
> > > > +
> > > > + backlight_device_unregister(supply->su1.bl);
> > > > + backlight_device_unregister(supply->su2.bl);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static struct platform_driver as3711_backlight_driver = {
> > > > + .driver = {
> > > > + .name = "as3711-backlight",
> > > > + .owner = THIS_MODULE,
> > > > + },
> > > > + .probe = as3711_backlight_probe,
> > > > + .remove = as3711_backlight_remove,
> > > > +};
> > > > +
> > > > +module_platform_driver(as3711_backlight_driver);
> > > > +
> > > > +MODULE_DESCRIPTION("Backlight Driver for AS3711 PMICs");
> > > > +MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@gmx.de");
> > > > +MODULE_LICENSE("GPL");
> > > > +MODULE_ALIAS("platform:as3711-backlight");
> > > > --
> > > > 1.7.2.5
> > > >
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > >
> >
> > ---
> > Guennadi Liakhovetski, Ph.D.
> > Freelance Open-Source Software Developer
> > http://www.open-technology.de/
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-02-05 9:08 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-01-08 17:54 [PATCH] backlight: add an AS3711 PMIC backlight driver Guennadi Liakhovetski
2013-01-25 5:20 ` Jingoo Han
2013-01-25 7:48 ` Guennadi Liakhovetski
2013-01-28 0:53 ` Jingoo Han
2013-02-05 9:08 ` Guennadi Liakhovetski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).