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

[Lee Jones <lee.jones@linaro.org>]

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 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.

> > > 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

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog