Hi Michael,

On Tue, Mar 6, 2018 at 2:33 AM, Michael Schmitz <schmitz...@gmail.com> wrote:
> On Tue, Mar 6, 2018 at 12:29 AM, Geert Uytterhoeven
> <ge...@linux-m68k.org> wrote:

>>> +static unsigned char ctrl_data;        /* Keep backup of the stuff written
>>> +                                * to ctrl_reg. Always write a copy
>>> +                                * to this register when writing to
>>> +                                * the hardware register!
>>> +                                */
>>
>> This should be part of the device's zorro_esp_priv.
>
> It's only used in the cyber_dma_setup, and I not actually read
> anywhere else. Might as well be on the stack instead of static...

OK, I missed that. Yes, local variable is better.

>>> +static int zorro_esp_init_one(struct zorro_dev *z,
>>> +                                      const struct zorro_device_id *ent)

BTW, please call the probe/remove functions zorro_esp_probe() resp.
zorro_esp_remove().

>>> +{
>>> +       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 = -ENOMEM;
>>
>> Initialization not needed.
>
> The initialized err variable is used when bailing out ..
>
>>> +
>>> +       board = zorro_resource_start(z);
>>> +       zdd = (struct zorro_driver_data *)ent->driver_data;
>>> +
>>> +       pr_info(PFX "%s found at address 0x%lx.\n", zdd->name, board);
>>> +
>>> +       if (zdd->absolute) {
>>> +               ioaddr  = zdd->offset;
>>> +               dmaaddr = zdd->dma_offset;
>>> +       } else {
>>> +               ioaddr  = board + zdd->offset;
>>> +               dmaaddr = board + zdd->dma_offset;
>>> +       }
>>> +
>>> +       if (!zorro_request_device(z, zdd->name)) {
>>> +               pr_err(PFX "cannot reserve region 0x%lx, abort\n",
>>> +                      board);
>>> +               return -EBUSY;
>>> +       }
>>> +
>>> +       /* Fill in the required pieces of hostdata */
>>> +
>>> +       host = scsi_host_alloc(tpnt, sizeof(struct esp));
>>> +
>>> +       if (!host) {
>>> +               pr_err(PFX "No host detected; board configuration 
>>> problem?\n");
>>> +               goto out_free;
>>> +       }
>
> here. But I can add the err=-NOMEM here.

After out_free it returns fixed -ENODEV ;-)

Doing "err = -ENOMEM" here, and returning err at the end is better, as
it propagates meaningful error codes.

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
--
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