diff mbox series

[v3,6/7] leds: rgb: leds-qcom-lpg: Support two-nvmem PPG Scheme

Message ID 20230814235918.10396-7-quic_amelende@quicinc.com (mailing list archive)
State Superseded
Headers show
Series Add support for LUT PPG | expand

Commit Message

Anjelique Melendez Aug. 14, 2023, 11:59 p.m. UTC 
On PMICs such as PM8350C, the lookup table containing the pattern data
is stored in a separate nvmem device from the one where the per-channel
data is stored.

Add support for two separate nvmems to handle this case while maintaining
backward compatibility for those targets that use only a single nvmem
device.

Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
---
 drivers/leds/rgb/leds-qcom-lpg.c | 112 ++++++++++++++++++++++++-------
 1 file changed, 89 insertions(+), 23 deletions(-)

Comments

Bjorn Andersson Aug. 15, 2023, 4:17 p.m. UTC | #1 
On Mon, Aug 14, 2023 at 04:59:17PM -0700, Anjelique Melendez wrote:
> On PMICs such as PM8350C, the lookup table containing the pattern data
> is stored in a separate nvmem device from the one where the per-channel

I think it would be better to say "separate SDAM" instead of "separate
nvmem device", to make it clearer to the reader what's being done.

> data is stored.
> 
> Add support for two separate nvmems to handle this case while maintaining

You're only adding the support for the dedicated LUT SDAM, not "for two
separate nvmems".

> backward compatibility for those targets that use only a single nvmem
> device.
> 
> Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>

If Guru was the first to sign off the change, then he must have authored
the patch. Or add a Co-developed-by if that's appropriate.

> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> ---
>  drivers/leds/rgb/leds-qcom-lpg.c | 112 ++++++++++++++++++++++++-------
>  1 file changed, 89 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> index 822c7bff00df..f3f83925ab41 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> @@ -60,6 +60,7 @@
>  #define RAMP_STEP_DURATION(x)		(((x) * 1000 / DEFAULT_TICK_DURATION_US) & 0xff)
>  
>  /* LPG common config settings for PPG */
> +#define SDAM_START_BASE			0x40

I really wish that we hid this offset in the SDAM nvmem driver - but
that ship has sailed...

>  #define SDAM_REG_RAMP_STEP_DURATION		0x47
>  #define SDAM_LUT_COUNT_MAX			64
>  
> @@ -69,6 +70,8 @@
>  #define SDAM_END_INDEX_OFFSET			0x3
>  #define SDAM_START_INDEX_OFFSET		0x4
>  #define SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET	0x6
> +#define SDAM_PAUSE_HI_MULTIPLIER_OFFSET	0x8
> +#define SDAM_PAUSE_LO_MULTIPLIER_OFFSET	0x9
>  
>  struct lpg_channel;
>  struct lpg_data;
> @@ -85,7 +88,9 @@ struct lpg_data;
>   * @lut_bitmap:	allocation bitmap for LUT entries
>   * @pbs_dev:	PBS device
>   * @lpg_chan_nvmem:	LPG nvmem peripheral device
> + * @lut_nvmem:	LUT nvmem peripheral device
>   * @pbs_en_bitmap:	bitmap for tracking PBS triggers
> + * @nvmem_count:	number of nvmems used for LUT and PPG config

I find the concept of "how many nvmem do we have" slightly unclear.
Perhaps we could split this that into two separate boolean properties,
something like use_sdam and use_sdam_lut?


On the other hand, use_sdam and use_sdam_lut should afaict always be
equal to lpg_chan_nvmem != NULL and lut_nvmem != NULL. So you could use
these directly instead.

>   * @lut_sdam_base:	offset where LUT pattern begins in nvmem
>   * @ppg_en:	Flag indicating whether PPG is enabled/used
>   * @triled_base:	base address of the TRILED block (optional)
> @@ -111,7 +116,9 @@ struct lpg {
>  
>  	struct pbs_dev *pbs_dev;
>  	struct nvmem_device *lpg_chan_nvmem;
> +	struct nvmem_device *lut_nvmem;
>  	unsigned long pbs_en_bitmap;
> +	unsigned int nvmem_count;
>  	u32 lut_sdam_base;
>  	bool ppg_en;
>  
> @@ -261,6 +268,8 @@ static int lpg_sdam_write(struct lpg *lpg, u16 addr, u8 val)
>  }
>  
>  #define SDAM_REG_PBS_SEQ_EN		0x42
> +#define SDAM_PBS_TRIG_SET		0xe5
> +#define SDAM_PBS_TRIG_CLR		0xe6

