[v2,3/9] mfd: cs5535-mfd: Request shared IO regions centrally
diff mbox series

Message ID 20191021105822.20271-4-lee.jones@linaro.org
State New
Headers show
Series
  • Simplify MFD Core
Related show

Commit Message

Lee Jones Oct. 21, 2019, 10:58 a.m. UTC
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 | 72 +++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 42 deletions(-)

Comments

Daniel Thompson Oct. 21, 2019, 12:26 p.m. UTC | #1
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 <daniel.thompson@linaro.org>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  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
>
Lee Jones Oct. 21, 2019, 12:46 p.m. UTC | #2
On Mon, 21 Oct 2019, Daniel Thompson wrote:

> 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 <daniel.thompson@linaro.org>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  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.

It is not used.

Will reply to your other comment.

Patch
diff mbox series

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