leds: rgb: leds-qcom-lpg: Only enable TRILED when LPG is used by LED

[PATCH] leds: rgb: leds-qcom-lpg: Only enable TRILED when LPG is used by LED

Posted by Fenglin Wu via B4 Relay 2 months, 3 weeks ago

From: Fenglin Wu <fenglin.wu@oss.qualcomm.com>

Enabling the TRILED channel will cause a voltage increase on its power
supply, which is unnecessary if the LPG channel is not being used to
control an LED.

Fixes: 24e2d05d1b68 ("leds: Add driver for Qualcomm LPG")
Signed-off-by: Fenglin Wu <fenglin.wu@oss.qualcomm.com>
---
 drivers/leds/rgb/leds-qcom-lpg.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index 4f2a178e3d265a2cc88e651d3e2ca6ae3dfac2e2..234059b4c0f49d0398030ae5f86967fc1905206d 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -2,7 +2,7 @@
 /*
  * Copyright (c) 2017-2022 Linaro Ltd
  * Copyright (c) 2010-2012, The Linux Foundation. All rights reserved.
- * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
  */
 #include <linux/bits.h>
 #include <linux/bitfield.h>
@@ -1247,7 +1247,9 @@ static int lpg_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	lpg_apply(chan);
 
-	triled_set(lpg, chan->triled_mask, chan->enabled ? chan->triled_mask : 0);
+	/* Only control TRILED if the LPG channel is used by TRILED */
+	if (chan->in_use && chan->triled_mask)
+		triled_set(lpg, chan->triled_mask, chan->enabled ? chan->triled_mask : 0);
 
 out_unlock:
 	mutex_unlock(&lpg->lock);

---
base-commit: ea1c4c7e648d1ca91577071fc42fdc219521098c
change-id: 20251114-lpg_triled_fix-44491b49b340

Best regards,
-- 
Fenglin Wu <fenglin.wu@oss.qualcomm.com>

Re: [PATCH] leds: rgb: leds-qcom-lpg: Only enable TRILED when LPG is used by LED

Posted by Bjorn Andersson 2 months, 3 weeks ago

On Fri, Nov 14, 2025 at 09:11:17AM +0800, Fenglin Wu via B4 Relay wrote:
> From: Fenglin Wu <fenglin.wu@oss.qualcomm.com>
> 
> Enabling the TRILED channel will cause a voltage increase on its power
> supply, which is unnecessary if the LPG channel is not being used to
> control an LED.
> 
> Fixes: 24e2d05d1b68 ("leds: Add driver for Qualcomm LPG")
> Signed-off-by: Fenglin Wu <fenglin.wu@oss.qualcomm.com>
> ---
>  drivers/leds/rgb/leds-qcom-lpg.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> index 4f2a178e3d265a2cc88e651d3e2ca6ae3dfac2e2..234059b4c0f49d0398030ae5f86967fc1905206d 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> @@ -2,7 +2,7 @@
>  /*
>   * Copyright (c) 2017-2022 Linaro Ltd
>   * Copyright (c) 2010-2012, The Linux Foundation. All rights reserved.
> - * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
>   */
>  #include <linux/bits.h>
>  #include <linux/bitfield.h>
> @@ -1247,7 +1247,9 @@ static int lpg_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	lpg_apply(chan);
>  
> -	triled_set(lpg, chan->triled_mask, chan->enabled ? chan->triled_mask : 0);
> +	/* Only control TRILED if the LPG channel is used by TRILED */
> +	if (chan->in_use && chan->triled_mask)

How is this possible?

If chan->in_use, then the channel is exposed as a LED and
lpg_pwm_request() should have returned -EBUSY, so we should never reach
lpg_pwm_apply()?


Why do you check chan->triled_mask? I guess we will still read/write the
triled regiter, but don't make any changes if this is 0?

Or is this the actual issue that you're fixing, that we read/write the
registers when we shouldn't? If so this should be clarified in the
commit message.

Regards,
Bjorn

> +		triled_set(lpg, chan->triled_mask, chan->enabled ? chan->triled_mask : 0);
>  
>  out_unlock:
>  	mutex_unlock(&lpg->lock);
> 
> ---
> base-commit: ea1c4c7e648d1ca91577071fc42fdc219521098c
> change-id: 20251114-lpg_triled_fix-44491b49b340
> 
> Best regards,
> -- 
> Fenglin Wu <fenglin.wu@oss.qualcomm.com>
> 
>

Re: [PATCH] leds: rgb: leds-qcom-lpg: Only enable TRILED when LPG is used by LED

Posted by Fenglin Wu 2 months, 3 weeks ago

On 11/14/2025 12:58 PM, Bjorn Andersson wrote:
> If chan->in_use, then the channel is exposed as a LED and
> lpg_pwm_request() should have returned -EBUSY, so we should never reach
> lpg_pwm_apply()?

Yes, I agree.

