[PATCH 00/13] DS1341 support and code cleanup

[PATCH 00/13] DS1341 support and code cleanup

[Andrey Smirnov]

Hi everyone,

This patch set contains code to add support for DS1341 variant of the
chip, as well as code to support enabling/disabling some of its power
savings features. Lastly the set contains a number of code cleanups
intended to improve the readability of driver's code.

Any feedback is appreciated!

Thank you,
Andrey Smirnov

Andrey Smirnov (13):
  RTC: ds1307: Add DS1341 variant
  RTC: ds1307: Disable square wave and timers as default
  RTC: ds1307: Add DS1341 specific power-saving options
  RTC: ds1307: Convert ds1307_can_wakeup_device into a predicate
  RTC: ds1307: Convert want_irq into a predicate
  RTC: ds1307: Move chip configuration into a separate routine
  RTC: ds1307: Move chip sanity checking into a separate routine
  RTC: ds1307: Remove register "cache"
  RTC: ds1307: Constify struct ds1307 where possible
  RTC: ds1307: Convert goto to a loop
  RTC: ds1307: Redefine RX8025_REG_* to minimize extra code
  RTC: ds1307: Report oscillator problems more intelligently
  RTC: ds1307: Move last bits of sanity checking out of chip_configure

 .../devicetree/bindings/rtc/dallas,ds1341.txt      |  23 +
 drivers/rtc/rtc-ds1307.c                           | 742 ++++++++++++---------
 2 files changed, 457 insertions(+), 308 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/dallas,ds1341.txt

-- 
2.5.5

