* [PATCH v2 2/2] backlight: add new lp8788 backlight driver
@ 2012-12-21 7:55 Kim, Milo
2012-12-21 22:28 ` Andrew Morton
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Kim, Milo @ 2012-12-21 7:55 UTC (permalink / raw)
To: Richard Purdie
Cc: Andrew Morton, linux-kernel@vger.kernel.org, Samuel Ortiz,
Thierry Reding
TI LP8788 PMU supports regulators, battery charger, RTC, ADC, backlight driver
and current sinks.
This patch enables LP8788 backlight module.
(Brightness mode)
The brightness is controlled by PWM input or I2C register.
All modes are supported in the driver.
(Platform data)
Configurable data can be defined in the platform side.
name : backlight driver name. (default: "lcd-backlight")
initial_brightness : initial value of backlight brightness
bl_mode : brightness control by PWM or lp8788 register
dim_mode : dimming mode selection
full_scale : full scale current setting
rise_time : brightness ramp up step time
fall_time : brightness ramp down step time
pwm_pol : PWM polarity setting when bl_mode is PWM based
period_ns : platform specific PWM period value. unit is nano.
The default values are set in case no platform data is defined.
Patch v2.
use generic PWM functions rather than lp8788 platform data function calls.
add configurable PWM period in the platform data.
replace module_init() and module_exit() with module_platform_driver().
Patch v1.
initial patch
Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
---
drivers/video/backlight/Kconfig | 6 +
drivers/video/backlight/Makefile | 1 +
drivers/video/backlight/lp8788_bl.c | 350 +++++++++++++++++++++++++++++++++++
include/linux/mfd/lp8788.h | 16 +-
4 files changed, 360 insertions(+), 13 deletions(-)
create mode 100644 drivers/video/backlight/lp8788_bl.c
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 765a945..455646a 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -369,6 +369,12 @@ config BACKLIGHT_LP855X
This supports TI LP8550, LP8551, LP8552, LP8553 and LP8556
backlight driver.
+config BACKLIGHT_LP8788
+ tristate "Backlight driver for TI LP8788 MFD"
+ depends on BACKLIGHT_CLASS_DEVICE && MFD_LP8788
+ help
+ This supports TI LP8788 backlight driver.
+
config BACKLIGHT_OT200
tristate "Backlight driver for ot200 visualisation device"
depends on BACKLIGHT_CLASS_DEVICE && CS5535_MFGPT && GPIO_CS5535
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index e7ce729..4d125ce 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_BACKLIGHT_LOCOMO) += locomolcd.o
obj-$(CONFIG_BACKLIGHT_LM3630) += lm3630_bl.o
obj-$(CONFIG_BACKLIGHT_LM3639) += lm3639_bl.o
obj-$(CONFIG_BACKLIGHT_LP855X) += lp855x_bl.o
+obj-$(CONFIG_BACKLIGHT_LP8788) += lp8788_bl.o
obj-$(CONFIG_BACKLIGHT_OMAP1) += omap1_bl.o
obj-$(CONFIG_BACKLIGHT_PANDORA) += pandora_bl.o
obj-$(CONFIG_BACKLIGHT_CARILLO_RANCH) += cr_bllcd.o
diff --git a/drivers/video/backlight/lp8788_bl.c b/drivers/video/backlight/lp8788_bl.c
new file mode 100644
index 0000000..fbf17bc
--- /dev/null
+++ b/drivers/video/backlight/lp8788_bl.c
@@ -0,0 +1,350 @@
+/*
+ * TI LP8788 MFD - backlight driver
+ *
+ * Copyright 2012 Texas Instruments
+ *
+ * Author: Milo(Woogyom) Kim <milo.kim@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/backlight.h>
+#include <linux/err.h>
+#include <linux/mfd/lp8788.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+
+/* register address */
+#define LP8788_BL_CONFIG 0x96
+#define LP8788_BL_BRIGHTNESS 0x97
+#define LP8788_BL_RAMP 0x98
+
+/* mask/shift bits */
+#define LP8788_BL_EN BIT(0) /* Addr 96h */
+#define LP8788_BL_PWM_EN BIT(5)
+#define LP8788_BL_FULLSCALE_S 2
+#define LP8788_BL_DIM_MODE_S 1
+#define LP8788_BL_PWM_POLARITY_S 6
+#define LP8788_BL_RAMP_RISE_S 4 /* Addr 98h */
+
+#define BUF_SIZE 20
+#define MAX_BRIGHTNESS 127
+#define DEFAULT_BL_NAME "lcd-backlight"
+
+struct lp8788_bl_config {
+ enum lp8788_bl_ctrl_mode bl_mode;
+ enum lp8788_bl_dim_mode dim_mode;
+ enum lp8788_bl_full_scale_current full_scale;
+ enum lp8788_bl_ramp_step rise_time;
+ enum lp8788_bl_ramp_step fall_time;
+ enum lp8788_bl_pwm_polarity pwm_pol;
+};
+
+struct lp8788_bl {
+ struct lp8788 *lp;
+ struct backlight_device *bl_dev;
+ struct lp8788_backlight_platform_data *pdata;
+ enum lp8788_bl_ctrl_mode mode;
+ struct pwm_device *pwm;
+};
+
+struct lp8788_bl_config default_bl_config = {
+ .bl_mode = LP8788_BL_REGISTER_ONLY,
+ .dim_mode = LP8788_DIM_EXPONENTIAL,
+ .full_scale = LP8788_FULLSCALE_1900uA,
+ .rise_time = LP8788_RAMP_8192us,
+ .fall_time = LP8788_RAMP_8192us,
+ .pwm_pol = LP8788_PWM_ACTIVE_HIGH,
+};
+
+static inline bool is_brightness_ctrl_by_pwm(enum lp8788_bl_ctrl_mode mode)
+{
+ return (mode == LP8788_BL_COMB_PWM_BASED);
+}
+
+static inline bool is_brightness_ctrl_by_register(enum lp8788_bl_ctrl_mode mode)
+{
+ return (mode == LP8788_BL_REGISTER_ONLY ||
+ mode == LP8788_BL_COMB_REGISTER_BASED);
+}
+
+static int lp8788_backlight_configure(struct lp8788_bl *bl)
+{
+ struct lp8788_backlight_platform_data *pdata = bl->pdata;
+ struct lp8788_bl_config *cfg = &default_bl_config;
+ int ret;
+ u8 val;
+
+ /* update chip configuration if platform data exists,
+ otherwise use the default settings */
+ if (pdata) {
+ cfg->bl_mode = pdata->bl_mode;
+ cfg->dim_mode = pdata->dim_mode;
+ cfg->full_scale = pdata->full_scale;
+ cfg->rise_time = pdata->rise_time;
+ cfg->fall_time = pdata->fall_time;
+ cfg->pwm_pol = pdata->pwm_pol;
+ }
+
+ /* brightness ramp up/down */
+ val = (cfg->rise_time << LP8788_BL_RAMP_RISE_S) | cfg->fall_time;
+ ret = lp8788_write_byte(bl->lp, LP8788_BL_RAMP, val);
+ if (ret) {
+ /* no I2C needed in case of pwm control mode */
+ if (is_brightness_ctrl_by_pwm(cfg->bl_mode))
+ goto no_err;
+ else
+ return ret;
+ }
+
+ /* fullscale current setting */
+ val = (cfg->full_scale << LP8788_BL_FULLSCALE_S) |
+ (cfg->dim_mode << LP8788_BL_DIM_MODE_S);
+
+ /* brightness control mode */
+ switch (cfg->bl_mode) {
+ case LP8788_BL_REGISTER_ONLY:
+ val |= LP8788_BL_EN;
+ break;
+ case LP8788_BL_COMB_PWM_BASED:
+ case LP8788_BL_COMB_REGISTER_BASED:
+ val |= LP8788_BL_EN | LP8788_BL_PWM_EN |
+ (cfg->pwm_pol << LP8788_BL_PWM_POLARITY_S);
+ break;
+ default:
+ dev_err(bl->lp->dev, "invalid mode: %d\n", cfg->bl_mode);
+ return -EINVAL;
+ }
+
+ bl->mode = cfg->bl_mode;
+
+ return lp8788_write_byte(bl->lp, LP8788_BL_CONFIG, val);
+
+no_err:
+ return 0;
+}
+
+static void lp8788_pwm_ctrl(struct lp8788_bl *bl, int br, int max_br)
+{
+ unsigned int period;
+ unsigned int duty;
+ struct device *dev;
+ struct pwm_device *pwm;
+
+ if (!bl->pdata)
+ return;
+
+ period = bl->pdata->period_ns;
+ duty = br * period / max_br;
+ dev = bl->lp->dev;
+
+ /* request pwm device with the consumer name */
+ if (!bl->pwm) {
+ pwm = devm_pwm_get(dev, LP8788_DEV_BACKLIGHT);
+ if (IS_ERR(pwm)) {
+ dev_err(dev, "can not get pwm device\n");
+ return;
+ }
+
+ bl->pwm = pwm;
+ }
+
+ pwm_config(bl->pwm, duty, period);
+ if (duty)
+ pwm_enable(bl->pwm);
+ else
+ pwm_disable(bl->pwm);
+}
+
+static int lp8788_bl_update_status(struct backlight_device *bl_dev)
+{
+ struct lp8788_bl *bl = bl_get_data(bl_dev);
+ enum lp8788_bl_ctrl_mode mode = bl->mode;
+
+ if (bl_dev->props.state & BL_CORE_SUSPENDED)
+ bl_dev->props.brightness = 0;
+
+ if (is_brightness_ctrl_by_pwm(mode)) {
+ int brt = bl_dev->props.brightness;
+ int max = bl_dev->props.max_brightness;
+
+ lp8788_pwm_ctrl(bl, brt, max);
+ } else if (is_brightness_ctrl_by_register(mode)) {
+ u8 brt = bl_dev->props.brightness;
+
+ lp8788_write_byte(bl->lp, LP8788_BL_BRIGHTNESS, brt);
+ }
+
+ return 0;
+}
+
+static int lp8788_bl_get_brightness(struct backlight_device *bl_dev)
+{
+ struct lp8788_bl *bl = bl_get_data(bl_dev);
+ enum lp8788_bl_ctrl_mode mode = bl->mode;
+
+ if (is_brightness_ctrl_by_register(mode)) {
+ u8 val = 0;
+
+ lp8788_read_byte(bl->lp, LP8788_BL_BRIGHTNESS, &val);
+ bl_dev->props.brightness = val;
+ }
+
+ return bl_dev->props.brightness;
+}
+
+static const struct backlight_ops lp8788_bl_ops = {
+ .options = BL_CORE_SUSPENDRESUME,
+ .update_status = lp8788_bl_update_status,
+ .get_brightness = lp8788_bl_get_brightness,
+};
+
+static int lp8788_backlight_register(struct lp8788_bl *bl)
+{
+ struct backlight_device *bl_dev;
+ struct backlight_properties props;
+ struct lp8788_backlight_platform_data *pdata = bl->pdata;
+ int init_brt;
+ char *name;
+
+ props.type = BACKLIGHT_PLATFORM;
+ props.max_brightness = MAX_BRIGHTNESS;
+
+ /* initial brightness */
+ if (pdata)
+ init_brt = min_t(int, pdata->initial_brightness,
+ props.max_brightness);
+ else
+ init_brt = 0;
+
+ props.brightness = init_brt;
+
+ /* backlight device name */
+ if (!pdata || !pdata->name)
+ name = DEFAULT_BL_NAME;
+ else
+ name = pdata->name;
+
+ bl_dev = backlight_device_register(name, bl->lp->dev, bl,
+ &lp8788_bl_ops, &props);
+ if (IS_ERR(bl_dev))
+ return PTR_ERR(bl_dev);
+
+ bl->bl_dev = bl_dev;
+
+ return 0;
+}
+
+static void lp8788_backlight_unregister(struct lp8788_bl *bl)
+{
+ struct backlight_device *bl_dev = bl->bl_dev;
+
+ if (bl_dev)
+ backlight_device_unregister(bl_dev);
+}
+
+static ssize_t lp8788_get_bl_ctl_mode(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct lp8788_bl *bl = dev_get_drvdata(dev);
+ enum lp8788_bl_ctrl_mode mode = bl->mode;
+ char *strmode;
+
+ if (is_brightness_ctrl_by_pwm(mode))
+ strmode = "pwm based";
+ else if (is_brightness_ctrl_by_register(mode))
+ strmode = "register based";
+ else
+ strmode = "invalid mode";
+
+ return scnprintf(buf, BUF_SIZE, "%s\n", strmode);
+}
+
+static DEVICE_ATTR(bl_ctl_mode, S_IRUGO, lp8788_get_bl_ctl_mode, NULL);
+
+static struct attribute *lp8788_attributes[] = {
+ &dev_attr_bl_ctl_mode.attr,
+ NULL,
+};
+
+static const struct attribute_group lp8788_attr_group = {
+ .attrs = lp8788_attributes,
+};
+
+static int lp8788_backlight_probe(struct platform_device *pdev)
+{
+ struct lp8788 *lp = dev_get_drvdata(pdev->dev.parent);
+ struct lp8788_bl *bl;
+ int ret;
+
+ bl = devm_kzalloc(lp->dev, sizeof(struct lp8788_bl), GFP_KERNEL);
+ if (!bl)
+ return -ENOMEM;
+
+ bl->lp = lp;
+ if (lp->pdata)
+ bl->pdata = lp->pdata->bl_pdata;
+
+ platform_set_drvdata(pdev, bl);
+
+ ret = lp8788_backlight_configure(bl);
+ if (ret) {
+ dev_err(lp->dev, "backlight config err: %d\n", ret);
+ goto err_dev;
+ }
+
+ ret = lp8788_backlight_register(bl);
+ if (ret) {
+ dev_err(lp->dev, "register backlight err: %d\n", ret);
+ goto err_dev;
+ }
+
+ ret = sysfs_create_group(&pdev->dev.kobj, &lp8788_attr_group);
+ if (ret) {
+ dev_err(lp->dev, "register sysfs err: %d\n", ret);
+ goto err_sysfs;
+ }
+
+ backlight_update_status(bl->bl_dev);
+
+ return 0;
+
+err_sysfs:
+ lp8788_backlight_unregister(bl);
+err_dev:
+ return ret;
+}
+
+static int lp8788_backlight_remove(struct platform_device *pdev)
+{
+ struct lp8788_bl *bl = platform_get_drvdata(pdev);
+ struct backlight_device *bl_dev = bl->bl_dev;
+
+ bl_dev->props.brightness = 0;
+ backlight_update_status(bl_dev);
+ sysfs_remove_group(&pdev->dev.kobj, &lp8788_attr_group);
+ lp8788_backlight_unregister(bl);
+ platform_set_drvdata(pdev, NULL);
+
+ return 0;
+}
+
+static struct platform_driver lp8788_bl_driver = {
+ .probe = lp8788_backlight_probe,
+ .remove = lp8788_backlight_remove,
+ .driver = {
+ .name = LP8788_DEV_BACKLIGHT,
+ .owner = THIS_MODULE,
+ },
+};
+module_platform_driver(lp8788_bl_driver);
+
+MODULE_DESCRIPTION("Texas Instruments LP8788 Backlight Driver");
+MODULE_AUTHOR("Milo Kim");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:lp8788-backlight");
diff --git a/include/linux/mfd/lp8788.h b/include/linux/mfd/lp8788.h
index 2a32b16..5c0a49b 100644
--- a/include/linux/mfd/lp8788.h
+++ b/include/linux/mfd/lp8788.h
@@ -229,16 +229,6 @@ struct lp8788_charger_platform_data {
};
/*
- * struct lp8788_bl_pwm_data
- * @pwm_set_intensity : set duty of pwm
- * @pwm_get_intensity : get current duty of pwm
- */
-struct lp8788_bl_pwm_data {
- void (*pwm_set_intensity) (int brightness, int max_brightness);
- int (*pwm_get_intensity) (int max_brightness);
-};
-
-/*
* struct lp8788_backlight_platform_data
* @name : backlight driver name. (default: "lcd-backlight")
* @initial_brightness : initial value of backlight brightness
@@ -248,8 +238,8 @@ struct lp8788_bl_pwm_data {
* @rise_time : brightness ramp up step time
* @fall_time : brightness ramp down step time
* @pwm_pol : pwm polarity setting when bl_mode is pwm based
- * @pwm_data : platform specific pwm generation functions
- * only valid when bl_mode is pwm based
+ * @period_ns : platform specific pwm period value. unit is nano.
+ Only valid when bl_mode is LP8788_BL_COMB_PWM_BASED
*/
struct lp8788_backlight_platform_data {
char *name;
@@ -260,7 +250,7 @@ struct lp8788_backlight_platform_data {
enum lp8788_bl_ramp_step rise_time;
enum lp8788_bl_ramp_step fall_time;
enum lp8788_bl_pwm_polarity pwm_pol;
- struct lp8788_bl_pwm_data pwm_data;
+ unsigned int period_ns;
};
/*
--
1.7.9.5
Best Regards,
Milo
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] backlight: add new lp8788 backlight driver
2012-12-21 7:55 [PATCH v2 2/2] backlight: add new lp8788 backlight driver Kim, Milo
@ 2012-12-21 22:28 ` Andrew Morton
2013-01-03 8:08 ` Thierry Reding
2013-01-22 0:32 ` Samuel Ortiz
2 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2012-12-21 22:28 UTC (permalink / raw)
To: Kim, Milo
Cc: Richard Purdie, linux-kernel@vger.kernel.org, Samuel Ortiz,
Thierry Reding
On Fri, 21 Dec 2012 07:55:23 +0000
"Kim, Milo" <Milo.Kim@ti.com> wrote:
> TI LP8788 PMU supports regulators, battery charger, RTC, ADC, backlight driver
> and current sinks.
> This patch enables LP8788 backlight module.
>
> (Brightness mode)
> The brightness is controlled by PWM input or I2C register.
> All modes are supported in the driver.
>
> (Platform data)
> Configurable data can be defined in the platform side.
> name : backlight driver name. (default: "lcd-backlight")
> initial_brightness : initial value of backlight brightness
> bl_mode : brightness control by PWM or lp8788 register
> dim_mode : dimming mode selection
> full_scale : full scale current setting
> rise_time : brightness ramp up step time
> fall_time : brightness ramp down step time
> pwm_pol : PWM polarity setting when bl_mode is PWM based
> period_ns : platform specific PWM period value. unit is nano.
There's also
DEVICE_ATTR(bl_ctl_mode, S_IRUGO, lp8788_get_bl_ctl_mode, NULL);
which isn't documented anywhere?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] backlight: add new lp8788 backlight driver
2012-12-21 7:55 [PATCH v2 2/2] backlight: add new lp8788 backlight driver Kim, Milo
2012-12-21 22:28 ` Andrew Morton
@ 2013-01-03 8:08 ` Thierry Reding
2013-01-03 8:54 ` Kim, Milo
2013-01-22 0:32 ` Samuel Ortiz
2 siblings, 1 reply; 6+ messages in thread
From: Thierry Reding @ 2013-01-03 8:08 UTC (permalink / raw)
To: Kim, Milo
Cc: Richard Purdie, Andrew Morton, linux-kernel@vger.kernel.org,
Samuel Ortiz
[-- Attachment #1: Type: text/plain, Size: 5210 bytes --]
On Fri, Dec 21, 2012 at 07:55:23AM +0000, Kim, Milo wrote:
> TI LP8788 PMU supports regulators, battery charger, RTC, ADC, backlight driver
> and current sinks.
> This patch enables LP8788 backlight module.
>
> (Brightness mode)
> The brightness is controlled by PWM input or I2C register.
> All modes are supported in the driver.
>
> (Platform data)
> Configurable data can be defined in the platform side.
> name : backlight driver name. (default: "lcd-backlight")
> initial_brightness : initial value of backlight brightness
> bl_mode : brightness control by PWM or lp8788 register
> dim_mode : dimming mode selection
> full_scale : full scale current setting
> rise_time : brightness ramp up step time
> fall_time : brightness ramp down step time
> pwm_pol : PWM polarity setting when bl_mode is PWM based
You might want to consider using enum pwm_polarity from linux/pwm.h
instead and convert to the driver representation internally.
I'm saying this because I'm thinking about extending the PWM framework
to allow PWM polarity and period to be specified in the PWM lookup table
so that they can be treated transparently, independent of whether they
are obtained from DT or the lookup table.
That would allow the polarity and period to be retrieved with accessors
like pwm_get_polarity() and pwm_get_period().
> period_ns : platform specific PWM period value. unit is nano.
If the period can be encoded in the PWM lookup table this can also go
away. But for now it's fine to keep it as the changes aren't in place
yet.
> diff --git a/drivers/video/backlight/lp8788_bl.c b/drivers/video/backlight/lp8788_bl.c
[...]
> +/* register address */
> +#define LP8788_BL_CONFIG 0x96
> +#define LP8788_BL_BRIGHTNESS 0x97
> +#define LP8788_BL_RAMP 0x98
> +
> +/* mask/shift bits */
> +#define LP8788_BL_EN BIT(0) /* Addr 96h */
> +#define LP8788_BL_PWM_EN BIT(5)
> +#define LP8788_BL_FULLSCALE_S 2
> +#define LP8788_BL_DIM_MODE_S 1
> +#define LP8788_BL_PWM_POLARITY_S 6
> +#define LP8788_BL_RAMP_RISE_S 4 /* Addr 98h */
The ordering of the defines confuses me. I know you've put comments in
place to explain which registers the individual bits/fields belong to,
but I think it would be much clearer if the field definitions were to
immediately follow the register addresses:
#define LP8788_BL_CONFIG 0x96
#define LP8788_BL_EN BIT(0)
#define LP8788_BL_PWM_EN BIT(5)
...
#define LP8788_BL_RAMP 0x98
#define LP8788_BL_RAMP_RISE_S 4
While at it, maybe change the _S suffix to _SHIFT, which seems to be
more commonly used. Also the PWM_EN bit has an unfortunate name. It
doesn't enable any PWM but, judging by the code, rather configures the
hardware to use a PWM input to control brightness. Maybe something like
USE_PWM would be more accurate. Or is the name taken from the datasheet?
In that case there might be some value in keeping it.
> +static inline bool is_brightness_ctrl_by_pwm(enum lp8788_bl_ctrl_mode mode)
> +{
> + return (mode == LP8788_BL_COMB_PWM_BASED);
> +}
> +
> +static inline bool is_brightness_ctrl_by_register(enum lp8788_bl_ctrl_mode mode)
> +{
> + return (mode == LP8788_BL_REGISTER_ONLY ||
> + mode == LP8788_BL_COMB_REGISTER_BASED);
> +}
> +
> +static int lp8788_backlight_configure(struct lp8788_bl *bl)
> +{
> + struct lp8788_backlight_platform_data *pdata = bl->pdata;
> + struct lp8788_bl_config *cfg = &default_bl_config;
> + int ret;
> + u8 val;
> +
> + /* update chip configuration if platform data exists,
> + otherwise use the default settings */
CodingStyle says this should be:
/*
* Update chip configuration if platform data exists,
* otherwise use the default settings.
*/
> + /* brightness ramp up/down */
> + val = (cfg->rise_time << LP8788_BL_RAMP_RISE_S) | cfg->fall_time;
> + ret = lp8788_write_byte(bl->lp, LP8788_BL_RAMP, val);
> + if (ret) {
> + /* no I2C needed in case of pwm control mode */
> + if (is_brightness_ctrl_by_pwm(cfg->bl_mode))
> + goto no_err;
> + else
> + return ret;
> + }
In that case, shouldn't you rather move the is_brightness_ctrl_by_pwm()
check up and only write the RAMP register if required? If RAMP doesn't
have any effect in PWM control mode, then you don't need to write it.
And I'd prefer if PWM was consistently spelled with all capitals in
text. There are a few other occurrences in the rest of the patch.
> +static ssize_t lp8788_get_bl_ctl_mode(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct lp8788_bl *bl = dev_get_drvdata(dev);
> + enum lp8788_bl_ctrl_mode mode = bl->mode;
> + char *strmode;
> +
> + if (is_brightness_ctrl_by_pwm(mode))
> + strmode = "pwm based";
> + else if (is_brightness_ctrl_by_register(mode))
> + strmode = "register based";
> + else
> + strmode = "invalid mode";
> +
> + return scnprintf(buf, BUF_SIZE, "%s\n", strmode);
> +}
According to Documentation/filesystems/sysfs.txt, buf is always a whole
page, so you can use PAGE_SIZE instead of BUF_SIZE.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v2 2/2] backlight: add new lp8788 backlight driver
2013-01-03 8:08 ` Thierry Reding
@ 2013-01-03 8:54 ` Kim, Milo
2013-01-03 9:05 ` Thierry Reding
0 siblings, 1 reply; 6+ messages in thread
From: Kim, Milo @ 2013-01-03 8:54 UTC (permalink / raw)
To: Thierry Reding
Cc: Richard Purdie, Andrew Morton, linux-kernel@vger.kernel.org,
Samuel Ortiz
Hi Thierry,
> > (Platform data)
> > Configurable data can be defined in the platform side.
> > name : backlight driver name. (default: "lcd-
> backlight")
> > initial_brightness : initial value of backlight brightness
> > bl_mode : brightness control by PWM or lp8788 register
> > dim_mode : dimming mode selection
> > full_scale : full scale current setting
> > rise_time : brightness ramp up step time
> > fall_time : brightness ramp down step time
> > pwm_pol : PWM polarity setting when bl_mode is PWM
> based
>
> You might want to consider using enum pwm_polarity from linux/pwm.h
> instead and convert to the driver representation internally.
>
> I'm saying this because I'm thinking about extending the PWM framework
> to allow PWM polarity and period to be specified in the PWM lookup
> table
> so that they can be treated transparently, independent of whether they
> are obtained from DT or the lookup table.
>
> That would allow the polarity and period to be retrieved with accessors
> like pwm_get_polarity() and pwm_get_period().
OK, pwm_pol will be replaced with pwm_get_polarity() in the next patch.
>
> > period_ns : platform specific PWM period value. unit is
> nano.
>
> If the period can be encoded in the PWM lookup table this can also go
> away. But for now it's fine to keep it as the changes aren't in place
> yet.
I'd remain it as configurable platform data.
>
> > diff --git a/drivers/video/backlight/lp8788_bl.c
> b/drivers/video/backlight/lp8788_bl.c
> [...]
> > +/* register address */
> > +#define LP8788_BL_CONFIG 0x96
> > +#define LP8788_BL_BRIGHTNESS 0x97
> > +#define LP8788_BL_RAMP 0x98
> > +
> > +/* mask/shift bits */
> > +#define LP8788_BL_EN BIT(0) /* Addr 96h */
> > +#define LP8788_BL_PWM_EN BIT(5)
> > +#define LP8788_BL_FULLSCALE_S 2
> > +#define LP8788_BL_DIM_MODE_S 1
> > +#define LP8788_BL_PWM_POLARITY_S 6
> > +#define LP8788_BL_RAMP_RISE_S 4 /* Addr 98h */
>
> The ordering of the defines confuses me. I know you've put comments in
> place to explain which registers the individual bits/fields belong to,
> but I think it would be much clearer if the field definitions were to
> immediately follow the register addresses:
>
> #define LP8788_BL_CONFIG 0x96
> #define LP8788_BL_EN BIT(0)
> #define LP8788_BL_PWM_EN BIT(5)
> ...
> #define LP8788_BL_RAMP 0x98
> #define LP8788_BL_RAMP_RISE_S 4
>
> While at it, maybe change the _S suffix to _SHIFT, which seems to be
> more commonly used. Also the PWM_EN bit has an unfortunate name. It
> doesn't enable any PWM but, judging by the code, rather configures the
> hardware to use a PWM input to control brightness. Maybe something like
> USE_PWM would be more accurate. Or is the name taken from the datasheet?
> In that case there might be some value in keeping it.
In the datasheet, the name is 'PWM ENABLE' and bit description is 'PWM input con
trol'. So I'd change it as 'LP8788_BL_PWM_INPUT_EN'.
Thanks for your suggestion!
>
> > +static inline bool is_brightness_ctrl_by_pwm(enum
> lp8788_bl_ctrl_mode mode)
> > +{
> > + return (mode == LP8788_BL_COMB_PWM_BASED);
> > +}
> > +
> > +static inline bool is_brightness_ctrl_by_register(enum
> lp8788_bl_ctrl_mode mode)
> > +{
> > + return (mode == LP8788_BL_REGISTER_ONLY ||
> > + mode == LP8788_BL_COMB_REGISTER_BASED);
> > +}
> > +
> > +static int lp8788_backlight_configure(struct lp8788_bl *bl)
> > +{
> > + struct lp8788_backlight_platform_data *pdata = bl->pdata;
> > + struct lp8788_bl_config *cfg = &default_bl_config;
> > + int ret;
> > + u8 val;
> > +
> > + /* update chip configuration if platform data exists,
> > + otherwise use the default settings */
>
> CodingStyle says this should be:
>
> /*
> * Update chip configuration if platform data exists,
> * otherwise use the default settings.
> */
>
OK, it will be fixed in the next patch.
> > + /* brightness ramp up/down */
> > + val = (cfg->rise_time << LP8788_BL_RAMP_RISE_S) | cfg->fall_time;
> > + ret = lp8788_write_byte(bl->lp, LP8788_BL_RAMP, val);
> > + if (ret) {
> > + /* no I2C needed in case of pwm control mode */
> > + if (is_brightness_ctrl_by_pwm(cfg->bl_mode))
> > + goto no_err;
> > + else
> > + return ret;
> > + }
>
> In that case, shouldn't you rather move the is_brightness_ctrl_by_pwm()
> check up and only write the RAMP register if required? If RAMP doesn't
> have any effect in PWM control mode, then you don't need to write it.
The first I2C command in this driver is accessing 'LP8788_BL_RAMP' register.
In case of PWM control mode, I2C communication may get failed because I2C lines
are not connected or activated.
But this condition is not harmful. Therefore checking PWM mode is unnecessary.
It will be removed in the next patch.
Thanks!
> And I'd prefer if PWM was consistently spelled with all capitals in
> text. There are a few other occurrences in the rest of the patch.
I'll keep it mind.
>
> > +static ssize_t lp8788_get_bl_ctl_mode(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct lp8788_bl *bl = dev_get_drvdata(dev);
> > + enum lp8788_bl_ctrl_mode mode = bl->mode;
> > + char *strmode;
> > +
> > + if (is_brightness_ctrl_by_pwm(mode))
> > + strmode = "pwm based";
> > + else if (is_brightness_ctrl_by_register(mode))
> > + strmode = "register based";
> > + else
> > + strmode = "invalid mode";
> > +
> > + return scnprintf(buf, BUF_SIZE, "%s\n", strmode);
> > +}
>
> According to Documentation/filesystems/sysfs.txt, buf is always a whole
> page, so you can use PAGE_SIZE instead of BUF_SIZE.
OK.
Thanks for all your comments.
Best Regards,
Milo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] backlight: add new lp8788 backlight driver
2013-01-03 8:54 ` Kim, Milo
@ 2013-01-03 9:05 ` Thierry Reding
0 siblings, 0 replies; 6+ messages in thread
From: Thierry Reding @ 2013-01-03 9:05 UTC (permalink / raw)
To: Kim, Milo
Cc: Richard Purdie, Andrew Morton, linux-kernel@vger.kernel.org,
Samuel Ortiz
[-- Attachment #1: Type: text/plain, Size: 1804 bytes --]
On Thu, Jan 03, 2013 at 08:54:27AM +0000, Kim, Milo wrote:
> Hi Thierry,
>
> > > (Platform data)
> > > Configurable data can be defined in the platform side.
> > > name : backlight driver name. (default: "lcd-
> > backlight")
> > > initial_brightness : initial value of backlight brightness
> > > bl_mode : brightness control by PWM or lp8788 register
> > > dim_mode : dimming mode selection
> > > full_scale : full scale current setting
> > > rise_time : brightness ramp up step time
> > > fall_time : brightness ramp down step time
> > > pwm_pol : PWM polarity setting when bl_mode is PWM
> > based
> >
> > You might want to consider using enum pwm_polarity from linux/pwm.h
> > instead and convert to the driver representation internally.
> >
> > I'm saying this because I'm thinking about extending the PWM framework
> > to allow PWM polarity and period to be specified in the PWM lookup
> > table
> > so that they can be treated transparently, independent of whether they
> > are obtained from DT or the lookup table.
> >
> > That would allow the polarity and period to be retrieved with accessors
> > like pwm_get_polarity() and pwm_get_period().
>
> OK, pwm_pol will be replaced with pwm_get_polarity() in the next patch.
No, you can't do that yet because it will only work if you get the PWM
from DT. I meant that once the pieces in the PWM framework are there we
could encode the polarity in the PWM lookup table and *then* use
pwm_get_polarity() to obtain it instead of via platform data.
For now, I think you should only convert it to enum pwm_polarity so that
at least the data type is the same as the one used in the PWM framework.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] backlight: add new lp8788 backlight driver
2012-12-21 7:55 [PATCH v2 2/2] backlight: add new lp8788 backlight driver Kim, Milo
2012-12-21 22:28 ` Andrew Morton
2013-01-03 8:08 ` Thierry Reding
@ 2013-01-22 0:32 ` Samuel Ortiz
2 siblings, 0 replies; 6+ messages in thread
From: Samuel Ortiz @ 2013-01-22 0:32 UTC (permalink / raw)
To: Kim, Milo
Cc: Richard Purdie, Andrew Morton, linux-kernel@vger.kernel.org,
Thierry Reding
Hi,
On Fri, Dec 21, 2012 at 07:55:23AM +0000, Kim, Milo wrote:
> TI LP8788 PMU supports regulators, battery charger, RTC, ADC, backlight driver
> and current sinks.
> This patch enables LP8788 backlight module.
>
> (Brightness mode)
> The brightness is controlled by PWM input or I2C register.
> All modes are supported in the driver.
>
> (Platform data)
> Configurable data can be defined in the platform side.
> name : backlight driver name. (default: "lcd-backlight")
> initial_brightness : initial value of backlight brightness
> bl_mode : brightness control by PWM or lp8788 register
> dim_mode : dimming mode selection
> full_scale : full scale current setting
> rise_time : brightness ramp up step time
> fall_time : brightness ramp down step time
> pwm_pol : PWM polarity setting when bl_mode is PWM based
> period_ns : platform specific PWM period value. unit is nano.
>
> The default values are set in case no platform data is defined.
>
> Patch v2.
> use generic PWM functions rather than lp8788 platform data function calls.
> add configurable PWM period in the platform data.
> replace module_init() and module_exit() with module_platform_driver().
>
> Patch v1.
> initial patch
>
> Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
> ---
> drivers/video/backlight/Kconfig | 6 +
> drivers/video/backlight/Makefile | 1 +
> drivers/video/backlight/lp8788_bl.c | 350 +++++++++++++++++++++++++++++++++++
> include/linux/mfd/lp8788.h | 16 +-
For the MFD parts:
Acked-by: Samuel Ortiz <sameo@linux.intel.com>
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-01-22 0:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-12-21 7:55 [PATCH v2 2/2] backlight: add new lp8788 backlight driver Kim, Milo
2012-12-21 22:28 ` Andrew Morton
2013-01-03 8:08 ` Thierry Reding
2013-01-03 8:54 ` Kim, Milo
2013-01-03 9:05 ` Thierry Reding
2013-01-22 0:32 ` Samuel Ortiz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox