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 <[email protected]>
---
.../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
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 <[email protected]>
> ---
> .../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
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
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
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
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
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
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