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...@kernel.org>
Cc: Sebastian Reichel <s...@kernel.org>
Cc: Tony Lindgren <t...@atomide.com>
Signed-off-by: Milo Kim <milo....@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.