[PATCH 01/13] RTC: ds1307: Add DS1341 variant

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/rtc/rtc-ds1307.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 821d9c0..7e65e2e 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -35,6 +35,7 @@ enum ds_type {
 	ds_1338,
 	ds_1339,
 	ds_1340,
+	ds_1341,
 	ds_1388,
 	ds_3231,
 	m41t00,
@@ -178,6 +179,7 @@ static const struct i2c_device_id ds1307_id[] = {
 	{ "ds1337", ds_1337 },
 	{ "ds1338", ds_1338 },
 	{ "ds1339", ds_1339 },
+	{ "ds1341", ds_1341 },
 	{ "ds1388", ds_1388 },
 	{ "ds1340", ds_1340 },
 	{ "ds3231", ds_3231 },
@@ -424,6 +426,7 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
 	case ds_1337:
 	case ds_1339:
 	case ds_3231:
+	case ds_1341:
 		buf[DS1307_REG_MONTH] |= DS1337_BIT_CENTURY;
 		break;
 	case ds_1340:
@@ -1242,6 +1245,7 @@ static int ds1307_probe(struct i2c_client *client,
 
 	static const int	bbsqi_bitpos[] = {
 		[ds_1337] = 0,
+		[ds_1341] = 0,
 		[ds_1339] = DS1339_BIT_BBSQI,
 		[ds_3231] = DS3231_BIT_BBSQW,
 	};
@@ -1301,6 +1305,7 @@ static int ds1307_probe(struct i2c_client *client,
 	case ds_1337:
 	case ds_1339:
 	case ds_3231:
+	case ds_1341:
 		/* get registers that the "rtc" read below won't read... */
 		tmp = ds1307->read_block_data(ds1307->client,
 				DS1337_REG_CONTROL, 2, buf);
-- 
2.5.5

[PATCH 02/13] RTC: ds1307: Disable square wave and timers as default

[Andrey Smirnov]

Disable square wave and timers as default for DS1337/39/41 and
DS3231. The rationale being that configuring a chip this way puts it
into a known state with lower power consumption. While it is not very
likely it is still possible that the code controlling RTCs that ran
before this driver configured it to produce square wave and left it in
such a state.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/rtc/rtc-ds1307.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 7e65e2e..c618c22 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -1320,19 +1320,17 @@ static int ds1307_probe(struct i2c_client *client,
 			ds1307->regs[0] &= ~DS1337_BIT_nEOSC;
 
 		/*
-		 * Using IRQ or defined as wakeup-source?
 		 * Disable the square wave and both alarms.
 		 * For some variants, be sure alarms can trigger when we're
 		 * running on Vbackup (BBSQI/BBSQW)
 		 */
-		if (chip->alarm && (ds1307->client->irq > 0 ||
-						ds1307_can_wakeup_device)) {
-			ds1307->regs[0] |= DS1337_BIT_INTCN
-					| bbsqi_bitpos[ds1307->type];
-			ds1307->regs[0] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);
+		ds1307->regs[0] |= DS1337_BIT_INTCN
+			| bbsqi_bitpos[ds1307->type];
+		ds1307->regs[0] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);
 
+		if (chip->alarm && (ds1307->client->irq > 0 ||
+				    ds1307_can_wakeup_device))
 			want_irq = true;
-		}
 
 		i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
 							ds1307->regs[0]);
-- 
2.5.5

[PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options

[Andrey Smirnov]

Add DS1341 specific power-saving options that allow to disable certain
functional aspects of the chip in order to minimize its power
consumption.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 .../devicetree/bindings/rtc/dallas,ds1341.txt      | 23 ++++++++++++++++++
 drivers/rtc/rtc-ds1307.c                           | 28 ++++++++++++++++++++++
 2 files changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/dallas,ds1341.txt

diff --git a/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
new file mode 100644
index 0000000..b8be7a4
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
@@ -0,0 +1,23 @@
+* Dallas DS1341		I2C Serial Real-Time Clock
+
+Required properties:
+
+- compatible: Should contain "dallas,ds1341".
+
+- reg: I2C address for chip
+
+Optional properties:
+
+- disable-oscillator-stop-flag : Configure chip to disable oscillator
+  fault detection circuitry
+
+- enable-glitch-filter : Configure chip to enable crystal oscillator
+  output glitch filtering
+
+Example:
+	ds1341: rtc@68 {
+		compatible = "dallas,ds1341";
+		disable-oscillator-stop-flag;
+		enable-glitch-filter;
+		reg = <0x68>;
+	};
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index c618c22..54cc527 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -78,6 +78,7 @@ enum ds_type {
 #define DS1337_REG_CONTROL	0x0e
 #	define DS1337_BIT_nEOSC		0x80
 #	define DS1339_BIT_BBSQI		0x20
+#	define DS1341_BIT_EGFIL		0x20
 #	define DS3231_BIT_BBSQW		0x40 /* same as BBSQI */
 #	define DS1337_BIT_RS2		0x10
 #	define DS1337_BIT_RS1		0x08
@@ -93,7 +94,9 @@ enum ds_type {
 #	define DS1340_BIT_OSF		0x80
 #define DS1337_REG_STATUS	0x0f
 #	define DS1337_BIT_OSF		0x80
+#	define DS1341_BIT_DOSF		0x40
 #	define DS3231_BIT_EN32KHZ	0x08
+#	define DS1341_BIT_ECLK		0x04
 #	define DS1337_BIT_A2I		0x02
 #	define DS1337_BIT_A1I		0x01
 #define DS1339_REG_ALARM1_SECS	0x07
@@ -1319,6 +1322,31 @@ static int ds1307_probe(struct i2c_client *client,
 		if (ds1307->regs[0] & DS1337_BIT_nEOSC)
 			ds1307->regs[0] &= ~DS1337_BIT_nEOSC;
 
+		if (ds1307->type == ds_1341) {
+			/* Make sure we are not generating square wave
+			 * output */
+			ds1307->regs[1] &= ~DS1341_BIT_ECLK;
+
+			if (of_property_read_bool(client->dev.of_node,
+						  "disable-oscillator-stop-flag"))
+				ds1307->regs[1] |= DS1341_BIT_DOSF;
+			else
+				ds1307->regs[1] &= ~DS1341_BIT_DOSF;
+
+			if (of_property_read_bool(client->dev.of_node,
+						  "enable-glitch-filter"))
+				ds1307->regs[0] |= DS1341_BIT_EGFIL;
+			else
+				ds1307->regs[0] &= ~DS1341_BIT_EGFIL;
+
+			/*
+			 * Write status register. Control register
+			 * would be set by the code below
+			 */
+			i2c_smbus_write_byte_data(client, DS1337_REG_STATUS,
+						  ds1307->regs[1]);
+		}
+
 		/*
 		 * Disable the square wave and both alarms.
 		 * For some variants, be sure alarms can trigger when we're
-- 
2.5.5

[PATCH 04/13] RTC: ds1307: Convert ds1307_can_wakeup_device into a predicate

[Andrey Smirnov]

Convert ds1307_can_wakeup_device in ds1307_probe into a predicate in
order to make various chunks of code in that function less connected to
each other.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/rtc/rtc-ds1307.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 54cc527..c482d8b 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -1232,6 +1232,21 @@ static void ds1307_clks_register(struct ds1307 *ds1307)
 
 #endif /* CONFIG_COMMON_CLK */
 
+static bool ds1307_can_wakeup_device(const struct ds1307 *ds1307)
+{
+
+/*
+ * For devices with no IRQ directly connected to the SoC, the RTC chip
+ * can be forced as a wakeup source by stating that explicitly in
+ * the device's .dts file using the "wakeup-source" boolean property.
+ * If the "wakeup-source" property is set, don't request an IRQ.
+ * This will guarantee the 'wakealarm' sysfs entry is available on the device,
+ * if supported by the RTC.
+ */
+	return of_property_read_bool(ds1307->client->dev.of_node,
+				     "wakeup-source");
+}
+
 static int ds1307_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -1241,7 +1256,6 @@ static int ds1307_probe(struct i2c_client *client,
 	struct chip_desc	*chip = &chips[id->driver_data];
 	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
 	bool			want_irq = false;
-	bool			ds1307_can_wakeup_device = false;
 	unsigned char		*buf;
 	struct ds1307_platform_data *pdata = dev_get_platdata(&client->dev);
 	irq_handler_t	irq_handler = ds1307_irq;
@@ -1290,20 +1304,6 @@ static int ds1307_probe(struct i2c_client *client,
 		ds1307->write_block_data = ds1307_write_block_data;
 	}
 
-#ifdef CONFIG_OF
-/*
- * For devices with no IRQ directly connected to the SoC, the RTC chip
- * can be forced as a wakeup source by stating that explicitly in
- * the device's .dts file using the "wakeup-source" boolean property.
- * If the "wakeup-source" property is set, don't request an IRQ.
- * This will guarantee the 'wakealarm' sysfs entry is available on the device,
- * if supported by the RTC.
- */
-	if (of_property_read_bool(client->dev.of_node, "wakeup-source")) {
-		ds1307_can_wakeup_device = true;
-	}
-#endif
-
 	switch (ds1307->type) {
 	case ds_1337:
 	case ds_1339:
@@ -1357,7 +1357,7 @@ static int ds1307_probe(struct i2c_client *client,
 		ds1307->regs[0] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);
 
 		if (chip->alarm && (ds1307->client->irq > 0 ||
-				    ds1307_can_wakeup_device))
+				    ds1307_can_wakeup_device(ds1307)))
 			want_irq = true;
 
 		i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
@@ -1567,7 +1567,8 @@ read_rtc:
 		return PTR_ERR(ds1307->rtc);
 	}
 
-	if (ds1307_can_wakeup_device && ds1307->client->irq <= 0) {
+	if (ds1307_can_wakeup_device(ds1307) &&
+	    ds1307->client->irq <= 0) {
 		/* Disable request for an IRQ */
 		want_irq = false;
 		dev_info(&client->dev, "'wakeup-source' is set, request for an IRQ is disabled!\n");
-- 
2.5.5

[PATCH 05/13] RTC: ds1307: Convert want_irq into a predicate

[Andrey Smirnov]

Convert want_irq variable into a predicate in order to detangle various
independent chunks of ds1307_probe().

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/rtc/rtc-ds1307.c | 44 +++++++++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index c482d8b..81967c1 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -1247,6 +1247,30 @@ static bool ds1307_can_wakeup_device(const struct ds1307 *ds1307)
 				     "wakeup-source");
 }
 
+static bool ds1307_want_irq(const struct ds1307 *ds1307,
+			    const struct chip_desc *chip)
+{
+
+
+	if (chip->alarm) {
+		switch (ds1307->type) {
+		case ds_1337:
+		case ds_1339:
+		case ds_3231:
+		case ds_1341:
+			return (ds1307->client->irq > 0 ||
+				ds1307_can_wakeup_device(ds1307));
+
+		case mcp794xx:
+			return (ds1307->client->irq > 0);
+		default:
+			break;
+		}
+	}
+
+	return false;
+}
+
 static int ds1307_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -1255,7 +1279,6 @@ static int ds1307_probe(struct i2c_client *client,
 	int			tmp;
 	struct chip_desc	*chip = &chips[id->driver_data];
 	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
-	bool			want_irq = false;
 	unsigned char		*buf;
 	struct ds1307_platform_data *pdata = dev_get_platdata(&client->dev);
 	irq_handler_t	irq_handler = ds1307_irq;
@@ -1356,10 +1379,6 @@ static int ds1307_probe(struct i2c_client *client,
 			| bbsqi_bitpos[ds1307->type];
 		ds1307->regs[0] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);
 
-		if (chip->alarm && (ds1307->client->irq > 0 ||
-				    ds1307_can_wakeup_device(ds1307)))
-			want_irq = true;
-
 		i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
 							ds1307->regs[0]);
 
@@ -1441,10 +1460,7 @@ static int ds1307_probe(struct i2c_client *client,
 		break;
 	case mcp794xx:
 		rtc_ops = &mcp794xx_rtc_ops;
-		if (ds1307->client->irq > 0 && chip->alarm) {
-			irq_handler = mcp794xx_irq;
-			want_irq = true;
-		}
+		irq_handler = mcp794xx_irq;
 		break;
 	default:
 		break;
@@ -1557,7 +1573,7 @@ read_rtc:
 				bin2bcd(tmp));
 	}
 
-	if (want_irq) {
+	if (ds1307_want_irq(ds1307, chip)) {
 		device_set_wakeup_capable(&client->dev, true);
 		set_bit(HAS_ALARM, &ds1307->flags);
 	}
@@ -1570,13 +1586,11 @@ read_rtc:
 	if (ds1307_can_wakeup_device(ds1307) &&
 	    ds1307->client->irq <= 0) {
 		/* Disable request for an IRQ */
-		want_irq = false;
-		dev_info(&client->dev, "'wakeup-source' is set, request for an IRQ is disabled!\n");
+		dev_info(&client->dev,
+			 "'wakeup-source' is set, request for an IRQ is disabled!\n");
 		/* We cannot support UIE mode if we do not have an IRQ line */
 		ds1307->rtc->uie_unsupported = 1;
-	}
-
-	if (want_irq) {
+	} else if (ds1307_want_irq(ds1307, chip)) {
 		err = devm_request_threaded_irq(&client->dev,
 						client->irq, NULL, irq_handler,
 						IRQF_SHARED | IRQF_ONESHOT,
-- 
2.5.5

[PATCH 06/13] RTC: ds1307: Move chip configuration into a separate routine

[Andrey Smirnov]

Move chip configuration into a separate routine to improve readablity of
the code.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/rtc/rtc-ds1307.c | 172 ++++++++++++++++++++++++++---------------------
 1 file changed, 97 insertions(+), 75 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 81967c1..df8c78a 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -1271,74 +1271,32 @@ static bool ds1307_want_irq(const struct ds1307 *ds1307,
 	return false;
 }
 
-static int ds1307_probe(struct i2c_client *client,
-			const struct i2c_device_id *id)
+static int ds1307_chip_configure(struct ds1307 *ds1307)
 {
-	struct ds1307		*ds1307;
-	int			err = -ENODEV;
-	int			tmp;
-	struct chip_desc	*chip = &chips[id->driver_data];
-	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
-	unsigned char		*buf;
-	struct ds1307_platform_data *pdata = dev_get_platdata(&client->dev);
-	irq_handler_t	irq_handler = ds1307_irq;
-
-	static const int	bbsqi_bitpos[] = {
-		[ds_1337] = 0,
-		[ds_1341] = 0,
-		[ds_1339] = DS1339_BIT_BBSQI,
-		[ds_3231] = DS3231_BIT_BBSQW,
-	};
-	const struct rtc_class_ops *rtc_ops = &ds13xx_rtc_ops;
-
-	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)
-	    && !i2c_check_functionality(adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
-		return -EIO;
-
-	ds1307 = devm_kzalloc(&client->dev, sizeof(struct ds1307), GFP_KERNEL);
-	if (!ds1307)
-		return -ENOMEM;
-
-	i2c_set_clientdata(client, ds1307);
-
-	ds1307->client	= client;
-	ds1307->type	= id->driver_data;
-
-	if (!pdata && client->dev.of_node)
-		ds1307_trickle_of_init(client, chip);
-	else if (pdata && pdata->trickle_charger_setup)
-		chip->trickle_charger_setup = pdata->trickle_charger_setup;
-
-	if (chip->trickle_charger_setup && chip->trickle_charger_reg) {
-		dev_dbg(&client->dev, "writing trickle charger info 0x%x to 0x%x\n",
-		    DS13XX_TRICKLE_CHARGER_MAGIC | chip->trickle_charger_setup,
-		    chip->trickle_charger_reg);
-		i2c_smbus_write_byte_data(client, chip->trickle_charger_reg,
-		    DS13XX_TRICKLE_CHARGER_MAGIC |
-		    chip->trickle_charger_setup);
-	}
+	int tmp;
+	unsigned char *buf;
+	struct i2c_client *client = ds1307->client;
 
 	buf = ds1307->regs;
-	if (i2c_check_functionality(adapter, I2C_FUNC_SMBUS_I2C_BLOCK)) {
-		ds1307->read_block_data = ds1307_native_smbus_read_block_data;
-		ds1307->write_block_data = ds1307_native_smbus_write_block_data;
-	} else {
-		ds1307->read_block_data = ds1307_read_block_data;
-		ds1307->write_block_data = ds1307_write_block_data;
-	}
 
 	switch (ds1307->type) {
 	case ds_1337:
 	case ds_1339:
 	case ds_3231:
-	case ds_1341:
+	case ds_1341: {
+		static const int bbsqi_bitpos[] = {
+			[ds_1337] = 0,
+			[ds_1341] = 0,
+			[ds_1339] = DS1339_BIT_BBSQI,
+			[ds_3231] = DS3231_BIT_BBSQW,
+		};
+
 		/* get registers that the "rtc" read below won't read... */
-		tmp = ds1307->read_block_data(ds1307->client,
-				DS1337_REG_CONTROL, 2, buf);
+		tmp = ds1307->read_block_data(client,
+					      DS1337_REG_CONTROL, 2, buf);
 		if (tmp != 2) {
-			dev_dbg(&client->dev, "read error %d\n", tmp);
-			err = -EIO;
-			goto exit;
+			dev_dbg(&ds1307->client->dev, "read error %d\n", tmp);
+			return -EIO;
 		}
 
 		/* oscillator off?  turn it on, so clock can tick. */
@@ -1366,7 +1324,8 @@ static int ds1307_probe(struct i2c_client *client,
 			 * Write status register. Control register
 			 * would be set by the code below
 			 */
-			i2c_smbus_write_byte_data(client, DS1337_REG_STATUS,
+			i2c_smbus_write_byte_data(client,
+						  DS1337_REG_STATUS,
 						  ds1307->regs[1]);
 		}
 
@@ -1379,24 +1338,26 @@ static int ds1307_probe(struct i2c_client *client,
 			| bbsqi_bitpos[ds1307->type];
 		ds1307->regs[0] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);
 
-		i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
-							ds1307->regs[0]);
+		i2c_smbus_write_byte_data(client,
+					  DS1337_REG_CONTROL,
+					  ds1307->regs[0]);
 
 		/* oscillator fault?  clear flag, and warn */
 		if (ds1307->regs[1] & DS1337_BIT_OSF) {
-			i2c_smbus_write_byte_data(client, DS1337_REG_STATUS,
-				ds1307->regs[1] & ~DS1337_BIT_OSF);
-			dev_warn(&client->dev, "SET TIME!\n");
+			i2c_smbus_write_byte_data(client,
+						  DS1337_REG_STATUS,
+						  ds1307->regs[1] & ~DS1337_BIT_OSF);
+			dev_warn(&ds1307->client->dev, "SET TIME!\n");
 		}
 		break;
-
+	}
 	case rx_8025:
-		tmp = i2c_smbus_read_i2c_block_data(ds1307->client,
-				RX8025_REG_CTRL1 << 4 | 0x08, 2, buf);
+		tmp = i2c_smbus_read_i2c_block_data(client,
+						    RX8025_REG_CTRL1 << 4 | 0x08,
+						    2, buf);
 		if (tmp != 2) {
 			dev_dbg(&client->dev, "read error %d\n", tmp);
-			err = -EIO;
-			goto exit;
+			return -EIO;
 		}
 
 		/* oscillator off?  turn it on, so clock can tick. */
@@ -1432,15 +1393,14 @@ static int ds1307_probe(struct i2c_client *client,
 			/* switch to 24 hour mode */
 			i2c_smbus_write_byte_data(client,
 						  RX8025_REG_CTRL1 << 4 | 0x08,
-						  ds1307->regs[0] |
-						  RX8025_BIT_2412);
+						  ds1307->regs[0] | RX8025_BIT_2412);
 
-			tmp = i2c_smbus_read_i2c_block_data(ds1307->client,
-					RX8025_REG_CTRL1 << 4 | 0x08, 2, buf);
+			tmp = i2c_smbus_read_i2c_block_data(client,
+							    RX8025_REG_CTRL1 << 4 | 0x08,
+							    2, buf);
 			if (tmp != 2) {
 				dev_dbg(&client->dev, "read error %d\n", tmp);
-				err = -EIO;
-				goto exit;
+				return -EIO;
 			}
 
 			/* correct hour */
@@ -1455,6 +1415,68 @@ static int ds1307_probe(struct i2c_client *client,
 						  hour);
 		}
 		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int ds1307_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct ds1307		*ds1307;
+	int			err = -ENODEV;
+	int			tmp;
+	struct chip_desc	*chip = &chips[id->driver_data];
+	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
+	unsigned char		*buf;
+	struct ds1307_platform_data *pdata = dev_get_platdata(&client->dev);
+	irq_handler_t	irq_handler = ds1307_irq;
+
+	const struct rtc_class_ops *rtc_ops = &ds13xx_rtc_ops;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)
+	    && !i2c_check_functionality(adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
+		return -EIO;
+
+	ds1307 = devm_kzalloc(&client->dev, sizeof(struct ds1307), GFP_KERNEL);
+	if (!ds1307)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, ds1307);
+
+	ds1307->client	= client;
+	ds1307->type	= id->driver_data;
+
+	if (!pdata && client->dev.of_node)
+		ds1307_trickle_of_init(client, chip);
+	else if (pdata && pdata->trickle_charger_setup)
+		chip->trickle_charger_setup = pdata->trickle_charger_setup;
+
+	if (chip->trickle_charger_setup && chip->trickle_charger_reg) {
+		dev_dbg(&client->dev, "writing trickle charger info 0x%x to 0x%x\n",
+		    DS13XX_TRICKLE_CHARGER_MAGIC | chip->trickle_charger_setup,
+		    chip->trickle_charger_reg);
+		i2c_smbus_write_byte_data(client, chip->trickle_charger_reg,
+		    DS13XX_TRICKLE_CHARGER_MAGIC |
+		    chip->trickle_charger_setup);
+	}
+
+	buf = ds1307->regs;
+	if (i2c_check_functionality(adapter, I2C_FUNC_SMBUS_I2C_BLOCK)) {
+		ds1307->read_block_data = ds1307_native_smbus_read_block_data;
+		ds1307->write_block_data = ds1307_native_smbus_write_block_data;
+	} else {
+		ds1307->read_block_data = ds1307_read_block_data;
+		ds1307->write_block_data = ds1307_write_block_data;
+	}
+
+	err = ds1307_chip_configure(ds1307);
+	if (err < 0)
+		return err;
+
+	switch (ds1307->type) {
 	case ds_1388:
 		ds1307->offset = 1; /* Seconds starts at 1 */
 		break;
-- 
2.5.5

[PATCH 07/13] RTC: ds1307: Move chip sanity checking into a separate routine

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/rtc/rtc-ds1307.c | 148 +++++++++++++++++++++++++----------------------
 1 file changed, 79 insertions(+), 69 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index df8c78a..5e7eb13 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -1422,79 +1422,20 @@ static int ds1307_chip_configure(struct ds1307 *ds1307)
 	return 0;
 }
 
-static int ds1307_probe(struct i2c_client *client,
-			const struct i2c_device_id *id)
+static int ds1307_chip_sanity_check(struct ds1307 *ds1307)
 {
-	struct ds1307		*ds1307;
-	int			err = -ENODEV;
-	int			tmp;
-	struct chip_desc	*chip = &chips[id->driver_data];
-	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
-	unsigned char		*buf;
-	struct ds1307_platform_data *pdata = dev_get_platdata(&client->dev);
-	irq_handler_t	irq_handler = ds1307_irq;
-
-	const struct rtc_class_ops *rtc_ops = &ds13xx_rtc_ops;
-
-	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)
-	    && !i2c_check_functionality(adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
-		return -EIO;
-
-	ds1307 = devm_kzalloc(&client->dev, sizeof(struct ds1307), GFP_KERNEL);
-	if (!ds1307)
-		return -ENOMEM;
-
-	i2c_set_clientdata(client, ds1307);
-
-	ds1307->client	= client;
-	ds1307->type	= id->driver_data;
-
-	if (!pdata && client->dev.of_node)
-		ds1307_trickle_of_init(client, chip);
-	else if (pdata && pdata->trickle_charger_setup)
-		chip->trickle_charger_setup = pdata->trickle_charger_setup;
-
-	if (chip->trickle_charger_setup && chip->trickle_charger_reg) {
-		dev_dbg(&client->dev, "writing trickle charger info 0x%x to 0x%x\n",
-		    DS13XX_TRICKLE_CHARGER_MAGIC | chip->trickle_charger_setup,
-		    chip->trickle_charger_reg);
-		i2c_smbus_write_byte_data(client, chip->trickle_charger_reg,
-		    DS13XX_TRICKLE_CHARGER_MAGIC |
-		    chip->trickle_charger_setup);
-	}
+	int tmp;
+	unsigned char *buf;
+	struct i2c_client *client = ds1307->client;
 
 	buf = ds1307->regs;
-	if (i2c_check_functionality(adapter, I2C_FUNC_SMBUS_I2C_BLOCK)) {
-		ds1307->read_block_data = ds1307_native_smbus_read_block_data;
-		ds1307->write_block_data = ds1307_native_smbus_write_block_data;
-	} else {
-		ds1307->read_block_data = ds1307_read_block_data;
-		ds1307->write_block_data = ds1307_write_block_data;
-	}
-
-	err = ds1307_chip_configure(ds1307);
-	if (err < 0)
-		return err;
-
-	switch (ds1307->type) {
-	case ds_1388:
-		ds1307->offset = 1; /* Seconds starts at 1 */
-		break;
-	case mcp794xx:
-		rtc_ops = &mcp794xx_rtc_ops;
-		irq_handler = mcp794xx_irq;
-		break;
-	default:
-		break;
-	}
 
 read_rtc:
 	/* read RTC registers */
 	tmp = ds1307->read_block_data(ds1307->client, ds1307->offset, 8, buf);
 	if (tmp != 8) {
 		dev_dbg(&client->dev, "read error %d\n", tmp);
-		err = -EIO;
-		goto exit;
+		return -EIO;
 	}
 
 	/*
@@ -1535,8 +1476,7 @@ read_rtc:
 		tmp = i2c_smbus_read_byte_data(client, DS1340_REG_FLAG);
 		if (tmp < 0) {
 			dev_dbg(&client->dev, "read error %d\n", tmp);
-			err = -EIO;
-			goto exit;
+			return -EIO;
 		}
 
 		/* oscillator fault?  clear flag, and warn */
@@ -1566,6 +1506,79 @@ read_rtc:
 		break;
 	}
 
+	return 0;
+}
+
+static int ds1307_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct ds1307		*ds1307;
+	int			err = -ENODEV;
+	int			tmp;
+	struct chip_desc	*chip = &chips[id->driver_data];
+	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
+	unsigned char		*buf;
+	struct ds1307_platform_data *pdata = dev_get_platdata(&client->dev);
+	irq_handler_t	irq_handler = ds1307_irq;
+
+	const struct rtc_class_ops *rtc_ops = &ds13xx_rtc_ops;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)
+	    && !i2c_check_functionality(adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
+		return -EIO;
+
+	ds1307 = devm_kzalloc(&client->dev, sizeof(struct ds1307), GFP_KERNEL);
+	if (!ds1307)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, ds1307);
+
+	ds1307->client	= client;
+	ds1307->type	= id->driver_data;
+
+	if (!pdata && client->dev.of_node)
+		ds1307_trickle_of_init(client, chip);
+	else if (pdata && pdata->trickle_charger_setup)
+		chip->trickle_charger_setup = pdata->trickle_charger_setup;
+
+	if (chip->trickle_charger_setup && chip->trickle_charger_reg) {
+		dev_dbg(&client->dev, "writing trickle charger info 0x%x to 0x%x\n",
+		    DS13XX_TRICKLE_CHARGER_MAGIC | chip->trickle_charger_setup,
+		    chip->trickle_charger_reg);
+		i2c_smbus_write_byte_data(client, chip->trickle_charger_reg,
+		    DS13XX_TRICKLE_CHARGER_MAGIC |
+		    chip->trickle_charger_setup);
+	}
+
+	buf = ds1307->regs;
+	if (i2c_check_functionality(adapter, I2C_FUNC_SMBUS_I2C_BLOCK)) {
+		ds1307->read_block_data = ds1307_native_smbus_read_block_data;
+		ds1307->write_block_data = ds1307_native_smbus_write_block_data;
+	} else {
+		ds1307->read_block_data = ds1307_read_block_data;
+		ds1307->write_block_data = ds1307_write_block_data;
+	}
+
+	err = ds1307_chip_configure(ds1307);
+	if (err < 0)
+		return err;
+
+	switch (ds1307->type) {
+	case ds_1388:
+		ds1307->offset = 1; /* Seconds starts at 1 */
+		break;
+	case mcp794xx:
+		rtc_ops = &mcp794xx_rtc_ops;
+		irq_handler = mcp794xx_irq;
+		break;
+	default:
+		break;
+	}
+
+	err = ds1307_chip_sanity_check(ds1307);
+	if (err < 0)
+		return err;
+
 	tmp = ds1307->regs[DS1307_REG_HOUR];
 	switch (ds1307->type) {
 	case ds_1340:
@@ -1663,9 +1676,6 @@ read_rtc:
 	ds1307_clks_register(ds1307);
 
 	return 0;
-
-exit:
-	return err;
 }
 
 static int ds1307_remove(struct i2c_client *client)
-- 
2.5.5

[PATCH 08/13] RTC: ds1307: Remove register "cache"

[Andrey Smirnov]

Remove shared area used by many subroutines to store values of RTC's
registers. There wasn't very much caching or sharing going on in the
code and that register cache, being a semi-global variable, only created
additional implicit dependencies between function and made code more
confusing (there were a number of functions that defined a convenience
variable pointing to ds1307->regs, but failed to use it in the code
consistently).

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/rtc/rtc-ds1307.c | 229 ++++++++++++++++++++++++-----------------------
 1 file changed, 116 insertions(+), 113 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 5e7eb13..3c137ab 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -110,10 +110,10 @@ enum ds_type {
 #	define RX8025_BIT_VDET		0x40
 #	define RX8025_BIT_XST		0x20
 
+#define DS1307_REG_COUNT	11
 
 struct ds1307 {
 	u8			offset; /* register's offset */
-	u8			regs[11];
 	u16			nvram_offset;
 	struct bin_attribute	*nvram;
 	enum ds_type		type;
@@ -366,30 +366,31 @@ out:
 
 static int ds1307_get_time(struct device *dev, struct rtc_time *t)
 {
+	u8              regs[DS1307_REG_COUNT];
 	struct ds1307	*ds1307 = dev_get_drvdata(dev);
 	int		tmp;
 
 	/* read the RTC date and time registers all at once */
 	tmp = ds1307->read_block_data(ds1307->client,
-		ds1307->offset, 7, ds1307->regs);
+		ds1307->offset, 7, regs);
 	if (tmp != 7) {
 		dev_err(dev, "%s error %d\n", "read", tmp);
 		return -EIO;
 	}
 
-	dev_dbg(dev, "%s: %7ph\n", "read", ds1307->regs);
+	dev_dbg(dev, "%s: %7ph\n", "read", regs);
 
-	t->tm_sec = bcd2bin(ds1307->regs[DS1307_REG_SECS] & 0x7f);
-	t->tm_min = bcd2bin(ds1307->regs[DS1307_REG_MIN] & 0x7f);
-	tmp = ds1307->regs[DS1307_REG_HOUR] & 0x3f;
+	t->tm_sec = bcd2bin(regs[DS1307_REG_SECS] & 0x7f);
+	t->tm_min = bcd2bin(regs[DS1307_REG_MIN] & 0x7f);
+	tmp = regs[DS1307_REG_HOUR] & 0x3f;
 	t->tm_hour = bcd2bin(tmp);
-	t->tm_wday = bcd2bin(ds1307->regs[DS1307_REG_WDAY] & 0x07) - 1;
-	t->tm_mday = bcd2bin(ds1307->regs[DS1307_REG_MDAY] & 0x3f);
-	tmp = ds1307->regs[DS1307_REG_MONTH] & 0x1f;
+	t->tm_wday = bcd2bin(regs[DS1307_REG_WDAY] & 0x07) - 1;
+	t->tm_mday = bcd2bin(regs[DS1307_REG_MDAY] & 0x3f);
+	tmp = regs[DS1307_REG_MONTH] & 0x1f;
 	t->tm_mon = bcd2bin(tmp) - 1;
 
 	/* assume 20YY not 19YY, and ignore DS1337_BIT_CENTURY */
-	t->tm_year = bcd2bin(ds1307->regs[DS1307_REG_YEAR]) + 100;
+	t->tm_year = bcd2bin(regs[DS1307_REG_YEAR]) + 100;
 
 	dev_dbg(dev, "%s secs=%d, mins=%d, "
 		"hours=%d, mday=%d, mon=%d, year=%d, wday=%d\n",
@@ -406,7 +407,7 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
 	struct ds1307	*ds1307 = dev_get_drvdata(dev);
 	int		result;
 	int		tmp;
-	u8		*buf = ds1307->regs;
+	u8		regs[DS1307_REG_COUNT];
 
 	dev_dbg(dev, "%s secs=%d, mins=%d, "
 		"hours=%d, mday=%d, mon=%d, year=%d, wday=%d\n",
@@ -414,26 +415,26 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
 		t->tm_hour, t->tm_mday,
 		t->tm_mon, t->tm_year, t->tm_wday);
 
-	buf[DS1307_REG_SECS] = bin2bcd(t->tm_sec);
-	buf[DS1307_REG_MIN] = bin2bcd(t->tm_min);
-	buf[DS1307_REG_HOUR] = bin2bcd(t->tm_hour);
-	buf[DS1307_REG_WDAY] = bin2bcd(t->tm_wday + 1);
-	buf[DS1307_REG_MDAY] = bin2bcd(t->tm_mday);
-	buf[DS1307_REG_MONTH] = bin2bcd(t->tm_mon + 1);
+	regs[DS1307_REG_SECS]  = bin2bcd(t->tm_sec);
+	regs[DS1307_REG_MIN]   = bin2bcd(t->tm_min);
+	regs[DS1307_REG_HOUR]  = bin2bcd(t->tm_hour);
+	regs[DS1307_REG_WDAY]  = bin2bcd(t->tm_wday + 1);
+	regs[DS1307_REG_MDAY]  = bin2bcd(t->tm_mday);
+	regs[DS1307_REG_MONTH] = bin2bcd(t->tm_mon + 1);
 
 	/* assume 20YY not 19YY */
 	tmp = t->tm_year - 100;
-	buf[DS1307_REG_YEAR] = bin2bcd(tmp);
+	regs[DS1307_REG_YEAR] = bin2bcd(tmp);
 
 	switch (ds1307->type) {
 	case ds_1337:
 	case ds_1339:
 	case ds_3231:
 	case ds_1341:
-		buf[DS1307_REG_MONTH] |= DS1337_BIT_CENTURY;
+		regs[DS1307_REG_MONTH] |= DS1337_BIT_CENTURY;
 		break;
 	case ds_1340:
-		buf[DS1307_REG_HOUR] |= DS1340_BIT_CENTURY_EN
+		regs[DS1307_REG_HOUR] |= DS1340_BIT_CENTURY_EN
 				| DS1340_BIT_CENTURY;
 		break;
 	case mcp794xx:
@@ -442,17 +443,17 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
 		 * values and need to be set again before writing the
 		 * buffer out to the device.
 		 */
-		buf[DS1307_REG_SECS] |= MCP794XX_BIT_ST;
-		buf[DS1307_REG_WDAY] |= MCP794XX_BIT_VBATEN;
+		regs[DS1307_REG_SECS] |= MCP794XX_BIT_ST;
+		regs[DS1307_REG_WDAY] |= MCP794XX_BIT_VBATEN;
 		break;
 	default:
 		break;
 	}
 
-	dev_dbg(dev, "%s: %7ph\n", "write", buf);
+	dev_dbg(dev, "%s: %7ph\n", "write", regs);
 
 	result = ds1307->write_block_data(ds1307->client,
-		ds1307->offset, 7, buf);
+		ds1307->offset, 7, regs);
 	if (result < 0) {
 		dev_err(dev, "%s error %d\n", "write", result);
 		return result;
@@ -465,29 +466,30 @@ static int ds1337_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 	struct i2c_client       *client = to_i2c_client(dev);
 	struct ds1307		*ds1307 = i2c_get_clientdata(client);
 	int			ret;
+	u8			regs[DS1307_REG_COUNT];
 
 	if (!test_bit(HAS_ALARM, &ds1307->flags))
 		return -EINVAL;
 
 	/* read all ALARM1, ALARM2, and status registers at once */
 	ret = ds1307->read_block_data(client,
-			DS1339_REG_ALARM1_SECS, 9, ds1307->regs);
+			DS1339_REG_ALARM1_SECS, 9, regs);
 	if (ret != 9) {
 		dev_err(dev, "%s error %d\n", "alarm read", ret);
 		return -EIO;
 	}
 
 	dev_dbg(dev, "%s: %4ph, %3ph, %2ph\n", "alarm read",
-		&ds1307->regs[0], &ds1307->regs[4], &ds1307->regs[7]);
+		&regs[0], &regs[4], &regs[7]);
 
 	/*
 	 * report alarm time (ALARM1); assume 24 hour and day-of-month modes,
 	 * and that all four fields are checked matches
 	 */
-	t->time.tm_sec = bcd2bin(ds1307->regs[0] & 0x7f);
-	t->time.tm_min = bcd2bin(ds1307->regs[1] & 0x7f);
-	t->time.tm_hour = bcd2bin(ds1307->regs[2] & 0x3f);
-	t->time.tm_mday = bcd2bin(ds1307->regs[3] & 0x3f);
+	t->time.tm_sec = bcd2bin(regs[0] & 0x7f);
+	t->time.tm_min = bcd2bin(regs[1] & 0x7f);
+	t->time.tm_hour = bcd2bin(regs[2] & 0x3f);
+	t->time.tm_mday = bcd2bin(regs[3] & 0x3f);
 	t->time.tm_mon = -1;
 	t->time.tm_year = -1;
 	t->time.tm_wday = -1;
@@ -495,8 +497,8 @@ static int ds1337_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 	t->time.tm_isdst = -1;
 
 	/* ... and status */
-	t->enabled = !!(ds1307->regs[7] & DS1337_BIT_A1IE);
-	t->pending = !!(ds1307->regs[8] & DS1337_BIT_A1I);
+	t->enabled = !!(regs[7] & DS1337_BIT_A1IE);
+	t->pending = !!(regs[8] & DS1337_BIT_A1I);
 
 	dev_dbg(dev, "%s secs=%d, mins=%d, "
 		"hours=%d, mday=%d, enabled=%d, pending=%d\n",
@@ -511,7 +513,7 @@ static int ds1337_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
 	struct i2c_client	*client = to_i2c_client(dev);
 	struct ds1307		*ds1307 = i2c_get_clientdata(client);
-	unsigned char		*buf = ds1307->regs;
+	u8			regs[DS1307_REG_COUNT];
 	u8			control, status;
 	int			ret;
 
@@ -526,34 +528,34 @@ static int ds1337_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 
 	/* read current status of both alarms and the chip */
 	ret = ds1307->read_block_data(client,
-			DS1339_REG_ALARM1_SECS, 9, buf);
+			DS1339_REG_ALARM1_SECS, 9, regs);
 	if (ret != 9) {
 		dev_err(dev, "%s error %d\n", "alarm write", ret);
 		return -EIO;
 	}
-	control = ds1307->regs[7];
-	status = ds1307->regs[8];
+	control = regs[7];
+	status  = regs[8];
 
 	dev_dbg(dev, "%s: %4ph, %3ph, %02x %02x\n", "alarm set (old status)",
-		&ds1307->regs[0], &ds1307->regs[4], control, status);
+		&regs[0], &regs[4], control, status);
 
 	/* set ALARM1, using 24 hour and day-of-month modes */
-	buf[0] = bin2bcd(t->time.tm_sec);
-	buf[1] = bin2bcd(t->time.tm_min);
-	buf[2] = bin2bcd(t->time.tm_hour);
-	buf[3] = bin2bcd(t->time.tm_mday);
+	regs[0] = bin2bcd(t->time.tm_sec);
+	regs[1] = bin2bcd(t->time.tm_min);
+	regs[2] = bin2bcd(t->time.tm_hour);
+	regs[3] = bin2bcd(t->time.tm_mday);
 
 	/* set ALARM2 to non-garbage */
-	buf[4] = 0;
-	buf[5] = 0;
-	buf[6] = 0;
+	regs[4] = 0;
+	regs[5] = 0;
+	regs[6] = 0;
 
 	/* disable alarms */
-	buf[7] = control & ~(DS1337_BIT_A1IE | DS1337_BIT_A2IE);
-	buf[8] = status & ~(DS1337_BIT_A1I | DS1337_BIT_A2I);
+	regs[7] = control & ~(DS1337_BIT_A1IE | DS1337_BIT_A2IE);
+	regs[8] = status & ~(DS1337_BIT_A1I | DS1337_BIT_A2I);
 
 	ret = ds1307->write_block_data(client,
-			DS1339_REG_ALARM1_SECS, 9, buf);
+			DS1339_REG_ALARM1_SECS, 9, regs);
 	if (ret < 0) {
 		dev_err(dev, "can't set alarm time\n");
 		return ret;
@@ -562,8 +564,8 @@ static int ds1337_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 	/* optionally enable ALARM1 */
 	if (t->enabled) {
 		dev_dbg(dev, "alarm IRQ armed\n");
-		buf[7] |= DS1337_BIT_A1IE;	/* only ALARM1 is used */
-		i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL, buf[7]);
+		regs[7] |= DS1337_BIT_A1IE;	/* only ALARM1 is used */
+		i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL, regs[7]);
 	}
 
 	return 0;
@@ -665,7 +667,7 @@ static int mcp794xx_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct ds1307 *ds1307 = i2c_get_clientdata(client);
-	u8 *regs = ds1307->regs;
+	u8  regs[DS1307_REG_COUNT];
 	int ret;
 
 	if (!test_bit(HAS_ALARM, &ds1307->flags))
@@ -679,12 +681,12 @@ static int mcp794xx_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 	t->enabled = !!(regs[0] & MCP794XX_BIT_ALM0_EN);
 
 	/* Report alarm 0 time assuming 24-hour and day-of-month modes. */
-	t->time.tm_sec = bcd2bin(ds1307->regs[3] & 0x7f);
-	t->time.tm_min = bcd2bin(ds1307->regs[4] & 0x7f);
-	t->time.tm_hour = bcd2bin(ds1307->regs[5] & 0x3f);
-	t->time.tm_wday = bcd2bin(ds1307->regs[6] & 0x7) - 1;
-	t->time.tm_mday = bcd2bin(ds1307->regs[7] & 0x3f);
-	t->time.tm_mon = bcd2bin(ds1307->regs[8] & 0x1f) - 1;
+	t->time.tm_sec = bcd2bin(regs[3] & 0x7f);
+	t->time.tm_min = bcd2bin(regs[4] & 0x7f);
+	t->time.tm_hour = bcd2bin(regs[5] & 0x3f);
+	t->time.tm_wday = bcd2bin(regs[6] & 0x7) - 1;
+	t->time.tm_mday = bcd2bin(regs[7] & 0x3f);
+	t->time.tm_mon = bcd2bin(regs[8] & 0x1f) - 1;
 	t->time.tm_year = -1;
 	t->time.tm_yday = -1;
 	t->time.tm_isdst = -1;
@@ -693,9 +695,9 @@ static int mcp794xx_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 		"enabled=%d polarity=%d irq=%d match=%d\n", __func__,
 		t->time.tm_sec, t->time.tm_min, t->time.tm_hour,
 		t->time.tm_wday, t->time.tm_mday, t->time.tm_mon, t->enabled,
-		!!(ds1307->regs[6] & MCP794XX_BIT_ALMX_POL),
-		!!(ds1307->regs[6] & MCP794XX_BIT_ALMX_IF),
-		(ds1307->regs[6] & MCP794XX_MSK_ALMX_MATCH) >> 4);
+		!!(regs[6] & MCP794XX_BIT_ALMX_POL),
+		!!(regs[6] & MCP794XX_BIT_ALMX_IF),
+		(regs[6] & MCP794XX_MSK_ALMX_MATCH) >> 4);
 
 	return 0;
 }
