| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 20 Mar 2017 12:03:48 +0000
From: Daniel Thompson <daniel.thompson@...aro.org>
To: Milo Kim <milo.kim@...com>,
–Lee Jones <lee.jones@...aro.org>,
Jingoo Han <jingoohan1@...il.com>
Cc: linux-kernel@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
Sebastian Reichel <sre@...nel.org>,
Tony Lindgren <tony@...mide.com>
Subject: Re: [PATCH] backlight: Add TI LMU backlight driver
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@...nel.org>
> Cc: Sebastian Reichel <sre@...nel.org>
> Cc: Tony Lindgren <tony@...mide.com>
> Signed-off-by: Milo Kim <milo.kim@...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.
Powered by blists - more mailing lists