In certain PMICs, LUT pattern and LPG configuration can be stored in SDAM
modules instead of LUT peripheral. This feature is called PPG.
This change series adds support for PPG. Thanks!
Changes since v1:
- Patch 1/7
- Fix dt_binding_check errors
- Update binding description
- Path 2/7
- Fix dt_binding_check errors
- Update per variant constraints
- Update nvmem description
- Patch 3/7
- Update get_pbs_client_device()
- Drop use of printk
- Remove unused function
Tested-by: Luca Weiss <[email protected]> # sdm632-fairphone-fp3 (pmi632)
Anjelique Melendez (7):
dt-bindings: soc: qcom: Add qcom-pbs bindings
dt-bindings: leds: leds-qcom-lpg: Add support for LPG PPG
soc: qcom: add QCOM PBS driver
leds: rgb: leds-qcom-lpg: Add support for PPG 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 | 92 +++-
.../bindings/soc/qcom/qcom-pbs.yaml | 40 ++
drivers/leds/rgb/leds-qcom-lpg.c | 395 ++++++++++++++++--
drivers/soc/qcom/Kconfig | 9 +
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/qcom-pbs.c | 302 +++++++++++++
include/linux/soc/qcom/qcom-pbs.h | 30 ++
7 files changed, 836 insertions(+), 33 deletions(-)
create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
create mode 100644 drivers/soc/qcom/qcom-pbs.c
create mode 100644 include/linux/soc/qcom/qcom-pbs.h
--
2.41.0
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 <[email protected]>
---
drivers/soc/qcom/Kconfig | 9 +
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/qcom-pbs.c | 302 ++++++++++++++++++++++++++++++
include/linux/soc/qcom/qcom-pbs.h | 30 +++
4 files changed, 342 insertions(+)
create mode 100644 drivers/soc/qcom/qcom-pbs.c
create mode 100644 include/linux/soc/qcom/qcom-pbs.h
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index e597799e8121..8cf690e46bf7 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -271,6 +271,15 @@ config QCOM_APR
used by audio driver to configure QDSP6
ASM, ADM and AFE modules.
+config QCOM_PBS
+ tristate "PBS trigger support for Qualcomm PMIC"
+ depends on SPMI
+ help
+ This driver supports configuring software programmable boot sequencer (PBS)
+ trigger event through PBS RAM on Qualcomm Technologies, Inc. PMICs.
+ This module provides the APIs to the client drivers that wants to send the
+ PBS trigger event to the PBS RAM.
+
config QCOM_ICC_BWMON
tristate "QCOM Interconnect Bandwidth Monitor driver"
depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 99114c71092b..3ffb04e2a275 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
+obj-$(CONFIG_QCOM_PBS) += qcom-pbs.o
obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
qcom_ice-objs += ice.o
obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += qcom_ice.o
diff --git a/drivers/soc/qcom/qcom-pbs.c b/drivers/soc/qcom/qcom-pbs.c
new file mode 100644
index 000000000000..73efef16650f
--- /dev/null
+++ b/drivers/soc/qcom/qcom-pbs.c
@@ -0,0 +1,302 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/regmap.h>
+#include <linux/spmi.h>
+#include <linux/soc/qcom/qcom-pbs.h>
+
+#define PBS_CLIENT_TRIG_CTL 0x42
+#define PBS_CLIENT_SW_TRIG_BIT BIT(7)
+#define PBS_CLIENT_SCRATCH1 0x50
+#define PBS_CLIENT_SCRATCH2 0x51
+
+struct pbs_dev {
+ struct device *dev;
+ struct regmap *regmap;
+ struct mutex lock;
+ struct device_link *link;
+
+ u32 base;
+};
+
+static int qcom_pbs_read(struct pbs_dev *pbs, u32 address, u8 *val)
+{
+ int ret;
+
+ address += pbs->base;
+ ret = regmap_bulk_read(pbs->regmap, address, val, 1);
+ if (ret)
+ dev_err(pbs->dev, "Failed to read address=%#x sid=%#x ret=%d\n",
+ address, to_spmi_device(pbs->dev->parent)->usid, ret);
+
+ return ret;
+}
+
+static int qcom_pbs_write(struct pbs_dev *pbs, u16 address, u8 val)
+{
+ int ret;
+
+ address += pbs->base;
+ ret = regmap_bulk_write(pbs->regmap, address, &val, 1);
+ if (ret < 0)
+ dev_err(pbs->dev, "Failed to write address=%#x sid=%#x ret=%d\n",
+ address, to_spmi_device(pbs->dev->parent)->usid, ret);
+
+ return ret;
+}
+
+static int qcom_pbs_masked_write(struct pbs_dev *pbs, u16 address, u8 mask, u8 val)
+{
+ int ret;
+
+ address += pbs->base;
+ ret = regmap_update_bits(pbs->regmap, address, mask, val);
+ if (ret < 0)
+ dev_err(pbs->dev, "Failed to write address=%#x ret=%d\n", address, ret);
+
+ return ret;
+}
+
+static int qcom_pbs_wait_for_ack(struct pbs_dev *pbs, u8 bit_pos)
+{
+ 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;
+
+ dev_err(pbs->dev, "NACK from PBS for bit %u\n", bit_pos);
+ return -EINVAL;
+ }
+
+ if (val & BIT(bit_pos)) {
+ dev_dbg(pbs->dev, "PBS sequence for bit %u executed!\n", bit_pos);
+ break;
+ }
+
+ usleep_range(delay, delay + 100);
+ }
+
+ if (!retries) {
+ dev_err(pbs->dev, "Timeout for PBS ACK/NACK for bit %u\n", bit_pos);
+ return -ETIMEDOUT;
+ }
+
+ return 0;
+}
+
+/**
+ * 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) {
+ dev_err(pbs->dev, "Invalid bitmap passed by client\n");
+ return -EINVAL;
+ }
+
+ if (IS_ERR_OR_NULL(pbs))
+ return -EINVAL;
+
+ mutex_lock(&pbs->lock);
+ ret = qcom_pbs_read(pbs, PBS_CLIENT_SCRATCH2, &val);
+ if (ret < 0)
+ goto out;
+
+ if (val == 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 platform_device *pdev;
+ struct pbs_dev *pbs;
+
+ pbs_dev_node = of_parse_phandle(dev->of_node, "qcom,pbs", 0);
+ if (!pbs_dev_node) {
+ dev_err(dev, "Missing qcom,pbs property\n");
+ return ERR_PTR(-ENODEV);
+ }
+
+ pdev = of_find_device_by_node(pbs_dev_node);
+ if (!pdev) {
+ dev_err(dev, "Unable to find PBS dev_node\n");
+ pbs = ERR_PTR(-EPROBE_DEFER);
+ goto out;
+ }
+
+ pbs = platform_get_drvdata(pdev);
+ if (!pbs) {
+ dev_err(dev, "Cannot get pbs instance from %s\n", dev_name(&pdev->dev));
+ platform_device_put(pdev);
+ pbs = ERR_PTR(-EINVAL);
+ goto out;
+ }
+
+ pbs->link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_SUPPLIER);
+ if (!pbs->link) {
+ dev_err(&pdev->dev, "Failed to create device link to consumer %s\n", dev_name(dev));
+ platform_device_put(pdev);
+ pbs = ERR_PTR(-EINVAL);
+ goto out;
+ }
+
+out:
+ of_node_put(pbs_dev_node);
+ return pbs;
+}
+EXPORT_SYMBOL(get_pbs_client_device);
+
+static int qcom_pbs_probe(struct platform_device *pdev)
+{
+ struct pbs_dev *pbs;
+ u32 val;
+ int ret;
+
+ pbs = devm_kzalloc(&pdev->dev, sizeof(*pbs), GFP_KERNEL);
+ if (!pbs)
+ return -ENOMEM;
+
+ pbs->dev = &pdev->dev;
+ pbs->regmap = dev_get_regmap(pbs->dev->parent, NULL);
+ if (!pbs->regmap) {
+ dev_err(pbs->dev, "Couldn't get parent's regmap\n");
+ return -EINVAL;
+ }
+
+ ret = device_property_read_u32(pbs->dev, "reg", &val);
+ if (ret < 0) {
+ dev_err(pbs->dev, "Couldn't find reg, ret = %d\n", ret);
+ return ret;
+ }
+ pbs->base = val;
+ mutex_init(&pbs->lock);
+
+ platform_set_drvdata(pdev, pbs);
+
+ return 0;
+}
+
+static const struct of_device_id qcom_pbs_match_table[] = {
+ { .compatible = "qcom,pmi632-pbs" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, qcom_pbs_match_table);
+
+static struct platform_driver qcom_pbs_driver = {
+ .driver = {
+ .name = "qcom-pbs",
+ .of_match_table = qcom_pbs_match_table,
+ },
+ .probe = qcom_pbs_probe,
+};
+module_platform_driver(qcom_pbs_driver)
+
+MODULE_DESCRIPTION("QCOM PBS DRIVER");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/soc/qcom/qcom-pbs.h b/include/linux/soc/qcom/qcom-pbs.h
new file mode 100644
index 000000000000..8a46209ccf13
--- /dev/null
+++ b/include/linux/soc/qcom/qcom-pbs.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef _QCOM_PBS_H
+#define _QCOM_PBS_H
+
+#include <linux/errno.h>
+#include <linux/types.h>
+
+struct device_node;
+struct pbs_dev;
+
+#if IS_ENABLED(CONFIG_QCOM_PBS)
+int qcom_pbs_trigger_event(struct pbs_dev *pbs, u8 bitmap);
+struct pbs_dev *get_pbs_client_device(struct device *client_dev);
+#else
+static inline int qcom_pbs_trigger_event(struct pbs_dev *pbs, u8 bitmap)
+{
+ return -ENODEV;
+}
+
+static inline struct pbs_dev *get_pbs_client_device(struct device *client_dev)
+{
+ return ERR_PTR(-ENODEV);
+}
+#endif
+
+#endif
--
2.41.0
Add binding for the Qualcomm Programmable Boot Sequencer device.
Signed-off-by: Anjelique Melendez <[email protected]>
---
.../bindings/soc/qcom/qcom-pbs.yaml | 40 +++++++++++++++++++
1 file changed, 40 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..753a3e9cc5fe
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
@@ -0,0 +1,40 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/qcom/qcom-pbs.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies, Inc. Programmable Boot Sequencer
+
+maintainers:
+ - Anjelique Melendez <[email protected]>
+
+description: |
+ The Qualcomm Technologies, Inc. Programmable Boot Sequencer (PBS)
+ supports triggering power up and power down sequences for clients
+ upon request.
+
+properties:
+ compatible:
+ const: qcom,pmi632-pbs
+
+ reg:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ pmic@0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ pbs@7400 {
+ compatible = "qcom,pmi632-pbs";
+ reg = <0x7400>;
+ };
+ };
--
2.41.0
On PMICs such as PM8350C, the lookup table containing the pattern data
is stored in a separate nvmem device from the one where the per-channel
data is stored.
Add support for two separate nvmems to handle this case while maintaining
backward compatibility for those targets that use only a single nvmem
device.
Signed-off-by: Guru Das Srinagesh <[email protected]>
Signed-off-by: Anjelique Melendez <[email protected]>
---
drivers/leds/rgb/leds-qcom-lpg.c | 112 ++++++++++++++++++++++++-------
1 file changed, 89 insertions(+), 23 deletions(-)
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index 822c7bff00df..f3f83925ab41 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 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.41.0
Update the pm8350c lpg_data struct so that pm8350c devices are treated as
PWM devices that support two-nvmem PPG scheme.
Signed-off-by: Anjelique Melendez <[email protected]>
---
drivers/leds/rgb/leds-qcom-lpg.c | 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 f3f83925ab41..bd54b023d509 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.41.0
Update leds-qcom-lpg bindings to support LPG PPG.
Signed-off-by: Anjelique Melendez <[email protected]>
---
.../bindings/leds/leds-qcom-lpg.yaml | 92 ++++++++++++++++++-
1 file changed, 91 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
index e6f1999cb22f..6feca859fb74 100644
--- a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
@@ -11,7 +11,7 @@ maintainers:
description: >
The Qualcomm Light Pulse Generator consists of three different hardware blocks;
- a ramp generator with lookup table, the light pulse generator and a three
+ a ramp generator with lookup table (LUT), the light pulse generator and a three
channel current sink. These blocks are found in a wide range of Qualcomm PMICs.
properties:
@@ -63,6 +63,27 @@ properties:
- description: dtest line to attach
- description: flags for the attachment
+ nvmem:
+ description: >
+ This property is required for PMICs that supports PPG, which is when a
+ PMIC stores LPG per-channel data and pattern LUT in SDAM modules instead
+ of in a LUT peripheral. For PMICs, such as PM8350C, per-channel data
+ and pattern LUT is separated into 2 SDAM modules. In that case, phandles
+ to both SDAM modules need to be specified.
+ minItems: 1
+ maxItems: 2
+
+ nvmem-names:
+ minItems: 1
+ maxItems: 2
+
+ qcom,pbs:
+ $ref: /schemas/types.yaml#/definitions/phandle
+ description: >
+ Phandle of the Qualcomm Programmable Boot Sequencer node (PBS).
+ PBS node is used to trigger LPG pattern sequences for PMICs that support
+ single SDAM PPG.
+
multi-led:
type: object
$ref: leds-class-multicolor.yaml#
@@ -106,6 +127,44 @@ required:
additionalProperties: false
+allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: qcom,pmi632-lpg
+ then:
+ properties:
+ nvmem:
+ maxItems: 1
+ nvmem-names:
+ items:
+ - const: lpg_chan_sdam
+ qcom,pbs:
+ maxItems: 1
+ required:
+ - nvmem
+ - nvmem-names
+ - qcom,pbs
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,pm8350c-pwm
+ - qcom,pm8550-pwm
+ then:
+ properties:
+ nvmem:
+ minItems: 2
+ nvmem-names:
+ items:
+ - const: lpg_chan_sdam
+ - const: lut_sdam
+ required:
+ - nvmem
+ - nvmem-names
+
examples:
- |
#include <dt-bindings/leds/common.h>
@@ -191,4 +250,35 @@ examples:
compatible = "qcom,pm8916-pwm";
#pwm-cells = <2>;
};
+ - |
+ #include <dt-bindings/leds/common.h>
+
+ led-controller {
+ compatible = "qcom,pmi632-lpg";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ #pwm-cells = <2>;
+ nvmem-names = "lpg_chan_sdam";
+ nvmem = <&pmi632_sdam_7>;
+ qcom,pbs = <&pmi632_pbs_client3>;
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_RED>;
+ label = "red";
+ };
+
+ led@2 {
+ reg = <2>;
+ color = <LED_COLOR_ID_GREEN>;
+ label = "green";
+ };
+
+ led@3 {
+ reg = <3>;
+ color = <LED_COLOR_ID_BLUE>;
+ label = "blue";
+ };
+ };
+
...
--
2.41.0
On Tue, 25 Jul 2023 12:34:17 -0700, Anjelique Melendez wrote:
> Add binding for the Qualcomm Programmable Boot Sequencer device.
>
> Signed-off-by: Anjelique Melendez <[email protected]>
> ---
> .../bindings/soc/qcom/qcom-pbs.yaml | 40 +++++++++++++++++++
> 1 file changed, 40 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:
dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/soc/qcom/qcom-pbs.example.dts:18.16-26.11: Warning (unit_address_vs_reg): /example-0/pmic@0: node has a unit name, but no reg or ranges property
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
On 25/07/2023 21:34, Anjelique Melendez wrote:
> Add binding for the Qualcomm Programmable Boot Sequencer device.
>
> Signed-off-by: Anjelique Melendez <[email protected]>
> ---
> .../bindings/soc/qcom/qcom-pbs.yaml | 40 +++++++++++++++++++
> 1 file changed, 40 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
Again not tested.
Also, you missed comments. :(
This is a friendly reminder during the review process.
It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.
Thank you.
Best regards,
Krzysztof
On 25.07.2023 21:34, 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 <[email protected]>
> ---
[...]
> +
> + u32 base;
> +};
> +
> +static int qcom_pbs_read(struct pbs_dev *pbs, u32 address, u8 *val)
> +{
> + int ret;
> +
> + address += pbs->base;
Any reason not to just keep the base address in struct pbs_dev and use
normal regmap r/w helpers?
[...]
> +
> +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) {
This should be a constant, not a magic value
> + /* PBS error - clear SCRATCH2 register */
> + ret = qcom_pbs_write(pbs, PBS_CLIENT_SCRATCH2, 0);
> + if (ret < 0)
> + return ret;
> +
> + dev_err(pbs->dev, "NACK from PBS for bit %u\n", bit_pos);
> + return -EINVAL;
> + }
> +
> + if (val & BIT(bit_pos)) {
> + dev_dbg(pbs->dev, "PBS sequence for bit %u executed!\n", bit_pos);
> + break;
> + }
> +
> + usleep_range(delay, delay + 100);
So worst case scenario this will wait for over 2 seconds?
> + }
> +
> + if (!retries) {
> + dev_err(pbs->dev, "Timeout for PBS ACK/NACK for bit %u\n", bit_pos);
> + return -ETIMEDOUT;
> + }
> +
> + return 0;
return 0 instead of break above?
> +}
> +
> +/**
> + * 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) {
> + dev_err(pbs->dev, "Invalid bitmap passed by client\n");
> + return -EINVAL;
> + }
> +
> + if (IS_ERR_OR_NULL(pbs))
> + return -EINVAL;
> +
> + mutex_lock(&pbs->lock);
> + ret = qcom_pbs_read(pbs, PBS_CLIENT_SCRATCH2, &val);
> + if (ret < 0)
> + goto out;
> +
> + if (val == 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.
> + */
Don't think the "in the X register" parts are useful.
> + 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);
You're using mask/val for half the function calls..
Stick with one approach.
[...]
> +struct pbs_dev *get_pbs_client_device(struct device *dev)
> +{
> + struct device_node *pbs_dev_node;
> + struct platform_device *pdev;
> + struct pbs_dev *pbs;
> +
> + pbs_dev_node = of_parse_phandle(dev->of_node, "qcom,pbs", 0);
> + if (!pbs_dev_node) {
> + dev_err(dev, "Missing qcom,pbs property\n");
> + return ERR_PTR(-ENODEV);
> + }
> +
> + pdev = of_find_device_by_node(pbs_dev_node);
> + if (!pdev) {
> + dev_err(dev, "Unable to find PBS dev_node\n");
> + pbs = ERR_PTR(-EPROBE_DEFER);
> + goto out;
> + }
> +
> + pbs = platform_get_drvdata(pdev);
> + if (!pbs) {
This check seems unnecessary, the PBS driver would have had to fail
probing if set_drvdata never got called.
Konrad
Hi Anjelique,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Anjelique-Melendez/dt-bindings-soc-qcom-Add-qcom-pbs-bindings/20230726-034011
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230725193423.25047-4-quic_amelende%40quicinc.com
patch subject: [PATCH v2 3/7] soc: qcom: add QCOM PBS driver
config: parisc-randconfig-m041-20230726 (https://download.01.org/0day-ci/archive/20230727/[email protected]/config)
compiler: hppa-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230727/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Closes: https://lore.kernel.org/r/[email protected]/
smatch warnings:
drivers/soc/qcom/qcom-pbs.c:97 qcom_pbs_wait_for_ack() warn: should this be 'retries == -1'
vim +97 drivers/soc/qcom/qcom-pbs.c
c261225d90e1d3 Anjelique Melendez 2023-07-25 68 static int qcom_pbs_wait_for_ack(struct pbs_dev *pbs, u8 bit_pos)
c261225d90e1d3 Anjelique Melendez 2023-07-25 69 {
c261225d90e1d3 Anjelique Melendez 2023-07-25 70 u16 retries = 2000, delay = 1000;
c261225d90e1d3 Anjelique Melendez 2023-07-25 71 int ret;
c261225d90e1d3 Anjelique Melendez 2023-07-25 72 u8 val;
c261225d90e1d3 Anjelique Melendez 2023-07-25 73
c261225d90e1d3 Anjelique Melendez 2023-07-25 74 while (retries--) {
Change this to while (--retries) {
c261225d90e1d3 Anjelique Melendez 2023-07-25 75 ret = qcom_pbs_read(pbs, PBS_CLIENT_SCRATCH2, &val);
c261225d90e1d3 Anjelique Melendez 2023-07-25 76 if (ret < 0)
c261225d90e1d3 Anjelique Melendez 2023-07-25 77 return ret;
c261225d90e1d3 Anjelique Melendez 2023-07-25 78
c261225d90e1d3 Anjelique Melendez 2023-07-25 79 if (val == 0xFF) {
c261225d90e1d3 Anjelique Melendez 2023-07-25 80 /* PBS error - clear SCRATCH2 register */
c261225d90e1d3 Anjelique Melendez 2023-07-25 81 ret = qcom_pbs_write(pbs, PBS_CLIENT_SCRATCH2, 0);
c261225d90e1d3 Anjelique Melendez 2023-07-25 82 if (ret < 0)
c261225d90e1d3 Anjelique Melendez 2023-07-25 83 return ret;
c261225d90e1d3 Anjelique Melendez 2023-07-25 84
c261225d90e1d3 Anjelique Melendez 2023-07-25 85 dev_err(pbs->dev, "NACK from PBS for bit %u\n", bit_pos);
c261225d90e1d3 Anjelique Melendez 2023-07-25 86 return -EINVAL;
c261225d90e1d3 Anjelique Melendez 2023-07-25 87 }
c261225d90e1d3 Anjelique Melendez 2023-07-25 88
c261225d90e1d3 Anjelique Melendez 2023-07-25 89 if (val & BIT(bit_pos)) {
c261225d90e1d3 Anjelique Melendez 2023-07-25 90 dev_dbg(pbs->dev, "PBS sequence for bit %u executed!\n", bit_pos);
c261225d90e1d3 Anjelique Melendez 2023-07-25 91 break;
c261225d90e1d3 Anjelique Melendez 2023-07-25 92 }
c261225d90e1d3 Anjelique Melendez 2023-07-25 93
c261225d90e1d3 Anjelique Melendez 2023-07-25 94 usleep_range(delay, delay + 100);
c261225d90e1d3 Anjelique Melendez 2023-07-25 95 }
c261225d90e1d3 Anjelique Melendez 2023-07-25 96
c261225d90e1d3 Anjelique Melendez 2023-07-25 @97 if (!retries) {
Otherwise this check needs to be: "if (retries == USHRT_MAX)".
Btw, I really feel like people are generally better off declaring list
iterators as int whenever possible. I have written a very rude blog
to that effect.
https://staticthinking.wordpress.com/2022/06/01/unsigned-int-i-is-stupid/
c261225d90e1d3 Anjelique Melendez 2023-07-25 98 dev_err(pbs->dev, "Timeout for PBS ACK/NACK for bit %u\n", bit_pos);
c261225d90e1d3 Anjelique Melendez 2023-07-25 99 return -ETIMEDOUT;
c261225d90e1d3 Anjelique Melendez 2023-07-25 100 }
c261225d90e1d3 Anjelique Melendez 2023-07-25 101
c261225d90e1d3 Anjelique Melendez 2023-07-25 102 return 0;
c261225d90e1d3 Anjelique Melendez 2023-07-25 103 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On 7/26/2023 12:53 AM, Krzysztof Kozlowski wrote:
> On 25/07/2023 21:34, Anjelique Melendez wrote:
>> Add binding for the Qualcomm Programmable Boot Sequencer device.
>>
>> Signed-off-by: Anjelique Melendez <[email protected]>
>> ---
>> .../bindings/soc/qcom/qcom-pbs.yaml | 40 +++++++++++++++++++
>> 1 file changed, 40 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
>
>
> Again not tested.
>
> Also, you missed comments. :(
>
> This is a friendly reminder during the review process.
>
> It seems my previous comments were not fully addressed. Maybe my
> feedback got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all
> requested changes or keep discussing them.
>
> Thank you.
>
>
> Best regards,
> Krzysztof
>
Hi Krzysztof,
Sorry about the testing, found that my dt_binding_checker was out dated and that
is why it has not been picking up those dt_binding errors :/
I went back to take a look at the original comments I missed and just wanted to
list them for a quick double check.
1. Rename binding to be qcom,pbs so that it matches compatible
2. Include Soc specific compatibles i.e.
compatible:
items:
- enum:
- qcom,pmi632-pbs
- const: qcom,pbs
3. Fix the example node
Thanks,
Anjelique
On 7/25/2023 12:34 PM, Anjelique Melendez wrote:
> +out:
> + mutex_unlock(&pbs->lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(qcom_pbs_trigger_event);
EXPORT_SYMBOL_GPL only please.
--
---Trilok Soni
On Wed, Jul 26, 2023 at 05:36:08PM +0200, Konrad Dybcio wrote:
> On 25.07.2023 21:34, Anjelique Melendez wrote:
> > +struct pbs_dev *get_pbs_client_device(struct device *dev)
> > +{
> > + struct device_node *pbs_dev_node;
> > + struct platform_device *pdev;
> > + struct pbs_dev *pbs;
> > +
> > + pbs_dev_node = of_parse_phandle(dev->of_node, "qcom,pbs", 0);
> > + if (!pbs_dev_node) {
> > + dev_err(dev, "Missing qcom,pbs property\n");
> > + return ERR_PTR(-ENODEV);
> > + }
> > +
> > + pdev = of_find_device_by_node(pbs_dev_node);
> > + if (!pdev) {
> > + dev_err(dev, "Unable to find PBS dev_node\n");
> > + pbs = ERR_PTR(-EPROBE_DEFER);
> > + goto out;
> > + }
> > +
> > + pbs = platform_get_drvdata(pdev);
> > + if (!pbs) {
> This check seems unnecessary, the PBS driver would have had to fail
> probing if set_drvdata never got called.
>
That's not necessarily the case, the platform_device will exist before
the probe function has been invoked. So checking this sounds
appropriate.
But if we have a valid link, but no drvdata, perhaps it would be more
appropriate to return -EPROBE_DEFER?
Regards,
Bjorn
On 7/26/2023 8:36 AM, Konrad Dybcio wrote:
> On 25.07.2023 21:34, 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 <[email protected]>
>> ---
> [...]
>
>> +
>> + u32 base;
>> +};
>> +
>> +static int qcom_pbs_read(struct pbs_dev *pbs, u32 address, u8 *val)
>> +{
>> + int ret;
>> +
>> + address += pbs->base;
> Any reason not to just keep the base address in struct pbs_dev and use
> normal regmap r/w helpers?
>
> [...]
We created the qcom_pbs read/write helpers to limit code duplication when printing
error messages.
I am ok with calling regmap_bulk_read/write() and regmap_update_bits()
in code instead of these helpers but wondering how everyone would feel with the error messages
either being duplicated or if error messages should just be removed?
qcom_pbs_read() is called twice, qcom_pbs_write() is called twice(), and
qcom_pbs_masked_write() is called 6 times.
>
>> +
>> +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) {
> This should be a constant, not a magic value
ack
>
>> + /* PBS error - clear SCRATCH2 register */
>> + ret = qcom_pbs_write(pbs, PBS_CLIENT_SCRATCH2, 0);
>> + if (ret < 0)
>> + return ret;
>> +
>> + dev_err(pbs->dev, "NACK from PBS for bit %u\n", bit_pos);
>> + return -EINVAL;
>> + }
>> +
>> + if (val & BIT(bit_pos)) {
>> + dev_dbg(pbs->dev, "PBS sequence for bit %u executed!\n", bit_pos);
>> + break;
>> + }
>> +
>> + usleep_range(delay, delay + 100);
> So worst case scenario this will wait for over 2 seconds?
Yes, worst case scenario will result in waiting for 2.2 seconds
>
>> + }
>> +
>> + if (!retries) {
>> + dev_err(pbs->dev, "Timeout for PBS ACK/NACK for bit %u\n", bit_pos);
>> + return -ETIMEDOUT;
>> + }
>> +
>> + return 0;
> return 0 instead of break above?
ack
>
>> +}
>> +
>> +/**
>> + * 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) {
>> + dev_err(pbs->dev, "Invalid bitmap passed by client\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (IS_ERR_OR_NULL(pbs))
>> + return -EINVAL;
>> +
>> + mutex_lock(&pbs->lock);
>> + ret = qcom_pbs_read(pbs, PBS_CLIENT_SCRATCH2, &val);
>> + if (ret < 0)
>> + goto out;
>> +
>> + if (val == 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.
>> + */
> Don't think the "in the X register" parts are useful.
ack
>
>> + 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);
> You're using mask/val for half the function calls..
> Stick with one approach.
ack
>
> [...]
>
>> +struct pbs_dev *get_pbs_client_device(struct device *dev)
>> +{
>> + struct device_node *pbs_dev_node;
>> + struct platform_device *pdev;
>> + struct pbs_dev *pbs;
>> +
>> + pbs_dev_node = of_parse_phandle(dev->of_node, "qcom,pbs", 0);
>> + if (!pbs_dev_node) {
>> + dev_err(dev, "Missing qcom,pbs property\n");
>> + return ERR_PTR(-ENODEV);
>> + }
>> +
>> + pdev = of_find_device_by_node(pbs_dev_node);
>> + if (!pdev) {
>> + dev_err(dev, "Unable to find PBS dev_node\n");
>> + pbs = ERR_PTR(-EPROBE_DEFER);
>> + goto out;
>> + }
>> +
>> + pbs = platform_get_drvdata(pdev);
>> + if (!pbs) {
> This check seems unnecessary, the PBS driver would have had to fail
> probing if set_drvdata never got called.
> > Konrad
On Tue, Jul 25, 2023 at 12:34:18PM -0700, Anjelique Melendez wrote:
> Update leds-qcom-lpg bindings to support LPG PPG.
>
> Signed-off-by: Anjelique Melendez <[email protected]>
> ---
> .../bindings/leds/leds-qcom-lpg.yaml | 92 ++++++++++++++++++-
> 1 file changed, 91 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> index e6f1999cb22f..6feca859fb74 100644
> --- a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> +++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> @@ -11,7 +11,7 @@ maintainers:
>
> description: >
> The Qualcomm Light Pulse Generator consists of three different hardware blocks;
> - a ramp generator with lookup table, the light pulse generator and a three
> + a ramp generator with lookup table (LUT), the light pulse generator and a three
> channel current sink. These blocks are found in a wide range of Qualcomm PMICs.
>
> properties:
> @@ -63,6 +63,27 @@ properties:
> - description: dtest line to attach
> - description: flags for the attachment
>
> + nvmem:
> + description: >
> + This property is required for PMICs that supports PPG, which is when a
> + PMIC stores LPG per-channel data and pattern LUT in SDAM modules instead
> + of in a LUT peripheral. For PMICs, such as PM8350C, per-channel data
> + and pattern LUT is separated into 2 SDAM modules. In that case, phandles
> + to both SDAM modules need to be specified.
> + minItems: 1
> + maxItems: 2
> +
> + nvmem-names:
> + minItems: 1
> + maxItems: 2
> +
> + qcom,pbs:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description: >
> + Phandle of the Qualcomm Programmable Boot Sequencer node (PBS).
> + PBS node is used to trigger LPG pattern sequences for PMICs that support
> + single SDAM PPG.
> +
> multi-led:
> type: object
> $ref: leds-class-multicolor.yaml#
> @@ -106,6 +127,44 @@ required:
>
> additionalProperties: false
>
> +allOf:
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: qcom,pmi632-lpg
> + then:
> + properties:
> + nvmem:
> + maxItems: 1
> + nvmem-names:
> + items:
> + - const: lpg_chan_sdam
> + qcom,pbs:
> + maxItems: 1
> + required:
> + - nvmem
> + - nvmem-names
> + - qcom,pbs
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - qcom,pm8350c-pwm
> + - qcom,pm8550-pwm
> + then:
> + properties:
> + nvmem:
> + minItems: 2
> + nvmem-names:
> + items:
> + - const: lpg_chan_sdam
> + - const: lut_sdam
This can go into the main section and then here you just say
'minItems: 2'. And similar for the 1st if/then.
> + required:
> + - nvmem
> + - nvmem-names
Looks like these are always required.
> +
> examples:
> - |
> #include <dt-bindings/leds/common.h>
> @@ -191,4 +250,35 @@ examples:
> compatible = "qcom,pm8916-pwm";
> #pwm-cells = <2>;
> };
> + - |
> + #include <dt-bindings/leds/common.h>
> +
> + led-controller {
> + compatible = "qcom,pmi632-lpg";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + #pwm-cells = <2>;
> + nvmem-names = "lpg_chan_sdam";
> + nvmem = <&pmi632_sdam_7>;
> + qcom,pbs = <&pmi632_pbs_client3>;
> +
> + led@1 {
> + reg = <1>;
> + color = <LED_COLOR_ID_RED>;
> + label = "red";
> + };
> +
> + led@2 {
> + reg = <2>;
> + color = <LED_COLOR_ID_GREEN>;
> + label = "green";
> + };
> +
> + led@3 {
> + reg = <3>;
> + color = <LED_COLOR_ID_BLUE>;
> + label = "blue";
> + };
> + };
> +
> ...
> --
> 2.41.0
>
On 8/2/2023 5:25 PM, Rob Herring wrote:
> On Tue, Jul 25, 2023 at 12:34:18PM -0700, Anjelique Melendez wrote:
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + enum:
>> + - qcom,pm8350c-pw
>> + - qcom,pm8550-pwm
>> + then:
>> + properties:
>> + nvmem:
>> + minItems: 2
>> + nvmem-names:
>> + items:
>> + - const: lpg_chan_sdam
>> + - const: lut_sdam
>
> This can go into the main section and then here you just say
> 'minItems: 2'. And similar for the 1st if/then.
>
ACK
>> + required:
>> + - nvmem
>> + - nvmem-names
>
> Looks like these are always required.
These are only required for the compatibles properties that do not
have lut peripherals. Right now this is is only for qcom,pmi632-lpg,
qcom,pm8350c-pwm and qcom,pm8550-pwm.