@@ -704,7 +706,7 @@ static int mcp794xx_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct ds1307 *ds1307 = i2c_get_clientdata(client);
-	unsigned char *regs = ds1307->regs;
+	u8  regs[DS1307_REG_COUNT];
 	int ret;
 
 	if (!test_bit(HAS_ALARM, &ds1307->flags))
@@ -1274,11 +1276,9 @@ static bool ds1307_want_irq(const struct ds1307 *ds1307,
 static int ds1307_chip_configure(struct ds1307 *ds1307)
 {
 	int tmp;
-	unsigned char *buf;
+	u8  regs[DS1307_REG_COUNT];
 	struct i2c_client *client = ds1307->client;
 
-	buf = ds1307->regs;
-
 	switch (ds1307->type) {
 	case ds_1337:
 	case ds_1339:
@@ -1293,32 +1293,32 @@ static int ds1307_chip_configure(struct ds1307 *ds1307)
 
 		/* get registers that the "rtc" read below won't read... */
 		tmp = ds1307->read_block_data(client,
-					      DS1337_REG_CONTROL, 2, buf);
+					      DS1337_REG_CONTROL, 2, regs);
 		if (tmp != 2) {
 			dev_dbg(&ds1307->client->dev, "read error %d\n", tmp);
 			return -EIO;
 		}
 
 		/* oscillator off?  turn it on, so clock can tick. */
-		if (ds1307->regs[0] & DS1337_BIT_nEOSC)
-			ds1307->regs[0] &= ~DS1337_BIT_nEOSC;
+		if (regs[0] & DS1337_BIT_nEOSC)
+			regs[0] &= ~DS1337_BIT_nEOSC;
 
 		if (ds1307->type == ds_1341) {
 			/* Make sure we are not generating square wave
 			 * output */
-			ds1307->regs[1] &= ~DS1341_BIT_ECLK;
+			regs[1] &= ~DS1341_BIT_ECLK;
 
 			if (of_property_read_bool(client->dev.of_node,
 						  "disable-oscillator-stop-flag"))
-				ds1307->regs[1] |= DS1341_BIT_DOSF;
+				regs[1] |= DS1341_BIT_DOSF;
 			else
-				ds1307->regs[1] &= ~DS1341_BIT_DOSF;
+				regs[1] &= ~DS1341_BIT_DOSF;
 
 			if (of_property_read_bool(client->dev.of_node,
 						  "enable-glitch-filter"))
-				ds1307->regs[0] |= DS1341_BIT_EGFIL;
+				regs[0] |= DS1341_BIT_EGFIL;
 			else
-				ds1307->regs[0] &= ~DS1341_BIT_EGFIL;
+				regs[0] &= ~DS1341_BIT_EGFIL;
 
 			/*
 			 * Write status register. Control register
@@ -1326,7 +1326,7 @@ static int ds1307_chip_configure(struct ds1307 *ds1307)
 			 */
 			i2c_smbus_write_byte_data(client,
 						  DS1337_REG_STATUS,
-						  ds1307->regs[1]);
+						  regs[1]);
 		}
 
 		/*
@@ -1334,19 +1334,19 @@ static int ds1307_chip_configure(struct ds1307 *ds1307)
 		 * For some variants, be sure alarms can trigger when we're
 		 * running on Vbackup (BBSQI/BBSQW)
 		 */
-		ds1307->regs[0] |= DS1337_BIT_INTCN
+		regs[0] |= DS1337_BIT_INTCN
 			| bbsqi_bitpos[ds1307->type];
-		ds1307->regs[0] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);
+		regs[0] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);
 
 		i2c_smbus_write_byte_data(client,
 					  DS1337_REG_CONTROL,
-					  ds1307->regs[0]);
+					  regs[0]);
 
 		/* oscillator fault?  clear flag, and warn */
