| Message ID | 1572935872-28394-1-git-send-email-schmitzmic@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | ide: falconide: convert to platform driver | expand |
Hi Michael, On Tue, Nov 5, 2019 at 7:38 AM Michael Schmitz <schmitzmic@gmail.com> wrote: > With the introduction of a platform device for the Atari Falcon IDE > interface, the old Falcon IDE driver no longer loads (resource already > claimed by the platform device). > > Convert falconide driver to use the same platform device that is used > by pata_falcon also. > > Tested (as built-in driver) on my Atari Falcon. > > Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> > CC: Geert Uytterhoeven <geert@linux-m68k.org> Thanks for your patch! > --- a/drivers/ide/falconide.c > +++ b/drivers/ide/falconide.c > @@ -15,6 +15,7 @@ > #include <linux/blkdev.h> > #include <linux/ide.h> > #include <linux/init.h> > +#include <linux/platform_device.h> > > #include <asm/setup.h> > #include <asm/atarihw.h> > @@ -23,6 +24,7 @@ > #include <asm/ide.h> > > #define DRV_NAME "falconide" > +#define DRV_VERSION "0.1.0" Does anyone care about that version? Will it ever be updated? > @@ -169,10 +177,21 @@ static int __init falconide_init(void) > err_free: > ide_host_free(host); > err: > - release_mem_region(ATA_HD_BASE, 0x40); > + release_mem_region(res->start, resource_size(res)); > return rc; > } > > -module_init(falconide_init); > +static struct platform_driver ide_falcon_driver = { > + .driver = { > + .name = "atari-falcon-ide", > + }, > +}; Missing .remove() callback. > + > +module_platform_driver_probe(ide_falcon_driver, falconide_init); > + > > +MODULE_AUTHOR("Geert Uytterhoeven"); > +MODULE_DESCRIPTION("low-level driver for Atari Falcon IDE"); > MODULE_LICENSE("GPL"); > +MODULE_ALIAS("platform:atari-falcon-ide"); > +MODULE_VERSION(DRV_VERSION); I'd drop the MODULE_VERSION(). Gr{oetje,eeting}s, Geert
Hi Geert, thanks for the review! On Tue, Nov 5, 2019 at 9:11 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Michael, > > On Tue, Nov 5, 2019 at 7:38 AM Michael Schmitz <schmitzmic@gmail.com> wrote: > > With the introduction of a platform device for the Atari Falcon IDE > > interface, the old Falcon IDE driver no longer loads (resource already > > claimed by the platform device). > > > > Convert falconide driver to use the same platform device that is used > > by pata_falcon also. > > > > Tested (as built-in driver) on my Atari Falcon. > > > > Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> > > CC: Geert Uytterhoeven <geert@linux-m68k.org> > > Thanks for your patch! > > > --- a/drivers/ide/falconide.c > > +++ b/drivers/ide/falconide.c > > @@ -15,6 +15,7 @@ > > #include <linux/blkdev.h> > > #include <linux/ide.h> > > #include <linux/init.h> > > +#include <linux/platform_device.h> > > > > #include <asm/setup.h> > > #include <asm/atarihw.h> > > @@ -23,6 +24,7 @@ > > #include <asm/ide.h> > > > > #define DRV_NAME "falconide" > > +#define DRV_VERSION "0.1.0" > > Does anyone care about that version? Not likely. > Will it ever be updated? You ask me? You're still listed as driver author! I'll remove the version. > > > @@ -169,10 +177,21 @@ static int __init falconide_init(void) Should I remove the __init here? Doesn't hurt in the built-in use case, what about use as a module? > > err_free: > > ide_host_free(host); > > err: > > - release_mem_region(ATA_HD_BASE, 0x40); > > + release_mem_region(res->start, resource_size(res)); > > return rc; > > } > > > > -module_init(falconide_init); > > +static struct platform_driver ide_falcon_driver = { > > + .driver = { > > + .name = "atari-falcon-ide", > > + }, > > +}; > > Missing .remove() callback. Can't easily test driver remove, but I can certainly add a callback for that. ide_unregister does the Right Thing (i.e. leaves the ST-DMA interrupt registered) so no reason why it shouldn't work. > > > + > > +module_platform_driver_probe(ide_falcon_driver, falconide_init); > > + > > > > +MODULE_AUTHOR("Geert Uytterhoeven"); > > +MODULE_DESCRIPTION("low-level driver for Atari Falcon IDE"); > > MODULE_LICENSE("GPL"); > > +MODULE_ALIAS("platform:atari-falcon-ide"); > > +MODULE_VERSION(DRV_VERSION); > > I'd drop the MODULE_VERSION(). Done. Shall I merge this one with part one of the old series so there's no chance of a bisection going wrong? Cheers, Michael > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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
Hi Michael, On Tue, Nov 5, 2019 at 7:31 PM Michael Schmitz <schmitzmic@gmail.com> wrote: > On Tue, Nov 5, 2019 at 9:11 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Tue, Nov 5, 2019 at 7:38 AM Michael Schmitz <schmitzmic@gmail.com> wrote: > > > With the introduction of a platform device for the Atari Falcon IDE > > > interface, the old Falcon IDE driver no longer loads (resource already > > > claimed by the platform device). > > > > > > Convert falconide driver to use the same platform device that is used > > > by pata_falcon also. > > > > > > Tested (as built-in driver) on my Atari Falcon. > > > > > > Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> > > > --- a/drivers/ide/falconide.c > > > +++ b/drivers/ide/falconide.c > > > @@ -23,6 +24,7 @@ > > > #include <asm/ide.h> > > > > > > #define DRV_NAME "falconide" > > > +#define DRV_VERSION "0.1.0" > > > > Does anyone care about that version? > > Not likely. > > > Will it ever be updated? > > You ask me? You're still listed as driver author! Yeah, I wrote a "it compiles, so it must work" driver in the IDE framework, based on a driver in an even older framework. I never ran it on hardware ;-) > > > @@ -169,10 +177,21 @@ static int __init falconide_init(void) > > Should I remove the __init here? Doesn't hurt in the built-in use > case, what about use as a module? Should be fine. > > > > err_free: > > > ide_host_free(host); > > > err: > > > - release_mem_region(ATA_HD_BASE, 0x40); > > > + release_mem_region(res->start, resource_size(res)); > > > return rc; > > > } > > > > > > -module_init(falconide_init); > > > +static struct platform_driver ide_falcon_driver = { > > > + .driver = { > > > + .name = "atari-falcon-ide", > > > + }, > > > +}; > > > > Missing .remove() callback. > > Can't easily test driver remove, but I can certainly add a callback for that. > > ide_unregister does the Right Thing (i.e. leaves the ST-DMA interrupt > registered) so no reason why it shouldn't work. gayle.c uses ide_host_remove(). > > > + > > > +module_platform_driver_probe(ide_falcon_driver, falconide_init); > > > + > > > > > > +MODULE_AUTHOR("Geert Uytterhoeven"); > > > +MODULE_DESCRIPTION("low-level driver for Atari Falcon IDE"); > > > MODULE_LICENSE("GPL"); > > > +MODULE_ALIAS("platform:atari-falcon-ide"); > > > +MODULE_VERSION(DRV_VERSION); > > > > I'd drop the MODULE_VERSION(). > > Done. > > Shall I merge this one with part one of the old series so there's no > chance of a bisection going wrong? Yes please. Thanks! Gr{oetje,eeting}s, Geert
Hi Geert, On Wed, Nov 6, 2019 at 7:46 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > @@ -169,10 +177,21 @@ static int __init falconide_init(void) > > > > Should I remove the __init here? Doesn't hurt in the built-in use > > case, what about use as a module? > > Should be fine. OK, I'll leave it. > > > > > > > err_free: > > > > ide_host_free(host); > > > > err: > > > > - release_mem_region(ATA_HD_BASE, 0x40); > > > > + release_mem_region(res->start, resource_size(res)); > > > > return rc; > > > > } > > > > > > > > -module_init(falconide_init); > > > > +static struct platform_driver ide_falcon_driver = { > > > > + .driver = { > > > > + .name = "atari-falcon-ide", > > > > + }, > > > > +}; > > > > > > Missing .remove() callback. > > > > Can't easily test driver remove, but I can certainly add a callback for that. > > > > ide_unregister does the Right Thing (i.e. leaves the ST-DMA interrupt > > registered) so no reason why it shouldn't work. > > gayle.c uses ide_host_remove(). I'm using that, too. But ide_host_remove() is not where free_irq() is called. > > > > > + > > > > +module_platform_driver_probe(ide_falcon_driver, falconide_init); > > > > + > > > > > > > > +MODULE_AUTHOR("Geert Uytterhoeven"); > > > > +MODULE_DESCRIPTION("low-level driver for Atari Falcon IDE"); > > > > MODULE_LICENSE("GPL"); > > > > +MODULE_ALIAS("platform:atari-falcon-ide"); > > > > +MODULE_VERSION(DRV_VERSION); > > > > > > I'd drop the MODULE_VERSION(). > > > > Done. > > > > Shall I merge this one with part one of the old series so there's no > > chance of a bisection going wrong? > > Yes please. > Thanks! Thanks, I'll send a new version shortly. Cheers, Michael
Hi Geert, > On Wed, Nov 6, 2019 at 7:46 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > Shall I merge this one with part one of the old series so there's no > > > chance of a bisection going wrong? > > > > Yes please. > > Thanks! > > Thanks, I'll send a new version shortly. Just confirming - the changes to pata_falcon.c will remain as a separate patch which should be applied together with the patch that will introduce the new platform device, and rewrite the legacy driver to use it. That would require Bartlomiej and you to coordinate closely. If that's too onerous, I can merge the lot and you just ack the m68k bits? Please let me know what you'd prefer. Cheers, Michael
Hi Michael, On Tue, Nov 5, 2019 at 10:13 PM Michael Schmitz <schmitzmic@gmail.com> wrote: > > On Wed, Nov 6, 2019 at 7:46 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > Shall I merge this one with part one of the old series so there's no > > > > chance of a bisection going wrong? > > > > > > Yes please. > > > Thanks! > > > > Thanks, I'll send a new version shortly. > > Just confirming - the changes to pata_falcon.c will remain as a > separate patch which should be applied together with the patch that > will introduce the new platform device, and rewrite the legacy driver > to use it. That would require Bartlomiej and you to coordinate > closely. Bartlomiej already acked both patches, so they can go in through the m68k tree. To avoid bisection regressions, both patches should be merged into a single patch... > If that's too onerous, I can merge the lot and you just ack the m68k > bits? Please let me know what you'd prefer. ... and with the falconide.c conversion, all three patches should be merged into a single patch. Gr{oetje,eeting}s, Geert
Hi Geert, thanks for confirming, I'll send a merged version shortly. Cheers, Michael On 6/11/19 10:43 AM, Geert Uytterhoeven wrote: > Hi Michael, > > On Tue, Nov 5, 2019 at 10:13 PM Michael Schmitz <schmitzmic@gmail.com> wrote: >>> On Wed, Nov 6, 2019 at 7:46 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: >>>>> Shall I merge this one with part one of the old series so there's no >>>>> chance of a bisection going wrong? >>>> Yes please. >>>> Thanks! >>> Thanks, I'll send a new version shortly. >> Just confirming - the changes to pata_falcon.c will remain as a >> separate patch which should be applied together with the patch that >> will introduce the new platform device, and rewrite the legacy driver >> to use it. That would require Bartlomiej and you to coordinate >> closely. > Bartlomiej already acked both patches, so they can go in through the m68k > tree. > > To avoid bisection regressions, both patches should be merged into a > single patch... > >> If that's too onerous, I can merge the lot and you just ack the m68k >> bits? Please let me know what you'd prefer. > ... and with the falconide.c conversion, all three patches should be merged > into a single patch. > > Gr{oetje,eeting}s, > > Geert >
diff --git a/drivers/ide/falconide.c b/drivers/ide/falconide.c index a5a07cc..d6dd772 100644 --- a/drivers/ide/falconide.c +++ b/drivers/ide/falconide.c @@ -15,6 +15,7 @@ #include <linux/blkdev.h> #include <linux/ide.h> #include <linux/init.h> +#include <linux/platform_device.h> #include <asm/setup.h> #include <asm/atarihw.h> @@ -23,6 +24,7 @@ #include <asm/ide.h> #define DRV_NAME "falconide" +#define DRV_VERSION "0.1.0" /* * Base of the IDE interface @@ -114,18 +116,18 @@ static void falconide_output_data(ide_drive_t *drive, struct ide_cmd *cmd, .chipset = ide_generic, }; -static void __init falconide_setup_ports(struct ide_hw *hw) +static void __init falconide_setup_ports(struct ide_hw *hw, unsigned long base) { int i; memset(hw, 0, sizeof(*hw)); - hw->io_ports.data_addr = ATA_HD_BASE; + hw->io_ports.data_addr = base; for (i = 1; i < 8; i++) - hw->io_ports_array[i] = ATA_HD_BASE + 1 + i * 4; + hw->io_ports_array[i] = base + 1 + i * 4; - hw->io_ports.ctl_addr = ATA_HD_BASE + ATA_HD_CONTROL; + hw->io_ports.ctl_addr = base + ATA_HD_CONTROL; hw->irq = IRQ_MFP_IDE; } @@ -134,23 +136,29 @@ static void __init falconide_setup_ports(struct ide_hw *hw) * Probe for a Falcon IDE interface */ -static int __init falconide_init(void) +static int __init falconide_init(struct platform_device *pdev) { + struct resource *res; struct ide_host *host; struct ide_hw hw, *hws[] = { &hw }; + unsigned long base; int rc; - if (!MACH_IS_ATARI || !ATARIHW_PRESENT(IDE)) - return -ENODEV; + dev_info(&pdev->dev, "Atari Falcon IDE controller\n"); - printk(KERN_INFO "ide: Falcon IDE controller\n"); + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) + return -ENODEV; - if (!request_mem_region(ATA_HD_BASE, 0x40, DRV_NAME)) { - printk(KERN_ERR "%s: resources busy\n", DRV_NAME); + if (!devm_request_mem_region(&pdev->dev, res->start, + resource_size(res), DRV_NAME)) { + dev_err(&pdev->dev, "resources busy\n"); return -EBUSY; } - falconide_setup_ports(&hw); + base = (unsigned long)res->start; + + falconide_setup_ports(&hw, base); host = ide_host_alloc(&falconide_port_info, hws, 1); if (host == NULL) { @@ -169,10 +177,21 @@ static int __init falconide_init(void) err_free: ide_host_free(host); err: - release_mem_region(ATA_HD_BASE, 0x40); + release_mem_region(res->start, resource_size(res)); return rc; } -module_init(falconide_init); +static struct platform_driver ide_falcon_driver = { + .driver = { + .name = "atari-falcon-ide", + }, +}; + +module_platform_driver_probe(ide_falcon_driver, falconide_init); + +MODULE_AUTHOR("Geert Uytterhoeven"); +MODULE_DESCRIPTION("low-level driver for Atari Falcon IDE"); MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:atari-falcon-ide"); +MODULE_VERSION(DRV_VERSION);
With the introduction of a platform device for the Atari Falcon IDE interface, the old Falcon IDE driver no longer loads (resource already claimed by the platform device). Convert falconide driver to use the same platform device that is used by pata_falcon also. Tested (as built-in driver) on my Atari Falcon. Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> CC: Geert Uytterhoeven <geert@linux-m68k.org> --- drivers/ide/falconide.c | 45 ++++++++++++++++++++++++++++++++------------- 1 files changed, 32 insertions(+), 13 deletions(-)