Re: [PATCH v2 2/2] backlight: add new lp8788 backlight driver

[Thierry Reding <thierry.reding@avionic-design.de>]

[-- 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 --]