Re: [PATCH] power: supply: lp8788: prevent out of bounds array access

[Kim, Milo]

On 3/26/2017 1:00 AM, Giedrius Statkevičius wrote:

val might become 7 in which case stime[7] (array of length 7) would be
accessed during the scnprintf call later and that will cause issues.
Obviously, string concatenation is not intended here so just a comma needs
to be added to fix the issue.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevic...@gmail.com>


Acked-by: Milo Kim <milo@ti.com>


---
 drivers/power/supply/lp8788-charger.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/supply/lp8788-charger.c 
b/drivers/power/supply/lp8788-charger.c
index 509e2b341bd6..677f7c40b25a 100644
--- a/drivers/power/supply/lp8788-charger.c
+++ b/drivers/power/supply/lp8788-charger.c
@@ -651,7 +651,7 @@ static ssize_t lp8788_show_eoc_time(struct device *dev,
 {
struct lp8788_charger *pchg = dev_get_drvdata(dev);
char *stime[] = { "400ms", "5min", "10min", "15min",
-   "20min", "25min", "30min" "No timeout" };
+   "20min", "25min", "30min", "No timeout" };
u8 val;

lp8788_read_byte(pchg->lp, LP8788_CHG_EOC, );

Re: [PATCH] power: supply: lp8788: prevent out of bounds array access

[Kim, Milo]

On 3/26/2017 1:00 AM, Giedrius Statkevičius wrote:

val might become 7 in which case stime[7] (array of length 7) would be
accessed during the scnprintf call later and that will cause issues.
Obviously, string concatenation is not intended here so just a comma needs
to be added to fix the issue.

Signed-off-by: Giedrius Statkevičius 


Acked-by: Milo Kim 


---
 drivers/power/supply/lp8788-charger.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/supply/lp8788-charger.c 
b/drivers/power/supply/lp8788-charger.c
index 509e2b341bd6..677f7c40b25a 100644
--- a/drivers/power/supply/lp8788-charger.c
+++ b/drivers/power/supply/lp8788-charger.c
@@ -651,7 +651,7 @@ static ssize_t lp8788_show_eoc_time(struct device *dev,
 {
struct lp8788_charger *pchg = dev_get_drvdata(dev);
char *stime[] = { "400ms", "5min", "10min", "15min",
-   "20min", "25min", "30min" "No timeout" };
+   "20min", "25min", "30min", "No timeout" };
u8 val;

lp8788_read_byte(pchg->lp, LP8788_CHG_EOC, );

Re: [PATCH 1/2] Documentation: dt-bindings: Use generic property for hardware enable pins

[Kim, Milo]

On 3/3/2017 3:21 PM, Rob Herring wrote:

On Tue, Feb 28, 2017 at 04:50:40PM +0900, Milo Kim wrote:

With index usages, device specific properties can be replaced with generic
one. Vpos is index 0 and Vneg is index 1.
DT examples are added as well.

Signed-off-by: Milo Kim <milo@ti.com>
---
 .../bindings/regulator/lm363x-regulator.txt| 78 +-
 1 file changed, 76 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/lm363x-regulator.txt 
b/Documentation/devicetree/bindings/regulator/lm363x-regulator.txt
index 8f14df9d1205..cc5a6151d85f 100644
--- a/Documentation/devicetree/bindings/regulator/lm363x-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/lm363x-regulator.txt
@@ -8,8 +8,8 @@ Required property:

 Optional properties:
   LM3632 has external enable pins for two LDOs.
-  - ti,lcm-en1-gpio: A GPIO specifier for Vpos control pin.
-  - ti,lcm-en2-gpio: A GPIO specifier for Vneg control pin.
+  - enable-gpios: Two GPIO specifiers for Vpos and Vneg control pins.
+  The first entry is Vpos, the second is Vneg enable pin.


You're breaking compatibility with existing DTBs. You need to explain
that and why it is okay in the commit message. In this case, I don't
think it is okay as this chip could be used across vendors' platforms.


Thanks for your comment.

The lm363x-regulator has a dependency on ti-lmu MFD driver which is not 
upstreamed. So I don't think this patch will break the compatibility 
because two properties are not used anywhere at this moment.

Please correct me if it's incorrect.

Using general DT property is simple/clear because two enable pins are 
differentiable by selecting the index number. That's the main reason of 
this patch.


Best regards,
Milo

Re: [PATCH 1/2] Documentation: dt-bindings: Use generic property for hardware enable pins

[Kim, Milo]

On 3/3/2017 3:21 PM, Rob Herring wrote:

On Tue, Feb 28, 2017 at 04:50:40PM +0900, Milo Kim wrote:

With index usages, device specific properties can be replaced with generic
one. Vpos is index 0 and Vneg is index 1.
DT examples are added as well.

Signed-off-by: Milo Kim 
---
 .../bindings/regulator/lm363x-regulator.txt| 78 +-
 1 file changed, 76 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/lm363x-regulator.txt 
b/Documentation/devicetree/bindings/regulator/lm363x-regulator.txt
index 8f14df9d1205..cc5a6151d85f 100644
--- a/Documentation/devicetree/bindings/regulator/lm363x-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/lm363x-regulator.txt
@@ -8,8 +8,8 @@ Required property:

 Optional properties:
   LM3632 has external enable pins for two LDOs.
-  - ti,lcm-en1-gpio: A GPIO specifier for Vpos control pin.
-  - ti,lcm-en2-gpio: A GPIO specifier for Vneg control pin.
+  - enable-gpios: Two GPIO specifiers for Vpos and Vneg control pins.
+  The first entry is Vpos, the second is Vneg enable pin.


You're breaking compatibility with existing DTBs. You need to explain
that and why it is okay in the commit message. In this case, I don't
think it is okay as this chip could be used across vendors' platforms.


Thanks for your comment.

The lm363x-regulator has a dependency on ti-lmu MFD driver which is not 
upstreamed. So I don't think this patch will break the compatibility 
because two properties are not used anywhere at this moment.

Please correct me if it's incorrect.

Using general DT property is simple/clear because two enable pins are 
differentiable by selecting the index number. That's the main reason of 
this patch.


Best regards,
Milo

Re: [patch] power: supply: lp8788: remove an unneeded NULL check

[Kim, Milo]

On 10/14/2016 4:33 PM, Dan Carpenter wrote:

We checked that "pdata->chg_params" is non-NULL earlier in this function
so when we add "i" to it, it's still non-NULL.

Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>


Acked-by: Milo Kim <milo@ti.com>

Thanks for catching this.

Best regards,
Milo

Re: [patch] power: supply: lp8788: remove an unneeded NULL check

[Kim, Milo]

On 10/14/2016 4:33 PM, Dan Carpenter wrote:

We checked that "pdata->chg_params" is non-NULL earlier in this function
so when we add "i" to it, it's still non-NULL.

Signed-off-by: Dan Carpenter 


Acked-by: Milo Kim 

Thanks for catching this.

Best regards,
Milo

Re: [PATCH v2] backlight: lp855x: Add enable regulator

[Kim, Milo]

On 6/11/2016 4:39 AM, Brian Norris wrote:

The LP8556 datasheet describes an EN/VDDIO input, which serves "both as
a chip enable and as a power supply reference for PWM, SDA, and SCL
inputs." The LP8556 that I'm testing doesn't respond properly if I try
to talk I2C to it too quickly after enabling VDDIO, and the LP8555
datasheet mentions a t_RESPONSE delay of up to 1 millisecond.

Support this EN/VDDIO by adding a regulator property to the binding;
enabling this regulator at probe time; and sleeping for 1 to 2ms, if the
EN/VDDIO regulator was provided.

Signed-off-by: Brian Norris <briannor...@chromium.org>


Acked-by: Milo Kim <milo@ti.com>

Thanks!


---
v2: s/TS8555/LP8555/ in comment (typo)

  .../devicetree/bindings/leds/backlight/lp855x.txt  |  2 ++
  drivers/video/backlight/lp855x_bl.c| 29 ++
  2 files changed, 31 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/backlight/lp855x.txt 
b/Documentation/devicetree/bindings/leds/backlight/lp855x.txt
index 0a3ecbc3a1b9..88f56641fc28 100644
--- a/Documentation/devicetree/bindings/leds/backlight/lp855x.txt
+++ b/Documentation/devicetree/bindings/leds/backlight/lp855x.txt
@@ -13,6 +13,7 @@ Optional properties:
- rom-addr: Register address of ROM area to be updated (u8)
- rom-val: Register value to be updated (u8)
- power-supply: Regulator which controls the 3V rail
+  - enable-supply: Regulator which controls the EN/VDDIO input

  Example:

@@ -57,6 +58,7 @@ Example:
backlight@2c {
compatible = "ti,lp8557";
reg = <0x2c>;
+   enable-supply = <_vddio>;
power-supply = <_vdd>;

dev-ctrl = /bits/ 8 <0x41>;
diff --git a/drivers/video/backlight/lp855x_bl.c 
b/drivers/video/backlight/lp855x_bl.c
index e5b14f52628f..939f057836e1 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -13,6 +13,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -74,6 +75,7 @@ struct lp855x {
struct lp855x_platform_data *pdata;
struct pwm_device *pwm;
struct regulator *supply;   /* regulator for VDD input */
+   struct regulator *enable;   /* regulator for EN/VDDIO input */
  };

  static int lp855x_write_byte(struct lp855x *lp, u8 reg, u8 data)
@@ -433,6 +435,19 @@ static int lp855x_probe(struct i2c_client *cl, const 
struct i2c_device_id *id)
lp->supply = NULL;
}

+   lp->enable = devm_regulator_get_optional(lp->dev, "enable");
+   if (IS_ERR(lp->enable)) {
+   ret = PTR_ERR(lp->enable);
+   if (ret == -ENODEV) {
+   lp->enable = NULL;
+   } else {
+   if (ret != -EPROBE_DEFER)
+   dev_err(lp->dev, "error getting enable regulator: 
%d\n",
+   ret);
+   return ret;
+   }
+   }
+
if (lp->supply) {
ret = regulator_enable(lp->supply);
if (ret < 0) {
@@ -441,6 +456,20 @@ static int lp855x_probe(struct i2c_client *cl, const 
struct i2c_device_id *id)
}
}

+   if (lp->enable) {
+   ret = regulator_enable(lp->enable);
+   if (ret < 0) {
+   dev_err(lp->dev, "failed to enable vddio: %d\n", ret);
+   return ret;
+   }
+
+   /*
+* LP8555 datasheet says t_RESPONSE (time between VDDIO and
+* I2C) is 1ms.
+*/
+   usleep_range(1000, 2000);
+   }
+
i2c_set_clientdata(cl, lp);

ret = lp855x_configure(lp);

Re: [PATCH v2] backlight: lp855x: Add enable regulator

[Kim, Milo]

On 6/11/2016 4:39 AM, Brian Norris wrote:

The LP8556 datasheet describes an EN/VDDIO input, which serves "both as
a chip enable and as a power supply reference for PWM, SDA, and SCL
inputs." The LP8556 that I'm testing doesn't respond properly if I try
to talk I2C to it too quickly after enabling VDDIO, and the LP8555
datasheet mentions a t_RESPONSE delay of up to 1 millisecond.

Support this EN/VDDIO by adding a regulator property to the binding;
enabling this regulator at probe time; and sleeping for 1 to 2ms, if the
EN/VDDIO regulator was provided.

Signed-off-by: Brian Norris 


Acked-by: Milo Kim 

Thanks!


---
v2: s/TS8555/LP8555/ in comment (typo)

  .../devicetree/bindings/leds/backlight/lp855x.txt  |  2 ++
  drivers/video/backlight/lp855x_bl.c| 29 ++
  2 files changed, 31 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/backlight/lp855x.txt 
b/Documentation/devicetree/bindings/leds/backlight/lp855x.txt
index 0a3ecbc3a1b9..88f56641fc28 100644
--- a/Documentation/devicetree/bindings/leds/backlight/lp855x.txt
+++ b/Documentation/devicetree/bindings/leds/backlight/lp855x.txt
@@ -13,6 +13,7 @@ Optional properties:
- rom-addr: Register address of ROM area to be updated (u8)
- rom-val: Register value to be updated (u8)
- power-supply: Regulator which controls the 3V rail
+  - enable-supply: Regulator which controls the EN/VDDIO input

  Example:

@@ -57,6 +58,7 @@ Example:
backlight@2c {
compatible = "ti,lp8557";
reg = <0x2c>;
+   enable-supply = <_vddio>;
power-supply = <_vdd>;

dev-ctrl = /bits/ 8 <0x41>;
diff --git a/drivers/video/backlight/lp855x_bl.c 
b/drivers/video/backlight/lp855x_bl.c
index e5b14f52628f..939f057836e1 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -13,6 +13,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -74,6 +75,7 @@ struct lp855x {
struct lp855x_platform_data *pdata;
struct pwm_device *pwm;
struct regulator *supply;   /* regulator for VDD input */
+   struct regulator *enable;   /* regulator for EN/VDDIO input */
  };

  static int lp855x_write_byte(struct lp855x *lp, u8 reg, u8 data)
@@ -433,6 +435,19 @@ static int lp855x_probe(struct i2c_client *cl, const 
struct i2c_device_id *id)
lp->supply = NULL;
}

+   lp->enable = devm_regulator_get_optional(lp->dev, "enable");
+   if (IS_ERR(lp->enable)) {
+   ret = PTR_ERR(lp->enable);
+   if (ret == -ENODEV) {
+   lp->enable = NULL;
+   } else {
+   if (ret != -EPROBE_DEFER)
+   dev_err(lp->dev, "error getting enable regulator: 
%d\n",
+   ret);
+   return ret;
+   }
+   }
+
if (lp->supply) {
ret = regulator_enable(lp->supply);
if (ret < 0) {
@@ -441,6 +456,20 @@ static int lp855x_probe(struct i2c_client *cl, const 
struct i2c_device_id *id)
}
}

+   if (lp->enable) {
+   ret = regulator_enable(lp->enable);
+   if (ret < 0) {
+   dev_err(lp->dev, "failed to enable vddio: %d\n", ret);
+   return ret;
+   }
+
+   /*
+* LP8555 datasheet says t_RESPONSE (time between VDDIO and
+* I2C) is 1ms.
+*/
+   usleep_range(1000, 2000);
+   }
+
i2c_set_clientdata(cl, lp);

ret = lp855x_configure(lp);

Re: [PATCH 2/7] mfd: lp8788: Use devm_mfd_add_devices and devm_regmap_add_irq_chip

[Kim, Milo]

Hi Laxman,

On 4/21/2016 9:25 PM, Laxman Dewangan wrote:

Use devm_mfd_add_devices() for adding MFD child devices and
devm_regmap_add_irq_chip() for IRQ chip registration.


This patch doesn't include the code regarding devm_mfd_add_devices(). 
Could you check it again? Or am I missing any previous patches?




This reduces the error code path and .remove callback for removing
MFD child devices and deleting IRQ chip data.

Signed-off-by: Laxman Dewangan <ldewan...@nvidia.com>
CC: Milo Kim <milo@ti.com>
---
  drivers/mfd/lp8788-irq.c | 12 +++-
  drivers/mfd/lp8788.c | 10 --
  2 files changed, 3 insertions(+), 19 deletions(-)

diff --git a/drivers/mfd/lp8788-irq.c b/drivers/mfd/lp8788-irq.c
index 792d51b..aacd39f 100644
--- a/drivers/mfd/lp8788-irq.c
+++ b/drivers/mfd/lp8788-irq.c
@@ -175,9 +175,9 @@ int lp8788_irq_init(struct lp8788 *lp, int irq)
lp->irqdm = irqd->domain;
mutex_init(>irq_lock);

-   ret = request_threaded_irq(irq, NULL, lp8788_irq_handler,
-   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-   "lp8788-irq", irqd);
+   ret = devm_request_threaded_irq(lp->dev, irq, NULL, lp8788_irq_handler,
+   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+   "lp8788-irq", irqd);
if (ret) {
dev_err(lp->dev, "failed to create a thread for IRQ_N\n");
return ret;
@@ -187,9 +187,3 @@ int lp8788_irq_init(struct lp8788 *lp, int irq)

return 0;
  }
-
-void lp8788_irq_exit(struct lp8788 *lp)
-{
-   if (lp->irq)
-   free_irq(lp->irq, lp->irqdm);
-}
diff --git a/drivers/mfd/lp8788.c b/drivers/mfd/lp8788.c
index acf6165..37fea46 100644
--- a/drivers/mfd/lp8788.c
+++ b/drivers/mfd/lp8788.c
@@ -203,15 +203,6 @@ static int lp8788_probe(struct i2c_client *cl, const 
struct i2c_device_id *id)
   ARRAY_SIZE(lp8788_devs), NULL, 0, NULL);
  }

-static int lp8788_remove(struct i2c_client *cl)
-{
-   struct lp8788 *lp = i2c_get_clientdata(cl);
-
-   mfd_remove_devices(lp->dev);
-   lp8788_irq_exit(lp);
-   return 0;
-}
-
  static const struct i2c_device_id lp8788_ids[] = {
{"lp8788", 0},
{ }
@@ -223,7 +214,6 @@ static struct i2c_driver lp8788_driver = {
.name = "lp8788",
},
.probe = lp8788_probe,
-   .remove = lp8788_remove,
.id_table = lp8788_ids,
  };




Best regards,
Milo

Re: [PATCH 2/7] mfd: lp8788: Use devm_mfd_add_devices and devm_regmap_add_irq_chip

[Kim, Milo]

Hi Laxman,

On 4/21/2016 9:25 PM, Laxman Dewangan wrote:

Use devm_mfd_add_devices() for adding MFD child devices and
devm_regmap_add_irq_chip() for IRQ chip registration.


This patch doesn't include the code regarding devm_mfd_add_devices(). 
Could you check it again? Or am I missing any previous patches?




This reduces the error code path and .remove callback for removing
MFD child devices and deleting IRQ chip data.

Signed-off-by: Laxman Dewangan 
CC: Milo Kim 
---
  drivers/mfd/lp8788-irq.c | 12 +++-
  drivers/mfd/lp8788.c | 10 --
  2 files changed, 3 insertions(+), 19 deletions(-)

diff --git a/drivers/mfd/lp8788-irq.c b/drivers/mfd/lp8788-irq.c
index 792d51b..aacd39f 100644
--- a/drivers/mfd/lp8788-irq.c
+++ b/drivers/mfd/lp8788-irq.c
@@ -175,9 +175,9 @@ int lp8788_irq_init(struct lp8788 *lp, int irq)
lp->irqdm = irqd->domain;
mutex_init(>irq_lock);

-   ret = request_threaded_irq(irq, NULL, lp8788_irq_handler,
-   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-   "lp8788-irq", irqd);
+   ret = devm_request_threaded_irq(lp->dev, irq, NULL, lp8788_irq_handler,
+   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+   "lp8788-irq", irqd);
if (ret) {
dev_err(lp->dev, "failed to create a thread for IRQ_N\n");
return ret;
@@ -187,9 +187,3 @@ int lp8788_irq_init(struct lp8788 *lp, int irq)

return 0;
  }
-
-void lp8788_irq_exit(struct lp8788 *lp)
-{
-   if (lp->irq)
-   free_irq(lp->irq, lp->irqdm);
-}
diff --git a/drivers/mfd/lp8788.c b/drivers/mfd/lp8788.c
index acf6165..37fea46 100644
--- a/drivers/mfd/lp8788.c
+++ b/drivers/mfd/lp8788.c
@@ -203,15 +203,6 @@ static int lp8788_probe(struct i2c_client *cl, const 
struct i2c_device_id *id)
   ARRAY_SIZE(lp8788_devs), NULL, 0, NULL);
  }

-static int lp8788_remove(struct i2c_client *cl)
-{
-   struct lp8788 *lp = i2c_get_clientdata(cl);
-
-   mfd_remove_devices(lp->dev);
-   lp8788_irq_exit(lp);
-   return 0;
-}
-
  static const struct i2c_device_id lp8788_ids[] = {
{"lp8788", 0},
{ }
@@ -223,7 +214,6 @@ static struct i2c_driver lp8788_driver = {
.name = "lp8788",
},
.probe = lp8788_probe,
-   .remove = lp8788_remove,
.id_table = lp8788_ids,
  };




Best regards,
Milo

Re: [PATCH 08/20] mfd: lp3943: Use devm_mfd_add_devices() for mfd_device registration

[Kim, Milo]

On 4/5/2016 8:48 PM, Laxman Dewangan wrote:

Use devm_mfd_add_devices() for mfd devices registration and get
rid of .remove callback to remove mfd devices. This is done
by managed device framework.

Signed-off-by: Laxman Dewangan <ldewan...@nvidia.com>
CC: Milo Kim <milo@ti.com>


Acked-by: Milo Kim <milo@ti.com>


---
  drivers/mfd/lp3943.c | 14 +++---
  1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/mfd/lp3943.c b/drivers/mfd/lp3943.c
index eecbb13..65a2a8f1 100644
--- a/drivers/mfd/lp3943.c
+++ b/drivers/mfd/lp3943.c
@@ -123,16 +123,9 @@ static int lp3943_probe(struct i2c_client *cl, const 
struct i2c_device_id *id)
lp3943->mux_cfg = lp3943_mux_cfg;
i2c_set_clientdata(cl, lp3943);

