Re: EP93xx PIO IDE driver proposal

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

Hello.

joao.ramos@inov.pt wrote:

>>> Is this correct? Sorry, has I stated earlier, I'm wasn't familiar with
>>> the IDE susbsystem untill I wrote this patch; but I'm willing to
>>> contribute in any way I can, so please, bear with me on this :-) .
>>
>> Sure, nobody starts from the expert level and not all maintainers are
>> into "prove the maintainer wrong" elitist's idiocy. ;)
>>
>>> >
>>> >>>
>>> >>>
>>> >>>> There's just only one issue; normally, I would setup the specific
>>> >>>> timings (t0, t1, t2, t2i, etc) in the 'pio_set_mode' hook. 
>>> However, if
>>> >>>> you look further in the driver, those timings aren't defined 
>>> through a
>>> >>>> memory controller but instead manually enforced by 'ndelay' 
>>> calls (arghhh).
>>> >>>> This means that in my low-level procedures for reading and 
>>> writing, I
>>> >>>> need to have access to the timings (or the struct ide_timing)
>>> >>>> corresponding to the PIO mode selected, in order to use the 
>>> correct delays.
>>> >>>>
>>> >>>> My question is: which is the best way to accomplish this? 
>>> Declaring a
>>> >>>> global struct ide_timing variable pointer that always holds the 
>>> correct
>>> >>>> ide_timing struct to the selected PIO mode? Or should I always 
>>> check (in
>>> >>>> some manner) what is the current PIO mode and then select the 
>>> adequate
>>> >>>> delays?
>>> >>>>
>>> >>>>
>>> >>> I think that the setting variable pointer in ->set_pio_mode 
>>> method would
>>> >>> work best.  Seems like the existing drive_data field of 
>>> ide_drive_t is well
>>> >>> suited for this purpose (however it may be worth to convert it 
>>> to 'void *'
>>> >>> type while we are it).
>>> >>>
>>> >>>
>>> >> Did you mean 'drive_data' field, or 'driver_data' field?
>>> >> 'drive_data' field is an unsigned int value; I guess you meant
>>> >> 'driver_data' field as it is a (void *) field, so I can define it 
>>> as a
>>> >> pointer to the correct 'struct ide_timing'.
>>> >>
>>> >
>>> > That is why I hinted that you may need to convert 'drive_data' to
>>> > 'void *' type first.  You may also try to use 'driver_data' instead
>>> > but you will discover rather quickly that you shouldn't do this... ;)
>>> >
>>> > 'driver_data' is for use by IDE core and IDE device drivers.
>>> >
>>> > 'drive_data' is for use by IDE host drivers.
>>> >
>>>
>>> And this conversion is made by my driver code, or should I fix directly
>>> in the ide_drive_t structure?
>>
>> The latter -- ide_drive_t is the place needing fixing.
>
>
> Same: I will fix that and propose a patch.

   Hey, don't do this please! This is actually a bad idea as some 
drivers (e.g. sl82c105) use drive_data as an integer entity. Bart, stop 
giving such advices please. :-)

> Regards,
> João Ramos
>
>> Thanks,
>> Bart

WBR, Sergei