Re: [PATCH] ide/macide: Convert Mac IDE driver to platform driver

From: Geert Uytterhoeven
Date: Thu Sep 10 2020 - 02:27:02 EST

Hi Finn,

On Thu, Sep 10, 2020 at 2:23 AM Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, 9 Sep 2020, Geert Uytterhoeven wrote:
> > > --- 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 think the DEFINE_RES_*() are sufficiently common (well, in pre-DT
platforms ;-)

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

You can still use DEFINE_RES_NAMED() to pass the flags.
Would you consider that to be a good compromise?

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

IC. But in the new code, the platform info is passed for Baboon only,
while the old code used it for all variants.

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

OK.

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

non-MAC_IDE_BABOON case?

> drivers/ata/pata_platform.c or drivers/ide/ide_platform.c, depending on
> .config.

Ideally, we do need to differentiate, right?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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