Please add these at the top, like requested in the previous patch.

>  #define PBS_SW_TRIG_BIT		BIT(0)
>  
[..]
>  static void lpg_sdam_apply_lut_control(struct lpg_channel *chan)
>  {
>  	u8 val, conf = 0;
> +	unsigned int hi_pause, lo_pause;
>  	struct lpg *lpg = chan->lpg;
>  
> +	hi_pause = DIV_ROUND_UP(chan->ramp_hi_pause_ms, chan->ramp_tick_ms);
> +	lo_pause = DIV_ROUND_UP(chan->ramp_lo_pause_ms, chan->ramp_tick_ms);
> +
>  	if (!chan->ramp_oneshot)
>  		conf |= LPG_PATTERN_CONFIG_REPEAT;
> +	if (chan->ramp_hi_pause_ms && lpg->nvmem_count != 1)
> +		conf |= LPG_PATTERN_CONFIG_PAUSE_HI;
> +	if (chan->ramp_lo_pause_ms && lpg->nvmem_count != 1)
> +		conf |= LPG_PATTERN_CONFIG_PAUSE_LO;
>  
>  	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 = lpg_get_sdam_lut_idx(chan, chan->pattern_hi_idx);

I'd suggest adding a local variable "lut_offset" to the values here,
instead of calling out to a separate function to perhaps adjust the
values.

> +	lpg_sdam_write(lpg, SDAM_END_INDEX_OFFSET + chan->sdam_offset, val);
> +
> +	val = lpg_get_sdam_lut_idx(chan, chan->pattern_lo_idx);
> +	lpg_sdam_write(lpg, SDAM_START_INDEX_OFFSET + chan->sdam_offset, val);
>  
>  	val = RAMP_STEP_DURATION(chan->ramp_tick_ms);
>  	if (val > 0)
>  		val--;
>  	lpg_sdam_write(lpg, SDAM_REG_RAMP_STEP_DURATION, val);
> +
> +	if (lpg->nvmem_count != 1) {
> +		lpg_sdam_write(lpg, SDAM_PAUSE_HI_MULTIPLIER_OFFSET + chan->sdam_offset, hi_pause);
> +		lpg_sdam_write(lpg, SDAM_PAUSE_LO_MULTIPLIER_OFFSET + chan->sdam_offset, lo_pause);
> +	}
> +
>  }
>  
>  static void lpg_apply_lut_control(struct lpg_channel *chan)
> @@ -1000,8 +1050,8 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
>  	 * 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"
> +	 * All NVMEM devices supporting LUT do not support "ping pong"
> +	 * Single NVMEM devices supporting LUT do not support "low pause" and "high pause"

How about: "SDAM-based devices does not support "ping pong", and only
supports "low pause" and "high pause" with dedicated SDAM LUT." ?

