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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id EB293C4345F for ; Sun, 28 Apr 2024 16:54:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=wa5X2YH65TLRDN8BKurdDGAHO0rPUGXl/hGFOVqKcts=; b=Rhs45F8wYqDKwc kMenot/UEmwS/yMHy1AOL9XyVdLEtW966MTV0bzVQZ1eOawlm8zL+/1bS+2JDnTLTQNf3wMSF+DVc 5nXUugrDSV4V8wIAYGVQnjAblWhmHI+BFX1Hg31yG6/3q1EP/18Nh0bkrcC+XIXh/Lzj69D1fCWpy laIoiGOHFl80q084Jefcu3rj0eoDZi8kWVyFbzVWlvWkUs+AYmKdCd1wH4m2lnScepUcSosDVrd0u W1etD6GOfQoFpN4nno9tFpDKmbEO3sAl1AWF1I4Cwwayl6o+TMOqZuIv1PK8V/yAhIYIb+oQ2/LUx jx4xkJveR9g3/cPJVdnA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1s17mk-00000000LIP-44Yg; Sun, 28 Apr 2024 16:53:54 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1s17mi-00000000LHx-0doO for linux-arm-kernel@lists.infradead.org; Sun, 28 Apr 2024 16:53:53 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 5244260B8B; Sun, 28 Apr 2024 16:53:51 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4A048C113CC; Sun, 28 Apr 2024 16:53:45 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1714323231; bh=vKlq852Mkt2LdjN+6R/Og0QA2JGqOCAcs1MaQ4RQDnc=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=LjR61GuMEFGAu419qGiabMclqWpfYwDouS36biVw7/fRhxjf2g4HgGKM8NUteeSZz JLeRT6vNf70R+Uvx7Ke8EzX01G8z5BwsfRDVnblc2Mx5uH/GhL/g+6QK0bVoasYoa3 scxwyecrhm9CLsnpZsVtalzmmNDr0HTP/vpeT8iyiq/cYnKgFfpRj5RF0zdL3LKtty aLWyMXoNDTbCC3DUBkJE3wHHgSX5AlhrwS8J1q3m72uJYJpEbDBFTXJWG93QS0JvDp LcU0xG+a1b0MM8xJepL9Fdlj50PP8nSrQzI1C5o8MtP7Uwg9/CdG78Pmxx5L8YvIau aM85Mca+qEjzw== Date: Sun, 28 Apr 2024 17:53:37 +0100 From: Jonathan Cameron To: Aren Moynihan Cc: Lars-Peter Clausen , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Chen-Yu Tsai , Jernej Skrabec , Samuel Holland , Liam Girdwood , Mark Brown , Andy Shevchenko , Ondrej Jirman , Uwe =?UTF-8?B?S2xlaW5lLUvDtm5pZw==?= , linux-iio@vger.kernel.org, phone-devel@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, Willow Barraco Subject: Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend Message-ID: <20240428175337.61850e2a@jic23-huawei> In-Reply-To: <20240423223309.1468198-4-aren@peacevolution.org> References: <20240423223309.1468198-2-aren@peacevolution.org> <20240423223309.1468198-4-aren@peacevolution.org> X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240428_095352_321931_1E3BC2CC X-CRM114-Status: GOOD ( 28.91 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, 23 Apr 2024 18:33:05 -0400 Aren Moynihan wrote: > From: Ondrej Jirman > > VDD power input can be used to completely power off the chip during > system suspend. Do so if available. > > Signed-off-by: Ondrej Jirman > Signed-off-by: Aren Moynihan Suggestions inline. Key thing is take the whole thing devm_ managed and your life gets much easier. It is mixing the two approaches that causes problems and often the best plan is to do everything in probe/remove with devm_ calls to do the cleanup for you. > --- > > Notes: > Changes in v2: > - always enable / disable regulators and rely on a dummy regulator if > one isn't specified > - replace usleep_range with fsleep > - reorder includes so iio headers are last > - add missing error handling to resume > > drivers/iio/light/stk3310.c | 49 ++++++++++++++++++++++++++++++++++--- > 1 file changed, 46 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c > index 7b71ad71d78d..a0547eeca3e3 100644 > --- a/drivers/iio/light/stk3310.c > +++ b/drivers/iio/light/stk3310.c > @@ -13,6 +13,8 @@ > #include > #include > #include > +#include > + > #include > #include > #include > @@ -117,6 +119,7 @@ struct stk3310_data { > struct regmap_field *reg_int_ps; > struct regmap_field *reg_flag_psint; > struct regmap_field *reg_flag_nf; > + struct regulator *vdd_reg; > }; > > static const struct iio_event_spec stk3310_events[] = { > @@ -607,6 +610,10 @@ static int stk3310_probe(struct i2c_client *client) > > mutex_init(&data->lock); > > + data->vdd_reg = devm_regulator_get(&client->dev, "vdd"); > + if (IS_ERR(data->vdd_reg)) > + return dev_err_probe(&client->dev, ret, "get regulator vdd failed\n"); > + > ret = stk3310_regmap_init(data); > if (ret < 0) > return ret; > @@ -617,9 +624,17 @@ static int stk3310_probe(struct i2c_client *client) > indio_dev->channels = stk3310_channels; > indio_dev->num_channels = ARRAY_SIZE(stk3310_channels); > > + ret = regulator_enable(data->vdd_reg); > + if (ret) > + return dev_err_probe(&client->dev, ret, > + "regulator vdd enable failed\n"); > + > + /* we need a short delay to allow the chip time to power on */ > + fsleep(1000); > + > ret = stk3310_init(indio_dev); > if (ret < 0) > - return ret; > + goto err_vdd_disable; > > if (client->irq > 0) { > ret = devm_request_threaded_irq(&client->dev, client->irq, > @@ -645,32 +660,60 @@ static int stk3310_probe(struct i2c_client *client) > > err_standby: > stk3310_set_state(data, STK3310_STATE_STANDBY); Move this handling to a devm_add_action_or_reset() callback in a precursor patch. That will fix the current ordering issue wrt to the irq registration. Then use devm_iio_device_register() in that precursor patch. > +err_vdd_disable: > + regulator_disable(data->vdd_reg); Add a devm_add_action_or_reset() callback to disable this regulator in this patch. Register that just after the enable. That way the ordering will be maintained for all calls. > return ret; > } > > static void stk3310_remove(struct i2c_client *client) > { > struct iio_dev *indio_dev = i2c_get_clientdata(client); > + struct stk3310_data *data = iio_priv(indio_dev); > > iio_device_unregister(indio_dev); > stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY); > + regulator_disable(data->vdd_reg); With above suggested changes, you can drop the remove function entirely. > } > > static int stk3310_suspend(struct device *dev) > { > struct stk3310_data *data; > + int ret; > > data = iio_priv(i2c_get_clientdata(to_i2c_client(dev))); > > - return stk3310_set_state(data, STK3310_STATE_STANDBY); > + ret = stk3310_set_state(data, STK3310_STATE_STANDBY); > + if (ret) > + return ret; > + > + regcache_mark_dirty(data->regmap); > + regulator_disable(data->vdd_reg); > + > + return 0; > } > > static int stk3310_resume(struct device *dev) > { > - u8 state = 0; > struct stk3310_data *data; > + u8 state = 0; > + int ret; > > data = iio_priv(i2c_get_clientdata(to_i2c_client(dev))); > + > + ret = regulator_enable(data->vdd_reg); > + if (ret) { > + dev_err(dev, "Failed to re-enable regulator vdd\n"); > + return ret; > + } > + > + fsleep(1000); > + > + ret = regcache_sync(data->regmap); > + if (ret) { > + dev_err(dev, "Failed to restore registers: %d\n", ret); > + return ret; > + } > + > if (data->ps_enabled) > state |= STK3310_STATE_EN_PS; > if (data->als_enabled) _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel