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. ?
------------------------------------------------------------------------------
See everything from the browser to the database with AppDynamics
Get end-to-end visibility with application monitoring from AppDynamics
Isolate bottlenecks and diagnose root cause in seconds.
Start your free trial of AppDynamics Pro today!
http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk
_______________________________________________
spi-devel-general mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
