Re: [PATCH 2/2] rtc: isl12022: Add alarm support

[Esben Haabendal]

Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx> writes:

> Esben Haabendal <esben@xxxxxxxxxx> writes:
>
>> Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx> writes:
>>
>>> Esben Haabendal <esben@xxxxxxxxxx> 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@xxxxxxxxxxxxxx/ .

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