linux-rtc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH v2 08/10] rtc: bd70528: Initial support for ROHM bd70528 RTC
@ 2019-01-25 11:05 Matti Vaittinen
  2019-01-26 16:30 ` Guenter Roeck
  2019-01-28 21:20 ` Alexandre Belloni
  0 siblings, 2 replies; 10+ messages in thread
From: Matti Vaittinen @ 2019-01-25 11:05 UTC (permalink / raw)
  To: mazziesaccount, matti.vaittinen
  Cc: heikki.haikola, mikko.mutanen, lee.jones, robh+dt, mark.rutland,
	broonie, gregkh, rafael, mturquette, sboyd, linus.walleij,
	bgolaszewski, sre, lgirdwood, a.zummo, alexandre.belloni, wim,
	linux, devicetree, linux-kernel, linux-clk, linux-gpio, linux-pm,
	linux-rtc, linux-watchdog

Support RTC block in ROHM bd70528 power management IC. Support
getting and setting the time and date as well as arming an alarm
which can also be used to wake the PMIC from standby state.

HW supports wake interrupt only for the next 24 hours (sec, minute
and hour information only) so we limit also the alarm interrupt to
this 24 hours for the sake of consistency.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/rtc/Kconfig       |   8 +
 drivers/rtc/Makefile      |   1 +
 drivers/rtc/rtc-bd70528.c | 451 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 460 insertions(+)
 create mode 100644 drivers/rtc/rtc-bd70528.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 225b0b8516f3..df6211cbd83f 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -487,6 +487,14 @@ config RTC_DRV_M41T80_WDT
 	help
 	  If you say Y here you will get support for the
 	  watchdog timer in the ST M41T60 and M41T80 RTC chips series.
+config RTC_DRV_BD70528
+	tristate "ROHM BD70528 PMIC RTC"
+	help
+	  If you say Y here you will get support for the RTC
+	  on ROHM BD70528 Power Management IC.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called rtc-bd70528.
 
 config RTC_DRV_BQ32K
 	tristate "TI BQ32000"
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index df022d820bee..740b13840913 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_RTC_DRV_ASM9260)	+= rtc-asm9260.o
 obj-$(CONFIG_RTC_DRV_AT91RM9200)+= rtc-at91rm9200.o
 obj-$(CONFIG_RTC_DRV_AT91SAM9)	+= rtc-at91sam9.o
 obj-$(CONFIG_RTC_DRV_AU1XXX)	+= rtc-au1xxx.o
+obj-$(CONFIG_RTC_DRV_BD70528)	+= rtc-bd70528.o
 obj-$(CONFIG_RTC_DRV_BQ32K)	+= rtc-bq32k.o
 obj-$(CONFIG_RTC_DRV_BQ4802)	+= rtc-bq4802.o
 obj-$(CONFIG_RTC_DRV_BRCMSTB)	+= rtc-brcmstb-waketimer.o
