In certain PMICs, LUT pattern and LPG configuration is stored in SDAM
modules instead of LUT peripheral. This feature is called PPG.
This change series adds support for PPG. Thanks!
Changes since v4:
- Patch 3/7
- Get rid of r/w helpers
- Use regmap_read_poll_timeout() in qcom_pbs_wait_for_ack()
- Update error path in qcom_pbs_trigger_event()
- Fix reverse christmas tree
- Patch 4/7
- Get rid of r/w helpers
- Update variables to use "sdam" instead of "nvmem"
- Fix comments
- Fix reverse christmas tree
- Update lpg_pattern_set() logic
- Patch 5/7
- Removed sdam_lut_base from lpg_data
Changes since v3:
- Patch 4/7
- Fix function returns
- Move register definition to top of file
- Revert max_brightness and probe accidental changes
- Combine init_sdam() and parse_sdam()
- Change error prints in probe to use dev_err_probe
- Remove ppg_en variable
- Update when pbs triggers are set/cleared
- Patch 6/7
- Remove use of nvmem_count
- Move register definition to top of file
- Remove lpg_get_sdam_lut_idx()
Changes since v2:
- Patch 1/7
- Fix dt_binding_check error
- Rename binding file to match compatible
- Iclude SoC specific comptaibles
- Patch 2/7
- Update nvmem-names list
- Patch 3/7
- Update EXPORT_SYMBOL to EXPORT_SYMBOL_GPL
- Fix return/break logic in qcom_pbs_wait_for_ack()
- Update iterators to be int
- Add constants
- Fix function calls in qcom_pbs_trigger_event()
- Remove unnessary comments
- Return -EPROBE_DEFER from get_pbs_client_device()
Changes since v1:
- Patch 1/7
- Fix dt_binding_check errors
- Update binding description
- Path 2/7
- Fix dt_binding_check errors
- Update per variant constraints
- Update nvmem description
- Patch 3/7
- Update get_pbs_client_device()
- Drop use of printk
- Remove unused function
Tested-by: Luca Weiss <[email protected]> # sdm632-fairphone-fp3 (pmi632)
Anjelique Melendez (7):
dt-bindings: soc: qcom: Add qcom,pbs bindings
dt-bindings: leds: leds-qcom-lpg: Add support for LPG PPG
soc: qcom: add QCOM PBS driver
leds: rgb: leds-qcom-lpg: Add support for single SDAM PPG
leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG
leds: rgb: leds-qcom-lpg: Include support for PPG with dedicated LUT
SDAM
leds: rgb: Update PM8350C lpg_data to support two-nvmem PPG Scheme
.../bindings/leds/leds-qcom-lpg.yaml | 89 ++++-
.../bindings/soc/qcom/qcom,pbs.yaml | 46 +++
drivers/leds/rgb/leds-qcom-lpg.c | 359 ++++++++++++++++--
drivers/soc/qcom/Kconfig | 9 +
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/qcom-pbs.c | 243 ++++++++++++
include/linux/soc/qcom/qcom-pbs.h | 30 ++
7 files changed, 749 insertions(+), 28 deletions(-)
create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,pbs.yaml
create mode 100644 drivers/soc/qcom/qcom-pbs.c
create mode 100644 include/linux/soc/qcom/qcom-pbs.h
--
2.41.0
Update the pm8350c lpg_data struct so that pm8350c devices are treated as
PWM devices that support two-nvmem PPG scheme.
Signed-off-by: Anjelique Melendez <[email protected]>
---
drivers/leds/rgb/leds-qcom-lpg.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index 910c7cf740cc..8962ea13df29 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -1800,11 +1800,13 @@ static const struct lpg_data pm8150l_lpg_data = {
static const struct lpg_data pm8350c_pwm_data = {
.triled_base = 0xef00,
+ .lut_size = 122,
+
.num_channels = 4,
.channels = (const struct lpg_channel_data[]) {
- { .base = 0xe800, .triled_mask = BIT(7) },
- { .base = 0xe900, .triled_mask = BIT(6) },
- { .base = 0xea00, .triled_mask = BIT(5) },
+ { .base = 0xe800, .triled_mask = BIT(7), .sdam_offset = 0x48 },
+ { .base = 0xe900, .triled_mask = BIT(6), .sdam_offset = 0x56 },
+ { .base = 0xea00, .triled_mask = BIT(5), .sdam_offset = 0x64 },
{ .base = 0xeb00 },
},
};
--
2.41.0
On PMICs such as PM8350C, the pattern lookup table (LUT) is stored in a
separate SDAM from the one where the lpg per-channel data is stored.
Add support for PPG with a dedicated LUT SDAM while maintaining backward
compatibility for those targets that use only a single SDAM.
Co-developed-by: Guru Das Srinagesh <[email protected]>
Signed-off-by: Guru Das Srinagesh <[email protected]>
Signed-off-by: Anjelique Melendez <[email protected]>
---
drivers/leds/rgb/leds-qcom-lpg.c | 95 ++++++++++++++++++++++++++------
1 file changed, 78 insertions(+), 17 deletions(-)
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index a6cea6bd7167..910c7cf740cc 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -42,6 +42,8 @@
#define PWM_DTEST_REG(x) (0xe2 + (x) - 1)
#define SDAM_REG_PBS_SEQ_EN 0x42
+#define SDAM_PBS_TRIG_SET 0xe5
+#define SDAM_PBS_TRIG_CLR 0xe6
#define TRI_LED_SRC_SEL 0x45
#define TRI_LED_EN_CTL 0x46
@@ -61,7 +63,10 @@
#define RAMP_STEP_DURATION(x) (((x) * 1000 / DEFAULT_TICK_DURATION_US) & 0xff)
/* LPG common config settings for PPG */
+#define SDAM_START_BASE 0x40
#define SDAM_REG_RAMP_STEP_DURATION 0x47
+
+#define SDAM_LUT_SDAM_LUT_PATTERN_OFFSET 0x45
#define SDAM_LPG_SDAM_LUT_PATTERN_OFFSET 0x80
/* LPG per channel config settings for PPG */
@@ -70,6 +75,8 @@
#define SDAM_END_INDEX_OFFSET 0x3
#define SDAM_START_INDEX_OFFSET 0x4
#define SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET 0x6
+#define SDAM_PAUSE_HI_MULTIPLIER_OFFSET 0x8
+#define SDAM_PAUSE_LO_MULTIPLIER_OFFSET 0x9
struct lpg_channel;
struct lpg_data;
@@ -86,6 +93,7 @@ struct lpg_data;
* @lut_bitmap: allocation bitmap for LUT entries
* @pbs_dev: PBS device
* @lpg_chan_sdam: LPG SDAM peripheral device
+ * @lut_sdam: LUT SDAM peripheral device
* @pbs_en_bitmap: bitmap for tracking PBS triggers
* @triled_base: base address of the TRILED block (optional)
* @triled_src: power-source for the TRILED
@@ -110,6 +118,7 @@ struct lpg {
struct pbs_dev *pbs_dev;
struct nvmem_device *lpg_chan_sdam;
+ struct nvmem_device *lut_sdam;
unsigned long pbs_en_bitmap;
u32 triled_base;
@@ -253,6 +262,13 @@ static int lpg_clear_pbs_trigger(struct lpg_channel *chan)
rc = nvmem_device_write(chan->lpg->lpg_chan_sdam, SDAM_REG_PBS_SEQ_EN, 1, &val);
if (rc < 0)
return rc;
+
+ if (chan->lpg->lut_sdam) {
+ val = PBS_SW_TRIG_BIT;
+ rc = nvmem_device_write(chan->lpg->lpg_chan_sdam, SDAM_PBS_TRIG_CLR, 1, &val);
+ if (rc < 0)
+ return rc;
+ }
}
return 0;
@@ -268,9 +284,15 @@ static int lpg_set_pbs_trigger(struct lpg_channel *chan)
if (rc < 0)
return rc;
- rc = qcom_pbs_trigger_event(chan->lpg->pbs_dev, val);
- if (rc < 0)
- return rc;
+ if (chan->lpg->lut_sdam) {
+ rc = nvmem_device_write(chan->lpg->lpg_chan_sdam, SDAM_PBS_TRIG_SET, 1, &val);
+ if (rc < 0)
+ return rc;
+ } else {
+ rc = qcom_pbs_trigger_event(chan->lpg->pbs_dev, val);
+ if (rc < 0)
+ return rc;
+ }
}
set_bit(chan->lpg_idx, &chan->lpg->pbs_en_bitmap);
@@ -341,8 +363,15 @@ static int lpg_lut_store_sdam(struct lpg *lpg, struct led_pattern *pattern,
for (i = 0; i < len; i++) {
brightness = pattern[i].brightness;
- addr = SDAM_LPG_SDAM_LUT_PATTERN_OFFSET + i + idx;
- rc = nvmem_device_write(lpg->lpg_chan_sdam, addr, 1, &brightness);
+
+ if (lpg->lut_sdam) {
+ addr = SDAM_LUT_SDAM_LUT_PATTERN_OFFSET + i + idx;
+ rc = nvmem_device_write(lpg->lut_sdam, addr, 1, &brightness);
+ } else {
+ addr = SDAM_LPG_SDAM_LUT_PATTERN_OFFSET + i + idx;
+ rc = nvmem_device_write(lpg->lpg_chan_sdam, addr, 1, &brightness);
+ }
+
if (rc < 0)
return rc;
}
@@ -606,13 +635,28 @@ static void lpg_sdam_apply_lut_control(struct lpg_channel *chan)
struct nvmem_device *lpg_chan_sdam = chan->lpg->lpg_chan_sdam;
unsigned int lo_idx = chan->pattern_lo_idx;
unsigned int hi_idx = chan->pattern_hi_idx;
- u8 val = 0, conf = 0;
+ u8 val = 0, conf = 0, lut_offset = 0;
+ unsigned int hi_pause, lo_pause;
+ struct lpg *lpg = chan->lpg;
if (!chan->ramp_enabled || chan->pattern_lo_idx == chan->pattern_hi_idx)
return;
+ hi_pause = DIV_ROUND_UP(chan->ramp_hi_pause_ms, chan->ramp_tick_ms);
+ lo_pause = DIV_ROUND_UP(chan->ramp_lo_pause_ms, chan->ramp_tick_ms);
+
if (!chan->ramp_oneshot)
conf |= LPG_PATTERN_CONFIG_REPEAT;
+ if (chan->ramp_hi_pause_ms && lpg->lut_sdam)
+ conf |= LPG_PATTERN_CONFIG_PAUSE_HI;
+ if (chan->ramp_lo_pause_ms && lpg->lut_sdam)
+ conf |= LPG_PATTERN_CONFIG_PAUSE_LO;
+
+ if (lpg->lut_sdam) {
+ lut_offset = SDAM_LUT_SDAM_LUT_PATTERN_OFFSET - SDAM_START_BASE;
+ hi_idx += lut_offset;
+ lo_idx += lut_offset;
+ }
nvmem_device_write(lpg_chan_sdam, SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET + chan->sdam_offset, 1, &val);
nvmem_device_write(lpg_chan_sdam, SDAM_PATTERN_CONFIG_OFFSET + chan->sdam_offset, 1, &conf);
@@ -621,6 +665,12 @@ static void lpg_sdam_apply_lut_control(struct lpg_channel *chan)
val = RAMP_STEP_DURATION(chan->ramp_tick_ms);
nvmem_device_write(lpg_chan_sdam, SDAM_REG_RAMP_STEP_DURATION, 1, &val);
+
+ if (lpg->lut_sdam || lpg->lut_base) {
+ nvmem_device_write(lpg_chan_sdam, SDAM_PAUSE_HI_MULTIPLIER_OFFSET + chan->sdam_offset, 1, &hi_pause);
+ nvmem_device_write(lpg_chan_sdam, SDAM_PAUSE_LO_MULTIPLIER_OFFSET + chan->sdam_offset, 1, &lo_pause);
+ }
+
}
static void lpg_apply_lut_control(struct lpg_channel *chan)
@@ -1004,8 +1054,8 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
* enabled. In this scenario the delta_t of the middle entry (i.e. the
* last in the programmed pattern) determines the "high pause".
*
- * SDAM devices supporting LUT do not support "low pause", "high pause"
- * or "ping pong"
+ * SDAM-based devices do not support "ping pong", and only supports
+ * "low pause" and "high pause" with a dedicated SDAM LUT.
*/
/* Detect palindromes and use "ping pong" to reduce LUT usage */
@@ -1050,9 +1100,9 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
/*
* Find "low pause" and "high pause" in the pattern in the LUT case.
- * LPGs using SDAM for pattern require equal duration of all steps
+ * PPG LPGs without LUT SDAM require equal duration of all steps.
*/
- if (lpg->lut_base) {
+ if (lpg->lut_base || lpg->lut_sdam) {
lo_pause = pattern[0].delta_t;
hi_pause = pattern[actual_len - 1].delta_t;
} else {
@@ -1517,17 +1567,28 @@ static int lpg_init_sdam(struct lpg *lpg)
sdam_count = of_property_count_strings(lpg->dev->of_node, "nvmem-names");
if (sdam_count <= 0)
return 0;
+ if (sdam_count > 2)
+ return -EINVAL;
- /* get the SDAM device for LPG/LUT config */
+ /* get the 1st nvmem device for LPG/LUT config */
lpg->lpg_chan_sdam = devm_nvmem_device_get(lpg->dev, "lpg_chan_sdam");
if (IS_ERR(lpg->lpg_chan_sdam))
return dev_err_probe(lpg->dev, PTR_ERR(lpg->lpg_chan_sdam),
- "Failed to get lpg sdam device\n");
-
- lpg->pbs_dev = get_pbs_client_device(lpg->dev);
- if (IS_ERR(lpg->pbs_dev))
- return dev_err_probe(lpg->dev, PTR_ERR(lpg->pbs_dev),
- "Failed to get PBS client device\n");
+ "Failed to get lpg_chan_sdam device\n");
+
+ if (sdam_count == 1) {
+ /* get PBS device node if single SDAM device */
+ lpg->pbs_dev = get_pbs_client_device(lpg->dev);
+ if (IS_ERR(lpg->pbs_dev))
+ return dev_err_probe(lpg->dev, PTR_ERR(lpg->pbs_dev),
+ "Failed to get PBS client device\n");
+ } else if (sdam_count == 2) {
+ /* get the 2nd SDAM device for LUT pattern */
+ lpg->lut_sdam = devm_nvmem_device_get(lpg->dev, "lut_sdam");
+ if (IS_ERR(lpg->lut_sdam))
+ return dev_err_probe(lpg->dev, PTR_ERR(lpg->lut_sdam),
+ "Failed to get lut_sdam device\n");
+ }
for (i = 0; i < lpg->num_channels; i++) {
chan = &lpg->channels[i];
--
2.41.0
On Fri Sep 29, 2023 at 2:38 AM CEST, Anjelique Melendez wrote:
> In certain PMICs, LUT pattern and LPG configuration is stored in SDAM
> modules instead of LUT peripheral. This feature is called PPG.
>
> This change series adds support for PPG. Thanks!
>
> Changes since v4:
> - Patch 3/7
> - Get rid of r/w helpers
> - Use regmap_read_poll_timeout() in qcom_pbs_wait_for_ack()
> - Update error path in qcom_pbs_trigger_event()
> - Fix reverse christmas tree
> - Patch 4/7
> - Get rid of r/w helpers
> - Update variables to use "sdam" instead of "nvmem"
> - Fix comments
> - Fix reverse christmas tree
> - Update lpg_pattern_set() logic
> - Patch 5/7
> - Removed sdam_lut_base from lpg_data
> Changes since v3:
> - Patch 4/7
> - Fix function returns
> - Move register definition to top of file
> - Revert max_brightness and probe accidental changes
> - Combine init_sdam() and parse_sdam()
> - Change error prints in probe to use dev_err_probe
> - Remove ppg_en variable
> - Update when pbs triggers are set/cleared
> - Patch 6/7
> - Remove use of nvmem_count
> - Move register definition to top of file
> - Remove lpg_get_sdam_lut_idx()
> Changes since v2:
> - Patch 1/7
> - Fix dt_binding_check error
> - Rename binding file to match compatible
> - Iclude SoC specific comptaibles
> - Patch 2/7
> - Update nvmem-names list
> - Patch 3/7
> - Update EXPORT_SYMBOL to EXPORT_SYMBOL_GPL
> - Fix return/break logic in qcom_pbs_wait_for_ack()
> - Update iterators to be int
> - Add constants
> - Fix function calls in qcom_pbs_trigger_event()
> - Remove unnessary comments
> - Return -EPROBE_DEFER from get_pbs_client_device()
> Changes since v1:
> - Patch 1/7
> - Fix dt_binding_check errors
> - Update binding description
> - Path 2/7
> - Fix dt_binding_check errors
> - Update per variant constraints
> - Update nvmem description
> - Patch 3/7
> - Update get_pbs_client_device()
> - Drop use of printk
> - Remove unused function
>
> Tested-by: Luca Weiss <[email protected]> # sdm632-fairphone-fp3 (pmi632)
Hi Anjelique,
Actually I've retested this now on PMI632 (and also realized that my
previous tests weren't correct and wasn't actually using hw_pattern).
Using the following commands (after boot) I'm expecting to get a
500ms on 500ms off blinking pattern between white (255 255 255) and off
(0 0 0).
echo pattern > /sys/class/leds/rgb:status/trigger
echo -1 > /sys/class/leds/rgb:status/repeat
echo "255 255 255" > /sys/class/leds/rgb:status/multi_intensity
echo "255 500 255 0 0 500 0 0" > /sys/class/leds/rgb:status/hw_pattern
What I actually see is it blinking between cyan (0 255 255) and red (255
0 0).
At some point after playing with many patterns I got it to actually
cycle between white and off, but I couldn't reproduce this again (or I
didn't try hard enough).
But with this example it correctly blinks red on-off.
echo "255 0 0" > /sys/class/leds/rgb:status/multi_intensity
echo "255 500 255 0 0 500 0 0" > /sys/class/leds/rgb:status/hw_pattern
With "0 255 0" and "0 0 255" the other colors also work fine, it's just
the combinations that seem somewhat broken.
Regards
Luca
>
> Anjelique Melendez (7):
> dt-bindings: soc: qcom: Add qcom,pbs bindings
> dt-bindings: leds: leds-qcom-lpg: Add support for LPG PPG
> soc: qcom: add QCOM PBS driver
> leds: rgb: leds-qcom-lpg: Add support for single SDAM PPG
> leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG
> leds: rgb: leds-qcom-lpg: Include support for PPG with dedicated LUT
> SDAM
> leds: rgb: Update PM8350C lpg_data to support two-nvmem PPG Scheme
>
> .../bindings/leds/leds-qcom-lpg.yaml | 89 ++++-
> .../bindings/soc/qcom/qcom,pbs.yaml | 46 +++
> drivers/leds/rgb/leds-qcom-lpg.c | 359 ++++++++++++++++--
> drivers/soc/qcom/Kconfig | 9 +
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/qcom-pbs.c | 243 ++++++++++++
> include/linux/soc/qcom/qcom-pbs.h | 30 ++
> 7 files changed, 749 insertions(+), 28 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,pbs.yaml
> create mode 100644 drivers/soc/qcom/qcom-pbs.c
> create mode 100644 include/linux/soc/qcom/qcom-pbs.h
On Thu, 28 Sep 2023, Anjelique Melendez wrote:
> Update the pm8350c lpg_data struct so that pm8350c devices are treated as
> PWM devices that support two-nvmem PPG scheme.
>
> Signed-off-by: Anjelique Melendez <[email protected]>
> ---
> drivers/leds/rgb/leds-qcom-lpg.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
Reviewed-by: Lee Jones <[email protected]>
> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> index 910c7cf740cc..8962ea13df29 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> @@ -1800,11 +1800,13 @@ static const struct lpg_data pm8150l_lpg_data = {
> static const struct lpg_data pm8350c_pwm_data = {
> .triled_base = 0xef00,
>
> + .lut_size = 122,
> +
> .num_channels = 4,
> .channels = (const struct lpg_channel_data[]) {
> - { .base = 0xe800, .triled_mask = BIT(7) },
> - { .base = 0xe900, .triled_mask = BIT(6) },
> - { .base = 0xea00, .triled_mask = BIT(5) },
> + { .base = 0xe800, .triled_mask = BIT(7), .sdam_offset = 0x48 },
> + { .base = 0xe900, .triled_mask = BIT(6), .sdam_offset = 0x56 },
> + { .base = 0xea00, .triled_mask = BIT(5), .sdam_offset = 0x64 },
> { .base = 0xeb00 },
> },
> };
> --
> 2.41.0
>
--
Lee Jones [李琼斯]
On Thu, 28 Sep 2023, Anjelique Melendez wrote:
> On PMICs such as PM8350C, the pattern lookup table (LUT) is stored in a
> separate SDAM from the one where the lpg per-channel data is stored.
>
> Add support for PPG with a dedicated LUT SDAM while maintaining backward
> compatibility for those targets that use only a single SDAM.
>
> Co-developed-by: Guru Das Srinagesh <[email protected]>
> Signed-off-by: Guru Das Srinagesh <[email protected]>
> Signed-off-by: Anjelique Melendez <[email protected]>
> ---
> drivers/leds/rgb/leds-qcom-lpg.c | 95 ++++++++++++++++++++++++++------
> 1 file changed, 78 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> index a6cea6bd7167..910c7cf740cc 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> @@ -42,6 +42,8 @@
> #define PWM_DTEST_REG(x) (0xe2 + (x) - 1)
>
> #define SDAM_REG_PBS_SEQ_EN 0x42
> +#define SDAM_PBS_TRIG_SET 0xe5
> +#define SDAM_PBS_TRIG_CLR 0xe6
>
> #define TRI_LED_SRC_SEL 0x45
> #define TRI_LED_EN_CTL 0x46
> @@ -61,7 +63,10 @@
> #define RAMP_STEP_DURATION(x) (((x) * 1000 / DEFAULT_TICK_DURATION_US) & 0xff)
>
> /* LPG common config settings for PPG */
> +#define SDAM_START_BASE 0x40
> #define SDAM_REG_RAMP_STEP_DURATION 0x47
> +
> +#define SDAM_LUT_SDAM_LUT_PATTERN_OFFSET 0x45
> #define SDAM_LPG_SDAM_LUT_PATTERN_OFFSET 0x80
>
> /* LPG per channel config settings for PPG */
> @@ -70,6 +75,8 @@
> #define SDAM_END_INDEX_OFFSET 0x3
> #define SDAM_START_INDEX_OFFSET 0x4
> #define SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET 0x6
> +#define SDAM_PAUSE_HI_MULTIPLIER_OFFSET 0x8
> +#define SDAM_PAUSE_LO_MULTIPLIER_OFFSET 0x9
>
> struct lpg_channel;
> struct lpg_data;
> @@ -86,6 +93,7 @@ struct lpg_data;
> * @lut_bitmap: allocation bitmap for LUT entries
> * @pbs_dev: PBS device
> * @lpg_chan_sdam: LPG SDAM peripheral device
> + * @lut_sdam: LUT SDAM peripheral device
> * @pbs_en_bitmap: bitmap for tracking PBS triggers
> * @triled_base: base address of the TRILED block (optional)
> * @triled_src: power-source for the TRILED
> @@ -110,6 +118,7 @@ struct lpg {
>
> struct pbs_dev *pbs_dev;
> struct nvmem_device *lpg_chan_sdam;
> + struct nvmem_device *lut_sdam;
> unsigned long pbs_en_bitmap;
>
> u32 triled_base;
> @@ -253,6 +262,13 @@ static int lpg_clear_pbs_trigger(struct lpg_channel *chan)
> rc = nvmem_device_write(chan->lpg->lpg_chan_sdam, SDAM_REG_PBS_SEQ_EN, 1, &val);
> if (rc < 0)
> return rc;
> +
> + if (chan->lpg->lut_sdam) {
> + val = PBS_SW_TRIG_BIT;
> + rc = nvmem_device_write(chan->lpg->lpg_chan_sdam, SDAM_PBS_TRIG_CLR, 1, &val);
> + if (rc < 0)
> + return rc;
> + }
> }
>
> return 0;
> @@ -268,9 +284,15 @@ static int lpg_set_pbs_trigger(struct lpg_channel *chan)
> if (rc < 0)
> return rc;
>
> - rc = qcom_pbs_trigger_event(chan->lpg->pbs_dev, val);
> - if (rc < 0)
> - return rc;
> + if (chan->lpg->lut_sdam) {
> + rc = nvmem_device_write(chan->lpg->lpg_chan_sdam, SDAM_PBS_TRIG_SET, 1, &val);
> + if (rc < 0)
> + return rc;
> + } else {
> + rc = qcom_pbs_trigger_event(chan->lpg->pbs_dev, val);
> + if (rc < 0)
> + return rc;
> + }
> }
> set_bit(chan->lpg_idx, &chan->lpg->pbs_en_bitmap);
>
> @@ -341,8 +363,15 @@ static int lpg_lut_store_sdam(struct lpg *lpg, struct led_pattern *pattern,
>
> for (i = 0; i < len; i++) {
> brightness = pattern[i].brightness;
> - addr = SDAM_LPG_SDAM_LUT_PATTERN_OFFSET + i + idx;
> - rc = nvmem_device_write(lpg->lpg_chan_sdam, addr, 1, &brightness);
> +
> + if (lpg->lut_sdam) {
> + addr = SDAM_LUT_SDAM_LUT_PATTERN_OFFSET + i + idx;
> + rc = nvmem_device_write(lpg->lut_sdam, addr, 1, &brightness);
> + } else {
> + addr = SDAM_LPG_SDAM_LUT_PATTERN_OFFSET + i + idx;
> + rc = nvmem_device_write(lpg->lpg_chan_sdam, addr, 1, &brightness);
> + }
> +
> if (rc < 0)
> return rc;
> }
> @@ -606,13 +635,28 @@ static void lpg_sdam_apply_lut_control(struct lpg_channel *chan)
> struct nvmem_device *lpg_chan_sdam = chan->lpg->lpg_chan_sdam;
> unsigned int lo_idx = chan->pattern_lo_idx;
> unsigned int hi_idx = chan->pattern_hi_idx;
> - u8 val = 0, conf = 0;
> + u8 val = 0, conf = 0, lut_offset = 0;
> + unsigned int hi_pause, lo_pause;
> + struct lpg *lpg = chan->lpg;
>
> if (!chan->ramp_enabled || chan->pattern_lo_idx == chan->pattern_hi_idx)
> return;
>
> + hi_pause = DIV_ROUND_UP(chan->ramp_hi_pause_ms, chan->ramp_tick_ms);
> + lo_pause = DIV_ROUND_UP(chan->ramp_lo_pause_ms, chan->ramp_tick_ms);
> +
> if (!chan->ramp_oneshot)
> conf |= LPG_PATTERN_CONFIG_REPEAT;
> + if (chan->ramp_hi_pause_ms && lpg->lut_sdam)
> + conf |= LPG_PATTERN_CONFIG_PAUSE_HI;
> + if (chan->ramp_lo_pause_ms && lpg->lut_sdam)
> + conf |= LPG_PATTERN_CONFIG_PAUSE_LO;
> +
> + if (lpg->lut_sdam) {
> + lut_offset = SDAM_LUT_SDAM_LUT_PATTERN_OFFSET - SDAM_START_BASE;
> + hi_idx += lut_offset;
> + lo_idx += lut_offset;
> + }
>
> nvmem_device_write(lpg_chan_sdam, SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET + chan->sdam_offset, 1, &val);
> nvmem_device_write(lpg_chan_sdam, SDAM_PATTERN_CONFIG_OFFSET + chan->sdam_offset, 1, &conf);
> @@ -621,6 +665,12 @@ static void lpg_sdam_apply_lut_control(struct lpg_channel *chan)
>
> val = RAMP_STEP_DURATION(chan->ramp_tick_ms);
> nvmem_device_write(lpg_chan_sdam, SDAM_REG_RAMP_STEP_DURATION, 1, &val);
> +
> + if (lpg->lut_sdam || lpg->lut_base) {
> + nvmem_device_write(lpg_chan_sdam, SDAM_PAUSE_HI_MULTIPLIER_OFFSET + chan->sdam_offset, 1, &hi_pause);
> + nvmem_device_write(lpg_chan_sdam, SDAM_PAUSE_LO_MULTIPLIER_OFFSET + chan->sdam_offset, 1, &lo_pause);
> + }
> +
> }
>
> static void lpg_apply_lut_control(struct lpg_channel *chan)
> @@ -1004,8 +1054,8 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
> * enabled. In this scenario the delta_t of the middle entry (i.e. the
> * last in the programmed pattern) determines the "high pause".
> *
> - * SDAM devices supporting LUT do not support "low pause", "high pause"
> - * or "ping pong"
> + * SDAM-based devices do not support "ping pong", and only supports
> + * "low pause" and "high pause" with a dedicated SDAM LUT.
> */
>
> /* Detect palindromes and use "ping pong" to reduce LUT usage */
> @@ -1050,9 +1100,9 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
>
> /*
> * Find "low pause" and "high pause" in the pattern in the LUT case.
> - * LPGs using SDAM for pattern require equal duration of all steps
> + * PPG LPGs without LUT SDAM require equal duration of all steps.
> */
> - if (lpg->lut_base) {
> + if (lpg->lut_base || lpg->lut_sdam) {
> lo_pause = pattern[0].delta_t;
> hi_pause = pattern[actual_len - 1].delta_t;
> } else {
> @@ -1517,17 +1567,28 @@ static int lpg_init_sdam(struct lpg *lpg)
> sdam_count = of_property_count_strings(lpg->dev->of_node, "nvmem-names");
> if (sdam_count <= 0)
> return 0;
> + if (sdam_count > 2)
I usually prefer that all magic numbers are #defined.
> + return -EINVAL;
>
> - /* get the SDAM device for LPG/LUT config */
> + /* get the 1st nvmem device for LPG/LUT config */
Take the opportunity to capitalise this.
> lpg->lpg_chan_sdam = devm_nvmem_device_get(lpg->dev, "lpg_chan_sdam");
> if (IS_ERR(lpg->lpg_chan_sdam))
> return dev_err_probe(lpg->dev, PTR_ERR(lpg->lpg_chan_sdam),
> - "Failed to get lpg sdam device\n");
> -
> - lpg->pbs_dev = get_pbs_client_device(lpg->dev);
> - if (IS_ERR(lpg->pbs_dev))
> - return dev_err_probe(lpg->dev, PTR_ERR(lpg->pbs_dev),
> - "Failed to get PBS client device\n");
> + "Failed to get lpg_chan_sdam device\n");
This is less readable for the user.
> +
> + if (sdam_count == 1) {
> + /* get PBS device node if single SDAM device */
Capital - and the one below.
Once these nits are fixed, please apply my:
Reviewed-by: Lee Jones <[email protected]>
> + lpg->pbs_dev = get_pbs_client_device(lpg->dev);
> + if (IS_ERR(lpg->pbs_dev))
> + return dev_err_probe(lpg->dev, PTR_ERR(lpg->pbs_dev),
> + "Failed to get PBS client device\n");
> + } else if (sdam_count == 2) {
> + /* get the 2nd SDAM device for LUT pattern */
> + lpg->lut_sdam = devm_nvmem_device_get(lpg->dev, "lut_sdam");
> + if (IS_ERR(lpg->lut_sdam))
> + return dev_err_probe(lpg->dev, PTR_ERR(lpg->lut_sdam),
> + "Failed to get lut_sdam device\n");
> + }
>
> for (i = 0; i < lpg->num_channels; i++) {
> chan = &lpg->channels[i];
> --
> 2.41.0
>
--
Lee Jones [李琼斯]
On 10/1/2023 7:15 AM, Luca Weiss wrote:
> On Fri Sep 29, 2023 at 2:38 AM CEST, Anjelique Melendez wrote:
>> In certain PMICs, LUT pattern and LPG configuration is stored in SDAM
>> modules instead of LUT peripheral. This feature is called PPG.
>>
>> This change series adds support for PPG. Thanks!
[..]
>>
>> Tested-by: Luca Weiss <[email protected]> # sdm632-fairphone-fp3 (pmi632)
>
> Hi Anjelique,
>
> Actually I've retested this now on PMI632 (and also realized that my
> previous tests weren't correct and wasn't actually using hw_pattern).
>
> Using the following commands (after boot) I'm expecting to get a
> 500ms on 500ms off blinking pattern between white (255 255 255) and off
> (0 0 0).
>
> echo pattern > /sys/class/leds/rgb:status/trigger
> echo -1 > /sys/class/leds/rgb:status/repeat
>
> echo "255 255 255" > /sys/class/leds/rgb:status/multi_intensity
> echo "255 500 255 0 0 500 0 0" > /sys/class/leds/rgb:status/hw_pattern
>
> What I actually see is it blinking between cyan (0 255 255) and red (255
> 0 0).
> At some point after playing with many patterns I got it to actually
> cycle between white and off, but I couldn't reproduce this again (or I
> didn't try hard enough).
>
>
> But with this example it correctly blinks red on-off.
>
> echo "255 0 0" > /sys/class/leds/rgb:status/multi_intensity
> echo "255 500 255 0 0 500 0 0" > /sys/class/leds/rgb:status/hw_pattern
>
> With "0 255 0" and "0 0 255" the other colors also work fine, it's just
> the combinations that seem somewhat broken.
>
> Regards
> Luca
>
>
Hi Luca,
Thanks for testing again and the feedback!
Looks like for multicolor devices there is a small concurrency issue with
enabling pattern at the same time for all the led channels. This could be
why you observed your device blinking between red (255 0 0) and cyan (0 255 255),
instead of seeing all channels (255 255 255) blink.
The fix I'm planing to include in the next series is is to disable the multicolor led
channels first, then configure all channels, and finally re-enable channels
so that pattern is triggered at the same time for each all of the channels.
I am currently testing with pm8350c device so if you are able to test next series
on pmi632 it would be very appreciated!
Thanks,
Anjelique
On Thu Oct 12, 2023 at 11:50 PM CEST, Anjelique Melendez wrote:
>
>
> On 10/1/2023 7:15 AM, Luca Weiss wrote:
> > On Fri Sep 29, 2023 at 2:38 AM CEST, Anjelique Melendez wrote:
> >> In certain PMICs, LUT pattern and LPG configuration is stored in SDAM
> >> modules instead of LUT peripheral. This feature is called PPG.
> >>
> >> This change series adds support for PPG. Thanks!
> [..]
> >>
> >> Tested-by: Luca Weiss <[email protected]> # sdm632-fairphone-fp3 (pmi632)
> >
> > Hi Anjelique,
> >
> > Actually I've retested this now on PMI632 (and also realized that my
> > previous tests weren't correct and wasn't actually using hw_pattern).
> >
> > Using the following commands (after boot) I'm expecting to get a
> > 500ms on 500ms off blinking pattern between white (255 255 255) and off
> > (0 0 0).
> >
> > echo pattern > /sys/class/leds/rgb:status/trigger
> > echo -1 > /sys/class/leds/rgb:status/repeat
> >
> > echo "255 255 255" > /sys/class/leds/rgb:status/multi_intensity
> > echo "255 500 255 0 0 500 0 0" > /sys/class/leds/rgb:status/hw_pattern
> >
> > What I actually see is it blinking between cyan (0 255 255) and red (255
> > 0 0).
> > At some point after playing with many patterns I got it to actually
> > cycle between white and off, but I couldn't reproduce this again (or I
> > didn't try hard enough).
> >
> >
> > But with this example it correctly blinks red on-off.
> >
> > echo "255 0 0" > /sys/class/leds/rgb:status/multi_intensity
> > echo "255 500 255 0 0 500 0 0" > /sys/class/leds/rgb:status/hw_pattern
> >
> > With "0 255 0" and "0 0 255" the other colors also work fine, it's just
> > the combinations that seem somewhat broken.
> >
> > Regards
> > Luca
> >
> >
> Hi Luca,
>
> Thanks for testing again and the feedback!
> Looks like for multicolor devices there is a small concurrency issue with
> enabling pattern at the same time for all the led channels. This could be
> why you observed your device blinking between red (255 0 0) and cyan (0 255 255),
> instead of seeing all channels (255 255 255) blink.
> The fix I'm planing to include in the next series is is to disable the multicolor led
> channels first, then configure all channels, and finally re-enable channels
> so that pattern is triggered at the same time for each all of the channels.
Sounds reasonable I think!
>
> I am currently testing with pm8350c device so if you are able to test next series
> on pmi632 it would be very appreciated!
Will do, thanks for fixing it up!
Regards
Luca
>
> Thanks,
> Anjelique