Re: [PATCH v2 13/15] arm64: dts: qcom: pmi632: define USB-C related blocks

From: Bjorn Andersson <quic_bjorande@quicinc.com>
To: Konrad Dybcio <konrad.dybcio@linaro.org>
Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	Bjorn Andersson <andersson@kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Wesley Cheng <quic_wcheng@quicinc.com>,
	Bryan O'Donoghue <bryan.odonoghue@linaro.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Vinod Koul <vkoul@kernel.org>,
	"Kishon Vijay Abraham I" <kishon@kernel.org>,
	Guenter Roeck <linux@roeck-us.net>,
	"Heikki Krogerus" <heikki.krogerus@linux.intel.com>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	<linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<linux-usb@vger.kernel.org>, <linux-phy@lists.infradead.org>
Subject: Re: [PATCH v2 13/15] arm64: dts: qcom: pmi632: define USB-C related blocks
Date: Wed, 17 Jan 2024 14:01:53 -0800	[thread overview]
Message-ID: <20240117220153.GA649327@hu-bjorande-lv.qualcomm.com> (raw)
In-Reply-To: <1d0d325d-d15e-4e86-b8e3-9f91b99e78bf@linaro.org>

On Mon, Jan 15, 2024 at 11:00:53AM +0100, Konrad Dybcio wrote:
> On 13.01.2024 21:55, Dmitry Baryshkov wrote:
> > Define VBUS regulator and the Type-C handling block as present on the
> > Quacomm PMI632 PMIC.
> > 
> > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/pmi632.dtsi | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/pmi632.dtsi b/arch/arm64/boot/dts/qcom/pmi632.dtsi
> > index 4eb79e0ce40a..d6832f0b7b80 100644
> > --- a/arch/arm64/boot/dts/qcom/pmi632.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/pmi632.dtsi
> > @@ -45,6 +45,36 @@ pmic@2 {
> >  		#address-cells = <1>;
> >  		#size-cells = <0>;
> >  
> > +		pmi632_vbus: usb-vbus-regulator@1100 {
> > +			compatible = "qcom,pmi632-vbus-reg", "qcom,pm8150b-vbus-reg";
> > +			reg = <0x1100>;
> > +			status = "disabled";
> > +		};
> > +
> > +		pmi632_typec: typec@1500 {
> > +			compatible = "qcom,pmi632-typec";
> > +			reg = <0x1500>;
> > +			interrupts = <0x2 0x15 0x00 IRQ_TYPE_EDGE_RISING>,
> > +				     <0x2 0x15 0x01 IRQ_TYPE_EDGE_BOTH>,
> > +				     <0x2 0x15 0x02 IRQ_TYPE_EDGE_RISING>,
> > +				     <0x2 0x15 0x03 IRQ_TYPE_EDGE_BOTH>,
> > +				     <0x2 0x15 0x04 IRQ_TYPE_EDGE_RISING>,
> > +				     <0x2 0x15 0x05 IRQ_TYPE_EDGE_RISING>,
> > +				     <0x2 0x15 0x06 IRQ_TYPE_EDGE_BOTH>,
> > +				     <0x2 0x15 0x07 IRQ_TYPE_EDGE_RISING>;
> This differs from the downstream irq types:
> 
> <0x2 0x15 0x0 IRQ_TYPE_EDGE_BOTH>,
> <0x2 0x15 0x1 IRQ_TYPE_EDGE_BOTH>,
> <0x2 0x15 0x2 IRQ_TYPE_EDGE_RISING>,
> <0x2 0x15 0x3 IRQ_TYPE_EDGE_RISING>,
> <0x2 0x15 0x4 IRQ_TYPE_EDGE_BOTH>,
> <0x2 0x15 0x5 IRQ_TYPE_EDGE_RISING>,
> <0x2 0x15 0x6 IRQ_TYPE_EDGE_RISING>,
> <0x2 0x15 0x7 IRQ_TYPE_EDGE_RISING>;
> 

Interrupt 1, 3, and 6 are level interrupts for which it's reasonable to
act on both edges. Interrupt 0, 2, 4, 5, and 7 are "pulse interrupts",
for which it seems reasonable to act on only one of the edges.

To me, Dmitry's proposed version makes more sense than downstream.

Regards,
Bjorn

> Is it intended?
> 
> Thanks a lot for working on this!
> 
> Konrad
> 

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy