Re: [PATCH] spmi: pmic-arb: Add support for PMIC v7

From: Stephen Boyd <sboyd@kernel.org>
To: David Collins <quic_collinsd@quicinc.com>, Vinod Koul <vkoul@kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	linux-arm-msm@vger.kernel.org,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
	Subbaraman Narayanamurthy <quic_subbaram@quicinc.com>
Subject: Re: [PATCH] spmi: pmic-arb: Add support for PMIC v7
Date: Thu, 09 Dec 2021 18:01:47 -0800	[thread overview]
Message-ID: <20211210020148.B2EA6C004DD@smtp.kernel.org> (raw)
In-Reply-To: <Ya9K/sF1YDCYCp2Y@matsya>

Quoting Vinod Koul (2021-12-07 03:52:30)
> On 03-12-21, 16:45, David Collins wrote:
> > On 12/2/21 7:13 PM, Stephen Boyd wrote:
> > > Quoting David Collins (2021-12-02 15:51:18)
> > >> On 12/2/21 3:06 PM, Stephen Boyd wrote:
> > >>> Quoting Vinod Koul (2021-11-30 23:27:18)
> 
> > > It feels like we should make a parent node that holds the core, chnls,
> > > obsrvr reg properties with a new compatible string and then have two
> > > child nodes for each bus. Then the various PMICs can hang off those two
> > > different bus nodes. Of course, it needs DT review to make sure nothing
> > > else goes wrong.
> > 
> > We considered this alternative DT layout when implementing PMIC arbiter
> > v7 support downstream.  The benefit is allowing common register ranges
> > to be specified once instead of in both SPMI bus nodes.  However, this
> > approach has several downsides.
> > 
> > It results in substantially more complex device tree binding
> > documentation that requires a different layout for SPMI buses for PMIC
> > arbiter v7 (and above) vs early versions even though support can be
> > provided with only a minimal modification (i.e. "qcom,bus-id").
> > Complexity is also increased inside of the spmi-pmic-arb driver.  There,
> > special data structures and logic would be needed to handle the shared
> > vs independent register addresses and data.
> > 
> > The SPMI framework currently uses a one-to-one mapping between SPMI
> > buses and struct device pointers.  This would not work if the new
> > top-level node represents the true struct device and the per-bus
> > subnodes are not true devices.  Also, platform_get_resource_byname()
> > (used in the spmi-pmic-arb probe function) needs a struct
> > platform_device pointer to read address ranges using "reg" +
> > "reg-names".  That wouldn't work for the subnodes.
> > 
> > I suppose that "compatible" could be specified for the top-level node
> > and the per bus subnodes so that all three get probed as struct devices.
> >  However, that makes things even more complicated and convoluted in the
> > driver (and DT).
> > 
> > I would prefer to stay with the approach of the two bus instances being
> > specified independently in DT.

The driver is already pretty hard to read because it combines so many
generations of spmi arbiter hardware from qcom into one file. It would
probably be better to start over and simplify the new version of the
driver, possibly sharing code between the two files if possible, but
otherwise dropping lots of cruft along the way and simplifying review
burden.

> 
> Steve, David,
> 
> Is this the way we are recommending for this to be move forward with?
> 

Please send the yamlification or update to the yamlification of the
binding.