mbox series

[0/3] Add PM8350C PMIC PWM support for backlight

Message ID 1630924867-4663-1-git-send-email-skakit@codeaurora.org
Headers show
Series Add PM8350C PMIC PWM support for backlight | expand

Message

Satya Priya Sept. 6, 2021, 10:41 a.m. UTC 
This series depends on [1], which adds driver for Qualcomm LPG.

[1] https://patchwork.kernel.org/project/linux-arm-msm/list/?series=505483&archive=both&state=*

satya priya (3):
  dt-bindings: leds: Add pm8350c pmic support
  leds: Add pm8350c support to Qualcomm LPG driver
  arm64: dts: qcom: pm8350c: Add pwm support

 Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml |  1 +
 arch/arm64/boot/dts/qcom/pm8350c.dtsi                     |  6 ++++++
 drivers/leds/rgb/leds-qcom-lpg.c                          | 10 ++++++++++
 3 files changed, 17 insertions(+)

Comments

Matthias Kaehlcke Sept. 7, 2021, 5:48 p.m. UTC | #1 
On Mon, Sep 06, 2021 at 04:11:05PM +0530, satya priya wrote:
> Add pm8350c pmic pwm support.
> 
> Signed-off-by: satya priya <skakit@codeaurora.org>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Matthias Kaehlcke Sept. 7, 2021, 6:16 p.m. UTC | #2 
On Mon, Sep 06, 2021 at 04:11:07PM +0530, satya priya wrote:
> Add pwm support for PM8350C pmic.

> 

> Signed-off-by: satya priya <skakit@codeaurora.org>

> ---

>  arch/arm64/boot/dts/qcom/pm8350c.dtsi | 6 ++++++

>  1 file changed, 6 insertions(+)

> 

> diff --git a/arch/arm64/boot/dts/qcom/pm8350c.dtsi b/arch/arm64/boot/dts/qcom/pm8350c.dtsi

> index e1b75ae..ecdae55 100644

> --- a/arch/arm64/boot/dts/qcom/pm8350c.dtsi

> +++ b/arch/arm64/boot/dts/qcom/pm8350c.dtsi

> @@ -29,6 +29,12 @@

>  			interrupt-controller;

>  			#interrupt-cells = <2>;

>  		};

> +

> +		pm8350c_pwm4: pwm {


What does the '4' represent, an internal channel number? It should
probably be omitted if the PM8350 only has a single output PWM
port.

> +			compatible = "qcom,pm8350c-pwm";

> +			#pwm-cells = <2>;

> +			status = "okay";


I don't think it should be enabled by default, there may be boards with
the PM8350C that don't use the PWM.
Stephen Boyd Sept. 7, 2021, 8:03 p.m. UTC | #3 
Quoting satya priya (2021-09-06 03:41:05)
> Add pm8350c pmic pwm support.
>
> Signed-off-by: satya priya <skakit@codeaurora.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Stephen Boyd Sept. 8, 2021, 3:34 a.m. UTC | #4 
Quoting satya priya (2021-09-06 03:41:07)
> Add pwm support for PM8350C pmic.

>

> Signed-off-by: satya priya <skakit@codeaurora.org>

> ---

>  arch/arm64/boot/dts/qcom/pm8350c.dtsi | 6 ++++++

>  1 file changed, 6 insertions(+)

>

> diff --git a/arch/arm64/boot/dts/qcom/pm8350c.dtsi b/arch/arm64/boot/dts/qcom/pm8350c.dtsi

> index e1b75ae..ecdae55 100644

> --- a/arch/arm64/boot/dts/qcom/pm8350c.dtsi

> +++ b/arch/arm64/boot/dts/qcom/pm8350c.dtsi

> @@ -29,6 +29,12 @@

>                         interrupt-controller;

>                         #interrupt-cells = <2>;

>                 };

> +

> +               pm8350c_pwm4: pwm {

> +                       compatible = "qcom,pm8350c-pwm";


Shouldn't there be a reg property?

> +                       #pwm-cells = <2>;

> +                       status = "okay";

> +               };

>         };

>  };

>

> --

> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member

> of Code Aurora Forum, hosted by The Linux Foundation

>
Satya Priya Sept. 8, 2021, 9:07 a.m. UTC | #5 
On 2021-09-07 23:46, Matthias Kaehlcke wrote:
> On Mon, Sep 06, 2021 at 04:11:07PM +0530, satya priya wrote:

>> Add pwm support for PM8350C pmic.

>> 

>> Signed-off-by: satya priya <skakit@codeaurora.org>

>> ---

>>  arch/arm64/boot/dts/qcom/pm8350c.dtsi | 6 ++++++

>>  1 file changed, 6 insertions(+)

>> 

>> diff --git a/arch/arm64/boot/dts/qcom/pm8350c.dtsi 

>> b/arch/arm64/boot/dts/qcom/pm8350c.dtsi

>> index e1b75ae..ecdae55 100644

>> --- a/arch/arm64/boot/dts/qcom/pm8350c.dtsi

>> +++ b/arch/arm64/boot/dts/qcom/pm8350c.dtsi

>> @@ -29,6 +29,12 @@

>>  			interrupt-controller;

>>  			#interrupt-cells = <2>;

>>  		};

>> +

