Re: [PATCH v5] ata/pata_buddha: Probe via modalias instead of initcall
On 8/20/19 6:42 PM, Max Staudt wrote: > Hi Bartlomiej, > > On 08/20/2019 02:06 PM, Bartlomiej Zolnierkiewicz wrote: >> WARNING: line over 80 characters >> #354: FILE: drivers/ata/pata_buddha.c:287: >> +while ((z = >> zorro_find_device(ZORRO_PROD_INDIVIDUAL_COMPUTERS_X_SURF, z))) { > > I see no good way to shorten this one. I think it's obvious enough to allow > overflowing by a few chars - do you agree? Yes, I agree. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R Institute Poland Samsung Electronics
Re: [PATCH v5] ata/pata_buddha: Probe via modalias instead of initcall
On 8/20/19 5:59 PM, Max Staudt wrote: > Hi Bartlomiej, > > Thank you very much for your review! > > Question below. > > > On 08/20/2019 02:06 PM, Bartlomiej Zolnierkiewicz wrote: >>> + /* Workaround for X-Surf: Save drvdata in case zorro8390 has set it */ >>> + old_drvdata = dev_get_drvdata(>dev); >> >> This should be done only for type == BOARD_XSURF. > > Agreed, as I want to keep unloading functional for Buddha/Catweasel - see > below. > > >>> +static struct zorro_driver pata_buddha_driver = { >>> + .name = "pata_buddha", >>> + .id_table = pata_buddha_zorro_tbl, >>> + .probe = pata_buddha_probe, >>> + .remove = pata_buddha_remove, >> >> I think that we should also add: >> >> .driver = { >> .suppress_bind_attrs = true, >> }, >> >> to prevent the device from being unbinded (and thus ->remove called) >> from the driver using sysfs interface. > > Interesting idea - here's my question now: > > My intention is to allow remove() for boards where we support IDE only > (Buddha, Catweasel) - these are autoprobed via zorro_register_driver(). > This shouldn't affect the X-Surf case, as it's not autoprobed in this way > anyway - and thus pata_buddha_driver isn't even used. > > Am I missing something? We want to inhibit module unloading (hence no > module_exit()), but driver unbinding for Buddha/Catweasel should be fine to > remain, right? Indeed, pata_buddha_driver is not even used for X-Surf so this is not an issue (please disregard my comment about suppress_bind_attrs). >> Please also always check your patches with scripts/checkpatch.pl and >> fix the reported issues: > > Apologies, must've been something in my coffee. I will. > > > Thanks for the review, I'll send a new patch once my question above is > resolved. > > Max Best regards, -- Bartlomiej Zolnierkiewicz Samsung R Institute Poland Samsung Electronics
Re: [PATCH v5] ata/pata_buddha: Probe via modalias instead of initcall
Hi Bartlomiej, On 08/20/2019 02:06 PM, Bartlomiej Zolnierkiewicz wrote: > WARNING: line over 80 characters > #354: FILE: drivers/ata/pata_buddha.c:287: > +while ((z = > zorro_find_device(ZORRO_PROD_INDIVIDUAL_COMPUTERS_X_SURF, z))) { I see no good way to shorten this one. I think it's obvious enough to allow overflowing by a few chars - do you agree? Max
Re: [PATCH v5] ata/pata_buddha: Probe via modalias instead of initcall
Hi Bartlomiej, Thank you very much for your review! Question below. On 08/20/2019 02:06 PM, Bartlomiej Zolnierkiewicz wrote: >> +/* Workaround for X-Surf: Save drvdata in case zorro8390 has set it */ >> +old_drvdata = dev_get_drvdata(>dev); > > This should be done only for type == BOARD_XSURF. Agreed, as I want to keep unloading functional for Buddha/Catweasel - see below. >> +static struct zorro_driver pata_buddha_driver = { >> +.name = "pata_buddha", >> +.id_table = pata_buddha_zorro_tbl, >> +.probe = pata_buddha_probe, >> +.remove = pata_buddha_remove, > > I think that we should also add: > > .driver = { > .suppress_bind_attrs = true, > }, > > to prevent the device from being unbinded (and thus ->remove called) > from the driver using sysfs interface. Interesting idea - here's my question now: My intention is to allow remove() for boards where we support IDE only (Buddha, Catweasel) - these are autoprobed via zorro_register_driver(). This shouldn't affect the X-Surf case, as it's not autoprobed in this way anyway - and thus pata_buddha_driver isn't even used. Am I missing something? We want to inhibit module unloading (hence no module_exit()), but driver unbinding for Buddha/Catweasel should be fine to remain, right? > Please also always check your patches with scripts/checkpatch.pl and > fix the reported issues: Apologies, must've been something in my coffee. I will. Thanks for the review, I'll send a new patch once my question above is resolved. Max
Re: [PATCH v5] ata/pata_buddha: Probe via modalias instead of initcall
Hi Max, Few more review comments below. On 8/12/19 6:48 PM, Max Staudt wrote: > Up until now, the pata_buddha driver would only check for cards on > initcall time. Now, the kernel will call its probe function as soon > as a compatible card is detected. > > v5: Remove module_exit(): There's no good way to handle the X-Surf hack. > Also include a workaround to save X-Surf's drvdata in case zorro8390 > is active. > > v4: Clean up pata_buddha_probe() by using ent->driver_data. > Support X-Surf via late_initcall() > > v3: Clean up devm_*, implement device removal. > > v2: Rename 'zdev' to 'z' to make the patch easy to analyse with > git diff --ignore-space-change > > Signed-off-by: Max Staudt > --- > drivers/ata/pata_buddha.c | 234 > +++--- > 1 file changed, 140 insertions(+), 94 deletions(-) > > diff --git a/drivers/ata/pata_buddha.c b/drivers/ata/pata_buddha.c > index 11a8044ff..6014befc9 100644 > --- a/drivers/ata/pata_buddha.c > +++ b/drivers/ata/pata_buddha.c > @@ -18,7 +18,9 @@ > #include > #include > #include > +#include > #include > +#include > #include > #include > #include > @@ -29,7 +31,7 @@ > #include > > #define DRV_NAME "pata_buddha" > -#define DRV_VERSION "0.1.0" > +#define DRV_VERSION "0.1.1" > > #define BUDDHA_BASE1 0x800 > #define BUDDHA_BASE2 0xa00 > @@ -47,11 +49,11 @@ enum { > BOARD_XSURF > }; > > -static unsigned int buddha_bases[3] __initdata = { > +static unsigned int buddha_bases[3] = { > BUDDHA_BASE1, BUDDHA_BASE2, BUDDHA_BASE3 > }; > > -static unsigned int xsurf_bases[2] __initdata = { > +static unsigned int xsurf_bases[2] = { > XSURF_BASE1, XSURF_BASE2 > }; > > @@ -145,111 +147,155 @@ static struct ata_port_operations pata_xsurf_ops = { > .set_mode = pata_buddha_set_mode, > }; > > -static int __init pata_buddha_init_one(void) > +static int pata_buddha_probe(struct zorro_dev *z, > + const struct zorro_device_id *ent) > { > - struct zorro_dev *z = NULL; > - > - while ((z = zorro_find_device(ZORRO_WILDCARD, z))) { > - static const char *board_name[] > - = { "Buddha", "Catweasel", "X-Surf" }; > - struct ata_host *host; > - void __iomem *buddha_board; > - unsigned long board; > - unsigned int type, nr_ports = 2; > - int i; > - > - if (z->id == ZORRO_PROD_INDIVIDUAL_COMPUTERS_BUDDHA) { > - type = BOARD_BUDDHA; > - } else if (z->id == ZORRO_PROD_INDIVIDUAL_COMPUTERS_CATWEASEL) { > - type = BOARD_CATWEASEL; > - nr_ports++; > - } else if (z->id == ZORRO_PROD_INDIVIDUAL_COMPUTERS_X_SURF) { > - type = BOARD_XSURF; > - } else > - continue; > - > - dev_info(>dev, "%s IDE controller\n", board_name[type]); > - > - board = z->resource.start; > + static const char * const board_name[] > + = { "Buddha", "Catweasel", "X-Surf" }; > + struct ata_host *host; > + void __iomem *buddha_board; > + unsigned long board; > + unsigned int type = ent->driver_data; > + unsigned int nr_ports = (type == BOARD_CATWEASEL) ? 3 : 2; > + void *old_drvdata; > + int i; > + > + dev_info(>dev, "%s IDE controller\n", board_name[type]); > + > + board = z->resource.start; > + > + if (type != BOARD_XSURF) { > + if (!devm_request_mem_region(>dev, > + board + BUDDHA_BASE1, > + 0x800, DRV_NAME)) > + return -ENXIO; > + } else { > + if (!devm_request_mem_region(>dev, > + board + XSURF_BASE1, > + 0x1000, DRV_NAME)) > + return -ENXIO; > + if (!devm_request_mem_region(>dev, > + board + XSURF_BASE2, > + 0x1000, DRV_NAME)) { > + } > + } > + > + /* Workaround for X-Surf: Save drvdata in case zorro8390 has set it */ > + old_drvdata = dev_get_drvdata(>dev); This should be done only for type == BOARD_XSURF. > + /* allocate host */ > + host = ata_host_alloc(>dev, nr_ports); > + dev_set_drvdata(>dev, old_drvdata); ditto > + if (!host) > + return -ENXIO; > + > + > + buddha_board = ZTWO_VADDR(board); > + > + /* enable the board IRQ on Buddha/Catweasel */ > + if (type != BOARD_XSURF) > + z_writeb(0, buddha_board + BUDDHA_IRQ_MR); > + > + for (i = 0; i < nr_ports; i++) { > + struct ata_port *ap = host->ports[i]; > + void __iomem *base, *irqport; > + unsigned long ctl = 0; > > if (type != BOARD_XSURF) { > -
Re: [PATCH v5] ata/pata_buddha: Probe via modalias instead of initcall
On 08/12/2019 06:48 PM, Max Staudt wrote: > +/* > + * We cannot have a modalias for X-Surf boards, as it competes with the > + * zorro8390 network driver. As a stopgap measure until we have proper > + * MFC support for this board, we manually attach to it late after Zorro > + * has enumerated its boards. > + */ I spotted a typo here, s/MFC/MFD/ Max
[PATCH v5] ata/pata_buddha: Probe via modalias instead of initcall
Up until now, the pata_buddha driver would only check for cards on initcall time. Now, the kernel will call its probe function as soon as a compatible card is detected. v5: Remove module_exit(): There's no good way to handle the X-Surf hack. Also include a workaround to save X-Surf's drvdata in case zorro8390 is active. v4: Clean up pata_buddha_probe() by using ent->driver_data. Support X-Surf via late_initcall() v3: Clean up devm_*, implement device removal. v2: Rename 'zdev' to 'z' to make the patch easy to analyse with git diff --ignore-space-change Signed-off-by: Max Staudt --- drivers/ata/pata_buddha.c | 234 +++--- 1 file changed, 140 insertions(+), 94 deletions(-) diff --git a/drivers/ata/pata_buddha.c b/drivers/ata/pata_buddha.c index 11a8044ff..6014befc9 100644 --- a/drivers/ata/pata_buddha.c +++ b/drivers/ata/pata_buddha.c @@ -18,7 +18,9 @@ #include #include #include +#include #include +#include #include #include #include @@ -29,7 +31,7 @@ #include #define DRV_NAME "pata_buddha" -#define DRV_VERSION "0.1.0" +#define DRV_VERSION "0.1.1" #define BUDDHA_BASE1 0x800 #define BUDDHA_BASE2 0xa00 @@ -47,11 +49,11 @@ enum { BOARD_XSURF }; -static unsigned int buddha_bases[3] __initdata = { +static unsigned int buddha_bases[3] = { BUDDHA_BASE1, BUDDHA_BASE2, BUDDHA_BASE3 }; -static unsigned int xsurf_bases[2] __initdata = { +static unsigned int xsurf_bases[2] = { XSURF_BASE1, XSURF_BASE2 }; @@ -145,111 +147,155 @@ static struct ata_port_operations pata_xsurf_ops = { .set_mode = pata_buddha_set_mode, }; -static int __init pata_buddha_init_one(void) +static int pata_buddha_probe(struct zorro_dev *z, +const struct zorro_device_id *ent) { - struct zorro_dev *z = NULL; - - while ((z = zorro_find_device(ZORRO_WILDCARD, z))) { - static const char *board_name[] - = { "Buddha", "Catweasel", "X-Surf" }; - struct ata_host *host; - void __iomem *buddha_board; - unsigned long board; - unsigned int type, nr_ports = 2; - int i; - - if (z->id == ZORRO_PROD_INDIVIDUAL_COMPUTERS_BUDDHA) { - type = BOARD_BUDDHA; - } else if (z->id == ZORRO_PROD_INDIVIDUAL_COMPUTERS_CATWEASEL) { - type = BOARD_CATWEASEL; - nr_ports++; - } else if (z->id == ZORRO_PROD_INDIVIDUAL_COMPUTERS_X_SURF) { - type = BOARD_XSURF; - } else - continue; - - dev_info(>dev, "%s IDE controller\n", board_name[type]); - - board = z->resource.start; + static const char * const board_name[] + = { "Buddha", "Catweasel", "X-Surf" }; + struct ata_host *host; + void __iomem *buddha_board; + unsigned long board; + unsigned int type = ent->driver_data; + unsigned int nr_ports = (type == BOARD_CATWEASEL) ? 3 : 2; + void *old_drvdata; + int i; + + dev_info(>dev, "%s IDE controller\n", board_name[type]); + + board = z->resource.start; + + if (type != BOARD_XSURF) { + if (!devm_request_mem_region(>dev, +board + BUDDHA_BASE1, +0x800, DRV_NAME)) + return -ENXIO; + } else { + if (!devm_request_mem_region(>dev, +board + XSURF_BASE1, +0x1000, DRV_NAME)) + return -ENXIO; + if (!devm_request_mem_region(>dev, +board + XSURF_BASE2, +0x1000, DRV_NAME)) { + } + } + + /* Workaround for X-Surf: Save drvdata in case zorro8390 has set it */ + old_drvdata = dev_get_drvdata(>dev); + + /* allocate host */ + host = ata_host_alloc(>dev, nr_ports); + dev_set_drvdata(>dev, old_drvdata); + if (!host) + return -ENXIO; + + + buddha_board = ZTWO_VADDR(board); + + /* enable the board IRQ on Buddha/Catweasel */ + if (type != BOARD_XSURF) + z_writeb(0, buddha_board + BUDDHA_IRQ_MR); + + for (i = 0; i < nr_ports; i++) { + struct ata_port *ap = host->ports[i]; + void __iomem *base, *irqport; + unsigned long ctl = 0; if (type != BOARD_XSURF) { - if (!devm_request_mem_region(>dev, -board + BUDDHA_BASE1, -0x800, DRV_NAME)) - continue; + ap->ops = _buddha_ops; +