[PATCH v3 0/4] rtc: ds1307: series with refactorings

[PATCH v3 0/4] rtc: ds1307: series with refactorings

[Heiner Kallweit]

Series with certain refactorings for the ds1307 rtc driver.

v2:
- improved nvmem patch

v3:
- use IS_REACHABLE in nvmem patch to make sure that nvmem API
  isn't used if ds1307 is built-in and nvmem is a module
- add Reviewed-by to first three patches

Heiner Kallweit (4):
  rtc: ds1307: make definition of ds1307_of_match more compact
  rtc: ds1307: use regmap_update_bits where applicable
  rtc: ds1307: factor out century bit handling
  rtc: ds1307: change nvram handling to use the nvmem subsystem

 drivers/rtc/rtc-ds1307.c | 316 ++++++++++++++---------------------------------
 1 file changed, 91 insertions(+), 225 deletions(-)

-- 
2.13.0

[PATCH v3 1/4] rtc: ds1307: make definition of ds1307_of_match more compact

[Heiner Kallweit]

Make the definition of ds1307_of_match more compact. This makes the
code better readable and more in line with the definition of
ds1307_id and ds1307_acpi_ids.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
v2:
- no changes
v3:
- add reviewed-by
---
 drivers/rtc/rtc-ds1307.c | 70 ++++++++++--------------------------------------
 1 file changed, 14 insertions(+), 56 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index a2649385..b960ec3f 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -198,62 +198,20 @@ MODULE_DEVICE_TABLE(i2c, ds1307_id);
 
 #ifdef CONFIG_OF
 static const struct of_device_id ds1307_of_match[] = {
-	{
-		.compatible = "dallas,ds1307",
-		.data = (void *)ds_1307
-	},
-	{
-		.compatible = "dallas,ds1337",
-		.data = (void *)ds_1337
-	},
-	{
-		.compatible = "dallas,ds1338",
-		.data = (void *)ds_1338
-	},
-	{
-		.compatible = "dallas,ds1339",
-		.data = (void *)ds_1339
-	},
-	{
-		.compatible = "dallas,ds1388",
-		.data = (void *)ds_1388
-	},
-	{
-		.compatible = "dallas,ds1340",
-		.data = (void *)ds_1340
-	},
-	{
-		.compatible = "maxim,ds3231",
-		.data = (void *)ds_3231
-	},
-	{
-		.compatible = "st,m41t0",
-		.data = (void *)m41t00
-	},
-	{
-		.compatible = "st,m41t00",
-		.data = (void *)m41t00
-	},
-	{
-		.compatible = "microchip,mcp7940x",
-		.data = (void *)mcp794xx
-	},
-	{
-		.compatible = "microchip,mcp7941x",
-		.data = (void *)mcp794xx
-	},
-	{
-		.compatible = "pericom,pt7c4338",
-		.data = (void *)ds_1307
-	},
-	{
-		.compatible = "epson,rx8025",
-		.data = (void *)rx_8025
-	},
-	{
-		.compatible = "isil,isl12057",
-		.data = (void *)ds_1337
-	},
+	{ .compatible = "dallas,ds1307",	.data = (void *)ds_1307 },
+	{ .compatible = "dallas,ds1337",	.data = (void *)ds_1337 },
+	{ .compatible = "dallas,ds1338",	.data = (void *)ds_1338 },
+	{ .compatible = "dallas,ds1339",	.data = (void *)ds_1339 },
+	{ .compatible = "dallas,ds1388",	.data = (void *)ds_1388 },
+	{ .compatible = "dallas,ds1340",	.data = (void *)ds_1340 },
+	{ .compatible = "maxim,ds3231",		.data = (void *)ds_3231 },
+	{ .compatible = "st,m41t0",		.data = (void *)m41t00 },
+	{ .compatible = "st,m41t00",		.data = (void *)m41t00 },
+	{ .compatible = "microchip,mcp7940x",	.data = (void *)mcp794xx },
+	{ .compatible = "microchip,mcp7941x",	.data = (void *)mcp794xx },
+	{ .compatible = "pericom,pt7c4338",	.data = (void *)ds_1307 },
+	{ .compatible = "epson,rx8025",		.data = (void *)rx_8025 },
+	{ .compatible = "isil,isl12057",	.data = (void *)ds_1337 },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ds1307_of_match);
-- 
2.13.0

