Re: [PATCH v2] leds: rgb: leds-qcom-lpg: Don't enable TRILED when configuring PWM

[Fenglin Wu <fenglin.wu@oss.qualcomm.com>]

On 11/19/2025 11:18 AM, Bjorn Andersson wrote:
> On Wed, Nov 19, 2025 at 10:17:44AM +0800, Fenglin Wu wrote:
>> On 11/19/2025 12:27 AM, Bjorn Andersson wrote:
>>> On Tue, Nov 18, 2025 at 10:48:03AM +0800, Fenglin Wu via B4 Relay wrote:
>>>> From: Fenglin Wu <fenglin.wu@oss.qualcomm.com>
>>>>
>>> Sorry, I didn't find the chance to answer your reply yesterday, and then
>>> you sent v2, so I'll have to continue the discussion here.
>>>
>>>> The PWM signal from the LPG channel can be routed to PMIC GPIOs with
>>>> proper GPIO configuration, and it is not necessary to enable the
>>>> TRILED channel in that case. This also applies to the LPG channels
>>>> that mapped to TRILED channels. Additionally, enabling the TRILED
>>>> channel unnecessarily would cause a voltage increase in its power
>>>> supply. Hence remove it.
>>>>
>>>> Fixes: 24e2d05d1b68 ("leds: Add driver for Qualcomm LPG")
>>>> Signed-off-by: Fenglin Wu <fenglin.wu@oss.qualcomm.com>
>>>> ---
>>>> Changes in v2:
>>>> - Check "chan->in_use" flag in lpg_pwm_apply() is not correct, as it
>>>>     indicates the channel is being used as a LED and this PWM API would
>>>>     never get called. Instead, remove the code line which enables TRILED
>>>>     in lpg_pwm_apply() and update the commit text to explain it clearly.
>>>>
>>> I understand that in your case you're routing the PWM signal to a GPIO,
>>> and in that case the TRILED output should be kept disabled.
>>>
>>> But what if I have my load connected to the TRILED and I describe my LPG
>>> channel as a PWM channel? Is this an invalid use case?
>> This is not a valid case. If a load (typically an LED) is connected to any
>> channel of theTRILED module, it means the LPG channel is used for driving an
>> LED, and sub-nodes for the LED devices must be defined. Otherwise, the PWM
>> signal will be gated by the TRILED module. Enabling a TRILED channel not
>> only opens the PWM gate but also activates an internal current sink to
>> manage the load. If you need to output PWM as a control signal, for example
>> for fan control, the hardware should connect the fan control input to a PMIC
>> GPIO. The PWM signal from an LPG channel can be routed there, rather than
>> using a TRILED channel.
> Thanks for elaborating, I think this is an okay stance to take in the
> question. Let's document it (like you propose below) and make the change
> you're proposing.
>
>>> With this patch,
>>> everything will look like it's working, except silently my signal won't
>>> come out.
>>>
>>> I presume there's no additional configuration on the LPG-side for your
>>> use case. We just configure the GPIO to tap into the PWM-signal through
>>> the pinmux settings?
>> That's correct.
>>> Also, if for some reason the triled was enabled by bootloader, you will
>>> now leave it enabled forever. This perhaps isn't a big issue though...
>> In that case, I would assume that the bootloader should also be customized
>> to not enabling any LED if there is not a physical LED device connected to
>> the TRILED channel.
>>> Perhaps none of this matters in practice, and we should just proceed
>>> with your approach. If that's the case, then we should at least document
>>> the behavior.
>> Does it look good if I push a change in the DT binding document to explain
>> this?
>>
>> --- a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
>> +++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
>> @@ -13,6 +13,10 @@ description: >
>>     The Qualcomm Light Pulse Generator consists of three different hardware
>> blocks;
>>     a ramp generator with lookup table (LUT), the light pulse generator and a
>> three
>>     channel current sink. These blocks are found in a wide range of Qualcomm
>> PMICs.
>> +  The light pulse generator (LPG) can also be used independently to output
>> PWM
>> +  signal for standard PWM applications. In this scenario, the LPG output
>> should
>> +  be routed to a specific PMIC GPIO by setting the GPIO pin mux to the
>> special
>> +  functions indicated in the datasheet.
> I like this. How about continuing this sentence with ", the TRILED
> driver for the channel will not be enabled in this configuration."?
>
> That way we make sure the decided TRILED behavior is documented as well.
>
> Regards,
> Bjorn

Thanks. I will push a change for this soon.

>>   properties:
>>     compatible:
>>
>>> Regards,
>>> Bjorn
>>>
>>>> - Link to v1: https://lore.kernel.org/r/20251114-lpg_triled_fix-v1-1-9b239832c53c@oss.qualcomm.com
>>>> ---
>>>>    drivers/leds/rgb/leds-qcom-lpg.c | 4 +---
>>>>    1 file changed, 1 insertion(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
>>>> index 4f2a178e3d265a2cc88e651d3e2ca6ae3dfac2e2..e197f548cddb03d079c54c4a0f402402c5d047e2 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,8 +1247,6 @@ 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);
>>>> -
>>>>    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>
>>>>
>>>>