Re: [PATCH v4 2/2] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC

[Xu Yilun <yilun.xu@intel.com>]

On Wed, Sep 09, 2020 at 08:31:40AM +0100, Lee Jones wrote:
> On Wed, 09 Sep 2020, Xu Yilun wrote:
> 
> > > > > > + * m10bmc_raw_read - read m10bmc register per addr
> > > > > > + * m10bmc_sys_read - read m10bmc system register per offset
> > > > > > + */
> > > > > > +static inline int
> > > > > > +m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
> > > > > > +		unsigned int *val)
> > > > > > +{
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	ret = regmap_read(m10bmc->regmap, addr, val);
> > > > > > +	if (ret)
> > > > > > +		dev_err(m10bmc->dev, "fail to read raw reg %x: %d\n",
> > > > > > +			addr, ret);
> > > > > > +
> > > > > > +	return ret;
> > > > > > +}
> > > > > > +
> > > > > > +#define m10bmc_sys_read(m10bmc, offset, val) \
> > > > > > +	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
> > > > > 
> > > > > No unnecessary abstractions.
> > > > > 
> > > > > Just use the Regmap API directly please.
> > > > 
> > > > Could we keep the 2 definition?
> > > > 
> > > > For m10bmc_raw_read(), we make it to help print some error info if
> > > > regmap RW fail. So we don't have to write "if (ret) dev_err" every time
> > > > we use regmap.
> > > 
> > > How many call sites are there?
> > 
> > There are about 20 calls of the register read in m10bmc base driver and
> > sub device drivers. Most of them calls m10bmc_sys_read().
> > I prefer to keep the function for unified error log, but I'm also good
> > to follow your opinion. How do you think?
> 
> Avoidable abstraction is one of my pet hates.  However,
> unified/centralised error handling is a valid(ish) reason for
> abstraction to exist.  Do you really need to know which read failed?
> Is there a case where a read from only a particular register would
> fail where others succeed?

I think it do helps we know which read failed in the first place when
communication error happens between FPGA & BMC.

Generally, if the error happens randomly on all registers, it may be the
problem of SPI bus.

But it is possible in some case error happens on some registers while
others are fine. The BMC has a internal Avalon mmio bus inside, and sub devices
connect on the bus. But the sub devices may response to the bus read/write
request differently according to hardware design. Once I run into a case
that the sub device stucks on one particular register read for long time
cause it prepares data so slowly. And the driver always timeout on that
register.

Thanks,
Yilun

> 
> > I also realize that it is not necessary that we define so many
> > m10bmc_raw_bulk_read/bulk_write/update_bits ... which are not frequently
> > used. We could change them.
> 
> Yes please.
>