[v3,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 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

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          |  89 +++-
 .../bindings/soc/qcom/qcom,pbs.yaml           |  46 ++
 drivers/leds/rgb/leds-qcom-lpg.c              | 395 ++++++++++++++++--
 drivers/soc/qcom/Kconfig                      |   9 +
 drivers/soc/qcom/Makefile                     |   1 +
 drivers/soc/qcom/qcom-pbs.c                   | 286 +++++++++++++
 include/linux/soc/qcom/qcom-pbs.h             |  30 ++
 7 files changed, 823 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 15/08/2023 01:59, Anjelique Melendez wrote:
> Update leds-qcom-lpg binding to support LPG PPG.
> 
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

On 8/15/2023 8:38 AM, Bjorn Andersson wrote:
> On Mon, Aug 14, 2023 at 04:59:15PM -0700, Anjelique Melendez wrote:

[...]>> @@ -65,7 +83,12 @@ struct lpg_data;
>>   * @lut_base:	base address of the LUT block (optional)
>>   * @lut_size:	number of entries in the LUT block
>>   * @lut_bitmap:	allocation bitmap for LUT entries
>> - * @triled_base: base address of the TRILED block (optional)
>> + * @pbs_dev:	PBS device
>> + * @lpg_chan_nvmem:	LPG nvmem peripheral device
>> + * @pbs_en_bitmap:	bitmap for tracking PBS triggers
>> + * @lut_sdam_base:	offset where LUT pattern begins in nvmem
>> + * @ppg_en:	Flag indicating whether PPG is enabled/used
> 
> Looking at its usage, it doesn't feel so much "is PPG enabled" as "does
> this instance use PPG", it's not a thing that can be enabled/disabled in
> runtime.
> 
> So "has_ppg" seems like a better name, or perhaps even "use_sdam" and
> avoid "PPG" completely and make it clearer to the average reader?
Sure, can update to be "use_sdam"


[...]
>> +static void lpg_sdam_configure_triggers(struct lpg_channel *chan)
>> +{
>> +	if (!chan->lpg->ppg_en)
>> +		return;
>> +
>> +	if (chan->enabled && chan->pattern_set) {
>> +		lpg_sdam_write(chan->lpg, SDAM_LUT_EN_OFFSET + chan->sdam_offset, 1);
>> +		lpg_set_pbs_trigger(chan);
>> +		chan->pattern_set = false;
> 
> Forgive me if I'm confused, but doesn't this mean that if I configure a
> pattern and then set the brightness twice the pattern will be disabled
> again?
Yes, you are correct. With current code we continuously disable pattern.
I took a look at the code again and found that it makes more sense to
disable pattern in clear_pattern().


[...]
>> @@ -1363,7 +1618,9 @@ static int lpg_probe(struct platform_device *pdev)
>>  	for (i = 0; i < lpg->num_channels; i++)
>>  		lpg_apply_dtest(&lpg->channels[i]);
>>  
>> -	return lpg_add_pwm(lpg);
>> +	ret = lpg_add_pwm(lpg);
>> +
>> +	return ret;
> 
> I'm failing to see the usefulness of this change.
Sorry, looks like this was never reverted from an old change when I was debugging.
Will revert back to original for next version. 

Thanks,
Anjelique