diff --git a/drivers/rtc/rtc-bd70528.c b/drivers/rtc/rtc-bd70528.c
new file mode 100644
index 000000000000..250fb218c784
--- /dev/null
+++ b/drivers/rtc/rtc-bd70528.c
@@ -0,0 +1,451 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+//
+// Copyright (C) 2018 ROHM Semiconductors
+//
+// RTC driver for ROHM BD70528 PMIC
+
+#include <linux/bcd.h>
+#include <linux/mfd/rohm-bd70528.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/rtc.h>
+
+/*
+ * We read regs RTC_SEC => RTC_YEAR
+ * this struct is ordered according to chip registers.
+ * Keep it u8 only to avoid padding issues.
+ */
+struct bd70528_rtc_day {
+	u8 sec;
+	u8 min;
+	u8 hour;
+};
+
+struct bd70528_rtc_data {
+	struct bd70528_rtc_day time;
+	u8 week;
+	u8 day;
+	u8 month;
+	u8 year;
+};
+
+struct bd70528_rtc_wake {
+	struct bd70528_rtc_day time;
+	u8 ctrl;
+};
+
+struct bd70528_rtc_alm {
+	struct bd70528_rtc_data data;
+	u8 alm_mask;
+	u8 alm_repeat;
+};
+
+struct bd70528_rtc {
+	struct bd70528 *mfd;
+	struct device *dev;
+};
+
+static int bd70528_set_wake(struct bd70528 *bd70528,
+			    int enable, int *old_state)
+{
+	int ret;
+	unsigned int ctrl_reg;
+
+	ret = regmap_read(bd70528->chip.regmap, BD70528_REG_WAKE_EN, &ctrl_reg);
+	if (ret)
+		return ret;
+
+	if (old_state) {
+		if (ctrl_reg & BD70528_MASK_WAKE_EN)
+			*old_state |= BD70528_WAKE_STATE_BIT;
+		else
+			*old_state &= ~BD70528_WAKE_STATE_BIT;
+
+		if ((!enable) == (!(*old_state & BD70528_WAKE_STATE_BIT)))
+			return 0;
+	}
+
+	if (enable)
+		ctrl_reg |= BD70528_MASK_WAKE_EN;
+	else
+		ctrl_reg &= ~BD70528_MASK_WAKE_EN;
+
+	return regmap_write(bd70528->chip.regmap, BD70528_REG_WAKE_EN,
+			    ctrl_reg);
+}
+
+static int bd70528_set_elapsed_tmr(struct bd70528 *bd70528,
+				   int enable, int *old_state)
+{
+	int ret;
+	unsigned int ctrl_reg;
+
+	/*
+	 * TBD
+	 * What is the purpose of elapsed timer ?
+	 * Is the timeout registers counting down, or is the disable - re-enable
+	 * going to restart the elapsed-time counting? If counting is restarted
+	 * the timeout should be decreased by the amount of time that has
+	 * elapsed since starting the timer. Maybe we should store the monotonic
+	 * clock value when timer is started so that if RTC is set while timer
+	 * is armed we could do the compensation. This is a hack if RTC/system
+	 * clk are drifting. OTOH, RTC controlled via I2C is in any case
+	 * inaccurate...
+	 */
+	ret = regmap_read(bd70528->chip.regmap, BD70528_REG_ELAPSED_TIMER_EN,
+			  &ctrl_reg);
+	if (ret)
+		return ret;
+
+	if (old_state) {
+		if (ctrl_reg & BD70528_MASK_ELAPSED_TIMER_EN)
+			*old_state |= BD70528_ELAPSED_STATE_BIT;
+		else
+			*old_state &= ~BD70528_ELAPSED_STATE_BIT;
+
+		if ((!enable) == (!(*old_state & BD70528_ELAPSED_STATE_BIT)))
+			return 0;
+	}
+
+	if (enable)
+		ctrl_reg |= BD70528_MASK_ELAPSED_TIMER_EN;
+	else
+		ctrl_reg &= ~BD70528_MASK_ELAPSED_TIMER_EN;
+
+	return regmap_write(bd70528->chip.regmap, BD70528_REG_ELAPSED_TIMER_EN,
+			    ctrl_reg);
+}
+
+static int bd70528_set_rtc_based_timers(struct bd70528_rtc *r, int new_state,
+							int *old_state)
+{
+	int ret;
+
+	ret = r->mfd->wdt_set(r->mfd, new_state & BD70528_WDT_STATE_BIT,
+			       old_state);
+	if (ret) {
+		dev_err(r->dev,
+			"Failed to disable WDG for RTC setting (%d)\n", ret);
+		return ret;
+	}
+	ret = bd70528_set_elapsed_tmr(r->mfd,
+				      new_state & BD70528_ELAPSED_STATE_BIT,
+				      old_state);
+	if (ret) {
+		dev_err(r->dev,
+			"Failed to disable 'elapsed timer' for RTC setting\n");
+		return ret;
+	}
+	ret = bd70528_set_wake(r->mfd, new_state & BD70528_WAKE_STATE_BIT,
+			       old_state);
+	if (ret) {
+		dev_err(r->dev,
+			"Failed to disable 'wake timer' for RTC setting\n");
+		return ret;
+	}
+
+	return ret;
+}
+
+static int bd70528_re_enable_rtc_based_timers(struct bd70528_rtc *r,
+							int old_state)
+{
+	int ret;
+
+	ret = bd70528_set_rtc_based_timers(r, old_state, NULL);
+	mutex_unlock(&r->mfd->rtc_timer_lock);
+
+	return ret;
+}
+
+static int bd70528_disable_rtc_based_timers(struct bd70528_rtc *r,
+							int *old_state)
+{
+	mutex_lock(&r->mfd->rtc_timer_lock);
+
+	return bd70528_set_rtc_based_timers(r, 0, old_state);
+}
+
+static inline void tmday2rtc(struct rtc_time *t, struct bd70528_rtc_day *d)
+{
+	d->sec &= ~BD70528_MASK_RTC_SEC;
+	d->min &= ~BD70528_MASK_RTC_MINUTE;
+	d->hour &= ~BD70528_MASK_RTC_HOUR;
+	d->sec |= bin2bcd(t->tm_sec);
+	d->min |= bin2bcd(t->tm_min);
+	d->hour |= bin2bcd(t->tm_hour);
+}
+
+static inline void tm2rtc(struct rtc_time *t, struct bd70528_rtc_data *r)
+{
+	r->day &= ~BD70528_MASK_RTC_DAY;
+	r->week &= ~BD70528_MASK_RTC_WEEK;
+	r->month &= ~BD70528_MASK_RTC_MONTH;
+	/*
+	 * PM and 24H bits are not used by Wake - thus we clear them
+	 * here and not in tmday2rtc() which is also used by wake.
+	 */
+	r->time.hour &= ~(BD70528_MASK_RTC_HOUR_PM | BD70528_MASK_RTC_HOUR_24H);
+
+	tmday2rtc(t, &r->time);
+	/*
+	 * We do always set time in 24H mode.
+	 */
+	r->time.hour |= BD70528_MASK_RTC_HOUR_24H;
+	r->day |= bin2bcd(t->tm_mday);
+	r->week |= bin2bcd(t->tm_wday);
+	r->month |= bin2bcd(t->tm_mon + 1);
+	r->year = bin2bcd(t->tm_year-100);
+}
+
+static inline void rtc2tm(struct bd70528_rtc_data *r, struct rtc_time *t)
+{
+	t->tm_sec = bcd2bin(r->time.sec & BD70528_MASK_RTC_SEC);
+	t->tm_min = bcd2bin(r->time.min & BD70528_MASK_RTC_MINUTE);
+	t->tm_hour = bcd2bin(r->time.hour & BD70528_MASK_RTC_HOUR);
+	/*
+	 * If RTC is in 12H mode, then bit BD70528_MASK_RTC_HOUR_PM
+	 * is not BCD value but tells whether it is AM or PM
+	 */
+	if (!(r->time.hour & BD70528_MASK_RTC_HOUR_24H)) {
+		t->tm_hour %= 12;
+		if (r->time.hour & BD70528_MASK_RTC_HOUR_PM)
+			t->tm_hour += 12;
+	}
+	t->tm_mday = bcd2bin(r->day & BD70528_MASK_RTC_DAY);
+	t->tm_mon = bcd2bin(r->month & BD70528_MASK_RTC_MONTH) - 1;
+	t->tm_year = 100 + bcd2bin(r->year & BD70528_MASK_RTC_YEAR);
+	t->tm_wday = bcd2bin(r->week & BD70528_MASK_RTC_WEEK);
+}
+
+static int bd70528_set_alarm(struct device *dev, struct rtc_wkalrm *a)
+{
+	struct bd70528_rtc_wake wake;
+	struct bd70528_rtc_alm alm;
+	int ret;
+	struct bd70528_rtc *r = dev_get_drvdata(dev);
+	struct bd70528 *bd70528 = r->mfd;
+
+	ret = regmap_bulk_read(bd70528->chip.regmap, BD70528_REG_RTC_WAKE_START,
+			       &wake, sizeof(wake));
+	if (ret) {
+		dev_err(dev, "Failed to read wake regs\n");
+		return ret;
+	}
+
+	ret = regmap_bulk_read(bd70528->chip.regmap, BD70528_REG_RTC_ALM_START,
+			       &alm, sizeof(alm));
+	if (ret) {
+		dev_err(dev, "Failed to read alarm regs\n");
+		return ret;
+	}
+
+	tm2rtc(&a->time, &alm.data);
+	tmday2rtc(&a->time, &wake.time);
+
+	if (a->enabled) {
+		alm.alm_mask &= ~BD70528_MASK_ALM_EN;
+		wake.ctrl |= BD70528_MASK_WAKE_EN;
+	} else {
+		alm.alm_mask |= BD70528_MASK_ALM_EN;
+		wake.ctrl &= ~BD70528_MASK_WAKE_EN;
+	}
+
+	ret = regmap_bulk_write(bd70528->chip.regmap,
+				BD70528_REG_RTC_WAKE_START, &wake,
+				sizeof(wake));
+	if (ret) {
+		dev_err(dev, "Failed to set wake time\n");
+		return ret;
+	}
+	ret = regmap_bulk_write(bd70528->chip.regmap, BD70528_REG_RTC_ALM_START,
+				&alm, sizeof(alm));
+	if (ret)
+		dev_err(dev, "Failed to set alarm time\n");
+
+	return ret;
+}
+
+static int bd70528_read_alarm(struct device *dev, struct rtc_wkalrm *a)
+{
+	struct bd70528_rtc_alm alm;
+	int ret;
+	struct bd70528_rtc *r = dev_get_drvdata(dev);
+	struct bd70528 *bd70528 = r->mfd;
+
+	ret = regmap_bulk_read(bd70528->chip.regmap, BD70528_REG_RTC_ALM_START,
+			  &alm, sizeof(alm));
+	if (ret) {
+		dev_err(dev, "Failed to read alarm regs\n");
+		return ret;
+	}
+
+	rtc2tm(&alm.data, &a->time);
+	a->time.tm_mday = -1;
+	a->time.tm_mon = -1;
+	a->time.tm_year = -1;
+	if (alm.alm_mask & BD70528_MASK_ALM_EN)
+		a->enabled = 0;
+	else
+		a->enabled = 1;
+
+	a->pending = 0;
+
+	return 0;
+}
+
+static int bd70528_set_time(struct device *dev, struct rtc_time *t)
+{
+	int ret, old_states;
+	struct bd70528_rtc_data rtc_data;
+	struct bd70528_rtc *r = dev_get_drvdata(dev);
+	struct bd70528 *bd70528 = r->mfd;
+
+	ret = bd70528_disable_rtc_based_timers(r, &old_states);
+
+	ret = regmap_bulk_read(bd70528->chip.regmap,
+			       BD70528_REG_RTC_START, &rtc_data,
+			       sizeof(rtc_data));
+
+	tm2rtc(t, &rtc_data);
+
+	ret = regmap_bulk_write(bd70528->chip.regmap,
+				BD70528_REG_RTC_START, &rtc_data,
+				sizeof(rtc_data));
+
+	ret = bd70528_re_enable_rtc_based_timers(r, old_states);
+
+	return 0;
+}
+static int bd70528_get_time(struct device *dev, struct rtc_time *t)
+{
+	struct bd70528_rtc *r = dev_get_drvdata(dev);
+	struct bd70528 *bd70528 = r->mfd;
+	struct bd70528_rtc_data rtc_data;
+	int ret;
+
+	/* read the RTC date and time registers all at once */
+	ret = regmap_bulk_read(bd70528->chip.regmap,
+			       BD70528_REG_RTC_START, &rtc_data,
+			       sizeof(rtc_data));
+	if (ret) {
+		dev_err(dev, "Failed to read RTC time (err %d)\n", ret);
+		return ret;
+	}
+
+	rtc2tm(&rtc_data, t);
+
+	return 0;
+}
+
+static const struct rtc_class_ops bd70528_rtc_ops = {
+	.read_time	= bd70528_get_time,
+	.set_time	= bd70528_set_time,
+	.read_alarm	= bd70528_read_alarm,
+	.set_alarm	= bd70528_set_alarm,
+};
+
+static irqreturn_t alm_hndlr(int irq, void *data)
+{
+	struct rtc_device *rtc = data;
+
+	rtc_update_irq(rtc, 1, RTC_IRQF | RTC_AF | RTC_PF);
+	return IRQ_HANDLED;
+}
+
+static int bd70528_probe(struct platform_device *pdev)
+{
+	struct bd70528_rtc *bd_rtc;
+	struct bd70528 *mfd;
+	int ret;
+	struct rtc_device *rtc;
+	int irq;
+	unsigned int hr;
+
+	mfd = dev_get_drvdata(pdev->dev.parent);
+	if (!mfd) {
+		dev_err(&pdev->dev, "No MFD driver data\n");
+		return -EINVAL;
+	}
+	bd_rtc = devm_kzalloc(&pdev->dev, sizeof(*bd_rtc), GFP_KERNEL);
+	if (!bd_rtc)
+		return -ENOMEM;
+
+	bd_rtc->mfd = mfd;
+	bd_rtc->dev = &pdev->dev;
+
+	irq = platform_get_irq_byname(pdev, "bd70528-rtc-alm");
+
+	if (irq < 0) {
+		dev_err(&pdev->dev, "Failed to get irq\n");
+		return irq;
+	}
+
+	platform_set_drvdata(pdev, bd_rtc);
+
+	ret = regmap_read(mfd->chip.regmap, BD70528_REG_RTC_HOUR, &hr);
+
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to reag RTC clock\n");
+		return ret;
+	}
+
+	if (!(hr & BD70528_MASK_RTC_HOUR_24H)) {
+		struct rtc_time t;
+
+		ret = bd70528_get_time(&pdev->dev, &t);
+		if (ret)
+			return ret;
+
+		ret = bd70528_set_time(&pdev->dev, &t);
+		if (ret)
+			return ret;
+	}
+
+	if (ret) {
+		dev_err(&pdev->dev, "Setting 24H clock for RTC failed\n");
+		return ret;
+	}
+
+	device_set_wakeup_capable(&pdev->dev, true);
+	device_wakeup_enable(&pdev->dev);
+	rtc = devm_rtc_device_register(&pdev->dev, "bd70528-rtc",
+				       &bd70528_rtc_ops, THIS_MODULE);
+	if (IS_ERR(rtc)) {
+		dev_err(&pdev->dev, "Registering RTC failed\n");
+		return PTR_ERR(rtc);
+	}
+
+	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, &alm_hndlr,
+					IRQF_ONESHOT, "bd70528-rtc", rtc);
+
+	/*
+	 *  BD70528 irq controller is not touching the main mask register.
+	 *  So enable the RTC block interrupts at main level. We can just
+	 *  leave them enabled as irq-controller should disable irqs
+	 *  from sub-registers when IRQ is disabled or freed.
+	 */
+	ret = regmap_update_bits(mfd->chip.regmap,
+				 BD70528_REG_INT_MAIN_MASK,
+				 BD70528_INT_RTC_MASK, 0);
+
+	if (ret)
+		dev_err(&pdev->dev, "Failed to enable RTC interrupts\n");
+
+	return ret;
+}
+
+static struct platform_driver bd70528_rtc = {
+	.driver = {
+		.name = "bd70528-rtc"
+	},
+	.probe = bd70528_probe,
+};
+
+module_platform_driver(bd70528_rtc);
+
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_DESCRIPTION("BD70528 RTC driver");
+MODULE_LICENSE("GPL");
-- 
2.14.3


