Re: [PATCH V7 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller.

[vikas <vikas.manocha@st.com>]

Hi,


On 08/18/2015 12:17 PM, Graham Moore wrote:
> Hi Vikas,
> 
> On 08/17/2015 09:34 PM, vikas wrote:
>  > Hi Marek,
>  >
> 
> [...]
> 
>  >> +
>>> +/* Operation timeout value */
>>> +#define CQSPI_TIMEOUT_MS                       500
>>> +#define CQSPI_READ_TIMEOUT_MS                  10
>>
>> please add some comment about the timeouts value selection.
>>
> 
> I wish I could comment, but I don't know the origin of these values. 
> The 500 ms value is probably just "a very long time".

In my opinion we should have some logical value based on some worst timing like read/write sector.
I let you decide on this point.

> 
> [...]
> 
>>> +
>>> +       cqspi->irq_mask = CQSPI_IRQ_MASK_RD;
>>> +       writel(cqspi->irq_mask, reg_base + CQSPI_REG_IRQMASK);
>>
>> here interrupt mask is being configured for every read, better would be to move it in init.
>>
> 
> [...]
> 
>>> +
>>> +       cqspi->irq_mask = CQSPI_IRQ_MASK_WR;
>>> +       writel(cqspi->irq_mask, reg_base + CQSPI_REG_IRQMASK);
>>
>> same like read, it should be moved to init.
>>
> 
> It uses different masks for read and write

Yeah i saw it but why not to OR these values & configure for once in init. 
After that in ISR, check for the interrupt source & take action accordingly. I think other drivers also use it this way.

Rgds,
Vikas

> 
> [...]
> 
> BR,
> Graham
> 
> .
>