[5/8] rtc: isl12022: implement RTC_VL_READ and RTC_VL_CLR ioctls

  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 | 41 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)
  

On 12/06/2023 13:30:55+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".
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  drivers/rtc/rtc-isl12022.c | 41 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c
> index cb8f1d92e116..1b6659a9b33a 100644
> --- a/drivers/rtc/rtc-isl12022.c
> +++ b/drivers/rtc/rtc-isl12022.c
> @@ -203,7 +203,48 @@ 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_read_sr(struct regmap *regmap)
> +{
> +	int ret;
> +	u32 val;
> +
> +	ret = regmap_read(regmap, ISL12022_REG_SR, &val);
> +	if (ret < 0)
> +		return ret;
> +	return val;
> +}
> +
> +static int isl12022_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
> +{
> +	struct regmap *regmap = dev_get_drvdata(dev);
> +	u32 user = 0;
> +	int ret;
> +
> +	switch (cmd) {
> +	case RTC_VL_READ:
> +		ret = isl12022_read_sr(regmap);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (ret & ISL12022_SR_LBAT85)
> +			user |= RTC_VL_BACKUP_LOW;
> +
> +		if (ret & ISL12022_SR_LBAT75)
> +			user |= RTC_VL_BACKUP_EMPTY;
> +
> +		return put_user(user, (u32 __user *)arg);
> +
> +	case RTC_VL_CLR:
> +		return regmap_clear_bits(regmap, ISL12022_REG_SR,
> +					 ISL12022_SR_LBAT85 | ISL12022_SR_LBAT75);

I'm against using RTC_VL_CLR for this as it deletes important
information (i.e. the date is probably invalid). You should let the RTC
clear the bits once the battery has been changed:

"The LBAT75 bit is set when the
VBAT has dropped below the pre-selected trip level, and will self
clear when the VBAT is above the pre-selected trip level at the
next detection cycle either by manual or automatic trigger."

> +
> +	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.2
>
  

On Mon, Jun 12, 2023 at 01:30:55PM +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".

...

> +static int isl12022_read_sr(struct regmap *regmap)
> +{
> +	int ret;
> +	u32 val;
> +
> +	ret = regmap_read(regmap, ISL12022_REG_SR, &val);
> +	if (ret < 0)
> +		return ret;
> +	return val;

Wondering if the bit 31 is in use with this register (note, I haven't checked
the register width nor datasheet).

> +}
  

On 12/06/2023 18:48:49+0300, Andy Shevchenko wrote:
> On Mon, Jun 12, 2023 at 01:30:55PM +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".
> 
> ...
> 
> > +static int isl12022_read_sr(struct regmap *regmap)
> > +{
> > +	int ret;
> > +	u32 val;
> > +
> > +	ret = regmap_read(regmap, ISL12022_REG_SR, &val);
> > +	if (ret < 0)
> > +		return ret;
> > +	return val;
> 
> Wondering if the bit 31 is in use with this register (note, I haven't checked
> the register width nor datasheet).
> 

register width is in the driver:

static const struct regmap_config regmap_config = {
	.reg_bits = 8,
	.val_bits = 8,
	.use_single_write = true,
};


> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>
  

On 12/06/2023 16.07, Alexandre Belloni wrote:
> On 12/06/2023 13:30:55+0200, Rasmus Villemoes wrote:

>> +static int isl12022_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
>> +{
>> +	struct regmap *regmap = dev_get_drvdata(dev);
>> +	u32 user = 0;
>> +	int ret;
>> +
>> +	switch (cmd) {
>> +	case RTC_VL_READ:
>> +		ret = isl12022_read_sr(regmap);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		if (ret & ISL12022_SR_LBAT85)
>> +			user |= RTC_VL_BACKUP_LOW;
>> +
>> +		if (ret & ISL12022_SR_LBAT75)
>> +			user |= RTC_VL_BACKUP_EMPTY;
>> +
>> +		return put_user(user, (u32 __user *)arg);
>> +
>> +	case RTC_VL_CLR:
>> +		return regmap_clear_bits(regmap, ISL12022_REG_SR,
>> +					 ISL12022_SR_LBAT85 | ISL12022_SR_LBAT75);
> 
> I'm against using RTC_VL_CLR for this as it deletes important
> information (i.e. the date is probably invalid). You should let the RTC
> clear the bits once the battery has been changed:
> 
> "The LBAT75 bit is set when the
> VBAT has dropped below the pre-selected trip level, and will self
> clear when the VBAT is above the pre-selected trip level at the
> next detection cycle either by manual or automatic trigger."

Well, the same thing means that the bit would get set again within a
minute after the RTC_VL_CLR, so the information isn't lost as such. I
actually don't understand what RTC_VL_CLR would be for if not this
(though, again, in this case at least it would only have a very
short-lived effect), but I'm perfectly happy to just rip out the
RTC_VL_CLR case.

Rasmus
  

On 12/06/2023 18.10, Alexandre Belloni wrote:
> On 12/06/2023 18:48:49+0300, Andy Shevchenko wrote:
>> On Mon, Jun 12, 2023 at 01:30:55PM +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".
>>
>> ...
>>
>>> +static int isl12022_read_sr(struct regmap *regmap)
>>> +{
>>> +	int ret;
>>> +	u32 val;
>>> +
>>> +	ret = regmap_read(regmap, ISL12022_REG_SR, &val);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +	return val;
>>
>> Wondering if the bit 31 is in use with this register (note, I haven't checked
>> the register width nor datasheet).
>>
> 
> register width is in the driver:
> 
> static const struct regmap_config regmap_config = {
> 	.reg_bits = 8,
> 	.val_bits = 8,
> 	.use_single_write = true,
> };

Yeah.

But I only factored that out because I wanted to read the SR also in the
isl12022_set_trip_levels() to emit the warning at boot time, but when
that goes away, there's no longer any reason to not just fold this back
into the ioctl() handler.

Rasmus
  

On 13/06/2023 09:53:03+0200, Rasmus Villemoes wrote:
> On 12/06/2023 18.10, Alexandre Belloni wrote:
> > On 12/06/2023 18:48:49+0300, Andy Shevchenko wrote:
> >> On Mon, Jun 12, 2023 at 01:30:55PM +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".
> >>
> >> ...
> >>
> >>> +static int isl12022_read_sr(struct regmap *regmap)
> >>> +{
> >>> +	int ret;
> >>> +	u32 val;
> >>> +
> >>> +	ret = regmap_read(regmap, ISL12022_REG_SR, &val);
> >>> +	if (ret < 0)
> >>> +		return ret;
> >>> +	return val;
> >>
> >> Wondering if the bit 31 is in use with this register (note, I haven't checked
> >> the register width nor datasheet).
> >>
> > 
> > register width is in the driver:
> > 
> > static const struct regmap_config regmap_config = {
> > 	.reg_bits = 8,
> > 	.val_bits = 8,
> > 	.use_single_write = true,
> > };
> 
> Yeah.
> 
> But I only factored that out because I wanted to read the SR also in the
> isl12022_set_trip_levels() to emit the warning at boot time, but when
> that goes away, there's no longer any reason to not just fold this back
> into the ioctl() handler.

That would be to clear a not self clearable battery low (but not empty)
flag or a backup voltage switch flag.

> 
> Rasmus
>