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