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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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

On Thu, 17 Nov 2022, Mark Brown wrote:

> On Thu, Nov 17, 2022 at 02:05:11PM +0200, Ilpo Järvinen wrote:
> > Add support for indirect register access via a regmap interface.
> > 
> > Indirect register access is a generic way to access registers indirectly.
> > One use case is accessing registers on Intel FPGA IPs with e.g. PMCI or
> > HSSI.
> 
> I can't tell from this changelog what exactly you're trying to
> implement here...
>
> > +// 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?!?

> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > +#include <linux/seq_file.h>
> 
> I can't see what seq_file.h is used for, which is probably good
> TBH since the interfaces it offers don't look like things I'd
> expect a regmap bus to use.

Yeah, it seems it was not even the only useless header. I'll clean 
them up.

> > +static int indirect_bus_reg_read(void *context, unsigned int reg,
> > +				     unsigned int *val)
> > +{
> > +	struct indirect_ctx *ctx = context;
> > +	unsigned int cmd, ack, tmpval;
> > +	int ret;
> > +
> > +	cmd = readl(ctx->base + ctx->indirect_cfg->cmd_offset);
> > +	if (cmd != ctx->indirect_cfg->idle_cmd)
> > +		dev_warn(ctx->dev, "residual cmd 0x%x on read entry\n", cmd);
> > +
> > +	writel(reg, ctx->base + ctx->indirect_cfg->addr_offset);
> > +	writel(ctx->indirect_cfg->read_cmd, ctx->base + ctx->indirect_cfg->cmd_offset);
> > +
> > +	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.

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

-- 
 i.