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.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/rtc/rtc-isl12022.c | 244 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 241 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c
index d82278fdc29b..682b1bf10160 100644
--- a/drivers/rtc/rtc-isl12022.c
+++ b/drivers/rtc/rtc-isl12022.c
@@ -21,7 +21,7 @@
 
 #include <asm/byteorder.h>
 
-/* ISL register offsets */
+/* RTC - Real time clock registers */
 #define ISL12022_REG_SC		0x00
 #define ISL12022_REG_MN		0x01
 #define ISL12022_REG_HR		0x02
@@ -30,21 +30,36 @@
 #define ISL12022_REG_YR		0x05
 #define ISL12022_REG_DW		0x06
 
+/* CSR - Control and status registers */
 #define ISL12022_REG_SR		0x07
 #define ISL12022_REG_INT	0x08
-
 #define ISL12022_REG_PWR_VBAT	0x0a
-
 #define ISL12022_REG_BETA	0x0d
+
+/* ALARM - Alarm registers */
+#define ISL12022_REG_SCA0	0x10
+#define ISL12022_REG_MNA0	0x11
+#define ISL12022_REG_HRA0	0x12
+#define ISL12022_REG_DTA0	0x13
+#define ISL12022_REG_MOA0	0x14
+#define ISL12022_REG_DWA0	0x15
+#define ISL12022_ALARM_SECTION		ISL12022_REG_SCA0
+#define ISL12022_ALARM_SECTION_LEN	(ISL12022_REG_DWA0 - ISL12022_REG_SCA0 + 1)
+
+/* TEMP - Temperature sensor registers */
 #define ISL12022_REG_TEMP_L	0x28
 
 /* ISL register bits */
 #define ISL12022_HR_MIL		(1 << 7)	/* military or 24 hour time */
 
+#define ISL12022_SR_ALM		(1 << 4)
 #define ISL12022_SR_LBAT85	(1 << 2)
 #define ISL12022_SR_LBAT75	(1 << 1)
 
+#define ISL12022_INT_ARST	(1 << 7)
 #define ISL12022_INT_WRTC	(1 << 6)
+#define ISL12022_INT_IM		(1 << 5)
+#define ISL12022_INT_FOBATB	(1 << 4)
 #define ISL12022_INT_FO_MASK	GENMASK(3, 0)
 #define ISL12022_INT_FO_OFF	0x0
 #define ISL12022_INT_FO_32K	0x1
@@ -52,10 +67,18 @@
 #define ISL12022_REG_VB85_MASK	GENMASK(5, 3)
 #define ISL12022_REG_VB75_MASK	GENMASK(2, 0)
 
+#define ISL12022_ALARM_ENABLE	(1 << 7)	/* for all ALARM registers  */
+
 #define ISL12022_BETA_TSE	(1 << 7)
 
+static struct i2c_driver isl12022_driver;
+
 struct isl12022 {
+	struct i2c_client *i2c;
+	struct rtc_device *rtc;
 	struct regmap *regmap;
+	int irq;
+	bool irq_enabled;
 };
 
 static umode_t isl12022_hwmon_is_visible(const void *data,
@@ -215,6 +238,208 @@ 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_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__);
+		return ret;
+	}
+
+	dev_dbg(dev,
+		"%s: sc=%02x, mn=%02x, hr=%02x, dt=%02x, mo=%02x, dw=%02x\n",
+		__func__, buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]);
+
+	tm->tm_sec = bcd2bin(buf[ISL12022_REG_SCA0 - ISL12022_ALARM_SECTION]
+			     & 0x7F);
+	tm->tm_min = bcd2bin(buf[ISL12022_REG_MNA0 - ISL12022_ALARM_SECTION]
+			     & 0x7F);
+	tm->tm_hour = bcd2bin(buf[ISL12022_REG_HRA0 - ISL12022_ALARM_SECTION]
+			      & 0x3F);
+	tm->tm_mday = bcd2bin(buf[ISL12022_REG_DTA0 - ISL12022_ALARM_SECTION]
+			      & 0x3F);
+	tm->tm_mon = bcd2bin(buf[ISL12022_REG_MOA0 - ISL12022_ALARM_SECTION]
+			     & 0x1F) - 1;
+	tm->tm_wday = buf[ISL12022_REG_DWA0 - ISL12022_ALARM_SECTION] & 0x07;
+
+	/* 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__);
+		return yr;
+	}
+	tm->tm_year = bcd2bin(yr) + 100;
+
+	for (i = 0 ; i < ISL12022_ALARM_SECTION_LEN ; i++) {
+		if (buf[i] & ISL12022_ALARM_ENABLE) {
+			alarm->enabled = 1;
+			break;
+		}
+	}
+
+	dev_dbg(dev, "%s: %ptR\n", __func__, tm);
+
+	return 0;
+}
+
+static int isl12022_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
+{
+	struct rtc_time *alarm_tm = &alarm->time;
+	struct isl12022 *isl12022 = dev_get_drvdata(dev);
+	struct regmap *regmap = isl12022->regmap;
+	u8 regs[ISL12022_ALARM_SECTION_LEN] = { 0, };
+	struct rtc_time rtc_tm;
+	int ret = 0, enable, dw;
+
+	ret = isl12022_rtc_read_time(dev, &rtc_tm);
+	if (ret)
+		return ret;
+
+	/* If the alarm time is before the current time disable the alarm */
+	if (!alarm->enabled || rtc_tm_sub(alarm_tm, &rtc_tm) <= 0)
+		enable = 0;
+	else
+		enable = ISL12022_ALARM_ENABLE;
+
+	/* Set non-matching tm_wday to safeguard against early false matching
+	 * while setting all the alarm registers (this rtc lacks a general
+	 * alarm/irq enable/disable bit).
+	 */
+	if (enable) {
+		ret = regmap_read(regmap, ISL12022_REG_DW, &dw);
+		if (ret) {
+			dev_err(dev, "%s: reading DW failed\n", __func__);
+			return ret;
+		}
+		/* ~4 days into the future should be enough to avoid match */
+		dw = ((dw + 4) % 7) | ISL12022_ALARM_ENABLE;
+		ret = regmap_write(regmap, ISL12022_REG_DWA0, dw);
+		if (ret) {
+			dev_err(dev, "%s: writing DWA0 failed\n", __func__);
+			return ret;
+		}
+	}
+
+	/* Program the alarm and enable it for each setting */
+	regs[ISL12022_REG_SCA0 - ISL12022_ALARM_SECTION] =
+		bin2bcd(alarm_tm->tm_sec) | enable;
+	regs[ISL12022_REG_MNA0 - ISL12022_ALARM_SECTION] =
+		bin2bcd(alarm_tm->tm_min) | enable;
+	regs[ISL12022_REG_HRA0 - ISL12022_ALARM_SECTION] =
+		bin2bcd(alarm_tm->tm_hour) | enable;
+	regs[ISL12022_REG_DTA0 - ISL12022_ALARM_SECTION] =
+		bin2bcd(alarm_tm->tm_mday) | enable;
+	regs[ISL12022_REG_MOA0 - ISL12022_ALARM_SECTION] =
+		bin2bcd(alarm_tm->tm_mon + 1) | enable;
+	regs[ISL12022_REG_DWA0 - ISL12022_ALARM_SECTION] =
+		bin2bcd(alarm_tm->tm_wday & 7) | enable;
+
+	/* write ALARM registers */
+	ret = regmap_bulk_write(regmap, ISL12022_REG_SCA0,
+				&regs, sizeof(regs));
+	if (ret) {
+		dev_err(dev, "%s: writing ALARM registers failed\n", __func__);
+		return ret;
+	}
+
+	return 0;
+}
+
+static irqreturn_t isl12022_rtc_interrupt(int irq, void *data)
+{
+	struct isl12022 *isl12022 = data;
+	struct rtc_device *rtc = isl12022->rtc;
+	struct device *dev = &rtc->dev;
+	struct regmap *regmap = isl12022->regmap;
+	u32 val = 0;
+	unsigned long events = 0;
+	int ret;
+
+	ret = regmap_read(regmap, ISL12022_REG_SR, &val);
+	if (ret) {
+		dev_err(dev, "%s: reading SR failed\n", __func__);
+		return IRQ_HANDLED;
+	}
+
+	if (val & ISL12022_SR_ALM)
+		events |= RTC_IRQF | RTC_AF;
+
+	if (events & RTC_AF)
+		dev_dbg(dev, "alarm!\n");
+
+	if (!events)
+		return IRQ_NONE;
+
+	rtc_update_irq(rtc, 1, events);
+	return IRQ_HANDLED;
+}
+
+static int isl12022_rtc_alarm_irq_enable(struct device *dev,
+					 unsigned int enabled)
+{
+	struct isl12022 *isl12022 = dev_get_drvdata(dev);
+
+	if (!isl12022->irq_enabled == !enabled)
+		return 0;
+
+	if (enabled)
+		enable_irq(isl12022->irq);
+	else
+		disable_irq(isl12022->irq);
+
+	isl12022->irq_enabled = !!enabled;
+
+	return 0;
+}
+
+static int isl12022_setup_irq(struct isl12022 *isl12022, int irq)
+{
+	struct device *dev = &isl12022->i2c->dev;
+	struct regmap *regmap = isl12022->regmap;
+	unsigned int reg_mask, reg_val;
+	u8 buf[ISL12022_ALARM_SECTION_LEN] = { 0, };
+	int ret;
+
+	/* Clear and disable all alarm registers */
+	ret = regmap_bulk_write(regmap, ISL12022_ALARM_SECTION,
+				buf, sizeof(buf));
+	if (ret)
+		return ret;
+
+	/* Enable automatic reset of ALM bit, enable single event interrupt
+	 * mode, and disable IRQ/fOUT pin during battery-backup mode.
+	 */
+	reg_mask = ISL12022_INT_ARST | ISL12022_INT_IM
+		| ISL12022_INT_FOBATB | ISL12022_INT_FO_MASK;
+	reg_val = ISL12022_INT_ARST | ISL12022_INT_FOBATB | ISL12022_INT_FO_OFF;
+	ret = regmap_write_bits(regmap, ISL12022_REG_INT,
+				reg_mask, reg_val);
+	if (ret)
+		return ret;
+
+	ret = devm_request_threaded_irq(dev, irq, NULL,
+					isl12022_rtc_interrupt,
+					IRQF_SHARED | IRQF_ONESHOT,
+					isl12022_driver.driver.name,
+					isl12022);
+	if (ret) {
+		dev_err(dev, "Unable to request irq %d\n", irq);
+		return ret;
+	}
+
+	isl12022->irq = irq;
+	return 0;
+}
+
 static int isl12022_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
 {
 	struct isl12022 *isl12022 = dev_get_drvdata(dev);
@@ -246,6 +471,9 @@ 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,
+	.read_alarm	= isl12022_rtc_read_alarm,
+	.set_alarm	= isl12022_rtc_set_alarm,
+	.alarm_irq_enable = isl12022_rtc_alarm_irq_enable,
 };
 
 static const struct regmap_config regmap_config = {
@@ -347,6 +575,7 @@ static int isl12022_probe(struct i2c_client *client)
 	isl12022 = devm_kzalloc(&client->dev, sizeof(*isl12022), GFP_KERNEL);
 	if (!isl12022)
 		return -ENOMEM;
+	isl12022->i2c = client;
 
 	regmap = devm_regmap_init_i2c(client, &regmap_config);
 	if (IS_ERR(regmap)) {
@@ -367,11 +596,20 @@ static int isl12022_probe(struct i2c_client *client)
 	rtc = devm_rtc_allocate_device(&client->dev);
 	if (IS_ERR(rtc))
 		return PTR_ERR(rtc);
+	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);
+		if (ret)
+			return ret;
+	} else {
+		clear_bit(RTC_FEATURE_ALARM, rtc->features);
+	}
+
 	return devm_rtc_register_device(rtc);
 }
 

