Re: [PATCH v5 2/8] spi: bcm-qspi: Add Broadcom MSPI driver

[Kamal Dasu <kdasu.kdev@xxxxxxxxx>]

On Tue, Aug 16, 2016 at 2:15 PM, Mark Brown <broonie@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html