Re: [PATCH] new mac_scsi driver

James Bottomley
2008-04-25T15:03:14Z

On Fri, 2008-04-25 at 11:06 +1000, Finn Thain wrote:
>
> On Thu, 24 Apr 2008, James Bottomley wrote:
>
> > On Thu, 2008-04-24 at 23:08 +0200, Geert Uytterhoeven wrote:
> > > On Thu, 24 Apr 2008, Finn Thain wrote:
> > > > Replace the mac_esp driver with a new one based on the esp_scsi
> > > > core.
> > > >
> > > > For esp_scsi: add support for sync transfers for the PIO mode, add a
> > > > new esp_driver_ops method to get the maximum dma transfer size (like
> > > > the old NCR53C9x driver), and some cleanups.
> > >
> > > Thanks!
> > >
> > > I added this patch to my series, after fixing the few checkpatch.pl
> > > issues and adding a test for MACH_IS_MAC() to esp_mac_probe().
> >
> > I got the tabs and spaces thing. I'm not too concerned about the
> > assignment in conditional, but I'm happy to go whichever way the author
> > does.
>
> FWIW, I happen to disagree with checkpatch about the spaces following tabs
> thing but I'm happy to follow convention. It did pick up on some other
> things that I overlooked (Geert has fixed them).
>
> >
> > Could you repost please because I already have this queued, so I need a
> > replacement (assuming everyone agrees).
>
> Geert's version is fine with me:
>
> http://linux-m68k-cvs.ubb.ca/~geert/linux-m68k-patches-2.6/m68k-new-mac_esp-driver.diff

It's a lot less easy to review stuff over the web (mainly, I'm afraid,
because people are lazy).

However, this:

> +static int __devinit esp_mac_probe(struct platform_device *dev)
> +{
> + struct scsi_host_template *tpnt = &scsi_esp_template;
> + struct Scsi_Host *host;
> + struct esp *esp;
> + int err;
> + int chips_present;
> + struct mac_esp_priv *mep;
> +
> + if (!MACH_IS_MAC)
> + return -ENODEV;

Looks strange ... it seems you have to do this because macintosh_config
which is used later can be uninitialised (i.e. pointing to rubbish) if
it's not set?

James