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(+)
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,
};
--
2.37.2On 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;
> + }
> +}
--
With Best Regards,
Andy Shevchenko
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
>
>
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
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; -- With Best Regards, Andy Shevchenko
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 > > -- Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
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; -- With Best Regards, Andy Shevchenko
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! -- With Best Regards, Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.