| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ccb718b2-d0e5-20d8-f30a-95f8f31a10ef@gmail.com>
Date: Tue, 10 Mar 2020 22:15:14 +0100
From: Jacek Anaszewski <jacek.anaszewski@...il.com>
To: Denis Osterland-Heim <denis.osterland@...hl.com>,
"dmurphy@...com" <dmurphy@...com>, "pavel@....cz" <pavel@....cz>,
"mark.rutland@....com" <mark.rutland@....com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-leds@...r.kernel.org" <linux-leds@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH v2] leds: pwm: add support for default-state device
property
Hi Denis,
Thank you for the update. Please find my remarks below.
On 3/10/20 4:19 PM, Denis Osterland-Heim wrote:
> Hi,
>
> should be
> In-Reply-To: <20200309082218.13263-1-Denis.Osterland@...hl.com>
> instead of
> Reply-To: <20200309082218.13263-1-Denis.Osterland@...hl.com>
>
> Sorry
>
> Am Dienstag, den 10.03.2020, 13:47 +0100 schrieb Denis Osterland-Heim:
>> This patch adds support for "default-state" devicetree property, which
>> allows to defer pwm init to first use of led.
>>
>> This allows to configure the PWM early in bootloader to let the LED
>> blink until an application in Linux userspace set something different.
>>
>> Signed-off-by: Denis Osterland-Heim <Denis.Osterland@...hl.com>
>> ---
>> v1->v2:
>> - use default-state = "keep", as suggested by Jacek Anaszewski
>> - calc initial brightness with PWM state from device
>>
>> .../devicetree/bindings/leds/leds-pwm.txt | 2 ++
>> drivers/leds/leds-pwm.c | 33 +++++++++++++++++--
>> include/linux/leds_pwm.h | 1 +
>> 3 files changed, 33 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-pwm.txt b/Documentation/devicetree/bindings/leds/leds-pwm.txt
>> index 6c6583c35f2f..d0f489680594 100644
>> --- a/Documentation/devicetree/bindings/leds/leds-pwm.txt
>> +++ b/Documentation/devicetree/bindings/leds/leds-pwm.txt
>> @@ -19,6 +19,8 @@ LED sub-node properties:
>> see Documentation/devicetree/bindings/leds/common.txt
>> - linux,default-trigger : (optional)
>> see Documentation/devicetree/bindings/leds/common.txt
>> +- default-state : (optional)
>> + see Documentation/devicetree/bindings/leds/common.yaml
>>
>> Example:
>>
>> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
>> index 8b6965a563e9..92726c2e43ba 100644
>> --- a/drivers/leds/leds-pwm.c
>> +++ b/drivers/leds/leds-pwm.c
>> @@ -75,7 +75,8 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>> led_data->active_low = led->active_low;
>> led_data->cdev.name = led->name;
>> led_data->cdev.default_trigger = led->default_trigger;
>> - led_data->cdev.brightness = LED_OFF;
>> + ret = led->default_state == LEDS_GPIO_DEFSTATE_ON;
ret is for return value and it should not be used for anything
else just because it is at hand. Also LEDS_GPIO* definitions have
nothing to do with pwm leds. This is legacy because default-state
property was primarily specific to leds-gpio bindings and only
later was made common.
Please introduce corresponding LEDS_PWM definitions, but in leds-pwm.c.
>> + led_data->cdev.brightness = ret ? led->max_brightness : LED_OFF;
Instead of above two changes I'd add below:
if (led->default_state == LEDS_PWM_DEFSTATE_ON) {
led_data->cdev.brightness = led->max_brightness;
} else if (led->default_state == LEDS_PWM_DEFSTATE_KEEP)) {
// here put what you're adding below, but please use
// pwm_get_state() instead of accessing ops directly
}
LED_OFF case is covered by kzalloc() in led_pwm_probe().
>> led_data->cdev.max_brightness = led->max_brightness;
>> led_data->cdev.flags = LED_CORE_SUSPENDRESUME;
>>
>> @@ -97,7 +98,8 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>> * FIXME: pwm_apply_args() should be removed when switching to the
>> * atomic PWM API.
>> */
>> - pwm_apply_args(led_data->pwm);
>> + if (led->default_state != LEDS_GPIO_DEFSTATE_KEEP)
>> + pwm_apply_args(led_data->pwm);
>>
>> pwm_get_args(led_data->pwm, &pargs);
>>
>> @@ -105,10 +107,23 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>> if (!led_data->period && (led->pwm_period_ns > 0))
>> led_data->period = led->pwm_period_ns;
>>
>> + if (led->default_state == LEDS_GPIO_DEFSTATE_KEEP) {
>> + uint64_t brightness;
>> + struct pwm_device *pwm = led_data->pwm;
>> + struct pwm_state state;
>> +
>> + pwm->chip->ops->get_state(pwm->chip, pwm, &state);
>> + brightness = led->max_brightness * state.duty_cycle;
>> + do_div(brightness, state.period);
>> + led_data->cdev.brightness = (enum led_brightness)brightness;
>> + }
>> +
>> ret = devm_led_classdev_register(dev, &led_data->cdev);
>> if (ret == 0) {
>> priv->num_leds++;
>> - led_pwm_set(&led_data->cdev, led_data->cdev.brightness);
>> + if (led->default_state != LEDS_GPIO_DEFSTATE_KEEP)
>> + led_pwm_set(&led_data->cdev,
>> + led_data->cdev.brightness);
>> } else {
>> dev_err(dev, "failed to register PWM led for %s: %d\n",
>> led->name, ret);
>> @@ -126,6 +141,8 @@ static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv)
>> memset(&led, 0, sizeof(led));
>>
>> device_for_each_child_node(dev, fwnode) {
>> + const char *state = NULL;
>> +
>> ret = fwnode_property_read_string(fwnode, "label", &led.name);
>> if (ret && is_of_node(fwnode))
>> led.name = to_of_node(fwnode)->name;
>> @@ -143,6 +160,16 @@ static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv)
>> fwnode_property_read_u32(fwnode, "max-brightness",
>> &led.max_brightness);
>>
>> + if (!fwnode_property_read_string(fwnode, "default-state",
>> + &state)) {
>> + if (!strcmp(state, "keep"))
>> + led.default_state = LEDS_GPIO_DEFSTATE_KEEP;
>> + else if (!strcmp(state, "on"))
>> + led.default_state = LEDS_GPIO_DEFSTATE_ON;
>> + else
>> + led.default_state = LEDS_GPIO_DEFSTATE_OFF;
>> + }
>> +
>> ret = led_pwm_add(dev, priv, &led, fwnode);
>> if (ret) {
>> fwnode_handle_put(fwnode);
>> diff --git a/include/linux/leds_pwm.h b/include/linux/leds_pwm.h
>> index 93d101d28943..c9ef9012913d 100644
>> --- a/include/linux/leds_pwm.h
>> +++ b/include/linux/leds_pwm.h
>> @@ -10,6 +10,7 @@ struct led_pwm {
>> const char *default_trigger;
>> unsigned pwm_id __deprecated;
>> u8 active_low;
>> + u8 default_state;
>> unsigned max_brightness;
>> unsigned pwm_period_ns;
>> };
>
>
> Diehl Connectivity Solutions GmbH
> Geschäftsführung: Horst Leonberger
> Sitz der Gesellschaft: Nürnberg - Registergericht: Amtsgericht
> Nürnberg: HRB 32315
> ___________________________________________________________________________________________________
>
> Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
> Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht.
> Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
> - Informationen zum Datenschutz, insbesondere zu Ihren Rechten, erhalten Sie unter https://www.diehl.com/group/de/transparenz-und-informationspflichten/
>
> The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by
> mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited.
> - For general information on data protection and your respective rights please visit https://www.diehl.com/group/en/transparency-and-information-obligations/
>
--
Best regards,
Jacek Anaszewski
Powered by blists - more mailing lists