[PATCH 0/4] rtc:abx80x: Enable distributed digital calibration

[PATCH 0/4] rtc:abx80x: Enable distributed digital calibration

[Kirill Kapranov]

This patch series enables a Distributed Digital Calibration function for
the RTC of the family. This feature allows to improve the RTC accuracy by
means of compensation an XT oscillator drift. To learn more, see:
AB08XX Series Ultra Low Power RTC IC User's Guide
https://abracon.com/realtimeclock/AB08XX-Application-Manual.pdf

The patches 1 and 2 enable SQW output, that is necessary for subsequent
measurement and computation. However, this feature may be enabled and used
independently, as is.

The patches 3 and 4 enable the XT calibration feature per se. The SQW
output must be enabled for usage of this feature.

TIA!
Kirill

Kirill Kapranov (4):
  dt-bindings: rtc: abracon,abx80x: Add sqw property
  rtc:abx80x: Enable SQW output
  dt-bindings: rtc: abracon,abx80x: Add xt-frequency property
  rtc:abx80x: Enable xt digital calibration

 .../devicetree/bindings/rtc/abracon,abx80x.txt |  25 ++
 drivers/rtc/rtc-abx80x.c                       | 252 +++++++++++++++++++++
 2 files changed, 277 insertions(+)

-- 
2.11.0

[PATCH 1/4] dt-bindings: rtc: abracon,abx80x: Add sqw property

[Kirill Kapranov]

Introduce the string property "sqw" to control Square Wave Output register.
This enables pulse generation output, that is useful for xtal calibration
or for an external usage.

Signed-off-by: Kirill Kapranov <kirill.kapranov@compulab.co.il>
---
 Documentation/devicetree/bindings/rtc/abracon,abx80x.txt | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt b/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt
index 2405e35a1bc0..4c545ece33b5 100644
--- a/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt
+++ b/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt
@@ -29,3 +29,15 @@ and valid to enable charging:
  - "abracon,tc-diode": should be "standard" (0.6V) or "schottky" (0.3V)
  - "abracon,tc-resistor": should be <0>, <3>, <6> or <11>. 0 disables the output
                           resistor, the other values are in kOhm.
+
+The RTC can produce square wave output on a dedicated pin for an external usage
+or for calibration purpose. A valid string should be assigned to the "sqw"
+property to enable the output:
+
+ - "sqw": should be one of the following:
+	"1_century", "32768_Hz", "8192_Hz", "4096_Hz", "2048_Hz", "1024_Hz",
+	"512_Hz", "256_Hz", "128_Hz", "64_Hz", "32_Hz", "16_Hz", "8_Hz", "4_Hz",
+	"2_Hz", "1_Hz",	"1/2_Hz", "1/4_Hz", "1/8_Hz", "1/16_Hz", "1/32_Hz",
+	"1_min", "16384_Hz", "100_Hz", "1_hour", "1_day", "TIRQ", "nTIRQ",
+	"1_year", "1_Hz_to_Counters", "1/32_Hz_from_Acal", "1/8_Hz_from_Acal",
+	"none"
-- 
2.11.0

[PATCH 2/4] rtc: abx80x: Enable SQW output

[Kirill Kapranov]

The RTCs of the family are able to produce square wave output for RTC
calibration purposes or for an external usage.

Signed-off-by: Kirill Kapranov <kirill.kapranov@compulab.co.il>
---
 drivers/rtc/rtc-abx80x.c | 126 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 126 insertions(+)

diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
index 9b0138d07232..153d89b56da9 100644
--- a/drivers/rtc/rtc-abx80x.c
+++ b/drivers/rtc/rtc-abx80x.c
@@ -51,6 +51,9 @@
 #define ABX8XX_IRQ_AIE		BIT(2)
 #define ABX8XX_IRQ_IM_1_4	(0x3 << 5)
 
+#define ABX8XX_REG_SQW		0x13
+#define ABX8XX_SQW_MODE_BITS	5
+
 #define ABX8XX_REG_CD_TIMER_CTL	0x18
 
 #define ABX8XX_REG_OSC		0x1c
@@ -117,6 +120,15 @@ struct abx80x_priv {
 	struct watchdog_device wdog;
 };
 
