Re: [PATCH v2 07/11] regmap: indirect: Add indirect regmap support

From: Mark Brown <broonie@kernel.org>
To: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: linux-fpga@vger.kernel.org, Xu Yilun <yilun.xu@intel.com>,
	Wu Hao <hao.wu@intel.com>, Tom Rix <trix@redhat.com>,
	Moritz Fischer <mdf@kernel.org>, Lee Jones <lee@kernel.org>,
	Matthew Gerlach <matthew.gerlach@linux.intel.com>,
	Russ Weight <russell.h.weight@intel.com>,
	Tianfei zhang <tianfei.zhang@intel.com>,
	Greg KH <gregkh@linuxfoundation.org>,
	Marco Pagani <marpagan@redhat.com>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 07/11] regmap: indirect: Add indirect regmap support
Date: Fri, 25 Nov 2022 18:53:37 +0000	[thread overview]
Message-ID: <Y4EPMbc+KCuDpuxJ@sirena.org.uk> (raw)
In-Reply-To: <a97ce076-4ca6-5f1b-eba4-4068dcb64b3d@linux.intel.com>

[-- Attachment #1: Type: text/plain, Size: 3593 bytes --]

On Mon, Nov 21, 2022 at 03:37:40PM +0200, Ilpo Järvinen wrote:

> Previously you were against saying it clearly that it's Intel FPGA 
> specific when Matthew proposed changing the name to not sound something 
> too generic. If you're ok with that now, I'm happy to make such change.

Saying it's for some Intel FPGA just makes it sound like it should only
live within the driver for that FPGA.  The issue with there being only
one user:

> > Perhaps you have some name for this
> > interface?  You're only adding one user here which isn't helping
> > make the case that this is something generic.

is part of it, as is the fact that the naming is so very generic.  Even
"Intel FPGA" seems to be heading to the generic side, this is presumably
some specific thing rather than just something that everyone using a
FPGA from Intel is going to need.  The issue with this being overly
specific isn't just the name, it's the code as well.

> > I can't tell what you're trying to say here.  Are you saying that
> > this is somehow baked into some FPGA design so that it's memory
> > mapped with only a few registers showing to the rest of the
> > system rather than just having a substantial memory mapped
> > window like is typically used for FPGAs, but someohow this
> > register window stuff is implemented in the soft IP so people are
> > just throwng vaugely similar interfaces into a random host mapped
> > register layout?
> 
> What I tried to say the users are not expected to be nicely confined into 
> drivers/mfd/ (and a single driver in there).

So this interface is part of the physical IP surrounding the actual
programmable bit of a FPGA or something?  That doesn't seem entirely
right though given the fact that the registers are apparently one of the
things that gets moved around a lot.  I still have no idea what this
hardware actually looks like or what this code is trying to represent,
especially given the very few things that you are trying to
parameterise.  It's really not obvious there's even any point in trying
to share this code at the abstraction level you've gone for.

Do you have any examples of even a second user that isn't this one MFD
which you can share?

> You didn't answer at all my question about where to place the code?
> I'm repeating it with the context below since you cut it off:

I keep telling you to either make this so that it's actually generic or
just have register get/set operations in the regmap for the device using
it.  As things stand with the code you've sent there's a bunch of things
like the way it's doing direct MMIO (which means it only works on top of
memory mapped devices) and the absolute requirement for an idle command
and a wait for completion which clearly look like this is device specific.

> Whether that is "generic" enough to reside in drivers/base/regmap can
> of course be debated but lets say I put it into drivers/mfd/ along with 
> the code currently using it. By doing that, we'll postpone this discussion 
> to the point when the first driver using it outside of drivers/mfd/ comes 
> by. At that point, having the indirect code in drivers/mfd/ is shown to 
> be a wrong choice.

> It's of course nothing that couldn't be fixed by patches moving the code 
> around to some more preferred location. And that location likely turns out 
> to be drivers/base/regmap, no? Or do you have a better place for it in 
> that case?

If you think this should be shared via regmap then make it shareable.
That needs more work than just repainting the name.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]