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

[Kamal Dasu <kdasu.kdev@xxxxxxxxx>]

On Tue, Aug 16, 2016 at 2:31 PM, Mark Brown <broonie@xxxxxxxxxx> 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
--
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