Re: [PATCH v3 7/9] mfd: intel-m10-bmc: Add PMCI driver

[Xu Yilun]

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.