Subject: Re: [PATCH] rtc: rs5c372: add offset correction support
To:     Camel Guo <Camel.Guo@axis.com>,
        Alessandro Zummo <a.zummo@towertech.it>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>
CC:     kernel <kernel@axis.com>,
        "linux-rtc@vger.kernel.org" <linux-rtc@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20211130095004.22777-1-camel.guo@axis.com>
From:   Camel Guo <camelg@axis.com>
Message-ID: <f0e2eb3c-8e50-f6db-4c52-810da9c83b7a@axis.com>
Date:   Tue, 30 Nov 2021 15:55:43 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.12.0
MIME-Version: 1.0
In-Reply-To: <20211130095004.22777-1-camel.guo@axis.com>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 11/30/21 10:50 AM, Camel Guo wrote:
> From: Camel Guo <camelg@axis.com>
> 
> This commit adds support of offset correction by configuring the
> oscillation adjustment register of rs5c372.
> 
> Signed-off-by: Camel Guo <camelg@axis.com>
> ---
>  ?drivers/rtc/rtc-rs5c372.c | 80 +++++++++++++++++++++++++++++++++++++++
>  ?1 file changed, 80 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-rs5c372.c b/drivers/rtc/rtc-rs5c372.c
> index 80980414890c..77027498cad5 100644
> --- a/drivers/rtc/rtc-rs5c372.c
> +++ b/drivers/rtc/rtc-rs5c372.c
> @@ -30,6 +30,8 @@
>  ?#define RS5C372_REG_TRIM??????? 7
>  ?#?????? define RS5C372_TRIM_XSL???????? 0x80
>  ?#?????? define RS5C372_TRIM_MASK??????? 0x7F
> +#????? define RS5C372_TRIM_DEV???????? (1 << 7)
> +#????? define RS5C372_TRIM_DECR??????? (1 << 6)
> 
>  ?#define RS5C_REG_ALARM_A_MIN??? 8?????????????????????? /* or ALARM_W */
>  ?#define RS5C_REG_ALARM_A_HOURS? 9
> @@ -485,6 +487,82 @@ static int rs5c372_rtc_proc(struct device *dev, 
> struct seq_file *seq)
>  ?#define rs5c372_rtc_proc??????? NULL
>  ?#endif
> 
> +static int rs5c372_read_offset(struct device *dev, long *offset)
> +{
> +?????? struct rs5c372 *rs5c = i2c_get_clientdata(to_i2c_client(dev));
> +?????? int addr = RS5C_ADDR(RS5C372_REG_TRIM);
> +?????? unsigned char val = i2c_smbus_read_byte_data(rs5c->client, addr);
> +?????? long ppb_per_step = (val & RS5C372_TRIM_DEV) ? 1017 : 3052;
> +?????? unsigned char decr = val & RS5C372_TRIM_DECR;
> +
> +?????? /* Only bits[0:5] repsents the time counts */
> +?????? val &= 0x3F;
> +
> +?????? /* If bits[1:5] are all 0, it means no increment or decrement */
> +?????? if (!(val & 0x3E)) {
> +?????????????? *offset = 0;
> +?????? } else {
> +?????????????? if (decr)
> +?????????????????????? *offset = -(((~val) & 0x3F) + 1) * ppb_per_step;
> +?????????????? else
> +?????????????????????? *offset = (val - 1) * ppb_per_step;
> +?????? }
> +
> +?????? return 0;
> +}
> +
> +static int rs5c372_set_offset(struct device *dev, long offset)
> +{
> +?????? struct rs5c372 *rs5c = i2c_get_clientdata(to_i2c_client(dev));
> +?????? int addr = RS5C_ADDR(RS5C372_REG_TRIM);
> +?????? unsigned char val = RS5C372_TRIM_DEV;
> +?????? long steps = 0;
> +
> +?????? /*
> +??????? * Check if it is possible to use high resolution mode (DEV=1). 
> In this
> +??????? * mode, the minimum resolution is 2 / (32768 * 20 * 3), which 
> is about
> +??????? * 1017 ppb
> +??????? */
> +?????? steps = DIV_ROUND_CLOSEST(offset, 1017);
> +?????? if (steps > 0x3E || steps < -0x3E) {
> +?????????????? /*
> +??????????????? * offset is out of the range of high resolution mode. 
> Try to
> +??????????????? * use low resolution mode (DEV=0). In this mode, the 
> minimum
> +??????????????? * resolution is 2 / (32768 * 20), which is about 3052 ppb.
> +??????????????? */
> +?????????????? val &= ~RS5C372_TRIM_DEV;
> +?????????????? steps = DIV_ROUND_CLOSEST(offset, 3052);
> +
> +?????????????? if (steps > 0x3E || steps < -0x3E)
> +?????????????????????? return -ERANGE;
> +?????? }
> +
> +?????? if (steps > 0) {
> +?????????????? val |= steps + 1;
> +?????? } else {
> +?????????????? val |= RS5C372_TRIM_DECR;
> +?????????????? val |= (~(-steps - 1)) & 0x3F;
> +?????? }
> +
> +?????? if (!steps || !(val & 0x3E)) {
> +?????????????? /*
> +??????????????? * if offset is too small, set oscillation adjustment 
> register
> +??????????????? * with the default values, which means no increment or
> +??????????????? * decrement.
> +??????????????? */
> +?????????????? val = 0;
> +?????? }
> +
> +?????? dev_dbg(&rs5c->client->dev, "write 0x%x for offset %ld\n", val, 
> offset);
> +
> +?????? if (i2c_smbus_write_byte_data(rs5c->client, addr, val) < 0) {
> +?????????????? dev_err(&rs5c->client->dev, "failed to write 0x%x to reg 
> %d\n", val, addr);
> +?????????????? return -EIO;
> +?????? }
> +
> +?????? return 0;
> +}
> +
>  ?static const struct rtc_class_ops rs5c372_rtc_ops = {
>  ???????? .proc?????????? = rs5c372_rtc_proc,
>  ???????? .read_time????? = rs5c372_rtc_read_time,
> @@ -492,6 +570,8 @@ static const struct rtc_class_ops rs5c372_rtc_ops = {
>  ???????? .read_alarm???? = rs5c_read_alarm,
>  ???????? .set_alarm????? = rs5c_set_alarm,
>  ???????? .alarm_irq_enable = rs5c_rtc_alarm_irq_enable,
> +?????? .read_offset??? = rs5c372_read_offset,
> +?????? .set_offset???? = rs5c372_set_offset,
>  ?};
> 
>  ?#if IS_ENABLED(CONFIG_RTC_INTF_SYSFS)
> -- 
> 2.20.1
> 

Just realize that the oscillation adjustment registers of r2*, rs5c* are 
not totally same and all of these differences need to be handled in 
different way. Hence this patch needs to be updated to cover all cases.