Re: EP93xx PIO IDE driver proposal

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

On Friday 08 May 2009 23:01:13 Sergei Shtylyov wrote:
> 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. :-)

I'm not sure what you're getting at with "such advices"... :)

We need to cast somewhere anyway so we may as well cast from 'void *' to
'unsigned int' where needed and not the other way around (or rather from
'unsigned int' to 'struct ide_timing *') -- which would be an ugly hack
and could cause maintainability/portability problems later.

BTW it seems like a good occasion to add ide_{get,set}_drivedata() helpers
(ala existing ide_{get,set}_hwifdata() ones) to <linux/ide.h> and thus make
internal IDE API more coherent.

[ Yes, I know that we may get away with s/unsigned int/unsigned long/
  and casting it to 'struct ide_timing *' for now but it always better
  to at least consider more elegant solution first... ]

Thanks,
Bart