| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20251116-51715-3266073@bhairav-test.ee.iitb.ac.in>
Date: Sun, 16 Nov 2025 10:47:15 +0530
From: Akhilesh Patil <akhilesh@...iitb.ac.in>
To: Alexandre Belloni <alexandre.belloni@...tlin.com>
Cc: andriy.shevchenko@...el.com, david.daney@...ium.com,
ddaney@...iumnetworks.com, david.hunter.linux@...il.com,
skhan@...uxfoundation.org, linux-rtc@...r.kernel.org,
linux-kernel@...r.kernel.org, akhileshpatilvnit@...il.com
Subject: Re: [PATCH] rtc: isl12026: Implement callbacks for alarm feature
On Sat, Nov 15, 2025 at 12:24:21PM +0100, Alexandre Belloni wrote:
> On 15/11/2025 16:35:06+0530, Akhilesh Patil wrote:
> > Add alarm support for isl12026 RTC. Implement alarm function rtc
> > class callbacks - set_alarm, read_alarm and alarm_irq_enable.
> > isl12026 rtc has 2 alarms, this patch adds support to configure alarm0.
> > Note: isl12026 rtc chip share same pin(4) for alarm interrupt and square
> > wave frequency generator, hence forcefully disable SQW functionality
> > while writing to device registers in alarm functions.
> >
> > Tested on TI am62x sk board on i2c-2 port using selftests/rtc/rtctest
> >
> > Signed-off-by: Akhilesh Patil <akhilesh@...iitb.ac.in>
> > ---
> > Datasheet of RTC chip.
> > https://www.renesas.com/en/document/dst/isl12026-isl12026a-datasheet?srsltid=AfmBOopgN4vtn8XoN-8sOCfTW6yiLH-T7eeH_IWakqZ2VmENmWFqqh7w
> >
> > drivers/rtc/rtc-isl12026.c | 127 +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 127 insertions(+)
> >
> > diff --git a/drivers/rtc/rtc-isl12026.c b/drivers/rtc/rtc-isl12026.c
> > index 2aabb9151d4c..7fa9ec7e4929 100644
> > --- a/drivers/rtc/rtc-isl12026.c
> > +++ b/drivers/rtc/rtc-isl12026.c
> > @@ -34,6 +34,11 @@
> > #define ISL12026_PAGESIZE 16
> > #define ISL12026_NVMEM_WRITE_TIME 20
Hi Alexandre, Thanks for the review.
Please find my comments below.
[...]
> >
> > +static int isl12026_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + int ret;
> > + u8 buf_alrm_vals[7];
> > + struct i2c_msg msg;
> > + int ir;
> > +
> > + msg.addr = client->addr;
> > + msg.flags = 0x0; /* Write operation */
> > + msg.buf = buf_alrm_vals;
> > + msg.len = sizeof(buf_alrm_vals);
> > +
> > + if (!alrm->enabled) {
> > + /* Disable alarm and return */
>
> You still need to set the alarm time and date properly, the application
> is allowed to first set the alarm and then enable it.
Sure. Understood.
Will fix this in v2.
>
> > + ir = isl12026_read_reg(client, ISL12026_REG_INT);
> > + if (ir < 0)
> > + return ir;
> > + ir &= ~ISL12026_AL0E;
[...]
> > +
> > + /* Non-volatile Page write to AL0 registers and enable INT */
> > + ret = isl12026_arm_write(client);
> > + if (ret < 0)
> > + return ret;
> > + ret = i2c_transfer(client->adapter, &msg, 1);
> > + msleep(ISL12026_NVMEM_WRITE_TIME);
> > + if (ret != 1) {
> > + dev_err(&client->dev, "Error writing to alarm registers\n");
>
> No need for an error message.
ACK.
Will remove error message and just return error.
>
> > + return ret < 0 ? ret : -EIO;
> > + }
> > +
> > + /* Enable AL0 interrupt */
> > + ret = isl12026_write_reg(client, ISL12026_REG_INT, ISL12026_AL0E);
> > +
> > + return ret;
> > +}
> > +
> > +static int isl12026_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + int ret;
> > + int sr, ir;
> > + u8 buf_alrm_vals[5];
> > + u8 addr[2] = {0x0, ISL12026_AL0_REG_SC};
> > + struct i2c_msg msgs[2] = { };
> > +
[...]
> > + alrm->pending = !!(sr & ISL12026_SR_AL0) && alrm->enabled;
> > +
> > + /* Page read for alarm registers */
> > + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > + if (ret != ARRAY_SIZE(msgs)) {
> > + dev_err(&client->dev, "Error reading alarm registers\n");
>
> Ditto
ACK. Will remove err message.
>
> > + return ret < 0 ? ret : -EIO;
> > + }
> > +
> > + /* Populate values read */
> > + alrm->time.tm_sec = bcd2bin(buf_alrm_vals[0] & 0x7f);
> > + alrm->time.tm_min = bcd2bin(buf_alrm_vals[1] & 0x7f);
> > + alrm->time.tm_hour = bcd2bin(buf_alrm_vals[2] & 0x3f);
> > + alrm->time.tm_mday = bcd2bin(buf_alrm_vals[3] & 0x3f);
> > + alrm->time.tm_mon = bcd2bin(buf_alrm_vals[4] & 0x1f) - 1;
> > +
> > + return 0;
> > +}
> > +
> > +static int isl12026_rtc_alarm_irq_en(struct device *dev, unsigned int enabled)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + int ret;
> > + int ir;
> > +
> > + if (enabled) {
> > + ret = isl12026_write_reg(client, ISL12026_REG_INT, ISL12026_AL0E);
> > + return ret;
> > + }
> > +
> > + /* Disable alarm */
> > + ir = isl12026_read_reg(client, ISL12026_REG_INT);
> > + if (ir < 0)
> > + return ir;
> > + ir &= ~ISL12026_AL0E;
> > + ret = isl12026_write_reg(client, ISL12026_REG_INT, ir);
> > +
> > + return ret;
> > +}
> > +
> > static const struct rtc_class_ops isl12026_rtc_ops = {
> > .read_time = isl12026_rtc_read_time,
> > .set_time = isl12026_rtc_set_time,
> > + .set_alarm = isl12026_rtc_set_alarm,
> > + .read_alarm = isl12026_rtc_read_alarm,
> > + .alarm_irq_enable = isl12026_rtc_alarm_irq_en,
> > };
> >
>
> This is missing an interrupt handler and proper handling in probe for
> the wakeup-source property as this seems to be how you use it.
Agree. However, I thought of first implementing alarm callbacks only and
test them independedntly using ioctls for alarm settings in this patch.
I will add interrupt handler and wakeup-source in v2 to complete this
functionality.
Regards,
Akhilesh
>
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Powered by blists - more mailing lists