-   return mfd_add_devices(dev, -1, lp3943_devs, ARRAY_SIZE(lp3943_devs),
-  NULL, 0, NULL);
-}
-
-static int lp3943_remove(struct i2c_client *cl)
-{
-   struct lp3943 *lp3943 = i2c_get_clientdata(cl);
-
-   mfd_remove_devices(lp3943->dev);
-   return 0;
+   return devm_mfd_add_devices(dev, -1, lp3943_devs,
+   ARRAY_SIZE(lp3943_devs),
+   NULL, 0, NULL);
  }

  static const struct i2c_device_id lp3943_ids[] = {
@@ -151,7 +144,6 @@ MODULE_DEVICE_TABLE(of, lp3943_of_match);

  static struct i2c_driver lp3943_driver = {
.probe = lp3943_probe,
-   .remove = lp3943_remove,
.driver = {
.name = "lp3943",
.of_match_table = of_match_ptr(lp3943_of_match),

Re: [PATCH 08/20] mfd: lp3943: Use devm_mfd_add_devices() for mfd_device registration

[Kim, Milo]

On 4/5/2016 8:48 PM, Laxman Dewangan wrote:

Use devm_mfd_add_devices() for mfd devices registration and get
rid of .remove callback to remove mfd devices. This is done
by managed device framework.

Signed-off-by: Laxman Dewangan 
CC: Milo Kim 


Acked-by: Milo Kim 


---
  drivers/mfd/lp3943.c | 14 +++---
  1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/mfd/lp3943.c b/drivers/mfd/lp3943.c
index eecbb13..65a2a8f1 100644
--- a/drivers/mfd/lp3943.c
+++ b/drivers/mfd/lp3943.c
@@ -123,16 +123,9 @@ static int lp3943_probe(struct i2c_client *cl, const 
struct i2c_device_id *id)
lp3943->mux_cfg = lp3943_mux_cfg;
i2c_set_clientdata(cl, lp3943);

-   return mfd_add_devices(dev, -1, lp3943_devs, ARRAY_SIZE(lp3943_devs),
-  NULL, 0, NULL);
-}
-
-static int lp3943_remove(struct i2c_client *cl)
-{
-   struct lp3943 *lp3943 = i2c_get_clientdata(cl);
-
-   mfd_remove_devices(lp3943->dev);
-   return 0;
+   return devm_mfd_add_devices(dev, -1, lp3943_devs,
+   ARRAY_SIZE(lp3943_devs),
+   NULL, 0, NULL);
  }

  static const struct i2c_device_id lp3943_ids[] = {
@@ -151,7 +144,6 @@ MODULE_DEVICE_TABLE(of, lp3943_of_match);

  static struct i2c_driver lp3943_driver = {
.probe = lp3943_probe,
-   .remove = lp3943_remove,
.driver = {
.name = "lp3943",
.of_match_table = of_match_ptr(lp3943_of_match),

Re: [patch] mfd: lp8788-irq: uninitialized variable in irq handler

[Kim, Milo]

On 3/11/2016 5:11 PM, Dan Carpenter wrote:

Instead to being true/false, the "handled" is true/uninitialized.
Presumably this doesn't cause that many problems in real life because
normally we handle the IRQ.

Fixes: eea6b7cc53aa ('mfd: Add lp8788 mfd driver')
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>


Acked-by: Milo Kim <milo@ti.com>

Thanks for catching this!



diff --git a/drivers/mfd/lp8788-irq.c b/drivers/mfd/lp8788-irq.c
index c7a9825..792d51b 100644
--- a/drivers/mfd/lp8788-irq.c
+++ b/drivers/mfd/lp8788-irq.c
@@ -112,7 +112,7 @@ static irqreturn_t lp8788_irq_handler(int irq, void *ptr)
struct lp8788_irq_data *irqd = ptr;
struct lp8788 *lp = irqd->lp;
u8 status[NUM_REGS], addr, mask;
-   bool handled;
+   bool handled = false;
int i;

if (lp8788_read_multi_bytes(lp, LP8788_INT_1, status, NUM_REGS))

Re: [patch] mfd: lp8788-irq: uninitialized variable in irq handler

[Kim, Milo]

On 3/11/2016 5:11 PM, Dan Carpenter wrote:

Instead to being true/false, the "handled" is true/uninitialized.
Presumably this doesn't cause that many problems in real life because
normally we handle the IRQ.

Fixes: eea6b7cc53aa ('mfd: Add lp8788 mfd driver')
Signed-off-by: Dan Carpenter 


Acked-by: Milo Kim 

Thanks for catching this!



diff --git a/drivers/mfd/lp8788-irq.c b/drivers/mfd/lp8788-irq.c
index c7a9825..792d51b 100644
--- a/drivers/mfd/lp8788-irq.c
+++ b/drivers/mfd/lp8788-irq.c
@@ -112,7 +112,7 @@ static irqreturn_t lp8788_irq_handler(int irq, void *ptr)
struct lp8788_irq_data *irqd = ptr;
struct lp8788 *lp = irqd->lp;
u8 status[NUM_REGS], addr, mask;
-   bool handled;
+   bool handled = false;
int i;

if (lp8788_read_multi_bytes(lp, LP8788_INT_1, status, NUM_REGS))

Re: [PATCH] power_supply: lp8788-charger: initialize boolean 'found'

[Kim, Milo]

On 2/27/2016 9:54 PM, Colin King wrote:

From: Colin Ian King <colin.k...@canonical.com>

The boolean 'found' is not initialized and hence garbage. It should
be initialized as false.

Found with static analysis using CoverityScan

Signed-off-by: Colin Ian King <colin.k...@canonical.com>


Acked-by: Milo Kim <milo@ti.com>

Thanks for catching this.


---
  drivers/power/lp8788-charger.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/lp8788-charger.c b/drivers/power/lp8788-charger.c
index f5a48fd..7321b72 100644
--- a/drivers/power/lp8788-charger.c
+++ b/drivers/power/lp8788-charger.c
@@ -455,7 +455,7 @@ static void lp8788_charger_event(struct work_struct *work)

  static bool lp8788_find_irq_id(struct lp8788_charger *pchg, int virq, int *id)
  {
-   bool found;
+   bool found = false;
int i;

for (i = 0; i < pchg->num_irqs; i++) {

Re: [PATCH] power_supply: lp8788-charger: initialize boolean 'found'

[Kim, Milo]

On 2/27/2016 9:54 PM, Colin King wrote:

From: Colin Ian King 

The boolean 'found' is not initialized and hence garbage. It should
be initialized as false.

Found with static analysis using CoverityScan

Signed-off-by: Colin Ian King 


Acked-by: Milo Kim 

Thanks for catching this.


---
  drivers/power/lp8788-charger.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/lp8788-charger.c b/drivers/power/lp8788-charger.c
index f5a48fd..7321b72 100644
--- a/drivers/power/lp8788-charger.c
+++ b/drivers/power/lp8788-charger.c
@@ -455,7 +455,7 @@ static void lp8788_charger_event(struct work_struct *work)

  static bool lp8788_find_irq_id(struct lp8788_charger *pchg, int virq, int *id)
  {
-   bool found;
+   bool found = false;
int i;

for (i = 0; i < pchg->num_irqs; i++) {

Re: [PATCH v2 2/4] regulator: lp872x: Add enable GPIO pin support

[Kim, Milo]

On 2/6/2016 3:42 AM, Paul Kocialkowski wrote:

LP872x regulators are made active via the EN pin, which might be hooked to a
GPIO. This adds support for driving the GPIO high when the driver is in use.

Signed-off-by: Paul Kocialkowski <cont...@paulk.fr>
---
  .../devicetree/bindings/regulator/lp872x.txt   |  1 +
  drivers/regulator/lp872x.c | 34 ++
  include/linux/regulator/lp872x.h   |  5 
  3 files changed, 40 insertions(+)


Acked-by: Milo Kim <milo@ti.com>

Best regards,
Milo

Re: [PATCH v2 2/4] regulator: lp872x: Add enable GPIO pin support

[Kim, Milo]

On 2/6/2016 3:42 AM, Paul Kocialkowski wrote:

LP872x regulators are made active via the EN pin, which might be hooked to a
GPIO. This adds support for driving the GPIO high when the driver is in use.

Signed-off-by: Paul Kocialkowski 
---
  .../devicetree/bindings/regulator/lp872x.txt   |  1 +
  drivers/regulator/lp872x.c | 34 ++
  include/linux/regulator/lp872x.h   |  5 
  3 files changed, 40 insertions(+)


Acked-by: Milo Kim 

Best regards,
Milo

Re: [PATCH v2 2/4] regulator: lp872x: Add enable GPIO pin support

[Kim, Milo]

On 2/13/2016 3:25 AM, Paul Kocialkowski wrote:

> >+
> >+  /* Always set enable GPIO high. */
> >+  ret = devm_gpio_request_one(lp->dev, gpio, GPIOF_OUT_INIT_HIGH, "LP872X 
EN");
> >+  if (ret) {
> >+  dev_err(lp->dev, "gpio request err: %d\n", ret);
> >+  return ret;
> >+  }
> >+
> >+  /* Each chip has a different enable delay. */
> >+  if (lp->chipid == LP8720)
> >+  usleep_range(LP8720_ENABLE_DELAY, 1.5 * LP8720_ENABLE_DELAY);
> >+  else
> >+  usleep_range(LP8725_ENABLE_DELAY, 1.5 * LP8725_ENABLE_DELAY);
> >+
> >+  return 0;
> >+}
> >+
> >   static int lp872x_config(struct lp872x *lp)
> >   {
> >   struct lp872x_platform_data *pdata = lp->pdata;
> >@@ -875,6 +903,8 @@ static struct lp872x_platform_data
> >   of_property_read_u8(np, "ti,dvs-state", _state);
> >   pdata->dvs->init_state = dvs_state ? DVS_HIGH : DVS_LOW;
> >
> >+  pdata->enable_gpio = of_get_named_gpio(np, "enable-gpios", 0);

>
>Please move this code to lp872x_populate_pdata_from_dt().

This already adds it in lp872x_populate_pdata_from_dt (see the context
around the insertion).


Ah, sorry. Your patch is correct.


I don't know why the diff makes it seem like it's added in
lp872x_config. It's really not.


I think it was from a line break of lp872x_populate_pdata_from_dt().
Let me add my ACK to this patch. Thank you.

Best regards,
Milo

Re: [PATCH v2 2/4] regulator: lp872x: Add enable GPIO pin support

[Kim, Milo]

On 2/13/2016 3:25 AM, Paul Kocialkowski wrote:

> >+
> >+  /* Always set enable GPIO high. */
> >+  ret = devm_gpio_request_one(lp->dev, gpio, GPIOF_OUT_INIT_HIGH, "LP872X 
EN");
> >+  if (ret) {
> >+  dev_err(lp->dev, "gpio request err: %d\n", ret);
> >+  return ret;
> >+  }
> >+
> >+  /* Each chip has a different enable delay. */
> >+  if (lp->chipid == LP8720)
> >+  usleep_range(LP8720_ENABLE_DELAY, 1.5 * LP8720_ENABLE_DELAY);
> >+  else
> >+  usleep_range(LP8725_ENABLE_DELAY, 1.5 * LP8725_ENABLE_DELAY);
> >+
> >+  return 0;
> >+}
> >+
> >   static int lp872x_config(struct lp872x *lp)
> >   {
> >   struct lp872x_platform_data *pdata = lp->pdata;
> >@@ -875,6 +903,8 @@ static struct lp872x_platform_data
> >   of_property_read_u8(np, "ti,dvs-state", _state);
> >   pdata->dvs->init_state = dvs_state ? DVS_HIGH : DVS_LOW;
> >
> >+  pdata->enable_gpio = of_get_named_gpio(np, "enable-gpios", 0);

>
>Please move this code to lp872x_populate_pdata_from_dt().

This already adds it in lp872x_populate_pdata_from_dt (see the context
around the insertion).


Ah, sorry. Your patch is correct.


I don't know why the diff makes it seem like it's added in
lp872x_config. It's really not.


I think it was from a line break of lp872x_populate_pdata_from_dt().
Let me add my ACK to this patch. Thank you.

Best regards,
Milo

Re: [PATCH v2 2/4] regulator: lp872x: Add enable GPIO pin support

[Kim, Milo]

Hi Paul,

Thanks for the patch. Please see my comments below.

On 2/6/2016 3:42 AM, Paul Kocialkowski wrote:

LP872x regulators are made active via the EN pin, which might be hooked to a
GPIO. This adds support for driving the GPIO high when the driver is in use.

Signed-off-by: Paul Kocialkowski 
---
  .../devicetree/bindings/regulator/lp872x.txt   |  1 +
  drivers/regulator/lp872x.c | 34 ++
  include/linux/regulator/lp872x.h   |  5 
  3 files changed, 40 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/lp872x.txt 
b/Documentation/devicetree/bindings/regulator/lp872x.txt
index 7818318..ca58a68 100644
--- a/Documentation/devicetree/bindings/regulator/lp872x.txt
+++ b/Documentation/devicetree/bindings/regulator/lp872x.txt
@@ -28,6 +28,7 @@ Optional properties:
- ti,dvs-gpio: GPIO specifier for external DVS pin control of LP872x 
devices.
- ti,dvs-vsel: DVS selector. 0 = SEL_V1, 1 = SEL_V2.
- ti,dvs-state: initial DVS pin state. 0 = DVS_LOW, 1 = DVS_HIGH.
+  - enable-gpios: GPIO specifier for EN pin control of LP872x devices.

Sub nodes for regulator_init_data
  LP8720 has maximum 6 nodes. (child name: ldo1 ~ 5 and buck)
diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c
index 21c49d8..3899211 100644
--- a/drivers/regulator/lp872x.c
+++ b/drivers/regulator/lp872x.c
@@ -15,6 +15,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -757,6 +758,33 @@ set_default_dvs_mode:
default_dvs_mode[lp->chipid]);
  }

+static int lp872x_hw_enable(struct lp872x *lp)
+{
+   int ret, gpio;
+
+   if (!lp->pdata)
+   return -EINVAL;
+
+   gpio = lp->pdata->enable_gpio;
+   if (!gpio_is_valid(gpio))
+   return 0;


It seems gpio is always 0 when it starts from DT. 'enable_gpio' is 
updated in lp872x_config() but lp872x_hw_enable() is called first.



+
+   /* Always set enable GPIO high. */
+   ret = devm_gpio_request_one(lp->dev, gpio, GPIOF_OUT_INIT_HIGH, "LP872X 
EN");
+   if (ret) {
+   dev_err(lp->dev, "gpio request err: %d\n", ret);
+   return ret;
+   }
+
+   /* Each chip has a different enable delay. */
+   if (lp->chipid == LP8720)
+   usleep_range(LP8720_ENABLE_DELAY, 1.5 * LP8720_ENABLE_DELAY);
+   else
+   usleep_range(LP8725_ENABLE_DELAY, 1.5 * LP8725_ENABLE_DELAY);
+
+   return 0;
+}
+
  static int lp872x_config(struct lp872x *lp)
  {
struct lp872x_platform_data *pdata = lp->pdata;
@@ -875,6 +903,8 @@ static struct lp872x_platform_data
of_property_read_u8(np, "ti,dvs-state", _state);
pdata->dvs->init_state = dvs_state ? DVS_HIGH : DVS_LOW;

+   pdata->enable_gpio = of_get_named_gpio(np, "enable-gpios", 0);


Please move this code to lp872x_populate_pdata_from_dt().
lp872x_hw_enable() is called prior to lp872x_config(), so enable_gpio 
should be set on parsing the DT properties.



+
if (of_get_child_count(np) == 0)
goto out;

@@ -948,6 +978,10 @@ static int lp872x_probe(struct i2c_client *cl, const 
struct i2c_device_id *id)
lp->chipid = id->driver_data;
i2c_set_clientdata(cl, lp);

+   ret = lp872x_hw_enable(lp);
+   if (ret)
+   return ret;
+
ret = lp872x_config(lp);
if (ret)
return ret;
diff --git a/include/linux/regulator/lp872x.h b/include/linux/regulator/lp872x.h
index 132e05c..6029279 100644
--- a/include/linux/regulator/lp872x.h
+++ b/include/linux/regulator/lp872x.h
@@ -18,6 +18,9 @@

  #define LP872X_MAX_REGULATORS 9

+#define LP8720_ENABLE_DELAY200
+#define LP8725_ENABLE_DELAY3
+
  enum lp872x_regulator_id {
LP8720_ID_BASE,
LP8720_ID_LDO1 = LP8720_ID_BASE,
@@ -79,12 +82,14 @@ struct lp872x_regulator_data {
   * @update_config : if LP872X_GENERAL_CFG register is updated, set true
   * @regulator_data: platform regulator id and init data
   * @dvs   : dvs data for buck voltage control
+ * @enable_gpio   : gpio pin number for enable control
   */
  struct lp872x_platform_data {
u8 general_config;
bool update_config;
struct lp872x_regulator_data regulator_data[LP872X_MAX_REGULATORS];
struct lp872x_dvs *dvs;
+   int enable_gpio;
  };

  #endif



Best regards,
Milo

Re: [PATCH v2 1/4] regulator: lp872x: Remove warning about invalid DVS GPIO

[Kim, Milo]

On 2/6/2016 3:42 AM, Paul Kocialkowski wrote:

Some devices don't hook the DVS pin to a GPIO but to ground or VCC.
In those cases, it is not a problem to have no DVS GPIO provided, as the current
code will already switch to software-only DVS selection:

When the DVS GPIO is invalid, lp872x_init_dvs jumps to the set_default_dvs_mode
label, which instructs the chip not to use the DVS pin at all and do it all in
software instead (by clearing the LP8720_EXT_DVS_M bit in the
LP872X_GENERAL_CFG register).

That is reflected later in the code, when setting the bucks (the DVS pin only
applies to the bucks) by checking for the LP8720_EXT_DVS_M bit on the
LP872X_GENERAL_CFG register (in lp872x_select_buck_vout_addr) to decide whether
to use software or hardware DVS selection.

Thus, there is no need to print a warning when the DVS GPIO is invalid.

Signed-off-by: Paul Kocialkowski 


Acked-by: Milo Kim 


---
  drivers/regulator/lp872x.c | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c
index 19d7584..21c49d8 100644
--- a/drivers/regulator/lp872x.c
+++ b/drivers/regulator/lp872x.c
@@ -738,10 +738,8 @@ static int lp872x_init_dvs(struct lp872x *lp)
goto set_default_dvs_mode;

gpio = dvs->gpio;
-   if (!gpio_is_valid(gpio)) {
-   dev_warn(lp->dev, "invalid gpio: %d\n", gpio);
+   if (!gpio_is_valid(gpio))
goto set_default_dvs_mode;
-   }

pinstate = dvs->init_state;
ret = devm_gpio_request_one(lp->dev, gpio, pinstate, "LP872X DVS");

Re: [PATCH v2 2/4] regulator: lp872x: Add enable GPIO pin support

[Kim, Milo]

Hi Paul,

Thanks for the patch. Please see my comments below.

On 2/6/2016 3:42 AM, Paul Kocialkowski wrote:

LP872x regulators are made active via the EN pin, which might be hooked to a
GPIO. This adds support for driving the GPIO high when the driver is in use.

Signed-off-by: Paul Kocialkowski 
---
  .../devicetree/bindings/regulator/lp872x.txt   |  1 +
  drivers/regulator/lp872x.c | 34 ++
  include/linux/regulator/lp872x.h   |  5 
  3 files changed, 40 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/lp872x.txt 
b/Documentation/devicetree/bindings/regulator/lp872x.txt
index 7818318..ca58a68 100644
--- a/Documentation/devicetree/bindings/regulator/lp872x.txt
+++ b/Documentation/devicetree/bindings/regulator/lp872x.txt
@@ -28,6 +28,7 @@ Optional properties:
- ti,dvs-gpio: GPIO specifier for external DVS pin control of LP872x 
devices.
- ti,dvs-vsel: DVS selector. 0 = SEL_V1, 1 = SEL_V2.
- ti,dvs-state: initial DVS pin state. 0 = DVS_LOW, 1 = DVS_HIGH.
+  - enable-gpios: GPIO specifier for EN pin control of LP872x devices.

Sub nodes for regulator_init_data
  LP8720 has maximum 6 nodes. (child name: ldo1 ~ 5 and buck)
diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c
index 21c49d8..3899211 100644
--- a/drivers/regulator/lp872x.c
+++ b/drivers/regulator/lp872x.c
@@ -15,6 +15,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -757,6 +758,33 @@ set_default_dvs_mode:
default_dvs_mode[lp->chipid]);
  }

+static int lp872x_hw_enable(struct lp872x *lp)
+{
+   int ret, gpio;
+
+   if (!lp->pdata)
+   return -EINVAL;
+
+   gpio = lp->pdata->enable_gpio;
+   if (!gpio_is_valid(gpio))
+   return 0;


It seems gpio is always 0 when it starts from DT. 'enable_gpio' is 
updated in lp872x_config() but lp872x_hw_enable() is called first.



+
+   /* Always set enable GPIO high. */
+   ret = devm_gpio_request_one(lp->dev, gpio, GPIOF_OUT_INIT_HIGH, "LP872X 
EN");
+   if (ret) {
+   dev_err(lp->dev, "gpio request err: %d\n", ret);
+   return ret;
+   }
+
+   /* Each chip has a different enable delay. */
+   if (lp->chipid == LP8720)
+   usleep_range(LP8720_ENABLE_DELAY, 1.5 * LP8720_ENABLE_DELAY);
+   else
+   usleep_range(LP8725_ENABLE_DELAY, 1.5 * LP8725_ENABLE_DELAY);
+
+   return 0;
+}
+
  static int lp872x_config(struct lp872x *lp)
  {
struct lp872x_platform_data *pdata = lp->pdata;
@@ -875,6 +903,8 @@ static struct lp872x_platform_data
of_property_read_u8(np, "ti,dvs-state", _state);
pdata->dvs->init_state = dvs_state ? DVS_HIGH : DVS_LOW;

+   pdata->enable_gpio = of_get_named_gpio(np, "enable-gpios", 0);


Please move this code to lp872x_populate_pdata_from_dt().
lp872x_hw_enable() is called prior to lp872x_config(), so enable_gpio 
should be set on parsing the DT properties.



+
if (of_get_child_count(np) == 0)
goto out;

@@ -948,6 +978,10 @@ static int lp872x_probe(struct i2c_client *cl, const 
struct i2c_device_id *id)
lp->chipid = id->driver_data;
i2c_set_clientdata(cl, lp);

+   ret = lp872x_hw_enable(lp);
+   if (ret)
+   return ret;
+
ret = lp872x_config(lp);
if (ret)
return ret;
diff --git a/include/linux/regulator/lp872x.h b/include/linux/regulator/lp872x.h
index 132e05c..6029279 100644
--- a/include/linux/regulator/lp872x.h
+++ b/include/linux/regulator/lp872x.h
@@ -18,6 +18,9 @@

  #define LP872X_MAX_REGULATORS 9

+#define LP8720_ENABLE_DELAY200
+#define LP8725_ENABLE_DELAY3
+
  enum lp872x_regulator_id {
LP8720_ID_BASE,
LP8720_ID_LDO1 = LP8720_ID_BASE,
@@ -79,12 +82,14 @@ struct lp872x_regulator_data {
   * @update_config : if LP872X_GENERAL_CFG register is updated, set true
   * @regulator_data: platform regulator id and init data
   * @dvs   : dvs data for buck voltage control
+ * @enable_gpio   : gpio pin number for enable control
   */
  struct lp872x_platform_data {
u8 general_config;
bool update_config;
struct lp872x_regulator_data regulator_data[LP872X_MAX_REGULATORS];
struct lp872x_dvs *dvs;
+   int enable_gpio;
  };

  #endif



Best regards,
Milo

Re: [PATCH v2 1/4] regulator: lp872x: Remove warning about invalid DVS GPIO

[Kim, Milo]

On 2/6/2016 3:42 AM, Paul Kocialkowski wrote:

Some devices don't hook the DVS pin to a GPIO but to ground or VCC.
In those cases, it is not a problem to have no DVS GPIO provided, as the current
code will already switch to software-only DVS selection:

When the DVS GPIO is invalid, lp872x_init_dvs jumps to the set_default_dvs_mode
label, which instructs the chip not to use the DVS pin at all and do it all in
software instead (by clearing the LP8720_EXT_DVS_M bit in the
LP872X_GENERAL_CFG register).

That is reflected later in the code, when setting the bucks (the DVS pin only
applies to the bucks) by checking for the LP8720_EXT_DVS_M bit on the
LP872X_GENERAL_CFG register (in lp872x_select_buck_vout_addr) to decide whether
to use software or hardware DVS selection.

Thus, there is no need to print a warning when the DVS GPIO is invalid.

Signed-off-by: Paul Kocialkowski <cont...@paulk.fr>


Acked-by: Milo Kim <milo@ti.com>


---
  drivers/regulator/lp872x.c | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c
index 19d7584..21c49d8 100644
--- a/drivers/regulator/lp872x.c
+++ b/drivers/regulator/lp872x.c
@@ -738,10 +738,8 @@ static int lp872x_init_dvs(struct lp872x *lp)
goto set_default_dvs_mode;

gpio = dvs->gpio;
-   if (!gpio_is_valid(gpio)) {
-   dev_warn(lp->dev, "invalid gpio: %d\n", gpio);
+   if (!gpio_is_valid(gpio))
goto set_default_dvs_mode;
-   }

pinstate = dvs->init_state;
ret = devm_gpio_request_one(lp->dev, gpio, pinstate, "LP872X DVS");

Re: [PATCH v2 3/9] Documentation: dt-bindings: leds: add LM3633 LED binding information

[Kim, Milo]

Hi Jacek,

On 11/30/2015 9:26 PM, Jacek Anaszewski wrote:

In case of LM3633 the real current outputs are banks and multiplexing
that occurs between banks and LVLEDn pins can be conveniently expressed
with the bindings I proposed above.


>If three output channels are controlled by one control bank, then it
>should be represented as below.
>
>lvled-group-A {
>  led-sources = <0>, <1>, <2>;
>  led-max-microamp = ;
>}
>
>Please let me know if I misunderstand.

You silently assumed some definition of a "channel". led-sources means
in fact current sources, and when device is configured so that three
output pins are routed to the same current source, then in fact
they share common current source.


Thanks for comments. I've modified 'led-sources' based on your feedback.
Could you check updated dt-bindings below?

* Updates
- led-sources: List of current sources from 0 to 5.
- ti,led-outputs: Output channel specifier

The 'ti,led-outputs' property is exactly matched with datasheet 
description, so it's easy to understand which current source is used for 
output channels.

---
TI LMU LM3633 LED device tree bindings

Required properties:
  - compatible: "ti,lm3633-leds"

Child nodes:
  Each node matches with LED control bank.
  Please refer to the datasheet [1].

  Required properties of a child node:
  - led-sources: List of current sources from 0 to 5.
 0: control bank C for output LED 1
control bank C for output LED 1 and 2
control bank C for output LED 1, 2 and 3
 1: control bank D for output LED 2
 2: control bank E for output LED 3
 3: control bank F for output LED 4
control bank F for output LED 4 and 5
control bank F for output LED 4, 5 and 6
 4: control bank G for output LED 5
 5: control bank H for output LED 6
 Please refer to LED binding [2].

  - ti,led-outputs: Output channel specifier from 0 to 5.
0: LED 1
1: LED 2
2: LED 3
3: LED 4
4: LED 5
5: LED 6

  Optional properties of a child node:
  - label: LED channel identification. Please refer to LED binding [2].
  - led-max-microamp: Max current setting. Type is .
  Unit is microampere. Range is from 5000 to 29800.
  Step is 800. Please refer to LED binding [2].

Examples:

LED 1 is assigned for status LED. LED 4,5 and 6 are used for RGB 
indicator. RGB channels are controlled by bank F internally.


leds {
compatible = "ti,lm3633-leds";

status {
led-sources = <0>;
led-max-microamp = <5000>;
ti,led-outputs = <0>;
};

rgb {
led-sources = <3>;
led-max-microamp = <6600>;
ti,led-outputs = <3 4 5>;
};
};

LED 2 is power LED. LED 5 is notification LED.

leds {
compatible = "ti,lm3633-leds";

lvled2 {
label = "power";
led-sources = <1>;
led-max-microamp = <29000>;
ti,led-outputs = <1>;
};

lvled5 {
label = "noti";
led-sources = <4>;
led-max-microamp = <6600>;
ti,led-outputs = <4>;
};
};


[1] http://www.ti.com/product/LM3633/datasheet
[2] ../leds/common.txt

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/9] Documentation: dt-bindings: leds: add LM3633 LED binding information

[Kim, Milo]

Hi Jacek,

On 11/30/2015 9:26 PM, Jacek Anaszewski wrote:

In case of LM3633 the real current outputs are banks and multiplexing
that occurs between banks and LVLEDn pins can be conveniently expressed
with the bindings I proposed above.


>If three output channels are controlled by one control bank, then it
>should be represented as below.
>
>lvled-group-A {
>  led-sources = <0>, <1>, <2>;
>  led-max-microamp = ;
>}
>
>Please let me know if I misunderstand.

You silently assumed some definition of a "channel". led-sources means
in fact current sources, and when device is configured so that three
output pins are routed to the same current source, then in fact
they share common current source.


Thanks for comments. I've modified 'led-sources' based on your feedback.
Could you check updated dt-bindings below?

* Updates
- led-sources: List of current sources from 0 to 5.
- ti,led-outputs: Output channel specifier

The 'ti,led-outputs' property is exactly matched with datasheet 
description, so it's easy to understand which current source is used for 
output channels.

---
TI LMU LM3633 LED device tree bindings

Required properties:
  - compatible: "ti,lm3633-leds"

Child nodes:
  Each node matches with LED control bank.
  Please refer to the datasheet [1].

  Required properties of a child node:
  - led-sources: List of current sources from 0 to 5.
 0: control bank C for output LED 1
control bank C for output LED 1 and 2
control bank C for output LED 1, 2 and 3
 1: control bank D for output LED 2
 2: control bank E for output LED 3
 3: control bank F for output LED 4
control bank F for output LED 4 and 5
control bank F for output LED 4, 5 and 6
 4: control bank G for output LED 5
 5: control bank H for output LED 6
 Please refer to LED binding [2].

  - ti,led-outputs: Output channel specifier from 0 to 5.
0: LED 1
1: LED 2
2: LED 3
3: LED 4
4: LED 5
5: LED 6

  Optional properties of a child node:
  - label: LED channel identification. Please refer to LED binding [2].
  - led-max-microamp: Max current setting. Type is .
  Unit is microampere. Range is from 5000 to 29800.
  Step is 800. Please refer to LED binding [2].

Examples:

LED 1 is assigned for status LED. LED 4,5 and 6 are used for RGB 
indicator. RGB channels are controlled by bank F internally.


leds {
compatible = "ti,lm3633-leds";

status {
led-sources = <0>;
led-max-microamp = <5000>;
ti,led-outputs = <0>;
};

rgb {
led-sources = <3>;
led-max-microamp = <6600>;
ti,led-outputs = <3 4 5>;
};
};

LED 2 is power LED. LED 5 is notification LED.

leds {
compatible = "ti,lm3633-leds";

lvled2 {
label = "power";
led-sources = <1>;
led-max-microamp = <29000>;
ti,led-outputs = <1>;
};

lvled5 {
label = "noti";
led-sources = <4>;
led-max-microamp = <6600>;
ti,led-outputs = <4>;
};
};


[1] http://www.ti.com/product/LM3633/datasheet
[2] ../leds/common.txt

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 8/9] leds: add LM3633 driver

[Kim, Milo]

Hi Jacek,

Thanks for your detailed feedback. Please refer to my comments below.

On 11/27/2015 8:19 PM, Jacek Anaszewski wrote:

Hi Milo,

Thanks for the update. I have few comments below.


diff --git a/drivers/leds/leds-lm3633.c b/drivers/leds/leds-lm3633.c
new file mode 100644
index 000..9c391ca
--- /dev/null
+++ b/drivers/leds/leds-lm3633.c
@@ -0,0 +1,840 @@
+/*
+ * TI LM3633 LED driver
+ *
+ * Copyright 2015 Texas Instruments
+ *
+ * Author: Milo Kim 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define LM3633_MAX_PWM 255
+#define LM3633_MIN_CURRENT 5000
+#define LM3633_MAX_CURRENT 3
+#define LM3633_MAX_PERIOD  9700
+#define LM3633_SHORT_TIMESTEP  16
+#define LM3633_LONG_TIMESTEP   131
+#define LM3633_TIME_OFFSET 61
+#define LM3633_PATTERN_REG_OFFSET  16
+#define LM3633_IMAX_OFFSET 6
+
+enum lm3633_led_bank_id {
+   LM3633_LED_BANK_C,
+   LM3633_LED_BANK_D,
+   LM3633_LED_BANK_E,
+   LM3633_LED_BANK_F,
+   LM3633_LED_BANK_G,
+   LM3633_LED_BANK_H,
+   LM3633_MAX_LEDS,
+};
+
+/**
+ * struct ti_lmu_led_chip
+ *
+ * @dev:   Parent device pointer
+ * @lmu:   LMU structure. Used for register R/W access.
+ * @lock:  Secure handling for multiple user interface access
+ * @lmu_led:   Multiple LED strings


Could you clarify what "string" means here?


+ * @num_leds:  Number of LED strings


and here?


Oops! I forgot to replace this term with 'output channel'. Thanks for 
catching this.



+static u8 lm3633_convert_time_to_index(unsigned int msec)
+{
+   u8 idx, offset;
+
+   /*
+* Find an approximate index


I think that we shouldn't approximate but clamp the msec
to the nearest possible device setting.


+*
+*  0 <= time <= 1000 : 16ms step
+*   1000 <  time <= 9700 : 131ms step, base index is 61
+*/
+
+   msec = min_t(int, msec, LM3633_MAX_PERIOD);
+
+   if (msec <= 1000) {
+   idx = msec / LM3633_SHORT_TIMESTEP;
+   if (idx > 1)
+   idx--;
+   offset = 0;
+   } else {
+   idx = (msec - 1000) / LM3633_LONG_TIMESTEP;
+   offset = LM3633_TIME_OFFSET;
+   }
+
+   return idx + offset;
+}
+
+static u8 lm3633_convert_ramp_to_index(unsigned int msec)
+{
+   const int ramp_table[] = { 2, 250, 500, 1000, 2000, 4000, 8000, 16000 };
+   int size = ARRAY_SIZE(ramp_table);
+   int i;
+
+   if (msec <= ramp_table[0])
+   return 0;
+
+   if (msec > ramp_table[size - 1])
+   return size - 1;
+
+   for (i = 1; i < size; i++) {
+   if (msec == ramp_table[i])
+   return i;
+
+   /* Find an approximate index by looking up the table */


Similarly here. So you should have an array of the possible msec
values and iterate through it to look for the nearest one.


Driver uses 'ramp_table[]' for this purpose. Could you describe it in 
more details?



+
+static int lm3633_led_brightness_set(struct led_classdev *cdev,
+enum led_brightness brightness)
+{
+   struct ti_lmu_led *lmu_led = container_of(cdev, struct ti_lmu_led,
+ cdev);
+   struct ti_lmu_led_chip *chip = lmu_led->chip;
+   struct regmap *regmap = chip->lmu->regmap;
+   u8 reg = LM3633_REG_BRT_LVLED_BASE + lmu_led->bank_id;
+   int ret;
+
+   mutex_lock(>lock);
+
+   ret = regmap_write(regmap, reg, brightness);
+   if (ret) {
+   mutex_unlock(>lock);
+   return ret;
+   }
+
+   if (brightness == 0)
+   lm3633_led_disable_bank(lmu_led);


This should be checked at first, as I suppose that disabling
a bank suffices to turn the LED off.


Well, it's not a big deal when turn off the LED. However, our silicon 
designer recommended brightness update should be done prior to enabling 
LED bank. Let me share some block diagram.


[I2C logic] - [brightness control] - [control bank] - [output channel]

If control bank is enabled first, then previous brightness value in 
register can be used instead of new updated brightness. So brightness 
update should be done first.



+static u8 lm3633_led_scale_max_brightness(struct ti_lmu_led *lmu_led, u32 imax)
+{
+   u8 max_current = lm3633_led_convert_current_to_index(imax);
+   const u8 max_brightness_table[] = {
+   [LMU_IMAX_5mA]  = 191,
+   [LMU_IMAX_6mA]  = 197,
+   

Re: [PATCH v2 3/9] Documentation: dt-bindings: leds: add LM3633 LED binding information

[Kim, Milo]

Hi Jacek,

On 11/27/2015 8:19 PM, Jacek Anaszewski wrote:

Hi Milo,

On 11/26/2015 07:56 AM, Milo Kim wrote:

LM3633 LED device is one of TI LMU device list.

Cc: Rob Herring 
Cc: devicet...@vger.kernel.org
Cc: Lee Jones 
Cc: Jacek Anaszewski 
Cc: Mark Brown 
Cc: linux-l...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Milo Kim 
---
   .../devicetree/bindings/leds/leds-lm3633.txt   | 24 
++
   1 file changed, 24 insertions(+)
   create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3633.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-lm3633.txt 
b/Documentation/devicetree/bindings/leds/leds-lm3633.txt
new file mode 100644
index 000..a553894
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-lm3633.txt
@@ -0,0 +1,24 @@
+TI LMU LM3633 LED device tree bindings
+
+Required properties:
+  - compatible: "ti,lm3633-leds"
+
+Child nodes:
+  Each node matches with LED control bank.
+  Please refer to the datasheet [1].


leds/common.txt documentation says that child nodes represent discrete
LED elements.


+  Required properties of a child node:
+  - led-sources: List of enabled channels from 0 to 5.
+ Please refer to LED binding [2].


led-sources property should contain always one element -
a control bank identifier that the iout is to be associated with.
For example, if there are three LEDs and they should be controlled
with control bank A, then the bindings should look as follows
(assuming that control bank identifiers start from 0 [A:0, B:1,
C:2, etc. - it has to be also explicitly stated in the documentation]:


My understanding is 'led-sources' means output channel rather than 
control bank. Output channel is visible and intuitive - just output LED.

On the other hand, control banks works inside the silicon, LM3633.
I'm not sure other LED devices have control bank or not, but output 
channel is common concept.




lvled1 {
led-sources = <2>;
led-max-microamp = <1000>;
}

lvled2 {
led-sources = <2>;
led-max-microamp = <29000>;
}

lvled3 {
led-sources = <2>;
led-max-microamp = <7000>;
}


For this reason, I don't understand this LED configuration - one output 
channel with multiple LED devices. LED child node should be matched with 
led class device.
If three output channels are controlled by one control bank, then it 
should be represented as below.


lvled-group-A {
led-sources = <0>, <1>, <2>;
led-max-microamp = ;
}

Please let me know if I misunderstand.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/9] Documentation: dt-bindings: leds: add LM3633 LED binding information

[Kim, Milo]

Hi Jacek,

On 11/27/2015 8:19 PM, Jacek Anaszewski wrote:

Hi Milo,

On 11/26/2015 07:56 AM, Milo Kim wrote:

LM3633 LED device is one of TI LMU device list.

Cc: Rob Herring <robh...@kernel.org>
Cc: devicet...@vger.kernel.org
Cc: Lee Jones <lee.jo...@linaro.org>
Cc: Jacek Anaszewski <j.anaszew...@samsung.com>
Cc: Mark Brown <broo...@kernel.org>
Cc: linux-l...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Milo Kim <milo@ti.com>
---
   .../devicetree/bindings/leds/leds-lm3633.txt   | 24 
++
   1 file changed, 24 insertions(+)
   create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3633.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-lm3633.txt 
b/Documentation/devicetree/bindings/leds/leds-lm3633.txt
new file mode 100644
index 000..a553894
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-lm3633.txt
@@ -0,0 +1,24 @@
+TI LMU LM3633 LED device tree bindings
+
+Required properties:
+  - compatible: "ti,lm3633-leds"
+
+Child nodes:
+  Each node matches with LED control bank.
+  Please refer to the datasheet [1].


leds/common.txt documentation says that child nodes represent discrete
LED elements.


+  Required properties of a child node:
+  - led-sources: List of enabled channels from 0 to 5.
+ Please refer to LED binding [2].


led-sources property should contain always one element -
a control bank identifier that the iout is to be associated with.
For example, if there are three LEDs and they should be controlled
with control bank A, then the bindings should look as follows
(assuming that control bank identifiers start from 0 [A:0, B:1,
C:2, etc. - it has to be also explicitly stated in the documentation]:


My understanding is 'led-sources' means output channel rather than 
control bank. Output channel is visible and intuitive - just output LED.

On the other hand, control banks works inside the silicon, LM3633.
I'm not sure other LED devices have control bank or not, but output 
channel is common concept.




lvled1 {
led-sources = <2>;
led-max-microamp = <1000>;
}

lvled2 {
led-sources = <2>;
led-max-microamp = <29000>;
}

lvled3 {
led-sources = <2>;
led-max-microamp = <7000>;
}


For this reason, I don't understand this LED configuration - one output 
channel with multiple LED devices. LED child node should be matched with 
led class device.
If three output channels are controlled by one control bank, then it 
should be represented as below.


lvled-group-A {
led-sources = <0>, <1>, <2>;
led-max-microamp = ;
}

Please let me know if I misunderstand.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 8/9] leds: add LM3633 driver

[Kim, Milo]

Hi Jacek,

Thanks for your detailed feedback. Please refer to my comments below.

On 11/27/2015 8:19 PM, Jacek Anaszewski wrote:

Hi Milo,

Thanks for the update. I have few comments below.


diff --git a/drivers/leds/leds-lm3633.c b/drivers/leds/leds-lm3633.c
new file mode 100644
index 000..9c391ca
--- /dev/null
+++ b/drivers/leds/leds-lm3633.c
@@ -0,0 +1,840 @@
+/*
+ * TI LM3633 LED driver
+ *
+ * Copyright 2015 Texas Instruments
+ *
+ * Author: Milo Kim <milo@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define LM3633_MAX_PWM 255
+#define LM3633_MIN_CURRENT 5000
+#define LM3633_MAX_CURRENT 3
+#define LM3633_MAX_PERIOD  9700
+#define LM3633_SHORT_TIMESTEP  16
+#define LM3633_LONG_TIMESTEP   131
+#define LM3633_TIME_OFFSET 61
+#define LM3633_PATTERN_REG_OFFSET  16
+#define LM3633_IMAX_OFFSET 6
+
+enum lm3633_led_bank_id {
+   LM3633_LED_BANK_C,
+   LM3633_LED_BANK_D,
+   LM3633_LED_BANK_E,
+   LM3633_LED_BANK_F,
+   LM3633_LED_BANK_G,
+   LM3633_LED_BANK_H,
+   LM3633_MAX_LEDS,
+};
+
+/**
+ * struct ti_lmu_led_chip
+ *
+ * @dev:   Parent device pointer
+ * @lmu:   LMU structure. Used for register R/W access.
+ * @lock:  Secure handling for multiple user interface access
+ * @lmu_led:   Multiple LED strings


Could you clarify what "string" means here?


+ * @num_leds:  Number of LED strings


and here?


Oops! I forgot to replace this term with 'output channel'. Thanks for 
catching this.



+static u8 lm3633_convert_time_to_index(unsigned int msec)
+{
+   u8 idx, offset;
+
+   /*
+* Find an approximate index


I think that we shouldn't approximate but clamp the msec
to the nearest possible device setting.


+*
+*  0 <= time <= 1000 : 16ms step
+*   1000 <  time <= 9700 : 131ms step, base index is 61
+*/
+
+   msec = min_t(int, msec, LM3633_MAX_PERIOD);
+
+   if (msec <= 1000) {
+   idx = msec / LM3633_SHORT_TIMESTEP;
+   if (idx > 1)
+   idx--;
+   offset = 0;
+   } else {
+   idx = (msec - 1000) / LM3633_LONG_TIMESTEP;
+   offset = LM3633_TIME_OFFSET;
+   }
+
+   return idx + offset;
+}
+
+static u8 lm3633_convert_ramp_to_index(unsigned int msec)
+{
+   const int ramp_table[] = { 2, 250, 500, 1000, 2000, 4000, 8000, 16000 };
+   int size = ARRAY_SIZE(ramp_table);
+   int i;
+
+   if (msec <= ramp_table[0])
+   return 0;
+
+   if (msec > ramp_table[size - 1])
+   return size - 1;
+
+   for (i = 1; i < size; i++) {
+   if (msec == ramp_table[i])
+   return i;
+
+   /* Find an approximate index by looking up the table */


Similarly here. So you should have an array of the possible msec
values and iterate through it to look for the nearest one.


Driver uses 'ramp_table[]' for this purpose. Could you describe it in 
more details?



+
+static int lm3633_led_brightness_set(struct led_classdev *cdev,
+enum led_brightness brightness)
+{
+   struct ti_lmu_led *lmu_led = container_of(cdev, struct ti_lmu_led,
+ cdev);
+   struct ti_lmu_led_chip *chip = lmu_led->chip;
+   struct regmap *regmap = chip->lmu->regmap;
+   u8 reg = LM3633_REG_BRT_LVLED_BASE + lmu_led->bank_id;
+   int ret;
+
+   mutex_lock(>lock);
+
+   ret = regmap_write(regmap, reg, brightness);
+   if (ret) {
+   mutex_unlock(>lock);
+   return ret;
+   }
+
+   if (brightness == 0)
+   lm3633_led_disable_bank(lmu_led);


This should be checked at first, as I suppose that disabling
a bank suffices to turn the LED off.


Well, it's not a big deal when turn off the LED. However, our silicon 
designer recommended brightness update should be done prior to enabling 
LED bank. Let me share some block diagram.


[I2C logic] - [brightness control] - [control bank] - [output channel]

If control bank is enabled first, then previous brightness value in 
register can be used instead of new updated brightness. So brightness 
update should be done first.



+static u8 lm3633_led_scale_max_brightness(struct ti_lmu_led *lmu_led, u32 imax)
+{
+   u8 max_current = lm3633_led_convert_current_to_index(imax);
+   const u8 max_brightness_table[] = {
+  

Re: [PATCH RESEND 00/16] Support TI LMU devices

[Kim, Milo]

On 11/3/2015 5:33 PM, Lee Jones wrote:

On Tue, 03 Nov 2015, Kim, Milo wrote:


Hi Lee,

On 11/2/2015 5:59 PM, Lee Jones wrote:

  drivers/video/backlight/Kconfig|  62 ++

  drivers/video/backlight/Makefile   |   7 +
  drivers/video/backlight/lm3532_bl.c| 183 +
  drivers/video/backlight/lm3631_bl.c| 129 
  drivers/video/backlight/lm3632_bl.c| 125 
  drivers/video/backlight/lm3633_bl.c| 210 ++
  drivers/video/backlight/lm3695_bl.c|  91 +++
  drivers/video/backlight/lm3697_bl.c| 187 +
  drivers/video/backlight/ti-lmu-backlight.c | 429 
  drivers/video/backlight/ti-lmu-backlight.h | 152 +

How different are all of these drivers?

Can you create one driver that supports them all instead?



Thanks for your suggestion.

'ti-lmu-backlight' is the common part of lm_bl drivers. And each
lmxxx_bl has its own operation functions by using ti_lmu_bl_ops.
I've tried to make consolidated driver but it contained too much
device specific code in one file. So I prefer simple drivers
structure - 'common part' and 'device specific operations'.
It would be appreciated if you could introduce better idea.


I wish to avoid having to apply the patches to conduct my own analysis
of the files, as I am severely restricted on time.  Can you tell me how
much duplicated code there is between the files?  How many lines would
be saved by supporting all of the lm* drivers in a single file?



Now, consolidated driver is ready. It improves not only lines of code 
but also code size. Patch v2 will be sent soon.


Lines of code
-
194 lines are saved in consolidated driver.

Patch v1: 1420 (8 files*)
Patch v2: 1226 (3 files**)

*   ti-lmu-backlight.c (429)
ti-lmu-backlight.h (152)
lm3532_bl.c (183)
lm3631_bl.c (129)
lm3632_bl.c (125)
lm3633_bl.c (210)
lm3695_bl.c (95)
lm3697_bl.c (97)

**  ti-lmu-backlight-core.c (649)
ti-lmu-backlight-data.c (287)
ti-lmu-backlight.h (290)

Size

The text segment is decreased by removing duplicate instructions.

Patch v1
   textdata bss dec hex filename
  12202 720  40   1296232a2 drivers/video/backlight/built-in.o

Patch v2
   textdata bss dec hex filename
   6883 712  4176361dd4 drivers/video/backlight/built-in.o

Test environment

- Kernel version: 4.4.0-rc2 (linux-next)
- backlight/built-in.o includes backlight core and ti-lmu-backlight 
drivers. No other backlight drivers included.


Change details will be described in the patch-set.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 06/16] mfd: add TI LMU driver

[Kim, Milo]

On 11/24/2015 5:18 PM, Lee Jones wrote:

On Tue, 24 Nov 2015, Kim, Milo wrote:


On 11/24/2015 11:35 AM, Kim, Milo wrote:

Hi Lee,

Thanks for all your comments. Please see my comments below.

On 11/23/2015 7:30 PM, Lee Jones wrote:

+int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)

+{
+   int ret;
+   unsigned int val;
+
+   ret = regmap_read(lmu->regmap, reg, );
+   if (ret < 0)
+   return ret;
+
+   *read = (u8)val;
+   return 0;
+}
+EXPORT_SYMBOL_GPL(ti_lmu_read_byte);

It doesn't get much more simple than this.

What's the purpose of abstracting it?


+int ti_lmu_write_byte(struct ti_lmu *lmu, u8 reg, u8 data)
+{
+   return regmap_write(lmu->regmap, reg, data);
+}
+EXPORT_SYMBOL_GPL(ti_lmu_write_byte);
+
+int ti_lmu_update_bits(struct ti_lmu *lmu, u8 reg, u8 mask, u8 data)
+{
+   return regmap_update_bits(lmu->regmap, reg, mask, data);
+}
+EXPORT_SYMBOL_GPL(ti_lmu_update_bits);

Okay, I lied, it does get more simple.

Seems like abstraction for the sake of abstraction here.

Feel free to try and convince me otherwise.



The main reason was that LMU MFD core provides consistent register
access among LMU drivers like ti-lmu-backlight, leds-lm3633,
lm363x-regulator and ti-lmu-fault-monitor('ti-lmu-hwmon' will be changed
to this in the 2nd patch).

However, LMU register helpers are exactly same as regmap interface
except using ti_lmu data structure. So let me replace them with regmap
functions. Thanks for pointing this out.


I just realized LMU MFD core helpers also provide module
dependencies by using EXPORT_SYMBOL_GPL().

# modprobe -D ti-lmu-backlight
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
# modprobe -D ti-lmu-fault-monitor
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
# modprobe -D lm363x-regulator
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
# modprobe -D leds-lm3633
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko

ti-lmu.ko should be loaded first because it has hardware enable pin
control code. Four other drivers have dependency on this module.
Without EXPORT_SYMBOL_GPL(), this dependency will be gone like
below.

# modprobe -D ti-lmu-backlight
insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
# modprobe -D ti-lmu-fault-monitor
insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
# modprobe -D lm363x-regulator
insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
# modprobe -D leds-lm3633
insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko

If LMU MFD core module is not loaded and other modules call regmap
helpers, then loaded drivers will not work because hardware is not
enabled yet.

So I'd like to keep LMU MFD helpers for module dependencies.
Additionally, I'll modify 'ti_lmu_read_byte()' as follows.

int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
{
return regmap_read(lmu->regmap, reg, (unsigned int *)read);
}
EXPORT_SYMBOL_GPL(ti_lmu_read_byte);

Please let me know if you have better idea.


You're keeping the helpers for the wrong reasons.  This has now become
a hack.  If you have dependencies between modules, either use the init
levels or defer probe in the usual way.



Yes, I was wrong. In case LMU MFD core is not loaded, other LMU modules 
like ti-lmu-backlight isn't probed because no platform device (mfd 
device) exists. So the situation which I've mentioned never happens.
And if GPIO controller is not ready and HW enable GPIO request gets 
failed on LMU MFD initialization, this will be processed later because 
it returns as error code '-EPROBE_DEFER' from GPIO subsystem. In other 
words, there is no dependency issue between LMU modules. Those are just 
platform device and driver.


OK, I'll remove LMU register helpers in the 2nd patch. Then, each LMU 
driver will call regmap helpers directly.

Thanks for your comments.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 00/16] Support TI LMU devices

[Kim, Milo]

On 11/3/2015 5:33 PM, Lee Jones wrote:

On Tue, 03 Nov 2015, Kim, Milo wrote:


Hi Lee,

On 11/2/2015 5:59 PM, Lee Jones wrote:

  drivers/video/backlight/Kconfig|  62 ++

  drivers/video/backlight/Makefile   |   7 +
  drivers/video/backlight/lm3532_bl.c| 183 +
  drivers/video/backlight/lm3631_bl.c| 129 
  drivers/video/backlight/lm3632_bl.c| 125 
  drivers/video/backlight/lm3633_bl.c| 210 ++
  drivers/video/backlight/lm3695_bl.c|  91 +++
  drivers/video/backlight/lm3697_bl.c| 187 +
  drivers/video/backlight/ti-lmu-backlight.c | 429 
  drivers/video/backlight/ti-lmu-backlight.h | 152 +

How different are all of these drivers?

Can you create one driver that supports them all instead?



Thanks for your suggestion.

'ti-lmu-backlight' is the common part of lm_bl drivers. And each
lmxxx_bl has its own operation functions by using ti_lmu_bl_ops.
I've tried to make consolidated driver but it contained too much
device specific code in one file. So I prefer simple drivers
structure - 'common part' and 'device specific operations'.
It would be appreciated if you could introduce better idea.


I wish to avoid having to apply the patches to conduct my own analysis
of the files, as I am severely restricted on time.  Can you tell me how
much duplicated code there is between the files?  How many lines would
be saved by supporting all of the lm* drivers in a single file?



Now, consolidated driver is ready. It improves not only lines of code 
but also code size. Patch v2 will be sent soon.


Lines of code
-
194 lines are saved in consolidated driver.

Patch v1: 1420 (8 files*)
Patch v2: 1226 (3 files**)

*   ti-lmu-backlight.c (429)
ti-lmu-backlight.h (152)
lm3532_bl.c (183)
lm3631_bl.c (129)
lm3632_bl.c (125)
lm3633_bl.c (210)
lm3695_bl.c (95)
lm3697_bl.c (97)

**  ti-lmu-backlight-core.c (649)
ti-lmu-backlight-data.c (287)
ti-lmu-backlight.h (290)

Size

The text segment is decreased by removing duplicate instructions.

Patch v1
   textdata bss dec hex filename
  12202 720  40   1296232a2 drivers/video/backlight/built-in.o

Patch v2
   textdata bss dec hex filename
   6883 712  4176361dd4 drivers/video/backlight/built-in.o

Test environment

- Kernel version: 4.4.0-rc2 (linux-next)
- backlight/built-in.o includes backlight core and ti-lmu-backlight 
drivers. No other backlight drivers included.


Change details will be described in the patch-set.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 06/16] mfd: add TI LMU driver

[Kim, Milo]

On 11/24/2015 5:18 PM, Lee Jones wrote:

On Tue, 24 Nov 2015, Kim, Milo wrote:


On 11/24/2015 11:35 AM, Kim, Milo wrote:

Hi Lee,

Thanks for all your comments. Please see my comments below.

On 11/23/2015 7:30 PM, Lee Jones wrote:

+int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)

+{
+   int ret;
+   unsigned int val;
+
+   ret = regmap_read(lmu->regmap, reg, );
+   if (ret < 0)
+   return ret;
+
+   *read = (u8)val;
+   return 0;
+}
+EXPORT_SYMBOL_GPL(ti_lmu_read_byte);

It doesn't get much more simple than this.

What's the purpose of abstracting it?


+int ti_lmu_write_byte(struct ti_lmu *lmu, u8 reg, u8 data)
+{
+   return regmap_write(lmu->regmap, reg, data);
+}
+EXPORT_SYMBOL_GPL(ti_lmu_write_byte);
+
+int ti_lmu_update_bits(struct ti_lmu *lmu, u8 reg, u8 mask, u8 data)
+{
+   return regmap_update_bits(lmu->regmap, reg, mask, data);
+}
+EXPORT_SYMBOL_GPL(ti_lmu_update_bits);

Okay, I lied, it does get more simple.

Seems like abstraction for the sake of abstraction here.

Feel free to try and convince me otherwise.



The main reason was that LMU MFD core provides consistent register
access among LMU drivers like ti-lmu-backlight, leds-lm3633,
lm363x-regulator and ti-lmu-fault-monitor('ti-lmu-hwmon' will be changed
to this in the 2nd patch).

However, LMU register helpers are exactly same as regmap interface
except using ti_lmu data structure. So let me replace them with regmap
functions. Thanks for pointing this out.


I just realized LMU MFD core helpers also provide module
dependencies by using EXPORT_SYMBOL_GPL().

# modprobe -D ti-lmu-backlight
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
# modprobe -D ti-lmu-fault-monitor
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
# modprobe -D lm363x-regulator
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
# modprobe -D leds-lm3633
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko

ti-lmu.ko should be loaded first because it has hardware enable pin
control code. Four other drivers have dependency on this module.
Without EXPORT_SYMBOL_GPL(), this dependency will be gone like
below.

# modprobe -D ti-lmu-backlight
insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
# modprobe -D ti-lmu-fault-monitor
insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
# modprobe -D lm363x-regulator
insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
# modprobe -D leds-lm3633
insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko

If LMU MFD core module is not loaded and other modules call regmap
helpers, then loaded drivers will not work because hardware is not
enabled yet.

So I'd like to keep LMU MFD helpers for module dependencies.
Additionally, I'll modify 'ti_lmu_read_byte()' as follows.

int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
{
return regmap_read(lmu->regmap, reg, (unsigned int *)read);
}
EXPORT_SYMBOL_GPL(ti_lmu_read_byte);

Please let me know if you have better idea.


You're keeping the helpers for the wrong reasons.  This has now become
a hack.  If you have dependencies between modules, either use the init
levels or defer probe in the usual way.



Yes, I was wrong. In case LMU MFD core is not loaded, other LMU modules 
like ti-lmu-backlight isn't probed because no platform device (mfd 
device) exists. So the situation which I've mentioned never happens.
And if GPIO controller is not ready and HW enable GPIO request gets 
failed on LMU MFD initialization, this will be processed later because 
it returns as error code '-EPROBE_DEFER' from GPIO subsystem. In other 
words, there is no dependency issue between LMU modules. Those are just 
platform device and driver.


OK, I'll remove LMU register helpers in the 2nd patch. Then, each LMU 
driver will call regmap helpers directly.

Thanks for your comments.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 06/16] mfd: add TI LMU driver

[Kim, Milo]

On 11/24/2015 11:35 AM, Kim, Milo wrote:

Hi Lee,

Thanks for all your comments. Please see my comments below.

On 11/23/2015 7:30 PM, Lee Jones wrote:

+int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)

+{
+   int ret;
+   unsigned int val;
+
+   ret = regmap_read(lmu->regmap, reg, );
+   if (ret < 0)
+   return ret;
+
+   *read = (u8)val;
+   return 0;
+}
+EXPORT_SYMBOL_GPL(ti_lmu_read_byte);

It doesn't get much more simple than this.

What's the purpose of abstracting it?


+int ti_lmu_write_byte(struct ti_lmu *lmu, u8 reg, u8 data)
+{
+   return regmap_write(lmu->regmap, reg, data);
+}
+EXPORT_SYMBOL_GPL(ti_lmu_write_byte);
+
+int ti_lmu_update_bits(struct ti_lmu *lmu, u8 reg, u8 mask, u8 data)
+{
+   return regmap_update_bits(lmu->regmap, reg, mask, data);
+}
+EXPORT_SYMBOL_GPL(ti_lmu_update_bits);

Okay, I lied, it does get more simple.

Seems like abstraction for the sake of abstraction here.

Feel free to try and convince me otherwise.



The main reason was that LMU MFD core provides consistent register
access among LMU drivers like ti-lmu-backlight, leds-lm3633,
lm363x-regulator and ti-lmu-fault-monitor('ti-lmu-hwmon' will be changed
to this in the 2nd patch).

However, LMU register helpers are exactly same as regmap interface
except using ti_lmu data structure. So let me replace them with regmap
functions. Thanks for pointing this out.


I just realized LMU MFD core helpers also provide module dependencies by 
using EXPORT_SYMBOL_GPL().


# modprobe -D ti-lmu-backlight
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
# modprobe -D ti-lmu-fault-monitor
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
# modprobe -D lm363x-regulator
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
# modprobe -D leds-lm3633
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko

ti-lmu.ko should be loaded first because it has hardware enable pin 
control code. Four other drivers have dependency on this module. Without 
EXPORT_SYMBOL_GPL(), this dependency will be gone like below.


# modprobe -D ti-lmu-backlight
insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
# modprobe -D ti-lmu-fault-monitor
insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
# modprobe -D lm363x-regulator
insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
# modprobe -D leds-lm3633
insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko

If LMU MFD core module is not loaded and other modules call regmap 
helpers, then loaded drivers will not work because hardware is not 
enabled yet.


So I'd like to keep LMU MFD helpers for module dependencies. 
Additionally, I'll modify 'ti_lmu_read_byte()' as follows.


int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
{
return regmap_read(lmu->regmap, reg, (unsigned int *)read);
}
EXPORT_SYMBOL_GPL(ti_lmu_read_byte);

Please let me know if you have better idea.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 06/16] mfd: add TI LMU driver

[Kim, Milo]

Hi Lee,

Thanks for all your comments. Please see my comments below.

On 11/23/2015 7:30 PM, Lee Jones wrote:

+int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
>+{
>+   int ret;
>+   unsigned int val;
>+
>+   ret = regmap_read(lmu->regmap, reg, );
>+   if (ret < 0)
>+   return ret;
>+
>+   *read = (u8)val;
>+   return 0;
>+}
>+EXPORT_SYMBOL_GPL(ti_lmu_read_byte);

It doesn't get much more simple than this.

What's the purpose of abstracting it?


>+int ti_lmu_write_byte(struct ti_lmu *lmu, u8 reg, u8 data)
>+{
>+   return regmap_write(lmu->regmap, reg, data);
>+}
>+EXPORT_SYMBOL_GPL(ti_lmu_write_byte);
>+
>+int ti_lmu_update_bits(struct ti_lmu *lmu, u8 reg, u8 mask, u8 data)
>+{
>+   return regmap_update_bits(lmu->regmap, reg, mask, data);
>+}
>+EXPORT_SYMBOL_GPL(ti_lmu_update_bits);

Okay, I lied, it does get more simple.

Seems like abstraction for the sake of abstraction here.

Feel free to try and convince me otherwise.



The main reason was that LMU MFD core provides consistent register 
access among LMU drivers like ti-lmu-backlight, leds-lm3633, 
lm363x-regulator and ti-lmu-fault-monitor('ti-lmu-hwmon' will be changed 
to this in the 2nd patch).


However, LMU register helpers are exactly same as regmap interface 
except using ti_lmu data structure. So let me replace them with regmap 
functions. Thanks for pointing this out.


Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 06/16] mfd: add TI LMU driver

[Kim, Milo]

On 11/24/2015 11:35 AM, Kim, Milo wrote:

Hi Lee,

Thanks for all your comments. Please see my comments below.

On 11/23/2015 7:30 PM, Lee Jones wrote:

+int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)

+{
+   int ret;
+   unsigned int val;
+
+   ret = regmap_read(lmu->regmap, reg, );
+   if (ret < 0)
+   return ret;
+
+   *read = (u8)val;
+   return 0;
+}
+EXPORT_SYMBOL_GPL(ti_lmu_read_byte);

It doesn't get much more simple than this.

What's the purpose of abstracting it?


+int ti_lmu_write_byte(struct ti_lmu *lmu, u8 reg, u8 data)
+{
+   return regmap_write(lmu->regmap, reg, data);
+}
+EXPORT_SYMBOL_GPL(ti_lmu_write_byte);
+
+int ti_lmu_update_bits(struct ti_lmu *lmu, u8 reg, u8 mask, u8 data)
+{
+   return regmap_update_bits(lmu->regmap, reg, mask, data);
+}
+EXPORT_SYMBOL_GPL(ti_lmu_update_bits);

Okay, I lied, it does get more simple.

Seems like abstraction for the sake of abstraction here.

Feel free to try and convince me otherwise.



The main reason was that LMU MFD core provides consistent register
access among LMU drivers like ti-lmu-backlight, leds-lm3633,
lm363x-regulator and ti-lmu-fault-monitor('ti-lmu-hwmon' will be changed
to this in the 2nd patch).

However, LMU register helpers are exactly same as regmap interface
except using ti_lmu data structure. So let me replace them with regmap
functions. Thanks for pointing this out.


I just realized LMU MFD core helpers also provide module dependencies by 
using EXPORT_SYMBOL_GPL().


# modprobe -D ti-lmu-backlight
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
# modprobe -D ti-lmu-fault-monitor
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
# modprobe -D lm363x-regulator
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
# modprobe -D leds-lm3633
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko

ti-lmu.ko should be loaded first because it has hardware enable pin 
control code. Four other drivers have dependency on this module. Without 
EXPORT_SYMBOL_GPL(), this dependency will be gone like below.


# modprobe -D ti-lmu-backlight
insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
# modprobe -D ti-lmu-fault-monitor
insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
# modprobe -D lm363x-regulator
insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
# modprobe -D leds-lm3633
insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko

If LMU MFD core module is not loaded and other modules call regmap 
helpers, then loaded drivers will not work because hardware is not 
enabled yet.


So I'd like to keep LMU MFD helpers for module dependencies. 
Additionally, I'll modify 'ti_lmu_read_byte()' as follows.


int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
{
return regmap_read(lmu->regmap, reg, (unsigned int *)read);
}
EXPORT_SYMBOL_GPL(ti_lmu_read_byte);

Please let me know if you have better idea.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 06/16] mfd: add TI LMU driver

[Kim, Milo]

Hi Lee,

Thanks for all your comments. Please see my comments below.

On 11/23/2015 7:30 PM, Lee Jones wrote:

+int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
>+{
>+   int ret;
>+   unsigned int val;
>+
>+   ret = regmap_read(lmu->regmap, reg, );
>+   if (ret < 0)
>+   return ret;
>+
>+   *read = (u8)val;
>+   return 0;
>+}
>+EXPORT_SYMBOL_GPL(ti_lmu_read_byte);

It doesn't get much more simple than this.

What's the purpose of abstracting it?


>+int ti_lmu_write_byte(struct ti_lmu *lmu, u8 reg, u8 data)
>+{
>+   return regmap_write(lmu->regmap, reg, data);
>+}
>+EXPORT_SYMBOL_GPL(ti_lmu_write_byte);
>+
>+int ti_lmu_update_bits(struct ti_lmu *lmu, u8 reg, u8 mask, u8 data)
>+{
>+   return regmap_update_bits(lmu->regmap, reg, mask, data);
>+}
>+EXPORT_SYMBOL_GPL(ti_lmu_update_bits);

Okay, I lied, it does get more simple.

Seems like abstraction for the sake of abstraction here.

Feel free to try and convince me otherwise.



The main reason was that LMU MFD core provides consistent register 
access among LMU drivers like ti-lmu-backlight, leds-lm3633, 
lm363x-regulator and ti-lmu-fault-monitor('ti-lmu-hwmon' will be changed 
to this in the 2nd patch).


However, LMU register helpers are exactly same as regmap interface 
except using ti_lmu data structure. So let me replace them with regmap 
functions. Thanks for pointing this out.


Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 15/16] leds: add LM3633 driver

[Kim, Milo]

Hi Jacek,

On 11/20/2015 6:22 PM, Jacek Anaszewski wrote:

On 11/10/2015 08:38 AM, Kim, Milo wrote:
[...]

+cat /sys/class/leds//pattern_levels
+low brightness: 0, high brightness: 255
+
+What:/sys/class/leds//run_pattern
+Date:Oct 2015
+KernelVersion:4.3
+Contact:Milo Kim 
+Description:write only
+After 'pattern_times' and 'pattern_levels' are updated,
+run the pattern by writing 1 to 'run_pattern'.
+To stop running pattern, writes 0 to 'run_pattern'.


I wonder how registering an in-driver trigger would work. It would
allow for hiding above pattern attributes when the trigger is inactive,
and thus making the sysfs interface more transparent. You could avoid
the need for run_pattern attribute, as setting the trigger would itself
activate the pattern, and setting brightness to 0 would turn it off.


I like this idea, let me try to fix it.


After thinking it over, I came to conclusion that implementing it as
an in-driver trigger is not a proper way to go, since triggers are
defined as kernel based source of LED events.

This is somehow abused in case of timer trigger which takes hardware
blinking feature as a first choice and applies software blinking as
a fallback only. To be consistent with that, we could go for adding
generic pattern trigger and add a led_pattern_set() API, similarly
to existing led_blink_set().

The problem is that different LED controllers may implement blinking
patterns that are configured with different set of parameters. This
subject would definitely require thorough analysis.

For now, please just expose pattern settings as separate sysfs
attributes of a LED class device.



Thanks for your suggestion.
Then, LM3633 LED driver will support 8 device attributes.

pattern_time_delay
pattern_time_rise
pattern_time_high
pattern_time_fall
pattern_time_low
pattern_brightness_low
pattern_brightness_high
pattern_run_pattern

Details will be updated in Documentation/ABI/testing/sysfs-class-led-lm3633.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 15/16] leds: add LM3633 driver

[Kim, Milo]

Hi Jacek,

On 11/20/2015 6:22 PM, Jacek Anaszewski wrote:

On 11/10/2015 08:38 AM, Kim, Milo wrote:
[...]

+cat /sys/class/leds//pattern_levels
+low brightness: 0, high brightness: 255
+
+What:/sys/class/leds//run_pattern
+Date:Oct 2015
+KernelVersion:4.3
+Contact:Milo Kim <milo@ti.com>
+Description:write only
+After 'pattern_times' and 'pattern_levels' are updated,
+run the pattern by writing 1 to 'run_pattern'.
+To stop running pattern, writes 0 to 'run_pattern'.


I wonder how registering an in-driver trigger would work. It would
allow for hiding above pattern attributes when the trigger is inactive,
and thus making the sysfs interface more transparent. You could avoid
the need for run_pattern attribute, as setting the trigger would itself
activate the pattern, and setting brightness to 0 would turn it off.


I like this idea, let me try to fix it.


After thinking it over, I came to conclusion that implementing it as
an in-driver trigger is not a proper way to go, since triggers are
defined as kernel based source of LED events.

This is somehow abused in case of timer trigger which takes hardware
blinking feature as a first choice and applies software blinking as
a fallback only. To be consistent with that, we could go for adding
generic pattern trigger and add a led_pattern_set() API, similarly
to existing led_blink_set().

The problem is that different LED controllers may implement blinking
patterns that are configured with different set of parameters. This
subject would definitely require thorough analysis.

For now, please just expose pattern settings as separate sysfs
attributes of a LED class device.



Thanks for your suggestion.
Then, LM3633 LED driver will support 8 device attributes.

pattern_time_delay
pattern_time_rise
pattern_time_high
pattern_time_fall
pattern_time_low
pattern_brightness_low
pattern_brightness_high
pattern_run_pattern

Details will be updated in Documentation/ABI/testing/sysfs-class-led-lm3633.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 01/16] Documentation: dt-bindings: mfd: add TI LMU device binding information

[Kim, Milo]

Thanks for all your comments. I'll create the 2nd patch-set in few weeks.

Best regards,
Milo

On 11/11/2015 6:49 PM, Lee Jones wrote:

On Mon, 02 Nov 2015, Milo Kim wrote:


This patch describes overall binding for TI LMU MFD devices.

Cc: devicet...@vger.kernel.org
Cc: Lee Jones 
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Milo Kim 
---
  Documentation/devicetree/bindings/mfd/ti-lmu.txt | 282 +++
  1 file changed, 282 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/mfd/ti-lmu.txt

diff --git a/Documentation/devicetree/bindings/mfd/ti-lmu.txt 
b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
new file mode 100644
index 000..7ccf07e
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
@@ -0,0 +1,282 @@
+TI LMU(Lighting Management Unit) device tree bindings


' ' here -^


+TI LMU driver supports lighting devices belows.


s/belows./below:/


+   Name  Child nodes
+  --  -
+  LM3532   Backlight
+  LM3631   Backlight and regulator
+  LM3632   Backlight and regulator
+  LM3633   Backlight, LED and HWMON
+  LM3695   Backlight
+  LM3697   Backlight and HWMON
+
+Required properties:
+  - compatible: Should be one of lists below.


s/ lists below./:/


+"ti,lm3532"
+"ti,lm3631"
+"ti,lm3632"
+"ti,lm3633"
+"ti,lm3695"
+"ti,lm3697"
+  - reg: I2C slave address.
+ 0x11 is LM3632
+ 0x29 is LM3631
+ 0x36 is LM3633, LM3697
+ 0x38 is LM3532
+ 0x63 is LM3695


s/is/for/


+Optional properties:
+  - enable-gpios: A GPIO specifier for hardware enable pin.
+
+Required node:
+  - backlight: All LMU devices have backlight child nodes.
+   For the properties, please refer to [1].
+
+Optional nodes:
+  - hwmon: Hardware fault monitoring driver for LM3633 and LM3697.
+   For the property, please refer to [2].
+  - leds: LED properties for LM3633. Please refer to [3].
+  - regulators: Regulator properties for LM3631 and LM3632.
+  Please refer to [4].
+
+[1] Documentation/devicetree/bindings/video/backlight/ti-lmu-backlight.txt
+[2] Documentation/devicetree/bindings/hwm/ti-lmu-hwmon.txt
+[3] Documentation/devicetree/bindings/leds/leds-lm3633.txt
+[4] Documentation/devicetree/bindings/regulator/lm363x-regulator.txt


s/Documentation/devicetree/bindings/../


+Examples:


These aren't examples, they're actual DT entries.

Please trim them down to just one, fully enabled node, only to be used
as an 'example'.


+LM3532 has a backlight device. External GPIO is used for enabling LM3532.
+
+lm3532@38 {
+   compatible = "ti,lm3532";
+   reg = <0x38>;
+
+   enable-gpios = < 2 GPIO_ACTIVE_HIGH>;
+
+   backlight {
+   compatible = "ti,lm3532-backlight";
+
+   lcd {
+   hvled1-used;
+   hvled2-used;
+   hvled3-used;
+
+   ramp-up-msec = <30>;
+   ramp-down-msec = <0>;
+
+   backlight-max-microamp = <5000>;
+   };
+   };
+};
+
+LM3631 has 5 regulators with one backlight device.
+
+lm3631@29 {
+   compatible = "ti,lm3631";
+   reg = <0x29>;
+
+   regulators {
+   compatible = "ti,lm363x-regulator";
+
+   vboost {
+   regulator-name = "lcd_boost";
+   regulator-min-microvolt = <450>;
+   regulator-max-microvolt = <635>;
+   regulator-always-on;
+   };
+
+   vcont {
+   regulator-name = "lcd_vcont";
+   regulator-min-microvolt = <180>;
+   regulator-max-microvolt = <330>;
+   };
+
+   voref {
+   regulator-name = "lcd_voref";
+   regulator-min-microvolt = <400>;
+   regulator-max-microvolt = <600>;
+   };
+
+   vpos {
+   regulator-name = "lcd_vpos";
+   regulator-min-microvolt = <400>;
+   regulator-max-microvolt = <600>;
+   regulator-boot-on;
+   };
+
+   vneg {
+   regulator-name = "lcd_vneg";
+   regulator-min-microvolt = <400>;
+   regulator-max-microvolt = <600>;
+   regulator-boot-on;
+   };
+   };
+
+   backlight {
+   compatible = "ti,lm3631-backlight";
+
+   lcd {
+   backlight-name = "lcd_bl";
+   hvled1-used;
+   hvled2-used;
+
+   ramp-up-msec = <300>;
+   };
+   };
+};
+
+LM3632 has 3 regulators with one 

Re: [PATCH RESEND 01/16] Documentation: dt-bindings: mfd: add TI LMU device binding information

[Kim, Milo]

Thanks for all your comments. I'll create the 2nd patch-set in few weeks.

Best regards,
Milo

On 11/11/2015 6:49 PM, Lee Jones wrote:

On Mon, 02 Nov 2015, Milo Kim wrote:


This patch describes overall binding for TI LMU MFD devices.

Cc: devicet...@vger.kernel.org
Cc: Lee Jones <lee.jo...@linaro.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Milo Kim <milo@ti.com>
---
  Documentation/devicetree/bindings/mfd/ti-lmu.txt | 282 +++
  1 file changed, 282 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/mfd/ti-lmu.txt

diff --git a/Documentation/devicetree/bindings/mfd/ti-lmu.txt 
b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
new file mode 100644
index 000..7ccf07e
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
@@ -0,0 +1,282 @@
+TI LMU(Lighting Management Unit) device tree bindings


' ' here -^


+TI LMU driver supports lighting devices belows.


s/belows./below:/


+   Name  Child nodes
+  --  -
+  LM3532   Backlight
+  LM3631   Backlight and regulator
+  LM3632   Backlight and regulator
+  LM3633   Backlight, LED and HWMON
+  LM3695   Backlight
+  LM3697   Backlight and HWMON
+
+Required properties:
+  - compatible: Should be one of lists below.


s/ lists below./:/


+"ti,lm3532"
+"ti,lm3631"
+"ti,lm3632"
+"ti,lm3633"
+"ti,lm3695"
+"ti,lm3697"
+  - reg: I2C slave address.
+ 0x11 is LM3632
+ 0x29 is LM3631
+ 0x36 is LM3633, LM3697
+ 0x38 is LM3532
+ 0x63 is LM3695


s/is/for/


+Optional properties:
+  - enable-gpios: A GPIO specifier for hardware enable pin.
+
+Required node:
+  - backlight: All LMU devices have backlight child nodes.
+   For the properties, please refer to [1].
+
+Optional nodes:
+  - hwmon: Hardware fault monitoring driver for LM3633 and LM3697.
+   For the property, please refer to [2].
+  - leds: LED properties for LM3633. Please refer to [3].
+  - regulators: Regulator properties for LM3631 and LM3632.
+  Please refer to [4].
+
+[1] Documentation/devicetree/bindings/video/backlight/ti-lmu-backlight.txt
+[2] Documentation/devicetree/bindings/hwm/ti-lmu-hwmon.txt
+[3] Documentation/devicetree/bindings/leds/leds-lm3633.txt
+[4] Documentation/devicetree/bindings/regulator/lm363x-regulator.txt


s/Documentation/devicetree/bindings/../


+Examples:


These aren't examples, they're actual DT entries.

Please trim them down to just one, fully enabled node, only to be used
as an 'example'.


+LM3532 has a backlight device. External GPIO is used for enabling LM3532.
+
+lm3532@38 {
+   compatible = "ti,lm3532";
+   reg = <0x38>;
+
+   enable-gpios = < 2 GPIO_ACTIVE_HIGH>;
+
+   backlight {
+   compatible = "ti,lm3532-backlight";
+
+   lcd {
+   hvled1-used;
+   hvled2-used;
+   hvled3-used;
+
+   ramp-up-msec = <30>;
+   ramp-down-msec = <0>;
+
+   backlight-max-microamp = <5000>;
+   };
+   };
+};
+
+LM3631 has 5 regulators with one backlight device.
+
+lm3631@29 {
+   compatible = "ti,lm3631";
+   reg = <0x29>;
+
+   regulators {
+   compatible = "ti,lm363x-regulator";
+
+   vboost {
+   regulator-name = "lcd_boost";
+   regulator-min-microvolt = <450>;
+   regulator-max-microvolt = <635>;
+   regulator-always-on;
+   };
+
+   vcont {
+   regulator-name = "lcd_vcont";
+   regulator-min-microvolt = <180>;
+   regulator-max-microvolt = <330>;
+   };
+
+   voref {
+   regulator-name = "lcd_voref";
+   regulator-min-microvolt = <400>;
+   regulator-max-microvolt = <600>;
+   };
+
+   vpos {
+   regulator-name = "lcd_vpos";
+   regulator-min-microvolt = <400>;
+   regulator-max-microvolt = <600>;
+   regulator-boot-on;
+   };
+
+   vneg {
+   regulator-name = "lcd_vneg";
+   regulator-min-microvolt = <400>;
+   regulator-max-microvolt = <600>;
+   regulator-boot-on;
+   };
+   };
+
+   

Re: [PATCH RESEND 15/16] leds: add LM3633 driver

[Kim, Milo]

Hi Jacek,

On 11/10/2015 10:44 PM, Jacek Anaszewski wrote:

+static ssize_t lm3633_led_store_pattern_levels(struct device *dev,
>>>+   struct device_attribute *attr,
>>>+   const char *buf, size_t len)
>>>+{
>>>+struct ti_lmu_led *lmu_led = to_ti_lmu_led(dev);
>>>+struct ti_lmu_led_chip *chip = lmu_led->chip;
>>>+unsigned int low, high;
>>>+u8 reg, offset, val;
>>>+int ret;
>>>+
>>>+/*
>>>+ * Sequence
>>>+ *
>>>+ * 1) Read pattern level data
>>>+ * 2) Disable a bank before programming a pattern
>>>+ * 3) Update LOW BRIGHTNESS register
>>>+ * 4) Update HIGH BRIGHTNESS register
>>>+ *
>>>+ * Level register addresses have offset number based on the LED
>>>bank.
>>>+ */
>>>+
>>>+ret = sscanf(buf, "%u %u", , );
>>>+if (ret != 2)
>>>+return -EINVAL;
>>>+
>>>+low = min_t(unsigned int, low, LM3633_LED_MAX_BRIGHTNESS);

>>
>>Why don't you take into account the value defined by led-max-microamp
>>DT property here?

>
>'low' and 'high' are brightness value. The range is from 0 to 255. It is
>mostly related with LED sysfs -/sys/class/led//brightness.
>On the other hand, led-max-microamp is current limit. It is from 5mA to
>30mA. It's different configuration in this device.

Doesn't the current has direct influence on the LED brightness?
Are there other means for adjusting brightness than current setting?
I see in your enum ti_lmu_max_current, that there are 26 possible
current levels. I think that they should reflect 26 possible
brightness levels, so max_brightness can be at most 26.


Let me describe LM3633 in more details. I'd like to have your opinion 
about led-max-microamp and max_brightness usages. Datasheet would be 
better to figure out characteristics.


http://www.ti.com/lit/ds/symlink/lm3633.pdf

Max current level is not same as brightness level in LM3633.
LM3633 device has two parameters for output brightness control.
One is brightness code(base register address is 0x44). The other is 
current limit(base register address is 0x22). It also provides hardware 
protection like excessive current.


0 <= brightness_code <= 0xff
0 <= current_limit_code <= 0x1f, it means
5mA <= current_limit <= 30mA.

LM3633 device calculates the output current below.

(1) Linear brightness mode
Iout = current_limit * brightness_code/255

(2) Exponential brightness mode
Iout = current_limit * 0.85 ^ (44 - (brightness_code + 1)/5.18)

So output current(Iout) can not be in excess of current_limit.



led-max-microamp was designed for defining max brightness limit.
It should be converted into max brightness level in the driver and
assigned to max_brightness property of struct led_classdev.
This attribute overrides legacy 0-255 brightness scale.



It could be applied when brightness is determined by only one parameter 
- current level. Flash/torch device would be a good example. In this 
device, current setting is directly scaled to the output brightness.


However, LM3633 has two parameters for the brightness control - current 
limit and brightness level. Max current setting is one of brightness 
control parameters. In this patch, 'led-max-microamp' from DT is 
converted to 'current_limit_code'. Then, this value is written once when 
the driver is initialized. On the other hand, 'brightness_code' can be 
changed at run time. And 'max_brightness' of led_classdev is set to max 
brightness register value, 0xff.


It sounds 'led-max-microamp' property might not be a general usage in 
LM3633. Do you have some idea?


Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 15/16] leds: add LM3633 driver

[Kim, Milo]

Hi Jacek,

On 11/10/2015 10:44 PM, Jacek Anaszewski wrote:

+static ssize_t lm3633_led_store_pattern_levels(struct device *dev,
>>>+   struct device_attribute *attr,
>>>+   const char *buf, size_t len)
>>>+{
>>>+struct ti_lmu_led *lmu_led = to_ti_lmu_led(dev);
>>>+struct ti_lmu_led_chip *chip = lmu_led->chip;
>>>+unsigned int low, high;
>>>+u8 reg, offset, val;
>>>+int ret;
>>>+
>>>+/*
>>>+ * Sequence
>>>+ *
>>>+ * 1) Read pattern level data
>>>+ * 2) Disable a bank before programming a pattern
>>>+ * 3) Update LOW BRIGHTNESS register
>>>+ * 4) Update HIGH BRIGHTNESS register
>>>+ *
>>>+ * Level register addresses have offset number based on the LED
>>>bank.
>>>+ */
>>>+
>>>+ret = sscanf(buf, "%u %u", , );
>>>+if (ret != 2)
>>>+return -EINVAL;
>>>+
>>>+low = min_t(unsigned int, low, LM3633_LED_MAX_BRIGHTNESS);

>>
>>Why don't you take into account the value defined by led-max-microamp
>>DT property here?

>
>'low' and 'high' are brightness value. The range is from 0 to 255. It is
>mostly related with LED sysfs -/sys/class/led//brightness.
>On the other hand, led-max-microamp is current limit. It is from 5mA to
>30mA. It's different configuration in this device.

Doesn't the current has direct influence on the LED brightness?
Are there other means for adjusting brightness than current setting?
I see in your enum ti_lmu_max_current, that there are 26 possible
current levels. I think that they should reflect 26 possible
brightness levels, so max_brightness can be at most 26.


Let me describe LM3633 in more details. I'd like to have your opinion 
about led-max-microamp and max_brightness usages. Datasheet would be 
better to figure out characteristics.


http://www.ti.com/lit/ds/symlink/lm3633.pdf

Max current level is not same as brightness level in LM3633.
LM3633 device has two parameters for output brightness control.
One is brightness code(base register address is 0x44). The other is 
current limit(base register address is 0x22). It also provides hardware 
protection like excessive current.


0 <= brightness_code <= 0xff
0 <= current_limit_code <= 0x1f, it means
5mA <= current_limit <= 30mA.

LM3633 device calculates the output current below.

(1) Linear brightness mode
Iout = current_limit * brightness_code/255

(2) Exponential brightness mode
Iout = current_limit * 0.85 ^ (44 - (brightness_code + 1)/5.18)

So output current(Iout) can not be in excess of current_limit.



led-max-microamp was designed for defining max brightness limit.
It should be converted into max brightness level in the driver and
assigned to max_brightness property of struct led_classdev.
This attribute overrides legacy 0-255 brightness scale.



It could be applied when brightness is determined by only one parameter 
- current level. Flash/torch device would be a good example. In this 
device, current setting is directly scaled to the output brightness.


However, LM3633 has two parameters for the brightness control - current 
limit and brightness level. Max current setting is one of brightness 
control parameters. In this patch, 'led-max-microamp' from DT is 
converted to 'current_limit_code'. Then, this value is written once when 
the driver is initialized. On the other hand, 'brightness_code' can be 
changed at run time. And 'max_brightness' of led_classdev is set to max 
brightness register value, 0xff.


It sounds 'led-max-microamp' property might not be a general usage in 
LM3633. Do you have some idea?


Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 16/16] regulator: add LM363X driver

[Kim, Milo]

On 11/4/2015 10:59 PM, Mark Brown wrote:

On Mon, Nov 02, 2015 at 02:24:35PM +0900, Milo Kim wrote:

This looks mostly good, just a few fairly small things:


+lm363x_regulator_of_get_init_data(struct device *dev,
+   struct lm363x_regulator *lm363x_regulator, int id)
+{
+   struct device_node *np = dev->of_node;
+   int count;
+
+   count = of_regulator_match(dev, np, _regulator_matches[id], 1);
+   if (count <= 0)
+   return ERR_PTR(-ENODEV);
+
+   return lm363x_regulator_matches[id].init_data;
+}


Don't open code DT matching, use of_match in the regulator_desc and let
the core do it for you.


OK, good.
I've also found wrong pointer reference of regulator_cfg.dev. It should 
be pdev->dev instead of pdev->dev->parent.


-   cfg.dev = dev->parent;
+   cfg.dev = dev;

Old code affects wrong parsing from the DT. So, registered regulator has 
different operation mask. In the end, regulators which has zero use 
count never gonna be disabled in regulator_init_compelete().





+   /*
+* Check LCM_EN1/2_GPIO is configured.
+* Those pins are used for enabling VPOS/VNEG LDOs.
+*/
+   if (id == LM3632_LDO_POS)
+   gpio = of_get_named_gpio(np, "ti,lcm-en1-gpio", 0);
+   else if (id == LM3632_LDO_NEG)
+   gpio = of_get_named_gpio(np, "ti,lcm-en2-gpio", 0);


This looks like it should be a switch statement.


Yep!




+   rdev = regulator_register(_regulator_desc[id], );
+   if (IS_ERR(rdev)) {


Use devm_regulator_register().


OK.




+static const struct of_device_id lm363x_regulator_of_match[] = {
+   { .compatible = "ti,lm363x-regulator" },
+   { }
+};
+MODULE_DEVICE_TABLE(of, lm363x_regulator_of_match);


You shouldn't need a compatible string for a device like this, the MFD
should just register a platform device based on the compatible string
for the MFD.



Thanks for catching this.
Let me apply all your comments in the next patch.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 15/16] leds: add LM3633 driver

[Kim, Milo]

Hi Jacek,

On 11/4/2015 1:15 AM, Jacek Anaszewski wrote:

Hi Milo,

Thanks for the patch. Please find my comments in the code.


diff --git a/Documentation/ABI/testing/sysfs-class-led-lm3633 
b/Documentation/ABI/testing/sysfs-class-led-lm3633
new file mode 100644
index 000..c1d8759
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-lm3633
@@ -0,0 +1,60 @@
+LM3633 LED driver generates programmable pattern via the sysfs.
+
+LED Pattern Generator Structure
+
+(3)
+  (a) --->  ___
+   /   \
+  (2) / \ (4)
+  (b) > _/   \_  ...
+   (1)   (5)
+
+ |<-   period   -> |
+
+What:  /sys/class/leds//pattern_times


"time" noun is uncountable.


+Date:  Oct 2015
+KernelVersion: 4.3


These properties certainly will not appear in 4.3.


Oops! 4.4 gonna be OK?




+Contact:   Milo Kim 
+Description:   read/write
+Set pattern time dimension. There are five arguments.
+  (1) startup delay
+  (2) rising dimming time
+  (3) how much time stays at high level
+  (4) falling dimming time
+  (5) how much time stays at low level
+Ranges are
+  (1), (3), (5): 0 ~ 1. Unit is millisecond.
+  (2), (4): 0 ~ 16000. Unit is millisecond.
+
+Example:
+No delay, rising 200ms, high 300ms, falling 100ms, low 400ms.
+echo "0 200 300 100 400" > /sys/class/leds//pattern_times
+
+cat /sys/class/leds//pattern_times
+delay: 0, rise: 200, high: 300, fall: 100, low: 400


Generally a sysfs attribute should represent a single value.
There are cases where the attribute comprises a list of space separated
values, but certainly colons and commas adds to much noise, and are
cumbersome to parse. I'd opt for creating a separate sysfs attribute
for each of the above 5 properties.


Got it, thanks!




+What:  /sys/class/leds//pattern_levels
+Date:  Oct 2015
+KernelVersion: 4.3
+Contact:   Milo Kim 
+Description:   read/write
+Set pattern level(brightness). There are two arguments.
+  (a) Low brightness level
+  (b) High brightness level
+Ranges are from 0 to 255.
+
+Example:
+Low level is 0, high level is 255.
+echo "0 255" > /sys/class/leds//pattern_levels


I'd go also for two separate attributes. E.g. pattern_brightness_lo and
pattern_brightness_hi, or maybe you'll have better idea.


OK.




+cat /sys/class/leds//pattern_levels
+low brightness: 0, high brightness: 255
+
+What:  /sys/class/leds//run_pattern
+Date:  Oct 2015
+KernelVersion: 4.3
+Contact:   Milo Kim 
+Description:   write only
+After 'pattern_times' and 'pattern_levels' are updated,
+run the pattern by writing 1 to 'run_pattern'.
+To stop running pattern, writes 0 to 'run_pattern'.


I wonder how registering an in-driver trigger would work. It would
allow for hiding above pattern attributes when the trigger is inactive,
and thus making the sysfs interface more transparent. You could avoid
the need for run_pattern attribute, as setting the trigger would itself
activate the pattern, and setting brightness to 0 would turn it off.


I like this idea, let me try to fix it.


+/**
+ * struct ti_lmu_led
+ *
+ * @chip:  Pointer to parent LED device
+ * @bank_id:   LED bank ID
+ * @cdev:  LED subsystem device structure
+ * @name:  LED channel name
+ * @led_string:LED string configuration.
+ * Bit mask is set on parsing DT.
+ * @imax:  [Optional] Max current index.
+ * It's result of ti_lmu_get_current_code().


Why is this optional?


You're correct, this is not optional. DT property is optional.


+static ssize_t lm3633_led_store_pattern_levels(struct device *dev,
+  struct device_attribute *attr,
+  const char *buf, size_t len)
+{
+   struct ti_lmu_led *lmu_led = to_ti_lmu_led(dev);
+   struct ti_lmu_led_chip *chip = lmu_led->chip;
+   unsigned int low, high;
+   u8 reg, offset, val;
+   int ret;
+
+   /*
+* Sequence
+*
+* 1) Read pattern level data
+* 2) Disable a bank before programming a pattern
+* 3) Update LOW BRIGHTNESS register
+* 4) Update HIGH BRIGHTNESS register
+*
+* Level register addresses have offset number based on the LED bank.
+*/
+
+   ret = sscanf(buf, "%u %u", , );
+   if (ret != 2)
+   

Re: [PATCH RESEND 04/16] Documentation: dt-bindings: leds: add LM3633 LED binding information

[Kim, Milo]

Hi Jacek,

On 11/4/2015 1:15 AM, Jacek Anaszewski wrote:

+  - led-max-microamp: Max current setting. Type is .
>+  Unit is microampere. Range is from 5000 to 3.

Could you specify also a step?



Yep, step is 1000. Thanks for catching this.
I'll update the binding in the next patch.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 04/16] Documentation: dt-bindings: leds: add LM3633 LED binding information

[Kim, Milo]

Hi Jacek,

On 11/4/2015 1:15 AM, Jacek Anaszewski wrote:

+  - led-max-microamp: Max current setting. Type is .
>+  Unit is microampere. Range is from 5000 to 3.

Could you specify also a step?



Yep, step is 1000. Thanks for catching this.
I'll update the binding in the next patch.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 15/16] leds: add LM3633 driver

[Kim, Milo]

Hi Jacek,

On 11/4/2015 1:15 AM, Jacek Anaszewski wrote:

Hi Milo,

Thanks for the patch. Please find my comments in the code.


diff --git a/Documentation/ABI/testing/sysfs-class-led-lm3633 
b/Documentation/ABI/testing/sysfs-class-led-lm3633
new file mode 100644
index 000..c1d8759
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-lm3633
@@ -0,0 +1,60 @@
+LM3633 LED driver generates programmable pattern via the sysfs.
+
+LED Pattern Generator Structure
+
+(3)
+  (a) --->  ___
+   /   \
+  (2) / \ (4)
+  (b) > _/   \_  ...
+   (1)   (5)
+
+ |<-   period   -> |
+
+What:  /sys/class/leds//pattern_times


"time" noun is uncountable.


+Date:  Oct 2015
+KernelVersion: 4.3


These properties certainly will not appear in 4.3.


Oops! 4.4 gonna be OK?




+Contact:   Milo Kim <milo@ti.com>
+Description:   read/write
+Set pattern time dimension. There are five arguments.
+  (1) startup delay
+  (2) rising dimming time
+  (3) how much time stays at high level
+  (4) falling dimming time
+  (5) how much time stays at low level
+Ranges are
+  (1), (3), (5): 0 ~ 1. Unit is millisecond.
+  (2), (4): 0 ~ 16000. Unit is millisecond.
+
+Example:
+No delay, rising 200ms, high 300ms, falling 100ms, low 400ms.
+echo "0 200 300 100 400" > /sys/class/leds//pattern_times
+
+cat /sys/class/leds//pattern_times
+delay: 0, rise: 200, high: 300, fall: 100, low: 400


Generally a sysfs attribute should represent a single value.
There are cases where the attribute comprises a list of space separated
values, but certainly colons and commas adds to much noise, and are
cumbersome to parse. I'd opt for creating a separate sysfs attribute
for each of the above 5 properties.


Got it, thanks!




+What:  /sys/class/leds//pattern_levels
+Date:  Oct 2015
+KernelVersion: 4.3
+Contact:   Milo Kim <milo@ti.com>
+Description:   read/write
+Set pattern level(brightness). There are two arguments.
+  (a) Low brightness level
+  (b) High brightness level
+Ranges are from 0 to 255.
+
+Example:
+Low level is 0, high level is 255.
+echo "0 255" > /sys/class/leds//pattern_levels


I'd go also for two separate attributes. E.g. pattern_brightness_lo and
pattern_brightness_hi, or maybe you'll have better idea.


OK.




+cat /sys/class/leds//pattern_levels
+low brightness: 0, high brightness: 255
+
+What:  /sys/class/leds//run_pattern
+Date:      Oct 2015
+KernelVersion: 4.3
+Contact:   Milo Kim <milo@ti.com>
+Description:   write only
+After 'pattern_times' and 'pattern_levels' are updated,
+run the pattern by writing 1 to 'run_pattern'.
+To stop running pattern, writes 0 to 'run_pattern'.


I wonder how registering an in-driver trigger would work. It would
allow for hiding above pattern attributes when the trigger is inactive,
and thus making the sysfs interface more transparent. You could avoid
the need for run_pattern attribute, as setting the trigger would itself
activate the pattern, and setting brightness to 0 would turn it off.


I like this idea, let me try to fix it.


+/**
+ * struct ti_lmu_led
+ *
+ * @chip:  Pointer to parent LED device
+ * @bank_id:   LED bank ID
+ * @cdev:  LED subsystem device structure
+ * @name:  LED channel name
+ * @led_string:LED string configuration.
+ * Bit mask is set on parsing DT.
+ * @imax:  [Optional] Max current index.
+ * It's result of ti_lmu_get_current_code().


Why is this optional?


You're correct, this is not optional. DT property is optional.


+static ssize_t lm3633_led_store_pattern_levels(struct device *dev,
+  struct device_attribute *attr,
+  const char *buf, size_t len)
+{
+   struct ti_lmu_led *lmu_led = to_ti_lmu_led(dev);
+   struct ti_lmu_led_chip *chip = lmu_led->chip;
+   unsigned int low, high;
+   u8 reg, offset, val;
+   int ret;
+
+   /*
+* Sequence
+*
+* 1) Read pattern level data
+* 2) Disable a bank before programming a pattern
+* 3) Update LOW BRIGHTNESS register
+* 4) Update HIGH BRIGHTNESS register
+*
+* Level register addresses have offset

Re: [PATCH RESEND 16/16] regulator: add LM363X driver

[Kim, Milo]

On 11/4/2015 10:59 PM, Mark Brown wrote:

On Mon, Nov 02, 2015 at 02:24:35PM +0900, Milo Kim wrote:

This looks mostly good, just a few fairly small things:


+lm363x_regulator_of_get_init_data(struct device *dev,
+   struct lm363x_regulator *lm363x_regulator, int id)
+{
+   struct device_node *np = dev->of_node;
+   int count;
+
+   count = of_regulator_match(dev, np, _regulator_matches[id], 1);
+   if (count <= 0)
+   return ERR_PTR(-ENODEV);
+
+   return lm363x_regulator_matches[id].init_data;
+}


Don't open code DT matching, use of_match in the regulator_desc and let
the core do it for you.


OK, good.
I've also found wrong pointer reference of regulator_cfg.dev. It should 
be pdev->dev instead of pdev->dev->parent.


-   cfg.dev = dev->parent;
+   cfg.dev = dev;

Old code affects wrong parsing from the DT. So, registered regulator has 
different operation mask. In the end, regulators which has zero use 
count never gonna be disabled in regulator_init_compelete().





+   /*
+* Check LCM_EN1/2_GPIO is configured.
+* Those pins are used for enabling VPOS/VNEG LDOs.
+*/
+   if (id == LM3632_LDO_POS)
+   gpio = of_get_named_gpio(np, "ti,lcm-en1-gpio", 0);
+   else if (id == LM3632_LDO_NEG)
+   gpio = of_get_named_gpio(np, "ti,lcm-en2-gpio", 0);


This looks like it should be a switch statement.


Yep!




+   rdev = regulator_register(_regulator_desc[id], );
+   if (IS_ERR(rdev)) {


Use devm_regulator_register().


OK.




+static const struct of_device_id lm363x_regulator_of_match[] = {
+   { .compatible = "ti,lm363x-regulator" },
+   { }
+};
+MODULE_DEVICE_TABLE(of, lm363x_regulator_of_match);


You shouldn't need a compatible string for a device like this, the MFD
should just register a platform device based on the compatible string
for the MFD.



Thanks for catching this.
Let me apply all your comments in the next patch.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 03/16] Documentation: dt-bindings: hwmon: add TI LMU HWMON binding information

[Kim, Milo]

On 11/6/2015 10:57 AM, Rob Herring wrote:

On Mon, Nov 02, 2015 at 02:24:22PM +0900, Milo Kim wrote:

Hardware fault monitoring driver is used in LM3633 and LM3697 device.
Just 'compatible' property is required to describe the driver.

Cc: devicet...@vger.kernel.org
Cc: Guenter Roeck 
Cc: Jean Delvare 
Cc: Lee Jones 
Cc: lm-sens...@lm-sensors.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Milo Kim 


Acked-by: Rob Herring 


Thanks for your review, Rob.
HWMON maintainer, Guenter suggested moving LMU HWMON driver to MFD as 
the sysfs attributes because it has no sensor data like temperature or 
voltage.

So I'll move this property to mfd/ti-lmu.txt in the next patch.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 03/16] Documentation: dt-bindings: hwmon: add TI LMU HWMON binding information

[Kim, Milo]

On 11/6/2015 10:57 AM, Rob Herring wrote:

On Mon, Nov 02, 2015 at 02:24:22PM +0900, Milo Kim wrote:

Hardware fault monitoring driver is used in LM3633 and LM3697 device.
Just 'compatible' property is required to describe the driver.

Cc: devicet...@vger.kernel.org
Cc: Guenter Roeck <li...@roeck-us.net>
Cc: Jean Delvare <jdelv...@suse.com>
Cc: Lee Jones <lee.jo...@linaro.org>
Cc: lm-sens...@lm-sensors.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Milo Kim <milo@ti.com>


Acked-by: Rob Herring <r...@kernel.org>


Thanks for your review, Rob.
HWMON maintainer, Guenter suggested moving LMU HWMON driver to MFD as 
the sysfs attributes because it has no sensor data like temperature or 
voltage.

So I'll move this property to mfd/ti-lmu.txt in the next patch.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 00/16] Support TI LMU devices

[Kim, Milo]

On 11/3/2015 5:33 PM, Lee Jones wrote:

On Tue, 03 Nov 2015, Kim, Milo wrote:


Hi Lee,

On 11/2/2015 5:59 PM, Lee Jones wrote:

  drivers/video/backlight/Kconfig|  62 ++

  drivers/video/backlight/Makefile   |   7 +
  drivers/video/backlight/lm3532_bl.c| 183 +
  drivers/video/backlight/lm3631_bl.c| 129 
  drivers/video/backlight/lm3632_bl.c| 125 
  drivers/video/backlight/lm3633_bl.c| 210 ++
  drivers/video/backlight/lm3695_bl.c|  91 +++
  drivers/video/backlight/lm3697_bl.c| 187 +
  drivers/video/backlight/ti-lmu-backlight.c | 429 
  drivers/video/backlight/ti-lmu-backlight.h | 152 +

How different are all of these drivers?

Can you create one driver that supports them all instead?



Thanks for your suggestion.

'ti-lmu-backlight' is the common part of lm_bl drivers. And each
lmxxx_bl has its own operation functions by using ti_lmu_bl_ops.
I've tried to make consolidated driver but it contained too much
device specific code in one file. So I prefer simple drivers
structure - 'common part' and 'device specific operations'.
It would be appreciated if you could introduce better idea.


I wish to avoid having to apply the patches to conduct my own analysis
of the files, as I am severely restricted on time.  Can you tell me how
much duplicated code there is between the files?  How many lines would
be saved by supporting all of the lm* drivers in a single file?



Understood. Let me try it again. I'll get back to you soon.
Thanks for your help.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 00/16] Support TI LMU devices

[Kim, Milo]

On 11/3/2015 5:33 PM, Lee Jones wrote:

On Tue, 03 Nov 2015, Kim, Milo wrote:


Hi Lee,

On 11/2/2015 5:59 PM, Lee Jones wrote:

  drivers/video/backlight/Kconfig|  62 ++

  drivers/video/backlight/Makefile   |   7 +
  drivers/video/backlight/lm3532_bl.c| 183 +
  drivers/video/backlight/lm3631_bl.c| 129 
  drivers/video/backlight/lm3632_bl.c| 125 
  drivers/video/backlight/lm3633_bl.c| 210 ++
  drivers/video/backlight/lm3695_bl.c|  91 +++
  drivers/video/backlight/lm3697_bl.c| 187 +
  drivers/video/backlight/ti-lmu-backlight.c | 429 
  drivers/video/backlight/ti-lmu-backlight.h | 152 +

How different are all of these drivers?

Can you create one driver that supports them all instead?



Thanks for your suggestion.

'ti-lmu-backlight' is the common part of lm_bl drivers. And each
lmxxx_bl has its own operation functions by using ti_lmu_bl_ops.
I've tried to make consolidated driver but it contained too much
device specific code in one file. So I prefer simple drivers
structure - 'common part' and 'device specific operations'.
It would be appreciated if you could introduce better idea.


I wish to avoid having to apply the patches to conduct my own analysis
of the files, as I am severely restricted on time.  Can you tell me how
much duplicated code there is between the files?  How many lines would
be saved by supporting all of the lm* drivers in a single file?



Understood. Let me try it again. I'll get back to you soon.
Thanks for your help.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 02/16] Documentation: dt-bindings: backlight: add TI LMU backlight binding information

[Kim, Milo]

On 11/3/2015 12:02 AM, Rob Herring wrote:

On Sun, Nov 1, 2015 at 11:24 PM, Milo Kim  wrote:

LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697 use common dt-bindings
for describing device.

Cc: devicet...@vger.kernel.org
Cc: Jingoo Han 
Cc: Lee Jones 
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Milo Kim 
---
  .../bindings/video/backlight/ti-lmu-backlight.txt  | 67 ++


Please move to bindings/leds/backlight/


There are backlight bindings under video/backlight. I'd like to know why 
this 'led' location is preferred. My guess is most of properties are 
from common LED properties. Any other reasons?





  1 file changed, 67 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/video/backlight/ti-lmu-backlight.txt

diff --git 
a/Documentation/devicetree/bindings/video/backlight/ti-lmu-backlight.txt 
b/Documentation/devicetree/bindings/video/backlight/ti-lmu-backlight.txt
new file mode 100644
index 000..27b0036
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/backlight/ti-lmu-backlight.txt
@@ -0,0 +1,67 @@
+TI LMU backlight device tree bindings
+
+Required properties:
+  - compatible: Should be one of lists below.
+"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:
+  - hvled1-used, hvled2-used, hvled3-used:
+High voltage backlight strings configuration. Type is .
+Please describe which output backlight string is used.
+Please refer to the datasheets [3].


Use led-sources.


OK.




+
+  Optional properties of a child node:
+  - backlight-name: Name string for backlight device identification.
+It is used for creating backlight sysfs,
+/sys/class/backlight//.


Use label.


Got it.




+  - backlight-max-microamp: Max current setting. Type is .
+Unit is microampere.
+Range is from 5000 to 3.


Use led-max-microamp


OK.




+  - initial-brightness: 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


Use default-brightness-level



I'll update the bindings and drivers based on your review. Many thanks!

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 00/16] Support TI LMU devices

[Kim, Milo]

Hi Lee,

On 11/2/2015 6:00 PM, Lee Jones wrote:

Is it just me, or have you missed lots of people off Cc?


Ah, that's what I was hesitating...
What is the best way to submit MFD code patches? Cc for all people from 
get_maintainer.pl?


Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 04/16] Documentation: dt-bindings: leds: add LM3633 LED binding information

[Kim, Milo]

Hi Rob,

On 11/2/2015 11:53 PM, Rob Herring wrote:

On Sun, Nov 1, 2015 at 11:01 PM, Milo Kim  wrote:

LM3633 LED device is one of TI LMU device list.

Cc: devicet...@vger.kernel.org
Cc: Jacek Anaszewski 
Cc: Lee Jones 
Cc: linux-kernel@vger.kernel.org
Cc: linux-l...@vger.kernel.org
Signed-off-by: Milo Kim 
---
  .../devicetree/bindings/leds/leds-lm3633.txt   | 28 ++
  1 file changed, 28 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3633.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-lm3633.txt 
b/Documentation/devicetree/bindings/leds/leds-lm3633.txt
new file mode 100644
index 000..bb7f213
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-lm3633.txt
@@ -0,0 +1,28 @@
+TI LMU LM3633 LED device tree bindings
+
+Required properties:
+  - compatible: "ti,lm3633-leds"
+
+Child nodes:
+  Each node matches with LED control bank.
+  Please refer to the datasheet [1].
+
+  Required properties of a child node:
+  - lvled1-used, lvled2-used, lvled3-used,
+lvled4-used, lvled5-used, lvled6-used:
+Low voltage LED string configuration. Type is .
+Please describe which output LED string is used.


What is "LED string"?


Let me replace these properties with 'led-sources'.



We have properties for which LED driver/channel of the parent to use
and "status" can be used to disable child nodes.


+
+  Optional properties of a child node:
+  - channel-name: Name string for LED channel identification
+  It is used for creating LED sysfs,
+  /sys/class/leds//.
+  If this property is empty, then default name is set to
+  "indicator:" by the driver.


I believe "label" already provides this function.



Yep, got it. Thanks!

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 14/16] hwmon: add TI LMU hardware fault monitoring driver

[Kim, Milo]

Hi Guenter,

On 11/2/2015 11:27 PM, Guenter Roeck wrote:

On 11/01/2015 09:24 PM, Milo Kim wrote:

LM3633 and LM3697 are TI LMU MFD device.
Those device have hardware monitoring feature which detects opened or
shorted circuit case.


Sure, but it only makes sense if you provide standard hwmon attributes
which can be interpreted by the "sensors" command. Which is not the case
here. You neither have a standard device type (light is not handled by hwmon),
nor standard attributes, nor do the attributes return standard values.

I think there may be a basic misunderstanding. hwmon is not intended
to monitor a specific chip on the board. Its scope is to monitor the
system (such as temperature, voltages, or current).

In your case, it might be better to attach the attributes to the mfd device.



OK, got your point. Thanks a lot for your suggestion.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 16/16] regulator: add LM363X driver

