[PATCH 0/7] Add support for LUT PPG

[PATCH 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!

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

[PATCH 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>
---
 .../bindings/soc/qcom/qcom-pbs.yaml           | 41 +++++++++++++++++++
 1 file changed, 41 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..0a89c334f95c
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
@@ -0,0 +1,41 @@
+# 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. PBS
+
+maintainers:
+  - Anjelique Melendez <quic_amelende@quicinc.com>
+
+description: |
+  Qualcomm PBS (programmable boot sequencer) supports triggering sequences
+  for clients upon request.
+
+properties:
+  compatible:
+    const: qcom,pbs
+
+  reg:
+    description: |
+      Base address of the PBS peripheral.
+    maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    pmic {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      qcom,pbs@7400 {
+        compatible = "qcom,pbs";
+        reg = <0x7400>;
+      };
+    };
-- 
2.40.0

[PATCH 2/7] dt-bindings: leds: leds-qcom-lpg: Add support for LUT through NVMEM devices

[Anjelique Melendez]

Update leds-qcom-lpg bindings to support LUT patterns through NVMEM
devices.

Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
---
 .../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

[PATCH 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       | 343 ++++++++++++++++++++++++++++++
 include/linux/soc/qcom/qcom-pbs.h |  36 ++++
 4 files changed, 389 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 a491718f8064..226b668f4690 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -260,6 +260,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 0f43a88b4894..4e154af3877a 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -31,5 +31,6 @@ 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
 obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE)	+= ice.o
diff --git a/drivers/soc/qcom/qcom-pbs.c b/drivers/soc/qcom/qcom-pbs.c
new file mode 100644
index 000000000000..4a2bb7ff8031
--- /dev/null
+++ b/drivers/soc/qcom/qcom-pbs.c
@@ -0,0 +1,343 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#define pr_fmt(fmt)	"PBS: %s: " fmt, __func__
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.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
+
+static LIST_HEAD(pbs_dev_list);
+static DEFINE_MUTEX(pbs_list_lock);
+
+struct pbs_dev {
+	struct device		*dev;
+	struct device_node	*dev_node;
+	struct regmap		*regmap;
+	struct mutex		lock;
+	struct list_head	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)
+		pr_err("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)
+		pr_err("Failed to write address=%#x sid=%#x ret=%d\n",
+			  address, to_spmi_device(pbs->dev->parent)->usid, ret);
+	else
+		pr_debug("Wrote %#x to addr %#x\n", val, address);
+
+	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)
+		pr_err("Failed to write address=%#x ret=%d\n", address, ret);
+	else
+		pr_debug("Wrote %#x to addr %#x\n", val, address);
+
+	return ret;
+}
+
+static int qcom_pbs_wait_for_ack(struct pbs_dev *pbs, u8 bit_pos)
+{
+	u16 retries = 2000, delay = 1000;
+	int ret;
+	u8 val;
+
+	while (retries--) {
+		ret = qcom_pbs_read(pbs, PBS_CLIENT_SCRATCH2, &val);
+		if (ret < 0)
+			return ret;
+
+		if (val == 0xFF) {
+			/* PBS error - clear SCRATCH2 register */
+			ret = qcom_pbs_write(pbs, PBS_CLIENT_SCRATCH2, 0);
+			if (ret < 0)
+				return ret;
+
+			pr_err("NACK from PBS for bit %u\n", bit_pos);
+			return -EINVAL;
+		}
+
+		if (val & BIT(bit_pos)) {
+			pr_debug("PBS sequence for bit %u executed!\n", bit_pos);
+			break;
+		}
+
+		usleep_range(delay, delay + 100);
+	}
+
+	if (!retries) {
+		pr_err("Timeout for PBS ACK/NACK for bit %u\n", bit_pos);
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+/**
+ * qcom_pbs_trigger_single_event() - Trigger PBS sequence without using bitmap.
+ * @pbs: Pointer to PBS device
+ *
+ * This function is used to trigger the PBS that is hooked on the
+ * SW_TRIGGER directly in PBS client.
+ *
+ * Return: 0 on success, < 0 on failure
+ */
+int qcom_pbs_trigger_single_event(struct pbs_dev *pbs)
+{
+	int ret = 0;
+
+	if (IS_ERR_OR_NULL(pbs))
+		return -EINVAL;
+
+	mutex_lock(&pbs->lock);
+	ret = qcom_pbs_masked_write(pbs, PBS_CLIENT_TRIG_CTL, PBS_CLIENT_SW_TRIG_BIT,
+				PBS_CLIENT_SW_TRIG_BIT);
+	if (ret < 0)
+		pr_err("Failed to write register %x ret=%d\n", PBS_CLIENT_TRIG_CTL, ret);
+	mutex_unlock(&pbs->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL(qcom_pbs_trigger_single_event);
+
+/**
+ * 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, mask;
+	u16 bit_pos;
+	int ret;
+
+	if (!bitmap) {
+		pr_err("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 == 0xFF) {
+		/* 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 in
+			 * PBS_CLIENT_SCRATCH2 mask register.
+			 */
+			ret = qcom_pbs_masked_write(pbs, PBS_CLIENT_SCRATCH2, BIT(bit_pos), 0);
+			if (ret < 0)
+				goto error;
+
+			/*
+			 * Set the PBS sequence bit position in
+			 * PBS_CLIENT_SCRATCH1 register.
+			 */
+			val = mask = BIT(bit_pos);
+			ret = qcom_pbs_masked_write(pbs, PBS_CLIENT_SCRATCH1, mask, val);
+			if (ret < 0)
+				goto error;
+
+			/* Initiate the SW trigger */
+			val = mask = PBS_CLIENT_SW_TRIG_BIT;
+			ret = qcom_pbs_masked_write(pbs, PBS_CLIENT_TRIG_CTL, mask, val);
+			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 in
+			 * PBS_CLIENT_SCRATCH1 register.
+			 */
+			ret = qcom_pbs_masked_write(pbs, PBS_CLIENT_SCRATCH1, BIT(bit_pos), 0);
+			if (ret < 0)
+				goto error;
+
+			/*
+			 * Clear the PBS sequence bit position in
+			 * PBS_CLIENT_SCRATCH2 mask register.
+			 */
+			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(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 pbs_dev *pbs;
+
+	pbs_dev_node = of_parse_phandle(dev->of_node, "qcom,pbs-client", 0);
+	if (!pbs_dev_node) {
+		pr_err("Missing qcom,pbs-client property\n");
+		return ERR_PTR(-ENODEV);
+	}
+
+	mutex_lock(&pbs_list_lock);
+	list_for_each_entry(pbs, &pbs_dev_list, link) {
+		if (pbs_dev_node == pbs->dev_node) {
+			of_node_put(pbs_dev_node);
+			mutex_unlock(&pbs_list_lock);
+			return pbs;
+		}
+	}
+	mutex_unlock(&pbs_list_lock);
+
+	pr_debug("Unable to find PBS dev_node\n");
+	of_node_put(pbs_dev_node);
+	return ERR_PTR(-EPROBE_DEFER);
+}
+EXPORT_SYMBOL(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->dev_node = pdev->dev.of_node;
+	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);
+
+	mutex_lock(&pbs_list_lock);
+	list_add(&pbs->link, &pbs_dev_list);
+	mutex_unlock(&pbs_list_lock);
+
+	return 0;
+}
+
+static int qcom_pbs_remove(struct platform_device *pdev)
+{
+	struct pbs_dev *pbs = platform_get_drvdata(pdev);
+
+	mutex_lock(&pbs_list_lock);
+	list_del(&pbs->link);
+	mutex_unlock(&pbs_list_lock);
+
+	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,
+	.remove = qcom_pbs_remove,
+};
+module_platform_driver(qcom_pbs_driver)
+
+MODULE_DESCRIPTION("QCOM PBS DRIVER");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:qcom-pbs");
diff --git a/include/linux/soc/qcom/qcom-pbs.h b/include/linux/soc/qcom/qcom-pbs.h
new file mode 100644
index 000000000000..4b065951686a
--- /dev/null
+++ b/include/linux/soc/qcom/qcom-pbs.h
@@ -0,0 +1,36 @@
+/* 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);
+int qcom_pbs_trigger_single_event(struct pbs_dev *pbs);
+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 int qcom_pbs_trigger_single_event(struct pbs_dev *pbs)
+{
+	return -ENODEV;
+}
+
+static inline struct pbs_dev *get_pbs_client_device(struct device *client_dev)
+{
+	return ERR_PTR(-ENODEV);
+}
+#endif
+
+#endif
-- 
2.40.0

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

[Anjelique Melendez]

In some PMICs like pmi632, the LUT pattern and LPG configuration can be
stored in a single SDAM module instead of LUT peripheral. This feature is
called PPG.

Add support for configuring and using LUT pattern from SDAM.

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

diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index 20469200961f..b60d920c67c4 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
@@ -49,9 +51,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 +83,12 @@ 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 client 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
+ * @ppg_en:	Flag indicating whether PPG is enabled/used
+ * @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 +109,12 @@ 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;
+	bool ppg_en;
+
 	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,21 +229,87 @@ 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
+ * @nvmem_count:	number of nvmems used for LUT and PPG config
  * @channels:		list of channel initialization data
  */
 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;
+	int nvmem_count;
 	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 > 0 ? 0 : rc;
+}
+
+#define SDAM_REG_PBS_SEQ_EN		0x42
+#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 void lpg_sdam_configure_triggers(struct lpg_channel *chan)
+{
+	if (!chan->lpg->ppg_en)
+		return;
+
+	if (chan->enabled && chan->pattern_set) {
+		lpg_sdam_write(chan->lpg, SDAM_LUT_EN_OFFSET + chan->sdam_offset, 1);
+		lpg_set_pbs_trigger(chan);
+		chan->pattern_set = false;
+	} else {
+		lpg_sdam_write(chan->lpg, SDAM_LUT_EN_OFFSET + chan->sdam_offset, 0);
+		lpg_clear_pbs_trigger(chan);
+	}
+}
+
 static int triled_set(struct lpg *lpg, unsigned int mask, unsigned int enable)
 {
 	/* Skip if we don't have a triled block */
@@ -217,6 +320,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 +601,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 +634,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->ppg_en)
+		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);
 
@@ -632,7 +793,7 @@ static void lpg_brightness_set(struct lpg_led *led, struct led_classdev *cdev,
 		} else {
 			lpg_calc_freq(chan, NSEC_PER_MSEC);
 
-			duty = div_u64(brightness * chan->period, cdev->max_brightness);
+			duty = div_u64(brightness * chan->period, LPG_RESOLUTION_9BIT);
 			lpg_calc_duty(chan, duty);
 			chan->enabled = true;
 			chan->ramp_enabled = false;
@@ -643,6 +804,7 @@ static void lpg_brightness_set(struct lpg_led *led, struct led_classdev *cdev,
 		triled_mask |= chan->triled_mask;
 
 		lpg_apply(chan);
+		lpg_sdam_configure_triggers(chan);
 	}
 
 	/* Toggle triled lines */
@@ -775,7 +937,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 */
@@ -837,16 +999,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 +1028,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->ppg_en) {
+		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->ppg_en)
 				goto out_free_pattern;
 		}
 	}
