2022-05-24 04:06:16

by Bjorn Andersson

[permalink] [raw]
Subject: [PATCH] leds: qcom-lpg: Require pattern to follow documentation

The leds-trigger-pattern documentation describes how the brightness of
the LED should transition linearly from one brightness value to the
next, over the given delta_t.

But the pattern engine in the Qualcomm LPG hardware only supports
holding the brightness for each entry for the period.
This subset of patterns can be represented in the leds-trigger-pattern
by injecting zero-time transitions after each entry in the pattern,
resulting in a pattern that pattern that can be rendered by the LPG.

Rework LPG pattern interface to require these zero-time transitions, to
make it comply with this subset of patterns and reject the patterns it
can't render.

Fixes: 24e2d05d1b68 ("leds: Add driver for Qualcomm LPG")
Signed-off-by: Bjorn Andersson <[email protected]>
---
Documentation/leds/leds-qcom-lpg.rst | 8 ++++--
drivers/leds/rgb/leds-qcom-lpg.c | 43 ++++++++++++++++++++++++----
2 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/Documentation/leds/leds-qcom-lpg.rst b/Documentation/leds/leds-qcom-lpg.rst
index f12416f02dd8..de7ceead9337 100644
--- a/Documentation/leds/leds-qcom-lpg.rst
+++ b/Documentation/leds/leds-qcom-lpg.rst
@@ -35,11 +35,13 @@ Specify a hardware pattern for a Qualcomm LPG LED.
The pattern is a series of brightness and hold-time pairs, with the hold-time
expressed in milliseconds. The hold time is a property of the pattern and must
therefor be identical for each element in the pattern (except for the pauses
-described below).
+described below). As the LPG hardware is not able to perform the linear
+transitions expected by the leds-trigger-pattern format, each entry in the
+pattern must be followed a zero-length entry of the same brightness.

Simple pattern::

- "255 500 0 500"
+ "255 500 255 0 0 500 0 0"

^
|
@@ -54,7 +56,7 @@ in the pattern, the so called "low pause" and "high pause".

Low-pause pattern::

- "255 1000 0 500 255 500 0 500"
+ "255 1000 255 0 0 500 0 0 255 500 255 0 0 500 0 0"

^
|
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index cfa3362b2457..02f51cc61837 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -704,11 +704,12 @@ static int lpg_blink_mc_set(struct led_classdev *cdev,
return ret;
}

-static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *pattern,
+static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
u32 len, int repeat)
{
struct lpg_channel *chan;
struct lpg *lpg = led->lpg;
+ struct led_pattern *pattern;
unsigned int brightness_a;
unsigned int brightness_b;
unsigned int actual_len;
@@ -719,18 +720,48 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *pattern,
unsigned int hi_idx;
unsigned int i;
bool ping_pong = true;
- int ret;
+ int ret = -EINVAL;

/* Hardware only support oneshot or indefinite loops */
if (repeat != -1 && repeat != 1)
return -EINVAL;

+ /*
+ * The standardized leds-trigger-pattern format defines that the
+ * brightness of the LED follows a linear transition from one entry
+ * in the pattern to the next, over the given delta_t time. It
+ * describes that the way to perform instant transitions a zero-length
+ * entry should be added following a pattern entry.
+ *
+ * The LPG hardware is only able to perform the latter (no linear
+ * transitions), so require each entry in the pattern to be followed by
+ * a zero-length transition.
+ */
+ if (len % 2)
+ return -EINVAL;
+
+ pattern = kcalloc(len / 2, sizeof(*pattern), GFP_KERNEL);
+ if (!pattern)
+ return -ENOMEM;
+
+ for (i = 0; i < len; i += 2) {
+ if (led_pattern[i].brightness != led_pattern[i + 1].brightness)
+ goto out_free_pattern;
+ if (led_pattern[i + 1].delta_t != 0)
+ goto out_free_pattern;
+
+ pattern[i / 2].brightness = led_pattern[i].brightness;
+ pattern[i / 2].delta_t = led_pattern[i].delta_t;
+ }
+
+ len /= 2;
+
/*
* Specifying a pattern of length 1 causes the hardware to iterate
* through the entire LUT, so prohibit this.
*/
if (len < 2)
- return -EINVAL;
+ goto out_free_pattern;

/*
* The LPG plays patterns with at a fixed pace, a "low pause" can be
@@ -781,13 +812,13 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *pattern,
* specify hi pause. Reject other variations.
*/
if (i != actual_len - 1)
- return -EINVAL;
+ goto out_free_pattern;
}
}

/* LPG_RAMP_DURATION_REG is a 9bit */
if (delta_t >= BIT(9))
- return -EINVAL;
+ goto out_free_pattern;

/* Find "low pause" and "high pause" in the pattern */
lo_pause = pattern[0].delta_t;
@@ -814,6 +845,8 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *pattern,

out_unlock:
mutex_unlock(&lpg->lock);
+out_free_pattern:
+ kfree(pattern);

return ret;
}
--
2.35.1



2022-05-27 09:14:19

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH] leds: qcom-lpg: Require pattern to follow documentation

Quoting Bjorn Andersson (2022-05-23 16:37:19)
> The leds-trigger-pattern documentation describes how the brightness of
> the LED should transition linearly from one brightness value to the
> next, over the given delta_t.
>
> But the pattern engine in the Qualcomm LPG hardware only supports
> holding the brightness for each entry for the period.
> This subset of patterns can be represented in the leds-trigger-pattern
> by injecting zero-time transitions after each entry in the pattern,
> resulting in a pattern that pattern that can be rendered by the LPG.

s/that pattern//

>
> Rework LPG pattern interface to require these zero-time transitions, to
> make it comply with this subset of patterns and reject the patterns it
> can't render.
>
> Fixes: 24e2d05d1b68 ("leds: Add driver for Qualcomm LPG")
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---

Reviewed-by: Stephen Boyd <[email protected]>