Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 7 Feb 2021 02:36:15 +0100
From:   Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <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
Message-ID: <YB9EDzI7mSrzXUUB@rocinante>
References: <cover.1612390291.git.gustavo.pimentel@synopsys.com>
 <bba090c3d9d3d90fb2dfe5f2aaa52c155d87958f.1612390291.git.gustavo.pimentel@synopsys.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <bba090c3d9d3d90fb2dfe5f2aaa52c155d87958f.1612390291.git.gustavo.pimentel@synopsys.com>
Precedence: bulk

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