Message ID | 20221109-i2c-waive-v5-1-2839667f8f6a@chromium.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp240997wru; Thu, 10 Nov 2022 08:23:12 -0800 (PST) X-Google-Smtp-Source: AMsMyM6e06YyAnWuKlZ3KFSMnQCUf+CPssVs5oSpw/ZiFbF6Fq9mZhtZIJUfxqd0RgYWa+Gnn6+G X-Received: by 2002:a17:903:2291:b0:187:21f6:fdce with SMTP id b17-20020a170903229100b0018721f6fdcemr57391823plh.51.1668097392395; Thu, 10 Nov 2022 08:23:12 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1668097392; cv=none; d=google.com; s=arc-20160816; b=qZfRA0Q4975rJ+g1L4QVBPVeUilZhRhrp17PYj0DPOjphyqj/Hc0tdEa9Sf4EMLecX zkULFDZar33/dsOF2+OARuv8zkkkaEDk4DU1e9CUaTbAMcxJWShZXMhjqeHcfvVF+RsG k4v+qrFKtWVmLBuDIeVWkOn15JDeeS18+z+0D0AsreXXq9QCSMfU0XxPaa6DfIU88mW3 I5efA9DAu1v6F5IU6tX2Z1PJgQZAMBLyAU/Am+QSQoMoxtkBjWXiyHIMI4tSs55IQFhB cHy5HJEWrorRw7Qm82jSw43HzJhKHzWat6RDgKTg8nFD3UgLiB8iiIbOFUNLx0zBoxOR r+qw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:in-reply-to:references:message-id :content-transfer-encoding:mime-version:subject:date:from :dkim-signature; bh=GdcwK+eSzO30vZBClseX8YSlW2qnvr/aVA4Sk8Y6wrQ=; b=jrqlIINDXnJTfbtC5wk9dNTBTdGOjl5j3q0TZVS2GUNHYwu8kfOWlFnbWf7Rtin4ML lVbvO6NECc+wH4kVXGncVwd4/hNdAgt6mb2SwtXZ8RrRTBpEnWfS9qwc/lRHCrimnXR7 ZLjtTykkxlbKDGSfcquY8hGMUnLqKf1Z1McWeteThyh9z9q520xkku/ndo9ENRQrImw8 ii46FXFCcvZyNaENCzIdCUDPkEEU3efqjB6x9aokZBDzlVW9k8XuFGqv2DNh15N4hoOy GhPxKy1X2GHlHgZE1GzJPiBWlRmXIIBkeuT44qbZk6ttL4RJO/jHoNaUHZwhgvQ/15gM lgFg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=lmQabTN+; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id nv8-20020a17090b1b4800b0020038eb8b5asi54320pjb.21.2022.11.10.08.22.57; Thu, 10 Nov 2022 08:23:12 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=lmQabTN+; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231249AbiKJQVA (ORCPT <rfc822;winker.wchi@gmail.com> + 99 others); Thu, 10 Nov 2022 11:21:00 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50292 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231147AbiKJQUy (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 10 Nov 2022 11:20:54 -0500 Received: from mail-ed1-x52b.google.com (mail-ed1-x52b.google.com [IPv6:2a00:1450:4864:20::52b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 421661A076 for <linux-kernel@vger.kernel.org>; Thu, 10 Nov 2022 08:20:53 -0800 (PST) Received: by mail-ed1-x52b.google.com with SMTP id a13so3915684edj.0 for <linux-kernel@vger.kernel.org>; Thu, 10 Nov 2022 08:20:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:from:to:cc:subject:date:message-id :reply-to; bh=GdcwK+eSzO30vZBClseX8YSlW2qnvr/aVA4Sk8Y6wrQ=; b=lmQabTN+8QKCKz3yUXpojvxqRdJ0oba9J5peWD98OhgJBpoScBRYh42GKxHS+y8UB2 yIKE0QAXtaW0k8pTtrX/nLC1Lri7rsFdO8d4/zQ8QbEy8nOmW0WgijsPd5zkNLLxuV+t KkVkkGeSN3s2/O7zCdBkax+rCf9Ixjq3ZogN0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=GdcwK+eSzO30vZBClseX8YSlW2qnvr/aVA4Sk8Y6wrQ=; b=2mrGnqXBexZpkm7+UuWWeljCuvZRSu3IuwndzpJC1no6Mn/V+K/M0J14A6wzs1MdsN /WfD3h52uZ/0M+Ddy5Ai0pJRhrJ2unPpLuGVNiY5cjnKAdSKYv/Jm+NqlbV+sTM5lyXV LL1inTkdHqlz8V919tS4ZZNabWtalpflusj/3knK7zp2La2t02AZOX7q4SgmgJ1zlBM+ sPmf+WXfNQeaDefiaRtL6Vmvf68tS9Envt2jEUuxmuT0HRUfgSalKCjknCyf+IU1zWuR o9FJDJqIloIowu0ZgJoCLSokVMGfJG3dlCQmuAWBBp+JnYrNNBjXa3MgGtyLPQ389gcu Qg7w== X-Gm-Message-State: ACrzQf2+YpllEjxIVfmbdYNvtRqBZHEUUWz5NW/xQpGjiRmjy4OON++2 QIgs/NNF/WrdcCRw91WaW6rYrQ== X-Received: by 2002:a50:ee0a:0:b0:463:4055:9db4 with SMTP id g10-20020a50ee0a000000b0046340559db4mr57833736eds.421.1668097251847; Thu, 10 Nov 2022 08:20:51 -0800 (PST) Received: from alco.roam.corp.google.com (80.71.134.83.ipv4.parknet.dk. [80.71.134.83]) by smtp.gmail.com with ESMTPSA id e8-20020a170906314800b0077b2b0563f4sm7510193eje.173.2022.11.10.08.20.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 10 Nov 2022 08:20:51 -0800 (PST) From: Ricardo Ribalda <ribalda@chromium.org> Date: Thu, 10 Nov 2022 17:20:39 +0100 Subject: [PATCH v5 1/1] i2c: Restore initial power state when we are done. MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Message-Id: <20221109-i2c-waive-v5-1-2839667f8f6a@chromium.org> References: <20221109-i2c-waive-v5-0-2839667f8f6a@chromium.org> In-Reply-To: <20221109-i2c-waive-v5-0-2839667f8f6a@chromium.org> To: Wolfram Sang <wsa@kernel.org>, Tomasz Figa <tfiga@chromium.org>, "Rafael J. Wysocki" <rafael.j.wysocki@intel.com> Cc: Hidenori Kobayashi <hidenorik@google.com>, stable@vger.kernel.org, linux-i2c@vger.kernel.org, Ricardo Ribalda <ribalda@chromium.org>, Sergey Senozhatsky <senozhatsky@chromium.org>, Sakari Ailus <sakari.ailus@linux.intel.com>, linux-kernel@vger.kernel.org X-Mailer: b4 0.11.0-dev-d93f8 X-Developer-Signature: v=1; a=openpgp-sha256; l=3480; i=ribalda@chromium.org; h=from:subject:message-id; bh=9UR0KCXvwEGgujXcvLJmsWuhlTeEco5WrFDyZoXy7AI=; b=owEBbQKS/ZANAwAKAdE30T7POsSIAcsmYgBjbSTgboLbO7ZKUNyOCb+DSb6zrvX6p3V3qvjb0Dep c13lUQqJAjMEAAEKAB0WIQREDzjr+/4oCDLSsx7RN9E+zzrEiAUCY20k4AAKCRDRN9E+zzrEiKPmD/ 9XgjLs771QO8mKlJcCcef36CvrMZYad411S3jgS5KkVZ+fCAsqbqI3ePJkT3YoCFYx8YbDys/6r4ZZ WJoSp4E56UsqFHMnoZuT3WSklxFlHi8wKP83gQbtl8+xjEc1FA31q6ka4Jd2NXFFGVki4CGGohBzrq 3qzt1gww3cD+4OOO9j8m44K6g704YfvSHWi5DDmPLBNe4bqSoteOMebCWW5HHMLLaOGRJwcGBvTTGM ShdJ9sRFlp8bA+cROI4sp5Dk38ccJsB0H9zUlKLWNZ4YuayFE7TS2zE6ZjZGsK69MN32gx57MRUyBp 2tD9Jk6Q7CbJ2ZwsgfXWHm18mOmHzJg2mAHXBSE0FNtGU/aPobxfIf7flwRdXpeMz5MqwhfBbvgWv2 bwmQPrUhgQtXpfYZ9+Od9HEv1ujxG/T99EE4+P4zhdMntWI/oBCT/O1OFhtXh+ENmBOjeyt5q2Bt5P JfmC9AXP4slsUZIboJ+aj7oW+v1thlCzibQottue2bAEHe+cwQ1rPk9mLZIx2xTHwdz3ktwFDKnfOD o57+Al4ZKMRAQdHmpTX/Fj5fxTnQpDKdYi6lAto1xdlVYj1HqvV7TeBPX/s/iLrif2ULZMZ7vOyF3P UFxLj6HBEs8egC0lL2oHkmG6abnaAtCcgmJGexUA089A514jxjUc9xVJSSkg== X-Developer-Key: i=ribalda@chromium.org; a=openpgp; fpr=9EC3BB66E2FC129A6F90B39556A0D81F9F782DA9 X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1749126891182952244?= X-GMAIL-MSGID: =?utf-8?q?1749126891182952244?= |
Series |
i2c: Restore power status of device if probe fail or device is removed
|
|
Commit Message
Ricardo Ribalda
Nov. 10, 2022, 4:20 p.m. UTC
A driver that supports I2C_DRV_ACPI_WAIVE_D0_PROBE is not expected to power off a device that it has not powered on previously. For devices operating in "full_power" mode, the first call to `i2c_acpi_waive_d0_probe` will return 0, which means that the device will be turned on with `dev_pm_domain_attach`. If probe fails or the device is removed the second call to `i2c_acpi_waive_d0_probe` will return 1, which means that the device will not be turned off. This is, it will be left in a different power state. Lets fix it. Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> Cc: stable@vger.kernel.org Fixes: b18c1ad685d9 ("i2c: Allow an ACPI driver to manage the device's power state during probe") Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
Comments
Hi Ricardo, On Thu, Nov 10, 2022 at 05:20:39PM +0100, Ricardo Ribalda wrote: > A driver that supports I2C_DRV_ACPI_WAIVE_D0_PROBE is not expected to > power off a device that it has not powered on previously. > > For devices operating in "full_power" mode, the first call to > `i2c_acpi_waive_d0_probe` will return 0, which means that the device > will be turned on with `dev_pm_domain_attach`. > > If probe fails or the device is removed the second call to > `i2c_acpi_waive_d0_probe` will return 1, which means that the device > will not be turned off. This is, it will be left in a different power > state. Lets fix it. > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Cc: stable@vger.kernel.org > Fixes: b18c1ad685d9 ("i2c: Allow an ACPI driver to manage the device's power state during probe") > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> It's much easier to read now. Thanks! Reviewed-by: Hidenori Kobayashi <hidenorik@chromium.org> > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index b4edf10e8fd0..6f4974c76404 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -467,6 +467,7 @@ static int i2c_device_probe(struct device *dev) > { > struct i2c_client *client = i2c_verify_client(dev); > struct i2c_driver *driver; > + bool do_power_on; > int status; > > if (!client) > @@ -545,8 +546,8 @@ static int i2c_device_probe(struct device *dev) > if (status < 0) > goto err_clear_wakeup_irq; > > - status = dev_pm_domain_attach(&client->dev, > - !i2c_acpi_waive_d0_probe(dev)); > + do_power_on = !i2c_acpi_waive_d0_probe(dev); > + status = dev_pm_domain_attach(&client->dev, do_power_on); > if (status) > goto err_clear_wakeup_irq; > > @@ -580,12 +581,14 @@ static int i2c_device_probe(struct device *dev) > if (status) > goto err_release_driver_resources; > > + client->power_off_on_remove = do_power_on; > + > return 0; > > err_release_driver_resources: > devres_release_group(&client->dev, client->devres_group_id); > err_detach_pm_domain: > - dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev)); > + dev_pm_domain_detach(&client->dev, do_power_on); > err_clear_wakeup_irq: > dev_pm_clear_wake_irq(&client->dev); > device_init_wakeup(&client->dev, false); > @@ -610,7 +613,7 @@ static void i2c_device_remove(struct device *dev) > > devres_release_group(&client->dev, client->devres_group_id); > > - dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev)); > + dev_pm_domain_detach(&client->dev, client->power_off_on_remove); > > dev_pm_clear_wake_irq(&client->dev); > device_init_wakeup(&client->dev, false); > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index f7c49bbdb8a1..eba83bc5459e 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -326,6 +326,8 @@ struct i2c_driver { > * calls it to pass on slave events to the slave driver. > * @devres_group_id: id of the devres group that will be created for resources > * acquired when probing this device. > + * @power_off_on_remove: Record if we have turned on the device before probing > + * so we can turn off the device at removal. > * > * An i2c_client identifies a single device (i.e. chip) connected to an > * i2c bus. The behaviour exposed to Linux is defined by the driver > @@ -355,6 +357,8 @@ struct i2c_client { > i2c_slave_cb_t slave_cb; /* callback for slave mode */ > #endif > void *devres_group_id; /* ID of probe devres group */ > + bool power_off_on_remove; /* if device needs to be turned */ > + /* off by framework at removal */ > }; > #define to_i2c_client(d) container_of(d, struct i2c_client, dev) > > > -- > b4 0.11.0-dev-d93f8 >
On (22/11/10 17:20), Ricardo Ribalda wrote: > A driver that supports I2C_DRV_ACPI_WAIVE_D0_PROBE is not expected to > power off a device that it has not powered on previously. > > For devices operating in "full_power" mode, the first call to > `i2c_acpi_waive_d0_probe` will return 0, which means that the device > will be turned on with `dev_pm_domain_attach`. > > If probe fails or the device is removed the second call to > `i2c_acpi_waive_d0_probe` will return 1, which means that the device > will not be turned off. This is, it will be left in a different power > state. Lets fix it. > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Cc: stable@vger.kernel.org > Fixes: b18c1ad685d9 ("i2c: Allow an ACPI driver to manage the device's power state during probe") > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
On Thu, Nov 10, 2022 at 05:20:39PM +0100, Ricardo Ribalda wrote: > A driver that supports I2C_DRV_ACPI_WAIVE_D0_PROBE is not expected to > power off a device that it has not powered on previously. > > For devices operating in "full_power" mode, the first call to > `i2c_acpi_waive_d0_probe` will return 0, which means that the device > will be turned on with `dev_pm_domain_attach`. > > If probe fails or the device is removed the second call to > `i2c_acpi_waive_d0_probe` will return 1, which means that the device > will not be turned off. This is, it will be left in a different power > state. Lets fix it. > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Cc: stable@vger.kernel.org > Fixes: b18c1ad685d9 ("i2c: Allow an ACPI driver to manage the device's power state during probe") > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> Adding I2C ACPI maintainer to CC. Mika, could you please help reviewing? > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index b4edf10e8fd0..6f4974c76404 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -467,6 +467,7 @@ static int i2c_device_probe(struct device *dev) > { > struct i2c_client *client = i2c_verify_client(dev); > struct i2c_driver *driver; > + bool do_power_on; > int status; > > if (!client) > @@ -545,8 +546,8 @@ static int i2c_device_probe(struct device *dev) > if (status < 0) > goto err_clear_wakeup_irq; > > - status = dev_pm_domain_attach(&client->dev, > - !i2c_acpi_waive_d0_probe(dev)); > + do_power_on = !i2c_acpi_waive_d0_probe(dev); > + status = dev_pm_domain_attach(&client->dev, do_power_on); > if (status) > goto err_clear_wakeup_irq; > > @@ -580,12 +581,14 @@ static int i2c_device_probe(struct device *dev) > if (status) > goto err_release_driver_resources; > > + client->power_off_on_remove = do_power_on; > + > return 0; > > err_release_driver_resources: > devres_release_group(&client->dev, client->devres_group_id); > err_detach_pm_domain: > - dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev)); > + dev_pm_domain_detach(&client->dev, do_power_on); > err_clear_wakeup_irq: > dev_pm_clear_wake_irq(&client->dev); > device_init_wakeup(&client->dev, false); > @@ -610,7 +613,7 @@ static void i2c_device_remove(struct device *dev) > > devres_release_group(&client->dev, client->devres_group_id); > > - dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev)); > + dev_pm_domain_detach(&client->dev, client->power_off_on_remove); > > dev_pm_clear_wake_irq(&client->dev); > device_init_wakeup(&client->dev, false); > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index f7c49bbdb8a1..eba83bc5459e 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -326,6 +326,8 @@ struct i2c_driver { > * calls it to pass on slave events to the slave driver. > * @devres_group_id: id of the devres group that will be created for resources > * acquired when probing this device. > + * @power_off_on_remove: Record if we have turned on the device before probing > + * so we can turn off the device at removal. > * > * An i2c_client identifies a single device (i.e. chip) connected to an > * i2c bus. The behaviour exposed to Linux is defined by the driver > @@ -355,6 +357,8 @@ struct i2c_client { > i2c_slave_cb_t slave_cb; /* callback for slave mode */ > #endif > void *devres_group_id; /* ID of probe devres group */ > + bool power_off_on_remove; /* if device needs to be turned */ > + /* off by framework at removal */ > }; > #define to_i2c_client(d) container_of(d, struct i2c_client, dev) > > > -- > b4 0.11.0-dev-d93f8
Hi, On Sat, Nov 12, 2022 at 09:24:14PM +0100, Wolfram Sang wrote: > On Thu, Nov 10, 2022 at 05:20:39PM +0100, Ricardo Ribalda wrote: > > A driver that supports I2C_DRV_ACPI_WAIVE_D0_PROBE is not expected to > > power off a device that it has not powered on previously. > > > > For devices operating in "full_power" mode, the first call to > > `i2c_acpi_waive_d0_probe` will return 0, which means that the device > > will be turned on with `dev_pm_domain_attach`. > > > > If probe fails or the device is removed the second call to > > `i2c_acpi_waive_d0_probe` will return 1, which means that the device > > will not be turned off. This is, it will be left in a different power > > state. Lets fix it. > > > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Cc: stable@vger.kernel.org > > Fixes: b18c1ad685d9 ("i2c: Allow an ACPI driver to manage the device's power state during probe") > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > Adding I2C ACPI maintainer to CC. Mika, could you please help > reviewing? Sure. > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > > index b4edf10e8fd0..6f4974c76404 100644 > > --- a/drivers/i2c/i2c-core-base.c > > +++ b/drivers/i2c/i2c-core-base.c > > @@ -467,6 +467,7 @@ static int i2c_device_probe(struct device *dev) > > { > > struct i2c_client *client = i2c_verify_client(dev); > > struct i2c_driver *driver; > > + bool do_power_on; > > int status; > > > > if (!client) > > @@ -545,8 +546,8 @@ static int i2c_device_probe(struct device *dev) > > if (status < 0) > > goto err_clear_wakeup_irq; > > > > - status = dev_pm_domain_attach(&client->dev, > > - !i2c_acpi_waive_d0_probe(dev)); > > + do_power_on = !i2c_acpi_waive_d0_probe(dev); > > + status = dev_pm_domain_attach(&client->dev, do_power_on); I think this is fine as the driver says it is OK to see the device in whatever power state (I assume this is what the i2c_acpi_waive_d0_probe() is supposed to be doing but there is no kernel-doc, though). > > if (status) > > goto err_clear_wakeup_irq; > > > > @@ -580,12 +581,14 @@ static int i2c_device_probe(struct device *dev) > > if (status) > > goto err_release_driver_resources; > > > > + client->power_off_on_remove = do_power_on; > > + > > return 0; > > > > err_release_driver_resources: > > devres_release_group(&client->dev, client->devres_group_id); > > err_detach_pm_domain: > > - dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev)); > > + dev_pm_domain_detach(&client->dev, do_power_on); > > err_clear_wakeup_irq: > > dev_pm_clear_wake_irq(&client->dev); > > device_init_wakeup(&client->dev, false); > > @@ -610,7 +613,7 @@ static void i2c_device_remove(struct device *dev) > > > > devres_release_group(&client->dev, client->devres_group_id); > > > > - dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev)); > > + dev_pm_domain_detach(&client->dev, client->power_off_on_remove); However, on the remove path I think we should not call i2c_acpi_waive_d0_probe() at all as that has nothing to do with remove (it is for whether the driver accepts any power state on probe AFAICT) so this should stil be "true" here. Unless I'm missing something. > > > > dev_pm_clear_wake_irq(&client->dev); > > device_init_wakeup(&client->dev, false); > > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > > index f7c49bbdb8a1..eba83bc5459e 100644 > > --- a/include/linux/i2c.h > > +++ b/include/linux/i2c.h > > @@ -326,6 +326,8 @@ struct i2c_driver { > > * calls it to pass on slave events to the slave driver. > > * @devres_group_id: id of the devres group that will be created for resources > > * acquired when probing this device. > > + * @power_off_on_remove: Record if we have turned on the device before probing > > + * so we can turn off the device at removal. > > * > > * An i2c_client identifies a single device (i.e. chip) connected to an > > * i2c bus. The behaviour exposed to Linux is defined by the driver > > @@ -355,6 +357,8 @@ struct i2c_client { > > i2c_slave_cb_t slave_cb; /* callback for slave mode */ > > #endif > > void *devres_group_id; /* ID of probe devres group */ > > + bool power_off_on_remove; /* if device needs to be turned */ > > + /* off by framework at removal */ > > }; > > #define to_i2c_client(d) container_of(d, struct i2c_client, dev) > > > > > > -- > > b4 0.11.0-dev-d93f8
Hi Mika Thanks for your review! On Mon, 14 Nov 2022 at 11:33, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > Hi, > > On Sat, Nov 12, 2022 at 09:24:14PM +0100, Wolfram Sang wrote: > > On Thu, Nov 10, 2022 at 05:20:39PM +0100, Ricardo Ribalda wrote: > > > A driver that supports I2C_DRV_ACPI_WAIVE_D0_PROBE is not expected to > > > power off a device that it has not powered on previously. > > > > > > For devices operating in "full_power" mode, the first call to > > > `i2c_acpi_waive_d0_probe` will return 0, which means that the device > > > will be turned on with `dev_pm_domain_attach`. > > > > > > If probe fails or the device is removed the second call to > > > `i2c_acpi_waive_d0_probe` will return 1, which means that the device > > > will not be turned off. This is, it will be left in a different power > > > state. Lets fix it. > > > > > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > Cc: stable@vger.kernel.org > > > Fixes: b18c1ad685d9 ("i2c: Allow an ACPI driver to manage the device's power state during probe") > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > Adding I2C ACPI maintainer to CC. Mika, could you please help > > reviewing? > > Sure. > > > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > > > index b4edf10e8fd0..6f4974c76404 100644 > > > --- a/drivers/i2c/i2c-core-base.c > > > +++ b/drivers/i2c/i2c-core-base.c > > > @@ -467,6 +467,7 @@ static int i2c_device_probe(struct device *dev) > > > { > > > struct i2c_client *client = i2c_verify_client(dev); > > > struct i2c_driver *driver; > > > + bool do_power_on; > > > int status; > > > > > > if (!client) > > > @@ -545,8 +546,8 @@ static int i2c_device_probe(struct device *dev) > > > if (status < 0) > > > goto err_clear_wakeup_irq; > > > > > > - status = dev_pm_domain_attach(&client->dev, > > > - !i2c_acpi_waive_d0_probe(dev)); > > > + do_power_on = !i2c_acpi_waive_d0_probe(dev); > > > + status = dev_pm_domain_attach(&client->dev, do_power_on); > > I think this is fine as the driver says it is OK to see the device in > whatever power state (I assume this is what the > i2c_acpi_waive_d0_probe() is supposed to be doing but there is no > kernel-doc, though). > > > > if (status) > > > goto err_clear_wakeup_irq; > > > > > > @@ -580,12 +581,14 @@ static int i2c_device_probe(struct device *dev) > > > if (status) > > > goto err_release_driver_resources; > > > > > > + client->power_off_on_remove = do_power_on; > > > + > > > return 0; > > > > > > err_release_driver_resources: > > > devres_release_group(&client->dev, client->devres_group_id); > > > err_detach_pm_domain: > > > - dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev)); > > > + dev_pm_domain_detach(&client->dev, do_power_on); > > > err_clear_wakeup_irq: > > > dev_pm_clear_wake_irq(&client->dev); > > > device_init_wakeup(&client->dev, false); > > > @@ -610,7 +613,7 @@ static void i2c_device_remove(struct device *dev) > > > > > > devres_release_group(&client->dev, client->devres_group_id); > > > > > > - dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev)); > > > + dev_pm_domain_detach(&client->dev, client->power_off_on_remove); > > However, on the remove path I think we should not call > i2c_acpi_waive_d0_probe() at all as that has nothing to do with remove > (it is for whether the driver accepts any power state on probe AFAICT) > so this should stil be "true" here. Unless I'm missing something. I guess the problem is that we would be undoing something (power-off), that we did not do. Sakari, can you think of a use-case where this can break something? If not I can send a v6. Regards! > > > > > > > dev_pm_clear_wake_irq(&client->dev); > > > device_init_wakeup(&client->dev, false); > > > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > > > index f7c49bbdb8a1..eba83bc5459e 100644 > > > --- a/include/linux/i2c.h > > > +++ b/include/linux/i2c.h > > > @@ -326,6 +326,8 @@ struct i2c_driver { > > > * calls it to pass on slave events to the slave driver. > > > * @devres_group_id: id of the devres group that will be created for resources > > > * acquired when probing this device. > > > + * @power_off_on_remove: Record if we have turned on the device before probing > > > + * so we can turn off the device at removal. > > > * > > > * An i2c_client identifies a single device (i.e. chip) connected to an > > > * i2c bus. The behaviour exposed to Linux is defined by the driver > > > @@ -355,6 +357,8 @@ struct i2c_client { > > > i2c_slave_cb_t slave_cb; /* callback for slave mode */ > > > #endif > > > void *devres_group_id; /* ID of probe devres group */ > > > + bool power_off_on_remove; /* if device needs to be turned */ > > > + /* off by framework at removal */ > > > }; > > > #define to_i2c_client(d) container_of(d, struct i2c_client, dev) > > > > > > > > > -- > > > b4 0.11.0-dev-d93f8 > >
Hi Ricardo, Mika, On Mon, Nov 14, 2022 at 11:51:24AM +0100, Ricardo Ribalda wrote: > Hi Mika > > Thanks for your review! > > On Mon, 14 Nov 2022 at 11:33, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > > > Hi, > > > > On Sat, Nov 12, 2022 at 09:24:14PM +0100, Wolfram Sang wrote: > > > On Thu, Nov 10, 2022 at 05:20:39PM +0100, Ricardo Ribalda wrote: > > > > A driver that supports I2C_DRV_ACPI_WAIVE_D0_PROBE is not expected to > > > > power off a device that it has not powered on previously. > > > > > > > > For devices operating in "full_power" mode, the first call to > > > > `i2c_acpi_waive_d0_probe` will return 0, which means that the device > > > > will be turned on with `dev_pm_domain_attach`. > > > > > > > > If probe fails or the device is removed the second call to > > > > `i2c_acpi_waive_d0_probe` will return 1, which means that the device > > > > will not be turned off. This is, it will be left in a different power > > > > state. Lets fix it. > > > > > > > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > Cc: stable@vger.kernel.org > > > > Fixes: b18c1ad685d9 ("i2c: Allow an ACPI driver to manage the device's power state during probe") > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > > > Adding I2C ACPI maintainer to CC. Mika, could you please help > > > reviewing? > > > > Sure. > > > > > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > > > > index b4edf10e8fd0..6f4974c76404 100644 > > > > --- a/drivers/i2c/i2c-core-base.c > > > > +++ b/drivers/i2c/i2c-core-base.c > > > > @@ -467,6 +467,7 @@ static int i2c_device_probe(struct device *dev) > > > > { > > > > struct i2c_client *client = i2c_verify_client(dev); > > > > struct i2c_driver *driver; > > > > + bool do_power_on; > > > > int status; > > > > > > > > if (!client) > > > > @@ -545,8 +546,8 @@ static int i2c_device_probe(struct device *dev) > > > > if (status < 0) > > > > goto err_clear_wakeup_irq; > > > > > > > > - status = dev_pm_domain_attach(&client->dev, > > > > - !i2c_acpi_waive_d0_probe(dev)); > > > > + do_power_on = !i2c_acpi_waive_d0_probe(dev); > > > > + status = dev_pm_domain_attach(&client->dev, do_power_on); > > > > I think this is fine as the driver says it is OK to see the device in > > whatever power state (I assume this is what the > > i2c_acpi_waive_d0_probe() is supposed to be doing but there is no > > kernel-doc, though). > > > > > > if (status) > > > > goto err_clear_wakeup_irq; > > > > > > > > @@ -580,12 +581,14 @@ static int i2c_device_probe(struct device *dev) > > > > if (status) > > > > goto err_release_driver_resources; > > > > > > > > + client->power_off_on_remove = do_power_on; > > > > + > > > > return 0; > > > > > > > > err_release_driver_resources: > > > > devres_release_group(&client->dev, client->devres_group_id); > > > > err_detach_pm_domain: > > > > - dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev)); > > > > + dev_pm_domain_detach(&client->dev, do_power_on); > > > > err_clear_wakeup_irq: > > > > dev_pm_clear_wake_irq(&client->dev); > > > > device_init_wakeup(&client->dev, false); > > > > @@ -610,7 +613,7 @@ static void i2c_device_remove(struct device *dev) > > > > > > > > devres_release_group(&client->dev, client->devres_group_id); > > > > > > > > - dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev)); > > > > + dev_pm_domain_detach(&client->dev, client->power_off_on_remove); > > > > However, on the remove path I think we should not call > > i2c_acpi_waive_d0_probe() at all as that has nothing to do with remove > > (it is for whether the driver accepts any power state on probe AFAICT) > > so this should stil be "true" here. Unless I'm missing something. > > I guess the problem is that we would be undoing something (power-off), > that we did not do. > > > Sakari, can you think of a use-case where this can break something? If > not I can send a v6. I don't think there's any harm from powering the device off here. The driver should have done it but there could be bugs... The genpd equivalent doesn't even use the argument but powers off the domain in any case.
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index b4edf10e8fd0..6f4974c76404 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -467,6 +467,7 @@ static int i2c_device_probe(struct device *dev) { struct i2c_client *client = i2c_verify_client(dev); struct i2c_driver *driver; + bool do_power_on; int status; if (!client) @@ -545,8 +546,8 @@ static int i2c_device_probe(struct device *dev) if (status < 0) goto err_clear_wakeup_irq; - status = dev_pm_domain_attach(&client->dev, - !i2c_acpi_waive_d0_probe(dev)); + do_power_on = !i2c_acpi_waive_d0_probe(dev); + status = dev_pm_domain_attach(&client->dev, do_power_on); if (status) goto err_clear_wakeup_irq; @@ -580,12 +581,14 @@ static int i2c_device_probe(struct device *dev) if (status) goto err_release_driver_resources; + client->power_off_on_remove = do_power_on; + return 0; err_release_driver_resources: devres_release_group(&client->dev, client->devres_group_id); err_detach_pm_domain: - dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev)); + dev_pm_domain_detach(&client->dev, do_power_on); err_clear_wakeup_irq: dev_pm_clear_wake_irq(&client->dev); device_init_wakeup(&client->dev, false); @@ -610,7 +613,7 @@ static void i2c_device_remove(struct device *dev) devres_release_group(&client->dev, client->devres_group_id); - dev_pm_domain_detach(&client->dev, !i2c_acpi_waive_d0_probe(dev)); + dev_pm_domain_detach(&client->dev, client->power_off_on_remove); dev_pm_clear_wake_irq(&client->dev); device_init_wakeup(&client->dev, false); diff --git a/include/linux/i2c.h b/include/linux/i2c.h index f7c49bbdb8a1..eba83bc5459e 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -326,6 +326,8 @@ struct i2c_driver { * calls it to pass on slave events to the slave driver. * @devres_group_id: id of the devres group that will be created for resources * acquired when probing this device. + * @power_off_on_remove: Record if we have turned on the device before probing + * so we can turn off the device at removal. * * An i2c_client identifies a single device (i.e. chip) connected to an * i2c bus. The behaviour exposed to Linux is defined by the driver @@ -355,6 +357,8 @@ struct i2c_client { i2c_slave_cb_t slave_cb; /* callback for slave mode */ #endif void *devres_group_id; /* ID of probe devres group */ + bool power_off_on_remove; /* if device needs to be turned */ + /* off by framework at removal */ }; #define to_i2c_client(d) container_of(d, struct i2c_client, dev)