[v2,0/7] Add support for LUT PPG

In certain PMICs, LUT pattern and LPG configuration can be stored in SDAM
modules instead of LUT peripheral. This feature is called PPG.

This change series adds support for PPG. Thanks!

Changes since v1:
  - Patch 1/7
    - Fix dt_binding_check errors
    - Update binding description
  - Path 2/7
    - Fix dt_binding_check errors
    - Update per variant constraints
    - Update nvmem description
  - Patch 3/7
    - Update get_pbs_client_device()
    - Drop use of printk
    - Remove unused function

Tested-by: Luca Weiss <luca.weiss@fairphone.com> # sdm632-fairphone-fp3 (pmi632)

Anjelique Melendez (7):
  dt-bindings: soc: qcom: Add qcom-pbs bindings
  dt-bindings: leds: leds-qcom-lpg: Add support for LPG PPG
  soc: qcom: add QCOM PBS driver
  leds: rgb: leds-qcom-lpg: Add support for PPG through single SDAM
  leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG
  leds: rgb: leds-qcom-lpg: Support two-nvmem PPG Scheme
  leds: rgb: Update PM8350C lpg_data to support two-nvmem PPG Scheme

 .../bindings/leds/leds-qcom-lpg.yaml          |  92 +++-
 .../bindings/soc/qcom/qcom-pbs.yaml           |  40 ++
 drivers/leds/rgb/leds-qcom-lpg.c              | 395 ++++++++++++++++--
 drivers/soc/qcom/Kconfig                      |   9 +
 drivers/soc/qcom/Makefile                     |   1 +
 drivers/soc/qcom/qcom-pbs.c                   | 302 +++++++++++++
 include/linux/soc/qcom/qcom-pbs.h             |  30 ++
 7 files changed, 836 insertions(+), 33 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-pbs.yaml
 create mode 100644 drivers/soc/qcom/qcom-pbs.c
 create mode 100644 include/linux/soc/qcom/qcom-pbs.h

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