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

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

Hello.

Bartlomiej Zolnierkiewicz 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. ;-)

> (one day this ioctl will go away together with
> the flag and issue will solve itself).
>   

  I certainly can live with this feature in the meantime... :-)
  BTW, looks like libata has already dropped HDIO_DRIVE_TASKFILE ioctl.

> Thanks,
> Bart
>   

MBR, Sergei