-- 
Matti Vaittinen
ROHM Semiconductors

~~~ "I don't think so," said Rene Descartes.  Just then, he vanished ~~~

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH v2 08/10] rtc: bd70528: Initial support for ROHM bd70528 RTC
  2019-01-25 11:05 [RFC PATCH v2 08/10] rtc: bd70528: Initial support for ROHM bd70528 RTC Matti Vaittinen
@ 2019-01-26 16:30 ` Guenter Roeck
  2019-01-28  7:48   ` Matti Vaittinen
  2019-01-28 20:26   ` Jerry Hoemann
  2019-01-28 21:20 ` Alexandre Belloni
  1 sibling, 2 replies; 10+ messages in thread
From: Guenter Roeck @ 2019-01-26 16:30 UTC (permalink / raw)
  To: Matti Vaittinen, mazziesaccount
  Cc: heikki.haikola, mikko.mutanen, lee.jones, robh+dt, mark.rutland,
	broonie, gregkh, rafael, mturquette, sboyd, linus.walleij,
	bgolaszewski, sre, lgirdwood, a.zummo, alexandre.belloni, wim,
	devicetree, linux-kernel, linux-clk, linux-gpio, linux-pm,
	linux-rtc, linux-watchdog

On 1/25/19 3:05 AM, Matti Vaittinen wrote:
> Support RTC block in ROHM bd70528 power management IC. Support
> getting and setting the time and date as well as arming an alarm
> which can also be used to wake the PMIC from standby state.
> 
> HW supports wake interrupt only for the next 24 hours (sec, minute
> and hour information only) so we limit also the alarm interrupt to
> this 24 hours for the sake of consistency.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
>   drivers/rtc/Kconfig       |   8 +
>   drivers/rtc/Makefile      |   1 +
>   drivers/rtc/rtc-bd70528.c | 451 ++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 460 insertions(+)
>   create mode 100644 drivers/rtc/rtc-bd70528.c
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 225b0b8516f3..df6211cbd83f 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -487,6 +487,14 @@ config RTC_DRV_M41T80_WDT
>   	help
>   	  If you say Y here you will get support for the
>   	  watchdog timer in the ST M41T60 and M41T80 RTC chips series.
> +config RTC_DRV_BD70528
> +	tristate "ROHM BD70528 PMIC RTC"
> +	help
> +	  If you say Y here you will get support for the RTC
> +	  on ROHM BD70528 Power Management IC.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called rtc-bd70528.
>   
>   config RTC_DRV_BQ32K
>   	tristate "TI BQ32000"
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index df022d820bee..740b13840913 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -36,6 +36,7 @@ obj-$(CONFIG_RTC_DRV_ASM9260)	+= rtc-asm9260.o
>   obj-$(CONFIG_RTC_DRV_AT91RM9200)+= rtc-at91rm9200.o
>   obj-$(CONFIG_RTC_DRV_AT91SAM9)	+= rtc-at91sam9.o
>   obj-$(CONFIG_RTC_DRV_AU1XXX)	+= rtc-au1xxx.o
> +obj-$(CONFIG_RTC_DRV_BD70528)	+= rtc-bd70528.o
>   obj-$(CONFIG_RTC_DRV_BQ32K)	+= rtc-bq32k.o
>   obj-$(CONFIG_RTC_DRV_BQ4802)	+= rtc-bq4802.o
>   obj-$(CONFIG_RTC_DRV_BRCMSTB)	+= rtc-brcmstb-waketimer.o
> diff --git a/drivers/rtc/rtc-bd70528.c b/drivers/rtc/rtc-bd70528.c
> new file mode 100644
> index 000000000000..250fb218c784
> --- /dev/null
> +++ b/drivers/rtc/rtc-bd70528.c
> @@ -0,0 +1,451 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +//
> +// Copyright (C) 2018 ROHM Semiconductors
> +//
> +// RTC driver for ROHM BD70528 PMIC
> +
> +#include <linux/bcd.h>
> +#include <linux/mfd/rohm-bd70528.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/rtc.h>
> +
> +/*
> + * We read regs RTC_SEC => RTC_YEAR
> + * this struct is ordered according to chip registers.
> + * Keep it u8 only to avoid padding issues.
> + */
> +struct bd70528_rtc_day {
> +	u8 sec;
> +	u8 min;
> +	u8 hour;
> +};
> +
> +struct bd70528_rtc_data {
> +	struct bd70528_rtc_day time;
> +	u8 week;
> +	u8 day;
> +	u8 month;
> +	u8 year;
> +};
> +
> +struct bd70528_rtc_wake {
> +	struct bd70528_rtc_day time;
> +	u8 ctrl;
> +};
> +
> +struct bd70528_rtc_alm {
> +	struct bd70528_rtc_data data;
> +	u8 alm_mask;
> +	u8 alm_repeat;
> +};

At least some of the above are directly associated with chip registers.
I don't think this will work for all architectures without explicit packed
attribute.

