On Fri, Jul 29, 2016 at 06:13:07PM -0400, Kamal Dasu wrote:
> +static int bcm_qspi_transfer_one(struct spi_master *master,
> + if (qspi->next_udelay) {
> + udelay(qspi->next_udelay);
> + qspi->next_udelay = 0;
> + }
Why is the driver manually implementing delays in a transfer_one()
function? The core will add any required delays between transfers, this
will result in us delaying twice.
> + read_from_hw(qspi, slots);
> + if (qspi->cs_change) {
> + usleep_range(10, 20);
> + qspi->cs_change = 0;
> + }
Similarly here, the core will do chip select changes.
> + if (spi_transfer_is_last(master, trans))
> + hw_stop(qspi);
The only thing I can see that looks like it might make a difference here
is this hw_stop() operation but all that does is update an internal
state machine so there's no limitation on it needing to be done before
the above operations that I can see.
> + ret = clk_prepare_enable(qspi->clk);
> + if (ret) {
> + dev_err(dev, "failed to prepare clock\n");
> + goto err2;
Please use named labels rather than numbered, it makes life a lot easier
if anything gets changed.
> +EXPORT_SYMBOL_GPL(bcm_qspi_probe);
This needs some explanation or to be added along with the user.
> diff --git a/drivers/spi/spi-brcmstb-qspi.c b/drivers/spi/spi-brcmstb-qspi.c
> new file mode 100644
> index 0000000..c7df92e
> --- /dev/null
> +++ b/drivers/spi/spi-brcmstb-qspi.c
This should really be a separate patch, it's a separate driver and very
surprising to find it here.
Attachment:
signature.asc
Description: PGP signature
