[PATCH 0/5] rtc: ds1307: factor out more stuff from ds1307_probe and improve ds1307_set_time

[PATCH 0/5] rtc: ds1307: factor out more stuff from ds1307_probe and improve ds1307_set_time

[Heiner Kallweit]

Final goal of the refactoring is to abstract everything that the chips
have in common and handle it in generic code. Then we can get rid of
all the "switch (chip)" and "if (chip == xxx)" code.

To give one example:
A lot of chips have a bit for setting 12hr / 24hr mode. However some
chips have this config bit in the timekeeping registers, others in
a config register, and on some chips it's inverted.
But the functionality of the bit is always the same.

Ultimately adding support for a chip just requires to add one config
structure member.

The way to reach this goal is a long one and to faciliate reviewing
the patches I'll split them into series of 5 to 10 patches.

Heiner Kallweit (5):
  rtc: ds1307: factor out determining the chip type
  rtc: ds1307: factor out trickle charger initialization
  rtc: ds1307: factor out fixing the weekday
  rtc: ds1307: introduce constants for the timekeeping register masks
  rtc: ds1307: improve ds1307_set_time to respect config flag bits

 drivers/rtc/rtc-ds1307.c | 256 ++++++++++++++++++++++++++---------------------
 1 file changed, 140 insertions(+), 116 deletions(-)

-- 
2.14.1

[PATCH 1/5] rtc: ds1307: factor out determining the chip type

[Heiner Kallweit]

ds1307_probe is very long and confusing, so let's factor out
functionalities. As a first step factor out determining the chip type.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/rtc/rtc-ds1307.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 4f00cea2..2b5c6d60 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -1329,6 +1329,21 @@ static void ds1307_clks_register(struct ds1307 *ds1307)
 
 #endif /* CONFIG_COMMON_CLK */
 
+static enum ds_type ds1307_get_type(struct device *dev,
+				    const struct i2c_device_id *id)
+{
+	const struct acpi_device_id *acpi_id;
+
+	if (dev->of_node)
+		return (enum ds_type) of_device_get_match_data(dev);
+
+	if (id)
+		return id->driver_data;
+
+	acpi_id = acpi_match_device(ACPI_PTR(ds1307_acpi_ids), dev);
+	return acpi_id ? acpi_id->driver_data : -ENODEV;
+}
+
 static const struct regmap_config regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -1365,23 +1380,11 @@ static int ds1307_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, ds1307);
 
-	if (client->dev.of_node) {
-		ds1307->type = (enum ds_type)
-			of_device_get_match_data(&client->dev);
-		chip = &chips[ds1307->type];
-	} else if (id) {
-		chip = &chips[id->driver_data];
-		ds1307->type = id->driver_data;
-	} else {
-		const struct acpi_device_id *acpi_id;
-
-		acpi_id = acpi_match_device(ACPI_PTR(ds1307_acpi_ids),
-					    ds1307->dev);
-		if (!acpi_id)
-			return -ENODEV;
-		chip = &chips[acpi_id->driver_data];
-		ds1307->type = acpi_id->driver_data;
-	}
+	ds1307->type = ds1307_get_type(&client->dev, id);
+	if (ds1307->type < 0)
+		return ds1307->type;
+
+	chip = &chips[ds1307->type];
 
 	want_irq = client->irq > 0 && chip->alarm;
 
-- 
2.14.1

[PATCH 2/5] rtc: ds1307: factor out trickle charger initialization

[Heiner Kallweit]

Rename the existing function ds1307_trickle_init to ds1307_trickle_setup
and factor out trickle charger initialization from ds1307_probe into a
new function ds1307_trickle_init.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/rtc/rtc-ds1307.c | 48 ++++++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 2b5c6d60..ea88e4b3 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -970,8 +970,8 @@ static u8 do_trickle_setup_ds1339(struct ds1307 *ds1307,
 	return setup;
 }
 
