linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Krzysztof Wilczyński" <kw@linux.com>
To: Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>
Cc: linux-doc@vger.kernel.org, linux-pci@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Derek Kiernan <derek.kiernan@xilinx.com>,
	Dragan Cvetic <dragan.cvetic@xilinx.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Andrew Morton <akpm@linux-foundation.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jonathan Corbet <corbet@lwn.net>
Subject: Re: [RESEND v4 1/6] misc: Add Synopsys DesignWare xData IP driver
Date: Sun, 7 Feb 2021 02:36:15 +0100	[thread overview]
Message-ID: <YB9EDzI7mSrzXUUB@rocinante> (raw)
In-Reply-To: <bba090c3d9d3d90fb2dfe5f2aaa52c155d87958f.1612390291.git.gustavo.pimentel@synopsys.com>

Hi Gustavo,

Thank you for all the work here!

A few suggestions.

[...]
> +static void dw_xdata_stop(struct dw_xdata *dw)
> +{
> +	u32 burst = readl(&(__dw_xdara_regs(dw)->burst_cnt));
> +
> +	if (burst & BIT(31)) {
> +		burst &= ~(u32)BIT(31);
> +		writel(burst, &(__dw_regs(dw)->burst_cnt));
> +	}
> +}

Would it be possible to add a define for this "BIT(31)", similarly to
the "XPERF_CONTROL_ENABLE", for example:

  #define XPERF_CONTROL_ENABLE		BIT(5)
  #define XPERF_CONTROL_DISABLE		BIT(31)

What do you think?

> +static void dw_xdata_start(struct dw_xdata *dw, bool write)
> +{
> +	u32 control, status;
> +
> +	/* Stop first if xfer in progress */
> +	dw_xdata_stop(dw);
> +
> +	/* Clear status register */
> +	writel(0x0, &(__dw_regs(dw)->status));
> +
> +	/* Burst count register set for continuous until stopped */
> +	writel(0x80001001, &(__dw_regs(dw)->burst_cnt));

Would you mind adding a define (possibly with a comment, if it makes
sense, of course) rather than open coding it here.

> +	/* Pattern register */
> +	writel(0x0, &(__dw_regs(dw)->pattern));
> +
> +	/* Control register */
> +	control = CONTROL_DOORBELL | CONTROL_PATTERN_INC | CONTROL_NO_ADDR_INC;
> +	if (write) {
> +		control |= CONTROL_IS_WRITE;
> +		control |= CONTROL_LENGTH(dw->max_wr_len);
> +	} else {
> +		control |= CONTROL_LENGTH(dw->max_rd_len);
> +	}
> +	writel(control, &(__dw_regs(dw)->control));
> +
> +	usleep_range(100, 150);
[...]

Why sleep here?

Do you just add some delay that changes were reflected, or is it
a requirement of sorts?  What do you think about documenting why the
sleep where? Would it make sense?

[...]
> +static void dw_xdata_perf(struct dw_xdata *dw, u64 *rate, bool write)
> +{
> +	u64 data[2], time[2], diff;
> +
> +	/* First measurement */
> +	writel(0x0, &(__dw_regs(dw)->perf_control));
> +	dw_xdata_perf_meas(dw, &data[0], write);
> +	time[0] = jiffies;
> +	writel((u32)XPERF_CONTROL_ENABLE, &(__dw_regs(dw)->perf_control));
> +
> +	/* Delay 100ms */
> +	mdelay(100);
[...]

The mdelay() is self-explanatory, so a comment that explains why to take
two measurements that are 100 ms apart and how rate is calculated might
be more useful (unless it would be an overkill here).

If this is an arbitrary delay without any special meaning, then probably
no comment is needed here.

What do you think?

[...]
> +	/* Calculations */

What sort of calculations precisely? :)

[...]
> +static int dw_xdata_pcie_probe(struct pci_dev *pdev,
> +			       const struct pci_device_id *pid)
> +{
> +	const struct dw_xdata_pcie_data *pdata = (void *)pid->driver_data;
> +	struct dw_xdata *dw;
> +	u64 addr;
> +	int err;
> +
> +	/* Enable PCI device */
> +	err = pcim_enable_device(pdev);

This comment might not be needed.

[...]
> +	/* Mapping PCI BAR regions */
> +	err = pcim_iomap_regions(pdev, BIT(pdata->rg_bar), pci_name(pdev));

This comment might also not be needed.

[...]
> +	/* Allocate memory */

And so this comment.

[...]
> +	/* Data structure initialization */
[...]
> +	/* Saving data structure reference */
[...]
> +	/* Sysfs */
[...]

And possibly few of these are also not needed.

Krzysztof

  parent reply	other threads:[~2021-02-07  1:37 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-03 22:12 [RESEND v4 0/6] misc: Add Add Synopsys DesignWare xData IP driver Gustavo Pimentel
2021-02-03 22:12 ` [RESEND v4 1/6] misc: " Gustavo Pimentel
2021-02-03 22:33   ` Greg Kroah-Hartman
2021-02-07  1:36   ` Krzysztof Wilczyński [this message]
2021-02-08 11:21     ` Gustavo Pimentel
2021-02-08 22:53       ` Krzysztof Wilczyński
2021-02-09 15:28         ` Gustavo Pimentel
2021-02-09 17:24           ` Krzysztof Wilczyński
2021-02-09 18:52           ` Bjorn Helgaas
2021-02-03 22:12 ` [RESEND v4 2/6] misc: Add Synopsys DesignWare xData IP driver to Makefile Gustavo Pimentel
2021-02-03 22:12 ` [RESEND v4 3/6] misc: Add Synopsys DesignWare xData IP driver to Kconfig Gustavo Pimentel
2021-02-03 22:26   ` Greg Kroah-Hartman
2021-02-03 22:27   ` Greg Kroah-Hartman
2021-02-03 22:12 ` [RESEND v4 4/6] Documentation: misc-devices: Add Documentation for dw-xdata-pcie driver Gustavo Pimentel
2021-02-06 23:31   ` Krzysztof Wilczyński
2021-02-08  9:24     ` Gustavo Pimentel
2021-02-08 23:11       ` Krzysztof Wilczyński
2021-02-03 22:12 ` [RESEND v4 5/6] MAINTAINERS: Add Synopsys xData IP driver maintainer Gustavo Pimentel
2021-02-03 22:12 ` [RESEND v4 6/6] docs: ABI: Add sysfs documentation interface of dw-xdata-pcie driver Gustavo Pimentel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YB9EDzI7mSrzXUUB@rocinante \
    --to=kw@linux.com \
    --cc=Gustavo.Pimentel@synopsys.com \
    --cc=akpm@linux-foundation.org \
    --cc=arnd@arndb.de \
    --cc=corbet@lwn.net \
    --cc=derek.kiernan@xilinx.com \
    --cc=dragan.cvetic@xilinx.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).