* [PATCH v2] Extend pca9532 device tree support @ 2017-03-30 13:33 Felix Brack 2017-04-02 14:41 ` Jacek Anaszewski 0 siblings, 1 reply; 14+ messages in thread From: Felix Brack @ 2017-03-30 13:33 UTC (permalink / raw) To: jacek.anaszewski, rpurdie, pavel, mark.rutland, riku.voipio, linux-leds, devicetree Cc: linux-kernel, fb This patch extends the device tree support for the pca9532 by adding the leds 'default-state' property. Changes in v2: - remove prescaler and pwm configuration by none generic DT properties Signed-off-by: Felix Brack <fb@ltec.ch> --- .../devicetree/bindings/leds/leds-pca9532.txt | 10 +++++++ drivers/leds/leds-pca9532.c | 31 +++++++++++++++++++++- include/linux/leds-pca9532.h | 4 +-- 3 files changed, 42 insertions(+), 3 deletions(-) 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 + }; }; For more product information please see the link below: diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c index 06e6310..66ef280 100644 --- a/drivers/leds/leds-pca9532.c +++ b/drivers/leds/leds-pca9532.c @@ -254,6 +254,21 @@ static void pca9532_input_work(struct work_struct *work) mutex_unlock(&data->update_lock); } +static enum pca9532_state pca9532_getled(struct pca9532_led *led) +{ + struct i2c_client *client = led->client; + struct pca9532_data *data = i2c_get_clientdata(client); + u8 maxleds = data->chip_info->num_leds; + char reg; + enum pca9532_state ret; + + mutex_lock(&data->update_lock); + reg = i2c_smbus_read_byte_data(client, LED_REG(maxleds, led->id)); + ret = reg >> LED_NUM(led->id)/2; + mutex_unlock(&data->update_lock); + return ret; +} + #ifdef CONFIG_LEDS_PCA9532_GPIO static int pca9532_gpio_request_pin(struct gpio_chip *gc, unsigned offset) { @@ -366,7 +381,10 @@ static int pca9532_configure(struct i2c_client *client, gpios++; break; case PCA9532_TYPE_LED: - led->state = pled->state; + if (pled->state == PCA9532_KEEP) + led->state = pca9532_getled(led); + else + led->state = pled->state; led->name = pled->name; led->ldev.name = led->name; led->ldev.default_trigger = pled->default_trigger; @@ -456,6 +474,7 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np) const struct of_device_id *match; int devid, maxleds; int i = 0; + const char *state; match = of_match_device(of_pca9532_leds_match, dev); if (!match) @@ -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; diff --git a/include/linux/leds-pca9532.h b/include/linux/leds-pca9532.h index d215b45..a327b1aa 100644 --- a/include/linux/leds-pca9532.h +++ b/include/linux/leds-pca9532.h @@ -22,7 +22,8 @@ enum pca9532_state { PCA9532_OFF = 0x0, PCA9532_ON = 0x1, PCA9532_PWM0 = 0x2, - PCA9532_PWM1 = 0x3 + PCA9532_PWM1 = 0x3, + PCA9532_KEEP = 0xff }; struct pca9532_led { @@ -44,4 +45,3 @@ struct pca9532_platform_data { }; #endif /* __LINUX_PCA9532_H */ - -- 2.7.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] Extend pca9532 device tree support 2017-03-30 13:33 [PATCH v2] Extend pca9532 device tree support Felix Brack @ 2017-04-02 14:41 ` Jacek Anaszewski [not found] ` <b54c77d5-b4f7-9517-8c5b-9200c766204a-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 0 siblings, 1 reply; 14+ messages in thread From: Jacek Anaszewski @ 2017-04-02 14:41 UTC (permalink / raw) To: Felix Brack, rpurdie, pavel, mark.rutland, riku.voipio, linux-leds, devicetree Cc: linux-kernel Hi Felix, Thanks for the patch. I made a few cosmetic amendments to it, please refer below. On 03/30/2017 03:33 PM, Felix Brack wrote: > This patch extends the device tree support for the pca9532 by adding > the leds 'default-state' property. > Added leds: pca9532: prefix to the patch title. Removed below change history from the commit message. > Changes in v2: > - remove prescaler and pwm configuration by none generic > DT properties > > Signed-off-by: Felix Brack <fb@ltec.ch> > --- > .../devicetree/bindings/leds/leds-pca9532.txt | 10 +++++++ > drivers/leds/leds-pca9532.c | 31 +++++++++++++++++++++- > include/linux/leds-pca9532.h | 4 +-- > 3 files changed, 42 insertions(+), 3 deletions(-) > > 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. > }; > > For more product information please see the link below: > diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c > index 06e6310..66ef280 100644 > --- a/drivers/leds/leds-pca9532.c > +++ b/drivers/leds/leds-pca9532.c > @@ -254,6 +254,21 @@ static void pca9532_input_work(struct work_struct *work) > mutex_unlock(&data->update_lock); > } > > +static enum pca9532_state pca9532_getled(struct pca9532_led *led) > +{ > + struct i2c_client *client = led->client; > + struct pca9532_data *data = i2c_get_clientdata(client); > + u8 maxleds = data->chip_info->num_leds; > + char reg; > + enum pca9532_state ret; > + > + mutex_lock(&data->update_lock); > + reg = i2c_smbus_read_byte_data(client, LED_REG(maxleds, led->id)); > + ret = reg >> LED_NUM(led->id)/2; > + mutex_unlock(&data->update_lock); > + return ret; > +} > + > #ifdef CONFIG_LEDS_PCA9532_GPIO > static int pca9532_gpio_request_pin(struct gpio_chip *gc, unsigned offset) > { > @@ -366,7 +381,10 @@ static int pca9532_configure(struct i2c_client *client, > gpios++; > break; > case PCA9532_TYPE_LED: > - led->state = pled->state; > + if (pled->state == PCA9532_KEEP) > + led->state = pca9532_getled(led); > + else > + led->state = pled->state; > led->name = pled->name; > led->ldev.name = led->name; > led->ldev.default_trigger = pled->default_trigger; > @@ -456,6 +474,7 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np) > const struct of_device_id *match; > int devid, maxleds; > int i = 0; > + const char *state; > > match = of_match_device(of_pca9532_leds_match, dev); > if (!match) > @@ -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; > diff --git a/include/linux/leds-pca9532.h b/include/linux/leds-pca9532.h > index d215b45..a327b1aa 100644 > --- a/include/linux/leds-pca9532.h > +++ b/include/linux/leds-pca9532.h > @@ -22,7 +22,8 @@ enum pca9532_state { > PCA9532_OFF = 0x0, > PCA9532_ON = 0x1, > PCA9532_PWM0 = 0x2, > - PCA9532_PWM1 = 0x3 > + PCA9532_PWM1 = 0x3, > + PCA9532_KEEP = 0xff Added a comma at the end of the above line to avoid the need for changing two lines on addition of a new enum value, like in this case. > }; > > struct pca9532_led { > @@ -44,4 +45,3 @@ struct pca9532_platform_data { > }; > > #endif /* __LINUX_PCA9532_H */ > - > The patch, with the aforementioned amendments, has been applied to the for-next branch of linux-leds.git. -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <b54c77d5-b4f7-9517-8c5b-9200c766204a-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH v2] Extend pca9532 device tree support 2017-04-02 14:41 ` Jacek Anaszewski @ 2017-04-06 15:50 ` Pavel Machek 0 siblings, 0 replies; 14+ messages in thread From: Pavel Machek @ 2017-04-06 15:50 UTC (permalink / raw) To: Jacek Anaszewski Cc: Felix Brack, rpurdie-Fm38FmjxZ/leoWH0uzbU5w, mark.rutland-5wv7dgnIgG8, riku.voipio-X3B1VOXEql0, linux-leds-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 2495 bytes --] 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? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] Extend pca9532 device tree support @ 2017-04-06 15:50 ` Pavel Machek 0 siblings, 0 replies; 14+ messages in thread From: Pavel Machek @ 2017-04-06 15:50 UTC (permalink / raw) To: Jacek Anaszewski Cc: Felix Brack, rpurdie, mark.rutland, riku.voipio, linux-leds, devicetree, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2495 bytes --] 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? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] Extend pca9532 device tree support 2017-04-06 15:50 ` Pavel Machek @ 2017-04-06 19:00 ` Jacek Anaszewski -1 siblings, 0 replies; 14+ messages in thread From: Jacek Anaszewski @ 2017-04-06 19:00 UTC (permalink / raw) To: Pavel Machek, Felix Brack Cc: rpurdie-Fm38FmjxZ/leoWH0uzbU5w, mark.rutland-5wv7dgnIgG8, riku.voipio-X3B1VOXEql0, linux-leds-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA 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. I guess that pwm0/1 got propagated to v2 by an omission. Regarding default-on: Felix, do you have any use case that require default-on set to "keep"? -- 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] Extend pca9532 device tree support @ 2017-04-06 19:00 ` Jacek Anaszewski 0 siblings, 0 replies; 14+ messages in thread From: Jacek Anaszewski @ 2017-04-06 19:00 UTC (permalink / raw) To: Pavel Machek, Felix Brack Cc: rpurdie, mark.rutland, riku.voipio, linux-leds, devicetree, linux-kernel 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. I guess that pwm0/1 got propagated to v2 by an omission. Regarding default-on: Felix, do you have any use case that require default-on set to "keep"? -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <a8509d26-6e35-e2f7-9784-90f3c54accbf-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH v2] Extend pca9532 device tree support 2017-04-06 19:00 ` Jacek Anaszewski @ 2017-04-07 8:22 ` Felix Brack -1 siblings, 0 replies; 14+ messages in thread From: Felix Brack @ 2017-04-07 8:22 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek Cc: rpurdie-Fm38FmjxZ/leoWH0uzbU5w, mark.rutland-5wv7dgnIgG8, riku.voipio-X3B1VOXEql0, linux-leds-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA 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. kind regards, Felix -- 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] Extend pca9532 device tree support @ 2017-04-07 8:22 ` Felix Brack 0 siblings, 0 replies; 14+ messages in thread From: Felix Brack @ 2017-04-07 8:22 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek Cc: rpurdie, mark.rutland, riku.voipio, linux-leds, devicetree, linux-kernel 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. kind regards, Felix ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <ec012568-f540-4e87-d8c4-4065fbcd1c2a-GovowT2ENgg@public.gmane.org>]
* default-state LED property (was Re: [PATCH v2] Extend pca9532 device tree support) 2017-04-07 8:22 ` Felix Brack @ 2017-04-07 11:57 ` Pavel Machek -1 siblings, 0 replies; 14+ messages in thread From: Pavel Machek @ 2017-04-07 11:57 UTC (permalink / raw) To: Felix Brack Cc: Jacek Anaszewski, rpurdie-Fm38FmjxZ/leoWH0uzbU5w, mark.rutland-5wv7dgnIgG8, riku.voipio-X3B1VOXEql0, linux-leds-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linus.walleij-QSEj5FYQhm4dnm+yROfE0A, robh-DgEjT+Ai2ygdnm+yROfE0A [-- Attachment #1: Type: text/plain, Size: 2311 bytes --] Hi! > >> 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. Well, they should be removed. No need to keep surprises in code. > > 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: Hmm, that was introduce by 1d1a77ddc8acfa3d506f1958e09a12085e71fc69 . I don't quite like the patch -- it duplicates default-trigger functionality without good reason. IMO we should 1) keep the led state by default 2) deprecate the default-state property 3) use default-triggers to emulate the functionality if someone really needs it. > 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. Yes, that's reasonable. Perhaps 'keep' should be the default. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* default-state LED property (was Re: [PATCH v2] Extend pca9532 device tree support) @ 2017-04-07 11:57 ` Pavel Machek 0 siblings, 0 replies; 14+ messages in thread From: Pavel Machek @ 2017-04-07 11:57 UTC (permalink / raw) To: Felix Brack Cc: Jacek Anaszewski, rpurdie, mark.rutland, riku.voipio, linux-leds, devicetree, linux-kernel, linus.walleij, robh [-- Attachment #1: Type: text/plain, Size: 2311 bytes --] Hi! > >> 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. Well, they should be removed. No need to keep surprises in code. > > 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: Hmm, that was introduce by 1d1a77ddc8acfa3d506f1958e09a12085e71fc69 . I don't quite like the patch -- it duplicates default-trigger functionality without good reason. IMO we should 1) keep the led state by default 2) deprecate the default-state property 3) use default-triggers to emulate the functionality if someone really needs it. > 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. Yes, that's reasonable. Perhaps 'keep' should be the default. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] Extend pca9532 device tree support 2017-04-07 8:22 ` Felix Brack @ 2017-04-09 12:37 ` Jacek Anaszewski -1 siblings, 0 replies; 14+ messages in thread From: Jacek Anaszewski @ 2017-04-09 12:37 UTC (permalink / raw) To: Felix Brack, Pavel Machek Cc: rpurdie-Fm38FmjxZ/leoWH0uzbU5w, mark.rutland-5wv7dgnIgG8, riku.voipio-X3B1VOXEql0, linux-leds-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA 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? -- 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] Extend pca9532 device tree support @ 2017-04-09 12:37 ` Jacek Anaszewski 0 siblings, 0 replies; 14+ messages in thread From: Jacek Anaszewski @ 2017-04-09 12:37 UTC (permalink / raw) To: Felix Brack, Pavel Machek Cc: rpurdie, mark.rutland, riku.voipio, linux-leds, devicetree, linux-kernel 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? -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <2f3815ab-71e1-b63d-aba8-167e11f719de-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH v2] Extend pca9532 device tree support 2017-04-09 12:37 ` Jacek Anaszewski @ 2017-04-09 13:11 ` Felix Brack -1 siblings, 0 replies; 14+ messages in thread From: Felix Brack @ 2017-04-09 13:11 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek Cc: rpurdie-Fm38FmjxZ/leoWH0uzbU5w, mark.rutland-5wv7dgnIgG8, riku.voipio-X3B1VOXEql0, linux-leds-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA 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 -- 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] Extend pca9532 device tree support @ 2017-04-09 13:11 ` Felix Brack 0 siblings, 0 replies; 14+ messages in thread From: Felix Brack @ 2017-04-09 13:11 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek Cc: rpurdie, mark.rutland, riku.voipio, linux-leds, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-04-09 13:11 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-30 13:33 [PATCH v2] Extend pca9532 device tree support Felix Brack
2017-04-02 14:41 ` Jacek Anaszewski
[not found] ` <b54c77d5-b4f7-9517-8c5b-9200c766204a-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-04-06 15:50 ` Pavel Machek
2017-04-06 15:50 ` Pavel Machek
2017-04-06 19:00 ` Jacek Anaszewski
2017-04-06 19:00 ` Jacek Anaszewski
[not found] ` <a8509d26-6e35-e2f7-9784-90f3c54accbf-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-04-07 8:22 ` Felix Brack
2017-04-07 8:22 ` Felix Brack
[not found] ` <ec012568-f540-4e87-d8c4-4065fbcd1c2a-GovowT2ENgg@public.gmane.org>
2017-04-07 11:57 ` default-state LED property (was Re: [PATCH v2] Extend pca9532 device tree support) Pavel Machek
2017-04-07 11:57 ` Pavel Machek
2017-04-09 12:37 ` [PATCH v2] Extend pca9532 device tree support Jacek Anaszewski
2017-04-09 12:37 ` Jacek Anaszewski
[not found] ` <2f3815ab-71e1-b63d-aba8-167e11f719de-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-04-09 13:11 ` Felix Brack
2017-04-09 13:11 ` Felix Brack
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.