[PATCH v3 2/4] rtc: ds1307: use regmap_update_bits where applicable

[Heiner Kallweit]

After the switch to regmap we can now make use of regmap_update_bits
to simplify read/modify/write ops.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
v2:
- no changes
v3:
- add Reviewed-by
---
 drivers/rtc/rtc-ds1307.c | 82 ++++++++++++------------------------------------
 1 file changed, 20 insertions(+), 62 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index b960ec3f..bf906e9f 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -247,7 +247,7 @@ static irqreturn_t ds1307_irq(int irq, void *dev_id)
 {
 	struct ds1307		*ds1307 = dev_id;
 	struct mutex		*lock = &ds1307->rtc->ops_lock;
-	int			stat, control, ret;
+	int			stat, ret;
 
 	mutex_lock(lock);
 	ret = regmap_read(ds1307->regmap, DS1337_REG_STATUS, &stat);
@@ -258,13 +258,11 @@ static irqreturn_t ds1307_irq(int irq, void *dev_id)
 		stat &= ~DS1337_BIT_A1I;
 		regmap_write(ds1307->regmap, DS1337_REG_STATUS, stat);
 
-		ret = regmap_read(ds1307->regmap, DS1337_REG_CONTROL, &control);
+		ret = regmap_update_bits(ds1307->regmap, DS1337_REG_CONTROL,
+					 DS1337_BIT_A1IE, 0);
 		if (ret)
 			goto out;
 
-		control &= ~DS1337_BIT_A1IE;
-		regmap_write(ds1307->regmap, DS1337_REG_CONTROL, control);
-
 		rtc_update_irq(ds1307->rtc, 1, RTC_AF | RTC_IRQF);
 	}
 
@@ -517,21 +515,13 @@ 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 ds1307		*ds1307 = dev_get_drvdata(dev);
-	int			control, ret;
 
 	if (!test_bit(HAS_ALARM, &ds1307->flags))
 		return -ENOTTY;
 
