From: Abdurrahman Hussain <abdurrahman@nexthop.ai>
Simplify the error code paths by switching to devres managed helper
functions.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/i2c/busses/i2c-xiic.c | 29 ++++++++++++-----------------
1 file changed, 12 insertions(+), 17 deletions(-)
diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 28015d77599d..16ff83fe280b 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -1423,6 +1423,7 @@ MODULE_DEVICE_TABLE(of, xiic_of_match);
static int xiic_i2c_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
struct xiic_i2c *i2c;
struct xiic_i2c_platform_data *pdata;
const struct of_device_id *match;
@@ -1461,7 +1462,10 @@ static int xiic_i2c_probe(struct platform_device *pdev)
snprintf(i2c->adap.name, sizeof(i2c->adap.name),
DRIVER_NAME " %s", pdev->name);
- mutex_init(&i2c->lock);
+ ret = devm_mutex_init(dev, &i2c->lock);
+ if (ret)
+ return ret;
+
spin_lock_init(&i2c->atomic_lock);
i2c->clk = devm_clk_get_enabled(&pdev->dev, NULL);
@@ -1472,8 +1476,9 @@ static int xiic_i2c_probe(struct platform_device *pdev)
i2c->dev = &pdev->dev;
pm_runtime_set_autosuspend_delay(i2c->dev, XIIC_PM_TIMEOUT);
pm_runtime_use_autosuspend(i2c->dev);
- pm_runtime_set_active(i2c->dev);
- pm_runtime_enable(i2c->dev);
+ ret = devm_pm_runtime_set_active_enabled(dev);
+ if (ret)
+ return ret;
/* SCL frequency configuration */
i2c->input_clk = clk_get_rate(i2c->clk);
@@ -1489,7 +1494,7 @@ static int xiic_i2c_probe(struct platform_device *pdev)
if (ret < 0) {
dev_err_probe(&pdev->dev, ret, "Cannot claim IRQ\n");
- goto err_pm_disable;
+ return ret;
}
i2c->singlemaster =
@@ -1508,16 +1513,14 @@ static int xiic_i2c_probe(struct platform_device *pdev)
i2c->endianness = BIG;
ret = xiic_reinit(i2c);
- if (ret < 0) {
- dev_err_probe(&pdev->dev, ret, "Cannot xiic_reinit\n");
- goto err_pm_disable;
- }
+ if (ret)
+ return dev_err_probe(dev, ret, "Cannot xiic_reinit\n");
/* add i2c adapter to i2c tree */
ret = i2c_add_adapter(&i2c->adap);
if (ret) {
xiic_deinit(i2c);
- goto err_pm_disable;
+ return ret;
}
if (pdata) {
@@ -1529,12 +1532,6 @@ static int xiic_i2c_probe(struct platform_device *pdev)
dev_dbg(&pdev->dev, "mmio %08lx irq %d scl clock frequency %d\n",
(unsigned long)res->start, irq, i2c->i2c_clk);
- return 0;
-
-err_pm_disable:
- pm_runtime_disable(&pdev->dev);
- pm_runtime_set_suspended(&pdev->dev);
-
return ret;
}
@@ -1555,8 +1552,6 @@ static void xiic_i2c_remove(struct platform_device *pdev)
xiic_deinit(i2c);
pm_runtime_put_sync(i2c->dev);
- pm_runtime_disable(&pdev->dev);
- pm_runtime_set_suspended(&pdev->dev);
pm_runtime_dont_use_autosuspend(&pdev->dev);
}
--
2.52.0On Wed, 04 Feb 2026 07:01:58 +0000
Abdurrahman Hussain via B4 Relay <devnull+abdurrahman.nexthop.ai@kernel.org> wrote:
> From: Abdurrahman Hussain <abdurrahman@nexthop.ai>
>
> Simplify the error code paths by switching to devres managed helper
> functions.
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
A quick drive by review whilst I have a coffee...
1st one is a taste thing. Second one is kind of a bug - be it one that
I think is probably harmless.
> ---
> drivers/i2c/busses/i2c-xiic.c | 29 ++++++++++++-----------------
> 1 file changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> index 28015d77599d..16ff83fe280b 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -1423,6 +1423,7 @@ MODULE_DEVICE_TABLE(of, xiic_of_match);
>
> static int xiic_i2c_probe(struct platform_device *pdev)
> {
> + struct device *dev = &pdev->dev;
> struct xiic_i2c *i2c;
> struct xiic_i2c_platform_data *pdata;
> const struct of_device_id *match;
> @@ -1461,7 +1462,10 @@ static int xiic_i2c_probe(struct platform_device *pdev)
> snprintf(i2c->adap.name, sizeof(i2c->adap.name),
> DRIVER_NAME " %s", pdev->name);
>
> - mutex_init(&i2c->lock);
> + ret = devm_mutex_init(dev, &i2c->lock);
> + if (ret)
> + return ret;
> +
> spin_lock_init(&i2c->atomic_lock);
>
> i2c->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> @@ -1472,8 +1476,9 @@ static int xiic_i2c_probe(struct platform_device *pdev)
> i2c->dev = &pdev->dev;
> pm_runtime_set_autosuspend_delay(i2c->dev, XIIC_PM_TIMEOUT);
> pm_runtime_use_autosuspend(i2c->dev);
> - pm_runtime_set_active(i2c->dev);
> - pm_runtime_enable(i2c->dev);
> + ret = devm_pm_runtime_set_active_enabled(dev);
> + if (ret)
> + return ret;
>
> /* SCL frequency configuration */
> i2c->input_clk = clk_get_rate(i2c->clk);
> @@ -1489,7 +1494,7 @@ static int xiic_i2c_probe(struct platform_device *pdev)
>
> if (ret < 0) {
> dev_err_probe(&pdev->dev, ret, "Cannot claim IRQ\n");
> - goto err_pm_disable;
> + return ret;
> }
>
> i2c->singlemaster =
> @@ -1508,16 +1513,14 @@ static int xiic_i2c_probe(struct platform_device *pdev)
> i2c->endianness = BIG;
>
> ret = xiic_reinit(i2c);
> - if (ret < 0) {
> - dev_err_probe(&pdev->dev, ret, "Cannot xiic_reinit\n");
> - goto err_pm_disable;
> - }
> + if (ret)
> + return dev_err_probe(dev, ret, "Cannot xiic_reinit\n");
>
> /* add i2c adapter to i2c tree */
> ret = i2c_add_adapter(&i2c->adap);
> if (ret) {
> xiic_deinit(i2c);
> - goto err_pm_disable;
> + return ret;
> }
>
> if (pdata) {
> @@ -1529,12 +1532,6 @@ static int xiic_i2c_probe(struct platform_device *pdev)
> dev_dbg(&pdev->dev, "mmio %08lx irq %d scl clock frequency %d\n",
> (unsigned long)res->start, irq, i2c->i2c_clk);
>
> - return 0;
> -
> -err_pm_disable:
> - pm_runtime_disable(&pdev->dev);
> - pm_runtime_set_suspended(&pdev->dev);
> -
> return ret;
Trivial but if you are respinning...
If you get here we know ret must be 0, so make that explicit to the reader as it
was before with
return 0;
Otherwise they need to look up a few lines to realize that is true.
> }
>
> @@ -1555,8 +1552,6 @@ static void xiic_i2c_remove(struct platform_device *pdev)
> xiic_deinit(i2c);
>
> pm_runtime_put_sync(i2c->dev);
> - pm_runtime_disable(&pdev->dev);
> - pm_runtime_set_suspended(&pdev->dev);
> pm_runtime_dont_use_autosuspend(&pdev->dev);
Take a look at docs for the devm_runtime_enable() that is called
by the cleanup for devm_pm_runtime_set_active_enabled()
Short story, it will call pm_runtime_dont_use_autosuspend() for you
> }
>
>
> On Feb 4, 2026, at 2:00 AM, Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
>
> On Wed, 04 Feb 2026 07:01:58 +0000
> Abdurrahman Hussain via B4 Relay <devnull+abdurrahman.nexthop.ai@kernel.org> wrote:
>
>> From: Abdurrahman Hussain <abdurrahman@nexthop.ai>
>>
>> Simplify the error code paths by switching to devres managed helper
>> functions.
>>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
>> Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
>
> A quick drive by review whilst I have a coffee...
>
> 1st one is a taste thing. Second one is kind of a bug - be it one that
> I think is probably harmless.
>
>> ---
>> drivers/i2c/busses/i2c-xiic.c | 29 ++++++++++++-----------------
>> 1 file changed, 12 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
>> index 28015d77599d..16ff83fe280b 100644
>> --- a/drivers/i2c/busses/i2c-xiic.c
>> +++ b/drivers/i2c/busses/i2c-xiic.c
>> @@ -1423,6 +1423,7 @@ MODULE_DEVICE_TABLE(of, xiic_of_match);
>>
>> static int xiic_i2c_probe(struct platform_device *pdev)
>> {
>> + struct device *dev = &pdev->dev;
>> struct xiic_i2c *i2c;
>> struct xiic_i2c_platform_data *pdata;
>> const struct of_device_id *match;
>> @@ -1461,7 +1462,10 @@ static int xiic_i2c_probe(struct platform_device *pdev)
>> snprintf(i2c->adap.name, sizeof(i2c->adap.name),
>> DRIVER_NAME " %s", pdev->name);
>>
>> - mutex_init(&i2c->lock);
>> + ret = devm_mutex_init(dev, &i2c->lock);
>> + if (ret)
>> + return ret;
>> +
>> spin_lock_init(&i2c->atomic_lock);
>>
>> i2c->clk = devm_clk_get_enabled(&pdev->dev, NULL);
>> @@ -1472,8 +1476,9 @@ static int xiic_i2c_probe(struct platform_device *pdev)
>> i2c->dev = &pdev->dev;
>> pm_runtime_set_autosuspend_delay(i2c->dev, XIIC_PM_TIMEOUT);
>> pm_runtime_use_autosuspend(i2c->dev);
>> - pm_runtime_set_active(i2c->dev);
>> - pm_runtime_enable(i2c->dev);
>> + ret = devm_pm_runtime_set_active_enabled(dev);
>> + if (ret)
>> + return ret;
>>
>> /* SCL frequency configuration */
>> i2c->input_clk = clk_get_rate(i2c->clk);
>> @@ -1489,7 +1494,7 @@ static int xiic_i2c_probe(struct platform_device *pdev)
>>
>> if (ret < 0) {
>> dev_err_probe(&pdev->dev, ret, "Cannot claim IRQ\n");
>> - goto err_pm_disable;
>> + return ret;
>> }
>>
>> i2c->singlemaster =
>> @@ -1508,16 +1513,14 @@ static int xiic_i2c_probe(struct platform_device *pdev)
>> i2c->endianness = BIG;
>>
>> ret = xiic_reinit(i2c);
>> - if (ret < 0) {
>> - dev_err_probe(&pdev->dev, ret, "Cannot xiic_reinit\n");
>> - goto err_pm_disable;
>> - }
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Cannot xiic_reinit\n");
>>
>> /* add i2c adapter to i2c tree */
>> ret = i2c_add_adapter(&i2c->adap);
>> if (ret) {
>> xiic_deinit(i2c);
>> - goto err_pm_disable;
>> + return ret;
>> }
>>
>> if (pdata) {
>> @@ -1529,12 +1532,6 @@ static int xiic_i2c_probe(struct platform_device *pdev)
>> dev_dbg(&pdev->dev, "mmio %08lx irq %d scl clock frequency %d\n",
>> (unsigned long)res->start, irq, i2c->i2c_clk);
>>
>> - return 0;
>> -
>> -err_pm_disable:
>> - pm_runtime_disable(&pdev->dev);
>> - pm_runtime_set_suspended(&pdev->dev);
>> -
>> return ret;
> Trivial but if you are respinning...
> If you get here we know ret must be 0, so make that explicit to the reader as it
> was before with
> return 0;
>
> Otherwise they need to look up a few lines to realize that is true.
Thanks! Will fix in v11.
>
>> }
>>
>> @@ -1555,8 +1552,6 @@ static void xiic_i2c_remove(struct platform_device *pdev)
>> xiic_deinit(i2c);
>>
>> pm_runtime_put_sync(i2c->dev);
>> - pm_runtime_disable(&pdev->dev);
>> - pm_runtime_set_suspended(&pdev->dev);
>> pm_runtime_dont_use_autosuspend(&pdev->dev);
>
> Take a look at docs for the devm_runtime_enable() that is called
> by the cleanup for devm_pm_runtime_set_active_enabled()
>
> Short story, it will call pm_runtime_dont_use_autosuspend() for you
Thank you for the feedback Jonathan! Will fix in v11.
On Wed, Feb 04, 2026 at 10:00:34AM +0000, Jonathan Cameron wrote: > On Wed, 04 Feb 2026 07:01:58 +0000 > Abdurrahman Hussain via B4 Relay <devnull+abdurrahman.nexthop.ai@kernel.org> wrote: ... > > - return 0; > > - > > -err_pm_disable: > > - pm_runtime_disable(&pdev->dev); > > - pm_runtime_set_suspended(&pdev->dev); > > - > > return ret; > Trivial but if you are respinning... > If you get here we know ret must be 0, so make that explicit to the reader as it > was before with > return 0; > Otherwise they need to look up a few lines to realize that is true. Right and this is already the line above, just wrong one was removed. ... > > - pm_runtime_disable(&pdev->dev); > > - pm_runtime_set_suspended(&pdev->dev); > > pm_runtime_dont_use_autosuspend(&pdev->dev); > > Take a look at docs for the devm_runtime_enable() that is called > by the cleanup for devm_pm_runtime_set_active_enabled() > > Short story, it will call pm_runtime_dont_use_autosuspend() for you Good catch! It's not obvious from the devm_pm_*() naming... :-( ... This definitely means v11 should be send at some point, but, Abdurrahman, do not hurry with it, this series missed the cycle anyway, we have a few *weeks* to polish this. That said, send it after v6.20-rc1 (or v7.0-rc1) is out. -- With Best Regards, Andy Shevchenko
> On Feb 4, 2026, at 2:11 AM, Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > > On Wed, Feb 04, 2026 at 10:00:34AM +0000, Jonathan Cameron wrote: >> On Wed, 04 Feb 2026 07:01:58 +0000 >> Abdurrahman Hussain via B4 Relay <devnull+abdurrahman.nexthop.ai@kernel.org> wrote: > > ... > >>> - return 0; >>> - >>> -err_pm_disable: >>> - pm_runtime_disable(&pdev->dev); >>> - pm_runtime_set_suspended(&pdev->dev); >>> - >>> return ret; >> Trivial but if you are respinning... >> If you get here we know ret must be 0, so make that explicit to the reader as it >> was before with >> return 0; > >> Otherwise they need to look up a few lines to realize that is true. > > Right and this is already the line above, just wrong one was removed. > > ... > >>> - pm_runtime_disable(&pdev->dev); >>> - pm_runtime_set_suspended(&pdev->dev); >>> pm_runtime_dont_use_autosuspend(&pdev->dev); >> >> Take a look at docs for the devm_runtime_enable() that is called >> by the cleanup for devm_pm_runtime_set_active_enabled() >> >> Short story, it will call pm_runtime_dont_use_autosuspend() for you > > Good catch! It's not obvious from the devm_pm_*() naming... :-( > > ... > > This definitely means v11 should be send at some point, but, > Abdurrahman, do not hurry with it, this series missed the cycle > anyway, we have a few *weeks* to polish this. > > That said, send it after v6.20-rc1 (or v7.0-rc1) is out. > Sounds good. Thank you for your patience!
© 2016 - 2026 Red Hat, Inc.