Message-ID: <3b6f6285-ec3b-4e12-aa65-d5f61937de6f@linaro.org>
Date:   Wed, 30 Aug 2023 20:34:10 +0200
From:   Konrad Dybcio <konrad.dybcio@...aro.org>
To:     Anjelique Melendez <quic_amelende@...cinc.com>, pavel@....cz,
        lee@...nel.org, thierry.reding@...il.com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        agross@...nel.org, andersson@...nel.org
Cc:     luca.weiss@...rphone.com, u.kleine-koenig@...gutronix.de,
        quic_subbaram@...cinc.com, quic_gurus@...cinc.com,
        linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-pwm@...r.kernel.org, kernel@...cinc.com
Subject: Re: [PATCH v4 4/7] leds: rgb: leds-qcom-lpg: Add support for PPG
 through single SDAM

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

Powered by blists - more mailing lists