Re: [PATCH v2 1/1] leds: pca9532: Add device tree binding

[Jacek Anaszewski <j.anaszewski@samsung.com>]

On 04/11/2016 08:23 AM, Phil Reid wrote:
> On 7/04/2016 10:02 PM, Jacek Anaszewski wrote:
>> Hi Phil,
>>
>> Thanks for the update. Few more remarks below.
>>
>> On 04/07/2016 09:22 AM, Phil Reid wrote:
>>> This patch adds basic device tree support for the pca9532 LEDs.
>>>
>>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>>> ---
>>>   .../devicetree/bindings/leds/leds-pca9532.txt      | 42 +++++++++++++
>>>   drivers/leds/leds-pca9532.c                        | 69
>>> ++++++++++++++++++++--
>>>   include/dt-bindings/leds/leds-pca9532.h            | 23 ++++++++
>>>   include/linux/leds-pca9532.h                       | 18 ++----
>>>   4 files changed, 135 insertions(+), 17 deletions(-)
>>>   create mode 100644
>>> Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>   create mode 100644 include/dt-bindings/leds/leds-pca9532.h
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>> b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>> new file mode 100644
>>> index 0000000..284b96c
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>> @@ -0,0 +1,42 @@
>>> +*NXP - pca9532 PWM LED Driver
>>> +
>>> +The PCA9532 family is SMBus I/O expander optimized for dimming LEDs.
>>> +The PWM support 256 steps.
>>> +
>>> +Required properties:
>>> +    - compatible:
>>> +        "nxp,pca9530"
>>> +        "nxp,pca9531"
>>> +        "nxp,pca9532"
>>> +        "nxp,pca9533"
>>> +    - reg -  I2C slave address
>>> +
>>> +Each led is represented as a sub-node of the nxp,pca9530.
>>> +
>>> +Optional sub-node properties:
>>> +    - label: Name for this LED. If omitted, the label is taken from
>>> the node name.
>>
>> Please also add a reference to common led bindings, as this property is
>> documented there:
>> "(see Documentation/devicetree/bindings/leds/common.txt):"
> done.
>
>>
>>> +    - type: Output configuration, see
>>> dt-bindings/leds/leds-pca9532.h (default NONE)
>>> +    - state: Initial LED state (default OFF)
>>
>> We already have "default-on" trigger for this purpose.
> Not sure it does exactly the same thing.
> This setting also associates a led with a particular PWM controller.
> The chip has 2 shared PWM that each led can be controlled by.

default-on trigger sets the LED to max_brightness on init.
As I see pca9532_set_brightness() sets PCA9532_ON state if
passed LED_FULL and uses PCA9532_PWM0 for other values
greater than zero.

So, maybe it would make more sense if we used default-on trigger to
initialize LED to LED_FULL on init, but provide another DT property
'default-pwm' to determine which PWM block should be used by default?
Currently I see that driver uses only PWM0:

led->state = PCA9532_PWM0; /* Thecus: hardcode one pwm */

Alternatively the driver could expose a dedicated sysfs attribute
for changing the default pwm. This is of course out of this patch
scope.

-- 
Best regards,
Jacek Anaszewski