> +
> +struct bd70528_rtc {
> +	struct bd70528 *mfd;
> +	struct device *dev;
> +};
> +
> +static int bd70528_set_wake(struct bd70528 *bd70528,
> +			    int enable, int *old_state)
> +{
> +	int ret;
> +	unsigned int ctrl_reg;
> +
> +	ret = regmap_read(bd70528->chip.regmap, BD70528_REG_WAKE_EN, &ctrl_reg);
> +	if (ret)
> +		return ret;
> +
> +	if (old_state) {
> +		if (ctrl_reg & BD70528_MASK_WAKE_EN)
> +			*old_state |= BD70528_WAKE_STATE_BIT;
> +		else
> +			*old_state &= ~BD70528_WAKE_STATE_BIT;
> +
> +		if ((!enable) == (!(*old_state & BD70528_WAKE_STATE_BIT)))
> +			return 0;

I think
		if (enable == !!(*old_state & BD70528_WAKE_STATE_BIT))
would be much better readable. Even if not, there are way too many ()
in the above conditional.


> +	}
> +
> +	if (enable)
> +		ctrl_reg |= BD70528_MASK_WAKE_EN;
> +	else
> +		ctrl_reg &= ~BD70528_MASK_WAKE_EN;
> +
> +	return regmap_write(bd70528->chip.regmap, BD70528_REG_WAKE_EN,
> +			    ctrl_reg);
> +}
> +
> +static int bd70528_set_elapsed_tmr(struct bd70528 *bd70528,
> +				   int enable, int *old_state)
> +{
> +	int ret;
> +	unsigned int ctrl_reg;
> +
> +	/*
> +	 * TBD
> +	 * What is the purpose of elapsed timer ?
> +	 * Is the timeout registers counting down, or is the disable - re-enable
> +	 * going to restart the elapsed-time counting? If counting is restarted
> +	 * the timeout should be decreased by the amount of time that has
> +	 * elapsed since starting the timer. Maybe we should store the monotonic
> +	 * clock value when timer is started so that if RTC is set while timer
> +	 * is armed we could do the compensation. This is a hack if RTC/system
> +	 * clk are drifting. OTOH, RTC controlled via I2C is in any case
> +	 * inaccurate...
> +	 */
> +	ret = regmap_read(bd70528->chip.regmap, BD70528_REG_ELAPSED_TIMER_EN,
> +			  &ctrl_reg);
> +	if (ret)
> +		return ret;
> +
> +	if (old_state) {
> +		if (ctrl_reg & BD70528_MASK_ELAPSED_TIMER_EN)
> +			*old_state |= BD70528_ELAPSED_STATE_BIT;
> +		else
> +			*old_state &= ~BD70528_ELAPSED_STATE_BIT;
> +
> +		if ((!enable) == (!(*old_state & BD70528_ELAPSED_STATE_BIT)))
> +			return 0;
> +	}
> +
> +	if (enable)
> +		ctrl_reg |= BD70528_MASK_ELAPSED_TIMER_EN;
> +	else
> +		ctrl_reg &= ~BD70528_MASK_ELAPSED_TIMER_EN;
> +
> +	return regmap_write(bd70528->chip.regmap, BD70528_REG_ELAPSED_TIMER_EN,
> +			    ctrl_reg);
> +}
> +
> +static int bd70528_set_rtc_based_timers(struct bd70528_rtc *r, int new_state,
> +							int *old_state)
> +{
> +	int ret;
> +
> +	ret = r->mfd->wdt_set(r->mfd, new_state & BD70528_WDT_STATE_BIT,
> +			       old_state);
> +	if (ret) {
> +		dev_err(r->dev,
> +			"Failed to disable WDG for RTC setting (%d)\n", ret);
> +		return ret;
> +	}
> +	ret = bd70528_set_elapsed_tmr(r->mfd,
> +				      new_state & BD70528_ELAPSED_STATE_BIT,
> +				      old_state);
> +	if (ret) {
> +		dev_err(r->dev,
> +			"Failed to disable 'elapsed timer' for RTC setting\n");
> +		return ret;
> +	}
> +	ret = bd70528_set_wake(r->mfd, new_state & BD70528_WAKE_STATE_BIT,
> +			       old_state);
> +	if (ret) {
> +		dev_err(r->dev,
> +			"Failed to disable 'wake timer' for RTC setting\n");
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static int bd70528_re_enable_rtc_based_timers(struct bd70528_rtc *r,
> +							int old_state)
> +{
> +	int ret;
> +
> +	ret = bd70528_set_rtc_based_timers(r, old_state, NULL);
> +	mutex_unlock(&r->mfd->rtc_timer_lock);
> +
> +	return ret;
> +}
> +
> +static int bd70528_disable_rtc_based_timers(struct bd70528_rtc *r,
> +							int *old_state)
> +{
> +	mutex_lock(&r->mfd->rtc_timer_lock);
> +
> +	return bd70528_set_rtc_based_timers(r, 0, old_state);
> +}
> +
> +static inline void tmday2rtc(struct rtc_time *t, struct bd70528_rtc_day *d)
> +{
> +	d->sec &= ~BD70528_MASK_RTC_SEC;
> +	d->min &= ~BD70528_MASK_RTC_MINUTE;
> +	d->hour &= ~BD70528_MASK_RTC_HOUR;
> +	d->sec |= bin2bcd(t->tm_sec);
> +	d->min |= bin2bcd(t->tm_min);
> +	d->hour |= bin2bcd(t->tm_hour);
> +}
> +
> +static inline void tm2rtc(struct rtc_time *t, struct bd70528_rtc_data *r)
> +{
> +	r->day &= ~BD70528_MASK_RTC_DAY;
> +	r->week &= ~BD70528_MASK_RTC_WEEK;
> +	r->month &= ~BD70528_MASK_RTC_MONTH;
> +	/*
> +	 * PM and 24H bits are not used by Wake - thus we clear them
> +	 * here and not in tmday2rtc() which is also used by wake.
> +	 */
> +	r->time.hour &= ~(BD70528_MASK_RTC_HOUR_PM | BD70528_MASK_RTC_HOUR_24H);
> +
> +	tmday2rtc(t, &r->time);
> +	/*
> +	 * We do always set time in 24H mode.
> +	 */
> +	r->time.hour |= BD70528_MASK_RTC_HOUR_24H;
> +	r->day |= bin2bcd(t->tm_mday);
> +	r->week |= bin2bcd(t->tm_wday);
> +	r->month |= bin2bcd(t->tm_mon + 1);
> +	r->year = bin2bcd(t->tm_year-100);
> +}
> +
> +static inline void rtc2tm(struct bd70528_rtc_data *r, struct rtc_time *t)
> +{
> +	t->tm_sec = bcd2bin(r->time.sec & BD70528_MASK_RTC_SEC);
> +	t->tm_min = bcd2bin(r->time.min & BD70528_MASK_RTC_MINUTE);
> +	t->tm_hour = bcd2bin(r->time.hour & BD70528_MASK_RTC_HOUR);
> +	/*
> +	 * If RTC is in 12H mode, then bit BD70528_MASK_RTC_HOUR_PM
> +	 * is not BCD value but tells whether it is AM or PM
> +	 */
> +	if (!(r->time.hour & BD70528_MASK_RTC_HOUR_24H)) {
> +		t->tm_hour %= 12;
> +		if (r->time.hour & BD70528_MASK_RTC_HOUR_PM)
> +			t->tm_hour += 12;
> +	}
> +	t->tm_mday = bcd2bin(r->day & BD70528_MASK_RTC_DAY);
> +	t->tm_mon = bcd2bin(r->month & BD70528_MASK_RTC_MONTH) - 1;
> +	t->tm_year = 100 + bcd2bin(r->year & BD70528_MASK_RTC_YEAR);
> +	t->tm_wday = bcd2bin(r->week & BD70528_MASK_RTC_WEEK);
> +}
> +
> +static int bd70528_set_alarm(struct device *dev, struct rtc_wkalrm *a)
> +{
> +	struct bd70528_rtc_wake wake;
> +	struct bd70528_rtc_alm alm;
> +	int ret;
> +	struct bd70528_rtc *r = dev_get_drvdata(dev);
> +	struct bd70528 *bd70528 = r->mfd;
> +
> +	ret = regmap_bulk_read(bd70528->chip.regmap, BD70528_REG_RTC_WAKE_START,
> +			       &wake, sizeof(wake));
> +	if (ret) {
> +		dev_err(dev, "Failed to read wake regs\n");
> +		return ret;
> +	}
> +
> +	ret = regmap_bulk_read(bd70528->chip.regmap, BD70528_REG_RTC_ALM_START,
> +			       &alm, sizeof(alm));
> +	if (ret) {
> +		dev_err(dev, "Failed to read alarm regs\n");
> +		return ret;
> +	}
> +
> +	tm2rtc(&a->time, &alm.data);
> +	tmday2rtc(&a->time, &wake.time);
> +
> +	if (a->enabled) {
> +		alm.alm_mask &= ~BD70528_MASK_ALM_EN;
> +		wake.ctrl |= BD70528_MASK_WAKE_EN;
> +	} else {
> +		alm.alm_mask |= BD70528_MASK_ALM_EN;
> +		wake.ctrl &= ~BD70528_MASK_WAKE_EN;
> +	}
> +
> +	ret = regmap_bulk_write(bd70528->chip.regmap,
> +				BD70528_REG_RTC_WAKE_START, &wake,
> +				sizeof(wake));
> +	if (ret) {
> +		dev_err(dev, "Failed to set wake time\n");
> +		return ret;
> +	}
> +	ret = regmap_bulk_write(bd70528->chip.regmap, BD70528_REG_RTC_ALM_START,
> +				&alm, sizeof(alm));
> +	if (ret)
> +		dev_err(dev, "Failed to set alarm time\n");
> +
> +	return ret;
> +}
> +
> +static int bd70528_read_alarm(struct device *dev, struct rtc_wkalrm *a)
> +{
> +	struct bd70528_rtc_alm alm;
> +	int ret;
> +	struct bd70528_rtc *r = dev_get_drvdata(dev);
> +	struct bd70528 *bd70528 = r->mfd;
> +
> +	ret = regmap_bulk_read(bd70528->chip.regmap, BD70528_REG_RTC_ALM_START,
> +			  &alm, sizeof(alm));
> +	if (ret) {
> +		dev_err(dev, "Failed to read alarm regs\n");
> +		return ret;
> +	}
> +
> +	rtc2tm(&alm.data, &a->time);
> +	a->time.tm_mday = -1;
> +	a->time.tm_mon = -1;
> +	a->time.tm_year = -1;
> +	if (alm.alm_mask & BD70528_MASK_ALM_EN)
> +		a->enabled = 0;
> +	else
> +		a->enabled = 1;
> +
Without conditional:
	a->enabled = !(alm.alm_mask & BD70528_MASK_ALM_EN);

> +	a->pending = 0;
> +
> +	return 0;
> +}
> +
> +static int bd70528_set_time(struct device *dev, struct rtc_time *t)
> +{
> +	int ret, old_states;
> +	struct bd70528_rtc_data rtc_data;
> +	struct bd70528_rtc *r = dev_get_drvdata(dev);
> +	struct bd70528 *bd70528 = r->mfd;
> +
> +	ret = bd70528_disable_rtc_based_timers(r, &old_states);
> +

AFAICS the disable/enable functions are only called once. Since they
also apply set / clear a mutex, I find that a bit confusing. I think
it would be better to fold the code into this function. If anything,
I could imagine something like

	mutex_lock();
	ret = bd70528_set_time_locked();
	mutex_unlock()

to simplify error handling.


> +	ret = regmap_bulk_read(bd70528->chip.regmap,
> +			       BD70528_REG_RTC_START, &rtc_data,
> +			       sizeof(rtc_data));
> +
> +	tm2rtc(t, &rtc_data);
> +
> +	ret = regmap_bulk_write(bd70528->chip.regmap,
> +				BD70528_REG_RTC_START, &rtc_data,
> +				sizeof(rtc_data));
> +
> +	ret = bd70528_re_enable_rtc_based_timers(r, old_states);
> +

Kind of off that all the error returns are ignored here.

> +	return 0;
> +}
> +static int bd70528_get_time(struct device *dev, struct rtc_time *t)
> +{
> +	struct bd70528_rtc *r = dev_get_drvdata(dev);
> +	struct bd70528 *bd70528 = r->mfd;
> +	struct bd70528_rtc_data rtc_data;
> +	int ret;
> +
> +	/* read the RTC date and time registers all at once */
> +	ret = regmap_bulk_read(bd70528->chip.regmap,
> +			       BD70528_REG_RTC_START, &rtc_data,
> +			       sizeof(rtc_data));
> +	if (ret) {
> +		dev_err(dev, "Failed to read RTC time (err %d)\n", ret);
> +		return ret;
> +	}
> +
> +	rtc2tm(&rtc_data, t);
> +
> +	return 0;
> +}
> +
> +static const struct rtc_class_ops bd70528_rtc_ops = {
> +	.read_time	= bd70528_get_time,
> +	.set_time	= bd70528_set_time,
> +	.read_alarm	= bd70528_read_alarm,
> +	.set_alarm	= bd70528_set_alarm,
> +};
> +
> +static irqreturn_t alm_hndlr(int irq, void *data)
> +{
> +	struct rtc_device *rtc = data;
> +
> +	rtc_update_irq(rtc, 1, RTC_IRQF | RTC_AF | RTC_PF);
> +	return IRQ_HANDLED;
> +}
> +
> +static int bd70528_probe(struct platform_device *pdev)
> +{
> +	struct bd70528_rtc *bd_rtc;
> +	struct bd70528 *mfd;
> +	int ret;
> +	struct rtc_device *rtc;
> +	int irq;
> +	unsigned int hr;
> +
> +	mfd = dev_get_drvdata(pdev->dev.parent);
> +	if (!mfd) {
> +		dev_err(&pdev->dev, "No MFD driver data\n");
> +		return -EINVAL;
> +	}
> +	bd_rtc = devm_kzalloc(&pdev->dev, sizeof(*bd_rtc), GFP_KERNEL);
> +	if (!bd_rtc)
> +		return -ENOMEM;
> +
> +	bd_rtc->mfd = mfd;
> +	bd_rtc->dev = &pdev->dev;
> +
> +	irq = platform_get_irq_byname(pdev, "bd70528-rtc-alm");
> +
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "Failed to get irq\n");
> +		return irq;
> +	}
> +
> +	platform_set_drvdata(pdev, bd_rtc);
> +
> +	ret = regmap_read(mfd->chip.regmap, BD70528_REG_RTC_HOUR, &hr);
> +
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to reag RTC clock\n");
> +		return ret;
> +	}
> +
> +	if (!(hr & BD70528_MASK_RTC_HOUR_24H)) {
> +		struct rtc_time t;
> +
> +		ret = bd70528_get_time(&pdev->dev, &t);
> +		if (ret)
> +			return ret;
> +
> +		ret = bd70528_set_time(&pdev->dev, &t);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (ret) {
> +		dev_err(&pdev->dev, "Setting 24H clock for RTC failed\n");
> +		return ret;
> +	}
> +
> +	device_set_wakeup_capable(&pdev->dev, true);
> +	device_wakeup_enable(&pdev->dev);
> +	rtc = devm_rtc_device_register(&pdev->dev, "bd70528-rtc",
> +				       &bd70528_rtc_ops, THIS_MODULE);
> +	if (IS_ERR(rtc)) {
> +		dev_err(&pdev->dev, "Registering RTC failed\n");
> +		return PTR_ERR(rtc);
> +	}
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, &alm_hndlr,
> +					IRQF_ONESHOT, "bd70528-rtc", rtc);
> +
> +	/*
> +	 *  BD70528 irq controller is not touching the main mask register.
> +	 *  So enable the RTC block interrupts at main level. We can just
> +	 *  leave them enabled as irq-controller should disable irqs
> +	 *  from sub-registers when IRQ is disabled or freed.
> +	 */
> +	ret = regmap_update_bits(mfd->chip.regmap,
> +				 BD70528_REG_INT_MAIN_MASK,
> +				 BD70528_INT_RTC_MASK, 0);
> +
> +	if (ret)
> +		dev_err(&pdev->dev, "Failed to enable RTC interrupts\n");
> +
> +	return ret;
> +}
> +
> +static struct platform_driver bd70528_rtc = {
> +	.driver = {
> +		.name = "bd70528-rtc"
> +	},
> +	.probe = bd70528_probe,
> +};
> +
> +module_platform_driver(bd70528_rtc);
> +
> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> +MODULE_DESCRIPTION("BD70528 RTC driver");
> +MODULE_LICENSE("GPL");
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH v2 08/10] rtc: bd70528: Initial support for ROHM bd70528 RTC
  2019-01-26 16:30 ` Guenter Roeck
@ 2019-01-28  7:48   ` Matti Vaittinen
  2019-01-28 14:02     ` Guenter Roeck
  2019-01-28 20:26   ` Jerry Hoemann
  1 sibling, 1 reply; 10+ messages in thread
