Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 5 Dec 2022 23:08:24 +0800
From:   Xu Yilun <yilun.xu@intel.com>
To:     Mark Brown <broonie@kernel.org>
Cc:     Ilpo =?iso-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        linux-fpga@vger.kernel.org, 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>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 7/9] mfd: intel-m10-bmc: Add PMCI driver
Message-ID: <Y44JaLtAnTll4gU0@yilunxu-OptiPlex-7050>
References: <20221202100841.4741-1-ilpo.jarvinen@linux.intel.com>
 <20221202100841.4741-8-ilpo.jarvinen@linux.intel.com>
 <Y4ox5J0junaUYyT7@yilunxu-OptiPlex-7050>
 <855d463e-fb84-1910-f53-58e6b0a633a4@linux.intel.com>
 <Y43eejWSYIBIlUKB@sirena.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <Y43eejWSYIBIlUKB@sirena.org.uk>
Precedence: bulk

On 2022-12-05 at 12:05:14 +0000, Mark Brown wrote:
> On Mon, Dec 05, 2022 at 11:51:15AM +0200, Ilpo J?rvinen wrote:
> > On Sat, 3 Dec 2022, Xu Yilun wrote:
> > > On 2022-12-02 at 12:08:39 +0200, Ilpo J?rvinen wrote:
> 
> > > > +struct regmap *__devm_m10_regmap_indirect(struct device *dev,
> 
> > > We name the file intel-m10-bmc-pmci-xxx.c, and this function
> > > xx_m10_regmap_xx(). But I can see the implementation is just about the indirect
> > > bus which from your commit message could be used by various DFL features
> > > like HSSI or PMCI. So is it better we put the implementation in
> > > drivers/fpga and name the file dfl-indirect-regmap.c and the
> > > initialization function dfl_indirect_regmap_init()?
> 
> > I guess that would be doable unless Mark objects. My understanding was 
> > that he preferred to have in the driver that is currently using it.
> 
> > Mark, any opinion on this?
> 
> The above does not look good.  As I have said several times now drivers
> implementing their own regmap operations should use the reg_read() and
> reg_write() operations in regmap_config when allocating their regmap
> unless they're doing something unusual.  There are a few cases where it
> makes sense but nothing I've seen here makes it look like this is one of
> them.  Most of the current users don't fit.

It is good for now to implement the indirect access interface in
regmap_config, as intel-m10-bmc is the only one who uses it. But I'm not
sure when a second IP block(like HSSI) in intel FPGA uses it, how to
implement? A shared library?

Some background about hardware:
Several IP blocks in intel FPGA integrate the same mmio register layout
(so called indirect access interface here) as the bridge to the IP's real
registers address space. Like:

 +---------+          +---------+
 | m10 BMC |          |  HSSI   |
 +---------+          +---------+
 |indirect |          |indirect |
 | access  |          | access  |
 |  MMIOs  |          |  MMIOs  |
 +----+----+          +----+----+
      |                    |
      |                    |
 +----+-----+         +---------+
 |m10 bmc   |         | HSSI    |
 |registers |         |registers|
 +----------+         +---------+

Thanks,
Yilun

> 
> Please, just implement a normal driver using a normal regmap_config as
> I've repeatedly said you should if you don't want to provide something
> generic.