On Wed, 9 Sep 2020, Geert Uytterhoeven wrote:
Thanks for your patch!
Thanks for your review.
--- a/arch/m68k/mac/config.c +++ b/arch/m68k/mac/config.c
@@ -940,6 +941,50 @@ static const struct resource mac_scsi_ccl_rsrc[] __initconst = {
},
};
+static const struct resource mac_ide_quadra_rsrc[] __initconst = {
+ {
+ .flags = IORESOURCE_MEM,
+ .start = 0x50F1A000,
+ .end = 0x50F1A103,
+ }, {
+ .flags = IORESOURCE_IRQ,
+ .start = IRQ_NUBUS_F,
+ .end = IRQ_NUBUS_F,
+ },
+};
+
+static const struct resource mac_ide_pb_rsrc[] __initconst = {
+ {
+ .flags = IORESOURCE_MEM,
+ .start = 0x50F1A000,
+ .end = 0x50F1A103,
+ }, {
+ .flags = IORESOURCE_IRQ,
+ .start = IRQ_NUBUS_C,
+ .end = IRQ_NUBUS_C,
+ },
+};
As the above two variants are almost identical, perhaps it makes sense
to drop one of them, drop the const, and override the irq values
dynamically?
I prefer a declarative or data-driven style, even if it takes a few more
lines of code. But there is a compromise:
static const struct resource mac_ide_quadra_rsrc[] __initconst = {
DEFINE_RES_MEM(0x50F1A000, 0x104),
DEFINE_RES_IRQ(IRQ_NUBUS_F),
}
static const struct resource mac_ide_pb_rsrc[] __initconst = {
DEFINE_RES_MEM(0x50F1A000, 0x104),
DEFINE_RES_IRQ(IRQ_NUBUS_C),
}
The reason I didn't use these macros was to avoid making the reader go and
look up their definitions. Anyway, would that style be preferred here?
I could do the same with the mac_ide_baboon_rsrc[] initializer:
static const struct resource mac_pata_baboon_rsrc[] __initconst = {
DEFINE_RES_MEM(0x50F1A000, 0x38),
DEFINE_RES_MEM(0x50F1A038, 0x04),
DEFINE_RES_IRQ(IRQ_BABOON_1),
};
... but that would lose the IORESOURCE_IRQ_SHAREABLE flag. I'm not sure
whether that matters (it's a vestige of macide.c).
+
+static const struct resource mac_pata_baboon_rsrc[] __initconst = {
+ {
+ .flags = IORESOURCE_MEM,
+ .start = 0x50F1A000,
+ .end = 0x50F1A037,
+ }, {
+ .flags = IORESOURCE_MEM,
+ .start = 0x50F1A038,
+ .end = 0x50F1A03B,
+ }, {
+ .flags = IORESOURCE_IRQ | IORESOURCE_IRQ_SHAREABLE,
+ .start = IRQ_BABOON_1,
+ .end = IRQ_BABOON_1,
+ },
+};
+
+static const struct pata_platform_info mac_pata_baboon_data __initconst = {
+ .ioport_shift = 2,
+};
Just wondering: how is this implemented in drivers/ide/macide.c, which
doesn't use the platform info?
That factor of 4 is embedded in the address caclulation:
for (i = 0; i < 8; i++)
hw->io_ports_array[i] = base + i * 4;
--- a/drivers/ide/macide.c
+++ b/drivers/ide/macide.c
@@ -18,10 +18,11 @@
#include <linux/delay.h>
#include <linux/ide.h>
#include <linux/module.h>
+#include <linux/platform_device.h>
#include <asm/macintosh.h>
-#include <asm/macints.h>
-#include <asm/mac_baboon.h>
+
+#define DRV_NAME "mac_ide"
#define IDE_BASE 0x50F1A000 /* Base address of IDE controller */
Do you still need this definition?
Yes, because it's still used to access IDE_IFR.
Ideally, that should be converted to use the base from the resource,
too.
Yes, that was my thought too. I can make the change if you like, but I
can't test it until I set up the appropriate hardware (MAC_IDE_QUADRA or
MAC_IDE_PB). I do own that hardware but it is located in Melbourne and it
is now illegal to visit Melbourne without official papers. Besides, once I
can test on that hardware I can replace the entire driver anyway, and
this kind of refactoring would become moot.
@@ -109,42 +110,65 @@ static const char *mac_ide_name[] =
* Probe for a Macintosh IDE interface
*/
-static int __init macide_init(void)
+static int mac_ide_probe(struct platform_device *pdev)
{
- unsigned long base;
- int irq;
+ struct resource *mem, *irq;
struct ide_hw hw, *hws[] = { &hw };
struct ide_port_info d = macide_port_info;
+ struct ide_host *host;
+ int rc;
if (!MACH_IS_MAC)
return -ENODEV;
- switch (macintosh_config->ide_type) {
- case MAC_IDE_QUADRA:
- base = IDE_BASE;
- irq = IRQ_NUBUS_F;
- break;
- case MAC_IDE_PB:
- base = IDE_BASE;
- irq = IRQ_NUBUS_C;
- break;
- case MAC_IDE_BABOON:
- base = BABOON_BASE;
- d.port_ops = NULL;
How does the driver know not to use the special port_ops after
this change?
The driver always uses the special port_ops after this change because it
no longer handles the MAC_IDE_BABOON case. That case is handled by either
drivers/ata/pata_platform.c or drivers/ide/ide_platform.c, depending on
.config.
- irq = IRQ_BABOON_1;
- break;
- default:
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!mem)
+ return -ENODEV;
+
+ irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ if (!irq)
return -ENODEV;
+
+ if (!devm_request_mem_region(&pdev->dev, mem->start,
+ resource_size(mem), DRV_NAME)) {
+ dev_err(&pdev->dev, "resources busy\n");
+ return -EBUSY;
}
printk(KERN_INFO "ide: Macintosh %s IDE controller\n",
mac_ide_name[macintosh_config->ide_type - 1]);
- macide_setup_ports(&hw, base, irq);
+ macide_setup_ports(&hw, mem->start, irq->start);
+
+ rc = ide_host_add(&d, hws, 1, &host);
+ if (rc)
+ goto release_mem;
+
+ platform_set_drvdata(pdev, host);
In general, it's safer to move the platform_set_drvdata() call before
the ide_host_add() call, as the IDE core may start calling into your
driver as soon as the host has been added. Fortunately you're using
dev_get_drvdata() in the .remove() callback only, and not in other parts
of the driver.
Right.
+ return 0;
+
+release_mem:
+ release_mem_region(mem->start, resource_size(mem));
Not needed, as you used devm_*() for allocation.
OK, I'll remove this. I put it there after I looked at falconide.c and
wondered whether the automatic release would take place after both init
failure and exit (or just exit). I see now that pata_gayle.c does it
differently.
+ return rc;
+}
+
+static int mac_ide_remove(struct platform_device *pdev)
+{
+ struct ide_host *host = dev_get_drvdata(&pdev->dev);
- return ide_host_add(&d, hws, 1, NULL);
+ ide_host_remove(host);
+ return 0;
}
Gr{oetje,eeting}s,
Geert
