Message-ID: <20200829182405.GA27132@yilunxu-OptiPlex-7050>
Date:   Sun, 30 Aug 2020 02:24:05 +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

Thanks for your comments, I'll fix them. Some comments inline.

On Fri, Aug 28, 2020 at 11:02:36AM +0100, Lee Jones wrote:
> On Wed, 19 Aug 2020, Xu Yilun wrote:
> 
> 
> > +enum m10bmc_type {
> > +	M10_N3000,
> > +};
> 
> Seems over-kill.  Will there be others?

There will be another version of the BMC which support the Intel PAC
D5005. The functionalities are similar with N3000. So I defined the
device type enum here.

> > +static ssize_t bmc_version_show(struct device *dev,
> > +				struct device_attribute *attr, char *buf)
> > +{
> 
> Does this line up to the '(' in code?

Yes, It does.

> 
> > +	struct intel_m10bmc *m10bmc = dev_get_drvdata(dev);
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	ret = m10bmc_sys_read(m10bmc, M10BMC_BUILD_VER, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return sprintf(buf, "0x%x\n", val);
> 
> Is this safe?  Have you considered snprintf()?

It formats a 32 bits value to string, so the strlen should be 8 chars at
most. So I think it should be safe here.

I see in Documentation/filesystems/sysfs.rst:
- show() must not use snprintf() when formatting the value to be
  returned to user space. If you can guarantee that an overflow
  will never happen you can use sprintf() otherwise you must use
  scnprintf().

And seeing from this mail https://lkml.org/lkml/2019/4/25/1050
Greg is discouraging use of scnprintf for sysfs attributes where it's not
needed.

> > +}
> > +static DEVICE_ATTR_RO(bmcfw_version);
> > +
> > +static struct attribute *m10bmc_attrs[] = {
> > +	&dev_attr_bmc_version.attr,
> > +	&dev_attr_bmcfw_version.attr,
> > +	NULL,
> > +};
> 
> Can this be const?

Seems we can not const this pointer or this array.

If we const the array, static const struct attribute *m10bmc_attrs[],
then:
	error: initialization from incompatible pointer type

If we const the pointer, static struct attribute * const m10bmc_attrs[],
then:
	warning: initialization discards ‘const’ qualifier from pointer
		 target type

> 
> > +static struct attribute_group m10bmc_attr_group = {
> > +	.attrs = m10bmc_attrs,
> > +};
> 
> Can this be const?

Yes, we can const it.

> 
> > +static const struct attribute_group *m10bmc_dev_groups[] = {
> > +	&m10bmc_attr_group,
> > +	NULL
> 
> > +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()

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


> > +	case M10_N3000:
> > +		cells = m10bmc_pacn3000_subdevs;
> > +		n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
> > +		break;
> > +	default:
> > +		return -ENODEV;
> > +	}
> 
> Will there be other versions?

There will be a M10_D5005, we haven't fully prepared the code yet, but I
mean to reserve the place for it.

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

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

Thanks,
Yilun

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

Powered by blists - more mailing lists