>  	 */
>  
>  	/* Detect palindromes and use "ping pong" to reduce LUT usage */
[..]
> @@ -1509,29 +1559,45 @@ static int lpg_parse_sdam(struct lpg *lpg)
>  {
>  	int rc = 0;
>  
> -	if (lpg->data->nvmem_count == 0)
> +	lpg->nvmem_count = lpg->data->nvmem_count;
> +	if (lpg->nvmem_count == 0)
>  		return 0;
>  
> -	/* get the nvmem device for LPG/LUT config */
> +	if (lpg->nvmem_count > 2)
> +		return -EINVAL;
> +
> +	/* get the 1st 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)) {
>  		rc = PTR_ERR(lpg->lpg_chan_nvmem);
> -		if (rc != -EPROBE_DEFER)
> -			dev_err(lpg->dev, "Failed to get nvmem device, rc=%d\n", rc);
> -		return rc;
> +		goto err;
>  	}
>  
> -	lpg->pbs_dev = get_pbs_client_device(lpg->dev);
> -	if (IS_ERR(lpg->pbs_dev)) {
> -		rc = PTR_ERR(lpg->pbs_dev);
> -		if (rc != -EPROBE_DEFER)
> -			dev_err(lpg->dev, "Failed to get PBS client device, rc=%d\n", rc);
> -		return rc;
> +	if (lpg->nvmem_count == 1) {
> +		/* get PBS device node if single NVMEM device */
> +		lpg->pbs_dev = get_pbs_client_device(lpg->dev);
> +		if (IS_ERR(lpg->pbs_dev)) {
> +			rc = PTR_ERR(lpg->pbs_dev);
> +			if (rc != -EPROBE_DEFER)
> +				dev_err(lpg->dev, "Failed to get PBS client device, rc=%d\n", rc);
> +			return rc;

return dev_err_probe() please.

> +		}
> +	} else if (lpg->nvmem_count == 2) {
> +		/* get the 2nd nvmem device for LUT pattern */
> +		lpg->lut_nvmem = devm_nvmem_device_get(lpg->dev, "lut_sdam");
> +		if (IS_ERR(lpg->lut_nvmem)) {
> +			rc = PTR_ERR(lpg->lut_nvmem);
> +			goto err;
> +		}
>  	}
>  
>  	lpg->ppg_en = true;
>  
>  	return rc;

rc is still 0 here.

> +err:
> +	if (rc != -EPROBE_DEFER)
> +		dev_err(lpg->dev, "Failed to get nvmem device, rc=%d\n", rc);

The idiomatic usage of goto is to share error handling, and release
resources in a reverse order that they where acquired. It's therefor
going to confuse future readers when you do:

if (error)
  goto err;

if (error2)
  return rc;

if (error3)
  goto err;


But more importantly, this error message gives no indication about which
nvmem device the driver failed to find. If you move the error print up,
and use dev_err_probe() the code will be easier to follow and you can
afford making the error message more helpful.

Regards,
Bjorn
diff mbox series

Patch

diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index 822c7bff00df..f3f83925ab41 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -60,6 +60,7 @@ 
 #define RAMP_STEP_DURATION(x)		(((x) * 1000 / DEFAULT_TICK_DURATION_US) & 0xff)
 
 /* LPG common config settings for PPG */
+#define SDAM_START_BASE			0x40
 #define SDAM_REG_RAMP_STEP_DURATION		0x47
 #define SDAM_LUT_COUNT_MAX			64
 
@@ -69,6 +70,8 @@ 
 #define SDAM_END_INDEX_OFFSET			0x3
 #define SDAM_START_INDEX_OFFSET		0x4
 #define SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET	0x6
+#define SDAM_PAUSE_HI_MULTIPLIER_OFFSET	0x8
+#define SDAM_PAUSE_LO_MULTIPLIER_OFFSET	0x9
 
 struct lpg_channel;
 struct lpg_data;
@@ -85,7 +88,9 @@  struct lpg_data;
  * @lut_bitmap:	allocation bitmap for LUT entries
  * @pbs_dev:	PBS device
  * @lpg_chan_nvmem:	LPG nvmem peripheral device
+ * @lut_nvmem:	LUT nvmem peripheral device
  * @pbs_en_bitmap:	bitmap for tracking PBS triggers
+ * @nvmem_count:	number of nvmems used for LUT and PPG config
  * @lut_sdam_base:	offset where LUT pattern begins in nvmem
  * @ppg_en:	Flag indicating whether PPG is enabled/used
  * @triled_base:	base address of the TRILED block (optional)
@@ -111,7 +116,9 @@  struct lpg {
 
 	struct pbs_dev *pbs_dev;
 	struct nvmem_device *lpg_chan_nvmem;
+	struct nvmem_device *lut_nvmem;
 	unsigned long pbs_en_bitmap;
+	unsigned int nvmem_count;
 	u32 lut_sdam_base;
 	bool ppg_en;
 
@@ -261,6 +268,8 @@  static int lpg_sdam_write(struct lpg *lpg, u16 addr, u8 val)
 }
 
 #define SDAM_REG_PBS_SEQ_EN		0x42
