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;
Message-ID: <76f0f62d-e53f-8708-e8a8-f71777a95f21@rasmusvillemoes.dk>
Date:   Tue, 13 Jun 2023 09:27:09 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH 5/8] rtc: isl12022: implement RTC_VL_READ and RTC_VL_CLR
 ioctls
Content-Language: en-US, da
To:     Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc:     Alessandro Zummo <a.zummo@towertech.it>,
        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,
        linux-kernel@vger.kernel.org
References: <20230612113059.247275-1-linux@rasmusvillemoes.dk>
 <20230612113059.247275-6-linux@rasmusvillemoes.dk>
 <2023061214074623dcc0cf@mail.local>
From:   Rasmus Villemoes <linux@rasmusvillemoes.dk>
In-Reply-To: <2023061214074623dcc0cf@mail.local>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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