Re: [PATCH v4 5/7] leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG

[Anjelique Melendez <quic_amelende@quicinc.com>]

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@quicinc.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