2022-01-06 21:46:11

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 00/32] component: Make into an aggregate bus

This series is from discussion we had on reordering the device lists for
drm shutdown paths[1]. I've introduced an 'aggregate' bus that we put
the aggregate device onto and then we probe the aggregate device once
all the components are probed and call component_add(). The probe/remove
hooks are where the bind/unbind calls go, and then a shutdown hook is
added that can be used to shutdown the drm display pipeline at the right
time.

This works for me on my sc7180 board. I no longer get a warning from i2c
at shutdown that we're trying to make an i2c transaction after the i2c
bus has been shutdown. There's more work to do on the msm drm driver to
extract component device resources like clks, regulators, etc. out of
the component bind function into the driver probe but I wanted to move
everything over now in other component drivers before tackling that
problem.

Tested-by tags would be appreciated, and Acked-by/Reviewed-by tags too.

Per Daniel, I've resent this to collect Acks or Review tags from gregkh
and once Greg is happy with the driver core bits it can be merged
through drm-misc tree via dianders (both are on the To line).

One last thing, I suspect I'll have to send this once again after the
merge window because something is probably in linux-next that conflicts
with some driver patch. I'll do that in about two weeks.

Changes since v4 (https://lore.kernel.org/r/[email protected]):
- Picked up tags
- Moved rename patch to first in the series
- Squashed device and bus type patch together

Changes since v3 (https://lore.kernel.org/r/[email protected]):
- Picked up tags
- Rebased to v5.16-rc2
- Updated component.c for a few new patches there
- Dropped a conversion patch
- Added a conversion patch

Changes since v2 (https://lore.kernel.org/r/[email protected]):
- Picked up acks
- Fixed build warnings/errors
- Reworked patch series to rename 'master' in a different patch

Changes since v1 (https://lore.kernel.org/r/[email protected]):
- Use devlink to connect components to the aggregate device
- Don't set the registering device as a parent of the aggregate device
- New patch for bind_component/unbind_component ops that takes the
aggregate device
- Convert all drivers in the tree to use the aggregate driver approach
- Allow one aggregate driver to be used for multiple aggregate devices

[1] https://lore.kernel.org/r/[email protected]

Stephen Boyd (32):
component: Replace most references to 'master' with 'aggregate device'
component: Introduce the aggregate bus_type
component: Move struct aggregate_device out to header file
component: Add {bind,unbind}_component() ops that take aggregate
device
drm/of: Add a drm_of_aggregate_probe() API
drm/msm: Migrate to aggregate driver
drm/komeda: Migrate to aggregate driver
drm/arm/hdlcd: Migrate to aggregate driver
drm/malidp: Migrate to aggregate driver
drm/armada: Migrate to aggregate driver
drm/etnaviv: Migrate to aggregate driver
drm/kirin: Migrate to aggregate driver
drm/exynos: Migrate to aggregate driver
drm/imx: Migrate to aggregate driver
drm/ingenic: Migrate to aggregate driver
drm/mcde: Migrate to aggregate driver
drm/mediatek: Migrate to aggregate driver
drm/meson: Migrate to aggregate driver
drm/omap: Migrate to aggregate driver
drm/rockchip: Migrate to aggregate driver
drm/sti: Migrate to aggregate driver
drm/sun4i: Migrate to aggregate driver
drm/tilcdc: Migrate to aggregate driver
drm/vc4: Migrate to aggregate driver
iommu/mtk: Migrate to aggregate driver
mei: Migrate to aggregate driver
power: supply: ab8500: Migrate to aggregate driver
fbdev: omap2: Migrate to aggregate driver
sound: hdac: Migrate to aggregate driver
ASoC: codecs: wcd938x: Migrate to aggregate driver
component: Get rid of drm_of_component_probe()
component: Remove component_master_ops and friends

drivers/base/component.c | 544 ++++++++++--------
.../gpu/drm/arm/display/komeda/komeda_drv.c | 20 +-
drivers/gpu/drm/arm/hdlcd_drv.c | 21 +-
drivers/gpu/drm/arm/malidp_drv.c | 21 +-
drivers/gpu/drm/armada/armada_drv.c | 23 +-
drivers/gpu/drm/drm_drv.c | 2 +-
drivers/gpu/drm/drm_of.c | 18 +-
drivers/gpu/drm/etnaviv/etnaviv_drv.c | 20 +-
drivers/gpu/drm/exynos/exynos_drm_drv.c | 21 +-
.../gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 20 +-
drivers/gpu/drm/imx/imx-drm-core.c | 20 +-
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 25 +-
drivers/gpu/drm/mcde/mcde_drv.c | 23 +-
drivers/gpu/drm/mediatek/mtk_drm_drv.c | 20 +-
drivers/gpu/drm/meson/meson_drv.c | 21 +-
drivers/gpu/drm/msm/msm_drv.c | 46 +-
drivers/gpu/drm/omapdrm/dss/dss.c | 20 +-
drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 20 +-
drivers/gpu/drm/sti/sti_drv.c | 20 +-
drivers/gpu/drm/sun4i/sun4i_drv.c | 26 +-
drivers/gpu/drm/tilcdc/tilcdc_drv.c | 28 +-
drivers/gpu/drm/vc4/vc4_drv.c | 20 +-
drivers/iommu/mtk_iommu.c | 14 +-
drivers/iommu/mtk_iommu.h | 6 +-
drivers/iommu/mtk_iommu_v1.c | 14 +-
drivers/misc/mei/hdcp/mei_hdcp.c | 22 +-
drivers/misc/mei/pxp/mei_pxp.c | 22 +-
drivers/power/supply/ab8500_charger.c | 22 +-
drivers/video/fbdev/omap2/omapfb/dss/dss.c | 20 +-
include/drm/drm_of.h | 10 +-
include/linux/component.h | 92 ++-
sound/hda/hdac_component.c | 21 +-
sound/soc/codecs/wcd938x.c | 20 +-
33 files changed, 772 insertions(+), 490 deletions(-)

Cc: Arnd Bergmann <[email protected]>
Cc: Chen Feng <[email protected]>
Cc: Chen-Yu Tsai <[email protected]>
Cc: Christian Gmeiner <[email protected]>
Cc: Chun-Kuang Hu <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: Emma Anholt <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: "Heiko Stübner" <[email protected]>
Cc: Inki Dae <[email protected]>
Cc: James Qian Wang (Arm Technology China) <[email protected]>
Cc: Jaroslav Kysela <[email protected]>
Cc: Joerg Roedel <[email protected]>
Cc: John Stultz <[email protected]>
Cc: Joonyoung Shim <[email protected]>
Cc: Jyri Sarha <[email protected]>
Cc: Kai Vehmanen <[email protected]>
Cc: Kyungmin Park <[email protected]>
Cc: Laurent Pinchart <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Liviu Dudau <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Maxime Ripard <[email protected]>
Cc: Neil Armstrong <[email protected]>
Cc: Paul Cercueil <[email protected]>
Cc: Philipp Zabel <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Russell King <[email protected]>
Cc: Russell King <[email protected]>
Cc: Sandy Huang <[email protected]>
Cc: Saravana Kannan <[email protected]>
Cc: Sebastian Reichel <[email protected]>
Cc: Seung-Woo Kim <[email protected]>
Cc: Takashi Iwai <[email protected]>
Cc: Tian Tao <[email protected]>
Cc: Tomas Winkler <[email protected]>
Cc: Tomi Valkeinen <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Xinliang Liu <[email protected]>
Cc: Xinwei Kong <[email protected]>
Cc: Yong Wu <[email protected]>
Cc: Vitaly Lubart <[email protected]>
Cc: Daniele Ceraolo Spurio <[email protected]>
Cc: Rodrigo Vivi <[email protected]>
Cc: Kai Vehmanen <[email protected]>

base-commit: 136057256686de39cc3a07c2e39ef6bc43003ff6
--
https://chromeos.dev



2022-01-06 21:46:14

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 01/32] component: Replace most references to 'master' with 'aggregate device'

Remove most references to 'master' in the code and replace them with
some form of 'aggregate device'. This better reflects the reality of
what this code does, i.e. an aggregate device that represents a
device like a GPU card once some set of devices that make up the
aggregate device probe and register with the component framework.

Cc: Daniel Vetter <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Laurent Pinchart <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/base/component.c | 242 +++++++++++++++++++-------------------
include/linux/component.h | 2 +-
2 files changed, 120 insertions(+), 124 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index 2d25a6416587..34f9e0802719 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -1,11 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
/*
* Componentized device handling.
- *
- * This is work in progress. We gather up the component devices into a list,
- * and bind them when instructed. At the moment, we're specific to the DRM
- * subsystem, and only handles one master device, but this doesn't have to be
- * the case.
*/
#include <linux/component.h>
#include <linux/device.h>
@@ -57,7 +52,7 @@ struct component_match {
struct component_match_array *compare;
};

-struct master {
+struct aggregate_device {
struct list_head node;
bool bound;

@@ -68,7 +63,7 @@ struct master {

struct component {
struct list_head node;
- struct master *master;
+ struct aggregate_device *adev;
bool bound;

const struct component_ops *ops;
@@ -78,7 +73,7 @@ struct component {

static DEFINE_MUTEX(component_mutex);
static LIST_HEAD(component_list);
-static LIST_HEAD(masters);
+static LIST_HEAD(aggregate_devices);

#ifdef CONFIG_DEBUG_FS

@@ -86,12 +81,12 @@ static struct dentry *component_debugfs_dir;

static int component_devices_show(struct seq_file *s, void *data)
{
- struct master *m = s->private;
+ struct aggregate_device *m = s->private;
struct component_match *match = m->match;
size_t i;

mutex_lock(&component_mutex);
- seq_printf(s, "%-40s %20s\n", "master name", "status");
+ seq_printf(s, "%-40s %20s\n", "aggregate_device name", "status");
seq_puts(s, "-------------------------------------------------------------\n");
seq_printf(s, "%-40s %20s\n\n",
dev_name(m->parent), m->bound ? "bound" : "not bound");
@@ -121,46 +116,46 @@ static int __init component_debug_init(void)

core_initcall(component_debug_init);

-static void component_master_debugfs_add(struct master *m)
+static void component_debugfs_add(struct aggregate_device *m)
{
debugfs_create_file(dev_name(m->parent), 0444, component_debugfs_dir, m,
&component_devices_fops);
}

-static void component_master_debugfs_del(struct master *m)
+static void component_debugfs_del(struct aggregate_device *m)
{
debugfs_remove(debugfs_lookup(dev_name(m->parent), component_debugfs_dir));
}

#else

-static void component_master_debugfs_add(struct master *m)
+static void component_debugfs_add(struct aggregate_device *m)
{ }

-static void component_master_debugfs_del(struct master *m)
+static void component_debugfs_del(struct aggregate_device *m)
{ }

#endif

-static struct master *__master_find(struct device *parent,
+static struct aggregate_device *__aggregate_find(struct device *parent,
const struct component_master_ops *ops)
{
- struct master *m;
+ struct aggregate_device *m;

- list_for_each_entry(m, &masters, node)
+ list_for_each_entry(m, &aggregate_devices, node)
if (m->parent == parent && (!ops || m->ops == ops))
return m;

return NULL;
}

-static struct component *find_component(struct master *master,
+static struct component *find_component(struct aggregate_device *adev,
struct component_match_array *mc)
{
struct component *c;

list_for_each_entry(c, &component_list, node) {
- if (c->master && c->master != master)
+ if (c->adev && c->adev != adev)
continue;

if (mc->compare && mc->compare(c->dev, mc->data))
@@ -174,102 +169,103 @@ static struct component *find_component(struct master *master,
return NULL;
}

-static int find_components(struct master *master)
+static int find_components(struct aggregate_device *adev)
{
- struct component_match *match = master->match;
+ struct component_match *match = adev->match;
size_t i;
int ret = 0;

/*
* Scan the array of match functions and attach
- * any components which are found to this master.
+ * any components which are found to this adev.
*/
for (i = 0; i < match->num; i++) {
struct component_match_array *mc = &match->compare[i];
struct component *c;

- dev_dbg(master->parent, "Looking for component %zu\n", i);
+ dev_dbg(adev->parent, "Looking for component %zu\n", i);

if (match->compare[i].component)
continue;

- c = find_component(master, mc);
+ c = find_component(adev, mc);
if (!c) {
ret = -ENXIO;
break;
}

- dev_dbg(master->parent, "found component %s, duplicate %u\n", dev_name(c->dev), !!c->master);
+ dev_dbg(adev->parent, "found component %s, duplicate %u\n",
+ dev_name(c->dev), !!c->adev);

- /* Attach this component to the master */
- match->compare[i].duplicate = !!c->master;
+ /* Attach this component to the adev */
+ match->compare[i].duplicate = !!c->adev;
match->compare[i].component = c;
- c->master = master;
+ c->adev = adev;
}
return ret;
}

-/* Detach component from associated master */
-static void remove_component(struct master *master, struct component *c)
+/* Detach component from associated aggregate_device */
+static void remove_component(struct aggregate_device *adev, struct component *c)
{
size_t i;

- /* Detach the component from this master. */
- for (i = 0; i < master->match->num; i++)
- if (master->match->compare[i].component == c)
- master->match->compare[i].component = NULL;
+ /* Detach the component from this adev. */
+ for (i = 0; i < adev->match->num; i++)
+ if (adev->match->compare[i].component == c)
+ adev->match->compare[i].component = NULL;
}

/*
- * Try to bring up a master. If component is NULL, we're interested in
- * this master, otherwise it's a component which must be present to try
- * and bring up the master.
+ * Try to bring up an aggregate device. If component is NULL, we're interested
+ * in this aggregate device, otherwise it's a component which must be present
+ * to try and bring up the aggregate device.
*
* Returns 1 for successful bringup, 0 if not ready, or -ve errno.
*/
-static int try_to_bring_up_master(struct master *master,
+static int try_to_bring_up_aggregate_device(struct aggregate_device *adev,
struct component *component)
{
int ret;

- dev_dbg(master->parent, "trying to bring up master\n");
+ dev_dbg(adev->parent, "trying to bring up adev\n");

- if (find_components(master)) {
- dev_dbg(master->parent, "master has incomplete components\n");
+ if (find_components(adev)) {
+ dev_dbg(adev->parent, "master has incomplete components\n");
return 0;
}

- if (component && component->master != master) {
- dev_dbg(master->parent, "master is not for this component (%s)\n",
+ if (component && component->adev != adev) {
+ dev_dbg(adev->parent, "master is not for this component (%s)\n",
dev_name(component->dev));
return 0;
}

- if (!devres_open_group(master->parent, master, GFP_KERNEL))
+ if (!devres_open_group(adev->parent, adev, GFP_KERNEL))
return -ENOMEM;

/* Found all components */
- ret = master->ops->bind(master->parent);
+ ret = adev->ops->bind(adev->parent);
if (ret < 0) {
- devres_release_group(master->parent, NULL);
+ devres_release_group(adev->parent, NULL);
if (ret != -EPROBE_DEFER)
- dev_info(master->parent, "master bind failed: %d\n", ret);
+ dev_info(adev->parent, "adev bind failed: %d\n", ret);
return ret;
}

- devres_close_group(master->parent, NULL);
- master->bound = true;
+ devres_close_group(adev->parent, NULL);
+ adev->bound = true;
return 1;
}

static int try_to_bring_up_masters(struct component *component)
{
- struct master *m;
+ struct aggregate_device *adev;
int ret = 0;

- list_for_each_entry(m, &masters, node) {
- if (!m->bound) {
- ret = try_to_bring_up_master(m, component);
+ list_for_each_entry(adev, &aggregate_devices, node) {
+ if (!adev->bound) {
+ ret = try_to_bring_up_aggregate_device(adev, component);
if (ret != 0)
break;
}
@@ -278,12 +274,12 @@ static int try_to_bring_up_masters(struct component *component)
return ret;
}

-static void take_down_master(struct master *master)
+static void take_down_aggregate_device(struct aggregate_device *adev)
{
- if (master->bound) {
- master->ops->unbind(master->parent);
- devres_release_group(master->parent, master);
- master->bound = false;
+ if (adev->bound) {
+ adev->ops->unbind(adev->parent);
+ devres_release_group(adev->parent, adev);
+ adev->bound = false;
}
}

@@ -324,7 +320,7 @@ static int component_match_realloc(struct component_match *match, size_t num)
return 0;
}

-static void __component_match_add(struct device *master,
+static void __component_match_add(struct device *parent,
struct component_match **matchptr,
void (*release)(struct device *, void *),
int (*compare)(struct device *, void *),
@@ -344,7 +340,7 @@ static void __component_match_add(struct device *master,
return;
}

- devres_add(master, match);
+ devres_add(parent, match);

*matchptr = match;
}
@@ -370,13 +366,13 @@ static void __component_match_add(struct device *master,

/**
* component_match_add_release - add a component match entry with release callback
- * @master: device with the aggregate driver
+ * @parent: parent device of the aggregate driver
* @matchptr: pointer to the list of component matches
* @release: release function for @compare_data
* @compare: compare function to match against all components
* @compare_data: opaque pointer passed to the @compare function
*
- * Adds a new component match to the list stored in @matchptr, which the @master
+ * Adds a new component match to the list stored in @matchptr, which the
* aggregate driver needs to function. The list of component matches pointed to
* by @matchptr must be initialized to NULL before adding the first match. This
* only matches against components added with component_add().
@@ -388,24 +384,24 @@ static void __component_match_add(struct device *master,
*
* See also component_match_add() and component_match_add_typed().
*/
-void component_match_add_release(struct device *master,
+void component_match_add_release(struct device *parent,
struct component_match **matchptr,
void (*release)(struct device *, void *),
int (*compare)(struct device *, void *), void *compare_data)
{
- __component_match_add(master, matchptr, release, compare, NULL,
+ __component_match_add(parent, matchptr, release, compare, NULL,
compare_data);
}
EXPORT_SYMBOL(component_match_add_release);

/**
* component_match_add_typed - add a component match entry for a typed component
- * @master: device with the aggregate driver
+ * @parent: parent device of the aggregate driver
* @matchptr: pointer to the list of component matches
* @compare_typed: compare function to match against all typed components
* @compare_data: opaque pointer passed to the @compare function
*
- * Adds a new component match to the list stored in @matchptr, which the @master
+ * Adds a new component match to the list stored in @matchptr, which the
* aggregate driver needs to function. The list of component matches pointed to
* by @matchptr must be initialized to NULL before adding the first match. This
* only matches against components added with component_add_typed().
@@ -415,32 +411,32 @@ EXPORT_SYMBOL(component_match_add_release);
*
* See also component_match_add_release() and component_match_add_typed().
*/
-void component_match_add_typed(struct device *master,
+void component_match_add_typed(struct device *parent,
struct component_match **matchptr,
int (*compare_typed)(struct device *, int, void *), void *compare_data)
{
- __component_match_add(master, matchptr, NULL, NULL, compare_typed,
+ __component_match_add(parent, matchptr, NULL, NULL, compare_typed,
compare_data);
}
EXPORT_SYMBOL(component_match_add_typed);

-static void free_master(struct master *master)
+static void free_aggregate_device(struct aggregate_device *adev)
{
- struct component_match *match = master->match;
+ struct component_match *match = adev->match;
int i;

- component_master_debugfs_del(master);
- list_del(&master->node);
+ component_debugfs_del(adev);
+ list_del(&adev->node);

if (match) {
for (i = 0; i < match->num; i++) {
struct component *c = match->compare[i].component;
if (c)
- c->master = NULL;
+ c->adev = NULL;
}
}

- kfree(master);
+ kfree(adev);
}

/**
@@ -459,7 +455,7 @@ int component_master_add_with_match(struct device *parent,
const struct component_master_ops *ops,
struct component_match *match)
{
- struct master *master;
+ struct aggregate_device *adev;
int ret;

/* Reallocate the match array for its true size */
@@ -467,23 +463,23 @@ int component_master_add_with_match(struct device *parent,
if (ret)
return ret;

- master = kzalloc(sizeof(*master), GFP_KERNEL);
- if (!master)
+ adev = kzalloc(sizeof(*adev), GFP_KERNEL);
+ if (!adev)
return -ENOMEM;

- master->parent = parent;
- master->ops = ops;
- master->match = match;
+ adev->parent = parent;
+ adev->ops = ops;
+ adev->match = match;

- component_master_debugfs_add(master);
- /* Add to the list of available masters. */
+ component_debugfs_add(adev);
+ /* Add to the list of available aggregate devices. */
mutex_lock(&component_mutex);
- list_add(&master->node, &masters);
+ list_add(&adev->node, &aggregate_devices);

- ret = try_to_bring_up_master(master, NULL);
+ ret = try_to_bring_up_aggregate_device(adev, NULL);

if (ret < 0)
- free_master(master);
+ free_aggregate_device(adev);

mutex_unlock(&component_mutex);

@@ -503,25 +499,25 @@ EXPORT_SYMBOL_GPL(component_master_add_with_match);
void component_master_del(struct device *parent,
const struct component_master_ops *ops)
{
- struct master *master;
+ struct aggregate_device *adev;

mutex_lock(&component_mutex);
- master = __master_find(parent, ops);
- if (master) {
- take_down_master(master);
- free_master(master);
+ adev = __aggregate_find(parent, ops);
+ if (adev) {
+ take_down_aggregate_device(adev);
+ free_aggregate_device(adev);
}
mutex_unlock(&component_mutex);
}
EXPORT_SYMBOL_GPL(component_master_del);

static void component_unbind(struct component *component,
- struct master *master, void *data)
+ struct aggregate_device *adev, void *data)
{
WARN_ON(!component->bound);

if (component->ops && component->ops->unbind)
- component->ops->unbind(component->dev, master->parent, data);
+ component->ops->unbind(component->dev, adev->parent, data);
component->bound = false;

/* Release all resources claimed in the binding of this component */
@@ -539,26 +535,26 @@ static void component_unbind(struct component *component,
*/
void component_unbind_all(struct device *parent, void *data)
{
- struct master *master;
+ struct aggregate_device *adev;
struct component *c;
size_t i;

WARN_ON(!mutex_is_locked(&component_mutex));

- master = __master_find(parent, NULL);
- if (!master)
+ adev = __aggregate_find(parent, NULL);
+ if (!adev)
return;

/* Unbind components in reverse order */
- for (i = master->match->num; i--; )
- if (!master->match->compare[i].duplicate) {
- c = master->match->compare[i].component;
- component_unbind(c, master, data);
+ for (i = adev->match->num; i--; )
+ if (!adev->match->compare[i].duplicate) {
+ c = adev->match->compare[i].component;
+ component_unbind(c, adev, data);
}
}
EXPORT_SYMBOL_GPL(component_unbind_all);

-static int component_bind(struct component *component, struct master *master,
+static int component_bind(struct component *component, struct aggregate_device *adev,
void *data)
{
int ret;
@@ -568,7 +564,7 @@ static int component_bind(struct component *component, struct master *master,
* This allows us to roll-back a failed component without
* affecting anything else.
*/
- if (!devres_open_group(master->parent, NULL, GFP_KERNEL))
+ if (!devres_open_group(adev->parent, NULL, GFP_KERNEL))
return -ENOMEM;

/*
@@ -577,14 +573,14 @@ static int component_bind(struct component *component, struct master *master,
* at the appropriate moment.
*/
if (!devres_open_group(component->dev, component, GFP_KERNEL)) {
- devres_release_group(master->parent, NULL);
+ devres_release_group(adev->parent, NULL);
return -ENOMEM;
}

- dev_dbg(master->parent, "binding %s (ops %ps)\n",
+ dev_dbg(adev->parent, "binding %s (ops %ps)\n",
dev_name(component->dev), component->ops);

- ret = component->ops->bind(component->dev, master->parent, data);
+ ret = component->ops->bind(component->dev, adev->parent, data);
if (!ret) {
component->bound = true;

@@ -595,16 +591,16 @@ static int component_bind(struct component *component, struct master *master,
* can clean those resources up independently.
*/
devres_close_group(component->dev, NULL);
- devres_remove_group(master->parent, NULL);
+ devres_remove_group(adev->parent, NULL);

- dev_info(master->parent, "bound %s (ops %ps)\n",
+ dev_info(adev->parent, "bound %s (ops %ps)\n",
dev_name(component->dev), component->ops);
} else {
devres_release_group(component->dev, NULL);
- devres_release_group(master->parent, NULL);
+ devres_release_group(adev->parent, NULL);

if (ret != -EPROBE_DEFER)
- dev_err(master->parent, "failed to bind %s (ops %ps): %d\n",
+ dev_err(adev->parent, "failed to bind %s (ops %ps): %d\n",
dev_name(component->dev), component->ops, ret);
}

@@ -622,31 +618,31 @@ static int component_bind(struct component *component, struct master *master,
*/
int component_bind_all(struct device *parent, void *data)
{
- struct master *master;
+ struct aggregate_device *adev;
struct component *c;
size_t i;
int ret = 0;

WARN_ON(!mutex_is_locked(&component_mutex));

- master = __master_find(parent, NULL);
- if (!master)
+ adev = __aggregate_find(parent, NULL);
+ if (!adev)
return -EINVAL;

/* Bind components in match order */
- for (i = 0; i < master->match->num; i++)
- if (!master->match->compare[i].duplicate) {
- c = master->match->compare[i].component;
- ret = component_bind(c, master, data);
+ for (i = 0; i < adev->match->num; i++)
+ if (!adev->match->compare[i].duplicate) {
+ c = adev->match->compare[i].component;
+ ret = component_bind(c, adev, data);
if (ret)
break;
}

if (ret != 0) {
for (; i > 0; i--)
- if (!master->match->compare[i - 1].duplicate) {
- c = master->match->compare[i - 1].component;
- component_unbind(c, master, data);
+ if (!adev->match->compare[i - 1].duplicate) {
+ c = adev->match->compare[i - 1].component;
+ component_unbind(c, adev, data);
}
}

@@ -675,8 +671,8 @@ static int __component_add(struct device *dev, const struct component_ops *ops,

ret = try_to_bring_up_masters(component);
if (ret < 0) {
- if (component->master)
- remove_component(component->master, component);
+ if (component->adev)
+ remove_component(component->adev, component);
list_del(&component->node);

kfree(component);
@@ -757,9 +753,9 @@ void component_del(struct device *dev, const struct component_ops *ops)
break;
}

- if (component && component->master) {
- take_down_master(component->master);
- remove_component(component->master, component);
+ if (component && component->adev) {
+ take_down_aggregate_device(component->adev);
+ remove_component(component->adev, component);
}

mutex_unlock(&component_mutex);
diff --git a/include/linux/component.h b/include/linux/component.h
index 16de18f473d7..71bfc3862633 100644
--- a/include/linux/component.h
+++ b/include/linux/component.h
@@ -41,7 +41,7 @@ void component_del(struct device *, const struct component_ops *);
int component_bind_all(struct device *master, void *master_data);
void component_unbind_all(struct device *master, void *master_data);

-struct master;
+struct aggregate_device;

/**
* struct component_master_ops - callback for the aggregate driver
--
https://chromeos.dev


2022-01-06 21:46:18

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 03/32] component: Move struct aggregate_device out to header file

This allows aggregate driver writers to use the device passed to their
probe/remove/shutdown functions properly instead of treating it as an
opaque pointer.

Cc: Daniel Vetter <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Laurent Pinchart <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/base/component.c | 15 ---------------
include/linux/component.h | 19 ++++++++++++++++---
2 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index eec82caeae5e..dc38a8939ae6 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -56,21 +56,6 @@ struct component_match {
struct component_match_array *compare;
};

-struct aggregate_device {
- const struct component_master_ops *ops;
- struct device *parent;
- struct device dev;
- struct component_match *match;
- struct aggregate_driver *adrv;
-
- int id;
-};
-
-static inline struct aggregate_device *to_aggregate_device(struct device *d)
-{
- return container_of(d, struct aggregate_device, dev);
-}
-
struct component {
struct list_head node;
struct aggregate_device *adev;
diff --git a/include/linux/component.h b/include/linux/component.h
index 95d1b23ede8a..e99cf8e910f0 100644
--- a/include/linux/component.h
+++ b/include/linux/component.h
@@ -5,6 +5,8 @@
#include <linux/stddef.h>
#include <linux/device.h>

+struct component_match;
+
/**
* struct component_ops - callbacks for component drivers
*
@@ -39,8 +41,6 @@ void component_del(struct device *, const struct component_ops *);
int component_bind_all(struct device *master, void *master_data);
void component_unbind_all(struct device *master, void *master_data);

-struct aggregate_device;
-
/**
* struct component_master_ops - callback for the aggregate driver
*
@@ -80,7 +80,20 @@ struct component_master_ops {
void (*unbind)(struct device *master);
};

-struct component_match;
+struct aggregate_device {
+ const struct component_master_ops *ops;
+ struct device *parent;
+ struct device dev;
+ struct component_match *match;
+ struct aggregate_driver *adrv;
+
+ int id;
+};
+
+static inline struct aggregate_device *to_aggregate_device(struct device *d)
+{
+ return container_of(d, struct aggregate_device, dev);
+}

/**
* struct aggregate_driver - Aggregate driver (made up of other drivers)
--
https://chromeos.dev


2022-01-06 21:46:24

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 05/32] drm/of: Add a drm_of_aggregate_probe() API

Similar to drm_of_component_probe() but using the new API that registers
a driver instead of an ops struct. This allows us to migrate the users
of drm_of_component_probe() to the new way of doing things.

Cc: Laurent Pinchart <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/drm_of.c | 85 +++++++++++++++++++++++++++++++---------
include/drm/drm_of.h | 12 ++++++
2 files changed, 78 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 37c34146eea8..008d6b7d2283 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -99,30 +99,18 @@ void drm_of_component_match_add(struct device *master,
}
EXPORT_SYMBOL_GPL(drm_of_component_match_add);

-/**
- * drm_of_component_probe - Generic probe function for a component based master
- * @dev: master device containing the OF node
- * @compare_of: compare function used for matching components
- * @m_ops: component master ops to be used
- *
- * Parse the platform device OF node and bind all the components associated
- * with the master. Interface ports are added before the encoders in order to
- * satisfy their .bind requirements
- * See Documentation/devicetree/bindings/graph.txt for the bindings.
- *
- * Returns zero if successful, or one of the standard error codes if it fails.
- */
-int drm_of_component_probe(struct device *dev,
+static int _drm_of_component_probe(struct device *dev,
int (*compare_of)(struct device *, void *),
- const struct component_master_ops *m_ops)
+ struct component_match **matchptr)
{
struct device_node *ep, *port, *remote;
- struct component_match *match = NULL;
int i;

if (!dev->of_node)
return -EINVAL;

+ *matchptr = NULL;
+
/*
* Bind the crtc's ports first, so that drm_of_find_possible_crtcs()
* called from encoder's .bind callbacks works as expected
@@ -133,7 +121,7 @@ int drm_of_component_probe(struct device *dev,
break;

if (of_device_is_available(port->parent))
- drm_of_component_match_add(dev, &match, compare_of,
+ drm_of_component_match_add(dev, matchptr, compare_of,
port);

of_node_put(port);
@@ -144,7 +132,7 @@ int drm_of_component_probe(struct device *dev,
return -ENODEV;
}

- if (!match) {
+ if (!*matchptr) {
dev_err(dev, "no available port\n");
return -ENODEV;
}
@@ -174,17 +162,76 @@ int drm_of_component_probe(struct device *dev,
continue;
}

- drm_of_component_match_add(dev, &match, compare_of,
+ drm_of_component_match_add(dev, matchptr, compare_of,
remote);
of_node_put(remote);
}
of_node_put(port);
}

+ return 0;
+}
+
+/**
+ * drm_of_component_probe - Generic probe function for a component based master
+ * @dev: master device containing the OF node
+ * @compare_of: compare function used for matching components
+ * @m_ops: component master ops to be used
+ *
+ * Parse the platform device OF node and bind all the components associated
+ * with the master. Interface ports are added before the encoders in order to
+ * satisfy their .bind requirements
+ * See Documentation/devicetree/bindings/graph.txt for the bindings.
+ *
+ * Deprecated: Use drm_of_aggregate_probe() instead.
+ *
+ * Returns zero if successful, or one of the standard error codes if it fails.
+ */
+int drm_of_component_probe(struct device *dev,
+ int (*compare_of)(struct device *, void *),
+ const struct component_master_ops *m_ops)
+{
+
+ struct component_match *match;
+ int ret;
+
+ ret = _drm_of_component_probe(dev, compare_of, &match);
+ if (ret)
+ return ret;
+
return component_master_add_with_match(dev, m_ops, match);
}
EXPORT_SYMBOL(drm_of_component_probe);

+
+/**
+ * drm_of_aggregate_probe - Generic probe function for a component based aggregate host
+ * @dev: device containing the OF node
+ * @compare_of: compare function used for matching components
+ * @adrv: aggregate driver to be used
+ *
+ * Parse the platform device OF node and bind all the components associated
+ * with the aggregate device. Interface ports are added before the encoders in
+ * order to satisfy their .bind_component requirements
+ * See Documentation/devicetree/bindings/graph.txt for the bindings.
+ *
+ * Returns zero if successful, or one of the standard error codes if it fails.
+ */
+int drm_of_aggregate_probe(struct device *dev,
+ int (*compare_of)(struct device *, void *),
+ struct aggregate_driver *adrv)
+{
+ struct component_match *match;
+ int ret;
+
+ ret = _drm_of_component_probe(dev, compare_of, &match);
+ if (ret)
+ return ret;
+
+ return component_aggregate_register(dev, adrv, match);
+}
+EXPORT_SYMBOL(drm_of_aggregate_probe);
+
/*
* drm_of_encoder_active_endpoint - return the active encoder endpoint
* @node: device tree node containing encoder input ports
diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
index b9b093add92e..9d35a141f888 100644
--- a/include/drm/drm_of.h
+++ b/include/drm/drm_of.h
@@ -7,6 +7,7 @@
#include <drm/drm_bridge.h>
#endif

+struct aggregate_driver;
struct component_master_ops;
struct component_match;
struct device;
@@ -40,6 +41,9 @@ void drm_of_component_match_add(struct device *master,
int drm_of_component_probe(struct device *dev,
int (*compare_of)(struct device *, void *),
const struct component_master_ops *m_ops);
+int drm_of_aggregate_probe(struct device *dev,
+ int (*compare_of)(struct device *, void *),
+ struct aggregate_driver *adrv);
int drm_of_encoder_active_endpoint(struct device_node *node,
struct drm_encoder *encoder,
struct of_endpoint *endpoint);
@@ -78,6 +82,14 @@ drm_of_component_probe(struct device *dev,
return -EINVAL;
}

+static inline int
+drm_of_aggregate_probe(struct device *dev,
+ int (*compare_of)(struct device *, void *),
+ struct aggregate_driver *adrv)
+{
+ return -EINVAL;
+}
+
static inline int drm_of_encoder_active_endpoint(struct device_node *node,
struct drm_encoder *encoder,
struct of_endpoint *endpoint)
--
https://chromeos.dev


2022-01-06 21:46:28

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 06/32] drm/msm: Migrate to aggregate driver

The device lists are poorly ordered when the component device code is
used. This is because component_master_add_with_match() returns 0
regardless of component devices calling component_add() first. It can
really only fail if an allocation fails, in which case everything is
going bad and we're out of memory. The driver that registers the
aggregate driver, can succeed at probe and put the attached device on
the DPM lists before any of the component devices are probed and put on
the lists.

Within the component device framework this usually isn't that bad
because the real driver work is done at bind time via
component{,master}_ops::bind(). It becomes a problem when the driver
core, or host driver, wants to operate on the component device outside
of the bind/unbind functions, e.g. via 'remove' or 'shutdown'. The
driver core doesn't understand the relationship between the host device
and the component devices and could possibly try to operate on component
devices when they're already removed from the system or shut down.

Normally, device links or probe defer would reorder the lists and put
devices that depend on other devices in the lists at the correct
location, but with component devices this doesn't happen because this
information isn't expressed anywhere. Drivers simply succeed at
registering their component or the aggregate driver with the component
framework and wait for their bind() callback to be called once the other
components are ready. In summary, the drivers that make up the aggregate
driver can probe in any order.

This ordering problem becomes fairly obvious when shutting down the
device with a DSI controller connected to a DSI bridge that is
controlled via i2c. In this case, the msm display driver wants to tear
down the display pipeline on shutdown via msm_pdev_shutdown() by calling
drm_atomic_helper_shutdown(), and it can't do that unless the whole
display chain is still probed and active in the system. When a display
bridge is on i2c, the i2c device for the bridge will be created whenever
the i2c controller probes, which could be before or after the msm
display driver probes. If the i2c controller probes after the display
driver, then the i2c controller will be shutdown before the display
controller during system wide shutdown and thus i2c transactions will
stop working before the display pipeline is shut down. This means we'll
have the display bridge trying to access an i2c bus that's shut down
because drm_atomic_helper_shutdown() is trying to disable the bridge
after the bridge is off.

The solution is to make the aggregate driver into a real struct driver
that is bound to a device when the other component devices have all
probed. Now that the component driver code is a proper bus, we can
simply register an aggregate driver with that bus via
component_aggregate_register() and then attach the shutdown hook to that
driver to be sure that the shutdown for the display pipeline is called
before any of the component device driver shutdown hooks are called.

Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/msm/msm_drv.c | 46 +++++++++++++++++++----------------
1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 7936e8d498dd..f6e9b0d318f5 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1360,19 +1360,35 @@ static int add_gpu_components(struct device *dev,
return 0;
}

-static int msm_drm_bind(struct device *dev)
+static int msm_drm_bind(struct aggregate_device *adev)
{
- return msm_drm_init(dev, &msm_driver);
+ return msm_drm_init(adev->parent, &msm_driver);
}

-static void msm_drm_unbind(struct device *dev)
+static void msm_drm_unbind(struct aggregate_device *adev)
{
- msm_drm_uninit(dev);
+ msm_drm_uninit(adev->parent);
+}
+
+static void msm_drm_shutdown(struct aggregate_device *adev)
+{
+ struct drm_device *drm = platform_get_drvdata(to_platform_device(adev->parent));
+ struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
+
+ if (!priv || !priv->kms)
+ return;
+
+ drm_atomic_helper_shutdown(drm);
}

-static const struct component_master_ops msm_drm_ops = {
- .bind = msm_drm_bind,
- .unbind = msm_drm_unbind,
+static struct aggregate_driver msm_drm_aggregate_driver = {
+ .probe = msm_drm_bind,
+ .remove = msm_drm_unbind,
+ .shutdown = msm_drm_shutdown,
+ .driver = {
+ .name = "msm_drm",
+ .owner = THIS_MODULE,
+ },
};

/*
@@ -1401,7 +1417,7 @@ static int msm_pdev_probe(struct platform_device *pdev)
if (ret)
goto fail;

- ret = component_master_add_with_match(&pdev->dev, &msm_drm_ops, match);
+ ret = component_aggregate_register(&pdev->dev, &msm_drm_aggregate_driver, match);
if (ret)
goto fail;

@@ -1414,23 +1430,12 @@ static int msm_pdev_probe(struct platform_device *pdev)

static int msm_pdev_remove(struct platform_device *pdev)
{
- component_master_del(&pdev->dev, &msm_drm_ops);
+ component_aggregate_unregister(&pdev->dev, &msm_drm_aggregate_driver);
of_platform_depopulate(&pdev->dev);

return 0;
}

-static void msm_pdev_shutdown(struct platform_device *pdev)
-{
- struct drm_device *drm = platform_get_drvdata(pdev);
- struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
-
- if (!priv || !priv->kms)
- return;
-
- drm_atomic_helper_shutdown(drm);
-}
-
static const struct of_device_id dt_match[] = {
{ .compatible = "qcom,mdp4", .data = (void *)KMS_MDP4 },
{ .compatible = "qcom,mdss", .data = (void *)KMS_MDP5 },
@@ -1446,7 +1451,6 @@ MODULE_DEVICE_TABLE(of, dt_match);
static struct platform_driver msm_platform_driver = {
.probe = msm_pdev_probe,
.remove = msm_pdev_remove,
- .shutdown = msm_pdev_shutdown,
.driver = {
.name = "msm",
.of_match_table = dt_match,
--
https://chromeos.dev


2022-01-06 21:46:26

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 02/32] component: Introduce the aggregate bus_type

The component framework only provides 'bind' and 'unbind' callbacks to
tell the host driver that it is time to assemble the aggregate driver
now that all the components have probed. The component framework doesn't
attempt to resolve runtime PM or suspend/resume ordering, and explicitly
mentions this in the code. This lack of support leads to some pretty
gnarly usages of the 'prepare' and 'complete' power management hooks in
drivers that host the aggregate device, and it fully breaks down when
faced with ordering shutdown between the various components, the
aggregate driver, and the host driver that registers the whole thing.

In a concrete example, the MSM display driver at drivers/gpu/drm/msm is
using 'prepare' and 'complete' to call the drm helpers
drm_mode_config_helper_suspend() and drm_mode_config_helper_resume()
respectively, so that it can move the aggregate driver suspend/resume
callbacks to be before and after the components that make up the drm
device call any suspend/resume hooks they have. This only works as long
as the component devices don't do anything in their own 'prepare' and
'complete' callbacks. If they did, then the ordering would be incorrect
and we would be doing something in the component drivers before the
aggregate driver could do anything. Yuck!

Similarly, when trying to add shutdown support to the MSM driver we run
across a problem where we're trying to shutdown the drm device via
drm_atomic_helper_shutdown(), but some of the devices in the encoder
chain have already been shutdown. This time, the component devices
aren't the problem (although they could be if they did anything in their
shutdown callbacks), but there's a DSI to eDP bridge in the encoder
chain that has already been shutdown before the driver hosting the
aggregate device runs shutdown. The ordering of driver probe is like
this:

1. msm_pdev_probe() (host driver)
2. DSI bridge
3. aggregate bind

When it comes to shutdown we have this order:

1. DSI bridge
2. msm_pdev_shutdown() (host driver)

and so the bridge is already off, but we want to communicate to it to
turn things off on the display during msm_pdev_shutdown(). Double yuck!
Unfortunately, this time we can't split shutdown into multiple phases
and swap msm_pdev_shutdown() with the DSI bridge.

Let's make the component_master_ops into an actual device driver that has
probe/remove/shutdown functions. The driver will only be bound to the
aggregate device once all component drivers have called component_add()
to indicate they're ready to assemble the aggregate driver. This allows
us to attach shutdown logic (and in the future runtime PM logic) to the
aggregate driver so that it runs the hooks in the correct order.

Cc: Daniel Vetter <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Laurent Pinchart <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/base/component.c | 471 ++++++++++++++++++++++++++++----------
include/linux/component.h | 62 ++++-
2 files changed, 402 insertions(+), 131 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index 34f9e0802719..eec82caeae5e 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -4,10 +4,14 @@
*/
#include <linux/component.h>
#include <linux/device.h>
+#include <linux/idr.h>
#include <linux/list.h>
#include <linux/mutex.h>
#include <linux/slab.h>
#include <linux/debugfs.h>
+#include <linux/pm_runtime.h>
+
+#include "base.h"

/**
* DOC: overview
@@ -31,8 +35,8 @@
*
* Aggregate drivers first assemble a component match list of what they need
* using component_match_add(). This is then registered as an aggregate driver
- * using component_master_add_with_match(), and unregistered using
- * component_master_del().
+ * using component_aggregate_register(), and unregistered using
+ * component_aggregate_unregister().
*/

struct component;
@@ -53,14 +57,20 @@ struct component_match {
};

struct aggregate_device {
- struct list_head node;
- bool bound;
-
const struct component_master_ops *ops;
struct device *parent;
+ struct device dev;
struct component_match *match;
+ struct aggregate_driver *adrv;
+
+ int id;
};

+static inline struct aggregate_device *to_aggregate_device(struct device *d)
+{
+ return container_of(d, struct aggregate_device, dev);
+}
+
struct component {
struct list_head node;
struct aggregate_device *adev;
@@ -69,11 +79,12 @@ struct component {
const struct component_ops *ops;
int subcomponent;
struct device *dev;
+ struct device_link *link;
};

static DEFINE_MUTEX(component_mutex);
static LIST_HEAD(component_list);
-static LIST_HEAD(aggregate_devices);
+static DEFINE_IDA(aggregate_ida);

#ifdef CONFIG_DEBUG_FS

@@ -89,7 +100,7 @@ static int component_devices_show(struct seq_file *s, void *data)
seq_printf(s, "%-40s %20s\n", "aggregate_device name", "status");
seq_puts(s, "-------------------------------------------------------------\n");
seq_printf(s, "%-40s %20s\n\n",
- dev_name(m->parent), m->bound ? "bound" : "not bound");
+ dev_name(m->parent), m->dev.driver ? "bound" : "not bound");

seq_printf(s, "%-40s %20s\n", "device name", "status");
seq_puts(s, "-------------------------------------------------------------\n");
@@ -137,16 +148,21 @@ static void component_debugfs_del(struct aggregate_device *m)

#endif

-static struct aggregate_device *__aggregate_find(struct device *parent,
- const struct component_master_ops *ops)
+struct aggregate_bus_find_data {
+ const struct component_master_ops *ops;
+ struct device *parent;
+};
+
+static int aggregate_bus_find_match(struct device *dev, const void *_data)
{
- struct aggregate_device *m;
+ struct aggregate_device *adev = to_aggregate_device(dev);
+ const struct aggregate_bus_find_data *data = _data;

- list_for_each_entry(m, &aggregate_devices, node)
- if (m->parent == parent && (!ops || m->ops == ops))
- return m;
+ if (adev->parent == data->parent &&
+ (!data->ops || adev->ops == data->ops))
+ return 1;

- return NULL;
+ return 0;
}

static struct component *find_component(struct aggregate_device *adev,
@@ -173,7 +189,6 @@ static int find_components(struct aggregate_device *adev)
{
struct component_match *match = adev->match;
size_t i;
- int ret = 0;

/*
* Scan the array of match functions and attach
@@ -182,6 +197,7 @@ static int find_components(struct aggregate_device *adev)
for (i = 0; i < match->num; i++) {
struct component_match_array *mc = &match->compare[i];
struct component *c;
+ bool duplicate;

dev_dbg(adev->parent, "Looking for component %zu\n", i);

@@ -189,20 +205,27 @@ static int find_components(struct aggregate_device *adev)
continue;

c = find_component(adev, mc);
- if (!c) {
- ret = -ENXIO;
- break;
- }
+ if (!c)
+ return 0;

+ duplicate = !!c->adev;
dev_dbg(adev->parent, "found component %s, duplicate %u\n",
- dev_name(c->dev), !!c->adev);
+ dev_name(c->dev), duplicate);

/* Attach this component to the adev */
- match->compare[i].duplicate = !!c->adev;
+ match->compare[i].duplicate = duplicate;
match->compare[i].component = c;
+ if (duplicate)
+ continue;
+
+ /* Matches put in component_del() */
+ get_device(&adev->dev);
+ c->link = device_link_add(&adev->dev, c->dev,
+ DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME);
c->adev = adev;
}
- return ret;
+
+ return 1;
}

/* Detach component from associated aggregate_device */
@@ -216,73 +239,6 @@ static void remove_component(struct aggregate_device *adev, struct component *c)
adev->match->compare[i].component = NULL;
}

-/*
- * Try to bring up an aggregate device. If component is NULL, we're interested
- * in this aggregate device, otherwise it's a component which must be present
- * to try and bring up the aggregate device.
- *
- * Returns 1 for successful bringup, 0 if not ready, or -ve errno.
- */
-static int try_to_bring_up_aggregate_device(struct aggregate_device *adev,
- struct component *component)
-{
- int ret;
-
- dev_dbg(adev->parent, "trying to bring up adev\n");
-
- if (find_components(adev)) {
- dev_dbg(adev->parent, "master has incomplete components\n");
- return 0;
- }
-
- if (component && component->adev != adev) {
- dev_dbg(adev->parent, "master is not for this component (%s)\n",
- dev_name(component->dev));
- return 0;
- }
-
- if (!devres_open_group(adev->parent, adev, GFP_KERNEL))
- return -ENOMEM;
-
- /* Found all components */
- ret = adev->ops->bind(adev->parent);
- if (ret < 0) {
- devres_release_group(adev->parent, NULL);
- if (ret != -EPROBE_DEFER)
- dev_info(adev->parent, "adev bind failed: %d\n", ret);
- return ret;
- }
-
- devres_close_group(adev->parent, NULL);
- adev->bound = true;
- return 1;
-}
-
-static int try_to_bring_up_masters(struct component *component)
-{
- struct aggregate_device *adev;
- int ret = 0;
-
- list_for_each_entry(adev, &aggregate_devices, node) {
- if (!adev->bound) {
- ret = try_to_bring_up_aggregate_device(adev, component);
- if (ret != 0)
- break;
- }
- }
-
- return ret;
-}
-
-static void take_down_aggregate_device(struct aggregate_device *adev)
-{
- if (adev->bound) {
- adev->ops->unbind(adev->parent);
- devres_release_group(adev->parent, adev);
- adev->bound = false;
- }
-}
-
static void devm_component_match_release(struct device *parent, void *res)
{
struct component_match *match = res;
@@ -426,7 +382,6 @@ static void free_aggregate_device(struct aggregate_device *adev)
int i;

component_debugfs_del(adev);
- list_del(&adev->node);

if (match) {
for (i = 0; i < match->num; i++) {
@@ -436,9 +391,186 @@ static void free_aggregate_device(struct aggregate_device *adev)
}
}

+ ida_free(&aggregate_ida, adev->id);
kfree(adev);
}

+static void aggregate_device_release(struct device *dev)
+{
+ struct aggregate_device *adev = to_aggregate_device(dev);
+
+ free_aggregate_device(adev);
+}
+
+static int aggregate_device_match(struct device *dev, struct device_driver *drv)
+{
+ const struct aggregate_driver *adrv = to_aggregate_driver(drv);
+ struct aggregate_device *adev = to_aggregate_device(dev);
+ int ret;
+
+ /* Is this driver associated with this device */
+ if (adrv != adev->adrv)
+ return 0;
+
+ /* Should we start to assemble? */
+ mutex_lock(&component_mutex);
+ ret = find_components(adev);
+ mutex_unlock(&component_mutex);
+
+ return ret;
+}
+
+/* TODO: Remove once all aggregate drivers use component_aggregate_register() */
+static int component_probe_bind(struct aggregate_device *adev)
+{
+ return adev->ops->bind(adev->parent);
+}
+
+static void component_remove_unbind(struct aggregate_device *adev)
+{
+ adev->ops->unbind(adev->parent);
+}
+
+static int aggregate_driver_probe(struct device *dev)
+{
+ const struct aggregate_driver *adrv = to_aggregate_driver(dev->driver);
+ struct aggregate_device *adev = to_aggregate_device(dev);
+ bool modern = adrv->probe != component_probe_bind;
+ int ret;
+
+ /* Only do runtime PM when drivers migrate */
+ if (modern) {
+ pm_runtime_get_noresume(dev);
+ pm_runtime_set_active(dev);
+ pm_runtime_enable(dev);
+ }
+
+ mutex_lock(&component_mutex);
+ if (devres_open_group(adev->parent, adev, GFP_KERNEL)) {
+ ret = adrv->probe(adev);
+ if (ret)
+ devres_release_group(adev->parent, NULL);
+ } else {
+ ret = -ENOMEM;
+ }
+ devres_close_group(adev->parent, NULL);
+ mutex_unlock(&component_mutex);
+
+ if (ret && modern) {
+ pm_runtime_disable(dev);
+ pm_runtime_set_suspended(dev);
+ pm_runtime_put_noidle(dev);
+ }
+
+ return ret;
+}
+
+static void aggregate_driver_remove(struct device *dev)
+{
+ const struct aggregate_driver *adrv = to_aggregate_driver(dev->driver);
+ struct aggregate_device *adev = to_aggregate_device(dev);
+ bool modern = adrv->remove != component_remove_unbind;
+
+ /* Only do runtime PM when drivers migrate */
+ if (modern)
+ pm_runtime_get_sync(dev);
+ adrv->remove(to_aggregate_device(dev));
+ devres_release_group(adev->parent, adev);
+ if (!modern)
+ return;
+
+ pm_runtime_put_noidle(dev);
+
+ pm_runtime_disable(dev);
+ pm_runtime_set_suspended(dev);
+ pm_runtime_put_noidle(dev);
+}
+
+static void aggregate_driver_shutdown(struct device *dev)
+{
+ const struct aggregate_driver *adrv = to_aggregate_driver(dev->driver);
+
+ if (adrv && adrv->shutdown)
+ adrv->shutdown(to_aggregate_device(dev));
+}
+
+static struct bus_type aggregate_bus_type = {
+ .name = "aggregate",
+ .match = aggregate_device_match,
+ .probe = aggregate_driver_probe,
+ .remove = aggregate_driver_remove,
+ .shutdown = aggregate_driver_shutdown,
+};
+
+/* Callers take ownership of return value, should call put_device() */
+static struct aggregate_device *__aggregate_find(struct device *parent,
+ const struct component_master_ops *ops)
+{
+ struct device *dev;
+ struct aggregate_bus_find_data data = {
+ .ops = ops,
+ .parent = parent,
+ };
+
+ dev = bus_find_device(&aggregate_bus_type, NULL, &data,
+ aggregate_bus_find_match);
+
+ return dev ? to_aggregate_device(dev) : NULL;
+}
+
+static int aggregate_driver_register(struct aggregate_driver *adrv)
+{
+ adrv->driver.bus = &aggregate_bus_type;
+ return driver_register(&adrv->driver);
+}
+
+static void aggregate_driver_unregister(struct aggregate_driver *adrv)
+{
+ driver_unregister(&adrv->driver);
+}
+
+static struct aggregate_device *aggregate_device_add(struct device *parent,
+ const struct component_master_ops *ops, struct aggregate_driver *adrv,
+ struct component_match *match)
+{
+ struct aggregate_device *adev;
+ int ret, id;
+
+ /* Reallocate the match array for its true size */
+ ret = component_match_realloc(match, match->num);
+ if (ret)
+ return ERR_PTR(ret);
+
+ adev = kzalloc(sizeof(*adev), GFP_KERNEL);
+ if (!adev)
+ return ERR_PTR(-ENOMEM);
+
+ id = ida_alloc(&aggregate_ida, GFP_KERNEL);
+ if (id < 0) {
+ kfree(adev);
+ return ERR_PTR(id);
+ }
+
+ adev->id = id;
+ adev->parent = parent;
+ adev->dev.bus = &aggregate_bus_type;
+ adev->dev.release = aggregate_device_release;
+ adev->ops = ops;
+ adev->match = match;
+ adev->adrv = adrv;
+ dev_set_name(&adev->dev, "aggregate%d", id);
+
+ ret = device_register(&adev->dev);
+ if (ret) {
+ put_device(&adev->dev);
+ return ERR_PTR(ret);
+ }
+
+ component_debugfs_add(adev);
+
+ return adev;
+}
+
/**
* component_master_add_with_match - register an aggregate driver
* @parent: parent device of the aggregate driver
@@ -450,42 +582,70 @@ static void free_aggregate_device(struct aggregate_device *adev)
* @match are available, it will be assembled by calling
* &component_master_ops.bind from @ops. Must be unregistered by calling
* component_master_del().
+ *
+ * Deprecated: Use component_aggregate_register() instead.
*/
int component_master_add_with_match(struct device *parent,
const struct component_master_ops *ops,
struct component_match *match)
{
+ struct aggregate_driver *adrv;
struct aggregate_device *adev;
- int ret;
-
- /* Reallocate the match array for its true size */
- ret = component_match_realloc(match, match->num);
- if (ret)
- return ret;
+ int ret = 0;

- adev = kzalloc(sizeof(*adev), GFP_KERNEL);
- if (!adev)
+ adrv = kzalloc(sizeof(*adrv), GFP_KERNEL);
+ if (!adrv)
return -ENOMEM;

- adev->parent = parent;
- adev->ops = ops;
- adev->match = match;
+ adev = aggregate_device_add(parent, ops, adrv, match);
+ if (IS_ERR(adev)) {
+ ret = PTR_ERR(adev);
+ goto err;
+ }

- component_debugfs_add(adev);
- /* Add to the list of available aggregate devices. */
- mutex_lock(&component_mutex);
- list_add(&adev->node, &aggregate_devices);
+ adrv->probe = component_probe_bind;
+ adrv->remove = component_remove_unbind;
+ adrv->driver.owner = THIS_MODULE;
+ adrv->driver.name = dev_name(&adev->dev);

- ret = try_to_bring_up_aggregate_device(adev, NULL);
+ ret = aggregate_driver_register(adrv);
+ if (!ret)
+ return 0;

- if (ret < 0)
- free_aggregate_device(adev);
+ put_device(&adev->dev);
+err:
+ kfree(adrv);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(component_master_add_with_match);

- mutex_unlock(&component_mutex);
+/**
+ * component_aggregate_register - register an aggregate driver
+ * @parent: parent device of the aggregate driver
+ * @adrv: aggregate driver to register
+ *
+ * Registers a new aggregate driver consisting of the components added to @adrv.match
+ * by calling one of the component_match_add() functions. Once all components in
+ * @match are available, the aggregate driver will be assembled by calling
+ * &adrv.bind. Must be unregistered by calling component_aggregate_unregister().
+ */
+int component_aggregate_register(struct device *parent,
+ struct aggregate_driver *adrv, struct component_match *match)
+{
+ struct aggregate_device *adev;
+ int ret;
+
+ adev = aggregate_device_add(parent, NULL, adrv, match);
+ if (IS_ERR(adev))
+ return PTR_ERR(adev);

- return ret < 0 ? ret : 0;
+ ret = aggregate_driver_register(adrv);
+ if (ret)
+ put_device(&adev->dev);
+
+ return ret;
}
-EXPORT_SYMBOL_GPL(component_master_add_with_match);
+EXPORT_SYMBOL_GPL(component_aggregate_register);

/**
* component_master_del - unregister an aggregate driver
@@ -495,22 +655,60 @@ EXPORT_SYMBOL_GPL(component_master_add_with_match);
* Unregisters an aggregate driver registered with
* component_master_add_with_match(). If necessary the aggregate driver is first
* disassembled by calling &component_master_ops.unbind from @ops.
+ *
+ * Deprecated: Use component_aggregate_unregister() instead.
*/
void component_master_del(struct device *parent,
const struct component_master_ops *ops)
{
struct aggregate_device *adev;
+ struct aggregate_driver *adrv;
+ struct device_driver *drv;

mutex_lock(&component_mutex);
adev = __aggregate_find(parent, ops);
+ mutex_unlock(&component_mutex);
+
if (adev) {
- take_down_aggregate_device(adev);
- free_aggregate_device(adev);
+ drv = adev->dev.driver;
+ if (drv) {
+ adrv = to_aggregate_driver(drv);
+ aggregate_driver_unregister(adrv);
+ kfree(adrv);
+ }
+
+ device_unregister(&adev->dev);
}
- mutex_unlock(&component_mutex);
+ put_device(&adev->dev);
}
EXPORT_SYMBOL_GPL(component_master_del);

+/**
+ * component_aggregate_unregister - unregister an aggregate driver
+ * @parent: parent device of the aggregate driver
+ * @adrv: registered aggregate driver
+ *
+ * Unregisters an aggregate driver registered with
+ * component_aggregate_register(). If necessary the aggregate driver is first
+ * disassembled.
+ */
+void component_aggregate_unregister(struct device *parent,
+ struct aggregate_driver *adrv)
+{
+ struct aggregate_device *adev;
+
+ mutex_lock(&component_mutex);
+ adev = __aggregate_find(parent, NULL);
+ mutex_unlock(&component_mutex);
+
+ if (adev)
+ device_unregister(&adev->dev);
+ put_device(&adev->dev);
+
+ aggregate_driver_unregister(adrv);
+}
+EXPORT_SYMBOL_GPL(component_aggregate_unregister);
+
static void component_unbind(struct component *component,
struct aggregate_device *adev, void *data)
{
@@ -551,6 +749,8 @@ void component_unbind_all(struct device *parent, void *data)
c = adev->match->compare[i].component;
component_unbind(c, adev, data);
}
+
+ put_device(&adev->dev);
}
EXPORT_SYMBOL_GPL(component_unbind_all);

@@ -645,6 +845,7 @@ int component_bind_all(struct device *parent, void *data)
component_unbind(c, adev, data);
}
}
+ put_device(&adev->dev);

return ret;
}
@@ -668,18 +869,22 @@ static int __component_add(struct device *dev, const struct component_ops *ops,

mutex_lock(&component_mutex);
list_add_tail(&component->node, &component_list);
-
- ret = try_to_bring_up_masters(component);
- if (ret < 0) {
- if (component->adev)
- remove_component(component->adev, component);
- list_del(&component->node);
-
- kfree(component);
- }
mutex_unlock(&component_mutex);

- return ret < 0 ? ret : 0;
+ /*
+ * Try to bind.
+ *
+ * Note: we don't check the return value here because component devices
+ * don't care that the aggregate device can actually probe or not. They
+ * only care about adding themselves to the component_list and then
+ * waiting for their component_ops::bind_component callback to be
+ * called.
+ */
+ ret = bus_rescan_devices(&aggregate_bus_type);
+ if (ret)
+ dev_dbg(dev, "rescan returned %d\n", ret);
+
+ return 0;
}

/**
@@ -743,6 +948,7 @@ EXPORT_SYMBOL_GPL(component_add);
*/
void component_del(struct device *dev, const struct component_ops *ops)
{
+ struct aggregate_device *adev = NULL;
struct component *c, *component = NULL;

mutex_lock(&component_mutex);
@@ -754,13 +960,26 @@ void component_del(struct device *dev, const struct component_ops *ops)
}

if (component && component->adev) {
- take_down_aggregate_device(component->adev);
- remove_component(component->adev, component);
+ adev = component->adev;
+ remove_component(adev, component);
}

mutex_unlock(&component_mutex);

+ if (adev) {
+ /* Force unbind */
+ device_driver_detach(&adev->dev);
+ device_link_del(component->link);
+ put_device(&adev->dev);
+ }
+
WARN_ON(!component);
kfree(component);
}
EXPORT_SYMBOL_GPL(component_del);
+
+static int __init aggregate_bus_init(void)
+{
+ return bus_register(&aggregate_bus_type);
+}
+postcore_initcall(aggregate_bus_init);
diff --git a/include/linux/component.h b/include/linux/component.h
index 71bfc3862633..95d1b23ede8a 100644
--- a/include/linux/component.h
+++ b/include/linux/component.h
@@ -3,9 +3,7 @@
#define COMPONENT_H

#include <linux/stddef.h>
-
-
-struct device;
+#include <linux/device.h>

/**
* struct component_ops - callbacks for component drivers
@@ -82,11 +80,65 @@ struct component_master_ops {
void (*unbind)(struct device *master);
};

+struct component_match;
+
+/**
+ * struct aggregate_driver - Aggregate driver (made up of other drivers)
+ * @driver: device driver
+ * @match: component match list
+ */
+struct aggregate_driver {
+ /**
+ * @probe:
+ *
+ * Called when all components or the aggregate driver, as specified in
+ * the @match list are
+ * ready. Usually there are 3 steps to bind an aggregate driver:
+ *
+ * 1. Allocate a struct aggregate_driver.
+ *
+ * 2. Bind all components to the aggregate driver by calling
+ * component_bind_all() with the aggregate driver structure as opaque
+ * pointer data.
+ *
+ * 3. Register the aggregate driver with the subsystem to publish its
+ * interfaces.
+ */
+ int (*probe)(struct aggregate_device *adev);
+ /**
+ * @remove:
+ *
+ * Called when either the aggregate driver, using
+ * component_aggregate_unregister(), or one of its components, using
+ * component_del(), is unregistered.
+ */
+ void (*remove)(struct aggregate_device *adev);
+ /**
+ * @shutdown:
+ *
+ * Called when the system is shutting down.
+ */
+ void (*shutdown)(struct aggregate_device *adev);
+
+ struct device_driver driver;
+};
+
+static inline struct aggregate_driver *to_aggregate_driver(struct device_driver *d)
+{
+ if (!d)
+ return NULL;
+
+ return container_of(d, struct aggregate_driver, driver);
+}
+
+int component_aggregate_register(struct device *parent,
+ struct aggregate_driver *adrv, struct component_match *match);
+void component_aggregate_unregister(struct device *parent,
+ struct aggregate_driver *adrv);
+
void component_master_del(struct device *,
const struct component_master_ops *);

-struct component_match;
-
int component_master_add_with_match(struct device *,
const struct component_master_ops *, struct component_match *);
void component_match_add_release(struct device *master,
--
https://chromeos.dev


2022-01-06 21:46:30

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 04/32] component: Add {bind,unbind}_component() ops that take aggregate device

We'd like to get more device model features in the component framework
so let's pass the struct aggregate_device pointer instead of the parent
device pointer to the component binding functions. This will allow
drivers to inspect and control things related to the aggregate device in
case they need it, and they'll always be able to get back to the device
they were using before by using the 'parent' member of the aggregate
device struct.

Suggested-by: Daniel Vetter <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Laurent Pinchart <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/base/component.c | 14 +++++++++++---
include/linux/component.h | 22 ++++++++++++++++++++++
2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index dc38a8939ae6..e9e58b56cda4 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -699,8 +699,13 @@ static void component_unbind(struct component *component,
{
WARN_ON(!component->bound);

- if (component->ops && component->ops->unbind)
- component->ops->unbind(component->dev, adev->parent, data);
+ if (component->ops) {
+ if (component->ops->unbind)
+ component->ops->unbind(component->dev, adev->parent, data);
+ else if (component->ops->unbind_component)
+ component->ops->unbind_component(component->dev, adev, data);
+ }
+
component->bound = false;

/* Release all resources claimed in the binding of this component */
@@ -765,7 +770,10 @@ static int component_bind(struct component *component, struct aggregate_device *
dev_dbg(adev->parent, "binding %s (ops %ps)\n",
dev_name(component->dev), component->ops);

- ret = component->ops->bind(component->dev, adev->parent, data);
+ if (component->ops->bind_component)
+ ret = component->ops->bind_component(component->dev, adev, data);
+ else
+ ret = component->ops->bind(component->dev, adev->parent, data);
if (!ret) {
component->bound = true;

diff --git a/include/linux/component.h b/include/linux/component.h
index e99cf8e910f0..d8dcbf9733da 100644
--- a/include/linux/component.h
+++ b/include/linux/component.h
@@ -6,6 +6,7 @@
#include <linux/device.h>

struct component_match;
+struct aggregate_device;

/**
* struct component_ops - callbacks for component drivers
@@ -19,18 +20,39 @@ struct component_ops {
*
* Called through component_bind_all() when the aggregate driver is
* ready to bind the overall driver.
+ *
+ * Deprecated: Use bind_component() instead.
*/
int (*bind)(struct device *comp, struct device *master,
void *master_data);
+ /**
+ * @bind_component:
+ *
+ * Called through component_bind_all() when the aggregate driver is
+ * ready to bind the overall driver.
+ */
+ int (*bind_component)(struct device *comp, struct aggregate_device *adev,
+ void *aggregate_data);
/**
* @unbind:
*
* Called through component_unbind_all() when the aggregate driver is
* ready to bind the overall driver, or when component_bind_all() fails
* part-ways through and needs to unbind some already bound components.
+ *
+ * Deprecated: Use unbind_component() instead.
*/
void (*unbind)(struct device *comp, struct device *master,
void *master_data);
+ /**
+ * @unbind_component:
+ *
+ * Called through component_unbind_all() when the aggregate driver is
+ * ready to unbind the overall driver, or when component_bind_all() fails
+ * part-ways through and needs to unbind some already bound components.
+ */
+ int (*unbind_component)(struct device *comp, struct aggregate_device *adev,
+ void *aggregate_data);
};

int component_add(struct device *, const struct component_ops *);
--
https://chromeos.dev


2022-01-06 21:46:34

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 11/32] drm/etnaviv: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Cc: Lucas Stach <[email protected]>
Cc: Russell King <[email protected]>
Cc: Christian Gmeiner <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/etnaviv/etnaviv_drv.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 7dcc6392792d..95d1e518ff13 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -494,8 +494,9 @@ static const struct drm_driver etnaviv_drm_driver = {
/*
* Platform driver:
*/
-static int etnaviv_bind(struct device *dev)
+static int etnaviv_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct etnaviv_drm_private *priv;
struct drm_device *drm;
int ret;
@@ -552,8 +553,9 @@ static int etnaviv_bind(struct device *dev)
return ret;
}

-static void etnaviv_unbind(struct device *dev)
+static void etnaviv_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct drm_device *drm = dev_get_drvdata(dev);
struct etnaviv_drm_private *priv = drm->dev_private;

@@ -569,9 +571,13 @@ static void etnaviv_unbind(struct device *dev)
drm_dev_put(drm);
}

-static const struct component_master_ops etnaviv_master_ops = {
- .bind = etnaviv_bind,
- .unbind = etnaviv_unbind,
+static struct aggregate_driver etnaviv_aggregate_driver = {
+ .probe = etnaviv_bind,
+ .remove = etnaviv_unbind,
+ .driver = {
+ .name = "etnaviv_drm",
+ .owner = THIS_MODULE,
+ },
};

static int compare_of(struct device *dev, void *data)
@@ -609,12 +615,12 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
component_match_add(dev, &match, compare_str, names[i]);
}

- return component_master_add_with_match(dev, &etnaviv_master_ops, match);
+ return component_aggregate_register(dev, &etnaviv_aggregate_driver, match);
}

static int etnaviv_pdev_remove(struct platform_device *pdev)
{
- component_master_del(&pdev->dev, &etnaviv_master_ops);
+ component_aggregate_unregister(&pdev->dev, &etnaviv_aggregate_driver);

return 0;
}
--
https://chromeos.dev


2022-01-06 21:46:38

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 10/32] drm/armada: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Cc: Russell King <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/armada/armada_drv.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
index 8e3e98f13db4..b3559363ea43 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -60,8 +60,9 @@ static const struct drm_mode_config_funcs armada_drm_mode_config_funcs = {
.atomic_commit = drm_atomic_helper_commit,
};

-static int armada_drm_bind(struct device *dev)
+static int armada_drm_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct armada_private *priv;
struct resource *mem = NULL;
int ret, n;
@@ -159,8 +160,9 @@ static int armada_drm_bind(struct device *dev)
return ret;
}

-static void armada_drm_unbind(struct device *dev)
+static void armada_drm_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct drm_device *drm = dev_get_drvdata(dev);
struct armada_private *priv = drm_to_armada_dev(drm);

@@ -202,9 +204,13 @@ static void armada_add_endpoints(struct device *dev,
}
}

-static const struct component_master_ops armada_master_ops = {
- .bind = armada_drm_bind,
- .unbind = armada_drm_unbind,
+static struct aggregate_driver armada_aggregate_driver = {
+ .probe = armada_drm_bind,
+ .remove = armada_drm_unbind,
+ .driver = {
+ .name = "armada_drm",
+ .owner = THIS_MODULE,
+ },
};

static int armada_drm_probe(struct platform_device *pdev)
@@ -213,7 +219,7 @@ static int armada_drm_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
int ret;

- ret = drm_of_component_probe(dev, compare_dev_name, &armada_master_ops);
+ ret = drm_of_aggregate_probe(dev, compare_dev_name, &armada_aggregate_driver);
if (ret != -EINVAL)
return ret;

@@ -240,13 +246,12 @@ static int armada_drm_probe(struct platform_device *pdev)
}
}

- return component_master_add_with_match(&pdev->dev, &armada_master_ops,
- match);
+ return component_aggregate_register(&pdev->dev, &armada_aggregate_driver, match);
}

static int armada_drm_remove(struct platform_device *pdev)
{
- component_master_del(&pdev->dev, &armada_master_ops);
+ component_aggregate_unregister(&pdev->dev, &armada_aggregate_driver);
return 0;
}

--
https://chromeos.dev


2022-01-06 21:46:40

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 09/32] drm/malidp: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

TODO: This can be updated to move the drm helper logic into the
aggregate driver shutdown op.

Cc: Laurent Pinchart <[email protected]>
Cc: Liviu Dudau <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/arm/malidp_drv.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 78d15b04b105..e6ee4d1e3bb8 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -702,8 +702,9 @@ static int malidp_runtime_pm_resume(struct device *dev)
return 0;
}

-static int malidp_bind(struct device *dev)
+static int malidp_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct resource *res;
struct drm_device *drm;
struct malidp_drm *malidp;
@@ -894,8 +895,9 @@ static int malidp_bind(struct device *dev)
return ret;
}

-static void malidp_unbind(struct device *dev)
+static void malidp_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct drm_device *drm = dev_get_drvdata(dev);
struct malidp_drm *malidp = drm->dev_private;
struct malidp_hw_device *hwdev = malidp->dev;
@@ -921,9 +923,13 @@ static void malidp_unbind(struct device *dev)
of_reserved_mem_device_release(dev);
}

-static const struct component_master_ops malidp_master_ops = {
- .bind = malidp_bind,
- .unbind = malidp_unbind,
+static struct aggregate_driver malidp_aggregate_driver = {
+ .probe = malidp_bind,
+ .remove = malidp_unbind,
+ .driver = {
+ .name = "malidp_drm",
+ .owner = THIS_MODULE,
+ },
};

static int malidp_compare_dev(struct device *dev, void *data)
@@ -949,13 +955,12 @@ static int malidp_platform_probe(struct platform_device *pdev)
drm_of_component_match_add(&pdev->dev, &match, malidp_compare_dev,
port);
of_node_put(port);
- return component_master_add_with_match(&pdev->dev, &malidp_master_ops,
- match);
+ return component_aggregate_register(&pdev->dev, &malidp_aggregate_driver, match);
}

static int malidp_platform_remove(struct platform_device *pdev)
{
- component_master_del(&pdev->dev, &malidp_master_ops);
+ component_aggregate_unregister(&pdev->dev, &malidp_aggregate_driver);
return 0;
}

--
https://chromeos.dev


2022-01-06 21:46:42

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 12/32] drm/kirin: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Cc: Xinliang Liu <[email protected]>
Cc: Tian Tao <[email protected]>
Cc: John Stultz <[email protected]>
Cc: Xinwei Kong <[email protected]>
Cc: Chen Feng <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
.../gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 20 ++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
index 98ae9a48f3fe..00d47c784cbb 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
@@ -217,8 +217,9 @@ static int kirin_drm_kms_cleanup(struct drm_device *dev)
return 0;
}

-static int kirin_drm_bind(struct device *dev)
+static int kirin_drm_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct kirin_drm_data *driver_data;
struct drm_device *drm_dev;
int ret;
@@ -253,8 +254,9 @@ static int kirin_drm_bind(struct device *dev)
return ret;
}

-static void kirin_drm_unbind(struct device *dev)
+static void kirin_drm_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct drm_device *drm_dev = dev_get_drvdata(dev);

drm_dev_unregister(drm_dev);
@@ -262,9 +264,13 @@ static void kirin_drm_unbind(struct device *dev)
drm_dev_put(drm_dev);
}

-static const struct component_master_ops kirin_drm_ops = {
- .bind = kirin_drm_bind,
- .unbind = kirin_drm_unbind,
+static struct aggregate_driver kirin_drm_aggregate_driver = {
+ .probe = kirin_drm_bind,
+ .remove = kirin_drm_unbind,
+ .driver = {
+ .name = "kirin_drm",
+ .owner = THIS_MODULE,
+ },
};

static int kirin_drm_platform_probe(struct platform_device *pdev)
@@ -281,12 +287,12 @@ static int kirin_drm_platform_probe(struct platform_device *pdev)
drm_of_component_match_add(dev, &match, compare_of, remote);
of_node_put(remote);

- return component_master_add_with_match(dev, &kirin_drm_ops, match);
+ return component_aggregate_register(dev, &kirin_drm_aggregate_driver, match);
}

static int kirin_drm_platform_remove(struct platform_device *pdev)
{
- component_master_del(&pdev->dev, &kirin_drm_ops);
+ component_aggregate_unregister(&pdev->dev, &kirin_drm_aggregate_driver);
return 0;
}

--
https://chromeos.dev


2022-01-06 21:46:45

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 13/32] drm/exynos: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Cc: Inki Dae <[email protected]>
Cc: Joonyoung Shim <[email protected]>
Cc: Seung-Woo Kim <[email protected]>
Cc: Kyungmin Park <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/exynos/exynos_drm_drv.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index d8f1cf4d6b69..dcb52ec2bd35 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -253,8 +253,9 @@ static struct component_match *exynos_drm_match_add(struct device *dev)
return match ?: ERR_PTR(-ENODEV);
}

-static int exynos_drm_bind(struct device *dev)
+static int exynos_drm_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct exynos_drm_private *private;
struct drm_encoder *encoder;
struct drm_device *drm;
@@ -330,8 +331,9 @@ static int exynos_drm_bind(struct device *dev)
return ret;
}

-static void exynos_drm_unbind(struct device *dev)
+static void exynos_drm_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct drm_device *drm = dev_get_drvdata(dev);

drm_dev_unregister(drm);
@@ -350,9 +352,13 @@ static void exynos_drm_unbind(struct device *dev)
drm_dev_put(drm);
}

-static const struct component_master_ops exynos_drm_ops = {
- .bind = exynos_drm_bind,
- .unbind = exynos_drm_unbind,
+static struct aggregate_driver exynos_drm_aggregate_driver = {
+ .probe = exynos_drm_bind,
+ .remove = exynos_drm_unbind,
+ .driver = {
+ .name = "exynos_drm",
+ .owner = THIS_MODULE,
+ },
};

static int exynos_drm_platform_probe(struct platform_device *pdev)
@@ -365,13 +371,12 @@ static int exynos_drm_platform_probe(struct platform_device *pdev)
if (IS_ERR(match))
return PTR_ERR(match);

- return component_master_add_with_match(&pdev->dev, &exynos_drm_ops,
- match);
+ return component_aggregate_register(&pdev->dev, &exynos_drm_aggregate_driver, match);
}

static int exynos_drm_platform_remove(struct platform_device *pdev)
{
- component_master_del(&pdev->dev, &exynos_drm_ops);
+ component_aggregate_unregister(&pdev->dev, &exynos_drm_aggregate_driver);
return 0;
}

--
https://chromeos.dev


2022-01-06 21:46:48

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 15/32] drm/ingenic: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

TODO: Move the helpers to PM in aggregate driver hooks.

Acked-by: Paul Cercueil <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 25 +++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index a5df1c8d34cd..d5330fb486e8 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -1150,8 +1150,10 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
return ret;
}

-static int ingenic_drm_bind_with_components(struct device *dev)
+static int ingenic_drm_bind_with_components(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
+
return ingenic_drm_bind(dev, true);
}

@@ -1174,9 +1176,20 @@ static void ingenic_drm_unbind(struct device *dev)
drm_atomic_helper_shutdown(&priv->drm);
}

-static const struct component_master_ops ingenic_master_ops = {
- .bind = ingenic_drm_bind_with_components,
- .unbind = ingenic_drm_unbind,
+static void ingenic_aggregate_remove(struct aggregate_device *adev)
+{
+ struct device *dev = adev->parent;
+
+ ingenic_drm_unbind(dev);
+}
+
+static struct aggregate_driver ingenic_aggregate_driver = {
+ .probe = ingenic_drm_bind_with_components,
+ .remove = ingenic_aggregate_remove,
+ .driver = {
+ .name = "ingenic_drm",
+ .owner = THIS_MODULE,
+ },
};

static int ingenic_drm_probe(struct platform_device *pdev)
@@ -1196,7 +1209,7 @@ static int ingenic_drm_probe(struct platform_device *pdev)
drm_of_component_match_add(dev, &match, compare_of, np);
of_node_put(np);

- return component_master_add_with_match(dev, &ingenic_master_ops, match);
+ return component_aggregate_register(dev, &ingenic_aggregate_driver, match);
}

static int ingenic_drm_remove(struct platform_device *pdev)
@@ -1206,7 +1219,7 @@ static int ingenic_drm_remove(struct platform_device *pdev)
if (!IS_ENABLED(CONFIG_DRM_INGENIC_IPU))
ingenic_drm_unbind(dev);
else
- component_master_del(dev, &ingenic_master_ops);
+ component_aggregate_unregister(dev, &ingenic_aggregate_driver);

return 0;
}
--
https://chromeos.dev


2022-01-06 21:46:51

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 14/32] drm/imx: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Cc: Philipp Zabel <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/imx/imx-drm-core.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index cb685fe2039b..9e28bb16364c 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -196,8 +196,9 @@ static int compare_of(struct device *dev, void *data)
return dev->of_node == np;
}

-static int imx_drm_bind(struct device *dev)
+static int imx_drm_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct drm_device *drm;
int ret;

@@ -264,8 +265,9 @@ static int imx_drm_bind(struct device *dev)
return ret;
}

-static void imx_drm_unbind(struct device *dev)
+static void imx_drm_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct drm_device *drm = dev_get_drvdata(dev);

drm_dev_unregister(drm);
@@ -279,14 +281,18 @@ static void imx_drm_unbind(struct device *dev)
dev_set_drvdata(dev, NULL);
}

-static const struct component_master_ops imx_drm_ops = {
- .bind = imx_drm_bind,
- .unbind = imx_drm_unbind,
+static struct aggregate_driver imx_drm_aggregate_driver = {
+ .probe = imx_drm_bind,
+ .remove = imx_drm_unbind,
+ .driver = {
+ .name = "imx_drm",
+ .owner = THIS_MODULE,
+ },
};

static int imx_drm_platform_probe(struct platform_device *pdev)
{
- int ret = drm_of_component_probe(&pdev->dev, compare_of, &imx_drm_ops);
+ int ret = drm_of_aggregate_probe(&pdev->dev, compare_of, &imx_drm_aggregate_driver);

if (!ret)
ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
@@ -296,7 +302,7 @@ static int imx_drm_platform_probe(struct platform_device *pdev)

static int imx_drm_platform_remove(struct platform_device *pdev)
{
- component_master_del(&pdev->dev, &imx_drm_ops);
+ component_aggregate_unregister(&pdev->dev, &imx_drm_aggregate_driver);
return 0;
}

--
https://chromeos.dev


2022-01-06 21:46:53

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 08/32] drm/arm/hdlcd: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Cc: Liviu Dudau <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/arm/hdlcd_drv.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index 479c2422a2e0..5c03eb98d814 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -270,8 +270,9 @@ static const struct drm_driver hdlcd_driver = {
.minor = 0,
};

-static int hdlcd_drm_bind(struct device *dev)
+static int hdlcd_drm_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct drm_device *drm;
struct hdlcd_drm_private *hdlcd;
int ret;
@@ -344,8 +345,9 @@ static int hdlcd_drm_bind(struct device *dev)
return ret;
}

-static void hdlcd_drm_unbind(struct device *dev)
+static void hdlcd_drm_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct drm_device *drm = dev_get_drvdata(dev);
struct hdlcd_drm_private *hdlcd = drm->dev_private;

@@ -367,9 +369,13 @@ static void hdlcd_drm_unbind(struct device *dev)
drm_dev_put(drm);
}

-static const struct component_master_ops hdlcd_master_ops = {
- .bind = hdlcd_drm_bind,
- .unbind = hdlcd_drm_unbind,
+static struct aggregate_driver hdlcd_aggregate_driver = {
+ .probe = hdlcd_drm_bind,
+ .remove = hdlcd_drm_unbind,
+ .driver = {
+ .name = "hdlcd_drm",
+ .owner = THIS_MODULE,
+ },
};

static int compare_dev(struct device *dev, void *data)
@@ -390,13 +396,12 @@ static int hdlcd_probe(struct platform_device *pdev)
drm_of_component_match_add(&pdev->dev, &match, compare_dev, port);
of_node_put(port);

- return component_master_add_with_match(&pdev->dev, &hdlcd_master_ops,
- match);
+ return component_aggregate_register(&pdev->dev, &hdlcd_aggregate_driver, match);
}

static int hdlcd_remove(struct platform_device *pdev)
{
- component_master_del(&pdev->dev, &hdlcd_master_ops);
+ component_aggregate_unregister(&pdev->dev, &hdlcd_aggregate_driver);
return 0;
}

--
https://chromeos.dev


2022-01-06 21:46:55

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 07/32] drm/komeda: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Cc: James Qian Wang (Arm Technology China) <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
.../gpu/drm/arm/display/komeda/komeda_drv.c | 20 ++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
index e7933930a657..0463386a6ed2 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
@@ -25,8 +25,9 @@ struct komeda_dev *dev_to_mdev(struct device *dev)
return mdrv ? mdrv->mdev : NULL;
}

-static void komeda_unbind(struct device *dev)
+static void komeda_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct komeda_drv *mdrv = dev_get_drvdata(dev);

if (!mdrv)
@@ -45,8 +46,9 @@ static void komeda_unbind(struct device *dev)
devm_kfree(dev, mdrv);
}

-static int komeda_bind(struct device *dev)
+static int komeda_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct komeda_drv *mdrv;
int err;

@@ -87,9 +89,13 @@ static int komeda_bind(struct device *dev)
return err;
}

-static const struct component_master_ops komeda_master_ops = {
- .bind = komeda_bind,
- .unbind = komeda_unbind,
+static struct aggregate_driver komeda_aggregate_driver = {
+ .probe = komeda_bind,
+ .remove = komeda_unbind,
+ .driver = {
+ .name = "komeda_drm",
+ .owner = THIS_MODULE,
+ },
};

static int compare_of(struct device *dev, void *data)
@@ -129,12 +135,12 @@ static int komeda_platform_probe(struct platform_device *pdev)
komeda_add_slave(dev, &match, child, KOMEDA_OF_PORT_OUTPUT, 1);
}

- return component_master_add_with_match(dev, &komeda_master_ops, match);
+ return component_aggregate_register(dev, &komeda_aggregate_driver, match);
}

static int komeda_platform_remove(struct platform_device *pdev)
{
- component_master_del(&pdev->dev, &komeda_master_ops);
+ component_aggregate_unregister(&pdev->dev, &komeda_aggregate_driver);
return 0;
}

--
https://chromeos.dev


2022-01-06 21:46:57

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 17/32] drm/mediatek: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Cc: Chun-Kuang Hu <[email protected]>
Cc: Philipp Zabel <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_drm_drv.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index aec39724ebeb..a3f27b8c9769 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -348,8 +348,9 @@ static int compare_of(struct device *dev, void *data)
return dev->of_node == data;
}

-static int mtk_drm_bind(struct device *dev)
+static int mtk_drm_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct mtk_drm_private *private = dev_get_drvdata(dev);
struct drm_device *drm;
int ret;
@@ -380,8 +381,9 @@ static int mtk_drm_bind(struct device *dev)
return ret;
}

-static void mtk_drm_unbind(struct device *dev)
+static void mtk_drm_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct mtk_drm_private *private = dev_get_drvdata(dev);

drm_dev_unregister(private->drm);
@@ -391,9 +393,13 @@ static void mtk_drm_unbind(struct device *dev)
private->drm = NULL;
}

-static const struct component_master_ops mtk_drm_ops = {
- .bind = mtk_drm_bind,
- .unbind = mtk_drm_unbind,
+static struct aggregate_driver mtk_drm_aggregate_driver = {
+ .probe = mtk_drm_bind,
+ .remove = mtk_drm_unbind,
+ .driver = {
+ .name = "mtk_drm",
+ .owner = THIS_MODULE,
+ },
};

static const struct of_device_id mtk_ddp_comp_dt_ids[] = {
@@ -593,7 +599,7 @@ static int mtk_drm_probe(struct platform_device *pdev)

platform_set_drvdata(pdev, private);

- ret = component_master_add_with_match(dev, &mtk_drm_ops, match);
+ ret = component_aggregate_register(dev, &mtk_drm_aggregate_driver, match);
if (ret)
goto err_pm;

@@ -616,7 +622,7 @@ static int mtk_drm_remove(struct platform_device *pdev)
struct mtk_drm_private *private = platform_get_drvdata(pdev);
int i;

- component_master_del(&pdev->dev, &mtk_drm_ops);
+ component_aggregate_unregister(&pdev->dev, &mtk_drm_aggregate_driver);
pm_runtime_disable(&pdev->dev);
of_node_put(private->mutex_node);
for (i = 0; i < DDP_COMPONENT_ID_MAX; i++)
--
https://chromeos.dev


2022-01-06 21:46:59

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 16/32] drm/mcde: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Tested-by: Linus Walleij <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/mcde/mcde_drv.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/mcde/mcde_drv.c b/drivers/gpu/drm/mcde/mcde_drv.c
index 5b5afc6aaf8e..1652f9e0601d 100644
--- a/drivers/gpu/drm/mcde/mcde_drv.c
+++ b/drivers/gpu/drm/mcde/mcde_drv.c
@@ -215,8 +215,9 @@ static const struct drm_driver mcde_drm_driver = {
DRM_GEM_CMA_DRIVER_OPS,
};

-static int mcde_drm_bind(struct device *dev)
+static int mcde_drm_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct drm_device *drm = dev_get_drvdata(dev);
int ret;

@@ -247,8 +248,9 @@ static int mcde_drm_bind(struct device *dev)
return ret;
}

-static void mcde_drm_unbind(struct device *dev)
+static void mcde_drm_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct drm_device *drm = dev_get_drvdata(dev);

drm_dev_unregister(drm);
@@ -256,9 +258,13 @@ static void mcde_drm_unbind(struct device *dev)
component_unbind_all(drm->dev, drm);
}

-static const struct component_master_ops mcde_drm_comp_ops = {
- .bind = mcde_drm_bind,
- .unbind = mcde_drm_unbind,
+static struct aggregate_driver mcde_drm_comp_driver = {
+ .probe = mcde_drm_bind,
+ .remove = mcde_drm_unbind,
+ .driver = {
+ .name = "mcde_drm",
+ .owner = THIS_MODULE,
+ },
};

static struct platform_driver *const mcde_component_drivers[] = {
@@ -419,7 +425,7 @@ static int mcde_probe(struct platform_device *pdev)
* Perform an invasive reset of the MCDE and all blocks by
* cutting the power to the subsystem, then bring it back up
* later when we enable the display as a result of
- * component_master_add_with_match().
+ * component_aggregate_register().
*/
ret = regulator_disable(mcde->epod);
if (ret) {
@@ -429,8 +435,7 @@ static int mcde_probe(struct platform_device *pdev)
/* Wait 50 ms so we are sure we cut the power */
usleep_range(50000, 70000);

- ret = component_master_add_with_match(&pdev->dev, &mcde_drm_comp_ops,
- match);
+ ret = component_aggregate_register(&pdev->dev, &mcde_drm_comp_driver, match);
if (ret) {
dev_err(dev, "failed to add component master\n");
/*
@@ -459,7 +464,7 @@ static int mcde_remove(struct platform_device *pdev)
struct drm_device *drm = platform_get_drvdata(pdev);
struct mcde *mcde = to_mcde(drm);

- component_master_del(&pdev->dev, &mcde_drm_comp_ops);
+ component_aggregate_unregister(&pdev->dev, &mcde_drm_comp_driver);
clk_disable_unprepare(mcde->mcde_clk);
regulator_disable(mcde->vana);
regulator_disable(mcde->epod);
--
https://chromeos.dev


2022-01-06 21:47:02

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 19/32] drm/omap: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Cc: Tomi Valkeinen <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/omapdrm/dss/dss.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dss.c b/drivers/gpu/drm/omapdrm/dss/dss.c
index d6a5862b4dbf..9328d97f19ab 100644
--- a/drivers/gpu/drm/omapdrm/dss/dss.c
+++ b/drivers/gpu/drm/omapdrm/dss/dss.c
@@ -1304,8 +1304,9 @@ static const struct soc_device_attribute dss_soc_devices[] = {
{ /* sentinel */ }
};

-static int dss_bind(struct device *dev)
+static int dss_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct dss_device *dss = dev_get_drvdata(dev);
struct platform_device *drm_pdev;
struct dss_pdata pdata;
@@ -1330,8 +1331,9 @@ static int dss_bind(struct device *dev)
return 0;
}

-static void dss_unbind(struct device *dev)
+static void dss_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct dss_device *dss = dev_get_drvdata(dev);

platform_device_unregister(dss->drm_pdev);
@@ -1339,9 +1341,13 @@ static void dss_unbind(struct device *dev)
component_unbind_all(dev, NULL);
}

-static const struct component_master_ops dss_component_ops = {
- .bind = dss_bind,
- .unbind = dss_unbind,
+static struct aggregate_driver dss_aggregate_driver = {
+ .probe = dss_bind,
+ .remove = dss_unbind,
+ .driver = {
+ .name = "dss_drm",
+ .owner = THIS_MODULE,
+ },
};

static int dss_component_compare(struct device *dev, void *data)
@@ -1504,7 +1510,7 @@ static int dss_probe(struct platform_device *pdev)
cmatch.match = &match;
device_for_each_child(&pdev->dev, &cmatch, dss_add_child_component);

- r = component_master_add_with_match(&pdev->dev, &dss_component_ops, match);
+ r = component_aggregate_register(&pdev->dev, &dss_aggregate_driver, match);
if (r)
goto err_of_depopulate;

@@ -1543,7 +1549,7 @@ static int dss_remove(struct platform_device *pdev)

of_platform_depopulate(&pdev->dev);

- component_master_del(&pdev->dev, &dss_component_ops);
+ component_aggregate_unregister(&pdev->dev, &dss_aggregate_driver);

dss_debugfs_remove_file(dss->debugfs.clk);
dss_debugfs_remove_file(dss->debugfs.dss);
--
https://chromeos.dev


2022-01-06 21:47:05

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 18/32] drm/meson: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Cc: Neil Armstrong <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/meson/meson_drv.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c
index 7f41a33592c8..3028f2a45f66 100644
--- a/drivers/gpu/drm/meson/meson_drv.c
+++ b/drivers/gpu/drm/meson/meson_drv.c
@@ -356,13 +356,16 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
return ret;
}

-static int meson_drv_bind(struct device *dev)
+static int meson_drv_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
+
return meson_drv_bind_master(dev, true);
}

-static void meson_drv_unbind(struct device *dev)
+static void meson_drv_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct meson_drm *priv = dev_get_drvdata(dev);
struct drm_device *drm = priv->drm;

@@ -386,9 +389,13 @@ static void meson_drv_unbind(struct device *dev)
}
}

