Re: [PATCH v6 6/8] arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280

From: Johan Hovold <johan@kernel.org>
To: Andrew Halaney <ahalaney@redhat.com>,
	Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
Cc: Johan Hovold <johan+linaro@kernel.org>,
	Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Andy Gross <agross@kernel.org>,
	Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
	quic_pkondeti@quicinc.com, quic_ppratap@quicinc.com,
	quic_wcheng@quicinc.com, quic_jackp@quicinc.com,
	quic_harshq@quicinc.com, quic_shazhuss@quicinc.com
Subject: Re: [PATCH v6 6/8] arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280
Date: Fri, 5 May 2023 09:55:52 +0200	[thread overview]
Message-ID: <ZFS2iJOfhsM8gxK5@hovoldconsulting.com> (raw)
In-Reply-To: <20230425203328.hrz5dw7f2vsbbbgk@halaney-x13s>

On Tue, Apr 25, 2023 at 03:33:28PM -0500, Andrew Halaney wrote:
> On Sat, Apr 22, 2023 at 09:38:44PM +0530, Krishna Kurapati PSSNV wrote:

> > Hi Andrew, Johan,
> > 
> >   I was looking at the pwr_event_irq interrupts for Multiport controller and
> > see that there are two of them as per HW specs. All targets till date have
> > only 1 pwr_event_irq required.
> > 
> > The reason I thought I missed pwr_event_irq in my patches is because in
> > downstream this is a required IRQ for all targets, so I was under assumption
> > that we need it for upstream targets as well. But upstream qcom driver
> > doesn't have support for this IRQ yet. And this has been made a required one
> > only for SC8280 [1]/[2].
> > 
> > Probably we can proceed in one of the following ways:
> > 1. Remove pwr_event_irq in both bindings and DT as driver support is not
> > present currently.
> > 2. Update the bindings for SC8280 to include an optional secondary
> > pwr_event_irq for multiport controller.
> > 
> > I would prefer option-1 as removing them would be better because they are
> > not being used. Please let me know your thoughts on this.
> > 
> > [1]:
> > https://lore.kernel.org/all/20220713131340.29401-2-johan+linaro@kernel.org/
> > [2]:
> > https://lore.kernel.org/all/20220713131340.29401-6-johan+linaro@kernel.org/
> > 
> 
> Personally, I prefer option 2 since the IRQ does exist technically
> (although it isn't currently used), I like it being described... it
> makes the dt-binding a more complete description of the hardware.
> 
> I am unsure of the rules wrt dt-bindings and usage in drivers, but I
> always like to view it as "this is a description of the hardware", and
> the driver bit is just nice to have to ensure that whoever is adding the
> binding is actually describing things sufficiently.

As Andrew mentioned, the binding should reflect the hardware and not
what is currently supported in some version of software. 

It looks like you even had four of these pwr_event interrupt line
judging from your last iteration of this series.

Johan