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

[Alexandre Belloni]

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.

> +#	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.

There is also more work needed to clean up that driver if you have time
and are willing to.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com