-		if (ds1307->regs[1] & DS1337_BIT_OSF) {
+		if (regs[1] & DS1337_BIT_OSF) {
 			i2c_smbus_write_byte_data(client,
 						  DS1337_REG_STATUS,
-						  ds1307->regs[1] & ~DS1337_BIT_OSF);
+						  regs[1] & ~DS1337_BIT_OSF);
 			dev_warn(&ds1307->client->dev, "SET TIME!\n");
 		}
 		break;
@@ -1354,60 +1354,60 @@ static int ds1307_chip_configure(struct ds1307 *ds1307)
 	case rx_8025:
 		tmp = i2c_smbus_read_i2c_block_data(client,
 						    RX8025_REG_CTRL1 << 4 | 0x08,
-						    2, buf);
+						    2, regs);
 		if (tmp != 2) {
 			dev_dbg(&client->dev, "read error %d\n", tmp);
 			return -EIO;
 		}
 
 		/* oscillator off?  turn it on, so clock can tick. */
-		if (!(ds1307->regs[1] & RX8025_BIT_XST)) {
-			ds1307->regs[1] |= RX8025_BIT_XST;
+		if (!(regs[1] & RX8025_BIT_XST)) {
+			regs[1] |= RX8025_BIT_XST;
 			i2c_smbus_write_byte_data(client,
 						  RX8025_REG_CTRL2 << 4 | 0x08,
-						  ds1307->regs[1]);
+						  regs[1]);
 			dev_warn(&client->dev,
 				 "oscillator stop detected - SET TIME!\n");
 		}
 
-		if (ds1307->regs[1] & RX8025_BIT_PON) {
-			ds1307->regs[1] &= ~RX8025_BIT_PON;
+		if (regs[1] & RX8025_BIT_PON) {
+			regs[1] &= ~RX8025_BIT_PON;
 			i2c_smbus_write_byte_data(client,
 						  RX8025_REG_CTRL2 << 4 | 0x08,
-						  ds1307->regs[1]);
+						  regs[1]);
 			dev_warn(&client->dev, "power-on detected\n");
 		}
 
-		if (ds1307->regs[1] & RX8025_BIT_VDET) {
-			ds1307->regs[1] &= ~RX8025_BIT_VDET;
+		if (regs[1] & RX8025_BIT_VDET) {
+			regs[1] &= ~RX8025_BIT_VDET;
 			i2c_smbus_write_byte_data(client,
 						  RX8025_REG_CTRL2 << 4 | 0x08,
-						  ds1307->regs[1]);
+						  regs[1]);
 			dev_warn(&client->dev, "voltage drop detected\n");
 		}
 
 		/* make sure we are running in 24hour mode */
-		if (!(ds1307->regs[0] & RX8025_BIT_2412)) {
+		if (!(regs[0] & RX8025_BIT_2412)) {
 			u8 hour;
 
 			/* switch to 24 hour mode */
 			i2c_smbus_write_byte_data(client,
 						  RX8025_REG_CTRL1 << 4 | 0x08,
-						  ds1307->regs[0] | RX8025_BIT_2412);
+						  regs[0] | RX8025_BIT_2412);
 
 			tmp = i2c_smbus_read_i2c_block_data(client,
 							    RX8025_REG_CTRL1 << 4 | 0x08,
-							    2, buf);
+							    2, regs);
 			if (tmp != 2) {
 				dev_dbg(&client->dev, "read error %d\n", tmp);
 				return -EIO;
 			}
 
 			/* correct hour */
-			hour = bcd2bin(ds1307->regs[DS1307_REG_HOUR]);
+			hour = bcd2bin(regs[DS1307_REG_HOUR]);
 			if (hour == 12)
 				hour = 0;
