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 (9):
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: 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 | 6 --
drivers/mfd/cs5535-mfd.c | 124 +++++++++++++--------------
drivers/mfd/mfd-core.c | 113 ++++--------------------
include/linux/mfd/core.h | 20 -----
4 files changed, 79 insertions(+), 184 deletions(-)
--
2.17.1
The current implementation abuses the platform 'id' mfd_cell member
to index into the correct resources entry. If we place all cells
into their numbered slots, we can cycle through all the cell entries
and only process the populated ones which avoids this behaviour.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/mfd/cs5535-mfd.c | 31 +++++++++++++------------------
1 file changed, 13 insertions(+), 18 deletions(-)
diff --git a/drivers/mfd/cs5535-mfd.c b/drivers/mfd/cs5535-mfd.c
index 2c47afc22d24..9ce6bbcdbda1 100644
--- a/drivers/mfd/cs5535-mfd.c
+++ b/drivers/mfd/cs5535-mfd.c
@@ -62,26 +62,22 @@ static int cs5535_mfd_res_disable(struct platform_device *pdev)
static struct resource cs5535_mfd_resources[NR_BARS];
static struct mfd_cell cs5535_mfd_cells[] = {
- {
- .id = SMB_BAR,
+ [SMB_BAR] = {
.name = "cs5535-smb",
.num_resources = 1,
.resources = &cs5535_mfd_resources[SMB_BAR],
},
- {
- .id = GPIO_BAR,
+ [GPIO_BAR] = {
.name = "cs5535-gpio",
.num_resources = 1,
.resources = &cs5535_mfd_resources[GPIO_BAR],
},
- {
- .id = MFGPT_BAR,
+ [MFGPT_BAR] = {
.name = "cs5535-mfgpt",
.num_resources = 1,
.resources = &cs5535_mfd_resources[MFGPT_BAR],
},
- {
- .id = PMS_BAR,
+ [PMS_BAR] = {
.name = "cs5535-pms",
.num_resources = 1,
.resources = &cs5535_mfd_resources[PMS_BAR],
@@ -89,8 +85,7 @@ static struct mfd_cell cs5535_mfd_cells[] = {
.enable = cs5535_mfd_res_enable,
.disable = cs5535_mfd_res_disable,
},
- {
- .id = ACPI_BAR,
+ [ACPI_BAR] = {
.name = "cs5535-acpi",
.num_resources = 1,
.resources = &cs5535_mfd_resources[ACPI_BAR],
@@ -115,16 +110,16 @@ static int cs5535_mfd_probe(struct pci_dev *pdev,
return err;
/* fill in IO range for each cell; subdrivers handle the region */
- for (i = 0; i < ARRAY_SIZE(cs5535_mfd_cells); i++) {
- int bar = cs5535_mfd_cells[i].id;
- struct resource *r = &cs5535_mfd_resources[bar];
+ for (i = 0; i < NR_BARS; i++) {
+ struct mfd_cell *cell = &cs5535_mfd_cells[i];
+ struct resource *r = &cs5535_mfd_resources[i];
- r->flags = IORESOURCE_IO;
- r->start = pci_resource_start(pdev, bar);
- r->end = pci_resource_end(pdev, bar);
+ if (cell->num_resources <= 0)
+ continue;
- /* id is used for temporarily storing BAR; unset it now */
- cs5535_mfd_cells[i].id = 0;
+ r->flags = IORESOURCE_IO;
+ r->start = pci_resource_start(pdev, i);
+ r->end = pci_resource_end(pdev, i);
}
err = mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, cs5535_mfd_cells,
--
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 | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/drivers/mfd/cs5535-mfd.c b/drivers/mfd/cs5535-mfd.c
index 053e33447808..96a99ac13384 100644
--- a/drivers/mfd/cs5535-mfd.c
+++ b/drivers/mfd/cs5535-mfd.c
@@ -57,9 +57,17 @@ static struct mfd_cell cs5535_mfd_cells[] = {
},
};
-static const char *olpc_acpi_clones[] = {
- "olpc-xo1-pm-acpi",
- "olpc-xo1-sci-acpi"
+static struct mfd_cell cs5535_olpc_mfd_cells[] = {
+ {
+ .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 int cs5535_mfd_probe(struct pci_dev *pdev,
@@ -105,10 +113,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 CS5532 OLPC sub-devices: %d\n",
+ err);
goto err_release_acpi;
}
}
--
2.17.1
Most of the complexity of mfd_platform_add_cell() has been removed. The
only functionality left duplicates cell memory into the child's platform
device. Since it's only a few lines, moving it to the main thread and
removing the superfluous function makes sense.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/mfd/mfd-core.c | 21 ++++-----------------
1 file changed, 4 insertions(+), 17 deletions(-)
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 5d56015baeeb..849dbe3798b0 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -49,19 +49,6 @@ int mfd_cell_disable(struct platform_device *pdev)
}
EXPORT_SYMBOL(mfd_cell_disable);
-static int mfd_platform_add_cell(struct platform_device *pdev,
- const struct mfd_cell *cell)
-{
- if (!cell)
- return 0;
-
- pdev->mfd_cell = kmemdup(cell, sizeof(*cell), GFP_KERNEL);
- if (!pdev->mfd_cell)
- return -ENOMEM;
-
- return 0;
-}
-
#if IS_ENABLED(CONFIG_ACPI)
static void mfd_acpi_add_device(const struct mfd_cell *cell,
struct platform_device *pdev)
@@ -141,6 +128,10 @@ static int mfd_add_device(struct device *parent, int id,
if (!pdev)
goto fail_alloc;
+ pdev->mfd_cell = kmemdup(cell, sizeof(*cell), GFP_KERNEL);
+ if (!pdev->mfd_cell)
+ goto fail_device;
+
res = kcalloc(cell->num_resources, sizeof(*res), GFP_KERNEL);
if (!res)
goto fail_device;
@@ -183,10 +174,6 @@ static int mfd_add_device(struct device *parent, int id,
goto fail_alias;
}
- ret = mfd_platform_add_cell(pdev, cell);
- if (ret)
- goto fail_alias;
-
for (r = 0; r < cell->num_resources; r++) {
res[r].name = cell->resources[r].name;
res[r].flags = cell->resources[r].flags;
--
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]>
---
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..90b43b44a15a 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->enable) {
+ 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
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]>
---
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 90b43b44a15a..5d56015baeeb 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->enable) {
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
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 | 6 ------
1 file changed, 6 deletions(-)
diff --git a/arch/x86/platform/olpc/olpc-xo1-pm.c b/arch/x86/platform/olpc/olpc-xo1-pm.c
index e1a32062a375..0fc57b59743c 100644
--- a/arch/x86/platform/olpc/olpc-xo1-pm.c
+++ b/arch/x86/platform/olpc/olpc-xo1-pm.c
@@ -126,10 +126,6 @@ static int xo1_pm_probe(struct platform_device *pdev)
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 +148,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
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;
+ }
+ }
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
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]>
---
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..2c47afc22d24 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 CS5532 sub-devices: %d\n", err);
goto err_disable;
}
--
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]>
---
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
On Mon, Oct 21, 2019 at 11:58:15AM +0100, Lee Jones wrote:
> The current implementation abuses the platform 'id' mfd_cell member
> to index into the correct resources entry. If we place all cells
> into their numbered slots, we can cycle through all the cell entries
> and only process the populated ones which avoids this behaviour.
>
> Signed-off-by: Lee Jones <[email protected]>
> ---
> drivers/mfd/cs5535-mfd.c | 31 +++++++++++++------------------
> 1 file changed, 13 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/mfd/cs5535-mfd.c b/drivers/mfd/cs5535-mfd.c
> index 2c47afc22d24..9ce6bbcdbda1 100644
> --- a/drivers/mfd/cs5535-mfd.c
> +++ b/drivers/mfd/cs5535-mfd.c
> @@ -62,26 +62,22 @@ static int cs5535_mfd_res_disable(struct platform_device *pdev)
> static struct resource cs5535_mfd_resources[NR_BARS];
>
> static struct mfd_cell cs5535_mfd_cells[] = {
This array is sized from the initializer...
> - {
> - .id = SMB_BAR,
> + [SMB_BAR] = {
> .name = "cs5535-smb",
> .num_resources = 1,
> .resources = &cs5535_mfd_resources[SMB_BAR],
> },
> - {
> - .id = GPIO_BAR,
> + [GPIO_BAR] = {
> .name = "cs5535-gpio",
> .num_resources = 1,
> .resources = &cs5535_mfd_resources[GPIO_BAR],
> },
> - {
> - .id = MFGPT_BAR,
> + [MFGPT_BAR] = {
> .name = "cs5535-mfgpt",
> .num_resources = 1,
> .resources = &cs5535_mfd_resources[MFGPT_BAR],
> },
> - {
> - .id = PMS_BAR,
> + [PMS_BAR] = {
> .name = "cs5535-pms",
> .num_resources = 1,
> .resources = &cs5535_mfd_resources[PMS_BAR],
> @@ -89,8 +85,7 @@ static struct mfd_cell cs5535_mfd_cells[] = {
> .enable = cs5535_mfd_res_enable,
> .disable = cs5535_mfd_res_disable,
> },
> - {
> - .id = ACPI_BAR,
> + [ACPI_BAR] = {
> .name = "cs5535-acpi",
> .num_resources = 1,
> .resources = &cs5535_mfd_resources[ACPI_BAR],
> @@ -115,16 +110,16 @@ static int cs5535_mfd_probe(struct pci_dev *pdev,
> return err;
>
> /* fill in IO range for each cell; subdrivers handle the region */
> - for (i = 0; i < ARRAY_SIZE(cs5535_mfd_cells); i++) {
> - int bar = cs5535_mfd_cells[i].id;
> - struct resource *r = &cs5535_mfd_resources[bar];
> + for (i = 0; i < NR_BARS; i++) {
... which means this translation from ARRAY_SIZE() to NR_BARS
is rather odd.
I don't care whether the array is sized using NR_BARS or the loop
uses ARRAY_SIZE() but IMHO the loop boundary condition must match
the array declaration.
With that fixed free to throw the following onto the next rev:
Reviewed-by: Daniel Thompson <[email protected]>
Daniel.
On Mon, Oct 21, 2019 at 11:58:14AM +0100, Lee Jones wrote:
> 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.
No objections... but won't the tag added by dev_err() already
disambiguate?
>
> Signed-off-by: Lee Jones <[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..2c47afc22d24 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 CS5532 sub-devices: %d\n", err);
^^^
Typo (and MODULE_DESCRIPTION() says this is a driver for CS5536 too).
Once that is fixed:
Reviewed-by: Daniel Thompson <[email protected]>
Daniel.
On Mon, Oct 21, 2019 at 12:58 PM Lee Jones <[email protected]> wrote:
>
> 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.
As the CS5535 is primarily used on the OLPC XO1, it would be
good to have someone test the series on such a machine.
I've added a few people to Cc that may be able to help test it, or
know someone who can.
For the actual patches, see
https://lore.kernel.org/lkml/[email protected]/T/#t
Arnd
> Lee Jones (9):
> 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: 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 | 6 --
> drivers/mfd/cs5535-mfd.c | 124 +++++++++++++--------------
> drivers/mfd/mfd-core.c | 113 ++++--------------------
> include/linux/mfd/core.h | 20 -----
> 4 files changed, 79 insertions(+), 184 deletions(-)
>
> --
> 2.17.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, 21 Oct 2019, Daniel Thompson wrote:
> On Mon, Oct 21, 2019 at 11:58:14AM +0100, Lee Jones wrote:
> > 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.
>
> No objections... but won't the tag added by dev_err() already
> disambiguate?
The difference will be between CS5532 and CS5532 OLPC.
Please see patch 4 in the series.
> > Signed-off-by: Lee Jones <[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..2c47afc22d24 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 CS5532 sub-devices: %d\n", err);
>
> ^^^
>
> Typo (and MODULE_DESCRIPTION() says this is a driver for CS5536 too).
> Once that is fixed:
> Reviewed-by: Daniel Thompson <[email protected]>
Ta.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 21 Oct 2019, Lee Jones wrote:
> On Mon, 21 Oct 2019, Daniel Thompson wrote:
>
> > On Mon, Oct 21, 2019 at 11:58:14AM +0100, Lee Jones wrote:
> > > 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.
> >
> > No objections... but won't the tag added by dev_err() already
> > disambiguate?
>
> The difference will be between CS5532 and CS5532 OLPC.
>
> Please see patch 4 in the series.
>
> > > Signed-off-by: Lee Jones <[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..2c47afc22d24 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 CS5532 sub-devices: %d\n", err);
> >
> > ^^^
> >
> > Typo (and MODULE_DESCRIPTION() says this is a driver for CS5536 too).
> > Once that is fixed:
> > Reviewed-by: Daniel Thompson <[email protected]>
>
> Ta.
Looks like this comes from the failed message from the
mfd_add_devices() above. I'll fix both.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 21 Oct 2019, Arnd Bergmann wrote:
> On Mon, Oct 21, 2019 at 12:58 PM Lee Jones <[email protected]> wrote:
> >
> > 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.
>
> As the CS5535 is primarily used on the OLPC XO1, it would be
> good to have someone test the series on such a machine.
>
> I've added a few people to Cc that may be able to help test it, or
> know someone who can.
Wonderful. Thank you.
> For the actual patches, see
> https://lore.kernel.org/lkml/[email protected]/T/#t
>
> Arnd
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 2019-10-21 at 13:29 +0200, Arnd Bergmann wrote:
> On Mon, Oct 21, 2019 at 12:58 PM Lee Jones <[email protected]> wrote:
> > 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.
>
> As the CS5535 is primarily used on the OLPC XO1, it would be
> good to have someone test the series on such a machine.
>
> I've added a few people to Cc that may be able to help test it, or
> know someone who can.
>
> For the actual patches, see
> https://lore.kernel.org/lkml/[email protected]/T/#t
Thanks for the pointer. I'd by happy to test this.
Which tree do the patches apply to?
Or, better, is there a tree with the patches applied that I could use?
Thanks
Lubo
>
> Arnd
>
> > Lee Jones (9):
> > 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: 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 | 6 --
> > drivers/mfd/cs5535-mfd.c | 124 +++++++++++++--------------
> > drivers/mfd/mfd-core.c | 113 ++++--------------------
> > include/linux/mfd/core.h | 20 -----
> > 4 files changed, 79 insertions(+), 184 deletions(-)
> >
> > --
> > 2.17.1
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, 21 Oct 2019, Daniel Thompson wrote:
> On Mon, Oct 21, 2019 at 11:58:15AM +0100, Lee Jones wrote:
> > The current implementation abuses the platform 'id' mfd_cell member
> > to index into the correct resources entry. If we place all cells
> > into their numbered slots, we can cycle through all the cell entries
> > and only process the populated ones which avoids this behaviour.
> >
> > Signed-off-by: Lee Jones <[email protected]>
> > ---
> > drivers/mfd/cs5535-mfd.c | 31 +++++++++++++------------------
> > 1 file changed, 13 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/mfd/cs5535-mfd.c b/drivers/mfd/cs5535-mfd.c
> > index 2c47afc22d24..9ce6bbcdbda1 100644
> > --- a/drivers/mfd/cs5535-mfd.c
> > +++ b/drivers/mfd/cs5535-mfd.c
> > @@ -62,26 +62,22 @@ static int cs5535_mfd_res_disable(struct platform_device *pdev)
> > static struct resource cs5535_mfd_resources[NR_BARS];
> >
> > static struct mfd_cell cs5535_mfd_cells[] = {
>
> This array is sized from the initializer...
>
> > - {
> > - .id = SMB_BAR,
> > + [SMB_BAR] = {
> > .name = "cs5535-smb",
> > .num_resources = 1,
> > .resources = &cs5535_mfd_resources[SMB_BAR],
> > },
> > - {
> > - .id = GPIO_BAR,
> > + [GPIO_BAR] = {
> > .name = "cs5535-gpio",
> > .num_resources = 1,
> > .resources = &cs5535_mfd_resources[GPIO_BAR],
> > },
> > - {
> > - .id = MFGPT_BAR,
> > + [MFGPT_BAR] = {
> > .name = "cs5535-mfgpt",
> > .num_resources = 1,
> > .resources = &cs5535_mfd_resources[MFGPT_BAR],
> > },
> > - {
> > - .id = PMS_BAR,
> > + [PMS_BAR] = {
> > .name = "cs5535-pms",
> > .num_resources = 1,
> > .resources = &cs5535_mfd_resources[PMS_BAR],
> > @@ -89,8 +85,7 @@ static struct mfd_cell cs5535_mfd_cells[] = {
> > .enable = cs5535_mfd_res_enable,
> > .disable = cs5535_mfd_res_disable,
> > },
> > - {
> > - .id = ACPI_BAR,
> > + [ACPI_BAR] = {
> > .name = "cs5535-acpi",
> > .num_resources = 1,
> > .resources = &cs5535_mfd_resources[ACPI_BAR],
> > @@ -115,16 +110,16 @@ static int cs5535_mfd_probe(struct pci_dev *pdev,
> > return err;
> >
> > /* fill in IO range for each cell; subdrivers handle the region */
> > - for (i = 0; i < ARRAY_SIZE(cs5535_mfd_cells); i++) {
> > - int bar = cs5535_mfd_cells[i].id;
> > - struct resource *r = &cs5535_mfd_resources[bar];
> > + for (i = 0; i < NR_BARS; i++) {
>
> ... which means this translation from ARRAY_SIZE() to NR_BARS
> is rather odd.
>
> I don't care whether the array is sized using NR_BARS or the loop
> uses ARRAY_SIZE() but IMHO the loop boundary condition must match
> the array declaration.
Sounds reasonable.
> With that fixed free to throw the following onto the next rev:
> Reviewed-by: Daniel Thompson <[email protected]>
Ta.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 21 Oct 2019, Lubomir Rintel wrote:
> On Mon, 2019-10-21 at 13:29 +0200, Arnd Bergmann wrote:
> > On Mon, Oct 21, 2019 at 12:58 PM Lee Jones <[email protected]> wrote:
> > > 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.
> >
> > As the CS5535 is primarily used on the OLPC XO1, it would be
> > good to have someone test the series on such a machine.
> >
> > I've added a few people to Cc that may be able to help test it, or
> > know someone who can.
> >
> > For the actual patches, see
> > https://lore.kernel.org/lkml/[email protected]/T/#t
>
> Thanks for the pointer. I'd by happy to test this.
>
> Which tree do the patches apply to?
> Or, better, is there a tree with the patches applied that I could use?
Ideal. Thank you.
http://git.linaro.org/people/lee.jones/linux.git/log/?h=topic/mfd-remove-clone-cs5535-mfd
> > > Lee Jones (9):
> > > 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: 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 | 6 --
> > > drivers/mfd/cs5535-mfd.c | 124 +++++++++++++--------------
> > > drivers/mfd/mfd-core.c | 113 ++++--------------------
> > > include/linux/mfd/core.h | 20 -----
> > > 4 files changed, 79 insertions(+), 184 deletions(-)
> > >
>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, Oct 21, 2019 at 11:58:19AM +0100, Lee Jones wrote:
> 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 | 6 ------
Why doesn't olpc-xo1-sci.c need a similar update.
Daniel.
> 1 file changed, 6 deletions(-)
>
> diff --git a/arch/x86/platform/olpc/olpc-xo1-pm.c b/arch/x86/platform/olpc/olpc-xo1-pm.c
> index e1a32062a375..0fc57b59743c 100644
> --- a/arch/x86/platform/olpc/olpc-xo1-pm.c
> +++ b/arch/x86/platform/olpc/olpc-xo1-pm.c
> @@ -126,10 +126,6 @@ static int xo1_pm_probe(struct platform_device *pdev)
> 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 +148,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
>
On Mon, 2019-10-21 at 12:53 +0100, Lee Jones wrote:
> On Mon, 21 Oct 2019, Lubomir Rintel wrote:
>
> > On Mon, 2019-10-21 at 13:29 +0200, Arnd Bergmann wrote:
> > > On Mon, Oct 21, 2019 at 12:58 PM Lee Jones <[email protected]> wrote:
> > > > 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.
> > >
> > > As the CS5535 is primarily used on the OLPC XO1, it would be
> > > good to have someone test the series on such a machine.
> > >
> > > I've added a few people to Cc that may be able to help test it, or
> > > know someone who can.
> > >
> > > For the actual patches, see
> > > https://lore.kernel.org/lkml/[email protected]/T/#t
> >
> > Thanks for the pointer. I'd by happy to test this.
> >
> > Which tree do the patches apply to?
> > Or, better, is there a tree with the patches applied that I could use?
>
> Ideal. Thank you.
>
> http://git.linaro.org/people/lee.jones/linux.git/log/?h=topic/mfd-remove-clone-cs5535-mfd
Thanks. My boot attempt ends up in a panic [1]:
[ 2.090943] cs5535-gpio cs5535-gpio: reserved resource region [io 0x1000-0x10ff]
[ 2.129084] cs5535-mfgpt cs5535-mfgpt: reserved resource region [io 0x1800-0x183f]
[ 2.173457] cs5535-mfgpt cs5535-mfgpt: 8 MFGPT timers available
[ 2.200731] BUG: kernel NULL pointer dereference, address: 00000000
[ 2.210655] #PF: supervisor read access in kernel mode
[ 2.210655] #PF: error_code(0x0000) - not-present page
[ 2.210655] *pde = 00000000
[ 2.210655] Oops: 0000 [#1] PREEMPT
[ 2.210655] CPU: 0 PID: 1 Comm: swapper Not tainted 5.4.0-rc3-00013-gd8518b1ac7282 #17
[ 2.210655] Hardware name: OLPC XO/XO, BIOS OLPC Ver 1.00.01 10/16/2019
[ 2.210655] EIP: strlen+0xb/0x17
[ 2.210655] Code: c3 55 89 e5 56 89 c6 89 d0 88 c4 ac 38 e0 74 09 84 c0 75 f7 be 01 00 00 00 89 f0 48 5e 5d c3 55 89 e5 83 c9 ff 57 89 c7 31 c0 <f2> ae b8 fe ff ff ff 5f 29 c8 5d c3 85 c9 74 17 55 89 e5 57 89 c7
[ 2.210655] EAX: 00000000 EBX: 00000000 ECX: ffffffff EDX: ffffffff
[ 2.210655] ESI: c0d214a0 EDI: 00000000 EBP: ce487dd8 ESP: ce487dd4
[ 2.210655] DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068 EFLAGS: 00010246
[ 2.210655] CR0: 80050033 CR2: 00000000 CR3: 00e08000 CR4: 00000090
[ 2.210655] Call Trace:
[ 2.210655] platform_device_alloc+0x11/0xb2
[ 2.210655] mfd_add_devices+0x3c/0x285
[ 2.210655] cs5535_mfd_probe+0x95/0x159
[ 2.210655] ? cs5535_mfd_remove+0x2e/0x2e
[ 2.210655] pci_device_probe+0x83/0xe9
[ 2.210655] really_probe+0x16f/0x335
[ 2.210655] driver_probe_device+0x113/0x148
[ 2.210655] device_driver_attach+0x2e/0x41
[ 2.210655] __driver_attach+0xe4/0xee
[ 2.210655] ? device_driver_attach+0x41/0x41
[ 2.210655] bus_for_each_dev+0x54/0x81
[ 2.210655] driver_attach+0x14/0x16
[ 2.210655] ? device_driver_attach+0x41/0x41
[ 2.210655] bus_add_driver+0xe9/0x190
[ 2.210655] ? max8925_i2c_init+0x2a/0x2a
[ 2.210655] driver_register+0x87/0xb9
[ 2.210655] ? max8925_i2c_init+0x2a/0x2a
[ 2.210655] __pci_register_driver+0x37/0x3a
[ 2.210655] cs5535_mfd_driver_init+0x14/0x16
[ 2.210655] do_one_initcall+0x78/0x169
[ 2.210655] ? do_early_param+0x75/0x75
[ 2.210655] kernel_init_freeable+0xe6/0x16d
[ 2.210655] ? rest_init+0x8e/0x8e
[ 2.210655] kernel_init+0x8/0xd5
[ 2.210655] ret_from_fork+0x2e/0x38
[ 2.210655] Modules linked in:
[ 2.210655] CR2: 0000000000000000
[ 2.210655] ---[ end trace b02c575c8463e16f ]---
[ 2.210655] EIP: strlen+0xb/0x17
[ 2.210655] Code: c3 55 89 e5 56 89 c6 89 d0 88 c4 ac 38 e0 74 09 84 c0 75 f7 be 01 00 00 00 89 f0 48 5e 5d c3 55 89 e5 83 c9 ff 57 89 c7 31 c0 <f2> ae b8 fe ff ff ff 5f 29 c8 5d c3 85 c9 74 17 55 89 e5 57 89 c7
[ 2.210655] EAX: 00000000 EBX: 00000000 ECX: ffffffff EDX: ffffffff
[ 2.210655] ESI: c0d214a0 EDI: 00000000 EBP: ce487dd8 ESP: ce487dd4
[ 2.210655] DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068 EFLAGS: 00010246
[ 2.210655] CR0: 80050033 CR2: 00000000 CR3: 00e08000 CR4: 00000090
[ 4.012823] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009
[ 4.022771] Kernel Offset: 0x0 from 0xc0400000 (relocation range: 0xc0000000-0xcf3fffff)
[ 4.022771] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009 ]---
[1] http://v3.sk/~lkundrak/mfp.txt
Also:
There's a build warning, caused by "x86: olpc: Remove invocation of
MFD's .enable()/.disable() call-backs" I suppose:
arch/x86/platform/olpc/olpc-xo1-pm.c: In function ‘xo1_pm_probe’:
arch/x86/platform/olpc/olpc-xo1-pm.c:123:6: warning: unused variable ‘err’ [-Wunused-variable]
123 | int err;
| ^~~
I didn't look further into it. I'm happy to do so if necessary or try
out a fix.
Take care
Lubo
>
> > > > Lee Jones (9):
> > > > 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: 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 | 6 --
> > > > drivers/mfd/cs5535-mfd.c | 124 +++++++++++++--------------
> > > > drivers/mfd/mfd-core.c | 113 ++++--------------------
> > > > include/linux/mfd/core.h | 20 -----
> > > > 4 files changed, 79 insertions(+), 184 deletions(-)
> > > >
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
>
On Mon, Oct 21, 2019 at 11:58:17AM +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]>
> ---
> drivers/mfd/cs5535-mfd.c | 24 ++++++++++++++++++------
> 1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mfd/cs5535-mfd.c b/drivers/mfd/cs5535-mfd.c
> index 053e33447808..96a99ac13384 100644
> --- a/drivers/mfd/cs5535-mfd.c
> +++ b/drivers/mfd/cs5535-mfd.c
> @@ -57,9 +57,17 @@ static struct mfd_cell cs5535_mfd_cells[] = {
> },
> };
>
> -static const char *olpc_acpi_clones[] = {
> - "olpc-xo1-pm-acpi",
> - "olpc-xo1-sci-acpi"
> +static struct mfd_cell cs5535_olpc_mfd_cells[] = {
> + {
> + .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],
> + },
Is the cs5535-acpi cell actually used by anything? I think it was only
ever used as a template and can be removed; I didn't spot any driver that
uses it.
PS If the cell were removed then my review comment on the previous patch
becomes moot ;-)
> };
>
> static int cs5535_mfd_probe(struct pci_dev *pdev,
> @@ -105,10 +113,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 CS5532 OLPC sub-devices: %d\n",
> + err);
> goto err_release_acpi;
> }
> }
> --
> 2.17.1
>
On Mon, Oct 21, 2019 at 11:58:18AM +0100, Lee Jones wrote:
> 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
>
On Mon, Oct 21, 2019 at 11:58:20AM +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]>
> ---
> 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..90b43b44a15a 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->enable) {
> + 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
>
On Mon, Oct 21, 2019 at 11:58:20AM +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]>
> ---
> 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..90b43b44a15a 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->enable) {
Oops.
Cancel the R-B: ;-). Shouldn't this be !cell->disable() ?
Daniel.
> + 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
>
On Mon, Oct 21, 2019 at 11:58:21AM +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.
>
> 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 90b43b44a15a..5d56015baeeb 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->enable) {
> 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 Mon, 21 Oct 2019, Daniel Thompson wrote:
> On Mon, Oct 21, 2019 at 11:58:20AM +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]>
> > ---
> > 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..90b43b44a15a 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->enable) {
>
> Oops.
>
> Cancel the R-B: ;-). Shouldn't this be !cell->disable() ?
Yes. Looks like a C/P error.
Will fix and add your R-b.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, Oct 21, 2019 at 11:58:22AM +0100, Lee Jones wrote:
> Most of the complexity of mfd_platform_add_cell() has been removed. The
> only functionality left duplicates cell memory into the child's platform
> device. Since it's only a few lines, moving it to the main thread and
> removing the superfluous function makes sense.
>
> Signed-off-by: Lee Jones <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
> ---
> drivers/mfd/mfd-core.c | 21 ++++-----------------
> 1 file changed, 4 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> index 5d56015baeeb..849dbe3798b0 100644
> --- a/drivers/mfd/mfd-core.c
> +++ b/drivers/mfd/mfd-core.c
> @@ -49,19 +49,6 @@ int mfd_cell_disable(struct platform_device *pdev)
> }
> EXPORT_SYMBOL(mfd_cell_disable);
>
> -static int mfd_platform_add_cell(struct platform_device *pdev,
> - const struct mfd_cell *cell)
> -{
> - if (!cell)
> - return 0;
> -
> - pdev->mfd_cell = kmemdup(cell, sizeof(*cell), GFP_KERNEL);
> - if (!pdev->mfd_cell)
> - return -ENOMEM;
> -
> - return 0;
> -}
> -
> #if IS_ENABLED(CONFIG_ACPI)
> static void mfd_acpi_add_device(const struct mfd_cell *cell,
> struct platform_device *pdev)
> @@ -141,6 +128,10 @@ static int mfd_add_device(struct device *parent, int id,
> if (!pdev)
> goto fail_alloc;
>
> + pdev->mfd_cell = kmemdup(cell, sizeof(*cell), GFP_KERNEL);
> + if (!pdev->mfd_cell)
> + goto fail_device;
> +
> res = kcalloc(cell->num_resources, sizeof(*res), GFP_KERNEL);
> if (!res)
> goto fail_device;
> @@ -183,10 +174,6 @@ static int mfd_add_device(struct device *parent, int id,
> goto fail_alias;
> }
>
> - ret = mfd_platform_add_cell(pdev, cell);
> - if (ret)
> - goto fail_alias;
> -
> for (r = 0; r < cell->num_resources; r++) {
> res[r].name = cell->resources[r].name;
> res[r].flags = cell->resources[r].flags;
> --
> 2.17.1
>
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 <[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.
It is not used.
Will reply to your other comment.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 21 Oct 2019, Lubomir Rintel wrote:
> On Mon, 2019-10-21 at 12:53 +0100, Lee Jones wrote:
> > On Mon, 21 Oct 2019, Lubomir Rintel wrote:
> >
> > > On Mon, 2019-10-21 at 13:29 +0200, Arnd Bergmann wrote:
> > > > On Mon, Oct 21, 2019 at 12:58 PM Lee Jones <[email protected]> wrote:
> > > > > 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.
> > > >
> > > > As the CS5535 is primarily used on the OLPC XO1, it would be
> > > > good to have someone test the series on such a machine.
> > > >
> > > > I've added a few people to Cc that may be able to help test it, or
> > > > know someone who can.
> > > >
> > > > For the actual patches, see
> > > > https://lore.kernel.org/lkml/[email protected]/T/#t
> > >
> > > Thanks for the pointer. I'd by happy to test this.
> > >
> > > Which tree do the patches apply to?
> > > Or, better, is there a tree with the patches applied that I could use?
> >
> > Ideal. Thank you.
> >
> > http://git.linaro.org/people/lee.jones/linux.git/log/?h=topic/mfd-remove-clone-cs5535-mfd
>
> Thanks. My boot attempt ends up in a panic [1]:
Ah yes, that makes sense.
I guess the subsystem doesn't like there being empty cells.
Please bear with me.
> [ 2.090943] cs5535-gpio cs5535-gpio: reserved resource region [io 0x1000-0x10ff]
> [ 2.129084] cs5535-mfgpt cs5535-mfgpt: reserved resource region [io 0x1800-0x183f]
> [ 2.173457] cs5535-mfgpt cs5535-mfgpt: 8 MFGPT timers available
> [ 2.200731] BUG: kernel NULL pointer dereference, address: 00000000
> [ 2.210655] #PF: supervisor read access in kernel mode
> [ 2.210655] #PF: error_code(0x0000) - not-present page
> [ 2.210655] *pde = 00000000
> [ 2.210655] Oops: 0000 [#1] PREEMPT
> [ 2.210655] CPU: 0 PID: 1 Comm: swapper Not tainted 5.4.0-rc3-00013-gd8518b1ac7282 #17
> [ 2.210655] Hardware name: OLPC XO/XO, BIOS OLPC Ver 1.00.01 10/16/2019
> [ 2.210655] EIP: strlen+0xb/0x17
> [ 2.210655] Code: c3 55 89 e5 56 89 c6 89 d0 88 c4 ac 38 e0 74 09 84 c0 75 f7 be 01 00 00 00 89 f0 48 5e 5d c3 55 89 e5 83 c9 ff 57 89 c7 31 c0 <f2> ae b8 fe ff ff ff 5f 29 c8 5d c3 85 c9 74 17 55 89 e5 57 89 c7
> [ 2.210655] EAX: 00000000 EBX: 00000000 ECX: ffffffff EDX: ffffffff
> [ 2.210655] ESI: c0d214a0 EDI: 00000000 EBP: ce487dd8 ESP: ce487dd4
> [ 2.210655] DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068 EFLAGS: 00010246
> [ 2.210655] CR0: 80050033 CR2: 00000000 CR3: 00e08000 CR4: 00000090
> [ 2.210655] Call Trace:
> [ 2.210655] platform_device_alloc+0x11/0xb2
> [ 2.210655] mfd_add_devices+0x3c/0x285
> [ 2.210655] cs5535_mfd_probe+0x95/0x159
> [ 2.210655] ? cs5535_mfd_remove+0x2e/0x2e
> [ 2.210655] pci_device_probe+0x83/0xe9
> [ 2.210655] really_probe+0x16f/0x335
> [ 2.210655] driver_probe_device+0x113/0x148
> [ 2.210655] device_driver_attach+0x2e/0x41
> [ 2.210655] __driver_attach+0xe4/0xee
> [ 2.210655] ? device_driver_attach+0x41/0x41
> [ 2.210655] bus_for_each_dev+0x54/0x81
> [ 2.210655] driver_attach+0x14/0x16
> [ 2.210655] ? device_driver_attach+0x41/0x41
> [ 2.210655] bus_add_driver+0xe9/0x190
> [ 2.210655] ? max8925_i2c_init+0x2a/0x2a
> [ 2.210655] driver_register+0x87/0xb9
> [ 2.210655] ? max8925_i2c_init+0x2a/0x2a
> [ 2.210655] __pci_register_driver+0x37/0x3a
> [ 2.210655] cs5535_mfd_driver_init+0x14/0x16
> [ 2.210655] do_one_initcall+0x78/0x169
> [ 2.210655] ? do_early_param+0x75/0x75
> [ 2.210655] kernel_init_freeable+0xe6/0x16d
> [ 2.210655] ? rest_init+0x8e/0x8e
> [ 2.210655] kernel_init+0x8/0xd5
> [ 2.210655] ret_from_fork+0x2e/0x38
> [ 2.210655] Modules linked in:
> [ 2.210655] CR2: 0000000000000000
> [ 2.210655] ---[ end trace b02c575c8463e16f ]---
> [ 2.210655] EIP: strlen+0xb/0x17
> [ 2.210655] Code: c3 55 89 e5 56 89 c6 89 d0 88 c4 ac 38 e0 74 09 84 c0 75 f7 be 01 00 00 00 89 f0 48 5e 5d c3 55 89 e5 83 c9 ff 57 89 c7 31 c0 <f2> ae b8 fe ff ff ff 5f 29 c8 5d c3 85 c9 74 17 55 89 e5 57 89 c7
> [ 2.210655] EAX: 00000000 EBX: 00000000 ECX: ffffffff EDX: ffffffff
> [ 2.210655] ESI: c0d214a0 EDI: 00000000 EBP: ce487dd8 ESP: ce487dd4
> [ 2.210655] DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068 EFLAGS: 00010246
> [ 2.210655] CR0: 80050033 CR2: 00000000 CR3: 00e08000 CR4: 00000090
> [ 4.012823] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009
> [ 4.022771] Kernel Offset: 0x0 from 0xc0400000 (relocation range: 0xc0000000-0xcf3fffff)
> [ 4.022771] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009 ]---
>
> [1] http://v3.sk/~lkundrak/mfp.txt
>
> Also:
>
> There's a build warning, caused by "x86: olpc: Remove invocation of
> MFD's .enable()/.disable() call-backs" I suppose:
>
> arch/x86/platform/olpc/olpc-xo1-pm.c: In function ‘xo1_pm_probe’:
> arch/x86/platform/olpc/olpc-xo1-pm.c:123:6: warning: unused variable ‘err’ [-Wunused-variable]
> 123 | int err;
> | ^~~
>
> I didn't look further into it. I'm happy to do so if necessary or try
> out a fix.
>
> Take care
> Lubo
>
> >
> > > > > Lee Jones (9):
> > > > > 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: 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 | 6 --
> > > > > drivers/mfd/cs5535-mfd.c | 124 +++++++++++++--------------
> > > > > drivers/mfd/mfd-core.c | 113 ++++--------------------
> > > > > include/linux/mfd/core.h | 20 -----
> > > > > 4 files changed, 79 insertions(+), 184 deletions(-)
> > > > >
>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 21 Oct 2019, Daniel Thompson wrote:
> On Mon, Oct 21, 2019 at 11:58:17AM +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]>
> > ---
> > drivers/mfd/cs5535-mfd.c | 24 ++++++++++++++++++------
> > 1 file changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mfd/cs5535-mfd.c b/drivers/mfd/cs5535-mfd.c
> > index 053e33447808..96a99ac13384 100644
> > --- a/drivers/mfd/cs5535-mfd.c
> > +++ b/drivers/mfd/cs5535-mfd.c
> > @@ -57,9 +57,17 @@ static struct mfd_cell cs5535_mfd_cells[] = {
> > },
> > };
> >
> > -static const char *olpc_acpi_clones[] = {
> > - "olpc-xo1-pm-acpi",
> > - "olpc-xo1-sci-acpi"
> > +static struct mfd_cell cs5535_olpc_mfd_cells[] = {
> > + {
> > + .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],
> > + },
>
> Is the cs5535-acpi cell actually used by anything? I think it was only
> ever used as a template and can be removed; I didn't spot any driver that
> uses it.
I did think about this, but I assumed removing it at this stage would
make the resource matching below more convoluted.
I'll take another look at see what I can do.
> PS If the cell were removed then my review comment on the previous patch
> becomes moot ;-)
>
>
> > };
> >
> > static int cs5535_mfd_probe(struct pci_dev *pdev,
> > @@ -105,10 +113,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 CS5532 OLPC sub-devices: %d\n",
> > + err);
> > goto err_release_acpi;
> > }
> > }
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 21 Oct 2019, Lubomir Rintel wrote:
> On Mon, 2019-10-21 at 12:53 +0100, Lee Jones wrote:
> > On Mon, 21 Oct 2019, Lubomir Rintel wrote:
> >
> > > On Mon, 2019-10-21 at 13:29 +0200, Arnd Bergmann wrote:
> > > > On Mon, Oct 21, 2019 at 12:58 PM Lee Jones <[email protected]> wrote:
> > > > > 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.
> > > >
> > > > As the CS5535 is primarily used on the OLPC XO1, it would be
> > > > good to have someone test the series on such a machine.
> > > >
> > > > I've added a few people to Cc that may be able to help test it, or
> > > > know someone who can.
> > > >
> > > > For the actual patches, see
> > > > https://lore.kernel.org/lkml/[email protected]/T/#t
> > >
> > > Thanks for the pointer. I'd by happy to test this.
> > >
> > > Which tree do the patches apply to?
> > > Or, better, is there a tree with the patches applied that I could use?
> >
> > Ideal. Thank you.
> >
> > http://git.linaro.org/people/lee.jones/linux.git/log/?h=topic/mfd-remove-clone-cs5535-mfd
>
> Thanks. My boot attempt ends up in a panic [1]:
New patches have been drafted, reviewed and pushed to the same branch.
Would you be kind enough to boot test them for me please Lubo?
TIA.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Fri, 2019-11-01 at 09:07 +0000, Lee Jones wrote:
> On Mon, 21 Oct 2019, Lubomir Rintel wrote:
> > On Mon, 2019-10-21 at 12:53 +0100, Lee Jones wrote:
> > > On Mon, 21 Oct 2019, Lubomir Rintel wrote:
> > >
> > > > On Mon, 2019-10-21 at 13:29 +0200, Arnd Bergmann wrote:
> > > > > On Mon, Oct 21, 2019 at 12:58 PM Lee Jones <[email protected]> wrote:
> > > > > > 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.
> > > > >
> > > > > As the CS5535 is primarily used on the OLPC XO1, it would be
> > > > > good to have someone test the series on such a machine.
> > > > >
> > > > > I've added a few people to Cc that may be able to help test it, or
> > > > > know someone who can.
> > > > >
> > > > > For the actual patches, see
> > > > > https://lore.kernel.org/lkml/[email protected]/T/#t
> > > >
> > > > Thanks for the pointer. I'd by happy to test this.
> > > >
> > > > Which tree do the patches apply to?
> > > > Or, better, is there a tree with the patches applied that I could use?
> > >
> > > Ideal. Thank you.
> > >
> > > http://git.linaro.org/people/lee.jones/linux.git/log/?h=topic/mfd-remove-clone-cs5535-mfd
> >
> > Thanks. My boot attempt ends up in a panic [1]:
>
> New patches have been drafted, reviewed and pushed to the same branch.
>
> Would you be kind enough to boot test them for me please Lubo?
The branch
Tested-by: Lubomir Rintel <[email protected]> (OLPC XO-1)
Here's a dmesg and partial sysfs listing indicating that the driver
indeed bound correctly: https://paste.centos.org/view/3aa89258
>
> TIA.
Take care
Lubo