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

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

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 single SDAM PPG
  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          |  89 ++++-
 .../bindings/soc/qcom/qcom,pbs.yaml           |  46 +++
 drivers/leds/rgb/leds-qcom-lpg.c              | 359 ++++++++++++++++--
 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, 749 insertions(+), 28 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 Fri Sep 29, 2023 at 2:38 AM CEST, 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 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
>
> Tested-by: Luca Weiss <luca.weiss@fairphone.com> # sdm632-fairphone-fp3 (pmi632)

Hi Anjelique,

Actually I've retested this now on PMI632 (and also realized that my
previous tests weren't correct and wasn't actually using hw_pattern).

Using the following commands (after boot) I'm expecting to get a
500ms on 500ms off blinking pattern between white (255 255 255) and off
(0 0 0).

  echo pattern > /sys/class/leds/rgb:status/trigger
  echo -1 > /sys/class/leds/rgb:status/repeat

  echo "255 255 255" > /sys/class/leds/rgb:status/multi_intensity
  echo "255 500 255 0 0 500 0 0" > /sys/class/leds/rgb:status/hw_pattern

What I actually see is it blinking between cyan (0 255 255) and red (255
0 0).
At some point after playing with many patterns I got it to actually
cycle between white and off, but I couldn't reproduce this again (or I
didn't try hard enough).


But with this example it correctly blinks red on-off.

  echo "255 0 0" > /sys/class/leds/rgb:status/multi_intensity
  echo "255 500 255 0 0 500 0 0" > /sys/class/leds/rgb:status/hw_pattern

With "0 255 0" and "0 0 255" the other colors also work fine, it's just
the combinations that seem somewhat broken.

Regards
Luca


>
> 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 single SDAM PPG
>   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          |  89 ++++-
>  .../bindings/soc/qcom/qcom,pbs.yaml           |  46 +++
>  drivers/leds/rgb/leds-qcom-lpg.c              | 359 ++++++++++++++++--
>  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, 749 insertions(+), 28 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 10/1/2023 7:15 AM, Luca Weiss wrote:
> On Fri Sep 29, 2023 at 2:38 AM CEST, 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!
[..]
>>
>> Tested-by: Luca Weiss <luca.weiss@fairphone.com> # sdm632-fairphone-fp3 (pmi632)
> 
> Hi Anjelique,
> 
> Actually I've retested this now on PMI632 (and also realized that my
> previous tests weren't correct and wasn't actually using hw_pattern).
> 
> Using the following commands (after boot) I'm expecting to get a
> 500ms on 500ms off blinking pattern between white (255 255 255) and off
> (0 0 0).
> 
>   echo pattern > /sys/class/leds/rgb:status/trigger
>   echo -1 > /sys/class/leds/rgb:status/repeat
> 
>   echo "255 255 255" > /sys/class/leds/rgb:status/multi_intensity
>   echo "255 500 255 0 0 500 0 0" > /sys/class/leds/rgb:status/hw_pattern
> 
> What I actually see is it blinking between cyan (0 255 255) and red (255
> 0 0).
> At some point after playing with many patterns I got it to actually
> cycle between white and off, but I couldn't reproduce this again (or I
> didn't try hard enough).
> 
> 
> But with this example it correctly blinks red on-off.
> 
>   echo "255 0 0" > /sys/class/leds/rgb:status/multi_intensity
>   echo "255 500 255 0 0 500 0 0" > /sys/class/leds/rgb:status/hw_pattern
> 
> With "0 255 0" and "0 0 255" the other colors also work fine, it's just
> the combinations that seem somewhat broken.
> 
> Regards
> Luca
> 
> 
Hi Luca,

Thanks for testing again and the feedback!
Looks like for multicolor devices there is a small concurrency issue with
enabling pattern at the same time for all the led channels. This could be
why you observed your device blinking between red (255 0 0) and cyan (0 255 255),
instead of seeing all channels (255 255 255) blink.
The fix I'm planing to include in the next series is is to disable the multicolor led
channels first, then configure all channels, and finally re-enable channels
so that pattern is triggered at the same time for each all of the channels.

I am currently testing with pm8350c device so if you are able to test next series
on pmi632 it would be very appreciated!

Thanks,
Anjelique
  

On Thu Oct 12, 2023 at 11:50 PM CEST, Anjelique Melendez wrote:
>
>
> On 10/1/2023 7:15 AM, Luca Weiss wrote:
> > On Fri Sep 29, 2023 at 2:38 AM CEST, 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!
> [..]
> >>
> >> Tested-by: Luca Weiss <luca.weiss@fairphone.com> # sdm632-fairphone-fp3 (pmi632)
> > 
> > Hi Anjelique,
> > 
> > Actually I've retested this now on PMI632 (and also realized that my
> > previous tests weren't correct and wasn't actually using hw_pattern).
> > 
> > Using the following commands (after boot) I'm expecting to get a
> > 500ms on 500ms off blinking pattern between white (255 255 255) and off
> > (0 0 0).
> > 
> >   echo pattern > /sys/class/leds/rgb:status/trigger
> >   echo -1 > /sys/class/leds/rgb:status/repeat
> > 
> >   echo "255 255 255" > /sys/class/leds/rgb:status/multi_intensity
> >   echo "255 500 255 0 0 500 0 0" > /sys/class/leds/rgb:status/hw_pattern
> > 
> > What I actually see is it blinking between cyan (0 255 255) and red (255
> > 0 0).
> > At some point after playing with many patterns I got it to actually
> > cycle between white and off, but I couldn't reproduce this again (or I
> > didn't try hard enough).
> > 
> > 
> > But with this example it correctly blinks red on-off.
> > 
> >   echo "255 0 0" > /sys/class/leds/rgb:status/multi_intensity
> >   echo "255 500 255 0 0 500 0 0" > /sys/class/leds/rgb:status/hw_pattern
> > 
> > With "0 255 0" and "0 0 255" the other colors also work fine, it's just
> > the combinations that seem somewhat broken.
> > 
> > Regards
> > Luca
> > 
> > 
> Hi Luca,
>
> Thanks for testing again and the feedback!
> Looks like for multicolor devices there is a small concurrency issue with
> enabling pattern at the same time for all the led channels. This could be
> why you observed your device blinking between red (255 0 0) and cyan (0 255 255),
> instead of seeing all channels (255 255 255) blink.
> The fix I'm planing to include in the next series is is to disable the multicolor led
> channels first, then configure all channels, and finally re-enable channels
> so that pattern is triggered at the same time for each all of the channels.

Sounds reasonable I think!

>
> I am currently testing with pm8350c device so if you are able to test next series
> on pmi632 it would be very appreciated!

Will do, thanks for fixing it up!

Regards
Luca

>
> Thanks,
> Anjelique