-			if (ds1307->regs[DS1307_REG_HOUR] & DS1307_BIT_PM)
+			if (regs[DS1307_REG_HOUR] & DS1307_BIT_PM)
 				hour += 12;
 
 			i2c_smbus_write_byte_data(client,
@@ -1425,14 +1425,12 @@ static int ds1307_chip_configure(struct ds1307 *ds1307)
 static int ds1307_chip_sanity_check(struct ds1307 *ds1307)
 {
 	int tmp;
-	unsigned char *buf;
+	u8  regs[DS1307_REG_COUNT];
 	struct i2c_client *client = ds1307->client;
 
-	buf = ds1307->regs;
-
 read_rtc:
 	/* read RTC registers */
-	tmp = ds1307->read_block_data(ds1307->client, ds1307->offset, 8, buf);
+	tmp = ds1307->read_block_data(ds1307->client, ds1307->offset, 8, regs);
 	if (tmp != 8) {
 		dev_dbg(&client->dev, "read error %d\n", tmp);
 		return -EIO;
@@ -1443,7 +1441,7 @@ read_rtc:
 	 * specify the extra bits as must-be-zero, but there are
 	 * still a few values that are clearly out-of-range.
 	 */
-	tmp = ds1307->regs[DS1307_REG_SECS];
+	tmp = regs[DS1307_REG_SECS];
 	switch (ds1307->type) {
 	case ds_1307:
 	case m41t00:
@@ -1460,9 +1458,9 @@ read_rtc:
 			i2c_smbus_write_byte_data(client, DS1307_REG_SECS, 0);
 
 		/* oscillator fault?  clear flag, and warn */
-		if (ds1307->regs[DS1307_REG_CONTROL] & DS1338_BIT_OSF) {
+		if (regs[DS1307_REG_CONTROL] & DS1338_BIT_OSF) {
 			i2c_smbus_write_byte_data(client, DS1307_REG_CONTROL,
-					ds1307->regs[DS1307_REG_CONTROL]
+					regs[DS1307_REG_CONTROL]
 					& ~DS1338_BIT_OSF);
 			dev_warn(&client->dev, "SET TIME!\n");
 			goto read_rtc;
@@ -1487,9 +1485,9 @@ read_rtc:
 		break;
 	case mcp794xx:
 		/* make sure that the backup battery is enabled */
-		if (!(ds1307->regs[DS1307_REG_WDAY] & MCP794XX_BIT_VBATEN)) {
+		if (!(regs[DS1307_REG_WDAY] & MCP794XX_BIT_VBATEN)) {
 			i2c_smbus_write_byte_data(client, DS1307_REG_WDAY,
-					ds1307->regs[DS1307_REG_WDAY]
+					regs[DS1307_REG_WDAY]
 					| MCP794XX_BIT_VBATEN);
 		}
 
@@ -1514,10 +1512,10 @@ static int ds1307_probe(struct i2c_client *client,
 {
 	struct ds1307		*ds1307;
 	int			err = -ENODEV;
-	int			tmp;
+	int			reg;
+	u8			hour;
 	struct chip_desc	*chip = &chips[id->driver_data];
 	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
-	unsigned char		*buf;
 	struct ds1307_platform_data *pdata = dev_get_platdata(&client->dev);
 	irq_handler_t	irq_handler = ds1307_irq;
 
@@ -1550,7 +1548,6 @@ static int ds1307_probe(struct i2c_client *client,
 		    chip->trickle_charger_setup);
 	}
 
-	buf = ds1307->regs;
 	if (i2c_check_functionality(adapter, I2C_FUNC_SMBUS_I2C_BLOCK)) {
 		ds1307->read_block_data = ds1307_native_smbus_read_block_data;
 		ds1307->write_block_data = ds1307_native_smbus_write_block_data;
@@ -1579,7 +1576,14 @@ static int ds1307_probe(struct i2c_client *client,
 	if (err < 0)
 		return err;
 
-	tmp = ds1307->regs[DS1307_REG_HOUR];
+	reg = i2c_smbus_read_byte_data(client,
+				       ds1307->offset + DS1307_REG_HOUR);
+	if (reg < 0) {
+		dev_err(&client->dev,
+			"failed to read HOUR register\n");
+		return reg;
+	}
+
 	switch (ds1307->type) {
 	case ds_1340:
 	case m41t00:
@@ -1591,21 +1595,21 @@ static int ds1307_probe(struct i2c_client *client,
 	case rx_8025:
 		break;
 	default:
-		if (!(tmp & DS1307_BIT_12HR))
+		if (!(reg & DS1307_BIT_12HR))
 			break;
 
 		/*
 		 * Be sure we're in 24 hour mode.  Multi-master systems
 		 * take note...
 		 */
-		tmp = bcd2bin(tmp & 0x1f);
-		if (tmp == 12)
-			tmp = 0;
-		if (ds1307->regs[DS1307_REG_HOUR] & DS1307_BIT_PM)
-			tmp += 12;
+		hour = bcd2bin(reg & 0x1f);
+		if (hour == 12)
+			hour = 0;
+		if (reg & DS1307_BIT_PM)
+			hour += 12;
 		i2c_smbus_write_byte_data(client,
 				ds1307->offset + DS1307_REG_HOUR,
-				bin2bcd(tmp));
+				bin2bcd(hour));
 	}
 
 	if (ds1307_want_irq(ds1307, chip)) {
@@ -1620,7 +1624,6 @@ static int ds1307_probe(struct i2c_client *client,
 
 	if (ds1307_can_wakeup_device(ds1307) &&
 	    ds1307->client->irq <= 0) {
-		/* Disable request for an IRQ */
 		dev_info(&client->dev,
 			 "'wakeup-source' is set, request for an IRQ is disabled!\n");
 		/* We cannot support UIE mode if we do not have an IRQ line */
-- 
2.5.5

[PATCH 09/13] RTC: ds1307: Constify struct ds1307 where possible

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/rtc/rtc-ds1307.c | 50 ++++++++++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 3c137ab..dbf8361 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -333,7 +333,7 @@ static s32 ds1307_native_smbus_read_block_data(const struct i2c_client *client,
 static irqreturn_t ds1307_irq(int irq, void *dev_id)
 {
 	struct i2c_client	*client = dev_id;
-	struct ds1307		*ds1307 = i2c_get_clientdata(client);
+	const struct ds1307	*ds1307 = i2c_get_clientdata(client);
 	struct mutex		*lock = &ds1307->rtc->ops_lock;
 	int			stat, control;
 
@@ -367,8 +367,8 @@ out:
 static int ds1307_get_time(struct device *dev, struct rtc_time *t)
 {
 	u8              regs[DS1307_REG_COUNT];
-	struct ds1307	*ds1307 = dev_get_drvdata(dev);
 	int		tmp;
+	const struct ds1307 *ds1307 = dev_get_drvdata(dev);
 
 	/* read the RTC date and time registers all at once */
 	tmp = ds1307->read_block_data(ds1307->client,
@@ -404,7 +404,7 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
 
 static int ds1307_set_time(struct device *dev, struct rtc_time *t)
 {
-	struct ds1307	*ds1307 = dev_get_drvdata(dev);
+	const struct ds1307 *ds1307 = dev_get_drvdata(dev);
 	int		result;
 	int		tmp;
 	u8		regs[DS1307_REG_COUNT];
@@ -464,7 +464,7 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
 static int ds1337_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
 	struct i2c_client       *client = to_i2c_client(dev);
-	struct ds1307		*ds1307 = i2c_get_clientdata(client);
+	const struct ds1307	*ds1307 = i2c_get_clientdata(client);
 	int			ret;
 	u8			regs[DS1307_REG_COUNT];
 
@@ -512,7 +512,7 @@ static int ds1337_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 static int ds1337_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
 	struct i2c_client	*client = to_i2c_client(dev);
-	struct ds1307		*ds1307 = i2c_get_clientdata(client);
+	const struct ds1307	*ds1307 = i2c_get_clientdata(client);
 	u8			regs[DS1307_REG_COUNT];
 	u8			control, status;
 	int			ret;
@@ -574,7 +574,7 @@ static int ds1337_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 static int ds1307_alarm_irq_enable(struct device *dev, unsigned int enabled)
 {
 	struct i2c_client	*client = to_i2c_client(dev);
-	struct ds1307		*ds1307 = i2c_get_clientdata(client);
+	const struct ds1307	*ds1307 = i2c_get_clientdata(client);
 	int			ret;
 
 	if (!test_bit(HAS_ALARM, &ds1307->flags))
@@ -629,7 +629,7 @@ static const struct rtc_class_ops ds13xx_rtc_ops = {
 static irqreturn_t mcp794xx_irq(int irq, void *dev_id)
 {
 	struct i2c_client       *client = dev_id;
-	struct ds1307           *ds1307 = i2c_get_clientdata(client);
+	const struct ds1307     *ds1307 = i2c_get_clientdata(client);
 	struct mutex            *lock = &ds1307->rtc->ops_lock;
 	int reg, ret;
 
@@ -666,7 +666,7 @@ out:
 static int mcp794xx_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	struct ds1307 *ds1307 = i2c_get_clientdata(client);
+	const struct ds1307 *ds1307 = i2c_get_clientdata(client);
 	u8  regs[DS1307_REG_COUNT];
 	int ret;
 
@@ -705,7 +705,7 @@ static int mcp794xx_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 static int mcp794xx_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	struct ds1307 *ds1307 = i2c_get_clientdata(client);
+	const struct ds1307 *ds1307 = i2c_get_clientdata(client);
 	u8  regs[DS1307_REG_COUNT];
 	int ret;
 
@@ -751,7 +751,7 @@ static int mcp794xx_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 static int mcp794xx_alarm_irq_enable(struct device *dev, unsigned int enabled)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	struct ds1307 *ds1307 = i2c_get_clientdata(client);
+	const struct ds1307 *ds1307 = i2c_get_clientdata(client);
 	int reg;
 
 	if (!test_bit(HAS_ALARM, &ds1307->flags))
@@ -785,7 +785,7 @@ ds1307_nvram_read(struct file *filp, struct kobject *kobj,
 		char *buf, loff_t off, size_t count)
 {
 	struct i2c_client	*client;
-	struct ds1307		*ds1307;
+	const struct ds1307	*ds1307;
 	int			result;
 
 	client = kobj_to_i2c_client(kobj);
@@ -804,7 +804,7 @@ ds1307_nvram_write(struct file *filp, struct kobject *kobj,
 		char *buf, loff_t off, size_t count)
 {
 	struct i2c_client	*client;
-	struct ds1307		*ds1307;
+	const struct ds1307	*ds1307;
 	int			result;
 
 	client = kobj_to_i2c_client(kobj);
@@ -880,7 +880,7 @@ out:
  */
 static int ds3231_hwmon_read_temp(struct device *dev, s32 *mC)
 {
-	struct ds1307 *ds1307 = dev_get_drvdata(dev);
+	const struct ds1307 *ds1307 = dev_get_drvdata(dev);
 	u8 temp_buf[2];
 	s16 temp;
 	int ret;
@@ -973,7 +973,7 @@ static int ds3231_clk_sqw_rates[] = {
 	8192,
 };
 
-static int ds1337_write_control(struct ds1307 *ds1307, u8 mask, u8 value)
+static int ds1337_write_control(const struct ds1307 *ds1307, u8 mask, u8 value)
 {
 	struct i2c_client *client = ds1307->client;
 	struct mutex *lock = &ds1307->rtc->ops_lock;
@@ -1001,7 +1001,7 @@ out:
 static unsigned long ds3231_clk_sqw_recalc_rate(struct clk_hw *hw,
 						unsigned long parent_rate)
 {
-	struct ds1307 *ds1307 = clk_sqw_to_ds1307(hw);
+	const struct ds1307 *ds1307 = clk_sqw_to_ds1307(hw);
 	int control;
 	int rate_sel = 0;
 
@@ -1032,7 +1032,7 @@ static long ds3231_clk_sqw_round_rate(struct clk_hw *hw, unsigned long rate,
 static int ds3231_clk_sqw_set_rate(struct clk_hw *hw, unsigned long rate,
 					unsigned long parent_rate)
 {
-	struct ds1307 *ds1307 = clk_sqw_to_ds1307(hw);
+	const struct ds1307 *ds1307 = clk_sqw_to_ds1307(hw);
 	int control = 0;
 	int rate_sel;
 
@@ -1056,21 +1056,21 @@ static int ds3231_clk_sqw_set_rate(struct clk_hw *hw, unsigned long rate,
 
 static int ds3231_clk_sqw_prepare(struct clk_hw *hw)
 {
-	struct ds1307 *ds1307 = clk_sqw_to_ds1307(hw);
+	const struct ds1307 *ds1307 = clk_sqw_to_ds1307(hw);
 
 	return ds1337_write_control(ds1307, DS1337_BIT_INTCN, 0);
 }
 
 static void ds3231_clk_sqw_unprepare(struct clk_hw *hw)
 {
-	struct ds1307 *ds1307 = clk_sqw_to_ds1307(hw);
+	const struct ds1307 *ds1307 = clk_sqw_to_ds1307(hw);
 
 	ds1337_write_control(ds1307, DS1337_BIT_INTCN, DS1337_BIT_INTCN);
 }
 
 static int ds3231_clk_sqw_is_prepared(struct clk_hw *hw)
 {
-	struct ds1307 *ds1307 = clk_sqw_to_ds1307(hw);
+	const struct ds1307 *ds1307 = clk_sqw_to_ds1307(hw);
 	int control;
 
 	control = i2c_smbus_read_byte_data(ds1307->client, DS1337_REG_CONTROL);
@@ -1095,7 +1095,7 @@ static unsigned long ds3231_clk_32khz_recalc_rate(struct clk_hw *hw,
 	return 32768;
 }
 
-static int ds3231_clk_32khz_control(struct ds1307 *ds1307, bool enable)
+static int ds3231_clk_32khz_control(const struct ds1307 *ds1307, bool enable)
 {
 	struct i2c_client *client = ds1307->client;
 	struct mutex *lock = &ds1307->rtc->ops_lock;
@@ -1124,21 +1124,21 @@ out:
 
 static int ds3231_clk_32khz_prepare(struct clk_hw *hw)
 {
-	struct ds1307 *ds1307 = clk_32khz_to_ds1307(hw);
+	const struct ds1307 *ds1307 = clk_32khz_to_ds1307(hw);
 
 	return ds3231_clk_32khz_control(ds1307, true);
 }
 
 static void ds3231_clk_32khz_unprepare(struct clk_hw *hw)
 {
-	struct ds1307 *ds1307 = clk_32khz_to_ds1307(hw);
+	const struct ds1307 *ds1307 = clk_32khz_to_ds1307(hw);
 
 	ds3231_clk_32khz_control(ds1307, false);
 }
 
 static int ds3231_clk_32khz_is_prepared(struct clk_hw *hw)
 {
-	struct ds1307 *ds1307 = clk_32khz_to_ds1307(hw);
+	const struct ds1307 *ds1307 = clk_32khz_to_ds1307(hw);
 	int status;
 
 	status = i2c_smbus_read_byte_data(ds1307->client, DS1337_REG_STATUS);
@@ -1273,7 +1273,7 @@ static bool ds1307_want_irq(const struct ds1307 *ds1307,
 	return false;
 }
 
-static int ds1307_chip_configure(struct ds1307 *ds1307)
+static int ds1307_chip_configure(const struct ds1307 *ds1307)
 {
 	int tmp;
 	u8  regs[DS1307_REG_COUNT];
@@ -1422,7 +1422,7 @@ static int ds1307_chip_configure(struct ds1307 *ds1307)
 	return 0;
 }
 
-static int ds1307_chip_sanity_check(struct ds1307 *ds1307)
+static int ds1307_chip_sanity_check(const struct ds1307 *ds1307)
 {
 	int tmp;
 	u8  regs[DS1307_REG_COUNT];
-- 
2.5.5

[PATCH 10/13] RTC: ds1307: Convert goto to a loop

[Andrey Smirnov]

Convert goto to a loop and set a hard upper limit on the number of times
driver would try to make RTC work (as opposed to having an infinite
retry loop).

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/rtc/rtc-ds1307.c | 151 +++++++++++++++++++++++++----------------------
 1 file changed, 82 insertions(+), 69 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index dbf8361..8ccfe5b 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -1424,87 +1424,100 @@ static int ds1307_chip_configure(const struct ds1307 *ds1307)
 
 static int ds1307_chip_sanity_check(const struct ds1307 *ds1307)
 {
-	int tmp;
+	int tmp, retries;
 	u8  regs[DS1307_REG_COUNT];
 	struct i2c_client *client = ds1307->client;
 
-read_rtc:
-	/* read RTC registers */
-	tmp = ds1307->read_block_data(ds1307->client, ds1307->offset, 8, regs);
-	if (tmp != 8) {
-		dev_dbg(&client->dev, "read error %d\n", tmp);
-		return -EIO;
-	}
-
-	/*
-	 * minimal sanity checking; some chips (like DS1340) don't
-	 * specify the extra bits as must-be-zero, but there are
-	 * still a few values that are clearly out-of-range.
-	 */
-	tmp = regs[DS1307_REG_SECS];
-	switch (ds1307->type) {
-	case ds_1307:
-	case m41t00:
-		/* clock halted?  turn it on, so clock can tick. */
-		if (tmp & DS1307_BIT_CH) {
-			i2c_smbus_write_byte_data(client, DS1307_REG_SECS, 0);
-			dev_warn(&client->dev, "SET TIME!\n");
-			goto read_rtc;
-		}
-		break;
-	case ds_1338:
-		/* clock halted?  turn it on, so clock can tick. */
-		if (tmp & DS1307_BIT_CH)
-			i2c_smbus_write_byte_data(client, DS1307_REG_SECS, 0);
-
-		/* oscillator fault?  clear flag, and warn */
-		if (regs[DS1307_REG_CONTROL] & DS1338_BIT_OSF) {
-			i2c_smbus_write_byte_data(client, DS1307_REG_CONTROL,
-					regs[DS1307_REG_CONTROL]
-					& ~DS1338_BIT_OSF);
-			dev_warn(&client->dev, "SET TIME!\n");
-			goto read_rtc;
-		}
-		break;
-	case ds_1340:
-		/* clock halted?  turn it on, so clock can tick. */
-		if (tmp & DS1340_BIT_nEOSC)
-			i2c_smbus_write_byte_data(client, DS1307_REG_SECS, 0);
-
-		tmp = i2c_smbus_read_byte_data(client, DS1340_REG_FLAG);
-		if (tmp < 0) {
+	for (retries = 0; retries < 5; retries++) {
+		/* read RTC registers */
+		tmp = ds1307->read_block_data(ds1307->client,
+					      ds1307->offset, 8, regs);
+		if (tmp != 8) {
 			dev_dbg(&client->dev, "read error %d\n", tmp);
 			return -EIO;
 		}
 
-		/* oscillator fault?  clear flag, and warn */
-		if (tmp & DS1340_BIT_OSF) {
-			i2c_smbus_write_byte_data(client, DS1340_REG_FLAG, 0);
-			dev_warn(&client->dev, "SET TIME!\n");
-		}
-		break;
-	case mcp794xx:
-		/* make sure that the backup battery is enabled */
-		if (!(regs[DS1307_REG_WDAY] & MCP794XX_BIT_VBATEN)) {
-			i2c_smbus_write_byte_data(client, DS1307_REG_WDAY,
-					regs[DS1307_REG_WDAY]
-					| MCP794XX_BIT_VBATEN);
-		}
+		/*
+		 * minimal sanity checking; some chips (like DS1340)
+		 * don't specify the extra bits as must-be-zero, but
+		 * there are still a few values that are clearly
+		 * out-of-range.
+		 */
+		tmp = regs[DS1307_REG_SECS];
+		switch (ds1307->type) {
+		case ds_1307:
+		case m41t00:
+			/* clock halted?  turn it on, so clock can tick. */
+			if (tmp & DS1307_BIT_CH) {
+				i2c_smbus_write_byte_data(client,
+							  DS1307_REG_SECS, 0);
+				dev_warn(&client->dev, "SET TIME!\n");
+				continue;
+			}
+			break;
+		case ds_1338:
+			/* clock halted?  turn it on, so clock can tick. */
+			if (tmp & DS1307_BIT_CH)
+				i2c_smbus_write_byte_data(client,
+							  DS1307_REG_SECS, 0);
+
+			/* oscillator fault?  clear flag, and warn */
+			if (regs[DS1307_REG_CONTROL] & DS1338_BIT_OSF) {
+				i2c_smbus_write_byte_data(client,
+							  DS1307_REG_CONTROL,
+							  regs[DS1307_REG_CONTROL]
+							  & ~DS1338_BIT_OSF);
+				dev_warn(&client->dev, "SET TIME!\n");
+				continue;
+			}
+			break;
+		case ds_1340:
+			/* clock halted?  turn it on, so clock can tick. */
+			if (tmp & DS1340_BIT_nEOSC)
+				i2c_smbus_write_byte_data(client,
+							  DS1307_REG_SECS, 0);
+
+			tmp = i2c_smbus_read_byte_data(client, DS1340_REG_FLAG);
+			if (tmp < 0) {
+				dev_dbg(&client->dev, "read error %d\n", tmp);
+				return -EIO;
+			}
+
+			/* oscillator fault?  clear flag, and warn */
+			if (tmp & DS1340_BIT_OSF) {
+				i2c_smbus_write_byte_data(client,
+							  DS1340_REG_FLAG, 0);
+				dev_warn(&client->dev, "SET TIME!\n");
+			}
+			return 0;
 
-		/* clock halted?  turn it on, so clock can tick. */
-		if (!(tmp & MCP794XX_BIT_ST)) {
-			i2c_smbus_write_byte_data(client, DS1307_REG_SECS,
-					MCP794XX_BIT_ST);
-			dev_warn(&client->dev, "SET TIME!\n");
-			goto read_rtc;
+		case mcp794xx:
+			/* make sure that the backup battery is enabled */
+			if (!(regs[DS1307_REG_WDAY] & MCP794XX_BIT_VBATEN)) {
+				i2c_smbus_write_byte_data(client,
+							  DS1307_REG_WDAY,
+							  regs[DS1307_REG_WDAY]
+							  | MCP794XX_BIT_VBATEN);
+			}
+
+			/* clock halted?  turn it on, so clock can tick. */
+			if (!(tmp & MCP794XX_BIT_ST)) {
+				i2c_smbus_write_byte_data(client,
+							  DS1307_REG_SECS,
+							  MCP794XX_BIT_ST);
+				dev_warn(&client->dev, "SET TIME!\n");
+				continue;
+			}
+
+			break;
+		default:
+			break;
 		}
 
-		break;
-	default:
-		break;
+		return 0;
 	}
 
-	return 0;
+	return -EIO;
 }
 
 static int ds1307_probe(struct i2c_client *client,
-- 
2.5.5

[PATCH 11/13] RTC: ds1307: Redefine RX8025_REG_* to minimize extra code

[Andrey Smirnov]

The only place in the driver where RX8025_REG_* are used they are always
shifted and ORed the same way, so instead of repeating that idiom make
it a part of symbolic constant.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/rtc/rtc-ds1307.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 8ccfe5b..76e66a3 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -103,9 +103,11 @@ enum ds_type {
 
 #define DS13XX_TRICKLE_CHARGER_MAGIC	0xa0
 
-#define RX8025_REG_CTRL1	0x0e
+#define RX8025_REG_CTRL1_	0x0e
+#define RX8025_REG_CTRL1	((RX8025_REG_CTRL1_ << 4) | 0x08)
 #	define RX8025_BIT_2412		0x20
-#define RX8025_REG_CTRL2	0x0f
+#define RX8025_REG_CTRL2_	0x0f
+#define RX8025_REG_CTRL2	((RX8025_REG_CTRL2_ << 4) | 0x08)
 #	define RX8025_BIT_PON		0x10
 #	define RX8025_BIT_VDET		0x40
 #	define RX8025_BIT_XST		0x20
@@ -1353,7 +1355,7 @@ static int ds1307_chip_configure(const struct ds1307 *ds1307)
 	}
 	case rx_8025:
 		tmp = i2c_smbus_read_i2c_block_data(client,
-						    RX8025_REG_CTRL1 << 4 | 0x08,
+						    RX8025_REG_CTRL1,
 						    2, regs);
 		if (tmp != 2) {
 			dev_dbg(&client->dev, "read error %d\n", tmp);
@@ -1364,7 +1366,7 @@ static int ds1307_chip_configure(const struct ds1307 *ds1307)
 		if (!(regs[1] & RX8025_BIT_XST)) {
 			regs[1] |= RX8025_BIT_XST;
 			i2c_smbus_write_byte_data(client,
-						  RX8025_REG_CTRL2 << 4 | 0x08,
+						  RX8025_REG_CTRL2,
 						  regs[1]);
 			dev_warn(&client->dev,
 				 "oscillator stop detected - SET TIME!\n");
@@ -1373,7 +1375,7 @@ static int ds1307_chip_configure(const struct ds1307 *ds1307)
 		if (regs[1] & RX8025_BIT_PON) {
 			regs[1] &= ~RX8025_BIT_PON;
 			i2c_smbus_write_byte_data(client,
-						  RX8025_REG_CTRL2 << 4 | 0x08,
+						  RX8025_REG_CTRL2,
 						  regs[1]);
 			dev_warn(&client->dev, "power-on detected\n");
 		}
@@ -1381,7 +1383,7 @@ static int ds1307_chip_configure(const struct ds1307 *ds1307)
 		if (regs[1] & RX8025_BIT_VDET) {
 			regs[1] &= ~RX8025_BIT_VDET;
 			i2c_smbus_write_byte_data(client,
-						  RX8025_REG_CTRL2 << 4 | 0x08,
+						  RX8025_REG_CTRL2,
 						  regs[1]);
 			dev_warn(&client->dev, "voltage drop detected\n");
 		}
@@ -1392,11 +1394,11 @@ static int ds1307_chip_configure(const struct ds1307 *ds1307)
 
 			/* switch to 24 hour mode */
 			i2c_smbus_write_byte_data(client,
-						  RX8025_REG_CTRL1 << 4 | 0x08,
+						  RX8025_REG_CTRL1,
 						  regs[0] | RX8025_BIT_2412);
 
 			tmp = i2c_smbus_read_i2c_block_data(client,
-							    RX8025_REG_CTRL1 << 4 | 0x08,
+							    RX8025_REG_CTRL1,
 							    2, regs);
 			if (tmp != 2) {
 				dev_dbg(&client->dev, "read error %d\n", tmp);
-- 
2.5.5

[PATCH 12/13] RTC: ds1307: Report oscillator problems more intelligently

[Andrey Smirnov]

Report oscillator problems more intelligently, by printing more
information about what cause the issue and not yelling "SET TIME!" at
the user.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/rtc/rtc-ds1307.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 76e66a3..d9693bd 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -1424,6 +1424,19 @@ static int ds1307_chip_configure(const struct ds1307 *ds1307)
 	return 0;
 }
 
+static void ds1307_report_clock_halt(const struct ds1307 *ds1307)
+{
+	dev_warn(&ds1307->client->dev, "RTC's oscillator is turned off. "
+		 "Turning it on. Please set time");
+}
+
+static void ds1307_report_oscillator_fault(const struct ds1307 *ds1307)
+{
+	dev_warn(&ds1307->client->dev, "RTC's reported oscillator fault. "
+		 "Clearing the fault flag and re-reading RTC status. "
+		 "Please set time");
+}
+
 static int ds1307_chip_sanity_check(const struct ds1307 *ds1307)
 {
 	int tmp, retries;
@@ -1453,7 +1466,7 @@ static int ds1307_chip_sanity_check(const struct ds1307 *ds1307)
 			if (tmp & DS1307_BIT_CH) {
 				i2c_smbus_write_byte_data(client,
 							  DS1307_REG_SECS, 0);
-				dev_warn(&client->dev, "SET TIME!\n");
+				ds1307_report_clock_halt(ds1307);
 				continue;
 			}
 			break;
@@ -1469,15 +1482,17 @@ static int ds1307_chip_sanity_check(const struct ds1307 *ds1307)
 							  DS1307_REG_CONTROL,
 							  regs[DS1307_REG_CONTROL]
 							  & ~DS1338_BIT_OSF);
-				dev_warn(&client->dev, "SET TIME!\n");
+				ds1307_report_oscillator_fault(ds1307);
 				continue;
 			}
 			break;
 		case ds_1340:
 			/* clock halted?  turn it on, so clock can tick. */
-			if (tmp & DS1340_BIT_nEOSC)
+			if (tmp & DS1340_BIT_nEOSC) {
 				i2c_smbus_write_byte_data(client,
 							  DS1307_REG_SECS, 0);
+				ds1307_report_clock_halt(ds1307);
+			}
 
 			tmp = i2c_smbus_read_byte_data(client, DS1340_REG_FLAG);
 			if (tmp < 0) {
@@ -1489,7 +1504,7 @@ static int ds1307_chip_sanity_check(const struct ds1307 *ds1307)
 			if (tmp & DS1340_BIT_OSF) {
 				i2c_smbus_write_byte_data(client,
 							  DS1340_REG_FLAG, 0);
-				dev_warn(&client->dev, "SET TIME!\n");
+				ds1307_report_oscillator_fault(ds1307);
 			}
 			return 0;
 
@@ -1500,6 +1515,9 @@ static int ds1307_chip_sanity_check(const struct ds1307 *ds1307)
 							  DS1307_REG_WDAY,
 							  regs[DS1307_REG_WDAY]
 							  | MCP794XX_BIT_VBATEN);
+				dev_warn(&client->dev,
+					 "battery backup was disabled. "
+					 "Re-enabling it\n");
 			}
 
 			/* clock halted?  turn it on, so clock can tick. */
@@ -1507,7 +1525,7 @@ static int ds1307_chip_sanity_check(const struct ds1307 *ds1307)
 				i2c_smbus_write_byte_data(client,
 							  DS1307_REG_SECS,
 							  MCP794XX_BIT_ST);
-				dev_warn(&client->dev, "SET TIME!\n");
+				ds1307_report_clock_halt(ds1307);
 				continue;
 			}
 
-- 
2.5.5

[PATCH 13/13] RTC: ds1307: Move last bits of sanity checking out of chip_configure

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/rtc/rtc-ds1307.c | 106 ++++++++++++++++++++++++++---------------------
 1 file changed, 59 insertions(+), 47 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index d9693bd..4452661 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -1344,66 +1344,23 @@ static int ds1307_chip_configure(const struct ds1307 *ds1307)
 					  DS1337_REG_CONTROL,
 					  regs[0]);
 
-		/* oscillator fault?  clear flag, and warn */
-		if (regs[1] & DS1337_BIT_OSF) {
-			i2c_smbus_write_byte_data(client,
-						  DS1337_REG_STATUS,
-						  regs[1] & ~DS1337_BIT_OSF);
-			dev_warn(&ds1307->client->dev, "SET TIME!\n");
-		}
 		break;
 	}
 	case rx_8025:
-		tmp = i2c_smbus_read_i2c_block_data(client,
-						    RX8025_REG_CTRL1,
-						    2, regs);
-		if (tmp != 2) {
+		tmp = i2c_smbus_read_byte_data(client, RX8025_REG_CTRL1);
+		if (tmp < 0) {
 			dev_dbg(&client->dev, "read error %d\n", tmp);
 			return -EIO;
 		}
 
-		/* oscillator off?  turn it on, so clock can tick. */
-		if (!(regs[1] & RX8025_BIT_XST)) {
-			regs[1] |= RX8025_BIT_XST;
-			i2c_smbus_write_byte_data(client,
-						  RX8025_REG_CTRL2,
-						  regs[1]);
-			dev_warn(&client->dev,
-				 "oscillator stop detected - SET TIME!\n");
-		}
-
-		if (regs[1] & RX8025_BIT_PON) {
-			regs[1] &= ~RX8025_BIT_PON;
-			i2c_smbus_write_byte_data(client,
-						  RX8025_REG_CTRL2,
-						  regs[1]);
-			dev_warn(&client->dev, "power-on detected\n");
-		}
-
-		if (regs[1] & RX8025_BIT_VDET) {
-			regs[1] &= ~RX8025_BIT_VDET;
-			i2c_smbus_write_byte_data(client,
-						  RX8025_REG_CTRL2,
-						  regs[1]);
-			dev_warn(&client->dev, "voltage drop detected\n");
-		}
-
 		/* make sure we are running in 24hour mode */
-		if (!(regs[0] & RX8025_BIT_2412)) {
+		if (!(tmp & RX8025_BIT_2412)) {
 			u8 hour;
 
 			/* switch to 24 hour mode */
 			i2c_smbus_write_byte_data(client,
 						  RX8025_REG_CTRL1,
-						  regs[0] | RX8025_BIT_2412);
-
-			tmp = i2c_smbus_read_i2c_block_data(client,
-							    RX8025_REG_CTRL1,
-							    2, regs);
-			if (tmp != 2) {
-				dev_dbg(&client->dev, "read error %d\n", tmp);
-				return -EIO;
-			}
+						  tmp | RX8025_BIT_2412);
 
 			/* correct hour */
 			hour = bcd2bin(regs[DS1307_REG_HOUR]);
@@ -1486,6 +1443,27 @@ static int ds1307_chip_sanity_check(const struct ds1307 *ds1307)
 				continue;
 			}
 			break;
+
+		case ds_1337:
+		case ds_1339:
+		case ds_3231:
+		case ds_1341:
+			tmp = i2c_smbus_read_byte_data(client,
+						       DS1337_REG_STATUS);
+			if (tmp < 0) {
+				dev_dbg(&client->dev, "read error %d\n", tmp);
+				return -EIO;
+			}
+
+			/* oscillator fault?  clear flag, and warn */
+			if (tmp & DS1337_BIT_OSF) {
+				i2c_smbus_write_byte_data(client,
+							  DS1337_REG_STATUS,
+							  regs[1] & ~DS1337_BIT_OSF);
+				ds1307_report_oscillator_fault(ds1307);
+			}
+			return 0;
+
 		case ds_1340:
 			/* clock halted?  turn it on, so clock can tick. */
 			if (tmp & DS1340_BIT_nEOSC) {
@@ -1530,6 +1508,40 @@ static int ds1307_chip_sanity_check(const struct ds1307 *ds1307)
 			}
 
 			break;
+
+		case rx_8025:
+			tmp = i2c_smbus_read_byte_data(client,
+						       RX8025_REG_CTRL2);
+			if (tmp < 0) {
+				dev_dbg(&client->dev, "read error %d\n", tmp);
+				return -EIO;
+			}
+
+			/* oscillator off?  turn it on, so clock can tick. */
+			if (!(tmp & RX8025_BIT_XST)) {
+				tmp |= RX8025_BIT_XST;
+				i2c_smbus_write_byte_data(client,
+							  RX8025_REG_CTRL2,
+							  tmp);
+				ds1307_report_clock_halt(ds1307);
+			}
+
+			if (tmp & RX8025_BIT_PON) {
+				tmp &= ~RX8025_BIT_PON;
+				i2c_smbus_write_byte_data(client,
+							  RX8025_REG_CTRL2,
+							  tmp);
+				dev_warn(&client->dev, "power-on detected\n");
+			}
+
+			if (tmp & RX8025_BIT_VDET) {
+				tmp &= ~RX8025_BIT_VDET;
+				i2c_smbus_write_byte_data(client,
+							  RX8025_REG_CTRL2,
+							  tmp);
+				dev_warn(&client->dev, "voltage drop detected\n");
+			}
+			break;
 		default:
 			break;
 		}
-- 
2.5.5

Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options

[Rob Herring]

On Tue, Jun 14, 2016 at 10:59:29PM -0700, Andrey Smirnov wrote:
> Add DS1341 specific power-saving options that allow to disable certain
> functional aspects of the chip in order to minimize its power
> consumption.

This description doesn't match that you are adding a new binding. It is 
preferred that bindings are a separate patch.

> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  .../devicetree/bindings/rtc/dallas,ds1341.txt      | 23 ++++++++++++++++++
>  drivers/rtc/rtc-ds1307.c                           | 28 ++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
> 
> diff --git a/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
> new file mode 100644
> index 0000000..b8be7a4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
> @@ -0,0 +1,23 @@
> +* Dallas DS1341		I2C Serial Real-Time Clock
> +
> +Required properties:
> +
> +- compatible: Should contain "dallas,ds1341".
> +
> +- reg: I2C address for chip
> +
> +Optional properties:
> +
> +- disable-oscillator-stop-flag : Configure chip to disable oscillator
> +  fault detection circuitry
> +
> +- enable-glitch-filter : Configure chip to enable crystal oscillator
> +  output glitch filtering

What determines setting these properties or not?

They should have vendor prefix and be explicit that they are boolean.

Rob

Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options

[Andrey Smirnov]

On Sun, Jun 19, 2016 at 7:29 AM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Jun 14, 2016 at 10:59:29PM -0700, Andrey Smirnov wrote:
>> Add DS1341 specific power-saving options that allow to disable certain
>> functional aspects of the chip in order to minimize its power
>> consumption.
>
> This description doesn't match that you are adding a new binding. It is
> preferred that bindings are a separate patch.

OK, will split this patch into two in v2.

>
>>
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>  .../devicetree/bindings/rtc/dallas,ds1341.txt      | 23 ++++++++++++++++++
>>  drivers/rtc/rtc-ds1307.c                           | 28 ++++++++++++++++++++++
>>  2 files changed, 51 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
>>
>> diff --git a/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
>> new file mode 100644
>> index 0000000..b8be7a4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
>> @@ -0,0 +1,23 @@
>> +* Dallas DS1341              I2C Serial Real-Time Clock
>> +
>> +Required properties:
>> +
>> +- compatible: Should contain "dallas,ds1341".
>> +
>> +- reg: I2C address for chip
>> +
>> +Optional properties:
>> +
>> +- disable-oscillator-stop-flag : Configure chip to disable oscillator
>> +  fault detection circuitry
>> +
>> +- enable-glitch-filter : Configure chip to enable crystal oscillator
>> +  output glitch filtering
>
> What determines setting these properties or not?

Setting those properties allows drastically reduce RTC's power
consumption at the expense of reliability and quality of service. In
my use case, DS1341 is powered by a supercap and enabling those two
setting allows to increase holdup from less than a day do 2+ weeks.

>
> They should have vendor prefix and be explicit that they are boolean.

I was trying to be consistent with ds1339 and ds1390 bindings which do
not have vendor prefixes. Will fix in v2.

Thank you,
Andrey Smirnov

Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options

[Rob Herring]

On Sun, Jun 19, 2016 at 11:12:55AM -0700, Andrey Smirnov wrote:
> On Sun, Jun 19, 2016 at 7:29 AM, Rob Herring <robh@kernel.org> wrote:
> > On Tue, Jun 14, 2016 at 10:59:29PM -0700, Andrey Smirnov wrote:
> >> Add DS1341 specific power-saving options that allow to disable certain
> >> functional aspects of the chip in order to minimize its power
> >> consumption.
> >
> > This description doesn't match that you are adding a new binding. It is
> > preferred that bindings are a separate patch.
> 
> OK, will split this patch into two in v2.
> 
> >
> >>
> >> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> >> ---
> >>  .../devicetree/bindings/rtc/dallas,ds1341.txt      | 23 ++++++++++++++++++
> >>  drivers/rtc/rtc-ds1307.c                           | 28 ++++++++++++++++++++++
> >>  2 files changed, 51 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
> >> new file mode 100644
> >> index 0000000..b8be7a4
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
> >> @@ -0,0 +1,23 @@
> >> +* Dallas DS1341              I2C Serial Real-Time Clock
> >> +
> >> +Required properties:
> >> +
> >> +- compatible: Should contain "dallas,ds1341".
> >> +
> >> +- reg: I2C address for chip
> >> +
> >> +Optional properties:
> >> +
> >> +- disable-oscillator-stop-flag : Configure chip to disable oscillator
> >> +  fault detection circuitry
> >> +
> >> +- enable-glitch-filter : Configure chip to enable crystal oscillator
> >> +  output glitch filtering
> >
> > What determines setting these properties or not?
> 
> Setting those properties allows drastically reduce RTC's power
> consumption at the expense of reliability and quality of service. In
> my use case, DS1341 is powered by a supercap and enabling those two
> setting allows to increase holdup from less than a day do 2+ weeks.

So wouldn't you want to set one mode while running and the lower power 
mode while suspended? I'm trying to understand the frequency of changing 
this. If it is always one setting for a board, then yes it belongs in 
DT. If it is a user decision, then it probably shouldn't be in DT.

Seeing as these are reused, I've probably already had this discussion...

> > They should have vendor prefix and be explicit that they are boolean.
> 
> I was trying to be consistent with ds1339 and ds1390 bindings which do
> not have vendor prefixes. Will fix in v2.

Okay, then they are fine if you are using existing properties. Perhaps 
these should all be in a common binding doc though.

Rob

Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options

[Alexandre Belloni]

On 21/06/2016 at 15:49:04 -0500, Rob Herring wrote :
> So wouldn't you want to set one mode while running and the lower power 
> mode while suspended? I'm trying to understand the frequency of changing 
> this. If it is always one setting for a board, then yes it belongs in 
> DT. If it is a user decision, then it probably shouldn't be in DT.
> 
> Seeing as these are reused, I've probably already had this discussion...
> 

I would agree with Rob here. It may be better to provide a sysfs
interface to configure that particular behavior. This is usually ok
because the use case is:
 - the RTC is not configured, time has never been set
 - time is set for the first time
 - the user can set the oscillator mode/detection/...
 - on subsequent reboots, the mode is kept alongside the time and date

I would advise against trying to set a mode automatically in the driver
because you may have unexpected power cuts and it may then let the RTC
consume more power than what you really want.

> > > They should have vendor prefix and be explicit that they are boolean.
> > 
> > I was trying to be consistent with ds1339 and ds1390 bindings which do
> > not have vendor prefixes. Will fix in v2.
> 
> Okay, then they are fine if you are using existing properties. Perhaps 
> these should all be in a common binding doc though.
> 

I'll try to collect the existing common properties and write that doc
this week.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options

[Andrey Smirnov]

> So wouldn't you want to set one mode while running and the lower power
> mode while suspended? I'm trying to understand the frequency of changing
> this. If it is always one setting for a board, then yes it belongs in
> DT. If it is a user decision, then it probably shouldn't be in DT.

I don't really see a use-case where you'd want setting that
dynamically. I might be wrong, but IMHO, power consumption of a system
in suspended mode would dwarf that of an RTC, so there's really much
to gain by enabling this feature dynamically. Where it matters the
most is time setting retention when the system is powered off and RTC
is ticking off of a battery or a some other power storage device. So
in my opinion it is more of a system design question where one has to
choose if reliability of RTC data is more important (detection of
oscillator faults and higher oscillator glitch immunity) and bigger
power storage device is needed or higher risk of RTC "malfunction" is
acceptable and cheaper/more convenient power storage device can be
used

>
> Seeing as these are reused, I've probably already had this discussion...
>
>> > They should have vendor prefix and be explicit that they are boolean.
>>
>> I was trying to be consistent with ds1339 and ds1390 bindings which do
>> not have vendor prefixes. Will fix in v2.
>
> Okay, then they are fine if you are using existing properties. Perhaps
> these should all be in a common binding doc though.

I think we have a misunderstanding. What I meant by "trying to be
consistent" was that bindings for other DS1307 variants do not prefix
their own properties with vendor name. Your comment about properties
being reused makes me suspicious that I misled you to believe that
other chip variants use those exact properties which is not the case.

Andrey

Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options

[Andrey Smirnov]

On Tue, Jun 21, 2016 at 2:07 PM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> On 21/06/2016 at 15:49:04 -0500, Rob Herring wrote :
>> So wouldn't you want to set one mode while running and the lower power
>> mode while suspended? I'm trying to understand the frequency of changing
>> this. If it is always one setting for a board, then yes it belongs in
>> DT. If it is a user decision, then it probably shouldn't be in DT.
>>
>> Seeing as these are reused, I've probably already had this discussion...
>>
>
> I would agree with Rob here. It may be better to provide a sysfs
> interface to configure that particular behavior.

I don't see any value in doing that, could you give me a realistic
example of a scenario in which a user would want to spend some of
uptime with RTC oscillator fault detection/glitch filtering disabled
and then enable it?

> This is usually ok because the use case is:
>  - the RTC is not configured, time has never been set
>  - time is set for the first time
>  - the user can set the oscillator mode/detection/...

Unfortunately exposing that feature using sysfs gives you a leaky
abstraction and your userspace instead of using a generic RTC starts
using DS1341 RTC. So to accommodate for that a user would have to:

a) Write + integrate a userspace tool to set the mode (which IMHO is
decided upon once and doesn't change)
b) If a board design is new and there's a chance of moving this chip
to a different I2C bus, the code above would have to account for that
and not hardcore sysfs path
c) If board's BSP is intended to be used in multiple generations of a
product, not all of which would use DS1341, it would be necessary to
accommodate for that by either more code in the tool or an additional
BSP build configuration variant

>  - on subsequent reboots, the mode is kept alongside the time and date
>

This assumes that your bootloader leaves those mode bits alone.

> I would advise against trying to set a mode automatically in the driver
> because you may have unexpected power cuts and it may then let the RTC
> consume more power than what you really want.
>

I fell like I am not understanding you correctly.  Why would moving
configuration decision logic into userspace improve the situation in
case of unexpected power loss?

Andrey

Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options

[Andrey Smirnov]

On Tue, Jun 21, 2016 at 7:34 PM, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> On Tue, Jun 21, 2016 at 2:07 PM, Alexandre Belloni
> <alexandre.belloni@free-electrons.com> wrote:
>> On 21/06/2016 at 15:49:04 -0500, Rob Herring wrote :
>>> So wouldn't you want to set one mode while running and the lower power
>>> mode while suspended? I'm trying to understand the frequency of changing
>>> this. If it is always one setting for a board, then yes it belongs in
>>> DT. If it is a user decision, then it probably shouldn't be in DT.
>>>
>>> Seeing as these are reused, I've probably already had this discussion...
>>>
>>
>> I would agree with Rob here. It may be better to provide a sysfs
>> interface to configure that particular behavior.
>
> I don't see any value in doing that, could you give me a realistic
> example of a scenario in which a user would want to spend some of
> uptime with RTC oscillator fault detection/glitch filtering disabled
> and then enable it?
>
>> This is usually ok because the use case is:
>>  - the RTC is not configured, time has never been set
>>  - time is set for the first time
>>  - the user can set the oscillator mode/detection/...
>
> Unfortunately exposing that feature using sysfs gives you a leaky
> abstraction and your userspace instead of using a generic RTC starts
> using DS1341 RTC. So to accommodate for that a user would have to:
>
> a) Write + integrate a userspace tool to set the mode (which IMHO is
> decided upon once and doesn't change)
> b) If a board design is new and there's a chance of moving this chip
> to a different I2C bus, the code above would have to account for that
> and not hardcore sysfs path
> c) If board's BSP is intended to be used in multiple generations of a
> product, not all of which would use DS1341, it would be necessary to
> accommodate for that by either more code in the tool or an additional
> BSP build configuration variant
>
>>  - on subsequent reboots, the mode is kept alongside the time and date
>>
>
> This assumes that your bootloader leaves those mode bits alone.
>
>> I would advise against trying to set a mode automatically in the driver
>> because you may have unexpected power cuts and it may then let the RTC
>> consume more power than what you really want.
>>
>
> I fell like I am not understanding you correctly.  Why would moving
> configuration decision logic into userspace improve the situation in
> case of unexpected power loss?
>
> Andrey


Alexandre, what's the status of this discussion/patchset? Would it be
more acceptable if I dropped all of the refactoring and just kept the
code adding new feature + DT binding for it? I am more than happy to
drop all but essential parts of the patches if that would allow us to
make progress.

Thank you,
Andrey

Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options

[Alexandre Belloni]

On 21/06/2016 at 19:34:56 -0700, Andrey Smirnov wrote :
> On Tue, Jun 21, 2016 at 2:07 PM, Alexandre Belloni
> <alexandre.belloni@free-electrons.com> wrote:
> > On 21/06/2016 at 15:49:04 -0500, Rob Herring wrote :
> >> So wouldn't you want to set one mode while running and the lower power
> >> mode while suspended? I'm trying to understand the frequency of changing
> >> this. If it is always one setting for a board, then yes it belongs in
> >> DT. If it is a user decision, then it probably shouldn't be in DT.
> >>
> >> Seeing as these are reused, I've probably already had this discussion...
> >>
> >
> > I would agree with Rob here. It may be better to provide a sysfs
> > interface to configure that particular behavior.
> 
> I don't see any value in doing that, could you give me a realistic
> example of a scenario in which a user would want to spend some of
> uptime with RTC oscillator fault detection/glitch filtering disabled
> and then enable it?
> 

Well, the issue is not being dynamic, it is differentiating between
hardware description and user configuration. Configuration must not be in
DT. And this choice is definitively not hardware related (as opposed to
the trickle charging that depends on the battery that is used on the
board).

> > This is usually ok because the use case is:
> >  - the RTC is not configured, time has never been set
> >  - time is set for the first time
> >  - the user can set the oscillator mode/detection/...
> 
> Unfortunately exposing that feature using sysfs gives you a leaky
> abstraction and your userspace instead of using a generic RTC starts
> using DS1341 RTC. So to accommodate for that a user would have to:
> 
> a) Write + integrate a userspace tool to set the mode (which IMHO is
> decided upon once and doesn't change)
> b) If a board design is new and there's a chance of moving this chip
> to a different I2C bus, the code above would have to account for that
> and not hardcore sysfs path
> c) If board's BSP is intended to be used in multiple generations of a
> product, not all of which would use DS1341, it would be necessary to
> accommodate for that by either more code in the tool or an additional
> BSP build configuration variant
> 

Well, it doesn't leak abstraction as long as all the RTC that are able
to disable the oscillator failure detection use the same ABI.

> >  - on subsequent reboots, the mode is kept alongside the time and date
> >
> 
> This assumes that your bootloader leaves those mode bits alone.
> 

Well, if that is not the case, the bootloader as to be fixed anyway and
silently changing the configuration back using DT is probably worse.

> > I would advise against trying to set a mode automatically in the driver
> > because you may have unexpected power cuts and it may then let the RTC
> > consume more power than what you really want.
> >
> 
> I fell like I am not understanding you correctly.  Why would moving
> configuration decision logic into userspace improve the situation in
> case of unexpected power loss?
> 

I was reacting to Rob's idea to running in the lower power mode when
suspend. I'm pretty sure this is not a good idea. I'd leave the whole
policy to userspace.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options

[Andrey Smirnov]

On Tue, Jul 19, 2016 at 3:47 PM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> On 21/06/2016 at 19:34:56 -0700, Andrey Smirnov wrote :
>> On Tue, Jun 21, 2016 at 2:07 PM, Alexandre Belloni
>> <alexandre.belloni@free-electrons.com> wrote:
>> > On 21/06/2016 at 15:49:04 -0500, Rob Herring wrote :
>> >> So wouldn't you want to set one mode while running and the lower power
>> >> mode while suspended? I'm trying to understand the frequency of changing
>> >> this. If it is always one setting for a board, then yes it belongs in
>> >> DT. If it is a user decision, then it probably shouldn't be in DT.
>> >>
>> >> Seeing as these are reused, I've probably already had this discussion...
>> >>
>> >
>> > I would agree with Rob here. It may be better to provide a sysfs
>> > interface to configure that particular behavior.
>>
>> I don't see any value in doing that, could you give me a realistic
>> example of a scenario in which a user would want to spend some of
>> uptime with RTC oscillator fault detection/glitch filtering disabled
>> and then enable it?
>>
>
> Well, the issue is not being dynamic, it is differentiating between
> hardware description and user configuration. Configuration must not be in
> DT.

Why? And I don't mean in a generic sense, but in this particular case.
What is gained by not having this bit of configuration, whose only
consumer is the driver, in the device tree file?

> And this choice is definitively not hardware related (as opposed to
> the trickle charging that depends on the battery that is used on the
> board).

There's most certainly plenty of precedents of non hardware-related in
device tree, first two that come to mind are "chosen" node and
"local-mac-address" property and, granted, those might be
exceptions/legacy bindings that are just there to stay, but even if
you look at RTC subsystem rtc-cmos.txt, atmel,at91sam-rtc.txt and
possibly rtc-st-lpc.txt are providing bindings that are not exactly
hardware related.

Rtc-cmos.txt is especially noticeable example since it literally does
what I am trying to do -- allows the user to specify initial values to
certain registers that would be initialized by the driver.

>
>> > This is usually ok because the use case is:
>> >  - the RTC is not configured, time has never been set
>> >  - time is set for the first time
>> >  - the user can set the oscillator mode/detection/...
>>
>> Unfortunately exposing that feature using sysfs gives you a leaky
>> abstraction and your userspace instead of using a generic RTC starts
>> using DS1341 RTC. So to accommodate for that a user would have to:
>>
>> a) Write + integrate a userspace tool to set the mode (which IMHO is
>> decided upon once and doesn't change)
>> b) If a board design is new and there's a chance of moving this chip
>> to a different I2C bus, the code above would have to account for that
>> and not hardcore sysfs path
>> c) If board's BSP is intended to be used in multiple generations of a
>> product, not all of which would use DS1341, it would be necessary to
>> accommodate for that by either more code in the tool or an additional
>> BSP build configuration variant
>>
>
> Well, it doesn't leak abstraction as long as all the RTC that are able
> to disable the oscillator failure detection use the same ABI.

Correct me if I am wrong, but no such, at least semi-standardized, ABI
exist as of today, correct? If that is so, then what you are saying is
that the abstraction doesn't leak as long as you put it inside of a
new abstraction that doesn't leak. I am not arguing that it is
impossible to create a new one that would allow to hide hardware
differences, I am positive it is, what I am arguing is that to do so
is a lot of work for as far as I can see for no gain.

>
>> >  - on subsequent reboots, the mode is kept alongside the time and date
>> >
>>
>> This assumes that your bootloader leaves those mode bits alone.
>>
>
> Well, if that is not the case, the bootloader as to be fixed anyway and
> silently changing the configuration back using DT is probably worse.
>

How so? Consider the following two scenarios with assumption that the
bootloader is broken:

  - Bits set wrong by bootloader, then corrected by kernel, device is
powered off RTC consumes expected amount of current

  - Bits set wrong by bootloader, kernel does nothing, device is
powered off RTC consumes more than anticipated and we drain the power
storage device and loose time.

What do you you think former is worse than latter?

Andrey

Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options

[Alexandre Belloni]

On 19/07/2016 at 16:56:56 -0700, Andrey Smirnov wrote :
> >> I don't see any value in doing that, could you give me a realistic
> >> example of a scenario in which a user would want to spend some of
> >> uptime with RTC oscillator fault detection/glitch filtering disabled
> >> and then enable it?
> >>
> >
> > Well, the issue is not being dynamic, it is differentiating between
> > hardware description and user configuration. Configuration must not be in
> > DT.
> 
> Why? And I don't mean in a generic sense, but in this particular case.
> What is gained by not having this bit of configuration, whose only
> consumer is the driver, in the device tree file?
> 

Because configuration doesn't belong to DT. DT is about hardware
description, not configuration.


> > And this choice is definitively not hardware related (as opposed to
> > the trickle charging that depends on the battery that is used on the
> > board).
> 
> There's most certainly plenty of precedents of non hardware-related in
> device tree, first two that come to mind are "chosen" node and
> "local-mac-address" property and, granted, those might be
> exceptions/legacy bindings that are just there to stay, but even if
> you look at RTC subsystem rtc-cmos.txt, atmel,at91sam-rtc.txt and
> possibly rtc-st-lpc.txt are providing bindings that are not exactly
> hardware related.
>
> Rtc-cmos.txt is especially noticeable example since it literally does
> what I am trying to do -- allows the user to specify initial values to
> certain registers that would be initialized by the driver.
> 

Well, the RTC subsystem has been left unmaintained for a while and it is
not because we made mistakes in the past that we have to make them
again.

rtc-st-lpc is only hardware related. The mode it is used in is board
dependant. And I have a plan to change how the gpbr register is
allocated for the at91 RTT. I agree that rtc-cmos is an example of what
not to do.

> > Well, it doesn't leak abstraction as long as all the RTC that are able
> > to disable the oscillator failure detection use the same ABI.
> 
> Correct me if I am wrong, but no such, at least semi-standardized, ABI
> exist as of today, correct? If that is so, then what you are saying is
> that the abstraction doesn't leak as long as you put it inside of a
> new abstraction that doesn't leak. I am not arguing that it is
> impossible to create a new one that would allow to hide hardware
> differences, I am positive it is, what I am arguing is that to do so
> is a lot of work for as far as I can see for no gain.
> 

You are correct, that ABI doesn't exist and I'm asking to create it.
That is how kernel development happens.

> >
> >> >  - on subsequent reboots, the mode is kept alongside the time and date
> >> >
> >>
> >> This assumes that your bootloader leaves those mode bits alone.
> >>
> >
> > Well, if that is not the case, the bootloader as to be fixed anyway and
> > silently changing the configuration back using DT is probably worse.
> >
> 
> How so? Consider the following two scenarios with assumption that the
> bootloader is broken:
> 
>   - Bits set wrong by bootloader, then corrected by kernel, device is
> powered off RTC consumes expected amount of current
> 
>   - Bits set wrong by bootloader, kernel does nothing, device is
> powered off RTC consumes more than anticipated and we drain the power
> storage device and loose time.
> 
> What do you you think former is worse than latter?
> 

Whether is is done in the kernel or in userspace doesn't change much
to that use case.



-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options

[Andrey Smirnov]

On Wed, Jul 20, 2016 at 2:02 AM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> On 19/07/2016 at 16:56:56 -0700, Andrey Smirnov wrote :
>> >> I don't see any value in doing that, could you give me a realistic
>> >> example of a scenario in which a user would want to spend some of
>> >> uptime with RTC oscillator fault detection/glitch filtering disabled
>> >> and then enable it?
>> >>
>> >
>> > Well, the issue is not being dynamic, it is differentiating between
>> > hardware description and user configuration. Configuration must not be in
>> > DT.
>>
>> Why? And I don't mean in a generic sense, but in this particular case.
>> What is gained by not having this bit of configuration, whose only
>> consumer is the driver, in the device tree file?
>>
>
> Because configuration doesn't belong to DT. DT is about hardware
> description, not configuration.

That doesn't really answer my question. You just re-iterating some
maxim without explaining what is the point behind applying it.

>
>
>> > And this choice is definitively not hardware related (as opposed to
>> > the trickle charging that depends on the battery that is used on the
>> > board).
>>
>> There's most certainly plenty of precedents of non hardware-related in
>> device tree, first two that come to mind are "chosen" node and
>> "local-mac-address" property and, granted, those might be
>> exceptions/legacy bindings that are just there to stay, but even if
>> you look at RTC subsystem rtc-cmos.txt, atmel,at91sam-rtc.txt and
>> possibly rtc-st-lpc.txt are providing bindings that are not exactly
>> hardware related.
>>
>> Rtc-cmos.txt is especially noticeable example since it literally does
>> what I am trying to do -- allows the user to specify initial values to
>> certain registers that would be initialized by the driver.
>>
>
> Well, the RTC subsystem has been left unmaintained for a while and it is
> not because we made mistakes in the past that we have to make them
> again.
>
> rtc-st-lpc is only hardware related. The mode it is used in is board
> dependant. And I have a plan to change how the gpbr register is
> allocated for the at91 RTT. I agree that rtc-cmos is an example of what
> not to do.
>
>> > Well, it doesn't leak abstraction as long as all the RTC that are able
>> > to disable the oscillator failure detection use the same ABI.
>>
>> Correct me if I am wrong, but no such, at least semi-standardized, ABI
>> exist as of today, correct? If that is so, then what you are saying is
>> that the abstraction doesn't leak as long as you put it inside of a
>> new abstraction that doesn't leak. I am not arguing that it is
>> impossible to create a new one that would allow to hide hardware
>> differences, I am positive it is, what I am arguing is that to do so
>> is a lot of work for as far as I can see for no gain.
>>
>
> You are correct, that ABI doesn't exist and I'm asking to create it.
> That is how kernel development happens.

OK, I don't think this is going anywhere. I am sure by now you know
very well, that I am not going to agree to that. I'll just drop this
part, gut the patchset to it's bare minimum and re-submit it.

Andrey

Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options

[Alexandre Belloni]

On 20/07/2016 at 07:36:55 -0700, Andrey Smirnov wrote :
> On Wed, Jul 20, 2016 at 2:02 AM, Alexandre Belloni
> <alexandre.belloni@free-electrons.com> wrote:
> > On 19/07/2016 at 16:56:56 -0700, Andrey Smirnov wrote :
> >> >> I don't see any value in doing that, could you give me a realistic
> >> >> example of a scenario in which a user would want to spend some of
> >> >> uptime with RTC oscillator fault detection/glitch filtering disabled
> >> >> and then enable it?
> >> >>
> >> >
> >> > Well, the issue is not being dynamic, it is differentiating between
> >> > hardware description and user configuration. Configuration must not be in
> >> > DT.
> >>
> >> Why? And I don't mean in a generic sense, but in this particular case.
> >> What is gained by not having this bit of configuration, whose only
> >> consumer is the driver, in the device tree file?
> >>
> >
> > Because configuration doesn't belong to DT. DT is about hardware
> > description, not configuration.
> 
> That doesn't really answer my question. You just re-iterating some
> maxim without explaining what is the point behind applying it.
> 

Well, that is from the device tree specification and how the device tree
maintainers want it...

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options

[Andrey Smirnov]

On Wed, Jul 20, 2016 at 8:38 AM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> On 20/07/2016 at 07:36:55 -0700, Andrey Smirnov wrote :
>> On Wed, Jul 20, 2016 at 2:02 AM, Alexandre Belloni
>> <alexandre.belloni@free-electrons.com> wrote:
>> > On 19/07/2016 at 16:56:56 -0700, Andrey Smirnov wrote :
>> >> >> I don't see any value in doing that, could you give me a realistic
>> >> >> example of a scenario in which a user would want to spend some of
>> >> >> uptime with RTC oscillator fault detection/glitch filtering disabled
>> >> >> and then enable it?
>> >> >>
>> >> >
>> >> > Well, the issue is not being dynamic, it is differentiating between
>> >> > hardware description and user configuration. Configuration must not be in
>> >> > DT.
>> >>
>> >> Why? And I don't mean in a generic sense, but in this particular case.
>> >> What is gained by not having this bit of configuration, whose only
>> >> consumer is the driver, in the device tree file?
>> >>
>> >
>> > Because configuration doesn't belong to DT. DT is about hardware
>> > description, not configuration.
>>
>> That doesn't really answer my question. You just re-iterating some
>> maxim without explaining what is the point behind applying it.
>>
>
> Well, that is from the device tree specification and how the device tree
> maintainers want it...

And yet, we have whole subsystems such as "nvmem" and I am sure plenty
of other smaller examples where that maxim is being applied very lax
if at all. So it seems people can and do make compromises regarding
the "no configuration in DT" rule if pros of doing so outweighs the
cons.

Andrey