-static const struct component_master_ops meson_drv_master_ops = {
- .bind = meson_drv_bind,
- .unbind = meson_drv_unbind,
+static struct aggregate_driver meson_aggregate_drv = {
+ .probe = meson_drv_bind,
+ .remove = meson_drv_unbind,
+ .driver = {
+ .name = "meson_drm",
+ .owner = THIS_MODULE,
+ },
};

static int __maybe_unused meson_drv_pm_suspend(struct device *dev)
@@ -502,9 +509,7 @@ static int meson_drv_probe(struct platform_device *pdev)
if (count) {
dev_info(&pdev->dev, "Queued %d outputs on vpu\n", count);

- return component_master_add_with_match(&pdev->dev,
- &meson_drv_master_ops,
- match);
+ return component_aggregate_register(&pdev->dev, &meson_aggregate_drv, match);
}

/* If no output endpoints were available, simply bail out */
--
https://chromeos.dev


2022-01-06 21:47:08

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 20/32] drm/rockchip: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Cc: Sandy Huang <[email protected]>
Cc: "Heiko Stübner" <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index e4ebe60b3cc1..6c755361d376 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -109,8 +109,9 @@ static void rockchip_iommu_cleanup(struct drm_device *drm_dev)
iommu_domain_free(private->domain);
}

