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

Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
---
 drivers/soc/qcom/Kconfig          |   9 +
 drivers/soc/qcom/Makefile         |   1 +
 drivers/soc/qcom/qcom-pbs.c       | 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

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

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/202307270539.1JVFQf6W-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230727/202307270539.1JVFQf6W-lkp@intel.com/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 <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202307270539.1JVFQf6W-lkp@intel.com/

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

> +
> +	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

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 <quic_amelende@quicinc.com>
>> ---
> [...]
> 
>> +
>> +	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 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