Date:   Sun, 29 Jan 2023 19:56:42 -0800
From:   Bjorn Andersson <quic_bjorande@quicinc.com>
To:     Johan Hovold <johan@kernel.org>
CC:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        <linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-arm-msm@vger.kernel.org>
Subject: Re: [PATCH v2 3/3] arm64: dts: qcom: sa8295p-adp: Add max20411 on
 i2c12
Message-ID: <20230130035642.GA464800@hu-bjorande-lv.qualcomm.com>
References: <20230124184440.1421074-1-quic_bjorande@quicinc.com>
 <20230124184440.1421074-4-quic_bjorande@quicinc.com>
 <Y9OfiTDwFHN0RR5G@hovoldconsulting.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Y9OfiTDwFHN0RR5G@hovoldconsulting.com>
Precedence: bulk

On Fri, Jan 27, 2023 at 10:55:21AM +0100, Johan Hovold wrote:
> On Tue, Jan 24, 2023 at 10:44:40AM -0800, Bjorn Andersson wrote:
> > From: Bjorn Andersson <bjorn.andersson@linaro.org>
> > 
> > The SA8295P ADP has a Maxim max20411 step-down converter on i2c12.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > ---
> > 
> > Changes since v1:
> > - i2c node had changed name
> > 
> >  arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 41 ++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> > index bb4270e8f551..642000d95812 100644
> > --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> > +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> > @@ -266,6 +266,27 @@ &dispcc1 {
> >  	status = "okay";
> >  };
> >  
> > +&i2c12 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&i2c12_state>;
> > +
> > +	status = "okay";
> > +
> > +	vdd_gfx: regulator@39 {
> 
> Nit: Should the label be named 'vreg_gfx' (or 'vreg_vdd_gfx)') for
> consistency with rest of the file?
> 

I will investigate what the appropriate name is, consistency would be
nice though.

> > +		compatible = "maxim,max20411";
> > +		reg = <0x39>;
> > +
> > +		regulator-name = "vdd_gfx";
> > +		regulator-min-microvolt = <800000>;
> > +		regulator-max-microvolt = <968750>;
> > +
> > +		enable-gpios = <&pmm8540a_gpios 2 GPIO_ACTIVE_HIGH>;
> > +
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&vdd_gfx_enable_state>;
> > +	};
> > +};
> > +
> >  &mdss0 {
> >  	status = "okay";
> >  };
> > @@ -476,6 +497,10 @@ &pcie4_phy {
> >  	status = "okay";
> >  };
> >  
> > +&qup1 {
> > +	status = "okay";
> > +};
> > +
> >  &qup2 {
> >  	status = "okay";
> >  };
> > @@ -636,7 +661,23 @@ &xo_board_clk {
> >  
> >  /* PINCTRL */
> >  
> > +&pmm8540a_gpios {
> > +	vdd_gfx_enable_state: vdd-gfx-enable-state {
> 
> For consistency with the rest of sc8280xp, can you rename this
> 
> 	vdd_gfx_en: vdd-gfx-en-state {
> 
> (i.e. drop the 'state' from the label and shorten 'enable')?
> 

Will do.

> > +		pins = "gpio2";
> > +		function = "normal";
> > +		output-enable;
> > +	};
> > +};
> > +
> >  &tlmm {
> > +	i2c12_state: i2c12-state {
> 
> Similar here, this should be
> 
> 	i2c12_default: i2c12-default-state {
> 

Sounds reasonable.

> > +		pins = "gpio0", "gpio1";
> > +		function = "qup12";
> > +
> 
> And this newline can be removed.
> 

Sure

> > +		drive-strength = <2>;
> > +		bias-pull-up;
> > +	};
> > +
> >  	pcie2a_default: pcie2a-default-state {
> >  		clkreq-n-pins {
> >  			pins = "gpio142";
> 
> Johan

Thanks Johan,
Bjorn