| Message ID | 20200608134300.76091-4-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | mfd: Make use of software nodes | expand |
On Mon, 08 Jun 2020, Andy Shevchenko wrote: > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > When ever device properties are supplied for a sub device, a software node > (fwnode) is actually created and then associated with that device. By allowing > the drivers to supply the complete software node group instead of just the > properties in it, the drivers can take advantage of the other features the > software nodes have on top of supplying the device properties. > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/mfd/mfd-core.c | 31 +++++++++++++++++++++++++++---- > include/linux/mfd/core.h | 3 +++ > 2 files changed, 30 insertions(+), 4 deletions(-) I'm not sure a change to the API is justified presently (same does go for 'properties' really, but as it was only a couple of lines, it didn't seem too intrusive). My recommendation is to handle this in-house (i.e. locally in-driver) for now. When (if) more users adopt the practice, then we should consider to draw down on line numbers and repetition and make it part of the API.
On Mon, Jun 08, 2020 at 08:25:24PM +0100, Lee Jones wrote: > On Mon, 08 Jun 2020, Andy Shevchenko wrote: > > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > When ever device properties are supplied for a sub device, a software node > > (fwnode) is actually created and then associated with that device. By allowing > > the drivers to supply the complete software node group instead of just the > > properties in it, the drivers can take advantage of the other features the > > software nodes have on top of supplying the device properties. > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > drivers/mfd/mfd-core.c | 31 +++++++++++++++++++++++++++---- > > include/linux/mfd/core.h | 3 +++ > > 2 files changed, 30 insertions(+), 4 deletions(-) > > I'm not sure a change to the API is justified presently (same does go > for 'properties' really, but as it was only a couple of lines, it > didn't seem too intrusive). This is better and comprehensive API, but I heard you. > My recommendation is to handle this in-house (i.e. locally in-driver) > for now. I think you understand that this is not gonna work (we need to attach fwnode to the child device before it's registration. > When (if) more users adopt the practice, then we should > consider to draw down on line numbers and repetition and make it part > of the API. I briefly looked at the current state of affairs and found that properties are used only for MFD LPSS driver. Would the conversion of that driver to swnodes work for you? Note, the long prospective is to get rid of platform_add_properties() API completely.
On Tue, 09 Jun 2020, Andy Shevchenko wrote: > On Mon, Jun 08, 2020 at 08:25:24PM +0100, Lee Jones wrote: > > On Mon, 08 Jun 2020, Andy Shevchenko wrote: > > > > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > > > When ever device properties are supplied for a sub device, a software node > > > (fwnode) is actually created and then associated with that device. By allowing > > > the drivers to supply the complete software node group instead of just the > > > properties in it, the drivers can take advantage of the other features the > > > software nodes have on top of supplying the device properties. > > > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > --- > > > drivers/mfd/mfd-core.c | 31 +++++++++++++++++++++++++++---- > > > include/linux/mfd/core.h | 3 +++ > > > 2 files changed, 30 insertions(+), 4 deletions(-) > > > > I'm not sure a change to the API is justified presently (same does go > > for 'properties' really, but as it was only a couple of lines, it > > didn't seem too intrusive). > > This is better and comprehensive API, but I heard you. > > > My recommendation is to handle this in-house (i.e. locally in-driver) > > for now. > > I think you understand that this is not gonna work (we need to attach fwnode > to the child device before it's registration. > > > When (if) more users adopt the practice, then we should > > consider to draw down on line numbers and repetition and make it part > > of the API. > > I briefly looked at the current state of affairs and found that properties are > used only for MFD LPSS driver. Would the conversion of that driver to swnodes > work for you? > > Note, the long prospective is to get rid of platform_add_properties() API > completely. That's a shame. Do you plan on replacing it with something else? MFD tends to only interact with the platform_device API, and even platform_device_add_properties() doesn't get involved in the nitty gritty of fwnodes. Instead it acts as a pass-through straight to device_add_properties() which is where the magic happens. For this to be acceptable you would need to add support into platform_device i.e. platform_device_add_property_group() or some such. I really do not want the MFD API to have knowledge about regarding the particulars i.e. software node registration, secondary fwnodes and the like.
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c index f5a73af60dd4..1a256f64dc9d 100644 --- a/drivers/mfd/mfd-core.c +++ b/drivers/mfd/mfd-core.c @@ -173,7 +173,24 @@ static int mfd_add_device(struct device *parent, int id, goto fail_alias; } - if (cell->properties) { + /* If software node group exists, use it, otherwise try properties */ + if (cell->node_group) { + struct fwnode_handle *fwnode; + + ret = software_node_register_node_group(cell->node_group); + if (ret) + goto fail_alias; + + /* + * The very first software node in the group is related to + * the device itself. The rest can be device-less children to + * fulfill the case of sub nodes (LEDs, GPIO keys, etc). + */ + fwnode = software_node_fwnode(cell->node_group[0]); + + /* Assign this firmware node as a secondary one of the device */ + set_secondary_fwnode(&pdev->dev, fwnode); + } else if (cell->properties) { ret = platform_device_add_properties(pdev, cell->properties); if (ret) goto fail_alias; @@ -213,18 +230,18 @@ static int mfd_add_device(struct device *parent, int id, if (has_acpi_companion(&pdev->dev)) { ret = acpi_check_resource_conflict(&res[r]); if (ret) - goto fail_alias; + goto fail_resources; } } } ret = platform_device_add_resources(pdev, res, cell->num_resources); if (ret) - goto fail_alias; + goto fail_resources; ret = platform_device_add(pdev); if (ret) - goto fail_alias; + goto fail_resources; if (cell->pm_runtime_no_callbacks) pm_runtime_no_callbacks(&pdev->dev); @@ -233,6 +250,9 @@ static int mfd_add_device(struct device *parent, int id, return 0; +fail_resources: + set_secondary_fwnode(&pdev->dev, NULL); + software_node_unregister_node_group(cell->node_group); fail_alias: regulator_bulk_unregister_supply_alias(&pdev->dev, cell->parent_supplies, @@ -294,6 +314,9 @@ static int mfd_remove_devices_fn(struct device *dev, void *data) pdev = to_platform_device(dev); cell = mfd_get_cell(pdev); + set_secondary_fwnode(&pdev->dev, NULL); + software_node_unregister_node_group(cell->node_group); + regulator_bulk_unregister_supply_alias(dev, cell->parent_supplies, cell->num_parent_supplies); diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h index d01d1299e49d..9a998568759f 100644 --- a/include/linux/mfd/core.h +++ b/include/linux/mfd/core.h @@ -72,6 +72,9 @@ struct mfd_cell { /* device properties passed to the sub devices drivers */ struct property_entry *properties; + /* Software node group for the sub device */ + const struct software_node **node_group; + /* * Device Tree compatible string * See: Documentation/devicetree/usage-model.txt Chapter 2.2 for details