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