+#define SDAM_PBS_TRIG_SET		0xe5
+#define SDAM_PBS_TRIG_CLR		0xe6
 #define PBS_SW_TRIG_BIT		BIT(0)
 
 static int lpg_clear_pbs_trigger(struct lpg_channel *chan)
@@ -272,6 +281,12 @@  static int lpg_clear_pbs_trigger(struct lpg_channel *chan)
 		rc = lpg_sdam_write(chan->lpg, SDAM_REG_PBS_SEQ_EN, 0);
 		if (rc < 0)
 			return rc;
+
+		if (chan->lpg->nvmem_count == 2) {
+			rc = lpg_sdam_write(chan->lpg, SDAM_PBS_TRIG_CLR, PBS_SW_TRIG_BIT);
+			if (rc < 0)
+				return rc;
+		}
 	}
 
 	return 0;
@@ -286,9 +301,15 @@  static int lpg_set_pbs_trigger(struct lpg_channel *chan)
 		if (rc < 0)
 			return rc;
 
-		rc = qcom_pbs_trigger_event(chan->lpg->pbs_dev, PBS_SW_TRIG_BIT);
-		if (rc < 0)
-			return rc;
+		if (chan->lpg->nvmem_count == 1) {
+			rc = qcom_pbs_trigger_event(chan->lpg->pbs_dev, PBS_SW_TRIG_BIT);
+			if (rc < 0)
+				return rc;
+		} else {
+			rc = lpg_sdam_write(chan->lpg, SDAM_PBS_TRIG_SET, PBS_SW_TRIG_BIT);
+			if (rc < 0)
+				return rc;
+		}
 	}
 	set_bit(chan->lpg_idx, &chan->lpg->pbs_en_bitmap);
 
@@ -342,7 +363,12 @@  static int lpg_lut_store_sdam(struct lpg *lpg, struct led_pattern *pattern,
 	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 (lpg->nvmem_count == 1)
+			rc = lpg_sdam_write(lpg, addr, brightness);
+		else
+			rc = nvmem_device_write(lpg->lut_nvmem, addr, 1, &brightness);
+
 		if (rc < 0)
 			return rc;
 	}
@@ -601,24 +627,48 @@  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 u8 lpg_get_sdam_lut_idx(struct lpg_channel *chan, u8 idx)
+{
+	if (chan->lpg->nvmem_count == 2)
+		return chan->lpg->lut_sdam_base - SDAM_START_BASE + idx;
+	return idx;
+}
+
 static void lpg_sdam_apply_lut_control(struct lpg_channel *chan)
 {
 	u8 val, conf = 0;
+	unsigned int hi_pause, lo_pause;
 	struct lpg *lpg = chan->lpg;
 
+	hi_pause = DIV_ROUND_UP(chan->ramp_hi_pause_ms, chan->ramp_tick_ms);
+	lo_pause = DIV_ROUND_UP(chan->ramp_lo_pause_ms, chan->ramp_tick_ms);
+
 	if (!chan->ramp_oneshot)
 		conf |= LPG_PATTERN_CONFIG_REPEAT;
+	if (chan->ramp_hi_pause_ms && lpg->nvmem_count != 1)
+		conf |= LPG_PATTERN_CONFIG_PAUSE_HI;
+	if (chan->ramp_lo_pause_ms && lpg->nvmem_count != 1)
+		conf |= LPG_PATTERN_CONFIG_PAUSE_LO;
 
 	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 = lpg_get_sdam_lut_idx(chan, chan->pattern_hi_idx);
+	lpg_sdam_write(lpg, SDAM_END_INDEX_OFFSET + chan->sdam_offset, val);
+
+	val = lpg_get_sdam_lut_idx(chan, chan->pattern_lo_idx);
+	lpg_sdam_write(lpg, SDAM_START_INDEX_OFFSET + chan->sdam_offset, val);
 
 	val = RAMP_STEP_DURATION(chan->ramp_tick_ms);
 	if (val > 0)
 		val--;
 	lpg_sdam_write(lpg, SDAM_REG_RAMP_STEP_DURATION, val);
+
+	if (lpg->nvmem_count != 1) {
+		lpg_sdam_write(lpg, SDAM_PAUSE_HI_MULTIPLIER_OFFSET + chan->sdam_offset, hi_pause);
+		lpg_sdam_write(lpg, SDAM_PAUSE_LO_MULTIPLIER_OFFSET + chan->sdam_offset, lo_pause);
+	}
+
 }
 
 static void lpg_apply_lut_control(struct lpg_channel *chan)