-static int rockchip_drm_bind(struct device *dev)
+static int rockchip_drm_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct drm_device *drm_dev;
struct rockchip_drm_private *private;
int ret;
@@ -183,8 +184,9 @@ static int rockchip_drm_bind(struct device *dev)
return ret;
}

-static void rockchip_drm_unbind(struct device *dev)
+static void rockchip_drm_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct drm_device *drm_dev = dev_get_drvdata(dev);

drm_dev_unregister(drm_dev);
@@ -346,9 +348,13 @@ static struct component_match *rockchip_drm_match_add(struct device *dev)
return match ?: ERR_PTR(-ENODEV);
}

-static const struct component_master_ops rockchip_drm_ops = {
- .bind = rockchip_drm_bind,
- .unbind = rockchip_drm_unbind,
+static struct aggregate_driver rockchip_aggregate_driver = {
+ .probe = rockchip_drm_bind,
+ .remove = rockchip_drm_unbind,
+ .driver = {
+ .name = "rockchip_drm",
+ .owner = THIS_MODULE,
+ },
};

static int rockchip_drm_platform_of_probe(struct device *dev)
@@ -419,7 +425,7 @@ static int rockchip_drm_platform_probe(struct platform_device *pdev)
if (IS_ERR(match))
return PTR_ERR(match);

- ret = component_master_add_with_match(dev, &rockchip_drm_ops, match);
+ ret = component_aggregate_register(dev, &rockchip_aggregate_driver, match);
if (ret < 0) {
rockchip_drm_match_remove(dev);
return ret;
@@ -430,7 +436,7 @@ static int rockchip_drm_platform_probe(struct platform_device *pdev)

static int rockchip_drm_platform_remove(struct platform_device *pdev)
{
- component_master_del(&pdev->dev, &rockchip_drm_ops);
+ component_aggregate_unregister(&pdev->dev, &rockchip_aggregate_driver);

rockchip_drm_match_remove(&pdev->dev);

--
https://chromeos.dev


2022-01-06 21:47:10

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 22/32] drm/sun4i: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Cc: Maxime Ripard <[email protected]>
Cc: Chen-Yu Tsai <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/sun4i/sun4i_drv.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index 54dd562e294c..700f5e32eaf7 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -56,8 +56,9 @@ static const struct drm_driver sun4i_drv_driver = {
DRM_GEM_CMA_DRIVER_OPS_VMAP_WITH_DUMB_CREATE(drm_sun4i_gem_dumb_create),
};

-static int sun4i_drv_bind(struct device *dev)
+static int sun4i_drv_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct drm_device *drm;
struct sun4i_drv *drv;
int ret;
@@ -125,8 +126,9 @@ static int sun4i_drv_bind(struct device *dev)
return ret;
}

-static void sun4i_drv_unbind(struct device *dev)
+static void sun4i_drv_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct drm_device *drm = dev_get_drvdata(dev);

drm_dev_unregister(drm);
@@ -140,9 +142,13 @@ static void sun4i_drv_unbind(struct device *dev)
drm_dev_put(drm);
}

