[v2,5/8] rtc: isl12022: implement RTC_VL_READ ioctl

  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(+)
  

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!