+union abx8xx_reg_sqw {
+	struct {
+		unsigned int sqws:5;
+		int:2;
+		unsigned int sqwe:2;
+	};
+	int val;
+} __packed;
+
 static int abx80x_write_config_key(struct i2c_client *client, u8 key)
 {
 	if (i2c_smbus_write_byte_data(client, ABX8XX_REG_CFG_KEY, key) < 0) {
@@ -486,9 +498,119 @@ static ssize_t oscillator_show(struct device *dev,
 
 static DEVICE_ATTR_RW(oscillator);
 
+#define SQFS_COUNT (1 << ABX8XX_SQW_MODE_BITS)
+/* The index of the array is the value to be written to the 'Square Wave Function
+ * Select' register to make the RTC generate the required square wave.
+ */
+static const char *const sqfs[SQFS_COUNT] = {
+	"1_century", "32768_Hz", "8192_Hz", "4096_Hz",
+	"2048_Hz", "1024_Hz", "512_Hz", "256_Hz",
+	"128_Hz", "64_Hz", "32_Hz", "16_Hz",
+	"8_Hz", "4_Hz", "2_Hz", "1_Hz",
+	"1/2_Hz", "1/4_Hz", "1/8_Hz", "1/16_Hz",
+	"1/32_Hz", "1_min", "16384_Hz", "100_Hz",
+	"1_hour", "1_day", "TIRQ", "nTIRQ",
+	"1_year", "1_Hz_to_Counters", "1/32_Hz_from_Acal", "1/8_Hz_from_Acal",
+};
+
+static const bool valid_for_rc_mode[SQFS_COUNT] = {
+	true, false, false, false,
+	false, false, false, false,
+	true, true, true, true,
+	true, true, true, true,
+	true, true, true, true,
+	true, true, false, false,
+	true, true, true, true,
+	true, true, true, true,
+};
+
+static int sqw_set(struct i2c_client *client, const char *buf)
+{
+	union abx8xx_reg_sqw reg_sqw;
+	int retval;
+
+	reg_sqw.val = i2c_smbus_read_byte_data(client, ABX8XX_REG_SQW);
+	if (reg_sqw.val < 0)
+		goto err;
+
+	if (sysfs_streq(buf, "none")) {
+		reg_sqw.sqwe = 0;
+		dev_info(&client->dev, "sqw output disabled\n");
+	} else {
+		int idx = __sysfs_match_string(sqfs, SQFS_COUNT, buf);
+
+		if (idx < 0)
+			return idx;
+
+		if (abx80x_is_rc_mode(client) && !valid_for_rc_mode[idx])
+			dev_warn(&client->dev, "sqw frequency %s valid only in xt mode\n",
+				sqfs[idx]);
+
+		dev_info(&client->dev, "sqw output enabled @ %s\n", sqfs[idx]);
+		reg_sqw.sqwe = 1;
+		reg_sqw.sqws = idx;
+	}
+
+	retval = i2c_smbus_write_byte_data(client, ABX8XX_REG_SQW, reg_sqw.val);
+	if (retval < 0)
+		goto err;
+
+	return 0;
+err:
+	dev_err(&client->dev, "Failed to set SQW\n");
+	return retval;
+
+}
+
+static ssize_t sqw_store(struct device *dev,
+			 struct device_attribute *attr,
+			 const char *buf, size_t count)
+{
+	int retval;
+
+	retval = sqw_set(to_i2c_client(dev->parent), buf);
+	if (retval)
+		return retval;
+	else
+		return count;
+}
+
+static ssize_t sqw_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	union abx8xx_reg_sqw reg_sqw;
+	int len = 0;
+	int i;
+
+	reg_sqw.val = i2c_smbus_read_byte_data(client, ABX8XX_REG_SQW);
+	if (reg_sqw.val < 0) {
+		dev_err(dev, "Failed to read SQW\n");
+		sprintf(buf, "\n");
+		return reg_sqw.val;
+	}
+
+	if (reg_sqw.sqwe)
+		len += scnprintf(buf+len, PAGE_SIZE - len, "none ");
+	else
+		len += scnprintf(buf+len, PAGE_SIZE - len, "[none] ");
+
+	for (i = 0; i < SQFS_COUNT; ++i) {
+		if (reg_sqw.sqwe && i == reg_sqw.sqws)
+			len += scnprintf(buf+len, PAGE_SIZE - len, "[%s] ", sqfs[i]);
+		else
+			len += scnprintf(buf+len, PAGE_SIZE - len, "%s ", sqfs[i]);
+	}
+
+	return len;
+}
+
+static DEVICE_ATTR_RW(sqw);
+
 static struct attribute *rtc_calib_attrs[] = {
 	&dev_attr_autocalibration.attr,
 	&dev_attr_oscillator.attr,
+	&dev_attr_sqw.attr,
 	NULL,
 };
 
@@ -686,6 +808,7 @@ static int abx80x_probe(struct i2c_client *client,
 	unsigned int lot;
 	unsigned int wafer;
 	unsigned int uid;
+	const char *sqw_mode_name;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
 		return -ENODEV;
@@ -800,6 +923,9 @@ static int abx80x_probe(struct i2c_client *client,
 		abx80x_enable_trickle_charger(client, trickle_cfg);
 	}
 
+	if (!of_property_read_string(np, "sqw", &sqw_mode_name))
+		sqw_set(client, sqw_mode_name);
+
 	err = i2c_smbus_write_byte_data(client, ABX8XX_REG_CD_TIMER_CTL,
 					BIT(2));
 	if (err)
-- 
2.11.0

[PATCH 3/4] dt-bindings: rtc: abracon,abx80x: Add xt-frequency property

[Kirill Kapranov]

Add the string property "xt-frequency" to control xtal calibration of the
RTC. This allows to improve the RTC accuracy using a Distributed Digital
Calibration function. See: ch. 5.9.1 of
AB08XX Series Ultra Low Power RTC IC User's Guide
https://abracon.com/realtimeclock/AB08XX-Application-Manual.pdf

Signed-off-by: Kirill Kapranov <kirill.kapranov@compulab.co.il>
---
 Documentation/devicetree/bindings/rtc/abracon,abx80x.txt | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt b/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt
index 4c545ece33b5..fac36db7460e 100644
--- a/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt
+++ b/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt
@@ -41,3 +41,16 @@ property to enable the output:
 	"1_min", "16384_Hz", "100_Hz", "1_hour", "1_day", "TIRQ", "nTIRQ",
 	"1_year", "1_Hz_to_Counters", "1/32_Hz_from_Acal", "1/8_Hz_from_Acal",
 	"none"
+
+The RTCs support XT calibration that allows to improve the RTC accuracy.
+To perform the calibration follow the instruction:
+
+	Write the nominal XT frequency (in milliHerz) 32768000 to
+		/sys/class/rtc/your_rtc/xt-frequency
+		to ensure the calibration is not occurring.
+	Select the XT oscillator by writing by writing xtal to
+		/sys/class/rtc/your_rtc/oscillator
+	Configure the square wave SQW output by writing
+		32768_Hz to /sys/class/rtc/your_rtc/sqw
+	Measure the square wave frequency on the output pin in milliHerz.
+	Assign the measured value to the property xt-frequency.
-- 
2.11.0

[PATCH 4/4] rtc:abx80x: Enable xt digital calibration

[Kirill Kapranov]

The XT digital calibration feature allows to improve the RTC accuracy,
using a Distributed Digital Calibration function.
See ch. 5.9.1 of AB08XX Series Ultra Low Power RTC IC User's Guide
https://abracon.com/realtimeclock/AB08XX-Application-Manual.pdf

Signed-off-by: Kirill Kapranov <kirill.kapranov@compulab.co.il>
---
 drivers/rtc/rtc-abx80x.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 145 insertions(+)

diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
index 153d89b56da9..e13fa88e87a5 100644
--- a/drivers/rtc/rtc-abx80x.c
+++ b/drivers/rtc/rtc-abx80x.c
@@ -54,6 +54,9 @@
 #define ABX8XX_REG_SQW		0x13
 #define ABX8XX_SQW_MODE_BITS	5
 
+#define ABX8XX_REG_CAL_XT	0x14
+#define ABX8XX_CAL_XT_OFFSETX_MASK	GENMASK(6, 0)
+
 #define ABX8XX_REG_CD_TIMER_CTL	0x18
 
 #define ABX8XX_REG_OSC		0x1c
@@ -120,6 +123,22 @@ struct abx80x_priv {
 	struct watchdog_device wdog;
 };
 
+union abx8xx_reg_cal_xt {
+	struct {
+		int offsetx:7;
+		unsigned int cmdx:1;
+	};
+	int val;
+} __packed;
+
+union abx8xx_reg_oss {
+	struct {
+		int:6;
+		unsigned int xtcal:2;
+	};
+	int val;
+} __packed;
+
 union abx8xx_reg_sqw {
 	struct {
 		unsigned int sqws:5;
@@ -498,6 +517,123 @@ static ssize_t oscillator_show(struct device *dev,
 
 static DEVICE_ATTR_RW(oscillator);
 
+static const int xt_freq_nom = 32768000; //Nominal XT frequency 32 kHz in mHz
+
+static int xt_frequency_set(struct i2c_client *client, u32 xt_freq)
+{
+	int retval;
+	long Adj;
+	union abx8xx_reg_cal_xt reg_cal_xt;
+	union abx8xx_reg_oss reg_oss;
+
+	retval	= i2c_smbus_read_byte_data(client, ABX8XX_REG_OSS);
+	if (retval < 0)
+		goto err;
+	reg_oss.val = retval;
+
+	Adj = (xt_freq_nom - (int)xt_freq) * 16 / 1000;
+	if (Adj < -320) {
+		dev_err(&client->dev,
+			"The XT oscillator is too fast to be adjusted\n");
+		return -ERANGE;
+	} else if (Adj < -256) {
+		reg_oss.xtcal = 3;
+		reg_cal_xt.cmdx = 1;
+		reg_cal_xt.offsetx = (Adj + 192) / 2;
+	} else if (Adj < -192) {
+		reg_oss.xtcal = 3;
+		reg_cal_xt.cmdx = 0;
+		reg_cal_xt.offsetx = Adj + 192;
+	} else if (Adj < -128) {
+		reg_oss.xtcal = 2;
+		reg_cal_xt.cmdx = 0;
+		reg_cal_xt.offsetx = Adj + 128;
+	} else if (Adj < -64) {
+		reg_oss.xtcal = 1;
+		reg_cal_xt.cmdx = 0;
+		reg_cal_xt.offsetx = Adj + 64;
+	} else if (Adj < 64) {
+		reg_oss.xtcal = 0;
+		reg_cal_xt.cmdx = 0;
+		reg_cal_xt.offsetx = Adj;
+	} else if (Adj < 128) {
+		reg_oss.xtcal = 0;
+		reg_cal_xt.cmdx = 1;
+		reg_cal_xt.offsetx = Adj / 2;
+	} else {
+		dev_err(&client->dev,
+			"The XT oscillator is too slow to be adjusted\n");
+		return -ERANGE;
+	}
+
+	retval = i2c_smbus_write_byte_data(client, ABX8XX_REG_OSS,
+			reg_oss.val);
+	if (retval < 0)
+		goto err;
+
+	retval = i2c_smbus_write_byte_data(client, ABX8XX_REG_CAL_XT,
+			reg_cal_xt.val);
+	if (retval < 0)
+		goto err;
+
+	return 0;
+err:
+	dev_err(&client->dev, "Failed to access calibration data\n");
+	return retval;
+}
+
+static ssize_t xt_frequency_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	int retval;
+	unsigned long xt_freq;
+
+	retval = kstrtoul(buf, 10, &xt_freq);
+	if (retval < 0) {
+		dev_err(dev, "Invalid value of oscillator frequency\n");
+		return -EINVAL;
+	}
+
+	dev_info(dev, "Computed xt osc drift is %li ppm\n",
+		1000000l * ((long)xt_freq - xt_freq_nom) / xt_freq_nom);
+
+	retval = xt_frequency_set(to_i2c_client(dev->parent), xt_freq);
+	if (retval)
+		return retval;
+	return count;
+}
+
+static DEVICE_ATTR_WO(xt_frequency);
+
+static ssize_t xt_calibration_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	int retval;
+	union abx8xx_reg_cal_xt reg_cal_xt;
+	union abx8xx_reg_oss reg_oss;
+	struct i2c_client *client = to_i2c_client(dev->parent);
+
+	retval = i2c_smbus_read_byte_data(client, ABX8XX_REG_OSS);
+	if (retval < 0)
+		goto err;
+	reg_oss.val = retval;
+
+	retval = i2c_smbus_read_byte_data(client, ABX8XX_REG_CAL_XT);
+	if (retval < 0)
+		goto err;
+	reg_cal_xt.val = retval;
+
+	return sprintf(buf, "XTCAL %x\nCMDX %x\nOFFSETX %i = 0x%lx\n",
+			reg_oss.xtcal, reg_cal_xt.cmdx, reg_cal_xt.offsetx,
+			reg_cal_xt.offsetx & ABX8XX_CAL_XT_OFFSETX_MASK);
+err:
+	dev_err(dev, "Failed to read calibration data\n");
+	sprintf(buf, "\n");
+	return retval;
+}
+static DEVICE_ATTR_RO(xt_calibration);
+
 #define SQFS_COUNT (1 << ABX8XX_SQW_MODE_BITS)
 /* The index of the array is the value to be written to the 'Square Wave Function
  * Select' register to make the RTC generate the required square wave.
@@ -611,6 +747,8 @@ static struct attribute *rtc_calib_attrs[] = {
 	&dev_attr_autocalibration.attr,
 	&dev_attr_oscillator.attr,
 	&dev_attr_sqw.attr,
+	&dev_attr_xt_calibration.attr,
+	&dev_attr_xt_frequency.attr,
 	NULL,
 };
 
@@ -809,6 +947,7 @@ static int abx80x_probe(struct i2c_client *client,
 	unsigned int wafer;
 	unsigned int uid;
 	const char *sqw_mode_name;
+	unsigned int xt_freq;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
 		return -ENODEV;
@@ -923,6 +1062,12 @@ static int abx80x_probe(struct i2c_client *client,
 		abx80x_enable_trickle_charger(client, trickle_cfg);
 	}
 
+	if (!of_property_read_u32(np, "xt-frequency", &xt_freq)) {
+		dev_info(&client->dev, "Calibrate XT %d mHz:\n",
+		xt_freq);
+		xt_frequency_set(client, xt_freq);
+	}
+
 	if (!of_property_read_string(np, "sqw", &sqw_mode_name))
 		sqw_set(client, sqw_mode_name);
 
-- 
2.11.0

Re: [PATCH 0/4] rtc:abx80x: Enable distributed digital calibration

[Alexandre Belloni]

Hello,

Thank you for working on that!

On 29/03/2021 00:02:28+0300, Kirill Kapranov wrote:
> This patch series enables a Distributed Digital Calibration function for
> the RTC of the family. This feature allows to improve the RTC accuracy by
> means of compensation an XT oscillator drift. To learn more, see:
> AB08XX Series Ultra Low Power RTC IC User's Guide
> https://abracon.com/realtimeclock/AB08XX-Application-Manual.pdf
> 
> The patches 1 and 2 enable SQW output, that is necessary for subsequent
> measurement and computation. However, this feature may be enabled and used
> independently, as is.
> 

Please use the common clock framework for this part. You will need a
dummy userspace user for this to work but I know Stephen is open to that
as we discussed that use case multiple times already.

> The patches 3 and 4 enable the XT calibration feature per se. The SQW
> output must be enabled for usage of this feature.

Please use the .set_offset and .get_offset rtc_ops for this part.


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

Re: [PATCH 2/4] rtc: abx80x: Enable SQW output

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3600 bytes --]

Hi Kirill,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on abelloni/rtc-next]
[also build test WARNING on v5.12-rc5 next-20210326]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Kirill-Kapranov/rtc-abx80x-Enable-distributed-digital-calibration/20210329-053233
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
config: arm-randconfig-r034-20210329 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 821547cabb5819ed42245376a9afcd11cdee5ddd)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/3f6d456de4f347f0d2fd0af648b1ca21b1212d17
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Kirill-Kapranov/rtc-abx80x-Enable-distributed-digital-calibration/20210329-053233
        git checkout 3f6d456de4f347f0d2fd0af648b1ca21b1212d17
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/rtc/rtc-abx80x.c:533:6: warning: variable 'retval' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
           if (reg_sqw.val < 0)
               ^~~~~~~~~~~~~~~
   drivers/rtc/rtc-abx80x.c:561:9: note: uninitialized use occurs here
           return retval;
                  ^~~~~~
   drivers/rtc/rtc-abx80x.c:533:2: note: remove the 'if' if its condition is always false
           if (reg_sqw.val < 0)
           ^~~~~~~~~~~~~~~~~~~~
   drivers/rtc/rtc-abx80x.c:530:12: note: initialize the variable 'retval' to silence this warning
           int retval;
                     ^
                      = 0
   1 warning generated.


vim +533 drivers/rtc/rtc-abx80x.c

   526	
   527	static int sqw_set(struct i2c_client *client, const char *buf)
   528	{
   529		union abx8xx_reg_sqw reg_sqw;
   530		int retval;
   531	
   532		reg_sqw.val = i2c_smbus_read_byte_data(client, ABX8XX_REG_SQW);
 > 533		if (reg_sqw.val < 0)
   534			goto err;
   535	
   536		if (sysfs_streq(buf, "none")) {
   537			reg_sqw.sqwe = 0;
   538			dev_info(&client->dev, "sqw output disabled\n");
   539		} else {
   540			int idx = __sysfs_match_string(sqfs, SQFS_COUNT, buf);
   541	
   542			if (idx < 0)
   543				return idx;
   544	
   545			if (abx80x_is_rc_mode(client) && !valid_for_rc_mode[idx])
   546				dev_warn(&client->dev, "sqw frequency %s valid only in xt mode\n",
   547					sqfs[idx]);
   548	
   549			dev_info(&client->dev, "sqw output enabled @ %s\n", sqfs[idx]);
   550			reg_sqw.sqwe = 1;
   551			reg_sqw.sqws = idx;
   552		}
   553	
   554		retval = i2c_smbus_write_byte_data(client, ABX8XX_REG_SQW, reg_sqw.val);
   555		if (retval < 0)
   556			goto err;
   557	
   558		return 0;
   559	err:
   560		dev_err(&client->dev, "Failed to set SQW\n");
   561		return retval;
   562	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32508 bytes --]

Re: [PATCH 2/4] rtc: abx80x: Enable SQW output

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 3682 bytes --]

Hi Kirill,

url:    https://github.com/0day-ci/linux/commits/Kirill-Kapranov/rtc-abx80x-Enable-distributed-digital-calibration/20210329-053233
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
config: i386-randconfig-m021-20210328 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/rtc/rtc-abx80x.c:561 sqw_set() error: uninitialized symbol 'retval'.

vim +/retval +561 drivers/rtc/rtc-abx80x.c

3f6d456de4f347 Kirill Kapranov 2021-03-29  527  static int sqw_set(struct i2c_client *client, const char *buf)
3f6d456de4f347 Kirill Kapranov 2021-03-29  528  {
3f6d456de4f347 Kirill Kapranov 2021-03-29  529  	union abx8xx_reg_sqw reg_sqw;
3f6d456de4f347 Kirill Kapranov 2021-03-29  530  	int retval;
3f6d456de4f347 Kirill Kapranov 2021-03-29  531  
3f6d456de4f347 Kirill Kapranov 2021-03-29  532  	reg_sqw.val = i2c_smbus_read_byte_data(client, ABX8XX_REG_SQW);
3f6d456de4f347 Kirill Kapranov 2021-03-29  533  	if (reg_sqw.val < 0)
3f6d456de4f347 Kirill Kapranov 2021-03-29  534  		goto err;

"retval" not set.  Forgetting to set the error code is the canonical
bug for do nothing gotos like this.

3f6d456de4f347 Kirill Kapranov 2021-03-29  535  
3f6d456de4f347 Kirill Kapranov 2021-03-29  536  	if (sysfs_streq(buf, "none")) {
3f6d456de4f347 Kirill Kapranov 2021-03-29  537  		reg_sqw.sqwe = 0;
3f6d456de4f347 Kirill Kapranov 2021-03-29  538  		dev_info(&client->dev, "sqw output disabled\n");
3f6d456de4f347 Kirill Kapranov 2021-03-29  539  	} else {
3f6d456de4f347 Kirill Kapranov 2021-03-29  540  		int idx = __sysfs_match_string(sqfs, SQFS_COUNT, buf);
3f6d456de4f347 Kirill Kapranov 2021-03-29  541  
3f6d456de4f347 Kirill Kapranov 2021-03-29  542  		if (idx < 0)
3f6d456de4f347 Kirill Kapranov 2021-03-29  543  			return idx;
                                                                        ^^^^^^^^^^^
These are direct returns.  Just do direct returns everywhere (more
readably, fewer bugs).

3f6d456de4f347 Kirill Kapranov 2021-03-29  544  
3f6d456de4f347 Kirill Kapranov 2021-03-29  545  		if (abx80x_is_rc_mode(client) && !valid_for_rc_mode[idx])
3f6d456de4f347 Kirill Kapranov 2021-03-29  546  			dev_warn(&client->dev, "sqw frequency %s valid only in xt mode\n",
3f6d456de4f347 Kirill Kapranov 2021-03-29  547  				sqfs[idx]);
3f6d456de4f347 Kirill Kapranov 2021-03-29  548  
3f6d456de4f347 Kirill Kapranov 2021-03-29  549  		dev_info(&client->dev, "sqw output enabled @ %s\n", sqfs[idx]);
3f6d456de4f347 Kirill Kapranov 2021-03-29  550  		reg_sqw.sqwe = 1;
3f6d456de4f347 Kirill Kapranov 2021-03-29  551  		reg_sqw.sqws = idx;
3f6d456de4f347 Kirill Kapranov 2021-03-29  552  	}
3f6d456de4f347 Kirill Kapranov 2021-03-29  553  
3f6d456de4f347 Kirill Kapranov 2021-03-29  554  	retval = i2c_smbus_write_byte_data(client, ABX8XX_REG_SQW, reg_sqw.val);
3f6d456de4f347 Kirill Kapranov 2021-03-29  555  	if (retval < 0)
3f6d456de4f347 Kirill Kapranov 2021-03-29  556  		goto err;
3f6d456de4f347 Kirill Kapranov 2021-03-29  557  
3f6d456de4f347 Kirill Kapranov 2021-03-29  558  	return 0;
3f6d456de4f347 Kirill Kapranov 2021-03-29  559  err:
3f6d456de4f347 Kirill Kapranov 2021-03-29  560  	dev_err(&client->dev, "Failed to set SQW\n");
3f6d456de4f347 Kirill Kapranov 2021-03-29 @561  	return retval;
                                                        ^^^^^^^^^^^^^

3f6d456de4f347 Kirill Kapranov 2021-03-29  562  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35066 bytes --]

Re: [PATCH 2/4] rtc: abx80x: Enable SQW output

[Kirill Kapranov]

Hi Dan,

Thank you very much for pointing out the issues.
However, I'm not sure that the code, you have analyzed, will be a part 
of final patch set. I intend to redesign all the code deeply, as 
Alexandre Belloni suggested [1].

Thank you again!

-- 
Best Regards,
Kirill Kapranov
Software Engineer
CompuLab Ltd.
[1] https://marc.info/?l=linux-rtc&m=161696606727215&w=2

On 3/29/21 9:19 AM, Dan Carpenter wrote:
> Hi Kirill,
> 
> url:    https://github.com/0day-ci/linux/commits/Kirill-Kapranov/rtc-abx80x-Enable-distributed-digital-calibration/20210329-053233
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
> config: i386-randconfig-m021-20210328 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> smatch warnings:
> drivers/rtc/rtc-abx80x.c:561 sqw_set() error: uninitialized symbol 'retval'.
> 
> vim +/retval +561 drivers/rtc/rtc-abx80x.c
> 
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  527  static int sqw_set(struct i2c_client *client, const char *buf)
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  528  {
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  529  	union abx8xx_reg_sqw reg_sqw;
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  530  	int retval;
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  531
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  532  	reg_sqw.val = i2c_smbus_read_byte_data(client, ABX8XX_REG_SQW);
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  533  	if (reg_sqw.val < 0)
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  534  		goto err;
> 
> "retval" not set.  Forgetting to set the error code is the canonical
> bug for do nothing gotos like this.
> 
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  535
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  536  	if (sysfs_streq(buf, "none")) {
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  537  		reg_sqw.sqwe = 0;
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  538  		dev_info(&client->dev, "sqw output disabled\n");
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  539  	} else {
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  540  		int idx = __sysfs_match_string(sqfs, SQFS_COUNT, buf);
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  541
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  542  		if (idx < 0)
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  543  			return idx;
>                                                                          ^^^^^^^^^^^
> These are direct returns.  Just do direct returns everywhere (more
> readably, fewer bugs).
> 
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  544
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  545  		if (abx80x_is_rc_mode(client) && !valid_for_rc_mode[idx])
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  546  			dev_warn(&client->dev, "sqw frequency %s valid only in xt mode\n",
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  547  				sqfs[idx]);
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  548
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  549  		dev_info(&client->dev, "sqw output enabled @ %s\n", sqfs[idx]);
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  550  		reg_sqw.sqwe = 1;
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  551  		reg_sqw.sqws = idx;
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  552  	}
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  553
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  554  	retval = i2c_smbus_write_byte_data(client, ABX8XX_REG_SQW, reg_sqw.val);
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  555  	if (retval < 0)
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  556  		goto err;
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  557
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  558  	return 0;
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  559  err:
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  560  	dev_err(&client->dev, "Failed to set SQW\n");
> 3f6d456de4f347 Kirill Kapranov 2021-03-29 @561  	return retval;
>                                                          ^^^^^^^^^^^^^
> 
> 3f6d456de4f347 Kirill Kapranov 2021-03-29  562
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> 

Re: [PATCH 4/4] rtc:abx80x: Enable xt digital calibration

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 971 bytes --]

Hi!

> The XT digital calibration feature allows to improve the RTC accuracy,
> using a Distributed Digital Calibration function.
> See ch. 5.9.1 of AB08XX Series Ultra Low Power RTC IC User's Guide
> https://abracon.com/realtimeclock/AB08XX-Application-Manual.pdf
> 
> Signed-off-by: Kirill Kapranov <kirill.kapranov@compulab.co.il>
> ---
>  drivers/rtc/rtc-abx80x.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 145 insertions(+)


>  
> +static const int xt_freq_nom = 32768000; //Nominal XT frequency 32 kHz in mHz

C-style comment?

is it 32 kHz or 32.768 kHz?

> +{
> +	int retval;
> +	long Adj;

Adj -> adj.

> +
> +static DEVICE_ATTR_WO(xt_frequency);

You are adding new user interface, you sould add documentation.

Best regards,
								Pavel
								
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 4/4] rtc:abx80x: Enable xt digital calibration

[Kirill Kapranov]

Hi Pavel,

Thank you very much for pointing out the issues.
I redesign all the code deeply, as Alexandre Belloni suggested [1], and 
the lines you have mentioned are to be fixed too.
Unfortunately, it takes more time than I supposed.

Thank you again!

-- 
Best Regards,
Kirill Kapranov
Software Engineer
CompuLab Ltd.
[1] https://marc.info/?l=linux-rtc&m=161696606727215&w=2

On 4/9/21 1:46 PM, Pavel Machek wrote:
> Hi!
> 
>> The XT digital calibration feature allows to improve the RTC accuracy,
>> using a Distributed Digital Calibration function.
>> See ch. 5.9.1 of AB08XX Series Ultra Low Power RTC IC User's Guide
>> https://abracon.com/realtimeclock/AB08XX-Application-Manual.pdf
>>
>> Signed-off-by: Kirill Kapranov <kirill.kapranov@compulab.co.il>
>> ---
>>   drivers/rtc/rtc-abx80x.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 145 insertions(+)
> 
> 
>>   
>> +static const int xt_freq_nom = 32768000; //Nominal XT frequency 32 kHz in mHz
> 
> C-style comment?
> 
> is it 32 kHz or 32.768 kHz?
> 
>> +{
>> +	int retval;
>> +	long Adj;
> 
> Adj -> adj.
> 
>> +
>> +static DEVICE_ATTR_WO(xt_frequency);
> 
> You are adding new user interface, you sould add documentation.
> 
> Best regards,
> 								Pavel
> 								
> 


-- 
Best Regards,
Kirill Kapranov
Software Engineer
CompuLab Ltd.
Tel.: (+972) 48-290-100.Ext 227
Cel.: (+972) 53-9332508