Messages in this thread | | | Date | Fri, 25 Oct 2019 09:50:17 +0100 | From | Daniel Thompson <> | Subject | Re: [PATCH v3 03/10] mfd: cs5535-mfd: Request shared IO regions centrally |
| |
On Thu, Oct 24, 2019 at 05:38:25PM +0100, Lee Jones wrote: > Prior to this patch, IO regions were requested via an MFD subsytem-level > .enable() call-back and similarly released by a .disable() call-back. > Double requests/releases were avoided by a centrally handled usage count > mechanism. > > This complexity can all be avoided by handling IO regions only once during > .probe() and .remove() of the parent device. Since this is the only > legitimate user of the aforementioned usage count mechanism, this patch > will allow it to be removed from MFD core in subsequent steps. > > Suggested-by: Daniel Thompson <daniel.thompson@linaro.org> > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > drivers/mfd/cs5535-mfd.c | 71 +++++++++++++++++----------------------- > 1 file changed, 30 insertions(+), 41 deletions(-) > > diff --git a/drivers/mfd/cs5535-mfd.c b/drivers/mfd/cs5535-mfd.c > index b35f1efa01f6..27fa8fa1ec9b 100644 > --- a/drivers/mfd/cs5535-mfd.c > +++ b/drivers/mfd/cs5535-mfd.c > @@ -27,38 +27,6 @@ enum cs5535_mfd_bars { > NR_BARS, > }; > > -static int cs5535_mfd_res_enable(struct platform_device *pdev) > -{ > - struct resource *res; > - > - res = platform_get_resource(pdev, IORESOURCE_IO, 0); > - if (!res) { > - dev_err(&pdev->dev, "can't fetch device resource info\n"); > - return -EIO; > - } > - > - if (!request_region(res->start, resource_size(res), DRV_NAME)) { > - dev_err(&pdev->dev, "can't request region\n"); > - return -EIO; > - } > - > - return 0; > -} > - > -static int cs5535_mfd_res_disable(struct platform_device *pdev) > -{ > - struct resource *res; > - > - res = platform_get_resource(pdev, IORESOURCE_IO, 0); > - if (!res) { > - dev_err(&pdev->dev, "can't fetch device resource info\n"); > - return -EIO; > - } > - > - release_region(res->start, resource_size(res)); > - return 0; > -} > - > static struct resource cs5535_mfd_resources[NR_BARS]; > > static struct mfd_cell cs5535_mfd_cells[] = { > @@ -81,17 +49,11 @@ static struct mfd_cell cs5535_mfd_cells[] = { > .name = "cs5535-pms", > .num_resources = 1, > .resources = &cs5535_mfd_resources[PMS_BAR], > - > - .enable = cs5535_mfd_res_enable, > - .disable = cs5535_mfd_res_disable, > }, > { > .name = "cs5535-acpi", > .num_resources = 1, > .resources = &cs5535_mfd_resources[ACPI_BAR], > - > - .enable = cs5535_mfd_res_enable, > - .disable = cs5535_mfd_res_disable, > }, > }; > > @@ -117,22 +79,47 @@ static int cs5535_mfd_probe(struct pci_dev *pdev, > r->end = pci_resource_end(pdev, bar); > } > > + err = pci_request_region(pdev, PMS_BAR, DRV_NAME); > + if (err) { > + dev_err(&pdev->dev, "Failed to request PMS_BAR's IO region\n"); > + goto err_disable; > + } > + > err = mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, cs5535_mfd_cells, > ARRAY_SIZE(cs5535_mfd_cells), NULL, 0, NULL); > if (err) { > dev_err(&pdev->dev, > "Failed to add CS5535 sub-devices: %d\n", err); > - goto err_disable; > + goto err_release_pms; > } > > - if (machine_is_olpc()) > - mfd_clone_cell("cs5535-acpi", olpc_acpi_clones, ARRAY_SIZE(olpc_acpi_clones)); > + if (machine_is_olpc()) { > + err = pci_request_region(pdev, ACPI_BAR, DRV_NAME); > + if (err) { > + dev_err(&pdev->dev, > + "Failed to request ACPI_BAR's IO region\n"); > + goto err_remove_devices; > + }
I agree cs5535-acpi isn't used is the kernel but I think it stills fails a w.t.f. per minute test to have a mismatch between when a device is added and when it requests resources.
Especially since I don't think this is transient within the patch series.
> + > + err = mfd_clone_cell("cs5535-acpi", olpc_acpi_clones, > + ARRAY_SIZE(olpc_acpi_clones)); > + if (err) { > + dev_err(&pdev->dev, "Failed to clone MFD cell\n"); > + goto err_release_acpi; > + } > + } > > dev_info(&pdev->dev, "%zu devices registered.\n", > ARRAY_SIZE(cs5535_mfd_cells)); > > return 0; > > +err_release_acpi: > + pci_release_region(pdev, ACPI_BAR); > +err_remove_devices: > + mfd_remove_devices(&pdev->dev); > +err_release_pms: > + pci_release_region(pdev, PMS_BAR); > err_disable: > pci_disable_device(pdev); > return err; > @@ -141,6 +128,8 @@ static int cs5535_mfd_probe(struct pci_dev *pdev, > static void cs5535_mfd_remove(struct pci_dev *pdev) > { > mfd_remove_devices(&pdev->dev); > + pci_release_region(pdev, ACPI_BAR);
This will issue a warning if !machine_is_olpc() .
For the release region family of calls "the described resource region must match a currently busy region."
Daniel.
| |