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

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

> > > +static int check_m10bmc_version(struct intel_m10bmc *m10bmc)
> > > +{
> > > +	unsigned int v;
> > > +
> > > +	if (m10bmc_raw_read(m10bmc, M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER,
> > > +			    &v))
> > > +		return -ENODEV;
> > 
> > Please break functions out of if statements.
> > 
> > Does m10bmc_raw_read() return 0 on success?
> 
> Yes, this function just wrappered the regmap_read()

Avoid unnecessarily wrapping functions if possible.

> > Seems odd for a read function.
> > 
> > > +	if (v != 0xffffffff) {
> > > +		dev_err(m10bmc->dev, "bad version M10BMC detected\n");
> > > +		return -ENODEV;
> > > +	}
> > 
> > The only acceptable version is -1?
> 
> As mentioned by Tom, this is a check if the board is using a very old legacy
> bmc version, the driver doesn't mean to support this old legacy bmc
> version.

Please add a descriptive comment and define the value.

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

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

I'll take a closer look on the next submission.

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