Message ID | 20230613130011.305589-6-linux@rasmusvillemoes.dk |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp542161vqr; Tue, 13 Jun 2023 06:24:47 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ7PtQlyvyCazctX43TO+ezz6D34kFy5D8Vk9U+GopBpl71wHiQ9Fq4nUrtqIrOKKAI9cPG+ X-Received: by 2002:a17:903:244e:b0:1b3:c7c7:74ae with SMTP id l14-20020a170903244e00b001b3c7c774aemr6596724pls.45.1686662686739; Tue, 13 Jun 2023 06:24:46 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1686662686; cv=none; d=google.com; s=arc-20160816; b=sri3z3IMRHrlDgX0Diou5aYsuNwOEy0K9yeM4ElFWQGNn449rY4L0C74DFSD+JNhiS AG2NpUDDm/GlUuAcgjAi0a2eOK7SMQeqb7mzQyOosRAKWmCHoY+RdewLzyln490jNqDP Oh3aE5B4M75ZkDb09e64YnikXaXtro8F46GbgZY1RxUsDbS5VhU55UzT3E7sOosVjRJn kW0DCzcsjIHZHRGT/utpTGgz0dJdAP59DGWvPNv99QiCmnOFI1NrxQ+sQx9xMoYF5Bzr /Q8FDc7giueMQzS9eA/tKc6SaIRmTSZmQRmGgf7DBUVNRN2kh97WFksp0bqgdqpMJO3M cW0Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=ur6n0qRjGzRHDGudKm3FuB0Bjxk6aZigQtAmr4SnRcc=; b=VHY4i4W96wY5gro898QKe0/qgZM3+RsD771J9nzXmujpHi/x7K08+4Gh1UT5zsOHPh oaUtJeaf5UfTyIR8ZA7S/deFc8eHMfpMCqcqPELBg1VYPfHpYhZOLbNEsQN3XAUYmT5J qL7gOnAtaYis0StqUQuqr5jdR1jEOZ13+g265sRymmnt+NPYmwfkKzsTBZRa0J5cgX12 0bQb+wfkS6IA51q5JZyJ5HtrG2ARELdaAk8iO0woYzPdJ7LnyN1opiR7mEbqBcGiV8yN eBVUaTEf0uImRgXIhLfAioEoMAxJ7utlha0D0gFNRenFpF8kXXIX59bg4MQxGw3uzoHy aYXQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@rasmusvillemoes.dk header.s=google header.b=TfQ+si9z; 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 Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id b18-20020a170903229200b001b248529a69si5769980plh.92.2023.06.13.06.24.31; Tue, 13 Jun 2023 06:24:46 -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=@rasmusvillemoes.dk header.s=google header.b=TfQ+si9z; 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 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S242537AbjFMNBd (ORCPT <rfc822;lekhanya01809@gmail.com> + 99 others); Tue, 13 Jun 2023 09:01:33 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55044 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S242477AbjFMNBL (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 13 Jun 2023 09:01:11 -0400 Received: from mail-lf1-x12c.google.com (mail-lf1-x12c.google.com [IPv6:2a00:1450:4864:20::12c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1E798184 for <linux-kernel@vger.kernel.org>; Tue, 13 Jun 2023 06:01:10 -0700 (PDT) Received: by mail-lf1-x12c.google.com with SMTP id 2adb3069b0e04-4f6370ddd27so6694434e87.0 for <linux-kernel@vger.kernel.org>; Tue, 13 Jun 2023 06:01:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rasmusvillemoes.dk; s=google; t=1686661268; x=1689253268; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=ur6n0qRjGzRHDGudKm3FuB0Bjxk6aZigQtAmr4SnRcc=; b=TfQ+si9zfMC1c2VVHNiGoJFdzNbiFiqOhy0k/qv87c7AZskvHQZAcsGE75D7fsI6UT xpU3MM8/fRLVZ8Z4hxL2fRx2LCGrN0vo2xp2CM84wCAUrxlpYIzH/Tb3e54Dvr5T6XOQ uPMvUj+0EbrFgQNI6pinpXztygCPyyh95V7HE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686661268; x=1689253268; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=ur6n0qRjGzRHDGudKm3FuB0Bjxk6aZigQtAmr4SnRcc=; b=ZIupDFnjBJ+f9rhIE1+E4B2Qd+cebDilVj+TrXWVVRP3KMQxtXJImWD1UmEDIAH/ue MaMQn7JYU2ZidnvhjzKpmxTIWhRATaqOcN3OKmGW0NdV+kBrs1qCnxlyYc/6QdIvrKIG 0Ya7mtXfU9EokGMs2Q35OVAgyyI9nMSEW4sDmRgCJGLuWyhKH9bsYNANPWKqqcQSGnm6 LEXnIDSF5F9zKyrrxHP6s1Tdm36dRFc1F8L42tM/i28GEeI8bRm1u3zA2glZS8Fz9rSk rNdRACxTpOhtDktWW59Z+AlQA2X6KqX6MfHAtJDN6EcL2Jl7SUD3cW8AClNDkVxt8Hz2 yJTw== X-Gm-Message-State: AC+VfDyaZ7XQrBf8SFsIQyDhjg4Q7wIDQQEHDA79ENIBj6rSN4iAQ/J+ KTOKtMATff1poJnh7vkG7Pvqth8+cxl7WwQVWLQsVg== X-Received: by 2002:a19:6756:0:b0:4f1:26f5:77fb with SMTP id e22-20020a196756000000b004f126f577fbmr6027137lfj.28.1686661268430; Tue, 13 Jun 2023 06:01:08 -0700 (PDT) Received: from prevas-ravi.prevas.se ([81.216.59.226]) by smtp.gmail.com with ESMTPSA id u24-20020ac243d8000000b004f14ae5ded8sm1793786lfl.28.2023.06.13.06.01.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 13 Jun 2023 06:01:08 -0700 (PDT) From: Rasmus Villemoes <linux@rasmusvillemoes.dk> To: Alessandro Zummo <a.zummo@towertech.it>, Alexandre Belloni <alexandre.belloni@bootlin.com> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>, devicetree@vger.kernel.org, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, linux-rtc@vger.kernel.org, Rasmus Villemoes <linux@rasmusvillemoes.dk>, linux-kernel@vger.kernel.org Subject: [PATCH v2 5/8] rtc: isl12022: implement RTC_VL_READ ioctl Date: Tue, 13 Jun 2023 15:00:07 +0200 Message-Id: <20230613130011.305589-6-linux@rasmusvillemoes.dk> X-Mailer: git-send-email 2.37.2 In-Reply-To: <20230613130011.305589-1-linux@rasmusvillemoes.dk> References: <20230612113059.247275-1-linux@rasmusvillemoes.dk> <20230613130011.305589-1-linux@rasmusvillemoes.dk> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED 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?1768594013468136256?= X-GMAIL-MSGID: =?utf-8?q?1768594013468136256?= |
Series |
rtc: isl12022: battery backup voltage and clock support
|
|
Commit Message
Rasmus Villemoes
June 13, 2023, 1 p.m. UTC
Hook up support for reading the values of the SR_LBAT85 and SR_LBAT75
bits. Translate the former to "battery low", and the latter to
"battery empty or not-present".
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
drivers/rtc/rtc-isl12022.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
Comments
On Tue, Jun 13, 2023 at 03:00:07PM +0200, Rasmus Villemoes wrote: > Hook up support for reading the values of the SR_LBAT85 and SR_LBAT75 > bits. Translate the former to "battery low", and the latter to > "battery empty or not-present". A couple of nit-picks below. ... > +static int isl12022_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long arg) > +{ > + struct regmap *regmap = dev_get_drvdata(dev); > + u32 user = 0, val; This assignment can be done in the actual case below. > + int ret; > + > + switch (cmd) { > + case RTC_VL_READ: > + ret = regmap_read(regmap, ISL12022_REG_SR, &val); > + if (ret < 0) I always feel uneasy with ' < 0' — Does positive error makes sense? Is it even possible? OTOH if the entire driver uses this idiom... oh well, let's make it consistent. > + return ret; user = 0; The rationale to avoid potential mistakes in the future in case this function will be expanded and user will be re-used. > + if (val & ISL12022_SR_LBAT85) > + user |= RTC_VL_BACKUP_LOW; > + > + if (val & ISL12022_SR_LBAT75) > + user |= RTC_VL_BACKUP_EMPTY; > + > + return put_user(user, (u32 __user *)arg); > + > + default: > + return -ENOIOCTLCMD; > + } > +}
On 13/06/2023 18:20:56+0300, Andy Shevchenko wrote: > On Tue, Jun 13, 2023 at 03:00:07PM +0200, Rasmus Villemoes wrote: > > Hook up support for reading the values of the SR_LBAT85 and SR_LBAT75 > > bits. Translate the former to "battery low", and the latter to > > "battery empty or not-present". > > A couple of nit-picks below. > > ... > > > +static int isl12022_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long arg) > > +{ > > + struct regmap *regmap = dev_get_drvdata(dev); > > + u32 user = 0, val; > > This assignment can be done in the actual case below. > > > + int ret; > > + > > + switch (cmd) { > > + case RTC_VL_READ: > > + ret = regmap_read(regmap, ISL12022_REG_SR, &val); > > + if (ret < 0) > > I always feel uneasy with ' < 0' — Does positive error makes sense? > Is it even possible? OTOH if the entire driver uses this idiom... > oh well, let's make it consistent. > /** * regmap_read() - Read a value from a single register * * @map: Register map to read from * @reg: Register to be read from * @val: Pointer to store read value * * A value of zero will be returned on success, a negative errno will * be returned in error cases. */ > > + return ret; > > user = 0; > > The rationale to avoid potential mistakes in the future in case this function > will be expanded and user will be re-used. > > > + if (val & ISL12022_SR_LBAT85) > > + user |= RTC_VL_BACKUP_LOW; > > + > > + if (val & ISL12022_SR_LBAT75) > > + user |= RTC_VL_BACKUP_EMPTY; > > + > > + return put_user(user, (u32 __user *)arg); > > + > > + default: > > + return -ENOIOCTLCMD; > > + } > > +} > > -- > With Best Regards, > Andy Shevchenko > >
On Tue, Jun 13, 2023 at 11:26:51PM +0200, Alexandre Belloni wrote: > On 13/06/2023 18:20:56+0300, Andy Shevchenko wrote: > > On Tue, Jun 13, 2023 at 03:00:07PM +0200, Rasmus Villemoes wrote: ... > > > + ret = regmap_read(regmap, ISL12022_REG_SR, &val); > > > + if (ret < 0) > > > > I always feel uneasy with ' < 0' — Does positive error makes sense? > > Is it even possible? OTOH if the entire driver uses this idiom... > > oh well, let's make it consistent. > > /** > * regmap_read() - Read a value from a single register > * > * @map: Register map to read from > * @reg: Register to be read from > * @val: Pointer to store read value > * > * A value of zero will be returned on success, a negative errno will > * be returned in error cases. > */ I'm not sure what you meant by this. Yes, I know that there is no possibility that regmap API returns positive value. Do you mean that regmap API documentation is unclear? > > > + return ret;
On 14/06/2023 15:16:14+0300, Andy Shevchenko wrote: > On Tue, Jun 13, 2023 at 11:26:51PM +0200, Alexandre Belloni wrote: > > On 13/06/2023 18:20:56+0300, Andy Shevchenko wrote: > > > On Tue, Jun 13, 2023 at 03:00:07PM +0200, Rasmus Villemoes wrote: > > ... > > > > > + ret = regmap_read(regmap, ISL12022_REG_SR, &val); > > > > + if (ret < 0) > > > > > > I always feel uneasy with ' < 0' — Does positive error makes sense? > > > Is it even possible? OTOH if the entire driver uses this idiom... > > > oh well, let's make it consistent. > > > > /** > > * regmap_read() - Read a value from a single register > > * > > * @map: Register map to read from > > * @reg: Register to be read from > > * @val: Pointer to store read value > > * > > * A value of zero will be returned on success, a negative errno will > > * be returned in error cases. > > */ > > I'm not sure what you meant by this. Yes, I know that there is no > possibility that regmap API returns positive value. Do you mean that > regmap API documentation is unclear? No, I mean that you'd have to be clearer as to why you are uneasy with a test for a negative value when the function returns 0 for success and a negative value for an error. Else, this is pure bullying. > > > > > + return ret; > > -- > With Best Regards, > Andy Shevchenko > >
On Wed, Jun 14, 2023 at 03:50:36PM +0200, Alexandre Belloni wrote: > On 14/06/2023 15:16:14+0300, Andy Shevchenko wrote: > > On Tue, Jun 13, 2023 at 11:26:51PM +0200, Alexandre Belloni wrote: > > > On 13/06/2023 18:20:56+0300, Andy Shevchenko wrote: > > > > On Tue, Jun 13, 2023 at 03:00:07PM +0200, Rasmus Villemoes wrote: ... > > > > > + ret = regmap_read(regmap, ISL12022_REG_SR, &val); > > > > > + if (ret < 0) > > > > > > > > I always feel uneasy with ' < 0' — Does positive error makes sense? > > > > Is it even possible? OTOH if the entire driver uses this idiom... > > > > oh well, let's make it consistent. > > > > > > /** > > > * regmap_read() - Read a value from a single register > > > * > > > * @map: Register map to read from > > > * @reg: Register to be read from > > > * @val: Pointer to store read value > > > * > > > * A value of zero will be returned on success, a negative errno will > > > * be returned in error cases. > > > */ > > > > I'm not sure what you meant by this. Yes, I know that there is no > > possibility that regmap API returns positive value. Do you mean that > > regmap API documentation is unclear? > > No, I mean that you'd have to be clearer as to why you are uneasy with a > test for a negative value when the function returns 0 for success and a > negative value for an error. Else, this is pure bullying. From the perspective of the code reader, a person, who might have not known all the implementation details of the calls this kind of check will always puzzle about positive value. When reading such a code the following questions are arisen: 1) Can the positive return value be the case? 2) If so, what is the meaning of a such? 3) Why do we not care about it? All this can simply gone if we use ret = foo(...); if (ret) return ret; As it's clear that whatever is non-zero we accept as something to be promoted to the upper layer. I hope this explains my position. > > > > > + return ret;
On 14/06/2023 17.13, Andy Shevchenko wrote: > When reading such a code the following questions are arisen: > 1) Can the positive return value be the case? > 2) If so, what is the meaning of a such? > 3) Why do we not care about it? > > All this can simply gone if we use > > ret = foo(...); > if (ret) > return ret; > > As it's clear that whatever is non-zero we accept as something to be promoted > to the upper layer. I hope this explains my position. But we're in a context (in this case an ->ioctl method) where _our_ caller expects 0/-ESOMETHING, so returning something positive would be a bug - i.e., either way of spelling that if(), the reader must know that foo() also has those 0/-ESOMETHING semantics. I honestly didn't think much about it, but looking at the existing code and the stuff I add, all other places actually do 'if (ret)', so I've updated this site for consistency. Rasmus
On Thu, Jun 15, 2023 at 12:53:24PM +0200, Rasmus Villemoes wrote: > On 14/06/2023 17.13, Andy Shevchenko wrote: > > When reading such a code the following questions are arisen: > > 1) Can the positive return value be the case? > > 2) If so, what is the meaning of a such? > > 3) Why do we not care about it? > > > > All this can simply gone if we use > > > > ret = foo(...); > > if (ret) > > return ret; > > > > As it's clear that whatever is non-zero we accept as something to be promoted > > to the upper layer. I hope this explains my position. > > But we're in a context (in this case an ->ioctl method) where _our_ > caller expects 0/-ESOMETHING, so returning something positive would be a > bug - i.e., either way of spelling that if(), the reader must know that > foo() also has those 0/-ESOMETHING semantics. I totally understand this. But then it's either problem of regmap API documentation or API itself. I.o.w. not _your_ problem. > I honestly didn't think much about it, but looking at the existing code > and the stuff I add, all other places actually do 'if (ret)', so I've > updated this site for consistency. Thank you!
diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c index 50bbd1fefad8..bf0d65643897 100644 --- a/drivers/rtc/rtc-isl12022.c +++ b/drivers/rtc/rtc-isl12022.c @@ -204,7 +204,33 @@ static int isl12022_rtc_set_time(struct device *dev, struct rtc_time *tm) return regmap_bulk_write(regmap, ISL12022_REG_SC, buf, sizeof(buf)); } +static int isl12022_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long arg) +{ + struct regmap *regmap = dev_get_drvdata(dev); + u32 user = 0, val; + int ret; + + switch (cmd) { + case RTC_VL_READ: + ret = regmap_read(regmap, ISL12022_REG_SR, &val); + if (ret < 0) + return ret; + + if (val & ISL12022_SR_LBAT85) + user |= RTC_VL_BACKUP_LOW; + + if (val & ISL12022_SR_LBAT75) + user |= RTC_VL_BACKUP_EMPTY; + + return put_user(user, (u32 __user *)arg); + + default: + return -ENOIOCTLCMD; + } +} + static const struct rtc_class_ops isl12022_rtc_ops = { + .ioctl = isl12022_rtc_ioctl, .read_time = isl12022_rtc_read_time, .set_time = isl12022_rtc_set_time, };