Re: EP93xx PIO IDE driver proposal

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

Hello.

Ryan Mallon wrote:

>> +static void
>> +ep93xx_ide_pio_set_irq(ide_hwif_t *hwif, int on)
>> +{
>> +    u8 ctl = ATA_DEVCTL_OBS;
>> +
>> +    if (on == 4) { /* hack for SRST */
>> +        ctl |= 4;
>> +        on &= ~4;
>> +    }
>> +
>> +    ctl |= on ? 0 : 2;
>>     
>
> This function all seems a bit magic/hackish. Can you replace the
> numbers here with #defines to make it clear what is being done.
> You may also want to expand on the comment here to explain why the
> 'hack' is needed.
>   

   It's standard implementation of this method, and looks the same in 
all the other IDE drivers that redefine it. It's going to be gone in 
2.6.30, so it's not worth wasting time...

>> +
>> +    ep93xx_ide_pio_writeb(hwif->config_data, ctl,
>> hwif->io_ports.ctl_addr);
>> +}
>> +
>> +static void
>> +ep93xx_ide_pio_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;
>>     
>
> Don't give local variables names with all capitals. What is hihi anyway?
>   

  He must've copied that from the other drivers, so that question is to 
the IDE maintainever rather. :-)


MBR, Sergei