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
