Re: [PATCH v4 4/7] leds: rgb: leds-qcom-lpg: Add support for PPG through single SDAM

[Anjelique Melendez <quic_amelende@quicinc.com>]

On 8/30/2023 11:34 AM, Konrad Dybcio wrote:
> On 30.08.2023 20:05, Anjelique Melendez wrote:
>> In some PMICs like pmi632, the pattern look up table (LUT) and LPG
>> configuration can be 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.
> I still fail to understand what benefit this brings.
> 
> Is this a "can be used", or "should be used", or maybe "must be used"?
> 
> Are there any distinct advantages to using one over the other?
> I see some limitations in the code below, but that's not being made
> obvious.
> 
> This all should be in the commit message, the current one includes
> a lot of cryptic names that mean nothing to most people.
> 
> [...]
This is a must be used if you would like to trigger patterns. Will update commit message to try and 
make that more clear for next patch.

>> @@ -775,7 +952,7 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
>>  	unsigned int lo_idx;
>>  	unsigned int hi_idx;
>>  	unsigned int i;
>> -	bool ping_pong = true;
>> +	bool ping_pong = false;
> Why?
> 
> This change combined with assigning true below for LUT mode
> is a NOP

LUT devices support a "ping pong" setting that PPG devices do not but honestly looking back at this
I'm not quite sure why I decided to make the change like this :)

Will revert back to bool ping_pong = true and just wrap loop in an if (lpg->lut_base) { for...} else {ping_pong = false;}
i.e.

	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;

> 
>>  	int ret = -EINVAL;
>>  
>>  	/* Hardware only support oneshot or indefinite loops */
>> @@ -824,7 +1001,7 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
>>  	 * used to stretch the first delay of the pattern and a "high pause"
>>  	 * the last one.
>>  	 *
>> -	 * In order to save space the pattern can be played in "ping pong"
>> +	 * In order to save space for the pattern can be played in "ping pong"
>>  	 * mode, in which the pattern is first played forward, then "high
>>  	 * pause" is applied, then the pattern is played backwards and finally
>>  	 * the "low pause" is applied.
>> @@ -837,16 +1014,22 @@ 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".
>> +	 *
>> +	 * NVMEM devices supporting LUT do not support "low pause", "high pause"
>> +	 * or "ping pong"
>>  	 */
>>  
>>  	/* 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) {
>> +		ping_pong = true;
>> +		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;
>> +			}
>>  		}
>>  	}
>>  
>> @@ -860,14 +1043,21 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
>>  	 * Validate that all delta_t in the pattern are the same, with the
>>  	 * exception of the middle element in case of ping_pong.
>>  	 */
>> -	delta_t = pattern[1].delta_t;
>> -	for (i = 2; i < len; i++) {
>> +	if (lpg->lpg_chan_nvmem) {
>> +		i = 1;
>> +		delta_t = pattern[0].delta_t;
>> +	} else {
>> +		i = 2;
>> +		delta_t = pattern[1].delta_t;
>> +	}
> Why?
> 
> What's the rationale behind this change?
Patterns are required to have the same duration for each step of the pattern. Devices with LUT peripherals support low/high
pause which is when the first/last entry of the pattern can have a longer duration. This loop checks that the all of the
pattern durations are the same with the exception of the first and last entry for low/hi pause.

This change was made because devices that use single SDAM do not support low/high pause, so we must check every
single pattern duration. Instead of changing the loop arguments with an if statement I was thinking we could either:

a. keep the original loop arguments and when loop exits we can check first element for single SDAM devices

   delta_t = pattern[1].delta_t;
   for (i = 2; i < len; i++) {
	if (pattern[i].delta_t != delta_t) {
+ 		if (i != actual_len - 1 || lpg->lpg_chan_nvmem)
  			goto out_free_pattern;
  		}
  	}

+ if (lpg->lpg_chan_nvmem) {
+	if (delta_t != pattern[0].delta_t)
+		goto out_free_pattern
+ }


b. Change the loop argument to start with i=0 and for LUT device we could just skip checking first and last element duration
  ** We would end up checking if pattern[1].delta_t == pattern[1].delta_t inside the loop when i == 1

   delta_t = pattern[1].delta_t;
+  for (i = 0; i < len; i++) {
	if (pattern[i].delta_t != delta_t) {
+		if (lpg->lut_base && (i == 0 || i == actual_len - 1)
+			continue;
+               else
+			goto out_free_pattern;

  	}

Let me know if you would rather go with one of these options to make this change cleaner.

Thanks,
Anjelique

> 
>> +
>> +	for (; i < len; i++) {
>>  		if (pattern[i].delta_t != delta_t) {
>>  			/*
>>  			 * Allow last entry in the full or shortened pattern to
>>  			 * specify hi pause. Reject other variations.
>>  			 */
>> -			if (i != actual_len - 1)
>> +			if (i != actual_len - 1 || lpg->lpg_chan_nvmem)
>>  				goto out_free_pattern;
>>  		}
>>  	}