| Message ID | 20230830180600.1865-8-quic_amelende@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for LUT PPG | expand |
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
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. Krzysztof, opinions? Konrad
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 > > Krzysztof, opinions? > > Konrad
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. Thanks, Anjelique
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
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
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, + .num_channels = 5, .channels = (const struct lpg_channel_data[]) { - { .base = 0xb300, .triled_mask = BIT(7) }, - { .base = 0xb400, .triled_mask = BIT(6) }, - { .base = 0xb500, .triled_mask = BIT(5) }, + { .base = 0xb300, .triled_mask = BIT(7), .sdam_offset = 0x48 }, + { .base = 0xb400, .triled_mask = BIT(6), .sdam_offset = 0x56 }, + { .base = 0xb500, .triled_mask = BIT(5), .sdam_offset = 0x64 }, { .base = 0xb600 }, { .base = 0xb700 }, },
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(-)