From: Matti Vaittinen @ 2019-01-28  7:48 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: mazziesaccount, heikki.haikola, mikko.mutanen, lee.jones,
	robh+dt, mark.rutland, broonie, gregkh, rafael, mturquette,
	sboyd, linus.walleij, bgolaszewski, sre, lgirdwood, a.zummo,
	alexandre.belloni, wim, devicetree, linux-kernel, linux-clk,
	linux-gpio, linux-pm, linux-rtc, linux-watchdog

Thanks again Guenter,

On Sat, Jan 26, 2019 at 08:30:24AM -0800, Guenter Roeck wrote:
> On 1/25/19 3:05 AM, Matti Vaittinen wrote:
> > +/*
> > + * We read regs RTC_SEC => RTC_YEAR
> > + * this struct is ordered according to chip registers.
> > + * Keep it u8 only to avoid padding issues.
> > + */
> > +struct bd70528_rtc_day {
> > +	u8 sec;
> > +	u8 min;
> > +	u8 hour;
> > +};
> > +
> > +struct bd70528_rtc_data {
> > +	struct bd70528_rtc_day time;
> > +	u8 week;
> > +	u8 day;
> > +	u8 month;
> > +	u8 year;
> > +};
> > +
> > +struct bd70528_rtc_wake {
> > +	struct bd70528_rtc_day time;
> > +	u8 ctrl;
> > +};
> > +
> > +struct bd70528_rtc_alm {
> > +	struct bd70528_rtc_data data;
> > +	u8 alm_mask;
> > +	u8 alm_repeat;
> > +};
> 
> At least some of the above are directly associated with chip registers.
> I don't think this will work for all architectures without explicit packed
> attribute.

