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

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

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? :-)

MBR, Sergei