@@ -876,12 +1051,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->ppg_en) {
+		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->ppg_en)
+		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 +1079,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 = lpg->ppg_en;
 	}
 
 out_unlock:
@@ -1190,8 +1374,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->ppg_en) {
 			cdev->pattern_set = lpg_pattern_mc_set;
 			cdev->pattern_clear = lpg_pattern_mc_clear;
 		}
@@ -1204,15 +1388,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->ppg_en) {
 			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->ppg_en)
+		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 +1441,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 +1489,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->ppg_en)
+		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 +1505,60 @@ static int lpg_init_lut(struct lpg *lpg)
 	return 0;
 }
 
+static int lpg_parse_sdam(struct lpg *lpg)
+{
+	int rc = 0;
+
+	if (lpg->data->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)) {
+		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;
+	}
+
+	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;
+	}
+
+	lpg->ppg_en = true;
+
+	return rc;
+}
+
+static int lpg_init_sdam(struct lpg *lpg)
+{
+	struct lpg_channel *chan;
+	int i, rc;
+
+	if (!lpg->ppg_en)
+		return 0;
+
+	for (i = 0; i < lpg->num_channels; i++) {
+		chan = &lpg->channels[i];
+		if (chan->sdam_offset) {
+			rc = lpg_sdam_write(lpg, SDAM_LUT_EN_OFFSET + chan->sdam_offset, 0);
+			if (rc < 0)
+				break;
+
+			rc = lpg_sdam_write(lpg,
+					SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET + chan->sdam_offset, 0);
+			if (rc < 0)
+				break;
+		}
+	}
+
+	return rc;
+}
+
 static int lpg_probe(struct platform_device *pdev)
 {
 	struct device_node *np;
@@ -1348,6 +1595,14 @@ static int lpg_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
+	ret = lpg_parse_sdam(lpg);
+	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;
@@ -1363,7 +1618,9 @@ static int lpg_probe(struct platform_device *pdev)
 	for (i = 0; i < lpg->num_channels; i++)
 		lpg_apply_dtest(&lpg->channels[i]);
 
-	return lpg_add_pwm(lpg);
+	ret = lpg_add_pwm(lpg);
+
+	return ret;
 }
 
 static int lpg_remove(struct platform_device *pdev)
-- 
2.40.0

