Re: [PATCH] backlight: Add TI LMU backlight driver

2017-03-20 Thread Daniel Thompson 

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 
Cc: Sebastian Reichel 
Cc: Tony Lindgren 
Signed-off-by: Milo Kim 


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

Re: [PATCH] backlight: Add TI LMU backlight driver

2017-03-20 Thread Daniel Thompson 

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 
Cc: Sebastian Reichel 
Cc: Tony Lindgren 
Signed-off-by: Milo Kim 


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

Re: [PATCH] backlight: Add TI LMU backlight driver

2017-03-20 Thread Sebastian Reichel 
Hi,

On Mon, Mar 20, 2017 at 11:21:04AM +0900, 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.
> 
> 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 
> Cc: Sebastian Reichel 
> Cc: Tony Lindgren 
> Signed-off-by: Milo Kim 

Works on Droid 4:

Tested-by: Sebastian Reichel 

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH] backlight: Add TI LMU backlight driver

2017-03-20 Thread Sebastian Reichel 
Hi,

On Mon, Mar 20, 2017 at 11:21:04AM +0900, 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.
> 
> 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 
> Cc: Sebastian Reichel 
> Cc: Tony Lindgren 
> Signed-off-by: Milo Kim 

Works on Droid 4:

Tested-by: Sebastian Reichel 

-- Sebastian


signature.asc
Description: PGP signature

[PATCH] backlight: Add TI LMU backlight driver

2017-03-19 Thread Milo Kim 
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.

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 
Cc: Sebastian Reichel 
Cc: Tony Lindgren 
Signed-off-by: Milo Kim 
---
 .../bindings/leds/backlight/ti-lmu-backlight.txt   |  65 ++
 drivers/video/backlight/Kconfig|   7 +
 drivers/video/backlight/Makefile   |   3 +
 drivers/video/backlight/ti-lmu-backlight-core.c| 655 +
 drivers/video/backlight/ti-lmu-backlight-data.c| 287 +
 include/linux/mfd/ti-lmu-backlight.h   | 290 +
 6 files changed, 1307 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/leds/backlight/ti-lmu-backlight.txt
 create mode 100644 drivers/video/backlight/ti-lmu-backlight-core.c
 create mode 100644 drivers/video/backlight/ti-lmu-backlight-data.c
 create mode 100644 include/linux/mfd/ti-lmu-backlight.h

diff --git 
a/Documentation/devicetree/bindings/leds/backlight/ti-lmu-backlight.txt 
b/Documentation/devicetree/bindings/leds/backlight/ti-lmu-backlight.txt
new file mode 100644
index ..c2c35b293716
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/backlight/ti-lmu-backlight.txt
@@ -0,0 +1,65 @@
+TI LMU backlight device tree bindings
+
+Required property:
+  - compatible: Should be one of:
+"ti,lm3532-backlight"
+"ti,lm3631-backlight"
+"ti,lm3632-backlight"
+"ti,lm3633-backlight"
+"ti,lm3695-backlight"
+"ti,lm3697-backlight"
+
+Optional properties:
+  There are two backlight control mode. One is I2C, the other is PWM mode.
+  Following properties are only specified in PWM mode.
+  Please note that LMU backlight device can have only one PWM channel.
+
+  - pwms: OF device-tree PWM specification.
+  - pwm-names: a list of names for the PWM devices specified in the "pwms"
+   property.
+
+  For the PWM user nodes, please refer to [1].
+
+Child nodes:
+  LMU backlight is represented as sub-nodes of the TI LMU device [2].
+  So, LMU backlight should have more than one backlight child node.
+  Each node exactly matches with backlight control bank configuration.
+  Maximum numbers of child nodes depend on the device.
+  1 = LM3631, LM3632, LM3695
+  2 = LM3633, LM3697
+  3 = LM3532
+
+  Required property of a child node:
+  - led-sources: List of enabled channels from 0 to 2.
+ Please refer to LED binding [3].
+ For output channels, please refer to the datasheets [4].
+
+  Optional properties of a child node:
+  - label: Backlight channel identification.
+   Please refer to LED binding [3].
+  - default-brightness-level: Backlight initial brightness value.
+  Type is . It is set as soon as backlight
+  device is created.
+  0 ~ 2047 = LM3631, LM3632, LM3633, LM3695 and
+ LM3697
+  0 ~ 255  = LM3532
+  - ramp-up-msec, ramp-down-msec: Light dimming effect properties.
+  Type is . Unit is millisecond.
+  0 ~ 65 msec= LM3532
+  0 ~ 4000 msec  = LM3631
+  0 ~ 16000 msec = LM3633 and LM3697
+  - pwm-period: PWM period. Only valid in PWM brightness mode.
+Type is . If this property is missing, then control
+mode is set to I2C by default.
+
+Examples: Please

