From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jacek Anaszewski Subject: Re: [PATCH v2] Extend pca9532 device tree support Date: Sun, 9 Apr 2017 14:37:42 +0200 Message-ID: <2f3815ab-71e1-b63d-aba8-167e11f719de@gmail.com> References: <1490880833-22199-1-git-send-email-fb@ltec.ch> <20170406155005.GA10363@amd> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Felix Brack , Pavel Machek Cc: rpurdie-Fm38FmjxZ/leoWH0uzbU5w@public.gmane.org, mark.rutland-5wv7dgnIgG8@public.gmane.org, riku.voipio-X3B1VOXEql0@public.gmane.org, linux-leds-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-leds@vger.kernel.org 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 . >>>>> >>>>> Example: >>>>> #include >>>>> @@ -33,6 +35,14 @@ Example: >>>>> label = "pca:green:power"; >>>>> type = ; >>>>> }; >>>>> + kernel-booting { >>>>> + type = ; >>>>> + default-state = "on"; >>>>> + }; >>>>> + sys-stat { >>>>> + type = ; >>>>> + 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? -- Best regards, Jacek Anaszewski -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752267AbdDIMia (ORCPT ); Sun, 9 Apr 2017 08:38:30 -0400 Received: from mail-wm0-f65.google.com ([74.125.82.65]:35702 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752066AbdDIMi2 (ORCPT ); Sun, 9 Apr 2017 08:38:28 -0400 Subject: Re: [PATCH v2] Extend pca9532 device tree support To: Felix Brack , Pavel Machek References: <1490880833-22199-1-git-send-email-fb@ltec.ch> <20170406155005.GA10363@amd> Cc: rpurdie@rpsys.net, mark.rutland@arm.com, riku.voipio@iki.fi, linux-leds@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org From: Jacek Anaszewski Message-ID: <2f3815ab-71e1-b63d-aba8-167e11f719de@gmail.com> Date: Sun, 9 Apr 2017 14:37:42 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 . >>>>> >>>>> Example: >>>>> #include >>>>> @@ -33,6 +35,14 @@ Example: >>>>> label = "pca:green:power"; >>>>> type = ; >>>>> }; >>>>> + kernel-booting { >>>>> + type = ; >>>>> + default-state = "on"; >>>>> + }; >>>>> + sys-stat { >>>>> + type = ; >>>>> + 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? -- Best regards, Jacek Anaszewski