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

[Anjelique Melendez]

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

Thanks,
Anjelique