Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Tue, 7 Jun 2022 17:27:38 -0700 (PDT)
From:   matthew.gerlach@linux.intel.com
To:     Mark Brown <broonie@kernel.org>
cc:     Tianfei Zhang <tianfei.zhang@intel.com>,
        gregkh@linuxfoundation.org, rafael@kernel.org,
        linux-kernel@vger.kernel.org, hao.wu@intel.com, trix@redhat.com,
        yilun.xu@intel.com, russell.h.weight@intel.com
Subject: Re: [PATCH v1] regmap: add generic indirect regmap support
In-Reply-To: <Yp9PdZn2Xu/oqiA8@sirena.org.uk>
Message-ID: <alpine.DEB.2.22.394.2206071714420.3001206@rhweight-WRK1>
References: <20220607013755.594554-1-tianfei.zhang@intel.com> <Yp9PdZn2Xu/oqiA8@sirena.org.uk>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII; format=flowed
Precedence: bulk


On Tue, 7 Jun 2022, Mark Brown wrote:

> On Mon, Jun 06, 2022 at 09:37:55PM -0400, Tianfei Zhang wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> This patch adds support for regmap APIs that are intended to be used by
>> the drivers of some devices which support generic indirect register access,
>> for example PMCI (Platform Management Control Interface) device, HSSI
>> (High Speed Serial Interface) device in FPGA.
>
> What is "generic indirect register access"?  I'm not clear what this is
> intended to support...

"indirect register access" is a RTL design pattern we use in FPGAs 
frequently.  The design pattern involves a small number of registers plus 
a little handshake code to access various register spaces inside the FPGA 
fabric.  The design pattern is "generic" in the sense that the same small 
number of registers and handshake can be used with many different IP 
components in the FPGA.  Historically, the bit definitions and handshaking 
was slightly different for each IP component.  This is an attempt at a 
consistent usage across IP components.

Would a different name help?


>
>> +static int indirect_bus_clr_cmd(struct indirect_ctx *ctx)
>> +{
>> +	unsigned int cmd;
>> +	int ret;
>> +
>> +	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?
>