[PATCH] backlight: Add TI LMU backlight driver

2017-03-19 Thread Milo Kim 
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.

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 
Cc: Sebastian Reichel 
Cc: Tony Lindgren 
Signed-off-by: Milo Kim 
---
 .../bindings/leds/backlight/ti-lmu-backlight.txt   |  65 ++
 drivers/video/backlight/Kconfig|   7 +
 drivers/video/backlight/Makefile   |   3 +
 drivers/video/backlight/ti-lmu-backlight-core.c| 655 +
 drivers/video/backlight/ti-lmu-backlight-data.c| 287 +
 include/linux/mfd/ti-lmu-backlight.h   | 290 +
 6 files changed, 1307 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/leds/backlight/ti-lmu-backlight.txt
 create mode 100644 drivers/video/backlight/ti-lmu-backlight-core.c
 create mode 100644 drivers/video/backlight/ti-lmu-backlight-data.c
 create mode 100644 include/linux/mfd/ti-lmu-backlight.h

diff --git 
a/Documentation/devicetree/bindings/leds/backlight/ti-lmu-backlight.txt 
b/Documentation/devicetree/bindings/leds/backlight/ti-lmu-backlight.txt
new file mode 100644
index ..c2c35b293716
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/backlight/ti-lmu-backlight.txt
@@ -0,0 +1,65 @@
+TI LMU backlight device tree bindings
+
+Required property:
+  - compatible: Should be one of:
+"ti,lm3532-backlight"
+"ti,lm3631-backlight"
+"ti,lm3632-backlight"
+"ti,lm3633-backlight"
+"ti,lm3695-backlight"
+"ti,lm3697-backlight"
+
+Optional properties:
+  There are two backlight control mode. One is I2C, the other is PWM mode.
+  Following properties are only specified in PWM mode.
+  Please note that LMU backlight device can have only one PWM channel.
+
+  - pwms: OF device-tree PWM specification.
+  - pwm-names: a list of names for the PWM devices specified in the "pwms"
+   property.
+
+  For the PWM user nodes, please refer to [1].
+
+Child nodes:
+  LMU backlight is represented as sub-nodes of the TI LMU device [2].
+  So, LMU backlight should have more than one backlight child node.
+  Each node exactly matches with backlight control bank configuration.
+  Maximum numbers of child nodes depend on the device.
+  1 = LM3631, LM3632, LM3695
+  2 = LM3633, LM3697
+  3 = LM3532
+
+  Required property of a child node:
+  - led-sources: List of enabled channels from 0 to 2.
+ Please refer to LED binding [3].
+ For output channels, please refer to the datasheets [4].
+
+  Optional properties of a child node:
+  - label: Backlight channel identification.
+   Please refer to LED binding [3].
+  - default-brightness-level: Backlight initial brightness value.
+  Type is . It is set as soon as backlight
+  device is created.
+  0 ~ 2047 = LM3631, LM3632, LM3633, LM3695 and
+ LM3697
+  0 ~ 255  = LM3532
+  - ramp-up-msec, ramp-down-msec: Light dimming effect properties.
+  Type is . Unit is millisecond.
+  0 ~ 65 msec= LM3532
+  0 ~ 4000 msec  = LM3631
+  0 ~ 16000 msec = LM3633 and LM3697
+  - pwm-period: PWM period. Only valid in PWM brightness mode.
+Type is . If this property is missing, then control
+mode is set to I2C by default.
+
+Examples: Please refer to ti-lmu dt-bindings. [2].
+
+[1] ../pwm/pwm.txt
+[2]