linux-kernel - Re: [PATCH v4 5/7] leds: rgb: leds-qcom-lpg: Update PMI632 lpg

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <18bdcbdf-9ff6-5498-41db-8388eb8bf1ed@quicinc.com>
Date:   Fri, 8 Sep 2023 11:58:06 -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 5/7] leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data
 to support PPG

On 9/8/2023 1:28 AM, Konrad Dybcio wrote:
> On 8.09.2023 02:30, Anjelique Melendez wrote:
>> On 9/7/2023 1:31 PM, Konrad Dybcio wrote:
>>> On 7.09.2023 22:26, Konrad Dybcio wrote:
>>>> On 7.09.2023 21:54, Anjelique Melendez wrote:
>>>>> On 8/30/2023 11:34 AM, Konrad Dybcio wrote:
>>>>>> On 30.08.2023 20:06, Anjelique Melendez wrote:
>>>>>>> Update the pmi632 lpg_data struct so that pmi632 devices use PPG
>>>>>>> for LUT pattern.
>>>>>>>
>>>>>>> Signed-off-by: Anjelique Melendez <quic_amelende@...cinc.com>
>>>>>>> ---
>>>>>>>  drivers/leds/rgb/leds-qcom-lpg.c | 9 ++++++---
>>>>>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
>>>>>>> index 90dc27d5eb7c..0b37d3b539f8 100644
>>>>>>> --- a/drivers/leds/rgb/leds-qcom-lpg.c
>>>>>>> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
>>>>>>> @@ -1672,11 +1672,14 @@ static const struct lpg_data pm8994_lpg_data = {
>>>>>>>  static const struct lpg_data pmi632_lpg_data = {
>>>>>>>  	.triled_base = 0xd000,
>>>>>>>  
>>>>>>> +	.lut_size = 64,
>>>>>>> +	.lut_sdam_base = 0x80,
>>>>>> Is that a predefined space for use with LPG?
>>>>>>
>>>>>> Or can it be reclaimed for something else?
>>>>>>
>>>>>> Konrad
>>>>> Yes, this is a predefined space for use with LPG
>>>> We represent the SDAM as a NVMEM device, generally it would
>>>> be nice to add all regions within it as subnodes in the devicetree.
>>> Wait hmm.. we already get it as a nvmem cell.. Or at least that's
>>> how I understand it (lut_sdam_base == lpg_chan_nvmem->start, pseudocode)
>>>
>>> Why don't we access it through the nvmem r/w ops then?
>>>
>>> Konrad
>> I think I might be a little confused on what you are asking so please let
>> me know if this does not answer your question.
>>
>> lut_sdam_base is the offset where lut pattern begins in the SDAM. So when we are writing back
>> our LED pattern we end up calling nvmem_device_write(lpg_chan_nvmem, lut_sdam_base + offset, 1, brightness).
>> So far for every single SDAM PPG devices we have seen the lpg_sdam_base be 0x80 and every
>> LUT SDAM PPG devices (pm8350c) we have seen lpg_sdam_base be 0x45, which is why we 
>> included this value in the lpg_data rather than as a devicetree property since it has
>> been consistent across a few pmics.
>>
>> I am ok if you would like the lut_sdam_base to be moved to a devicetree property.
> So.. we have a slice of SDAM represented as an NVMEM cell (and that
> part of SDAM is reserved solely for LPG), and then within that cell,
> we need to add an additional offset to get to what we want. Correct?
> 
> What's in LPG_NVMEM_CELL[0:offset-1] then?
> 
> Konrad
All SDAMs being used for lpg have the first few registers (0x40 - 0x44) used by PBS
and also contain register map info and sdam size. For the lpg_chan_nvmem SDAM, after
the first few registers we have all of the per channel data such as LUT_EN,
PATTERN_CONFIG, START_INDEX, and END_INDEX. All of these register addresses
that we write back to are defined at the top of leds-qcom-lpg.c and qcom-pbs.c.

When we have single SDAM PPG, pattern entries begin after all of the per channel data at 0x80.
When we have a second SDAM used for LUT, pattern entries begin after the PBS registers at 0x45.

I just went through all of the code again and lut_sdam_base is really only used twice, so we could
define these register addresses instead of having them in device_data if you think that would
make more sense. Would just need to work on variable name that makes the most sense

#define SDAM_LPG_CHAN_SDAM_LUT_PATTERN_OFFSET 0x80
#define SDAM_LUT_SDAM_LUT_PATTERN_OFFSET 0x45

Anjelique