MFD currently has one over-complicated user. CS5535 uses a mixture of
cell cloning, reference counting and subsystem-level call-backs to
achieve its goal of requesting an IO memory region only once across 3
consumers. The same can be achieved by handling the region centrally
during the parent device's .probe() sequence. Releasing can be handed
in a similar way during .remove().
While we're here, take the opportunity to provide some clean-ups and
error checking to issues noticed along the way.
This also paves the way for clean cell disabling via Device Tree being
discussed at [0]
[0] https://lkml.org/lkml/2019/10/18/612.
Lee Jones (10):
mfd: cs5535-mfd: Use PLATFORM_DEVID_* defines and tidy error message
mfd: cs5535-mfd: Remove mfd_cell->id hack
mfd: cs5535-mfd: Request shared IO regions centrally
mfd: cs5535-mfd: Register clients using their own dedicated MFD cell
entries
mfd: mfd-core: Remove mfd_clone_cell()
x86: olpc-xo1-pm: Remove invocation of MFD's .enable()/.disable()
call-backs
x86: olpc-xo1-sci: Remove invocation of MFD's .enable()/.disable()
call-backs
mfd: mfd-core: Protect against NULL call-back function pointer
mfd: mfd-core: Remove usage counting for .{en,dis}able() call-backs
mfd: mfd-core: Move pdev->mfd_cell creation back into mfd_add_device()
arch/x86/platform/olpc/olpc-xo1-pm.c | 8 --
arch/x86/platform/olpc/olpc-xo1-sci.c | 6 --
drivers/mfd/cs5535-mfd.c | 105 +++++++++++-------------
drivers/mfd/mfd-core.c | 113 +++++---------------------
include/linux/mfd/core.h | 20 -----
5 files changed, 65 insertions(+), 187 deletions(-)
--
2.17.1
In most contexts '-1' doesn't really mean much to the casual observer.
In almost all cases, it's better to use a human readable define. In
this case PLATFORM_DEVID_* defines have already been provided for this
purpose.
While we're here, let's be specific about the 'MFD devices' which
failed. It will help when trying to distinguish which of the 2 sets
of sub-devices we actually failed to register.
Signed-off-by: Lee Jones <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
---
drivers/mfd/cs5535-mfd.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/mfd/cs5535-mfd.c b/drivers/mfd/cs5535-mfd.c
index f1825c0ccbd0..cda7f5b942e7 100644
--- a/drivers/mfd/cs5535-mfd.c
+++ b/drivers/mfd/cs5535-mfd.c
@@ -127,10 +127,11 @@ static int cs5535_mfd_probe(struct pci_dev *pdev,
cs5535_mfd_cells[i].id = 0;
}
- err = mfd_add_devices(&pdev->dev, -1, cs5535_mfd_cells,
+ 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, "MFD add devices failed: %d\n", err);
+ dev_err(&pdev->dev,
+ "Failed to add CS5535 sub-devices: %d\n", err);
goto err_disable;
}
--
2.17.1
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 | 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;
+ }
+
+ 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);
+ pci_release_region(pdev, PMS_BAR);
pci_disable_device(pdev);
}
--
2.17.1
IO regions are now requested and released by this device's parent.
Signed-off-by: Lee Jones <[email protected]>
---
arch/x86/platform/olpc/olpc-xo1-pm.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/arch/x86/platform/olpc/olpc-xo1-pm.c b/arch/x86/platform/olpc/olpc-xo1-pm.c
index e1a32062a375..f067ac780ba7 100644
--- a/arch/x86/platform/olpc/olpc-xo1-pm.c
+++ b/arch/x86/platform/olpc/olpc-xo1-pm.c
@@ -12,7 +12,6 @@
#include <linux/platform_device.h>
#include <linux/export.h>
#include <linux/pm.h>
-#include <linux/mfd/core.h>
#include <linux/suspend.h>
#include <linux/olpc-ec.h>
@@ -120,16 +119,11 @@ static const struct platform_suspend_ops xo1_suspend_ops = {
static int xo1_pm_probe(struct platform_device *pdev)
{
struct resource *res;
- int err;
/* don't run on non-XOs */
if (!machine_is_olpc())
return -ENODEV;
- err = mfd_cell_enable(pdev);
- if (err)
- return err;
-
res = platform_get_resource(pdev, IORESOURCE_IO, 0);
if (!res) {
dev_err(&pdev->dev, "can't fetch device resource info\n");
@@ -152,8 +146,6 @@ static int xo1_pm_probe(struct platform_device *pdev)
static int xo1_pm_remove(struct platform_device *pdev)
{
- mfd_cell_disable(pdev);
-
if (strcmp(pdev->name, "cs5535-pms") == 0)
pms_base = 0;
else if (strcmp(pdev->name, "olpc-xo1-pm-acpi") == 0)
--
2.17.1
CS5535 is the only user of mfd_clone_cell(). It makes more sense to
register child devices in the traditional way and remove the quite
bespoke mfd_clone_cell() call from the MFD API.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/mfd/cs5535-mfd.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/drivers/mfd/cs5535-mfd.c b/drivers/mfd/cs5535-mfd.c
index 27fa8fa1ec9b..4c034c9f2303 100644
--- a/drivers/mfd/cs5535-mfd.c
+++ b/drivers/mfd/cs5535-mfd.c
@@ -50,16 +50,19 @@ static struct mfd_cell cs5535_mfd_cells[] = {
.num_resources = 1,
.resources = &cs5535_mfd_resources[PMS_BAR],
},
+};
+
+static struct mfd_cell cs5535_olpc_mfd_cells[] = {
{
- .name = "cs5535-acpi",
+ .name = "olpc-xo1-pm-acpi",
+ .num_resources = 1,
+ .resources = &cs5535_mfd_resources[ACPI_BAR],
+ },
+ {
+ .name = "olpc-xo1-sci-acpi",
.num_resources = 1,
.resources = &cs5535_mfd_resources[ACPI_BAR],
},
-};
-
-static const char *olpc_acpi_clones[] = {
- "olpc-xo1-pm-acpi",
- "olpc-xo1-sci-acpi"
};
static int cs5535_mfd_probe(struct pci_dev *pdev,
@@ -101,10 +104,14 @@ static int cs5535_mfd_probe(struct pci_dev *pdev,
goto err_remove_devices;
}
- err = mfd_clone_cell("cs5535-acpi", olpc_acpi_clones,
- ARRAY_SIZE(olpc_acpi_clones));
+ err = mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
+ cs5535_olpc_mfd_cells,
+ ARRAY_SIZE(cs5535_olpc_mfd_cells),
+ NULL, 0, NULL);
if (err) {
- dev_err(&pdev->dev, "Failed to clone MFD cell\n");
+ dev_err(&pdev->dev,
+ "Failed to add CS5535 OLPC sub-devices: %d\n",
+ err);
goto err_release_acpi;
}
}
--
2.17.1
IO regions are now requested and released by this device's parent.
Signed-off-by: Lee Jones <[email protected]>
---
arch/x86/platform/olpc/olpc-xo1-sci.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/arch/x86/platform/olpc/olpc-xo1-sci.c b/arch/x86/platform/olpc/olpc-xo1-sci.c
index 99a28ce2244c..933dd4fe3a97 100644
--- a/arch/x86/platform/olpc/olpc-xo1-sci.c
+++ b/arch/x86/platform/olpc/olpc-xo1-sci.c
@@ -15,7 +15,6 @@
#include <linux/platform_device.h>
#include <linux/pm.h>
#include <linux/pm_wakeup.h>
-#include <linux/mfd/core.h>
#include <linux/power_supply.h>
#include <linux/suspend.h>
#include <linux/workqueue.h>
@@ -537,10 +536,6 @@ static int xo1_sci_probe(struct platform_device *pdev)
if (!machine_is_olpc())
return -ENODEV;
- r = mfd_cell_enable(pdev);
- if (r)
- return r;
-
res = platform_get_resource(pdev, IORESOURCE_IO, 0);
if (!res) {
dev_err(&pdev->dev, "can't fetch device resource info\n");
@@ -605,7 +600,6 @@ static int xo1_sci_probe(struct platform_device *pdev)
static int xo1_sci_remove(struct platform_device *pdev)
{
- mfd_cell_disable(pdev);
free_irq(sci_irq, pdev);
cancel_work_sync(&sci_work);
free_ec_sci();
--
2.17.1
If a child device calls mfd_cell_{en,dis}able() without an appropriate
call-back being set, we are likely to encounter a panic. Avoid this
by adding suitable checking.
Signed-off-by: Lee Jones <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
---
drivers/mfd/mfd-core.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 8126665bb2d8..e38e411ca775 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -28,6 +28,11 @@ int mfd_cell_enable(struct platform_device *pdev)
const struct mfd_cell *cell = mfd_get_cell(pdev);
int err = 0;
+ if (!cell->enable) {
+ dev_dbg(&pdev->dev, "No .enable() call-back registered\n");
+ return 0;
+ }
+
/* only call enable hook if the cell wasn't previously enabled */
if (atomic_inc_return(cell->usage_count) == 1)
err = cell->enable(pdev);
@@ -45,6 +50,11 @@ int mfd_cell_disable(struct platform_device *pdev)
const struct mfd_cell *cell = mfd_get_cell(pdev);
int err = 0;
+ if (!cell->disable) {
+ dev_dbg(&pdev->dev, "No .disable() call-back registered\n");
+ return 0;
+ }
+
/* only disable if no other clients are using it */
if (atomic_dec_return(cell->usage_count) == 0)
err = cell->disable(pdev);
--
2.17.1
Providing a subsystem-level API helper seems over-kill just to save a
few lines of C-code. Previous commits saw us convert mfd_clone_cell()'s
only user over to use a more traditional style of MFD child-device
registration. Now we can remove the superfluous helper from the MFD API.
Signed-off-by: Lee Jones <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
---
drivers/mfd/mfd-core.c | 33 ---------------------------------
include/linux/mfd/core.h | 18 ------------------
2 files changed, 51 deletions(-)
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 23276a80e3b4..8126665bb2d8 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -382,38 +382,5 @@ int devm_mfd_add_devices(struct device *dev, int id,
}
EXPORT_SYMBOL(devm_mfd_add_devices);
-int mfd_clone_cell(const char *cell, const char **clones, size_t n_clones)
-{
- struct mfd_cell cell_entry;
- struct device *dev;
- struct platform_device *pdev;
- int i;
-
- /* fetch the parent cell's device (should already be registered!) */
- dev = bus_find_device_by_name(&platform_bus_type, NULL, cell);
- if (!dev) {
- printk(KERN_ERR "failed to find device for cell %s\n", cell);
- return -ENODEV;
- }
- pdev = to_platform_device(dev);
- memcpy(&cell_entry, mfd_get_cell(pdev), sizeof(cell_entry));
-
- WARN_ON(!cell_entry.enable);
-
- for (i = 0; i < n_clones; i++) {
- cell_entry.name = clones[i];
- /* don't give up if a single call fails; just report error */
- if (mfd_add_device(pdev->dev.parent, -1, &cell_entry,
- cell_entry.usage_count, NULL, 0, NULL))
- dev_err(dev, "failed to create platform device '%s'\n",
- clones[i]);
- }
-
- put_device(dev);
-
- return 0;
-}
-EXPORT_SYMBOL(mfd_clone_cell);
-
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Ian Molton, Dmitry Baryshkov");
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index b43fc5773ad7..bd8c0e089164 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -86,24 +86,6 @@ struct mfd_cell {
extern int mfd_cell_enable(struct platform_device *pdev);
extern int mfd_cell_disable(struct platform_device *pdev);
-/*
- * "Clone" multiple platform devices for a single cell. This is to be used
- * for devices that have multiple users of a cell. For example, if an mfd
- * driver wants the cell "foo" to be used by a GPIO driver, an MTD driver,
- * and a platform driver, the following bit of code would be use after first
- * calling mfd_add_devices():
- *
- * const char *fclones[] = { "foo-gpio", "foo-mtd" };
- * err = mfd_clone_cells("foo", fclones, ARRAY_SIZE(fclones));
- *
- * Each driver (MTD, GPIO, and platform driver) would then register
- * platform_drivers for "foo-mtd", "foo-gpio", and "foo", respectively.
- * The cell's .enable/.disable hooks should be used to deal with hardware
- * resource contention.
- */
-extern int mfd_clone_cell(const char *cell, const char **clones,
- size_t n_clones);
-
/*
* Given a platform device that's been created by mfd_add_devices(), fetch
* the mfd_cell that created it.
--
2.17.1
The MFD implementation for reference counting was complex and unnecessary.
There was only one bona fide user which has now been converted to handle
the process in a different way. Any future resource protection, shared
enablement functions should be handed by the parent device, rather than
through the MFD subsystem API.
Signed-off-by: Lee Jones <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
---
drivers/mfd/mfd-core.c | 57 +++++++---------------------------------
include/linux/mfd/core.h | 2 --
2 files changed, 9 insertions(+), 50 deletions(-)
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index e38e411ca775..2535dd3605c0 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -26,53 +26,31 @@ static struct device_type mfd_dev_type = {
int mfd_cell_enable(struct platform_device *pdev)
{
const struct mfd_cell *cell = mfd_get_cell(pdev);
- int err = 0;
if (!cell->enable) {
dev_dbg(&pdev->dev, "No .enable() call-back registered\n");
return 0;
}
- /* only call enable hook if the cell wasn't previously enabled */
- if (atomic_inc_return(cell->usage_count) == 1)
- err = cell->enable(pdev);
-
- /* if the enable hook failed, decrement counter to allow retries */
- if (err)
- atomic_dec(cell->usage_count);
-
- return err;
+ return cell->enable(pdev);
}
EXPORT_SYMBOL(mfd_cell_enable);
int mfd_cell_disable(struct platform_device *pdev)
{
const struct mfd_cell *cell = mfd_get_cell(pdev);
- int err = 0;
if (!cell->disable) {
dev_dbg(&pdev->dev, "No .disable() call-back registered\n");
return 0;
}
- /* only disable if no other clients are using it */
- if (atomic_dec_return(cell->usage_count) == 0)
- err = cell->disable(pdev);
-
- /* if the disable hook failed, increment to allow retries */
- if (err)
- atomic_inc(cell->usage_count);
-
- /* sanity check; did someone call disable too many times? */
- WARN_ON(atomic_read(cell->usage_count) < 0);
-
- return err;
+ return cell->disable(pdev);
}
EXPORT_SYMBOL(mfd_cell_disable);
static int mfd_platform_add_cell(struct platform_device *pdev,
- const struct mfd_cell *cell,
- atomic_t *usage_count)
+ const struct mfd_cell *cell)
{
if (!cell)
return 0;
@@ -81,7 +59,6 @@ static int mfd_platform_add_cell(struct platform_device *pdev,
if (!pdev->mfd_cell)
return -ENOMEM;
- pdev->mfd_cell->usage_count = usage_count;
return 0;
}
@@ -144,7 +121,7 @@ static inline void mfd_acpi_add_device(const struct mfd_cell *cell,
#endif
static int mfd_add_device(struct device *parent, int id,
- const struct mfd_cell *cell, atomic_t *usage_count,
+ const struct mfd_cell *cell,
struct resource *mem_base,
int irq_base, struct irq_domain *domain)
{
@@ -206,7 +183,7 @@ static int mfd_add_device(struct device *parent, int id,
goto fail_alias;
}
- ret = mfd_platform_add_cell(pdev, cell, usage_count);
+ ret = mfd_platform_add_cell(pdev, cell);
if (ret)
goto fail_alias;
@@ -296,16 +273,9 @@ int mfd_add_devices(struct device *parent, int id,
{
int i;
int ret;
- atomic_t *cnts;
-
- /* initialize reference counting for all cells */
- cnts = kcalloc(n_devs, sizeof(*cnts), GFP_KERNEL);
- if (!cnts)
- return -ENOMEM;
for (i = 0; i < n_devs; i++) {
- atomic_set(&cnts[i], 0);
- ret = mfd_add_device(parent, id, cells + i, cnts + i, mem_base,
+ ret = mfd_add_device(parent, id, cells + i, mem_base,
irq_base, domain);
if (ret)
goto fail;
@@ -316,17 +286,15 @@ int mfd_add_devices(struct device *parent, int id,
fail:
if (i)
mfd_remove_devices(parent);
- else
- kfree(cnts);
+
return ret;
}
EXPORT_SYMBOL(mfd_add_devices);
-static int mfd_remove_devices_fn(struct device *dev, void *c)
+static int mfd_remove_devices_fn(struct device *dev, void *data)
{
struct platform_device *pdev;
const struct mfd_cell *cell;
- atomic_t **usage_count = c;
if (dev->type != &mfd_dev_type)
return 0;
@@ -337,20 +305,13 @@ static int mfd_remove_devices_fn(struct device *dev, void *c)
regulator_bulk_unregister_supply_alias(dev, cell->parent_supplies,
cell->num_parent_supplies);
- /* find the base address of usage_count pointers (for freeing) */
- if (!*usage_count || (cell->usage_count < *usage_count))
- *usage_count = cell->usage_count;
-
platform_device_unregister(pdev);
return 0;
}
void mfd_remove_devices(struct device *parent)
{
- atomic_t *cnts = NULL;
-
- device_for_each_child_reverse(parent, &cnts, mfd_remove_devices_fn);
- kfree(cnts);
+ device_for_each_child_reverse(parent, NULL, mfd_remove_devices_fn);
}
EXPORT_SYMBOL(mfd_remove_devices);
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index bd8c0e089164..919f09fb07b7 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -30,8 +30,6 @@ struct mfd_cell {
const char *name;
int id;
- /* refcounting for multiple drivers to use a single cell */
- atomic_t *usage_count;
int (*enable)(struct platform_device *dev);
int (*disable)(struct platform_device *dev);
--
2.17.1
On Thu, Oct 24, 2019 at 05:38:30PM +0100, Lee Jones wrote:
> If a child device calls mfd_cell_{en,dis}able() without an appropriate
> call-back being set, we are likely to encounter a panic. Avoid this
> by adding suitable checking.
>
> Signed-off-by: Lee Jones <[email protected]>
> Reviewed-by: Daniel Thompson <[email protected]>
Shouldn't this be earlier in the patch set (to avoid transient
regressions)?
Daniel.
On Thu, Oct 24, 2019 at 05:38:29PM +0100, Lee Jones wrote:
> IO regions are now requested and released by this device's parent.
>
> Signed-off-by: Lee Jones <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
> ---
> arch/x86/platform/olpc/olpc-xo1-sci.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/arch/x86/platform/olpc/olpc-xo1-sci.c b/arch/x86/platform/olpc/olpc-xo1-sci.c
> index 99a28ce2244c..933dd4fe3a97 100644
> --- a/arch/x86/platform/olpc/olpc-xo1-sci.c
> +++ b/arch/x86/platform/olpc/olpc-xo1-sci.c
> @@ -15,7 +15,6 @@
> #include <linux/platform_device.h>
> #include <linux/pm.h>
> #include <linux/pm_wakeup.h>
> -#include <linux/mfd/core.h>
> #include <linux/power_supply.h>
> #include <linux/suspend.h>
> #include <linux/workqueue.h>
> @@ -537,10 +536,6 @@ static int xo1_sci_probe(struct platform_device *pdev)
> if (!machine_is_olpc())
> return -ENODEV;
>
> - r = mfd_cell_enable(pdev);
> - if (r)
> - return r;
> -
> res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> if (!res) {
> dev_err(&pdev->dev, "can't fetch device resource info\n");
> @@ -605,7 +600,6 @@ static int xo1_sci_probe(struct platform_device *pdev)
>
> static int xo1_sci_remove(struct platform_device *pdev)
> {
> - mfd_cell_disable(pdev);
> free_irq(sci_irq, pdev);
> cancel_work_sync(&sci_work);
> free_ec_sci();
> --
> 2.17.1
>
On Thu, Oct 24, 2019 at 05:38:30PM +0100, Lee Jones wrote:
> If a child device calls mfd_cell_{en,dis}able() without an appropriate
> call-back being set, we are likely to encounter a panic. Avoid this
> by adding suitable checking.
Reviwed-by: Mark Brown <[email protected]>
On Thu, Oct 24, 2019 at 05:38:31PM +0100, Lee Jones wrote:
> The MFD implementation for reference counting was complex and unnecessary.
> There was only one bona fide user which has now been converted to handle
> the process in a different way. Any future resource protection, shared
> enablement functions should be handed by the parent device, rather than
> through the MFD subsystem API.
Reviewed-by: Mark Brown <[email protected]>
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 <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>
> ---
> 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.
On Thu, Oct 24, 2019 at 05:38:26PM +0100, Lee Jones wrote:
> CS5535 is the only user of mfd_clone_cell(). It makes more sense to
> register child devices in the traditional way and remove the quite
> bespoke mfd_clone_cell() call from the MFD API.
>
> Signed-off-by: Lee Jones <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
> ---
> drivers/mfd/cs5535-mfd.c | 25 ++++++++++++++++---------
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mfd/cs5535-mfd.c b/drivers/mfd/cs5535-mfd.c
> index 27fa8fa1ec9b..4c034c9f2303 100644
> --- a/drivers/mfd/cs5535-mfd.c
> +++ b/drivers/mfd/cs5535-mfd.c
> @@ -50,16 +50,19 @@ static struct mfd_cell cs5535_mfd_cells[] = {
> .num_resources = 1,
> .resources = &cs5535_mfd_resources[PMS_BAR],
> },
> +};
> +
> +static struct mfd_cell cs5535_olpc_mfd_cells[] = {
> {
> - .name = "cs5535-acpi",
> + .name = "olpc-xo1-pm-acpi",
> + .num_resources = 1,
> + .resources = &cs5535_mfd_resources[ACPI_BAR],
> + },
> + {
> + .name = "olpc-xo1-sci-acpi",
> .num_resources = 1,
> .resources = &cs5535_mfd_resources[ACPI_BAR],
> },
> -};
> -
> -static const char *olpc_acpi_clones[] = {
> - "olpc-xo1-pm-acpi",
> - "olpc-xo1-sci-acpi"
> };
>
> static int cs5535_mfd_probe(struct pci_dev *pdev,
> @@ -101,10 +104,14 @@ static int cs5535_mfd_probe(struct pci_dev *pdev,
> goto err_remove_devices;
> }
>
> - err = mfd_clone_cell("cs5535-acpi", olpc_acpi_clones,
> - ARRAY_SIZE(olpc_acpi_clones));
> + err = mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
> + cs5535_olpc_mfd_cells,
> + ARRAY_SIZE(cs5535_olpc_mfd_cells),
> + NULL, 0, NULL);
> if (err) {
> - dev_err(&pdev->dev, "Failed to clone MFD cell\n");
> + dev_err(&pdev->dev,
> + "Failed to add CS5535 OLPC sub-devices: %d\n",
> + err);
> goto err_release_acpi;
> }
> }
> --
> 2.17.1
>
On Thu, Oct 24, 2019 at 05:38:28PM +0100, Lee Jones wrote:
> IO regions are now requested and released by this device's parent.
>
> Signed-off-by: Lee Jones <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
> ---
> arch/x86/platform/olpc/olpc-xo1-pm.c | 8 --------
> 1 file changed, 8 deletions(-)
>
> diff --git a/arch/x86/platform/olpc/olpc-xo1-pm.c b/arch/x86/platform/olpc/olpc-xo1-pm.c
> index e1a32062a375..f067ac780ba7 100644
> --- a/arch/x86/platform/olpc/olpc-xo1-pm.c
> +++ b/arch/x86/platform/olpc/olpc-xo1-pm.c
> @@ -12,7 +12,6 @@
> #include <linux/platform_device.h>
> #include <linux/export.h>
> #include <linux/pm.h>
> -#include <linux/mfd/core.h>
> #include <linux/suspend.h>
> #include <linux/olpc-ec.h>
>
> @@ -120,16 +119,11 @@ static const struct platform_suspend_ops xo1_suspend_ops = {
> static int xo1_pm_probe(struct platform_device *pdev)
> {
> struct resource *res;
> - int err;
>
> /* don't run on non-XOs */
> if (!machine_is_olpc())
> return -ENODEV;
>
> - err = mfd_cell_enable(pdev);
> - if (err)
> - return err;
> -
> res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> if (!res) {
> dev_err(&pdev->dev, "can't fetch device resource info\n");
> @@ -152,8 +146,6 @@ static int xo1_pm_probe(struct platform_device *pdev)
>
> static int xo1_pm_remove(struct platform_device *pdev)
> {
> - mfd_cell_disable(pdev);
> -
> if (strcmp(pdev->name, "cs5535-pms") == 0)
> pms_base = 0;
> else if (strcmp(pdev->name, "olpc-xo1-pm-acpi") == 0)
> --
> 2.17.1
>