@@ -1000,8 +1050,8 @@  static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
 	 * 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"
+	 * All NVMEM devices supporting LUT do not support "ping pong"
+	 * Single NVMEM devices supporting LUT do not support "low pause" and "high pause"
 	 */
 
 	/* Detect palindromes and use "ping pong" to reduce LUT usage */
@@ -1028,7 +1078,7 @@  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.
 	 */
-	if (lpg->ppg_en) {
+	if (lpg->nvmem_count == 1) {
 		i = 1;
 		delta_t = pattern[0].delta_t;
 	} else {
@@ -1042,7 +1092,7 @@  static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
 			 * Allow last entry in the full or shortened pattern to
 			 * specify hi pause. Reject other variations.
 			 */
-			if (i != actual_len - 1 || lpg->ppg_en)
+			if (i != actual_len - 1 || lpg->nvmem_count == 1)
 				goto out_free_pattern;
 		}
 	}
@@ -1051,8 +1101,8 @@  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 if not an NVMEM device*/
-	if (!lpg->ppg_en) {
+	/* Find "low pause" and "high pause" in the pattern if not a single NVMEM device*/
+	if (lpg->nvmem_count != 1) {
 		lo_pause = pattern[0].delta_t;
 		hi_pause = pattern[actual_len - 1].delta_t;
 	}
@@ -1509,29 +1559,45 @@  static int lpg_parse_sdam(struct lpg *lpg)
 {
 	int rc = 0;
 
-	if (lpg->data->nvmem_count == 0)
+	lpg->nvmem_count = lpg->data->nvmem_count;
+	if (lpg->nvmem_count == 0)
 		return 0;
 
-	/* get the nvmem device for LPG/LUT config */
+	if (lpg->nvmem_count > 2)
+		return -EINVAL;
+
+	/* get the 1st 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)) {
 		rc = PTR_ERR(lpg->lpg_chan_nvmem);
-		if (rc != -EPROBE_DEFER)
-			dev_err(lpg->dev, "Failed to get nvmem device, rc=%d\n", rc);
-		return rc;
+		goto err;
 	}
 
-	lpg->pbs_dev = get_pbs_client_device(lpg->dev);
-	if (IS_ERR(lpg->pbs_dev)) {
-		rc = PTR_ERR(lpg->pbs_dev);
-		if (rc != -EPROBE_DEFER)
-			dev_err(lpg->dev, "Failed to get PBS client device, rc=%d\n", rc);
-		return rc;
+	if (lpg->nvmem_count == 1) {
+		/* get PBS device node if single NVMEM device */
+		lpg->pbs_dev = get_pbs_client_device(lpg->dev);
+		if (IS_ERR(lpg->pbs_dev)) {
+			rc = PTR_ERR(lpg->pbs_dev);
+			if (rc != -EPROBE_DEFER)
+				dev_err(lpg->dev, "Failed to get PBS client device, rc=%d\n", rc);
+			return rc;
+		}
+	} else if (lpg->nvmem_count == 2) {
+		/* get the 2nd nvmem device for LUT pattern */
+		lpg->lut_nvmem = devm_nvmem_device_get(lpg->dev, "lut_sdam");
+		if (IS_ERR(lpg->lut_nvmem)) {
+			rc = PTR_ERR(lpg->lut_nvmem);
+			goto err;
+		}
 	}
 
 	lpg->ppg_en = true;
 
 	return rc;
+err:
+	if (rc != -EPROBE_DEFER)
+		dev_err(lpg->dev, "Failed to get nvmem device, rc=%d\n", rc);
+	return rc;
 }
 
 static int lpg_init_sdam(struct lpg *lpg)