mbox series

[v7,0/6] LM3697 dedicated LED driver

Message ID 20180911170825.17789-1-dmurphy@ti.com
Headers show
Series LM3697 dedicated LED driver | expand

Message

Dan Murphy Sept. 11, 2018, 5:08 p.m. UTC
All

The LM3697 LED driver has two opportunities to be included in the Linux
kernel source.

The ti-lmu MFD driver and the leds-lm3697 driver contained in this series.

This LED driver is basic in nature and is not really suitable for the MFD
inclusion.  This device has no other function than to control the LEDs.

So this series will remove the lm3697 from the ti-lmu and add the driver
in the LEDs directory.

Dan Murphy


Dan Murphy (6):
  dt-bindings: ti-lmu: Remove LM3697
  mfd: ti-lmu: Remove support for LM3697
  dt-bindings: leds: Add bindings for lm3697 driver
  leds: lm3697: Introduce the lm3697 driver
  dt-bindings: leds: Add runtime ramp node for LM3697
  leds: lm3697: Add ramp rate feature

 .../devicetree/bindings/leds/leds-lm3697.txt  |  98 ++++
 .../devicetree/bindings/mfd/ti-lmu.txt        |  26 +-
 drivers/leds/Kconfig                          |   9 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-lm3697.c                    | 449 ++++++++++++++++++
 drivers/mfd/Kconfig                           |   2 +-
 drivers/mfd/ti-lmu.c                          |  17 -
 include/linux/mfd/ti-lmu-register.h           |  44 --
 include/linux/mfd/ti-lmu.h                    |   1 -
 9 files changed, 559 insertions(+), 88 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3697.txt
 create mode 100644 drivers/leds/leds-lm3697.c

-- 
2.17.0.1855.g63749b2dea

Comments

Dan Murphy Sept. 11, 2018, 9:48 p.m. UTC | #1
Pavel

On 09/11/2018 03:05 PM, Pavel Machek wrote:
> On Tue 2018-09-11 12:08:20, Dan Murphy wrote:

>> Remove support for the LM3697 LED device

>> from the ti-lmu.  The LM3697 will be supported

>> via a stand alone LED driver.

>>

>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

> 

> I'd really like to see better explanation here.

> 


I will update the commit message but....

How do I politely explain that the original implementation was wrong for certain devices?
How do I politely explain that this binding has information that does not exist?
Isn't code and documentation supposed to be pushed in stages together?
Like document the current supported source features and then submit patches to amend new
features or changes to the source.  I did that in this series
where I introduced the driver and when I added a feature I added the binding information.

Unfortunately Milo left TI before he had a chance to finish upstreaming.

Some issues with the current binding:
1) The documentation referenced in the ti-lmu.txt does not exist but the binding was accepted.
[1] ../leds/backlight/ti-lmu-backlight.txt
[2] ../leds/leds-lm3633.txt
[3] ../regulator/lm363x-regulator.txt

2) ramp-up-msec and ramp-down-msec are not explained or defined in this current binding.
Looks to be defined in the staged code though.

3) The ramp-up-msec and ramp-down-msec are not the right node parameters
the code shows ti,ramp-down-ms and ti,ramp-up-ms.  Looking at the clean up of the binding
in the staged commits the binding still does not match the code.

https://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git/commit/?h=droid4-pending-v4.19&id=d774c7e447ac911e73a1b3c775e6d89f0422218c

4) The ramp rates ranges are incorrect for the LM3697 it should be from 0 -> 117400 ms

5) The children compatibles are not defined in this binding but appear to be removed in the staged code.

6) address-cells and size-cells are missing from the binding

I am attempting to clean this up for all the dedicated LED drivers.
There is a lot of bloat with this driver to support a single LED driver that is a single function device.

This ti-lmu driver may be a great thing for the Droid 4 but from a customers
stand point that just wants to use just 1 of the dedicated LED drivers this driver is
overly complex and possibly hard to add code for differentiation.

> We have existing binding, for lm3697 and similar devices. With this

> series, different binding is introduced, without documented reason.

> 


I can update the commit message to indicate that this device is not a MFD
device and only is a SFD that needs to be supported by a dedicated driver.

> That's bad.


This ti-lmu binding is more misleading in the current state.

> 

> Now, maybe you are right and the hardware should be handled by

> drivers/leds, not drivers/mfd. But we should have solution for all the

> similar chips, and that still does not mean we have to modify the

> binding. (But maybe we want to move it to different

> directory). Bindings are supposed to describe hardware, not mirror

> structure of our drivers.


But this is not clean to have this device in the MFD bindings directory
when the support is in the LEDs directory.

I am working on creating driver for the other single function devices.
It will take some time.  I am waiting on the hardware to come in.

> 

> Unless there's something fatally wrong with the binding... but in such

> case we'd like to know what is wrong.

> 

