Subject: Re: [PATCH] backlight: Add TI LMU backlight driver
To: Milo Kim <milo.kim@ti.com>,
        =?UTF-8?Q?=c2=96Lee_Jones?= <lee.jones@linaro.org>,
        Jingoo Han <jingoohan1@gmail.com>
References: <20170320022104.6448-1-milo.kim@ti.com>
Cc: linux-kernel@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
        Sebastian Reichel <sre@kernel.org>, Tony Lindgren <tony@atomide.com>
From: Daniel Thompson <daniel.thompson@linaro.org>
Message-ID: <105b1272-deab-c565-cd71-bc0729ca649d@linaro.org>
Date: Mon, 20 Mar 2017 12:03:48 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <20170320022104.6448-1-milo.kim@ti.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 7764
Lines: 276

On 20/03/17 02:21, Milo Kim wrote:
> This is consolidated driver which supports backlight devices below.
>   LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697.
>
> Structure
> ---------
>   It consists of two parts - core and data.
>
>   Core part supports features below.
>     - Backlight subsystem control
>     - Channel configuration from DT properties
>     - Light dimming effect control: ramp up and down.
>     - LMU fault monitor notifier handling
>     - PWM brightness control
>
>   Data part describes device specific data.
>     - Register value configuration for each LMU device
>       : initialization, channel configuration, control mode, enable and
>         brightness.
>     - PWM action configuration
>     - Light dimming effect table
>     - Option for LMU fault monitor support
>
> Macros for register data
> ------------------------
>   All LMU devices have 8-bit based registers. LMU_BL_REG() creates 24-bit
>   register value in data part. It consists of address, mask and value.
>   On the other hand, register value should be parsed when the driver
>   reads/writes data from/to I2C registers. Driver uses LMU_BL_GET_ADDR(),
>   LMU_BL_GET_MASK() and LMU_BL_GET_VAL() for this purpose.

This sounds suspiciously like you have hand rolled your own structure 
type and, bluntly, this strikes me as pretty crazy

What is wrong with struct { u8 addr; u8 mask; u8 val; }?


> Data structure
> --------------
>   ti_lmu_bl:         Backlight output channel data
>   ti_lmu_bl_chip:    Backlight device data. One device can have multiple
>                      backlight channel data.
>   ti_lmu_bl_reg:     Backlight device register data
>   ti_lmu_bl_cfg:     Backlight configuration data for each LMU device
>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Sebastian Reichel <sre@kernel.org>
> Cc: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Milo Kim <milo.kim@ti.com>

I've only done a very quick review of this patch since I'd prefer to see 
the above sorted out before I do a more detailed review.

However I did spot a couple of other small things that I might as well 
share now. Nevertheless please don't be suprised when further issues 
come out after you share v2!


> ---
>  .../bindings/leds/backlight/ti-lmu-backlight.txt   |  65 ++

Device tree bindings should be sent in a seperate patch,  see 
Documentation/devicetree/bindings/submitting-patches.txt .


> +static int ti_lmu_backlight_enable(struct ti_lmu_bl *lmu_bl, int enable)
> +{
> +	struct ti_lmu_bl_chip *chip = lmu_bl->chip;
> +	struct regmap *regmap = chip->lmu->regmap;
> +	unsigned long enable_time = chip->cfg->reginfo->enable_usec;
> +	u8 *reg = chip->cfg->reginfo->enable;
> +	u8 mask = BIT(lmu_bl->bank_id);
> +
> +	if (!reg)
> +		return -EINVAL;
> +
> +	if (enable)
> +		return regmap_update_bits(regmap, *reg, mask, mask);
> +	else
> +		return regmap_update_bits(regmap, *reg, mask, 0);
> +
> +	if (enable_time > 0)
> +		usleep_range(enable_time, enable_time + 100);
> +}

That sleep is unreachable.


> +
> +static void ti_lmu_backlight_pwm_ctrl(struct ti_lmu_bl *lmu_bl, int brightness,
> +				      int max_brightness)
> +{
> +	struct pwm_device *pwm;
> +	unsigned int duty, period;
> +
> +	if (!lmu_bl->pwm) {
> +		pwm = devm_pwm_get(lmu_bl->chip->dev, DEFAULT_PWM_NAME);
> +		if (IS_ERR(pwm)) {
> +			dev_err(lmu_bl->chip->dev,
> +				"Can not get PWM device, err: %ld\n",
> +				PTR_ERR(pwm));
> +			return;
> +		}
> +
> +		lmu_bl->pwm = pwm;
> +
> +		/*
> +		 * FIXME: pwm_apply_args() should be removed when switching to
> +		 * the atomic PWM API.
> +		 */
> +		pwm_apply_args(pwm);

What is a plan for this FIXME?


> +	}
> +
> +	period = lmu_bl->pwm_period;
> +	duty = brightness * period / max_brightness;
> +
> +	pwm_config(lmu_bl->pwm, duty, period);
> +	if (duty)
> +		pwm_enable(lmu_bl->pwm);
> +	else
> +		pwm_disable(lmu_bl->pwm);
> +}
> +
> +static int ti_lmu_backlight_update_brightness_register(struct ti_lmu_bl *lmu_bl,
> +						       int brightness)