-- 
2.46.0

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 ;)

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

> +	/* 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

> +	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" ;)


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

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.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

On 11/09/2024 10:20:07+0200, Esben Haabendal wrote:
> 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 :)

Sure.

> 
> >> +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?

This is fine, there are other I didn't point out.

> >
> >> +	/* 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.

Yes, this is the main problematic part. The other one being disabling
the IRQ output when in battery backup mode as this will surely prevent
wakeup of some devices.

> 
> 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()...

I guess you mean requesting the interrupt on the SoC side. Enabling the
RTC interrupt should be left untouched in the probe.


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

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

> On 11/09/2024 10:20:07+0200, Esben Haabendal wrote:
>> Alexandre Belloni <alexandre.belloni@bootlin.com> writes:
>> > On 10/09/2024 12:27:11+0200, Esben Haabendal wrote:
>> 
>> >> +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?
>
> This is fine, there are other I didn't point out.

Ok. I will change all of these type of error messages to dev_dbg. No problem.

>> >> +	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.
>
> Yes, this is the main problematic part. The other one being disabling
> the IRQ output when in battery backup mode as this will surely prevent
> wakeup of some devices.

I know. I did this on purpose, as I don't have a setup where I can test
wakeup, so I thought it was better to start out without this instead of
shipping something that is most likely broken.

If I leave IRQ output from RTC chip enabled during battery backup mode,
I assume I have to add working suspend/resume also. Or do you just want
me to flip the bit?

>> 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()...
>
> I guess you mean requesting the interrupt on the SoC side.

Yes.

> Enabling the RTC interrupt should be left untouched in the probe.

Ok, so if/when an alarm is already set before probe, do application need
to enable it using RTC_AIE_ON?

/Esben

On 12/09/2024 09:09:40+0200, Esben Haabendal wrote:
> Alexandre Belloni <alexandre.belloni@bootlin.com> writes:
> 
> > On 11/09/2024 10:20:07+0200, Esben Haabendal wrote:
> >> Alexandre Belloni <alexandre.belloni@bootlin.com> writes:
> >> > On 10/09/2024 12:27:11+0200, Esben Haabendal wrote:
> >> 
> >> >> +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?
> >
> > This is fine, there are other I didn't point out.
> 
> Ok. I will change all of these type of error messages to dev_dbg. No problem.
> 
> >> >> +	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.
> >
> > Yes, this is the main problematic part. The other one being disabling
> > the IRQ output when in battery backup mode as this will surely prevent
> > wakeup of some devices.
> 
> I know. I did this on purpose, as I don't have a setup where I can test
> wakeup, so I thought it was better to start out without this instead of
> shipping something that is most likely broken.
> 
> If I leave IRQ output from RTC chip enabled during battery backup mode,
> I assume I have to add working suspend/resume also. Or do you just want
> me to flip the bit?

The issue is still about the lifecycle. The RTC will remember the
setting so if you change it from the default value without providing a
control, there is no way to change back the driver behavior in the
future because this is going to break a use case and there is no way to
win. So my preference is that you leave the bit to its default value.
You don't necessarily need the suspend/resume callbacks.

> 
> >> 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()...
> >
> > I guess you mean requesting the interrupt on the SoC side.
> 
> Yes.
> 
> > Enabling the RTC interrupt should be left untouched in the probe.
> 
> Ok, so if/when an alarm is already set before probe, do application need
> to enable it using RTC_AIE_ON?

If the alarm is on on boot, it must be kept on without any user
intervention.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

> On 12/09/2024 09:09:40+0200, Esben Haabendal wrote:
>> Alexandre Belloni <alexandre.belloni@bootlin.com> writes:
>> 
>> > On 11/09/2024 10:20:07+0200, Esben Haabendal wrote:
>> >> Alexandre Belloni <alexandre.belloni@bootlin.com> writes:
>> >> > On 10/09/2024 12:27:11+0200, Esben Haabendal wrote:
>> >> 
>> >> >> +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?
>> >
>> > This is fine, there are other I didn't point out.
>> 
>> Ok. I will change all of these type of error messages to dev_dbg. No problem.
>> 
>> >> >> +	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.
>> >
>> > Yes, this is the main problematic part. The other one being disabling
>> > the IRQ output when in battery backup mode as this will surely prevent
>> > wakeup of some devices.
>> 
>> I know. I did this on purpose, as I don't have a setup where I can test
>> wakeup, so I thought it was better to start out without this instead of
>> shipping something that is most likely broken.
>> 
>> If I leave IRQ output from RTC chip enabled during battery backup mode,
>> I assume I have to add working suspend/resume also. Or do you just want
>> me to flip the bit?
>
> The issue is still about the lifecycle. The RTC will remember the
> setting so if you change it from the default value without providing a
> control, there is no way to change back the driver behavior in the
> future because this is going to break a use case and there is no way to
> win. So my preference is that you leave the bit to its default value.

Yes, sounds like the right approach.

But should I actively set FOBATB bit to the default value, or leave it
at its current value (which potentially could be non-default)?

> You don't necessarily need the suspend/resume callbacks.
>
>> >> 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()...
>> >
>> > I guess you mean requesting the interrupt on the SoC side.
>> 
>> Yes.
>> 
>> > Enabling the RTC interrupt should be left untouched in the probe.
>> 
>> Ok, so if/when an alarm is already set before probe, do application need
>> to enable it using RTC_AIE_ON?
>
> If the alarm is on on boot, it must be kept on without any user
> intervention.

Sure. But do we want to check for an active alarm, and then call
enable_irq() if there is one? If not, the alarm would assert the
interrupt line, but the irq might not be raised as the handler is
disabled.

/Esben

Esben Haabendal <esben@geanix.com> writes:

> 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.
>
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
>  drivers/rtc/rtc-isl12022.c | 244 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 241 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c
> index d82278fdc29b..682b1bf10160 100644
> --- a/drivers/rtc/rtc-isl12022.c
> +++ b/drivers/rtc/rtc-isl12022.c
> @@ -21,7 +21,7 @@
>  
>  #include <asm/byteorder.h>
>  
> -/* ISL register offsets */
> +/* RTC - Real time clock registers */
>  #define ISL12022_REG_SC		0x00
>  #define ISL12022_REG_MN		0x01
>  #define ISL12022_REG_HR		0x02
> @@ -30,21 +30,36 @@
>  #define ISL12022_REG_YR		0x05
>  #define ISL12022_REG_DW		0x06
>  
> +/* CSR - Control and status registers */
>  #define ISL12022_REG_SR		0x07
>  #define ISL12022_REG_INT	0x08
> -
>  #define ISL12022_REG_PWR_VBAT	0x0a
> -
>  #define ISL12022_REG_BETA	0x0d
> +
> +/* ALARM - Alarm registers */
> +#define ISL12022_REG_SCA0	0x10
> +#define ISL12022_REG_MNA0	0x11
> +#define ISL12022_REG_HRA0	0x12
> +#define ISL12022_REG_DTA0	0x13
> +#define ISL12022_REG_MOA0	0x14
> +#define ISL12022_REG_DWA0	0x15
> +#define ISL12022_ALARM_SECTION		ISL12022_REG_SCA0
> +#define ISL12022_ALARM_SECTION_LEN	(ISL12022_REG_DWA0 - ISL12022_REG_SCA0 + 1)
> +
> +/* TEMP - Temperature sensor registers */
>  #define ISL12022_REG_TEMP_L	0x28
>  
>  /* ISL register bits */
>  #define ISL12022_HR_MIL		(1 << 7)	/* military or 24 hour time */
>  
> +#define ISL12022_SR_ALM		(1 << 4)
>  #define ISL12022_SR_LBAT85	(1 << 2)
>  #define ISL12022_SR_LBAT75	(1 << 1)
>  
> +#define ISL12022_INT_ARST	(1 << 7)
>  #define ISL12022_INT_WRTC	(1 << 6)
> +#define ISL12022_INT_IM		(1 << 5)
> +#define ISL12022_INT_FOBATB	(1 << 4)
>  #define ISL12022_INT_FO_MASK	GENMASK(3, 0)
>  #define ISL12022_INT_FO_OFF	0x0
>  #define ISL12022_INT_FO_32K	0x1
> @@ -52,10 +67,18 @@
>  #define ISL12022_REG_VB85_MASK	GENMASK(5, 3)
>  #define ISL12022_REG_VB75_MASK	GENMASK(2, 0)
>  
> +#define ISL12022_ALARM_ENABLE	(1 << 7)	/* for all ALARM registers  */
> +
>  #define ISL12022_BETA_TSE	(1 << 7)
>  
> +static struct i2c_driver isl12022_driver;
> +
>  struct isl12022 {
> +	struct i2c_client *i2c;
> +	struct rtc_device *rtc;
>  	struct regmap *regmap;
> +	int irq;
> +	bool irq_enabled;
>  };
>  
>  static umode_t isl12022_hwmon_is_visible(const void *data,
> @@ -215,6 +238,208 @@ 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_rtc_read_alarm(struct device *dev,
> +				   struct rtc_wkalrm *alarm)
> +{

Style nit, but I think it's easier to read and grep for if the prototype
is on one line, and it wouldn't go significantly over 80 chars. The file
already has a few lines > 80 chars, and the 80 char limit doesn't really
exist anymore.

> 
> +	struct rtc_time *const tm = &alarm->time;

Hm, declaring auto variables const is quite unusual. I see that a few
other rtc drivers have done this, but I don't it's an example to copy.

> +	struct isl12022 *isl12022 = dev_get_drvdata(dev);
> +	struct regmap *regmap = isl12022->regmap;
> +	uint8_t buf[ISL12022_ALARM_SECTION_LEN];

The kernel normally says u8 (and you do as well in _set_alarm()).

> +	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__);
> +		return ret;
> +	}
> +
> +	dev_dbg(dev,
> +		"%s: sc=%02x, mn=%02x, hr=%02x, dt=%02x, mo=%02x, dw=%02x\n",
> +		__func__, buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]);
> +
> +	tm->tm_sec = bcd2bin(buf[ISL12022_REG_SCA0 - ISL12022_ALARM_SECTION]
> +			     & 0x7F);
> +	tm->tm_min = bcd2bin(buf[ISL12022_REG_MNA0 - ISL12022_ALARM_SECTION]
> +			     & 0x7F);
> +	tm->tm_hour = bcd2bin(buf[ISL12022_REG_HRA0 - ISL12022_ALARM_SECTION]
> +			      & 0x3F);
> +	tm->tm_mday = bcd2bin(buf[ISL12022_REG_DTA0 - ISL12022_ALARM_SECTION]
> +			      & 0x3F);
> +	tm->tm_mon = bcd2bin(buf[ISL12022_REG_MOA0 - ISL12022_ALARM_SECTION]
> +			     & 0x1F) - 1;
> +	tm->tm_wday = buf[ISL12022_REG_DWA0 - ISL12022_ALARM_SECTION] & 0x07;
> +

