Re: [RFC][PATCH] at91_ide driver

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

Hello, I wrote:

>>>>> +#ifdef AT91_GPIO_IRQ_HACK
>>>>> +#define NR_TRIES 10
>>>>> +    int ntries = 0;
>>>>> +    int pin_val1, pin_val2;   
>>>>> +    do {
>>>>> +        pin_val1 = at91_get_gpio_value(AT91_PIN_PB20);
>>>>> +        pin_val2 = at91_get_gpio_value(AT91_PIN_PB20);
>>>>> +    } while (pin_val1 != pin_val2 && ntries++ < NR_TRIES);
>>>>>             
>>>> You really don't want to put special board specific code in generic
>>>> locations. In the libata case you don't need to and I think in the ide
>>>> case you can avoid it too by wrapping the IRQ handler.
>>>>       
>>>    Unfortunately, it seems you can't wrap ide_intr(), at least with 
>>> the current code.
>>>     
>>
>> Well... there shouldn't be much problem with:
>>
>> * adding ->irq_handler method to struct ide_port_info and struct 
>> ide_host
>>
>>   [ which reminds me that struct ide_port_info would be better named 
>> struct
>>     ide_host_info and IIRC somebody has already noticed it in the 
>> past ;-) ]
>>
>> * exporting ide_intr()
>>
>> * adding ide_interrupt() wrapper around ide_intr() which will do sth 
>> like:
>>
>>     if (host->irq_handler)
>>         return host->irq_handler()
>>     else
>>         return ide_intr()
>>
>>   and then passing &ide_interrupt instead of &ide_intr to request_irq()
>>
>> * implementing at91_irq_handler()
>>
>> * Et Voila!
>>
>> In the longer term it would also be useful for other purposes
>> (like adding ATA-like flash devices support to IDE).
>>   
>
>   Oh, you must be meaning that brain damaged Disk-On-Chip H3... but I 
> don't think it would need to wrap ide_intr() as it should have its own 
> "class driver" (like ide-disk).
>
>>>> Other comments:
>>>>     - The old and new ATA layers both have timing tables and timing
>>>> functions so you don't need all the duplicated timing table logic.
>>>>         
>>>    Stanislaw's patch is adding the DIOx- to address hold time (t9) 
>>> to the existing ones. While there's has been already a patch by 
>>> David Daney adding this timing to libata (however, the author have 
>>> ditched this idea finally), the table in ide-timings.c still misses 
>>> it, as well as the PIO mode 6 timings...
>>>     
>>
>> Indeed... should be easy and quick to fix though.
>>   
>
>   We need to add support for CFA's MWDMA modes 3 and 4 then as well...
>
>>>    Hm, besides the address setup and active/recovery times there 
>>> seem wrong for the PIO mode 5: they should be 15 and 65/25, not 20 
>>> and 50/30. Bart, are you reading this? :-)
>>>     
>>
>> Yeah.  Where's the patch? :-)
>>   
>
>   I was not feeling confident because the address setup and recovery 
> times defined by CF spec. are less than those we have (that were 
> spec'ed by Quantum I guess?). However, Wikipedia's article about PIO 
> tells me that no PIO5 capable hard disks were manufactured...

   I've already started the patch adding support of the CFA modes... I'm 
still not sure how/whether to keep the non-standard PIO5 mode support. 
Probalbly I'll use the maximum timings: 20/65/30 (although with typical 
30 ns cycle time of the PCI chips, there shouldn't be much difference).

>> Bart
> Thanks,

MBR, Sergei