Re: [PATCH 64/86] pata_radisys: unify code for programming PIO and MWDMA timings

Sergei Shtylyov
2009-11-26T19:02:57Z

Bartlomiej Zolnierkiewicz wrote:

> It results in ~6% decrease in the driver LOC count and also ~1%
> decrease in the driver binary size (as measured on x86-32).

> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
> drivers/ata/pata_radisys.c | 66 +++++++++++++++++----------------------------
> 1 file changed, 25 insertions(+), 41 deletions(-)

> Index: b/drivers/ata/pata_radisys.c
> ===================================================================
> --- a/drivers/ata/pata_radisys.c
> +++ b/drivers/ata/pata_radisys.c
> @@ -26,20 +26,9 @@
> #define DRV_NAME "pata_radisys"
> #define DRV_VERSION "0.4.4"
>
> -/**
> - * radisys_set_piomode - Initialize host controller PATA PIO timings
> - * @ap: ATA port
> - * @adev: Device whose timings we are configuring
> - *
> - * Set PIO mode for device, in host controller PCI config space.
> - *
> - * LOCKING:
> - * None (inherited from caller).
> - */
> -
> -static void radisys_set_piomode (struct ata_port *ap, struct ata_device *adev)
> +static void radisys_set_timings(struct ata_port *ap, struct ata_device *adev,
> + u8 pio, bool use_mwdma)
> {
> - unsigned int pio = adev->pio_mode - XFER_PIO_0;
> struct pci_dev *dev = to_pci_dev(ap->host->dev);
> u16 idetm_data;
> int control = 0;
> @@ -52,7 +41,7 @@ static void radisys_set_piomode (struct
> */
>
> static const /* ISP RTC */
> - u8 timings[][2] = { { 0, 0 }, /* Check me */
> + u8 timings[][2] = { { 0, 0 },
> { 0, 0 },
> { 1, 1 },
> { 2, 2 },
> @@ -62,6 +51,10 @@ static void radisys_set_piomode (struct
> control |= 1; /* TIME1 enable */
> if (ata_pio_need_iordy(adev))
> control |= 2; /* IE IORDY */
> + /* If the drive MWDMA is faster than it can do PIO then
> + we must force PIO0 for PIO cycles. */

Multi-line comment style should preferrably be:

/*
* blah
* blah
*/

> + if (use_mwdma && adev->pio_mode < (XFER_PIO_0 + pio))
> + control = 1;

Hm, shouldn't that be 8?

> @@ -115,20 +112,8 @@ static void radisys_set_dmamode (struct
> XFER_PIO_0, XFER_PIO_3, XFER_PIO_4
> };
> int pio = needed_pio[mwdma] - XFER_PIO_0;
> - int control = 3; /* IORDY|TIME0 */
> -
> - /* If the drive MWDMA is faster than it can do PIO then
> - we must force PIO0 for PIO cycles. */
> -
> - if (adev->pio_mode < needed_pio[mwdma])
> - control = 1;

Perhaps there's a bug here?

MBR, Sergei