linux-kernel - Re: [PATCH v10 1/7] i2c: xiic: switch to devres managed APIs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <9BCFA228-EC6D-4084-A63E-060917C948E0@nexthop.ai>
Date: Wed, 4 Feb 2026 09:58:14 -0800
From: Abdurrahman Hussain <abdurrahman@...thop.ai>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Abdurrahman Hussain via B4 Relay <devnull+abdurrahman.nexthop.ai@...nel.org>,
 Michal Simek <michal.simek@....com>,
 Andi Shyti <andi.shyti@...nel.org>,
 Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Andy Shevchenko <andriy.shevchenko@...el.com>,
 linux-arm-kernel@...ts.infradead.org,
 linux-i2c@...r.kernel.org,
 linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org
Subject: Re: [PATCH v10 1/7] i2c: xiic: switch to devres managed APIs



> On Feb 4, 2026, at 2:00 AM, Jonathan Cameron <Jonathan.Cameron@...wei.com> wrote:
> 
> On Wed, 04 Feb 2026 07:01:58 +0000
> Abdurrahman Hussain via B4 Relay <devnull+abdurrahman.nexthop.ai@...nel.org> wrote:
> 
>> From: Abdurrahman Hussain <abdurrahman@...thop.ai>
>> 
>> Simplify the error code paths by switching to devres managed helper
>> functions.
>> 
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...el.com>
>> Signed-off-by: Abdurrahman Hussain <abdurrahman@...thop.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.