> [And yes, I recognize current situation is ... not ideal and I'm

> willing to help. But I'm not sure this is step in right direction.]


It will take some time to get this all cleaned up.  But I am willing to
get this done.

Dan

> 

> Thanks,

> 								Pavel

> 

> 

>> ---

>>

>> v7 - New change for the series based on the comments in https://lore.kernel.org/patchwork/patch/982550/

>>

>>  .../devicetree/bindings/mfd/ti-lmu.txt        | 26 +------------------

>>  1 file changed, 1 insertion(+), 25 deletions(-)

>>

>> diff --git a/Documentation/devicetree/bindings/mfd/ti-lmu.txt b/Documentation/devicetree/bindings/mfd/ti-lmu.txt

>> index c885cf89b8ce..920f910be4e9 100644

>> --- a/Documentation/devicetree/bindings/mfd/ti-lmu.txt

>> +++ b/Documentation/devicetree/bindings/mfd/ti-lmu.txt

>> @@ -9,7 +9,6 @@ TI LMU driver supports lighting devices below.

>>    LM3632       Backlight and regulator

>>    LM3633       Backlight, LED and fault monitor

>>    LM3695       Backlight

>> -  LM3697       Backlight and fault monitor

>>  

>>  Required properties:

>>    - compatible: Should be one of:

>> @@ -18,11 +17,10 @@ Required properties:

>>                  "ti,lm3632"

>>                  "ti,lm3633"

>>                  "ti,lm3695"

>> -                "ti,lm3697"

>>    - reg: I2C slave address.

>>           0x11 for LM3632

>>           0x29 for LM3631

>> -         0x36 for LM3633, LM3697

>> +         0x36 for LM3633

>>           0x38 for LM3532

>>           0x63 for LM3695

>>  

>> @@ -38,7 +36,6 @@ Optional nodes:

>>      Required properties:

>>        - compatible: Should be one of:

>>                      "ti,lm3633-fault-monitor"

>> -                    "ti,lm3697-fault-monitor"

>>    - leds: LED properties for LM3633. Please refer to [2].

>>    - regulators: Regulator properties for LM3631 and LM3632.

>>                  Please refer to [3].

>> @@ -220,24 +217,3 @@ lm3695@63 {

>>  		};

>>  	};

>>  };

>> -

>> -lm3697@36 {

>> -	compatible = "ti,lm3697";

>> -	reg = <0x36>;

>> -

>> -	enable-gpios = <&pioC 2 GPIO_ACTIVE_HIGH>;

>> -

>> -	backlight {

>> -		compatible = "ti,lm3697-backlight";

>> -

>> -		lcd {

>> -			led-sources = <0 1 2>;

>> -			ramp-up-msec = <200>;

>> -			ramp-down-msec = <200>;

>> -		};

>> -	};

>> -

>> -	fault-monitor {

>> -		compatible = "ti,lm3697-fault-monitor";

>> -	};

>> -};

> 



-- 
------------------
Dan Murphy
Rob Herring (Arm) Sept. 25, 2018, 7:39 p.m. UTC | #2
On Mon, Sep 24, 2018 at 1:02 PM Pavel Machek <pavel@ucw.cz> wrote:
>

> Hi!

>

> > > Add the device tree bindings for the lm3697

> > > LED driver for backlighting and display.

> > >

> > > Signed-off-by: Dan Murphy <dmurphy@ti.com>

>

> > >  .../devicetree/bindings/leds/leds-lm3697.txt  | 86 +++++++++++++++++++

> > >  1 file changed, 86 insertions(+)

> > >  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3697.txt

> >

> > Reviewed-by: Rob Herring <robh@kernel.org>

>

> We already have binding for lm3697 in

> Documentation/devicetree/bindings/mfd/ti-lmu.txt . Is it good idea to

> have second one?


Of course not. Now that you mention it, I do remember seeing some
discussion on this.

Rob
Dan Murphy Sept. 25, 2018, 9:19 p.m. UTC | #3
Rob/Pavel/Jacek

On 09/25/2018 02:39 PM, Rob Herring wrote:
> On Mon, Sep 24, 2018 at 1:02 PM Pavel Machek <pavel@ucw.cz> wrote:

>>

>> Hi!

>>

>>>> Add the device tree bindings for the lm3697

>>>> LED driver for backlighting and display.

>>>>

>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>>

>>>>  .../devicetree/bindings/leds/leds-lm3697.txt  | 86 +++++++++++++++++++

>>>>  1 file changed, 86 insertions(+)

>>>>  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3697.txt

>>>

>>> Reviewed-by: Rob Herring <robh@kernel.org>

>>

>> We already have binding for lm3697 in

>> Documentation/devicetree/bindings/mfd/ti-lmu.txt . Is it good idea to

>> have second one?

> 

> Of course not. Now that you mention it, I do remember seeing some

> discussion on this.

> 

> Rob

> 


I will be posting a RFC tomorrow with all the changes that shows 3 devices
attached to the common code along with extending out the features on the LED
drivers themselves.

Its not complete or tested code but it should give a basis for discussion.  This way I
can do some other code while this is being discussed.

Dan

-- 
------------------
Dan Murphy