[PATCH v4 0/7] Add support for LUT PPG

[PATCH v4 0/7] Add support for LUT PPG

[Anjelique Melendez]

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!

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 <luca.weiss@fairphone.com> # 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 PPG through single SDAM
  leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG
  leds: rgb: leds-qcom-lpg: Include support for dedicated LUT SDAM PPG
    Scheme
  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              | 370 ++++++++++++++++--
 drivers/soc/qcom/Kconfig                      |   9 +
 drivers/soc/qcom/Makefile                     |   1 +
 drivers/soc/qcom/qcom-pbs.c                   | 286 ++++++++++++++
 include/linux/soc/qcom/qcom-pbs.h             |  30 ++
 7 files changed, 801 insertions(+), 30 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

[PATCH v4 1/7] dt-bindings: soc: qcom: Add qcom,pbs bindings

[Anjelique Melendez]

Add binding for the Qualcomm Programmable Boot Sequencer device.

Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/soc/qcom/qcom,pbs.yaml           | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,pbs.yaml

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,pbs.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,pbs.yaml
new file mode 100644
index 000000000000..b502ca72266a
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,pbs.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/qcom/qcom,pbs.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies, Inc. Programmable Boot Sequencer
+
+maintainers:
+  - Anjelique Melendez <quic_amelende@quicinc.com>
+
+description: |
+  The Qualcomm Technologies, Inc. Programmable Boot Sequencer (PBS)
+  supports triggering power up and power down sequences for clients
+  upon request.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - qcom,pmi632-pbs
+      - const: qcom,pbs
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/spmi/spmi.h>
+
+    pmic@0 {
+      reg = <0x0 SPMI_USID>;
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      pbs@7400 {
+        compatible = "qcom,pmi632-pbs", "qcom,pbs";
+        reg = <0x7400>;
+      };
+    };
-- 
2.41.0

[PATCH v4 2/7] dt-bindings: leds: leds-qcom-lpg: Add support for LPG PPG

[Anjelique Melendez]

Update leds-qcom-lpg binding to support LPG PPG.

Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/leds/leds-qcom-lpg.yaml          | 89 ++++++++++++++++++-
 1 file changed, 88 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
index e6f1999cb22f..067ebe35ca5e 100644
--- a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
@@ -11,7 +11,7 @@ maintainers:
 
 description: >
   The Qualcomm Light Pulse Generator consists of three different hardware blocks;
-  a ramp generator with lookup table, the light pulse generator and a three
+  a ramp generator with lookup table (LUT), the light pulse generator and a three
   channel current sink. These blocks are found in a wide range of Qualcomm PMICs.
 
 properties:
@@ -63,6 +63,29 @@ properties:
         - description: dtest line to attach
         - description: flags for the attachment
 
+  nvmem:
+    description: >
+      This property is required for PMICs that supports PPG, which is when a
+      PMIC stores LPG per-channel data and pattern LUT in SDAM modules instead
+      of in a LUT peripheral. For PMICs, such as PM8350C, per-channel data
+      and pattern LUT is separated into 2 SDAM modules. In that case, phandles
+      to both SDAM modules need to be specified.
+    minItems: 1
+    maxItems: 2
+
+  nvmem-names:
+    minItems: 1
+    items:
+      - const: lpg_chan_sdam
+      - const: lut_sdam
+
+  qcom,pbs:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: >
+      Phandle of the Qualcomm Programmable Boot Sequencer node (PBS).
+      PBS node is used to trigger LPG pattern sequences for PMICs that support
+      single SDAM PPG.
+
   multi-led:
     type: object
     $ref: leds-class-multicolor.yaml#
@@ -106,6 +129,39 @@ required:
 
 additionalProperties: false
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: qcom,pmi632-lpg
+    then:
+      properties:
+        nvmem:
+          maxItems: 1
+        nvmem-names:
+          maxItems: 1
+      required:
+        - nvmem
+        - nvmem-names
+        - qcom,pbs
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,pm8350c-pwm
+              - qcom,pm8550-pwm
+    then:
+      properties:
+        nvmem:
+          minItems: 2
+        nvmem-names:
+          minItems: 2
+      required:
+        - nvmem
+        - nvmem-names
+
 examples:
   - |
     #include <dt-bindings/leds/common.h>
@@ -191,4 +247,35 @@ examples:
       compatible = "qcom,pm8916-pwm";
       #pwm-cells = <2>;
     };
+  - |
+    #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_sdam_7>;
+      qcom,pbs = <&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.41.0

[PATCH v4 3/7] soc: qcom: add QCOM PBS driver

[Anjelique Melendez]

Add the Qualcomm PBS (Programmable Boot Sequencer) driver. The QCOM PBS
driver supports configuring software PBS trigger events through PBS RAM
on Qualcomm Technologies, Inc (QTI) PMICs.

Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
---
 drivers/soc/qcom/Kconfig          |   9 +
 drivers/soc/qcom/Makefile         |   1 +
 drivers/soc/qcom/qcom-pbs.c       | 286 ++++++++++++++++++++++++++++++
 include/linux/soc/qcom/qcom-pbs.h |  30 ++++
 4 files changed, 326 insertions(+)
 create mode 100644 drivers/soc/qcom/qcom-pbs.c
 create mode 100644 include/linux/soc/qcom/qcom-pbs.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index e597799e8121..8cf690e46bf7 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -271,6 +271,15 @@ config QCOM_APR
 	  used by audio driver to configure QDSP6
 	  ASM, ADM and AFE modules.
 
+config QCOM_PBS
+	tristate "PBS trigger support for Qualcomm PMIC"
+	depends on SPMI
+	help
+	  This driver supports configuring software programmable boot sequencer (PBS)
+	  trigger event through PBS RAM on Qualcomm Technologies, Inc. PMICs.
+	  This module provides the APIs to the client drivers that wants to send the
+	  PBS trigger event to the PBS RAM.
+
 config QCOM_ICC_BWMON
 	tristate "QCOM Interconnect Bandwidth Monitor driver"
 	depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 99114c71092b..3ffb04e2a275 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
 obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
 obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
 obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) +=	kryo-l2-accessors.o
+obj-$(CONFIG_QCOM_PBS) +=	qcom-pbs.o
 obj-$(CONFIG_QCOM_ICC_BWMON)	+= icc-bwmon.o
 qcom_ice-objs			+= ice.o
 obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE)	+= qcom_ice.o
