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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5f9e3845-2478-2739-ec5b-38e17bb37379@quicinc.com>
Date:   Thu, 7 Sep 2023 15:01:10 -0700
From:   Anjelique Melendez <quic_amelende@...cinc.com>
To:     Konrad Dybcio <konrad.dybcio@...aro.org>, <pavel@....cz>,
        <lee@...nel.org>, <thierry.reding@...il.com>, <robh+dt@...nel.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
        <agross@...nel.org>, <andersson@...nel.org>
CC:     <luca.weiss@...rphone.com>, <u.kleine-koenig@...gutronix.de>,
        <quic_subbaram@...cinc.com>, <quic_gurus@...cinc.com>,
        <linux-leds@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-pwm@...r.kernel.org>, <kernel@...cinc.com>
Subject: Re: [PATCH v4 4/7] leds: rgb: leds-qcom-lpg: Add support for PPG
 through single SDAM



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