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

[Jan Glauber]

On Sun, Jul 24, 2016 at 10:04:52PM +0100, Mark Brown wrote:
> 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)?

I agree that it looks weird, the reasoning is that we would like
to avoid making the driver depend on something like ARCH_THUNDER.

So I made the driver depend on the things it actually uses
(PCI for probing and 64BIT because of readq/writeq) and don't care if it
compiles on other platforms too (like x86).

That said, I can remove the !CAVIUM_OCTEON_SOC, it compiles without
errors on MIPS too. Would that be ok?

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

OK, I'll merge it into the probe function.

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

I find it more readable instead of PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, 0xa00b),
or did I miss your point?

thanks,
Jan