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 > > . > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
