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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to