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
