On 9/7/2023 1:42 PM, Konrad Dybcio wrote: > On 7.09.2023 21:55, Anjelique Melendez wrote: >> >> >> 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. > So essentially without this patchset, PM8350C and PMI632 are not capable > of producing LED patterns. Is that correct? Yes, that is correct. Since PMI632 and PM8350C do not have LUT peripherals, current code will not allow them to produce patterns. Luca mentioned this briefly when adding the PMI632 LPG device https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/leds/rgb/leds-qcom-lpg.c?h=v6.5&id=d11a79dd047e18dd0b76bc9abebb8470858856d6 > > Though I think that (in a separate patch, or perhaps series), it would > be worth redoing the code such that hi/lo_pause expresses the deviation > from the duration of the rest instead of the duration itself. Then we > could just: > > if ((lo_pause || hi_pause)) && lpg->lpg_chan_nvmem) > goto out_free_pattern; > > But that's just a suggestion from somebody that didn't work on this code. > The value that is written back for hi/low_pause is "how many steps should we hold the first/last pattern values" where step = delta_t. So if delta_t == hi/low_pause we would need to write back 1. I can look into seeing if expressing hi/lo_pause as a deviation can easily translate for a different patch series. > Also, I think that using lpg_chan_nvmem interchangeably with SDAM is a > bit confusing. Do we expect NVMEMs/SRAMs that aren't SDAM to make an > appearence here? > I believe we only expect SDAMs. I can change the use of nvmem to sdam in following patch for comments and variable names. Thanks, Anjelique