[PATCH 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 | 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

[PATCH 6/7] leds: rgb: leds-qcom-lpg: Support two-nvmem PPG Scheme

[Anjelique Melendez]

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 <quic_gurus@quicinc.com>
Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
---
 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

[PATCH 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 | 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

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

[Rob Herring]

On Wed, 21 Jun 2023 11:59:45 -0700, Anjelique Melendez wrote:
> Add binding for the Qualcomm Programmable Boot Sequencer device.
> 
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> ---
>  .../bindings/soc/qcom/qcom-pbs.yaml           | 41 +++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
> 

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:
./Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml:26:2: [warning] wrong indentation: expected 2 but found 1 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230621185949.2068-2-quic_amelende@quicinc.com

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.

Re: [PATCH 2/7] dt-bindings: leds: leds-qcom-lpg: Add support for LUT through NVMEM devices

[Rob Herring]

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 <quic_amelende@quicinc.com>
> ---
>  .../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/20230621185949.2068-3-quic_amelende@quicinc.com

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.

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

[Krzysztof Kozlowski]

On 21/06/2023 20:59, Anjelique Melendez wrote:
> Add binding for the Qualcomm Programmable Boot Sequencer device.
> 
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> ---
>  .../bindings/soc/qcom/qcom-pbs.yaml           | 41 +++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml

Except missing testing... few more comments:


> 
> 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..0a89c334f95c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml

Filename matching compatibles, so qcom,pbs

> @@ -0,0 +1,41 @@
> +# 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. PBS

Qualcomm PBS foo bar a bit more than just one word.

E.g. expand PBS acronym

> +
> +maintainers:
> +  - Anjelique Melendez <quic_amelende@quicinc.com>
> +
> +description: |
> +  Qualcomm PBS (programmable boot sequencer) supports triggering sequences
> +  for clients upon request.

I don't understand what's this. What is "triggering sequences"? What
sequences?

> +
> +properties:
> +  compatible:
> +    const: qcom,pbs

Missing SoC specific comaptibles.


> +
> +  reg:
> +    description: |
> +      Base address of the PBS peripheral.

Drop description, it's obvious.

> +    maxItems: 1
> +

Binding looks very incomplete...

> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    pmic {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      qcom,pbs@7400 {

That's not a proper node name. Do you see anywhere such node? Please, do
not work on downstream code, but on mainline.

> +        compatible = "qcom,pbs";
> +        reg = <0x7400>;
> +      };
> +    };

Best regards,
Krzysztof

Re: [PATCH 2/7] dt-bindings: leds: leds-qcom-lpg: Add support for LUT through NVMEM devices

[Krzysztof Kozlowski]

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 <quic_amelende@quicinc.com>
> ---
>  .../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

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

[Krzysztof Kozlowski]

On 21/06/2023 20:59, 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>
> ---
>  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 ++++
>  4 files changed, 389 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 a491718f8064..226b668f4690 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -260,6 +260,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 0f43a88b4894..4e154af3877a 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -31,5 +31,6 @@ 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
>  obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE)	+= ice.o
> diff --git a/drivers/soc/qcom/qcom-pbs.c b/drivers/soc/qcom/qcom-pbs.c
> new file mode 100644
> index 000000000000..4a2bb7ff8031
> --- /dev/null
> +++ b/drivers/soc/qcom/qcom-pbs.c
> @@ -0,0 +1,343 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#define pr_fmt(fmt)	"PBS: %s: " fmt, __func__
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.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
> +
> +static LIST_HEAD(pbs_dev_list);
> +static DEFINE_MUTEX(pbs_list_lock);

No file-scope variables. Drop both. You don't even need it.

> +
> +struct pbs_dev {
> +	struct device		*dev;
> +	struct device_node	*dev_node;
> +	struct regmap		*regmap;
> +	struct mutex		lock;
> +	struct list_head	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)
> +		pr_err("Failed to read address=%#x sid=%#x ret=%d\n",

dev_err

> +			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)
> +		pr_err("Failed to write address=%#x sid=%#x ret=%d\n",
> +			  address, to_spmi_device(pbs->dev->parent)->usid, ret);
> +	else
> +		pr_debug("Wrote %#x to addr %#x\n", val, address);

No, there is regmap debug for this. Drop such debug statements from the
driver.

Actually the error print is also wrong, IMO.

> +
> +	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)
> +		pr_err("Failed to write address=%#x ret=%d\n", address, ret);
> +	else
> +		pr_debug("Wrote %#x to addr %#x\n", val, address);

Drop