This function appears to do a lot more than "update the brightness 
register". It seems like a lot of the logic at the top of this function 
belongs in the caller instead.


> +{
> +	const struct ti_lmu_bl_cfg *cfg = lmu_bl->chip->cfg;
> +	const struct ti_lmu_bl_reg *reginfo = cfg->reginfo;
> +	struct regmap *regmap = lmu_bl->chip->lmu->regmap;
> +	u8 reg, val;
> +	int ret;
> +
> +	if (lmu_bl->mode == BL_PWM_BASED) {
> +		switch (cfg->pwm_action) {
> +		case UPDATE_PWM_ONLY:
> +			/* No register update is required */
> +			return 0;
> +		case UPDATE_MAX_BRT:
> +			/*
> +			 * PWM can start from any non-zero code and dim down
> +			 * to zero. So, brightness register should be updated
> +			 * even in PWM mode.
> +			 */
> +			if (brightness > 0)
> +				brightness = MAX_BRIGHTNESS_11BIT;
> +			else
> +				brightness = 0;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	/*
> +	 * Brightness register update
> +	 *
> +	 * 11 bit dimming: update LSB bits and write MSB byte.
> +	 *		   MSB brightness should be shifted.
> +	 *  8 bit dimming: write MSB byte.
> +	 */
> +
> +	if (!reginfo->brightness_msb)
> +		return -EINVAL;

Under what circumstances could brightness_msb be invalid?

If you're worried this might be unset this should have been checked 
(once) during registration.I could see would be inadequate error 
checking during registration...


> +
> +	if (cfg->max_brightness == MAX_BRIGHTNESS_11BIT) {
> +		if (!reginfo->brightness_lsb)
> +			return -EINVAL;
> +
> +		reg = reginfo->brightness_lsb[lmu_bl->bank_id];
> +		ret = regmap_update_bits(regmap, reg,
> +					 LMU_BACKLIGHT_11BIT_LSB_MASK,
> +					 brightness);
> +		if (ret)
> +			return ret;
> +
> +		val = (brightness >> LMU_BACKLIGHT_11BIT_MSB_SHIFT) & 0xFF;
> +	} else {
> +		val = brightness & 0xFF;
> +	}
> +
> +	reg = reginfo->brightness_msb[lmu_bl->bank_id];
> +	return regmap_write(regmap, reg, val);
> +}
> +
> +static int ti_lmu_backlight_update_status(struct backlight_device *bl_dev)
> +{
> +	struct ti_lmu_bl *lmu_bl = bl_get_data(bl_dev);
> +	int brightness = bl_dev->props.brightness;
> +	int ret;
> +
> +	if (bl_dev->props.state & BL_CORE_SUSPENDED)
> +		brightness = 0;
> +
> +	if (brightness > 0)
> +		ret = ti_lmu_backlight_enable(lmu_bl, 1);
> +	else
> +		ret = ti_lmu_backlight_enable(lmu_bl, 0);

ret = ti_lmu_backlight_enable(lmu_bl, brightness > 0);

> [...]
> +static struct ti_lmu_bl_chip *
> +ti_lmu_backlight_register(struct device *dev, struct ti_lmu *lmu,
> +			  const struct ti_lmu_bl_cfg *cfg)
> +{
> +	struct ti_lmu_bl_chip *chip;
> +	struct ti_lmu_bl *each;
> +	int i, ret;
> +
> +	if (!cfg) {
> +		dev_err(dev, "Operation is not configured\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return ERR_PTR(-ENOMEM);
> +
> +	chip->dev = dev;
> +	chip->lmu = lmu;
> +	chip->cfg = cfg;
> +
> +	ret = ti_lmu_backlight_of_create(chip, dev->of_node);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ret = ti_lmu_backlight_init(chip);
> +	if (ret) {
> +		dev_err(dev, "Backlight init err: %d\n", ret);
> +		return ERR_PTR(ret);
> +	}
> +
> +	for (i = 0; i < chip->num_backlights; i++) {
> +		each = chip->lmu_bl + i;
> +
> +		ret = ti_lmu_backlight_configure(each);
> +		if (ret) {
> +			dev_err(dev, "Backlight config err: %d\n", ret);
> +			return ERR_PTR(ret);
> +		}
> +
> +		ret = ti_lmu_backlight_add_device(dev, each);
> +		if (ret) {
> +			dev_err(dev, "Backlight device err: %d\n", ret);
> +			return ERR_PTR(ret);
> +		}
> +
> +		backlight_update_status(each->bl_dev);

Having asked why brightness_msb could ever by unset it is ironic to see 
the error code ignored here (so in the case it was unset we would 
spuriously have a successful probe anyway).


 > [...]


Daniel.