-	ret = regmap_read(ds1307->regmap, DS1337_REG_CONTROL, &control);
-	if (ret)
-		return ret;
-
-	if (enabled)
-		control |= DS1337_BIT_A1IE;
-	else
-		control &= ~DS1337_BIT_A1IE;
-
-	return regmap_write(ds1307->regmap, DS1337_REG_CONTROL, control);
+	return regmap_update_bits(ds1307->regmap, DS1337_REG_CONTROL,
+				  DS1337_BIT_A1IE,
+				  enabled ? DS1337_BIT_A1IE : 0);
 }
 
 static const struct rtc_class_ops ds13xx_rtc_ops = {
@@ -586,11 +576,8 @@ static irqreturn_t mcp794xx_irq(int irq, void *dev_id)
 		goto out;
 
 	/* Disable alarm 0. */
-	ret = regmap_read(ds1307->regmap, MCP794XX_REG_CONTROL, &reg);
-	if (ret)
-		goto out;
-	reg &= ~MCP794XX_BIT_ALM0_EN;
-	ret = regmap_write(ds1307->regmap, MCP794XX_REG_CONTROL, reg);
+	ret = regmap_update_bits(ds1307->regmap, MCP794XX_REG_CONTROL,
+				 MCP794XX_BIT_ALM0_EN, 0);
 	if (ret)
 		goto out;
 
@@ -688,21 +675,13 @@ 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 ds1307 *ds1307 = dev_get_drvdata(dev);
-	int reg, ret;
 
 	if (!test_bit(HAS_ALARM, &ds1307->flags))
 		return -EINVAL;
 
-	ret = regmap_read(ds1307->regmap, MCP794XX_REG_CONTROL, &reg);
-	if (ret)
-		return ret;
-
-	if (enabled)
-		reg |= MCP794XX_BIT_ALM0_EN;
-	else
-		reg &= ~MCP794XX_BIT_ALM0_EN;
-
-	return regmap_write(ds1307->regmap, MCP794XX_REG_CONTROL, reg);
+	return regmap_update_bits(ds1307->regmap, MCP794XX_REG_CONTROL,
+				  MCP794XX_BIT_ALM0_EN,
+				  enabled ? MCP794XX_BIT_ALM0_EN : 0);
 }
 
 static const struct rtc_class_ops mcp794xx_rtc_ops = {
@@ -905,20 +884,11 @@ static int ds3231_clk_sqw_rates[] = {
 static int ds1337_write_control(struct ds1307 *ds1307, u8 mask, u8 value)
 {
 	struct mutex *lock = &ds1307->rtc->ops_lock;
-	int control;
 	int ret;
 
 	mutex_lock(lock);
-
-	ret = regmap_read(ds1307->regmap, DS1337_REG_CONTROL, &control);
-	if (ret)
-		goto out;
-
-	control &= ~mask;
-	control |= value;
-
-	ret = regmap_write(ds1307->regmap, DS1337_REG_CONTROL, control);
-out:
+	ret = regmap_update_bits(ds1307->regmap, DS1337_REG_CONTROL,
+				 mask, value);
 	mutex_unlock(lock);
 
 	return ret;
@@ -1024,22 +994,12 @@ static unsigned long ds3231_clk_32khz_recalc_rate(struct clk_hw *hw,
 static int ds3231_clk_32khz_control(struct ds1307 *ds1307, bool enable)
 {
 	struct mutex *lock = &ds1307->rtc->ops_lock;
-	int status;
 	int ret;
 
 	mutex_lock(lock);
-
-	ret = regmap_read(ds1307->regmap, DS1337_REG_STATUS, &status);
-	if (ret)
-		goto out;
-
-	if (enable)
-		status |= DS3231_BIT_EN32KHZ;
-	else
-		status &= ~DS3231_BIT_EN32KHZ;
-
-	ret = regmap_write(ds1307->regmap, DS1337_REG_STATUS, status);
-out:
+	ret = regmap_update_bits(ds1307->regmap, DS1337_REG_STATUS,
+				 DS3231_BIT_EN32KHZ,
+				 enable ? DS3231_BIT_EN32KHZ : 0);
 	mutex_unlock(lock);
 
 	return ret;
@@ -1495,12 +1455,10 @@ static int ds1307_probe(struct i2c_client *client,
 	 * If different then set the wday which we computed using
 	 * timestamp
 	 */
-	if (wday != tm.tm_wday) {
-		regmap_read(ds1307->regmap, MCP794XX_REG_WEEKDAY, &wday);
-		wday = wday & ~MCP794XX_REG_WEEKDAY_WDAY_MASK;
-		wday = wday | (tm.tm_wday + 1);
-		regmap_write(ds1307->regmap, MCP794XX_REG_WEEKDAY, wday);
-	}
+	if (wday != tm.tm_wday)
+		regmap_update_bits(ds1307->regmap, MCP794XX_REG_WEEKDAY,
+				   MCP794XX_REG_WEEKDAY_WDAY_MASK,
+				   tm.tm_wday + 1);
 
 	if (want_irq) {
 		device_set_wakeup_capable(ds1307->dev, true);
-- 
2.13.0

[PATCH v3 3/4] rtc: ds1307: factor out century bit handling

[Heiner Kallweit]

The driver has lots of places with chip-specific code what doesn't
necessarily facilitate maintenance.

Let's describe chip-specific differences in century bit handling
in struct chip_desc to improve this.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
v2:
- no changes
v3:
- add Reviewed-by
---
 drivers/rtc/rtc-ds1307.c | 70 ++++++++++++++++++------------------------------
 1 file changed, 26 insertions(+), 44 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index bf906e9f..79160027 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -135,6 +135,9 @@ struct chip_desc {
 	unsigned		alarm:1;
 	u16			nvram_offset;
 	u16			nvram_size;
+	u8			century_reg;
+	u8			century_enable_bit;
+	u8			century_bit;
 	u16			trickle_charger_reg;
 	u8			trickle_charger_setup;
 	u8			(*do_trickle_setup)(struct ds1307 *, uint32_t,
@@ -150,6 +153,8 @@ static struct chip_desc chips[last_ds_type] = {
 	},
 	[ds_1337] = {
 		.alarm		= 1,
+		.century_reg	= DS1307_REG_MONTH,
+		.century_bit	= DS1337_BIT_CENTURY,
 	},
 	[ds_1338] = {
 		.nvram_offset	= 8,
@@ -157,10 +162,15 @@ static struct chip_desc chips[last_ds_type] = {
 	},
 	[ds_1339] = {
 		.alarm		= 1,
+		.century_reg	= DS1307_REG_MONTH,
+		.century_bit	= DS1337_BIT_CENTURY,
 		.trickle_charger_reg = 0x10,
 		.do_trickle_setup = &do_trickle_setup_ds1339,
 	},
 	[ds_1340] = {
+		.century_reg	= DS1307_REG_HOUR,
+		.century_enable_bit = DS1340_BIT_CENTURY_EN,
+		.century_bit	= DS1340_BIT_CENTURY,
 		.trickle_charger_reg = 0x08,
 	},
 	[ds_1388] = {
@@ -168,6 +178,8 @@ static struct chip_desc chips[last_ds_type] = {
 	},
 	[ds_3231] = {
 		.alarm		= 1,
+		.century_reg	= DS1307_REG_MONTH,
+		.century_bit	= DS1337_BIT_CENTURY,
 	},
 	[mcp794xx] = {
 		.alarm		= 1,
@@ -277,6 +289,7 @@ 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);
+	const struct chip_desc *chip = &chips[ds1307->type];
 	int		tmp, ret;
 
 	/* read the RTC date and time registers all at once */
@@ -306,20 +319,8 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
 	t->tm_year = bcd2bin(ds1307->regs[DS1307_REG_YEAR]) + 100;
 
 #ifdef CONFIG_RTC_DRV_DS1307_CENTURY
-	switch (ds1307->type) {
-	case ds_1337:
-	case ds_1339:
-	case ds_3231:
-		if (ds1307->regs[DS1307_REG_MONTH] & DS1337_BIT_CENTURY)
-			t->tm_year += 100;
-		break;
-	case ds_1340:
-		if (ds1307->regs[DS1307_REG_HOUR] & DS1340_BIT_CENTURY)
-			t->tm_year += 100;
-		break;
-	default:
-		break;
-	}
+	if (ds1307->regs[chip->century_reg] & chip->century_bit)
+		t->tm_year += 100;
 #endif
 
 	dev_dbg(dev, "%s secs=%d, mins=%d, "
@@ -335,6 +336,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 chip_desc *chip = &chips[ds1307->type];
 	int		result;
 	int		tmp;
 	u8		*buf = ds1307->regs;
@@ -345,24 +347,14 @@ 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);
 
-#ifdef CONFIG_RTC_DRV_DS1307_CENTURY
 	if (t->tm_year < 100)
 		return -EINVAL;
 
-	switch (ds1307->type) {
-	case ds_1337:
-	case ds_1339:
-	case ds_3231:
-	case ds_1340:
-		if (t->tm_year > 299)
-			return -EINVAL;
-	default:
-		if (t->tm_year > 199)
-			return -EINVAL;
-		break;
-	}
+#ifdef CONFIG_RTC_DRV_DS1307_CENTURY
+	if (t->tm_year > (chip->century_bit ? 299 : 199))
+		return -EINVAL;
 #else
-	if (t->tm_year < 100 || t->tm_year > 199)
+	if (t->tm_year > 199)
 		return -EINVAL;
 #endif
 
@@ -377,19 +369,12 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
 	tmp = t->tm_year - 100;
 	buf[DS1307_REG_YEAR] = bin2bcd(tmp);
 
-	switch (ds1307->type) {
-	case ds_1337:
-	case ds_1339:
-	case ds_3231:
-		if (t->tm_year > 199)
-			buf[DS1307_REG_MONTH] |= DS1337_BIT_CENTURY;
-		break;
-	case ds_1340:
-		buf[DS1307_REG_HOUR] |= DS1340_BIT_CENTURY_EN;
-		if (t->tm_year > 199)
-			buf[DS1307_REG_HOUR] |= DS1340_BIT_CENTURY;
-		break;
-	case mcp794xx:
+	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
@@ -397,9 +382,6 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
 		 */
 		buf[DS1307_REG_SECS] |= MCP794XX_BIT_ST;
 		buf[DS1307_REG_WDAY] |= MCP794XX_BIT_VBATEN;
-		break;
-	default:
-		break;
 	}
 
 	dev_dbg(dev, "%s: %7ph\n", "write", buf);
-- 
2.13.0

[PATCH v3 4/4] rtc: ds1307: change nvram handling to use the nvmem subsystem

[Heiner Kallweit]

The open coded handling of attribute nvram is from 2007 when the
nvmem subsystem obviously didn't exist yet.

Now we can use nvmem to simplify the driver. A side effect is that
attribute nvram is replaced. New location is:
/sys/bus/nvmem/devices/ds1307_nvram0/nvmem

This might break user space applications. However attribute nvram
is nowhere officially documented and I'm not sure whether anybody
is actually using the few nvram bytes on a rtc chip.

This patch is compile-tested only as I lack hardware with nvram.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- embed nvmem_config structure part in struct ds1307
- set name and owner members of struct nvmem_config
v3:
- extend Kconfig help text
- use IS_REACHABLE to not access the NVMEM API if ds1307 is
  built-in and NVMEM is a module. I don't use Kconfig dependencies
  because only few supported chips have NVRAM.
---
 drivers/rtc/Kconfig      |   5 ++-
 drivers/rtc/rtc-ds1307.c | 101 ++++++++++++++++++-----------------------------
 2 files changed, 42 insertions(+), 64 deletions(-)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 8d3b9572..22ac31fd 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -219,8 +219,9 @@ config RTC_DRV_DS1307
 
 	  The first seven registers on these chips hold an RTC, and other
 	  registers may add features such as NVRAM, a trickle charger for
-	  the RTC/NVRAM backup power, and alarms. NVRAM is visible in
-	  sysfs, but other chip features may not be available.
+	  the RTC/NVRAM backup power, and alarms.
+	  If your chip has NVRAM and CONFIG_NVMEM is set then the NVRAM
+	  is accessible via sysfs.
 
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-ds1307.
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 79160027..a2a90cd2 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -25,6 +25,7 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/clk-provider.h>
 #include <linux/regmap.h>
+#include <linux/nvmem-provider.h>
 
 /*
  * We can't determine type by probing, but if we expect pre-Linux code
@@ -116,11 +117,11 @@ struct ds1307 {
 	u8			offset; /* register's offset */
 	u8			regs[11];
 	u16			nvram_offset;
-	struct bin_attribute	*nvram;
+	struct nvmem_config	nvram_cfg;
+	struct nvmem_device	*nvram;
 	enum ds_type		type;
 	unsigned long		flags;
-#define HAS_NVRAM	0		/* bit 0 == sysfs file active */
-#define HAS_ALARM	1		/* bit 1 == irq claimed */
+#define HAS_ALARM	0		/* bit 0 == irq claimed */
 	struct device		*dev;
 	struct regmap		*regmap;
 	const char		*name;
@@ -676,42 +677,27 @@ static const struct rtc_class_ops mcp794xx_rtc_ops = {
 
 /*----------------------------------------------------------------------*/
 
-static ssize_t
-ds1307_nvram_read(struct file *filp, struct kobject *kobj,
-		struct bin_attribute *attr,
-		char *buf, loff_t off, size_t count)
-{
-	struct ds1307		*ds1307;
-	int			result;
+#if IS_REACHABLE(CONFIG_NVMEM)
 
-	ds1307 = dev_get_drvdata(kobj_to_dev(kobj));
+static int ds1307_nvram_read(void *priv, unsigned int offset, void *val,
+			     size_t bytes)
+{
+	struct ds1307 *ds1307 = priv;
 
-	result = regmap_bulk_read(ds1307->regmap, ds1307->nvram_offset + off,
-				  buf, count);
-	if (result)
-		dev_err(ds1307->dev, "%s error %d\n", "nvram read", result);
-	return result;
+	return regmap_bulk_read(ds1307->regmap, ds1307->nvram_offset + offset,
+				val, bytes);
 }
 
-static ssize_t
-ds1307_nvram_write(struct file *filp, struct kobject *kobj,
-		struct bin_attribute *attr,
-		char *buf, loff_t off, size_t count)
+static int ds1307_nvram_write(void *priv, unsigned int offset, void *val,
+			      size_t bytes)
 {
-	struct ds1307		*ds1307;
-	int			result;
-
-	ds1307 = dev_get_drvdata(kobj_to_dev(kobj));
+	struct ds1307 *ds1307 = priv;
 
-	result = regmap_bulk_write(ds1307->regmap, ds1307->nvram_offset + off,
-				   buf, count);
-	if (result) {
-		dev_err(ds1307->dev, "%s error %d\n", "nvram write", result);
-		return result;
-	}
-	return count;
+	return regmap_bulk_write(ds1307->regmap, ds1307->nvram_offset + offset,
+				 val, bytes);
 }
 
+#endif
 
 /*----------------------------------------------------------------------*/
 
@@ -1475,39 +1461,28 @@ static int ds1307_probe(struct i2c_client *client,
 			dev_dbg(ds1307->dev, "got IRQ %d\n", client->irq);
 	}
 
+#if IS_REACHABLE(CONFIG_NVMEM)
 	if (chip->nvram_size) {
-
-		ds1307->nvram = devm_kzalloc(ds1307->dev,
-					sizeof(struct bin_attribute),
-					GFP_KERNEL);
-		if (!ds1307->nvram) {
-			dev_err(ds1307->dev,
-				"cannot allocate memory for nvram sysfs\n");
+		ds1307->nvram_cfg.name = "ds1307_nvram";
+		ds1307->nvram_cfg.dev = ds1307->dev;
+		ds1307->nvram_cfg.reg_read = ds1307_nvram_read;
+		ds1307->nvram_cfg.reg_write = ds1307_nvram_write;
+		ds1307->nvram_cfg.size = chip->nvram_size;
+		ds1307->nvram_cfg.word_size = 1;
+		ds1307->nvram_cfg.stride = 1;
+		ds1307->nvram_cfg.priv = ds1307;
+		ds1307->nvram_cfg.owner = THIS_MODULE;
+
+		ds1307->nvram = nvmem_register(&ds1307->nvram_cfg);
+		if (IS_ERR(ds1307->nvram)) {
+			dev_err(ds1307->dev, "unable to register nvmem\n");
+			ds1307->nvram = NULL;
 		} else {
-
-			ds1307->nvram->attr.name = "nvram";
-			ds1307->nvram->attr.mode = S_IRUGO | S_IWUSR;
-
-			sysfs_bin_attr_init(ds1307->nvram);
-
-			ds1307->nvram->read = ds1307_nvram_read;
-			ds1307->nvram->write = ds1307_nvram_write;
-			ds1307->nvram->size = chip->nvram_size;
-			ds1307->nvram_offset = chip->nvram_offset;
-
-			err = sysfs_create_bin_file(&ds1307->dev->kobj,
-						    ds1307->nvram);
-			if (err) {
-				dev_err(ds1307->dev,
-					"unable to create sysfs file: %s\n",
-					ds1307->nvram->attr.name);
-			} else {
-				set_bit(HAS_NVRAM, &ds1307->flags);
-				dev_info(ds1307->dev, "%zu bytes nvram\n",
-					 ds1307->nvram->size);
-			}
+			dev_info(ds1307->dev, "%d bytes nvram\n",
+				 chip->nvram_size);
 		}
 	}
+#endif
 
 	ds1307_hwmon_register(ds1307);
 	ds1307_clks_register(ds1307);
@@ -1522,8 +1497,10 @@ static int ds1307_remove(struct i2c_client *client)
 {
 	struct ds1307 *ds1307 = i2c_get_clientdata(client);
 
-	if (test_and_clear_bit(HAS_NVRAM, &ds1307->flags))
-		sysfs_remove_bin_file(&ds1307->dev->kobj, ds1307->nvram);
+#if IS_REACHABLE(CONFIG_NVMEM)
+	if (ds1307->nvram)
+		nvmem_unregister(ds1307->nvram);
+#endif
 
 	return 0;
 }
-- 
2.13.0

Re: [PATCH v3 1/4] rtc: ds1307: make definition of ds1307_of_match more compact

[Alexandre Belloni]

On 05/06/2017 at 17:57:22 +0200, Heiner Kallweit wrote:
> Make the definition of ds1307_of_match more compact. This makes the
> code better readable and more in line with the definition of
> ds1307_id and ds1307_acpi_ids.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> v2:
> - no changes
> v3:
> - add reviewed-by

Well, I don't like it, and I don't think it actually improves
readability. My experience shows that the opposite of this patch is
quite often necessary.

> ---
>  drivers/rtc/rtc-ds1307.c | 70 ++++++++++--------------------------------------
>  1 file changed, 14 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index a2649385..b960ec3f 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -198,62 +198,20 @@ MODULE_DEVICE_TABLE(i2c, ds1307_id);
>  
>  #ifdef CONFIG_OF
>  static const struct of_device_id ds1307_of_match[] = {
> -	{
> -		.compatible = "dallas,ds1307",
> -		.data = (void *)ds_1307
> -	},
> -	{
> -		.compatible = "dallas,ds1337",
> -		.data = (void *)ds_1337
> -	},
> -	{
> -		.compatible = "dallas,ds1338",
> -		.data = (void *)ds_1338
> -	},
> -	{
> -		.compatible = "dallas,ds1339",
> -		.data = (void *)ds_1339
> -	},
> -	{
> -		.compatible = "dallas,ds1388",
> -		.data = (void *)ds_1388
> -	},
> -	{
> -		.compatible = "dallas,ds1340",
> -		.data = (void *)ds_1340
> -	},
> -	{
> -		.compatible = "maxim,ds3231",
> -		.data = (void *)ds_3231
> -	},
> -	{
> -		.compatible = "st,m41t0",
> -		.data = (void *)m41t00
> -	},
> -	{
> -		.compatible = "st,m41t00",
> -		.data = (void *)m41t00
> -	},
> -	{
> -		.compatible = "microchip,mcp7940x",
> -		.data = (void *)mcp794xx
> -	},
> -	{
> -		.compatible = "microchip,mcp7941x",
> -		.data = (void *)mcp794xx
> -	},
> -	{
> -		.compatible = "pericom,pt7c4338",
> -		.data = (void *)ds_1307
> -	},
> -	{
> -		.compatible = "epson,rx8025",
> -		.data = (void *)rx_8025
> -	},
> -	{
> -		.compatible = "isil,isl12057",
> -		.data = (void *)ds_1337
> -	},
> +	{ .compatible = "dallas,ds1307",	.data = (void *)ds_1307 },
> +	{ .compatible = "dallas,ds1337",	.data = (void *)ds_1337 },
> +	{ .compatible = "dallas,ds1338",	.data = (void *)ds_1338 },
> +	{ .compatible = "dallas,ds1339",	.data = (void *)ds_1339 },
> +	{ .compatible = "dallas,ds1388",	.data = (void *)ds_1388 },
> +	{ .compatible = "dallas,ds1340",	.data = (void *)ds_1340 },
> +	{ .compatible = "maxim,ds3231",		.data = (void *)ds_3231 },
> +	{ .compatible = "st,m41t0",		.data = (void *)m41t00 },
> +	{ .compatible = "st,m41t00",		.data = (void *)m41t00 },
> +	{ .compatible = "microchip,mcp7940x",	.data = (void *)mcp794xx },
> +	{ .compatible = "microchip,mcp7941x",	.data = (void *)mcp794xx },
> +	{ .compatible = "pericom,pt7c4338",	.data = (void *)ds_1307 },
> +	{ .compatible = "epson,rx8025",		.data = (void *)rx_8025 },
> +	{ .compatible = "isil,isl12057",	.data = (void *)ds_1337 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, ds1307_of_match);
> -- 
> 2.13.0
> 
> 
> 
> 

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

Re: [PATCH v3 1/4] rtc: ds1307: make definition of ds1307_of_match more compact

[Linus Walleij]

On Mon, Jun 5, 2017 at 7:00 PM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> On 05/06/2017 at 17:57:22 +0200, Heiner Kallweit wrote:
>> Make the definition of ds1307_of_match more compact. This makes the
>> code better readable and more in line with the definition of
>> ds1307_id and ds1307_acpi_ids.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>> v2:
>> - no changes
>> v3:
>> - add reviewed-by
>
> Well, I don't like it, and I don't think it actually improves
> readability. My experience shows that the opposite of this patch is
> quite often necessary.

The patch is certainly not important and should ideally be last in the
series.

Is it possible to apply patches 2-4 right off? They seem fairly orthogonal
to this one.

Yours,
Linus Walleij

Re: [PATCH v3 2/4] rtc: ds1307: use regmap_update_bits where applicable

[Alexandre Belloni]

On 05/06/2017 at 17:57:29 +0200, Heiner Kallweit wrote:
> After the switch to regmap we can now make use of regmap_update_bits
> to simplify read/modify/write ops.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> v2:
> - no changes
> v3:
> - add Reviewed-by
> ---
>  drivers/rtc/rtc-ds1307.c | 82 ++++++++++++------------------------------------
>  1 file changed, 20 insertions(+), 62 deletions(-)
> 
Applied, thanks.

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

Re: [PATCH v3 3/4] rtc: ds1307: factor out century bit handling

[Alexandre Belloni]

On 05/06/2017 at 17:57:33 +0200, Heiner Kallweit wrote:
> The driver has lots of places with chip-specific code what doesn't
> necessarily facilitate maintenance.
> 
> Let's describe chip-specific differences in century bit handling
> in struct chip_desc to improve this.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> v2:
> - no changes
> v3:
> - add Reviewed-by
> ---
>  drivers/rtc/rtc-ds1307.c | 70 ++++++++++++++++++------------------------------
>  1 file changed, 26 insertions(+), 44 deletions(-)
> 
Applied, thanks.

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