Change is trying to ignore enabling TRILED channel when the LPG channel 
mapping to TRILED is not used for controlling the LED (not defining the 
LED child nodes).

So the fix should be just removing this line instead of adding the if check.

I will update it in patch v2.

>
> Why do you check chan->triled_mask? I guess we will still read/write the
> triled regiter, but don't make any changes if this is 0?
>
> Or is this the actual issue that you're fixing, that we read/write the
> registers when we shouldn't? If so this should be clarified in the
> commit message.

Yes, there was a case that a LPG channel mapping to TRILED is repurposed 
to control a fan, and it was seen that the BOB1 (supplies to TRILED) 
voltage bumped to higher voltage when the PWM channel was enabled.

>
> Regards,
> Bjorn

Re: [PATCH] leds: rgb: leds-qcom-lpg: Only enable TRILED when LPG is used by LED

Posted by Bjorn Andersson 2 months, 3 weeks ago

On Fri, Nov 14, 2025 at 03:13:18PM +0800, Fenglin Wu wrote:
> 
> On 11/14/2025 12:58 PM, Bjorn Andersson wrote:
> > If chan->in_use, then the channel is exposed as a LED and
> > lpg_pwm_request() should have returned -EBUSY, so we should never reach
> > lpg_pwm_apply()?
> 
> Yes, I agree.
> 
> Change is trying to ignore enabling TRILED channel when the LPG channel
> mapping to TRILED is not used for controlling the LED (not defining the LED
> child nodes).
> 
> So the fix should be just removing this line instead of adding the if check.
> 

Sorry, it's been a while since I looked at this code, but isn't it
possible to configure a channel going through the triled to be exposed
as a PWM channel and if so, don't we need to enable the TRILED driver
for this channel in those cases?

> I will update it in patch v2.
> 
> > 
> > Why do you check chan->triled_mask? I guess we will still read/write the
> > triled regiter, but don't make any changes if this is 0?
> > 
> > Or is this the actual issue that you're fixing, that we read/write the
> > registers when we shouldn't? If so this should be clarified in the
> > commit message.
> 
> Yes, there was a case that a LPG channel mapping to TRILED is repurposed to
> control a fan, and it was seen that the BOB1 (supplies to TRILED) voltage
> bumped to higher voltage when the PWM channel was enabled.
> 

Is the signal still routed through the TRILED, or is it muxed to another
driver?

Regards,
Bjorn

> > 
> > Regards,
> > Bjorn

Re: [PATCH] leds: rgb: leds-qcom-lpg: Only enable TRILED when LPG is used by LED

Posted by Fenglin Wu 2 months, 3 weeks ago

On 11/14/2025 10:56 PM, Bjorn Andersson wrote:
> On Fri, Nov 14, 2025 at 03:13:18PM +0800, Fenglin Wu wrote:
>> On 11/14/2025 12:58 PM, Bjorn Andersson wrote:
>>> If chan->in_use, then the channel is exposed as a LED and
>>> lpg_pwm_request() should have returned -EBUSY, so we should never reach
>>> lpg_pwm_apply()?
>> Yes, I agree.
>>
>> Change is trying to ignore enabling TRILED channel when the LPG channel
>> mapping to TRILED is not used for controlling the LED (not defining the LED
>> child nodes).
>>
>> So the fix should be just removing this line instead of adding the if check.
>>
> Sorry, it's been a while since I looked at this code, but isn't it
> possible to configure a channel going through the triled to be exposed
> as a PWM channel and if so, don't we need to enable the TRILED driver
> for this channel in those cases?

Yes, this is possible, and enabling TRILED is not necessary in this 
case. The signals from the LPG channels mapped to the TRILED channels 
can also be routed to the PMIC GPIOs by setting the GPIO pinctrl state 
to the "funcx" function. For example, for LPG channels in PMH0101, based 
on the PMIC GPIO usage table, these GPIOs can be used to output the PWM 
signals if they are configured to the "func1" function.

GPIO05 -- PWM1

GPIO06 -- PWM2

GPIO11 -- PWM3

GPIO08 -- PWM4

GPIO09 -- PWM4

>
>> I will update it in patch v2.
>>
>>> Why do you check chan->triled_mask? I guess we will still read/write the
>>> triled regiter, but don't make any changes if this is 0?
>>>
>>> Or is this the actual issue that you're fixing, that we read/write the
>>> registers when we shouldn't? If so this should be clarified in the
>>> commit message.
>> Yes, there was a case that a LPG channel mapping to TRILED is repurposed to
>> control a fan, and it was seen that the BOB1 (supplies to TRILED) voltage
>> bumped to higher voltage when the PWM channel was enabled.
>>
> Is the signal still routed through the TRILED, or is it muxed to another
> driver?
No, the signals will not be routed to TRILED but through other PMIC GPIOs.
>
> Regards,
> Bjorn
>
>>> Regards,
>>> Bjorn