-static u8 ds1307_trickle_init(struct ds1307 *ds1307,
-			      const struct chip_desc *chip)
+static u8 ds1307_trickle_setup(struct ds1307 *ds1307,
+			       const struct chip_desc *chip)
 {
 	uint32_t ohms;
 	bool diode = true;
@@ -1344,6 +1344,30 @@ static enum ds_type ds1307_get_type(struct device *dev,
 	return acpi_id ? acpi_id->driver_data : -ENODEV;
 }
 
+static void ds1307_trickle_init(struct ds1307 *ds1307)
+{
+	struct ds1307_platform_data *pdata = dev_get_platdata(ds1307->dev);
+	const struct chip_desc *chip = &chips[ds1307->type];
+	u8 trickle_charger_setup;
+
+	if (!chip->trickle_charger_reg)
+		return;
+
+	if (pdata)
+		trickle_charger_setup = pdata->trickle_charger_setup;
+	else
+		trickle_charger_setup = ds1307_trickle_setup(ds1307, chip);
+
+	if (trickle_charger_setup) {
+		trickle_charger_setup |= DS13XX_TRICKLE_CHARGER_MAGIC;
+		dev_dbg(ds1307->dev,
+			"writing trickle charger info 0x%x to 0x%x\n",
+			trickle_charger_setup, chip->trickle_charger_reg);
+		regmap_write(ds1307->regmap, chip->trickle_charger_reg,
+			     trickle_charger_setup);
+	}
+}
+
 static const struct regmap_config regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -1359,10 +1383,8 @@ static int ds1307_probe(struct i2c_client *client,
 	bool			want_irq;
 	bool			ds1307_can_wakeup_device = false;
 	unsigned char		*buf;
-	struct ds1307_platform_data *pdata = dev_get_platdata(&client->dev);
 	struct rtc_time		tm;
 	unsigned long		timestamp;
-	u8			trickle_charger_setup = 0;
 
 	ds1307 = devm_kzalloc(&client->dev, sizeof(struct ds1307), GFP_KERNEL);
 	if (!ds1307)
@@ -1385,25 +1407,11 @@ static int ds1307_probe(struct i2c_client *client,
 		return ds1307->type;
 
 	chip = &chips[ds1307->type];
-
 	want_irq = client->irq > 0 && chip->alarm;
-
-	if (!pdata)
-		trickle_charger_setup = ds1307_trickle_init(ds1307, chip);
-	else if (pdata->trickle_charger_setup)
-		trickle_charger_setup = pdata->trickle_charger_setup;
-
-	if (trickle_charger_setup && chip->trickle_charger_reg) {
-		trickle_charger_setup |= DS13XX_TRICKLE_CHARGER_MAGIC;
-		dev_dbg(ds1307->dev,
-			"writing trickle charger info 0x%x to 0x%x\n",
-			trickle_charger_setup, chip->trickle_charger_reg);
-		regmap_write(ds1307->regmap, chip->trickle_charger_reg,
-			     trickle_charger_setup);
-	}
-
 	buf = ds1307->regs;
 
+	ds1307_trickle_init(ds1307);
+
 #ifdef CONFIG_OF
 /*
  * For devices with no IRQ directly connected to the SoC, the RTC chip
-- 
2.14.1

[PATCH 3/5] rtc: ds1307: factor out fixing the weekday

[Heiner Kallweit]

Factor out checking and fixing the weekday.

In addition fix two issues with the old implementation:
- For variable timestamp use correct type time64_t instead of
  unsigned long which may be just 32bit long.
- Updating the weekday register only may be racy, therefore write all
  timekeeping registers.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/rtc/rtc-ds1307.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index ea88e4b3..69f514b6 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -1368,6 +1368,29 @@ static void ds1307_trickle_init(struct ds1307 *ds1307)
 	}
 }
 
+static void ds1307_fix_weekday(struct ds1307 *ds1307)
+{
+	struct rtc_time tm;
+	time64_t timestamp;
+	int wday;
+
+	if (ds1307_get_time(ds1307->dev, &tm))
+		return;
+
+	wday = tm.tm_wday;
+	timestamp = rtc_tm_to_time64(&tm);
+	rtc_time64_to_tm(timestamp, &tm);
+
+	/*
+	 * Check if reset wday is different from the computed wday
+	 * If different then set the wday which we computed using
+	 * timestamp
+	 * Set not only wday but complete date to avoid potential races.
+	 */
+	if (wday != tm.tm_wday)
+		ds1307_set_time(ds1307->dev, &tm);
+}
+
 static const struct regmap_config regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -1378,13 +1401,11 @@ static int ds1307_probe(struct i2c_client *client,
 {
 	struct ds1307		*ds1307;
 	int			err = -ENODEV;
-	int			tmp, wday;
+	int			tmp;
 	const struct chip_desc	*chip;
 	bool			want_irq;
 	bool			ds1307_can_wakeup_device = false;
 	unsigned char		*buf;
-	struct rtc_time		tm;
-	unsigned long		timestamp;
 
 	ds1307 = devm_kzalloc(&client->dev, sizeof(struct ds1307), GFP_KERNEL);
 	if (!ds1307)
@@ -1642,20 +1663,7 @@ static int ds1307_probe(struct i2c_client *client,
 	 * Some IPs have weekday reset value = 0x1 which might not correct
 	 * hence compute the wday using the current date/month/year values
 	 */
-	ds1307_get_time(ds1307->dev, &tm);
-	wday = tm.tm_wday;
-	timestamp = rtc_tm_to_time64(&tm);
-	rtc_time64_to_tm(timestamp, &tm);
-
-	/*
-	 * Check if reset wday is different from the computed wday
-	 * If different then set the wday which we computed using
-	 * timestamp
-	 */
-	if (wday != tm.tm_wday)
-		regmap_update_bits(ds1307->regmap, MCP794XX_REG_WEEKDAY,
-				   MCP794XX_REG_WEEKDAY_WDAY_MASK,
-				   tm.tm_wday + 1);
+	ds1307_fix_weekday(ds1307);
 
 	if (want_irq || ds1307_can_wakeup_device) {
 		device_set_wakeup_capable(ds1307->dev, true);
-- 
2.14.1

[PATCH 4/5] rtc: ds1307: introduce constants for the timekeeping register masks

[Heiner Kallweit]

So far for masking the relevant bits of the timekeeping registers
magic numbers are used. Replace them with proper constants.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/rtc/rtc-ds1307.c | 63 ++++++++++++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 29 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 69f514b6..a43a80b2 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -54,22 +54,29 @@ enum ds_type {
 
 /* RTC registers don't differ much, except for the century flag */
 #define DS1307_REG_SECS		0x00	/* 00-59 */
+#define DS1307_SECS_MASK	GENMASK(6, 0)
 #	define DS1307_BIT_CH		0x80
 #	define DS1340_BIT_nEOSC		0x80
 #	define MCP794XX_BIT_ST		0x80
 #define DS1307_REG_MIN		0x01	/* 00-59 */
+#define DS1307_MIN_MASK		GENMASK(6, 0)
 #	define M41T0_BIT_OF		0x80
 #define DS1307_REG_HOUR		0x02	/* 00-23, or 1-12{am,pm} */
+#define DS1307_HOUR_MASK	GENMASK(5, 0)
 #	define DS1307_BIT_12HR		0x40	/* in REG_HOUR */
 #	define DS1307_BIT_PM		0x20	/* in REG_HOUR */
 #	define DS1340_BIT_CENTURY_EN	0x80	/* in REG_HOUR */
 #	define DS1340_BIT_CENTURY	0x40	/* in REG_HOUR */
 #define DS1307_REG_WDAY		0x03	/* 01-07 */
+#define DS1307_WDAY_MASK	GENMASK(2, 0)
 #	define MCP794XX_BIT_VBATEN	0x08
 #define DS1307_REG_MDAY		0x04	/* 01-31 */
+#define DS1307_MDAY_MASK	GENMASK(5, 0)
 #define DS1307_REG_MONTH	0x05	/* 01-12 */
+#define DS1307_MONTH_MASK	GENMASK(4, 0)
 #	define DS1337_BIT_CENTURY	0x80	/* in REG_MONTH */
 #define DS1307_REG_YEAR		0x06	/* 00-99 */
+#define DS1307_YEAR_MASK	GENMASK(7, 0)
 
 /*
  * Other registers (control, status, alarms, trickle charge, NVRAM, etc)
@@ -395,36 +402,34 @@ static irqreturn_t ds1307_irq(int irq, void *dev_id)
 static int ds1307_get_time(struct device *dev, struct rtc_time *t)
 {
 	struct ds1307	*ds1307 = dev_get_drvdata(dev);
-	int		tmp, ret;
 	const struct chip_desc *chip = &chips[ds1307->type];
+	u8		*buf = ds1307->regs;
+	int		ret;
 
 	/* read the RTC date and time registers all at once */
-	ret = regmap_bulk_read(ds1307->regmap, chip->offset, ds1307->regs, 7);
+	ret = regmap_bulk_read(ds1307->regmap, chip->offset, buf, 7);
 	if (ret) {
 		dev_err(dev, "%s error %d\n", "read", ret);
 		return ret;
 	}
 
-	dev_dbg(dev, "%s: %7ph\n", "read", ds1307->regs);
+	dev_dbg(dev, "%s: %7ph\n", "read", buf);
 
 	/* if oscillator fail bit is set, no data can be trusted */
-	if (ds1307->type == m41t0 &&
-	    ds1307->regs[DS1307_REG_MIN] & M41T0_BIT_OF) {
+	if (ds1307->type == m41t0 && buf[DS1307_REG_MIN] & M41T0_BIT_OF) {
 		dev_warn_once(dev, "oscillator failed, set time!\n");
 		return -EINVAL;
 	}
 
-	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_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_mon = bcd2bin(tmp) - 1;
-	t->tm_year = bcd2bin(ds1307->regs[DS1307_REG_YEAR]) + 100;
-
-	if (ds1307->regs[chip->century_reg] & chip->century_bit &&
+	t->tm_sec = bcd2bin(buf[DS1307_REG_SECS] & DS1307_SECS_MASK);
+	t->tm_min = bcd2bin(buf[DS1307_REG_MIN] & DS1307_MIN_MASK);
+	t->tm_hour = bcd2bin(buf[DS1307_REG_HOUR] & DS1307_HOUR_MASK);
+	t->tm_wday = bcd2bin(buf[DS1307_REG_WDAY] & DS1307_WDAY_MASK) - 1;
+	t->tm_mday = bcd2bin(buf[DS1307_REG_MDAY] & DS1307_MDAY_MASK);
+	t->tm_mon = bcd2bin(buf[DS1307_REG_MONTH] & DS1307_MONTH_MASK) - 1;
+	t->tm_year = bcd2bin(buf[DS1307_REG_YEAR] & DS1307_YEAR_MASK) + 100;
+
+	if (buf[chip->century_reg] & chip->century_bit &&
 	    IS_ENABLED(CONFIG_RTC_DRV_DS1307_CENTURY))
 		t->tm_year += 100;
 
@@ -522,10 +527,10 @@ static int ds1337_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 	 * 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(ds1307->regs[0] & DS1307_SECS_MASK);
+	t->time.tm_min = bcd2bin(ds1307->regs[1] & DS1307_MIN_MASK);
+	t->time.tm_hour = bcd2bin(ds1307->regs[2] & DS1307_HOUR_MASK);
+	t->time.tm_mday = bcd2bin(ds1307->regs[3] & DS1307_MDAY_MASK);
 
 	/* ... and status */
 	t->enabled = !!(ds1307->regs[7] & DS1337_BIT_A1IE);
@@ -689,10 +694,10 @@ static int rx8130_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 
 	/* Report alarm 0 time assuming 24-hour and day-of-month modes. */
 	t->time.tm_sec = -1;
-	t->time.tm_min = bcd2bin(ald[0] & 0x7f);
-	t->time.tm_hour = bcd2bin(ald[1] & 0x7f);
+	t->time.tm_min = bcd2bin(ald[0] & DS1307_MIN_MASK);
+	t->time.tm_hour = bcd2bin(ald[1] & DS1307_HOUR_MASK);
 	t->time.tm_wday = -1;
-	t->time.tm_mday = bcd2bin(ald[2] & 0x7f);
+	t->time.tm_mday = bcd2bin(ald[2] & DS1307_MDAY_MASK);
 	t->time.tm_mon = -1;
 	t->time.tm_year = -1;
 	t->time.tm_yday = -1;
@@ -844,12 +849,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(ds1307->regs[3] & DS1307_SECS_MASK);
+	t->time.tm_min = bcd2bin(ds1307->regs[4] & DS1307_MIN_MASK);
+	t->time.tm_hour = bcd2bin(ds1307->regs[5] & DS1307_HOUR_MASK);
+	t->time.tm_wday = bcd2bin(ds1307->regs[6] & DS1307_WDAY_MASK) - 1;
+	t->time.tm_mday = bcd2bin(ds1307->regs[7] & DS1307_MDAY_MASK);
+	t->time.tm_mon = bcd2bin(ds1307->regs[8] & DS1307_MONTH_MASK) - 1;
 	t->time.tm_year = -1;
 	t->time.tm_yday = -1;
 	t->time.tm_isdst = -1;
-- 
2.14.1

[PATCH 5/5] rtc: ds1307: improve ds1307_set_time to respect config flag bits

[Heiner Kallweit]

Some chips use the unused bits in the timekeeping registers for config
bits. Therefore, when setting the time, we should read the timekeeping
registers and leave the unused bits intact when setting the date / time
values.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/rtc/rtc-ds1307.c | 68 ++++++++++++++++++++++++------------------------
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index a43a80b2..1255b165 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -447,9 +447,8 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
 {
 	struct ds1307	*ds1307 = dev_get_drvdata(dev);
 	const struct chip_desc *chip = &chips[ds1307->type];
-	int		result;
-	int		tmp;
 	u8		*buf = ds1307->regs;
+	int		ret;
 
 	dev_dbg(dev, "%s secs=%d, mins=%d, "
 		"hours=%d, mday=%d, mon=%d, year=%d, wday=%d\n",
@@ -457,51 +456,52 @@ 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);
 
-	if (t->tm_year < 100)
+	if (t->tm_year < 100 || t->tm_year > 299)
 		return -EINVAL;
 
-#ifdef CONFIG_RTC_DRV_DS1307_CENTURY
-	if (t->tm_year > (chip->century_bit ? 299 : 199))
-		return -EINVAL;
-#else
-	if (t->tm_year > 199)
-		return -EINVAL;
-#endif
+	if (!IS_ENABLED(CONFIG_RTC_DRV_DS1307_CENTURY) || !chip->century_bit)
+		if (t->tm_year > 199)
+			return -EINVAL;
 
-	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);
+	ret = regmap_bulk_read(ds1307->regmap, chip->offset, buf, 7);
+	if (ret) {
+		dev_err(dev, "%s error %d\n", "read", ret);
+		return ret;
+	}
 
+	/*
+	 * Several chips use upper bits of these registers for
+	 * config bits, so leave them intact.
+	 */
+	buf[DS1307_REG_SECS] &= ~DS1307_SECS_MASK;
+	buf[DS1307_REG_SECS] |= bin2bcd(t->tm_sec);
+	buf[DS1307_REG_MIN] &= ~DS1307_MIN_MASK;
+	buf[DS1307_REG_MIN] |= bin2bcd(t->tm_min);
+	buf[DS1307_REG_HOUR] &= ~DS1307_HOUR_MASK;
+	buf[DS1307_REG_HOUR] |= bin2bcd(t->tm_hour);
+	buf[DS1307_REG_WDAY] &= ~DS1307_WDAY_MASK;
+	buf[DS1307_REG_WDAY] |= bin2bcd(t->tm_wday + 1);
+	buf[DS1307_REG_MDAY] &= ~DS1307_MDAY_MASK;
+	buf[DS1307_REG_MDAY] |= bin2bcd(t->tm_mday);
+	buf[DS1307_REG_MONTH] &= ~DS1307_MONTH_MASK;
+	buf[DS1307_REG_MONTH] |= bin2bcd(t->tm_mon + 1);
+
+	buf[DS1307_REG_YEAR] &= ~DS1307_YEAR_MASK;
 	/* assume 20YY not 19YY */
-	tmp = t->tm_year - 100;
-	buf[DS1307_REG_YEAR] = bin2bcd(tmp);
+	buf[DS1307_REG_YEAR] |= bin2bcd(t->tm_year - 100);
 
 	if (chip->century_enable_bit)
 		buf[chip->century_reg] |= chip->century_enable_bit;
 	if (t->tm_year > 199 && chip->century_bit)
 		buf[chip->century_reg] |= chip->century_bit;
 
-	if (ds1307->type == mcp794xx) {
-		/*
-		 * these bits were cleared when preparing the date/time
-		 * 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;
-	}
-
 	dev_dbg(dev, "%s: %7ph\n", "write", buf);
 
-	result = regmap_bulk_write(ds1307->regmap, chip->offset, buf, 7);
-	if (result) {
-		dev_err(dev, "%s error %d\n", "write", result);
-		return result;
-	}
-	return 0;
+	ret = regmap_bulk_write(ds1307->regmap, chip->offset, buf, 7);
+	if (ret)
+		dev_err(dev, "%s error %d\n", "write", ret);
+
+	return ret;
 }
 
 static int ds1337_read_alarm(struct device *dev, struct rtc_wkalrm *t)
-- 
2.14.1

Re: [PATCH 0/5] rtc: ds1307: factor out more stuff from ds1307_probe and improve ds1307_set_time

[Alexandre Belloni]

Hi,

Could you explain were you are going with this because this is a lot of
churn that takes a lot of my reviewing time and this doesn't seem to end
soon.

I want that driver to stop growing, maybe by creating a library. Some of
the supported RTCs doesn't share much more than the time and date layout
and should be in a separate driver.

On 25/08/2017 at 21:30:09 +0200, Heiner Kallweit wrote:
> Final goal of the refactoring is to abstract everything that the chips
> have in common and handle it in generic code. Then we can get rid of
> all the "switch (chip)" and "if (chip == xxx)" code.
> 
> To give one example:
> A lot of chips have a bit for setting 12hr / 24hr mode. However some
> chips have this config bit in the timekeeping registers, others in
> a config register, and on some chips it's inverted.
> But the functionality of the bit is always the same.
> 
> Ultimately adding support for a chip just requires to add one config
> structure member.
> 
> The way to reach this goal is a long one and to faciliate reviewing
> the patches I'll split them into series of 5 to 10 patches.
> 
> Heiner Kallweit (5):
>   rtc: ds1307: factor out determining the chip type
>   rtc: ds1307: factor out trickle charger initialization
>   rtc: ds1307: factor out fixing the weekday
>   rtc: ds1307: introduce constants for the timekeeping register masks
>   rtc: ds1307: improve ds1307_set_time to respect config flag bits
> 
>  drivers/rtc/rtc-ds1307.c | 256 ++++++++++++++++++++++++++---------------------
>  1 file changed, 140 insertions(+), 116 deletions(-)
> 
> -- 
> 2.14.1
> 

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

Re: [PATCH 0/5] rtc: ds1307: factor out more stuff from ds1307_probe and improve ds1307_set_time

[Heiner Kallweit]

Am 26.08.2017 um 10:19 schrieb Alexandre Belloni:
> Hi,
> 
> Could you explain were you are going with this because this is a lot of
> churn that takes a lot of my reviewing time and this doesn't seem to end
> soon.
> 
> I want that driver to stop growing, maybe by creating a library. Some of
> the supported RTCs doesn't share much more than the time and date layout
> and should be in a separate driver.
> 
That's exactly my point. The driver is too big already.

The patches so far increase size of the driver a little, only subsequent
patches start to reduce it.

More things like even exporting clocks work the same on all chips
supporting this feature. Just the layout of the alarm registers usually is
quite different. Therefore it's my plan to create such a ds1307_lib with all
the generic code.

If it helps I can provide the full patch set (as far as I came so far) via
Github, then you can check whether it's the right direction also from your
point of view (w/o having to review each single patch in detail already).

By the way: This current patch set with the 5 patches I have to change,
so there will be a v2. No need for you to spend reviewing effort on it now.

Regards, Heiner


> On 25/08/2017 at 21:30:09 +0200, Heiner Kallweit wrote:
>> Final goal of the refactoring is to abstract everything that the chips
>> have in common and handle it in generic code. Then we can get rid of
>> all the "switch (chip)" and "if (chip == xxx)" code.
>>
>> To give one example:
>> A lot of chips have a bit for setting 12hr / 24hr mode. However some
>> chips have this config bit in the timekeeping registers, others in
>> a config register, and on some chips it's inverted.
>> But the functionality of the bit is always the same.
>>
>> Ultimately adding support for a chip just requires to add one config
>> structure member.
>>
>> The way to reach this goal is a long one and to faciliate reviewing
>> the patches I'll split them into series of 5 to 10 patches.
>>
>> Heiner Kallweit (5):
>>   rtc: ds1307: factor out determining the chip type
>>   rtc: ds1307: factor out trickle charger initialization
>>   rtc: ds1307: factor out fixing the weekday
>>   rtc: ds1307: introduce constants for the timekeeping register masks
>>   rtc: ds1307: improve ds1307_set_time to respect config flag bits
>>
>>  drivers/rtc/rtc-ds1307.c | 256 ++++++++++++++++++++++++++---------------------
>>  1 file changed, 140 insertions(+), 116 deletions(-)
>>
>> -- 
>> 2.14.1
>>
> 

Re: [PATCH 0/5] rtc: ds1307: factor out more stuff from ds1307_probe and improve ds1307_set_time

[Alexandre Belloni]

On 26/08/2017 at 12:16:53 +0200, Heiner Kallweit wrote:
> That's exactly my point. The driver is too big already.
> 
> The patches so far increase size of the driver a little, only subsequent
> patches start to reduce it.
> 
> More things like even exporting clocks work the same on all chips
> supporting this feature. Just the layout of the alarm registers usually is
> quite different. Therefore it's my plan to create such a ds1307_lib with all
> the generic code.
> 
> If it helps I can provide the full patch set (as far as I came so far) via
> Github, then you can check whether it's the right direction also from your
> point of view (w/o having to review each single patch in detail already).
> 

Yes, please do that.

> By the way: This current patch set with the 5 patches I have to change,
> so there will be a v2. No need for you to spend reviewing effort on it now.
> 

Ok, thanks.

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

Re: [PATCH 0/5] rtc: ds1307: factor out more stuff from ds1307_probe and improve ds1307_set_time

[Heiner Kallweit]

Am 26.08.2017 um 12:44 schrieb Alexandre Belloni:
> On 26/08/2017 at 12:16:53 +0200, Heiner Kallweit wrote:
>> That's exactly my point. The driver is too big already.
>>
>> The patches so far increase size of the driver a little, only subsequent
>> patches start to reduce it.
>>
>> More things like even exporting clocks work the same on all chips
>> supporting this feature. Just the layout of the alarm registers usually is
>> quite different. Therefore it's my plan to create such a ds1307_lib with all
>> the generic code.
>>
>> If it helps I can provide the full patch set (as far as I came so far) via
>> Github, then you can check whether it's the right direction also from your
>> point of view (w/o having to review each single patch in detail already).
>>
> 
> Yes, please do that.
> 
I rebased the patch set on top of your latest ds1307 changes and published
it here: https://github.com/hkallweit/linux_rtc_ds1307/tree/rtc-ds1307

Most code went into a new rtc-ds1307-lib library in a generic form, the driver
contains mainly chip configuration info now plus chip-specific routines for
reading / setting alarm.
Now it would require very little effort to e.g. make ds3231 a separate driver
because the hwmon code is needed for this chip only.

Regards,
Heiner

>> By the way: This current patch set with the 5 patches I have to change,
>> so there will be a v2. No need for you to spend reviewing effort on it now.
>>
> 
> Ok, thanks.
> 

Re: [PATCH 0/5] rtc: ds1307: factor out more stuff from ds1307_probe and improve ds1307_set_time

[Alexandre Belloni]

Hi,

On 05/09/2017 at 07:27:08 +0200, Heiner Kallweit wrote:
> Most code went into a new rtc-ds1307-lib library in a generic form, the driver
> contains mainly chip configuration info now plus chip-specific routines for
> reading / setting alarm.
> Now it would require very little effort to e.g. make ds3231 a separate driver
> because the hwmon code is needed for this chip only.

So I had a closer look (very late, sorry...)

I think too much went in the lib and it is not generic enough.

What I'd like are functions that could be used from every driver
handling RTC in [centisec] sec min hour day month year format, not just
ds1307. The ds1307 driver has been abused because it handles that
format. Maybe, we could call that rtc_bcd_* or something similar.

For example, there is nothing ds1307 specific in ds1307_regs_to_tm apart
from the century handling. The same for the function reading or writing
the registers. A function taking a regmap and an offset will be generic
enough for most drivers.

So, the first step would be to make functions to handle that generically
and call them from the driver. It would be good to also have the
conversion of other drivers as a PoC but I can take care of that.

Then, regarding the individual bits, they would be better handled using
regmap_fields instead of rolling our own implementation. Those will have
to be ds1307 specific. I wouldn't bother with their polarity as if it is
different, it probably means a different driver is needed instead.

Have read_time/set_time functions handling the osc bit the century
bit(s) will make them reusable for around 40 drivers. (BTW, it seems
ds1307_set_time() never resets the osc bit).
I'm not sure about the alarm ops yet but they can probably be made
generic enough.

Also, the buffer update optimization instead of updating bits in the
registers makes the code quite complicated. I'm not sure it is worth it.
Also, it doesn't work well with the regmap_field idea.

rtc_ops, irq handling, nvram handling, clock handling, trickle charger
handling and probe should probably stay in the driver for now and will
need to be separated in their own drivers later.

So, let's start small and introduce functions that will work for most
current drivers, handling bcd time. regmap can be used to abstract
register accesses. A struct to pass register offsets and other data
(like presence of wday or number of century bits) will probably be
needed.

I hope this is clear enough.

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