Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx> 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