> +
> +	return ret;
> +}
> +
> +static int qcom_pbs_wait_for_ack(struct pbs_dev *pbs, u8 bit_pos)
> +{
> +	u16 retries = 2000, delay = 1000;
> +	int ret;
> +	u8 val;
> +
> +	while (retries--) {
> +		ret = qcom_pbs_read(pbs, PBS_CLIENT_SCRATCH2, &val);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (val == 0xFF) {
> +			/* PBS error - clear SCRATCH2 register */
> +			ret = qcom_pbs_write(pbs, PBS_CLIENT_SCRATCH2, 0);
> +			if (ret < 0)
> +				return ret;
> +
> +			pr_err("NACK from PBS for bit %u\n", bit_pos);
> +			return -EINVAL;
> +		}
> +
> +		if (val & BIT(bit_pos)) {
> +			pr_debug("PBS sequence for bit %u executed!\n", bit_pos);

dev_dbg

> +			break;
> +		}
> +
> +		usleep_range(delay, delay + 100);
> +	}
> +
> +	if (!retries) {
> +		pr_err("Timeout for PBS ACK/NACK for bit %u\n", bit_pos);

dev_err
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * qcom_pbs_trigger_single_event() - Trigger PBS sequence without using bitmap.
> + * @pbs: Pointer to PBS device
> + *
> + * This function is used to trigger the PBS that is hooked on the
> + * SW_TRIGGER directly in PBS client.
> + *
> + * Return: 0 on success, < 0 on failure
> + */
> +int qcom_pbs_trigger_single_event(struct pbs_dev *pbs)
> +{
> +	int ret = 0;
> +
> +	if (IS_ERR_OR_NULL(pbs))
> +		return -EINVAL;
> +
> +	mutex_lock(&pbs->lock);
> +	ret = qcom_pbs_masked_write(pbs, PBS_CLIENT_TRIG_CTL, PBS_CLIENT_SW_TRIG_BIT,
> +				PBS_CLIENT_SW_TRIG_BIT);
> +	if (ret < 0)
> +		pr_err("Failed to write register %x ret=%d\n", PBS_CLIENT_TRIG_CTL, ret);

dev_* everywhere

> +	mutex_unlock(&pbs->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(qcom_pbs_trigger_single_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 pbs_dev *pbs;
> +
> +	pbs_dev_node = of_parse_phandle(dev->of_node, "qcom,pbs-client", 0);
> +	if (!pbs_dev_node) {
> +		pr_err("Missing qcom,pbs-client property\n");
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	mutex_lock(&pbs_list_lock);
> +	list_for_each_entry(pbs, &pbs_dev_list, link) {

It does not make sense. You have the reference to the device, so you
have the pbs (via container_of). Don't add some
global-list-lookup-functions.

Look for example at Abel Vesa's ICE patchset.

> +		if (pbs_dev_node == pbs->dev_node) {
> +			of_node_put(pbs_dev_node);
> +			mutex_unlock(&pbs_list_lock);
> +			return pbs;
> +		}
> +	}
> +	mutex_unlock(&pbs_list_lock);

Where is device_link handling?

> +
> +	pr_debug("Unable to find PBS dev_node\n");
> +	of_node_put(pbs_dev_node);
> +	return ERR_PTR(-EPROBE_DEFER);
> +}
> +EXPORT_SYMBOL(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->dev_node = pdev->dev.of_node;

Why do you need to store it?

> +	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);
> +
> +	mutex_lock(&pbs_list_lock);
> +	list_add(&pbs->link, &pbs_dev_list);
> +	mutex_unlock(&pbs_list_lock);
> +
> +	return 0;
> +}
> +
> +static int qcom_pbs_remove(struct platform_device *pdev)
> +{
> +	struct pbs_dev *pbs = platform_get_drvdata(pdev);
> +
> +	mutex_lock(&pbs_list_lock);
> +	list_del(&pbs->link);
> +	mutex_unlock(&pbs_list_lock);
> +
> +	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,
> +	.remove = qcom_pbs_remove,
> +};
> +module_platform_driver(qcom_pbs_driver)
> +
> +MODULE_DESCRIPTION("QCOM PBS DRIVER");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:qcom-pbs");

Drop alias. Not needed. If you need it, you have missing ID table.

> diff --git a/include/linux/soc/qcom/qcom-pbs.h b/include/linux/soc/qcom/qcom-pbs.h
> new file mode 100644
> index 000000000000..4b065951686a
> --- /dev/null



Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 24/06/2023 11:55, Krzysztof Kozlowski wrote:
>> +/**
>> + * 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 pbs_dev *pbs;
>> +
>> +	pbs_dev_node = of_parse_phandle(dev->of_node, "qcom,pbs-client", 0);
>> +	if (!pbs_dev_node) {
>> +		pr_err("Missing qcom,pbs-client property\n");
>> +		return ERR_PTR(-ENODEV);
>> +	}
>> +
>> +	mutex_lock(&pbs_list_lock);
>> +	list_for_each_entry(pbs, &pbs_dev_list, link) {
> 
> It does not make sense. You have the reference to the device, so you
> have the pbs (via container_of). Don't add some
> global-list-lookup-functions.
> 
> Look for example at Abel Vesa's ICE patchset.

To be specific:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/soc/qcom/ice.c?h=v6.4-rc7#n293

(+CC Abel)

Best regards,
Krzysztof

Re: [PATCH 0/7] Add support for LUT PPG

[Luca Weiss]

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 <luca.weiss@fairphone.com> # 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

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

[Rob Herring]

On Wed, Jun 21, 2023 at 11:59:45AM -0700, Anjelique Melendez wrote:
> Add binding for the Qualcomm Programmable Boot Sequencer device.
> 
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> ---
>  .../bindings/soc/qcom/qcom-pbs.yaml           | 41 +++++++++++++++++++
>  1 file changed, 41 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..0a89c334f95c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
> @@ -0,0 +1,41 @@
> +# 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. PBS
> +
> +maintainers:
> +  - Anjelique Melendez <quic_amelende@quicinc.com>
> +
> +description: |
> +  Qualcomm PBS (programmable boot sequencer) supports triggering sequences
> +  for clients upon request.
> +
> +properties:
> +  compatible:
> +    const: qcom,pbs
> +
> +  reg:
> +    description: |
> +      Base address of the PBS peripheral.
> +    maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    pmic {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      qcom,pbs@7400 {
> +        compatible = "qcom,pbs";
> +        reg = <0x7400>;
> +      };

Why do you need a child node for this? Is there more than 1 instance in 
a PMIC? Every sub-function of a PMIC doesn't have to have a DT node.

Rob

Re: [PATCH 2/7] dt-bindings: leds: leds-qcom-lpg: Add support for LUT through NVMEM devices

[Anjelique Melendez]

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 <quic_amelende@quicinc.com>
>> ---
>>  .../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

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

[Anjelique Melendez]

On 6/24/2023 2:55 AM, Krzysztof Kozlowski wrote:
> On 21/06/2023 20:59, 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>
>> ---
>>  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 ++++
>>  4 files changed, 389 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 a491718f8064..226b668f4690 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -260,6 +260,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 0f43a88b4894..4e154af3877a 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -31,5 +31,6 @@ 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
>>  obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE)	+= ice.o
>> diff --git a/drivers/soc/qcom/qcom-pbs.c b/drivers/soc/qcom/qcom-pbs.c
>> new file mode 100644
>> index 000000000000..4a2bb7ff8031
>> --- /dev/null
>> +++ b/drivers/soc/qcom/qcom-pbs.c
>> @@ -0,0 +1,343 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#define pr_fmt(fmt)	"PBS: %s: " fmt, __func__
>> +
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.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
>> +
>> +static LIST_HEAD(pbs_dev_list);
>> +static DEFINE_MUTEX(pbs_list_lock);
> 
> No file-scope variables. Drop both. You don't even need it.
Ack
> 
>> +
>> +struct pbs_dev {
>> +	struct device		*dev;
>> +	struct device_node	*dev_node;
>> +	struct regmap		*regmap;
>> +	struct mutex		lock;
>> +	struct list_head	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)
>> +		pr_err("Failed to read address=%#x sid=%#x ret=%d\n",
> 
> dev_err
Ack
> 
>> +			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)
>> +		pr_err("Failed to write address=%#x sid=%#x ret=%d\n",
>> +			  address, to_spmi_device(pbs->dev->parent)->usid, ret);
>> +	else
>> +		pr_debug("Wrote %#x to addr %#x\n", val, address);
> 
> No, there is regmap debug for this. Drop such debug statements from the
> driver.
Ack
> 
> Actually the error print is also wrong, IMO> 
>> +
>> +	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)
>> +		pr_err("Failed to write address=%#x ret=%d\n", address, ret);
>> +	else
>> +		pr_debug("Wrote %#x to addr %#x\n", val, address);
> 
> Drop
Ack
> 
>> +
>> +	return ret;
>> +}
>> +
>> +static int qcom_pbs_wait_for_ack(struct pbs_dev *pbs, u8 bit_pos)
>> +{
>> +	u16 retries = 2000, delay = 1000;
>> +	int ret;
>> +	u8 val;
>> +
>> +	while (retries--) {
>> +		ret = qcom_pbs_read(pbs, PBS_CLIENT_SCRATCH2, &val);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		if (val == 0xFF) {
>> +			/* PBS error - clear SCRATCH2 register */
>> +			ret = qcom_pbs_write(pbs, PBS_CLIENT_SCRATCH2, 0);
>> +			if (ret < 0)
>> +				return ret;
>> +
>> +			pr_err("NACK from PBS for bit %u\n", bit_pos);
>> +			return -EINVAL;
>> +		}
>> +
>> +		if (val & BIT(bit_pos)) {
>> +			pr_debug("PBS sequence for bit %u executed!\n", bit_pos);
> 
> dev_dbg
Ack
> 
>> +			break;
>> +		}
>> +
>> +		usleep_range(delay, delay + 100);
>> +	}
>> +
>> +	if (!retries) {
>> +		pr_err("Timeout for PBS ACK/NACK for bit %u\n", bit_pos);
> 
> dev_err
Ack
>> +		return -ETIMEDOUT;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * qcom_pbs_trigger_single_event() - Trigger PBS sequence without using bitmap.
>> + * @pbs: Pointer to PBS device
>> + *
>> + * This function is used to trigger the PBS that is hooked on the
>> + * SW_TRIGGER directly in PBS client.
>> + *
>> + * Return: 0 on success, < 0 on failure
>> + */
>> +int qcom_pbs_trigger_single_event(struct pbs_dev *pbs)
>> +{
>> +	int ret = 0;
>> +
>> +	if (IS_ERR_OR_NULL(pbs))
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&pbs->lock);
>> +	ret = qcom_pbs_masked_write(pbs, PBS_CLIENT_TRIG_CTL, PBS_CLIENT_SW_TRIG_BIT,
>> +				PBS_CLIENT_SW_TRIG_BIT);
>> +	if (ret < 0)
>> +		pr_err("Failed to write register %x ret=%d\n", PBS_CLIENT_TRIG_CTL, ret);
> 
> dev_* everywhere
Ack
> 
>> +	mutex_unlock(&pbs->lock);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(qcom_pbs_trigger_single_event);
>> +
> 
> ...
qcom_pbs_trigger_single_event() is used in our downstream haptics driver,
will remove for now and add it back when haptics driver get upstreamed
> 
>> +/**
>> + * 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 pbs_dev *pbs;
>> +
>> +	pbs_dev_node = of_parse_phandle(dev->of_node, "qcom,pbs-client", 0);
>> +	if (!pbs_dev_node) {
>> +		pr_err("Missing qcom,pbs-client property\n");
>> +		return ERR_PTR(-ENODEV);
>> +	}
>> +
>> +	mutex_lock(&pbs_list_lock);
>> +	list_for_each_entry(pbs, &pbs_dev_list, link) {
> 
> It does not make sense. You have the reference to the device, so you
> have the pbs (via container_of). Don't add some
> global-list-lookup-functions.
> 
> Look for example at Abel Vesa's ICE patchset.
> 
>> +		if (pbs_dev_node == pbs->dev_node) {
>> +			of_node_put(pbs_dev_node);
>> +			mutex_unlock(&pbs_list_lock);
>> +			return pbs;
>> +		}
>> +	}
>> +	mutex_unlock(&pbs_list_lock);
> 
> Where is device_link handling?
Thank you for pointing me to Abel's ICE patchset! Will be updating patch to
use container_of as well as having device_link_add().
> 
>> +
>> +	pr_debug("Unable to find PBS dev_node\n");
>> +	of_node_put(pbs_dev_node);
>> +	return ERR_PTR(-EPROBE_DEFER);
>> +}
>> +EXPORT_SYMBOL(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->dev_node = pdev->dev.of_node;
> 
> Why do you need to store it?
Ack - removing storing dev_node
> 
>> +	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);
>> +
>> +	mutex_lock(&pbs_list_lock);
>> +	list_add(&pbs->link, &pbs_dev_list);
>> +	mutex_unlock(&pbs_list_lock);
>> +
>> +	return 0;
>> +}
>> +
>> +static int qcom_pbs_remove(struct platform_device *pdev)
>> +{
>> +	struct pbs_dev *pbs = platform_get_drvdata(pdev);
>> +
>> +	mutex_lock(&pbs_list_lock);
>> +	list_del(&pbs->link);
>> +	mutex_unlock(&pbs_list_lock);
>> +
>> +	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,
>> +	.remove = qcom_pbs_remove,
>> +};
>> +module_platform_driver(qcom_pbs_driver)
>> +
>> +MODULE_DESCRIPTION("QCOM PBS DRIVER");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:qcom-pbs");
> 
> Drop alias. Not needed. If you need it, you have missing ID table.
Ack
> 
>> diff --git a/include/linux/soc/qcom/qcom-pbs.h b/include/linux/soc/qcom/qcom-pbs.h
>> new file mode 100644
>> index 000000000000..4b065951686a
>> --- /dev/null
> 
> 
> 
> Best regards,
> Krzysztof
> 

Thanks,
Anjelque

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

[Anjelique Melendez]

On 6/26/2023 6:58 AM, Rob Herring wrote:
> On Wed, Jun 21, 2023 at 11:59:45AM -0700, Anjelique Melendez wrote:
>> Add binding for the Qualcomm Programmable Boot Sequencer device.
>>
>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>> ---
>>  .../bindings/soc/qcom/qcom-pbs.yaml           | 41 +++++++++++++++++++
>>  1 file changed, 41 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..0a89c334f95c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
>> @@ -0,0 +1,41 @@
>> +# 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. PBS
>> +
>> +maintainers:
>> +  - Anjelique Melendez <quic_amelende@quicinc.com>
>> +
>> +description: |
>> +  Qualcomm PBS (programmable boot sequencer) supports triggering sequences
>> +  for clients upon request.
>> +
>> +properties:
>> +  compatible:
>> +    const: qcom,pbs
>> +
>> +  reg:
>> +    description: |
>> +      Base address of the PBS peripheral.
>> +    maxItems: 1
>> +
>> +required:
>> + - compatible
>> + - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    pmic {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      qcom,pbs@7400 {
>> +        compatible = "qcom,pbs";
>> +        reg = <0x7400>;
>> +      };
> 
> Why do you need a child node for this? Is there more than 1 instance in 
> a PMIC? Every sub-function of a PMIC doesn't have to have a DT node.
>

We currently have another downstream driver (which is planned to get upstreamed)
which also needs a handle to a pbs device in order to properly trigger events. 

> Rob

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

[Dmitry Baryshkov]

On 29/06/2023 04:19, Anjelique Melendez wrote:
> 
> 
> On 6/26/2023 6:58 AM, Rob Herring wrote:
>> On Wed, Jun 21, 2023 at 11:59:45AM -0700, Anjelique Melendez wrote:
>>> Add binding for the Qualcomm Programmable Boot Sequencer device.
>>>
>>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>>> ---
>>>   .../bindings/soc/qcom/qcom-pbs.yaml           | 41 +++++++++++++++++++
>>>   1 file changed, 41 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..0a89c334f95c
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
>>> @@ -0,0 +1,41 @@
>>> +# 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. PBS
>>> +
>>> +maintainers:
>>> +  - Anjelique Melendez <quic_amelende@quicinc.com>
>>> +
>>> +description: |
>>> +  Qualcomm PBS (programmable boot sequencer) supports triggering sequences
>>> +  for clients upon request.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: qcom,pbs
>>> +
>>> +  reg:
>>> +    description: |
>>> +      Base address of the PBS peripheral.
>>> +    maxItems: 1
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    pmic {
>>> +      #address-cells = <1>;
>>> +      #size-cells = <0>;
>>> +
>>> +      qcom,pbs@7400 {
>>> +        compatible = "qcom,pbs";
>>> +        reg = <0x7400>;
>>> +      };
>>
>> Why do you need a child node for this? Is there more than 1 instance in
>> a PMIC? Every sub-function of a PMIC doesn't have to have a DT node.
>>
> 
> We currently have another downstream driver (which is planned to get upstreamed)
> which also needs a handle to a pbs device in order to properly trigger events.

Does it have to be a separate driver? Or is it a part of the LPG driver, 
just being artificially split away?

> 
>> Rob
> 
> 
> 

-- 
With best wishes
Dmitry

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

[Anjelique Melendez]

On 6/29/2023 1:45 AM, Dmitry Baryshkov wrote:
> On 29/06/2023 04:19, Anjelique Melendez wrote:
>>
>>
>> On 6/26/2023 6:58 AM, Rob Herring wrote:
>>> On Wed, Jun 21, 2023 at 11:59:45AM -0700, Anjelique Melendez wrote:
>>>> Add binding for the Qualcomm Programmable Boot Sequencer device.
>>>>
>>>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>>>> ---
>>>>   .../bindings/soc/qcom/qcom-pbs.yaml           | 41 +++++++++++++++++++
>>>>   1 file changed, 41 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..0a89c334f95c
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
>>>> @@ -0,0 +1,41 @@
>>>> +# 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. PBS
>>>> +
>>>> +maintainers:
>>>> +  - Anjelique Melendez <quic_amelende@quicinc.com>
>>>> +
>>>> +description: |
>>>> +  Qualcomm PBS (programmable boot sequencer) supports triggering sequences
>>>> +  for clients upon request.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: qcom,pbs
>>>> +
>>>> +  reg:
>>>> +    description: |
>>>> +      Base address of the PBS peripheral.
>>>> +    maxItems: 1
>>>> +
>>>> +required:
>>>> + - compatible
>>>> + - reg
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    pmic {
>>>> +      #address-cells = <1>;
>>>> +      #size-cells = <0>;
>>>> +
>>>> +      qcom,pbs@7400 {
>>>> +        compatible = "qcom,pbs";
>>>> +        reg = <0x7400>;
>>>> +      };
>>>
>>> Why do you need a child node for this? Is there more than 1 instance in
>>> a PMIC? Every sub-function of a PMIC doesn't have to have a DT node.
>>>
>>
>> We currently have another downstream driver (which is planned to get upstreamed)
>> which also needs a handle to a pbs device in order to properly trigger events.
> 
> Does it have to be a separate driver? Or is it a part of the LPG driver, just being artificially split away?

Sure, I just discussed with team and we are ok with removing this as a separate driver. Will have that 
for next version. 
> 
>>
>>> Rob
>>
>>
>>
> 

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

[Dmitry Baryshkov]

On 30/06/2023 00:53, Anjelique Melendez wrote:
> 
> 
> On 6/29/2023 1:45 AM, Dmitry Baryshkov wrote:
>> On 29/06/2023 04:19, Anjelique Melendez wrote:
>>>
>>>
>>> On 6/26/2023 6:58 AM, Rob Herring wrote:
>>>> On Wed, Jun 21, 2023 at 11:59:45AM -0700, Anjelique Melendez wrote:
>>>>> Add binding for the Qualcomm Programmable Boot Sequencer device.
>>>>>
>>>>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>>>>> ---
>>>>>    .../bindings/soc/qcom/qcom-pbs.yaml           | 41 +++++++++++++++++++
>>>>>    1 file changed, 41 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..0a89c334f95c
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
>>>>> @@ -0,0 +1,41 @@
>>>>> +# 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. PBS
>>>>> +
>>>>> +maintainers:
>>>>> +  - Anjelique Melendez <quic_amelende@quicinc.com>
>>>>> +
>>>>> +description: |
>>>>> +  Qualcomm PBS (programmable boot sequencer) supports triggering sequences
>>>>> +  for clients upon request.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: qcom,pbs
>>>>> +
>>>>> +  reg:
>>>>> +    description: |
>>>>> +      Base address of the PBS peripheral.
>>>>> +    maxItems: 1
>>>>> +
>>>>> +required:
>>>>> + - compatible
>>>>> + - reg
>>>>> +
>>>>> +additionalProperties: false
>>>>> +
>>>>> +examples:
>>>>> +  - |
>>>>> +    pmic {
>>>>> +      #address-cells = <1>;
>>>>> +      #size-cells = <0>;
>>>>> +
>>>>> +      qcom,pbs@7400 {
>>>>> +        compatible = "qcom,pbs";
>>>>> +        reg = <0x7400>;
>>>>> +      };
>>>>
>>>> Why do you need a child node for this? Is there more than 1 instance in
>>>> a PMIC? Every sub-function of a PMIC doesn't have to have a DT node.
>>>>
>>>
>>> We currently have another downstream driver (which is planned to get upstreamed)
>>> which also needs a handle to a pbs device in order to properly trigger events.
>>
>> Does it have to be a separate driver? Or is it a part of the LPG driver, just being artificially split away?
> 
> Sure, I just discussed with team and we are ok with removing this as a separate driver. Will have that
> for next version.

I saw that the PBS can also be used with the haptics device. Will it 
talk to the LPG driver?

>>
>>>
>>>> Rob
>>>
>>>
>>>
>>

-- 
With best wishes
Dmitry

Re: [PATCH 2/7] dt-bindings: leds: leds-qcom-lpg: Add support for LUT through NVMEM devices

[Krzysztof Kozlowski]

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

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

[Krzysztof Kozlowski]

On 29/06/2023 03:19, Anjelique Melendez wrote:

>>> +examples:
>>> +  - |
>>> +    pmic {
>>> +      #address-cells = <1>;
>>> +      #size-cells = <0>;
>>> +
>>> +      qcom,pbs@7400 {
>>> +        compatible = "qcom,pbs";
>>> +        reg = <0x7400>;
>>> +      };
>>
>> Why do you need a child node for this? Is there more than 1 instance in 
>> a PMIC? Every sub-function of a PMIC doesn't have to have a DT node.
>>
> 
> We currently have another downstream driver (which is planned to get upstreamed)
> which also needs a handle to a pbs device in order to properly trigger events. 

I don't see how does it answer Rob's concerns. Neither mine about
incomplete binding. You don't need pbs node here for that.

Anyway, whatever you have downstream also does not justify any changes.
Either upstream these so we can see it or drop this binding.

Best regards,
Krzysztof

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

[Anjelique Melendez]

On 7/1/2023 4:03 AM, Krzysztof Kozlowski wrote:
> On 29/06/2023 03:19, Anjelique Melendez wrote:
> 
>>>> +examples:
>>>> +  - |
>>>> +    pmic {
>>>> +      #address-cells = <1>;
>>>> +      #size-cells = <0>;
>>>> +
>>>> +      qcom,pbs@7400 {
>>>> +        compatible = "qcom,pbs";
>>>> +        reg = <0x7400>;
>>>> +      };
>>>
>>> Why do you need a child node for this? Is there more than 1 instance in 
>>> a PMIC? Every sub-function of a PMIC doesn't have to have a DT node.
>>>
>>
>> We currently have another downstream driver (which is planned to get upstreamed)
>> which also needs a handle to a pbs device in order to properly trigger events. 
> 
> I don't see how does it answer Rob's concerns. Neither mine about
> incomplete binding. You don't need pbs node here for that.
> 
> Anyway, whatever you have downstream also does not justify any changes.
> Either upstream these so we can see it or drop this binding.
> 
> Best regards,
> Krzysztof
> 

On PMI632, peripherals are partitioned over 2 different SIDs
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/pmi632.dtsi?h=v6.5-rc1#n42
and https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/pmi632.dtsi?h=v6.5-rc1#n149).
Unfortunately, the pbs peripheral and the lpg peripherals are on different
PMI632 devices and therefore have different regmaps.
 
If we get rid of the pbs node we need to get a handle to the proper regmap.
I see two possible options, we could either introduce a new client property
which points to a peripheral on the same device as pbs.

i.e.
	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-phandle = <&pmi632_gpios>;
      		..... 
	};
Then when client is probing could do something like the following to get the regmap

	dn = of_parse_phandle(node, "qcom,pbs-phandle", 0);
	pdev = of_find_device_by_node(dn);
	pbs_regmap = dev_get_regmap(&pdev->dev->parent, NULL);



Or we could use the nvmem phandle and just have something like this in client's probe

	dn = of_parse_phandle(node, "nvmem", 0);
	pdev = of_find_device_by_node(dn);
	pbs_regmap = dev_get_regmap(&pdev->dev->parent, NULL);



Let me know what your thoughts are on this.

Thanks,
Anjelique

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

[Krzysztof Kozlowski]

On 11/07/2023 05:52, Anjelique Melendez wrote:
> 
> 
> On 7/1/2023 4:03 AM, Krzysztof Kozlowski wrote:
>> On 29/06/2023 03:19, Anjelique Melendez wrote:
>>
>>>>> +examples:
>>>>> +  - |
>>>>> +    pmic {
>>>>> +      #address-cells = <1>;
>>>>> +      #size-cells = <0>;
>>>>> +
>>>>> +      qcom,pbs@7400 {
>>>>> +        compatible = "qcom,pbs";
>>>>> +        reg = <0x7400>;
>>>>> +      };
>>>>
>>>> Why do you need a child node for this? Is there more than 1 instance in 
>>>> a PMIC? Every sub-function of a PMIC doesn't have to have a DT node.
>>>>
>>>
>>> We currently have another downstream driver (which is planned to get upstreamed)
>>> which also needs a handle to a pbs device in order to properly trigger events. 
>>
>> I don't see how does it answer Rob's concerns. Neither mine about
>> incomplete binding. You don't need pbs node here for that.
>>
>> Anyway, whatever you have downstream also does not justify any changes.
>> Either upstream these so we can see it or drop this binding.
>>
>> Best regards,
>> Krzysztof
>>
> 
> On PMI632, peripherals are partitioned over 2 different SIDs
> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/pmi632.dtsi?h=v6.5-rc1#n42
> and https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/pmi632.dtsi?h=v6.5-rc1#n149).
> Unfortunately, the pbs peripheral and the lpg peripherals are on different
> PMI632 devices and therefore have different regmaps.
>  
> If we get rid of the pbs node we need to get a handle to the proper regmap.
> I see two possible options, we could either introduce a new client property
> which points to a peripheral on the same device as pbs.
> 
> i.e.
> 	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-phandle = <&pmi632_gpios>;
>       		..... 
> 	};
> Then when client is probing could do something like the following to get the regmap
> 
> 	dn = of_parse_phandle(node, "qcom,pbs-phandle", 0);
> 	pdev = of_find_device_by_node(dn);
> 	pbs_regmap = dev_get_regmap(&pdev->dev->parent, NULL);
> 
> 
> 
> Or we could use the nvmem phandle and just have something like this in client's probe
> 
> 	dn = of_parse_phandle(node, "nvmem", 0);
> 	pdev = of_find_device_by_node(dn);
> 	pbs_regmap = dev_get_regmap(&pdev->dev->parent, NULL);
> 
> 
> 
> Let me know what your thoughts are on this.

Rob asked you - "Is there more than 1 instance in a PMIC?" - and you did
not answer positively, just mentioned something about drivers in
downstream, which do not matter. So is the answer for that question:
yes, you have two instances of the same PMIC differing by presence of
PBS and other features"?

Best regards,
Krzysztof

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

[Anjelique Melendez]

On 7/10/2023 10:58 PM, Krzysztof Kozlowski wrote:
> On 11/07/2023 05:52, Anjelique Melendez wrote:
>>
>>
>> On 7/1/2023 4:03 AM, Krzysztof Kozlowski wrote:
>>> On 29/06/2023 03:19, Anjelique Melendez wrote:
>>>
>>>>>> +examples:
>>>>>> +  - |
>>>>>> +    pmic {
>>>>>> +      #address-cells = <1>;
>>>>>> +      #size-cells = <0>;
>>>>>> +
>>>>>> +      qcom,pbs@7400 {
>>>>>> +        compatible = "qcom,pbs";
>>>>>> +        reg = <0x7400>;
>>>>>> +      };
>>>>>
>>>>> Why do you need a child node for this? Is there more than 1 instance in 
>>>>> a PMIC? Every sub-function of a PMIC doesn't have to have a DT node.
>>>>>
>>>>
>>>> We currently have another downstream driver (which is planned to get upstreamed)
>>>> which also needs a handle to a pbs device in order to properly trigger events. 
>>>
>>> I don't see how does it answer Rob's concerns. Neither mine about
>>> incomplete binding. You don't need pbs node here for that.
>>>
>>> Anyway, whatever you have downstream also does not justify any changes.
>>> Either upstream these so we can see it or drop this binding.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> On PMI632, peripherals are partitioned over 2 different SIDs
>> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/pmi632.dtsi?h=v6.5-rc1#n42
>> and https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/pmi632.dtsi?h=v6.5-rc1#n149).
>> Unfortunately, the pbs peripheral and the lpg peripherals are on different
>> PMI632 devices and therefore have different regmaps.
>>  
>> If we get rid of the pbs node we need to get a handle to the proper regmap.
>> I see two possible options, we could either introduce a new client property
>> which points to a peripheral on the same device as pbs.
>>
>> i.e.
>> 	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-phandle = <&pmi632_gpios>;
>>       		..... 
>> 	};
>> Then when client is probing could do something like the following to get the regmap
>>
>> 	dn = of_parse_phandle(node, "qcom,pbs-phandle", 0);
>> 	pdev = of_find_device_by_node(dn);
>> 	pbs_regmap = dev_get_regmap(&pdev->dev->parent, NULL);
>>
>>
>>
>> Or we could use the nvmem phandle and just have something like this in client's probe
>>
>> 	dn = of_parse_phandle(node, "nvmem", 0);
>> 	pdev = of_find_device_by_node(dn);
>> 	pbs_regmap = dev_get_regmap(&pdev->dev->parent, NULL);
>>
>>
>>
>> Let me know what your thoughts are on this.
> 
> Rob asked you - "Is there more than 1 instance in a PMIC?" - and you did
> not answer positively, just mentioned something about drivers in
> downstream, which do not matter. So is the answer for that question:
> yes, you have two instances of the same PMIC differing by presence of
> PBS and other features"?
> 
Sorry that was a misunderstanding on my part.
Yes, answer to Rob's question should have been "We have two instances of PMI632,
where one instance holds the pbs peripheral and the other holds the lpg
peripherals. The child node for pbs is needed so lpg client can access
the PMI632 regmap which contains the pbs peripheral."

Thanks,
Anjelique 

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

[Krzysztof Kozlowski]

On 11/07/2023 22:12, Anjelique Melendez wrote:

>>>
>>> On PMI632, peripherals are partitioned over 2 different SIDs
>>> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/pmi632.dtsi?h=v6.5-rc1#n42
>>> and https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/pmi632.dtsi?h=v6.5-rc1#n149).
>>> Unfortunately, the pbs peripheral and the lpg peripherals are on different
>>> PMI632 devices and therefore have different regmaps.
>>>  
>>> If we get rid of the pbs node we need to get a handle to the proper regmap.
>>> I see two possible options, we could either introduce a new client property
>>> which points to a peripheral on the same device as pbs.
>>>
>>> i.e.
>>> 	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-phandle = <&pmi632_gpios>;
>>>       		..... 
>>> 	};
>>> Then when client is probing could do something like the following to get the regmap
>>>
>>> 	dn = of_parse_phandle(node, "qcom,pbs-phandle", 0);
>>> 	pdev = of_find_device_by_node(dn);
>>> 	pbs_regmap = dev_get_regmap(&pdev->dev->parent, NULL);
>>>
>>>
>>>
>>> Or we could use the nvmem phandle and just have something like this in client's probe
>>>
>>> 	dn = of_parse_phandle(node, "nvmem", 0);
>>> 	pdev = of_find_device_by_node(dn);
>>> 	pbs_regmap = dev_get_regmap(&pdev->dev->parent, NULL);
>>>
>>>
>>>
>>> Let me know what your thoughts are on this.
>>
>> Rob asked you - "Is there more than 1 instance in a PMIC?" - and you did
>> not answer positively, just mentioned something about drivers in
>> downstream, which do not matter. So is the answer for that question:
>> yes, you have two instances of the same PMIC differing by presence of
>> PBS and other features"?
>>
> Sorry that was a misunderstanding on my part.
> Yes, answer to Rob's question should have been "We have two instances of PMI632,
> where one instance holds the pbs peripheral and the other holds the lpg
> peripherals. The child node for pbs is needed so lpg client can access
> the PMI632 regmap which contains the pbs peripheral."

I guess I miss here something. What is "LPG client"? I don't understand
why this LPG client needs existence of PBS node, to be able to get the
regmap.

PBS is a child of PMIC, so it can get regmap from the parent. What's
more, which DT property passes the regmap from PMIC to LPG client?

Best regards,
Krzysztof

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

[Dmitry Baryshkov]

On Wed, 12 Jul 2023 at 17:22, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 11/07/2023 22:12, Anjelique Melendez wrote:
>
> >>>
> >>> On PMI632, peripherals are partitioned over 2 different SIDs
> >>> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/pmi632.dtsi?h=v6.5-rc1#n42
> >>> and https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/pmi632.dtsi?h=v6.5-rc1#n149).
> >>> Unfortunately, the pbs peripheral and the lpg peripherals are on different
> >>> PMI632 devices and therefore have different regmaps.
> >>>
> >>> If we get rid of the pbs node we need to get a handle to the proper regmap.
> >>> I see two possible options, we could either introduce a new client property
> >>> which points to a peripheral on the same device as pbs.
> >>>
> >>> i.e.
> >>>     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-phandle = <&pmi632_gpios>;
> >>>                     .....
> >>>     };
> >>> Then when client is probing could do something like the following to get the regmap
> >>>
> >>>     dn = of_parse_phandle(node, "qcom,pbs-phandle", 0);
> >>>     pdev = of_find_device_by_node(dn);
> >>>     pbs_regmap = dev_get_regmap(&pdev->dev->parent, NULL);
> >>>
> >>>
> >>>
> >>> Or we could use the nvmem phandle and just have something like this in client's probe
> >>>
> >>>     dn = of_parse_phandle(node, "nvmem", 0);
> >>>     pdev = of_find_device_by_node(dn);
> >>>     pbs_regmap = dev_get_regmap(&pdev->dev->parent, NULL);
> >>>
> >>>
> >>>
> >>> Let me know what your thoughts are on this.
> >>
> >> Rob asked you - "Is there more than 1 instance in a PMIC?" - and you did
> >> not answer positively, just mentioned something about drivers in
> >> downstream, which do not matter. So is the answer for that question:
> >> yes, you have two instances of the same PMIC differing by presence of
> >> PBS and other features"?
> >>
> > Sorry that was a misunderstanding on my part.
> > Yes, answer to Rob's question should have been "We have two instances of PMI632,
> > where one instance holds the pbs peripheral and the other holds the lpg
> > peripherals. The child node for pbs is needed so lpg client can access
> > the PMI632 regmap which contains the pbs peripheral."
>
> I guess I miss here something. What is "LPG client"? I don't understand
> why this LPG client needs existence of PBS node, to be able to get the
> regmap.
>
> PBS is a child of PMIC, so it can get regmap from the parent. What's
> more, which DT property passes the regmap from PMIC to LPG client?

There are some PMICs which claim two SPMI SIDs. For such PMICs, each
SID is a separate device, so it is not directly possible to get the
regmap of the other SID.

-- 
With best wishes
Dmitry

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

[Krzysztof Kozlowski]

On 12/07/2023 16:35, Dmitry Baryshkov wrote:
>>>> Rob asked you - "Is there more than 1 instance in a PMIC?" - and you did
>>>> not answer positively, just mentioned something about drivers in
>>>> downstream, which do not matter. So is the answer for that question:
>>>> yes, you have two instances of the same PMIC differing by presence of
>>>> PBS and other features"?
>>>>
>>> Sorry that was a misunderstanding on my part.
>>> Yes, answer to Rob's question should have been "We have two instances of PMI632,
>>> where one instance holds the pbs peripheral and the other holds the lpg
>>> peripherals. The child node for pbs is needed so lpg client can access
>>> the PMI632 regmap which contains the pbs peripheral."
>>
>> I guess I miss here something. What is "LPG client"? I don't understand
>> why this LPG client needs existence of PBS node, to be able to get the
>> regmap.
>>
>> PBS is a child of PMIC, so it can get regmap from the parent. What's
>> more, which DT property passes the regmap from PMIC to LPG client?
> 
> There are some PMICs which claim two SPMI SIDs. For such PMICs, each
> SID is a separate device, so it is not directly possible to get the
> regmap of the other SID.

OK, maybe after implementing all the review changes - including dropping
that singleton pattern - this will be clearer. Please send new version
and we will discuss it from there.

Thank you.
Best regards,
Krzysztof

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

[Anjelique Melendez]

On 7/12/2023 1:11 PM, Krzysztof Kozlowski wrote:
> On 12/07/2023 16:35, Dmitry Baryshkov wrote:
>>>>> Rob asked you - "Is there more than 1 instance in a PMIC?" - and you did
>>>>> not answer positively, just mentioned something about drivers in
>>>>> downstream, which do not matter. So is the answer for that question:
>>>>> yes, you have two instances of the same PMIC differing by presence of
>>>>> PBS and other features"?
>>>>>
>>>> Sorry that was a misunderstanding on my part.
>>>> Yes, answer to Rob's question should have been "We have two instances of PMI632,
>>>> where one instance holds the pbs peripheral and the other holds the lpg
>>>> peripherals. The child node for pbs is needed so lpg client can access
>>>> the PMI632 regmap which contains the pbs peripheral."
>>>
>>> I guess I miss here something. What is "LPG client"? I don't understand
>>> why this LPG client needs existence of PBS node, to be able to get the
>>> regmap.
>>>
>>> PBS is a child of PMIC, so it can get regmap from the parent. What's
>>> more, which DT property passes the regmap from PMIC to LPG client?
>>
>> There are some PMICs which claim two SPMI SIDs. For such PMICs, each
>> SID is a separate device, so it is not directly possible to get the
>> regmap of the other SID.
> 
> OK, maybe after implementing all the review changes - including dropping
> that singleton pattern - this will be clearer. Please send new version
> and we will discuss it from there.
> 
Sure, will work on getting that new version sent but I did just have clarifying question.
When you say "dropping that singleton pattern" are you referring to dropping the 
PBS node?
Want to make sure we are all on the same page with what the next version will include :)

Thanks,
Anjelique

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

[Krzysztof Kozlowski]

On 14/07/2023 22:32, Anjelique Melendez wrote:
> 
> 
> On 7/12/2023 1:11 PM, Krzysztof Kozlowski wrote:
>> On 12/07/2023 16:35, Dmitry Baryshkov wrote:
>>>>>> Rob asked you - "Is there more than 1 instance in a PMIC?" - and you did
>>>>>> not answer positively, just mentioned something about drivers in
>>>>>> downstream, which do not matter. So is the answer for that question:
>>>>>> yes, you have two instances of the same PMIC differing by presence of
>>>>>> PBS and other features"?
>>>>>>
>>>>> Sorry that was a misunderstanding on my part.
>>>>> Yes, answer to Rob's question should have been "We have two instances of PMI632,
>>>>> where one instance holds the pbs peripheral and the other holds the lpg
>>>>> peripherals. The child node for pbs is needed so lpg client can access
>>>>> the PMI632 regmap which contains the pbs peripheral."
>>>>
>>>> I guess I miss here something. What is "LPG client"? I don't understand
>>>> why this LPG client needs existence of PBS node, to be able to get the
>>>> regmap.
>>>>
>>>> PBS is a child of PMIC, so it can get regmap from the parent. What's
>>>> more, which DT property passes the regmap from PMIC to LPG client?
>>>
>>> There are some PMICs which claim two SPMI SIDs. For such PMICs, each
>>> SID is a separate device, so it is not directly possible to get the
>>> regmap of the other SID.
>>
>> OK, maybe after implementing all the review changes - including dropping
>> that singleton pattern - this will be clearer. Please send new version
>> and we will discuss it from there.
>>
> Sure, will work on getting that new version sent but I did just have clarifying question.
> When you say "dropping that singleton pattern" are you referring to dropping the 
> PBS node?
> Want to make sure we are all on the same page with what the next version will include :)

I was referring to my comments on driver, that you should not have
file-scope variable and get_pbs_client_device() iterating over global
list. It isn't singleton, actually, but the pattern in coding is very
similar to singleton approach.

Best regards,
Krzysztof

Re: [PATCH 0/7] Add support for LUT PPG

[Anjelique Melendez]

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 <luca.weiss@fairphone.com> # 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