Message-ID: <871q1qimah.fsf@prevas.dk>
Date: Wed, 11 Sep 2024 11:15:18 +0200
From: Rasmus Villemoes <linux@...musvillemoes.dk>
To: Esben Haabendal <esben@...nix.com>
Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>,
  linux-rtc@...r.kernel.org,  linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] rtc: isl12022: Add alarm support

Esben Haabendal <esben@...nix.com> writes:

> Rasmus Villemoes <linux@...musvillemoes.dk> writes:
>
>> Esben Haabendal <esben@...nix.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

Powered by blists - more mailing lists