Re: [PATCH v4 1/2] ata: pata_cswarp: Add Amiga cslab ata support

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Paolo,

On Tue, 25 Mar 2025 at 16:56, Paolo Pisati <p.pisati@gmail.com> wrote:
> Driver for the on-board IDE interface on CS-Lab Warp Expansion Card
> (NOTE that idemode=native has to be set in Warp Configuration)
>
> Signed-off-by: Paolo Pisati <p.pisati@gmail.com>

Thanks for your patch!

> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -1025,6 +1025,17 @@ config PATA_GAYLE
>
>           If unsure, say N.
>
> +config PATA_CSWARP
> +       tristate "Amiga CS Warp PATA support"
> +       depends on M68K && AMIGA && ZORRO
> +       help
> +         This option enables support for the on-board IDE interface on
> +         CS-Lab Warp Expansion Card (NOTE that idemode=native has to be
> +         set in Warp Configuration)
> +
> +         If unsure, say N.
> +
> +

Please drop one blank line.

>  config PATA_BUDDHA
>         tristate "Buddha/Catweasel/X-Surf PATA support"
>         depends on ZORRO

> index 000000000000..e0a0db6f7cfd
> --- /dev/null
> +++ b/drivers/ata/pata_cswarp.c

> +static unsigned int pata_cswarp_data_xfer(struct ata_queued_cmd *qc,
> +                                        unsigned char *buf,
> +                                        unsigned int buflen, int rw)
> +{
> +       struct ata_device *dev = qc->dev;
> +       struct ata_port *ap = dev->link->ap;
> +       volatile void __iomem *data_addr = ap->ioaddr.data_addr;
> +       unsigned int words = buflen >> 1;
> +       u16 *buf16 = (u16 *)buf;
> +
> +       /* Transfer multiple of 2 bytes */
> +       if (rw == READ)
> +               raw_insw(data_addr, buf16, words);
> +       else
> +               raw_outsw(data_addr, buf16, words);
> +
> +       /* Transfer trailing byte, if any. */
> +       if (unlikely(buflen & 0x01)) {
> +               unsigned char pad[2] = { };
> +
> +               /* Point buf to the tail of buffer */
> +               buf16 += words;
> +
> +               if (rw == READ) {
> +                       *pad = raw_inw(data_addr);
> +                       *buf16 = pad[0];

This looks wrong to me: as *pad is the same as pad[0], you could just
eliminate the pad[] intermediate...
Moreover, this writes one byte too much into the buffer.

> +               } else {
> +                       pad[0] = *buf16;
> +                       raw_outw(*pad, data_addr);
> +               }

Comparing to what v3 did, I think you want:

    if (rw == READ)
            buf[buflen-1] = raw_inw(data_addr) >> 8;
    else
            raw_outw(buf[buflen-1] << 8, data_addr);

As all Amigas are big-endian, I think there is no need to implement
this using some endian conversion function.

> +       }
> +
> +       return buflen;
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds