Re: [PATCH 6/6] spi: octeon: Add thunderx driver

From: Mark Brown <broonie@kernel.org>
To: Jan Glauber <jglauber@cavium.com>
Cc: linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org,
	"Steven J. Hill" <steven.hill@cavium.com>,
	David Daney <david.daney@cavium.com>
Subject: Re: [PATCH 6/6] spi: octeon: Add thunderx driver
Date: Sun, 24 Jul 2016 22:04:52 +0100	[thread overview]
Message-ID: <20160724210452.GB6345@sirena.org.uk> (raw)
In-Reply-To: <f87d7a5ef8a713fb6e64c5d9471e7e5bf2051d18.1469174814.git.jglauber@cavium.com>

[-- Attachment #1: Type: text/plain, Size: 1524 bytes --]

On Sat, Jul 23, 2016 at 12:42:55PM +0200, Jan Glauber wrote:

> +config SPI_THUNDERX
> +	tristate "Cavium ThunderX SPI controller"
> +	depends on 64BIT && PCI && !CAVIUM_OCTEON_SOC

This is a *weird* and most likely broken set of dependencies - why
exclude this if we're on Octeon (or Octeon happens to have been enabled
in a config)?

> +static void thunderx_spi_clock_enable(struct device *dev, struct octeon_spi *p)
> +{
> +	int ret;
> +
> +	p->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(p->clk)) {
> +		p->clk = NULL;
> +		goto skip;
> +	}

This is really not clever - we should be requesting clocks on probe, not
only when we're trying to enable them, and using devm_ outside of probe
paths is usually a warning sign too.  Now, this is actually called from
probe so it works out fine but obviously it'd be better to improve the
power management to only enable the clock when needed and at that point
this function will be used and we'll fall into a bad pattern.

Given how tiny this function is and that we've not bothered splitting
out any of the other resource acquisition it's probably better to just
inline it into probe.

> +	dev_info(&pdev->dev, "Cavium SPI bus driver probed\n");

Again, this is just adding noise to the boot log.

> +#define PCI_DEVICE_ID_THUNDERX_SPI      0xa00b
> +
> +static const struct pci_device_id thunderx_spi_pci_id_table[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, PCI_DEVICE_ID_THUNDERX_SPI) },
> +	{ 0, }
> +};

The define for the device ID doesn't seem to be adding much here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]