[PATCH 0/9] rtc: ds1307: series with refactorings / improvements

[PATCH 0/9] rtc: ds1307: series with refactorings / improvements

[Heiner Kallweit]

This series includes smaller refactorings and improvements.

Heiner Kallweit (9):
  rtc: ds1307: remove member irq from struct ds1307
  rtc: ds1307: factor out bbsqi bit to struct chip_desc
  rtc: ds1307: improve trickle charger initialization
  rtc: ds1307: constify struct chip_desc variables
  rtc: ds1307: improve irq setup
  rtc: ds1307: factor out irq_handler to struct chip_desc
  rtc: ds1307: factor out rtc_ops to struct chip_desc
  rtc: ds1307: factor out offset to struct chip_desc
  rtc: ds1307: remove member nvram_offset from struct ds1307

 drivers/rtc/rtc-ds1307.c | 165 ++++++++++++++++++++++-------------------------
 1 file changed, 76 insertions(+), 89 deletions(-)

-- 
2.13.2

[PATCH 1/9] rtc: ds1307: remove member irq from struct ds1307

[Heiner Kallweit]

The irq number is used in the probe function only, so we don't have
to store it in struct ds1307.

Signed-off-by: Heiner Kallweit <hkallweit1@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 1cedb21b..adc90f18 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -126,7 +126,6 @@ struct ds1307 {
 	struct device		*dev;
 	struct regmap		*regmap;
 	const char		*name;
-	int			irq;
 	struct rtc_device	*rtc;
 #ifdef CONFIG_COMMON_CLK
 	struct clk_hw		clks[2];
@@ -1334,7 +1333,6 @@ static int ds1307_probe(struct i2c_client *client,
 	dev_set_drvdata(&client->dev, ds1307);
 	ds1307->dev = &client->dev;
 	ds1307->name = client->name;
-	ds1307->irq = client->irq;
 
 	ds1307->regmap = devm_regmap_init_i2c(client, &regmap_config);
 	if (IS_ERR(ds1307->regmap)) {
@@ -1414,7 +1412,7 @@ static int ds1307_probe(struct i2c_client *client,
 		 * For some variants, be sure alarms can trigger when we're
 		 * running on Vbackup (BBSQI/BBSQW)
 		 */
-		if (chip->alarm && (ds1307->irq > 0 ||
+		if (chip->alarm && (client->irq > 0 ||
 				    ds1307_can_wakeup_device)) {
 			ds1307->regs[0] |= DS1337_BIT_INTCN
 					| bbsqi_bitpos[ds1307->type];
@@ -1499,7 +1497,7 @@ static int ds1307_probe(struct i2c_client *client,
 	case rx_8130:
 		ds1307->offset = 0x10; /* Seconds starts at 0x10 */
 		rtc_ops = &rx8130_rtc_ops;
-		if (chip->alarm && ds1307->irq > 0) {
+		if (chip->alarm && client->irq > 0) {
 			irq_handler = rx8130_irq;
 			want_irq = true;
 		}
@@ -1509,7 +1507,7 @@ static int ds1307_probe(struct i2c_client *client,
 		break;
 	case mcp794xx:
 		rtc_ops = &mcp794xx_rtc_ops;
-		if (chip->alarm && (ds1307->irq > 0 ||
+		if (chip->alarm && (client->irq > 0 ||
 				    ds1307_can_wakeup_device)) {
 			irq_handler = mcp794xx_irq;
 			want_irq = true;
@@ -1655,7 +1653,7 @@ static int ds1307_probe(struct i2c_client *client,
 		return PTR_ERR(ds1307->rtc);
 	}
 
-	if (ds1307_can_wakeup_device && ds1307->irq <= 0) {
+	if (ds1307_can_wakeup_device && client->irq <= 0) {
 		/* Disable request for an IRQ */
 		want_irq = false;
 		dev_info(ds1307->dev,
@@ -1666,7 +1664,7 @@ static int ds1307_probe(struct i2c_client *client,
 
 	if (want_irq) {
 		err = devm_request_threaded_irq(ds1307->dev,
-						ds1307->irq, NULL, irq_handler,
+						client->irq, NULL, irq_handler,
 						IRQF_SHARED | IRQF_ONESHOT,
 						ds1307->name, ds1307);
 		if (err) {
-- 
2.13.2

[PATCH 2/9] rtc: ds1307: factor out bbsqi bit to struct chip_desc

[Heiner Kallweit]

Factor out the bbsqi bit to struct chip_desc.

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

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index adc90f18..eecf6b27 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -139,6 +139,7 @@ struct chip_desc {
 	u8			century_reg;
 	u8			century_enable_bit;
 	u8			century_bit;
+	u8			bbsqi_bit;
 	u16			trickle_charger_reg;
 	u8			trickle_charger_setup;
 	u8			(*do_trickle_setup)(struct ds1307 *, uint32_t,
@@ -169,6 +170,7 @@ static struct chip_desc chips[last_ds_type] = {
 		.alarm		= 1,
 		.century_reg	= DS1307_REG_MONTH,
 		.century_bit	= DS1337_BIT_CENTURY,
+		.bbsqi_bit	= DS1339_BIT_BBSQI,
 		.trickle_charger_reg = 0x10,
 		.do_trickle_setup = &do_trickle_setup_ds1339,
 	},
@@ -185,6 +187,7 @@ static struct chip_desc chips[last_ds_type] = {
 		.alarm		= 1,
 		.century_reg	= DS1307_REG_MONTH,
 		.century_bit	= DS1337_BIT_CENTURY,
+		.bbsqi_bit	= DS3231_BIT_BBSQW,
 	},
 	[rx_8130] = {
 		.alarm		= 1,
@@ -1319,11 +1322,6 @@ static int ds1307_probe(struct i2c_client *client,
 
 	irq_handler_t	irq_handler = ds1307_irq;
 
-	static const int	bbsqi_bitpos[] = {
-		[ds_1337] = 0,
-		[ds_1339] = DS1339_BIT_BBSQI,
-		[ds_3231] = DS3231_BIT_BBSQW,
-	};
 	const struct rtc_class_ops *rtc_ops = &ds13xx_rtc_ops;
 
 	ds1307 = devm_kzalloc(&client->dev, sizeof(struct ds1307), GFP_KERNEL);
@@ -1414,8 +1412,7 @@ static int ds1307_probe(struct i2c_client *client,
 		 */
 		if (chip->alarm && (client->irq > 0 ||
 				    ds1307_can_wakeup_device)) {
-			ds1307->regs[0] |= DS1337_BIT_INTCN
-					| bbsqi_bitpos[ds1307->type];
+			ds1307->regs[0] |= DS1337_BIT_INTCN | chip->bbsqi_bit;
 			ds1307->regs[0] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);
 
 			want_irq = true;
-- 
2.13.2

[PATCH 3/9] rtc: ds1307: improve trickle charger initialization

[Heiner Kallweit]

Instead of storing the trickle_charger_setup value in struct chip_desc
we can let function ds1307_trickle_init return it because it's used
in the probe function only.
This allows us to constify struct chip_desc variables in a next step.

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

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index eecf6b27..d7158c9d 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -141,7 +141,6 @@ struct chip_desc {
 	u8			century_bit;
 	u8			bbsqi_bit;
 	u16			trickle_charger_reg;
-	u8			trickle_charger_setup;
 	u8			(*do_trickle_setup)(struct ds1307 *, uint32_t,
 						    bool);
 };
@@ -941,23 +940,23 @@ static u8 do_trickle_setup_ds1339(struct ds1307 *ds1307,
 	return setup;
 }
 
-static void ds1307_trickle_init(struct ds1307 *ds1307,
-				struct chip_desc *chip)
+static u8 ds1307_trickle_init(struct ds1307 *ds1307,
+			      struct chip_desc *chip)
 {
-	uint32_t ohms = 0;
+	uint32_t ohms;
 	bool diode = true;
 
 	if (!chip->do_trickle_setup)
-		goto out;
+		return 0;
+
 	if (device_property_read_u32(ds1307->dev, "trickle-resistor-ohms",
 				     &ohms))
-		goto out;
+		return 0;
+
 	if (device_property_read_bool(ds1307->dev, "trickle-diode-disable"))
 		diode = false;
-	chip->trickle_charger_setup = chip->do_trickle_setup(ds1307,
-							     ohms, diode);
-out:
-	return;
+
+	return chip->do_trickle_setup(ds1307, ohms, diode);
 }
 
 /*----------------------------------------------------------------------*/
@@ -1319,6 +1318,7 @@ static int ds1307_probe(struct i2c_client *client,
 	struct ds1307_platform_data *pdata = dev_get_platdata(&client->dev);
 	struct rtc_time		tm;
 	unsigned long		timestamp;
+	u8 			trickle_charger_setup = 0;
 
 	irq_handler_t	irq_handler = ds1307_irq;
 
@@ -1359,18 +1359,17 @@ static int ds1307_probe(struct i2c_client *client,
 	}
 
 	if (!pdata)
-		ds1307_trickle_init(ds1307, chip);
+		trickle_charger_setup = ds1307_trickle_init(ds1307, chip);
 	else if (pdata->trickle_charger_setup)
-		chip->trickle_charger_setup = pdata->trickle_charger_setup;
+		trickle_charger_setup = pdata->trickle_charger_setup;
 
-	if (chip->trickle_charger_setup && chip->trickle_charger_reg) {
+	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",
-		    DS13XX_TRICKLE_CHARGER_MAGIC | chip->trickle_charger_setup,
-		    chip->trickle_charger_reg);
+			trickle_charger_setup, chip->trickle_charger_reg);
 		regmap_write(ds1307->regmap, chip->trickle_charger_reg,
-		    DS13XX_TRICKLE_CHARGER_MAGIC |
-		    chip->trickle_charger_setup);
+			     trickle_charger_setup);
 	}
 
 	buf = ds1307->regs;
-- 
2.13.2

[PATCH 4/9] rtc: ds1307: constify struct chip_desc variables

[Heiner Kallweit]

Constify struct chip_desc variables.

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

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index d7158c9d..115fe1c5 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -147,7 +147,7 @@ struct chip_desc {
 
 static u8 do_trickle_setup_ds1339(struct ds1307 *, uint32_t ohms, bool diode);
 
-static struct chip_desc chips[last_ds_type] = {
+static const struct chip_desc chips[last_ds_type] = {
 	[ds_1307] = {
 		.nvram_offset	= 8,
 		.nvram_size	= 56,
@@ -941,7 +941,7 @@ static u8 do_trickle_setup_ds1339(struct ds1307 *ds1307,
 }
 
 static u8 ds1307_trickle_init(struct ds1307 *ds1307,
-			      struct chip_desc *chip)
+			      const struct chip_desc *chip)
 {
 	uint32_t ohms;
 	bool diode = true;
@@ -1311,7 +1311,7 @@ static int ds1307_probe(struct i2c_client *client,
 	struct ds1307		*ds1307;
 	int			err = -ENODEV;
 	int			tmp, wday;
-	struct chip_desc	*chip;
+	const struct chip_desc	*chip;
 	bool			want_irq = false;
 	bool			ds1307_can_wakeup_device = false;
 	unsigned char		*buf;
-- 
2.13.2

[PATCH 5/9] rtc: ds1307: improve irq setup

[Heiner Kallweit]

Change the usage of variable want_irq to reflect its name. Don't set
it to true in case wakeup is enabled but no interrupt number is given.
In addition set variable ds1307_can_wakeup_device if chip->alarm
is set only.
This allows to simplify the code and make it better understandable.

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

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 115fe1c5..5aec1761 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -1312,7 +1312,7 @@ static int ds1307_probe(struct i2c_client *client,
 	int			err = -ENODEV;
 	int			tmp, wday;
 	const struct chip_desc	*chip;
-	bool			want_irq = false;
+	bool			want_irq;
 	bool			ds1307_can_wakeup_device = false;
 	unsigned char		*buf;
 	struct ds1307_platform_data *pdata = dev_get_platdata(&client->dev);
@@ -1358,6 +1358,8 @@ static int ds1307_probe(struct i2c_client *client,
 		ds1307->type = acpi_id->driver_data;
 	}
 
+	want_irq = client->irq > 0 && chip->alarm;
+
 	if (!pdata)
 		trickle_charger_setup = ds1307_trickle_init(ds1307, chip);
 	else if (pdata->trickle_charger_setup)
@@ -1383,7 +1385,8 @@ static int ds1307_probe(struct i2c_client *client,
  * 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"))
+	if (chip->alarm && of_property_read_bool(client->dev.of_node,
+	    "wakeup-source"))
 		ds1307_can_wakeup_device = true;
 #endif
 
@@ -1409,12 +1412,9 @@ static int ds1307_probe(struct i2c_client *client,
 		 * For some variants, be sure alarms can trigger when we're
 		 * running on Vbackup (BBSQI/BBSQW)
 		 */
-		if (chip->alarm && (client->irq > 0 ||
-				    ds1307_can_wakeup_device)) {
+		if (want_irq || ds1307_can_wakeup_device) {
 			ds1307->regs[0] |= DS1337_BIT_INTCN | chip->bbsqi_bit;
 			ds1307->regs[0] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);
-
-			want_irq = true;
 		}
 
 		regmap_write(ds1307->regmap, DS1337_REG_CONTROL,
@@ -1493,21 +1493,16 @@ static int ds1307_probe(struct i2c_client *client,
 	case rx_8130:
 		ds1307->offset = 0x10; /* Seconds starts at 0x10 */
 		rtc_ops = &rx8130_rtc_ops;
-		if (chip->alarm && client->irq > 0) {
+		if (want_irq)
 			irq_handler = rx8130_irq;
-			want_irq = true;
-		}
 		break;
 	case ds_1388:
 		ds1307->offset = 1; /* Seconds starts at 1 */
 		break;
 	case mcp794xx:
 		rtc_ops = &mcp794xx_rtc_ops;
-		if (chip->alarm && (client->irq > 0 ||
-				    ds1307_can_wakeup_device)) {
+		if (want_irq || ds1307_can_wakeup_device)
 			irq_handler = mcp794xx_irq;
-			want_irq = true;
-		}
 		break;
 	default:
 		break;
@@ -1639,7 +1634,7 @@ static int ds1307_probe(struct i2c_client *client,
 				   MCP794XX_REG_WEEKDAY_WDAY_MASK,
 				   tm.tm_wday + 1);
 
-	if (want_irq) {
+	if (want_irq || ds1307_can_wakeup_device) {
 		device_set_wakeup_capable(ds1307->dev, true);
 		set_bit(HAS_ALARM, &ds1307->flags);
 	}
@@ -1649,9 +1644,7 @@ static int ds1307_probe(struct i2c_client *client,
 		return PTR_ERR(ds1307->rtc);
 	}
 
-	if (ds1307_can_wakeup_device && client->irq <= 0) {
-		/* Disable request for an IRQ */
-		want_irq = false;
+	if (ds1307_can_wakeup_device && !want_irq) {
 		dev_info(ds1307->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.13.2

[PATCH 6/9] rtc: ds1307: factor out irq_handler to struct chip_desc

[Heiner Kallweit]

Factor out irq_handler to struct chip_desc and use ds1307_irq as default.

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

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 5aec1761..e64ea250 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -140,12 +140,15 @@ struct chip_desc {
 	u8			century_enable_bit;
 	u8			century_bit;
 	u8			bbsqi_bit;
+	irq_handler_t		irq_handler;
 	u16			trickle_charger_reg;
 	u8			(*do_trickle_setup)(struct ds1307 *, uint32_t,
 						    bool);
 };
 
 static u8 do_trickle_setup_ds1339(struct ds1307 *, uint32_t ohms, bool diode);
+static irqreturn_t rx8130_irq(int irq, void *dev_id);
+static irqreturn_t mcp794xx_irq(int irq, void *dev_id);
 
 static const struct chip_desc chips[last_ds_type] = {
 	[ds_1307] = {
@@ -193,12 +196,14 @@ static const struct chip_desc chips[last_ds_type] = {
 		/* this is battery backed SRAM */
 		.nvram_offset	= 0x20,
 		.nvram_size	= 4,	/* 32bit (4 word x 8 bit) */
+		.irq_handler = rx8130_irq,
 	},
 	[mcp794xx] = {
 		.alarm		= 1,
 		/* this is battery backed SRAM */
 		.nvram_offset	= 0x20,
 		.nvram_size	= 0x40,
+		.irq_handler = mcp794xx_irq,
 	},
 };
 
@@ -1320,8 +1325,6 @@ static int ds1307_probe(struct i2c_client *client,
 	unsigned long		timestamp;
 	u8 			trickle_charger_setup = 0;
 
-	irq_handler_t	irq_handler = ds1307_irq;
-
 	const struct rtc_class_ops *rtc_ops = &ds13xx_rtc_ops;
 
 	ds1307 = devm_kzalloc(&client->dev, sizeof(struct ds1307), GFP_KERNEL);
@@ -1493,16 +1496,12 @@ static int ds1307_probe(struct i2c_client *client,
 	case rx_8130:
 		ds1307->offset = 0x10; /* Seconds starts at 0x10 */
 		rtc_ops = &rx8130_rtc_ops;
-		if (want_irq)
-			irq_handler = rx8130_irq;
 		break;
 	case ds_1388:
 		ds1307->offset = 1; /* Seconds starts at 1 */
 		break;
 	case mcp794xx:
 		rtc_ops = &mcp794xx_rtc_ops;
-		if (want_irq || ds1307_can_wakeup_device)
-			irq_handler = mcp794xx_irq;
 		break;
 	default:
 		break;
@@ -1652,8 +1651,8 @@ static int ds1307_probe(struct i2c_client *client,
 	}
 
 	if (want_irq) {
-		err = devm_request_threaded_irq(ds1307->dev,
-						client->irq, NULL, irq_handler,
+		err = devm_request_threaded_irq(ds1307->dev, client->irq, NULL,
+						chip->irq_handler ?: ds1307_irq,
 						IRQF_SHARED | IRQF_ONESHOT,
 						ds1307->name, ds1307);
 		if (err) {
-- 
2.13.2

[PATCH 7/9] rtc: ds1307: factor out rtc_ops to struct chip_desc

[Heiner Kallweit]

Factor out rtc_ops to struct chip_desc and use ds13xx_rtc_ops as default.

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

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index e64ea250..4083464f 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -141,14 +141,39 @@ struct chip_desc {
 	u8			century_bit;
 	u8			bbsqi_bit;
 	irq_handler_t		irq_handler;
+	const struct rtc_class_ops *rtc_ops;
 	u16			trickle_charger_reg;
 	u8			(*do_trickle_setup)(struct ds1307 *, uint32_t,
 						    bool);
 };
 
+static int ds1307_get_time(struct device *dev, struct rtc_time *t);
+static int ds1307_set_time(struct device *dev, struct rtc_time *t);
 static u8 do_trickle_setup_ds1339(struct ds1307 *, uint32_t ohms, bool diode);
 static irqreturn_t rx8130_irq(int irq, void *dev_id);
+static int rx8130_read_alarm(struct device *dev, struct rtc_wkalrm *t);
+static int rx8130_set_alarm(struct device *dev, struct rtc_wkalrm *t);
+static int rx8130_alarm_irq_enable(struct device *dev, unsigned int enabled);
 static irqreturn_t mcp794xx_irq(int irq, void *dev_id);
+static int mcp794xx_read_alarm(struct device *dev, struct rtc_wkalrm *t);
+static int mcp794xx_set_alarm(struct device *dev, struct rtc_wkalrm *t);
+static int mcp794xx_alarm_irq_enable(struct device *dev, unsigned int enabled);
+
+static const struct rtc_class_ops rx8130_rtc_ops = {
+        .read_time      = ds1307_get_time,
+        .set_time       = ds1307_set_time,
+        .read_alarm     = rx8130_read_alarm,
+        .set_alarm      = rx8130_set_alarm,
+        .alarm_irq_enable = rx8130_alarm_irq_enable,
+};
+
+static const struct rtc_class_ops mcp794xx_rtc_ops = {
+        .read_time      = ds1307_get_time,
+        .set_time       = ds1307_set_time,
+        .read_alarm     = mcp794xx_read_alarm,
+        .set_alarm      = mcp794xx_set_alarm,
+        .alarm_irq_enable = mcp794xx_alarm_irq_enable,
+};
 
 static const struct chip_desc chips[last_ds_type] = {
 	[ds_1307] = {
@@ -197,6 +222,7 @@ static const struct chip_desc chips[last_ds_type] = {
 		.nvram_offset	= 0x20,
 		.nvram_size	= 4,	/* 32bit (4 word x 8 bit) */
 		.irq_handler = rx8130_irq,
+		.rtc_ops = &rx8130_rtc_ops,
 	},
 	[mcp794xx] = {
 		.alarm		= 1,
@@ -204,6 +230,7 @@ static const struct chip_desc chips[last_ds_type] = {
 		.nvram_offset	= 0x20,
 		.nvram_size	= 0x40,
 		.irq_handler = mcp794xx_irq,
+		.rtc_ops = &mcp794xx_rtc_ops,
 	},
 };
 
@@ -731,14 +758,6 @@ static int rx8130_alarm_irq_enable(struct device *dev, unsigned int enabled)
 	return regmap_write(ds1307->regmap, RX8130_REG_CONTROL0, reg);
 }
 
-static const struct rtc_class_ops rx8130_rtc_ops = {
-	.read_time	= ds1307_get_time,
-	.set_time	= ds1307_set_time,
-	.read_alarm	= rx8130_read_alarm,
-	.set_alarm	= rx8130_set_alarm,
-	.alarm_irq_enable = rx8130_alarm_irq_enable,
-};
-
 /*----------------------------------------------------------------------*/
 
 /*
@@ -891,14 +910,6 @@ static int mcp794xx_alarm_irq_enable(struct device *dev, unsigned int enabled)
 				  enabled ? MCP794XX_BIT_ALM0_EN : 0);
 }
 
-static const struct rtc_class_ops mcp794xx_rtc_ops = {
-	.read_time	= ds1307_get_time,
-	.set_time	= ds1307_set_time,
-	.read_alarm	= mcp794xx_read_alarm,
-	.set_alarm	= mcp794xx_set_alarm,
-	.alarm_irq_enable = mcp794xx_alarm_irq_enable,
-};
-
 /*----------------------------------------------------------------------*/
 
 static int ds1307_nvram_read(void *priv, unsigned int offset, void *val,
@@ -1325,8 +1336,6 @@ static int ds1307_probe(struct i2c_client *client,
 	unsigned long		timestamp;
 	u8 			trickle_charger_setup = 0;
 
-	const struct rtc_class_ops *rtc_ops = &ds13xx_rtc_ops;
-
 	ds1307 = devm_kzalloc(&client->dev, sizeof(struct ds1307), GFP_KERNEL);
 	if (!ds1307)
 		return -ENOMEM;
@@ -1495,14 +1504,10 @@ static int ds1307_probe(struct i2c_client *client,
 		break;
 	case rx_8130:
 		ds1307->offset = 0x10; /* Seconds starts at 0x10 */
-		rtc_ops = &rx8130_rtc_ops;
 		break;
 	case ds_1388:
 		ds1307->offset = 1; /* Seconds starts at 1 */
 		break;
-	case mcp794xx:
-		rtc_ops = &mcp794xx_rtc_ops;
-		break;
 	default:
 		break;
 	}
@@ -1678,7 +1683,7 @@ static int ds1307_probe(struct i2c_client *client,
 		ds1307->rtc->nvram_old_abi = true;
 	}
 
-	ds1307->rtc->ops = rtc_ops;
+	ds1307->rtc->ops = chip->rtc_ops ?: &ds13xx_rtc_ops;
 	err = rtc_register_device(ds1307->rtc);
 	if (err)
 		return err;
-- 
2.13.2

[PATCH 8/9] rtc: ds1307: factor out offset to struct chip_desc

[Heiner Kallweit]

Factor out offset to struct chip_desc and remove it from struct ds1307.

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

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 4083464f..43babd74 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -115,7 +115,6 @@ enum ds_type {
 
 
 struct ds1307 {
-	u8			offset; /* register's offset */
 	u8			regs[11];
 	u16			nvram_offset;
 	struct nvmem_config	nvmem_cfg;
@@ -136,6 +135,7 @@ struct chip_desc {
 	unsigned		alarm:1;
 	u16			nvram_offset;
 	u16			nvram_size;
+	u8			offset; /* register's offset */
 	u8			century_reg;
 	u8			century_enable_bit;
 	u8			century_bit;
@@ -208,6 +208,7 @@ static const struct chip_desc chips[last_ds_type] = {
 		.trickle_charger_reg = 0x08,
 	},
 	[ds_1388] = {
+		.offset		= 1,
 		.trickle_charger_reg = 0x0a,
 	},
 	[ds_3231] = {
@@ -221,6 +222,7 @@ static const struct chip_desc chips[last_ds_type] = {
 		/* this is battery backed SRAM */
 		.nvram_offset	= 0x20,
 		.nvram_size	= 4,	/* 32bit (4 word x 8 bit) */
+		.offset		= 0x10,
 		.irq_handler = rx8130_irq,
 		.rtc_ops = &rx8130_rtc_ops,
 	},
@@ -387,7 +389,7 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
 	const struct chip_desc *chip = &chips[ds1307->type];
 
 	/* read the RTC date and time registers all at once */
-	ret = regmap_bulk_read(ds1307->regmap, ds1307->offset, ds1307->regs, 7);
+	ret = regmap_bulk_read(ds1307->regmap, chip->offset, ds1307->regs, 7);
 	if (ret) {
 		dev_err(dev, "%s error %d\n", "read", ret);
 		return ret;
@@ -479,7 +481,7 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
 
 	dev_dbg(dev, "%s: %7ph\n", "write", buf);
 
-	result = regmap_bulk_write(ds1307->regmap, ds1307->offset, buf, 7);
+	result = regmap_bulk_write(ds1307->regmap, chip->offset, buf, 7);
 	if (result) {
 		dev_err(dev, "%s error %d\n", "write", result);
 		return result;
@@ -1502,19 +1504,13 @@ static int ds1307_probe(struct i2c_client *client,
 				     DS1307_REG_HOUR << 4 | 0x08, hour);
 		}
 		break;
-	case rx_8130:
-		ds1307->offset = 0x10; /* Seconds starts at 0x10 */
-		break;
-	case ds_1388:
-		ds1307->offset = 1; /* Seconds starts at 1 */
-		break;
 	default:
 		break;
 	}
 
 read_rtc:
 	/* read RTC registers */
-	err = regmap_bulk_read(ds1307->regmap, ds1307->offset, buf, 8);
+	err = regmap_bulk_read(ds1307->regmap, chip->offset, buf, 8);
 	if (err) {
 		dev_dbg(ds1307->dev, "read error %d\n", err);
 		goto exit;
@@ -1615,7 +1611,7 @@ static int ds1307_probe(struct i2c_client *client,
 			tmp = 0;
 		if (ds1307->regs[DS1307_REG_HOUR] & DS1307_BIT_PM)
 			tmp += 12;
-		regmap_write(ds1307->regmap, ds1307->offset + DS1307_REG_HOUR,
+		regmap_write(ds1307->regmap, chip->offset + DS1307_REG_HOUR,
 			     bin2bcd(tmp));
 	}
 
-- 
2.13.2

[PATCH 9/9] rtc: ds1307: remove member nvram_offset from struct ds1307

[Heiner Kallweit]

Remove member nvram_offset from struct ds1307 and use the value stored
in struct chip_desc directly.

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

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 43babd74..6eb7e86e 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -116,7 +116,6 @@ enum ds_type {
 
 struct ds1307 {
 	u8			regs[11];
-	u16			nvram_offset;
 	struct nvmem_config	nvmem_cfg;
 	enum ds_type		type;
 	unsigned long		flags;
@@ -918,8 +917,9 @@ static int ds1307_nvram_read(void *priv, unsigned int offset, void *val,
 			     size_t bytes)
 {
 	struct ds1307 *ds1307 = priv;
+	const struct chip_desc *chip = &chips[ds1307->type];
 
-	return regmap_bulk_read(ds1307->regmap, ds1307->nvram_offset + offset,
+	return regmap_bulk_read(ds1307->regmap, chip->nvram_offset + offset,
 				val, bytes);
 }
 
@@ -927,8 +927,9 @@ static int ds1307_nvram_write(void *priv, unsigned int offset, void *val,
 			      size_t bytes)
 {
 	struct ds1307 *ds1307 = priv;
+	const struct chip_desc *chip = &chips[ds1307->type];
 
-	return regmap_bulk_write(ds1307->regmap, ds1307->nvram_offset + offset,
+	return regmap_bulk_write(ds1307->regmap, chip->nvram_offset + offset,
 				 val, bytes);
 }
 
@@ -1673,7 +1674,6 @@ static int ds1307_probe(struct i2c_client *client,
 		ds1307->nvmem_cfg.reg_read = ds1307_nvram_read;
 		ds1307->nvmem_cfg.reg_write = ds1307_nvram_write;
 		ds1307->nvmem_cfg.priv = ds1307;
-		ds1307->nvram_offset = chip->nvram_offset;
 
 		ds1307->rtc->nvmem_config = &ds1307->nvmem_cfg;
 		ds1307->rtc->nvram_old_abi = true;
-- 
2.13.2

Re: [PATCH 0/9] rtc: ds1307: series with refactorings / improvements

[Alexandre Belloni]

Hi,

On 12/07/2017 at 07:23:31 +0200, Heiner Kallweit wrote:
> This series includes smaller refactorings and improvements.
> 
> Heiner Kallweit (9):
>   rtc: ds1307: remove member irq from struct ds1307
>   rtc: ds1307: factor out bbsqi bit to struct chip_desc
>   rtc: ds1307: improve trickle charger initialization
>   rtc: ds1307: constify struct chip_desc variables
>   rtc: ds1307: improve irq setup
>   rtc: ds1307: factor out irq_handler to struct chip_desc
>   rtc: ds1307: factor out rtc_ops to struct chip_desc
>   rtc: ds1307: factor out offset to struct chip_desc
>   rtc: ds1307: remove member nvram_offset from struct ds1307
> 
>  drivers/rtc/rtc-ds1307.c | 165 ++++++++++++++++++++++-------------------------
>  1 file changed, 76 insertions(+), 89 deletions(-)
> 

I finally reviewed and applied the series. I had to fix a few whitespace
issues though.

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