Re: [PATCHv6 1/2] drivers: spi: Add qspi flash controller

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 2621 bytes --]

Hi,

On Mon, Jul 29, 2013 at 03:42:03PM +0530, Sourav Poddar wrote:
> >>+	frame_length = (m->frame_length<<  3) / spi->bits_per_word;
> >there's another way to optimize this. If you assume bits_per_word to
> >always be power of two you can:
> >
> >	frame_length = (m->frame_length<<  3)>>  __ffs(spi->bits_per_word);
> >
> >but that would need a comment stating that you're assuming bits_per_word
> >to always be a power of two. Not sure if this is a correct assumption.
> >
> I dont think it will be a correct assumption, since our controller is
> flexible to
> handle anything from 1 to 128 as bits_per_word.

right, but can the framework handle non-power-of-two bits_per_word ?

> >>+	spin_unlock(&qspi->lock);
> >You should, before releasing the lock, mask your IRQs, so that you can
> >get rid of IRQF_ONESHOT. Unmask them before returning from the IRQ
> >thread.
> >
> Sorry, did not get  you here?
>  I am reading interrupt status register before and clearing them in
> the threaded irq.

you need to mask the IRQs, clear WC and FIRQ in IRQENABLE register...
set them back at the end of the thread handler.

> >>+static int ti_qspi_probe(struct platform_device *pdev)
> >>+{
> >>+	struct  ti_qspi *qspi;
> >>+	struct spi_master *master;
> >>+	struct resource         *r;
> >>+	struct device_node *np = pdev->dev.of_node;
> >>+	u32 max_freq;
> >>+	int ret = 0, num_cs, irq;
> >>+
> >>+	master = spi_alloc_master(&pdev->dev, sizeof(*qspi));
> >>+	if (!master)
> >>+		return -ENOMEM;
> >>+
> >>+	master->mode_bits = SPI_CPOL | SPI_CPHA;
> >>+
> >>+	master->num_chipselect = 1;
> >again with the num_chipselect = 1 ? IIRC this controller has 4
> >chipselects, just read 24.5.1 on your TRM.
> >
> this is unnecessary. I intended to configure chip selects through
> dt)as done below).
> Will remove.

which brings me to the point of:

Do you review your own code before sending it out ? Doesn't look like...

> >>+	irq = platform_get_irq(pdev, 0);
> >>+	if (irq<  0) {
> >>+		dev_err(&pdev->dev, "no irq resource?\n");
> >>+		return irq;
> >>+	}
> >>+
> >>+	spin_lock_init(&qspi->lock);
> >>+
> >>+	qspi->base = devm_ioremap_resource(&pdev->dev, r);
> >>+	if (IS_ERR(qspi->base)) {
> >>+		ret = PTR_ERR(qspi->base);
> >>+		goto free_master;
> >>+	}
> >>+
> >>+	ret = devm_request_threaded_irq(&pdev->dev, irq, ti_qspi_isr,
> >>+			ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT,
> >why do you need IRQF_NO_SUSPEND ?
> >
> I should get away with this.

why ? Do you need or do you *not* need it ? And in either case, why ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]