Allright. I was thinking of that but thought that most of the
architectures using this PMIC would handle alignments fine if I used
only u8 members. I did consider using __attribute__((packed)) - but I'm
not sure if we hit into troubles with that too. I guess some people
would like to compile kernel with other compiler(s) but gcc - although
I'm not sure if this should be taken into account. I'll try doing some
study on this - unless someone replies to this and just tells how this
should be done. (I am pretty sure I can find the answer from mail
archives though). I'll try adding some packing hint for compiler at v3.

> > +		if ((!enable) == (!(*old_state & BD70528_WAKE_STATE_BIT)))
> > +			return 0;
> 
> I think
> 		if (enable == !!(*old_state & BD70528_WAKE_STATE_BIT))
> would be much better readable. Even if not, there are way too many ()
> in the above conditional.

Allright. I'll fix this
 
> > +	if (alm.alm_mask & BD70528_MASK_ALM_EN)
> > +		a->enabled = 0;
> > +	else
> > +		a->enabled = 1;
> > +
> Without conditional:
> 	a->enabled = !(alm.alm_mask & BD70528_MASK_ALM_EN);
> 

Right. Much nicer, thanks! I'll change this.

> > +static int bd70528_set_time(struct device *dev, struct rtc_time *t)
> > +{
> > +	int ret, old_states;
> > +	struct bd70528_rtc_data rtc_data;
> > +	struct bd70528_rtc *r = dev_get_drvdata(dev);
> > +	struct bd70528 *bd70528 = r->mfd;
> > +
> > +	ret = bd70528_disable_rtc_based_timers(r, &old_states);
> > +
> 
> AFAICS the disable/enable functions are only called once. Since they
> also apply set / clear a mutex, I find that a bit confusing. I think
> it would be better to fold the code into this function. If anything,
> I could imagine something like
> 
> 	mutex_lock();
> 	ret = bd70528_set_time_locked();
> 	mutex_unlock()
> 
> to simplify error handling.

Yep. Makes sense. I'll tidy this.

> > +	ret = regmap_bulk_read(bd70528->chip.regmap,
> > +			       BD70528_REG_RTC_START, &rtc_data,
> > +			       sizeof(rtc_data));
> > +
> > +	tm2rtc(t, &rtc_data);
> > +
> > +	ret = regmap_bulk_write(bd70528->chip.regmap,
> > +				BD70528_REG_RTC_START, &rtc_data,
> > +				sizeof(rtc_data));
> > +
> > +	ret = bd70528_re_enable_rtc_based_timers(r, old_states);
> > +
> 
> Kind of off that all the error returns are ignored here.

And I'll fix this too.

Br,
	Matti Vaittinen

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH v2 08/10] rtc: bd70528: Initial support for ROHM bd70528 RTC
  2019-01-28  7:48   ` Matti Vaittinen
@ 2019-01-28 14:02     ` Guenter Roeck
  2019-01-28 16:16       ` Matti Vaittinen
  0 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2019-01-28 14:02 UTC (permalink / raw)
  To: Matti Vaittinen
  Cc: mazziesaccount, heikki.haikola, mikko.mutanen, lee.jones,
	robh+dt, mark.rutland, broonie, gregkh, rafael, mturquette,
	sboyd, linus.walleij, bgolaszewski, sre, lgirdwood, a.zummo,
	alexandre.belloni, wim, devicetree, linux-kernel, linux-clk,
	linux-gpio, linux-pm, linux-rtc, linux-watchdog

On 1/27/19 11:48 PM, Matti Vaittinen wrote:
> Thanks again Guenter,
> 
> On Sat, Jan 26, 2019 at 08:30:24AM -0800, Guenter Roeck wrote:
>> On 1/25/19 3:05 AM, Matti Vaittinen wrote:
>>> +/*
>>> + * We read regs RTC_SEC => RTC_YEAR
>>> + * this struct is ordered according to chip registers.
>>> + * Keep it u8 only to avoid padding issues.
>>> + */
>>> +struct bd70528_rtc_day {
>>> +	u8 sec;
>>> +	u8 min;
>>> +	u8 hour;
>>> +};
>>> +
>>> +struct bd70528_rtc_data {
>>> +	struct bd70528_rtc_day time;
>>> +	u8 week;
>>> +	u8 day;
>>> +	u8 month;
>>> +	u8 year;
>>> +};
>>> +
>>> +struct bd70528_rtc_wake {
>>> +	struct bd70528_rtc_day time;
>>> +	u8 ctrl;
>>> +};
>>> +
>>> +struct bd70528_rtc_alm {
>>> +	struct bd70528_rtc_data data;
>>> +	u8 alm_mask;
>>> +	u8 alm_repeat;
>>> +};
>>
>> At least some of the above are directly associated with chip registers.
>> I don't think this will work for all architectures without explicit packed
>> attribute.
> 
> Allright. I was thinking of that but thought that most of the
> architectures using this PMIC would handle alignments fine if I used
> only u8 members. I did consider using __attribute__((packed)) - but I'm
> not sure if we hit into troubles with that too. I guess some people
> would like to compile kernel with other compiler(s) but gcc - although
> I'm not sure if this should be taken into account. I'll try doing some
> study on this - unless someone replies to this and just tells how this
> should be done. (I am pretty sure I can find the answer from mail
> archives though). I'll try adding some packing hint for compiler at v3.
> 

Use __packed ?

>>> +		if ((!enable) == (!(*old_state & BD70528_WAKE_STATE_BIT)))
>>> +			return 0;
>>
>> I think
>> 		if (enable == !!(*old_state & BD70528_WAKE_STATE_BIT))
>> would be much better readable. Even if not, there are way too many ()
>> in the above conditional.
> 
> Allright. I'll fix this
>   
>>> +	if (alm.alm_mask & BD70528_MASK_ALM_EN)
>>> +		a->enabled = 0;
>>> +	else
>>> +		a->enabled = 1;
>>> +
>> Without conditional:
>> 	a->enabled = !(alm.alm_mask & BD70528_MASK_ALM_EN);
>>
> 
> Right. Much nicer, thanks! I'll change this.
> 
>>> +static int bd70528_set_time(struct device *dev, struct rtc_time *t)
>>> +{
>>> +	int ret, old_states;
>>> +	struct bd70528_rtc_data rtc_data;
>>> +	struct bd70528_rtc *r = dev_get_drvdata(dev);
>>> +	struct bd70528 *bd70528 = r->mfd;
>>> +
>>> +	ret = bd70528_disable_rtc_based_timers(r, &old_states);
>>> +
>>
>> AFAICS the disable/enable functions are only called once. Since they
>> also apply set / clear a mutex, I find that a bit confusing. I think
>> it would be better to fold the code into this function. If anything,
>> I could imagine something like
>>
>> 	mutex_lock();
>> 	ret = bd70528_set_time_locked();
>> 	mutex_unlock()
>>
>> to simplify error handling.
> 
> Yep. Makes sense. I'll tidy this.
> 
>>> +	ret = regmap_bulk_read(bd70528->chip.regmap,
>>> +			       BD70528_REG_RTC_START, &rtc_data,
>>> +			       sizeof(rtc_data));
>>> +
>>> +	tm2rtc(t, &rtc_data);
>>> +
>>> +	ret = regmap_bulk_write(bd70528->chip.regmap,
>>> +				BD70528_REG_RTC_START, &rtc_data,
>>> +				sizeof(rtc_data));
>>> +
>>> +	ret = bd70528_re_enable_rtc_based_timers(r, old_states);
>>> +
>>
>> Kind of off that all the error returns are ignored here.
> 
> And I'll fix this too.
> 
> Br,
> 	Matti Vaittinen
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH v2 08/10] rtc: bd70528: Initial support for ROHM bd70528 RTC
  2019-01-28 14:02     ` Guenter Roeck