-static const struct component_master_ops sun4i_drv_master_ops = {
- .bind = sun4i_drv_bind,
- .unbind = sun4i_drv_unbind,
+static struct aggregate_driver sun4i_aggregate_driver = {
+ .probe = sun4i_drv_bind,
+ .remove = sun4i_drv_unbind,
+ .driver = {
+ .name = "sun4i_drm",
+ .owner = THIS_MODULE,
+ },
};

static bool sun4i_drv_node_is_connector(struct device_node *node)
@@ -398,16 +404,14 @@ static int sun4i_drv_probe(struct platform_device *pdev)
}

if (count)
- return component_master_add_with_match(&pdev->dev,
- &sun4i_drv_master_ops,
- match);
- else
- return 0;
+ return component_aggregate_register(&pdev->dev, &sun4i_aggregate_driver, match);
+
+ return 0;
}

static int sun4i_drv_remove(struct platform_device *pdev)
{
- component_master_del(&pdev->dev, &sun4i_drv_master_ops);
+ component_aggregate_unregister(&pdev->dev, &sun4i_aggregate_driver);

return 0;
}
--
https://chromeos.dev


2022-01-06 21:47:14

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 23/32] drm/tilcdc: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Tested-by: Jyri Sarha <[email protected]>
Cc: Tomi Valkeinen <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/tilcdc/tilcdc_drv.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 3ddb7c710a3d..92ff516fb6de 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -529,13 +529,16 @@ static const struct dev_pm_ops tilcdc_pm_ops = {
/*
* Platform driver:
*/
-static int tilcdc_bind(struct device *dev)
+static int tilcdc_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
+
return tilcdc_init(&tilcdc_driver, dev);
}

-static void tilcdc_unbind(struct device *dev)
+static void tilcdc_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct drm_device *ddev = dev_get_drvdata(dev);

/* Check if a subcomponent has already triggered the unloading. */
@@ -545,9 +548,13 @@ static void tilcdc_unbind(struct device *dev)
tilcdc_fini(dev_get_drvdata(dev));
}

-static const struct component_master_ops tilcdc_comp_ops = {
- .bind = tilcdc_bind,
- .unbind = tilcdc_unbind,
+static struct aggregate_driver tilcdc_aggregate_driver = {
+ .probe = tilcdc_bind,
+ .remove = tilcdc_unbind,
+ .driver = {
+ .name = "tilcdc_drm",
+ .owner = THIS_MODULE,
+ },
};

static int tilcdc_pdev_probe(struct platform_device *pdev)
@@ -564,12 +571,9 @@ static int tilcdc_pdev_probe(struct platform_device *pdev)
ret = tilcdc_get_external_components(&pdev->dev, &match);
if (ret < 0)
return ret;
- else if (ret == 0)
+ if (ret == 0)
return tilcdc_init(&tilcdc_driver, &pdev->dev);
- else
- return component_master_add_with_match(&pdev->dev,
- &tilcdc_comp_ops,
- match);
+ return component_aggregate_register(&pdev->dev, &tilcdc_aggregate_driver, match);
}

static int tilcdc_pdev_remove(struct platform_device *pdev)
@@ -579,10 +583,10 @@ static int tilcdc_pdev_remove(struct platform_device *pdev)
ret = tilcdc_get_external_components(&pdev->dev, NULL);
if (ret < 0)
return ret;
- else if (ret == 0)
+ if (ret == 0)
tilcdc_fini(platform_get_drvdata(pdev));
else
- component_master_del(&pdev->dev, &tilcdc_comp_ops);
+ component_aggregate_unregister(&pdev->dev, &tilcdc_aggregate_driver);

return 0;
}
--
https://chromeos.dev


2022-01-06 21:47:19

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 21/32] drm/sti: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/sti/sti_drv.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c
index c7efb43b83ee..b277cc679154 100644
--- a/drivers/gpu/drm/sti/sti_drv.c
+++ b/drivers/gpu/drm/sti/sti_drv.c
@@ -182,8 +182,9 @@ static void sti_cleanup(struct drm_device *ddev)
ddev->dev_private = NULL;
}

-static int sti_bind(struct device *dev)
+static int sti_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct drm_device *ddev;
int ret;

@@ -216,8 +217,9 @@ static int sti_bind(struct device *dev)
return ret;
}

-static void sti_unbind(struct device *dev)
+static void sti_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct drm_device *ddev = dev_get_drvdata(dev);

drm_dev_unregister(ddev);
@@ -225,9 +227,13 @@ static void sti_unbind(struct device *dev)
drm_dev_put(ddev);
}

-static const struct component_master_ops sti_ops = {
- .bind = sti_bind,
- .unbind = sti_unbind,
+static struct aggregate_driver sti_aggregate_driver = {
+ .probe = sti_bind,
+ .remove = sti_unbind,
+ .driver = {
+ .name = "sti_drm",
+ .owner = THIS_MODULE,
+ },
};

static int sti_platform_probe(struct platform_device *pdev)
@@ -249,12 +255,12 @@ static int sti_platform_probe(struct platform_device *pdev)
child_np = of_get_next_available_child(node, child_np);
}

- return component_master_add_with_match(dev, &sti_ops, match);
+ return component_aggregate_register(dev, &sti_aggregate_driver, match);
}

static int sti_platform_remove(struct platform_device *pdev)
{
- component_master_del(&pdev->dev, &sti_ops);
+ component_aggregate_unregister(&pdev->dev, &sti_aggregate_driver);

return 0;
}
--
https://chromeos.dev


2022-01-06 21:47:23

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 24/32] drm/vc4: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Cc: Emma Anholt <[email protected]>
Cc: Maxime Ripard <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/vc4/vc4_drv.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 16abc3a3d601..82a44ebf9121 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -212,8 +212,9 @@ static void vc4_match_add_drivers(struct device *dev,
}
}

-static int vc4_drm_bind(struct device *dev)
+static int vc4_drm_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct platform_device *pdev = to_platform_device(dev);
struct drm_device *drm;
struct vc4_dev *vc4;
@@ -284,8 +285,9 @@ static int vc4_drm_bind(struct device *dev)
return ret;
}

-static void vc4_drm_unbind(struct device *dev)
+static void vc4_drm_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct drm_device *drm = dev_get_drvdata(dev);

drm_dev_unregister(drm);
@@ -293,9 +295,13 @@ static void vc4_drm_unbind(struct device *dev)
drm_atomic_helper_shutdown(drm);
}

