Message ID | ZFIw/KdApZe1euN8@fedora |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp1225699vqo; Wed, 3 May 2023 03:42:08 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4L/iN+aPFbooPn89dFdHmTbtqR0Mv0mTjuwZNG/osfewfSMFHscRzeBsoJbDB0wWuQcAEw X-Received: by 2002:a05:6a00:1342:b0:63f:32ed:92b1 with SMTP id k2-20020a056a00134200b0063f32ed92b1mr2280780pfu.7.1683110528293; Wed, 03 May 2023 03:42:08 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1683110528; cv=none; d=google.com; s=arc-20160816; b=bbaMlfEFBzw1NnGzaNOdcor44c6I544nyk51cjuuvqN2xkDayHJ6bxO58GB1Wyjpjf DvyGcVg/9utQTtpv25VEaGrxlyfEiyALcE4N2hGNhECftd452sfHvDs1Ug77b9ADWcL7 HgZBWv1AH6YG4ddwQ+wt3YPJz3YixpKWj44pqcbuGbHMn26xolR95zyA1qmy7yqoSTMe 8Coxr0XiT4EXZ9ghDkxvZUp9eGqrxmmVT8PGy5xtj7+jFjZ1XgURgJg0IvLSglf/thfW P1WHM+rBeFEa63xwmlUycnRCVlhz44rTWNepA+WioJ0GxK29vjJUCgY7R11uRWixosO3 QA8w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-disposition:mime-version:message-id :subject:cc:to:from:date:dkim-signature; bh=cbLddoF+dAfHfXpgdlmLObjxEgG+kEshDJTymk4l9ms=; b=FPT0P09Y8FeHQkkJ6GiMx41/Zu6TSXYBXB6779OCSz6VztHkbkNygfxCMdaOiLrsZn PL0n9Qpm+Xq+cDSj85lU3lxRt7OIFNFQvACoOa6JZf+ToNIXJrAbIGxaM5dhUfQqCcOi lQCARhIbPT1jMjSH9AhAszg/avE514L1JbKmQ4A8aUY0WsiWbqYhzMdMJPigL7Qt3L+U b0gsMoZ+NcHr8cPHzSIep3OfcnMWCvSgt4BzGpdwvXavMPG+aga+6sr29tBzy1MOJwX2 1SM/BauYXxNRMc8B0zj+ApxESRgT0By6fFRKkxbz/Y/EOTSZlwPpgbfEDT68rzI7dNfM suBg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20221208 header.b=gI5XkbYs; 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=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id u130-20020a627988000000b006434d66782bsi545861pfc.321.2023.05.03.03.41.55; Wed, 03 May 2023 03:42:08 -0700 (PDT) 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=@gmail.com header.s=20221208 header.b=gI5XkbYs; 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=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229848AbjECKBq (ORCPT <rfc822;heyuhang3455@gmail.com> + 99 others); Wed, 3 May 2023 06:01:46 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55314 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229555AbjECKBk (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 3 May 2023 06:01:40 -0400 Received: from mail-lj1-x22e.google.com (mail-lj1-x22e.google.com [IPv6:2a00:1450:4864:20::22e]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BD08F1A7; Wed, 3 May 2023 03:01:38 -0700 (PDT) Received: by mail-lj1-x22e.google.com with SMTP id 38308e7fff4ca-2ac70c975fcso7007581fa.3; Wed, 03 May 2023 03:01:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1683108097; x=1685700097; h=content-disposition:mime-version:message-id:subject:cc:to:from:date :from:to:cc:subject:date:message-id:reply-to; bh=cbLddoF+dAfHfXpgdlmLObjxEgG+kEshDJTymk4l9ms=; b=gI5XkbYsbtK/jwU0afMsdo76wKVXF4M7QiDt3zprFZQ28XDkIjmMXMD2kenFHE65yV ohgIcOOnH11dknn3BhPL7HkLAE7ZcO65CyNqAINsi5rBHTRyTaj8SC/B+HOSlRT15CBF l2juIQliZwBDGP32kNlp9QmTm7coAlGYJIlZlfYg7mFb6mLf0jUNhj3Lnz/6eXu7Qhry wfWvoJZsr2eWay/PnYJyGZ93nFKA5xPxB/I43OjbaT9t0x/XGWIoq/pOavFebqYBL/Ue baCm2WRe++NvNI3LW8GsvRP71O9YFt0UWpPmCEMEyxak1vRVWl6nLb8o+Ngp2Iw1iSzk tzoA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683108097; x=1685700097; h=content-disposition:mime-version:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=cbLddoF+dAfHfXpgdlmLObjxEgG+kEshDJTymk4l9ms=; b=Hc4L9YO3y2IBNL3mVL2L/9/xH2zc8mmKm8OpkkKmh/VvykPbT+10gPKb0oI0LypyW8 trozfL1pOmxkzjbjPjfs5vh+YeU6zD2O63kQGyhA/u2xhgYYdNJJLsji84EOGmmOv23J xQrGfRYMDzXgX+1J+AxKEtgFeAgT0YRyEdYXhpaQ5qTWF/LNFQWbjMyeDEWo3ksk8vzD wuZ8DTfoTfoLd/Hc9zc19UI4elJARsanXgTb66Tli0HcUUHrxMHVFb/Ei8Fg2qxt8wyE j8zMFAcRY+/NUutPS/Fpov6+v3rjo117bC2cX1an+WPInZsqgOPQ8/wu0PsiY7G8Pypz maGg== X-Gm-Message-State: AC+VfDx0xsVWtPDWRYjVr5Sa/Jxx0rDwKwbpfJnWSL2aoJyzTHNniM57 mjXs3GL21FkG+5m9eGYB3OWb9n5QDGE= X-Received: by 2002:a2e:8295:0:b0:2a6:1682:3a1e with SMTP id y21-20020a2e8295000000b002a616823a1emr5224079ljg.31.1683108096971; Wed, 03 May 2023 03:01:36 -0700 (PDT) Received: from fedora (62-78-225-252.bb.dnainternet.fi. [62.78.225.252]) by smtp.gmail.com with ESMTPSA id y11-20020a05651c020b00b002a8a8f2dc89sm5907418ljn.72.2023.05.03.03.01.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 03 May 2023 03:01:36 -0700 (PDT) Date: Wed, 3 May 2023 13:01:32 +0300 From: Matti Vaittinen <mazziesaccount@gmail.com> To: Matti Vaittinen <mazziesaccount@gmail.com>, Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> Cc: Matti Vaittinen <mazziesaccount@gmail.com>, Jonathan Cameron <jic23@kernel.org>, Lars-Peter Clausen <lars@metafoo.de>, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH v2] iio: bu27034: Ensure reset is written Message-ID: <ZFIw/KdApZe1euN8@fedora> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Ru/DL4Tb+YxI3Pq5" Content-Disposition: inline X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE 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?1764869305454893135?= X-GMAIL-MSGID: =?utf-8?q?1764869305454893135?= |
Series |
[v2] iio: bu27034: Ensure reset is written
|
|
Commit Message
Matti Vaittinen
May 3, 2023, 10:01 a.m. UTC
The reset bit must be always written to the hardware no matter what value
is in a cache or register. Ensure this by using regmap_write_bits()
instead of the regmap_update_bits(). Furthermore, the RESET bit may be
self-clearing, so mark the SYSTEM_CONTROL register volatile to guarantee
we do also read the right state - should we ever need to read it.
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Fixes: e52afbd61039 ("iio: light: ROHM BU27034 Ambient Light Sensor")
---
Changelog:
v1 => v2:
- Fix SoB tag
I haven't verified if the reset bit is self-clearin as I did temporarily
give away the HW.
In worst case the bit is not self clearing - but we don't really
get performance penalty even if we set the register volatile because the
SYSTEM_CONTROL register only has the part-ID and the reset fields. The
part-id is only read once at probe.
---
drivers/iio/light/rohm-bu27034.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
base-commit: 7fcbd72176076c44b47e8f68f0223c02c411f420
Comments
On Wed, 3 May 2023 13:01:32 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > The reset bit must be always written to the hardware no matter what value > is in a cache or register. Ensure this by using regmap_write_bits() > instead of the regmap_update_bits(). Furthermore, the RESET bit may be > self-clearing, so mark the SYSTEM_CONTROL register volatile to guarantee > we do also read the right state - should we ever need to read it. > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > Fixes: e52afbd61039 ("iio: light: ROHM BU27034 Ambient Light Sensor") This obviously interacts with the regcache update. Fun question is whether a register is volatile if it results in all registers (including itself) resetting. In my view, no it isn't volatile. So fixing the regcache stuff as in your other patch is more appropriate. Jonathan > > --- > Changelog: > v1 => v2: > - Fix SoB tag > > > I haven't verified if the reset bit is self-clearin as I did temporarily > give away the HW. > > In worst case the bit is not self clearing - but we don't really > get performance penalty even if we set the register volatile because the > SYSTEM_CONTROL register only has the part-ID and the reset fields. The > part-id is only read once at probe. > > --- > drivers/iio/light/rohm-bu27034.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/light/rohm-bu27034.c b/drivers/iio/light/rohm-bu27034.c > index 25c9b79574a5..740ebd86b6e5 100644 > --- a/drivers/iio/light/rohm-bu27034.c > +++ b/drivers/iio/light/rohm-bu27034.c > @@ -231,6 +231,9 @@ struct bu27034_result { > > static const struct regmap_range bu27034_volatile_ranges[] = { > { > + .range_min = BU27034_REG_SYSTEM_CONTROL, > + .range_max = BU27034_REG_SYSTEM_CONTROL, > + }, { > .range_min = BU27034_REG_MODE_CONTROL4, > .range_max = BU27034_REG_MODE_CONTROL4, > }, { > @@ -1272,7 +1275,7 @@ static int bu27034_chip_init(struct bu27034_data *data) > int ret, sel; > > /* Reset */ > - ret = regmap_update_bits(data->regmap, BU27034_REG_SYSTEM_CONTROL, > + ret = regmap_write_bits(data->regmap, BU27034_REG_SYSTEM_CONTROL, > BU27034_MASK_SW_RESET, BU27034_MASK_SW_RESET); > if (ret) > return dev_err_probe(data->dev, ret, "Sensor reset failed\n"); > > base-commit: 7fcbd72176076c44b47e8f68f0223c02c411f420
On 5/6/23 21:27, Jonathan Cameron wrote: > On Wed, 3 May 2023 13:01:32 +0300 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> The reset bit must be always written to the hardware no matter what value >> is in a cache or register. Ensure this by using regmap_write_bits() >> instead of the regmap_update_bits(). Furthermore, the RESET bit may be >> self-clearing, so mark the SYSTEM_CONTROL register volatile to guarantee >> we do also read the right state - should we ever need to read it. >> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> >> Fixes: e52afbd61039 ("iio: light: ROHM BU27034 Ambient Light Sensor") > > This obviously interacts with the regcache update. > > Fun question is whether a register is volatile if it results in all > registers (including itself) resetting. In my view, no it isn't volatile. > So fixing the regcache stuff as in your other patch is more appropriate. Hi Jonathan, I think the key thing here is to ensure writing the reset-bit will always be performed no matter what value is found from cache/hardware. I guess marking the register as volatile is indeed unnecessary, although I don't think it is wrong though, as it underlines we have something special in this register. However, using the write_bits() instead of update_bits() is in my opinion very much "the right thing" to do :) Yours¸ -- Matti > > Jonathan > >> >> --- >> Changelog: >> v1 => v2: >> - Fix SoB tag >> >> >> I haven't verified if the reset bit is self-clearin as I did temporarily >> give away the HW. >> >> In worst case the bit is not self clearing - but we don't really >> get performance penalty even if we set the register volatile because the >> SYSTEM_CONTROL register only has the part-ID and the reset fields. The >> part-id is only read once at probe. >> >> --- >> drivers/iio/light/rohm-bu27034.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/iio/light/rohm-bu27034.c b/drivers/iio/light/rohm-bu27034.c >> index 25c9b79574a5..740ebd86b6e5 100644 >> --- a/drivers/iio/light/rohm-bu27034.c >> +++ b/drivers/iio/light/rohm-bu27034.c >> @@ -231,6 +231,9 @@ struct bu27034_result { >> >> static const struct regmap_range bu27034_volatile_ranges[] = { >> { >> + .range_min = BU27034_REG_SYSTEM_CONTROL, >> + .range_max = BU27034_REG_SYSTEM_CONTROL, >> + }, { >> .range_min = BU27034_REG_MODE_CONTROL4, >> .range_max = BU27034_REG_MODE_CONTROL4, >> }, { >> @@ -1272,7 +1275,7 @@ static int bu27034_chip_init(struct bu27034_data *data) >> int ret, sel; >> >> /* Reset */ >> - ret = regmap_update_bits(data->regmap, BU27034_REG_SYSTEM_CONTROL, >> + ret = regmap_write_bits(data->regmap, BU27034_REG_SYSTEM_CONTROL, >> BU27034_MASK_SW_RESET, BU27034_MASK_SW_RESET); >> if (ret) >> return dev_err_probe(data->dev, ret, "Sensor reset failed\n"); >> >> base-commit: 7fcbd72176076c44b47e8f68f0223c02c411f420 >
On Sun, 7 May 2023 12:58:52 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 5/6/23 21:27, Jonathan Cameron wrote: > > On Wed, 3 May 2023 13:01:32 +0300 > > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > > > >> The reset bit must be always written to the hardware no matter what value > >> is in a cache or register. Ensure this by using regmap_write_bits() > >> instead of the regmap_update_bits(). Furthermore, the RESET bit may be > >> self-clearing, so mark the SYSTEM_CONTROL register volatile to guarantee > >> we do also read the right state - should we ever need to read it. > >> > >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > >> Fixes: e52afbd61039 ("iio: light: ROHM BU27034 Ambient Light Sensor") > > > > This obviously interacts with the regcache update. > > > > Fun question is whether a register is volatile if it results in all > > registers (including itself) resetting. In my view, no it isn't volatile. > > So fixing the regcache stuff as in your other patch is more appropriate. > > Hi Jonathan, > > I think the key thing here is to ensure writing the reset-bit will > always be performed no matter what value is found from cache/hardware. I > guess marking the register as volatile is indeed unnecessary, although I > don't think it is wrong though, as it underlines we have something > special in this register. However, using the write_bits() instead of > update_bits() is in my opinion very much "the right thing" to do :) It's a reasonable change, but whether it's fixing a bug is more complex. If we handle the cache correctly so it always says the bits need writing then there will be no difference between update_bits() and write_bits(). Meh, better safe than sorry. Jonathan > > Yours¸ > -- Matti > > > > > Jonathan > > > >> > >> --- > >> Changelog: > >> v1 => v2: > >> - Fix SoB tag > >> > >> > >> I haven't verified if the reset bit is self-clearin as I did temporarily > >> give away the HW. > >> > >> In worst case the bit is not self clearing - but we don't really > >> get performance penalty even if we set the register volatile because the > >> SYSTEM_CONTROL register only has the part-ID and the reset fields. The > >> part-id is only read once at probe. > >> > >> --- > >> drivers/iio/light/rohm-bu27034.c | 5 ++++- > >> 1 file changed, 4 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/iio/light/rohm-bu27034.c b/drivers/iio/light/rohm-bu27034.c > >> index 25c9b79574a5..740ebd86b6e5 100644 > >> --- a/drivers/iio/light/rohm-bu27034.c > >> +++ b/drivers/iio/light/rohm-bu27034.c > >> @@ -231,6 +231,9 @@ struct bu27034_result { > >> > >> static const struct regmap_range bu27034_volatile_ranges[] = { > >> { > >> + .range_min = BU27034_REG_SYSTEM_CONTROL, > >> + .range_max = BU27034_REG_SYSTEM_CONTROL, > >> + }, { > >> .range_min = BU27034_REG_MODE_CONTROL4, > >> .range_max = BU27034_REG_MODE_CONTROL4, > >> }, { > >> @@ -1272,7 +1275,7 @@ static int bu27034_chip_init(struct bu27034_data *data) > >> int ret, sel; > >> > >> /* Reset */ > >> - ret = regmap_update_bits(data->regmap, BU27034_REG_SYSTEM_CONTROL, > >> + ret = regmap_write_bits(data->regmap, BU27034_REG_SYSTEM_CONTROL, > >> BU27034_MASK_SW_RESET, BU27034_MASK_SW_RESET); > >> if (ret) > >> return dev_err_probe(data->dev, ret, "Sensor reset failed\n"); > >> > >> base-commit: 7fcbd72176076c44b47e8f68f0223c02c411f420 > > >
diff --git a/drivers/iio/light/rohm-bu27034.c b/drivers/iio/light/rohm-bu27034.c index 25c9b79574a5..740ebd86b6e5 100644 --- a/drivers/iio/light/rohm-bu27034.c +++ b/drivers/iio/light/rohm-bu27034.c @@ -231,6 +231,9 @@ struct bu27034_result { static const struct regmap_range bu27034_volatile_ranges[] = { { + .range_min = BU27034_REG_SYSTEM_CONTROL, + .range_max = BU27034_REG_SYSTEM_CONTROL, + }, { .range_min = BU27034_REG_MODE_CONTROL4, .range_max = BU27034_REG_MODE_CONTROL4, }, { @@ -1272,7 +1275,7 @@ static int bu27034_chip_init(struct bu27034_data *data) int ret, sel; /* Reset */ - ret = regmap_update_bits(data->regmap, BU27034_REG_SYSTEM_CONTROL, + ret = regmap_write_bits(data->regmap, BU27034_REG_SYSTEM_CONTROL, BU27034_MASK_SW_RESET, BU27034_MASK_SW_RESET); if (ret) return dev_err_probe(data->dev, ret, "Sensor reset failed\n");