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

[Sourav Poddar <sourav.poddar@ti.com>]

On Monday 29 July 2013 03:50 PM, Felipe Balbi wrote:
> 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 ?
>
The only limitation I see is the max bits_per_word, which is 32.
Though,  I did set "master->bits_per_word_mask" as power of 2
in my probe.
>>>> +	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.
>
Ok
>>>> +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 ?
>
I was thinking, this will keep the irqs up even when we are
hitting suspend, we will not be prepared to handle it. ?