Re: [PATCH 2/2] rtc: isl12022: Add alarm support

[Esben Haabendal <esben@geanix.com>]

Alexandre Belloni <alexandre.belloni@bootlin.com> writes:

> Hello Esben,
>
> On 10/09/2024 12:27:11+0200, Esben Haabendal wrote:
>> The ISL12022 RTC has a combined INT/fOUT pin, which can be used for alarm
>> interrupt when frequency output is not enabled.
>> 
>> The device-tree bindings should ensure that interrupt and clock output is
>> not enabled at the same time.
>
> Ideally, we would get a pinmuxing part in the driver to ensure this ;)

I hope we can leave this as future work :)

>> +static int isl12022_rtc_read_alarm(struct device *dev,
>> +				   struct rtc_wkalrm *alarm)
>> +{
>> +	struct rtc_time *const tm = &alarm->time;
>> +	struct isl12022 *isl12022 = dev_get_drvdata(dev);
>> +	struct regmap *regmap = isl12022->regmap;
>> +	uint8_t buf[ISL12022_ALARM_SECTION_LEN];
>> +	int ret, yr, i;
>> +
>> +	ret = regmap_bulk_read(regmap, ISL12022_ALARM_SECTION,
>> +			       buf, sizeof(buf));
>> +	if (ret) {
>> +		dev_err(dev, "%s: reading ALARM registers failed\n",
>> +			__func__);
>
> I don't really like those error messages because there is nothing the
> user can actually do apart from trying again and this bloats the
> kernel.

Ok. Maybe keep it as dev_dbg() then?
>
>> +	/* The alarm doesn't store the year so get it from the rtc section */
>> +	ret = regmap_read(regmap, ISL12022_REG_YR, &yr);
>> +	if (ret) {
>> +		dev_err(dev, "%s: reading YR register failed\n", __func__);
>
> Ditto

Ditto.

>> +	isl12022->rtc = rtc;
>>  
>>  	rtc->ops = &isl12022_rtc_ops;
>>  	rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
>>  	rtc->range_max = RTC_TIMESTAMP_END_2099;
>>  
>> +	if (client->irq > 0) {
>> +		ret = isl12022_setup_irq(isl12022, client->irq);
>
> You can't do this in probe, the RTC lifecycle is longer than the linux
> system. Or said differently: "oh no, my linux has rebooted and now I
> lost my future alarm" ;)

Oh.

We do need to setup the irq here, so I assume you mean I need to drop
the part of _setup_irq() that clears alarm registers.

And I guess we need to enable irq in probe as well. At least if/when an
alarm is set. I think it should be safe to enable irq unconditionally in
_probe()...

>> +		if (ret)
>> +			return ret;
>> +	} else {
>> +		clear_bit(RTC_FEATURE_ALARM, rtc->features);
>> +	}
>> +
>>  	return devm_rtc_register_device(rtc);
>>  }

/Esben