Re: [PATCH v2 2/4] ata_piix: unify code for programming PIO and MWDMA timings

[Sergei Shtylyov <sshtylyov@mvista.com>]

Hello.

On 10/14/2011 08:43 PM, Sergei Shtylyov wrote:

>>>> From: Bartlomiej Zolnierkiewicz<bzolnier@gmail.com>
>>>> Subject: [PATCH v2] ata_piix: unify code for programming PIO and MWDMA timings

>>>> Besides making things noticably simpler it results in ~2% decrease in
>>>> the driver LOC count and also ~2% decrease in the driver binary size
>>>> (as measured on x86-32).

>>>> Fix piix_set_piomode() documentation while at it.

>>>> Signed-off-by: Bartlomiej Zolnierkiewicz<bzolnier@gmail.com>

>>> I'm still inclined to NAK this patch.

>>>> ---
>>>> v2: remove use_mwdma variable and fix comment style

>>>> earlier references:
>>>> https://lkml.org/lkml/2011/2/8/99

>>>> drivers/ata/ata_piix.c | 111 ++++++++++++++++---------------------------------
>>>> 1 file changed, 38 insertions(+), 73 deletions(-)

>>>> Index: b/drivers/ata/ata_piix.c
>>>> ===================================================================
>>>> --- a/drivers/ata/ata_piix.c
>>>> +++ b/drivers/ata/ata_piix.c
[...]

>> We still have following code at the end of piix_set_timings():

>> /* Ensure the UDMA bit is off - it will be turned back on if
>> UDMA is selected */
>>
>> if (ap->udma_mask) {
>> pci_read_config_byte(dev, 0x48,&udma_enable);
>> udma_enable&= ~(1<< (2 * ap->port_no + adev->devno));
>> pci_write_config_byte(dev, 0x48, udma_enable);
>> }

>> so everything should be OK.

> Ah, I should have looked... Didn't expect it to be there, and it in fact
> shouldn't be there, but now I'm OK with your patch. :-)

    But I would have preferred the above code to be removed, of course.

WBR, Sergei