-static const struct component_master_ops vc4_drm_ops = {
- .bind = vc4_drm_bind,
- .unbind = vc4_drm_unbind,
+static struct aggregate_driver vc4_aggregate_driver = {
+ .probe = vc4_drm_bind,
+ .remove = vc4_drm_unbind,
+ .driver = {
+ .name = "vc4_drm",
+ .owner = THIS_MODULE,
+ },
};

/*
@@ -326,12 +332,12 @@ static int vc4_platform_drm_probe(struct platform_device *pdev)
vc4_match_add_drivers(dev, &match,
component_drivers, ARRAY_SIZE(component_drivers));

- return component_master_add_with_match(dev, &vc4_drm_ops, match);
+ return component_aggregate_register(dev, &vc4_aggregate_driver, match);
}

static int vc4_platform_drm_remove(struct platform_device *pdev)
{
- component_master_del(&pdev->dev, &vc4_drm_ops);
+ component_aggregate_unregister(&pdev->dev, &vc4_aggregate_driver);

return 0;
}
--
https://chromeos.dev


2022-01-06 21:47:25

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 25/32] iommu/mtk: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Cc: Yong Wu <[email protected]>
Cc: Joerg Roedel <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/iommu/mtk_iommu.c | 14 +++++++++-----
drivers/iommu/mtk_iommu.h | 6 ++++--
drivers/iommu/mtk_iommu_v1.c | 14 +++++++++-----
3 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 25b834104790..8e722898cbe2 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -752,9 +752,13 @@ static int mtk_iommu_hw_init(const struct mtk_iommu_data *data)
return 0;
}

-static const struct component_master_ops mtk_iommu_com_ops = {
- .bind = mtk_iommu_bind,
- .unbind = mtk_iommu_unbind,
+static struct aggregate_driver mtk_iommu_aggregate_driver = {
+ .probe = mtk_iommu_bind,
+ .remove = mtk_iommu_unbind,
+ .driver = {
+ .name = "mtk_iommu_agg",
+ .owner = THIS_MODULE,
+ },
};

static int mtk_iommu_probe(struct platform_device *pdev)
@@ -895,7 +899,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
goto out_list_del;
}

- ret = component_master_add_with_match(dev, &mtk_iommu_com_ops, match);
+ ret = component_aggregate_register(dev, &mtk_iommu_aggregate_driver, match);
if (ret)
goto out_bus_set_null;
return ret;
@@ -928,7 +932,7 @@ static int mtk_iommu_remove(struct platform_device *pdev)
device_link_remove(data->smicomm_dev, &pdev->dev);
pm_runtime_disable(&pdev->dev);
devm_free_irq(&pdev->dev, data->irq, data);
- component_master_del(&pdev->dev, &mtk_iommu_com_ops);
+ component_aggregate_unregister(&pdev->dev, &mtk_iommu_aggregate_driver);
return 0;
}

diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
index f81fa8862ed0..064fd4f4eade 100644
--- a/drivers/iommu/mtk_iommu.h
+++ b/drivers/iommu/mtk_iommu.h
@@ -94,15 +94,17 @@ static inline void release_of(struct device *dev, void *data)
of_node_put(data);
}

-static inline int mtk_iommu_bind(struct device *dev)
+static inline int mtk_iommu_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct mtk_iommu_data *data = dev_get_drvdata(dev);

return component_bind_all(dev, &data->larb_imu);
}

-static inline void mtk_iommu_unbind(struct device *dev)
+static inline void mtk_iommu_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct mtk_iommu_data *data = dev_get_drvdata(dev);

component_unbind_all(dev, &data->larb_imu);
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index be22fcf988ce..5fb29058a165 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -534,9 +534,13 @@ static const struct of_device_id mtk_iommu_of_ids[] = {
{}
};

-static const struct component_master_ops mtk_iommu_com_ops = {
- .bind = mtk_iommu_bind,
- .unbind = mtk_iommu_unbind,
+static struct aggregate_driver mtk_iommu_aggregate_driver = {
+ .probe = mtk_iommu_bind,
+ .remove = mtk_iommu_unbind,
+ .driver = {
+ .name = "mtk_iommu_agg",
+ .owner = THIS_MODULE,
+ },
};

static int mtk_iommu_probe(struct platform_device *pdev)
@@ -624,7 +628,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
goto out_dev_unreg;
}

- ret = component_master_add_with_match(dev, &mtk_iommu_com_ops, match);
+ ret = component_aggregate_register(dev, &mtk_iommu_aggregate_driver, match);
if (ret)
goto out_bus_set_null;
return ret;
@@ -650,7 +654,7 @@ static int mtk_iommu_remove(struct platform_device *pdev)

clk_disable_unprepare(data->bclk);
devm_free_irq(&pdev->dev, data->irq, data);
- component_master_del(&pdev->dev, &mtk_iommu_com_ops);
+ component_aggregate_unregister(&pdev->dev, &mtk_iommu_aggregate_driver);
return 0;
}

--
https://chromeos.dev


2022-01-06 21:47:31

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 26/32] mei: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Cc: Tomas Winkler <[email protected]>
Cc: Vitaly Lubart <[email protected]>
Cc: Daniele Ceraolo Spurio <[email protected]>
Cc: Rodrigo Vivi <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Cc: Alexander Usyskin <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/misc/mei/hdcp/mei_hdcp.c | 22 +++++++++++++---------
drivers/misc/mei/pxp/mei_pxp.c | 22 +++++++++++++---------
2 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c
index ec2a4fce8581..ae903a09fb06 100644
--- a/drivers/misc/mei/hdcp/mei_hdcp.c
+++ b/drivers/misc/mei/hdcp/mei_hdcp.c
@@ -732,8 +732,9 @@ static const struct i915_hdcp_component_ops mei_hdcp_ops = {
.close_hdcp_session = mei_hdcp_close_session,
};

-static int mei_component_master_bind(struct device *dev)
+static int mei_hdcp_aggregate_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct mei_cl_device *cldev = to_mei_cl_device(dev);
struct i915_hdcp_comp_master *comp_master =
mei_cldev_get_drvdata(cldev);
@@ -749,8 +750,9 @@ static int mei_component_master_bind(struct device *dev)
return 0;
}

-static void mei_component_master_unbind(struct device *dev)
+static void mei_hdcp_aggregate_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct mei_cl_device *cldev = to_mei_cl_device(dev);
struct i915_hdcp_comp_master *comp_master =
mei_cldev_get_drvdata(cldev);
@@ -759,9 +761,13 @@ static void mei_component_master_unbind(struct device *dev)
component_unbind_all(dev, comp_master);
}

-static const struct component_master_ops mei_component_master_ops = {
- .bind = mei_component_master_bind,
- .unbind = mei_component_master_unbind,
+static struct aggregate_driver mei_aggregate_driver = {
+ .probe = mei_hdcp_aggregate_bind,
+ .remove = mei_hdcp_aggregate_unbind,
+ .driver = {
+ .name = "mei_hdcp_agg",
+ .owner = THIS_MODULE,
+ },
};

/**
@@ -826,9 +832,7 @@ static int mei_hdcp_probe(struct mei_cl_device *cldev,
}

mei_cldev_set_drvdata(cldev, comp_master);
- ret = component_master_add_with_match(&cldev->dev,
- &mei_component_master_ops,
- master_match);
+ ret = component_aggregate_register(&cldev->dev, &mei_aggregate_driver, master_match);
if (ret < 0) {
dev_err(&cldev->dev, "Master comp add failed %d\n", ret);
goto err_exit;
@@ -850,7 +854,7 @@ static void mei_hdcp_remove(struct mei_cl_device *cldev)
mei_cldev_get_drvdata(cldev);
int ret;

- component_master_del(&cldev->dev, &mei_component_master_ops);
+ component_aggregate_unregister(&cldev->dev, &mei_aggregate_driver);
kfree(comp_master);
mei_cldev_set_drvdata(cldev, NULL);

diff --git a/drivers/misc/mei/pxp/mei_pxp.c b/drivers/misc/mei/pxp/mei_pxp.c
index f7380d387bab..7b7bd7c0e8b1 100644
--- a/drivers/misc/mei/pxp/mei_pxp.c
+++ b/drivers/misc/mei/pxp/mei_pxp.c
@@ -83,8 +83,9 @@ static const struct i915_pxp_component_ops mei_pxp_ops = {
.recv = mei_pxp_receive_message,
};

-static int mei_component_master_bind(struct device *dev)
+static int mei_pxp_aggregate_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct mei_cl_device *cldev = to_mei_cl_device(dev);
struct i915_pxp_component *comp_master = mei_cldev_get_drvdata(cldev);
int ret;
@@ -98,17 +99,22 @@ static int mei_component_master_bind(struct device *dev)
return 0;
}

-static void mei_component_master_unbind(struct device *dev)
+static void mei_pxp_aggregate_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct mei_cl_device *cldev = to_mei_cl_device(dev);
struct i915_pxp_component *comp_master = mei_cldev_get_drvdata(cldev);

component_unbind_all(dev, comp_master);
}

-static const struct component_master_ops mei_component_master_ops = {
- .bind = mei_component_master_bind,
- .unbind = mei_component_master_unbind,
+static struct aggregate_driver mei_aggregate_driver = {
+ .probe = mei_pxp_aggregate_bind,
+ .remove = mei_pxp_aggregate_unbind,
+ .driver = {
+ .name = "mei_pxp_agg",
+ .owner = THIS_MODULE,
+ }
};

/**
@@ -173,9 +179,7 @@ static int mei_pxp_probe(struct mei_cl_device *cldev,
}

mei_cldev_set_drvdata(cldev, comp_master);
- ret = component_master_add_with_match(&cldev->dev,
- &mei_component_master_ops,
- master_match);
+ ret = component_aggregate_register(&cldev->dev, &mei_aggregate_driver, master_match);
if (ret < 0) {
dev_err(&cldev->dev, "Master comp add failed %d\n", ret);
goto err_exit;
@@ -196,7 +200,7 @@ static void mei_pxp_remove(struct mei_cl_device *cldev)
struct i915_pxp_component *comp_master = mei_cldev_get_drvdata(cldev);
int ret;

- component_master_del(&cldev->dev, &mei_component_master_ops);
+ component_aggregate_unregister(&cldev->dev, &mei_aggregate_driver);
kfree(comp_master);
mei_cldev_set_drvdata(cldev, NULL);

--
https://chromeos.dev


2022-01-06 21:47:39

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 27/32] power: supply: ab8500: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Acked-by: Sebastian Reichel <[email protected]>
Tested-by: Linus Walleij <[email protected]>
Cc: <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/power/supply/ab8500_charger.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/power/supply/ab8500_charger.c b/drivers/power/supply/ab8500_charger.c
index 15eadaf46f14..52d4105e28f2 100644
--- a/drivers/power/supply/ab8500_charger.c
+++ b/drivers/power/supply/ab8500_charger.c
@@ -3312,8 +3312,9 @@ static const struct power_supply_desc ab8500_usb_chg_desc = {
.get_property = ab8500_charger_usb_get_property,
};

-static int ab8500_charger_bind(struct device *dev)
+static int ab8500_charger_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct ab8500_charger *di = dev_get_drvdata(dev);
int ch_stat;
int ret;
@@ -3354,8 +3355,9 @@ static int ab8500_charger_bind(struct device *dev)
return 0;
}

-static void ab8500_charger_unbind(struct device *dev)
+static void ab8500_charger_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct ab8500_charger *di = dev_get_drvdata(dev);
int ret;

@@ -3380,9 +3382,13 @@ static void ab8500_charger_unbind(struct device *dev)
component_unbind_all(dev, di);
}

-static const struct component_master_ops ab8500_charger_comp_ops = {
- .bind = ab8500_charger_bind,
- .unbind = ab8500_charger_unbind,
+static struct aggregate_driver ab8500_charger_aggregate_driver = {
+ .probe = ab8500_charger_bind,
+ .remove = ab8500_charger_unbind,
+ .driver = {
+ .name = "ab8500_charger_agg",
+ .owner = THIS_MODULE,
+ },
};

static struct platform_driver *const ab8500_charger_component_drivers[] = {
@@ -3663,9 +3669,7 @@ static int ab8500_charger_probe(struct platform_device *pdev)
}


- ret = component_master_add_with_match(&pdev->dev,
- &ab8500_charger_comp_ops,
- match);
+ ret = component_aggregate_register(&pdev->dev, &ab8500_charger_aggregate_driver, match);
if (ret) {
dev_err(dev, "failed to add component master\n");
goto free_notifier;
@@ -3688,7 +3692,7 @@ static int ab8500_charger_remove(struct platform_device *pdev)
{
struct ab8500_charger *di = platform_get_drvdata(pdev);

- component_master_del(&pdev->dev, &ab8500_charger_comp_ops);
+ component_aggregate_unregister(&pdev->dev, &ab8500_charger_aggregate_driver);

usb_unregister_notifier(di->usb_phy, &di->nb);
usb_put_phy(di->usb_phy);
--
https://chromeos.dev


2022-01-06 21:47:51

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 28/32] fbdev: omap2: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Cc: <[email protected]>
Cc: <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/video/fbdev/omap2/omapfb/dss/dss.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss.c b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
index a6b1c1598040..f12663c39ceb 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dss.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
@@ -1067,8 +1067,9 @@ static int dss_video_pll_probe(struct platform_device *pdev)
}

/* DSS HW IP initialisation */
-static int dss_bind(struct device *dev)
+static int dss_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct platform_device *pdev = to_platform_device(dev);
struct resource *dss_mem;
u32 rev;
@@ -1167,8 +1168,9 @@ static int dss_bind(struct device *dev)
return r;
}

-static void dss_unbind(struct device *dev)
+static void dss_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct platform_device *pdev = to_platform_device(dev);

dss_initialized = false;
@@ -1188,9 +1190,13 @@ static void dss_unbind(struct device *dev)
dss_put_clocks();
}

-static const struct component_master_ops dss_component_ops = {
- .bind = dss_bind,
- .unbind = dss_unbind,
+static struct aggregate_driver dss_aggregate_driver = {
+ .probe = dss_bind,
+ .remove = dss_unbind,
+ .driver = {
+ .name = "dss_fbdev",
+ .owner = THIS_MODULE,
+ },
};

static int dss_component_compare(struct device *dev, void *data)
@@ -1225,7 +1231,7 @@ static int dss_probe(struct platform_device *pdev)
/* add all the child devices as components */
device_for_each_child(&pdev->dev, &match, dss_add_child_component);

- r = component_master_add_with_match(&pdev->dev, &dss_component_ops, match);
+ r = component_aggregate_register(&pdev->dev, &dss_aggregate_driver, match);
if (r)
return r;

@@ -1234,7 +1240,7 @@ static int dss_probe(struct platform_device *pdev)

static int dss_remove(struct platform_device *pdev)
{
- component_master_del(&pdev->dev, &dss_component_ops);
+ component_aggregate_unregister(&pdev->dev, &dss_aggregate_driver);
return 0;
}

--
https://chromeos.dev


2022-01-06 21:47:56

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 29/32] sound: hdac: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Cc: Jaroslav Kysela <[email protected]>
Cc: Takashi Iwai <[email protected]>
Cc: Kai Vehmanen <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
sound/hda/hdac_component.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/sound/hda/hdac_component.c b/sound/hda/hdac_component.c
index bb37e7e0bd79..9e4dab97f485 100644
--- a/sound/hda/hdac_component.c
+++ b/sound/hda/hdac_component.c
@@ -181,8 +181,9 @@ int snd_hdac_acomp_get_eld(struct hdac_device *codec, hda_nid_t nid, int dev_id,
}
EXPORT_SYMBOL_GPL(snd_hdac_acomp_get_eld);

-static int hdac_component_master_bind(struct device *dev)
+static int hdac_component_master_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct drm_audio_component *acomp = hdac_get_acomp(dev);
int ret;

@@ -222,8 +223,9 @@ static int hdac_component_master_bind(struct device *dev)
return ret;
}

-static void hdac_component_master_unbind(struct device *dev)
+static void hdac_component_master_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct drm_audio_component *acomp = hdac_get_acomp(dev);

if (acomp->audio_ops && acomp->audio_ops->master_unbind)
@@ -233,9 +235,13 @@ static void hdac_component_master_unbind(struct device *dev)
WARN_ON(acomp->ops || acomp->dev);
}

