Re: [PATCH v2] Extend pca9532 device tree support

[Felix Brack <fb@ltec.ch>]

Hello Jacek,

On 09.04.2017 14:37, Jacek Anaszewski wrote:
> Hello Felix,
> 
> On 04/07/2017 10:22 AM, Felix Brack wrote:
>> Hello Jacek,
>>
>> On 06.04.2017 21:00, Jacek Anaszewski wrote:
>>> Hi Pavel,
>>>
>>> On 04/06/2017 05:50 PM, Pavel Machek wrote:
>>>> Hi!
>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-pca9532.txt b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>>>> index 198f3ba..8374075 100644
>>>>>> --- a/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>>>> +++ b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>>>> @@ -17,6 +17,8 @@ Optional sub-node properties:
>>>>>>  	- label: see Documentation/devicetree/bindings/leds/common.txt
>>>>>>  	- type: Output configuration, see dt-bindings/leds/leds-pca9532.h (default NONE)
>>>>>>  	- linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt
>>>>>> +	- default-state: see Documentation/devicetree/bindings/leds/common.txt
>>>>>> +	  This property is only valid for sub-nodes of type <PCA9532_TYPE_LED>.
>>>>>>  
>>>>>>  Example:
>>>>>>    #include <dt-bindings/leds/leds-pca9532.h>
>>>>>> @@ -33,6 +35,14 @@ Example:
>>>>>>        label = "pca:green:power";
>>>>>>        type = <PCA9532_TYPE_LED>;
>>>>>>      };
>>>>>> +    kernel-booting {
>>>>>> +    	type = <PCA9532_TYPE_LED>;
>>>>>> +    	default-state = "on";
>>>>>> +    };
>>>>>> +    sys-stat {
>>>>>> +    	type = <PCA9532_TYPE_LED>;
>>>>>> +    	default-state = "keep"; // don't touch, was set by U-Boot
>>>>>> +    };
>>>>>
>>>>> Adjusted above indentation to match the preceding lines.
>>>>
>>>>>> @@ -475,6 +494,16 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np)
>>>>>>  		of_property_read_u32(child, "type", &pdata->leds[i].type);
>>>>>>  		of_property_read_string(child, "linux,default-trigger",
>>>>>>  					&pdata->leds[i].default_trigger);
>>>>>> +		if (!of_property_read_string(child, "default-state", &state)) {
>>>>>> +			if (!strcmp(state, "on"))
>>>>>> +				pdata->leds[i].state = PCA9532_ON;
>>>>>> +			else if (!strcmp(state, "keep"))
>>>>>> +				pdata->leds[i].state = PCA9532_KEEP;
>>>>>> +			else if (!strcmp(state, "pwm0"))
>>>>>> +				pdata->leds[i].state = PCA9532_PWM0;
>>>>>> +			else if (!strcmp(state, "pwm1"))
>>>>>> +				pdata->leds[i].state = PCA9532_PWM1;
>>>>>> +		}
>>>>>>  		if (++i >= maxleds) {
>>>>>>  			of_node_put(child);
>>>>>>  			break;
>>>>
>>>> This seems to look for "pwm0" and "pwm1" strings, which do not seem to
>>>> be documented.
>>>>
>>>> Plus... is it useful to have default-state? We already have default
>>>> trigger. If we keep the value by default (on PC, we do something like
>>>> that) this patch should not be neccessary?
>>>
>>> Thanks for the heads-up. Dropping the patch for now.
>>
>> No, please do not drop the patch.
>>
>>> I guess that pwm0/1 got propagated to v2 by an omission.
>>>
>>
>> Yes, I agree. However the two strings do not break anything and behave
>> analog to the 'on' or 'keep' string. Though this code could be removed
>> if absolutely necessary. An alternative would be to add a description
>> for the strings. Just to be clear: these strings have nothing to with
>> the exposition of device specific registers to the DT.
>>
>>> Regarding default-on: Felix, do you have any use case that require
>>> default-on set to "keep"?
>>>
>>
>> This patch is not about 'default-on' which is a value that could be
>> assigned to the property 'linux,default-trigger' (according to DT
>> bindings documentation file 'common.txt').
>> My patch does not introduce anything new with the'keep' state, it rather
>> completes the existing bindings according to the description in
>> Documentation/devicetree/bindings/leds/common.txt which states:
>>
>> ....
>> - default-state : The initial state of the LED. Valid values are "on",
>> "off", and "keep". If the LED is already on or off and the default-state
>> property is set the to same value, then no glitch should be produced
>> where the LED momentarily turns off (or on). The "keep" setting will
>> keep the LED at whatever its current state is, without producing a
>> glitch.  The default is off if this property is not present.
>> ....
>>
>> One of my use cases is to turn a LED on by U-Boot. This LED must remain
>> on until eventually, under certain conditions, some userland code
>> changes it's state.
>> Setting 'default-state' to 'keep' is how you can sort of tell the
>> kernel, or better the driver, 'not to initialize the LED' which would
>> turn it off.
> 
> Thanks for the explanation. Could you please sent v3 with removed pwm*
> cases then?
> 

Yes, I will try to do so next week.

--
regards Felix