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

From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: Mark Brown <broonie@kernel.org>
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, 18 Nov 2022 14:49:45 +0200 (EET)	[thread overview]
Message-ID: <a82565d-c282-7ace-e759-6514c29cca1c@linux.intel.com> (raw)
In-Reply-To: <Y3ZTSSLUqHil4dfl@sirena.org.uk>

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

On Thu, 17 Nov 2022, Mark Brown wrote:

> On Thu, Nov 17, 2022 at 04:35:23PM +0200, Ilpo Järvinen wrote:
> > On Thu, 17 Nov 2022, Mark Brown wrote:
> > > On Thu, Nov 17, 2022 at 02:05:11PM +0200, Ilpo Järvinen wrote:

> > > > +	ret = readl_poll_timeout(ctx->base + ctx->indirect_cfg->ack_offset, ack,
> > > > +				 (ack & ctx->indirect_cfg->ack_mask) == ctx->indirect_cfg->ack_mask,
> > > > +				 ctx->indirect_cfg->sleep_us, ctx->indirect_cfg->timeout_us);
> 
> > > This all looks very specific to one particular implementation,
> > > requiring a particular set of memory mapped registers and
> > > operations - things like the initial read of the command for
> > > example. It's not clear to me how much reuse this is likely to
> > > see outside of the one driver you're trying to add - if you want
> > > to implement something device specific you can just provide
> > > the custom operations in the device's regmap configuration rather
> > > than having to provide a bus.  Why add a bus?
> 
> > The point of not doing it in a particular driver is that the users will 
> > be spread around more than into a single driver. This is a generic 
> > mechanism for accessing registers of IPs on Intel FPGA. The point being 
> > that IPs can use this common mechanism rather than each coming up their 
> > own way.
> 
> You're saying that this is generic but it's really not looking
> very generic at all, like I say there's a bunch of assumptions in
> the code that look entirely specific to your implementation here.
> Any abstraction and reusability seems extremely unclear, I'm not
> seeing what this is doing that is diffrent to the driver using
> this providing it's own register read and write operations.
>
> > Mark Brown objected earlier naming it something related to Intel FPGAs [1]
> > but I certainly know it still fixes the operations like you note even if 
> > the offsets and values are now "customizable" (they weren't in the 
> > earliest versions of this patch).
> 
> > [1] https://lore.kernel.org/all/YqB9O8HhZV2tXo8g@sirena.org.uk/T/#m75d4abdfd00f05866d837246ddc357a8af53cf99
> 
> No, what I'm objecting to there is pretty much the same thing I'm
> saying here - this doesn't seem like it's a particularly generic
> implementation and I'm really not clear that there'd be anything
> meaningful left by the time the implementation assumptions are
> removed.

That's probably because it sounds to me you're trying to extend its 
genericness beyond the domain where it's generic. That is, you're looking 
for genericness outside of IPs (that have their own driver each) in Intel 
FPGA domain.

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?

Please also keep in mind that we're talking about an FPGA device here, a 
device that is capable of implementing other devices that fall under 
various drivers/xx/. Obviously each would have a driver of their own so
there is no as strong only single device/driver mapping here as you might 
be thinking.

-- 
 i.