Re: [PATCH v1] regmap: add generic indirect regmap support

[matthew . gerlach]

On Thu, 9 Jun 2022, Mark Brown wrote:

> On Wed, Jun 08, 2022 at 11:54:26PM +0000, Zhang, Tianfei wrote:
>
> > > > Would a different name help?
> > >
> > > This wouldn't address the major problem which is...
> > >
> > > > > > +	writel(0, ctx->base + INDIRECT_CMD_OFF);
> > > > > > +	ret = readl_poll_timeout((ctx->base + INDIRECT_CMD_OFF), cmd,
> > > > > > +				 (!cmd), INDIRECT_INT_US,
> > > INDIRECT_TIMEOUT_US);
> > > > > > +	if (ret)
> > > > > > +		dev_err(ctx->dev, "%s timed out on clearing cmd 0x%xn",
> > > > > > +__func__, cmd);
> > >
> > > > > ...and this doesn't look particularly generic, it looks like it's
> > > > > for some particular controller/bridge?
> > >
> > > ...that this appears to be entirely specific to some particular device, it's got
> > > things like hard coded register addresses and timeouts which mean it can't be
> > > reused.
> >
> > Yet, this is a register access hardware controller/bridge widely used in FPGA IP blocks, like PMCI, HSSI.
> > How about we change the patch title like this:
>
> > regmap: add indirect register controller support
>
> No, please enage with my feedback above.

Hi Mark,

I think part of the confusion is that this patch should have been included 
in a patch set that actually uses this regmap.  This patch really should 
be included with the following:

https://lore.kernel.org/all/20220607032833.3482-1-tianfei.zhang@xxxxxxxxx

The hard coded register definitions are offsets to the passed in void 
__iomem base address.  This set of registers provides the semantics of 
indirect register read/write to whatever the register set is connected 
to on the back end.  Conceptually this could be considered a specific type 
indirect register access controller, but currently we have very different 
backend implementations in RTL.  Part of our intent is to have consistent 
register interfaces for our FPGA IP so multiple drivers can reuse this 
regmap.

I totally agree the hardcoded timeout values used for polling should be 
parameterized.

We would like to submit a v2 patch set that combines this patch with the 
first consumer of the regmap, PMCI.  We would also parameterize the 
timeout values, but most importantly the name must be better.  It is a 
long name, but how about something like "Intel FPGA Indirect Register 
Interface"?

Matthew Gerlach