Re: [PATCH 2/3] ide: add at91_ide driver

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

On Sunday 08 February 2009, Sergei Shtylyov wrote:
> Hello, I wrote:
> 
> >>>>>>> +void at91_ide_tf_load(ide_drive_t *drive, ide_task_t *task)
> >>>>>>> +{
> >>>>>>> +    ide_hwif_t *hwif = drive->hwif;
> >>>>>>> +    struct ide_io_ports *io_ports = &hwif->io_ports;
> >>>>>>> +    struct ide_taskfile *tf = &task->tf;
> >>>>>>> +    u8 HIHI = (task->tf_flags & IDE_TFLAG_LBA48) ? 0xE0 : 0xEF;
> >>>>>>> +
> >>>>>>> +    if (task->tf_flags & IDE_TFLAG_FLAGGED)
> >>>>>>> +        HIHI = 0xFF;
> >>>>>>> +
> >>>>>>> +    if (task->tf_flags & IDE_TFLAG_OUT_DATA) {
> >>>>>>>                   
> >>>>>>    Sigh. Bart, couldn't we drop that stupid flag? I bet nobody 
> >>>>>> ever used it.
> >>>>>>             
> >>>>> It is there for HDIO_DRIVE_TASKFILE ioctl and I prefer not to 
> >>>>> break it.
> >>>>>
> >>>>> Just add ->{read,write}_data methods for IDE_TFLAG_{IN,OUT}_DATA 
> >>>>> to struct
> >>>>> ide_tp_ops -- it should also help some other host drivers like 
> >>>>> tx493*.
> >>>>>         
> >>>>   That would be extremely senseless activity sicne I believe this 
> >>>> flag is totally useless. I have better thing to do. :-)
> >>>>     
> >>>
> >>> I would love to remove this flag but it is used by user-space exposed
> >>> interface 
> >>
> >>   I know that...
> >>
> >>> (which was used by few drive vendors for their diagnostics
> >>> tools -- doesn't matter whether internal or external) so you should put
> >>> some technical arguments behind its removal (you know many of low-level
> >>> technical details better than me so I may be missing something which is
> >>> obvious to you).
> >>>   
> >>
> >>   Well, the vendors can do strange things, of course...
> >>   However, accessing the data register is certainly not a part of any 
> >> ATA/PI defined command's inputs/outputs (the corresponding tables 
> >> just don't have this register). I suspect that this flag was added 
> >> just "for completeness".
> >>
> >>> OTOH while ->{read,write}_data approach would result in something like
> >>> ~50 extra LOC (or even less with be_tp_ops) compared to removal it is
> >>> completely safe and we don't need to spend a single second wondering
> >>> about potential breakage 
> >>
> >>   Go for it. ;-)
> >
> >   I think I have a better idea than creating another (useless) couple 
> > of "transport" methods. Why not (ab)use the exisitng 
> > {in|out}put_data() methods instead? :-)

Good idea.

>    Besides, those methods are clearly subject to some size 
> optimization... I'll cook up a patch while I'm at it. :-)

:)