From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id AF832C43217 for ; Thu, 10 Nov 2022 09:25:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229885AbiKJJZk (ORCPT ); Thu, 10 Nov 2022 04:25:40 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50360 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229881AbiKJJZi (ORCPT ); Thu, 10 Nov 2022 04:25:38 -0500 Received: from mail-pj1-x102c.google.com (mail-pj1-x102c.google.com [IPv6:2607:f8b0:4864:20::102c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id ACD726A6A4 for ; Thu, 10 Nov 2022 01:25:37 -0800 (PST) Received: by mail-pj1-x102c.google.com with SMTP id m14-20020a17090a3f8e00b00212dab39bcdso4248866pjc.0 for ; Thu, 10 Nov 2022 01:25:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=OBAvZyVdKdxCrRJk4/Y4XRx0xKLrQ3v4QwzwswlPqrU=; b=oac+QW/UrS149OaflUweDXjkJMQeL/m3U7pGrsm5gPYMlsPBpgRTkUJ3BzhD6NEZbJ H3iXY5bIIzadESW4ITb0IcWQ9mIiM1mho7JGp1fLvgNd4Y6csCVTmZONd7PhWS2XBPmi y/jEWJYrKcXze1JywRRncF1dgNHv9sxjJoA7M= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=OBAvZyVdKdxCrRJk4/Y4XRx0xKLrQ3v4QwzwswlPqrU=; b=2r0tG2dMJLn+lLq+s+smuOwYxpc2+gYtxGW7r4jGFNi8TgmjeDUS1ZqKbazv+n+GAI 8ij7y1agbWXa6+Xpw39sVC/X5vQ6WTkdV3VkhuEudz8saOuKQmps+Pj08GiRPfZm5pp1 MKShcfu4Zg4b6QFXkWNok6wElyAvy++wNzTaWjzhdMDGhJXcvvFVwMCzO3/Tt2D5DjQw mGHc3QtEMoDfeqMnZ5nHcOcPw+pSy7LKelXx3EO2lLP1zMjaEdO7iLtrusxsG+5p5oss goQRdF0F+Xt+6Uc2G9m/kT+dkd0P77bHhHXWZgDef6mtzMw0CKnEYiQoNy72dzTwuW77 sHiA== X-Gm-Message-State: ACrzQf3pGFDmuanH79vjYMAr2otVrfFfW7yf+pvT+Rft4ZEWCi2iFjl0 kvUIukolyRPQLF9kwHieZ2MqBA== X-Google-Smtp-Source: AMsMyM4RU6NgaZ4HDrUMXilA6ONITeC18JIEPpoKBJTmP+DDz48JBS2vElX3I7KDn2AYi50739d0Yg== X-Received: by 2002:a17:902:db12:b0:187:4736:f780 with SMTP id m18-20020a170902db1200b001874736f780mr42281570plx.145.1668072337213; Thu, 10 Nov 2022 01:25:37 -0800 (PST) Received: from localhost ([2401:fa00:8f:203:729a:7662:547f:f4a3]) by smtp.gmail.com with ESMTPSA id t72-20020a63784b000000b0044ed37dbca8sm8814450pgc.2.2022.11.10.01.25.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 10 Nov 2022 01:25:36 -0800 (PST) Date: Thu, 10 Nov 2022 18:25:34 +0900 From: Hidenori Kobayashi To: Ricardo Ribalda Cc: Tomasz Figa , "Rafael J. Wysocki" , Wolfram Sang , Sakari Ailus , linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org Subject: Re: [PATCH v2 1/1] i2c: Restore initial power state when we are done. Message-ID: <20221110092534.uzxfevcig4dllvb2@google.com> References: <20221109-i2c-waive-v2-0-07550bf2dacc@chromium.org> <20221109-i2c-waive-v2-1-07550bf2dacc@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221109-i2c-waive-v2-1-07550bf2dacc@chromium.org> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Ricardo, On Wed, Nov 09, 2022 at 04:17:06PM +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. > > Fixes: b18c1ad685d9 ("i2c: Allow an ACPI driver to manage the device's power state during probe") > Signed-off-by: Ricardo Ribalda > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index b4edf10e8fd0..96623e0647bd 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -545,8 +545,9 @@ static int i2c_device_probe(struct device *dev) > if (status < 0) > goto err_clear_wakeup_irq; > > + client->turn_off_on_remove = !i2c_acpi_waive_d0_probe(dev); > status = dev_pm_domain_attach(&client->dev, > - !i2c_acpi_waive_d0_probe(dev)); > + client->turn_off_on_remove); > if (status) > goto err_clear_wakeup_irq; > > @@ -585,7 +586,7 @@ static int i2c_device_probe(struct device *dev) > 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, client->turn_off_on_remove); > err_clear_wakeup_irq: > dev_pm_clear_wake_irq(&client->dev); > device_init_wakeup(&client->dev, false); > @@ -610,7 +611,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->turn_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..6b2dacb0bae1 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. > + * @turn_off_on_remove: Record if we have turned on the device before probing > + * so we can restore the initial state after remove/probe error. > * > * 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,7 @@ struct i2c_client { > i2c_slave_cb_t slave_cb; /* callback for slave mode */ > #endif > void *devres_group_id; /* ID of probe devres group */ > + bool turn_off_on_remove; /* power state when done */ Can we have a different name that also makes sense for attach()? To me, it's kind of hard to see immediately what the second argument to attach() meant. Since this is used for both power_on and power_off, I think something more neutral would be easier to read? For example: power_flag? Or I guess we could name it like full_power and provide a wrapper for the detach() cases? > }; > #define to_i2c_client(d) container_of(d, struct i2c_client, dev) > > > -- > b4 0.11.0-dev-d93f8 >