[Kim, Milo]

On 11/2/2015 9:26 PM, Mark Brown wrote:

On Mon, Nov 02, 2015 at 02:24:35PM +0900, Milo Kim wrote:

LM363X regulator driver supports LM3631 and LM3632.
LM3631 has 5 regulators. LM3632 provides 3 regulators.
One boost output and LDOs are used for the display module.
Boost voltage is configurable but always on.
Supported operations for LDOs are enabled/disabled and voltage change.


As far as I can tell this is another partial resend of a series you
originally sent about 15 minutes previously - what's going on here?



My apologies, I did send patches without --thread and 
--no-chain-reply-to options. So I've resent the patch-set.


Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 00/16] Support TI LMU devices

[Kim, Milo]

Hi Lee,

On 11/2/2015 5:59 PM, Lee Jones wrote:

  drivers/video/backlight/Kconfig|  62 ++
>  drivers/video/backlight/Makefile   |   7 +
>  drivers/video/backlight/lm3532_bl.c| 183 +
>  drivers/video/backlight/lm3631_bl.c| 129 
>  drivers/video/backlight/lm3632_bl.c| 125 
>  drivers/video/backlight/lm3633_bl.c| 210 ++
>  drivers/video/backlight/lm3695_bl.c|  91 +++
>  drivers/video/backlight/lm3697_bl.c| 187 +
>  drivers/video/backlight/ti-lmu-backlight.c | 429 
>  drivers/video/backlight/ti-lmu-backlight.h | 152 +