diff --git a/drivers/soc/qcom/qcom-pbs.c b/drivers/soc/qcom/qcom-pbs.c
new file mode 100644
index 000000000000..825ff6de7266
--- /dev/null
+++ b/drivers/soc/qcom/qcom-pbs.c
@@ -0,0 +1,286 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/regmap.h>
+#include <linux/spmi.h>
+#include <linux/soc/qcom/qcom-pbs.h>
+
+#define PBS_CLIENT_TRIG_CTL		0x42
+#define PBS_CLIENT_SW_TRIG_BIT		BIT(7)
+#define PBS_CLIENT_SCRATCH1		0x50
+#define PBS_CLIENT_SCRATCH2		0x51
+#define PBS_CLIENT_SCRATCH2_ERROR	0xFF
+
+struct pbs_dev {
+	struct device		*dev;
+	struct regmap		*regmap;
+	struct mutex		lock;
+	struct device_link	*link;
+
+	u32			base;
+};
+
+static int qcom_pbs_read(struct pbs_dev *pbs, u32 address, u8 *val)
+{
+	int ret;
+
+	address += pbs->base;
+	ret = regmap_bulk_read(pbs->regmap, address, val, 1);
+	if (ret)
+		dev_err(pbs->dev, "Failed to read address=%#x sid=%#x ret=%d\n",
+			address, to_spmi_device(pbs->dev->parent)->usid, ret);
+
+	return ret;
+}
+
+static int qcom_pbs_write(struct pbs_dev *pbs, u16 address, u8 val)
+{
+	int ret;
+
+	address += pbs->base;
+	ret = regmap_bulk_write(pbs->regmap, address, &val, 1);
+	if (ret < 0)
+		dev_err(pbs->dev, "Failed to write address=%#x sid=%#x ret=%d\n",
+			  address, to_spmi_device(pbs->dev->parent)->usid, ret);
+
+	return ret;
+}
+
+static int qcom_pbs_masked_write(struct pbs_dev *pbs, u16 address, u8 mask, u8 val)
+{
+	int ret;
+
+	address += pbs->base;
+	ret = regmap_update_bits(pbs->regmap, address, mask, val);
+	if (ret < 0)
+		dev_err(pbs->dev, "Failed to write address=%#x ret=%d\n", address, ret);
+
+	return ret;
+}
+
+static int qcom_pbs_wait_for_ack(struct pbs_dev *pbs, u8 bit_pos)
+{
+	int ret, retries = 2000, delay = 1000;
+	u8 val;
+
+	while (retries--) {
+		ret = qcom_pbs_read(pbs, PBS_CLIENT_SCRATCH2, &val);
+		if (ret < 0)
+			return ret;
+
+		if (val == PBS_CLIENT_SCRATCH2_ERROR) {
+			/* PBS error - clear SCRATCH2 register */
+			ret = qcom_pbs_write(pbs, PBS_CLIENT_SCRATCH2, 0);
+			if (ret < 0)
+				return ret;
+
+			dev_err(pbs->dev, "NACK from PBS for bit %u\n", bit_pos);
+			return -EINVAL;
+		}
+
+		if (val & BIT(bit_pos)) {
+			dev_dbg(pbs->dev, "PBS sequence for bit %u executed!\n", bit_pos);
+			return 0;
+		}
+
+		usleep_range(delay, delay + 100);
+	}
+
+	dev_err(pbs->dev, "Timeout for PBS ACK/NACK for bit %u\n", bit_pos);
+	return -ETIMEDOUT;
+}
+
+/**
+ * qcom_pbs_trigger_event() - Trigger the PBS RAM sequence
+ * @pbs: Pointer to PBS device
+ * @bitmap: bitmap
+ *
+ * This function is used to trigger the PBS RAM sequence to be
+ * executed by the client driver.
+ *
+ * The PBS trigger sequence involves
+ * 1. setting the PBS sequence bit in PBS_CLIENT_SCRATCH1
+ * 2. Initiating the SW PBS trigger
+ * 3. Checking the equivalent bit in PBS_CLIENT_SCRATCH2 for the
+ *    completion of the sequence.
+ * 4. If PBS_CLIENT_SCRATCH2 == 0xFF, the PBS sequence failed to execute
+ *
+ * Returns: 0 on success, < 0 on failure
+ */
+int qcom_pbs_trigger_event(struct pbs_dev *pbs, u8 bitmap)
+{
+	u8 val;
+	u16 bit_pos;
+	int ret;
+
+	if (!bitmap) {
+		dev_err(pbs->dev, "Invalid bitmap passed by client\n");
+		return -EINVAL;
+	}
+
+	if (IS_ERR_OR_NULL(pbs))
+		return -EINVAL;
+
+	mutex_lock(&pbs->lock);
+	ret = qcom_pbs_read(pbs, PBS_CLIENT_SCRATCH2, &val);
+	if (ret < 0)
+		goto out;
+
+	if (val == PBS_CLIENT_SCRATCH2_ERROR) {
+		/* PBS error - clear SCRATCH2 register */
+		ret = qcom_pbs_write(pbs, PBS_CLIENT_SCRATCH2, 0);
+		if (ret < 0)
+			goto out;
+	}
+
+	for (bit_pos = 0; bit_pos < 8; bit_pos++) {
+		if (bitmap & BIT(bit_pos)) {
+			/* Clear the PBS sequence bit position */
+			ret = qcom_pbs_masked_write(pbs, PBS_CLIENT_SCRATCH2, BIT(bit_pos), 0);
+			if (ret < 0)
+				goto error;
+
+			/* Set the PBS sequence bit position */
+			ret = qcom_pbs_masked_write(pbs, PBS_CLIENT_SCRATCH1,
+						BIT(bit_pos), BIT(bit_pos));
+			if (ret < 0)
+				goto error;
+
+			/* Initiate the SW trigger */
+			ret = qcom_pbs_masked_write(pbs, PBS_CLIENT_TRIG_CTL,
+						PBS_CLIENT_SW_TRIG_BIT, PBS_CLIENT_SW_TRIG_BIT);
+			if (ret < 0)
+				goto error;
+
+			ret = qcom_pbs_wait_for_ack(pbs, bit_pos);
+			if (ret < 0)
+				goto error;
+
+			/* Clear the PBS sequence bit position */
+			ret = qcom_pbs_masked_write(pbs, PBS_CLIENT_SCRATCH1, BIT(bit_pos), 0);
+			if (ret < 0)
+				goto error;
+
+			/* Clear the PBS sequence bit position */
+			ret = qcom_pbs_masked_write(pbs, PBS_CLIENT_SCRATCH2, BIT(bit_pos), 0);
+			if (ret < 0)
+				goto error;
+		}
+	}
+
+error:
+	/* Clear all the requested bitmap */
+	ret = qcom_pbs_masked_write(pbs, PBS_CLIENT_SCRATCH1, bitmap, 0);
+
+out:
+	mutex_unlock(&pbs->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_pbs_trigger_event);
+
+/**
+ * get_pbs_client_device() - Get the PBS device used by client
+ * @dev: Client device
+ *
+ * This function is used to get the PBS device that is being
+ * used by the client.
+ *
+ * Returns: pbs_dev on success, ERR_PTR on failure
+ */
+struct pbs_dev *get_pbs_client_device(struct device *dev)
+{
+	struct device_node *pbs_dev_node;
+	struct platform_device *pdev;
+	struct pbs_dev *pbs;
+
+	pbs_dev_node = of_parse_phandle(dev->of_node, "qcom,pbs", 0);
+	if (!pbs_dev_node) {
+		dev_err(dev, "Missing qcom,pbs property\n");
+		return ERR_PTR(-ENODEV);
+	}
+
+	pdev = of_find_device_by_node(pbs_dev_node);
+	if (!pdev) {
+		dev_err(dev, "Unable to find PBS dev_node\n");
+		pbs = ERR_PTR(-EPROBE_DEFER);
+		goto out;
+	}
+
+	pbs = platform_get_drvdata(pdev);
+	if (!pbs) {
+		dev_err(dev, "Cannot get pbs instance from %s\n", dev_name(&pdev->dev));
+		platform_device_put(pdev);
+		pbs = ERR_PTR(-EPROBE_DEFER);
+		goto out;
+	}
+
+	pbs->link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_SUPPLIER);
+	if (!pbs->link) {
+		dev_err(&pdev->dev, "Failed to create device link to consumer %s\n", dev_name(dev));
+		platform_device_put(pdev);
+		pbs = ERR_PTR(-EINVAL);
+		goto out;
+	}
+
+out:
+	of_node_put(pbs_dev_node);
+	return pbs;
+}
+EXPORT_SYMBOL_GPL(get_pbs_client_device);
+
+static int qcom_pbs_probe(struct platform_device *pdev)
+{
+	struct pbs_dev *pbs;
+	u32 val;
+	int ret;
+
+	pbs = devm_kzalloc(&pdev->dev, sizeof(*pbs), GFP_KERNEL);
+	if (!pbs)
+		return -ENOMEM;
+
+	pbs->dev = &pdev->dev;
+	pbs->regmap = dev_get_regmap(pbs->dev->parent, NULL);
+	if (!pbs->regmap) {
+		dev_err(pbs->dev, "Couldn't get parent's regmap\n");
+		return -EINVAL;
+	}
+
+	ret = device_property_read_u32(pbs->dev, "reg", &val);
+	if (ret < 0) {
+		dev_err(pbs->dev, "Couldn't find reg, ret = %d\n", ret);
+		return ret;
+	}
+	pbs->base = val;
+	mutex_init(&pbs->lock);
+
+	platform_set_drvdata(pdev, pbs);
+
+	return 0;
+}
+
+static const struct of_device_id qcom_pbs_match_table[] = {
+	{ .compatible = "qcom,pbs" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, qcom_pbs_match_table);
+
+static struct platform_driver qcom_pbs_driver = {
+	.driver = {
+		.name		= "qcom-pbs",
+		.of_match_table	= qcom_pbs_match_table,
+	},
+	.probe = qcom_pbs_probe,
+};
+module_platform_driver(qcom_pbs_driver)
+
+MODULE_DESCRIPTION("QCOM PBS DRIVER");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/soc/qcom/qcom-pbs.h b/include/linux/soc/qcom/qcom-pbs.h
new file mode 100644
index 000000000000..8a46209ccf13
--- /dev/null
+++ b/include/linux/soc/qcom/qcom-pbs.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef _QCOM_PBS_H
+#define _QCOM_PBS_H
+
+#include <linux/errno.h>
+#include <linux/types.h>
+
+struct device_node;
+struct pbs_dev;
+
+#if IS_ENABLED(CONFIG_QCOM_PBS)
+int qcom_pbs_trigger_event(struct pbs_dev *pbs, u8 bitmap);
+struct pbs_dev *get_pbs_client_device(struct device *client_dev);
+#else
+static inline int qcom_pbs_trigger_event(struct pbs_dev *pbs, u8 bitmap)
+{
+	return -ENODEV;
+}
+
+static inline struct pbs_dev *get_pbs_client_device(struct device *client_dev)
+{
+	return ERR_PTR(-ENODEV);
+}
+#endif
+
+#endif
-- 
2.41.0

[PATCH v4 4/7] leds: rgb: leds-qcom-lpg: Add support for PPG through single SDAM

[Anjelique Melendez]

In some PMICs like pmi632, the pattern look up table (LUT) and LPG
configuration can be stored in a single SDAM module instead of LUT
peripheral. This feature is called PPG. PPG uses Qualcomm Programmable
Boot Sequencer (PBS) inorder to trigger pattern sequences for PMICs.

Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
---
 drivers/leds/rgb/leds-qcom-lpg.c | 298 ++++++++++++++++++++++++++++---
 1 file changed, 274 insertions(+), 24 deletions(-)

diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index 59581b3e25ca..90dc27d5eb7c 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -8,12 +8,14 @@
 #include <linux/bitfield.h>
 #include <linux/led-class-multicolor.h>
 #include <linux/module.h>
+#include <linux/nvmem-consumer.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
+#include <linux/soc/qcom/qcom-pbs.h>
 
 #define LPG_SUBTYPE_REG		0x05
 #define  LPG_SUBTYPE_LPG	0x2
@@ -40,6 +42,8 @@
 #define PWM_SEC_ACCESS_REG	0xd0
 #define PWM_DTEST_REG(x)	(0xe2 + (x) - 1)
 
+#define SDAM_REG_PBS_SEQ_EN		0x42
+
 #define TRI_LED_SRC_SEL		0x45
 #define TRI_LED_EN_CTL		0x46
 #define TRI_LED_ATC_CTL		0x47
@@ -49,9 +53,25 @@
 
 #define LPG_RESOLUTION_9BIT	BIT(9)
 #define LPG_RESOLUTION_15BIT	BIT(15)
+#define PPG_MAX_LED_BRIGHTNESS	255
+
 #define LPG_MAX_M		7
 #define LPG_MAX_PREDIV		6
 
+#define DEFAULT_TICK_DURATION_US	7800
+#define RAMP_STEP_DURATION(x)		(((x) * 1000 / DEFAULT_TICK_DURATION_US) & 0xff)
+
+/* LPG common config settings for PPG */
+#define SDAM_REG_RAMP_STEP_DURATION		0x47
+#define SDAM_LUT_COUNT_MAX			64
+
+/* LPG per channel config settings for PPG */
+#define SDAM_LUT_EN_OFFSET			0x0
+#define SDAM_PATTERN_CONFIG_OFFSET		0x1
+#define SDAM_END_INDEX_OFFSET			0x3
+#define SDAM_START_INDEX_OFFSET		0x4
+#define SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET	0x6
+
 struct lpg_channel;
 struct lpg_data;
 
@@ -65,7 +85,11 @@ struct lpg_data;
  * @lut_base:	base address of the LUT block (optional)
  * @lut_size:	number of entries in the LUT block
  * @lut_bitmap:	allocation bitmap for LUT entries
- * @triled_base: base address of the TRILED block (optional)
+ * @pbs_dev:	PBS device
+ * @lpg_chan_nvmem:	LPG nvmem peripheral device
+ * @pbs_en_bitmap:	bitmap for tracking PBS triggers
+ * @lut_sdam_base:	offset where LUT pattern begins in nvmem
+ * @triled_base:	base address of the TRILED block (optional)
  * @triled_src:	power-source for the TRILED
  * @triled_has_atc_ctl:	true if there is TRI_LED_ATC_CTL register
  * @triled_has_src_sel:	true if there is TRI_LED_SRC_SEL register
@@ -86,6 +110,11 @@ struct lpg {
 	u32 lut_size;
 	unsigned long *lut_bitmap;
 
+	struct pbs_dev *pbs_dev;
+	struct nvmem_device *lpg_chan_nvmem;
+	unsigned long pbs_en_bitmap;
+	u32 lut_sdam_base;
+
 	u32 triled_base;
 	u32 triled_src;
 	bool triled_has_atc_ctl;
@@ -102,6 +131,8 @@ struct lpg {
  * @triled_mask: mask in TRILED to enable this channel
  * @lut_mask:	mask in LUT to start pattern generator for this channel
  * @subtype:	PMIC hardware block subtype
+ * @sdam_offset:	Channel offset in LPG nvmem
+ * @lpg_idx:	index of the channel
  * @in_use:	channel is exposed to LED framework
  * @color:	color of the LED attached to this channel
  * @dtest_line:	DTEST line for output, or 0 if disabled
@@ -113,6 +144,7 @@ struct lpg {
  * @pre_div_sel: divider selector of the reference clock
  * @pre_div_exp: exponential divider of the reference clock
  * @pwm_resolution_sel:	pwm resolution selector
+ * @pattern_set: true when setting pattern
  * @ramp_enabled: duty cycle is driven by iterating over lookup table
  * @ramp_ping_pong: reverse through pattern, rather than wrapping to start
  * @ramp_oneshot: perform only a single pass over the pattern
@@ -130,6 +162,8 @@ struct lpg_channel {
 	unsigned int triled_mask;
 	unsigned int lut_mask;
 	unsigned int subtype;
+	u32 sdam_offset;
+	u32 lpg_idx;
 
 	bool in_use;
 
@@ -147,6 +181,7 @@ struct lpg_channel {
 	unsigned int pre_div_exp;
 	unsigned int pwm_resolution_sel;
 
+	bool pattern_set;
 	bool ramp_enabled;
 	bool ramp_ping_pong;
 	bool ramp_oneshot;
@@ -181,10 +216,12 @@ struct lpg_led {
  * struct lpg_channel_data - per channel initialization data
  * @base:		base address for PWM channel registers
  * @triled_mask:	bitmask for controlling this channel in TRILED
+ * @sdam_offset:	Channel offset in LPG nvmem
  */
 struct lpg_channel_data {
 	unsigned int base;
 	u8 triled_mask;
+	unsigned int sdam_offset;
 };
 
 /**
@@ -192,6 +229,7 @@ struct lpg_channel_data {
  * @lut_base:		base address of LUT block
  * @lut_size:		number of entries in LUT
  * @triled_base:	base address of TRILED
+ * @lut_sdam_base:	base address where LUT pattern begins in nvmem device
  * @triled_has_atc_ctl:	true if there is TRI_LED_ATC_CTL register
  * @triled_has_src_sel:	true if there is TRI_LED_SRC_SEL register
  * @num_channels:	number of channels in LPG
@@ -201,12 +239,90 @@ struct lpg_data {
 	unsigned int lut_base;
 	unsigned int lut_size;
 	unsigned int triled_base;
+	unsigned int lut_sdam_base;
 	bool triled_has_atc_ctl;
 	bool triled_has_src_sel;
 	int num_channels;
 	const struct lpg_channel_data *channels;
 };
 
+static int lpg_sdam_write(struct lpg *lpg, u16 addr, u8 val)
+{
+	int rc;
+
+	rc = nvmem_device_write(lpg->lpg_chan_nvmem, addr, 1, &val);
+	if (rc < 0) {
+		dev_err(lpg->dev, "writing %u to SDAM addr %#x failed, rc=%d\n",
+			val, addr, rc);
+		return rc;
+	}
+
+	return 0;
+}
+
+#define PBS_SW_TRIG_BIT		BIT(0)
+
+static int lpg_clear_pbs_trigger(struct lpg_channel *chan)
+{
+	int rc;
+
+	clear_bit(chan->lpg_idx, &chan->lpg->pbs_en_bitmap);
+	if (!chan->lpg->pbs_en_bitmap) {
+		rc = lpg_sdam_write(chan->lpg, SDAM_REG_PBS_SEQ_EN, 0);
+		if (rc < 0)
+			return rc;
+	}
+
+	return 0;
+}
+
+static int lpg_set_pbs_trigger(struct lpg_channel *chan)
+{
+	int rc;
+
+	if (!chan->lpg->pbs_en_bitmap) {
+		rc = lpg_sdam_write(chan->lpg, SDAM_REG_PBS_SEQ_EN, PBS_SW_TRIG_BIT);
+		if (rc < 0)
+			return rc;
+
+		rc = qcom_pbs_trigger_event(chan->lpg->pbs_dev, PBS_SW_TRIG_BIT);
+		if (rc < 0)
+			return rc;
+	}
+	set_bit(chan->lpg_idx, &chan->lpg->pbs_en_bitmap);
+
+	return 0;
+}
+
+static int lpg_sdam_configure_triggers(struct lpg_channel *chan, bool set_trig)
+{
+	int rc;
+
+	if (chan->lpg->lut_base)
+		return 0;
+
+	if (set_trig) {
+		rc = lpg_sdam_write(chan->lpg, SDAM_LUT_EN_OFFSET + chan->sdam_offset, 1);
+		if (rc < 0)
+			return rc;
+
+		rc = lpg_set_pbs_trigger(chan);
+		if (rc < 0)
+			return rc;
+		chan->pattern_set = false;
+	} else {
+		rc = lpg_sdam_write(chan->lpg, SDAM_LUT_EN_OFFSET + chan->sdam_offset, 0);
+		if (rc < 0)
+			return rc;
+
+		rc = lpg_clear_pbs_trigger(chan);
+		if (rc < 0)
+			return rc;
+	}
+
+	return 0;
+}
+
 static int triled_set(struct lpg *lpg, unsigned int mask, unsigned int enable)
 {
 	/* Skip if we don't have a triled block */
@@ -217,6 +333,41 @@ static int triled_set(struct lpg *lpg, unsigned int mask, unsigned int enable)
 				  mask, enable);
 }
 
+static int lpg_lut_store_sdam(struct lpg *lpg, struct led_pattern *pattern,
+			 size_t len, unsigned int *lo_idx, unsigned int *hi_idx)
+{
+	u8 brightness;
+	u16 addr;
+	unsigned int idx;
+	int i, rc;
+
+	if (len > SDAM_LUT_COUNT_MAX) {
+		dev_err(lpg->dev, "Pattern length (%zu) exceeds maximum pattern length (%d)\n",
+			len, SDAM_LUT_COUNT_MAX);
+		return -EINVAL;
+	}
+
+	idx = bitmap_find_next_zero_area(lpg->lut_bitmap, lpg->lut_size,
+					 0, len, 0);
+	if (idx >= lpg->lut_size)
+		return -ENOSPC;
+
+	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 (rc < 0)
+			return rc;
+	}
+
+	bitmap_set(lpg->lut_bitmap, idx, len);
+
+	*lo_idx = idx;
+	*hi_idx = idx + len - 1;
+
+	return 0;
+}
+
 static int lpg_lut_store(struct lpg *lpg, struct led_pattern *pattern,
 			 size_t len, unsigned int *lo_idx, unsigned int *hi_idx)
 {
@@ -463,6 +614,26 @@ 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 void lpg_sdam_apply_lut_control(struct lpg_channel *chan)
+{
+	u8 val, conf = 0;
+	struct lpg *lpg = chan->lpg;
+
+	if (!chan->ramp_oneshot)
+		conf |= LPG_PATTERN_CONFIG_REPEAT;
+
+	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 = RAMP_STEP_DURATION(chan->ramp_tick_ms);
+	if (val > 0)
+		val--;
+	lpg_sdam_write(lpg, SDAM_REG_RAMP_STEP_DURATION, val);
+}
+
 static void lpg_apply_lut_control(struct lpg_channel *chan)
 {
 	struct lpg *lpg = chan->lpg;
@@ -476,6 +647,9 @@ static void lpg_apply_lut_control(struct lpg_channel *chan)
 	if (!chan->ramp_enabled || chan->pattern_lo_idx == chan->pattern_hi_idx)
 		return;
 
+	if (lpg->lpg_chan_nvmem)
+		return lpg_sdam_apply_lut_control(chan);
+
 	hi_pause = DIV_ROUND_UP(chan->ramp_hi_pause_ms, step);
 	lo_pause = DIV_ROUND_UP(chan->ramp_lo_pause_ms, step);
 
@@ -643,6 +817,9 @@ static void lpg_brightness_set(struct lpg_led *led, struct led_classdev *cdev,
 		triled_mask |= chan->triled_mask;
 
 		lpg_apply(chan);
+
+		if (chan->pattern_set)
+			lpg_sdam_configure_triggers(chan, true);
 	}
 
 	/* Toggle triled lines */
@@ -775,7 +952,7 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
 	unsigned int lo_idx;
 	unsigned int hi_idx;
 	unsigned int i;
-	bool ping_pong = true;
+	bool ping_pong = false;
 	int ret = -EINVAL;
 
 	/* Hardware only support oneshot or indefinite loops */
@@ -824,7 +1001,7 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
 	 * used to stretch the first delay of the pattern and a "high pause"
 	 * the last one.
 	 *
-	 * In order to save space the pattern can be played in "ping pong"
+	 * In order to save space for the pattern can be played in "ping pong"
 	 * mode, in which the pattern is first played forward, then "high
 	 * pause" is applied, then the pattern is played backwards and finally
 	 * the "low pause" is applied.
@@ -837,16 +1014,22 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
 	 * If the specified pattern is a palindrome the ping pong mode is
 	 * 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"
 	 */
 
 	/* Detect palindromes and use "ping pong" to reduce LUT usage */
-	for (i = 0; i < len / 2; i++) {
-		brightness_a = pattern[i].brightness;
-		brightness_b = pattern[len - i - 1].brightness;
-
-		if (brightness_a != brightness_b) {
-			ping_pong = false;
-			break;
+	if (lpg->lut_base) {
+		ping_pong = true;
+		for (i = 0; i < len / 2; i++) {
+			brightness_a = pattern[i].brightness;
+			brightness_b = pattern[len - i - 1].brightness;
+
+			if (brightness_a != brightness_b) {
+				ping_pong = false;
+				break;
+			}
 		}
 	}
 
@@ -860,14 +1043,21 @@ 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.
 	 */
-	delta_t = pattern[1].delta_t;
-	for (i = 2; i < len; i++) {
+	if (lpg->lpg_chan_nvmem) {
+		i = 1;
+		delta_t = pattern[0].delta_t;
+	} else {
+		i = 2;
+		delta_t = pattern[1].delta_t;
+	}
+
+	for (; i < len; i++) {
 		if (pattern[i].delta_t != delta_t) {
 			/*
 			 * Allow last entry in the full or shortened pattern to
 			 * specify hi pause. Reject other variations.
 			 */
-			if (i != actual_len - 1)
+			if (i != actual_len - 1 || lpg->lpg_chan_nvmem)
 				goto out_free_pattern;
 		}
 	}
@@ -876,12 +1066,19 @@ 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 */
-	lo_pause = pattern[0].delta_t;
-	hi_pause = pattern[actual_len - 1].delta_t;
+	/* Find "low pause" and "high pause" in the pattern if not an NVMEM device*/
+	if (lpg->lut_base) {
+		lo_pause = pattern[0].delta_t;
+		hi_pause = pattern[actual_len - 1].delta_t;
+	}
 
 	mutex_lock(&lpg->lock);
-	ret = lpg_lut_store(lpg, pattern, actual_len, &lo_idx, &hi_idx);
+
+	if (lpg->lpg_chan_nvmem)
+		ret = lpg_lut_store_sdam(lpg, pattern, actual_len, &lo_idx, &hi_idx);
+	else
+		ret = lpg_lut_store(lpg, pattern, actual_len, &lo_idx, &hi_idx);
+
 	if (ret < 0)
 		goto out_unlock;
 
@@ -897,6 +1094,8 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
 
 		chan->pattern_lo_idx = lo_idx;
 		chan->pattern_hi_idx = hi_idx;
+
+		chan->pattern_set = true;
 	}
 
 out_unlock:
@@ -954,6 +1153,7 @@ static int lpg_pattern_clear(struct lpg_led *led)
 
 	for (i = 0; i < led->num_channels; i++) {
 		chan = led->channels[i];
+		lpg_sdam_configure_triggers(chan, false);
 		chan->pattern_lo_idx = 0;
 		chan->pattern_hi_idx = 0;
 	}
@@ -1190,8 +1390,8 @@ static int lpg_add_led(struct lpg *lpg, struct device_node *np)
 		cdev->brightness_set_blocking = lpg_brightness_mc_set;
 		cdev->blink_set = lpg_blink_mc_set;
 
-		/* Register pattern accessors only if we have a LUT block */
-		if (lpg->lut_base) {
+		/* Register pattern accessors if we have a LUT block or when using PPG */
+		if (lpg->lut_base || lpg->lpg_chan_nvmem) {
 			cdev->pattern_set = lpg_pattern_mc_set;
 			cdev->pattern_clear = lpg_pattern_mc_clear;
 		}
@@ -1204,15 +1404,19 @@ static int lpg_add_led(struct lpg *lpg, struct device_node *np)
 		cdev->brightness_set_blocking = lpg_brightness_single_set;
 		cdev->blink_set = lpg_blink_single_set;
 
-		/* Register pattern accessors only if we have a LUT block */
-		if (lpg->lut_base) {
+		/* Register pattern accessors if we have a LUT block or when using PPG */
+		if (lpg->lut_base || lpg->lpg_chan_nvmem) {
 			cdev->pattern_set = lpg_pattern_single_set;
 			cdev->pattern_clear = lpg_pattern_single_clear;
 		}
 	}
 
 	cdev->default_trigger = of_get_property(np, "linux,default-trigger", NULL);
-	cdev->max_brightness = LPG_RESOLUTION_9BIT - 1;
+
+	if (lpg->lpg_chan_nvmem)
+		cdev->max_brightness = PPG_MAX_LED_BRIGHTNESS;
+	else
+		cdev->max_brightness = LPG_RESOLUTION_9BIT - 1;
 
 	if (!of_property_read_string(np, "default-state", &state) &&
 	    !strcmp(state, "on"))
@@ -1253,6 +1457,8 @@ static int lpg_init_channels(struct lpg *lpg)
 		chan->base = data->channels[i].base;
 		chan->triled_mask = data->channels[i].triled_mask;
 		chan->lut_mask = BIT(i);
+		chan->sdam_offset = data->channels[i].sdam_offset;
+		chan->lpg_idx = i;
 
 		regmap_read(lpg->map, chan->base + LPG_SUBTYPE_REG, &chan->subtype);
 	}
@@ -1299,10 +1505,13 @@ static int lpg_init_lut(struct lpg *lpg)
 {
 	const struct lpg_data *data = lpg->data;
 
-	if (!data->lut_base)
+	if (data->lut_base)
+		lpg->lut_base = data->lut_base;
+	else if (lpg->lpg_chan_nvmem)
+		lpg->lut_sdam_base = data->lut_sdam_base;
+	else
 		return 0;
 
-	lpg->lut_base = data->lut_base;
 	lpg->lut_size = data->lut_size;
 
 	lpg->lut_bitmap = devm_bitmap_zalloc(lpg->dev, lpg->lut_size, GFP_KERNEL);
@@ -1312,6 +1521,43 @@ static int lpg_init_lut(struct lpg *lpg)
 	return 0;
 }
 
+static int lpg_init_sdam(struct lpg *lpg)
+{
+	struct lpg_channel *chan;
+	int i, nvmem_count, rc;
+
+	nvmem_count = of_property_count_strings(lpg->dev->of_node, "nvmem-names");
+	if (nvmem_count <= 0)
+		return 0;
+
+	/* get the 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))
+		return dev_err_probe(lpg->dev, PTR_ERR(lpg->lpg_chan_nvmem),
+				"Failed to get nvmem 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");
+
+	for (i = 0; i < lpg->num_channels; i++) {
+		chan = &lpg->channels[i];
+		if (chan->sdam_offset) {
+			rc = lpg_sdam_write(lpg,
+				SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET + chan->sdam_offset, 0);
+			if (rc < 0)
+				return rc;
+
+			rc = lpg_sdam_configure_triggers(chan, false);
+			if (rc < 0)
+				return rc;
+		}
+	}
+
+	return 0;
+}
+
 static int lpg_probe(struct platform_device *pdev)
 {
 	struct device_node *np;
@@ -1348,6 +1594,10 @@ static int lpg_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
+	ret = lpg_init_sdam(lpg);
+	if (ret < 0)
+		return ret;
+
 	ret = lpg_init_lut(lpg);
 	if (ret < 0)
 		return ret;
-- 
2.41.0

[PATCH v4 5/7] leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG

[Anjelique Melendez]

Update the pmi632 lpg_data struct so that pmi632 devices use PPG
for LUT pattern.

Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
---
 drivers/leds/rgb/leds-qcom-lpg.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index 90dc27d5eb7c..0b37d3b539f8 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -1672,11 +1672,14 @@ 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,
+
 	.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.41.0

[PATCH v4 6/7] leds: rgb: leds-qcom-lpg: Include support for dedicated LUT SDAM PPG Scheme

[Anjelique Melendez]

On PMICs such as PM8350C, the lookup table containing the pattern data
is stored in a separate SDAM from the one where the per-channel
data is stored.

Add support for the dedicated LUT SDAM while maintaining backward
compatibility for those targets that use only a single SDAM.

Co-developed-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
---
 drivers/leds/rgb/leds-qcom-lpg.c | 102 ++++++++++++++++++++++++-------
 1 file changed, 79 insertions(+), 23 deletions(-)

diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index 0b37d3b539f8..9f1580e81ab0 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -43,6 +43,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
@@ -62,6 +64,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
 
@@ -71,6 +74,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;
@@ -87,6 +92,7 @@ struct lpg_data;
  * @lut_bitmap:	allocation bitmap for LUT entries
  * @pbs_dev:	PBS device
  * @lpg_chan_nvmem:	LPG nvmem peripheral device
+ * @lut_nvmem:	LUT nvmem peripheral device
  * @pbs_en_bitmap:	bitmap for tracking PBS triggers
  * @lut_sdam_base:	offset where LUT pattern begins in nvmem
  * @triled_base:	base address of the TRILED block (optional)
@@ -112,6 +118,7 @@ struct lpg {
 
 	struct pbs_dev *pbs_dev;
 	struct nvmem_device *lpg_chan_nvmem;
+	struct nvmem_device *lut_nvmem;
 	unsigned long pbs_en_bitmap;
 	u32 lut_sdam_base;
 
@@ -271,6 +278,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->lut_nvmem) {
+			rc = lpg_sdam_write(chan->lpg, SDAM_PBS_TRIG_CLR, PBS_SW_TRIG_BIT);
+			if (rc < 0)
+				return rc;
+		}
 	}
 
 	return 0;
@@ -285,9 +298,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->lut_nvmem) {
+			rc = lpg_sdam_write(chan->lpg, SDAM_PBS_TRIG_SET, PBS_SW_TRIG_BIT);
+			if (rc < 0)
+				return rc;
+		} else {
+			rc = qcom_pbs_trigger_event(chan->lpg->pbs_dev, PBS_SW_TRIG_BIT);
+			if (rc < 0)
+				return rc;
+		}
 	}
 	set_bit(chan->lpg_idx, &chan->lpg->pbs_en_bitmap);
 
@@ -355,7 +374,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->lut_nvmem)
+			rc = nvmem_device_write(lpg->lut_nvmem, addr, 1, &brightness);
+		else
+			rc = lpg_sdam_write(lpg, addr, brightness);
+
 		if (rc < 0)
 			return rc;
 	}
@@ -616,22 +640,40 @@ static void lpg_apply_pwm_value(struct lpg_channel *chan)
 
 static void lpg_sdam_apply_lut_control(struct lpg_channel *chan)
 {
-	u8 val, conf = 0;
+	u8 val, conf = 0, lut_offset = 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->lut_nvmem)
+		conf |= LPG_PATTERN_CONFIG_PAUSE_HI;
+	if (chan->ramp_lo_pause_ms && lpg->lut_nvmem)
+		conf |= LPG_PATTERN_CONFIG_PAUSE_LO;
+	if (lpg->lut_nvmem)
+		lut_offset = chan->lpg->lut_sdam_base - SDAM_START_BASE;
 
 	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);
+	lpg_sdam_write(lpg, SDAM_END_INDEX_OFFSET + chan->sdam_offset,
+			chan->pattern_hi_idx + lut_offset);
+	lpg_sdam_write(lpg, SDAM_START_INDEX_OFFSET + chan->sdam_offset,
+			chan->pattern_lo_idx + lut_offset);
 
 	val = RAMP_STEP_DURATION(chan->ramp_tick_ms);
 	if (val > 0)
 		val--;
 	lpg_sdam_write(lpg, SDAM_REG_RAMP_STEP_DURATION, val);
+
+	if (lpg->lut_nvmem || lpg->lut_base) {
+		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)
@@ -1015,8 +1057,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"
+	 * 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 */
@@ -1043,12 +1085,12 @@ 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->lpg_chan_nvmem) {
-		i = 1;
-		delta_t = pattern[0].delta_t;
-	} else {
+	if (lpg->lut_base || lpg->lut_nvmem) {
 		i = 2;
 		delta_t = pattern[1].delta_t;
+	} else {
+		i = 1;
+		delta_t = pattern[0].delta_t;
 	}
 
 	for (; i < len; i++) {
@@ -1057,7 +1099,9 @@ 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->lpg_chan_nvmem)
+			if (lpg->lpg_chan_nvmem && !lpg->lut_nvmem)
+				goto out_free_pattern;
+			if (i != actual_len - 1)
 				goto out_free_pattern;
 		}
 	}
@@ -1066,8 +1110,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->lut_base) {
+	/* Find "low pause" and "high pause" in the pattern if not a single NVMEM device*/
+	if (lpg->lut_base || lpg->lut_nvmem) {
 		lo_pause = pattern[0].delta_t;
 		hi_pause = pattern[actual_len - 1].delta_t;
 	}
@@ -1530,16 +1574,28 @@ static int lpg_init_sdam(struct lpg *lpg)
 	if (nvmem_count <= 0)
 		return 0;
 
-	/* get the nvmem device for LPG/LUT config */
+	if (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))
 		return dev_err_probe(lpg->dev, PTR_ERR(lpg->lpg_chan_nvmem),
-				"Failed to get nvmem 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 (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))
+			return dev_err_probe(lpg->dev, PTR_ERR(lpg->pbs_dev),
+					"Failed to get PBS client device\n");
+	} else if (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))
+			return dev_err_probe(lpg->dev, PTR_ERR(lpg->lut_nvmem),
+					"Failed to get lut_sdam device\n");
+	}
 
 	for (i = 0; i < lpg->num_channels; i++) {
 		chan = &lpg->channels[i];
-- 
2.41.0

[PATCH v4 7/7] leds: rgb: Update PM8350C lpg_data to support two-nvmem PPG Scheme

[Anjelique Melendez]

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 <quic_amelende@quicinc.com>
---
 drivers/leds/rgb/leds-qcom-lpg.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index 9f1580e81ab0..b2435693b50e 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -1808,11 +1808,14 @@ 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,
+
 	.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

Re: [PATCH v4 3/7] soc: qcom: add QCOM PBS driver

[Konrad Dybcio]

On 30.08.2023 20:05, Anjelique Melendez wrote:
> Add the Qualcomm PBS (Programmable Boot Sequencer) driver. The QCOM PBS
> driver supports configuring software PBS trigger events through PBS RAM
> on Qualcomm Technologies, Inc (QTI) PMICs.
> 
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> ---
[...]

> +static int qcom_pbs_read(struct pbs_dev *pbs, u32 address, u8 *val)
I've seen your answer in v2, but I'm still not convinced about
two things:

1. why are you using bulk APIs with count=1 instead of just
   regmap_write/read?
2. do we expect the accesses to ever fail (realistically), if not
   we don't have to care about the retval and skip the conditional
   error message (1-2 cycles less per invocation)

You insisted on keeping the error messages, but firstly you'll soon
get an angry response from Krzysztof saying register accesses can't
fail (hence making checking the retval useless) and secondly I think
spmi core already spits out some errors on disallowed r/w

If you agree access failures are very edge cases, you can simply
convert all r/w ops to regmap_read/write/modify_bits and pass
pbs->base + reg

> +{
> +	int ret;
> +
> +	address += pbs->base;
> +	ret = regmap_bulk_read(pbs->regmap, address, val, 1);
> +	if (ret)
> +		dev_err(pbs->dev, "Failed to read address=%#x sid=%#x ret=%d\n",
> +			address, to_spmi_device(pbs->dev->parent)->usid, ret);
> +
> +	return ret;
> +}
[...]

> +static int qcom_pbs_wait_for_ack(struct pbs_dev *pbs, u8 bit_pos)
> +{
> +	int ret, retries = 2000, delay = 1000;
> +	u8 val;
> +
> +	while (retries--) {
> +		ret = qcom_pbs_read(pbs, PBS_CLIENT_SCRATCH2, &val);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (val == PBS_CLIENT_SCRATCH2_ERROR) {
> +			/* PBS error - clear SCRATCH2 register */
> +			ret = qcom_pbs_write(pbs, PBS_CLIENT_SCRATCH2, 0);
> +			if (ret < 0)
> +				return ret;
> +
> +			dev_err(pbs->dev, "NACK from PBS for bit %u\n", bit_pos);
> +			return -EINVAL;
> +		}
> +
> +		if (val & BIT(bit_pos)) {
> +			dev_dbg(pbs->dev, "PBS sequence for bit %u executed!\n", bit_pos);
> +			return 0;
> +		}
> +
> +		usleep_range(delay, delay + 100);
> +	}
Since the SCRATCH2_ERROR path exits the loop, this can simply be
made into:

regmap_read_poll_timeout

if (SCARTCH2_ERROR)
  do something
  return einval

return etimedout

[...]

> +int qcom_pbs_trigger_event(struct pbs_dev *pbs, u8 bitmap)
> +{
> +	u8 val;
> +	u16 bit_pos;
> +	int ret;
Reverse-Christmas-tree?

> +
> +	if (!bitmap) {
> +		dev_err(pbs->dev, "Invalid bitmap passed by client\n");
> +		return -EINVAL;
> +	}
> +
> +	if (IS_ERR_OR_NULL(pbs))
> +		return -EINVAL;
> +
> +	mutex_lock(&pbs->lock);
> +	ret = qcom_pbs_read(pbs, PBS_CLIENT_SCRATCH2, &val);
> +	if (ret < 0)
> +		goto out;
> +
> +	if (val == PBS_CLIENT_SCRATCH2_ERROR) {
> +		/* PBS error - clear SCRATCH2 register */
> +		ret = qcom_pbs_write(pbs, PBS_CLIENT_SCRATCH2, 0);
> +		if (ret < 0)
> +			goto out;
> +	}
Probably deserves an error message?

> +
> +	for (bit_pos = 0; bit_pos < 8; bit_pos++) {
> +		if (bitmap & BIT(bit_pos)) {
if (!(bitmap & BIT(bit_pos))
    continue

would save you a level of indentation

Konrad

Re: [PATCH v4 4/7] leds: rgb: leds-qcom-lpg: Add support for PPG through single SDAM

[Konrad Dybcio]

On 30.08.2023 20:05, Anjelique Melendez wrote:
> In some PMICs like pmi632, the pattern look up table (LUT) and LPG
> configuration can be stored in a single SDAM module instead of LUT
> peripheral. This feature is called PPG. PPG uses Qualcomm Programmable
> Boot Sequencer (PBS) inorder to trigger pattern sequences for PMICs.
I still fail to understand what benefit this brings.

Is this a "can be used", or "should be used", or maybe "must be used"?

Are there any distinct advantages to using one over the other?
I see some limitations in the code below, but that's not being made
obvious.

This all should be in the commit message, the current one includes
a lot of cryptic names that mean nothing to most people.

[...]

>  
> +static int lpg_sdam_write(struct lpg *lpg, u16 addr, u8 val)
Again, looks like excessive helpers for r/w accessors.

> +{
> +	int rc;
> +
> +	rc = nvmem_device_write(lpg->lpg_chan_nvmem, addr, 1, &val);
> +	if (rc < 0) {
> +		dev_err(lpg->dev, "writing %u to SDAM addr %#x failed, rc=%d\n",
> +			val, addr, rc);
> +		return rc;
> +	}
> +
> +	return 0;
> +}
> +
> +#define PBS_SW_TRIG_BIT		BIT(0)
> +
> +static int lpg_clear_pbs_trigger(struct lpg_channel *chan)
> +{
> +	int rc;
> +
> +	clear_bit(chan->lpg_idx, &chan->lpg->pbs_en_bitmap);
> +	if (!chan->lpg->pbs_en_bitmap) {
> +		rc = lpg_sdam_write(chan->lpg, SDAM_REG_PBS_SEQ_EN, 0);
> +		if (rc < 0)
> +			return rc;
> +	}
> +
> +	return 0;
> +}
> +
> +static int lpg_set_pbs_trigger(struct lpg_channel *chan)
> +{
> +	int rc;
> +
> +	if (!chan->lpg->pbs_en_bitmap) {
> +		rc = lpg_sdam_write(chan->lpg, SDAM_REG_PBS_SEQ_EN, PBS_SW_TRIG_BIT);
> +		if (rc < 0)
> +			return rc;
> +
> +		rc = qcom_pbs_trigger_event(chan->lpg->pbs_dev, PBS_SW_TRIG_BIT);
> +		if (rc < 0)
> +			return rc;
> +	}
> +	set_bit(chan->lpg_idx, &chan->lpg->pbs_en_bitmap);
> +
> +	return 0;
> +}
> +
> +static int lpg_sdam_configure_triggers(struct lpg_channel *chan, bool set_trig)
> +{
> +	int rc;
> +
> +	if (chan->lpg->lut_base)
> +		return 0;
> +
> +	if (set_trig) {
> +		rc = lpg_sdam_write(chan->lpg, SDAM_LUT_EN_OFFSET + chan->sdam_offset, 1);
> +		if (rc < 0)
> +			return rc;
> +
> +		rc = lpg_set_pbs_trigger(chan);
> +		if (rc < 0)
> +			return rc;
> +		chan->pattern_set = false;
> +	} else {
> +		rc = lpg_sdam_write(chan->lpg, SDAM_LUT_EN_OFFSET + chan->sdam_offset, 0);
> +		if (rc < 0)
> +			return rc;
> +
> +		rc = lpg_clear_pbs_trigger(chan);
> +		if (rc < 0)
> +			return rc;
> +	}
> +
> +	return 0;
> +}
> +
>  static int triled_set(struct lpg *lpg, unsigned int mask, unsigned int enable)
>  {
>  	/* Skip if we don't have a triled block */
> @@ -217,6 +333,41 @@ static int triled_set(struct lpg *lpg, unsigned int mask, unsigned int enable)
>  				  mask, enable);
>  }
>  
> +static int lpg_lut_store_sdam(struct lpg *lpg, struct led_pattern *pattern,
> +			 size_t len, unsigned int *lo_idx, unsigned int *hi_idx)
> +{
> +	u8 brightness;
> +	u16 addr;
> +	unsigned int idx;
> +	int i, rc;
Reverse-Christmas-tree?

> +
> +	if (len > SDAM_LUT_COUNT_MAX) {
> +		dev_err(lpg->dev, "Pattern length (%zu) exceeds maximum pattern length (%d)\n",
> +			len, SDAM_LUT_COUNT_MAX);
> +		return -EINVAL;
> +	}
> +
> +	idx = bitmap_find_next_zero_area(lpg->lut_bitmap, lpg->lut_size,
> +					 0, len, 0);
> +	if (idx >= lpg->lut_size)
> +		return -ENOSPC;
> +
> +	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 (rc < 0)
> +			return rc;
> +	}
> +
> +	bitmap_set(lpg->lut_bitmap, idx, len);
> +
> +	*lo_idx = idx;
> +	*hi_idx = idx + len - 1;
> +
> +	return 0;
> +}
> +
>  static int lpg_lut_store(struct lpg *lpg, struct led_pattern *pattern,
>  			 size_t len, unsigned int *lo_idx, unsigned int *hi_idx)
>  {
> @@ -463,6 +614,26 @@ 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 void lpg_sdam_apply_lut_control(struct lpg_channel *chan)
> +{
> +	u8 val, conf = 0;
> +	struct lpg *lpg = chan->lpg;
Reverse-Christmas-tree?

> +
> +	if (!chan->ramp_oneshot)
> +		conf |= LPG_PATTERN_CONFIG_REPEAT;
> +
> +	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 = RAMP_STEP_DURATION(chan->ramp_tick_ms);
> +	if (val > 0)
> +		val--;
????

that sounds very cryptic.. almost like some sort of a bad fixup
maybe the RAMP_STEP_DURATION should contain that "-1"?

not only that, but val is an unsigned value, so it's always > 0

[...]

> @@ -775,7 +952,7 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
>  	unsigned int lo_idx;
>  	unsigned int hi_idx;
>  	unsigned int i;
> -	bool ping_pong = true;
> +	bool ping_pong = false;
Why?

This change combined with assigning true below for LUT mode
is a NOP

>  	int ret = -EINVAL;
>  
>  	/* Hardware only support oneshot or indefinite loops */
> @@ -824,7 +1001,7 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
>  	 * used to stretch the first delay of the pattern and a "high pause"
>  	 * the last one.
>  	 *
> -	 * In order to save space the pattern can be played in "ping pong"
> +	 * In order to save space for the pattern can be played in "ping pong"
>  	 * mode, in which the pattern is first played forward, then "high
>  	 * pause" is applied, then the pattern is played backwards and finally
>  	 * the "low pause" is applied.
> @@ -837,16 +1014,22 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
>  	 * If the specified pattern is a palindrome the ping pong mode is
>  	 * 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"
>  	 */
>  
>  	/* Detect palindromes and use "ping pong" to reduce LUT usage */
> -	for (i = 0; i < len / 2; i++) {
> -		brightness_a = pattern[i].brightness;
> -		brightness_b = pattern[len - i - 1].brightness;
> -
> -		if (brightness_a != brightness_b) {
> -			ping_pong = false;
> -			break;
> +	if (lpg->lut_base) {
> +		ping_pong = true;
> +		for (i = 0; i < len / 2; i++) {
> +			brightness_a = pattern[i].brightness;
> +			brightness_b = pattern[len - i - 1].brightness;
> +
> +			if (brightness_a != brightness_b) {
> +				ping_pong = false;
> +				break;
> +			}
>  		}
>  	}
>  
> @@ -860,14 +1043,21 @@ 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.
>  	 */
> -	delta_t = pattern[1].delta_t;
> -	for (i = 2; i < len; i++) {
> +	if (lpg->lpg_chan_nvmem) {
> +		i = 1;
> +		delta_t = pattern[0].delta_t;
> +	} else {
> +		i = 2;
> +		delta_t = pattern[1].delta_t;
> +	}
Why?

What's the rationale behind this change?

> +
> +	for (; i < len; i++) {
>  		if (pattern[i].delta_t != delta_t) {
>  			/*
>  			 * Allow last entry in the full or shortened pattern to
>  			 * specify hi pause. Reject other variations.
>  			 */
> -			if (i != actual_len - 1)
> +			if (i != actual_len - 1 || lpg->lpg_chan_nvmem)
>  				goto out_free_pattern;
>  		}
>  	}
> @@ -876,12 +1066,19 @@ 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 */
> -	lo_pause = pattern[0].delta_t;
> -	hi_pause = pattern[actual_len - 1].delta_t;
> +	/* Find "low pause" and "high pause" in the pattern if not an NVMEM device*/
missing a space before '*/'
"if not an NVMEM device" -> "in the LUT case"?

Konrad

Re: [PATCH v4 5/7] leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG

[Konrad Dybcio]

On 30.08.2023 20:06, Anjelique Melendez wrote:
> Update the pmi632 lpg_data struct so that pmi632 devices use PPG
> for LUT pattern.
> 
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> ---
>  drivers/leds/rgb/leds-qcom-lpg.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> index 90dc27d5eb7c..0b37d3b539f8 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> @@ -1672,11 +1672,14 @@ 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,
Is that a predefined space for use with LPG?

Or can it be reclaimed for something else?

Konrad

Re: [PATCH v4 2/7] dt-bindings: leds: leds-qcom-lpg: Add support for LPG PPG

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4159 bytes --]

On Wed, Aug 30, 2023 at 11:05:57AM -0700, Anjelique Melendez wrote:
> Update leds-qcom-lpg binding to support LPG PPG.
> 
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  .../bindings/leds/leds-qcom-lpg.yaml          | 89 ++++++++++++++++++-
>  1 file changed, 88 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> index e6f1999cb22f..067ebe35ca5e 100644
> --- a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> +++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> @@ -11,7 +11,7 @@ maintainers:
>  
>  description: >
>    The Qualcomm Light Pulse Generator consists of three different hardware blocks;
> -  a ramp generator with lookup table, the light pulse generator and a three
> +  a ramp generator with lookup table (LUT), the light pulse generator and a three
>    channel current sink. These blocks are found in a wide range of Qualcomm PMICs.
>  
>  properties:
> @@ -63,6 +63,29 @@ properties:
>          - description: dtest line to attach
>          - description: flags for the attachment
>  
> +  nvmem:

> +    description: >

Why do you have these chomping operators? I can't see any formatting
that'd require them. Unless you're respinning for other reasons, you can
ignore this comment.

Thanks,
Conor.


> +      This property is required for PMICs that supports PPG, which is when a
> +      PMIC stores LPG per-channel data and pattern LUT in SDAM modules instead
> +      of in a LUT peripheral. For PMICs, such as PM8350C, per-channel data
> +      and pattern LUT is separated into 2 SDAM modules. In that case, phandles
> +      to both SDAM modules need to be specified.
> +    minItems: 1
> +    maxItems: 2
> +
> +  nvmem-names:
> +    minItems: 1
> +    items:
> +      - const: lpg_chan_sdam
> +      - const: lut_sdam
> +
> +  qcom,pbs:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: >
> +      Phandle of the Qualcomm Programmable Boot Sequencer node (PBS).
> +      PBS node is used to trigger LPG pattern sequences for PMICs that support
> +      single SDAM PPG.
> +
>    multi-led:
>      type: object
>      $ref: leds-class-multicolor.yaml#
> @@ -106,6 +129,39 @@ required:
>  
>  additionalProperties: false
>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: qcom,pmi632-lpg
> +    then:
> +      properties:
> +        nvmem:
> +          maxItems: 1
> +        nvmem-names:
> +          maxItems: 1
> +      required:
> +        - nvmem
> +        - nvmem-names
> +        - qcom,pbs
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,pm8350c-pwm
> +              - qcom,pm8550-pwm
> +    then:
> +      properties:
> +        nvmem:
> +          minItems: 2
> +        nvmem-names:
> +          minItems: 2
> +      required:
> +        - nvmem
> +        - nvmem-names
> +
>  examples:
>    - |
>      #include <dt-bindings/leds/common.h>
> @@ -191,4 +247,35 @@ examples:
>        compatible = "qcom,pm8916-pwm";
>        #pwm-cells = <2>;
>      };
> +  - |
> +    #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_sdam_7>;
> +      qcom,pbs = <&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.41.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 5/7] leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG

[Anjelique Melendez]

On 8/30/2023 11:34 AM, Konrad Dybcio wrote:
> On 30.08.2023 20:06, Anjelique Melendez wrote:
>> Update the pmi632 lpg_data struct so that pmi632 devices use PPG
>> for LUT pattern.
>>
>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>> ---
>>  drivers/leds/rgb/leds-qcom-lpg.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
>> index 90dc27d5eb7c..0b37d3b539f8 100644
>> --- a/drivers/leds/rgb/leds-qcom-lpg.c
>> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
>> @@ -1672,11 +1672,14 @@ 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,
> Is that a predefined space for use with LPG?
> 
> Or can it be reclaimed for something else?
> 
> Konrad
Yes, this is a predefined space for use with LPG

Re: [PATCH v4 4/7] leds: rgb: leds-qcom-lpg: Add support for PPG through single SDAM

[Anjelique Melendez]

On 8/30/2023 11:34 AM, Konrad Dybcio wrote:
> On 30.08.2023 20:05, Anjelique Melendez wrote:
>> In some PMICs like pmi632, the pattern look up table (LUT) and LPG
>> configuration can be stored in a single SDAM module instead of LUT
>> peripheral. This feature is called PPG. PPG uses Qualcomm Programmable
>> Boot Sequencer (PBS) inorder to trigger pattern sequences for PMICs.
> I still fail to understand what benefit this brings.
> 
> Is this a "can be used", or "should be used", or maybe "must be used"?
> 
> Are there any distinct advantages to using one over the other?
> I see some limitations in the code below, but that's not being made
> obvious.
> 
> This all should be in the commit message, the current one includes
> a lot of cryptic names that mean nothing to most people.
> 
> [...]
This is a must be used if you would like to trigger patterns. Will update commit message to try and 
make that more clear for next patch.

>> @@ -775,7 +952,7 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
>>  	unsigned int lo_idx;
>>  	unsigned int hi_idx;
>>  	unsigned int i;
>> -	bool ping_pong = true;
>> +	bool ping_pong = false;
> Why?
> 
> This change combined with assigning true below for LUT mode
> is a NOP

LUT devices support a "ping pong" setting that PPG devices do not but honestly looking back at this
I'm not quite sure why I decided to make the change like this :)

Will revert back to bool ping_pong = true and just wrap loop in an if (lpg->lut_base) { for...} else {ping_pong = false;}
i.e.

	if (lpg->lut_base) {
		for (i = 0; i < len / 2; i++) {
			brightness_a = pattern[i].brightness;
			brightness_b = pattern[len - i - 1].brightness;

			if (brightness_a != brightness_b) {
				ping_pong = false;
				break;
			}
  		}
  	} else
		ping_pong = false;

> 
>>  	int ret = -EINVAL;
>>  
>>  	/* Hardware only support oneshot or indefinite loops */
>> @@ -824,7 +1001,7 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
>>  	 * used to stretch the first delay of the pattern and a "high pause"
>>  	 * the last one.
>>  	 *
>> -	 * In order to save space the pattern can be played in "ping pong"
>> +	 * In order to save space for the pattern can be played in "ping pong"
>>  	 * mode, in which the pattern is first played forward, then "high
>>  	 * pause" is applied, then the pattern is played backwards and finally
>>  	 * the "low pause" is applied.
>> @@ -837,16 +1014,22 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
>>  	 * If the specified pattern is a palindrome the ping pong mode is
>>  	 * 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"
>>  	 */
>>  
>>  	/* Detect palindromes and use "ping pong" to reduce LUT usage */
>> -	for (i = 0; i < len / 2; i++) {
>> -		brightness_a = pattern[i].brightness;
>> -		brightness_b = pattern[len - i - 1].brightness;
>> -
>> -		if (brightness_a != brightness_b) {
>> -			ping_pong = false;
>> -			break;
>> +	if (lpg->lut_base) {
>> +		ping_pong = true;
>> +		for (i = 0; i < len / 2; i++) {
>> +			brightness_a = pattern[i].brightness;
>> +			brightness_b = pattern[len - i - 1].brightness;
>> +
>> +			if (brightness_a != brightness_b) {
>> +				ping_pong = false;
>> +				break;
>> +			}
>>  		}
>>  	}
>>  
>> @@ -860,14 +1043,21 @@ 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.
>>  	 */
>> -	delta_t = pattern[1].delta_t;
>> -	for (i = 2; i < len; i++) {
>> +	if (lpg->lpg_chan_nvmem) {
>> +		i = 1;
>> +		delta_t = pattern[0].delta_t;
>> +	} else {
>> +		i = 2;
>> +		delta_t = pattern[1].delta_t;
>> +	}
> Why?
> 
> What's the rationale behind this change?
Patterns are required to have the same duration for each step of the pattern. Devices with LUT peripherals support low/high
pause which is when the first/last entry of the pattern can have a longer duration. This loop checks that the all of the
pattern durations are the same with the exception of the first and last entry for low/hi pause.

This change was made because devices that use single SDAM do not support low/high pause, so we must check every
single pattern duration. Instead of changing the loop arguments with an if statement I was thinking we could either:

a. keep the original loop arguments and when loop exits we can check first element for single SDAM devices

   delta_t = pattern[1].delta_t;
   for (i = 2; i < len; i++) {
	if (pattern[i].delta_t != delta_t) {
+ 		if (i != actual_len - 1 || lpg->lpg_chan_nvmem)
  			goto out_free_pattern;
  		}
  	}

+ if (lpg->lpg_chan_nvmem) {
+	if (delta_t != pattern[0].delta_t)
+		goto out_free_pattern
+ }


b. Change the loop argument to start with i=0 and for LUT device we could just skip checking first and last element duration
  ** We would end up checking if pattern[1].delta_t == pattern[1].delta_t inside the loop when i == 1

   delta_t = pattern[1].delta_t;
+  for (i = 0; i < len; i++) {
	if (pattern[i].delta_t != delta_t) {
+		if (lpg->lut_base && (i == 0 || i == actual_len - 1)
+			continue;
+               else
+			goto out_free_pattern;

  	}

Let me know if you would rather go with one of these options to make this change cleaner.

Thanks,
Anjelique

> 
>> +
>> +	for (; i < len; i++) {
>>  		if (pattern[i].delta_t != delta_t) {
>>  			/*
>>  			 * Allow last entry in the full or shortened pattern to
>>  			 * specify hi pause. Reject other variations.
>>  			 */
>> -			if (i != actual_len - 1)
>> +			if (i != actual_len - 1 || lpg->lpg_chan_nvmem)
>>  				goto out_free_pattern;
>>  		}
>>  	}

Re: [PATCH v4 5/7] leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG

[Konrad Dybcio]

On 7.09.2023 21:54, Anjelique Melendez wrote:
> 
> 
> On 8/30/2023 11:34 AM, Konrad Dybcio wrote:
>> On 30.08.2023 20:06, Anjelique Melendez wrote:
>>> Update the pmi632 lpg_data struct so that pmi632 devices use PPG
>>> for LUT pattern.
>>>
>>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>>> ---
>>>  drivers/leds/rgb/leds-qcom-lpg.c | 9 ++++++---
>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
>>> index 90dc27d5eb7c..0b37d3b539f8 100644
>>> --- a/drivers/leds/rgb/leds-qcom-lpg.c
>>> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
>>> @@ -1672,11 +1672,14 @@ 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,
>> Is that a predefined space for use with LPG?
>>
>> Or can it be reclaimed for something else?
>>
>> Konrad
> Yes, this is a predefined space for use with LPG
We represent the SDAM as a NVMEM device, generally it would
be nice to add all regions within it as subnodes in the devicetree.

Krzysztof, opinions?

Konrad

Re: [PATCH v4 5/7] leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG

[Konrad Dybcio]

On 7.09.2023 22:26, Konrad Dybcio wrote:
> On 7.09.2023 21:54, Anjelique Melendez wrote:
>>
>>
>> On 8/30/2023 11:34 AM, Konrad Dybcio wrote:
>>> On 30.08.2023 20:06, Anjelique Melendez wrote:
>>>> Update the pmi632 lpg_data struct so that pmi632 devices use PPG
>>>> for LUT pattern.
>>>>
>>>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>>>> ---
>>>>  drivers/leds/rgb/leds-qcom-lpg.c | 9 ++++++---
>>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
>>>> index 90dc27d5eb7c..0b37d3b539f8 100644
>>>> --- a/drivers/leds/rgb/leds-qcom-lpg.c
>>>> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
>>>> @@ -1672,11 +1672,14 @@ 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,
>>> Is that a predefined space for use with LPG?
>>>
>>> Or can it be reclaimed for something else?
>>>
>>> Konrad
>> Yes, this is a predefined space for use with LPG
> We represent the SDAM as a NVMEM device, generally it would
> be nice to add all regions within it as subnodes in the devicetree.
Wait hmm.. we already get it as a nvmem cell.. Or at least that's
how I understand it (lut_sdam_base == lpg_chan_nvmem->start, pseudocode)

Why don't we access it through the nvmem r/w ops then?

Konrad
> 
> Krzysztof, opinions?
> 
> Konrad

Re: [PATCH v4 4/7] leds: rgb: leds-qcom-lpg: Add support for PPG through single SDAM

[Konrad Dybcio]

On 7.09.2023 21:55, Anjelique Melendez wrote:
> 
> 
> On 8/30/2023 11:34 AM, Konrad Dybcio wrote:
>> On 30.08.2023 20:05, Anjelique Melendez wrote:
>>> In some PMICs like pmi632, the pattern look up table (LUT) and LPG
>>> configuration can be stored in a single SDAM module instead of LUT
>>> peripheral. This feature is called PPG. PPG uses Qualcomm Programmable
>>> Boot Sequencer (PBS) inorder to trigger pattern sequences for PMICs.
>> I still fail to understand what benefit this brings.
>>
>> Is this a "can be used", or "should be used", or maybe "must be used"?
>>
>> Are there any distinct advantages to using one over the other?
>> I see some limitations in the code below, but that's not being made
>> obvious.
>>
>> This all should be in the commit message, the current one includes
>> a lot of cryptic names that mean nothing to most people.
>>
>> [...]
> This is a must be used if you would like to trigger patterns. Will update commit message to try and 
> make that more clear for next patch.
So essentially without this patchset, PM8350C and PMI632 are not capable
of producing LED patterns. Is that correct?

[...]

>>> @@ -860,14 +1043,21 @@ 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.
>>>  	 */
>>> -	delta_t = pattern[1].delta_t;
>>> -	for (i = 2; i < len; i++) {
>>> +	if (lpg->lpg_chan_nvmem) {
>>> +		i = 1;
>>> +		delta_t = pattern[0].delta_t;
>>> +	} else {
>>> +		i = 2;
>>> +		delta_t = pattern[1].delta_t;
>>> +	}
>> Why?
>>
>> What's the rationale behind this change?
> Patterns are required to have the same duration for each step of the pattern. Devices with LUT peripherals support low/high
> pause which is when the first/last entry of the pattern can have a longer duration. This loop checks that the all of the
> pattern durations are the same with the exception of the first and last entry for low/hi pause.
That's the explanation I was looking for! :)

Things like these that are only known to inside folks should
definitely be stated either as a comment, or inside the commit
message. Since you're changing the code flow in a noticeable manner,
this could probably be a good fit for a comment.

> 
> This change was made because devices that use single SDAM do not support low/high pause, so we must check every
> single pattern duration. Instead of changing the loop arguments with an if statement I was thinking we could either:
> 
> a. keep the original loop arguments and when loop exits we can check first element for single SDAM devices
> 
>    delta_t = pattern[1].delta_t;
>    for (i = 2; i < len; i++) {
> 	if (pattern[i].delta_t != delta_t) {
> + 		if (i != actual_len - 1 || lpg->lpg_chan_nvmem)
>   			goto out_free_pattern;
>   		}
>   	}
> 
> + if (lpg->lpg_chan_nvmem) {
> +	if (delta_t != pattern[0].delta_t)
> +		goto out_free_pattern
> + }
We assign hi/lo_pause a couple lines below. Moving these assignments
a bit higher up could let us make this clearer:

/* LPGs using SDAM for patterns require equal duration of all steps */
if ((delta_t != lo_pause) && lpg->lpg_chan_nvmem)
	goto out_free_pattern;

Though I think that (in a separate patch, or perhaps series), it would
be worth redoing the code such that hi/lo_pause expresses the deviation
from the duration of the rest instead of the duration itself. Then we
could just:

if ((lo_pause || hi_pause)) && lpg->lpg_chan_nvmem)
	goto out_free_pattern;

But that's just a suggestion from somebody that didn't work on this code.

Also, I think that using lpg_chan_nvmem interchangeably with SDAM is a
bit confusing. Do we expect NVMEMs/SRAMs that aren't SDAM to make an
appearence here?

> 
> b. Change the loop argument to start with i=0 and for LUT device we could just skip checking first and last element duration
>   ** We would end up checking if pattern[1].delta_t == pattern[1].delta_t inside the loop when i == 1
> 
>    delta_t = pattern[1].delta_t;
> +  for (i = 0; i < len; i++) {
> 	if (pattern[i].delta_t != delta_t) {
> +		if (lpg->lut_base && (i == 0 || i == actual_len - 1)
> +			continue;
> +               else
> +			goto out_free_pattern;
Meh, too many magic literals for my liking

Konrad

Re: [PATCH v4 4/7] leds: rgb: leds-qcom-lpg: Add support for PPG through single SDAM

[Anjelique Melendez]

On 9/7/2023 1:42 PM, Konrad Dybcio wrote:
> On 7.09.2023 21:55, Anjelique Melendez wrote:
>>
>>
>> On 8/30/2023 11:34 AM, Konrad Dybcio wrote:
>>> On 30.08.2023 20:05, Anjelique Melendez wrote:
>>>> In some PMICs like pmi632, the pattern look up table (LUT) and LPG
>>>> configuration can be stored in a single SDAM module instead of LUT
>>>> peripheral. This feature is called PPG. PPG uses Qualcomm Programmable
>>>> Boot Sequencer (PBS) inorder to trigger pattern sequences for PMICs.
>>> I still fail to understand what benefit this brings.
>>>
>>> Is this a "can be used", or "should be used", or maybe "must be used"?
>>>
>>> Are there any distinct advantages to using one over the other?
>>> I see some limitations in the code below, but that's not being made
>>> obvious.
>>>
>>> This all should be in the commit message, the current one includes
>>> a lot of cryptic names that mean nothing to most people.
>>>
>>> [...]
>> This is a must be used if you would like to trigger patterns. Will update commit message to try and 
>> make that more clear for next patch.
> So essentially without this patchset, PM8350C and PMI632 are not capable
> of producing LED patterns. Is that correct?
Yes, that is correct. Since PMI632 and PM8350C do not have LUT peripherals, current code
will not allow them to produce patterns. Luca mentioned this briefly when adding the 
PMI632 LPG device 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/leds/rgb/leds-qcom-lpg.c?h=v6.5&id=d11a79dd047e18dd0b76bc9abebb8470858856d6

> 
> Though I think that (in a separate patch, or perhaps series), it would
> be worth redoing the code such that hi/lo_pause expresses the deviation
> from the duration of the rest instead of the duration itself. Then we
> could just:
> 
> if ((lo_pause || hi_pause)) && lpg->lpg_chan_nvmem)
> 	goto out_free_pattern;
> 
> But that's just a suggestion from somebody that didn't work on this code.
> 
The value that is written back for hi/low_pause is
"how many steps should we hold the first/last pattern values" where step = delta_t.
So if delta_t == hi/low_pause we would need to write back 1. I can look into seeing
if expressing hi/lo_pause as a deviation can easily translate for a different patch
series.

> Also, I think that using lpg_chan_nvmem interchangeably with SDAM is a
> bit confusing. Do we expect NVMEMs/SRAMs that aren't SDAM to make an
> appearence here?
> I believe we only expect SDAMs. I can change the use of nvmem to sdam in
following patch for comments and variable names.

Thanks,
Anjelique

Re: [PATCH v4 5/7] leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG

[Anjelique Melendez]

On 9/7/2023 1:31 PM, Konrad Dybcio wrote:
> On 7.09.2023 22:26, Konrad Dybcio wrote:
>> On 7.09.2023 21:54, Anjelique Melendez wrote:
>>>
>>>
>>> On 8/30/2023 11:34 AM, Konrad Dybcio wrote:
>>>> On 30.08.2023 20:06, Anjelique Melendez wrote:
>>>>> Update the pmi632 lpg_data struct so that pmi632 devices use PPG
>>>>> for LUT pattern.
>>>>>
>>>>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>>>>> ---
>>>>>  drivers/leds/rgb/leds-qcom-lpg.c | 9 ++++++---
>>>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
>>>>> index 90dc27d5eb7c..0b37d3b539f8 100644
>>>>> --- a/drivers/leds/rgb/leds-qcom-lpg.c
>>>>> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
>>>>> @@ -1672,11 +1672,14 @@ 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,
>>>> Is that a predefined space for use with LPG?
>>>>
>>>> Or can it be reclaimed for something else?
>>>>
>>>> Konrad
>>> Yes, this is a predefined space for use with LPG
>> We represent the SDAM as a NVMEM device, generally it would
>> be nice to add all regions within it as subnodes in the devicetree.
> Wait hmm.. we already get it as a nvmem cell.. Or at least that's
> how I understand it (lut_sdam_base == lpg_chan_nvmem->start, pseudocode)
> 
> Why don't we access it through the nvmem r/w ops then?
> 
> Konrad
I think I might be a little confused on what you are asking so please let
me know if this does not answer your question.

lut_sdam_base is the offset where lut pattern begins in the SDAM. So when we are writing back
our LED pattern we end up calling nvmem_device_write(lpg_chan_nvmem, lut_sdam_base + offset, 1, brightness).
So far for every single SDAM PPG devices we have seen the lpg_sdam_base be 0x80 and every
LUT SDAM PPG devices (pm8350c) we have seen lpg_sdam_base be 0x45, which is why we 
included this value in the lpg_data rather than as a devicetree property since it has
been consistent across a few pmics.

I am ok if you would like the lut_sdam_base to be moved to a devicetree property.

Thanks,
Anjelique

Re: [PATCH v4 5/7] leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG

[Konrad Dybcio]

On 8.09.2023 02:30, Anjelique Melendez wrote:
> 
> 
> On 9/7/2023 1:31 PM, Konrad Dybcio wrote:
>> On 7.09.2023 22:26, Konrad Dybcio wrote:
>>> On 7.09.2023 21:54, Anjelique Melendez wrote:
>>>>
>>>>
>>>> On 8/30/2023 11:34 AM, Konrad Dybcio wrote:
>>>>> On 30.08.2023 20:06, Anjelique Melendez wrote:
>>>>>> Update the pmi632 lpg_data struct so that pmi632 devices use PPG
>>>>>> for LUT pattern.
>>>>>>
>>>>>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>>>>>> ---
>>>>>>  drivers/leds/rgb/leds-qcom-lpg.c | 9 ++++++---
>>>>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
>>>>>> index 90dc27d5eb7c..0b37d3b539f8 100644
>>>>>> --- a/drivers/leds/rgb/leds-qcom-lpg.c
>>>>>> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
>>>>>> @@ -1672,11 +1672,14 @@ 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,
>>>>> Is that a predefined space for use with LPG?
>>>>>
>>>>> Or can it be reclaimed for something else?
>>>>>
>>>>> Konrad
>>>> Yes, this is a predefined space for use with LPG
>>> We represent the SDAM as a NVMEM device, generally it would
>>> be nice to add all regions within it as subnodes in the devicetree.
>> Wait hmm.. we already get it as a nvmem cell.. Or at least that's
>> how I understand it (lut_sdam_base == lpg_chan_nvmem->start, pseudocode)
>>
>> Why don't we access it through the nvmem r/w ops then?
>>
>> Konrad
> I think I might be a little confused on what you are asking so please let
> me know if this does not answer your question.
> 
> lut_sdam_base is the offset where lut pattern begins in the SDAM. So when we are writing back
> our LED pattern we end up calling nvmem_device_write(lpg_chan_nvmem, lut_sdam_base + offset, 1, brightness).
> So far for every single SDAM PPG devices we have seen the lpg_sdam_base be 0x80 and every
> LUT SDAM PPG devices (pm8350c) we have seen lpg_sdam_base be 0x45, which is why we 
> included this value in the lpg_data rather than as a devicetree property since it has
> been consistent across a few pmics.
> 
> I am ok if you would like the lut_sdam_base to be moved to a devicetree property.
So.. we have a slice of SDAM represented as an NVMEM cell (and that
part of SDAM is reserved solely for LPG), and then within that cell,
we need to add an additional offset to get to what we want. Correct?

What's in LPG_NVMEM_CELL[0:offset-1] then?

Konrad

Re: [PATCH v4 5/7] leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG

[Anjelique Melendez]

On 9/8/2023 1:28 AM, Konrad Dybcio wrote:
> On 8.09.2023 02:30, Anjelique Melendez wrote:
>> On 9/7/2023 1:31 PM, Konrad Dybcio wrote:
>>> On 7.09.2023 22:26, Konrad Dybcio wrote:
>>>> On 7.09.2023 21:54, Anjelique Melendez wrote:
>>>>> On 8/30/2023 11:34 AM, Konrad Dybcio wrote:
>>>>>> On 30.08.2023 20:06, Anjelique Melendez wrote:
>>>>>>> Update the pmi632 lpg_data struct so that pmi632 devices use PPG
>>>>>>> for LUT pattern.
>>>>>>>
>>>>>>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>>>>>>> ---
>>>>>>>  drivers/leds/rgb/leds-qcom-lpg.c | 9 ++++++---
>>>>>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
>>>>>>> index 90dc27d5eb7c..0b37d3b539f8 100644
>>>>>>> --- a/drivers/leds/rgb/leds-qcom-lpg.c
>>>>>>> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
>>>>>>> @@ -1672,11 +1672,14 @@ 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,
>>>>>> Is that a predefined space for use with LPG?
>>>>>>
>>>>>> Or can it be reclaimed for something else?
>>>>>>
>>>>>> Konrad
>>>>> Yes, this is a predefined space for use with LPG
>>>> We represent the SDAM as a NVMEM device, generally it would
>>>> be nice to add all regions within it as subnodes in the devicetree.
>>> Wait hmm.. we already get it as a nvmem cell.. Or at least that's
>>> how I understand it (lut_sdam_base == lpg_chan_nvmem->start, pseudocode)
>>>
>>> Why don't we access it through the nvmem r/w ops then?
>>>
>>> Konrad
>> I think I might be a little confused on what you are asking so please let
>> me know if this does not answer your question.
>>
>> lut_sdam_base is the offset where lut pattern begins in the SDAM. So when we are writing back
>> our LED pattern we end up calling nvmem_device_write(lpg_chan_nvmem, lut_sdam_base + offset, 1, brightness).
>> So far for every single SDAM PPG devices we have seen the lpg_sdam_base be 0x80 and every
>> LUT SDAM PPG devices (pm8350c) we have seen lpg_sdam_base be 0x45, which is why we 
>> included this value in the lpg_data rather than as a devicetree property since it has
>> been consistent across a few pmics.
>>
>> I am ok if you would like the lut_sdam_base to be moved to a devicetree property.
> So.. we have a slice of SDAM represented as an NVMEM cell (and that
> part of SDAM is reserved solely for LPG), and then within that cell,
> we need to add an additional offset to get to what we want. Correct?
> 
> What's in LPG_NVMEM_CELL[0:offset-1] then?
> 
> Konrad
All SDAMs being used for lpg have the first few registers (0x40 - 0x44) used by PBS
and also contain register map info and sdam size. For the lpg_chan_nvmem SDAM, after
the first few registers we have all of the per channel data such as LUT_EN,
PATTERN_CONFIG, START_INDEX, and END_INDEX. All of these register addresses
that we write back to are defined at the top of leds-qcom-lpg.c and qcom-pbs.c.

When we have single SDAM PPG, pattern entries begin after all of the per channel data at 0x80.
When we have a second SDAM used for LUT, pattern entries begin after the PBS registers at 0x45.

I just went through all of the code again and lut_sdam_base is really only used twice, so we could
define these register addresses instead of having them in device_data if you think that would
make more sense. Would just need to work on variable name that makes the most sense

#define SDAM_LPG_CHAN_SDAM_LUT_PATTERN_OFFSET 0x80
#define SDAM_LUT_SDAM_LUT_PATTERN_OFFSET 0x45

Anjelique