In certain PMICs, LUT pattern and LPG configuration can be stored in SDAM
modules instead of LUT peripheral. This feature is called PPG.
This change series adds support for PPG. Thanks!
Anjelique Melendez (7):
dt-bindings: soc: qcom: Add qcom-pbs bindings
dt-bindings: leds: leds-qcom-lpg: Add support for LUT through NVMEM
devices
soc: qcom: add QCOM PBS driver
leds: rgb: leds-qcom-lpg: Add support for LUT pattern through single
SDAM
leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG
leds: rgb: leds-qcom-lpg: Support two-nvmem PPG Scheme
leds: rgb: Update PM8350C lpg_data to support two-nvmem PPG Scheme
.../bindings/leds/leds-qcom-lpg.yaml | 85 ++++
.../bindings/soc/qcom/qcom-pbs.yaml | 41 ++
drivers/leds/rgb/leds-qcom-lpg.c | 393 ++++++++++++++++--
drivers/soc/qcom/Kconfig | 9 +
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/qcom-pbs.c | 343 +++++++++++++++
include/linux/soc/qcom/qcom-pbs.h | 36 ++
7 files changed, 877 insertions(+), 31 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.40.0
Update leds-qcom-lpg bindings to support LUT patterns through NVMEM
devices.
Signed-off-by: Anjelique Melendez <[email protected]>
---
.../bindings/leds/leds-qcom-lpg.yaml | 85 +++++++++++++++++++
1 file changed, 85 insertions(+)
diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
index e6f1999cb22f..c9d53820bf83 100644
--- a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
@@ -63,6 +63,31 @@ properties:
- description: dtest line to attach
- description: flags for the attachment
+ nvmem:
+ description: >
+ Phandle(s) of the nvmem device(s) to access the LUT stored in the SDAM module(s).
+ This property is required only when LUT mode is supported and the LUT pattern is
+ stored in SDAM modules instead of a LUT module.
+ minItems: 1
+ maxItems: 2
+
+ nvmem-names:
+ description: >
+ The nvmem device name(s) for the SDAM module(s) where the LUT pattern data is stored.
+ This property is required only when LUT mode is supported with SDAM module instead of
+ LUT module.
+ minItems: 1
+ items:
+ - const: lpg_chan_sdam
+ - const: lut_sdam
+
+ qcom,pbs-client:
+ $ref: /schemas/types.yaml#/definitions/phandle
+ description: >
+ Phandle of the PBS client used for sending the PBS trigger. This property is
+ required when LUT mode is supported and the LUT pattern is stored in a single
+ SDAM module instead of a LUT module.
+
multi-led:
type: object
$ref: leds-class-multicolor.yaml#
@@ -191,4 +216,64 @@ examples:
compatible = "qcom,pm8916-pwm";
#pwm-cells = <2>;
};
+ - |
+ #include <dt-bindings/leds/common.h>
+
+ led-controller {
+ compatible = "qcom,pm8350c-pwm";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ #pwm-cells = <2>;
+ nvmem-names = "lpg_chan_sdam" , "lut_sdam";
+ nvmem = <&pmk8550_sdam_21 &pmk8550_sdam_22>;
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_RED>;
+ label = "red";
+ };
+
+ led@2 {
+ reg = <2>;
+ color = <LED_COLOR_ID_GREEN>;
+ label = "green";
+ };
+
+ led@3 {
+ reg = <3>;
+ color = <LED_COLOR_ID_BLUE>;
+ label = "blue";
+ };
+ };
+ - |
+ #include <dt-bindings/leds/common.h>
+
+ led-controller {
+ compatible = "qcom,pmi632-lpg";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ #pwm-cells = <2>;
+ nvmem-names = "lpg_chan_sdam";
+ nvmem = <&pmi632_sdam7>;
+ qcom,pbs-client = <&pmi632_pbs_client3>;
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_RED>;
+ label = "red";
+ };
+
+ led@2 {
+ reg = <2>;
+ color = <LED_COLOR_ID_GREEN>;
+ label = "green";
+ };
+
+ led@3 {
+ reg = <3>;
+ color = <LED_COLOR_ID_BLUE>;
+ label = "blue";
+ };
+ };
+
...
--
2.40.0
On PMICs such as PM8350C, the lookup table containing the pattern data
is stored in a separate nvmem device from the one where the per-channel
data is stored.
Add support for two separate nvmems to handle this case while maintaining
backward compatibility for those targets that use only a single nvmem
device.
Signed-off-by: Guru Das Srinagesh <[email protected]>
Signed-off-by: Anjelique Melendez <[email protected]>
---
drivers/leds/rgb/leds-qcom-lpg.c | 112 ++++++++++++++++++++++++-------
1 file changed, 89 insertions(+), 23 deletions(-)
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index ac814a509781..273cb81260e7 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -60,6 +60,7 @@
#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_COUNT_MAX 64
@@ -69,6 +70,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;
@@ -85,7 +88,9 @@ struct lpg_data;
* @lut_bitmap: allocation bitmap for LUT entries
* @pbs_dev: PBS client device
* @lpg_chan_nvmem: LPG nvmem peripheral device
+ * @lut_nvmem: LUT nvmem peripheral device
* @pbs_en_bitmap: bitmap for tracking PBS triggers
+ * @nvmem_count: number of nvmems used for LUT and PPG config
* @lut_sdam_base: offset where LUT pattern begins in nvmem
* @ppg_en: Flag indicating whether PPG is enabled/used
* @triled_base: base address of the TRILED block (optional)
@@ -111,7 +116,9 @@ struct lpg {
struct pbs_dev *pbs_dev;
struct nvmem_device *lpg_chan_nvmem;
+ struct nvmem_device *lut_nvmem;
unsigned long pbs_en_bitmap;
+ unsigned int nvmem_count;
u32 lut_sdam_base;
bool ppg_en;
@@ -261,6 +268,8 @@ static int lpg_sdam_write(struct lpg *lpg, u16 addr, u8 val)
}
#define SDAM_REG_PBS_SEQ_EN 0x42
+#define SDAM_PBS_TRIG_SET 0xe5
+#define SDAM_PBS_TRIG_CLR 0xe6
#define PBS_SW_TRIG_BIT BIT(0)
static int lpg_clear_pbs_trigger(struct lpg_channel *chan)
@@ -272,6 +281,12 @@ static int lpg_clear_pbs_trigger(struct lpg_channel *chan)
rc = lpg_sdam_write(chan->lpg, SDAM_REG_PBS_SEQ_EN, 0);
if (rc < 0)
return rc;
+
+ if (chan->lpg->nvmem_count == 2) {
+ rc = lpg_sdam_write(chan->lpg, SDAM_PBS_TRIG_CLR, PBS_SW_TRIG_BIT);
+ if (rc < 0)
+ return rc;
+ }
}
return 0;
@@ -286,9 +301,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, PBS_SW_TRIG_BIT);
- if (rc < 0)
- return rc;
+ if (chan->lpg->nvmem_count == 1) {
+ rc = qcom_pbs_trigger_event(chan->lpg->pbs_dev, PBS_SW_TRIG_BIT);
+ if (rc < 0)
+ return rc;
+ } else {
+ rc = lpg_sdam_write(chan->lpg, SDAM_PBS_TRIG_SET, PBS_SW_TRIG_BIT);
+ if (rc < 0)
+ return rc;
+ }
}
set_bit(chan->lpg_idx, &chan->lpg->pbs_en_bitmap);
@@ -342,7 +363,12 @@ static int lpg_lut_store_sdam(struct lpg *lpg, struct led_pattern *pattern,
for (i = 0; i < len; i++) {
brightness = pattern[i].brightness;
addr = lpg->lut_sdam_base + i + idx;
- rc = lpg_sdam_write(lpg, addr, brightness);
+
+ if (lpg->nvmem_count == 1)
+ rc = lpg_sdam_write(lpg, addr, brightness);
+ else
+ rc = nvmem_device_write(lpg->lut_nvmem, addr, 1, &brightness);
+
if (rc < 0)
return rc;
}
@@ -601,24 +627,48 @@ static void lpg_apply_pwm_value(struct lpg_channel *chan)
#define LPG_PATTERN_CONFIG_PAUSE_HI BIT(1)
#define LPG_PATTERN_CONFIG_PAUSE_LO BIT(0)
+static u8 lpg_get_sdam_lut_idx(struct lpg_channel *chan, u8 idx)
+{
+ if (chan->lpg->nvmem_count == 2)
+ return chan->lpg->lut_sdam_base - SDAM_START_BASE + idx;
+ return idx;
+}
+
static void lpg_sdam_apply_lut_control(struct lpg_channel *chan)
{
u8 val, conf = 0;
+ unsigned int hi_pause, lo_pause;
struct lpg *lpg = chan->lpg;
+ 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->nvmem_count != 1)
+ conf |= LPG_PATTERN_CONFIG_PAUSE_HI;
+ if (chan->ramp_lo_pause_ms && lpg->nvmem_count != 1)
+ conf |= LPG_PATTERN_CONFIG_PAUSE_LO;
lpg_sdam_write(lpg, SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET + chan->sdam_offset, 0);
lpg_sdam_write(lpg, SDAM_PATTERN_CONFIG_OFFSET + chan->sdam_offset, conf);
- lpg_sdam_write(lpg, SDAM_END_INDEX_OFFSET + chan->sdam_offset, chan->pattern_hi_idx);
- lpg_sdam_write(lpg, SDAM_START_INDEX_OFFSET + chan->sdam_offset, chan->pattern_lo_idx);
+ val = lpg_get_sdam_lut_idx(chan, chan->pattern_hi_idx);
+ lpg_sdam_write(lpg, SDAM_END_INDEX_OFFSET + chan->sdam_offset, val);
+
+ val = lpg_get_sdam_lut_idx(chan, chan->pattern_lo_idx);
+ lpg_sdam_write(lpg, SDAM_START_INDEX_OFFSET + chan->sdam_offset, val);
val = RAMP_STEP_DURATION(chan->ramp_tick_ms);
if (val > 0)
val--;
lpg_sdam_write(lpg, SDAM_REG_RAMP_STEP_DURATION, val);
+
+ if (lpg->nvmem_count != 1) {
+ lpg_sdam_write(lpg, SDAM_PAUSE_HI_MULTIPLIER_OFFSET + chan->sdam_offset, hi_pause);
+ lpg_sdam_write(lpg, SDAM_PAUSE_LO_MULTIPLIER_OFFSET + chan->sdam_offset, lo_pause);
+ }
+
}
static void lpg_apply_lut_control(struct lpg_channel *chan)
@@ -1000,8 +1050,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".
*
- * NVMEM devices supporting LUT do not support "low pause", "high pause"
- * or "ping pong"
+ * All NVMEM devices supporting LUT do not support "ping pong"
+ * Single NVMEM devices supporting LUT do not support "low pause" and "high pause"
*/
/* Detect palindromes and use "ping pong" to reduce LUT usage */
@@ -1028,7 +1078,7 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
* Validate that all delta_t in the pattern are the same, with the
* exception of the middle element in case of ping_pong.
*/
- if (lpg->ppg_en) {
+ if (lpg->nvmem_count == 1) {
i = 1;
delta_t = pattern[0].delta_t;
} else {
@@ -1042,7 +1092,7 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
* Allow last entry in the full or shortened pattern to
* specify hi pause. Reject other variations.
*/
- if (i != actual_len - 1 || lpg->ppg_en)
+ if (i != actual_len - 1 || lpg->nvmem_count == 1)
goto out_free_pattern;
}
}
@@ -1051,8 +1101,8 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
if (delta_t >= BIT(9))
goto out_free_pattern;
- /* Find "low pause" and "high pause" in the pattern if not an NVMEM device*/
- if (!lpg->ppg_en) {
+ /* Find "low pause" and "high pause" in the pattern if not a single NVMEM device*/
+ if (lpg->nvmem_count != 1) {
lo_pause = pattern[0].delta_t;
hi_pause = pattern[actual_len - 1].delta_t;
}
@@ -1509,29 +1559,45 @@ static int lpg_parse_sdam(struct lpg *lpg)
{
int rc = 0;
- if (lpg->data->nvmem_count == 0)
+ lpg->nvmem_count = lpg->data->nvmem_count;
+ if (lpg->nvmem_count == 0)
return 0;
- /* get the nvmem device for LPG/LUT config */
+ if (lpg->nvmem_count > 2)
+ return -EINVAL;
+
+ /* get the 1st nvmem device for LPG/LUT config */
lpg->lpg_chan_nvmem = devm_nvmem_device_get(lpg->dev, "lpg_chan_sdam");
if (IS_ERR(lpg->lpg_chan_nvmem)) {
rc = PTR_ERR(lpg->lpg_chan_nvmem);
- if (rc != -EPROBE_DEFER)
- dev_err(lpg->dev, "Failed to get nvmem device, rc=%d\n", rc);
- return rc;
+ goto err;
}
- lpg->pbs_dev = get_pbs_client_device(lpg->dev);
- if (IS_ERR(lpg->pbs_dev)) {
- rc = PTR_ERR(lpg->pbs_dev);
- if (rc != -EPROBE_DEFER)
- dev_err(lpg->dev, "Failed to get PBS client device, rc=%d\n", rc);
- return rc;
+ if (lpg->nvmem_count == 1) {
+ /* get PBS device node if single NVMEM device */
+ lpg->pbs_dev = get_pbs_client_device(lpg->dev);
+ if (IS_ERR(lpg->pbs_dev)) {
+ rc = PTR_ERR(lpg->pbs_dev);
+ if (rc != -EPROBE_DEFER)
+ dev_err(lpg->dev, "Failed to get PBS client device, rc=%d\n", rc);
+ return rc;
+ }
+ } else if (lpg->nvmem_count == 2) {
+ /* get the 2nd nvmem device for LUT pattern */
+ lpg->lut_nvmem = devm_nvmem_device_get(lpg->dev, "lut_sdam");
+ if (IS_ERR(lpg->lut_nvmem)) {
+ rc = PTR_ERR(lpg->lut_nvmem);
+ goto err;
+ }
}
lpg->ppg_en = true;
return rc;
+err:
+ if (rc != -EPROBE_DEFER)
+ dev_err(lpg->dev, "Failed to get nvmem device, rc=%d\n", rc);
+ return rc;
}
static int lpg_init_sdam(struct lpg *lpg)
--
2.40.0
Update the pmi632 lpg_data struct so that pmi632 devices use PPG
for LUT pattern.
Signed-off-by: Anjelique Melendez <[email protected]>
---
drivers/leds/rgb/leds-qcom-lpg.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index b60d920c67c4..ac814a509781 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -1679,11 +1679,15 @@ static const struct lpg_data pm8994_lpg_data = {
static const struct lpg_data pmi632_lpg_data = {
.triled_base = 0xd000,
+ .lut_size = 64,
+ .lut_sdam_base = 0x80,
+ .nvmem_count = 1,
+
.num_channels = 5,
.channels = (const struct lpg_channel_data[]) {
- { .base = 0xb300, .triled_mask = BIT(7) },
- { .base = 0xb400, .triled_mask = BIT(6) },
- { .base = 0xb500, .triled_mask = BIT(5) },
+ { .base = 0xb300, .triled_mask = BIT(7), .sdam_offset = 0x48 },
+ { .base = 0xb400, .triled_mask = BIT(6), .sdam_offset = 0x56 },
+ { .base = 0xb500, .triled_mask = BIT(5), .sdam_offset = 0x64 },
{ .base = 0xb600 },
{ .base = 0xb700 },
},
--
2.40.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 | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index 273cb81260e7..6260e2c9fd94 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -1826,11 +1826,15 @@ static const struct lpg_data pm8150l_lpg_data = {
static const struct lpg_data pm8350c_pwm_data = {
.triled_base = 0xef00,
+ .lut_size = 122,
+ .lut_sdam_base = 0x45,
+ .nvmem_count = 2,
+
.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.40.0
On Wed, 21 Jun 2023 11:59:46 -0700, Anjelique Melendez wrote:
> Update leds-qcom-lpg bindings to support LUT patterns through NVMEM
> devices.
>
> Signed-off-by: Anjelique Melendez <[email protected]>
> ---
> .../bindings/leds/leds-qcom-lpg.yaml | 85 +++++++++++++++++++
> 1 file changed, 85 insertions(+)
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/leds/leds-qcom-lpg.example.dtb: /example-4/led-controller: failed to match any schema with compatible: ['qcom,pmi632-lpg']
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
On 21/06/2023 20:59, Anjelique Melendez wrote:
> Update leds-qcom-lpg bindings to support LUT patterns through NVMEM
> devices.
>
> Signed-off-by: Anjelique Melendez <[email protected]>
> ---
> .../bindings/leds/leds-qcom-lpg.yaml | 85 +++++++++++++++++++
> 1 file changed, 85 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> index e6f1999cb22f..c9d53820bf83 100644
> --- a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> +++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> @@ -63,6 +63,31 @@ properties:
> - description: dtest line to attach
> - description: flags for the attachment
>
> + nvmem:
> + description: >
> + Phandle(s) of the nvmem device(s) to access the LUT stored in the SDAM module(s).
It's the first time in this binding the "LUT" appears. Drop redundant
parts like "Phandle(s) of ...." and describe what do you expect there
and why the hardware needs it.
> + This property is required only when LUT mode is supported and the LUT pattern is
> + stored in SDAM modules instead of a LUT module.
> + minItems: 1
> + maxItems: 2
> +
> + nvmem-names:
> + description: >
> + The nvmem device name(s) for the SDAM module(s) where the LUT pattern data is stored.
> + This property is required only when LUT mode is supported with SDAM module instead of
> + LUT module.
> + minItems: 1
> + items:
> + - const: lpg_chan_sdam
> + - const: lut_sdam
> +
> + qcom,pbs-client:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description: >
> + Phandle of the PBS client used for sending the PBS trigger. This property is
You need to explain what is PBS and what this property is doing.
Phandle of PBS client? This is the PBS client! It does not make sense.
> + required when LUT mode is supported and the LUT pattern is stored in a single
> + SDAM module instead of a LUT module.
Which devices support LUT? Why this is not constrained per variant?
> +
> multi-led:
> type: object
> $ref: leds-class-multicolor.yaml#
> @@ -191,4 +216,64 @@ examples:
> compatible = "qcom,pm8916-pwm";
> #pwm-cells = <2>;
> };
> + - |
> + #include <dt-bindings/leds/common.h>
> +
> + led-controller {
> + compatible = "qcom,pm8350c-pwm";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + #pwm-cells = <2>;
> + nvmem-names = "lpg_chan_sdam" , "lut_sdam";
Fix your whitespaces.
> + nvmem = <&pmk8550_sdam_21 &pmk8550_sdam_22>;
Two entries, not one.
Anyway, adding one property does not justify new example. Integrate it
into existing one.
> +
> + led@1 {
> + reg = <1>;
> + color = <LED_COLOR_ID_RED>;
> + label = "red";
> + };
> +
> + led@2 {
> + reg = <2>;
> + color = <LED_COLOR_ID_GREEN>;
> + label = "green";
> + };
> +
> + led@3 {
> + reg = <3>;
> + color = <LED_COLOR_ID_BLUE>;
> + label = "blue";
> + };
> + };
> + - |
> + #include <dt-bindings/leds/common.h>
> +
> + led-controller {
> + compatible = "qcom,pmi632-lpg";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + #pwm-cells = <2>;
> + nvmem-names = "lpg_chan_sdam";
> + nvmem = <&pmi632_sdam7>;
> + qcom,pbs-client = <&pmi632_pbs_client3>;
One more example? Why?
Why do you have here only one NVMEM cell? Aren't you missing constraints
in the binding?
Best regards,
Krzysztof
Hi Anjelique,
On Wed Jun 21, 2023 at 8:59 PM CEST, Anjelique Melendez wrote:
> In certain PMICs, LUT pattern and LPG configuration can be stored in SDAM
> modules instead of LUT peripheral. This feature is called PPG.
>
> This change series adds support for PPG. Thanks!
Thanks for sending this series!
I've tested this on SDM632 Fairphone 3 and everything appears to work
fine with setting the pattern. Using fbcli from feedbackd[0] the pattern
shows up correctly.
The only thing missing really is the addition of the pbs node and adding
the nvmem/nvmem-names & qcom,pbs-client to the lpg node in pmi632.dtsi -
something like the patch below.
Are you planning to include this in the next revision? Otherwise I can
also send a patch for the pmi632.dtsi after this series has landed.
Tested-by: Luca Weiss <[email protected]> # sdm632-fairphone-fp3 (pmi632)
[0] https://source.puri.sm/Librem5/feedbackd
Regards
Luca
diff --git a/arch/arm64/boot/dts/qcom/pmi632.dtsi b/arch/arm64/boot/dts/qcom/pmi632.dtsi
index add206dee01d2e..92ddb7ac6bf311 100644
--- a/arch/arm64/boot/dts/qcom/pmi632.dtsi
+++ b/arch/arm64/boot/dts/qcom/pmi632.dtsi
@@ -127,6 +127,11 @@
status = "disabled";
};
+ pmi632_pbs_client3: qcom,pbs@7400 { // TODO: generic node name
+ compatible = "qcom,pbs";
+ reg = <0x7400>;
+ };
+
pmi632_sdam_7: nvram@b600 {
compatible = "qcom,spmi-sdam";
reg = <0xb600>;
@@ -155,6 +160,10 @@
pmi632_lpg: pwm {
compatible = "qcom,pmi632-lpg";
+ nvmem = <&pmi632_sdam_7>;
+ nvmem-names = "lpg_chan_sdam";
+ qcom,pbs-client = <&pmi632_pbs_client3>;
+
#address-cells = <1>;
#size-cells = <0>;
#pwm-cells = <2>;
>
> Anjelique Melendez (7):
> dt-bindings: soc: qcom: Add qcom-pbs bindings
> dt-bindings: leds: leds-qcom-lpg: Add support for LUT through NVMEM
> devices
> soc: qcom: add QCOM PBS driver
> leds: rgb: leds-qcom-lpg: Add support for LUT pattern through single
> SDAM
> leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG
> leds: rgb: leds-qcom-lpg: Support two-nvmem PPG Scheme
> leds: rgb: Update PM8350C lpg_data to support two-nvmem PPG Scheme
>
> .../bindings/leds/leds-qcom-lpg.yaml | 85 ++++
> .../bindings/soc/qcom/qcom-pbs.yaml | 41 ++
> drivers/leds/rgb/leds-qcom-lpg.c | 393 ++++++++++++++++--
> drivers/soc/qcom/Kconfig | 9 +
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/qcom-pbs.c | 343 +++++++++++++++
> include/linux/soc/qcom/qcom-pbs.h | 36 ++
> 7 files changed, 877 insertions(+), 31 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 6/24/2023 2:42 AM, Krzysztof Kozlowski wrote:
> On 21/06/2023 20:59, Anjelique Melendez wrote:
>> Update leds-qcom-lpg bindings to support LUT patterns through NVMEM
>> devices.
>>
>> Signed-off-by: Anjelique Melendez <[email protected]>
>> ---
>> .../bindings/leds/leds-qcom-lpg.yaml | 85 +++++++++++++++++++
>> 1 file changed, 85 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
>> index e6f1999cb22f..c9d53820bf83 100644
>> --- a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
>> +++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
>> @@ -63,6 +63,31 @@ properties:
>> - description: dtest line to attach
>> - description: flags for the attachment
>>
>> + nvmem:
>> + description: >
>> + Phandle(s) of the nvmem device(s) to access the LUT stored in the SDAM module(s).
>
> It's the first time in this binding the "LUT" appears. Drop redundant
> parts like "Phandle(s) of ...." and describe what do you expect there
> and why the hardware needs it.
LUT is a "lookup table"(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml?h=v6.4#n14)
and in this case holds pattern data i.e LUT patterns.
Currently, qcom,pm660l-lpg, qcom,pm8150b-lpg, qcom,pm8150l-lpg, qcom,pm8941-lpg, qcom,pm8994-lpg,
qcom,pmc8180c-lpg, qcom,pmi8994-lpg, and qcom,pmi8998-lpg all have an a specific LUT peripheral
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/rgb/leds-qcom-lpg.c?h=v6.4-rc7#n1382)
and this is already being handled in leds-qcom-lpg.
What is new with this patch set is that instead of having a LUT peripheral, some PMICs use nvmem to
hold LUT pattern and other lpg per channel data.
The use of nvmems to store LUT pattern and lpg data is called PPG.
You can either have a single nvmem PPG (a single nvmem device that holds LUT pattern
and lpg per channel data) or two-nvmem PPG(1 nvmem for LUT pattern and 1 nvmem for lpg per channel data)
I can update the descritpion for the entire binding to mention PPG and LUT so this is more clear.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml?h=v6.4#n12
>> + This property is required only when LUT mode is supported and the LUT pattern is
>> + stored in SDAM modules instead of a LUT module.
>> + minItems: 1
>> + maxItems: 2
>> +
>> + nvmem-names:
>> + description: >
>> + The nvmem device name(s) for the SDAM module(s) where the LUT pattern data is stored.
>> + This property is required only when LUT mode is supported with SDAM module instead of
>> + LUT module.
>> + minItems: 1
>> + items:
>> + - const: lpg_chan_sdam
>> + - const: lut_sdam
>> +
>> + qcom,pbs-client:
>> + $ref: /schemas/types.yaml#/definitions/phandle
>> + description: >
>> + Phandle of the PBS client used for sending the PBS trigger. This property is
>
>
> You need to explain what is PBS and what this property is doing.
>
> Phandle of PBS client? This is the PBS client! It does not make sense.
Ack
>
>
>
>> + required when LUT mode is supported and the LUT pattern is stored in a single
>> + SDAM module instead of a LUT module.
>
> Which devices support LUT? Why this is not constrained per variant?
When you say constrained per variant, are you looking for something more like this?
i.e.
allOf:
- if:
properties:
compatible:
contains:
const: qcom,pmi632-lpg
then:
properties:
nvmem:
maxItems: 1
nvmem-names:
items:
- const: lpg_chan_sdam
required:
- nvmem
- qcom,pbs-client
- if:
properties:
compatible:
contains:
const: qcom,pm8350c-pwm
then:
properties:
nvmem:
maxItems: 2
nvmem-names:
items:
- const: lpg_chan_sdam
- const: lut_sdam
required:
- nvmem
>
>> +
>> multi-led:
>> type: object
>> $ref: leds-class-multicolor.yaml#
>> @@ -191,4 +216,64 @@ examples:
>> compatible = "qcom,pm8916-pwm";
>> #pwm-cells = <2>;
>> };
>> + - |
>> + #include <dt-bindings/leds/common.h>
>> +
>> + led-controller {
>> + compatible = "qcom,pm8350c-pwm";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + #pwm-cells = <2>;
>> + nvmem-names = "lpg_chan_sdam" , "lut_sdam";
>
> Fix your whitespaces.
Ack
>
>> + nvmem = <&pmk8550_sdam_21 &pmk8550_sdam_22>;
>
> Two entries, not one>
> Anyway, adding one property does not justify new example. Integrate it
> into existing one.
So we actually cannot integrate these properties into existing examples.
The current examples are for PMICs that use LUT peripherals (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/rgb/leds-qcom-lpg.c?h=v6.4#n1417).
This patch series is adding support for PMICs that do not have a LUT peripheral
and instead store LUT patterns and LPG configurations in either 1 or 2 NVMEM(s).
>
>> +
>> + led@1 {
>> + reg = <1>;
>> + color = <LED_COLOR_ID_RED>;
>> + label = "red";
>> + };
>> +
>> + led@2 {
>> + reg = <2>;
>> + color = <LED_COLOR_ID_GREEN>;
>> + label = "green";
>> + };
>> +
>> + led@3 {
>> + reg = <3>;
>> + color = <LED_COLOR_ID_BLUE>;
>> + label = "blue";
>> + };
>> + };
>> + - |
>> + #include <dt-bindings/leds/common.h>
>> +
>> + led-controller {
>> + compatible = "qcom,pmi632-lpg";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + #pwm-cells = <2>;
>> + nvmem-names = "lpg_chan_sdam";
>> + nvmem = <&pmi632_sdam7>;
>> + qcom,pbs-client = <&pmi632_pbs_client3>;
>
> One more example? Why?
>
> Why do you have here only one NVMEM cell? Aren't you missing constraints
> in the binding?The use of the qcom,pbs-client is only used when we have a PMIC device that has a single PPG NVMEM,
which is why this was not included in the above 2 nvmem PPG example. I see how these two PPG examples
are repetitive so I am ok with getting rid of one of them but I do think we should have at least one PPG example.
>
> Best regards,
> Krzysztof
>
Thanks,
Anjelique
On 29/06/2023 02:12, Anjelique Melendez wrote:
>>
>>
>>
>>> + required when LUT mode is supported and the LUT pattern is stored in a single
>>> + SDAM module instead of a LUT module.
>>
>> Which devices support LUT? Why this is not constrained per variant?
> When you say constrained per variant, are you looking for something more like this?
> i.e.
> allOf:
> - if:
> properties:
> compatible:
> contains:
> const: qcom,pmi632-lpg
> then:
> properties:
> nvmem:
> maxItems: 1
> nvmem-names:
> items:
> - const: lpg_chan_sdam
> required:
> - nvmem
> - qcom,pbs-client
> - if:
> properties:
> compatible:
> contains:
> const: qcom,pm8350c-pwm
> then:
> properties:
> nvmem:
> maxItems: 2
> nvmem-names:
> items:
> - const: lpg_chan_sdam
> - const: lut_sdam
> required:
> - nvmem
Yes.
>
>>
>>> +
>>> multi-led:
>>> type: object
>>> $ref: leds-class-multicolor.yaml#
>>> @@ -191,4 +216,64 @@ examples:
>>> compatible = "qcom,pm8916-pwm";
>>> #pwm-cells = <2>;
>>> };
>>> + - |
>>> + #include <dt-bindings/leds/common.h>
>>> +
>>> + led-controller {
>>> + compatible = "qcom,pm8350c-pwm";
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> + #pwm-cells = <2>;
>>> + nvmem-names = "lpg_chan_sdam" , "lut_sdam";
>>
>> Fix your whitespaces.
> Ack
>>
>>> + nvmem = <&pmk8550_sdam_21 &pmk8550_sdam_22>;
>>
>> Two entries, not one>
>> Anyway, adding one property does not justify new example. Integrate it
>> into existing one.
>
> So we actually cannot integrate these properties into existing examples.
> The current examples are for PMICs that use LUT peripherals (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/rgb/leds-qcom-lpg.c?h=v6.4#n1417).
> This patch series is adding support for PMICs that do not have a LUT peripheral
> and instead store LUT patterns and LPG configurations in either 1 or 2 NVMEM(s).
>>
>>> +
>>> + led@1 {
>>> + reg = <1>;
>>> + color = <LED_COLOR_ID_RED>;
>>> + label = "red";
>>> + };
>>> +
>>> + led@2 {
>>> + reg = <2>;
>>> + color = <LED_COLOR_ID_GREEN>;
>>> + label = "green";
>>> + };
>>> +
>>> + led@3 {
>>> + reg = <3>;
>>> + color = <LED_COLOR_ID_BLUE>;
>>> + label = "blue";
>>> + };
>>> + };
>>> + - |
>>> + #include <dt-bindings/leds/common.h>
>>> +
>>> + led-controller {
>>> + compatible = "qcom,pmi632-lpg";
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> + #pwm-cells = <2>;
>>> + nvmem-names = "lpg_chan_sdam";
>>> + nvmem = <&pmi632_sdam7>;
>>> + qcom,pbs-client = <&pmi632_pbs_client3>;
>>
>> One more example? Why?
>>
>> Why do you have here only one NVMEM cell? Aren't you missing constraints
>> in the binding?The use of the qcom,pbs-client is only used when we have a PMIC device that has a single PPG NVMEM,
> which is why this was not included in the above 2 nvmem PPG example. I see how these two PPG examples
> are repetitive so I am ok with getting rid of one of them but I do think we should have at least one PPG example.
This example probably should replace one of the previous ones, because
it is bigger / more complete.
Best regards,
Krzysztof
On 6/26/2023 1:28 AM, Luca Weiss wrote:
> Hi Anjelique,
>
> On Wed Jun 21, 2023 at 8:59 PM CEST, Anjelique Melendez wrote:
>> In certain PMICs, LUT pattern and LPG configuration can be stored in SDAM
>> modules instead of LUT peripheral. This feature is called PPG.
>>
>> This change series adds support for PPG. Thanks!
>
> Thanks for sending this series!
>
> I've tested this on SDM632 Fairphone 3 and everything appears to work
> fine with setting the pattern. Using fbcli from feedbackd[0] the pattern
> shows up correctly.
>
> The only thing missing really is the addition of the pbs node and adding
> the nvmem/nvmem-names & qcom,pbs-client to the lpg node in pmi632.dtsi -
> something like the patch below.
>
> Are you planning to include this in the next revision? Otherwise I can
> also send a patch for the pmi632.dtsi after this series has landed.
>
> Tested-by: Luca Weiss <[email protected]> # sdm632-fairphone-fp3 (pmi632)
>
> [0] https://source.puri.sm/Librem5/feedbackd
>
> Regards
> Luca
>
Hi Luca,
So sorry for the late response, I missed this message! Thank you for testing!
I was not planning to include adding the pbs node or updating lpg node in pmi632.dtsi.
If you are able to send a patch for pmi632.dtsi after that would be great!
Thanks,
Anjelique