How different are all of these drivers?

Can you create one driver that supports them all instead?



Thanks for your suggestion.

'ti-lmu-backlight' is the common part of lm_bl drivers. And each 
lmxxx_bl has its own operation functions by using ti_lmu_bl_ops.
I've tried to make consolidated driver but it contained too much device 
specific code in one file. So I prefer simple drivers structure - 
'common part' and 'device specific operations'.

It would be appreciated if you could introduce better idea.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 00/16] Support TI LMU devices

[Kim, Milo]

Hi Lee,

On 11/2/2015 5:59 PM, Lee Jones wrote:

  drivers/video/backlight/Kconfig|  62 ++
>  drivers/video/backlight/Makefile   |   7 +
>  drivers/video/backlight/lm3532_bl.c| 183 +
>  drivers/video/backlight/lm3631_bl.c| 129 
>  drivers/video/backlight/lm3632_bl.c| 125 
>  drivers/video/backlight/lm3633_bl.c| 210 ++
>  drivers/video/backlight/lm3695_bl.c|  91 +++
>  drivers/video/backlight/lm3697_bl.c| 187 +
>  drivers/video/backlight/ti-lmu-backlight.c | 429 
>  drivers/video/backlight/ti-lmu-backlight.h | 152 +

How different are all of these drivers?

