Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx> writes:
> 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.
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,
>> + ®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.
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