Here I'd also suggest keeping each assignment on one line, it's rather
hard to read this way.

> 
> +	/* 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__);
> +		return yr;

return ret, presumably.

regmap_read() takes an 'unsigned int *', but yr is int. If the compiler
doesn't warn I suppose it doesn't matter.

I suggest moving the reading of the yr register up to right after the
other regmap_read, then you could also include it in the dev_dbg output,
and all the bcd2bin() conversions are done next to each other.

> +	}
> +	tm->tm_year = bcd2bin(yr) + 100;
> +
> +	for (i = 0 ; i < ISL12022_ALARM_SECTION_LEN ; i++) {

Nit: no spaces before the semicolons.

> +		if (buf[i] & ISL12022_ALARM_ENABLE) {
> +			alarm->enabled = 1;
> +			break;
> +		}
> +	}
> +
> +	dev_dbg(dev, "%s: %ptR\n", __func__, tm);
> +
> +	return 0;
> +}
> +
> +static int isl12022_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
> +{
> +	struct rtc_time *alarm_tm = &alarm->time;
> +	struct isl12022 *isl12022 = dev_get_drvdata(dev);
> +	struct regmap *regmap = isl12022->regmap;
> +	u8 regs[ISL12022_ALARM_SECTION_LEN] = { 0, };
> +	struct rtc_time rtc_tm;
> +	int ret = 0, enable, dw;
> +

Nit: No need to initialize ret when the very first thing you do is
assigning to it.

> +	ret = isl12022_rtc_read_time(dev, &rtc_tm);
> +	if (ret)
> +		return ret;
> +
> +	/* If the alarm time is before the current time disable the alarm */
> +	if (!alarm->enabled || rtc_tm_sub(alarm_tm, &rtc_tm) <= 0)
> +		enable = 0;
> +	else
> +		enable = ISL12022_ALARM_ENABLE;
> +
> +	/* Set non-matching tm_wday to safeguard against early false matching
> +	 * while setting all the alarm registers (this rtc lacks a general
> +	 * alarm/irq enable/disable bit).
> +	 */

