linux-kernel - Re: [PATCH 07/12] pcie: qcom: add tx term offset support

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20200320192245.GA242952@google.com>
Date:   Fri, 20 Mar 2020 14:22:45 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Ansuel Smith <ansuelsmth@...il.com>
Cc:     Stanimir Varbanov <svarbanov@...sol.com>,
        Sham Muthayyan <smuthayy@...eaurora.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Andrew Murray <amurray@...goodpenguin.co.uk>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        linux-arm-msm@...r.kernel.org, linux-pci@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 07/12] pcie: qcom: add tx term offset support

On Fri, Mar 20, 2020 at 07:34:49PM +0100, Ansuel Smith wrote:
> From: Sham Muthayyan <smuthayy@...eaurora.org>
> 
> Add tx term offset support to pcie qcom driver
> need in some revision of the ipq806x soc

The usual (s/pcie/PCIe/, s/soc/SoC/, wrap to fill 75 columns, add
period at end).  Apply to all patches.

> +#define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK	(0x1f << 16)
> +#define PHY_CTRL_PHY_TX0_TERM_OFFSET(x)		(x << 16)

Since the rest of the file uses BIT(), I think it would make sense to
use GENMASK() here.  And below, of course.

> +static inline void
> +writel_masked(void __iomem *addr, u32 clear_mask, u32 set_mask)

Follow coding style of rest of file, i.e.,

  static inline void writel_masked(...)

The name "writel_masked" suggests that we're writing "val & mask" to a
register, but that's not what's happening.  This is really a "clear
some bits and set others" operation.

The names are wordy, but we do have pci_clear_and_set_dword(),
pcie_capability_clear_and_set_word(), etc., functions that work
similarly.

> +{
> +	u32 val = readl(addr);
> +
> +	val &= ~clear_mask;
> +	val |= set_mask;
> +	writel(val, addr);
> +}

> +	/* Set Tx termination offset */

Capitalize consistently with other comments in the file.  Below also.
Hmm, I see the file is already a bit of a mess in that regard.  So
just do what you can.