| Message ID | 20240115-lpg-v5-1-3c56f77f9cec@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v5] arm64: dts: qcom: qcm6490-idp: Add definition for three LEDs | expand |
On Wed, 17 Jan 2024 at 05:02, hui liu <quic_huliu@quicinc.com> wrote: > > > > On 1/15/2024 6:26 PM, Krzysztof Kozlowski wrote: > > On 15/01/2024 11:18, hui liu wrote: > >> > >> > >> On 1/15/2024 5:56 PM, Dmitry Baryshkov wrote: > >>> On Mon, 15 Jan 2024 at 11:48, Hui Liu via B4 Relay > >>> <devnull+quic_huliu.quicinc.com@kernel.org> wrote: > >>>> > >>>> From: Hui Liu <quic_huliu@quicinc.com> > >>>> > >>>> Add definition for three LEDs to make sure they can > >>>> be enabled base on QCOM LPG LED driver. > >>> > >>> The "function" property is still placed incorrectly. Posting the next > >>> iteration before concluding the discussion on the previous one is not > >>> the best idea. > >> Do you mean I should update it as below? Seems there is no consumer to > >> use the function config, do we need to add now? > > > > Paste the output of dtbs_check for your board (or CHECK_DTBS=y for your > > Makefile target). > I checked the dt-binding file of LPG LED, I will update the dts as > below, if you think it's correct, I will push v6. Is there any reason why you are defining three different LEDs instead of multi-led with three components? > > +&pm8350c_pwm { > + #address-cells = <1>; > + #size-cells = <0>; > + status = "okay"; > + > + led@1 { > + reg = <1>; > + color = <LED_COLOR_ID_RED>; > + function = LED_FUNCTION_STATUS; > + }; > + > + led@2 { > + reg = <2>; > + color = <LED_COLOR_ID_GREEN>; > + function = LED_FUNCTION_STATUS; > + }; > + > + led@3 { > + reg = <3>; > + color = <LED_COLOR_ID_BLUE>; > + function = LED_FUNCTION_STATUS; > + }; > +};
On 1/17/2024 11:41 AM, Dmitry Baryshkov wrote: > On Wed, 17 Jan 2024 at 05:02, hui liu <quic_huliu@quicinc.com> wrote: >> >> >> >> On 1/15/2024 6:26 PM, Krzysztof Kozlowski wrote: >>> On 15/01/2024 11:18, hui liu wrote: >>>> >>>> >>>> On 1/15/2024 5:56 PM, Dmitry Baryshkov wrote: >>>>> On Mon, 15 Jan 2024 at 11:48, Hui Liu via B4 Relay >>>>> <devnull+quic_huliu.quicinc.com@kernel.org> wrote: >>>>>> >>>>>> From: Hui Liu <quic_huliu@quicinc.com> >>>>>> >>>>>> Add definition for three LEDs to make sure they can >>>>>> be enabled base on QCOM LPG LED driver. >>>>> >>>>> The "function" property is still placed incorrectly. Posting the next >>>>> iteration before concluding the discussion on the previous one is not >>>>> the best idea. >>>> Do you mean I should update it as below? Seems there is no consumer to >>>> use the function config, do we need to add now? >>> >>> Paste the output of dtbs_check for your board (or CHECK_DTBS=y for your >>> Makefile target). >> I checked the dt-binding file of LPG LED, I will update the dts as >> below, if you think it's correct, I will push v6. > > Is there any reason why you are defining three different LEDs instead > of multi-led with three components? In the HW design, they are three seprete LEDs, there are three LEDs on device. why do we need to add for multi-led? Thanks, Hui > >> >> +&pm8350c_pwm { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + status = "okay"; >> + >> + led@1 { >> + reg = <1>; >> + color = <LED_COLOR_ID_RED>; >> + function = LED_FUNCTION_STATUS; >> + }; >> + >> + led@2 { >> + reg = <2>; >> + color = <LED_COLOR_ID_GREEN>; >> + function = LED_FUNCTION_STATUS; >> + }; >> + >> + led@3 { >> + reg = <3>; >> + color = <LED_COLOR_ID_BLUE>; >> + function = LED_FUNCTION_STATUS; >> + }; >> +}; > > >
On 1/22/2024 1:42 PM, hui liu wrote: > > > On 1/18/2024 10:06 AM, hui liu wrote: >> >> >> On 1/17/2024 11:41 AM, Dmitry Baryshkov wrote: >>> On Wed, 17 Jan 2024 at 05:02, hui liu <quic_huliu@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 1/15/2024 6:26 PM, Krzysztof Kozlowski wrote: >>>>> On 15/01/2024 11:18, hui liu wrote: >>>>>> >>>>>> >>>>>> On 1/15/2024 5:56 PM, Dmitry Baryshkov wrote: >>>>>>> On Mon, 15 Jan 2024 at 11:48, Hui Liu via B4 Relay >>>>>>> <devnull+quic_huliu.quicinc.com@kernel.org> wrote: >>>>>>>> >>>>>>>> From: Hui Liu <quic_huliu@quicinc.com> >>>>>>>> >>>>>>>> Add definition for three LEDs to make sure they can >>>>>>>> be enabled base on QCOM LPG LED driver. >>>>>>> >>>>>>> The "function" property is still placed incorrectly. Posting the >>>>>>> next >>>>>>> iteration before concluding the discussion on the previous one is >>>>>>> not >>>>>>> the best idea. >>>>>> Do you mean I should update it as below? Seems there is no >>>>>> consumer to >>>>>> use the function config, do we need to add now? >>>>> >>>>> Paste the output of dtbs_check for your board (or CHECK_DTBS=y for >>>>> your >>>>> Makefile target). >>>> I checked the dt-binding file of LPG LED, I will update the dts as >>>> below, if you think it's correct, I will push v6. >>> >>> Is there any reason why you are defining three different LEDs instead >>> of multi-led with three components? > >> In the HW design, they are three seprete LEDs, there are three LEDs on >> device. why do we need to add for multi-led? >> >> Thanks, >> Hui I double confirmed the HW design, for IDP devcie, we should set it to multi led, for another similar device(RB3-GEN2, I will push LED change for this device later), it should be set to seperate LED. They are different, so I will push V6 to set it for multi-led for QCM6490-IDP device. Thanks for your review. >>> >>>> >>>> +&pm8350c_pwm { >>>> + #address-cells = <1>; >>>> + #size-cells = <0>; >>>> + status = "okay"; >>>> + >>>> + led@1 { >>>> + reg = <1>; >>>> + color = <LED_COLOR_ID_RED>; >>>> + function = LED_FUNCTION_STATUS; >>>> + }; >>>> + >>>> + led@2 { >>>> + reg = <2>; >>>> + color = <LED_COLOR_ID_GREEN>; >>>> + function = LED_FUNCTION_STATUS; >>>> + }; >>>> + >>>> + led@3 { >>>> + reg = <3>; >>>> + color = <LED_COLOR_ID_BLUE>; >>>> + function = LED_FUNCTION_STATUS; >>>> + }; >>>> +}; >>> >>> >>>
On Mon, 22 Jan 2024 at 08:26, hui liu <quic_huliu@quicinc.com> wrote: > > > > On 1/22/2024 1:42 PM, hui liu wrote: > > > > > > On 1/18/2024 10:06 AM, hui liu wrote: > >> > >> > >> On 1/17/2024 11:41 AM, Dmitry Baryshkov wrote: > >>> On Wed, 17 Jan 2024 at 05:02, hui liu <quic_huliu@quicinc.com> wrote: > >>>> > >>>> > >>>> > >>>> On 1/15/2024 6:26 PM, Krzysztof Kozlowski wrote: > >>>>> On 15/01/2024 11:18, hui liu wrote: > >>>>>> > >>>>>> > >>>>>> On 1/15/2024 5:56 PM, Dmitry Baryshkov wrote: > >>>>>>> On Mon, 15 Jan 2024 at 11:48, Hui Liu via B4 Relay > >>>>>>> <devnull+quic_huliu.quicinc.com@kernel.org> wrote: > >>>>>>>> > >>>>>>>> From: Hui Liu <quic_huliu@quicinc.com> > >>>>>>>> > >>>>>>>> Add definition for three LEDs to make sure they can > >>>>>>>> be enabled base on QCOM LPG LED driver. > >>>>>>> > >>>>>>> The "function" property is still placed incorrectly. Posting the > >>>>>>> next > >>>>>>> iteration before concluding the discussion on the previous one is > >>>>>>> not > >>>>>>> the best idea. > >>>>>> Do you mean I should update it as below? Seems there is no > >>>>>> consumer to > >>>>>> use the function config, do we need to add now? > >>>>> > >>>>> Paste the output of dtbs_check for your board (or CHECK_DTBS=y for > >>>>> your > >>>>> Makefile target). > >>>> I checked the dt-binding file of LPG LED, I will update the dts as > >>>> below, if you think it's correct, I will push v6. > >>> > >>> Is there any reason why you are defining three different LEDs instead > >>> of multi-led with three components? > > > >> In the HW design, they are three seprete LEDs, there are three LEDs on > >> device. why do we need to add for multi-led? > >> > >> Thanks, > >> Hui > > I double confirmed the HW design, for IDP devcie, we should set it to > multi led, for another similar device(RB3-GEN2, I will push LED change > for this device later), it should be set to seperate LED. > They are different, so I will push V6 to set it for multi-led for > QCM6490-IDP device. Thanks for your review. Ack, thank you. > > >>> > >>>> > >>>> +&pm8350c_pwm { > >>>> + #address-cells = <1>; > >>>> + #size-cells = <0>; > >>>> + status = "okay"; > >>>> + > >>>> + led@1 { > >>>> + reg = <1>; > >>>> + color = <LED_COLOR_ID_RED>; > >>>> + function = LED_FUNCTION_STATUS; > >>>> + }; > >>>> + > >>>> + led@2 { > >>>> + reg = <2>; > >>>> + color = <LED_COLOR_ID_GREEN>; > >>>> + function = LED_FUNCTION_STATUS; > >>>> + }; > >>>> + > >>>> + led@3 { > >>>> + reg = <3>; > >>>> + color = <LED_COLOR_ID_BLUE>; > >>>> + function = LED_FUNCTION_STATUS; > >>>> + }; > >>>> +}; > >>> > >>> > >>>
On 1/22/2024 9:37 PM, Dmitry Baryshkov wrote: > On Mon, 22 Jan 2024 at 08:26, hui liu <quic_huliu@quicinc.com> wrote: >> >> >> >> On 1/22/2024 1:42 PM, hui liu wrote: >>> >>> >>> On 1/18/2024 10:06 AM, hui liu wrote: >>>> >>>> >>>> On 1/17/2024 11:41 AM, Dmitry Baryshkov wrote: >>>>> On Wed, 17 Jan 2024 at 05:02, hui liu <quic_huliu@quicinc.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 1/15/2024 6:26 PM, Krzysztof Kozlowski wrote: >>>>>>> On 15/01/2024 11:18, hui liu wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 1/15/2024 5:56 PM, Dmitry Baryshkov wrote: >>>>>>>>> On Mon, 15 Jan 2024 at 11:48, Hui Liu via B4 Relay >>>>>>>>> <devnull+quic_huliu.quicinc.com@kernel.org> wrote: >>>>>>>>>> >>>>>>>>>> From: Hui Liu <quic_huliu@quicinc.com> >>>>>>>>>> >>>>>>>>>> Add definition for three LEDs to make sure they can >>>>>>>>>> be enabled base on QCOM LPG LED driver. >>>>>>>>> >>>>>>>>> The "function" property is still placed incorrectly. Posting the >>>>>>>>> next >>>>>>>>> iteration before concluding the discussion on the previous one is >>>>>>>>> not >>>>>>>>> the best idea. >>>>>>>> Do you mean I should update it as below? Seems there is no >>>>>>>> consumer to >>>>>>>> use the function config, do we need to add now? >>>>>>> >>>>>>> Paste the output of dtbs_check for your board (or CHECK_DTBS=y for >>>>>>> your >>>>>>> Makefile target). >>>>>> I checked the dt-binding file of LPG LED, I will update the dts as >>>>>> below, if you think it's correct, I will push v6. >>>>> >>>>> Is there any reason why you are defining three different LEDs instead >>>>> of multi-led with three components? >>> >>>> In the HW design, they are three seprete LEDs, there are three LEDs on >>>> device. why do we need to add for multi-led? >>>> >>>> Thanks, >>>> Hui >> >> I double confirmed the HW design, for IDP devcie, we should set it to >> multi led, for another similar device(RB3-GEN2, I will push LED change >> for this device later), it should be set to seperate LED. >> They are different, so I will push V6 to set it for multi-led for >> QCM6490-IDP device. Thanks for your review. > > Ack, thank you. Hi Dmitry, Could you give the approval for V6? https://lore.kernel.org/all/20240126-lpg-v6-1-f879cecbce69@quicinc.com/ Thanks, Hui > >> >>>>> >>>>>> >>>>>> +&pm8350c_pwm { >>>>>> + #address-cells = <1>; >>>>>> + #size-cells = <0>; >>>>>> + status = "okay"; >>>>>> + >>>>>> + led@1 { >>>>>> + reg = <1>; >>>>>> + color = <LED_COLOR_ID_RED>; >>>>>> + function = LED_FUNCTION_STATUS; >>>>>> + }; >>>>>> + >>>>>> + led@2 { >>>>>> + reg = <2>; >>>>>> + color = <LED_COLOR_ID_GREEN>; >>>>>> + function = LED_FUNCTION_STATUS; >>>>>> + }; >>>>>> + >>>>>> + led@3 { >>>>>> + reg = <3>; >>>>>> + color = <LED_COLOR_ID_BLUE>; >>>>>> + function = LED_FUNCTION_STATUS; >>>>>> + }; >>>>>> +}; >>>>> >>>>> >>>>> > > >
diff --git a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts index 37c91fdf3ab9..8268fad505e7 100644 --- a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts +++ b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts @@ -5,6 +5,7 @@ /dts-v1/; +#include <dt-bindings/leds/common.h> #include <dt-bindings/regulator/qcom,rpmh-regulator.h> #include "sc7280.dtsi" #include "pm7325.dtsi" @@ -414,6 +415,28 @@ vreg_bob_3p296: bob { }; }; +&pm8350c_pwm { + function = LED_FUNCTION_STATUS; + #address-cells = <1>; + #size-cells = <0>; + status = "okay"; + + led@1 { + reg = <1>; + color = <LED_COLOR_ID_RED>; + }; + + led@2 { + reg = <2>; + color = <LED_COLOR_ID_GREEN>; + }; + + led@3 { + reg = <3>; + color = <LED_COLOR_ID_BLUE>; + }; +}; + &qupv3_id_0 { status = "okay"; };