On Thu, Dec 01, 2016 at 11:27:15AM +0100, Romain Perier wrote:
> +config SPI_ARMADA_3700
> + tristate "Marvell Armada 3700 SPI Controller"
> + depends on ARCH_MVEBU && OF
Why does this not have a COMPILE_TEST dependency?
> + /* Reset SPI unit */
> + val = spireg_read(a3700_spi, A3700_SPI_IF_CFG_REG);
> + val |= A3700_SPI_SRST;
> + spireg_write(a3700_spi, A3700_SPI_IF_CFG_REG, val);
> +
> + for (i = 0; i < A3700_SPI_TIMEOUT; i++)
> + udelay(1);
Why not just do a single udelay()?
> +static irqreturn_t a3700_spi_interrupt(int irq, void *dev_id)
> +{
> + struct spi_master *master = dev_id;
> + struct a3700_spi *a3700_spi;
> + u32 cause;
> +
> + a3700_spi = spi_master_get_devdata(master);
> +
> + /* Get interrupt causes */
> + cause = spireg_read(a3700_spi, A3700_SPI_INT_STAT_REG);
> +
> + /* mask and acknowledge the SPI interrupts */
> + spireg_write(a3700_spi, A3700_SPI_INT_MASK_REG, 0);
> + spireg_write(a3700_spi, A3700_SPI_INT_STAT_REG, cause);
> +
> + /* Wake up the transfer */
> + if (a3700_spi->wait_mask & cause)
> + complete(&a3700_spi->done);
> +
> + return IRQ_HANDLED;
> +}
This reports that we handled an interrupt even if we did not in fact
handle an interrupt. It's also a bit dodgy that it doesn't check what
the interrupt was but that's less serious.
> + master->bus_num = (pdev->id != -1) ? pdev->id : 0;
At most this should be just setting the bus number to pdev->id like
other drivers do.
> + ret = clk_prepare_enable(spi->clk);
> + if (ret) {
> + dev_err(dev, "could not prepare clk: %d\n", ret);
> + goto error;
> + }
I'd expect the hardware prepare/unprepare to be managing the enable and
disable of the clock in order to save a little power.
> + dev_info(dev, "Marvell Armada 3700 SPI Controller at 0x%08lx, irq %d\n",
> + (unsigned long)res->start, spi->irq);
This is just adding noise to the boot, remove it. It's not telling us
anything we read from the hardware or anything.
Attachment:
signature.asc
Description: PGP signature