>> +		pm8350c_pwm4: pwm {

> 

> What does the '4' represent, an internal channel number? It should

> probably be omitted if the PM8350 only has a single output PWM

> port.

> 


pm8350c has four PWMs, but I think we can drop the '4' here.

>> +			compatible = "qcom,pm8350c-pwm";

>> +			#pwm-cells = <2>;

>> +			status = "okay";

> 

> I don't think it should be enabled by default, there may be boards with

> the PM8350C that don't use the PWM.


Okay.
Satya Priya Sept. 8, 2021, 9:14 a.m. UTC | #6 
On 2021-09-08 09:04, Stephen Boyd wrote:
> Quoting satya priya (2021-09-06 03:41:07)
>> Add pwm support for PM8350C pmic.
>> 
>> Signed-off-by: satya priya <skakit@codeaurora.org>
>> ---
>>  arch/arm64/boot/dts/qcom/pm8350c.dtsi | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/arch/arm64/boot/dts/qcom/pm8350c.dtsi 
>> b/arch/arm64/boot/dts/qcom/pm8350c.dtsi
>> index e1b75ae..ecdae55 100644
>> --- a/arch/arm64/boot/dts/qcom/pm8350c.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/pm8350c.dtsi
>> @@ -29,6 +29,12 @@
>>                         interrupt-controller;
>>                         #interrupt-cells = <2>;
>>                 };
>> +
>> +               pm8350c_pwm4: pwm {
>> +                       compatible = "qcom,pm8350c-pwm";
> 
> Shouldn't there be a reg property?
> 

The bindings do not specify reg property. I think it is because we are 
adding the base address in struct "pm8350c_pwm_data".

>> +                       #pwm-cells = <2>;
>> +                       status = "okay";
>> +               };
>>         };
>>  };
>> 
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
>> member
>> of Code Aurora Forum, hosted by The Linux Foundation
>>
Rob Herring (Arm) Sept. 8, 2021, 1:52 p.m. UTC | #7 
On Mon, 06 Sep 2021 16:11:05 +0530, satya priya wrote:
> Add pm8350c pmic pwm support.

> 

> Signed-off-by: satya priya <skakit@codeaurora.org>

> ---

>  Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml | 1 +

>  1 file changed, 1 insertion(+)

> 


Acked-by: Rob Herring <robh@kernel.org>
Matthias Kaehlcke Sept. 8, 2021, 3:29 p.m. UTC | #8 
On Wed, Sep 08, 2021 at 02:37:39PM +0530, skakit@codeaurora.org wrote:
> On 2021-09-07 23:46, Matthias Kaehlcke wrote:
> > On Mon, Sep 06, 2021 at 04:11:07PM +0530, satya priya wrote:
> > > Add pwm support for PM8350C pmic.
> > > 
> > > Signed-off-by: satya priya <skakit@codeaurora.org>
> > > ---
> > >  arch/arm64/boot/dts/qcom/pm8350c.dtsi | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/pm8350c.dtsi
> > > b/arch/arm64/boot/dts/qcom/pm8350c.dtsi
> > > index e1b75ae..ecdae55 100644
> > > --- a/arch/arm64/boot/dts/qcom/pm8350c.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/pm8350c.dtsi
> > > @@ -29,6 +29,12 @@
> > >  			interrupt-controller;
> > >  			#interrupt-cells = <2>;
> > >  		};
> > > +
> > > +		pm8350c_pwm4: pwm {
> > 
> > What does the '4' represent, an internal channel number? It should
> > probably be omitted if the PM8350 only has a single output PWM
> > port.
> > 
> 
> pm8350c has four PWMs, but I think we can drop the '4' here.

Why is only one PWM exposed if the PMIC has for of them? Why number 4
and not one of the others?
Satya Priya Sept. 9, 2021, 6:11 a.m. UTC | #9 
On 2021-09-08 22:38, Bjorn Andersson wrote:
> On Wed 08 Sep 08:29 PDT 2021, Matthias Kaehlcke wrote:
> 
>> On Wed, Sep 08, 2021 at 02:37:39PM +0530, skakit@codeaurora.org wrote:
>> > On 2021-09-07 23:46, Matthias Kaehlcke wrote:
>> > > On Mon, Sep 06, 2021 at 04:11:07PM +0530, satya priya wrote:
>> > > > Add pwm support for PM8350C pmic.
>> > > >
>> > > > Signed-off-by: satya priya <skakit@codeaurora.org>
>> > > > ---
>> > > >  arch/arm64/boot/dts/qcom/pm8350c.dtsi | 6 ++++++
>> > > >  1 file changed, 6 insertions(+)
>> > > >
>> > > > diff --git a/arch/arm64/boot/dts/qcom/pm8350c.dtsi
>> > > > b/arch/arm64/boot/dts/qcom/pm8350c.dtsi
>> > > > index e1b75ae..ecdae55 100644
>> > > > --- a/arch/arm64/boot/dts/qcom/pm8350c.dtsi
>> > > > +++ b/arch/arm64/boot/dts/qcom/pm8350c.dtsi
>> > > > @@ -29,6 +29,12 @@
>> > > >  			interrupt-controller;
>> > > >  			#interrupt-cells = <2>;
>> > > >  		};
>> > > > +
>> > > > +		pm8350c_pwm4: pwm {
>> > >
>> > > What does the '4' represent, an internal channel number? It should
>> > > probably be omitted if the PM8350 only has a single output PWM
>> > > port.
>> > >
>> >
>> > pm8350c has four PWMs, but I think we can drop the '4' here.
>> 
>> Why is only one PWM exposed if the PMIC has for of them? Why number 4
>> and not one of the others?
> 

pwm4 is used for backlight support on kodiak crd board, so I mentioned 
4, thinking 4 nodes should be present for 4 pwms.
but I see that we need to represent all the four channels as one node. 
will drop the '4' in next version.

Thanks,
Satya Priya

> The node should represent all 4 channels, which ones the board uses is
> captured in how they are bound to other clients - or defines as LEDs by
> additional child nodes.
> 
> Regards,
> Bjorn