Can you create one driver that supports them all instead?



Thanks for your suggestion.

'ti-lmu-backlight' is the common part of lm_bl drivers. And each 
lmxxx_bl has its own operation functions by using ti_lmu_bl_ops.
I've tried to make consolidated driver but it contained too much device 
specific code in one file. So I prefer simple drivers structure - 
'common part' and 'device specific operations'.

It would be appreciated if you could introduce better idea.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 02/16] Documentation: dt-bindings: backlight: add TI LMU backlight binding information

[Kim, Milo]

On 11/3/2015 12:02 AM, Rob Herring wrote:

On Sun, Nov 1, 2015 at 11:24 PM, Milo Kim <milo@ti.com> wrote:

LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697 use common dt-bindings
for describing device.

Cc: devicet...@vger.kernel.org
Cc: Jingoo Han <jingooh...@gmail.com>
Cc: Lee Jones <lee.jo...@linaro.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Milo Kim <milo@ti.com>
---
  .../bindings/video/backlight/ti-lmu-backlight.txt  | 67 ++


Please move to bindings/leds/backlight/


There are backlight bindings under video/backlight. I'd like to know why 
this 'led' location is preferred. My guess is most of properties are 
from common LED properties. Any other reasons?





  1 file changed, 67 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/video/backlight/ti-lmu-backlight.txt

