Re: [PATCH v2 8/8] rtc: isl12022: implement support for the #clock-cells DT property

[Rasmus Villemoes]

On 13/06/2023 17.25, Andy Shevchenko wrote:
> On Tue, Jun 13, 2023 at 03:00:10PM +0200, Rasmus Villemoes wrote:
>> If device tree implies that the chip's IRQ/F_OUT pin is used as a
>> clock, expose that in the driver. For now, pretend it is a
>> fixed-rate (32kHz) clock; if other use cases appear the driver can be
>> updated to provide its own clk_ops etc.
>>
>> When the clock output is not used on a given board, one can prolong
>> the battery life by ensuring that the FOx bits are 0. For the hardware
>> I'm currently working on, the RTC draws 1.2uA with the FOx bits at
>> their default 0001 value, dropping to 0.88uA when those bits are
>> cleared.
> 
> ...
> 
>> +#define ISL12022_INT_FO_MASK	GENMASK(3, 0)
>> +#define ISL12022_INT_FO_OFF	0x0
>> +#define ISL12022_INT_FO_32K	0x1
> 
> A nit-pick. Are they decimal or bit fields? 

-ENOPARSE. A number is a number. Its representation in C code may be
decimal or hexadecimal (or...). And sure, 0 and 0x0 are different
spellings of the same thing. The data sheet lists the possible values in
terms of individual bits, so I suppose I could even do 0b0000 and
0b0001, but that's too unusual (even if perfectly acceptable by gcc).

> To me seems like the 0x can be dropped.

Can, but won't, a single hex digit is more natural way to represent a
four-bit field.

>> +	ret = regmap_update_bits(regmap, ISL12022_REG_INT, ISL12022_INT_FO_MASK, ISL12022_INT_FO_32K);
> 
> Seems too long even for 100 limit.
> Maybe:
> 
> 	ret = regmap_update_bits(regmap, ISL12022_REG_INT,
> 				 ISL12022_INT_FO_MASK, ISL12022_INT_FO_32K);

Sure.

Rasmus