@ 2019-01-28 16:16       ` Matti Vaittinen
  0 siblings, 0 replies; 10+ messages in thread
From: Matti Vaittinen @ 2019-01-28 16:16 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: mazziesaccount, heikki.haikola, mikko.mutanen, lee.jones,
	robh+dt, mark.rutland, broonie, gregkh, rafael, mturquette,
	sboyd, linus.walleij, bgolaszewski, sre, lgirdwood, a.zummo,
	alexandre.belloni, wim, devicetree, linux-kernel, linux-clk,
	linux-gpio, linux-pm, linux-rtc, linux-watchdog

On Mon, Jan 28, 2019 at 06:02:47AM -0800, Guenter Roeck wrote:
> On 1/27/19 11:48 PM, Matti Vaittinen wrote:
> > Thanks again Guenter,
> > 
> > On Sat, Jan 26, 2019 at 08:30:24AM -0800, Guenter Roeck wrote:
> > > On 1/25/19 3:05 AM, Matti Vaittinen wrote:
> > > > +/*
> > > > + * We read regs RTC_SEC => RTC_YEAR
> > > > + * this struct is ordered according to chip registers.
> > > > + * Keep it u8 only to avoid padding issues.
> > > > + */
> > > > +struct bd70528_rtc_day {
> > > > +	u8 sec;
> > > > +	u8 min;
> > > > +	u8 hour;
> > > > +};
> > > > +
> > > > +struct bd70528_rtc_data {
> > > > +	struct bd70528_rtc_day time;
> > > > +	u8 week;
> > > > +	u8 day;
> > > > +	u8 month;
> > > > +	u8 year;
> > > > +};
> > > > +
> > > > +struct bd70528_rtc_wake {
> > > > +	struct bd70528_rtc_day time;
> > > > +	u8 ctrl;
> > > > +};
> > > > +
> > > > +struct bd70528_rtc_alm {
> > > > +	struct bd70528_rtc_data data;
> > > > +	u8 alm_mask;
> > > > +	u8 alm_repeat;
> > > > +};
> > > 
> > > At least some of the above are directly associated with chip registers.
> > > I don't think this will work for all architectures without explicit packed
> > > attribute.
> > 
> > Allright. I was thinking of that but thought that most of the
> > architectures using this PMIC would handle alignments fine if I used
> > only u8 members. I did consider using __attribute__((packed)) - but I'm
> > not sure if we hit into troubles with that too. I guess some people
> > would like to compile kernel with other compiler(s) but gcc - although
> > I'm not sure if this should be taken into account. I'll try doing some
> > study on this - unless someone replies to this and just tells how this
> > should be done. (I am pretty sure I can find the answer from mail
> > archives though). I'll try adding some packing hint for compiler at v3.
> > 
> 
> Use __packed ?

Right. That appends to __attribute__((packed)) on gcc. I'll use that.
Thanks for the tip :)

Br,
	Matti Vaittinen

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH v2 08/10] rtc: bd70528: Initial support for ROHM bd70528 RTC
  2019-01-26 16:30 ` Guenter Roeck
  2019-01-28  7:48   ` Matti Vaittinen
@ 2019-01-28 20:26   ` Jerry Hoemann
  2019-01-29  7:01     ` Matti Vaittinen
  1 sibling, 1 reply; 10+ messages in thread
From: Jerry Hoemann @ 2019-01-28 20:26 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Matti Vaittinen, mazziesaccount, heikki.haikola, mikko.mutanen,
	lee.jones, robh+dt, mark.rutland, broonie, gregkh, rafael,
	mturquette, sboyd, linus.walleij, bgolaszewski, sre, lgirdwood,
	a.zummo, alexandre.belloni, wim, devicetree, linux-kernel,
	linux-clk, linux-gpio, linux-pm, linux-rtc, linux-watchdog

On Sat, Jan 26, 2019 at 08:30:24AM -0800, Guenter Roeck wrote:
> On 1/25/19 3:05 AM, Matti Vaittinen wrote:



> > +static int bd70528_set_wake(struct bd70528 *bd70528,
> > +			    int enable, int *old_state)
> > +{
> > +	int ret;
> > +	unsigned int ctrl_reg;
> > +
> > +	ret = regmap_read(bd70528->chip.regmap, BD70528_REG_WAKE_EN, &ctrl_reg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (old_state) {
> > +		if (ctrl_reg & BD70528_MASK_WAKE_EN)
> > +			*old_state |= BD70528_WAKE_STATE_BIT;
> > +		else
> > +			*old_state &= ~BD70528_WAKE_STATE_BIT;
> > +
> > +		if ((!enable) == (!(*old_state & BD70528_WAKE_STATE_BIT)))
> > +			return 0;
> 
> I think
> 		if (enable == !!(*old_state & BD70528_WAKE_STATE_BIT))
> would be much better readable. Even if not, there are way too many ()
> in the above conditional.
> 

The substitution is not equivalent to original.  I think you mean:

 		if (!!enable == !!(*old_state & BD70528_WAKE_STATE_BIT))



-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH v2 08/10] rtc: bd70528: Initial support for ROHM bd70528 RTC
  2019-01-25 11:05 [RFC PATCH v2 08/10] rtc: bd70528: Initial support for ROHM bd70528 RTC Matti Vaittinen
  2019-01-26 16:30 ` Guenter Roeck
@ 2019-01-28 21:20 ` Alexandre Belloni
  2019-01-29 11:46   ` Matti Vaittinen
  1 sibling, 1 reply; 10+ messages in thread
From: Alexandre Belloni @ 2019-01-28 21:20 UTC (permalink / raw)
  To: Matti Vaittinen
  Cc: mazziesaccount, heikki.haikola, mikko.mutanen, lee.jones,
	robh+dt, mark.rutland, broonie, gregkh, rafael, mturquette,
	sboyd, linus.walleij, bgolaszewski, sre, lgirdwood, a.zummo, wim,
	linux, devicetree, linux-kernel, linux-clk, linux-gpio, linux-pm,
	linux-rtc, linux-watchdog

Hello,