-static const struct component_master_ops hdac_component_master_ops = {
- .bind = hdac_component_master_bind,
- .unbind = hdac_component_master_unbind,
+static struct aggregate_driver hdac_aggregate_driver = {
+ .probe = hdac_component_master_bind,
+ .remove = hdac_component_master_unbind,
+ .driver = {
+ .name = "hdac_agg",
+ .owner = THIS_MODULE,
+ },
};

/**
@@ -303,8 +309,7 @@ int snd_hdac_acomp_init(struct hdac_bus *bus,
devres_add(dev, acomp);

component_match_add_typed(dev, &match, match_master, bus);
- ret = component_master_add_with_match(dev, &hdac_component_master_ops,
- match);
+ ret = component_aggregate_register(dev, &hdac_aggregate_driver, match);
if (ret < 0)
goto out_err;

@@ -344,7 +349,7 @@ int snd_hdac_acomp_exit(struct hdac_bus *bus)
bus->display_power_active = 0;
bus->display_power_status = 0;

- component_master_del(dev, &hdac_component_master_ops);
+ component_aggregate_unregister(dev, &hdac_aggregate_driver);

bus->audio_component = NULL;
devres_destroy(dev, hdac_acomp_release, NULL, NULL);
--
https://chromeos.dev


2022-01-06 21:48:00

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 32/32] component: Remove component_master_ops and friends

The struct is unused now so drop it along with the functions that use
it.

Cc: Daniel Vetter <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/base/component.c | 148 +++++---------------------------------
drivers/gpu/drm/drm_drv.c | 2 +-
include/linux/component.h | 45 ------------
3 files changed, 17 insertions(+), 178 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index e9e58b56cda4..cd50137753b4 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -133,18 +133,12 @@ static void component_debugfs_del(struct aggregate_device *m)

#endif

-struct aggregate_bus_find_data {
- const struct component_master_ops *ops;
- struct device *parent;
-};
-
static int aggregate_bus_find_match(struct device *dev, const void *_data)
{
struct aggregate_device *adev = to_aggregate_device(dev);
- const struct aggregate_bus_find_data *data = _data;
+ const struct device *parent = _data;

- if (adev->parent == data->parent &&
- (!data->ops || adev->ops == data->ops))
+ if (adev->parent == parent)
return 1;

return 0;
@@ -405,30 +399,15 @@ static int aggregate_device_match(struct device *dev, struct device_driver *drv)
return ret;
}

-/* TODO: Remove once all aggregate drivers use component_aggregate_register() */
-static int component_probe_bind(struct aggregate_device *adev)
-{
- return adev->ops->bind(adev->parent);
-}
-
-static void component_remove_unbind(struct aggregate_device *adev)
-{
- adev->ops->unbind(adev->parent);
-}
-
static int aggregate_driver_probe(struct device *dev)
{
const struct aggregate_driver *adrv = to_aggregate_driver(dev->driver);
struct aggregate_device *adev = to_aggregate_device(dev);
- bool modern = adrv->probe != component_probe_bind;
int ret;

- /* Only do runtime PM when drivers migrate */
- if (modern) {
- pm_runtime_get_noresume(dev);
- pm_runtime_set_active(dev);
- pm_runtime_enable(dev);
- }
+ pm_runtime_get_noresume(dev);
+ pm_runtime_set_active(dev);
+ pm_runtime_enable(dev);

mutex_lock(&component_mutex);
if (devres_open_group(adev->parent, adev, GFP_KERNEL)) {
@@ -441,7 +420,7 @@ static int aggregate_driver_probe(struct device *dev)
devres_close_group(adev->parent, NULL);
mutex_unlock(&component_mutex);

- if (ret && modern) {
+ if (ret) {
pm_runtime_disable(dev);
pm_runtime_set_suspended(dev);
pm_runtime_put_noidle(dev);
@@ -454,15 +433,10 @@ static void aggregate_driver_remove(struct device *dev)
{
const struct aggregate_driver *adrv = to_aggregate_driver(dev->driver);
struct aggregate_device *adev = to_aggregate_device(dev);
- bool modern = adrv->remove != component_remove_unbind;

- /* Only do runtime PM when drivers migrate */
- if (modern)
- pm_runtime_get_sync(dev);
+ pm_runtime_get_sync(dev);
adrv->remove(to_aggregate_device(dev));
devres_release_group(adev->parent, adev);
- if (!modern)
- return;

pm_runtime_put_noidle(dev);

@@ -488,16 +462,11 @@ static struct bus_type aggregate_bus_type = {
};

/* Callers take ownership of return value, should call put_device() */
-static struct aggregate_device *__aggregate_find(struct device *parent,
- const struct component_master_ops *ops)
+static struct aggregate_device *__aggregate_find(struct device *parent)
{
struct device *dev;
- struct aggregate_bus_find_data data = {
- .ops = ops,
- .parent = parent,
- };

- dev = bus_find_device(&aggregate_bus_type, NULL, &data,
+ dev = bus_find_device(&aggregate_bus_type, NULL, parent,
aggregate_bus_find_match);

return dev ? to_aggregate_device(dev) : NULL;
@@ -515,7 +484,7 @@ static void aggregate_driver_unregister(struct aggregate_driver *adrv)
}

static struct aggregate_device *aggregate_device_add(struct device *parent,
- const struct component_master_ops *ops, struct aggregate_driver *adrv,
+ struct aggregate_driver *adrv,
struct component_match *match)
{
struct aggregate_device *adev;
@@ -540,7 +509,6 @@ static struct aggregate_device *aggregate_device_add(struct device *parent,
adev->parent = parent;
adev->dev.bus = &aggregate_bus_type;
adev->dev.release = aggregate_device_release;
- adev->ops = ops;
adev->match = match;
adev->adrv = adrv;
dev_set_name(&adev->dev, "aggregate%d", id);
@@ -556,54 +524,6 @@ static struct aggregate_device *aggregate_device_add(struct device *parent,
return adev;
}

-/**
- * component_master_add_with_match - register an aggregate driver
- * @parent: parent device of the aggregate driver
- * @ops: callbacks for the aggregate driver
- * @match: component match list for the aggregate driver
- *
- * Registers a new aggregate driver consisting of the components added to @match
- * by calling one of the component_match_add() functions. Once all components in
- * @match are available, it will be assembled by calling
- * &component_master_ops.bind from @ops. Must be unregistered by calling
- * component_master_del().
- *
- * Deprecated: Use component_aggregate_register() instead.
- */
-int component_master_add_with_match(struct device *parent,
- const struct component_master_ops *ops,
- struct component_match *match)
-{
- struct aggregate_driver *adrv;
- struct aggregate_device *adev;
- int ret = 0;
-
- adrv = kzalloc(sizeof(*adrv), GFP_KERNEL);
- if (!adrv)
- return -ENOMEM;
-
- adev = aggregate_device_add(parent, ops, adrv, match);
- if (IS_ERR(adev)) {
- ret = PTR_ERR(adev);
- goto err;
- }
-
- adrv->probe = component_probe_bind;
- adrv->remove = component_remove_unbind;
- adrv->driver.owner = THIS_MODULE;
- adrv->driver.name = dev_name(&adev->dev);
-
- ret = aggregate_driver_register(adrv);
- if (!ret)
- return 0;
-
- put_device(&adev->dev);
-err:
- kfree(adrv);
- return ret;
-}
-EXPORT_SYMBOL_GPL(component_master_add_with_match);
-
/**
* component_aggregate_register - register an aggregate driver
* @parent: parent device of the aggregate driver
@@ -620,7 +540,7 @@ int component_aggregate_register(struct device *parent,
struct aggregate_device *adev;
int ret;

- adev = aggregate_device_add(parent, NULL, adrv, match);
+ adev = aggregate_device_add(parent, adrv, match);
if (IS_ERR(adev))
return PTR_ERR(adev);

@@ -632,42 +552,6 @@ int component_aggregate_register(struct device *parent,
}
EXPORT_SYMBOL_GPL(component_aggregate_register);

-/**
- * component_master_del - unregister an aggregate driver
- * @parent: parent device of the aggregate driver
- * @ops: callbacks for the aggregate driver
- *
- * Unregisters an aggregate driver registered with
- * component_master_add_with_match(). If necessary the aggregate driver is first
- * disassembled by calling &component_master_ops.unbind from @ops.
- *
- * Deprecated: Use component_aggregate_unregister() instead.
- */
-void component_master_del(struct device *parent,
- const struct component_master_ops *ops)
-{
- struct aggregate_device *adev;
- struct aggregate_driver *adrv;
- struct device_driver *drv;
-
- mutex_lock(&component_mutex);
- adev = __aggregate_find(parent, ops);
- mutex_unlock(&component_mutex);
-
- if (adev) {
- drv = adev->dev.driver;
- if (drv) {
- adrv = to_aggregate_driver(drv);
- aggregate_driver_unregister(adrv);
- kfree(adrv);
- }
-
- device_unregister(&adev->dev);
- }
- put_device(&adev->dev);
-}
-EXPORT_SYMBOL_GPL(component_master_del);
-
/**
* component_aggregate_unregister - unregister an aggregate driver
* @parent: parent device of the aggregate driver
@@ -683,7 +567,7 @@ void component_aggregate_unregister(struct device *parent,
struct aggregate_device *adev;

mutex_lock(&component_mutex);
- adev = __aggregate_find(parent, NULL);
+ adev = __aggregate_find(parent);
mutex_unlock(&component_mutex);

if (adev)
@@ -719,7 +603,7 @@ static void component_unbind(struct component *component,
*
* Unbinds all components of the aggregate device by passing @data to their
* &component_ops.unbind functions. Should be called from
- * &component_master_ops.unbind.
+ * &aggregate_driver.remove.
*/
void component_unbind_all(struct device *parent, void *data)
{
@@ -729,7 +613,7 @@ void component_unbind_all(struct device *parent, void *data)

WARN_ON(!mutex_is_locked(&component_mutex));

- adev = __aggregate_find(parent, NULL);
+ adev = __aggregate_find(parent);
if (!adev)
return;

@@ -807,7 +691,7 @@ static int component_bind(struct component *component, struct aggregate_device *
*
* Binds all components of the aggregate @dev by passing @data to their
* &component_ops.bind functions. Should be called from
- * &component_master_ops.bind.
+ * &aggregate_driver.probe.
*/
int component_bind_all(struct device *parent, void *data)
{
@@ -818,7 +702,7 @@ int component_bind_all(struct device *parent, void *data)

WARN_ON(!mutex_is_locked(&component_mutex));

- adev = __aggregate_find(parent, NULL);
+ adev = __aggregate_find(parent);
if (!adev)
return -EINVAL;

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 7a5097467ba5..d188fa26bb1b 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -544,7 +544,7 @@ static void drm_fs_inode_free(struct inode *inode)
* following guidelines apply:
*
* - The entire device initialization procedure should be run from the
- * &component_master_ops.master_bind callback, starting with
+ * &aggregate_driver.probe callback, starting with
* devm_drm_dev_alloc(), then binding all components with
* component_bind_all() and finishing with drm_dev_register().
*
diff --git a/include/linux/component.h b/include/linux/component.h
index d8dcbf9733da..07fe481d4e3b 100644
--- a/include/linux/component.h
+++ b/include/linux/component.h
@@ -63,47 +63,7 @@ void component_del(struct device *, const struct component_ops *);
int component_bind_all(struct device *master, void *master_data);
void component_unbind_all(struct device *master, void *master_data);

-/**
- * struct component_master_ops - callback for the aggregate driver
- *
- * Aggregate drivers are registered with component_master_add_with_match() and
- * unregistered with component_master_del().
- */
-struct component_master_ops {
- /**
- * @bind:
- *
- * Called when all components or the aggregate driver, as specified in
- * the match list passed to component_master_add_with_match(), are
- * ready. Usually there are 3 steps to bind an aggregate driver:
- *
- * 1. Allocate a structure for the aggregate driver.
- *
- * 2. Bind all components to the aggregate driver by calling
- * component_bind_all() with the aggregate driver structure as opaque
- * pointer data.
- *
- * 3. Register the aggregate driver with the subsystem to publish its
- * interfaces.
- *
- * Note that the lifetime of the aggregate driver does not align with
- * any of the underlying &struct device instances. Therefore devm cannot
- * be used and all resources acquired or allocated in this callback must
- * be explicitly released in the @unbind callback.
- */
- int (*bind)(struct device *master);
- /**
- * @unbind:
- *
- * Called when either the aggregate driver, using
- * component_master_del(), or one of its components, using
- * component_del(), is unregistered.
- */
- void (*unbind)(struct device *master);
-};
-
struct aggregate_device {
- const struct component_master_ops *ops;
struct device *parent;
struct device dev;
struct component_match *match;
@@ -171,11 +131,6 @@ int component_aggregate_register(struct device *parent,
void component_aggregate_unregister(struct device *parent,
struct aggregate_driver *adrv);

-void component_master_del(struct device *,
- const struct component_master_ops *);
-
-int component_master_add_with_match(struct device *,
- const struct component_master_ops *, struct component_match *);
void component_match_add_release(struct device *master,
struct component_match **matchptr,
void (*release)(struct device *, void *),
--
https://chromeos.dev


2022-01-06 21:48:04

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 31/32] component: Get rid of drm_of_component_probe()

There aren't any users anymore so drop it.

Cc: Laurent Pinchart <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/drm_of.c | 85 +++++++++-------------------------------
include/drm/drm_of.h | 12 ------
2 files changed, 19 insertions(+), 78 deletions(-)

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 008d6b7d2283..c57216e28b70 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -99,18 +99,30 @@ void drm_of_component_match_add(struct device *master,
}
EXPORT_SYMBOL_GPL(drm_of_component_match_add);

-static int _drm_of_component_probe(struct device *dev,
+/**
+ * drm_of_aggregate_probe - Generic probe function for a component based aggregate host
+ * @dev: device containing the OF node
+ * @compare_of: compare function used for matching components
+ * @adrv: aggregate driver to be used
+ *
+ * Parse the platform device OF node and bind all the components associated
+ * with the aggregate device. Interface ports are added before the encoders in
+ * order to satisfy their .bind_component requirements
+ * See Documentation/devicetree/bindings/graph.txt for the bindings.
+ *
+ * Returns zero if successful, or one of the standard error codes if it fails.
+ */
+int drm_of_aggregate_probe(struct device *dev,
int (*compare_of)(struct device *, void *),
- struct component_match **matchptr)
+ struct aggregate_driver *adrv)
{
struct device_node *ep, *port, *remote;
+ struct component_match *match = NULL;
int i;

if (!dev->of_node)
return -EINVAL;

- *matchptr = NULL;
-
/*
* Bind the crtc's ports first, so that drm_of_find_possible_crtcs()
* called from encoder's .bind callbacks works as expected
@@ -121,7 +133,7 @@ static int _drm_of_component_probe(struct device *dev,
break;

if (of_device_is_available(port->parent))
- drm_of_component_match_add(dev, matchptr, compare_of,
+ drm_of_component_match_add(dev, &match, compare_of,
port);

of_node_put(port);
@@ -132,7 +144,7 @@ static int _drm_of_component_probe(struct device *dev,
return -ENODEV;
}

- if (!*matchptr) {
+ if (!match) {
dev_err(dev, "no available port\n");
return -ENODEV;
}
@@ -162,72 +174,13 @@ static int _drm_of_component_probe(struct device *dev,
continue;
}

- drm_of_component_match_add(dev, matchptr, compare_of,
+ drm_of_component_match_add(dev, &match, compare_of,
remote);
of_node_put(remote);
}
of_node_put(port);
}

- return 0;
-}
-
-/**
- * drm_of_component_probe - Generic probe function for a component based master
- * @dev: master device containing the OF node
- * @compare_of: compare function used for matching components
- * @m_ops: component master ops to be used
- *
- * Parse the platform device OF node and bind all the components associated
- * with the master. Interface ports are added before the encoders in order to
- * satisfy their .bind requirements
- * See Documentation/devicetree/bindings/graph.txt for the bindings.
- *
- * Deprecated: Use drm_of_aggregate_probe() instead.
- *
- * Returns zero if successful, or one of the standard error codes if it fails.
- */
-int drm_of_component_probe(struct device *dev,
- int (*compare_of)(struct device *, void *),
- const struct component_master_ops *m_ops)
-{
-
- struct component_match *match;
- int ret;
-
- ret = _drm_of_component_probe(dev, compare_of, &match);
- if (ret)
- return ret;
-
- return component_master_add_with_match(dev, m_ops, match);
-}
-EXPORT_SYMBOL(drm_of_component_probe);
-
-
-/**
- * drm_of_aggregate_probe - Generic probe function for a component based aggregate host
- * @dev: device containing the OF node
- * @compare_of: compare function used for matching components
- * @adrv: aggregate driver to be used
- *
- * Parse the platform device OF node and bind all the components associated
- * with the aggregate device. Interface ports are added before the encoders in
- * order to satisfy their .bind_component requirements
- * See Documentation/devicetree/bindings/graph.txt for the bindings.
- *
- * Returns zero if successful, or one of the standard error codes if it fails.
- */
-int drm_of_aggregate_probe(struct device *dev,
- int (*compare_of)(struct device *, void *),
- struct aggregate_driver *adrv)
-{
- struct component_match *match;
- int ret;
-
- ret = _drm_of_component_probe(dev, compare_of, &match);
- if (ret)
- return ret;
-
return component_aggregate_register(dev, adrv, match);
}
EXPORT_SYMBOL(drm_of_aggregate_probe);
diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
index 9d35a141f888..33fba4f8e304 100644
--- a/include/drm/drm_of.h
+++ b/include/drm/drm_of.h
@@ -8,7 +8,6 @@
#endif

struct aggregate_driver;
-struct component_master_ops;
struct component_match;
struct device;
struct drm_device;
@@ -38,9 +37,6 @@ void drm_of_component_match_add(struct device *master,
struct component_match **matchptr,
int (*compare)(struct device *, void *),
struct device_node *node);
-int drm_of_component_probe(struct device *dev,
- int (*compare_of)(struct device *, void *),
- const struct component_master_ops *m_ops);
int drm_of_aggregate_probe(struct device *dev,
int (*compare_of)(struct device *, void *),
struct aggregate_driver *adrv);
@@ -74,14 +70,6 @@ drm_of_component_match_add(struct device *master,
{
}

-static inline int
-drm_of_component_probe(struct device *dev,
- int (*compare_of)(struct device *, void *),
- const struct component_master_ops *m_ops)
-{
- return -EINVAL;
-}
-
static inline int
drm_of_aggregate_probe(struct device *dev,
int (*compare_of)(struct device *, void *),
--
https://chromeos.dev


2022-01-06 21:48:07

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v5 30/32] ASoC: codecs: wcd938x: Migrate to aggregate driver

Use an aggregate driver instead of component ops so that we can get
proper driver probe ordering of the aggregate device with respect to all
the component devices that make up the aggregate device.

Acked-by: Mark Brown <[email protected]>
Cc: Jaroslav Kysela <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Saravana Kannan <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
sound/soc/codecs/wcd938x.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c
index 52de7d14b139..f44f5d41bfdb 100644
--- a/sound/soc/codecs/wcd938x.c
+++ b/sound/soc/codecs/wcd938x.c
@@ -4316,8 +4316,9 @@ static struct snd_soc_dai_driver wcd938x_dais[] = {
},
};

-static int wcd938x_bind(struct device *dev)
+static int wcd938x_bind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct wcd938x_priv *wcd938x = dev_get_drvdata(dev);
int ret;

@@ -4400,8 +4401,9 @@ static int wcd938x_bind(struct device *dev)

}

-static void wcd938x_unbind(struct device *dev)
+static void wcd938x_unbind(struct aggregate_device *adev)
{
+ struct device *dev = adev->parent;
struct wcd938x_priv *wcd938x = dev_get_drvdata(dev);

device_link_remove(dev, wcd938x->txdev);
@@ -4411,9 +4413,13 @@ static void wcd938x_unbind(struct device *dev)
component_unbind_all(dev, wcd938x);
}

-static const struct component_master_ops wcd938x_comp_ops = {
- .bind = wcd938x_bind,
- .unbind = wcd938x_unbind,
+static struct aggregate_driver wcd938x_aggregate_driver = {
+ .probe = wcd938x_bind,
+ .remove = wcd938x_unbind,
+ .driver = {
+ .name = "wcd938x_snd",
+ .owner = THIS_MODULE,
+ },
};

static int wcd938x_compare_of(struct device *dev, void *data)
@@ -4482,7 +4488,7 @@ static int wcd938x_probe(struct platform_device *pdev)

wcd938x_reset(wcd938x);

- ret = component_master_add_with_match(dev, &wcd938x_comp_ops, match);
+ ret = component_aggregate_register(dev, &wcd938x_aggregate_driver, match);
if (ret)
return ret;

@@ -4498,7 +4504,7 @@ static int wcd938x_probe(struct platform_device *pdev)

static int wcd938x_remove(struct platform_device *pdev)
{
- component_master_del(&pdev->dev, &wcd938x_comp_ops);
+ component_aggregate_unregister(&pdev->dev, &wcd938x_aggregate_driver);

return 0;
}
--
https://chromeos.dev


2022-01-07 13:08:13

by Jani Nikula

[permalink] [raw]
Subject: Re: [PATCH v5 03/32] component: Move struct aggregate_device out to header file

On Thu, 06 Jan 2022, Stephen Boyd <[email protected]> wrote:
> This allows aggregate driver writers to use the device passed to their
> probe/remove/shutdown functions properly instead of treating it as an
> opaque pointer.

You say it like having opaque pointers with interfaces instead of
exposed data is a bad thing.

Data is not an interface. IMO if you can get by with keeping the types
private, go for it. Unless I'm missing something, you only need the
parent dev pointer. Maybe add a helper function for it?

It's trivial to expose the guts like this, but it's usually a lot of
hard work to go the other way. Look at the dependencies of component.h
now. To keep it self-contained, i.e. buildable without implicit
dependencies, you'd need to add #include <device.h>, which goes on to
include the world. Then have a look at [1].

Please at least let's not do this lightly.


BR,
Jani.


[1] https://lwn.net/ml/linux-kernel/[email protected]/


>
> Cc: Daniel Vetter <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Laurent Pinchart <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Rob Clark <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Saravana Kannan <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> drivers/base/component.c | 15 ---------------
> include/linux/component.h | 19 ++++++++++++++++---
> 2 files changed, 16 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index eec82caeae5e..dc38a8939ae6 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -56,21 +56,6 @@ struct component_match {
> struct component_match_array *compare;
> };
>
> -struct aggregate_device {
> - const struct component_master_ops *ops;
> - struct device *parent;
> - struct device dev;
> - struct component_match *match;
> - struct aggregate_driver *adrv;
> -
> - int id;
> -};
> -
> -static inline struct aggregate_device *to_aggregate_device(struct device *d)
> -{
> - return container_of(d, struct aggregate_device, dev);
> -}
> -
> struct component {
> struct list_head node;
> struct aggregate_device *adev;
> diff --git a/include/linux/component.h b/include/linux/component.h
> index 95d1b23ede8a..e99cf8e910f0 100644
> --- a/include/linux/component.h
> +++ b/include/linux/component.h
> @@ -5,6 +5,8 @@
> #include <linux/stddef.h>
> #include <linux/device.h>
>
> +struct component_match;
> +
> /**
> * struct component_ops - callbacks for component drivers
> *
> @@ -39,8 +41,6 @@ void component_del(struct device *, const struct component_ops *);
> int component_bind_all(struct device *master, void *master_data);
> void component_unbind_all(struct device *master, void *master_data);
>
> -struct aggregate_device;
> -
> /**
> * struct component_master_ops - callback for the aggregate driver
> *
> @@ -80,7 +80,20 @@ struct component_master_ops {
> void (*unbind)(struct device *master);
> };
>
> -struct component_match;
> +struct aggregate_device {
> + const struct component_master_ops *ops;
> + struct device *parent;
> + struct device dev;
> + struct component_match *match;
> + struct aggregate_driver *adrv;
> +
> + int id;
> +};
> +
> +static inline struct aggregate_device *to_aggregate_device(struct device *d)
> +{
> + return container_of(d, struct aggregate_device, dev);
> +}
>
> /**
> * struct aggregate_driver - Aggregate driver (made up of other drivers)

--
Jani Nikula, Intel Open Source Graphics Center

2022-01-07 20:12:53

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v5 03/32] component: Move struct aggregate_device out to header file

Quoting Jani Nikula (2022-01-07 05:07:59)
> On Thu, 06 Jan 2022, Stephen Boyd <[email protected]> wrote:
> > This allows aggregate driver writers to use the device passed to their
> > probe/remove/shutdown functions properly instead of treating it as an
> > opaque pointer.
>
> You say it like having opaque pointers with interfaces instead of
> exposed data is a bad thing.

I didn't intend to convey that message at all and in fact I didn't write
that opaque pointers are a bad thing.

>
> Data is not an interface. IMO if you can get by with keeping the types
> private, go for it. Unless I'm missing something, you only need the
> parent dev pointer. Maybe add a helper function for it?

Sure I'll add a function for that.

>
> It's trivial to expose the guts like this, but it's usually a lot of
> hard work to go the other way. Look at the dependencies of component.h
> now. To keep it self-contained, i.e. buildable without implicit
> dependencies, you'd need to add #include <device.h>, which goes on to
> include the world. Then have a look at [1].
>
> Please at least let's not do this lightly.
>

Got it. Thanks! How does this look?

---8<---
diff --git a/drivers/base/component.c b/drivers/base/component.c
index cd50137753b4..e8f09945c261 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -56,6 +56,27 @@ struct component_match {
struct component_match_array *compare;
};

+struct aggregate_device {
+ const struct component_master_ops *ops;
+ struct device *parent;
+ struct device dev;
+ struct component_match *match;
+ struct aggregate_driver *adrv;
+
+ int id;
+};
+
+static inline struct aggregate_device *to_aggregate_device(struct device *d)
+{
+ return container_of(d, struct aggregate_device, dev);
+}
+
+struct device *aggregate_device_parent(struct aggregate_device *adev)
+{
+ return adev->parent;
+}
+EXPORT_SYMBOL_GPL(aggregate_device_parent);
+
struct component {
struct list_head node;
struct aggregate_device *adev;
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
index 0463386a6ed2..5fa868cf9825 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
@@ -27,7 +27,7 @@ struct komeda_dev *dev_to_mdev(struct device *dev)

static void komeda_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct komeda_drv *mdrv = dev_get_drvdata(dev);

if (!mdrv)
@@ -48,7 +48,7 @@ static void komeda_unbind(struct aggregate_device *adev)

static int komeda_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct komeda_drv *mdrv;
int err;

diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index 5c03eb98d814..e3ed925797d5 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -272,7 +272,7 @@ static const struct drm_driver hdlcd_driver = {

static int hdlcd_drm_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct drm_device *drm;
struct hdlcd_drm_private *hdlcd;
int ret;
@@ -347,7 +347,7 @@ static int hdlcd_drm_bind(struct aggregate_device *adev)

static void hdlcd_drm_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct drm_device *drm = dev_get_drvdata(dev);
struct hdlcd_drm_private *hdlcd = drm->dev_private;

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index e6ee4d1e3bb8..7b946b962b22 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -704,7 +704,7 @@ static int malidp_runtime_pm_resume(struct device *dev)

static int malidp_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct resource *res;
struct drm_device *drm;
struct malidp_drm *malidp;
@@ -897,7 +897,7 @@ static int malidp_bind(struct aggregate_device *adev)

static void malidp_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct drm_device *drm = dev_get_drvdata(dev);
struct malidp_drm *malidp = drm->dev_private;
struct malidp_hw_device *hwdev = malidp->dev;
diff --git a/drivers/gpu/drm/armada/armada_drv.c
b/drivers/gpu/drm/armada/armada_drv.c
index b3559363ea43..27739cbe2291 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -62,7 +62,7 @@ static const struct drm_mode_config_funcs
armada_drm_mode_config_funcs = {

static int armada_drm_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct armada_private *priv;
struct resource *mem = NULL;
int ret, n;
@@ -162,7 +162,7 @@ static int armada_drm_bind(struct aggregate_device *adev)

static void armada_drm_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct drm_device *drm = dev_get_drvdata(dev);
struct armada_private *priv = drm_to_armada_dev(drm);

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 95d1e518ff13..2ea655fd7a70 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -496,7 +496,7 @@ static const struct drm_driver etnaviv_drm_driver = {
*/
static int etnaviv_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct etnaviv_drm_private *priv;
struct drm_device *drm;
int ret;
@@ -555,7 +555,7 @@ static int etnaviv_bind(struct aggregate_device *adev)

static void etnaviv_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct drm_device *drm = dev_get_drvdata(dev);
struct etnaviv_drm_private *priv = drm->dev_private;

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index dcb52ec2bd35..f58c3069b591 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -255,7 +255,7 @@ static struct component_match
*exynos_drm_match_add(struct device *dev)

static int exynos_drm_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct exynos_drm_private *private;
struct drm_encoder *encoder;
struct drm_device *drm;
@@ -333,7 +333,7 @@ static int exynos_drm_bind(struct aggregate_device *adev)

static void exynos_drm_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct drm_device *drm = dev_get_drvdata(dev);

drm_dev_unregister(drm);
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
index 00d47c784cbb..338077908177 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
@@ -219,7 +219,7 @@ static int kirin_drm_kms_cleanup(struct drm_device *dev)

static int kirin_drm_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct kirin_drm_data *driver_data;
struct drm_device *drm_dev;
int ret;
@@ -256,7 +256,7 @@ static int kirin_drm_bind(struct aggregate_device *adev)

static void kirin_drm_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct drm_device *drm_dev = dev_get_drvdata(dev);

drm_dev_unregister(drm_dev);
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c
b/drivers/gpu/drm/imx/imx-drm-core.c
index 9e28bb16364c..82645e42b7d3 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -198,7 +198,7 @@ static int compare_of(struct device *dev, void *data)

static int imx_drm_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct drm_device *drm;
int ret;

@@ -267,7 +267,7 @@ static int imx_drm_bind(struct aggregate_device *adev)

static void imx_drm_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct drm_device *drm = dev_get_drvdata(dev);

drm_dev_unregister(drm);
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index d5330fb486e8..db61efc35b2d 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -1152,7 +1152,7 @@ static int ingenic_drm_bind(struct device *dev,
bool has_components)

static int ingenic_drm_bind_with_components(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);

return ingenic_drm_bind(dev, true);
}
@@ -1178,7 +1178,7 @@ static void ingenic_drm_unbind(struct device *dev)

static void ingenic_aggregate_remove(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);

ingenic_drm_unbind(dev);
}
diff --git a/drivers/gpu/drm/mcde/mcde_drv.c b/drivers/gpu/drm/mcde/mcde_drv.c
index 1652f9e0601d..b8479355844e 100644
--- a/drivers/gpu/drm/mcde/mcde_drv.c
+++ b/drivers/gpu/drm/mcde/mcde_drv.c
@@ -217,7 +217,7 @@ static const struct drm_driver mcde_drm_driver = {

static int mcde_drm_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct drm_device *drm = dev_get_drvdata(dev);
int ret;

@@ -250,7 +250,7 @@ static int mcde_drm_bind(struct aggregate_device *adev)

static void mcde_drm_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct drm_device *drm = dev_get_drvdata(dev);

drm_dev_unregister(drm);
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index a3f27b8c9769..af0dda5e45bf 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -350,7 +350,7 @@ static int compare_of(struct device *dev, void *data)

static int mtk_drm_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct mtk_drm_private *private = dev_get_drvdata(dev);
struct drm_device *drm;
int ret;
@@ -383,7 +383,7 @@ static int mtk_drm_bind(struct aggregate_device *adev)

static void mtk_drm_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct mtk_drm_private *private = dev_get_drvdata(dev);

drm_dev_unregister(private->drm);
diff --git a/drivers/gpu/drm/meson/meson_drv.c
b/drivers/gpu/drm/meson/meson_drv.c
index 3028f2a45f66..426caea3d570 100644
--- a/drivers/gpu/drm/meson/meson_drv.c
+++ b/drivers/gpu/drm/meson/meson_drv.c
@@ -358,14 +358,14 @@ static int meson_drv_bind_master(struct device
*dev, bool has_components)

static int meson_drv_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);

return meson_drv_bind_master(dev, true);
}

static void meson_drv_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct meson_drm *priv = dev_get_drvdata(dev);
struct drm_device *drm = priv->drm;

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index f6e9b0d318f5..b2735355ea81 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1362,17 +1362,19 @@ static int add_gpu_components(struct device *dev,

static int msm_drm_bind(struct aggregate_device *adev)
{
- return msm_drm_init(adev->parent, &msm_driver);
+ return msm_drm_init(aggregate_device_parent(adev), &msm_driver);
}

static void msm_drm_unbind(struct aggregate_device *adev)
{
- msm_drm_uninit(adev->parent);
+ msm_drm_uninit(aggregate_device_parent(adev));
}

static void msm_drm_shutdown(struct aggregate_device *adev)
{
- struct drm_device *drm =
platform_get_drvdata(to_platform_device(adev->parent));
+ const struct device *parent = aggregate_device_parent(adev);
+ const struct platform_device *pdev = to_platform_device(parent);
+ struct drm_device *drm = platform_get_drvdata(pdev);
struct msm_drm_private *priv = drm ? drm->dev_private : NULL;

if (!priv || !priv->kms)
diff --git a/drivers/gpu/drm/omapdrm/dss/dss.c
b/drivers/gpu/drm/omapdrm/dss/dss.c
index 9328d97f19ab..6226ef389694 100644
--- a/drivers/gpu/drm/omapdrm/dss/dss.c
+++ b/drivers/gpu/drm/omapdrm/dss/dss.c
@@ -1306,7 +1306,7 @@ static const struct soc_device_attribute
dss_soc_devices[] = {

static int dss_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct dss_device *dss = dev_get_drvdata(dev);
struct platform_device *drm_pdev;
struct dss_pdata pdata;
@@ -1333,7 +1333,7 @@ static int dss_bind(struct aggregate_device *adev)

static void dss_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct dss_device *dss = dev_get_drvdata(dev);

platform_device_unregister(dss->drm_pdev);
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 6c755361d376..5179ca899dbb 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -111,7 +111,7 @@ static void rockchip_iommu_cleanup(struct
drm_device *drm_dev)

static int rockchip_drm_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct drm_device *drm_dev;
struct rockchip_drm_private *private;
int ret;
@@ -186,7 +186,7 @@ static int rockchip_drm_bind(struct aggregate_device *adev)

static void rockchip_drm_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct drm_device *drm_dev = dev_get_drvdata(dev);

drm_dev_unregister(drm_dev);
diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c
index b277cc679154..958db315d547 100644
--- a/drivers/gpu/drm/sti/sti_drv.c
+++ b/drivers/gpu/drm/sti/sti_drv.c
@@ -184,7 +184,7 @@ static void sti_cleanup(struct drm_device *ddev)

static int sti_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct drm_device *ddev;
int ret;

@@ -219,7 +219,7 @@ static int sti_bind(struct aggregate_device *adev)

static void sti_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct drm_device *ddev = dev_get_drvdata(dev);

drm_dev_unregister(ddev);
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c
b/drivers/gpu/drm/sun4i/sun4i_drv.c
index 700f5e32eaf7..35c5e575132f 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -58,7 +58,7 @@ static const struct drm_driver sun4i_drv_driver = {

static int sun4i_drv_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct drm_device *drm;
struct sun4i_drv *drv;
int ret;
@@ -128,7 +128,7 @@ static int sun4i_drv_bind(struct aggregate_device *adev)

static void sun4i_drv_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct drm_device *drm = dev_get_drvdata(dev);

drm_dev_unregister(drm);
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 92ff516fb6de..c12c579ce66f 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -531,14 +531,14 @@ static const struct dev_pm_ops tilcdc_pm_ops = {
*/
static int tilcdc_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);

return tilcdc_init(&tilcdc_driver, dev);
}

static void tilcdc_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct drm_device *ddev = dev_get_drvdata(dev);

/* Check if a subcomponent has already triggered the unloading. */
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 82a44ebf9121..297ecddea5fb 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -214,7 +214,7 @@ static void vc4_match_add_drivers(struct device *dev,

static int vc4_drm_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct platform_device *pdev = to_platform_device(dev);
struct drm_device *drm;
struct vc4_dev *vc4;
@@ -287,7 +287,7 @@ static int vc4_drm_bind(struct aggregate_device *adev)

static void vc4_drm_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct drm_device *drm = dev_get_drvdata(dev);

drm_dev_unregister(drm);
diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
index 064fd4f4eade..125be5819c42 100644
--- a/drivers/iommu/mtk_iommu.h
+++ b/drivers/iommu/mtk_iommu.h
@@ -96,7 +96,7 @@ static inline void release_of(struct device *dev, void *data)

static inline int mtk_iommu_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct mtk_iommu_data *data = dev_get_drvdata(dev);

return component_bind_all(dev, &data->larb_imu);
@@ -104,7 +104,7 @@ static inline int mtk_iommu_bind(struct
aggregate_device *adev)

static inline void mtk_iommu_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct mtk_iommu_data *data = dev_get_drvdata(dev);

component_unbind_all(dev, &data->larb_imu);
diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c
index ae903a09fb06..3c6e4e3bf212 100644
--- a/drivers/misc/mei/hdcp/mei_hdcp.c
+++ b/drivers/misc/mei/hdcp/mei_hdcp.c
@@ -734,7 +734,7 @@ static const struct i915_hdcp_component_ops mei_hdcp_ops = {

static int mei_hdcp_aggregate_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct mei_cl_device *cldev = to_mei_cl_device(dev);
struct i915_hdcp_comp_master *comp_master =
mei_cldev_get_drvdata(cldev);
@@ -752,7 +752,7 @@ static int mei_hdcp_aggregate_bind(struct
aggregate_device *adev)

static void mei_hdcp_aggregate_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct mei_cl_device *cldev = to_mei_cl_device(dev);
struct i915_hdcp_comp_master *comp_master =
mei_cldev_get_drvdata(cldev);
diff --git a/drivers/misc/mei/pxp/mei_pxp.c b/drivers/misc/mei/pxp/mei_pxp.c
index 7b7bd7c0e8b1..887e43e6ba5f 100644
--- a/drivers/misc/mei/pxp/mei_pxp.c
+++ b/drivers/misc/mei/pxp/mei_pxp.c
@@ -85,7 +85,7 @@ static const struct i915_pxp_component_ops mei_pxp_ops = {

static int mei_pxp_aggregate_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct mei_cl_device *cldev = to_mei_cl_device(dev);
struct i915_pxp_component *comp_master = mei_cldev_get_drvdata(cldev);
int ret;
@@ -101,7 +101,7 @@ static int mei_pxp_aggregate_bind(struct
aggregate_device *adev)

static void mei_pxp_aggregate_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct mei_cl_device *cldev = to_mei_cl_device(dev);
struct i915_pxp_component *comp_master = mei_cldev_get_drvdata(cldev);

diff --git a/drivers/power/supply/ab8500_charger.c
b/drivers/power/supply/ab8500_charger.c
index 52d4105e28f2..e1e5c9387b57 100644
--- a/drivers/power/supply/ab8500_charger.c
+++ b/drivers/power/supply/ab8500_charger.c
@@ -3314,7 +3314,7 @@ static const struct power_supply_desc
ab8500_usb_chg_desc = {

static int ab8500_charger_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct ab8500_charger *di = dev_get_drvdata(dev);
int ch_stat;
int ret;
@@ -3357,7 +3357,7 @@ static int ab8500_charger_bind(struct
aggregate_device *adev)

static void ab8500_charger_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct ab8500_charger *di = dev_get_drvdata(dev);
int ret;

diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss.c
b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
index f12663c39ceb..0bdb9f909992 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dss.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
@@ -1069,7 +1069,7 @@ static int dss_video_pll_probe(struct
platform_device *pdev)
/* DSS HW IP initialisation */
static int dss_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct platform_device *pdev = to_platform_device(dev);
struct resource *dss_mem;
u32 rev;
@@ -1170,7 +1170,7 @@ static int dss_bind(struct aggregate_device *adev)

static void dss_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct platform_device *pdev = to_platform_device(dev);

dss_initialized = false;
diff --git a/include/linux/component.h b/include/linux/component.h
index 07fe481d4e3b..7c86f4cc718e 100644
--- a/include/linux/component.h
+++ b/include/linux/component.h
@@ -8,6 +8,8 @@
struct component_match;
struct aggregate_device;

+struct device *aggregate_device_parent(struct aggregate_device *adev);
+
/**
* struct component_ops - callbacks for component drivers
*
@@ -63,20 +65,6 @@ void component_del(struct device *, const struct
component_ops *);
int component_bind_all(struct device *master, void *master_data);
void component_unbind_all(struct device *master, void *master_data);

-struct aggregate_device {
- struct device *parent;
- struct device dev;
- struct component_match *match;
- struct aggregate_driver *adrv;
-
- int id;
-};
-
-static inline struct aggregate_device *to_aggregate_device(struct device *d)
-{
- return container_of(d, struct aggregate_device, dev);
-}
-
/**
* struct aggregate_driver - Aggregate driver (made up of other drivers)
* @driver: device driver
diff --git a/sound/hda/hdac_component.c b/sound/hda/hdac_component.c
index 9e4dab97f485..4ec5d9bf8533 100644
--- a/sound/hda/hdac_component.c
+++ b/sound/hda/hdac_component.c
@@ -183,7 +183,7 @@ EXPORT_SYMBOL_GPL(snd_hdac_acomp_get_eld);

static int hdac_component_master_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct drm_audio_component *acomp = hdac_get_acomp(dev);
int ret;

@@ -225,7 +225,7 @@ static int hdac_component_master_bind(struct
aggregate_device *adev)

static void hdac_component_master_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct drm_audio_component *acomp = hdac_get_acomp(dev);

if (acomp->audio_ops && acomp->audio_ops->master_unbind)
diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c
index f44f5d41bfdb..6edb040d0639 100644
--- a/sound/soc/codecs/wcd938x.c
+++ b/sound/soc/codecs/wcd938x.c
@@ -4318,7 +4318,7 @@ static struct snd_soc_dai_driver wcd938x_dais[] = {

static int wcd938x_bind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct wcd938x_priv *wcd938x = dev_get_drvdata(dev);
int ret;

@@ -4403,7 +4403,7 @@ static int wcd938x_bind(struct aggregate_device *adev)

static void wcd938x_unbind(struct aggregate_device *adev)
{
- struct device *dev = adev->parent;
+ struct device *dev = aggregate_device_parent(adev);
struct wcd938x_priv *wcd938x = dev_get_drvdata(dev);

device_link_remove(dev, wcd938x->txdev);

2022-01-10 11:24:00

by Jani Nikula

[permalink] [raw]
Subject: Re: [PATCH v5 03/32] component: Move struct aggregate_device out to header file

On Fri, 07 Jan 2022, Stephen Boyd <[email protected]> wrote:
> Quoting Jani Nikula (2022-01-07 05:07:59)
>> On Thu, 06 Jan 2022, Stephen Boyd <[email protected]> wrote:
>> > This allows aggregate driver writers to use the device passed to their
>> > probe/remove/shutdown functions properly instead of treating it as an
>> > opaque pointer.
>>
>> You say it like having opaque pointers with interfaces instead of
>> exposed data is a bad thing.
>
> I didn't intend to convey that message at all and in fact I didn't write
> that opaque pointers are a bad thing.
>
>>
>> Data is not an interface. IMO if you can get by with keeping the types
>> private, go for it. Unless I'm missing something, you only need the
>> parent dev pointer. Maybe add a helper function for it?
>
> Sure I'll add a function for that.
>
>>
>> It's trivial to expose the guts like this, but it's usually a lot of
>> hard work to go the other way. Look at the dependencies of component.h
>> now. To keep it self-contained, i.e. buildable without implicit
>> dependencies, you'd need to add #include <device.h>, which goes on to
>> include the world. Then have a look at [1].
>>
>> Please at least let's not do this lightly.
>>
>
> Got it. Thanks! How does this look?

Thanks, I think this is better.

BR,
Jani.

>
> ---8<---
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index cd50137753b4..e8f09945c261 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -56,6 +56,27 @@ struct component_match {
> struct component_match_array *compare;
> };
>
> +struct aggregate_device {
> + const struct component_master_ops *ops;
> + struct device *parent;
> + struct device dev;
> + struct component_match *match;
> + struct aggregate_driver *adrv;
> +
> + int id;
> +};
> +
> +static inline struct aggregate_device *to_aggregate_device(struct device *d)
> +{
> + return container_of(d, struct aggregate_device, dev);
> +}
> +
> +struct device *aggregate_device_parent(struct aggregate_device *adev)
> +{
> + return adev->parent;
> +}
> +EXPORT_SYMBOL_GPL(aggregate_device_parent);
> +
> struct component {
> struct list_head node;
> struct aggregate_device *adev;
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> index 0463386a6ed2..5fa868cf9825 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> @@ -27,7 +27,7 @@ struct komeda_dev *dev_to_mdev(struct device *dev)
>
> static void komeda_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct komeda_drv *mdrv = dev_get_drvdata(dev);
>
> if (!mdrv)
> @@ -48,7 +48,7 @@ static void komeda_unbind(struct aggregate_device *adev)
>
> static int komeda_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct komeda_drv *mdrv;
> int err;
>
> diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
> index 5c03eb98d814..e3ed925797d5 100644
> --- a/drivers/gpu/drm/arm/hdlcd_drv.c
> +++ b/drivers/gpu/drm/arm/hdlcd_drv.c
> @@ -272,7 +272,7 @@ static const struct drm_driver hdlcd_driver = {
>
> static int hdlcd_drm_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_device *drm;
> struct hdlcd_drm_private *hdlcd;
> int ret;
> @@ -347,7 +347,7 @@ static int hdlcd_drm_bind(struct aggregate_device *adev)
>
> static void hdlcd_drm_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_device *drm = dev_get_drvdata(dev);
> struct hdlcd_drm_private *hdlcd = drm->dev_private;
>
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> index e6ee4d1e3bb8..7b946b962b22 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -704,7 +704,7 @@ static int malidp_runtime_pm_resume(struct device *dev)
>
> static int malidp_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct resource *res;
> struct drm_device *drm;
> struct malidp_drm *malidp;
> @@ -897,7 +897,7 @@ static int malidp_bind(struct aggregate_device *adev)
>
> static void malidp_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_device *drm = dev_get_drvdata(dev);
> struct malidp_drm *malidp = drm->dev_private;
> struct malidp_hw_device *hwdev = malidp->dev;
> diff --git a/drivers/gpu/drm/armada/armada_drv.c
> b/drivers/gpu/drm/armada/armada_drv.c
> index b3559363ea43..27739cbe2291 100644
> --- a/drivers/gpu/drm/armada/armada_drv.c
> +++ b/drivers/gpu/drm/armada/armada_drv.c
> @@ -62,7 +62,7 @@ static const struct drm_mode_config_funcs
> armada_drm_mode_config_funcs = {
>
> static int armada_drm_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct armada_private *priv;
> struct resource *mem = NULL;
> int ret, n;
> @@ -162,7 +162,7 @@ static int armada_drm_bind(struct aggregate_device *adev)
>
> static void armada_drm_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_device *drm = dev_get_drvdata(dev);
> struct armada_private *priv = drm_to_armada_dev(drm);
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index 95d1e518ff13..2ea655fd7a70 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -496,7 +496,7 @@ static const struct drm_driver etnaviv_drm_driver = {
> */
> static int etnaviv_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct etnaviv_drm_private *priv;
> struct drm_device *drm;
> int ret;
> @@ -555,7 +555,7 @@ static int etnaviv_bind(struct aggregate_device *adev)
>
> static void etnaviv_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_device *drm = dev_get_drvdata(dev);
> struct etnaviv_drm_private *priv = drm->dev_private;
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index dcb52ec2bd35..f58c3069b591 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -255,7 +255,7 @@ static struct component_match
> *exynos_drm_match_add(struct device *dev)
>
> static int exynos_drm_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct exynos_drm_private *private;
> struct drm_encoder *encoder;
> struct drm_device *drm;
> @@ -333,7 +333,7 @@ static int exynos_drm_bind(struct aggregate_device *adev)
>
> static void exynos_drm_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_device *drm = dev_get_drvdata(dev);
>
> drm_dev_unregister(drm);
> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
> b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
> index 00d47c784cbb..338077908177 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
> @@ -219,7 +219,7 @@ static int kirin_drm_kms_cleanup(struct drm_device *dev)
>
> static int kirin_drm_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct kirin_drm_data *driver_data;
> struct drm_device *drm_dev;
> int ret;
> @@ -256,7 +256,7 @@ static int kirin_drm_bind(struct aggregate_device *adev)
>
> static void kirin_drm_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_device *drm_dev = dev_get_drvdata(dev);
>
> drm_dev_unregister(drm_dev);
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c
> b/drivers/gpu/drm/imx/imx-drm-core.c
> index 9e28bb16364c..82645e42b7d3 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -198,7 +198,7 @@ static int compare_of(struct device *dev, void *data)
>
> static int imx_drm_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_device *drm;
> int ret;
>
> @@ -267,7 +267,7 @@ static int imx_drm_bind(struct aggregate_device *adev)
>
> static void imx_drm_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_device *drm = dev_get_drvdata(dev);
>
> drm_dev_unregister(drm);
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index d5330fb486e8..db61efc35b2d 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -1152,7 +1152,7 @@ static int ingenic_drm_bind(struct device *dev,
> bool has_components)
>
> static int ingenic_drm_bind_with_components(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
>
> return ingenic_drm_bind(dev, true);
> }
> @@ -1178,7 +1178,7 @@ static void ingenic_drm_unbind(struct device *dev)
>
> static void ingenic_aggregate_remove(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
>
> ingenic_drm_unbind(dev);
> }
> diff --git a/drivers/gpu/drm/mcde/mcde_drv.c b/drivers/gpu/drm/mcde/mcde_drv.c
> index 1652f9e0601d..b8479355844e 100644
> --- a/drivers/gpu/drm/mcde/mcde_drv.c
> +++ b/drivers/gpu/drm/mcde/mcde_drv.c
> @@ -217,7 +217,7 @@ static const struct drm_driver mcde_drm_driver = {
>
> static int mcde_drm_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_device *drm = dev_get_drvdata(dev);
> int ret;
>
> @@ -250,7 +250,7 @@ static int mcde_drm_bind(struct aggregate_device *adev)
>
> static void mcde_drm_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_device *drm = dev_get_drvdata(dev);
>
> drm_dev_unregister(drm);
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index a3f27b8c9769..af0dda5e45bf 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -350,7 +350,7 @@ static int compare_of(struct device *dev, void *data)
>
> static int mtk_drm_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct mtk_drm_private *private = dev_get_drvdata(dev);
> struct drm_device *drm;
> int ret;
> @@ -383,7 +383,7 @@ static int mtk_drm_bind(struct aggregate_device *adev)
>
> static void mtk_drm_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct mtk_drm_private *private = dev_get_drvdata(dev);
>
> drm_dev_unregister(private->drm);
> diff --git a/drivers/gpu/drm/meson/meson_drv.c
> b/drivers/gpu/drm/meson/meson_drv.c
> index 3028f2a45f66..426caea3d570 100644
> --- a/drivers/gpu/drm/meson/meson_drv.c
> +++ b/drivers/gpu/drm/meson/meson_drv.c
> @@ -358,14 +358,14 @@ static int meson_drv_bind_master(struct device
> *dev, bool has_components)
>
> static int meson_drv_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
>
> return meson_drv_bind_master(dev, true);
> }
>
> static void meson_drv_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct meson_drm *priv = dev_get_drvdata(dev);
> struct drm_device *drm = priv->drm;
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index f6e9b0d318f5..b2735355ea81 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -1362,17 +1362,19 @@ static int add_gpu_components(struct device *dev,
>
> static int msm_drm_bind(struct aggregate_device *adev)
> {
> - return msm_drm_init(adev->parent, &msm_driver);
> + return msm_drm_init(aggregate_device_parent(adev), &msm_driver);
> }
>
> static void msm_drm_unbind(struct aggregate_device *adev)
> {
> - msm_drm_uninit(adev->parent);
> + msm_drm_uninit(aggregate_device_parent(adev));
> }
>
> static void msm_drm_shutdown(struct aggregate_device *adev)
> {
> - struct drm_device *drm =
> platform_get_drvdata(to_platform_device(adev->parent));
> + const struct device *parent = aggregate_device_parent(adev);
> + const struct platform_device *pdev = to_platform_device(parent);
> + struct drm_device *drm = platform_get_drvdata(pdev);
> struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
>
> if (!priv || !priv->kms)
> diff --git a/drivers/gpu/drm/omapdrm/dss/dss.c
> b/drivers/gpu/drm/omapdrm/dss/dss.c
> index 9328d97f19ab..6226ef389694 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dss.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dss.c
> @@ -1306,7 +1306,7 @@ static const struct soc_device_attribute
> dss_soc_devices[] = {
>
> static int dss_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct dss_device *dss = dev_get_drvdata(dev);
> struct platform_device *drm_pdev;
> struct dss_pdata pdata;
> @@ -1333,7 +1333,7 @@ static int dss_bind(struct aggregate_device *adev)
>
> static void dss_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct dss_device *dss = dev_get_drvdata(dev);
>
> platform_device_unregister(dss->drm_pdev);
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> index 6c755361d376..5179ca899dbb 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -111,7 +111,7 @@ static void rockchip_iommu_cleanup(struct
> drm_device *drm_dev)
>
> static int rockchip_drm_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_device *drm_dev;
> struct rockchip_drm_private *private;
> int ret;
> @@ -186,7 +186,7 @@ static int rockchip_drm_bind(struct aggregate_device *adev)
>
> static void rockchip_drm_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_device *drm_dev = dev_get_drvdata(dev);
>
> drm_dev_unregister(drm_dev);
> diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c
> index b277cc679154..958db315d547 100644
> --- a/drivers/gpu/drm/sti/sti_drv.c
> +++ b/drivers/gpu/drm/sti/sti_drv.c
> @@ -184,7 +184,7 @@ static void sti_cleanup(struct drm_device *ddev)
>
> static int sti_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_device *ddev;
> int ret;
>
> @@ -219,7 +219,7 @@ static int sti_bind(struct aggregate_device *adev)
>
> static void sti_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_device *ddev = dev_get_drvdata(dev);
>
> drm_dev_unregister(ddev);
> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c
> b/drivers/gpu/drm/sun4i/sun4i_drv.c
> index 700f5e32eaf7..35c5e575132f 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
> @@ -58,7 +58,7 @@ static const struct drm_driver sun4i_drv_driver = {
>
> static int sun4i_drv_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_device *drm;
> struct sun4i_drv *drv;
> int ret;
> @@ -128,7 +128,7 @@ static int sun4i_drv_bind(struct aggregate_device *adev)
>
> static void sun4i_drv_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_device *drm = dev_get_drvdata(dev);
>
> drm_dev_unregister(drm);
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index 92ff516fb6de..c12c579ce66f 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -531,14 +531,14 @@ static const struct dev_pm_ops tilcdc_pm_ops = {
> */
> static int tilcdc_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
>
> return tilcdc_init(&tilcdc_driver, dev);
> }
>
> static void tilcdc_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_device *ddev = dev_get_drvdata(dev);
>
> /* Check if a subcomponent has already triggered the unloading. */
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> index 82a44ebf9121..297ecddea5fb 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> @@ -214,7 +214,7 @@ static void vc4_match_add_drivers(struct device *dev,
>
> static int vc4_drm_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct platform_device *pdev = to_platform_device(dev);
> struct drm_device *drm;
> struct vc4_dev *vc4;
> @@ -287,7 +287,7 @@ static int vc4_drm_bind(struct aggregate_device *adev)
>
> static void vc4_drm_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_device *drm = dev_get_drvdata(dev);
>
> drm_dev_unregister(drm);
> diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> index 064fd4f4eade..125be5819c42 100644
> --- a/drivers/iommu/mtk_iommu.h
> +++ b/drivers/iommu/mtk_iommu.h
> @@ -96,7 +96,7 @@ static inline void release_of(struct device *dev, void *data)
>
> static inline int mtk_iommu_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct mtk_iommu_data *data = dev_get_drvdata(dev);
>
> return component_bind_all(dev, &data->larb_imu);
> @@ -104,7 +104,7 @@ static inline int mtk_iommu_bind(struct
> aggregate_device *adev)
>
> static inline void mtk_iommu_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct mtk_iommu_data *data = dev_get_drvdata(dev);
>
> component_unbind_all(dev, &data->larb_imu);
> diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c
> index ae903a09fb06..3c6e4e3bf212 100644
> --- a/drivers/misc/mei/hdcp/mei_hdcp.c
> +++ b/drivers/misc/mei/hdcp/mei_hdcp.c
> @@ -734,7 +734,7 @@ static const struct i915_hdcp_component_ops mei_hdcp_ops = {
>
> static int mei_hdcp_aggregate_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct mei_cl_device *cldev = to_mei_cl_device(dev);
> struct i915_hdcp_comp_master *comp_master =
> mei_cldev_get_drvdata(cldev);
> @@ -752,7 +752,7 @@ static int mei_hdcp_aggregate_bind(struct
> aggregate_device *adev)
>
> static void mei_hdcp_aggregate_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct mei_cl_device *cldev = to_mei_cl_device(dev);
> struct i915_hdcp_comp_master *comp_master =
> mei_cldev_get_drvdata(cldev);
> diff --git a/drivers/misc/mei/pxp/mei_pxp.c b/drivers/misc/mei/pxp/mei_pxp.c
> index 7b7bd7c0e8b1..887e43e6ba5f 100644
> --- a/drivers/misc/mei/pxp/mei_pxp.c
> +++ b/drivers/misc/mei/pxp/mei_pxp.c
> @@ -85,7 +85,7 @@ static const struct i915_pxp_component_ops mei_pxp_ops = {
>
> static int mei_pxp_aggregate_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct mei_cl_device *cldev = to_mei_cl_device(dev);
> struct i915_pxp_component *comp_master = mei_cldev_get_drvdata(cldev);
> int ret;
> @@ -101,7 +101,7 @@ static int mei_pxp_aggregate_bind(struct
> aggregate_device *adev)
>
> static void mei_pxp_aggregate_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct mei_cl_device *cldev = to_mei_cl_device(dev);
> struct i915_pxp_component *comp_master = mei_cldev_get_drvdata(cldev);
>
> diff --git a/drivers/power/supply/ab8500_charger.c
> b/drivers/power/supply/ab8500_charger.c
> index 52d4105e28f2..e1e5c9387b57 100644
> --- a/drivers/power/supply/ab8500_charger.c
> +++ b/drivers/power/supply/ab8500_charger.c
> @@ -3314,7 +3314,7 @@ static const struct power_supply_desc
> ab8500_usb_chg_desc = {
>
> static int ab8500_charger_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct ab8500_charger *di = dev_get_drvdata(dev);
> int ch_stat;
> int ret;
> @@ -3357,7 +3357,7 @@ static int ab8500_charger_bind(struct
> aggregate_device *adev)
>
> static void ab8500_charger_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct ab8500_charger *di = dev_get_drvdata(dev);
> int ret;
>
> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss.c
> b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
> index f12663c39ceb..0bdb9f909992 100644
> --- a/drivers/video/fbdev/omap2/omapfb/dss/dss.c
> +++ b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
> @@ -1069,7 +1069,7 @@ static int dss_video_pll_probe(struct
> platform_device *pdev)
> /* DSS HW IP initialisation */
> static int dss_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct platform_device *pdev = to_platform_device(dev);
> struct resource *dss_mem;
> u32 rev;
> @@ -1170,7 +1170,7 @@ static int dss_bind(struct aggregate_device *adev)
>
> static void dss_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct platform_device *pdev = to_platform_device(dev);
>
> dss_initialized = false;
> diff --git a/include/linux/component.h b/include/linux/component.h
> index 07fe481d4e3b..7c86f4cc718e 100644
> --- a/include/linux/component.h
> +++ b/include/linux/component.h
> @@ -8,6 +8,8 @@
> struct component_match;
> struct aggregate_device;
>
> +struct device *aggregate_device_parent(struct aggregate_device *adev);
> +
> /**
> * struct component_ops - callbacks for component drivers
> *
> @@ -63,20 +65,6 @@ void component_del(struct device *, const struct
> component_ops *);
> int component_bind_all(struct device *master, void *master_data);
> void component_unbind_all(struct device *master, void *master_data);
>
> -struct aggregate_device {
> - struct device *parent;
> - struct device dev;
> - struct component_match *match;
> - struct aggregate_driver *adrv;
> -
> - int id;
> -};
> -
> -static inline struct aggregate_device *to_aggregate_device(struct device *d)
> -{
> - return container_of(d, struct aggregate_device, dev);
> -}
> -
> /**
> * struct aggregate_driver - Aggregate driver (made up of other drivers)
> * @driver: device driver
> diff --git a/sound/hda/hdac_component.c b/sound/hda/hdac_component.c
> index 9e4dab97f485..4ec5d9bf8533 100644
> --- a/sound/hda/hdac_component.c
> +++ b/sound/hda/hdac_component.c
> @@ -183,7 +183,7 @@ EXPORT_SYMBOL_GPL(snd_hdac_acomp_get_eld);
>
> static int hdac_component_master_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_audio_component *acomp = hdac_get_acomp(dev);
> int ret;
>
> @@ -225,7 +225,7 @@ static int hdac_component_master_bind(struct
> aggregate_device *adev)
>
> static void hdac_component_master_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_audio_component *acomp = hdac_get_acomp(dev);
>
> if (acomp->audio_ops && acomp->audio_ops->master_unbind)
> diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c
> index f44f5d41bfdb..6edb040d0639 100644
> --- a/sound/soc/codecs/wcd938x.c
> +++ b/sound/soc/codecs/wcd938x.c
> @@ -4318,7 +4318,7 @@ static struct snd_soc_dai_driver wcd938x_dais[] = {
>
> static int wcd938x_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct wcd938x_priv *wcd938x = dev_get_drvdata(dev);
> int ret;
>
> @@ -4403,7 +4403,7 @@ static int wcd938x_bind(struct aggregate_device *adev)
>
> static void wcd938x_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct wcd938x_priv *wcd938x = dev_get_drvdata(dev);
>
> device_link_remove(dev, wcd938x->txdev);

--
Jani Nikula, Intel Open Source Graphics Center

2022-01-11 12:22:30

by Yong Wu (吴勇)

[permalink] [raw]
Subject: Re: [PATCH v5 25/32] iommu/mtk: Migrate to aggregate driver

Hi Stephen,

Thanks for helping update here.

On Thu, 2022-01-06 at 13:45 -0800, Stephen Boyd wrote:
> Use an aggregate driver instead of component ops so that we can get
> proper driver probe ordering of the aggregate device with respect to
> all
> the component devices that make up the aggregate device.
>
> Cc: Yong Wu <[email protected]>
> Cc: Joerg Roedel <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Rob Clark <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Saravana Kannan <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>

When I test this on mt8195 which have two IOMMU HWs(calling
component_aggregate_regsiter twice), it will abort like this. Then what
should we do if we have two instances?
Thanks.

[ 2.652424] Error: Driver 'mtk_iommu_agg' is already registered,
aborting...
[ 2.654033] mtk-iommu: probe of 1c01f000.iommu failed with error -16
[ 2.662034] Unable to handle kernel NULL pointer dereference at
virtual address 0000000000000020
...
[ 2.672413] pc : aggregate_device_match+0xa8/0x1c8
[ 2.673027] lr : aggregate_device_match+0x68/0x1c8
...
[ 2.683091] Call trace:
[ 2.683403] aggregate_device_match+0xa8/0x1c8
[ 2.683970] __device_attach_driver+0x38/0xd0
[ 2.684526] bus_for_each_drv+0x68/0xd0
[ 2.685015] __device_attach+0xec/0x148
[ 2.685503] device_attach+0x14/0x20
[ 2.685960] bus_rescan_devices_helper+0x50/0x90
[ 2.686545] bus_for_each_dev+0x7c/0xd8
[ 2.687033] bus_rescan_devices+0x20/0x30
[ 2.687542] __component_add+0x7c/0xa0
[ 2.688022] component_add+0x14/0x20
[ 2.688479] mtk_smi_larb_probe+0xe0/0x120


> ---
> drivers/iommu/mtk_iommu.c | 14 +++++++++-----
> drivers/iommu/mtk_iommu.h | 6 ++++--
> drivers/iommu/mtk_iommu_v1.c | 14 +++++++++-----
> 3 files changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 25b834104790..8e722898cbe2 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -752,9 +752,13 @@ static int mtk_iommu_hw_init(const struct
> mtk_iommu_data *data)
> return 0;
> }
>
> -static const struct component_master_ops mtk_iommu_com_ops = {
> - .bind = mtk_iommu_bind,
> - .unbind = mtk_iommu_unbind,
> +static struct aggregate_driver mtk_iommu_aggregate_driver = {
> + .probe = mtk_iommu_bind,
> + .remove = mtk_iommu_unbind,
> + .driver = {
> + .name = "mtk_iommu_agg",
> + .owner = THIS_MODULE,
> + },
> };
>
> static int mtk_iommu_probe(struct platform_device *pdev)
> @@ -895,7 +899,7 @@ static int mtk_iommu_probe(struct platform_device
> *pdev)
> goto out_list_del;
> }
>
> - ret = component_master_add_with_match(dev, &mtk_iommu_com_ops,
> match);
> + ret = component_aggregate_register(dev,
> &mtk_iommu_aggregate_driver, match);
> if (ret)
> goto out_bus_set_null;
> return ret;
> @@ -928,7 +932,7 @@ static int mtk_iommu_remove(struct
> platform_device *pdev)
> device_link_remove(data->smicomm_dev, &pdev->dev);
> pm_runtime_disable(&pdev->dev);
> devm_free_irq(&pdev->dev, data->irq, data);
> - component_master_del(&pdev->dev, &mtk_iommu_com_ops);
> + component_aggregate_unregister(&pdev->dev,
> &mtk_iommu_aggregate_driver);
> return 0;
> }
>
> diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> index f81fa8862ed0..064fd4f4eade 100644
> --- a/drivers/iommu/mtk_iommu.h
> +++ b/drivers/iommu/mtk_iommu.h
> @@ -94,15 +94,17 @@ static inline void release_of(struct device *dev,
> void *data)
> of_node_put(data);
> }
>
> -static inline int mtk_iommu_bind(struct device *dev)
> +static inline int mtk_iommu_bind(struct aggregate_device *adev)
> {
> + struct device *dev = adev->parent;
> struct mtk_iommu_data *data = dev_get_drvdata(dev);
>
> return component_bind_all(dev, &data->larb_imu);
> }
>
> -static inline void mtk_iommu_unbind(struct device *dev)
> +static inline void mtk_iommu_unbind(struct aggregate_device *adev)
> {
> + struct device *dev = adev->parent;
> struct mtk_iommu_data *data = dev_get_drvdata(dev);
>
> component_unbind_all(dev, &data->larb_imu);
> diff --git a/drivers/iommu/mtk_iommu_v1.c
> b/drivers/iommu/mtk_iommu_v1.c
> index be22fcf988ce..5fb29058a165 100644
> --- a/drivers/iommu/mtk_iommu_v1.c
> +++ b/drivers/iommu/mtk_iommu_v1.c
> @@ -534,9 +534,13 @@ static const struct of_device_id
> mtk_iommu_of_ids[] = {
> {}
> };
>
> -static const struct component_master_ops mtk_iommu_com_ops = {
> - .bind = mtk_iommu_bind,
> - .unbind = mtk_iommu_unbind,
> +static struct aggregate_driver mtk_iommu_aggregate_driver = {
> + .probe = mtk_iommu_bind,
> + .remove = mtk_iommu_unbind,
> + .driver = {
> + .name = "mtk_iommu_agg",
> + .owner = THIS_MODULE,
> + },
> };
>
> static int mtk_iommu_probe(struct platform_device *pdev)
> @@ -624,7 +628,7 @@ static int mtk_iommu_probe(struct platform_device
> *pdev)
> goto out_dev_unreg;
> }
>
> - ret = component_master_add_with_match(dev, &mtk_iommu_com_ops,
> match);
> + ret = component_aggregate_register(dev,
> &mtk_iommu_aggregate_driver, match);
> if (ret)
> goto out_bus_set_null;
> return ret;
> @@ -650,7 +654,7 @@ static int mtk_iommu_remove(struct
> platform_device *pdev)
>
> clk_disable_unprepare(data->bclk);
> devm_free_irq(&pdev->dev, data->irq, data);
> - component_master_del(&pdev->dev, &mtk_iommu_com_ops);
> + component_aggregate_unregister(&pdev->dev,
> &mtk_iommu_aggregate_driver);
> return 0;
> }
>


2022-01-12 00:27:32

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v5 25/32] iommu/mtk: Migrate to aggregate driver

Quoting Yong Wu (2022-01-11 04:22:23)
> Hi Stephen,
>
> Thanks for helping update here.
>
> On Thu, 2022-01-06 at 13:45 -0800, Stephen Boyd wrote:
> > Use an aggregate driver instead of component ops so that we can get
> > proper driver probe ordering of the aggregate device with respect to
> > all
> > the component devices that make up the aggregate device.
> >
> > Cc: Yong Wu <[email protected]>
> > Cc: Joerg Roedel <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Cc: Daniel Vetter <[email protected]>
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > Cc: Rob Clark <[email protected]>
> > Cc: Russell King <[email protected]>
> > Cc: Saravana Kannan <[email protected]>
> > Signed-off-by: Stephen Boyd <[email protected]>
>
> When I test this on mt8195 which have two IOMMU HWs(calling
> component_aggregate_regsiter twice), it will abort like this. Then what
> should we do if we have two instances?
>

Thanks for testing it out. We can't register the struct driver more than
once but this driver is calling the component_aggregate_register()
function from the driver probe and there are two devices bound to the
mtk-iommu driver so we try to register it more than once. Sigh!

I see a couple options. One is to do a deep copy of the driver structure
and change the driver name. Then it's a one to one relationship between
device and driver. That's not very great because it leaves around junk
so it should probably be avoided.

Another option is to reference count the driver registration calls when
component_aggregate_register() is called multiple times. Then we would
only register the driver once and keep it pinned until the last
unregister call is made, but still remove devices that are created for
the match table.

Can you try the attached patch? It is based on the next version of this
patch series so the include part of the patch may not apply cleanly.

---8<---
diff --git a/drivers/base/component.c b/drivers/base/component.c
index 64ad7478c67a..97f253a41bdf 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -492,15 +492,30 @@ static struct aggregate_device
*__aggregate_find(struct device *parent)
return dev ? to_aggregate_device(dev) : NULL;
}

+static DEFINE_MUTEX(aggregate_mutex);
+
static int aggregate_driver_register(struct aggregate_driver *adrv)
{
- adrv->driver.bus = &aggregate_bus_type;
- return driver_register(&adrv->driver);
+ int ret = 0;
+
+ mutex_lock(&aggregate_mutex);
+ if (!refcount_inc_not_zero(&adrv->count)) {
+ adrv->driver.bus = &aggregate_bus_type;
+ ret = driver_register(&adrv->driver);
+ if (!ret)
+ refcount_inc(&adrv->count);
+ }
+ mutex_unlock(&aggregate_mutex);
+
+ return ret;
}

static void aggregate_driver_unregister(struct aggregate_driver *adrv)
{
- driver_unregister(&adrv->driver);
+ if (refcount_dec_and_mutex_lock(&adrv->count, &aggregate_mutex)) {
+ driver_unregister(&adrv->driver);
+ mutex_unlock(&aggregate_mutex);
+ }
}

static struct aggregate_device *aggregate_device_add(struct device *parent,
diff --git a/include/linux/component.h b/include/linux/component.h
index 53d81203c095..b061341938aa 100644
--- a/include/linux/component.h
+++ b/include/linux/component.h
@@ -4,6 +4,7 @@

#include <linux/stddef.h>
#include <linux/device.h>
+#include <linux/refcount.h>

struct aggregate_device;

@@ -66,6 +67,7 @@ struct device *aggregate_device_parent(const struct
aggregate_device *adev);

/**
* struct aggregate_driver - Aggregate driver (made up of other drivers)
+ * @count: driver registration refcount
* @driver: device driver
*/
struct aggregate_driver {
@@ -101,6 +103,7 @@ struct aggregate_driver {
*/
void (*shutdown)(struct aggregate_device *adev);

+ refcount_t count;
struct device_driver driver;
};

2022-01-12 09:09:26

by Yong Wu (吴勇)

[permalink] [raw]
Subject: Re: [PATCH v5 25/32] iommu/mtk: Migrate to aggregate driver

On Tue, 2022-01-11 at 16:27 -0800, Stephen Boyd wrote:
> Quoting Yong Wu (2022-01-11 04:22:23)
> > Hi Stephen,
> >
> > Thanks for helping update here.
> >
> > On Thu, 2022-01-06 at 13:45 -0800, Stephen Boyd wrote:
> > > Use an aggregate driver instead of component ops so that we can
> > > get
> > > proper driver probe ordering of the aggregate device with respect
> > > to
> > > all
> > > the component devices that make up the aggregate device.
> > >
> > > Cc: Yong Wu <[email protected]>
> > > Cc: Joerg Roedel <[email protected]>
> > > Cc: Will Deacon <[email protected]>
> > > Cc: Daniel Vetter <[email protected]>
> > > Cc: "Rafael J. Wysocki" <[email protected]>
> > > Cc: Rob Clark <[email protected]>
> > > Cc: Russell King <[email protected]>
> > > Cc: Saravana Kannan <[email protected]>
> > > Signed-off-by: Stephen Boyd <[email protected]>
> >
> > When I test this on mt8195 which have two IOMMU HWs(calling
> > component_aggregate_regsiter twice), it will abort like this. Then
> > what
> > should we do if we have two instances?
> >
>
> Thanks for testing it out. We can't register the struct driver more
> than
> once but this driver is calling the component_aggregate_register()
> function from the driver probe and there are two devices bound to the
> mtk-iommu driver so we try to register it more than once. Sigh!
>
> I see a couple options. One is to do a deep copy of the driver
> structure
> and change the driver name. Then it's a one to one relationship
> between
> device and driver. That's not very great because it leaves around
> junk
> so it should probably be avoided.
>
> Another option is to reference count the driver registration calls
> when
> component_aggregate_register() is called multiple times. Then we
> would
> only register the driver once and keep it pinned until the last
> unregister call is made, but still remove devices that are created
> for
> the match table.
>
> Can you try the attached patch? It is based on the next version of
> this
> patch series so the include part of the patch may not apply cleanly.
>
> ---8<---
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index 64ad7478c67a..97f253a41bdf 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -492,15 +492,30 @@ static struct aggregate_device
> *__aggregate_find(struct device *parent)
> return dev ? to_aggregate_device(dev) : NULL;
> }
>
> +static DEFINE_MUTEX(aggregate_mutex);
> +
> static int aggregate_driver_register(struct aggregate_driver *adrv)
> {
> - adrv->driver.bus = &aggregate_bus_type;
> - return driver_register(&adrv->driver);
> + int ret = 0;
> +
> + mutex_lock(&aggregate_mutex);
> + if (!refcount_inc_not_zero(&adrv->count)) {
> + adrv->driver.bus = &aggregate_bus_type;
> + ret = driver_register(&adrv->driver);
> + if (!ret)
> + refcount_inc(&adrv->count);

This should be refcount_set(&adrv->count, 1)?

Otherwise, it will warning like this:

[ 2.654526] ------------[ cut here ]------------
[ 2.655558] refcount_t: addition on 0; use-after-free.
[ 2.656219] WARNING: CPU: 7 PID: 74 at ../v5.16-
rc1/kernel/mediatek/lib/refcount.c:25
refcount_warn_saturate+0x128/0x148
...
[ 2.672227] Call trace:
[ 2.672539] refcount_warn_saturate+0x128/0x148
[ 2.673118] component_aggregate_register+0x388/0x390
[ 2.673763] mtk_iommu_probe+0x638/0x690

[ 2.686467] ------------[ cut here ]------------
[ 2.687049] refcount_t: saturated; leaking memory.
[ 2.687666] WARNING: CPU: 5 PID: 74 at ../v5.16-
rc1/kernel/mediatek/lib/refcount.c:19 refcount_warn_saturate+0xfc/0x148

[ 2.703805] Call trace:
[ 2.704117] refcount_warn_saturate+0xfc/0x148
[ 2.704685] component_aggregate_register+0x1fc/0x390
[ 2.705330] mtk_iommu_probe+0x638/0x690

> + }
> + mutex_unlock(&aggregate_mutex);
> +
> + return ret;
> }
>
> static void aggregate_driver_unregister(struct aggregate_driver
> *adrv)
> {
> - driver_unregister(&adrv->driver);
> + if (refcount_dec_and_mutex_lock(&adrv->count,
> &aggregate_mutex)) {
> + driver_unregister(&adrv->driver);
> + mutex_unlock(&aggregate_mutex);
> + }
> }
>
> static struct aggregate_device *aggregate_device_add(struct device
> *parent,
> diff --git a/include/linux/component.h b/include/linux/component.h
> index 53d81203c095..b061341938aa 100644
> --- a/include/linux/component.h
> +++ b/include/linux/component.h
> @@ -4,6 +4,7 @@
>
> #include <linux/stddef.h>
> #include <linux/device.h>
> +#include <linux/refcount.h>
>
> struct aggregate_device;
>
> @@ -66,6 +67,7 @@ struct device *aggregate_device_parent(const struct
> aggregate_device *adev);
>
> /**
> * struct aggregate_driver - Aggregate driver (made up of other
> drivers)
> + * @count: driver registration refcount
> * @driver: device driver
> */
> struct aggregate_driver {
> @@ -101,6 +103,7 @@ struct aggregate_driver {
> */
> void (*shutdown)(struct aggregate_device *adev);
>
> + refcount_t count;
> struct device_driver driver;
> };

After this patch, the aggregate_driver flow looks ok. But our driver
still aborts like this:

[ 2.721316] Unable to handle kernel NULL pointer dereference at
virtual address 0000000000000000
...
[ 2.731658] pc : mtk_smi_larb_config_port_gen2_general+0xa4/0x138
[ 2.732434] lr : mtk_smi_larb_resume+0x54/0x98
...
[ 2.742457] Call trace:
[ 2.742768] mtk_smi_larb_config_port_gen2_general+0xa4/0x138
[ 2.743496] pm_generic_runtime_resume+0x2c/0x48
[ 2.744090] __genpd_runtime_resume+0x30/0xa8
[ 2.744648] genpd_runtime_resume+0x94/0x2c8
[ 2.745191] __rpm_callback+0x44/0x150
[ 2.745669] rpm_callback+0x6c/0x78
[ 2.746114] rpm_resume+0x314/0x558
[ 2.746559] __pm_runtime_resume+0x3c/0x88
[ 2.747080] pm_runtime_get_suppliers+0x7c/0x110
[ 2.747668] __driver_probe_device+0x4c/0xe8
[ 2.748212] driver_probe_device+0x44/0x130
[ 2.748745] __device_attach_driver+0x98/0xd0
[ 2.749300] bus_for_each_drv+0x68/0xd0
[ 2.749787] __device_attach+0xec/0x148
[ 2.750277] device_attach+0x14/0x20
[ 2.750733] bus_rescan_devices_helper+0x50/0x90
[ 2.751319] bus_for_each_dev+0x7c/0xd8
[ 2.751806] bus_rescan_devices+0x20/0x30
[ 2.752315] __component_add+0x7c/0xa0
[ 2.752795] component_add+0x14/0x20
[ 2.753253] mtk_smi_larb_probe+0xe0/0x120

This is because the device runtime_resume is called before the bind
operation(In our case this detailed function is mtk_smi_larb_bind).
The issue doesn't happen without this patchset. I'm not sure the right
sequence. If we should fix in mediatek driver, the patch could be:


diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
index b883dcc0bbfa..288841555067 100644
--- a/drivers/memory/mtk-smi.c
+++ b/drivers/memory/mtk-smi.c
@@ -483,8 +483,9 @@ static int __maybe_unused
mtk_smi_larb_resume(struct device *dev)
if (ret < 0)
return ret;

- /* Configure the basic setting for this larb */
- larb_gen->config_port(dev);
+ /* Configure the basic setting for this larb after it binds
with iommu */
+ if (larb->mmu)
+ larb_gen->config_port(dev);

return 0;
}


Another nitpick, the title should be: iommu/mediatek: xxxx



2022-01-13 04:25:50

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v5 25/32] iommu/mtk: Migrate to aggregate driver

Quoting Yong Wu (2022-01-12 01:09:19)
> On Tue, 2022-01-11 at 16:27 -0800, Stephen Boyd wrote:
> > ---8<---
> > diff --git a/drivers/base/component.c b/drivers/base/component.c
> > index 64ad7478c67a..97f253a41bdf 100644
> > --- a/drivers/base/component.c
> > +++ b/drivers/base/component.c
> > @@ -492,15 +492,30 @@ static struct aggregate_device
> > *__aggregate_find(struct device *parent)
> > return dev ? to_aggregate_device(dev) : NULL;
> > }
> >
> > +static DEFINE_MUTEX(aggregate_mutex);
> > +
> > static int aggregate_driver_register(struct aggregate_driver *adrv)
> > {
> > - adrv->driver.bus = &aggregate_bus_type;
> > - return driver_register(&adrv->driver);
> > + int ret = 0;
> > +
> > + mutex_lock(&aggregate_mutex);
> > + if (!refcount_inc_not_zero(&adrv->count)) {
> > + adrv->driver.bus = &aggregate_bus_type;
> > + ret = driver_register(&adrv->driver);
> > + if (!ret)
> > + refcount_inc(&adrv->count);
>
> This should be refcount_set(&adrv->count, 1)?
>
> Otherwise, it will warning like this:

Yeah I'll fix it, thanks.

>
> [ 2.654526] ------------[ cut here ]------------
> [ 2.655558] refcount_t: addition on 0; use-after-free.
>
> After this patch, the aggregate_driver flow looks ok. But our driver
> still aborts like this:
>
> [ 2.721316] Unable to handle kernel NULL pointer dereference at
> virtual address 0000000000000000
> ...
> [ 2.731658] pc : mtk_smi_larb_config_port_gen2_general+0xa4/0x138
> [ 2.732434] lr : mtk_smi_larb_resume+0x54/0x98
> ...
> [ 2.742457] Call trace:
> [ 2.742768] mtk_smi_larb_config_port_gen2_general+0xa4/0x138
> [ 2.743496] pm_generic_runtime_resume+0x2c/0x48
> [ 2.744090] __genpd_runtime_resume+0x30/0xa8
> [ 2.744648] genpd_runtime_resume+0x94/0x2c8
> [ 2.745191] __rpm_callback+0x44/0x150
> [ 2.745669] rpm_callback+0x6c/0x78
> [ 2.746114] rpm_resume+0x314/0x558
> [ 2.746559] __pm_runtime_resume+0x3c/0x88
> [ 2.747080] pm_runtime_get_suppliers+0x7c/0x110
> [ 2.747668] __driver_probe_device+0x4c/0xe8
> [ 2.748212] driver_probe_device+0x44/0x130
> [ 2.748745] __device_attach_driver+0x98/0xd0
> [ 2.749300] bus_for_each_drv+0x68/0xd0
> [ 2.749787] __device_attach+0xec/0x148
> [ 2.750277] device_attach+0x14/0x20
> [ 2.750733] bus_rescan_devices_helper+0x50/0x90
> [ 2.751319] bus_for_each_dev+0x7c/0xd8
> [ 2.751806] bus_rescan_devices+0x20/0x30
> [ 2.752315] __component_add+0x7c/0xa0
> [ 2.752795] component_add+0x14/0x20
> [ 2.753253] mtk_smi_larb_probe+0xe0/0x120
>
> This is because the device runtime_resume is called before the bind
> operation(In our case this detailed function is mtk_smi_larb_bind).
> The issue doesn't happen without this patchset. I'm not sure the right
> sequence. If we should fix in mediatek driver, the patch could be:

Oh, the runtime PM is moved around with these patches. The aggregate
device is runtime PM enabled before the probe is called, and there are
supplier links made to each component, so each component is runtime
resumed before the aggregate probe function is called. It means that all
the component drivers need to have their resources ready to power on
before their component_bind() callback is made. Thinking more about it
that may be wrong if something from the aggregate device is needed to
fully power on the component. Is that what is happening here?

>
>
> diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
> index b883dcc0bbfa..288841555067 100644
> --- a/drivers/memory/mtk-smi.c
> +++ b/drivers/memory/mtk-smi.c
> @@ -483,8 +483,9 @@ static int __maybe_unused
> mtk_smi_larb_resume(struct device *dev)
> if (ret < 0)
> return ret;
>
> - /* Configure the basic setting for this larb */
> - larb_gen->config_port(dev);
> + /* Configure the basic setting for this larb after it binds
> with iommu */
> + if (larb->mmu)
> + larb_gen->config_port(dev);
>
> return 0;
> }
>
>
> Another nitpick, the title should be: iommu/mediatek: xxxx
>

Fixed it.

2022-01-14 09:06:43

by Yong Wu (吴勇)

[permalink] [raw]
Subject: Re: [PATCH v5 25/32] iommu/mtk: Migrate to aggregate driver

On Wed, 2022-01-12 at 20:25 -0800, Stephen Boyd wrote:
> >
> > [ 2.654526] ------------[ cut here ]------------
> > [ 2.655558] refcount_t: addition on 0; use-after-free.
> >
> > After this patch, the aggregate_driver flow looks ok. But our
> > driver
> > still aborts like this:
> >
> > [ 2.721316] Unable to handle kernel NULL pointer dereference at
> > virtual address 0000000000000000
> > ...
> > [ 2.731658] pc :
> > mtk_smi_larb_config_port_gen2_general+0xa4/0x138
> > [ 2.732434] lr : mtk_smi_larb_resume+0x54/0x98
> > ...
> > [ 2.742457] Call trace:
> > [ 2.742768] mtk_smi_larb_config_port_gen2_general+0xa4/0x138
> > [ 2.743496] pm_generic_runtime_resume+0x2c/0x48
> > [ 2.744090] __genpd_runtime_resume+0x30/0xa8
> > [ 2.744648] genpd_runtime_resume+0x94/0x2c8
> > [ 2.745191] __rpm_callback+0x44/0x150
> > [ 2.745669] rpm_callback+0x6c/0x78
> > [ 2.746114] rpm_resume+0x314/0x558
> > [ 2.746559] __pm_runtime_resume+0x3c/0x88
> > [ 2.747080] pm_runtime_get_suppliers+0x7c/0x110
> > [ 2.747668] __driver_probe_device+0x4c/0xe8
> > [ 2.748212] driver_probe_device+0x44/0x130
> > [ 2.748745] __device_attach_driver+0x98/0xd0
> > [ 2.749300] bus_for_each_drv+0x68/0xd0
> > [ 2.749787] __device_attach+0xec/0x148
> > [ 2.750277] device_attach+0x14/0x20
> > [ 2.750733] bus_rescan_devices_helper+0x50/0x90
> > [ 2.751319] bus_for_each_dev+0x7c/0xd8
> > [ 2.751806] bus_rescan_devices+0x20/0x30
> > [ 2.752315] __component_add+0x7c/0xa0
> > [ 2.752795] component_add+0x14/0x20
> > [ 2.753253] mtk_smi_larb_probe+0xe0/0x120
> >
> > This is because the device runtime_resume is called before the bind
> > operation(In our case this detailed function is mtk_smi_larb_bind).
> > The issue doesn't happen without this patchset. I'm not sure the
> > right
> > sequence. If we should fix in mediatek driver, the patch could be:
>
> Oh, the runtime PM is moved around with these patches. The aggregate
> device is runtime PM enabled before the probe is called,

In our case, the component device may probe before the aggregate
device. thus the component device runtime PM has already been enabled
when aggregate device probe.

> and there are
> supplier links made to each component, so each component is runtime
> resumed before the aggregate probe function is called.

Yes. This is the current flow.

> It means that all
> the component drivers need to have their resources ready to power on
> before their component_bind() callback is made.

Sorry, I don't understand here well. In this case, The component
drivers prepare the resource for power on in the component_bind since
the resource comes from the aggregate driver. Thus, we expect the
component_bind run before the runtime resume callback.

Another solution is moving the component's pm_runtime_enable into the
component_bind(It's mtk_smi_larb_bind here), then the runtime callback
is called after component_bind in which the resource for power on is
ready.

> Thinking more about it
> that may be wrong if something from the aggregate device is needed to
> fully power on the component. Is that what is happening here?
>
> > diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
> > index b883dcc0bbfa..288841555067 100644
> > --- a/drivers/memory/mtk-smi.c
> > +++ b/drivers/memory/mtk-smi.c
> > @@ -483,8 +483,9 @@ static int __maybe_unused
> > mtk_smi_larb_resume(struct device *dev)
> > if (ret < 0)
> > return ret;
> >
> > - /* Configure the basic setting for this larb */
> > - larb_gen->config_port(dev);
> > + /* Configure the basic setting for this larb after it binds
> > with iommu */
> > + if (larb->mmu)
> > + larb_gen->config_port(dev);
> >
> > return 0;
> > }
> >


2022-01-14 23:09:02

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v5 25/32] iommu/mtk: Migrate to aggregate driver

Quoting Yong Wu (2022-01-14 01:06:31)
> On Wed, 2022-01-12 at 20:25 -0800, Stephen Boyd wrote:
> > >
> > > [ 2.654526] ------------[ cut here ]------------
> > > [ 2.655558] refcount_t: addition on 0; use-after-free.
> > >
> > > After this patch, the aggregate_driver flow looks ok. But our
> > > driver
> > > still aborts like this:
> > >
> > > [ 2.721316] Unable to handle kernel NULL pointer dereference at
> > > virtual address 0000000000000000
> > > ...
> > > [ 2.731658] pc :
> > > mtk_smi_larb_config_port_gen2_general+0xa4/0x138
> > > [ 2.732434] lr : mtk_smi_larb_resume+0x54/0x98
> > > ...
> > > [ 2.742457] Call trace:
> > > [ 2.742768] mtk_smi_larb_config_port_gen2_general+0xa4/0x138
> > > [ 2.743496] pm_generic_runtime_resume+0x2c/0x48
> > > [ 2.744090] __genpd_runtime_resume+0x30/0xa8
> > > [ 2.744648] genpd_runtime_resume+0x94/0x2c8
> > > [ 2.745191] __rpm_callback+0x44/0x150
> > > [ 2.745669] rpm_callback+0x6c/0x78
> > > [ 2.746114] rpm_resume+0x314/0x558
> > > [ 2.746559] __pm_runtime_resume+0x3c/0x88
> > > [ 2.747080] pm_runtime_get_suppliers+0x7c/0x110
> > > [ 2.747668] __driver_probe_device+0x4c/0xe8
> > > [ 2.748212] driver_probe_device+0x44/0x130
> > > [ 2.748745] __device_attach_driver+0x98/0xd0
> > > [ 2.749300] bus_for_each_drv+0x68/0xd0
> > > [ 2.749787] __device_attach+0xec/0x148
> > > [ 2.750277] device_attach+0x14/0x20
> > > [ 2.750733] bus_rescan_devices_helper+0x50/0x90
> > > [ 2.751319] bus_for_each_dev+0x7c/0xd8
> > > [ 2.751806] bus_rescan_devices+0x20/0x30
> > > [ 2.752315] __component_add+0x7c/0xa0
> > > [ 2.752795] component_add+0x14/0x20
> > > [ 2.753253] mtk_smi_larb_probe+0xe0/0x120
> > >
> > > This is because the device runtime_resume is called before the bind
> > > operation(In our case this detailed function is mtk_smi_larb_bind).
> > > The issue doesn't happen without this patchset. I'm not sure the
> > > right
> > > sequence. If we should fix in mediatek driver, the patch could be:
> >
> > Oh, the runtime PM is moved around with these patches. The aggregate
> > device is runtime PM enabled before the probe is called,
>
> In our case, the component device may probe before the aggregate
> device. thus the component device runtime PM has already been enabled
> when aggregate device probe.

This is always the case. The component device always probes before the
aggregate device because the component device registers with the
component framework. But the component device can decide to enable
runtime PM during driver probe or during component bind.

>
> > and there are
> > supplier links made to each component, so each component is runtime
> > resumed before the aggregate probe function is called.
>
> Yes. This is the current flow.

Got it.

>
> > It means that all
> > the component drivers need to have their resources ready to power on
> > before their component_bind() callback is made.
>
> Sorry, I don't understand here well. In this case, The component
> drivers prepare the resource for power on in the component_bind since
> the resource comes from the aggregate driver. Thus, we expect the
> component_bind run before the runtime resume callback.

What resource comes from the aggregate device that the component device
manages in runtime PM?

>
> Another solution is moving the component's pm_runtime_enable into the
> component_bind(It's mtk_smi_larb_bind here), then the runtime callback
> is called after component_bind in which the resource for power on is
> ready.

This sounds more correct to me. I'm not an expert on runtime PM though
as I always have to read the code to remember how it works. if the
device isn't ready for runtime PM until the component bind function is
called then runtime PM shouldn't be enabled on the component device.

2022-01-15 19:15:05

by Yong Wu (吴勇)

[permalink] [raw]
Subject: Re: [PATCH v5 25/32] iommu/mtk: Migrate to aggregate driver

On Fri, 2022-01-14 at 15:30 -0600, Stephen Boyd wrote:
> Quoting Yong Wu (2022-01-14 01:06:31)
> > On Wed, 2022-01-12 at 20:25 -0800, Stephen Boyd wrote:
> > > >
> > > > [ 2.654526] ------------[ cut here ]------------
> > > > [ 2.655558] refcount_t: addition on 0; use-after-free.
> > > >
> > > > After this patch, the aggregate_driver flow looks ok. But our
> > > > driver
> > > > still aborts like this:
> > > >
> > > > [ 2.721316] Unable to handle kernel NULL pointer dereference
> > > > at
> > > > virtual address 0000000000000000
> > > > ...
> > > > [ 2.731658] pc :
> > > > mtk_smi_larb_config_port_gen2_general+0xa4/0x138
> > > > [ 2.732434] lr : mtk_smi_larb_resume+0x54/0x98
> > > > ...
> > > > [ 2.742457] Call trace:
> > > > [ 2.742768] mtk_smi_larb_config_port_gen2_general+0xa4/0x13
> > > > 8
> > > > [ 2.743496] pm_generic_runtime_resume+0x2c/0x48
> > > > [ 2.744090] __genpd_runtime_resume+0x30/0xa8
> > > > [ 2.744648] genpd_runtime_resume+0x94/0x2c8
> > > > [ 2.745191] __rpm_callback+0x44/0x150
> > > > [ 2.745669] rpm_callback+0x6c/0x78
> > > > [ 2.746114] rpm_resume+0x314/0x558
> > > > [ 2.746559] __pm_runtime_resume+0x3c/0x88
> > > > [ 2.747080] pm_runtime_get_suppliers+0x7c/0x110
> > > > [ 2.747668] __driver_probe_device+0x4c/0xe8
> > > > [ 2.748212] driver_probe_device+0x44/0x130
> > > > [ 2.748745] __device_attach_driver+0x98/0xd0
> > > > [ 2.749300] bus_for_each_drv+0x68/0xd0
> > > > [ 2.749787] __device_attach+0xec/0x148
> > > > [ 2.750277] device_attach+0x14/0x20
> > > > [ 2.750733] bus_rescan_devices_helper+0x50/0x90
> > > > [ 2.751319] bus_for_each_dev+0x7c/0xd8
> > > > [ 2.751806] bus_rescan_devices+0x20/0x30
> > > > [ 2.752315] __component_add+0x7c/0xa0
> > > > [ 2.752795] component_add+0x14/0x20
> > > > [ 2.753253] mtk_smi_larb_probe+0xe0/0x120
> > > >
> > > > This is because the device runtime_resume is called before the
> > > > bind
> > > > operation(In our case this detailed function is
> > > > mtk_smi_larb_bind).
> > > > The issue doesn't happen without this patchset. I'm not sure
> > > > the
> > > > right
> > > > sequence. If we should fix in mediatek driver, the patch could
> > > > be:
> > >
> > > Oh, the runtime PM is moved around with these patches. The
> > > aggregate
> > > device is runtime PM enabled before the probe is called,
> >
> > In our case, the component device may probe before the aggregate
> > device. thus the component device runtime PM has already been
> > enabled
> > when aggregate device probe.
>
> This is always the case. The component device always probes before
> the
> aggregate device because the component device registers with the
> component framework. But the component device can decide to enable
> runtime PM during driver probe or during component bind.
>
> >
> > > and there are
> > > supplier links made to each component, so each component is
> > > runtime
> > > resumed before the aggregate probe function is called.
> >
> > Yes. This is the current flow.
>
> Got it.
>
> >
> > > It means that all
> > > the component drivers need to have their resources ready to power
> > > on
> > > before their component_bind() callback is made.
> >
> > Sorry, I don't understand here well. In this case, The component
> > drivers prepare the resource for power on in the component_bind
> > since
> > the resource comes from the aggregate driver. Thus, we expect the
> > component_bind run before the runtime resume callback.
>
> What resource comes from the aggregate device that the component
> device manages in runtime PM?

Here the aggregate device is the iommu device. It knows who enabled the
iommu from the dtsi, then transfers this information to the
component(smi_larb) devices. smi_larb will configure the registers to
enable iommu for these masters in the runtime resume.

> >
> > Another solution is moving the component's pm_runtime_enable into
> > the
> > component_bind(It's mtk_smi_larb_bind here), then the runtime
> > callback
> > is called after component_bind in which the resource for power on
> > is
> > ready.
>
> This sounds more correct to me. I'm not an expert on runtime PM
> though
> as I always have to read the code to remember how it works. if the
> device isn't ready for runtime PM until the component bind function
> is
> called then runtime PM shouldn't be enabled on the component device.

Anyway, We should update the SMI driver for this case. I prepare a
patch into this patchset or I send it independently? which way is
better?

The patch could be:

diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
index b883dcc0bbfa..88854c2270a1 100644
--- a/drivers/memory/mtk-smi.c
+++ b/drivers/memory/mtk-smi.c
@@ -175,6 +175,8 @@ mtk_smi_larb_bind(struct device *dev, struct device
*master, void *data)
larb->larbid = i;
larb->mmu = &larb_mmu[i].mmu;
larb->bank = larb_mmu[i].bank;
+
+ pm_runtime_enable(dev);
return 0;
}
}
@@ -450,7 +452,6 @@ static int mtk_smi_larb_probe(struct
platform_device *pdev)
if (ret < 0)
return ret;

- pm_runtime_enable(dev);
platform_set_drvdata(pdev, larb);
ret = component_add(dev, &mtk_smi_larb_component_ops);
if (ret)

Thanks.

2022-01-15 19:42:29

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v5 25/32] iommu/mtk: Migrate to aggregate driver

Quoting Yong Wu (2022-01-14 23:39:52)
> On Fri, 2022-01-14 at 15:30 -0600, Stephen Boyd wrote:
> >
> > This sounds more correct to me. I'm not an expert on runtime PM
> > though
> > as I always have to read the code to remember how it works. if the
> > device isn't ready for runtime PM until the component bind function
> > is
> > called then runtime PM shouldn't be enabled on the component device.
>
> Anyway, We should update the SMI driver for this case. I prepare a
> patch into this patchset or I send it independently? which way is
> better?

I can roll it into this patch. It needs to be combined otherwise it
breaks the bisectability of the series.