Hi Michael,

On Mon, Mar 12, 2018 at 8:26 AM, Michael Schmitz <schmitz...@gmail.com> wrote:
> From: Michael Schmitz <schm...@debian.org>
>
> New combined SCSI driver for all ESP based Zorro SCSI boards for
> m68k Amiga.
>
> Code largely based on board specific parts of the old drivers (blz1230.c,
> blz2060.c, cyberstorm.c, cyberstormII.c, fastlane.c which were removed
> after the 2.6 kernel series for lack of maintenance) with contributions
> by Tuomas Vainikka (TCQ bug tests and workaround) and Finn Thain (TCQ
> bugfix by use of PIO in extended message in transfer).
>
> New Kconfig option and Makefile entries for new Amiga Zorro ESP SCSI
> driver included in this patch.
>
> Use DMA transfers wherever possible, with board-specific DMA set-up
> functions copied from the old driver code. Three byte reselection messages
>  do appear to cause DMA timeouts. So wire up a PIO transfer routine for
> these instead.
>
> PIO code taken from mac_esp.c where the reselection timeout issue was
> debugged and fixed first, with the following modifications:
>
> esp_reselect_with_tag explicitly sets esp->cmd_block_dma as target address
> for the message bytes. Fixup to use kernel virtual address esp->cmd_block
> in PIO transfer if DMA address is esp->cmd_block_dma.
>
> Signed-off-by: Michael Schmitz <schmitz...@gmail.com>
>
> ---
>
> Changes since v1:

[...]

Thanks for the update!

A few more comments below, mostly stylistic / practice.

> --- /dev/null
> +++ b/drivers/scsi/zorro_esp.c

> +struct zorro_driver_data {
> +       const char *name;
> +       unsigned long offset;
> +       unsigned long dma_offset;
> +       int absolute;   /* offset is absolute address */
> +       int scsi_option;
> +};
> +
> +static struct zorro_driver_data cyberstormI_data = {

const

> +static struct zorro_device_id zorro_esp_zorro_tbl[] = {

const

> +       {
> +               .id = ZORRO_PROD_PHASE5_BLIZZARD_1220_CYBERSTORM,
> +               .driver_data = (unsigned long)&cyberstormI_data,
> +       },
> +       {
> +               .id = ZORRO_PROD_PHASE5_CYBERSTORM_MK_II,
> +               .driver_data = (unsigned long)&cyberstormII_data,
> +       },
> +       {
> +               .id = ZORRO_PROD_PHASE5_BLIZZARD_2060,
> +               .driver_data = (unsigned long)&blizzard2060_data,
> +       },
> +       {
> +               .id = ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260,
> +               .driver_data = (unsigned long)&blizzard1230_data,
> +       },
> +       {
> +               .id = 
> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060,
> +               .driver_data = (unsigned long)&blizzard1230II_data,
> +       },
> +       { 0 }
> +};
> +MODULE_DEVICE_TABLE(zorro, zorro_esp_zorro_tbl);
> +
> +/* Blizzard 1230 DMA interface */
> +
> +struct blz1230_dma_registers {
> +       volatile unsigned char dma_addr;        /* DMA address      [0x0000] 
> */

volatile considered harmful.
If you would use proper *{read,write}*() accessors instead of direct
assignments,
you can drop the volatile's here.

> +#define ZORRO_ESP_GET_PRIV(esp) ((struct zorro_esp_priv *) \
> +                               dev_get_drvdata(esp->dev))

This macro can be dropped, just use "dev_get_drvdata(esp->dev)" directly.
No cast is needed.

> +static int fastlane_esp_irq_pending(struct esp *esp)
> +{
> +       struct fastlane_dma_registers *dregs =
> +               (struct fastlane_dma_registers *) (esp->dma_regs);

struct fastlane_dma_registers __iomem *dregs = esp->dma_regs;

(and make C=1 will become (more) happy)

> +       unsigned char dma_status;
> +
> +       dma_status = dregs->cond_reg;

readb().

> +#define ZORRO_ESP_PIO_LOOP(operands, reg1) \
> +       { \
> +       asm volatile ( \
> +            "1:     moveb " operands "\n" \
> +            "       subqw #1,%1       \n" \
> +            "       jbne 1b           \n" \
> +            : "+a" (addr), "+r" (reg1) \
> +            : "a" (fifo)); \
> +       }

Please pass "addr" and "fifo" as macro parameters, too, so it's easier for
the reviewer to notice they are used.

> +#define ZORRO_ESP_PIO_FILL(operands, reg1) \
> +       { \
> +       asm volatile ( \
> +            "       moveb " operands "\n" \
> +            "       moveb " operands "\n" \
> +            "       moveb " operands "\n" \
> +            "       moveb " operands "\n" \
> +            "       moveb " operands "\n" \
> +            "       moveb " operands "\n" \
> +            "       moveb " operands "\n" \
> +            "       moveb " operands "\n" \
> +            "       moveb " operands "\n" \
> +            "       moveb " operands "\n" \
> +            "       moveb " operands "\n" \
> +            "       moveb " operands "\n" \
> +            "       moveb " operands "\n" \
> +            "       moveb " operands "\n" \
> +            "       moveb " operands "\n" \
> +            "       moveb " operands "\n" \
> +            "       subqw #8,%1       \n" \
> +            "       subqw #8,%1       \n" \
> +            : "+a" (addr), "+r" (reg1) \
> +            : "a" (fifo)); \
> +       }

Likewise.

> +static int zorro_esp_probe(struct zorro_dev *z,
> +                                      const struct zorro_device_id *ent)
> +{
> +       struct scsi_host_template *tpnt = &scsi_esp_template;
> +       struct Scsi_Host *host;
> +       struct esp *esp;
> +       struct zorro_driver_data *zdd;
> +       struct zorro_esp_priv *zep;
> +       unsigned long board, ioaddr, dmaaddr;
> +       int err;
> +
> +       board = zorro_resource_start(z);
> +       zdd = (struct zorro_driver_data *)ent->driver_data;
> +
> +       pr_info("%s found at address 0x%lx.\n", zdd->name, board);
> +
> +       zep = kmalloc(sizeof(*zep), GFP_KERNEL);
> +       if (!zep) {
> +               pr_err("Can't allocate device private data!\n");
> +               return -ENOMEM;
> +       }
> +
> +       /* let's figure out whether we have a Zorro II or Zorro III board */
> +       if ((z->rom.er_Type & ERT_TYPEMASK) == ERT_ZORROIII) {
> +               /* note this is a Zorro III board */
> +               if (board > 0xffffff)
> +                       zep->zorro3 = 1;
> +       } else

} else {

> +               /* Even though most of these boards identify as Zorro II,
> +                * they are in fact CPU expansion slot boards and have full
> +                * access to all of memory. Fix up DMA bitmask here.
> +                */
> +               z->dev.coherent_dma_mask = DMA_BIT_MASK(32);

}

> +       /* Switch to the correct the DMA routine and clock frequency. */
> +       switch (ent->id) {
> +       case ZORRO_PROD_PHASE5_BLIZZARD_2060:
> +               zorro_esp_ops.send_dma_cmd = zorro_esp_send_blz2060_dma_cmd;

Please use function pointers in struct zorro_driver_data, so you don't need
a switch() here (except for Fastlane vs. B1230II).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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

Reply via email to