Hi Finn,
Am 12.03.2018 um 22:04 schrieb Finn Thain:
+ if (addr == esp->command_block_dma)
+ addr = (u32) esp->command_block;
Since you've removed the alternative branch and phys_to_virt(), I suggest
you do this at function invocation... (see below)
Keeps it together with the phase and address tests, so easier to follow.
+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;
The comment here seems to be redundant (?)
Not the only one.
More importantly, I think you have to initialize zep->zorro3 = 0 in the
other branch, or else use kzalloc() instead of kmalloc() above.
Using kzalloc() saves the zep->error=0 initialization later on so it's a
win.
+ case ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060:
+ if (zep->zorro3) {
+ /* Fastlane Zorro III board */
+ /* map address space up to ESP address for DMA */
Same here?
I've left the comment explaining what the board low address space is
used for.
+ zep->board_base = ioremap_nocache(board,
+ FASTLANE_ESP_ADDR-1);
+ if (!zep->board_base) {
+ pr_err("Cannot allocate board address space\n");
+ err = -ENOMEM;
+ goto fail_free_host;
+ }
+ /* initialize DMA control shadow register */
+ zep->ctrl_data = (FASTLANE_DMA_FCODE |
+ FASTLANE_DMA_EDI | FASTLANE_DMA_ESI);
+ zorro_esp_ops.send_dma_cmd =
+ zorro_esp_send_fastlane_dma_cmd;
+ zorro_esp_ops.irq_pending = fastlane_esp_irq_pending;
+ zorro_esp_ops.dma_invalidate =
+ fastlane_esp_dma_invalidate;
+ } else {
+ /* Blizzard 1230 II Zorro II board */
and that one - the board might be a Cyberstorm060 which would require
replacement of driver data, and replacement of the ESP register mapping.
Might need to clarify that here again...
+ zorro_esp_ops.send_dma_cmd =
+ zorro_esp_send_blz1230II_dma_cmd;
+ //zorro_set_drvdata(z, host);
+
Can that be deleted?
Leftover from when private data were static, so can go now.
+static void zorro_esp_remove(struct zorro_dev *z)
+{
+ /* equivalent to dev_get_drvdata(z->dev) */
If zorro_get_drvdata(z) needs a comment to explain it then why not just
write dev_get_drvdata(z->dev) instead?
dev_get_drvdata(&z->dev) (struct zorro device incorporates the whole
struct device, not a pointer), but yes.
+ struct zorro_esp_priv *zep = zorro_get_drvdata(z);
+ struct esp *esp = zep->esp;
+ struct Scsi_Host *host = esp->host;
+
+ scsi_esp_unregister(esp);
+
+ /* Disable interrupts. Perhaps use disable_irq instead ... */
+
Can you clarify? free_irq() calls irq_shutdown()...
Which calls disable_irq() eventually if it's the last of shared
interrupts, true. disable_irq() would clearly be wrong here.
Thanks!
My pleasure. I forgot to add your Reviewed-by tag - will do that for the
next version, OK?
Cheers,
Michael
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
