Esben Haabendal <esben@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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,
> + ®s, 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