Nit: Don't use network comment style.

> +	if (enable) {
> +		ret = regmap_read(regmap, ISL12022_REG_DW, &dw);
> +		if (ret) {
> +			dev_err(dev, "%s: reading DW failed\n", __func__);
> +			return ret;
> +		}
> +		/* ~4 days into the future should be enough to avoid match */
> +		dw = ((dw + 4) % 7) | ISL12022_ALARM_ENABLE;
> +		ret = regmap_write(regmap, ISL12022_REG_DWA0, dw);
> +		if (ret) {
> +			dev_err(dev, "%s: writing DWA0 failed\n", __func__);
> +			return ret;
> +		}
> +	}
> +
> +	/* Program the alarm and enable it for each setting */
> +	regs[ISL12022_REG_SCA0 - ISL12022_ALARM_SECTION] =
> +		bin2bcd(alarm_tm->tm_sec) | enable;
> +	regs[ISL12022_REG_MNA0 - ISL12022_ALARM_SECTION] =
> +		bin2bcd(alarm_tm->tm_min) | enable;
> +	regs[ISL12022_REG_HRA0 - ISL12022_ALARM_SECTION] =
> +		bin2bcd(alarm_tm->tm_hour) | enable;
> +	regs[ISL12022_REG_DTA0 - ISL12022_ALARM_SECTION] =
> +		bin2bcd(alarm_tm->tm_mday) | enable;
> +	regs[ISL12022_REG_MOA0 - ISL12022_ALARM_SECTION] =
> +		bin2bcd(alarm_tm->tm_mon + 1) | enable;
> +	regs[ISL12022_REG_DWA0 - ISL12022_ALARM_SECTION] =
> +		bin2bcd(alarm_tm->tm_wday & 7) | enable;
> +

The dwa0 handling is a nice trick for avoiding triggering a false
alarm. But I do wonder if you might need to do it also for the !enable
case. That is, suppose we've had the alarm set for 01:02:15. The alarm
fires, we do stuff, and then we want to turn it off. So this gets called
with some 00:00:00 value in alarm_tm and enable==0. Then when we start
writing the new register values, as soon as REG_SCA0 has been written
to, the alarm condition for 01:02:xx is automatically satisfied.

If you unconditionally write a "four days in the future, with alarm bit
set" value to DWA0, that should prevent this and the DWA0 does get its
!enable value set via the bulk_write.


> +	/* write ALARM registers */
> +	ret = regmap_bulk_write(regmap, ISL12022_REG_SCA0,
> +				&regs, sizeof(regs));

Nit: Fits in one line (I think), and you probably want to use the
ISL12022_ALARM_SECTION name here, even if they're of course the same.

