[PATCH v5 2/8] spi: bcm-qspi: Add Broadcom MSPI driver
Kamal Dasu
kdasu.kdev at gmail.com
Fri Aug 19 08:20:05 PDT 2016
On Tue, Aug 16, 2016 at 2:15 PM, Mark Brown <broonie at kernel.org> wrote:
> 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.
I will cleanup above 3 areas of redundant code.
>> + 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.
>
Ok will make this change as well
>> +EXPORT_SYMBOL_GPL(bcm_qspi_probe);
>
> This needs some explanation or to be added along with the user.
>
Will add appropriate comments for 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.
Its very specific to the spi-block and hence is here, I will separate the patch,
Kamal
More information about the linux-mtd
mailing list