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

[Xu Yilun]

> > > > + * 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?

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.

> 
> > For m10bmc_sys_read(), the offset of BMC system registers could be
> > configured by HW developers (The MAX 10 is an CPLD, it could be easily
> > reprogrammed). And the HW SPEC will not add the offset when describing
> > the addresses of system registers. So:
> > 1. It makes the definition of system registers in code align with HW SPEC.
> > 2. It makes developers easier to make changes when the offset is adjusted
> >    in HW (I've been told by HW guys, it is sometimes necessary to adjust
> >    the offset when changing RTL, required by Altera EDA tool - Quartus).
> 
> Make sure you justify this for the function(s) you keep.

Yes, I could add some comments.

Thanks,
Yilun