> +	if (ret) {
> +		dev_err(dev, "%s: writing ALARM registers failed\n", __func__);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static irqreturn_t isl12022_rtc_interrupt(int irq, void *data)
> +{
> +	struct isl12022 *isl12022 = data;
> +	struct rtc_device *rtc = isl12022->rtc;
> +	struct device *dev = &rtc->dev;
> +	struct regmap *regmap = isl12022->regmap;
> +	u32 val = 0;
> +	unsigned long events = 0;
> +	int ret;
> +
> +	ret = regmap_read(regmap, ISL12022_REG_SR, &val);
> +	if (ret) {
> +		dev_err(dev, "%s: reading SR failed\n", __func__);
> +		return IRQ_HANDLED;
> +	}
> +
> +	if (val & ISL12022_SR_ALM)
> +		events |= RTC_IRQF | RTC_AF;
> +
> +	if (events & RTC_AF)
> +		dev_dbg(dev, "alarm!\n");
> +
> +	if (!events)
> +		return IRQ_NONE;
> +
> +	rtc_update_irq(rtc, 1, events);
> +	return IRQ_HANDLED;
> +}
> +
> +static int isl12022_rtc_alarm_irq_enable(struct device *dev,
> +					 unsigned int enabled)
> +{
> +	struct isl12022 *isl12022 = dev_get_drvdata(dev);
> +
> +	if (!isl12022->irq_enabled == !enabled)
> +		return 0;
> +
> +	if (enabled)
> +		enable_irq(isl12022->irq);
> +	else
> +		disable_irq(isl12022->irq);
> +
> +	isl12022->irq_enabled = !!enabled;
> +

I see why you do the ! and !! dances to canonicalize boolean values for
comparison, but it's not very pretty. But ->alarm_irq_enable has the
signature it has (that should probably get changed), so to be safe I
guess you do need them. That said, I don't think it's unreasonable to
assume that ->alarm_irq_enable is only ever invoked with the values 0
and 1 for the enabled argument, and e.g. rtc-cpcap.c gets away with that
assumption.


> +	return 0;
> +}
> +
> +static int isl12022_setup_irq(struct isl12022 *isl12022, int irq)
> +{
> +	struct device *dev = &isl12022->i2c->dev;

I was wondering why you needed to stash the i2c_client, but I see it
here. The other initialization helpers (_set_trip_levels and
_hwmon_register) are passed &client->dev so they have this dev directly,
and they then get the regmap (or, with patch 1, the struct isl12022)
from that with dev_get_drvdata(). For consistency I think you should do
the same, and then you can drop the i2c field in struct isl12022.

> +	struct regmap *regmap = isl12022->regmap;
> +	unsigned int reg_mask, reg_val;
> +	u8 buf[ISL12022_ALARM_SECTION_LEN] = { 0, };
> +	int ret;
> +
> +	/* Clear and disable all alarm registers */
> +	ret = regmap_bulk_write(regmap, ISL12022_ALARM_SECTION,
> +				buf, sizeof(buf));
> +	if (ret)
> +		return ret;
> +
> +	/* Enable automatic reset of ALM bit, enable single event interrupt
> +	 * mode, and disable IRQ/fOUT pin during battery-backup mode.
> +	 */

Network-style.

> +	reg_mask = ISL12022_INT_ARST | ISL12022_INT_IM
> +		| ISL12022_INT_FOBATB | ISL12022_INT_FO_MASK;
> +	reg_val = ISL12022_INT_ARST | ISL12022_INT_FOBATB | ISL12022_INT_FO_OFF;
> +	ret = regmap_write_bits(regmap, ISL12022_REG_INT,
> +				reg_mask, reg_val);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_request_threaded_irq(dev, irq, NULL,
> +					isl12022_rtc_interrupt,
> +					IRQF_SHARED | IRQF_ONESHOT,
> +					isl12022_driver.driver.name,
> +					isl12022);
> +	if (ret) {
> +		dev_err(dev, "Unable to request irq %d\n", irq);
> +		return ret;

This should probably be "return dev_err_probe(...);" - the irq could in
theory be routed to some gpio expander which is not yet probed, so we
could get -EPROBE_DEFER. And regardless, dev_err_probe has the advantage
of printing what the err code actually is.

Rasmus

Rasmus Villemoes <linux@rasmusvillemoes.dk> writes:

> Esben Haabendal <esben@geanix.com> writes:
>
>> 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.
>>
>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>> ---
>>  drivers/rtc/rtc-isl12022.c | 244 ++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 241 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c
>> index d82278fdc29b..682b1bf10160 100644
>> --- a/drivers/rtc/rtc-isl12022.c
>> +++ b/drivers/rtc/rtc-isl12022.c
>> @@ -21,7 +21,7 @@
>>  
>>  #include <asm/byteorder.h>
>>  
>> -/* ISL register offsets */
>> +/* RTC - Real time clock registers */
>>  #define ISL12022_REG_SC		0x00
>>  #define ISL12022_REG_MN		0x01
>>  #define ISL12022_REG_HR		0x02
>> @@ -30,21 +30,36 @@
>>  #define ISL12022_REG_YR		0x05
>>  #define ISL12022_REG_DW		0x06
>>  
>> +/* CSR - Control and status registers */
>>  #define ISL12022_REG_SR		0x07
>>  #define ISL12022_REG_INT	0x08
>> -
>>  #define ISL12022_REG_PWR_VBAT	0x0a
>> -
>>  #define ISL12022_REG_BETA	0x0d
>> +
>> +/* ALARM - Alarm registers */
>> +#define ISL12022_REG_SCA0	0x10
>> +#define ISL12022_REG_MNA0	0x11
>> +#define ISL12022_REG_HRA0	0x12
>> +#define ISL12022_REG_DTA0	0x13
>> +#define ISL12022_REG_MOA0	0x14
>> +#define ISL12022_REG_DWA0	0x15
>> +#define ISL12022_ALARM_SECTION		ISL12022_REG_SCA0
>> +#define ISL12022_ALARM_SECTION_LEN	(ISL12022_REG_DWA0 - ISL12022_REG_SCA0 + 1)
>> +
>> +/* TEMP - Temperature sensor registers */
>>  #define ISL12022_REG_TEMP_L	0x28
>>  
>>  /* ISL register bits */
>>  #define ISL12022_HR_MIL		(1 << 7)	/* military or 24 hour time */
>>  
>> +#define ISL12022_SR_ALM		(1 << 4)
>>  #define ISL12022_SR_LBAT85	(1 << 2)
>>  #define ISL12022_SR_LBAT75	(1 << 1)
>>  
>> +#define ISL12022_INT_ARST	(1 << 7)
>>  #define ISL12022_INT_WRTC	(1 << 6)
>> +#define ISL12022_INT_IM		(1 << 5)
>> +#define ISL12022_INT_FOBATB	(1 << 4)
>>  #define ISL12022_INT_FO_MASK	GENMASK(3, 0)
>>  #define ISL12022_INT_FO_OFF	0x0
>>  #define ISL12022_INT_FO_32K	0x1
>> @@ -52,10 +67,18 @@
>>  #define ISL12022_REG_VB85_MASK	GENMASK(5, 3)
>>  #define ISL12022_REG_VB75_MASK	GENMASK(2, 0)
>>  
>> +#define ISL12022_ALARM_ENABLE	(1 << 7)	/* for all ALARM registers  */
>> +
>>  #define ISL12022_BETA_TSE	(1 << 7)
>>  
>> +static struct i2c_driver isl12022_driver;
>> +
>>  struct isl12022 {
>> +	struct i2c_client *i2c;
>> +	struct rtc_device *rtc;
>>  	struct regmap *regmap;
>> +	int irq;
>> +	bool irq_enabled;
>>  };
>>  
>>  static umode_t isl12022_hwmon_is_visible(const void *data,
>> @@ -215,6 +238,208 @@ 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_rtc_read_alarm(struct device *dev,
>> +				   struct rtc_wkalrm *alarm)
>> +{
>
> Style nit, but I think it's easier to read and grep for if the prototype
> is on one line, and it wouldn't go significantly over 80 chars. The file
> already has a few lines > 80 chars, and the 80 char limit doesn't really
> exist anymore.

Ok. I will change it to a single line. No problem.

>
>> 
>> +	struct rtc_time *const tm = &alarm->time;
>
> Hm, declaring auto variables const is quite unusual. I see that a few
> other rtc drivers have done this, but I don't it's an example to copy.

Ok. Dropping the const here. And yes, it had crept via copy-paste.

>> +	struct isl12022 *isl12022 = dev_get_drvdata(dev);
>> +	struct regmap *regmap = isl12022->regmap;
>> +	uint8_t buf[ISL12022_ALARM_SECTION_LEN];
>
> The kernel normally says u8 (and you do as well in _set_alarm()).

Another copy-paste issue. This time it was from _read_time() and
_set_time().

To avoid inconsistent coding style, I guess I should add a commit
changing to u8 in _read_time() and _set_time() as well.

>> +	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__);
>> +		return ret;
>> +	}
>> +
>> +	dev_dbg(dev,
>> +		"%s: sc=%02x, mn=%02x, hr=%02x, dt=%02x, mo=%02x, dw=%02x\n",
>> +		__func__, buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]);
>> +
>> +	tm->tm_sec = bcd2bin(buf[ISL12022_REG_SCA0 - ISL12022_ALARM_SECTION]
>> +			     & 0x7F);
>> +	tm->tm_min = bcd2bin(buf[ISL12022_REG_MNA0 - ISL12022_ALARM_SECTION]
>> +			     & 0x7F);
>> +	tm->tm_hour = bcd2bin(buf[ISL12022_REG_HRA0 - ISL12022_ALARM_SECTION]
>> +			      & 0x3F);
>> +	tm->tm_mday = bcd2bin(buf[ISL12022_REG_DTA0 - ISL12022_ALARM_SECTION]
>> +			      & 0x3F);
>> +	tm->tm_mon = bcd2bin(buf[ISL12022_REG_MOA0 - ISL12022_ALARM_SECTION]
>> +			     & 0x1F) - 1;
>> +	tm->tm_wday = buf[ISL12022_REG_DWA0 - ISL12022_ALARM_SECTION] & 0x07;
>> +
>
> Here I'd also suggest keeping each assignment on one line, it's rather
> hard to read this way.

I agree, and I will change it here. But if the 80 columns rule is out,
what kind of rule for line width is used instead?

>> +	/* 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__);
>> +		return yr;
>
> return ret, presumably.

Oops. Fixing.

> regmap_read() takes an 'unsigned int *', but yr is int. If the compiler
> doesn't warn I suppose it doesn't matter.

My compiler seems happy. But no harm in fixing it.

> I suggest moving the reading of the yr register up to right after the
> other regmap_read, then you could also include it in the dev_dbg output,
> and all the bcd2bin() conversions are done next to each other.
>
>> +	}
>> +	tm->tm_year = bcd2bin(yr) + 100;
>> +
>> +	for (i = 0 ; i < ISL12022_ALARM_SECTION_LEN ; i++) {
>
> Nit: no spaces before the semicolons.

Nit removal in progress.

>> +		if (buf[i] & ISL12022_ALARM_ENABLE) {
>> +			alarm->enabled = 1;
>> +			break;
>> +		}
>> +	}
>> +
>> +	dev_dbg(dev, "%s: %ptR\n", __func__, tm);
>> +
>> +	return 0;
>> +}
>> +
>> +static int isl12022_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
>> +{
>> +	struct rtc_time *alarm_tm = &alarm->time;
>> +	struct isl12022 *isl12022 = dev_get_drvdata(dev);
>> +	struct regmap *regmap = isl12022->regmap;
>> +	u8 regs[ISL12022_ALARM_SECTION_LEN] = { 0, };
>> +	struct rtc_time rtc_tm;
>> +	int ret = 0, enable, dw;
>> +
>
> Nit: No need to initialize ret when the very first thing you do is
> assigning to it.

Fixing.

>> +	ret = isl12022_rtc_read_time(dev, &rtc_tm);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* If the alarm time is before the current time disable the alarm */
>> +	if (!alarm->enabled || rtc_tm_sub(alarm_tm, &rtc_tm) <= 0)
>> +		enable = 0;
>> +	else
>> +		enable = ISL12022_ALARM_ENABLE;
>> +
>> +	/* Set non-matching tm_wday to safeguard against early false matching
>> +	 * while setting all the alarm registers (this rtc lacks a general
>> +	 * alarm/irq enable/disable bit).
>> +	 */
>
> Nit: Don't use network comment style.

Ok. I did not know this was network comment style only.
So it should be with both '/*' and '*/' on separate lines?

>> +	if (enable) {
>> +		ret = regmap_read(regmap, ISL12022_REG_DW, &dw);
>> +		if (ret) {
>> +			dev_err(dev, "%s: reading DW failed\n", __func__);
>> +			return ret;
>> +		}
>> +		/* ~4 days into the future should be enough to avoid match */
>> +		dw = ((dw + 4) % 7) | ISL12022_ALARM_ENABLE;
>> +		ret = regmap_write(regmap, ISL12022_REG_DWA0, dw);
>> +		if (ret) {
>> +			dev_err(dev, "%s: writing DWA0 failed\n", __func__);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	/* Program the alarm and enable it for each setting */
>> +	regs[ISL12022_REG_SCA0 - ISL12022_ALARM_SECTION] =
>> +		bin2bcd(alarm_tm->tm_sec) | enable;
>> +	regs[ISL12022_REG_MNA0 - ISL12022_ALARM_SECTION] =
>> +		bin2bcd(alarm_tm->tm_min) | enable;
>> +	regs[ISL12022_REG_HRA0 - ISL12022_ALARM_SECTION] =
>> +		bin2bcd(alarm_tm->tm_hour) | enable;
>> +	regs[ISL12022_REG_DTA0 - ISL12022_ALARM_SECTION] =
>> +		bin2bcd(alarm_tm->tm_mday) | enable;
>> +	regs[ISL12022_REG_MOA0 - ISL12022_ALARM_SECTION] =
>> +		bin2bcd(alarm_tm->tm_mon + 1) | enable;
>> +	regs[ISL12022_REG_DWA0 - ISL12022_ALARM_SECTION] =
>> +		bin2bcd(alarm_tm->tm_wday & 7) | enable;
>> +
>
> The dwa0 handling is a nice trick for avoiding triggering a false
> alarm. But I do wonder if you might need to do it also for the !enable
> case. That is, suppose we've had the alarm set for 01:02:15. The alarm
> fires, we do stuff, and then we want to turn it off. So this gets called
> with some 00:00:00 value in alarm_tm and enable==0. Then when we start
> writing the new register values, as soon as REG_SCA0 has been written
> to, the alarm condition for 01:02:xx is automatically satisfied.
>
> If you unconditionally write a "four days in the future, with alarm bit
> set" value to DWA0, that should prevent this and the DWA0 does get its
> !enable value set via the bulk_write.

Good idea. I will remove the condition for the DWA0 trick.

>> +	/* write ALARM registers */
>> +	ret = regmap_bulk_write(regmap, ISL12022_REG_SCA0,
>> +				&regs, sizeof(regs));
>
> Nit: Fits in one line (I think), and you probably want to use the
> ISL12022_ALARM_SECTION name here, even if they're of course the same.

Using ISL12022_ALARM_SECTION makes the line 85 columns. I must admit I
feel a bit uneasy about going over the 80 columns, as I have no idea
when to wrap the lines then...

>> +	if (ret) {
>> +		dev_err(dev, "%s: writing ALARM registers failed\n", __func__);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static irqreturn_t isl12022_rtc_interrupt(int irq, void *data)
>> +{
>> +	struct isl12022 *isl12022 = data;
>> +	struct rtc_device *rtc = isl12022->rtc;
>> +	struct device *dev = &rtc->dev;
>> +	struct regmap *regmap = isl12022->regmap;
>> +	u32 val = 0;
>> +	unsigned long events = 0;
>> +	int ret;
>> +
>> +	ret = regmap_read(regmap, ISL12022_REG_SR, &val);
>> +	if (ret) {
>> +		dev_err(dev, "%s: reading SR failed\n", __func__);
>> +		return IRQ_HANDLED;
>> +	}
>> +
>> +	if (val & ISL12022_SR_ALM)
>> +		events |= RTC_IRQF | RTC_AF;
>> +
>> +	if (events & RTC_AF)
>> +		dev_dbg(dev, "alarm!\n");
>> +
>> +	if (!events)
>> +		return IRQ_NONE;
>> +
>> +	rtc_update_irq(rtc, 1, events);
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int isl12022_rtc_alarm_irq_enable(struct device *dev,
>> +					 unsigned int enabled)
>> +{
>> +	struct isl12022 *isl12022 = dev_get_drvdata(dev);
>> +
>> +	if (!isl12022->irq_enabled == !enabled)
>> +		return 0;
>> +
>> +	if (enabled)
>> +		enable_irq(isl12022->irq);
>> +	else
>> +		disable_irq(isl12022->irq);
>> +
>> +	isl12022->irq_enabled = !!enabled;
>> +
>
> I see why you do the ! and !! dances to canonicalize boolean values for
> comparison, but it's not very pretty. But ->alarm_irq_enable has the
> signature it has (that should probably get changed), so to be safe I
> guess you do need them. That said, I don't think it's unreasonable to
> assume that ->alarm_irq_enable is only ever invoked with the values 0
> and 1 for the enabled argument, and e.g. rtc-cpcap.c gets away with that
> assumption.

The handling in rtc-cpcap.c looks a bit strange IMHO. The comparison is
without using !, and then the assignment is done with !!. I think we
should either rely on enabled always being either 0 or 1, or handle the
cases where it might be something else.

I prefer to play it safe for now.

But if I explicitly do this first

    /* Make sure enabled is 0 or 1 */
    enabled = !!enabled;

Then we can leave out the ! and !! below. The code should be more
readable, and it will be much clearer for anyone that later on will want
to get rid of this.

>> +	return 0;
>> +}
>> +
>> +static int isl12022_setup_irq(struct isl12022 *isl12022, int irq)
>> +{
>> +	struct device *dev = &isl12022->i2c->dev;
>
> I was wondering why you needed to stash the i2c_client, but I see it
> here. The other initialization helpers (_set_trip_levels and
> _hwmon_register) are passed &client->dev so they have this dev directly,
> and they then get the regmap (or, with patch 1, the struct isl12022)
> from that with dev_get_drvdata(). For consistency I think you should do
> the same, and then you can drop the i2c field in struct isl12022.

Good idea. I had been thinking about something like this, but got away
from it again. I will change it in v2.

>> +	struct regmap *regmap = isl12022->regmap;
>> +	unsigned int reg_mask, reg_val;
>> +	u8 buf[ISL12022_ALARM_SECTION_LEN] = { 0, };
>> +	int ret;
>> +
>> +	/* Clear and disable all alarm registers */
>> +	ret = regmap_bulk_write(regmap, ISL12022_ALARM_SECTION,
>> +				buf, sizeof(buf));
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Enable automatic reset of ALM bit, enable single event interrupt
>> +	 * mode, and disable IRQ/fOUT pin during battery-backup mode.
>> +	 */
>
> Network-style.

Got it.

>
>> +	reg_mask = ISL12022_INT_ARST | ISL12022_INT_IM
>> +		| ISL12022_INT_FOBATB | ISL12022_INT_FO_MASK;
>> +	reg_val = ISL12022_INT_ARST | ISL12022_INT_FOBATB | ISL12022_INT_FO_OFF;
>> +	ret = regmap_write_bits(regmap, ISL12022_REG_INT,
>> +				reg_mask, reg_val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = devm_request_threaded_irq(dev, irq, NULL,
>> +					isl12022_rtc_interrupt,
>> +					IRQF_SHARED | IRQF_ONESHOT,
>> +					isl12022_driver.driver.name,
>> +					isl12022);
>> +	if (ret) {
>> +		dev_err(dev, "Unable to request irq %d\n", irq);
>> +		return ret;
>
> This should probably be "return dev_err_probe(...);" - the irq could in
> theory be routed to some gpio expander which is not yet probed, so we
> could get -EPROBE_DEFER. And regardless, dev_err_probe has the advantage
> of printing what the err code actually is.

I will change this both this and the other dev_err() in _probe() to
dev_err_probe().

/Esben

Esben Haabendal <esben@geanix.com> writes:

> Rasmus Villemoes <linux@rasmusvillemoes.dk> writes:
>
>> Esben Haabendal <esben@geanix.com> writes:
>>

>>> +	struct isl12022 *isl12022 = dev_get_drvdata(dev);
>>> +	struct regmap *regmap = isl12022->regmap;
>>> +	uint8_t buf[ISL12022_ALARM_SECTION_LEN];
>>
>> The kernel normally says u8 (and you do as well in _set_alarm()).
>
> Another copy-paste issue. This time it was from _read_time() and
> _set_time().
>
> To avoid inconsistent coding style, I guess I should add a commit
> changing to u8 in _read_time() and _set_time() as well.
>

Ah, hadn't noticed that. Yes, please fix that up while in here.

>>> +	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__);
>>> +		return ret;
>>> +	}
>>> +
>>> +	dev_dbg(dev,
>>> +		"%s: sc=%02x, mn=%02x, hr=%02x, dt=%02x, mo=%02x, dw=%02x\n",
>>> +		__func__, buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]);
>>> +
>>> +	tm->tm_sec = bcd2bin(buf[ISL12022_REG_SCA0 - ISL12022_ALARM_SECTION]
>>> +			     & 0x7F);
>>> +	tm->tm_min = bcd2bin(buf[ISL12022_REG_MNA0 - ISL12022_ALARM_SECTION]
>>> +			     & 0x7F);
>>> +	tm->tm_hour = bcd2bin(buf[ISL12022_REG_HRA0 - ISL12022_ALARM_SECTION]
>>> +			      & 0x3F);
>>> +	tm->tm_mday = bcd2bin(buf[ISL12022_REG_DTA0 - ISL12022_ALARM_SECTION]
>>> +			      & 0x3F);
>>> +	tm->tm_mon = bcd2bin(buf[ISL12022_REG_MOA0 - ISL12022_ALARM_SECTION]
>>> +			     & 0x1F) - 1;
>>> +	tm->tm_wday = buf[ISL12022_REG_DWA0 - ISL12022_ALARM_SECTION] & 0x07;
>>> +
>>
>> Here I'd also suggest keeping each assignment on one line, it's rather
>> hard to read this way.
>
> I agree, and I will change it here. But if the 80 columns rule is out,
> what kind of rule for line width is used instead?

See commit bdc48fa11e and the current wording in coding-style.rst. In
particular I think

+Statements longer than 80 columns should be broken into sensible chunks,
+unless exceeding 80 columns significantly increases readability and does
+not hide information.

applies here. I'd even say you could use spaces to align the = and &
operators (that is, make it '->tm_min  = ' and '->tm_hour = ').

So the 80 char limit is still there, just not as strongly enforced as it
used to, and once you hit 100, there has to be really strong reasons for
exceeding that. But 85 for avoiding putting '& 0x7F); on its own line?
Absolutely, do it.

>>> +
>>> +	/* Set non-matching tm_wday to safeguard against early false matching
>>> +	 * while setting all the alarm registers (this rtc lacks a general
>>> +	 * alarm/irq enable/disable bit).
>>> +	 */
>>
>> Nit: Don't use network comment style.
>
> Ok. I did not know this was network comment style only.
> So it should be with both '/*' and '*/' on separate lines?

Yes. I wanted to point you at the coding-style part which explains the
different preferred style for net/ and drivers/net, but then it turns
out I couldn't because 82b8000c28. Also, see
https://lore.kernel.org/lkml/CA+55aFyQYJerovMsSoSKS7PessZBr4vNp-3QUUwhqk4A4_jcbg@mail.gmail.com/ .

>>> +	/* write ALARM registers */
>>> +	ret = regmap_bulk_write(regmap, ISL12022_REG_SCA0,
>>> +				&regs, sizeof(regs));
>>
>> Nit: Fits in one line (I think), and you probably want to use the
>> ISL12022_ALARM_SECTION name here, even if they're of course the same.
>
> Using ISL12022_ALARM_SECTION makes the line 85 columns. I must admit I
> feel a bit uneasy about going over the 80 columns, as I have no idea
> when to wrap the lines then...

As for the name used, you should at least use the same in all the
regmap_bulk_*() calls. If you don't want to hardcode that SCA0 is the
first and thus have a name for the whole region, you could make that
name a little shorter (_ALARM_REGS maybe?).

I think vertical real estate is much more precious than horizontal, so
I'd prefer to have this be

  ret = regmap_bulk_write(regmap, ISL12022_ALARM_SECTION, &regs, sizeof(regs));

regardless of line length (as long as it's not crazy), because then I
can see more context.

>> I see why you do the ! and !! dances to canonicalize boolean values for
>> comparison, but it's not very pretty. But ->alarm_irq_enable has the
>> signature it has (that should probably get changed), so to be safe I
>> guess you do need them. That said, I don't think it's unreasonable to
>> assume that ->alarm_irq_enable is only ever invoked with the values 0
>> and 1 for the enabled argument, and e.g. rtc-cpcap.c gets away with that
>> assumption.
>
> The handling in rtc-cpcap.c looks a bit strange IMHO. The comparison is
> without using !, and then the assignment is done with !!. I think we
> should either rely on enabled always being either 0 or 1, or handle the
> cases where it might be something else.
>
> I prefer to play it safe for now.
>
> But if I explicitly do this first
>
>     /* Make sure enabled is 0 or 1 */
>     enabled = !!enabled;
>
> Then we can leave out the ! and !! below. The code should be more
> readable, and it will be much clearer for anyone that later on will want
> to get rid of this.

Yes, that's a good compromise.

Rasmus

Rasmus Villemoes <linux@rasmusvillemoes.dk> writes:

> Esben Haabendal <esben@geanix.com> writes:
>
>> Rasmus Villemoes <linux@rasmusvillemoes.dk> writes:
>>
>>> Esben Haabendal <esben@geanix.com> writes:
>>>
>
>>>> +	struct isl12022 *isl12022 = dev_get_drvdata(dev);
>>>> +	struct regmap *regmap = isl12022->regmap;
>>>> +	uint8_t buf[ISL12022_ALARM_SECTION_LEN];
>>>
>>> The kernel normally says u8 (and you do as well in _set_alarm()).
>>
>> Another copy-paste issue. This time it was from _read_time() and
>> _set_time().
>>
>> To avoid inconsistent coding style, I guess I should add a commit
>> changing to u8 in _read_time() and _set_time() as well.
>>
>
> Ah, hadn't noticed that. Yes, please fix that up while in here.

Done.

>>>> +	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__);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	dev_dbg(dev,
>>>> +		"%s: sc=%02x, mn=%02x, hr=%02x, dt=%02x, mo=%02x, dw=%02x\n",
>>>> +		__func__, buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]);
>>>> +
>>>> +	tm->tm_sec = bcd2bin(buf[ISL12022_REG_SCA0 - ISL12022_ALARM_SECTION]
>>>> +			     & 0x7F);
>>>> +	tm->tm_min = bcd2bin(buf[ISL12022_REG_MNA0 - ISL12022_ALARM_SECTION]
>>>> +			     & 0x7F);
>>>> +	tm->tm_hour = bcd2bin(buf[ISL12022_REG_HRA0 - ISL12022_ALARM_SECTION]
>>>> +			      & 0x3F);
>>>> +	tm->tm_mday = bcd2bin(buf[ISL12022_REG_DTA0 - ISL12022_ALARM_SECTION]
>>>> +			      & 0x3F);
>>>> +	tm->tm_mon = bcd2bin(buf[ISL12022_REG_MOA0 - ISL12022_ALARM_SECTION]
>>>> +			     & 0x1F) - 1;
>>>> +	tm->tm_wday = buf[ISL12022_REG_DWA0 - ISL12022_ALARM_SECTION] & 0x07;
>>>> +
>>>
>>> Here I'd also suggest keeping each assignment on one line, it's rather
>>> hard to read this way.
>>
>> I agree, and I will change it here. But if the 80 columns rule is out,
>> what kind of rule for line width is used instead?
>
> See commit bdc48fa11e and the current wording in coding-style.rst. In
> particular I think
>
> +Statements longer than 80 columns should be broken into sensible chunks,
> +unless exceeding 80 columns significantly increases readability and does
> +not hide information.
>
> applies here. I'd even say you could use spaces to align the = and &
> operators (that is, make it '->tm_min  = ' and '->tm_hour = ').
>
> So the 80 char limit is still there, just not as strongly enforced as it
> used to, and once you hit 100, there has to be really strong reasons for
> exceeding that. But 85 for avoiding putting '& 0x7F); on its own line?
> Absolutely, do it.

Got it.

>>>> +
>>>> +	/* Set non-matching tm_wday to safeguard against early false matching
>>>> +	 * while setting all the alarm registers (this rtc lacks a general
>>>> +	 * alarm/irq enable/disable bit).
>>>> +	 */
>>>
>>> Nit: Don't use network comment style.
>>
>> Ok. I did not know this was network comment style only.
>> So it should be with both '/*' and '*/' on separate lines?
>
> Yes. I wanted to point you at the coding-style part which explains the
> different preferred style for net/ and drivers/net, but then it turns
> out I couldn't because 82b8000c28. Also, see
> https://lore.kernel.org/lkml/CA+55aFyQYJerovMsSoSKS7PessZBr4vNp-3QUUwhqk4A4_jcbg@mail.gmail.com/ .

Haha. You are so out of touch :D

I have changed to the normal kernel style.

>>>> +	/* write ALARM registers */
>>>> +	ret = regmap_bulk_write(regmap, ISL12022_REG_SCA0,
>>>> +				&regs, sizeof(regs));
>>>
>>> Nit: Fits in one line (I think), and you probably want to use the
>>> ISL12022_ALARM_SECTION name here, even if they're of course the same.
>>
>> Using ISL12022_ALARM_SECTION makes the line 85 columns. I must admit I
>> feel a bit uneasy about going over the 80 columns, as I have no idea
>> when to wrap the lines then...
>
> As for the name used, you should at least use the same in all the
> regmap_bulk_*() calls. If you don't want to hardcode that SCA0 is the
> first and thus have a name for the whole region, you could make that
> name a little shorter (_ALARM_REGS maybe?).

I am changing it to _ALARM and _ALARM_LEN. It definitely makes the code
more readable IMHO.

> I think vertical real estate is much more precious than horizontal, so
> I'd prefer to have this be
>
>   ret = regmap_bulk_write(regmap, ISL12022_ALARM_SECTION, &regs, sizeof(regs));
>
> regardless of line length (as long as it's not crazy), because then I
> can see more context.

With _ALARM, it even fits within 80 columns.

>>> I see why you do the ! and !! dances to canonicalize boolean values for
>>> comparison, but it's not very pretty. But ->alarm_irq_enable has the
>>> signature it has (that should probably get changed), so to be safe I
>>> guess you do need them. That said, I don't think it's unreasonable to
>>> assume that ->alarm_irq_enable is only ever invoked with the values 0
>>> and 1 for the enabled argument, and e.g. rtc-cpcap.c gets away with that
>>> assumption.
>>
>> The handling in rtc-cpcap.c looks a bit strange IMHO. The comparison is
>> without using !, and then the assignment is done with !!. I think we
>> should either rely on enabled always being either 0 or 1, or handle the
>> cases where it might be something else.
>>
>> I prefer to play it safe for now.
>>
>> But if I explicitly do this first
>>
>>     /* Make sure enabled is 0 or 1 */
>>     enabled = !!enabled;
>>
>> Then we can leave out the ! and !! below. The code should be more
>> readable, and it will be much clearer for anyone that later on will want
>> to get rid of this.
>
> Yes, that's a good compromise.

Ok, then I am waiting for clarification from Alexandre on how to change
_setup_irq() before sending out v2.

/Esben