On 25/01/2019 13:05:36+0200, Matti Vaittinen wrote:
> +static const struct rtc_class_ops bd70528_rtc_ops = {
> +	.read_time	= bd70528_get_time,
> +	.set_time	= bd70528_set_time,
> +	.read_alarm	= bd70528_read_alarm,
> +	.set_alarm	= bd70528_set_alarm,

You actually need to provide a .alarm_irq_enable callback, else there is
no way to disable a set alarm.

> +	rtc = devm_rtc_device_register(&pdev->dev, "bd70528-rtc",
> +				       &bd70528_rtc_ops, THIS_MODULE);
> +	if (IS_ERR(rtc)) {
> +		dev_err(&pdev->dev, "Registering RTC failed\n");
> +		return PTR_ERR(rtc);
> +	}

Please use devm_rtc_allocate_device() and set rtc->range_min and
rtc->range_max before registering with rtc_register_device() (which can
be done after trying to request the irq).


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH v2 08/10] rtc: bd70528: Initial support for ROHM bd70528 RTC
  2019-01-28 20:26   ` Jerry Hoemann
@ 2019-01-29  7:01     ` Matti Vaittinen
  2019-01-30  0:24       ` Jerry Hoemann
  0 siblings, 1 reply; 10+ messages in thread
From: Matti Vaittinen @ 2019-01-29  7:01 UTC (permalink / raw)
  To: Jerry Hoemann
  Cc: Guenter Roeck, mazziesaccount, heikki.haikola, mikko.mutanen,
	lee.jones, robh+dt, mark.rutland, broonie, gregkh, rafael,
	mturquette, sboyd, linus.walleij, bgolaszewski, sre, lgirdwood,
	a.zummo, alexandre.belloni, wim, devicetree, linux-kernel,
	linux-clk, linux-gpio, linux-pm, linux-rtc, linux-watchdog

On Mon, Jan 28, 2019 at 01:26:56PM -0700, Jerry Hoemann wrote:
> On Sat, Jan 26, 2019 at 08:30:24AM -0800, Guenter Roeck wrote:
> > On 1/25/19 3:05 AM, Matti Vaittinen wrote:
> > > +static int bd70528_set_wake(struct bd70528 *bd70528,
> > > +			    int enable, int *old_state)
> > > +{
> > > +	int ret;
> > > +	unsigned int ctrl_reg;
> > > +
> > > +	ret = regmap_read(bd70528->chip.regmap, BD70528_REG_WAKE_EN, &ctrl_reg);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (old_state) {
> > > +		if (ctrl_reg & BD70528_MASK_WAKE_EN)
> > > +			*old_state |= BD70528_WAKE_STATE_BIT;
> > > +		else
> > > +			*old_state &= ~BD70528_WAKE_STATE_BIT;
> > > +
> > > +		if ((!enable) == (!(*old_state & BD70528_WAKE_STATE_BIT)))
> > > +			return 0;
> > 
> > I think
> > 		if (enable == !!(*old_state & BD70528_WAKE_STATE_BIT))
> > would be much better readable. Even if not, there are way too many ()
> > in the above conditional.
> > 
> 
> The substitution is not equivalent to original.  I think you mean:
> 
>  		if (!!enable == !!(*old_state & BD70528_WAKE_STATE_BIT))

Thanks Jerry! Good catch! I originally wanted that all non-zero values
of 'enable' would be 'true'. So maybe I just use the original approach
but get rid of extra parenthesis which were pointed out by Guenter.

		if (!enable == !(*old_state & BD70528_WAKE_STATE_BIT))
should do it just fine, right?

Br,
	Matti

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH v2 08/10] rtc: bd70528: Initial support for ROHM bd70528 RTC
  2019-01-28 21:20 ` Alexandre Belloni
@ 2019-01-29 11:46   ` Matti Vaittinen
  0 siblings, 0 replies; 10+ messages in thread
From: Matti Vaittinen @ 2019-01-29 11:46 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: mazziesaccount, heikki.haikola, mikko.mutanen, lee.jones,
	robh+dt, mark.rutland, broonie, gregkh, rafael, mturquette,
	sboyd, linus.walleij, bgolaszewski, sre, lgirdwood, a.zummo, wim,
	linux, devicetree, linux-kernel, linux-clk, linux-gpio, linux-pm,
	linux-rtc, linux-watchdog

Hello Alexandre,

Big thanks for the review!

On Mon, Jan 28, 2019 at 10:20:09PM +0100, Alexandre Belloni wrote:
> Hello,
> 
> On 25/01/2019 13:05:36+0200, Matti Vaittinen wrote:
> > +static const struct rtc_class_ops bd70528_rtc_ops = {
> > +	.read_time	= bd70528_get_time,
> > +	.set_time	= bd70528_set_time,
> > +	.read_alarm	= bd70528_read_alarm,
> > +	.set_alarm	= bd70528_set_alarm,
> 
> You actually need to provide a .alarm_irq_enable callback, else there is
> no way to disable a set alarm.

Glad that you pointed this out. I thought the RTC core would use
set_alarm with enabled in struct rtc_wkalrm being zero for disabling
the alarm. I'll add alarm_irq_enable for disabling alarm at v3.

> > +	rtc = devm_rtc_device_register(&pdev->dev, "bd70528-rtc",
> > +				       &bd70528_rtc_ops, THIS_MODULE);
> > +	if (IS_ERR(rtc)) {
> > +		dev_err(&pdev->dev, "Registering RTC failed\n");
> > +		return PTR_ERR(rtc);
> > +	}
> 
> Please use devm_rtc_allocate_device() and set rtc->range_min and
> rtc->range_max before registering with rtc_register_device() (which can
> be done after trying to request the irq).

Right. I should've noted that devm_rtc_device_register was marked
deprecated. I'll fix this too at v3.

Br,
	Matti Vaittinen

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH v2 08/10] rtc: bd70528: Initial support for ROHM bd70528 RTC
  2019-01-29  7:01     ` Matti Vaittinen
@ 2019-01-30  0:24       ` Jerry Hoemann
  0 siblings, 0 replies; 10+ messages in thread
From: Jerry Hoemann @ 2019-01-30  0:24 UTC (permalink / raw)
  To: Matti Vaittinen
  Cc: Guenter Roeck, mazziesaccount, heikki.haikola, mikko.mutanen,
	lee.jones, robh+dt, mark.rutland, broonie, gregkh, rafael,
	mturquette, sboyd, linus.walleij, bgolaszewski, sre, lgirdwood,
	a.zummo, alexandre.belloni, wim, devicetree, linux-kernel,
	linux-clk, linux-gpio, linux-pm, linux-rtc, linux-watchdog

On Tue, Jan 29, 2019 at 09:01:03AM +0200, Matti Vaittinen wrote:
> On Mon, Jan 28, 2019 at 01:26:56PM -0700, Jerry Hoemann wrote:
> > On Sat, Jan 26, 2019 at 08:30:24AM -0800, Guenter Roeck wrote:
> > > On 1/25/19 3:05 AM, Matti Vaittinen wrote:
> > > > +static int bd70528_set_wake(struct bd70528 *bd70528,
> > > > +			    int enable, int *old_state)
> > > > +{
> > > > +	int ret;
> > > > +	unsigned int ctrl_reg;
> > > > +
> > > > +	ret = regmap_read(bd70528->chip.regmap, BD70528_REG_WAKE_EN, &ctrl_reg);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	if (old_state) {
> > > > +		if (ctrl_reg & BD70528_MASK_WAKE_EN)
> > > > +			*old_state |= BD70528_WAKE_STATE_BIT;
> > > > +		else
> > > > +			*old_state &= ~BD70528_WAKE_STATE_BIT;
> > > > +
> > > > +		if ((!enable) == (!(*old_state & BD70528_WAKE_STATE_BIT)))
> > > > +			return 0;
> > > 
> > > I think
> > > 		if (enable == !!(*old_state & BD70528_WAKE_STATE_BIT))
> > > would be much better readable. Even if not, there are way too many ()
> > > in the above conditional.
> > > 
> > 
> > The substitution is not equivalent to original.  I think you mean:
> > 
> >  		if (!!enable == !!(*old_state & BD70528_WAKE_STATE_BIT))
> 
> Thanks Jerry! Good catch! I originally wanted that all non-zero values
> of 'enable' would be 'true'. So maybe I just use the original approach
> but get rid of extra parenthesis which were pointed out by Guenter.
> 
> 		if (!enable == !(*old_state & BD70528_WAKE_STATE_BIT))
> should do it just fine, right?
> 

The use of "!!" to turn an int into one of two Boolean values (0 | 1)
is used extensively in Linux and as such might make the intent of
the code a bit clearer which I take as checking to see the states
match.

But, I will defer to you and Guenter on the question.

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-01-30  0:25 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-25 11:05 [RFC PATCH v2 08/10] rtc: bd70528: Initial support for ROHM bd70528 RTC Matti Vaittinen
2019-01-26 16:30 ` Guenter Roeck
2019-01-28  7:48   ` Matti Vaittinen
2019-01-28 14:02     ` Guenter Roeck
2019-01-28 16:16       ` Matti Vaittinen
2019-01-28 20:26   ` Jerry Hoemann
2019-01-29  7:01     ` Matti Vaittinen
2019-01-30  0:24       ` Jerry Hoemann
2019-01-28 21:20 ` Alexandre Belloni
2019-01-29 11:46   ` Matti Vaittinen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).