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:
signature.asc
Description: PGP signature