Date:   Tue, 11 Jan 2022 10:13:43 +0000
From:   Lee Jones <lee.jones@linaro.org>
To:     Colin Foster <colin.foster@in-advantage.com>
Cc:     broonie@kernel.org, linux-gpio@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        Linus Walleij <linus.walleij@linaro.org>,
        Russell King <linux@armlinux.org.uk>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Jakub Kicinski <kuba@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>, UNGLinuxDriver@microchip.com,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        Claudiu Manoil <claudiu.manoil@nxp.com>,
        Vladimir Oltean <vladimir.oltean@nxp.com>
Subject: Re: [RFC v5 net-next 01/13] mfd: ocelot: add support for external
 mfd control over SPI for the VSC7512
Message-ID: <Yd1YV+eUIaCnttYd@google.com>
References: <20211218214954.109755-1-colin.foster@in-advantage.com>
 <20211218214954.109755-2-colin.foster@in-advantage.com>
 <Ycx9MMc+2ZhgXzvb@google.com>
 <20211230014300.GA1347882@euler>
 <Ydwju35sN9QJqJ/P@google.com>
 <20220111003306.GA27854@COLIN-DESKTOP1.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220111003306.GA27854@COLIN-DESKTOP1.localdomain>
Precedence: bulk

> > > > No magic numbers please.
> > > 
> > > I've gotten conflicting feedback on this. Several of the ocelot drivers
> > > (drivers/net/dsa/ocelot/felix_vsc9959.c) have these ranges hard-coded.
> > > Others (Documentation/devicetree/bindings/net/mscc-ocelot.txt) have them
> > > all passed in through the device tree. 
> > > 
> > > https://lore.kernel.org/netdev/20211126213225.okrskqm26lgprxrk@skbuf/
> > 
> > Ref or quote?
> > 
> > I'm not brain grepping it searching for what you might be referring to.
> > 
> > I'm not sure what you're trying to say here.  I'm asking you to define
> > this numbers please.
> 
> I'll define the numbers as you suggest.
> 
> The quote I was referring to is this:
> 
> > The last option I haven't put much consideration toward would be to
> > move some of the decision making to the device tree. The main ocelot
> > driver appears to leave a lot of these addresses out. For instance
> > Documentation/devicetree/bindings/pinctrl/mscc,ocelot-pinctrl.txt.
> > That added DT complexity could remove needs for lines like this:
> > > > +              ocelot->map[GCB][GCB_MIIM_MII_STATUS & REG_MASK],
> > But that would probably impose DT changes on Seville and Felix, which
> > is the last thing I want to do.
> 
> The thing with putting the targets in the device tree is that you're
> inflicting yourself unnecessary pain. Take a look at
> Documentation/devicetree/bindings/net/mscc-ocelot.txt, and notice that
> they mark the "ptp" target as optional because it wasn't needed when
> they first published the device tree, and now they need to maintain
> compatibility with those old blobs.

I wasn't asking you to put it in DT, just to define the numbers.

> > > There's yet another complexity with these, and I'm not sure what the
> > > answer is. Currently all regmaps are tied to the ocelot_spi device...
> > > ocelot_spi calls devm_regmap_init. So those regmaps hang around if
> > > they're created by a module that has been removed. At least until the
> > > entire MFD module is removed. Maybe there's something I haven't seen yet
> > > where the devres or similar has a reference count. I don't know the best
> > > path forward on this one.
> > 
> > Why are you worrying about creating them 2 different ways?
> > 
> > If it's possible for them to all create and use their own regmaps,
> > what's preventing you from just do that all the time?
> 
> There isn't really any worry, there just might be efficiencies to be
> had if two children share the same regmap. But so long as any regmap is
> created with unique names, there's no reason multiple regmaps can't
> overlap the same regions. In those cases, maybe syscon would be the best
> thing to implement if it becomes needed.
> 
> I have nothing against making every child regmap be unique if that's the
> desire.

Unless something has changed or my understanding is not correct,
regmap does not support over-lapping register ranges.

However, even if that is required, I still think we can come up with
something cleaner than creating a whole API based around creating
and fetching different regmap configurations depending on how the
system was initialised.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog