| Message ID | 20230830180600.1865-7-quic_amelende@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for LUT PPG | expand |
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. [...] > > +static int lpg_sdam_write(struct lpg *lpg, u16 addr, u8 val) Again, looks like excessive helpers for r/w accessors. > +{ > + int rc; > + > + rc = nvmem_device_write(lpg->lpg_chan_nvmem, addr, 1, &val); > + if (rc < 0) { > + dev_err(lpg->dev, "writing %u to SDAM addr %#x failed, rc=%d\n", > + val, addr, rc); > + return rc; > + } > + > + return 0; > +} > + > +#define PBS_SW_TRIG_BIT BIT(0) > + > +static int lpg_clear_pbs_trigger(struct lpg_channel *chan) > +{ > + int rc; > + > + clear_bit(chan->lpg_idx, &chan->lpg->pbs_en_bitmap); > + if (!chan->lpg->pbs_en_bitmap) { > + rc = lpg_sdam_write(chan->lpg, SDAM_REG_PBS_SEQ_EN, 0); > + if (rc < 0) > + return rc; > + } > + > + return 0; > +} > + > +static int lpg_set_pbs_trigger(struct lpg_channel *chan) > +{ > + int rc; > + > + if (!chan->lpg->pbs_en_bitmap) { > + rc = lpg_sdam_write(chan->lpg, SDAM_REG_PBS_SEQ_EN, PBS_SW_TRIG_BIT); > + if (rc < 0) > + return rc; > + > + rc = qcom_pbs_trigger_event(chan->lpg->pbs_dev, PBS_SW_TRIG_BIT); > + if (rc < 0) > + return rc; > + } > + set_bit(chan->lpg_idx, &chan->lpg->pbs_en_bitmap); > + > + return 0; > +} > + > +static int lpg_sdam_configure_triggers(struct lpg_channel *chan, bool set_trig) > +{ > + int rc; > + > + if (chan->lpg->lut_base) > + return 0; > + > + if (set_trig) { > + rc = lpg_sdam_write(chan->lpg, SDAM_LUT_EN_OFFSET + chan->sdam_offset, 1); > + if (rc < 0) > + return rc; > + > + rc = lpg_set_pbs_trigger(chan); > + if (rc < 0) > + return rc; > + chan->pattern_set = false; > + } else { > + rc = lpg_sdam_write(chan->lpg, SDAM_LUT_EN_OFFSET + chan->sdam_offset, 0); > + if (rc < 0) > + return rc; > + > + rc = lpg_clear_pbs_trigger(chan); > + if (rc < 0) > + return rc; > + } > + > + return 0; > +} > + > static int triled_set(struct lpg *lpg, unsigned int mask, unsigned int enable) > { > /* Skip if we don't have a triled block */ > @@ -217,6 +333,41 @@ static int triled_set(struct lpg *lpg, unsigned int mask, unsigned int enable) > mask, enable); > } > > +static int lpg_lut_store_sdam(struct lpg *lpg, struct led_pattern *pattern, > + size_t len, unsigned int *lo_idx, unsigned int *hi_idx) > +{ > + u8 brightness; > + u16 addr; > + unsigned int idx; > + int i, rc; Reverse-Christmas-tree? > + > + if (len > SDAM_LUT_COUNT_MAX) { > + dev_err(lpg->dev, "Pattern length (%zu) exceeds maximum pattern length (%d)\n", > + len, SDAM_LUT_COUNT_MAX); > + return -EINVAL; > + } > + > + idx = bitmap_find_next_zero_area(lpg->lut_bitmap, lpg->lut_size, > + 0, len, 0); > + if (idx >= lpg->lut_size) > + return -ENOSPC; > + > + for (i = 0; i < len; i++) { > + brightness = pattern[i].brightness; > + addr = lpg->lut_sdam_base + i + idx; > + rc = lpg_sdam_write(lpg, addr, brightness); > + if (rc < 0) > + return rc; > + } > + > + bitmap_set(lpg->lut_bitmap, idx, len); > + > + *lo_idx = idx; > + *hi_idx = idx + len - 1; > + > + return 0; > +} > + > static int lpg_lut_store(struct lpg *lpg, struct led_pattern *pattern, > size_t len, unsigned int *lo_idx, unsigned int *hi_idx) > { > @@ -463,6 +614,26 @@ static void lpg_apply_pwm_value(struct lpg_channel *chan) > #define LPG_PATTERN_CONFIG_PAUSE_HI BIT(1) > #define LPG_PATTERN_CONFIG_PAUSE_LO BIT(0) > > +static void lpg_sdam_apply_lut_control(struct lpg_channel *chan) > +{ > + u8 val, conf = 0; > + struct lpg *lpg = chan->lpg; Reverse-Christmas-tree? > + > + if (!chan->ramp_oneshot) > + conf |= LPG_PATTERN_CONFIG_REPEAT; > + > + lpg_sdam_write(lpg, SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET + chan->sdam_offset, 0); > + lpg_sdam_write(lpg, SDAM_PATTERN_CONFIG_OFFSET + chan->sdam_offset, conf); > + > + lpg_sdam_write(lpg, SDAM_END_INDEX_OFFSET + chan->sdam_offset, chan->pattern_hi_idx); > + lpg_sdam_write(lpg, SDAM_START_INDEX_OFFSET + chan->sdam_offset, chan->pattern_lo_idx); > + > + val = RAMP_STEP_DURATION(chan->ramp_tick_ms); > + if (val > 0) > + val--; ???? that sounds very cryptic.. almost like some sort of a bad fixup maybe the RAMP_STEP_DURATION should contain that "-1"? not only that, but val is an unsigned value, so it's always > 0 [...] > @@ -775,7 +952,7 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern, > unsigned int lo_idx; > unsigned int hi_idx; > unsigned int i; > - bool ping_pong = true; > + bool ping_pong = false; Why? This change combined with assigning true below for LUT mode is a NOP > int ret = -EINVAL; > > /* Hardware only support oneshot or indefinite loops */ > @@ -824,7 +1001,7 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern, > * used to stretch the first delay of the pattern and a "high pause" > * the last one. > * > - * In order to save space the pattern can be played in "ping pong" > + * In order to save space for the pattern can be played in "ping pong" > * mode, in which the pattern is first played forward, then "high > * pause" is applied, then the pattern is played backwards and finally > * the "low pause" is applied. > @@ -837,16 +1014,22 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern, > * If the specified pattern is a palindrome the ping pong mode is > * enabled. In this scenario the delta_t of the middle entry (i.e. the > * last in the programmed pattern) determines the "high pause". > + * > + * NVMEM devices supporting LUT do not support "low pause", "high pause" > + * or "ping pong" > */ > > /* Detect palindromes and use "ping pong" to reduce LUT usage */ > - for (i = 0; i < len / 2; i++) { > - brightness_a = pattern[i].brightness; > - brightness_b = pattern[len - i - 1].brightness; > - > - if (brightness_a != brightness_b) { > - ping_pong = false; > - break; > + if (lpg->lut_base) { > + ping_pong = true; > + for (i = 0; i < len / 2; i++) { > + brightness_a = pattern[i].brightness; > + brightness_b = pattern[len - i - 1].brightness; > + > + if (brightness_a != brightness_b) { > + ping_pong = false; > + break; > + } > } > } > > @@ -860,14 +1043,21 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern, > * Validate that all delta_t in the pattern are the same, with the > * exception of the middle element in case of ping_pong. > */ > - delta_t = pattern[1].delta_t; > - for (i = 2; i < len; i++) { > + if (lpg->lpg_chan_nvmem) { > + i = 1; > + delta_t = pattern[0].delta_t; > + } else { > + i = 2; > + delta_t = pattern[1].delta_t; > + } Why? What's the rationale behind this change? > + > + for (; i < len; i++) { > if (pattern[i].delta_t != delta_t) { > /* > * Allow last entry in the full or shortened pattern to > * specify hi pause. Reject other variations. > */ > - if (i != actual_len - 1) > + if (i != actual_len - 1 || lpg->lpg_chan_nvmem) > goto out_free_pattern; > } > } > @@ -876,12 +1066,19 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern, > if (delta_t >= BIT(9)) > goto out_free_pattern; > > - /* Find "low pause" and "high pause" in the pattern */ > - lo_pause = pattern[0].delta_t; > - hi_pause = pattern[actual_len - 1].delta_t; > + /* Find "low pause" and "high pause" in the pattern if not an NVMEM device*/ missing a space before '*/' "if not an NVMEM device" -> "in the LUT case"? Konrad
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. >> @@ -775,7 +952,7 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern, >> unsigned int lo_idx; >> unsigned int hi_idx; >> unsigned int i; >> - bool ping_pong = true; >> + bool ping_pong = false; > Why? > > This change combined with assigning true below for LUT mode > is a NOP LUT devices support a "ping pong" setting that PPG devices do not but honestly looking back at this I'm not quite sure why I decided to make the change like this :) Will revert back to bool ping_pong = true and just wrap loop in an if (lpg->lut_base) { for...} else {ping_pong = false;} i.e. if (lpg->lut_base) { for (i = 0; i < len / 2; i++) { brightness_a = pattern[i].brightness; brightness_b = pattern[len - i - 1].brightness; if (brightness_a != brightness_b) { ping_pong = false; break; } } } else ping_pong = false; > >> int ret = -EINVAL; >> >> /* Hardware only support oneshot or indefinite loops */ >> @@ -824,7 +1001,7 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern, >> * used to stretch the first delay of the pattern and a "high pause" >> * the last one. >> * >> - * In order to save space the pattern can be played in "ping pong" >> + * In order to save space for the pattern can be played in "ping pong" >> * mode, in which the pattern is first played forward, then "high >> * pause" is applied, then the pattern is played backwards and finally >> * the "low pause" is applied. >> @@ -837,16 +1014,22 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern, >> * If the specified pattern is a palindrome the ping pong mode is >> * enabled. In this scenario the delta_t of the middle entry (i.e. the >> * last in the programmed pattern) determines the "high pause". >> + * >> + * NVMEM devices supporting LUT do not support "low pause", "high pause" >> + * or "ping pong" >> */ >> >> /* Detect palindromes and use "ping pong" to reduce LUT usage */ >> - for (i = 0; i < len / 2; i++) { >> - brightness_a = pattern[i].brightness; >> - brightness_b = pattern[len - i - 1].brightness; >> - >> - if (brightness_a != brightness_b) { >> - ping_pong = false; >> - break; >> + if (lpg->lut_base) { >> + ping_pong = true; >> + for (i = 0; i < len / 2; i++) { >> + brightness_a = pattern[i].brightness; >> + brightness_b = pattern[len - i - 1].brightness; >> + >> + if (brightness_a != brightness_b) { >> + ping_pong = false; >> + break; >> + } >> } >> } >> >> @@ -860,14 +1043,21 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern, >> * Validate that all delta_t in the pattern are the same, with the >> * exception of the middle element in case of ping_pong. >> */ >> - delta_t = pattern[1].delta_t; >> - for (i = 2; i < len; i++) { >> + if (lpg->lpg_chan_nvmem) { >> + i = 1; >> + delta_t = pattern[0].delta_t; >> + } else { >> + i = 2; >> + delta_t = pattern[1].delta_t; >> + } > Why? > > What's the rationale behind this change? Patterns are required to have the same duration for each step of the pattern. Devices with LUT peripherals support low/high pause which is when the first/last entry of the pattern can have a longer duration. This loop checks that the all of the pattern durations are the same with the exception of the first and last entry for low/hi pause. This change was made because devices that use single SDAM do not support low/high pause, so we must check every single pattern duration. Instead of changing the loop arguments with an if statement I was thinking we could either: a. keep the original loop arguments and when loop exits we can check first element for single SDAM devices delta_t = pattern[1].delta_t; for (i = 2; i < len; i++) { if (pattern[i].delta_t != delta_t) { + if (i != actual_len - 1 || lpg->lpg_chan_nvmem) goto out_free_pattern; } } + if (lpg->lpg_chan_nvmem) { + if (delta_t != pattern[0].delta_t) + goto out_free_pattern + } b. Change the loop argument to start with i=0 and for LUT device we could just skip checking first and last element duration ** We would end up checking if pattern[1].delta_t == pattern[1].delta_t inside the loop when i == 1 delta_t = pattern[1].delta_t; + for (i = 0; i < len; i++) { if (pattern[i].delta_t != delta_t) { + if (lpg->lut_base && (i == 0 || i == actual_len - 1) + continue; + else + goto out_free_pattern; } Let me know if you would rather go with one of these options to make this change cleaner. Thanks, Anjelique > >> + >> + for (; i < len; i++) { >> if (pattern[i].delta_t != delta_t) { >> /* >> * Allow last entry in the full or shortened pattern to >> * specify hi pause. Reject other variations. >> */ >> - if (i != actual_len - 1) >> + if (i != actual_len - 1 || lpg->lpg_chan_nvmem) >> goto out_free_pattern; >> } >> }
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? [...] >>> @@ -860,14 +1043,21 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern, >>> * Validate that all delta_t in the pattern are the same, with the >>> * exception of the middle element in case of ping_pong. >>> */ >>> - delta_t = pattern[1].delta_t; >>> - for (i = 2; i < len; i++) { >>> + if (lpg->lpg_chan_nvmem) { >>> + i = 1; >>> + delta_t = pattern[0].delta_t; >>> + } else { >>> + i = 2; >>> + delta_t = pattern[1].delta_t; >>> + } >> Why? >> >> What's the rationale behind this change? > Patterns are required to have the same duration for each step of the pattern. Devices with LUT peripherals support low/high > pause which is when the first/last entry of the pattern can have a longer duration. This loop checks that the all of the > pattern durations are the same with the exception of the first and last entry for low/hi pause. That's the explanation I was looking for! :) Things like these that are only known to inside folks should definitely be stated either as a comment, or inside the commit message. Since you're changing the code flow in a noticeable manner, this could probably be a good fit for a comment. > > This change was made because devices that use single SDAM do not support low/high pause, so we must check every > single pattern duration. Instead of changing the loop arguments with an if statement I was thinking we could either: > > a. keep the original loop arguments and when loop exits we can check first element for single SDAM devices > > delta_t = pattern[1].delta_t; > for (i = 2; i < len; i++) { > if (pattern[i].delta_t != delta_t) { > + if (i != actual_len - 1 || lpg->lpg_chan_nvmem) > goto out_free_pattern; > } > } > > + if (lpg->lpg_chan_nvmem) { > + if (delta_t != pattern[0].delta_t) > + goto out_free_pattern > + } We assign hi/lo_pause a couple lines below. Moving these assignments a bit higher up could let us make this clearer: /* LPGs using SDAM for patterns require equal duration of all steps */ if ((delta_t != lo_pause) && lpg->lpg_chan_nvmem) goto out_free_pattern; 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. 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? > > b. Change the loop argument to start with i=0 and for LUT device we could just skip checking first and last element duration > ** We would end up checking if pattern[1].delta_t == pattern[1].delta_t inside the loop when i == 1 > > delta_t = pattern[1].delta_t; > + for (i = 0; i < len; i++) { > if (pattern[i].delta_t != delta_t) { > + if (lpg->lut_base && (i == 0 || i == actual_len - 1) > + continue; > + else > + goto out_free_pattern; Meh, too many magic literals for my liking Konrad
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
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c index 59581b3e25ca..90dc27d5eb7c 100644 --- a/drivers/leds/rgb/leds-qcom-lpg.c +++ b/drivers/leds/rgb/leds-qcom-lpg.c @@ -8,12 +8,14 @@ #include <linux/bitfield.h> #include <linux/led-class-multicolor.h> #include <linux/module.h> +#include <linux/nvmem-consumer.h> #include <linux/of.h> #include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/pwm.h> #include <linux/regmap.h> #include <linux/slab.h> +#include <linux/soc/qcom/qcom-pbs.h> #define LPG_SUBTYPE_REG 0x05 #define LPG_SUBTYPE_LPG 0x2 @@ -40,6 +42,8 @@ #define PWM_SEC_ACCESS_REG 0xd0 #define PWM_DTEST_REG(x) (0xe2 + (x) - 1) +#define SDAM_REG_PBS_SEQ_EN 0x42 + #define TRI_LED_SRC_SEL 0x45 #define TRI_LED_EN_CTL 0x46 #define TRI_LED_ATC_CTL 0x47 @@ -49,9 +53,25 @@ #define LPG_RESOLUTION_9BIT BIT(9) #define LPG_RESOLUTION_15BIT BIT(15) +#define PPG_MAX_LED_BRIGHTNESS 255 + #define LPG_MAX_M 7 #define LPG_MAX_PREDIV 6 +#define DEFAULT_TICK_DURATION_US 7800 +#define RAMP_STEP_DURATION(x) (((x) * 1000 / DEFAULT_TICK_DURATION_US) & 0xff) + +/* LPG common config settings for PPG */ +#define SDAM_REG_RAMP_STEP_DURATION 0x47 +#define SDAM_LUT_COUNT_MAX 64 + +/* LPG per channel config settings for PPG */ +#define SDAM_LUT_EN_OFFSET 0x0 +#define SDAM_PATTERN_CONFIG_OFFSET 0x1 +#define SDAM_END_INDEX_OFFSET 0x3 +#define SDAM_START_INDEX_OFFSET 0x4 +#define SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET 0x6 + struct lpg_channel; struct lpg_data; @@ -65,7 +85,11 @@ struct lpg_data; * @lut_base: base address of the LUT block (optional) * @lut_size: number of entries in the LUT block * @lut_bitmap: allocation bitmap for LUT entries - * @triled_base: base address of the TRILED block (optional) + * @pbs_dev: PBS device + * @lpg_chan_nvmem: LPG nvmem peripheral device + * @pbs_en_bitmap: bitmap for tracking PBS triggers + * @lut_sdam_base: offset where LUT pattern begins in nvmem + * @triled_base: base address of the TRILED block (optional) * @triled_src: power-source for the TRILED * @triled_has_atc_ctl: true if there is TRI_LED_ATC_CTL register * @triled_has_src_sel: true if there is TRI_LED_SRC_SEL register @@ -86,6 +110,11 @@ struct lpg { u32 lut_size; unsigned long *lut_bitmap; + struct pbs_dev *pbs_dev; + struct nvmem_device *lpg_chan_nvmem; + unsigned long pbs_en_bitmap; + u32 lut_sdam_base; + u32 triled_base; u32 triled_src; bool triled_has_atc_ctl; @@ -102,6 +131,8 @@ struct lpg { * @triled_mask: mask in TRILED to enable this channel * @lut_mask: mask in LUT to start pattern generator for this channel * @subtype: PMIC hardware block subtype + * @sdam_offset: Channel offset in LPG nvmem + * @lpg_idx: index of the channel * @in_use: channel is exposed to LED framework * @color: color of the LED attached to this channel * @dtest_line: DTEST line for output, or 0 if disabled @@ -113,6 +144,7 @@ struct lpg { * @pre_div_sel: divider selector of the reference clock * @pre_div_exp: exponential divider of the reference clock * @pwm_resolution_sel: pwm resolution selector + * @pattern_set: true when setting pattern * @ramp_enabled: duty cycle is driven by iterating over lookup table * @ramp_ping_pong: reverse through pattern, rather than wrapping to start * @ramp_oneshot: perform only a single pass over the pattern @@ -130,6 +162,8 @@ struct lpg_channel { unsigned int triled_mask; unsigned int lut_mask; unsigned int subtype; + u32 sdam_offset; + u32 lpg_idx; bool in_use; @@ -147,6 +181,7 @@ struct lpg_channel { unsigned int pre_div_exp; unsigned int pwm_resolution_sel; + bool pattern_set; bool ramp_enabled; bool ramp_ping_pong; bool ramp_oneshot; @@ -181,10 +216,12 @@ struct lpg_led { * struct lpg_channel_data - per channel initialization data * @base: base address for PWM channel registers * @triled_mask: bitmask for controlling this channel in TRILED + * @sdam_offset: Channel offset in LPG nvmem */ struct lpg_channel_data { unsigned int base; u8 triled_mask; + unsigned int sdam_offset; }; /** @@ -192,6 +229,7 @@ struct lpg_channel_data { * @lut_base: base address of LUT block * @lut_size: number of entries in LUT * @triled_base: base address of TRILED + * @lut_sdam_base: base address where LUT pattern begins in nvmem device * @triled_has_atc_ctl: true if there is TRI_LED_ATC_CTL register * @triled_has_src_sel: true if there is TRI_LED_SRC_SEL register * @num_channels: number of channels in LPG @@ -201,12 +239,90 @@ struct lpg_data { unsigned int lut_base; unsigned int lut_size; unsigned int triled_base; + unsigned int lut_sdam_base; bool triled_has_atc_ctl; bool triled_has_src_sel; int num_channels; const struct lpg_channel_data *channels; }; +static int lpg_sdam_write(struct lpg *lpg, u16 addr, u8 val) +{ + int rc; + + rc = nvmem_device_write(lpg->lpg_chan_nvmem, addr, 1, &val); + if (rc < 0) { + dev_err(lpg->dev, "writing %u to SDAM addr %#x failed, rc=%d\n", + val, addr, rc); + return rc; + } + + return 0; +} + +#define PBS_SW_TRIG_BIT BIT(0) + +static int lpg_clear_pbs_trigger(struct lpg_channel *chan) +{ + int rc; + + clear_bit(chan->lpg_idx, &chan->lpg->pbs_en_bitmap); + if (!chan->lpg->pbs_en_bitmap) { + rc = lpg_sdam_write(chan->lpg, SDAM_REG_PBS_SEQ_EN, 0); + if (rc < 0) + return rc; + } + + return 0; +} + +static int lpg_set_pbs_trigger(struct lpg_channel *chan) +{ + int rc; + + if (!chan->lpg->pbs_en_bitmap) { + rc = lpg_sdam_write(chan->lpg, SDAM_REG_PBS_SEQ_EN, PBS_SW_TRIG_BIT); + if (rc < 0) + return rc; + + rc = qcom_pbs_trigger_event(chan->lpg->pbs_dev, PBS_SW_TRIG_BIT); + if (rc < 0) + return rc; + } + set_bit(chan->lpg_idx, &chan->lpg->pbs_en_bitmap); + + return 0; +} + +static int lpg_sdam_configure_triggers(struct lpg_channel *chan, bool set_trig) +{ + int rc; + + if (chan->lpg->lut_base) + return 0; + + if (set_trig) { + rc = lpg_sdam_write(chan->lpg, SDAM_LUT_EN_OFFSET + chan->sdam_offset, 1); + if (rc < 0) + return rc; + + rc = lpg_set_pbs_trigger(chan); + if (rc < 0) + return rc; + chan->pattern_set = false; + } else { + rc = lpg_sdam_write(chan->lpg, SDAM_LUT_EN_OFFSET + chan->sdam_offset, 0); + if (rc < 0) + return rc; + + rc = lpg_clear_pbs_trigger(chan); + if (rc < 0) + return rc; + } + + return 0; +} + static int triled_set(struct lpg *lpg, unsigned int mask, unsigned int enable) { /* Skip if we don't have a triled block */ @@ -217,6 +333,41 @@ static int triled_set(struct lpg *lpg, unsigned int mask, unsigned int enable) mask, enable); } +static int lpg_lut_store_sdam(struct lpg *lpg, struct led_pattern *pattern, + size_t len, unsigned int *lo_idx, unsigned int *hi_idx) +{ + u8 brightness; + u16 addr; + unsigned int idx; + int i, rc; + + if (len > SDAM_LUT_COUNT_MAX) { + dev_err(lpg->dev, "Pattern length (%zu) exceeds maximum pattern length (%d)\n", + len, SDAM_LUT_COUNT_MAX); + return -EINVAL; + } + + idx = bitmap_find_next_zero_area(lpg->lut_bitmap, lpg->lut_size, + 0, len, 0); + if (idx >= lpg->lut_size) + return -ENOSPC; + + for (i = 0; i < len; i++) { + brightness = pattern[i].brightness; + addr = lpg->lut_sdam_base + i + idx; + rc = lpg_sdam_write(lpg, addr, brightness); + if (rc < 0) + return rc; + } + + bitmap_set(lpg->lut_bitmap, idx, len); + + *lo_idx = idx; + *hi_idx = idx + len - 1; + + return 0; +} + static int lpg_lut_store(struct lpg *lpg, struct led_pattern *pattern, size_t len, unsigned int *lo_idx, unsigned int *hi_idx) { @@ -463,6 +614,26 @@ static void lpg_apply_pwm_value(struct lpg_channel *chan) #define LPG_PATTERN_CONFIG_PAUSE_HI BIT(1) #define LPG_PATTERN_CONFIG_PAUSE_LO BIT(0) +static void lpg_sdam_apply_lut_control(struct lpg_channel *chan) +{ + u8 val, conf = 0; + struct lpg *lpg = chan->lpg; + + if (!chan->ramp_oneshot) + conf |= LPG_PATTERN_CONFIG_REPEAT; + + lpg_sdam_write(lpg, SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET + chan->sdam_offset, 0); + lpg_sdam_write(lpg, SDAM_PATTERN_CONFIG_OFFSET + chan->sdam_offset, conf); + + lpg_sdam_write(lpg, SDAM_END_INDEX_OFFSET + chan->sdam_offset, chan->pattern_hi_idx); + lpg_sdam_write(lpg, SDAM_START_INDEX_OFFSET + chan->sdam_offset, chan->pattern_lo_idx); + + val = RAMP_STEP_DURATION(chan->ramp_tick_ms); + if (val > 0) + val--; + lpg_sdam_write(lpg, SDAM_REG_RAMP_STEP_DURATION, val); +} + static void lpg_apply_lut_control(struct lpg_channel *chan) { struct lpg *lpg = chan->lpg; @@ -476,6 +647,9 @@ static void lpg_apply_lut_control(struct lpg_channel *chan) if (!chan->ramp_enabled || chan->pattern_lo_idx == chan->pattern_hi_idx) return; + if (lpg->lpg_chan_nvmem) + return lpg_sdam_apply_lut_control(chan); + hi_pause = DIV_ROUND_UP(chan->ramp_hi_pause_ms, step); lo_pause = DIV_ROUND_UP(chan->ramp_lo_pause_ms, step); @@ -643,6 +817,9 @@ static void lpg_brightness_set(struct lpg_led *led, struct led_classdev *cdev, triled_mask |= chan->triled_mask; lpg_apply(chan); + + if (chan->pattern_set) + lpg_sdam_configure_triggers(chan, true); } /* Toggle triled lines */ @@ -775,7 +952,7 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern, unsigned int lo_idx; unsigned int hi_idx; unsigned int i; - bool ping_pong = true; + bool ping_pong = false; int ret = -EINVAL; /* Hardware only support oneshot or indefinite loops */ @@ -824,7 +1001,7 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern, * used to stretch the first delay of the pattern and a "high pause" * the last one. * - * In order to save space the pattern can be played in "ping pong" + * In order to save space for the pattern can be played in "ping pong" * mode, in which the pattern is first played forward, then "high * pause" is applied, then the pattern is played backwards and finally * the "low pause" is applied. @@ -837,16 +1014,22 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern, * If the specified pattern is a palindrome the ping pong mode is * enabled. In this scenario the delta_t of the middle entry (i.e. the * last in the programmed pattern) determines the "high pause". + * + * NVMEM devices supporting LUT do not support "low pause", "high pause" + * or "ping pong" */ /* Detect palindromes and use "ping pong" to reduce LUT usage */ - for (i = 0; i < len / 2; i++) { - brightness_a = pattern[i].brightness; - brightness_b = pattern[len - i - 1].brightness; - - if (brightness_a != brightness_b) { - ping_pong = false; - break; + if (lpg->lut_base) { + ping_pong = true; + for (i = 0; i < len / 2; i++) { + brightness_a = pattern[i].brightness; + brightness_b = pattern[len - i - 1].brightness; + + if (brightness_a != brightness_b) { + ping_pong = false; + break; + } } } @@ -860,14 +1043,21 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern, * Validate that all delta_t in the pattern are the same, with the * exception of the middle element in case of ping_pong. */ - delta_t = pattern[1].delta_t; - for (i = 2; i < len; i++) { + if (lpg->lpg_chan_nvmem) { + i = 1; + delta_t = pattern[0].delta_t; + } else { + i = 2; + delta_t = pattern[1].delta_t; + } + + for (; i < len; i++) { if (pattern[i].delta_t != delta_t) { /* * Allow last entry in the full or shortened pattern to * specify hi pause. Reject other variations. */ - if (i != actual_len - 1) + if (i != actual_len - 1 || lpg->lpg_chan_nvmem) goto out_free_pattern; } } @@ -876,12 +1066,19 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern, if (delta_t >= BIT(9)) goto out_free_pattern; - /* Find "low pause" and "high pause" in the pattern */ - lo_pause = pattern[0].delta_t; - hi_pause = pattern[actual_len - 1].delta_t; + /* Find "low pause" and "high pause" in the pattern if not an NVMEM device*/ + if (lpg->lut_base) { + lo_pause = pattern[0].delta_t; + hi_pause = pattern[actual_len - 1].delta_t; + } mutex_lock(&lpg->lock); - ret = lpg_lut_store(lpg, pattern, actual_len, &lo_idx, &hi_idx); + + if (lpg->lpg_chan_nvmem) + ret = lpg_lut_store_sdam(lpg, pattern, actual_len, &lo_idx, &hi_idx); + else + ret = lpg_lut_store(lpg, pattern, actual_len, &lo_idx, &hi_idx); + if (ret < 0) goto out_unlock; @@ -897,6 +1094,8 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern, chan->pattern_lo_idx = lo_idx; chan->pattern_hi_idx = hi_idx; + + chan->pattern_set = true; } out_unlock: @@ -954,6 +1153,7 @@ static int lpg_pattern_clear(struct lpg_led *led) for (i = 0; i < led->num_channels; i++) { chan = led->channels[i]; + lpg_sdam_configure_triggers(chan, false); chan->pattern_lo_idx = 0; chan->pattern_hi_idx = 0; } @@ -1190,8 +1390,8 @@ static int lpg_add_led(struct lpg *lpg, struct device_node *np) cdev->brightness_set_blocking = lpg_brightness_mc_set; cdev->blink_set = lpg_blink_mc_set; - /* Register pattern accessors only if we have a LUT block */ - if (lpg->lut_base) { + /* Register pattern accessors if we have a LUT block or when using PPG */ + if (lpg->lut_base || lpg->lpg_chan_nvmem) { cdev->pattern_set = lpg_pattern_mc_set; cdev->pattern_clear = lpg_pattern_mc_clear; } @@ -1204,15 +1404,19 @@ static int lpg_add_led(struct lpg *lpg, struct device_node *np) cdev->brightness_set_blocking = lpg_brightness_single_set; cdev->blink_set = lpg_blink_single_set; - /* Register pattern accessors only if we have a LUT block */ - if (lpg->lut_base) { + /* Register pattern accessors if we have a LUT block or when using PPG */ + if (lpg->lut_base || lpg->lpg_chan_nvmem) { cdev->pattern_set = lpg_pattern_single_set; cdev->pattern_clear = lpg_pattern_single_clear; } } cdev->default_trigger = of_get_property(np, "linux,default-trigger", NULL); - cdev->max_brightness = LPG_RESOLUTION_9BIT - 1; + + if (lpg->lpg_chan_nvmem) + cdev->max_brightness = PPG_MAX_LED_BRIGHTNESS; + else + cdev->max_brightness = LPG_RESOLUTION_9BIT - 1; if (!of_property_read_string(np, "default-state", &state) && !strcmp(state, "on")) @@ -1253,6 +1457,8 @@ static int lpg_init_channels(struct lpg *lpg) chan->base = data->channels[i].base; chan->triled_mask = data->channels[i].triled_mask; chan->lut_mask = BIT(i); + chan->sdam_offset = data->channels[i].sdam_offset; + chan->lpg_idx = i; regmap_read(lpg->map, chan->base + LPG_SUBTYPE_REG, &chan->subtype); } @@ -1299,10 +1505,13 @@ static int lpg_init_lut(struct lpg *lpg) { const struct lpg_data *data = lpg->data; - if (!data->lut_base) + if (data->lut_base) + lpg->lut_base = data->lut_base; + else if (lpg->lpg_chan_nvmem) + lpg->lut_sdam_base = data->lut_sdam_base; + else return 0; - lpg->lut_base = data->lut_base; lpg->lut_size = data->lut_size; lpg->lut_bitmap = devm_bitmap_zalloc(lpg->dev, lpg->lut_size, GFP_KERNEL); @@ -1312,6 +1521,43 @@ static int lpg_init_lut(struct lpg *lpg) return 0; } +static int lpg_init_sdam(struct lpg *lpg) +{ + struct lpg_channel *chan; + int i, nvmem_count, rc; + + nvmem_count = of_property_count_strings(lpg->dev->of_node, "nvmem-names"); + if (nvmem_count <= 0) + return 0; + + /* get the nvmem device for LPG/LUT config */ + lpg->lpg_chan_nvmem = devm_nvmem_device_get(lpg->dev, "lpg_chan_sdam"); + if (IS_ERR(lpg->lpg_chan_nvmem)) + return dev_err_probe(lpg->dev, PTR_ERR(lpg->lpg_chan_nvmem), + "Failed to get nvmem device\n"); + + lpg->pbs_dev = get_pbs_client_device(lpg->dev); + if (IS_ERR(lpg->pbs_dev)) + return dev_err_probe(lpg->dev, PTR_ERR(lpg->pbs_dev), + "Failed to get PBS client device\n"); + + for (i = 0; i < lpg->num_channels; i++) { + chan = &lpg->channels[i]; + if (chan->sdam_offset) { + rc = lpg_sdam_write(lpg, + SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET + chan->sdam_offset, 0); + if (rc < 0) + return rc; + + rc = lpg_sdam_configure_triggers(chan, false); + if (rc < 0) + return rc; + } + } + + return 0; +} + static int lpg_probe(struct platform_device *pdev) { struct device_node *np; @@ -1348,6 +1594,10 @@ static int lpg_probe(struct platform_device *pdev) if (ret < 0) return ret; + ret = lpg_init_sdam(lpg); + if (ret < 0) + return ret; + ret = lpg_init_lut(lpg); if (ret < 0) return ret;
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. Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> --- drivers/leds/rgb/leds-qcom-lpg.c | 298 ++++++++++++++++++++++++++++--- 1 file changed, 274 insertions(+), 24 deletions(-)