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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 9 Sep 2020 14:01:40 +0800
From:   Xu Yilun <yilun.xu@...el.com>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     broonie@...nel.org, linux-kernel@...r.kernel.org, trix@...hat.com,
        matthew.gerlach@...ux.intel.com, russell.h.weight@...el.com,
        lgoncalv@...hat.com, hao.wu@...el.com, yilun.xu@...el.com
Subject: Re: [PATCH v4 2/2] mfd: intel-m10-bmc: add Max10 BMC chip support
  for Intel FPGA PAC

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