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

[Mark Brown]

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:

> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Indirect Register Access.
> > > + *
> > > + * Copyright (C) 2020-2022 Intel Corporation, Inc.
> > > + */
> > > +#include <linux/debugfs.h>

> > Please make the entire comment a C++ one so things look more
> > intentional.

> Eh, all/most SPDX-License-Identifier lines are done like this one?!?

The SPDX header has to be a C++ comment, please make the rest of
this a C++ comment.

> > > +	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@xxxxxxxxxxxxx/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.

Attachment: signature.asc
Description: PGP signature