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