In certain PMICs, LUT pattern and LPG configuration is stored in SDAM
modules instead of LUT peripheral. This feature is called PPG.
This change series adds support for PPG. Thanks!
Changes since v7:
- Patch 4/7
- Initialize hi/lo_pause variables in lpg_pattern_set()
Changes since v6:
- Patch 2/7
- Removed required by constraint on PPG dt properties
Changes since v5:
- Patch 4/7
- Update logic so that multicolor led device triggers pattern
on all LEDs at the same time
- Update nitpicks from Lee
- Patch 5/7
- Update nitpicks from Lee
Changes since v4:
- Patch 3/7
- Get rid of r/w helpers
- Use regmap_read_poll_timeout() in qcom_pbs_wait_for_ack()
- Update error path in qcom_pbs_trigger_event()
- Fix reverse christmas tree
- Patch 4/7
- Get rid of r/w helpers
- Update variables to use "sdam" instead of "nvmem"
- Fix comments
- Fix reverse christmas tree
- Update lpg_pattern_set() logic
- Patch 5/7
- Removed sdam_lut_base from lpg_data
Changes since v3:
- Patch 4/7
- Fix function returns
- Move register definition to top of file
- Revert max_brightness and probe accidental changes
- Combine init_sdam() and parse_sdam()
- Change error prints in probe to use dev_err_probe
- Remove ppg_en variable
- Update when pbs triggers are set/cleared
- Patch 6/7
- Remove use of nvmem_count
- Move register definition to top of file
- Remove lpg_get_sdam_lut_idx()
Changes since v2:
- Patch 1/7
- Fix dt_binding_check error
- Rename binding file to match compatible
- Iclude SoC specific comptaibles
- Patch 2/7
- Update nvmem-names list
- Patch 3/7
- Update EXPORT_SYMBOL to EXPORT_SYMBOL_GPL
- Fix return/break logic in qcom_pbs_wait_for_ack()
- Update iterators to be int
- Add constants
- Fix function calls in qcom_pbs_trigger_event()
- Remove unnessary comments
- Return -EPROBE_DEFER from get_pbs_client_device()
Changes since v1:
- Patch 1/7
- Fix dt_binding_check errors
- Update binding description
- Path 2/7
- Fix dt_binding_check errors
- Update per variant constraints
- Update nvmem description
- Patch 3/7
- Update get_pbs_client_device()
- Drop use of printk
- Remove unused function
Anjelique Melendez (7):
dt-bindings: soc: qcom: Add qcom,pbs bindings
dt-bindings: leds: leds-qcom-lpg: Add support for LPG PPG
soc: qcom: add QCOM PBS driver
leds: rgb: leds-qcom-lpg: Add support for PPG through single SDAM
leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG
leds: rgb: leds-qcom-lpg: Include support for PPG with dedicated LUT
SDAM
leds: rgb: Update PM8350C lpg_data to support two-nvmem PPG Scheme
.../bindings/leds/leds-qcom-lpg.yaml | 82 ++++-
.../bindings/soc/qcom/qcom,pbs.yaml | 46 +++
drivers/leds/rgb/leds-qcom-lpg.c | 348 ++++++++++++++++--
drivers/soc/qcom/Kconfig | 9 +
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/qcom-pbs.c | 243 ++++++++++++
include/linux/soc/qcom/qcom-pbs.h | 30 ++
7 files changed, 728 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.41.0
Update leds-qcom-lpg binding to support LPG PPG.
Signed-off-by: Anjelique Melendez <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
---
.../bindings/leds/leds-qcom-lpg.yaml | 82 ++++++++++++++++++-
1 file changed, 81 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
index ea84ad426df1..6649ca2ec805 100644
--- a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
@@ -11,7 +11,7 @@ maintainers:
description: >
The Qualcomm Light Pulse Generator consists of three different hardware blocks;
- a ramp generator with lookup table, the light pulse generator and a three
+ a ramp generator with lookup table (LUT), the light pulse generator and a three
channel current sink. These blocks are found in a wide range of Qualcomm PMICs.
properties:
@@ -63,6 +63,29 @@ properties:
- description: dtest line to attach
- description: flags for the attachment
+ nvmem:
+ description: >
+ This property is required for PMICs that supports PPG, which is when a
+ PMIC stores LPG per-channel data and pattern LUT in SDAM modules instead
+ of in a LUT peripheral. For PMICs, such as PM8350C, per-channel data
+ and pattern LUT is separated into 2 SDAM modules. In that case, phandles
+ to both SDAM modules need to be specified.
+ minItems: 1
+ maxItems: 2
+
+ nvmem-names:
+ minItems: 1
+ items:
+ - const: lpg_chan_sdam
+ - const: lut_sdam
+
+ qcom,pbs:
+ $ref: /schemas/types.yaml#/definitions/phandle
+ description: >
+ Phandle of the Qualcomm Programmable Boot Sequencer node (PBS).
+ PBS node is used to trigger LPG pattern sequences for PMICs that support
+ single SDAM PPG.
+
multi-led:
type: object
$ref: leds-class-multicolor.yaml#
@@ -106,6 +129,32 @@ required:
additionalProperties: false
+allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: qcom,pmi632-lpg
+ then:
+ properties:
+ nvmem:
+ maxItems: 1
+ nvmem-names:
+ maxItems: 1
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,pm8350c-pwm
+ - qcom,pm8550-pwm
+ then:
+ properties:
+ nvmem:
+ minItems: 2
+ nvmem-names:
+ minItems: 2
+
examples:
- |
#include <dt-bindings/leds/common.h>
@@ -191,4 +240,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
Update the pmi632 lpg_data struct so that pmi632 devices use PPG
for LUT pattern.
Signed-off-by: Anjelique Melendez <[email protected]>
Reviewed-by: Lee Jones <[email protected]>
Tested-by: Luca Weiss <[email protected]>
---
drivers/leds/rgb/leds-qcom-lpg.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index a76cb1d6b7b5..976eaac97f40 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -1637,11 +1637,13 @@ static const struct lpg_data pm8994_lpg_data = {
static const struct lpg_data pmi632_lpg_data = {
.triled_base = 0xd000,
+ .lut_size = 64,
+
.num_channels = 5,
.channels = (const struct lpg_channel_data[]) {
- { .base = 0xb300, .triled_mask = BIT(7) },
- { .base = 0xb400, .triled_mask = BIT(6) },
- { .base = 0xb500, .triled_mask = BIT(5) },
+ { .base = 0xb300, .triled_mask = BIT(7), .sdam_offset = 0x48 },
+ { .base = 0xb400, .triled_mask = BIT(6), .sdam_offset = 0x56 },
+ { .base = 0xb500, .triled_mask = BIT(5), .sdam_offset = 0x64 },
{ .base = 0xb600 },
{ .base = 0xb700 },
},
--
2.41.0
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 | 243 ++++++++++++++++++++++++++++++
include/linux/soc/qcom/qcom-pbs.h | 30 ++++
4 files changed, 283 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 b3634e10f6f5..52aa58b02e72 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -254,4 +254,13 @@ config QCOM_INLINE_CRYPTO_ENGINE
tristate
select QCOM_SCM
+config QCOM_PBS
+ tristate "PBS trigger support for Qualcomm Technologies, Inc. PMICS"
+ 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.
+
endmenu
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index bbca2e1e55bb..49bb86b0fe33 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -32,3 +32,4 @@ obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
qcom_ice-objs += ice.o
obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += qcom_ice.o
+obj-$(CONFIG_QCOM_PBS) += qcom-pbs.o
diff --git a/drivers/soc/qcom/qcom-pbs.c b/drivers/soc/qcom/qcom-pbs.c
new file mode 100644
index 000000000000..b49d14bb7d08
--- /dev/null
+++ b/drivers/soc/qcom/qcom-pbs.c
@@ -0,0 +1,243 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/regmap.h>
+#include <linux/spmi.h>
+#include <linux/soc/qcom/qcom-pbs.h>
+
+#define PBS_CLIENT_TRIG_CTL 0x42
+#define PBS_CLIENT_SW_TRIG_BIT BIT(7)
+#define PBS_CLIENT_SCRATCH1 0x50
+#define PBS_CLIENT_SCRATCH2 0x51
+#define PBS_CLIENT_SCRATCH2_ERROR 0xFF
+
+struct pbs_dev {
+ struct device *dev;
+ struct regmap *regmap;
+ struct mutex lock;
+ struct device_link *link;
+
+ u32 base;
+};
+
+static int qcom_pbs_wait_for_ack(struct pbs_dev *pbs, u8 bit_pos)
+{
+ int ret, retries = 2000, delay = 1100;
+ unsigned int val;
+
+ ret = regmap_read_poll_timeout(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2,
+ val, val & BIT(bit_pos), delay, delay * retries);
+
+ if (ret < 0) {
+ dev_err(pbs->dev, "Timeout for PBS ACK/NACK for bit %u\n", bit_pos);
+ return -ETIMEDOUT;
+ }
+
+ if (val == PBS_CLIENT_SCRATCH2_ERROR) {
+ ret = regmap_write(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2, 0);
+ dev_err(pbs->dev, "NACK from PBS for bit %u\n", bit_pos);
+ return -EINVAL;
+ }
+
+ dev_dbg(pbs->dev, "PBS sequence for bit %u executed!\n", bit_pos);
+ 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)
+{
+ unsigned int val;
+ u16 bit_pos;
+ int ret;
+
+ if (!bitmap) {
+ dev_err(pbs->dev, "Invalid bitmap passed by client\n");
+ return -EINVAL;
+ }
+
+ if (IS_ERR_OR_NULL(pbs))
+ return -EINVAL;
+
+ mutex_lock(&pbs->lock);
+ ret = regmap_read(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2, &val);
+ if (ret < 0)
+ goto out;
+
+ if (val == PBS_CLIENT_SCRATCH2_ERROR) {
+ /* PBS error - clear SCRATCH2 register */
+ ret = regmap_write(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2, 0);
+ if (ret < 0)
+ goto out;
+ }
+
+ for (bit_pos = 0; bit_pos < 8; bit_pos++) {
+ if (!(bitmap & BIT(bit_pos)))
+ continue;
+
+ /* Clear the PBS sequence bit position */
+ ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2,
+ BIT(bit_pos), 0);
+ if (ret < 0)
+ goto error;
+
+ /* Set the PBS sequence bit position */
+ ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH1,
+ BIT(bit_pos), BIT(bit_pos));
+ if (ret < 0)
+ goto error;
+
+ /* Initiate the SW trigger */
+ ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_TRIG_CTL,
+ PBS_CLIENT_SW_TRIG_BIT, PBS_CLIENT_SW_TRIG_BIT);
+ if (ret < 0)
+ goto error;
+
+ ret = qcom_pbs_wait_for_ack(pbs, bit_pos);
+ if (ret < 0)
+ goto error;
+
+ /* Clear the PBS sequence bit position */
+ ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH1,
+ BIT(bit_pos), 0);
+ if (ret < 0)
+ goto error;
+
+ /* Clear the PBS sequence bit position */
+ ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2,
+ BIT(bit_pos), 0);
+ if (ret < 0)
+ goto error;
+ }
+
+error:
+ /* Clear all the requested bitmap */
+ ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH1, bitmap, 0);
+
+out:
+ mutex_unlock(&pbs->lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_pbs_trigger_event);
+
+/**
+ * get_pbs_client_device() - Get the PBS device used by client
+ * @dev: Client device
+ *
+ * This function is used to get the PBS device that is being
+ * used by the client.
+ *
+ * Returns: pbs_dev on success, ERR_PTR on failure
+ */
+struct pbs_dev *get_pbs_client_device(struct device *dev)
+{
+ struct device_node *pbs_dev_node;
+ struct platform_device *pdev;
+ struct pbs_dev *pbs;
+
+ pbs_dev_node = of_parse_phandle(dev->of_node, "qcom,pbs", 0);
+ if (!pbs_dev_node) {
+ dev_err(dev, "Missing qcom,pbs property\n");
+ return ERR_PTR(-ENODEV);
+ }
+
+ pdev = of_find_device_by_node(pbs_dev_node);
+ if (!pdev) {
+ dev_err(dev, "Unable to find PBS dev_node\n");
+ pbs = ERR_PTR(-EPROBE_DEFER);
+ goto out;
+ }
+
+ pbs = platform_get_drvdata(pdev);
+ if (!pbs) {
+ dev_err(dev, "Cannot get pbs instance from %s\n", dev_name(&pdev->dev));
+ platform_device_put(pdev);
+ pbs = ERR_PTR(-EPROBE_DEFER);
+ goto out;
+ }
+
+ pbs->link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_SUPPLIER);
+ if (!pbs->link) {
+ dev_err(&pdev->dev, "Failed to create device link to consumer %s\n", dev_name(dev));
+ platform_device_put(pdev);
+ pbs = ERR_PTR(-EINVAL);
+ goto out;
+ }
+
+out:
+ of_node_put(pbs_dev_node);
+ return pbs;
+}
+EXPORT_SYMBOL_GPL(get_pbs_client_device);
+
+static int qcom_pbs_probe(struct platform_device *pdev)
+{
+ struct pbs_dev *pbs;
+ u32 val;
+ int ret;
+
+ pbs = devm_kzalloc(&pdev->dev, sizeof(*pbs), GFP_KERNEL);
+ if (!pbs)
+ return -ENOMEM;
+
+ pbs->dev = &pdev->dev;
+ pbs->regmap = dev_get_regmap(pbs->dev->parent, NULL);
+ if (!pbs->regmap) {
+ dev_err(pbs->dev, "Couldn't get parent's regmap\n");
+ return -EINVAL;
+ }
+
+ ret = device_property_read_u32(pbs->dev, "reg", &val);
+ if (ret < 0) {
+ dev_err(pbs->dev, "Couldn't find reg, ret = %d\n", ret);
+ return ret;
+ }
+ pbs->base = val;
+ mutex_init(&pbs->lock);
+
+ platform_set_drvdata(pdev, pbs);
+
+ return 0;
+}
+
+static const struct of_device_id qcom_pbs_match_table[] = {
+ { .compatible = "qcom,pbs" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, qcom_pbs_match_table);
+
+static struct platform_driver qcom_pbs_driver = {
+ .driver = {
+ .name = "qcom-pbs",
+ .of_match_table = qcom_pbs_match_table,
+ },
+ .probe = qcom_pbs_probe,
+};
+module_platform_driver(qcom_pbs_driver)
+
+MODULE_DESCRIPTION("QCOM PBS DRIVER");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/soc/qcom/qcom-pbs.h b/include/linux/soc/qcom/qcom-pbs.h
new file mode 100644
index 000000000000..8a46209ccf13
--- /dev/null
+++ b/include/linux/soc/qcom/qcom-pbs.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef _QCOM_PBS_H
+#define _QCOM_PBS_H
+
+#include <linux/errno.h>
+#include <linux/types.h>
+
+struct device_node;
+struct pbs_dev;
+
+#if IS_ENABLED(CONFIG_QCOM_PBS)
+int qcom_pbs_trigger_event(struct pbs_dev *pbs, u8 bitmap);
+struct pbs_dev *get_pbs_client_device(struct device *client_dev);
+#else
+static inline int qcom_pbs_trigger_event(struct pbs_dev *pbs, u8 bitmap)
+{
+ return -ENODEV;
+}
+
+static inline struct pbs_dev *get_pbs_client_device(struct device *client_dev)
+{
+ return ERR_PTR(-ENODEV);
+}
+#endif
+
+#endif
--
2.41.0
In some PMICs like pmi632, the pattern look up table (LUT) and LPG
configuration is stored in a single SDAM module instead of LUT
peripheral. This feature is called PPG. PPG uses Qualcomm Programmable
Boot Sequencer (PBS) inorder to trigger pattern sequences for PMICs.
Signed-off-by: Anjelique Melendez <[email protected]>
Tested-by: Luca Weiss <[email protected]>
---
drivers/leds/rgb/leds-qcom-lpg.c | 268 ++++++++++++++++++++++++++++---
1 file changed, 244 insertions(+), 24 deletions(-)
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index 68d82a682bf6..a76cb1d6b7b5 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -8,11 +8,13 @@
#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/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
@@ -39,6 +41,8 @@
#define PWM_SEC_ACCESS_REG 0xd0
#define PWM_DTEST_REG(x) (0xe2 + (x) - 1)
+#define SDAM_REG_PBS_SEQ_EN 0x42
+
#define TRI_LED_SRC_SEL 0x45
#define TRI_LED_EN_CTL 0x46
#define TRI_LED_ATC_CTL 0x47
@@ -48,9 +52,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_LPG_SDAM_LUT_PATTERN_OFFSET 0x80
+
+/* 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;
@@ -64,6 +84,9 @@ 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
+ * @pbs_dev: PBS device
+ * @lpg_chan_sdam: LPG SDAM peripheral device
+ * @pbs_en_bitmap: bitmap for tracking PBS triggers
* @triled_base: base address of the TRILED block (optional)
* @triled_src: power-source for the TRILED
* @triled_has_atc_ctl: true if there is TRI_LED_ATC_CTL register
@@ -85,6 +108,10 @@ struct lpg {
u32 lut_size;
unsigned long *lut_bitmap;
+ struct pbs_dev *pbs_dev;
+ struct nvmem_device *lpg_chan_sdam;
+ unsigned long pbs_en_bitmap;
+
u32 triled_base;
u32 triled_src;
bool triled_has_atc_ctl;
@@ -101,6 +128,7 @@ 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 SDAM
* @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
@@ -129,6 +157,7 @@ struct lpg_channel {
unsigned int triled_mask;
unsigned int lut_mask;
unsigned int subtype;
+ u32 sdam_offset;
bool in_use;
@@ -178,10 +207,12 @@ struct lpg_led {
/**
* struct lpg_channel_data - per channel initialization data
+ * @sdam_offset: Channel offset in LPG SDAM
* @base: base address for PWM channel registers
* @triled_mask: bitmask for controlling this channel in TRILED
*/
struct lpg_channel_data {
+ unsigned int sdam_offset;
unsigned int base;
u8 triled_mask;
};
@@ -206,6 +237,52 @@ struct lpg_data {
const struct lpg_channel_data *channels;
};
+#define PBS_SW_TRIG_BIT BIT(0)
+
+static int lpg_clear_pbs_trigger(struct lpg *lpg, unsigned int lut_mask)
+{
+ u8 val = 0;
+ int rc;
+
+ lpg->pbs_en_bitmap &= (~lut_mask);
+ if (!lpg->pbs_en_bitmap) {
+ rc = nvmem_device_write(lpg->lpg_chan_sdam, SDAM_REG_PBS_SEQ_EN, 1, &val);
+ if (rc < 0)
+ return rc;
+ }
+
+ return 0;
+}
+
+static int lpg_set_pbs_trigger(struct lpg *lpg, unsigned int lut_mask)
+{
+ u8 val = PBS_SW_TRIG_BIT;
+ int rc;
+
+ if (!lpg->pbs_en_bitmap) {
+ rc = nvmem_device_write(lpg->lpg_chan_sdam, SDAM_REG_PBS_SEQ_EN, 1, &val);
+ if (rc < 0)
+ return rc;
+
+ rc = qcom_pbs_trigger_event(lpg->pbs_dev, val);
+ if (rc < 0)
+ return rc;
+ }
+ lpg->pbs_en_bitmap |= lut_mask;
+
+ return 0;
+}
+
+static int lpg_sdam_configure_triggers(struct lpg_channel *chan, u8 set_trig)
+{
+ u32 addr = SDAM_LUT_EN_OFFSET + chan->sdam_offset;
+
+ if (!chan->lpg->lpg_chan_sdam)
+ return 0;
+
+ return nvmem_device_write(chan->lpg->lpg_chan_sdam, addr, 1, &set_trig);
+}
+
static int triled_set(struct lpg *lpg, unsigned int mask, unsigned int enable)
{
/* Skip if we don't have a triled block */
@@ -216,6 +293,40 @@ 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)
+{
+ unsigned int idx;
+ u8 brightness;
+ int i, rc;
+ u16 addr;
+
+ if (len > lpg->lut_size) {
+ dev_err(lpg->dev, "Pattern length (%zu) exceeds maximum pattern length (%d)\n",
+ len, lpg->lut_size);
+ 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 = SDAM_LPG_SDAM_LUT_PATTERN_OFFSET + i + idx;
+ rc = nvmem_device_write(lpg->lpg_chan_sdam, addr, 1, &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)
{
@@ -256,6 +367,9 @@ static void lpg_lut_free(struct lpg *lpg, unsigned int lo_idx, unsigned int hi_i
static int lpg_lut_sync(struct lpg *lpg, unsigned int mask)
{
+ if (!lpg->lut_base)
+ return 0;
+
return regmap_write(lpg->map, lpg->lut_base + RAMP_CONTROL_REG, mask);
}
@@ -462,6 +576,28 @@ 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)
+{
+ struct nvmem_device *lpg_chan_sdam = chan->lpg->lpg_chan_sdam;
+ unsigned int lo_idx = chan->pattern_lo_idx;
+ unsigned int hi_idx = chan->pattern_hi_idx;
+ u8 val = 0, conf = 0;
+
+ if (!chan->ramp_enabled || chan->pattern_lo_idx == chan->pattern_hi_idx)
+ return;
+
+ if (!chan->ramp_oneshot)
+ conf |= LPG_PATTERN_CONFIG_REPEAT;
+
+ nvmem_device_write(lpg_chan_sdam, SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET + chan->sdam_offset, 1, &val);
+ nvmem_device_write(lpg_chan_sdam, SDAM_PATTERN_CONFIG_OFFSET + chan->sdam_offset, 1, &conf);
+ nvmem_device_write(lpg_chan_sdam, SDAM_END_INDEX_OFFSET + chan->sdam_offset, 1, &hi_idx);
+ nvmem_device_write(lpg_chan_sdam, SDAM_START_INDEX_OFFSET + chan->sdam_offset, 1, &lo_idx);
+
+ val = RAMP_STEP_DURATION(chan->ramp_tick_ms);
+ nvmem_device_write(lpg_chan_sdam, SDAM_REG_RAMP_STEP_DURATION, 1, &val);
+}
+
static void lpg_apply_lut_control(struct lpg_channel *chan)
{
struct lpg *lpg = chan->lpg;
@@ -597,7 +733,10 @@ static void lpg_apply(struct lpg_channel *chan)
lpg_apply_pwm_value(chan);
lpg_apply_control(chan);
lpg_apply_sync(chan);
- lpg_apply_lut_control(chan);
+ if (chan->lpg->lpg_chan_sdam)
+ lpg_sdam_apply_lut_control(chan);
+ else
+ lpg_apply_lut_control(chan);
lpg_enable_glitch(chan);
}
@@ -622,6 +761,7 @@ static void lpg_brightness_set(struct lpg_led *led, struct led_classdev *cdev,
chan->ramp_enabled = false;
} else if (chan->pattern_lo_idx != chan->pattern_hi_idx) {
lpg_calc_freq(chan, NSEC_PER_MSEC);
+ lpg_sdam_configure_triggers(chan, 1);
chan->enabled = true;
chan->ramp_enabled = true;
@@ -649,8 +789,10 @@ static void lpg_brightness_set(struct lpg_led *led, struct led_classdev *cdev,
triled_set(lpg, triled_mask, triled_enabled);
/* Trigger start of ramp generator(s) */
- if (lut_mask)
+ if (lut_mask) {
lpg_lut_sync(lpg, lut_mask);
+ lpg_set_pbs_trigger(lpg, lut_mask);
+ }
}
static int lpg_brightness_single_set(struct led_classdev *cdev,
@@ -767,9 +909,9 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
struct led_pattern *pattern;
unsigned int brightness_a;
unsigned int brightness_b;
+ unsigned int hi_pause = 0;
+ unsigned int lo_pause = 0;
unsigned int actual_len;
- unsigned int hi_pause;
- unsigned int lo_pause;
unsigned int delta_t;
unsigned int lo_idx;
unsigned int hi_idx;
@@ -836,18 +978,23 @@ 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".
+ *
+ * SDAM-based devices do not support "ping-pong", "low pause" or "high pause"
*/
/* 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) {
+ for (i = 0; i < len / 2; i++) {
+ brightness_a = pattern[i].brightness;
+ brightness_b = pattern[len - i - 1].brightness;
+
+ if (brightness_a != brightness_b) {
+ ping_pong = false;
+ break;
+ }
}
- }
+ } else
+ ping_pong = false;
/* The pattern length to be written to the LUT */
if (ping_pong)
@@ -875,12 +1022,26 @@ 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 in the LUT case.
+ * SDAM-based devices require equal duration of all steps
+ */
+ if (lpg->lut_base) {
+ lo_pause = pattern[0].delta_t;
+ hi_pause = pattern[actual_len - 1].delta_t;
+ } else {
+ if (delta_t != pattern[0].delta_t || delta_t != pattern[actual_len - 1].delta_t)
+ goto out_free_pattern;
+ }
+
mutex_lock(&lpg->lock);
- ret = lpg_lut_store(lpg, pattern, actual_len, &lo_idx, &hi_idx);
+
+ if (lpg->lut_base)
+ ret = lpg_lut_store(lpg, pattern, actual_len, &lo_idx, &hi_idx);
+ else
+ ret = lpg_lut_store_sdam(lpg, pattern, actual_len, &lo_idx, &hi_idx);
+
if (ret < 0)
goto out_unlock;
@@ -928,7 +1089,12 @@ static int lpg_pattern_mc_set(struct led_classdev *cdev,
{
struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
struct lpg_led *led = container_of(mc, struct lpg_led, mcdev);
- int ret;
+ unsigned int triled_mask = 0;
+ int ret, i;
+
+ for (i = 0; i < led->num_channels; i++)
+ triled_mask |= led->channels[i]->triled_mask;
+ triled_set(led->lpg, triled_mask, 0);
ret = lpg_pattern_set(led, pattern, len, repeat);
if (ret < 0)
@@ -953,6 +1119,8 @@ static int lpg_pattern_clear(struct lpg_led *led)
for (i = 0; i < led->num_channels; i++) {
chan = led->channels[i];
+ lpg_sdam_configure_triggers(chan, 0);
+ lpg_clear_pbs_trigger(chan->lpg, chan->lut_mask);
chan->pattern_lo_idx = 0;
chan->pattern_hi_idx = 0;
}
@@ -1187,8 +1355,8 @@ static int lpg_add_led(struct lpg *lpg, struct device_node *np)
cdev->brightness_set_blocking = lpg_brightness_mc_set;
cdev->blink_set = lpg_blink_mc_set;
- /* Register pattern accessors only if we have a LUT block */
- if (lpg->lut_base) {
+ /* Register pattern accessors if we have a LUT block or when using PPG */
+ if (lpg->lut_base || lpg->lpg_chan_sdam) {
cdev->pattern_set = lpg_pattern_mc_set;
cdev->pattern_clear = lpg_pattern_mc_clear;
}
@@ -1201,15 +1369,19 @@ static int lpg_add_led(struct lpg *lpg, struct device_node *np)
cdev->brightness_set_blocking = lpg_brightness_single_set;
cdev->blink_set = lpg_blink_single_set;
- /* Register pattern accessors only if we have a LUT block */
- if (lpg->lut_base) {
+ /* Register pattern accessors if we have a LUT block or when using PPG */
+ if (lpg->lut_base || lpg->lpg_chan_sdam) {
cdev->pattern_set = lpg_pattern_single_set;
cdev->pattern_clear = lpg_pattern_single_clear;
}
}
cdev->default_trigger = of_get_property(np, "linux,default-trigger", NULL);
- cdev->max_brightness = LPG_RESOLUTION_9BIT - 1;
+
+ if (lpg->lpg_chan_sdam)
+ 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"))
@@ -1250,6 +1422,7 @@ 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;
regmap_read(lpg->map, chan->base + LPG_SUBTYPE_REG, &chan->subtype);
}
@@ -1296,11 +1469,12 @@ static int lpg_init_lut(struct lpg *lpg)
{
const struct lpg_data *data = lpg->data;
- if (!data->lut_base)
+ if (!data->lut_size)
return 0;
- lpg->lut_base = data->lut_base;
lpg->lut_size = data->lut_size;
+ if (data->lut_base)
+ lpg->lut_base = data->lut_base;
lpg->lut_bitmap = devm_bitmap_zalloc(lpg->dev, lpg->lut_size, GFP_KERNEL);
if (!lpg->lut_bitmap)
@@ -1309,6 +1483,48 @@ static int lpg_init_lut(struct lpg *lpg)
return 0;
}
+static int lpg_init_sdam(struct lpg *lpg)
+{
+ int i, sdam_count, rc;
+ u8 val = 0;
+
+ sdam_count = of_property_count_strings(lpg->dev->of_node, "nvmem-names");
+ if (sdam_count <= 0)
+ return 0;
+
+ /* Get the SDAM device for LPG/LUT config */
+ lpg->lpg_chan_sdam = devm_nvmem_device_get(lpg->dev, "lpg_chan_sdam");
+ if (IS_ERR(lpg->lpg_chan_sdam))
+ return dev_err_probe(lpg->dev, PTR_ERR(lpg->lpg_chan_sdam),
+ "Failed to get LPG chan SDAM device\n");
+
+ lpg->pbs_dev = get_pbs_client_device(lpg->dev);
+ if (IS_ERR(lpg->pbs_dev))
+ return dev_err_probe(lpg->dev, PTR_ERR(lpg->pbs_dev),
+ "Failed to get PBS client device\n");
+
+ for (i = 0; i < lpg->num_channels; i++) {
+ struct lpg_channel *chan = &lpg->channels[i];
+
+ if (chan->sdam_offset) {
+ rc = nvmem_device_write(lpg->lpg_chan_sdam,
+ SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET + chan->sdam_offset, 1, &val);
+ if (rc < 0)
+ return rc;
+
+ rc = lpg_sdam_configure_triggers(chan, 0);
+ if (rc < 0)
+ return rc;
+
+ rc = lpg_clear_pbs_trigger(chan->lpg, chan->lut_mask);
+ if (rc < 0)
+ return rc;
+ }
+ }
+
+ return 0;
+}
+
static int lpg_probe(struct platform_device *pdev)
{
struct device_node *np;
@@ -1345,6 +1561,10 @@ static int lpg_probe(struct platform_device *pdev)
if (ret < 0)
return ret;
+ ret = lpg_init_sdam(lpg);
+ if (ret < 0)
+ return ret;
+
ret = lpg_init_lut(lpg);
if (ret < 0)
return ret;
--
2.41.0
On PMICs such as PM8350C, the pattern lookup table (LUT) is stored in a
separate SDAM from the one where the lpg per-channel data is stored.
Add support for PPG with a dedicated LUT SDAM while maintaining backward
compatibility for those targets that use only a single SDAM.
Co-developed-by: Guru Das Srinagesh <[email protected]>
Signed-off-by: Guru Das Srinagesh <[email protected]>
Signed-off-by: Anjelique Melendez <[email protected]>
Reviewed-by: Lee Jones <[email protected]>
---
drivers/leds/rgb/leds-qcom-lpg.c | 92 +++++++++++++++++++++++++++-----
1 file changed, 78 insertions(+), 14 deletions(-)
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index 976eaac97f40..eec49027ccf1 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -42,6 +42,8 @@
#define PWM_DTEST_REG(x) (0xe2 + (x) - 1)
#define SDAM_REG_PBS_SEQ_EN 0x42
+#define SDAM_PBS_TRIG_SET 0xe5
+#define SDAM_PBS_TRIG_CLR 0xe6
#define TRI_LED_SRC_SEL 0x45
#define TRI_LED_EN_CTL 0x46
@@ -60,8 +62,12 @@
#define DEFAULT_TICK_DURATION_US 7800
#define RAMP_STEP_DURATION(x) (((x) * 1000 / DEFAULT_TICK_DURATION_US) & 0xff)
+#define SDAM_MAX_DEVICES 2
/* LPG common config settings for PPG */
+#define SDAM_START_BASE 0x40
#define SDAM_REG_RAMP_STEP_DURATION 0x47
+
+#define SDAM_LUT_SDAM_LUT_PATTERN_OFFSET 0x45
#define SDAM_LPG_SDAM_LUT_PATTERN_OFFSET 0x80
/* LPG per channel config settings for PPG */
@@ -70,6 +76,8 @@
#define SDAM_END_INDEX_OFFSET 0x3
#define SDAM_START_INDEX_OFFSET 0x4
#define SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET 0x6
+#define SDAM_PAUSE_HI_MULTIPLIER_OFFSET 0x8
+#define SDAM_PAUSE_LO_MULTIPLIER_OFFSET 0x9
struct lpg_channel;
struct lpg_data;
@@ -86,6 +94,7 @@ struct lpg_data;
* @lut_bitmap: allocation bitmap for LUT entries
* @pbs_dev: PBS device
* @lpg_chan_sdam: LPG SDAM peripheral device
+ * @lut_sdam: LUT SDAM peripheral device
* @pbs_en_bitmap: bitmap for tracking PBS triggers
* @triled_base: base address of the TRILED block (optional)
* @triled_src: power-source for the TRILED
@@ -110,6 +119,7 @@ struct lpg {
struct pbs_dev *pbs_dev;
struct nvmem_device *lpg_chan_sdam;
+ struct nvmem_device *lut_sdam;
unsigned long pbs_en_bitmap;
u32 triled_base;
@@ -249,6 +259,13 @@ static int lpg_clear_pbs_trigger(struct lpg *lpg, unsigned int lut_mask)
rc = nvmem_device_write(lpg->lpg_chan_sdam, SDAM_REG_PBS_SEQ_EN, 1, &val);
if (rc < 0)
return rc;
+
+ if (lpg->lut_sdam) {
+ val = PBS_SW_TRIG_BIT;
+ rc = nvmem_device_write(lpg->lpg_chan_sdam, SDAM_PBS_TRIG_CLR, 1, &val);
+ if (rc < 0)
+ return rc;
+ }
}
return 0;
@@ -264,9 +281,15 @@ static int lpg_set_pbs_trigger(struct lpg *lpg, unsigned int lut_mask)
if (rc < 0)
return rc;
- rc = qcom_pbs_trigger_event(lpg->pbs_dev, val);
- if (rc < 0)
- return rc;
+ if (lpg->lut_sdam) {
+ rc = nvmem_device_write(lpg->lpg_chan_sdam, SDAM_PBS_TRIG_SET, 1, &val);
+ if (rc < 0)
+ return rc;
+ } else {
+ rc = qcom_pbs_trigger_event(lpg->pbs_dev, val);
+ if (rc < 0)
+ return rc;
+ }
}
lpg->pbs_en_bitmap |= lut_mask;
@@ -313,8 +336,15 @@ static int lpg_lut_store_sdam(struct lpg *lpg, struct led_pattern *pattern,
for (i = 0; i < len; i++) {
brightness = pattern[i].brightness;
- addr = SDAM_LPG_SDAM_LUT_PATTERN_OFFSET + i + idx;
- rc = nvmem_device_write(lpg->lpg_chan_sdam, addr, 1, &brightness);
+
+ if (lpg->lut_sdam) {
+ addr = SDAM_LUT_SDAM_LUT_PATTERN_OFFSET + i + idx;
+ rc = nvmem_device_write(lpg->lut_sdam, addr, 1, &brightness);
+ } else {
+ addr = SDAM_LPG_SDAM_LUT_PATTERN_OFFSET + i + idx;
+ rc = nvmem_device_write(lpg->lpg_chan_sdam, addr, 1, &brightness);
+ }
+
if (rc < 0)
return rc;
}
@@ -581,13 +611,28 @@ static void lpg_sdam_apply_lut_control(struct lpg_channel *chan)
struct nvmem_device *lpg_chan_sdam = chan->lpg->lpg_chan_sdam;
unsigned int lo_idx = chan->pattern_lo_idx;
unsigned int hi_idx = chan->pattern_hi_idx;
- u8 val = 0, conf = 0;
+ u8 val = 0, conf = 0, lut_offset = 0;
+ unsigned int hi_pause, lo_pause;
+ struct lpg *lpg = chan->lpg;
if (!chan->ramp_enabled || chan->pattern_lo_idx == chan->pattern_hi_idx)
return;
+ hi_pause = DIV_ROUND_UP(chan->ramp_hi_pause_ms, chan->ramp_tick_ms);
+ lo_pause = DIV_ROUND_UP(chan->ramp_lo_pause_ms, chan->ramp_tick_ms);
+
if (!chan->ramp_oneshot)
conf |= LPG_PATTERN_CONFIG_REPEAT;
+ if (chan->ramp_hi_pause_ms && lpg->lut_sdam)
+ conf |= LPG_PATTERN_CONFIG_PAUSE_HI;
+ if (chan->ramp_lo_pause_ms && lpg->lut_sdam)
+ conf |= LPG_PATTERN_CONFIG_PAUSE_LO;
+
+ if (lpg->lut_sdam) {
+ lut_offset = SDAM_LUT_SDAM_LUT_PATTERN_OFFSET - SDAM_START_BASE;
+ hi_idx += lut_offset;
+ lo_idx += lut_offset;
+ }
nvmem_device_write(lpg_chan_sdam, SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET + chan->sdam_offset, 1, &val);
nvmem_device_write(lpg_chan_sdam, SDAM_PATTERN_CONFIG_OFFSET + chan->sdam_offset, 1, &conf);
@@ -596,6 +641,12 @@ static void lpg_sdam_apply_lut_control(struct lpg_channel *chan)
val = RAMP_STEP_DURATION(chan->ramp_tick_ms);
nvmem_device_write(lpg_chan_sdam, SDAM_REG_RAMP_STEP_DURATION, 1, &val);
+
+ if (lpg->lut_sdam) {
+ nvmem_device_write(lpg_chan_sdam, SDAM_PAUSE_HI_MULTIPLIER_OFFSET + chan->sdam_offset, 1, &hi_pause);
+ nvmem_device_write(lpg_chan_sdam, SDAM_PAUSE_LO_MULTIPLIER_OFFSET + chan->sdam_offset, 1, &lo_pause);
+ }
+
}
static void lpg_apply_lut_control(struct lpg_channel *chan)
@@ -979,7 +1030,8 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
* enabled. In this scenario the delta_t of the middle entry (i.e. the
* last in the programmed pattern) determines the "high pause".
*
- * SDAM-based devices do not support "ping-pong", "low pause" or "high pause"
+ * SDAM-based devices do not support "ping pong", and only supports
+ * "low pause" and "high pause" with a dedicated SDAM LUT.
*/
/* Detect palindromes and use "ping pong" to reduce LUT usage */
@@ -1024,9 +1076,10 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
/*
* Find "low pause" and "high pause" in the pattern in the LUT case.
- * SDAM-based devices require equal duration of all steps
+ * SDAM-based devices without dedicated LUT SDAM require equal
+ * duration of all steps.
*/
- if (lpg->lut_base) {
+ if (lpg->lut_base || lpg->lut_sdam) {
lo_pause = pattern[0].delta_t;
hi_pause = pattern[actual_len - 1].delta_t;
} else {
@@ -1491,17 +1544,28 @@ static int lpg_init_sdam(struct lpg *lpg)
sdam_count = of_property_count_strings(lpg->dev->of_node, "nvmem-names");
if (sdam_count <= 0)
return 0;
+ if (sdam_count > SDAM_MAX_DEVICES)
+ return -EINVAL;
- /* Get the SDAM device for LPG/LUT config */
+ /* Get the 1st SDAM device for LPG/LUT config */
lpg->lpg_chan_sdam = devm_nvmem_device_get(lpg->dev, "lpg_chan_sdam");
if (IS_ERR(lpg->lpg_chan_sdam))
return dev_err_probe(lpg->dev, PTR_ERR(lpg->lpg_chan_sdam),
"Failed to get LPG chan SDAM device\n");
- lpg->pbs_dev = get_pbs_client_device(lpg->dev);
- if (IS_ERR(lpg->pbs_dev))
- return dev_err_probe(lpg->dev, PTR_ERR(lpg->pbs_dev),
- "Failed to get PBS client device\n");
+ if (sdam_count == 1) {
+ /* Get PBS device node if single SDAM device */
+ lpg->pbs_dev = get_pbs_client_device(lpg->dev);
+ if (IS_ERR(lpg->pbs_dev))
+ return dev_err_probe(lpg->dev, PTR_ERR(lpg->pbs_dev),
+ "Failed to get PBS client device\n");
+ } else if (sdam_count == 2) {
+ /* Get the 2nd SDAM device for LUT pattern */
+ lpg->lut_sdam = devm_nvmem_device_get(lpg->dev, "lut_sdam");
+ if (IS_ERR(lpg->lut_sdam))
+ return dev_err_probe(lpg->dev, PTR_ERR(lpg->lut_sdam),
+ "Failed to get LPG LUT SDAM device\n");
+ }
for (i = 0; i < lpg->num_channels; i++) {
struct lpg_channel *chan = &lpg->channels[i];
--
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]>
Reviewed-by: Lee Jones <[email protected]>
---
drivers/leds/rgb/leds-qcom-lpg.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index eec49027ccf1..b11412af860e 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -1780,11 +1780,13 @@ static const struct lpg_data pm8150l_lpg_data = {
static const struct lpg_data pm8350c_pwm_data = {
.triled_base = 0xef00,
+ .lut_size = 122,
+
.num_channels = 4,
.channels = (const struct lpg_channel_data[]) {
- { .base = 0xe800, .triled_mask = BIT(7) },
- { .base = 0xe900, .triled_mask = BIT(6) },
- { .base = 0xea00, .triled_mask = BIT(5) },
+ { .base = 0xe800, .triled_mask = BIT(7), .sdam_offset = 0x48 },
+ { .base = 0xe900, .triled_mask = BIT(6), .sdam_offset = 0x56 },
+ { .base = 0xea00, .triled_mask = BIT(5), .sdam_offset = 0x64 },
{ .base = 0xeb00 },
},
};
--
2.41.0
On Thu, 21 Dec 2023 10:58:30 -0800, Anjelique Melendez wrote:
> In certain PMICs, LUT pattern and LPG configuration is stored in SDAM
> modules instead of LUT peripheral. This feature is called PPG.
>
> This change series adds support for PPG. Thanks!
> Changes since v7:
> - Patch 4/7
> - Initialize hi/lo_pause variables in lpg_pattern_set()
> Changes since v6:
> - Patch 2/7
> - Removed required by constraint on PPG dt properties
> Changes since v5:
> - Patch 4/7
> - Update logic so that multicolor led device triggers pattern
> on all LEDs at the same time
> - Update nitpicks from Lee
> - Patch 5/7
> - Update nitpicks from Lee
> Changes since v4:
> - Patch 3/7
> - Get rid of r/w helpers
> - Use regmap_read_poll_timeout() in qcom_pbs_wait_for_ack()
> - Update error path in qcom_pbs_trigger_event()
> - Fix reverse christmas tree
> - Patch 4/7
> - Get rid of r/w helpers
> - Update variables to use "sdam" instead of "nvmem"
> - Fix comments
> - Fix reverse christmas tree
> - Update lpg_pattern_set() logic
> - Patch 5/7
> - Removed sdam_lut_base from lpg_data
> Changes since v3:
> - Patch 4/7
> - Fix function returns
> - Move register definition to top of file
> - Revert max_brightness and probe accidental changes
> - Combine init_sdam() and parse_sdam()
> - Change error prints in probe to use dev_err_probe
> - Remove ppg_en variable
> - Update when pbs triggers are set/cleared
> - Patch 6/7
> - Remove use of nvmem_count
> - Move register definition to top of file
> - Remove lpg_get_sdam_lut_idx()
> Changes since v2:
> - Patch 1/7
> - Fix dt_binding_check error
> - Rename binding file to match compatible
> - Iclude SoC specific comptaibles
> - Patch 2/7
> - Update nvmem-names list
> - Patch 3/7
> - Update EXPORT_SYMBOL to EXPORT_SYMBOL_GPL
> - Fix return/break logic in qcom_pbs_wait_for_ack()
> - Update iterators to be int
> - Add constants
> - Fix function calls in qcom_pbs_trigger_event()
> - Remove unnessary comments
> - Return -EPROBE_DEFER from get_pbs_client_device()
> Changes since v1:
> - Patch 1/7
> - Fix dt_binding_check errors
> - Update binding description
> - Path 2/7
> - Fix dt_binding_check errors
> - Update per variant constraints
> - Update nvmem description
> - Patch 3/7
> - Update get_pbs_client_device()
> - Drop use of printk
> - Remove unused function
>
> [...]
Applied, thanks!
[2/7] dt-bindings: leds: leds-qcom-lpg: Add support for LPG PPG
commit: 2fdd08fec742e0c94a2a06a0c9ee0912b6f7ac39
[4/7] leds: rgb: leds-qcom-lpg: Add support for PPG through single SDAM
commit: 07a1afc8fbb77cc893e2285112482902ac88a295
[5/7] leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG
commit: f4f5f6a6f8d7bcc8efd0eee6751def22c9a38fd0
[6/7] leds: rgb: leds-qcom-lpg: Include support for PPG with dedicated LUT SDAM
commit: 7399a927272de1fc42f4da8af1d8d60b65a15b84
[7/7] leds: rgb: Update PM8350C lpg_data to support two-nvmem PPG Scheme
commit: 7b4066868689b1f341e61957611d252b6fa8cafc
--
Lee Jones [李琼斯]
On Thu, 21 Dec 2023, Anjelique Melendez wrote:
> In some PMICs like pmi632, the pattern look up table (LUT) and LPG
> configuration is stored in a single SDAM module instead of LUT
> peripheral. This feature is called PPG. PPG uses Qualcomm Programmable
> Boot Sequencer (PBS) inorder to trigger pattern sequences for PMICs.
>
> Signed-off-by: Anjelique Melendez <[email protected]>
> Tested-by: Luca Weiss <[email protected]>
> ---
> drivers/leds/rgb/leds-qcom-lpg.c | 268 ++++++++++++++++++++++++++++---
> 1 file changed, 244 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> index 68d82a682bf6..a76cb1d6b7b5 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> @@ -8,11 +8,13 @@
> #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/platform_device.h>
> #include <linux/pwm.h>
> #include <linux/regmap.h>
> #include <linux/slab.h>
> +#include <linux/soc/qcom/qcom-pbs.h>
[...]
> +static void lpg_sdam_apply_lut_control(struct lpg_channel *chan)
> +{
> + struct nvmem_device *lpg_chan_sdam = chan->lpg->lpg_chan_sdam;
> + unsigned int lo_idx = chan->pattern_lo_idx;
> + unsigned int hi_idx = chan->pattern_hi_idx;
> + u8 val = 0, conf = 0;
> +
> + if (!chan->ramp_enabled || chan->pattern_lo_idx == chan->pattern_hi_idx)
Nit: you can use lo_idx and hi_idx here instead, right?
Please fix this up subsequently.
--
Lee Jones [李琼斯]
On Thu, 11 Jan 2024, Lee Jones wrote:
> On Thu, 21 Dec 2023 10:58:30 -0800, Anjelique Melendez wrote:
> > In certain PMICs, LUT pattern and LPG configuration is stored in SDAM
> > modules instead of LUT peripheral. This feature is called PPG.
> >
> > This change series adds support for PPG. Thanks!
> > Changes since v7:
> > - Patch 4/7
> > - Initialize hi/lo_pause variables in lpg_pattern_set()
> > Changes since v6:
> > - Patch 2/7
> > - Removed required by constraint on PPG dt properties
> > Changes since v5:
> > - Patch 4/7
> > - Update logic so that multicolor led device triggers pattern
> > on all LEDs at the same time
> > - Update nitpicks from Lee
> > - Patch 5/7
> > - Update nitpicks from Lee
> > Changes since v4:
> > - Patch 3/7
> > - Get rid of r/w helpers
> > - Use regmap_read_poll_timeout() in qcom_pbs_wait_for_ack()
> > - Update error path in qcom_pbs_trigger_event()
> > - Fix reverse christmas tree
> > - Patch 4/7
> > - Get rid of r/w helpers
> > - Update variables to use "sdam" instead of "nvmem"
> > - Fix comments
> > - Fix reverse christmas tree
> > - Update lpg_pattern_set() logic
> > - Patch 5/7
> > - Removed sdam_lut_base from lpg_data
> > Changes since v3:
> > - Patch 4/7
> > - Fix function returns
> > - Move register definition to top of file
> > - Revert max_brightness and probe accidental changes
> > - Combine init_sdam() and parse_sdam()
> > - Change error prints in probe to use dev_err_probe
> > - Remove ppg_en variable
> > - Update when pbs triggers are set/cleared
> > - Patch 6/7
> > - Remove use of nvmem_count
> > - Move register definition to top of file
> > - Remove lpg_get_sdam_lut_idx()
> > Changes since v2:
> > - Patch 1/7
> > - Fix dt_binding_check error
> > - Rename binding file to match compatible
> > - Iclude SoC specific comptaibles
> > - Patch 2/7
> > - Update nvmem-names list
> > - Patch 3/7
> > - Update EXPORT_SYMBOL to EXPORT_SYMBOL_GPL
> > - Fix return/break logic in qcom_pbs_wait_for_ack()
> > - Update iterators to be int
> > - Add constants
> > - Fix function calls in qcom_pbs_trigger_event()
> > - Remove unnessary comments
> > - Return -EPROBE_DEFER from get_pbs_client_device()
> > Changes since v1:
> > - Patch 1/7
> > - Fix dt_binding_check errors
> > - Update binding description
> > - Path 2/7
> > - Fix dt_binding_check errors
> > - Update per variant constraints
> > - Update nvmem description
> > - Patch 3/7
> > - Update get_pbs_client_device()
> > - Drop use of printk
> > - Remove unused function
> >
> > [...]
>
> Applied, thanks!
>
> [2/7] dt-bindings: leds: leds-qcom-lpg: Add support for LPG PPG
> commit: 2fdd08fec742e0c94a2a06a0c9ee0912b6f7ac39
> [4/7] leds: rgb: leds-qcom-lpg: Add support for PPG through single SDAM
> commit: 07a1afc8fbb77cc893e2285112482902ac88a295
> [5/7] leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG
> commit: f4f5f6a6f8d7bcc8efd0eee6751def22c9a38fd0
> [6/7] leds: rgb: leds-qcom-lpg: Include support for PPG with dedicated LUT SDAM
> commit: 7399a927272de1fc42f4da8af1d8d60b65a15b84
> [7/7] leds: rgb: Update PM8350C lpg_data to support two-nvmem PPG Scheme
> commit: 7b4066868689b1f341e61957611d252b6fa8cafc
This set had a bunch of checkpatch.pl errors.
Please fix them up subsequently.
--
Lee Jones [李琼斯]
On 1/11/2024 2:07 AM, Lee Jones wrote:
> On Thu, 11 Jan 2024, Lee Jones wrote:
>
>> On Thu, 21 Dec 2023 10:58:30 -0800, Anjelique Melendez wrote:
>>> In certain PMICs, LUT pattern and LPG configuration is stored in SDAM
>>> modules instead of LUT peripheral. This feature is called PPG.
>>>
>>> This change series adds support for PPG. Thanks!
>>> Changes since v7:
>>> - Patch 4/7
>>> - Initialize hi/lo_pause variables in lpg_pattern_set()
>>> Changes since v6:
>>> - Patch 2/7
>>> - Removed required by constraint on PPG dt properties
>>> Changes since v5:
>>> - Patch 4/7
>>> - Update logic so that multicolor led device triggers pattern
>>> on all LEDs at the same time
>>> - Update nitpicks from Lee
>>> - Patch 5/7
>>> - Update nitpicks from Lee
>>> Changes since v4:
>>> - Patch 3/7
>>> - Get rid of r/w helpers
>>> - Use regmap_read_poll_timeout() in qcom_pbs_wait_for_ack()
>>> - Update error path in qcom_pbs_trigger_event()
>>> - Fix reverse christmas tree
>>> - Patch 4/7
>>> - Get rid of r/w helpers
>>> - Update variables to use "sdam" instead of "nvmem"
>>> - Fix comments
>>> - Fix reverse christmas tree
>>> - Update lpg_pattern_set() logic
>>> - Patch 5/7
>>> - Removed sdam_lut_base from lpg_data
>>> Changes since v3:
>>> - Patch 4/7
>>> - Fix function returns
>>> - Move register definition to top of file
>>> - Revert max_brightness and probe accidental changes
>>> - Combine init_sdam() and parse_sdam()
>>> - Change error prints in probe to use dev_err_probe
>>> - Remove ppg_en variable
>>> - Update when pbs triggers are set/cleared
>>> - Patch 6/7
>>> - Remove use of nvmem_count
>>> - Move register definition to top of file
>>> - Remove lpg_get_sdam_lut_idx()
>>> Changes since v2:
>>> - Patch 1/7
>>> - Fix dt_binding_check error
>>> - Rename binding file to match compatible
>>> - Iclude SoC specific comptaibles
>>> - Patch 2/7
>>> - Update nvmem-names list
>>> - Patch 3/7
>>> - Update EXPORT_SYMBOL to EXPORT_SYMBOL_GPL
>>> - Fix return/break logic in qcom_pbs_wait_for_ack()
>>> - Update iterators to be int
>>> - Add constants
>>> - Fix function calls in qcom_pbs_trigger_event()
>>> - Remove unnessary comments
>>> - Return -EPROBE_DEFER from get_pbs_client_device()
>>> Changes since v1:
>>> - Patch 1/7
>>> - Fix dt_binding_check errors
>>> - Update binding description
>>> - Path 2/7
>>> - Fix dt_binding_check errors
>>> - Update per variant constraints
>>> - Update nvmem description
>>> - Patch 3/7
>>> - Update get_pbs_client_device()
>>> - Drop use of printk
>>> - Remove unused function
>>>
>>> [...]
>>
>> Applied, thanks!
>>
>> [2/7] dt-bindings: leds: leds-qcom-lpg: Add support for LPG PPG
>> commit: 2fdd08fec742e0c94a2a06a0c9ee0912b6f7ac39
>> [4/7] leds: rgb: leds-qcom-lpg: Add support for PPG through single SDAM
>> commit: 07a1afc8fbb77cc893e2285112482902ac88a295
>> [5/7] leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG
>> commit: f4f5f6a6f8d7bcc8efd0eee6751def22c9a38fd0
>> [6/7] leds: rgb: leds-qcom-lpg: Include support for PPG with dedicated LUT SDAM
>> commit: 7399a927272de1fc42f4da8af1d8d60b65a15b84
>> [7/7] leds: rgb: Update PM8350C lpg_data to support two-nvmem PPG Scheme
>> commit: 7b4066868689b1f341e61957611d252b6fa8cafc
>
> This set had a bunch of checkpatch.pl errors.
>
> Please fix them up subsequently.
>
Hi Lee,
Just wanted to get some quick clarification. Would you like checkpatch.pl issues fixed in a new version
of this series or would you like a new patch to fix all the issues? Looks like these patches are in your
for-leds-next-next branch so I am guessing you would like a new follow up patch
but I just wanted to double check.
Thanks,
Anjelique
On Tue, 16 Jan 2024, Anjelique Melendez wrote:
>
>
> On 1/11/2024 2:07 AM, Lee Jones wrote:
> > On Thu, 11 Jan 2024, Lee Jones wrote:
> >
> >> On Thu, 21 Dec 2023 10:58:30 -0800, Anjelique Melendez wrote:
> >>> In certain PMICs, LUT pattern and LPG configuration is stored in SDAM
> >>> modules instead of LUT peripheral. This feature is called PPG.
> >>>
> >>> This change series adds support for PPG. Thanks!
> >>> Changes since v7:
> >>> - Patch 4/7
> >>> - Initialize hi/lo_pause variables in lpg_pattern_set()
> >>> Changes since v6:
> >>> - Patch 2/7
> >>> - Removed required by constraint on PPG dt properties
> >>> Changes since v5:
> >>> - Patch 4/7
> >>> - Update logic so that multicolor led device triggers pattern
> >>> on all LEDs at the same time
> >>> - Update nitpicks from Lee
> >>> - Patch 5/7
> >>> - Update nitpicks from Lee
> >>> Changes since v4:
> >>> - Patch 3/7
> >>> - Get rid of r/w helpers
> >>> - Use regmap_read_poll_timeout() in qcom_pbs_wait_for_ack()
> >>> - Update error path in qcom_pbs_trigger_event()
> >>> - Fix reverse christmas tree
> >>> - Patch 4/7
> >>> - Get rid of r/w helpers
> >>> - Update variables to use "sdam" instead of "nvmem"
> >>> - Fix comments
> >>> - Fix reverse christmas tree
> >>> - Update lpg_pattern_set() logic
> >>> - Patch 5/7
> >>> - Removed sdam_lut_base from lpg_data
> >>> Changes since v3:
> >>> - Patch 4/7
> >>> - Fix function returns
> >>> - Move register definition to top of file
> >>> - Revert max_brightness and probe accidental changes
> >>> - Combine init_sdam() and parse_sdam()
> >>> - Change error prints in probe to use dev_err_probe
> >>> - Remove ppg_en variable
> >>> - Update when pbs triggers are set/cleared
> >>> - Patch 6/7
> >>> - Remove use of nvmem_count
> >>> - Move register definition to top of file
> >>> - Remove lpg_get_sdam_lut_idx()
> >>> Changes since v2:
> >>> - Patch 1/7
> >>> - Fix dt_binding_check error
> >>> - Rename binding file to match compatible
> >>> - Iclude SoC specific comptaibles
> >>> - Patch 2/7
> >>> - Update nvmem-names list
> >>> - Patch 3/7
> >>> - Update EXPORT_SYMBOL to EXPORT_SYMBOL_GPL
> >>> - Fix return/break logic in qcom_pbs_wait_for_ack()
> >>> - Update iterators to be int
> >>> - Add constants
> >>> - Fix function calls in qcom_pbs_trigger_event()
> >>> - Remove unnessary comments
> >>> - Return -EPROBE_DEFER from get_pbs_client_device()
> >>> Changes since v1:
> >>> - Patch 1/7
> >>> - Fix dt_binding_check errors
> >>> - Update binding description
> >>> - Path 2/7
> >>> - Fix dt_binding_check errors
> >>> - Update per variant constraints
> >>> - Update nvmem description
> >>> - Patch 3/7
> >>> - Update get_pbs_client_device()
> >>> - Drop use of printk
> >>> - Remove unused function
> >>>
> >>> [...]
> >>
> >> Applied, thanks!
> >>
> >> [2/7] dt-bindings: leds: leds-qcom-lpg: Add support for LPG PPG
> >> commit: 2fdd08fec742e0c94a2a06a0c9ee0912b6f7ac39
> >> [4/7] leds: rgb: leds-qcom-lpg: Add support for PPG through single SDAM
> >> commit: 07a1afc8fbb77cc893e2285112482902ac88a295
> >> [5/7] leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG
> >> commit: f4f5f6a6f8d7bcc8efd0eee6751def22c9a38fd0
> >> [6/7] leds: rgb: leds-qcom-lpg: Include support for PPG with dedicated LUT SDAM
> >> commit: 7399a927272de1fc42f4da8af1d8d60b65a15b84
> >> [7/7] leds: rgb: Update PM8350C lpg_data to support two-nvmem PPG Scheme
> >> commit: 7b4066868689b1f341e61957611d252b6fa8cafc
> >
> > This set had a bunch of checkpatch.pl errors.
> >
> > Please fix them up subsequently.
> >
> Hi Lee,
>
> Just wanted to get some quick clarification. Would you like checkpatch.pl issues fixed in a new version
> of this series or would you like a new patch to fix all the issues? Looks like these patches are in your
> for-leds-next-next branch so I am guessing you would like a new follow up patch
> but I just wanted to double check.
A follow-up please.
--
Lee Jones [李琼斯]
On Thu, 21 Dec 2023, 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]>
> ---
> drivers/soc/qcom/Kconfig | 9 ++
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/qcom-pbs.c | 243 ++++++++++++++++++++++++++++++
> include/linux/soc/qcom/qcom-pbs.h | 30 ++++
> 4 files changed, 283 insertions(+)
> create mode 100644 drivers/soc/qcom/qcom-pbs.c
> create mode 100644 include/linux/soc/qcom/qcom-pbs.h
This needs to be applied soon, or I'll have to remove all of the LEDS
patches, since the lack of include/linux/soc/qcom/qcom-pbs.h is causing
build failures.
--
Lee Jones [李琼斯]
On Thu, Dec 21, 2023 at 10:58:33AM -0800, Anjelique Melendez wrote:
> diff --git a/drivers/soc/qcom/qcom-pbs.c b/drivers/soc/qcom/qcom-pbs.c
[..]
> +static int qcom_pbs_wait_for_ack(struct pbs_dev *pbs, u8 bit_pos)
> +{
> + int ret, retries = 2000, delay = 1100;
retries and delay are not variable, please use defines instead.
> + unsigned int val;
> +
> + ret = regmap_read_poll_timeout(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2,
> + val, val & BIT(bit_pos), delay, delay * retries);
> +
> + if (ret < 0) {
> + dev_err(pbs->dev, "Timeout for PBS ACK/NACK for bit %u\n", bit_pos);
> + return -ETIMEDOUT;
> + }
> +
> + if (val == PBS_CLIENT_SCRATCH2_ERROR) {
> + ret = regmap_write(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2, 0);
> + dev_err(pbs->dev, "NACK from PBS for bit %u\n", bit_pos);
> + return -EINVAL;
> + }
> +
> + dev_dbg(pbs->dev, "PBS sequence for bit %u executed!\n", bit_pos);
> + 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
Return: without the 's' is the appropriate form here.
> + */
> +int qcom_pbs_trigger_event(struct pbs_dev *pbs, u8 bitmap)
> +{
> + unsigned int val;
> + u16 bit_pos;
> + int ret;
> +
> + if (!bitmap) {
> + dev_err(pbs->dev, "Invalid bitmap passed by client\n");
No one is going to spot that hidden in the kernel log, and if someone
sees it it does not give an indication to which client it is that's
broken (if there are multiple clients...)
Instead do:
if (WARN_ON(!bitmap))
return -EINVAL;
> + return -EINVAL;
> + }
> +
> + if (IS_ERR_OR_NULL(pbs))
> + return -EINVAL;
> +
> + mutex_lock(&pbs->lock);
> + ret = regmap_read(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2, &val);
> + if (ret < 0)
> + goto out;
> +
> + if (val == PBS_CLIENT_SCRATCH2_ERROR) {
> + /* PBS error - clear SCRATCH2 register */
> + ret = regmap_write(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2, 0);
> + if (ret < 0)
> + goto out;
> + }
> +
> + for (bit_pos = 0; bit_pos < 8; bit_pos++) {
> + if (!(bitmap & BIT(bit_pos)))
> + continue;
> +
> + /* Clear the PBS sequence bit position */
> + ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2,
> + BIT(bit_pos), 0);
> + if (ret < 0)
> + goto error;
> +
> + /* Set the PBS sequence bit position */
> + ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH1,
> + BIT(bit_pos), BIT(bit_pos));
> + if (ret < 0)
> + goto error;
> +
> + /* Initiate the SW trigger */
> + ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_TRIG_CTL,
> + PBS_CLIENT_SW_TRIG_BIT, PBS_CLIENT_SW_TRIG_BIT);
> + if (ret < 0)
> + goto error;
> +
> + ret = qcom_pbs_wait_for_ack(pbs, bit_pos);
> + if (ret < 0)
> + goto error;
In the case that this fails, you're jumping to error, which clears all
of SCRATCH1, but you're leaving SCRATCH2 untouched.
> +
> + /* Clear the PBS sequence bit position */
> + ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH1,
> + BIT(bit_pos), 0);
> + if (ret < 0)
> + goto error;
Does it make sense to handle this error by jumping to error and trying
to clear it once more - while leaving SCRATCH2?
Perhaps you should just ignore the errors from clearing SCRATCH1 and
SCRATCH2? You where able to trigger the PBS and you got your ack...
> +
> + /* Clear the PBS sequence bit position */
> + ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2,
> + BIT(bit_pos), 0);
> + if (ret < 0)
> + goto error;
> + }
> +
> +error:
We're passing "error" in the successful case as well, please name this
"out_clear_scratch1" (or something) instead, to not confuse the reader.
> + /* Clear all the requested bitmap */
> + ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH1, bitmap, 0);
> +
> +out:
> + mutex_unlock(&pbs->lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(qcom_pbs_trigger_event);
> +
> +/**
> + * get_pbs_client_device() - Get the PBS device used by client
> + * @dev: Client device
> + *
> + * This function is used to get the PBS device that is being
> + * used by the client.
> + *
> + * Returns: pbs_dev on success, ERR_PTR on failure
Return:
Regards,
Bjorn
Intentional generic top-post reply.
Please work quickly to resolve Bjorn's comments.
I'm being hounded over a broken LEDs tree due to the missing headerfile.
/end
On Tue, 30 Jan 2024, Bjorn Andersson wrote:
> On Thu, Dec 21, 2023 at 10:58:33AM -0800, Anjelique Melendez wrote:
> > diff --git a/drivers/soc/qcom/qcom-pbs.c b/drivers/soc/qcom/qcom-pbs.c
> [..]
> > +static int qcom_pbs_wait_for_ack(struct pbs_dev *pbs, u8 bit_pos)
> > +{
> > + int ret, retries = 2000, delay = 1100;
>
> retries and delay are not variable, please use defines instead.
>
> > + unsigned int val;
> > +
> > + ret = regmap_read_poll_timeout(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2,
> > + val, val & BIT(bit_pos), delay, delay * retries);
> > +
> > + if (ret < 0) {
> > + dev_err(pbs->dev, "Timeout for PBS ACK/NACK for bit %u\n", bit_pos);
> > + return -ETIMEDOUT;
> > + }
> > +
> > + if (val == PBS_CLIENT_SCRATCH2_ERROR) {
> > + ret = regmap_write(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2, 0);
> > + dev_err(pbs->dev, "NACK from PBS for bit %u\n", bit_pos);
> > + return -EINVAL;
> > + }
> > +
> > + dev_dbg(pbs->dev, "PBS sequence for bit %u executed!\n", bit_pos);
> > + 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
>
> Return: without the 's' is the appropriate form here.
>
> > + */
> > +int qcom_pbs_trigger_event(struct pbs_dev *pbs, u8 bitmap)
> > +{
> > + unsigned int val;
> > + u16 bit_pos;
> > + int ret;
> > +
> > + if (!bitmap) {
> > + dev_err(pbs->dev, "Invalid bitmap passed by client\n");
>
> No one is going to spot that hidden in the kernel log, and if someone
> sees it it does not give an indication to which client it is that's
> broken (if there are multiple clients...)
>
> Instead do:
>
> if (WARN_ON(!bitmap))
> return -EINVAL;
>
> > + return -EINVAL;
> > + }
> > +
> > + if (IS_ERR_OR_NULL(pbs))
> > + return -EINVAL;
> > +
> > + mutex_lock(&pbs->lock);
> > + ret = regmap_read(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2, &val);
> > + if (ret < 0)
> > + goto out;
> > +
> > + if (val == PBS_CLIENT_SCRATCH2_ERROR) {
> > + /* PBS error - clear SCRATCH2 register */
> > + ret = regmap_write(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2, 0);
> > + if (ret < 0)
> > + goto out;
> > + }
> > +
> > + for (bit_pos = 0; bit_pos < 8; bit_pos++) {
> > + if (!(bitmap & BIT(bit_pos)))
> > + continue;
> > +
> > + /* Clear the PBS sequence bit position */
> > + ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2,
> > + BIT(bit_pos), 0);
> > + if (ret < 0)
> > + goto error;
> > +
> > + /* Set the PBS sequence bit position */
> > + ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH1,
> > + BIT(bit_pos), BIT(bit_pos));
> > + if (ret < 0)
> > + goto error;
> > +
> > + /* Initiate the SW trigger */
> > + ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_TRIG_CTL,
> > + PBS_CLIENT_SW_TRIG_BIT, PBS_CLIENT_SW_TRIG_BIT);
> > + if (ret < 0)
> > + goto error;
> > +
> > + ret = qcom_pbs_wait_for_ack(pbs, bit_pos);
> > + if (ret < 0)
> > + goto error;
>
> In the case that this fails, you're jumping to error, which clears all
> of SCRATCH1, but you're leaving SCRATCH2 untouched.
>
> > +
> > + /* Clear the PBS sequence bit position */
> > + ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH1,
> > + BIT(bit_pos), 0);
> > + if (ret < 0)
> > + goto error;
>
> Does it make sense to handle this error by jumping to error and trying
> to clear it once more - while leaving SCRATCH2?
>
> Perhaps you should just ignore the errors from clearing SCRATCH1 and
> SCRATCH2? You where able to trigger the PBS and you got your ack...
>
> > +
> > + /* Clear the PBS sequence bit position */
> > + ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2,
> > + BIT(bit_pos), 0);
> > + if (ret < 0)
> > + goto error;
> > + }
> > +
> > +error:
>
> We're passing "error" in the successful case as well, please name this
> "out_clear_scratch1" (or something) instead, to not confuse the reader.
>
> > + /* Clear all the requested bitmap */
> > + ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH1, bitmap, 0);
> > +
> > +out:
> > + mutex_unlock(&pbs->lock);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(qcom_pbs_trigger_event);
> > +
> > +/**
> > + * get_pbs_client_device() - Get the PBS device used by client
> > + * @dev: Client device
> > + *
> > + * This function is used to get the PBS device that is being
> > + * used by the client.
> > + *
> > + * Returns: pbs_dev on success, ERR_PTR on failure
>
> Return:
>
> Regards,
> Bjorn
--
Lee Jones [李琼斯]
On 1/31/2024 12:58 AM, Lee Jones wrote:
> Intentional generic top-post reply.
>
> Please work quickly to resolve Bjorn's comments.
>
> I'm being hounded over a broken LEDs tree due to the missing headerfile.
>
> /end
>
Updated changes for QCOM PBS driver with Bjorn's comments can be found
at new series:
https://lore.kernel.org/all/[email protected]/
On Wed, Jan 31, 2024 at 08:58:42AM +0000, Lee Jones wrote:
> Intentional generic top-post reply.
>
> Please work quickly to resolve Bjorn's comments.
>
> I'm being hounded over a broken LEDs tree due to the missing headerfile.
>
I've merged the updated patches into the qcom tree.
I presume though that you'd like to have this build issue resolved in
your tree as well. Please feel free to pull the branch:
[email protected] (5b2dd77be1d85ac3a8be3749f5605bf0830e2998)
from https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git
Regards,
Bjorn
On Thu, 01 Feb 2024, Bjorn Andersson wrote:
> On Wed, Jan 31, 2024 at 08:58:42AM +0000, Lee Jones wrote:
> > Intentional generic top-post reply.
> >
> > Please work quickly to resolve Bjorn's comments.
> >
> > I'm being hounded over a broken LEDs tree due to the missing headerfile.
> >
>
> I've merged the updated patches into the qcom tree.
>
> I presume though that you'd like to have this build issue resolved in
> your tree as well. Please feel free to pull the branch:
>
> [email protected] (5b2dd77be1d85ac3a8be3749f5605bf0830e2998)
>
> from https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git
Great, thanks.
--
Lee Jones [李琼斯]
On Thu, 01 Feb 2024, Bjorn Andersson wrote:
> On Wed, Jan 31, 2024 at 08:58:42AM +0000, Lee Jones wrote:
> > Intentional generic top-post reply.
> >
> > Please work quickly to resolve Bjorn's comments.
> >
> > I'm being hounded over a broken LEDs tree due to the missing headerfile.
> >
>
> I've merged the updated patches into the qcom tree.
>
> I presume though that you'd like to have this build issue resolved in
> your tree as well. Please feel free to pull the branch:
>
> [email protected] (5b2dd77be1d85ac3a8be3749f5605bf0830e2998)
>
> from https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git
Pulled, thanks.
--
Lee Jones [李琼斯]