On Mon, Oct 21, 2019 at 11:58:16AM +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 <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>
> ---
> drivers/mfd/cs5535-mfd.c | 72 +++++++++++++++++-----------------------
> 1 file changed, 30 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/mfd/cs5535-mfd.c b/drivers/mfd/cs5535-mfd.c
> index 9ce6bbcdbda1..053e33447808 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,
> },
> [ACPI_BAR] = {
> .name = "cs5535-acpi",
> .num_resources = 1,
> .resources = &cs5535_mfd_resources[ACPI_BAR],
> -
> - .enable = cs5535_mfd_res_enable,
> - .disable = cs5535_mfd_res_disable,
> },
> };
>
> @@ -109,7 +71,6 @@ static int cs5535_mfd_probe(struct pci_dev *pdev,
> if (err)
> return err;
>
> - /* fill in IO range for each cell; subdrivers handle the region */
> for (i = 0; i < NR_BARS; i++) {
> struct mfd_cell *cell = &cs5535_mfd_cells[i];
> struct resource *r = &cs5535_mfd_resources[i];
> @@ -122,22 +83,47 @@ static int cs5535_mfd_probe(struct pci_dev *pdev,
> r->end = pci_resource_end(pdev, i);
> }
>
> + 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 CS5532 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;
> + }
> +
> + 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;
> + }
> + }
Making the request_region() conditional on machine_is_olpc() seems to be
best on the assumption that the cs5535-acpi is not otherwise used.
I suspect the assumption is true but you have to combine knowledge from
several bits of code to figure that out.
Daniel.
>
> 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;
> @@ -145,6 +131,8 @@ static int cs5535_mfd_probe(struct pci_dev *pdev,
>
> static void cs5535_mfd_remove(struct pci_dev *pdev)
> {
> + pci_release_region(pdev, PMS_BAR);
> + pci_release_region(pdev, ACPI_BAR);
> mfd_remove_devices(&pdev->dev);
> pci_disable_device(pdev);
> }
> --
> 2.17.1
>
