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

Hi Geert,

On 25/08/26 19:53, Geert Uytterhoeven wrote:
>> +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;
>> +       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)) {
>> +               if (rw == READ)
>> +                       buf[buflen - 1] = raw_inw(data_addr) >> 8;
>> +               else
>> +                       raw_outw(buf[buflen - 1] << 8, data_addr);
>> +               words++;
>> +       }
>> +
>> +       return words << 1;
> This may be one less than the actual number of bytes
> Why not buflen?
>
words = buflen >> 1;

followed by

if (buflen & 0x01) words++;

makes 'words' the correct (i.e. rounded upwards if buflen was odd) 
number of words transferred.

The return value is then either correct, or one larger than the actual 
number of bytes?

I believe the template for these functions was 
drivers/ata/libata-sff.c:ata_sff_data_xfer() which follows the exact 
same logic.

Cheers,

MIchael

>> +}
>> +static int pata_cswarp_probe(struct zorro_dev *z,
>> +                            const struct zorro_device_id *ent)
>> +{
>> +       static const char board_name[] = "csWarp";
>> +       struct ata_host *host;
>> +       struct ata_port *ap;
>> +       void __iomem *base;
>> +       unsigned long board = z->resource.start;
>> +
>> +       dev_info(&z->dev, "%s IDE controller (board: 0x%lx)\n", board_name,
>> +                board);
>> +
>> +       if (!devm_request_mem_region(&z->dev, board + WARP_OFFSET_ATA, 0x1800,
>> +                                    DRV_NAME))
>> +               return -ENXIO;
>> +
>> +       host = ata_host_alloc(&z->dev, 1);
>> +       if (!host)
>> +               return -ENXIO;
>> +
>> +       ap = host->ports[0];
>> +       base = ioremap(board + WARP_OFFSET_ATA, 0x1800);
>> +
>> +       ap->ops = &pata_cswarp_ops;
>> +
>> +       ap->pio_mask = ATA_PIO4;
>> +       ap->flags |= ATA_FLAG_SLAVE_POSS | ATA_FLAG_NO_IORDY |
>> +               ATA_FLAG_PIO_POLLING;
>> +
>> +       ap->ioaddr.data_addr            = base;
>> +       ap->ioaddr.error_addr           = base + 1 * 4;
>> +       ap->ioaddr.feature_addr         = base + 1 * 4;
>> +       ap->ioaddr.nsect_addr           = base + 2 * 4;
>> +       ap->ioaddr.lbal_addr            = base + 3 * 4;
>> +       ap->ioaddr.lbam_addr            = base + 4 * 4;
>> +       ap->ioaddr.lbah_addr            = base + 5 * 4;
>> +       ap->ioaddr.device_addr          = base + 6 * 4;
>> +       ap->ioaddr.status_addr          = base + 7 * 4;
>> +       ap->ioaddr.command_addr         = base + 7 * 4;
>> +
>> +       ap->ioaddr.altstatus_addr       = base + (0x1000 | (6UL << 2));
>> +       ap->ioaddr.ctl_addr             = base + (0x1000 | (6UL << 2));
>> +
>> +       ata_port_desc(ap, "  cmd 0x%lx ctl 0x%lx", (unsigned long)base,
>> +                     (unsigned long)ap->ioaddr.ctl_addr);
> Both printed addresses are virtual addresses hence not really useful.
> If you want to print something, please print board or z->resource
> instead.
>
>> +static const struct zorro_device_id pata_cswarp_zorro_tbl[] = {
>> +       { ZORRO_PROD_CSLAB_WARP_1260, 0},
>> +       { 0 }
> Please use named initializers, and drop unneeded zeroes, like Uwe
> just did in all existing Zorro drivers:
>
>      { .id = ZORRO_PROD_CSLAB_WARP_1260 },
>      { }
>
>> +};
> Gr{oetje,eeting}s,
>
>                          Geert
>