diff --git 
a/Documentation/devicetree/bindings/video/backlight/ti-lmu-backlight.txt 
b/Documentation/devicetree/bindings/video/backlight/ti-lmu-backlight.txt
new file mode 100644
index 000..27b0036
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/backlight/ti-lmu-backlight.txt
@@ -0,0 +1,67 @@
+TI LMU backlight device tree bindings
+
+Required properties:
+  - compatible: Should be one of lists below.
+"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:
+  - hvled1-used, hvled2-used, hvled3-used:
+High voltage backlight strings configuration. Type is .
+Please describe which output backlight string is used.
+Please refer to the datasheets [3].


Use led-sources.


OK.




+
+  Optional properties of a child node:
+  - backlight-name: Name string for backlight device identification.
+It is used for creating backlight sysfs,
+/sys/class/backlight//.


Use label.


Got it.




+  - backlight-max-microamp: Max current setting. Type is .
+Unit is microampere.
+Range is from 5000 to 3.


Use led-max-microamp


OK.




+  - initial-brightness: 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


Use default-brightness-level



I'll update the bindings and drivers based on your review. Many thanks!

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 14/16] hwmon: add TI LMU hardware fault monitoring driver

[Kim, Milo]

Hi Guenter,

On 11/2/2015 11:27 PM, Guenter Roeck wrote:

On 11/01/2015 09:24 PM, Milo Kim wrote:

LM3633 and LM3697 are TI LMU MFD device.
Those device have hardware monitoring feature which detects opened or
shorted circuit case.


Sure, but it only makes sense if you provide standard hwmon attributes
which can be interpreted by the "sensors" command. Which is not the case
here. You neither have a standard device type (light is not handled by hwmon),
nor standard attributes, nor do the attributes return standard values.

I think there may be a basic misunderstanding. hwmon is not intended
to monitor a specific chip on the board. Its scope is to monitor the
system (such as temperature, voltages, or current).

In your case, it might be better to attach the attributes to the mfd device.



OK, got your point. Thanks a lot for your suggestion.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 00/16] Support TI LMU devices

[Kim, Milo]

Hi Lee,

On 11/2/2015 6:00 PM, Lee Jones wrote:

Is it just me, or have you missed lots of people off Cc?


Ah, that's what I was hesitating...
What is the best way to submit MFD code patches? Cc for all people from 
get_maintainer.pl?


Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 04/16] Documentation: dt-bindings: leds: add LM3633 LED binding information

[Kim, Milo]

Hi Rob,

On 11/2/2015 11:53 PM, Rob Herring wrote:

On Sun, Nov 1, 2015 at 11:01 PM, Milo Kim <milo@ti.com> wrote:

LM3633 LED device is one of TI LMU device list.

Cc: devicet...@vger.kernel.org
Cc: Jacek Anaszewski <j.anaszew...@samsung.com>
Cc: Lee Jones <lee.jo...@linaro.org>
Cc: linux-kernel@vger.kernel.org
Cc: linux-l...@vger.kernel.org
Signed-off-by: Milo Kim <milo@ti.com>
---
  .../devicetree/bindings/leds/leds-lm3633.txt   | 28 ++
  1 file changed, 28 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3633.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-lm3633.txt 
b/Documentation/devicetree/bindings/leds/leds-lm3633.txt
new file mode 100644
index 000..bb7f213
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-lm3633.txt
@@ -0,0 +1,28 @@
+TI LMU LM3633 LED device tree bindings
+
+Required properties:
+  - compatible: "ti,lm3633-leds"
+
+Child nodes:
+  Each node matches with LED control bank.
+  Please refer to the datasheet [1].
+
+  Required properties of a child node:
+  - lvled1-used, lvled2-used, lvled3-used,
+lvled4-used, lvled5-used, lvled6-used:
+Low voltage LED string configuration. Type is .
+Please describe which output LED string is used.


What is "LED string"?


Let me replace these properties with 'led-sources'.



We have properties for which LED driver/channel of the parent to use
and "status" can be used to disable child nodes.


+
+  Optional properties of a child node:
+  - channel-name: Name string for LED channel identification
+  It is used for creating LED sysfs,
+  /sys/class/leds//.
+  If this property is empty, then default name is set to
+  "indicator:" by the driver.


I believe "label" already provides this function.



Yep, got it. Thanks!

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 16/16] regulator: add LM363X driver

[Kim, Milo]

On 11/2/2015 9:26 PM, Mark Brown wrote:

On Mon, Nov 02, 2015 at 02:24:35PM +0900, Milo Kim wrote:

LM363X regulator driver supports LM3631 and LM3632.
LM3631 has 5 regulators. LM3632 provides 3 regulators.
One boost output and LDOs are used for the display module.
Boost voltage is configurable but always on.
Supported operations for LDOs are enabled/disabled and voltage change.


As far as I can tell this is another partial resend of a series you
originally sent about 15 minutes previously - what's going on here?



My apologies, I did send patches without --thread and 
--no-chain-reply-to options. So I've resent the patch-set.


Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 08/16] backlight: ti-lmu-backlight: add LM3532 driver

[Kim, Milo]

Ouch! The notifier.h must be included. I didn't notice this error in ARM 
architecture. Let me fix this error in next patch-set.

Thanks a lot for catching this!

Best regards,
Milo

diff --git a/include/linux/mfd/ti-lmu.h b/include/linux/mfd/ti-lmu.h
index eeb6b9e..44268c7 100644
--- a/include/linux/mfd/ti-lmu.h
+++ b/include/linux/mfd/ti-lmu.h
@@ -14,6 +14,7 @@
 #define __MFD_TI_LMU_H__

 #include 
+#include 
 #include 

 /* Notifier event */

On 11/2/2015 2:37 PM, kbuild test robot wrote:

Hi Milo,

[auto build test ERROR on ljones-mfd/for-mfd-next -- if it's inappropriate 
base, please suggest rules for selecting the more suitable base]

url:
https://github.com/0day-ci/linux/commits/Milo-Kim/Support-TI-LMU-devices/20151102-130804
config: mips-allyesconfig (attached as .config)
reproduce:
 wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
 chmod +x ~/bin/make.cross
 # save the attached .config to linux build tree
 make.cross ARCH=mips

All errors (new ones prefixed by >>):

In file included from drivers/video/backlight/lm3532_bl.c:14:0:

include/linux/mfd/ti-lmu.h:74:32: error: field 'notifier' has incomplete type

  struct blocking_notifier_head notifier;
^

vim +/notifier +74 include/linux/mfd/ti-lmu.h

51194f72 Milo Kim 2015-11-02  68   * @notifier: Notifier for reporting hwmon 
event
51194f72 Milo Kim 2015-11-02  69   */
51194f72 Milo Kim 2015-11-02  70  struct ti_lmu {
51194f72 Milo Kim 2015-11-02  71struct device *dev;
51194f72 Milo Kim 2015-11-02  72struct regmap *regmap;
51194f72 Milo Kim 2015-11-02  73int en_gpio;
51194f72 Milo Kim 2015-11-02 @74struct blocking_notifier_head notifier;
51194f72 Milo Kim 2015-11-02  75  };
51194f72 Milo Kim 2015-11-02  76
51194f72 Milo Kim 2015-11-02  77  int ti_lmu_read_byte(struct ti_lmu *lmu, u8 
reg, u8 *read);

:: The code at line 74 was first introduced by commit
:: 51194f72221b7b83a8f6a5ba9bf0c49a10233ca2 mfd: add TI LMU driver

:: TO: Milo Kim 
:: CC: 0day robot 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 00/16] Support TI LMU devices

[Kim, Milo]

My apologies for sending without no-chain-reply-to option.
So, I've resent the patch-set named 'PATCH RESEND 00/16 .. 16/16'.

Best regards,
Milo

On 11/2/2015 2:01 PM, Milo Kim wrote:

TI LMU(Lighting Management Unit) driver supports lighting devices below.

  Enable pin  Backlights  HWMON  LEDs   Regulators
  --  --  -    
LM3532   o   o x xx
LM3631   o   o x x5 regulators
LM3632   o   o x x3 regulators
LM3633   o   o o ox
LM3695   o   o x xx
LM3697   o   o o xx

This patch-set consists of several parts below.

   DT bindings   : Binding information for each module
   TI LMU MFD: Device registration, HW enable pin control and
   I2C register access
   TI LMU backlight  : Common driver for TI LMU backlight devices
   Each backlight driver : Chip dependent code
   HWMON : LMU hardware fault monitoring driver
   LM3633 LED: LED subsystem and dimming pattern generation
   supported
   LM363X regulator  : LM3631 and LM3632 regulator driver for the
   display bias

Git location:
   Please refer to the location below. Branch name is 'ti-lmu'.
   git clone -b ti-lmu https://github.com/milokim/linux.git

Milo Kim (16):
   Documentation: dt-bindings: mfd: add TI LMU device binding information
   Documentation: dt-bindings: backlight: add TI LMU backlight binding
 information
   Documentation: dt-bindings: hwmon: add TI LMU HWMON binding
 information
   Documentation: dt-bindings: leds: add LM3633 LED binding information
   Documentation: dt-bindings: regulator: add LM363x regulator binding
 information
   mfd: add TI LMU driver
   backlight: add TI LMU backlight common driver
   backlight: ti-lmu-backlight: add LM3532 driver
   backlight: ti-lmu-backlight: add LM3631 driver
   backlight: ti-lmu-backlight: add LM3632 driver
   backlight: ti-lmu-backlight: add LM3633 driver
   backlight: ti-lmu-backlight: add LM3695 driver
   backlight: ti-lmu-backlight: add LM3697 driver
   hwmon: add TI LMU hardware fault monitoring driver
   leds: add LM3633 driver
   regulator: add LM363X driver

  Documentation/ABI/testing/sysfs-class-led-lm3633   |  60 ++
  .../devicetree/bindings/hwmon/ti-lmu-hwmon.txt |  12 +
  .../devicetree/bindings/leds/leds-lm3633.txt   |  28 +
  Documentation/devicetree/bindings/mfd/ti-lmu.txt   | 282 
  .../bindings/regulator/lm363x-regulator.txt|  28 +
  .../bindings/video/backlight/ti-lmu-backlight.txt  |  67 ++
  drivers/hwmon/Kconfig  |  10 +
  drivers/hwmon/Makefile |   1 +
  drivers/hwmon/ti-lmu-hwmon.c   | 393 +++
  drivers/leds/Kconfig   |  10 +
  drivers/leds/Makefile  |   1 +
  drivers/leds/leds-lm3633.c | 749 +
  drivers/mfd/Kconfig|  12 +
  drivers/mfd/Makefile   |   1 +
  drivers/mfd/ti-lmu.c   | 324 +
  drivers/regulator/Kconfig  |   9 +
  drivers/regulator/Makefile |   1 +
  drivers/regulator/lm363x-regulator.c   | 349 ++
  drivers/video/backlight/Kconfig|  62 ++
  drivers/video/backlight/Makefile   |   7 +
  drivers/video/backlight/lm3532_bl.c| 183 +
  drivers/video/backlight/lm3631_bl.c| 129 
  drivers/video/backlight/lm3632_bl.c| 125 
  drivers/video/backlight/lm3633_bl.c| 210 ++
  drivers/video/backlight/lm3695_bl.c|  91 +++
  drivers/video/backlight/lm3697_bl.c| 187 +
  drivers/video/backlight/ti-lmu-backlight.c | 429 
  drivers/video/backlight/ti-lmu-backlight.h | 152 +
  include/linux/mfd/ti-lmu-register.h| 277 
  include/linux/mfd/ti-lmu.h |  81 +++
  30 files changed, 4270 insertions(+)
  create mode 100644 Documentation/ABI/testing/sysfs-class-led-lm3633
  create mode 100644 Documentation/devicetree/bindings/hwmon/ti-lmu-hwmon.txt
  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3633.txt
  create mode 100644 Documentation/devicetree/bindings/mfd/ti-lmu.txt
  create mode 100644 
Documentation/devicetree/bindings/regulator/lm363x-regulator.txt
  create mode 100644 
Documentation/devicetree/bindings/video/backlight/ti-lmu-backlight.txt
  create mode 100644 drivers/hwmon/ti-lmu-hwmon.c
  create mode 100644 drivers/leds/leds-lm3633.c
  create mode 100644 drivers/mfd/ti-lmu.c
  create mode 100644 

Re: [PATCH 00/16] Support TI LMU devices

[Kim, Milo]

My apologies for sending without no-chain-reply-to option.
So, I've resent the patch-set named 'PATCH RESEND 00/16 .. 16/16'.

Best regards,
Milo

On 11/2/2015 2:01 PM, Milo Kim wrote:

TI LMU(Lighting Management Unit) driver supports lighting devices below.

  Enable pin  Backlights  HWMON  LEDs   Regulators
  --  --  -    
LM3532   o   o x xx
LM3631   o   o x x5 regulators
LM3632   o   o x x3 regulators
LM3633   o   o o ox
LM3695   o   o x xx
LM3697   o   o o xx

This patch-set consists of several parts below.

   DT bindings   : Binding information for each module
   TI LMU MFD: Device registration, HW enable pin control and
   I2C register access
   TI LMU backlight  : Common driver for TI LMU backlight devices
   Each backlight driver : Chip dependent code
   HWMON : LMU hardware fault monitoring driver
   LM3633 LED: LED subsystem and dimming pattern generation
   supported
   LM363X regulator  : LM3631 and LM3632 regulator driver for the
   display bias

Git location:
   Please refer to the location below. Branch name is 'ti-lmu'.
   git clone -b ti-lmu https://github.com/milokim/linux.git

Milo Kim (16):
   Documentation: dt-bindings: mfd: add TI LMU device binding information
   Documentation: dt-bindings: backlight: add TI LMU backlight binding
 information
   Documentation: dt-bindings: hwmon: add TI LMU HWMON binding
 information
   Documentation: dt-bindings: leds: add LM3633 LED binding information
   Documentation: dt-bindings: regulator: add LM363x regulator binding
 information
   mfd: add TI LMU driver
   backlight: add TI LMU backlight common driver
   backlight: ti-lmu-backlight: add LM3532 driver
   backlight: ti-lmu-backlight: add LM3631 driver
   backlight: ti-lmu-backlight: add LM3632 driver
   backlight: ti-lmu-backlight: add LM3633 driver
   backlight: ti-lmu-backlight: add LM3695 driver
   backlight: ti-lmu-backlight: add LM3697 driver
   hwmon: add TI LMU hardware fault monitoring driver
   leds: add LM3633 driver
   regulator: add LM363X driver

  Documentation/ABI/testing/sysfs-class-led-lm3633   |  60 ++
  .../devicetree/bindings/hwmon/ti-lmu-hwmon.txt |  12 +
  .../devicetree/bindings/leds/leds-lm3633.txt   |  28 +
  Documentation/devicetree/bindings/mfd/ti-lmu.txt   | 282 
  .../bindings/regulator/lm363x-regulator.txt|  28 +
  .../bindings/video/backlight/ti-lmu-backlight.txt  |  67 ++
  drivers/hwmon/Kconfig  |  10 +
  drivers/hwmon/Makefile |   1 +
  drivers/hwmon/ti-lmu-hwmon.c   | 393 +++
  drivers/leds/Kconfig   |  10 +
  drivers/leds/Makefile  |   1 +
  drivers/leds/leds-lm3633.c | 749 +
  drivers/mfd/Kconfig|  12 +
  drivers/mfd/Makefile   |   1 +
  drivers/mfd/ti-lmu.c   | 324 +
  drivers/regulator/Kconfig  |   9 +
  drivers/regulator/Makefile |   1 +
  drivers/regulator/lm363x-regulator.c   | 349 ++
  drivers/video/backlight/Kconfig|  62 ++
  drivers/video/backlight/Makefile   |   7 +
  drivers/video/backlight/lm3532_bl.c| 183 +
  drivers/video/backlight/lm3631_bl.c| 129 
  drivers/video/backlight/lm3632_bl.c| 125 
  drivers/video/backlight/lm3633_bl.c| 210 ++
  drivers/video/backlight/lm3695_bl.c|  91 +++
  drivers/video/backlight/lm3697_bl.c| 187 +
  drivers/video/backlight/ti-lmu-backlight.c | 429 
  drivers/video/backlight/ti-lmu-backlight.h | 152 +
  include/linux/mfd/ti-lmu-register.h| 277 
  include/linux/mfd/ti-lmu.h |  81 +++
  30 files changed, 4270 insertions(+)
  create mode 100644 Documentation/ABI/testing/sysfs-class-led-lm3633
  create mode 100644 Documentation/devicetree/bindings/hwmon/ti-lmu-hwmon.txt
  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3633.txt
  create mode 100644 Documentation/devicetree/bindings/mfd/ti-lmu.txt
  create mode 100644 
Documentation/devicetree/bindings/regulator/lm363x-regulator.txt
  create mode 100644 
Documentation/devicetree/bindings/video/backlight/ti-lmu-backlight.txt
  create mode 100644 drivers/hwmon/ti-lmu-hwmon.c
  create mode 100644 drivers/leds/leds-lm3633.c
  create mode 100644 drivers/mfd/ti-lmu.c
  create mode 100644 

Re: [PATCH RESEND 08/16] backlight: ti-lmu-backlight: add LM3532 driver

[Kim, Milo]

Ouch! The notifier.h must be included. I didn't notice this error in ARM 
architecture. Let me fix this error in next patch-set.

Thanks a lot for catching this!

Best regards,
Milo

diff --git a/include/linux/mfd/ti-lmu.h b/include/linux/mfd/ti-lmu.h
index eeb6b9e..44268c7 100644
--- a/include/linux/mfd/ti-lmu.h
+++ b/include/linux/mfd/ti-lmu.h
@@ -14,6 +14,7 @@
 #define __MFD_TI_LMU_H__

 #include 
+#include 
 #include 

 /* Notifier event */

On 11/2/2015 2:37 PM, kbuild test robot wrote:

Hi Milo,

[auto build test ERROR on ljones-mfd/for-mfd-next -- if it's inappropriate 
base, please suggest rules for selecting the more suitable base]

url:
https://github.com/0day-ci/linux/commits/Milo-Kim/Support-TI-LMU-devices/20151102-130804
config: mips-allyesconfig (attached as .config)
reproduce:
 wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
 chmod +x ~/bin/make.cross
 # save the attached .config to linux build tree
 make.cross ARCH=mips

All errors (new ones prefixed by >>):

In file included from drivers/video/backlight/lm3532_bl.c:14:0:

include/linux/mfd/ti-lmu.h:74:32: error: field 'notifier' has incomplete type

  struct blocking_notifier_head notifier;
^

vim +/notifier +74 include/linux/mfd/ti-lmu.h

51194f72 Milo Kim 2015-11-02  68   * @notifier: Notifier for reporting hwmon 
event
51194f72 Milo Kim 2015-11-02  69   */
51194f72 Milo Kim 2015-11-02  70  struct ti_lmu {
51194f72 Milo Kim 2015-11-02  71struct device *dev;
51194f72 Milo Kim 2015-11-02  72struct regmap *regmap;
51194f72 Milo Kim 2015-11-02  73int en_gpio;
51194f72 Milo Kim 2015-11-02 @74struct blocking_notifier_head notifier;
51194f72 Milo Kim 2015-11-02  75  };
51194f72 Milo Kim 2015-11-02  76
51194f72 Milo Kim 2015-11-02  77  int ti_lmu_read_byte(struct ti_lmu *lmu, u8 
reg, u8 *read);

:: The code at line 74 was first introduced by commit
:: 51194f72221b7b83a8f6a5ba9bf0c49a10233ca2 mfd: add TI LMU driver

:: TO: Milo Kim <milo@ti.com>
:: CC: 0day robot <fengguang...@intel.com>

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] leds:lp55xx: Correct Kconfig dependency for f/w user helper

[Kim, Milo]

Hi Takashi,

On 9/8/2015 2:06 PM, Takashi Iwai wrote:

On Tue, 08 Sep 2015 02:30:07 +0200,
Kim, Milo wrote:


Hi Takashi,

On 9/7/2015 11:19 PM, Jacek Anaszewski wrote:

Hi Takashi,

Thanks for chasing this.
Milo, could you express your opinion?

On 09/07/2015 02:25 PM, Takashi Iwai wrote:

The commit [b67893206fc0: leds:lp55xx: fix firmware loading error]
tries to address the firmware file handling with user helper, but it
sets a wrong Kconfig CONFIG_FW_LOADER_USER_HELPER_FALLBACK.  Since the
wrong option was enabled, the system got a regression -- it suffers
from the unexpected long delays for non-present firmware files.

This patch corrects the Kconfig dependency to the right one,
CONFIG_FW_LOADER_USER_HELPER.  This doesn't change the fallback
behavior but only enables UMH when needed.

Bugzilla: https://bugzilla.opensuse.org/show_bug.cgi?id=944661
Fixes: b67893206fc0 ('leds:lp55xx: fix firmware loading error')
Cc:  # v4.2+
Signed-off-by: Takashi Iwai 
---
drivers/leds/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 70f4255ff291..2ba52bc2e174 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -229,7 +229,7 @@ config LEDS_LP55XX_COMMON
tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501"
depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501
select FW_LOADER
-   select FW_LOADER_USER_HELPER_FALLBACK
+   select FW_LOADER_USER_HELPER
help
  This option supports common operations for LP5521/5523/55231/5562/8501
  devices.


Thank for catching this. It seems I misunderstood firmware helper
configuration. LP55xx driver uses firmware interface to activate LED
visual effect. So this driver enables FW_LOADER_USER_HELPER_FALLBACK and
calls request_firmware_nowait() without uevent. Then, it will try to
load raw data manually when binary(firmware) file doesn't exist.

I'm still not clear what the difference is between FW_LOADER_USER_HELPER
and FW_LOADER_USER_HELPER_FALLBACK. Kconfig description makes me confused.
Could you explain it in more details?


FW_LOADER_USER_HELPER_FALLBACK globally enables the fallback to user
helper mode when no file is loaded by the direct f/w loader.  It
automatically sets FW_LOADER_USER_HELPER.

OTOH, when FW_LOADER_USER_HELPER is set, requeset_firmware_nowait()
does user mode fallback only when uevent (the second) argument is
false.  Note that this is a special case.  In the usual cases --
uevent = true or request_firmware() -- its doesn't enable the
fallback.


Yes, I misunderstood here. lp55xx driver needs to enable user mode 
helper as *fallback*, so FW_LOADER_USER_HELPER_FALLBACK was set wrong.
Eventually, it enables the option flag, 'FW_OPT_USERHELPER'. So it 
affects other drivers which call request_firmware().



The fallback to user helper mode is bad for the recent udev, since
udev already dropped the f/w support code completely.  Thus every
non-existing f/w load will result in 60 seconds stall.


However, timeout is changed to MAX_JIFFY_OFFSET when FW_OPT_UEVENT flag 
is not set.


static int _request_firmware_load(struct firmware_priv *fw_priv,
  unsigned int opt_flags, long timeout)
{
(snip)

if (opt_flags & FW_OPT_UEVENT) {
buf->need_uevent = true;
dev_set_uevent_suppress(f_dev, false);
dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id);
kobject_uevent(_priv->dev.kobj, KOBJ_ADD);
} else {
timeout = MAX_JIFFY_OFFSET;
}

retval = wait_for_completion_interruptible_timeout(>completion,
timeout);
}

It will take too long to get the result. I don't know the reason why 
timeout was modified in the commit [68ff2a00dbf5: firmware_loader: 
handle timeout via wait_for_completion_interruptible_timeout()].
Moreover, this time value is not identical to the result of 
timeout_show(). Is it OK to remove the line as follows?


diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 171841a..8187404 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -909,8 +909,6 @@ static int _request_firmware_load(struct 
firmware_priv *fw_priv,

dev_set_uevent_suppress(f_dev, false);
dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id);
kobject_uevent(_priv->dev.kobj, KOBJ_ADD);
-   } else {
-   timeout = MAX_JIFFY_OFFSET;
}

retval = wait_for_completion_interruptible_timeout(>completion,

If the driver requires longer loading time, then it can be done by 
updating '/sys/class/firmware/timeout'.



In short, FW_LOAD_USER_HELPER_FALLBACK is present mostly only for
older systems, just for compatibility.  For drivers that need the no
direct f/w load a

Re: [PATCH] leds:lp55xx: Correct Kconfig dependency for f/w user helper

[Kim, Milo]

Hi Takashi,

On 9/8/2015 2:06 PM, Takashi Iwai wrote:

On Tue, 08 Sep 2015 02:30:07 +0200,
Kim, Milo wrote:


Hi Takashi,

On 9/7/2015 11:19 PM, Jacek Anaszewski wrote:

Hi Takashi,

Thanks for chasing this.
Milo, could you express your opinion?

On 09/07/2015 02:25 PM, Takashi Iwai wrote:

The commit [b67893206fc0: leds:lp55xx: fix firmware loading error]
tries to address the firmware file handling with user helper, but it
sets a wrong Kconfig CONFIG_FW_LOADER_USER_HELPER_FALLBACK.  Since the
wrong option was enabled, the system got a regression -- it suffers
from the unexpected long delays for non-present firmware files.

This patch corrects the Kconfig dependency to the right one,
CONFIG_FW_LOADER_USER_HELPER.  This doesn't change the fallback
behavior but only enables UMH when needed.

Bugzilla: https://bugzilla.opensuse.org/show_bug.cgi?id=944661
Fixes: b67893206fc0 ('leds:lp55xx: fix firmware loading error')
Cc: <sta...@vger.kernel.org> # v4.2+
Signed-off-by: Takashi Iwai <ti...@suse.de>
---
drivers/leds/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 70f4255ff291..2ba52bc2e174 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -229,7 +229,7 @@ config LEDS_LP55XX_COMMON
tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501"
depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501
select FW_LOADER
-   select FW_LOADER_USER_HELPER_FALLBACK
+   select FW_LOADER_USER_HELPER
help
  This option supports common operations for LP5521/5523/55231/5562/8501
  devices.


Thank for catching this. It seems I misunderstood firmware helper
configuration. LP55xx driver uses firmware interface to activate LED
visual effect. So this driver enables FW_LOADER_USER_HELPER_FALLBACK and
calls request_firmware_nowait() without uevent. Then, it will try to
load raw data manually when binary(firmware) file doesn't exist.

I'm still not clear what the difference is between FW_LOADER_USER_HELPER
and FW_LOADER_USER_HELPER_FALLBACK. Kconfig description makes me confused.
Could you explain it in more details?


FW_LOADER_USER_HELPER_FALLBACK globally enables the fallback to user
helper mode when no file is loaded by the direct f/w loader.  It
automatically sets FW_LOADER_USER_HELPER.

OTOH, when FW_LOADER_USER_HELPER is set, requeset_firmware_nowait()
does user mode fallback only when uevent (the second) argument is
false.  Note that this is a special case.  In the usual cases --
uevent = true or request_firmware() -- its doesn't enable the
fallback.


Yes, I misunderstood here. lp55xx driver needs to enable user mode 
helper as *fallback*, so FW_LOADER_USER_HELPER_FALLBACK was set wrong.
Eventually, it enables the option flag, 'FW_OPT_USERHELPER'. So it 
affects other drivers which call request_firmware().



The fallback to user helper mode is bad for the recent udev, since
udev already dropped the f/w support code completely.  Thus every
non-existing f/w load will result in 60 seconds stall.


However, timeout is changed to MAX_JIFFY_OFFSET when FW_OPT_UEVENT flag 
is not set.


static int _request_firmware_load(struct firmware_priv *fw_priv,
  unsigned int opt_flags, long timeout)
{
(snip)

if (opt_flags & FW_OPT_UEVENT) {
buf->need_uevent = true;
dev_set_uevent_suppress(f_dev, false);
dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id);
kobject_uevent(_priv->dev.kobj, KOBJ_ADD);
} else {
timeout = MAX_JIFFY_OFFSET;
}

retval = wait_for_completion_interruptible_timeout(>completion,
timeout);
}

It will take too long to get the result. I don't know the reason why 
timeout was modified in the commit [68ff2a00dbf5: firmware_loader: 
handle timeout via wait_for_completion_interruptible_timeout()].
Moreover, this time value is not identical to the result of 
timeout_show(). Is it OK to remove the line as follows?


diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 171841a..8187404 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -909,8 +909,6 @@ static int _request_firmware_load(struct 
firmware_priv *fw_priv,

dev_set_uevent_suppress(f_dev, false);
dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id);
kobject_uevent(_priv->dev.kobj, KOBJ_ADD);
-   } else {
-   timeout = MAX_JIFFY_OFFSET;
}

retval = wait_for_completion_interruptible_timeout(>completion,

If the driver requires longer loading time, then it can be done by 
updating '/sys/class/firmware/timeout'.



In short, FW_LOAD_USER_HELPER_FALLBACK is present mostly only for
older systems, just for compa

Re: [PATCH] leds:lp55xx: Correct Kconfig dependency for f/w user helper

[Kim, Milo]

Hi Takashi,

On 9/7/2015 11:19 PM, Jacek Anaszewski wrote:

Hi Takashi,

Thanks for chasing this.
Milo, could you express your opinion?

On 09/07/2015 02:25 PM, Takashi Iwai wrote:

The commit [b67893206fc0: leds:lp55xx: fix firmware loading error]
tries to address the firmware file handling with user helper, but it
sets a wrong Kconfig CONFIG_FW_LOADER_USER_HELPER_FALLBACK.  Since the
wrong option was enabled, the system got a regression -- it suffers
from the unexpected long delays for non-present firmware files.

This patch corrects the Kconfig dependency to the right one,
CONFIG_FW_LOADER_USER_HELPER.  This doesn't change the fallback
behavior but only enables UMH when needed.

Bugzilla: https://bugzilla.opensuse.org/show_bug.cgi?id=944661
Fixes: b67893206fc0 ('leds:lp55xx: fix firmware loading error')
Cc:  # v4.2+
Signed-off-by: Takashi Iwai 
---
   drivers/leds/Kconfig | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 70f4255ff291..2ba52bc2e174 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -229,7 +229,7 @@ config LEDS_LP55XX_COMMON
tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501"
depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501
select FW_LOADER
-   select FW_LOADER_USER_HELPER_FALLBACK
+   select FW_LOADER_USER_HELPER
help
  This option supports common operations for LP5521/5523/55231/5562/8501
  devices.


Thank for catching this. It seems I misunderstood firmware helper 
configuration. LP55xx driver uses firmware interface to activate LED 
visual effect. So this driver enables FW_LOADER_USER_HELPER_FALLBACK and 
calls request_firmware_nowait() without uevent. Then, it will try to 
load raw data manually when binary(firmware) file doesn't exist.


I'm still not clear what the difference is between FW_LOADER_USER_HELPER 
and FW_LOADER_USER_HELPER_FALLBACK. Kconfig description makes me confused.

Could you explain it in more details?

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] leds:lp55xx: Correct Kconfig dependency for f/w user helper

[Kim, Milo]

Hi Takashi,

On 9/7/2015 11:19 PM, Jacek Anaszewski wrote:

Hi Takashi,

Thanks for chasing this.
Milo, could you express your opinion?

On 09/07/2015 02:25 PM, Takashi Iwai wrote:

The commit [b67893206fc0: leds:lp55xx: fix firmware loading error]
tries to address the firmware file handling with user helper, but it
sets a wrong Kconfig CONFIG_FW_LOADER_USER_HELPER_FALLBACK.  Since the
wrong option was enabled, the system got a regression -- it suffers
from the unexpected long delays for non-present firmware files.

This patch corrects the Kconfig dependency to the right one,
CONFIG_FW_LOADER_USER_HELPER.  This doesn't change the fallback
behavior but only enables UMH when needed.

Bugzilla: https://bugzilla.opensuse.org/show_bug.cgi?id=944661
Fixes: b67893206fc0 ('leds:lp55xx: fix firmware loading error')
Cc:  # v4.2+
Signed-off-by: Takashi Iwai 
---
   drivers/leds/Kconfig | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 70f4255ff291..2ba52bc2e174 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -229,7 +229,7 @@ config LEDS_LP55XX_COMMON
tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501"
depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501
select FW_LOADER
-   select FW_LOADER_USER_HELPER_FALLBACK
+   select FW_LOADER_USER_HELPER
help
  This option supports common operations for LP5521/5523/55231/5562/8501
  devices.


Thank for catching this. It seems I misunderstood firmware helper 
configuration. LP55xx driver uses firmware interface to activate LED 
visual effect. So this driver enables FW_LOADER_USER_HELPER_FALLBACK and 
calls request_firmware_nowait() without uevent. Then, it will try to 
load raw data manually when binary(firmware) file doesn't exist.


I'm still not clear what the difference is between FW_LOADER_USER_HELPER 
and FW_LOADER_USER_HELPER_FALLBACK. Kconfig description makes me confused.

Could you explain it in more details?

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC 0/4] of: introduce of_dev_get_platdata()

[Kim, Milo]

Hi Rob,

On 8/28/2015 9:59 PM, Rob Herring wrote:

On Fri, Aug 28, 2015 at 4:12 AM, Milo Kim  wrote:

>New function, 'of_dev_get_platdata()'
>  - provides unified handling of getting device platform data
>  - supports DT and non-DT(legacy) cases
>  - removes duplicated code from each driver
>  - keeps driver specific code simple in each driver

This works in cases where DT data and platform_data are aligned. In
many cases they are not. A common binding problem is people blindly
copying platform_data fields to DT properties for things that are not
h/w properties. I worry that this would encourage this behavior.


OK, got your point. Thanks.



We already have a generalized method for retrieving properties
independent of DT or ACPI. There's no reason this couldn't be extended
to retrieve properties out of platform_data using the same interface.

Also, perhaps in some drivers we can remove platform_data now if all
users are converted to DT.


This work requires two steps.
1) remove platform data configuration from board-*.c
2) if all host/platform support only the DT, then move device platform 
data structure in a header to each driver space.


However, I'm concerning about some use case.
After this cleanup, drivers may not work properly in some platforms 
which do not support the DT. There is no way to configure HW properties.
Drivers should run on any kind of hosts(processors) so I'm thinking 
about the compatibility/dependency issue. Am I missing something?


Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC 0/4] of: introduce of_dev_get_platdata()

[Kim, Milo]

Hi Rob,

On 8/28/2015 9:59 PM, Rob Herring wrote:

On Fri, Aug 28, 2015 at 4:12 AM, Milo Kim<milo@ti.com>  wrote:

>New function, 'of_dev_get_platdata()'
>  - provides unified handling of getting device platform data
>  - supports DT and non-DT(legacy) cases
>  - removes duplicated code from each driver
>  - keeps driver specific code simple in each driver

This works in cases where DT data and platform_data are aligned. In
many cases they are not. A common binding problem is people blindly
copying platform_data fields to DT properties for things that are not
h/w properties. I worry that this would encourage this behavior.


OK, got your point. Thanks.



We already have a generalized method for retrieving properties
independent of DT or ACPI. There's no reason this couldn't be extended
to retrieve properties out of platform_data using the same interface.

Also, perhaps in some drivers we can remove platform_data now if all
users are converted to DT.


This work requires two steps.
1) remove platform data configuration from board-*.c
2) if all host/platform support only the DT, then move device platform 
data structure in a header to each driver space.


However, I'm concerning about some use case.
After this cleanup, drivers may not work properly in some platforms 
which do not support the DT. There is no way to configure HW properties.
Drivers should run on any kind of hosts(processors) so I'm thinking 
about the compatibility/dependency issue. Am I missing something?


Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] backlight: lp855x: Make sure props struct is zeroed

[Kim, Milo]

On 8/28/2015 2:41 AM, Bjorn Andersson wrote:

From: Werner Johansson 

The driver occasionally got stuck in suspend mode or other strange
states as those parts of the props struct were never initialized.


Acked-by: Milo Kim 

Thanks for catching this.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] backlight: lp855x: Make sure props struct is zeroed

[Kim, Milo]

On 8/28/2015 2:41 AM, Bjorn Andersson wrote:

From: Werner Johansson werner.johans...@sonymobile.com

The driver occasionally got stuck in suspend mode or other strange
states as those parts of the props struct were never initialized.


Acked-by: Milo Kim milo@ti.com

Thanks for catching this.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Kernel Oops: btusb: 4.2rc1 System lockup with BT dongle insert - log attached

[Kim, Milo]

Hi Simon,

On 7/17/2015 3:14 PM, si...@mungewell.org wrote:



It looks like the firmware 'opt_flags' must be different, so this may be a
contributing factor.


Plot thickens kernel config has changed since I built 4.1.0rc7, but I
don't recall doing it or starting a fresh.

/boot/config-4.1.0-rc7+
--
CONFIG_PREVENT_FIRMWARE_BUILD=y
CONFIG_FW_LOADER=y
CONFIG_FIRMWARE_IN_KERNEL=y
CONFIG_EXTRA_FIRMWARE=""
CONFIG_FW_LOADER_USER_HELPER=y
# CONFIG_FW_LOADER_USER_HELPER_FALLBACK is not set
CONFIG_WANT_DEV_COREDUMP=y
CONFIG_ALLOW_DEV_COREDUMP=y
CONFIG_DEV_COREDUMP=y
--

/boot/config-4.2.0-rc1+
--
CONFIG_PREVENT_FIRMWARE_BUILD=y
CONFIG_FW_LOADER=y
CONFIG_FIRMWARE_IN_KERNEL=y
CONFIG_EXTRA_FIRMWARE=""
CONFIG_FW_LOADER_USER_HELPER=y
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y   <!!!
CONFIG_WANT_DEV_COREDUMP=y
CONFIG_ALLOW_DEV_COREDUMP=y
CONFIG_DEV_COREDUMP=y
--


Has a kconfig forced a change? Grrr
--
$ git blame ./drivers/leds/Kconfig
--
c93d08fa7 (Milo(Woogyom) Kim 2013-02-05 18:01:23 +0900 228) config
LEDS_LP55XX_COMMON
33b3a561f (Kim, Milo 2013-07-09 02:11:37 -0700 229)
tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501"
33b3a561f (Kim, Milo 2013-07-09 02:11:37 -0700 230)
depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501
10c06d178 (Milo(Woogyom) Kim 2013-02-05 19:17:20 +0900 231)
select FW_LOADER
b67893206 (Milo Kim  2015-06-28 17:39:14 -0700 232)
select FW_LOADER_USER_HELPER_FALLBACK
<-
c93d08fa7 (Milo(Woogyom) Kim 2013-02-05 18:01:23 +0900 233) help
33b3a561f (Kim, Milo 2013-07-09 02:11:37 -0700 234)
This option supports common operations for LP5521/5523/55231/5562/8501
c93d08fa7 (Milo(Woogyom) Kim 2013-02-05 18:01:23 +0900 235)
devices.
--

So in summary this problem is showing up now as the 'User Helper Fallback'
is now forced on, obviously the underlying problem needs to be fixed - but
I don't know when it crept in.



The 'CONFIG_FW_LOADER_USER_HELPER_FALLBACK' enables to load firmware 
data manually by accessing /sys/class/firmware//data. It runs in 
case the firmware file is missing.
This user helper fallback will be enabled if one of LP55xx driver is 
included in your dot config. Please see my patch below.


https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/leds?id=b67893206fc0a0e8af87130e67f3d8ae553fc87c

However, I'm not sure why this affects your system lockup. Can I have 
more details?


Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Kernel Oops: btusb: 4.2rc1 System lockup with BT dongle insert - log attached

[Kim, Milo]

Hi Simon,

On 7/17/2015 3:14 PM, si...@mungewell.org wrote:



It looks like the firmware 'opt_flags' must be different, so this may be a
contributing factor.


Plot thickens kernel config has changed since I built 4.1.0rc7, but I
don't recall doing it or starting a fresh.

/boot/config-4.1.0-rc7+
--
CONFIG_PREVENT_FIRMWARE_BUILD=y
CONFIG_FW_LOADER=y
CONFIG_FIRMWARE_IN_KERNEL=y
CONFIG_EXTRA_FIRMWARE=
CONFIG_FW_LOADER_USER_HELPER=y
# CONFIG_FW_LOADER_USER_HELPER_FALLBACK is not set
CONFIG_WANT_DEV_COREDUMP=y
CONFIG_ALLOW_DEV_COREDUMP=y
CONFIG_DEV_COREDUMP=y
--

/boot/config-4.2.0-rc1+
--
CONFIG_PREVENT_FIRMWARE_BUILD=y
CONFIG_FW_LOADER=y
CONFIG_FIRMWARE_IN_KERNEL=y
CONFIG_EXTRA_FIRMWARE=
CONFIG_FW_LOADER_USER_HELPER=y
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y   !!!
CONFIG_WANT_DEV_COREDUMP=y
CONFIG_ALLOW_DEV_COREDUMP=y
CONFIG_DEV_COREDUMP=y
--


Has a kconfig forced a change? Grrr
--
$ git blame ./drivers/leds/Kconfig
--
c93d08fa7 (Milo(Woogyom) Kim 2013-02-05 18:01:23 +0900 228) config
LEDS_LP55XX_COMMON
33b3a561f (Kim, Milo 2013-07-09 02:11:37 -0700 229)
tristate Common Driver for TI/National LP5521/5523/55231/5562/8501
33b3a561f (Kim, Milo 2013-07-09 02:11:37 -0700 230)
depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501
10c06d178 (Milo(Woogyom) Kim 2013-02-05 19:17:20 +0900 231)
select FW_LOADER
b67893206 (Milo Kim  2015-06-28 17:39:14 -0700 232)
select FW_LOADER_USER_HELPER_FALLBACK
-
c93d08fa7 (Milo(Woogyom) Kim 2013-02-05 18:01:23 +0900 233) help
33b3a561f (Kim, Milo 2013-07-09 02:11:37 -0700 234)
This option supports common operations for LP5521/5523/55231/5562/8501
c93d08fa7 (Milo(Woogyom) Kim 2013-02-05 18:01:23 +0900 235)
devices.
--

So in summary this problem is showing up now as the 'User Helper Fallback'
is now forced on, obviously the underlying problem needs to be fixed - but
I don't know when it crept in.



The 'CONFIG_FW_LOADER_USER_HELPER_FALLBACK' enables to load firmware 
data manually by accessing /sys/class/firmware/name/data. It runs in 
case the firmware file is missing.
This user helper fallback will be enabled if one of LP55xx driver is 
included in your dot config. Please see my patch below.


https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/leds?id=b67893206fc0a0e8af87130e67f3d8ae553fc87c

However, I'm not sure why this affects your system lockup. Can I have 
more details?


Best regards,
Milo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] backlight: lp855x: use private data for regulator control

[Kim, Milo]

Hi Paul,

On 7/11/2015 5:52 AM, Kim, Milo wrote:

Hi Paul,

On 7/11/2015 5:49 AM, Sean Paul wrote:

On Fri, Jul 10, 2015 at 4:43 PM, Kim, Milo  wrote:

Hi Paul,


On 7/11/2015 12:01 AM, Sean Paul wrote:


On Fri, Jul 10, 2015 at 4:26 AM, Milo Kim  wrote:


LP855x backlight device can be enabled by external VDD input.
The 'supply' data is used for this purpose.
It's kind of private data which runs internally, so there is no reason to
expose to the platform data.

And devm_regulator_get() is moved from _parse_dt() to _probe().
Regulator consumer(lp855x) can control regulator not only from DT but
also
from platform data configuration in a source file such like board-*.c.

If 'power' regulator driver is not ready, lp855x should continue to work
because the power supply control is optional. So -EPROBE_DEFER return
code
is removed.

v1->v2:
 Keeps optional property '-supply' in LP855x DT binding.

Cc: Sean Paul 
Cc: Jingoo Han 
Cc: Lee Jones 
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Milo Kim 
---
drivers/video/backlight/lp855x_bl.c  | 20 +---
include/linux/platform_data/lp855x.h |  2 --
2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/video/backlight/lp855x_bl.c
b/drivers/video/backlight/lp855x_bl.c
index a26d3bb..277d5ca 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -73,6 +73,7 @@ struct lp855x {
   struct device *dev;
   struct lp855x_platform_data *pdata;
   struct pwm_device *pwm;
+   struct regulator *supply;   /* regulator for VDD input */
};

static int lp855x_write_byte(struct lp855x *lp, u8 reg, u8 data)
@@ -384,13 +385,6 @@ static int lp855x_parse_dt(struct lp855x *lp)
   pdata->rom_data = [0];
   }

-   pdata->supply = devm_regulator_get(dev, "power");
-   if (IS_ERR(pdata->supply)) {
-   if (PTR_ERR(pdata->supply) == -EPROBE_DEFER)
-   return -EPROBE_DEFER;
-   pdata->supply = NULL;
-   }
-
   lp->pdata = pdata;

   return 0;
@@ -431,8 +425,12 @@ static int lp855x_probe(struct i2c_client *cl, const
struct i2c_device_id *id)
   else
   lp->mode = REGISTER_BASED;

-   if (lp->pdata->supply) {
-   ret = regulator_enable(lp->pdata->supply);
+   lp->supply = devm_regulator_get(lp->dev, "power");
+   if (IS_ERR(lp->supply))
+   lp->supply = NULL;




Hi Milo,
You removed the probe deferral handling on the regulator and broke
probe deferral in cases where the regulator isn't ready.



This power supply is optional. Even if lp855x can not get regulator driver,
it should work. (And I saw same comment in the DT. The 'power-supply'
property is optional). So -EPORBE_DEFER is not necessary in _probe().



I respectfully disagree. devm_regulator_get can return EPROBE_DEFER if
the regulator is valid (and specified in the dt), but not ready to be
used yet. In this case, your patch will assume it doesn't exist and
will never use it. This Is Bad.


So do you think this power supply should be mandatory in this driver?


The devm_regulator_get_optinonal() seems the right API for this case.
Let me take a look at and get back to you.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] backlight: lp855x: use private data for regulator control

[Kim, Milo]

Hi Paul,

On 7/11/2015 5:49 AM, Sean Paul wrote:

On Fri, Jul 10, 2015 at 4:43 PM, Kim, Milo  wrote:

Hi Paul,


On 7/11/2015 12:01 AM, Sean Paul wrote:


On Fri, Jul 10, 2015 at 4:26 AM, Milo Kim  wrote:


LP855x backlight device can be enabled by external VDD input.
The 'supply' data is used for this purpose.
It's kind of private data which runs internally, so there is no reason to
expose to the platform data.

And devm_regulator_get() is moved from _parse_dt() to _probe().
Regulator consumer(lp855x) can control regulator not only from DT but
also
from platform data configuration in a source file such like board-*.c.

If 'power' regulator driver is not ready, lp855x should continue to work
because the power supply control is optional. So -EPROBE_DEFER return
code
is removed.

v1->v2:
Keeps optional property '-supply' in LP855x DT binding.

Cc: Sean Paul 
Cc: Jingoo Han 
Cc: Lee Jones 
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Milo Kim 
---
   drivers/video/backlight/lp855x_bl.c  | 20 +---
   include/linux/platform_data/lp855x.h |  2 --
   2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/video/backlight/lp855x_bl.c
b/drivers/video/backlight/lp855x_bl.c
index a26d3bb..277d5ca 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -73,6 +73,7 @@ struct lp855x {
  struct device *dev;
  struct lp855x_platform_data *pdata;
  struct pwm_device *pwm;
+   struct regulator *supply;   /* regulator for VDD input */
   };

   static int lp855x_write_byte(struct lp855x *lp, u8 reg, u8 data)
@@ -384,13 +385,6 @@ static int lp855x_parse_dt(struct lp855x *lp)
  pdata->rom_data = [0];
  }

-   pdata->supply = devm_regulator_get(dev, "power");
-   if (IS_ERR(pdata->supply)) {
-   if (PTR_ERR(pdata->supply) == -EPROBE_DEFER)
-   return -EPROBE_DEFER;
-   pdata->supply = NULL;
-   }
-
  lp->pdata = pdata;

  return 0;
@@ -431,8 +425,12 @@ static int lp855x_probe(struct i2c_client *cl, const
struct i2c_device_id *id)
  else
  lp->mode = REGISTER_BASED;

-   if (lp->pdata->supply) {
-   ret = regulator_enable(lp->pdata->supply);
+   lp->supply = devm_regulator_get(lp->dev, "power");
+   if (IS_ERR(lp->supply))
+   lp->supply = NULL;




Hi Milo,
You removed the probe deferral handling on the regulator and broke
probe deferral in cases where the regulator isn't ready.



This power supply is optional. Even if lp855x can not get regulator driver,
it should work. (And I saw same comment in the DT. The 'power-supply'
property is optional). So -EPORBE_DEFER is not necessary in _probe().



I respectfully disagree. devm_regulator_get can return EPROBE_DEFER if
the regulator is valid (and specified in the dt), but not ready to be
used yet. In this case, your patch will assume it doesn't exist and
will never use it. This Is Bad.


So do you think this power supply should be mandatory in this driver?

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] backlight: lp855x: use private data for regulator control

[Kim, Milo]

Hi Paul,

On 7/11/2015 12:01 AM, Sean Paul wrote:

On Fri, Jul 10, 2015 at 4:26 AM, Milo Kim  wrote:

LP855x backlight device can be enabled by external VDD input.
The 'supply' data is used for this purpose.
It's kind of private data which runs internally, so there is no reason to
expose to the platform data.

And devm_regulator_get() is moved from _parse_dt() to _probe().
Regulator consumer(lp855x) can control regulator not only from DT but also
from platform data configuration in a source file such like board-*.c.

If 'power' regulator driver is not ready, lp855x should continue to work
because the power supply control is optional. So -EPROBE_DEFER return code
is removed.

v1->v2:
   Keeps optional property '-supply' in LP855x DT binding.

Cc: Sean Paul 
Cc: Jingoo Han 
Cc: Lee Jones 
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Milo Kim 
---
  drivers/video/backlight/lp855x_bl.c  | 20 +---
  include/linux/platform_data/lp855x.h |  2 --
  2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/video/backlight/lp855x_bl.c 
b/drivers/video/backlight/lp855x_bl.c
index a26d3bb..277d5ca 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -73,6 +73,7 @@ struct lp855x {
 struct device *dev;
 struct lp855x_platform_data *pdata;
 struct pwm_device *pwm;
+   struct regulator *supply;   /* regulator for VDD input */
  };

  static int lp855x_write_byte(struct lp855x *lp, u8 reg, u8 data)
@@ -384,13 +385,6 @@ static int lp855x_parse_dt(struct lp855x *lp)
 pdata->rom_data = [0];
 }

-   pdata->supply = devm_regulator_get(dev, "power");
-   if (IS_ERR(pdata->supply)) {
-   if (PTR_ERR(pdata->supply) == -EPROBE_DEFER)
-   return -EPROBE_DEFER;
-   pdata->supply = NULL;
-   }
-
 lp->pdata = pdata;

 return 0;
@@ -431,8 +425,12 @@ static int lp855x_probe(struct i2c_client *cl, const 
struct i2c_device_id *id)
 else
 lp->mode = REGISTER_BASED;

-   if (lp->pdata->supply) {
-   ret = regulator_enable(lp->pdata->supply);
+   lp->supply = devm_regulator_get(lp->dev, "power");
+   if (IS_ERR(lp->supply))
+   lp->supply = NULL;



Hi Milo,
You removed the probe deferral handling on the regulator and broke
probe deferral in cases where the regulator isn't ready.


This power supply is optional. Even if lp855x can not get regulator 
driver, it should work. (And I saw same comment in the DT. The 
'power-supply' property is optional). So -EPORBE_DEFER is not necessary 
in _probe().


Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] backlight: lp855x: use private data for regulator control

