[PATCH v5 3/8] spi: bcm-qspi: Add BSPI spi-nor flash controller driver

Kamal Dasu kdasu.kdev at gmail.com
Fri Aug 19 08:33:01 PDT 2016


On Tue, Aug 16, 2016 at 2:31 PM, Mark Brown <broonie at kernel.org> wrote:
> On Fri, Jul 29, 2016 at 06:13:08PM -0400, Kamal Dasu wrote:
>
>> +     bcm_qspi_bspi_lr_start(qspi);
>> +     /* Must flush previous writes before starting BSPI operation */
>> +     mb();
>> +     if (!wait_for_completion_timeout(&qspi->bspi_done, timeo)) {
>
> The comment suggests that the memory barrier is misplaced or that
> there's something missing?
>

Yes it got misplaced during refactoring the code. will move it to the
right place.

>> +             if (retry--)
>> +                     goto retry;
>
> Please write loops using normal C looping constructs rather than goto
> statements, it makes life a lot easier.  I'm not convinced that the loop
> is safe as is with the completion reinitializaiton and so on, we should
> at least be turning off the hardware before we reinitialize the
> completion to avoid races with slow hardware.
>

Actually will get rid of the retry logic.

>> +     io_width = msg->data_nbits ? msg->data_nbits : SPI_NBITS_SINGLE;
>
> Normal if statements please.
>

Ok will fix this.

>> +static irqreturn_t bcm_qspi_bspi_lr_err_l2_isr(int irq, void *dev_id)
>> +{
>> +     struct bcm_qspi_dev_id *qspi_dev_id = dev_id;
>> +     struct bcm_qspi *qspi = qspi_dev_id->dev;
>> +
>> +     dev_dbg(&qspi->pdev->dev, "BSPI INT error status %x\n",
>> +             qspi_dev_id->irqp->mask);
>
> I'd expect this to be dev_err() or something, but then the message
> claims this an error status and it doesn't seem to actually read that
> from the hardware.
>

Agreed will change this.

>> +#ifdef  QSPI_INT_DEBUG
>> +     /* this interrupt is for debug purposes only, dont request irq */
>> +     {
>> +             .irq_name = "spi_lr_overread",
>> +             .irq_handler = bcm_qspi_bspi_lr_err_l2_isr,
>> +             .mask = INTR_BSPI_LR_OVERREAD_MASK,
>> +     },
>> +#endif
>
> Why not?

Is there in the HW block but will never get triggered,  will never get
triggered in the current driver. But kept it for completeness.

Kamal



More information about the linux-mtd mailing list