diff mbox series

[v6,1/6] i2c: core: Setup i2c_adapter runtime-pm before calling device_add()

Message ID 20240704125643.22946-2-hdegoede@redhat.com (mailing list archive)
State Superseded, archived
Headers show
Series i2c-i801 / dell-lis3lv02d: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-lis3lv02d | expand

Commit Message

Hans de Goede July 4, 2024, 12:56 p.m. UTC 
Platform glue code, which is not build into the kernel and thus cannot
use i2c_register_board_info() may want to use bus_register_notifier()
to listen for i2c-adapters to show up on which the platform code needs
to manually instantiate platform specific i2c_clients.

This results in calling i2c_new_client_device() from the bus notifier
which happens near the device_add() call.

If the i2c-core has not yet setup runtime-pm (specifically the
no-callbacks and ignore-children flags) for the device embedded
inside struct i2c_adapter and the driver for the i2c_client
calls pm_runtime_set_active() this will trigger the following
error inside __pm_runtime_set_status():

"runtime PM trying to activate child device %s but parent (%s) is not active\n"

and the i2c_client's runtime-status will not be updated.

Split the device_register() call for the adapter into device_initialize()
and device_add() and move the pm-runtime init calls inbetween these 2 calls
so that the runtime-status can be correctly set when a driver binds from
the bus-notifier.

Note the moved pm-runtime init calls just override the initial value of
some flags in struct device set by device_initialize() and calling these
before device_add() is safe.

Reviewed-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
- Add a comment explaining why runtime-pm needs to be setup before
  the device_add()

Changes in v3:
- This is a new patch in v3 of this patch-set
---
 drivers/i2c/i2c-core-base.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Comments

Andi Shyti July 4, 2024, 9:24 p.m. UTC | #1 
Hi Hans,

...

> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1521,7 +1521,18 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>  	dev_set_name(&adap->dev, "i2c-%d", adap->nr);
>  	adap->dev.bus = &i2c_bus_type;
>  	adap->dev.type = &i2c_adapter_type;
> -	res = device_register(&adap->dev);
> +	device_initialize(&adap->dev);
> +
> +	/*
> +	 * This adapter can be used as a parent immediately after device_add(),
> +	 * setup runtime-pm (especially ignore-children) before hand.
> +	 */
> +	device_enable_async_suspend(&adap->dev);
> +	pm_runtime_no_callbacks(&adap->dev);
> +	pm_suspend_ignore_children(&adap->dev, true);
> +	pm_runtime_enable(&adap->dev);
> +
> +	res = device_add(&adap->dev);

I see a pattern here around in the kernel:

	device_initialize(...)
	...
	device_add(...)

which makes the device_register() helper not that useful. Perhaps
for the future we could add a device_register_fn() where we pass
the reference of a function that does whatever lies in between
the initialize and the add... food for thoughts.

As of now, looks good to me:

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Thanks,
Andi

>  	if (res) {
>  		pr_err("adapter '%s': can't register device (%d)\n", adap->name, res);
>  		goto out_list;
> @@ -1533,11 +1544,6 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>  	if (res)
>  		goto out_reg;
>  
> -	device_enable_async_suspend(&adap->dev);
> -	pm_runtime_no_callbacks(&adap->dev);
> -	pm_suspend_ignore_children(&adap->dev, true);
> -	pm_runtime_enable(&adap->dev);
> -
>  	res = i2c_init_recovery(adap);
>  	if (res == -EPROBE_DEFER)
>  		goto out_reg;
> -- 
> 2.45.1
>
diff mbox series

Patch

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index db0d1ac82910..c4517d97a49f 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1521,7 +1521,18 @@  static int i2c_register_adapter(struct i2c_adapter *adap)
 	dev_set_name(&adap->dev, "i2c-%d", adap->nr);
 	adap->dev.bus = &i2c_bus_type;
 	adap->dev.type = &i2c_adapter_type;
-	res = device_register(&adap->dev);
+	device_initialize(&adap->dev);
+
+	/*
+	 * This adapter can be used as a parent immediately after device_add(),
+	 * setup runtime-pm (especially ignore-children) before hand.
+	 */
+	device_enable_async_suspend(&adap->dev);
+	pm_runtime_no_callbacks(&adap->dev);
+	pm_suspend_ignore_children(&adap->dev, true);
+	pm_runtime_enable(&adap->dev);
+
+	res = device_add(&adap->dev);
 	if (res) {
 		pr_err("adapter '%s': can't register device (%d)\n", adap->name, res);
 		goto out_list;
@@ -1533,11 +1544,6 @@  static int i2c_register_adapter(struct i2c_adapter *adap)
 	if (res)
 		goto out_reg;
 
-	device_enable_async_suspend(&adap->dev);
-	pm_runtime_no_callbacks(&adap->dev);
-	pm_suspend_ignore_children(&adap->dev, true);
-	pm_runtime_enable(&adap->dev);
-
 	res = i2c_init_recovery(adap);
 	if (res == -EPROBE_DEFER)
 		goto out_reg;