[Kim, Milo]

Hi Paul,

On 7/11/2015 12:01 AM, Sean Paul wrote:

On Fri, Jul 10, 2015 at 4:26 AM, Milo Kim milo@ti.com wrote:

LP855x backlight device can be enabled by external VDD input.
The 'supply' data is used for this purpose.
It's kind of private data which runs internally, so there is no reason to
expose to the platform data.

And devm_regulator_get() is moved from _parse_dt() to _probe().
Regulator consumer(lp855x) can control regulator not only from DT but also
from platform data configuration in a source file such like board-*.c.

If 'power' regulator driver is not ready, lp855x should continue to work
because the power supply control is optional. So -EPROBE_DEFER return code
is removed.

v1-v2:
   Keeps optional property 'name-supply' in LP855x DT binding.

Cc: Sean Paul seanp...@chromium.org
Cc: Jingoo Han jingooh...@gmail.com
Cc: Lee Jones lee.jo...@linaro.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Milo Kim milo@ti.com
---
  drivers/video/backlight/lp855x_bl.c  | 20 +---
  include/linux/platform_data/lp855x.h |  2 --
  2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/video/backlight/lp855x_bl.c 
b/drivers/video/backlight/lp855x_bl.c
index a26d3bb..277d5ca 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -73,6 +73,7 @@ struct lp855x {
 struct device *dev;
 struct lp855x_platform_data *pdata;
 struct pwm_device *pwm;
+   struct regulator *supply;   /* regulator for VDD input */
  };

  static int lp855x_write_byte(struct lp855x *lp, u8 reg, u8 data)
@@ -384,13 +385,6 @@ static int lp855x_parse_dt(struct lp855x *lp)
 pdata-rom_data = rom[0];
 }

-   pdata-supply = devm_regulator_get(dev, power);
-   if (IS_ERR(pdata-supply)) {
-   if (PTR_ERR(pdata-supply) == -EPROBE_DEFER)
-   return -EPROBE_DEFER;
-   pdata-supply = NULL;
-   }
-
 lp-pdata = pdata;

 return 0;
@@ -431,8 +425,12 @@ static int lp855x_probe(struct i2c_client *cl, const 
struct i2c_device_id *id)
 else
 lp-mode = REGISTER_BASED;

-   if (lp-pdata-supply) {
-   ret = regulator_enable(lp-pdata-supply);
+   lp-supply = devm_regulator_get(lp-dev, power);
+   if (IS_ERR(lp-supply))
+   lp-supply = NULL;



Hi Milo,
You removed the probe deferral handling on the regulator and broke
probe deferral in cases where the regulator isn't ready.


This power supply is optional. Even if lp855x can not get regulator 
driver, it should work. (And I saw same comment in the DT. The 
'power-supply' property is optional). So -EPORBE_DEFER is not necessary 
in _probe().


Best regards,
Milo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] backlight: lp855x: use private data for regulator control

[Kim, Milo]

Hi Paul,

On 7/11/2015 5:49 AM, Sean Paul wrote:

On Fri, Jul 10, 2015 at 4:43 PM, Kim, Milo milo@ti.com wrote:

Hi Paul,


On 7/11/2015 12:01 AM, Sean Paul wrote:


On Fri, Jul 10, 2015 at 4:26 AM, Milo Kim milo@ti.com wrote:


LP855x backlight device can be enabled by external VDD input.
The 'supply' data is used for this purpose.
It's kind of private data which runs internally, so there is no reason to
expose to the platform data.

And devm_regulator_get() is moved from _parse_dt() to _probe().
Regulator consumer(lp855x) can control regulator not only from DT but
also
from platform data configuration in a source file such like board-*.c.

If 'power' regulator driver is not ready, lp855x should continue to work
because the power supply control is optional. So -EPROBE_DEFER return
code
is removed.

v1-v2:
Keeps optional property 'name-supply' in LP855x DT binding.

Cc: Sean Paul seanp...@chromium.org
Cc: Jingoo Han jingooh...@gmail.com
Cc: Lee Jones lee.jo...@linaro.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Milo Kim milo@ti.com
---
   drivers/video/backlight/lp855x_bl.c  | 20 +---
   include/linux/platform_data/lp855x.h |  2 --
   2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/video/backlight/lp855x_bl.c
b/drivers/video/backlight/lp855x_bl.c
index a26d3bb..277d5ca 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -73,6 +73,7 @@ struct lp855x {
  struct device *dev;
  struct lp855x_platform_data *pdata;
  struct pwm_device *pwm;
+   struct regulator *supply;   /* regulator for VDD input */
   };

   static int lp855x_write_byte(struct lp855x *lp, u8 reg, u8 data)
@@ -384,13 +385,6 @@ static int lp855x_parse_dt(struct lp855x *lp)
  pdata-rom_data = rom[0];
  }

-   pdata-supply = devm_regulator_get(dev, power);
-   if (IS_ERR(pdata-supply)) {
-   if (PTR_ERR(pdata-supply) == -EPROBE_DEFER)
-   return -EPROBE_DEFER;
-   pdata-supply = NULL;
-   }
-
  lp-pdata = pdata;

  return 0;
@@ -431,8 +425,12 @@ static int lp855x_probe(struct i2c_client *cl, const
struct i2c_device_id *id)
  else
  lp-mode = REGISTER_BASED;

-   if (lp-pdata-supply) {
-   ret = regulator_enable(lp-pdata-supply);
+   lp-supply = devm_regulator_get(lp-dev, power);
+   if (IS_ERR(lp-supply))
+   lp-supply = NULL;




Hi Milo,
You removed the probe deferral handling on the regulator and broke
probe deferral in cases where the regulator isn't ready.



This power supply is optional. Even if lp855x can not get regulator driver,
it should work. (And I saw same comment in the DT. The 'power-supply'
property is optional). So -EPORBE_DEFER is not necessary in _probe().



I respectfully disagree. devm_regulator_get can return EPROBE_DEFER if
the regulator is valid (and specified in the dt), but not ready to be
used yet. In this case, your patch will assume it doesn't exist and
will never use it. This Is Bad.


So do you think this power supply should be mandatory in this driver?

Best regards,
Milo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] backlight: lp855x: use private data for regulator control

[Kim, Milo]

Hi Paul,

On 7/11/2015 5:52 AM, Kim, Milo wrote:

Hi Paul,

On 7/11/2015 5:49 AM, Sean Paul wrote:

On Fri, Jul 10, 2015 at 4:43 PM, Kim, Milo milo@ti.com wrote:

Hi Paul,


On 7/11/2015 12:01 AM, Sean Paul wrote:


On Fri, Jul 10, 2015 at 4:26 AM, Milo Kim milo@ti.com wrote:


LP855x backlight device can be enabled by external VDD input.
The 'supply' data is used for this purpose.
It's kind of private data which runs internally, so there is no reason to
expose to the platform data.

And devm_regulator_get() is moved from _parse_dt() to _probe().
Regulator consumer(lp855x) can control regulator not only from DT but
also
from platform data configuration in a source file such like board-*.c.

If 'power' regulator driver is not ready, lp855x should continue to work
because the power supply control is optional. So -EPROBE_DEFER return
code
is removed.

v1-v2:
 Keeps optional property 'name-supply' in LP855x DT binding.

Cc: Sean Paul seanp...@chromium.org
Cc: Jingoo Han jingooh...@gmail.com
Cc: Lee Jones lee.jo...@linaro.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Milo Kim milo@ti.com
---
drivers/video/backlight/lp855x_bl.c  | 20 +---
include/linux/platform_data/lp855x.h |  2 --
2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/video/backlight/lp855x_bl.c
b/drivers/video/backlight/lp855x_bl.c
index a26d3bb..277d5ca 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -73,6 +73,7 @@ struct lp855x {
   struct device *dev;
   struct lp855x_platform_data *pdata;
   struct pwm_device *pwm;
+   struct regulator *supply;   /* regulator for VDD input */
};

static int lp855x_write_byte(struct lp855x *lp, u8 reg, u8 data)
@@ -384,13 +385,6 @@ static int lp855x_parse_dt(struct lp855x *lp)
   pdata-rom_data = rom[0];
   }

-   pdata-supply = devm_regulator_get(dev, power);
-   if (IS_ERR(pdata-supply)) {
-   if (PTR_ERR(pdata-supply) == -EPROBE_DEFER)
-   return -EPROBE_DEFER;
-   pdata-supply = NULL;
-   }
-
   lp-pdata = pdata;

   return 0;
@@ -431,8 +425,12 @@ static int lp855x_probe(struct i2c_client *cl, const
struct i2c_device_id *id)
   else
   lp-mode = REGISTER_BASED;

-   if (lp-pdata-supply) {
-   ret = regulator_enable(lp-pdata-supply);
+   lp-supply = devm_regulator_get(lp-dev, power);
+   if (IS_ERR(lp-supply))
+   lp-supply = NULL;




Hi Milo,
You removed the probe deferral handling on the regulator and broke
probe deferral in cases where the regulator isn't ready.



This power supply is optional. Even if lp855x can not get regulator driver,
it should work. (And I saw same comment in the DT. The 'power-supply'
property is optional). So -EPORBE_DEFER is not necessary in _probe().



I respectfully disagree. devm_regulator_get can return EPROBE_DEFER if
the regulator is valid (and specified in the dt), but not ready to be
used yet. In this case, your patch will assume it doesn't exist and
will never use it. This Is Bad.


So do you think this power supply should be mandatory in this driver?


The devm_regulator_get_optinonal() seems the right API for this case.
Let me take a look at and get back to you.

Best regards,
Milo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Documentation: leds-lp5523: describe master fader attributes

[Kim, Milo]

On 5/13/2015 10:15 AM, Toshi Kikuchi wrote:

Add the usage of the new attributes for master faders.

Signed-off-by: Toshi Kikuchi 


Acked-by: Milo Kim 

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] leds: lp5523: add master_fader support

[Kim, Milo]

Hi Jacek and Toshi,

On 5/12/2015 4:25 PM, Jacek Anaszewski wrote:

Hi Toshi,

On 05/11/2015 09:10 PM, Toshi Kikuchi wrote:

This patch introduces 4 new attributes:
master_fader_leds
master_fader1
master_fader2
master_fader3

Fo example, to map channel 0,6 to master_fader1,
map channel 1,7 to master_fader2,
map channel 2,8 to master_fader3, and
map channel 3,4,5 to none

  >

echo "123000123" > master_fader_leds


I propose to add ABI documentation for this driver. It already exposes
many custom attributes but I can't find documentation for them.


I agree with Jacek's comment but it would be better if we could see the 
description in lp5523 driver 
documentation(Documentation/leds/leds-lp5523.txt).


Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] leds: lp5523: add master_fader support

[Kim, Milo]

Hi Jacek and Toshi,

On 5/12/2015 4:25 PM, Jacek Anaszewski wrote:

Hi Toshi,

On 05/11/2015 09:10 PM, Toshi Kikuchi wrote:

This patch introduces 4 new attributes:
master_fader_leds
master_fader1
master_fader2
master_fader3

Fo example, to map channel 0,6 to master_fader1,
map channel 1,7 to master_fader2,
map channel 2,8 to master_fader3, and
map channel 3,4,5 to none

  

echo 123000123  master_fader_leds


I propose to add ABI documentation for this driver. It already exposes
many custom attributes but I can't find documentation for them.


I agree with Jacek's comment but it would be better if we could see the 
description in lp5523 driver 
documentation(Documentation/leds/leds-lp5523.txt).


Best regards,
Milo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Documentation: leds-lp5523: describe master fader attributes

[Kim, Milo]

On 5/13/2015 10:15 AM, Toshi Kikuchi wrote:

Add the usage of the new attributes for master faders.

Signed-off-by: Toshi Kikuchi tos...@chromium.org


Acked-by: Milo Kim milo@ti.com

Best regards,
Milo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] leds: lp5523: add master_fader support

[Kim, Milo]

Hi Bryan,

On 5/12/2015 4:10 AM, Toshi Kikuchi wrote:

This patch introduces 4 new attributes:
   master_fader_leds
   master_fader1
   master_fader2
   master_fader3

Fo example, to map channel 0,6 to master_fader1,
map channel 1,7 to master_fader2,
map channel 2,8 to master_fader3, and
map channel 3,4,5 to none

   echo "123000123" > master_fader_leds

A different factor can be set to each master_fader:

   echo 255 > master_fader1
   echo 100 > master_fader2
   echo 0 > master_fader3

Signed-off-by: Toshi Kikuchi 
Acked-by: Milo Kim 


Tested-by: Milo Kim 

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] leds: lp5523: add master_fader support

[Kim, Milo]

Hi Bryan,

On 5/12/2015 4:10 AM, Toshi Kikuchi wrote:

This patch introduces 4 new attributes:
   master_fader_leds
   master_fader1
   master_fader2
   master_fader3

Fo example, to map channel 0,6 to master_fader1,
map channel 1,7 to master_fader2,
map channel 2,8 to master_fader3, and
map channel 3,4,5 to none

   echo 123000123  master_fader_leds

A different factor can be set to each master_fader:

   echo 255  master_fader1
   echo 100  master_fader2
   echo 0  master_fader3

Signed-off-by: Toshi Kikuchi tos...@chromium.org
Acked-by: Milo Kim milo@ti.com


Tested-by: Milo Kim milo@ti.com

Best regards,
Milo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver

[Kim, Milo]

Hello Jonathan and Stanimir,

> >> See Documentation/ABI/sysfs-bus-iio
> >> Millivolts I think... We copied hwmon where possible.
> >
> > I'm a bit confused about these units. I searched references of
> > iio_read_channel_processed() and found a few.
> >
> > The iio_hwmon expecting milivolts. On the other side lp8788-charger.c
> > registers a get_property method in charger-manager.c, which expects
> > microvolts in get_batt_uV().
> It's definitely meant to be millivolts (lifted from hwmon a while back).
> See Documentation/ABI/testing/sysfs-bus-iio
> 
> Looks like we have a bug in lp8788-charger - it might be matched with one in
> lp8788-adc, but then there will be a bug there...
> 
> Cc'd Milo Kim.

'lp8788-charger' registers not charger-manager but power-supply subsystem.
'lp8788-adc' is the IIO driver.
'lp8788-charger' is the IIO consumer of lp8788-adc.

(How to communicate between lp8788-adc and lp8788-charger)
1. Application requests vbatt(battery voltage) by accessing /sys/class/power/
2. lp8788-charger asks lp8788-adc to get converted vbatt value
3. lp8788-adc get the battery ADC from the device.
   It returns calculated micro voltage to lp8788-charger.
4. lp8788-charger reports this uV value to the application
5. Application uses uV or converts to mV or V unit. It's up to the app.

Please note that battery app accesses not /sys/bus/iio but /sys/class/power/.

Best regards,
Milo

RE: [PATCH v2 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver

[Kim, Milo]

Hello Jonathan and Stanimir,

  See Documentation/ABI/sysfs-bus-iio
  Millivolts I think... We copied hwmon where possible.
 
  I'm a bit confused about these units. I searched references of
  iio_read_channel_processed() and found a few.
 
  The iio_hwmon expecting milivolts. On the other side lp8788-charger.c
  registers a get_property method in charger-manager.c, which expects
  microvolts in get_batt_uV().
 It's definitely meant to be millivolts (lifted from hwmon a while back).
 See Documentation/ABI/testing/sysfs-bus-iio
 
 Looks like we have a bug in lp8788-charger - it might be matched with one in
 lp8788-adc, but then there will be a bug there...
 
 Cc'd Milo Kim.

'lp8788-charger' registers not charger-manager but power-supply subsystem.
'lp8788-adc' is the IIO driver.
'lp8788-charger' is the IIO consumer of lp8788-adc.

(How to communicate between lp8788-adc and lp8788-charger)
1. Application requests vbatt(battery voltage) by accessing /sys/class/power/
2. lp8788-charger asks lp8788-adc to get converted vbatt value
3. lp8788-adc get the battery ADC from the device.
   It returns calculated micro voltage to lp8788-charger.
4. lp8788-charger reports this uV value to the application
5. Application uses uV or converts to mV or V unit. It's up to the app.

Please note that battery app accesses not /sys/bus/iio but /sys/class/power/.

Best regards,
Milo

RE: [PATCH 01/10] leds: lp55xx: add common data structure for program

[Kim, Milo]

Hi Bryan,

> -Original Message-
> From: Bryan Wu [mailto:coolo...@gmail.com]
> Sent: Wednesday, August 14, 2013 3:56 AM
> To: Milo Kim
> Cc: Pali Rohár; Linux LED Subsystem; lkml; Kim, Milo
> Subject: Re: [PATCH 01/10] leds: lp55xx: add common data structure for
> program
> 
> On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim  wrote:
> > LP55xx family devices have internal three program engines which are
> > used for loading LED patterns.
> > To maintain legacy device attributes, specific data structure is used,
> 'mode'
> > and 'led_mux'.
> > The mode is used for showing/storing current engine mode such like
> > disabled, load and run.
> > Then led_mux is used for showing/storing current output LED selection.
> > This is only for LP5523/55231.
> >
> 
> This patch looks good to me, but the commit message format is little bit odd
> to me. I will fix that and merge into my tree.

Thanks for your help.
Can I get more detailed information about this format problem?
I need to check my configurations.

Thanks,
Milo-
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 01/10] leds: lp55xx: add common data structure for program

[Kim, Milo]

Hi Bryan,

 -Original Message-
 From: Bryan Wu [mailto:coolo...@gmail.com]
 Sent: Wednesday, August 14, 2013 3:56 AM
 To: Milo Kim
 Cc: Pali Rohár; Linux LED Subsystem; lkml; Kim, Milo
 Subject: Re: [PATCH 01/10] leds: lp55xx: add common data structure for
 program
 
 On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim woogyom@gmail.com wrote:
  LP55xx family devices have internal three program engines which are
  used for loading LED patterns.
  To maintain legacy device attributes, specific data structure is used,
 'mode'
  and 'led_mux'.
  The mode is used for showing/storing current engine mode such like
  disabled, load and run.
  Then led_mux is used for showing/storing current output LED selection.
  This is only for LP5523/55231.
 
 
 This patch looks good to me, but the commit message format is little bit odd
 to me. I will fix that and merge into my tree.

Thanks for your help.
Can I get more detailed information about this format problem?
I need to check my configurations.

Thanks,
Milo-
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: leds-lp5523: Broken commit db6eaf8388a413a5ee1b4547ce78506b9c6456b0

[Kim, Milo]

> Hello,
> 
> git commit "leds-lp5523: use generic firmware interface"
> db6eaf8388a413a5ee1b4547ce78506b9c6456b0 introduced in kernel
> 3.10 changed user space API for modifing lp5523 led patterns via /sys.
> 
> Before this commit there were sysfs attributes engineX_mode, engineX_load and
> engineX_leds (for every engine X).
> 
> Now (after commit) there are sysfs attributes: select_engine, loading and
> data (see: 10c06d178df11b0b2b746321a80ea14241997127
> for description). Old sysfs attributes were removed.
> 
> So commit db6eaf8388a413a5ee1b4547ce78506b9c6456b0 totally broke all existing
> userspace applications which use lp5523 leds.
> 
> This also broke application MCE which is used on Nokia N900 phone for
> changing LED patterns in lp5523 led.

First of all, I'm really sorry for this conflict.

> 
> I think that kernel api which is used by userspace applications (like
> attributes in /sys) should not be changed in way that totally broke existing
> applications.
> 
> Can you fix this api problem? (maybe adding old sysfs attributes which would
> call new api function...)

I'll recover old sysfs attributes and implement them by using new functions.
Additionally, I'll do same works in LP5521 driver also.

Best Regards,
Milo
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH v2 1/4] mfd: add LP3943 MFD driver

[Kim, Milo]

> > > Although, I think the 0 = 1, 1 = 2 ... stuff is really confusing. Is
> > > there nothing we can do about that?
> >
> > OK, enum value of lp3943_pwm_output can be changed as below because
> > LP3943_PWM_INVALID is not used anymore.
> >
> > enum lp3943_pwm_output {
> > LP3943_PWM_OUT0,
> > LP3943_PWM_OUT1,
> > ...
> > LP3943_PWM_OUT15,
> > };
> >
> > Then, output index will match each enum integer value.
> > Does it make sense?
> 
> Not really. IIRC the documentation said that LED0 (which I believe you're
> calling OUT0 here) is located at pin one. So your enum above is now incorrect
> isn't it? As *_OUT0 will be 0 and not 1? Or am I missing something?

If we consider this naming as the pin control description, it maybe confusing.
However, this enum type means configurable platform data which output channel(s)
are connected to LP3943 PWM controller.

I've changed this name from _PWM_LEDx to _PWM_OUTx in the second patch because
PWM is used for not only LED function but also other operations.
Zero base index notation is derived from the datasheet.
If I remove LP3943_PWM_INVALID, then each enum type matches with
register index(or offset) exactly. (But I need to fix LP3943 PWM driver)

In the meantime, I've reviewed the pin control subsystem,
I think it is not the best way to implement LP3943 driver.
The GPIO controller is OK, but I can't make flexible pin assignment for the PWM
operation.
For example, multiple output pins can be controlled by one PWM generator.
These pin assignment are configurable - not fixed type.
And pinmux are only two cases - GPIO and PWM.
I think current driver structure is better because LP3943 uses very limited
pinctrl functionalities.
Any suggestion for this?

> > > > +static int __init lp3943_init(void) {
> > > > +   return i2c_add_driver(_driver); }
> > > > +subsys_initcall(lp3943_init);
> > > > +
> > > > +static void __exit lp3943_exit(void) {
> > > > +   i2c_del_driver(_driver);
> > > > +}
> > > > +module_exit(lp3943_exit);
> > >
> > > I think you want to replace:
> > >   lp3943_init()
> > >   lp3943_exit
> > >
> > > With:
> > >   module_i2c_driver()
> >
> > This is related with initcall sequence.
> > Some problem may happen if any GPIO or PWM consumer tries to request
> > before
> > LP3943 MFDs are added.
> > For example, a GPIO is requested in _probe() of some device.
> > Let's assume the GPIO number is in range of what LP3943 GPIO driver
> provided.
> > Then, gpio_request() will be failed because the GPIO is invalid at this
> moment.
> > If the device request again later, it will be OK, but we can't expect
> > this situation for every driver.
> > Some drivers request a GPIO only once in _probe(), other devices may
> > request a GPIO in some cases.
> > So, I think lp3943_init() should be defined as subsys_initcall()
> > instead of module_init().
> 
> No I don't think so. Instead, you should use -EPROBE_DEFER in lieu of messing
> around with initialisation orders.

OK, got it. I'll replace them with module_i2c_driver(). Thanks!

Best Regards,
Milo

RE: [PATCH v2 1/4] mfd: add LP3943 MFD driver

[Kim, Milo]

   Although, I think the 0 = 1, 1 = 2 ... stuff is really confusing. Is
   there nothing we can do about that?
 
  OK, enum value of lp3943_pwm_output can be changed as below because
  LP3943_PWM_INVALID is not used anymore.
 
  enum lp3943_pwm_output {
  LP3943_PWM_OUT0,
  LP3943_PWM_OUT1,
  ...
  LP3943_PWM_OUT15,
  };
 
  Then, output index will match each enum integer value.
  Does it make sense?
 
 Not really. IIRC the documentation said that LED0 (which I believe you're
 calling OUT0 here) is located at pin one. So your enum above is now incorrect
 isn't it? As *_OUT0 will be 0 and not 1? Or am I missing something?

If we consider this naming as the pin control description, it maybe confusing.
However, this enum type means configurable platform data which output channel(s)
are connected to LP3943 PWM controller.

I've changed this name from _PWM_LEDx to _PWM_OUTx in the second patch because
PWM is used for not only LED function but also other operations.
Zero base index notation is derived from the datasheet.
If I remove LP3943_PWM_INVALID, then each enum type matches with
register index(or offset) exactly. (But I need to fix LP3943 PWM driver)

In the meantime, I've reviewed the pin control subsystem,
I think it is not the best way to implement LP3943 driver.
The GPIO controller is OK, but I can't make flexible pin assignment for the PWM
operation.
For example, multiple output pins can be controlled by one PWM generator.
These pin assignment are configurable - not fixed type.
And pinmux are only two cases - GPIO and PWM.
I think current driver structure is better because LP3943 uses very limited
pinctrl functionalities.
Any suggestion for this?

+static int __init lp3943_init(void) {
+   return i2c_add_driver(lp3943_driver); }
+subsys_initcall(lp3943_init);
+
+static void __exit lp3943_exit(void) {
+   i2c_del_driver(lp3943_driver);
+}
+module_exit(lp3943_exit);
  
   I think you want to replace:
 lp3943_init()
 lp3943_exit
  
   With:
 module_i2c_driver()
 
  This is related with initcall sequence.
  Some problem may happen if any GPIO or PWM consumer tries to request
  before
  LP3943 MFDs are added.
  For example, a GPIO is requested in _probe() of some device.
  Let's assume the GPIO number is in range of what LP3943 GPIO driver
 provided.
  Then, gpio_request() will be failed because the GPIO is invalid at this
 moment.
  If the device request again later, it will be OK, but we can't expect
  this situation for every driver.
  Some drivers request a GPIO only once in _probe(), other devices may
  request a GPIO in some cases.
  So, I think lp3943_init() should be defined as subsys_initcall()
  instead of module_init().
 
 No I don't think so. Instead, you should use -EPROBE_DEFER in lieu of messing
 around with initialisation orders.

OK, got it. I'll replace them with module_i2c_driver(). Thanks!

Best Regards,
Milo

RE: leds-lp5523: Broken commit db6eaf8388a413a5ee1b4547ce78506b9c6456b0

[Kim, Milo]

 Hello,
 
 git commit leds-lp5523: use generic firmware interface
 db6eaf8388a413a5ee1b4547ce78506b9c6456b0 introduced in kernel
 3.10 changed user space API for modifing lp5523 led patterns via /sys.
 
 Before this commit there were sysfs attributes engineX_mode, engineX_load and
 engineX_leds (for every engine X).
 
 Now (after commit) there are sysfs attributes: select_engine, loading and
 data (see: 10c06d178df11b0b2b746321a80ea14241997127
 for description). Old sysfs attributes were removed.
 
 So commit db6eaf8388a413a5ee1b4547ce78506b9c6456b0 totally broke all existing
 userspace applications which use lp5523 leds.
 
 This also broke application MCE which is used on Nokia N900 phone for
 changing LED patterns in lp5523 led.

First of all, I'm really sorry for this conflict.

 
 I think that kernel api which is used by userspace applications (like
 attributes in /sys) should not be changed in way that totally broke existing
 applications.
 
 Can you fix this api problem? (maybe adding old sysfs attributes which would
 call new api function...)

I'll recover old sysfs attributes and implement them by using new functions.
Additionally, I'll do same works in LP5521 driver also.

Best Regards,
Milo
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a���
0��h���i