Just a few style comments:
> +static int zorro_esp_irq_pending(struct esp *);
> +static int cyber_esp_irq_pending(struct esp *);
> +static int fastlane_esp_irq_pending(struct esp *);
Please avoid forward declarations wherever possible.
> +struct zorro_driver_data {
> + const char *name;
> + unsigned long offset;
> + unsigned long dma_offset;
> + int absolute; /* offset is absolute address */
> + int scsi_option;
> + int (*irq_pending)(struct esp *esp);
> + void (*dma_invalidate)(struct esp *esp);
> + void (*send_dma_cmd)(struct esp *esp, u32 dma_addr, u32 esp_count,
> + u32 dma_count, int write, u8 cmd);
> +};
Please use different esp_driver_ops instances for the different board
types.
> +static const struct zorro_driver_data cyberstormI_data = {
> + .name = "CyberStormI",
> + .offset = 0xf400,
> + .dma_offset = 0xf800,
> + .absolute = 0,
> + .scsi_option = 0,
You can remove all the xero initializations on static data.
Also please align the = signs with tabs in struct declarations.
> +static dma_addr_t zorro_esp_map_single(struct esp *esp, void *buf,
> + size_t sz, int dir)
> +{
> + return dma_map_single(esp->dev, buf, sz, dir);
> +}
> +
> +static int zorro_esp_map_sg(struct esp *esp, struct scatterlist *sg,
> + int num_sg, int dir)
> +{
> + return dma_map_sg(esp->dev, sg, num_sg, dir);
> +}
> +
> +static void zorro_esp_unmap_single(struct esp *esp, dma_addr_t addr,
> + size_t sz, int dir)
> +{
> + dma_unmap_single(esp->dev, addr, sz, dir);
> +}
> +
> +static void zorro_esp_unmap_sg(struct esp *esp, struct scatterlist *sg,
> + int num_sg, int dir)
> +{
> + dma_unmap_sg(esp->dev, sg, num_sg, dir);
> +}
These wrappers seem rather pointless.
> +/* PIO macros as used in mac_esp.c.
> + * Note that addr and fifo arguments are local-scope variables declared
> + * in zorro_esp_send_pio_cmd(), the macros are only used in that function,
> + * and addr and fifo are referenced in each use of the macros so there
> + * is no need to pass them as macro parameters.
> + */
Please use normal Linux comment style:
/*
* Blah, blah, blah.
*/
> +#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)); \
> + }
What is the point of the curly braces around the asm statement?
> +static void zorro_esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
> + u32 dma_count, int write, u8 cmd)
> +{
> + struct zorro_esp_priv *zep = dev_get_drvdata(esp->dev);
> + u8 __iomem *fifo = esp->regs + ESP_FDATA * 16;
> + u8 phase = esp->sreg & ESP_STAT_PMASK;
> +
> + cmd &= ~ESP_CMD_DMA;
> +
> + if (write) {
It seems like this routine would benefit from being split into a read
and a write one.
> +// Blizzard 1230/60 SCSI-IV DMA
/* ... */
> + /* Use PIO if transferring message bytes to esp->command_block_dma.
> + * PIO requires a virtual address, so substitute esp->command_block
> + * for addr.
> + */
Comment style, again.
> +static struct esp_driver_ops zorro_esp_ops = {
should be marked const.
> + .esp_write8 = zorro_esp_write8,
Just use a space after the '='.
> +
> + if (zep->zorro3) {
> + /* Only Fastlane Z3 for now - add switch for correct struct
> + * dma_registers size if adding any more
> + */
> + esp->dma_regs = ioremap_nocache(dmaaddr,
> + sizeof(struct fastlane_dma_registers));
> + } else
> + esp->dma_regs = (void __iomem *)ZTWO_VADDR(dmaaddr);
doesn't this need a __force (and a comment on why it is Ń•afe) to make
sparse happy?
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
