Re: [PATCH] rtc: rs5c372: r2221: fix to use the correct XSTP bit

[Oliver.Rohe]

Hi Alexandre,

On 11.01.19 11:05, Alexandre Belloni wrote:
> Hello,
> 
> On 09/01/2019 10:59:40+0000, Oliver.Rohe@xxxxxxxx wrote:
>> The Ricoh chips have slightly different register layouts
>> and the r2221 chip uses bit 5 as the oscillator halt sensor bit.
>>
>> Signed-off-by: Olive Rohe <oliver.rohe@xxxxxxxx>
>> ---
>>  drivers/rtc/rtc-rs5c372.c | 32 +++++++++++++++++++++-----------
>>  1 file changed, 21 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-rs5c372.c b/drivers/rtc/rtc-rs5c372.c
>> index c503832..ff35dce 100644
>> --- a/drivers/rtc/rtc-rs5c372.c
>> +++ b/drivers/rtc/rtc-rs5c372.c
>> @@ -52,8 +52,8 @@
>>  #	define RS5C_CTRL1_CT4		(4 << 0)	/* 1 Hz level irq */
>>  #define RS5C_REG_CTRL2		15
>>  #	define RS5C372_CTRL2_24		(1 << 5)
>> -#	define R2025_CTRL2_XST		(1 << 5)
>> -#	define RS5C_CTRL2_XSTP		(1 << 4)	/* only if !R2025S/D */
>> +#	define R2x2x_CTRL2_XSTP		(1 << 5)	/* only if R2x2x */
> 
> I wouldn't rename that define as later on, it may be used for RTCs that
> doesn't match R2x2x (as is the case for RV5C387_CTRL1_24 for example).
> the bit doesn't even have the same meaning on R2025 and R2221.
Yes, but R2025 and R2221 have a few registers in common (PON, VDET). The meaning
of XST and XSTP I think are the same, even though they have flipped logic.
> 
>> +#	define RS5C_CTRL2_XSTP		(1 << 4)	/* only if !R2x2x */
>>  #	define RS5C_CTRL2_CTFG		(1 << 2)
>>  #	define RS5C_CTRL2_AAFG		(1 << 1)	/* or WAFG */
>>  #	define RS5C_CTRL2_BAFG		(1 << 0)	/* or DAFG */
>> @@ -519,20 +519,30 @@ static int rs5c_oscillator_setup(struct rs5c372 *rs5c372)
>>  	unsigned char buf[2];
>>  	int addr, i, ret = 0;
>>  
>> -	if (rs5c372->type == rtc_r2025sd) {
>> -		if (rs5c372->regs[RS5C_REG_CTRL2] & R2025_CTRL2_XST)
>> +	addr   = RS5C_ADDR(RS5C_REG_CTRL1);
>> +	buf[0] = rs5c372->regs[RS5C_REG_CTRL1];
>> +	buf[1] = rs5c372->regs[RS5C_REG_CTRL2];
>> +
>> +	/* handle xstp bit */
>> +	switch (rs5c372->type) {
>> +	case rtc_r2025sd:
>> +		if (buf[1] & R2x2x_CTRL2_XSTP)
>>  			return ret;
>> -		rs5c372->regs[RS5C_REG_CTRL2] |= R2025_CTRL2_XST;
>> -	} else {
>> -		if (!(rs5c372->regs[RS5C_REG_CTRL2] & RS5C_CTRL2_XSTP))
>> +		rs5c372->regs[RS5C_REG_CTRL2] |= R2x2x_CTRL2_XSTP;
>> +		break;
>> +	case rtc_r2221tl:
>> +		if (!(buf[1] & R2x2x_CTRL2_XSTP))
>> +			return ret;
>> +		rs5c372->regs[RS5C_REG_CTRL2] &= ~R2x2x_CTRL2_XSTP;
>> +		break;
>> +
>> +	default:
>> +		if (!(buf[1] & RS5C_CTRL2_XSTP))
>>  			return ret;
>>  		rs5c372->regs[RS5C_REG_CTRL2] &= ~RS5C_CTRL2_XSTP;
>> +		break;
>>  	}
>>  
> 
> Note that this is actually quite bad to restart the oscillator on probe.
> The best would be to do that only in rs5c372_rtc_set_time once you know
> the set time is good. then you can return -EINVAL from
> rs5c372_rtc_read_time when you know the oscillator is stopped so
> userspace know the RTC time is bad. This could be done as a follw up
> patch.
Yes, I agree. I have another patch that does exactly what you suggest. We reset
the different bits (XSTP, PON, VDET) in set time and return an error in read
time if the xstp bit is set. I will send it to you.

> There is also more work needed to clean up that driver if you have time
> and are willing to.
Yes sure. With some help, or examples from you guys I can do that.