Re: [PATCH v2] ata: add Amiga Gayle PATA controller driver

[Andy Shevchenko]

On Thu, Mar 1, 2018 at 1:02 PM, Bartlomiej Zolnierkiewicz
<b.zolnierkie@xxxxxxxxxxx> wrote:
> Add Amiga Gayle PATA controller driver. It enables libata support
> for the on-board IDE interfaces on some Amiga models (A600, A1200,
> A4000 and A4000T) and also for IDE interfaces on the Zorro expansion
> bus (M-Tech E-Matrix 530 expansion card).
>
> Thanks to John Paul Adrian Glaubitz and Michael Schmitz for help
> with testing the driver.




> +#include <linux/kernel.h>

> +#include <linux/module.h>
> +#include <linux/init.h>

Choose one of those two.

> +#include <linux/blkdev.h>
> +#include <linux/delay.h>
> +#include <scsi/scsi_host.h>
> +#include <scsi/scsi_cmnd.h>
> +#include <linux/ata.h>
> +#include <linux/libata.h>
> +#include <linux/mm.h>
> +#include <linux/interrupt.h>
> +#include <linux/zorro.h>
> +#include <linux/platform_device.h>

Better to keep in order.

> +#include <asm/setup.h>
> +#include <asm/amigahw.h>
> +#include <asm/amigaints.h>
> +#include <asm/amigayle.h>
> +#include <asm/ide.h>

Ditto.

> +static bool pata_gayle_irq_check(struct ata_port *ap)
> +{
> +       u8 ch;
> +
> +       ch = z_readb((unsigned long)ap->private_data);

> +       if (!(ch & GAYLE_IRQ_IDE))
> +               return false;
> +       return true;

return !!(ch & GAYLE_IRQ_IDE);

?

> +}

> +static int __init pata_gayle_init_one(struct platform_device *pdev)
> +{

> +       pr_info(DRV_NAME ": Amiga Gayle IDE controller (A%u style)\n",
> +               pdata->explicit_ack ? 1200 : 4000);

What's wrong with dev_info() ?

> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

> +       if (!res)
> +               return -ENODEV;

> +
> +       if (!devm_request_mem_region(&pdev->dev, res->start,
> +                                    resource_size(res), DRV_NAME)) {
> +               pr_err(DRV_NAME ": resources busy\n");
> +               return -EBUSY;
> +       }
> +

> +       base = ZTWO_VADDR(pdata->base);

Hmm... Can't you use devm_ioremap_resources() to get the virtual
address for I/O ?

> +       ap->private_data = (void *)ZTWO_VADDR(pdata->irqport);
> +
> +       ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx", (unsigned long)base,
> +                     (unsigned long)base + GAYLE_CONTROL);

When you use explicit casting in printf() you are doing in 99.9% cases
something wrong.

> +       /* activate */

Noise.

> +       ret = ata_host_activate(host, IRQ_AMIGA_PORTS, ata_sff_interrupt,
> +                               IRQF_SHARED, &pata_gayle_sht);
> +       if (ret)
> +               return ret;

> +}

-- 
With Best Regards,
Andy Shevchenko