diff mbox series

[v11,2/8] mfd: bd70528: Support ROHM bd70528 PMIC - core

Message ID 39efebe0396dd42de30d37c6f3c1ef2926c90210.1553515333.git.matti.vaittinen@fi.rohmeurope.com
State Not Applicable
Headers show
Series support ROHM BD70528 PMIC | expand

Commit Message

Vaittinen, Matti March 25, 2019, 12:05 p.m. UTC
ROHM BD70528MWV is an ultra-low quiescent current general
purpose single-chip power management IC for battery-powered
portable devices.

Add MFD core which enables chip access for following subdevices:
	- regulators/LED drivers
	- battery-charger
	- gpios
	- 32.768kHz clk
	- RTC
	- watchdog

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/mfd/Kconfig              |  17 ++
 drivers/mfd/Makefile             |   1 +
 drivers/mfd/rohm-bd70528.c       | 438 +++++++++++++++++++++++++++++++
 include/linux/mfd/rohm-bd70528.h | 383 +++++++++++++++++++++++++++
 4 files changed, 839 insertions(+)
 create mode 100644 drivers/mfd/rohm-bd70528.c
 create mode 100644 include/linux/mfd/rohm-bd70528.h

Comments

Lee Jones April 3, 2019, 7:31 a.m. UTC | #1
On Mon, 25 Mar 2019, Matti Vaittinen wrote:

> ROHM BD70528MWV is an ultra-low quiescent current general
> purpose single-chip power management IC for battery-powered
> portable devices.
> 
> Add MFD core which enables chip access for following subdevices:
> 	- regulators/LED drivers
> 	- battery-charger
> 	- gpios
> 	- 32.768kHz clk
> 	- RTC
> 	- watchdog
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
>  drivers/mfd/Kconfig              |  17 ++
>  drivers/mfd/Makefile             |   1 +
>  drivers/mfd/rohm-bd70528.c       | 438 +++++++++++++++++++++++++++++++
>  include/linux/mfd/rohm-bd70528.h | 383 +++++++++++++++++++++++++++
>  4 files changed, 839 insertions(+)
>  create mode 100644 drivers/mfd/rohm-bd70528.c
>  create mode 100644 include/linux/mfd/rohm-bd70528.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 0ce2d8dfc5f1..2fbd6ed14329 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1866,6 +1866,23 @@ config MFD_ROHM_BD718XX
>  	  NXP i.MX8. It contains 8 BUCK outputs and 7 LDOs, voltage monitoring
>  	  and emergency shut down as well as 32,768KHz clock output.
>  
> +config MFD_ROHM_BD70528
> +	tristate "ROHM BD70528 Power Management IC"
> +	depends on I2C=y
> +	depends on OF
> +	select REGMAP_I2C
> +	select REGMAP_IRQ
> +	select MFD_CORE
> +	help
> +	  Select this option to get support for the ROHM BD70528 Power
> +	  Management IC. BD71837 is general purpose single-chip power
> +	  management IC for battery-powered portable devices. It contains
> +	  3 ultra-low current consumption buck converters, 3 LDOs and 2 LED
> +	  Drivers. Also included are 4 GPIOs, a real-time clock (RTC), a 32kHz

Nit: "drivers"

> +	  crystal oscillator, high-accuracy VREF for use with an external ADC,
> +	  10 bits SAR ADC for battery temperature monitor and 1S battery
> +	  charger.
> +
>  config MFD_STM32_LPTIMER
>  	tristate "Support for STM32 Low-Power Timer"
>  	depends on (ARCH_STM32 && OF) || COMPILE_TEST
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index b4569ed7f3f3..dc1c9431c8d9 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -246,4 +246,5 @@ obj-$(CONFIG_MFD_MXS_LRADC)     += mxs-lradc.o
>  obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
>  obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
>  obj-$(CONFIG_MFD_ROHM_BD718XX)	+= rohm-bd718x7.o
> +obj-$(CONFIG_MFD_ROHM_BD70528)	+= rohm-bd70528.o
>  
> diff --git a/drivers/mfd/rohm-bd70528.c b/drivers/mfd/rohm-bd70528.c
> new file mode 100644
> index 000000000000..a46bbcdefce0
> --- /dev/null
> +++ b/drivers/mfd/rohm-bd70528.c
> @@ -0,0 +1,438 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +//
> +// Copyright (C) 2019 ROHM Semiconductors
> +//
> +// ROHM BD70528 PMIC driver
> +
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/ioport.h>
> +#include <linux/irq.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/rohm-bd70528.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +
> +struct pmic_data {
> +	struct rohm_regmap_dev chip;
> +	struct mutex rtc_timer_lock;
> +};
> +
> +static const struct resource rtc_irqs[] = {
> +	DEFINE_RES_IRQ_NAMED(BD70528_INT_RTC_ALARM, "bd70528-rtc-alm"),
> +	DEFINE_RES_IRQ_NAMED(BD70528_INT_ELPS_TIM, "bd70528-elapsed-timer"),
> +};
> +
> +static const struct resource charger_irqs[] = {
> +	DEFINE_RES_IRQ_NAMED(BD70528_INT_BAT_OV_RES, "bd70528-bat-ov-res"),
> +	DEFINE_RES_IRQ_NAMED(BD70528_INT_BAT_OV_DET, "bd70528-bat-ov-det"),
> +	DEFINE_RES_IRQ_NAMED(BD70528_INT_DBAT_DET, "bd70528-bat-dead"),
> +	DEFINE_RES_IRQ_NAMED(BD70528_INT_BATTSD_COLD_RES, "bd70528-bat-warmed"),
> +	DEFINE_RES_IRQ_NAMED(BD70528_INT_BATTSD_COLD_DET, "bd70528-bat-cold"),
> +	DEFINE_RES_IRQ_NAMED(BD70528_INT_BATTSD_HOT_RES, "bd70528-bat-cooled"),
> +	DEFINE_RES_IRQ_NAMED(BD70528_INT_BATTSD_HOT_DET, "bd70528-bat-hot"),
> +	DEFINE_RES_IRQ_NAMED(BD70528_INT_CHG_TSD, "bd70528-chg-tshd"),
> +	DEFINE_RES_IRQ_NAMED(BD70528_INT_BAT_RMV, "bd70528-bat-removed"),
> +	DEFINE_RES_IRQ_NAMED(BD70528_INT_BAT_DET, "bd70528-bat-detected"),
> +	DEFINE_RES_IRQ_NAMED(BD70528_INT_DCIN2_OV_RES, "bd70528-dcin2-ov-res"),
> +	DEFINE_RES_IRQ_NAMED(BD70528_INT_DCIN2_OV_DET, "bd70528-dcin2-ov-det"),
> +	DEFINE_RES_IRQ_NAMED(BD70528_INT_DCIN2_RMV, "bd70528-dcin2-removed"),
> +	DEFINE_RES_IRQ_NAMED(BD70528_INT_DCIN2_DET, "bd70528-dcin2-detected"),
> +	DEFINE_RES_IRQ_NAMED(BD70528_INT_DCIN1_RMV, "bd70528-dcin1-removed"),
> +	DEFINE_RES_IRQ_NAMED(BD70528_INT_DCIN1_DET, "bd70528-dcin1-detected"),
> +};
> +
> +static struct mfd_cell bd70528_mfd_cells[] = {
> +	{ .name = "bd70528-pmic", },
> +	{ .name = "bd70528-gpio", },
> +	/*
> +	 * We use BD71837 driver to drive the clk block. Only differences to

Nit: s/clk/clock/

> +	 * BD70528 clock gate are the register address and mask.
> +	 */
> +	{ .name = "bd718xx-clk", },
> +	{ .name = "bd70528-wdt", },
> +	{
> +		.name = "bd70528-power",
> +		.resources = charger_irqs,
> +		.num_resources = ARRAY_SIZE(charger_irqs),
> +	},
> +	{

Nit: Put these on the same line, like:

	}, {

> +		.name = "bd70528-rtc",
> +		.resources = rtc_irqs,
> +		.num_resources = ARRAY_SIZE(rtc_irqs),
> +	},
> +};
> +
> +static const struct regmap_range volatile_ranges[] = {
> +	/* IRQ regs */
> +	{
> +		.range_min = BD70528_REG_INT_MAIN,
> +		.range_max = BD70528_REG_INT_OP_FAIL,
> +	},
> +	/* RTC regs */

These 2 comments are superfluous.

> +	{

Same line as the one before.

And for all the other instances of this.

> +		.range_min = BD70528_REG_RTC_COUNT_H,
> +		.range_max = BD70528_REG_RTC_ALM_REPEAT,
> +	},
> +	/*
> +	 * WDT control reg is special. Magic values must be
> +	 * written to it in order to change the control. Should
> +	 * not be cached.
> +	 */
> +	{
> +		.range_min = BD70528_REG_WDT_CTRL,
> +		.range_max = BD70528_REG_WDT_CTRL,
> +	},
> +	/*
> +	 * BD70528 also contains a few other registers which require
> +	 * magic sequences to be written in order to update the value.
> +	 * At least SHIPMODE, HWRESET, WARMRESET,and STANDBY
> +	 */
> +	{
> +		.range_min = BD70528_REG_SHIPMODE,
> +		.range_max = BD70528_REG_STANDBY,
> +	},
> +};
> +
> +static const struct regmap_access_table volatile_regs = {
> +	.yes_ranges = &volatile_ranges[0],
> +	.n_yes_ranges = ARRAY_SIZE(volatile_ranges),
> +};
> +
> +static struct regmap_config bd70528_regmap = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.volatile_table = &volatile_regs,
> +	.max_register = BD70528_MAX_REGISTER,
> +	.cache_type = REGCACHE_RBTREE,
> +};
> +
> +/*
> + * Mapping of main IRQ register bits to sub irq register offsets so

"sub-IRQ"

> + * that we can access corect sub IRQ registers based on bits that

"sub IRQ" is also fine, but please standardise.

I do prefer "sub-IRQ" though.

> + * are set in main IRQ register.
> + */
> +
> +/* bit [0] - Shutdown register */
> +unsigned int bit0_offsets[] = {0};
> +/* bit [1] - Power failure register */
> +unsigned int bit1_offsets[] = {1};
> +/* bit [2] - VR FAULT register */
> +unsigned int bit2_offsets[] = {2};
> +/* bit [3] - PMU register interrupts */
> +unsigned int bit3_offsets[] = {3};
> +/* bit [4] - Charger 1 and Charger 2 registers */
> +unsigned int bit4_offsets[] = {4, 5};
> +/* bit [5] - RTC register */
> +unsigned int bit5_offsets[] = {6};
> +/* bit [6] - GPIO register */
> +unsigned int bit6_offsets[] = {7};
> +/* bit [7] - Invalid operation register */
> +unsigned int bit7_offsets[] = {8};

unsigned int bit0_offsets[] = {0};    /* Shutdown register */
unsigned int bit1_offsets[] = {1};    /* Power failure register */
unsigned int bit2_offsets[] = {2};    /* VR FAULT register */
unsigned int bit3_offsets[] = {3};    /* PMU register interrupts*/
unsigned int bit4_offsets[] = {4, 5}; /* Charger 1 and Charger 2 registers */
unsigned int bit5_offsets[] = {6};    /* RTC register */
unsigned int bit6_offsets[] = {7};    /* GPIO register */
unsigned int bit7_offsets[] = {8};    /* Invalid operation register */

> +static struct regmap_irq_sub_irq_map bd70528_sub_irq_offsets[] = {
> +	REGMAP_IRQ_MAIN_REG_OFFSET(bit0_offsets),
> +	REGMAP_IRQ_MAIN_REG_OFFSET(bit1_offsets),
> +	REGMAP_IRQ_MAIN_REG_OFFSET(bit2_offsets),
> +	REGMAP_IRQ_MAIN_REG_OFFSET(bit3_offsets),
> +	REGMAP_IRQ_MAIN_REG_OFFSET(bit4_offsets),
> +	REGMAP_IRQ_MAIN_REG_OFFSET(bit5_offsets),
> +	REGMAP_IRQ_MAIN_REG_OFFSET(bit6_offsets),
> +	REGMAP_IRQ_MAIN_REG_OFFSET(bit7_offsets),
> +};
> +
> +static struct regmap_irq irqs[] = {

Please prefix "irqs".

> +	REGMAP_IRQ_REG(BD70528_INT_LONGPUSH, 0, BD70528_INT_LONGPUSH_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_WDT, 0, BD70528_INT_WDT_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_HWRESET, 0, BD70528_INT_HWRESET_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_RSTB_FAULT, 0, BD70528_INT_RSTB_FAULT_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_VBAT_UVLO, 0, BD70528_INT_VBAT_UVLO_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_TSD, 0, BD70528_INT_TSD_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_RSTIN, 0, BD70528_INT_RSTIN_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_BUCK1_FAULT, 1,
> +		       BD70528_INT_BUCK1_FAULT_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_BUCK2_FAULT, 1,
> +		       BD70528_INT_BUCK2_FAULT_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_BUCK3_FAULT, 1,
> +		       BD70528_INT_BUCK3_FAULT_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_LDO1_FAULT, 1, BD70528_INT_LDO1_FAULT_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_LDO2_FAULT, 1, BD70528_INT_LDO2_FAULT_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_LDO3_FAULT, 1, BD70528_INT_LDO3_FAULT_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_LED1_FAULT, 1, BD70528_INT_LED1_FAULT_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_LED2_FAULT, 1, BD70528_INT_LED2_FAULT_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_BUCK1_OCP, 2, BD70528_INT_BUCK1_OCP_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_BUCK2_OCP, 2, BD70528_INT_BUCK2_OCP_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_BUCK3_OCP, 2, BD70528_INT_BUCK3_OCP_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_LED1_OCP, 2, BD70528_INT_LED1_OCP_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_LED2_OCP, 2, BD70528_INT_LED2_OCP_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_BUCK1_FULLON, 2,
> +		       BD70528_INT_BUCK1_FULLON_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_BUCK2_FULLON, 2,
> +		       BD70528_INT_BUCK2_FULLON_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_SHORTPUSH, 3, BD70528_INT_SHORTPUSH_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_AUTO_WAKEUP, 3,
> +		       BD70528_INT_AUTO_WAKEUP_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_STATE_CHANGE, 3,
> +		       BD70528_INT_STATE_CHANGE_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_BAT_OV_RES, 4, BD70528_INT_BAT_OV_RES_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_BAT_OV_DET, 4, BD70528_INT_BAT_OV_DET_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_DBAT_DET, 4, BD70528_INT_DBAT_DET_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_BATTSD_COLD_RES, 4,
> +		       BD70528_INT_BATTSD_COLD_RES_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_BATTSD_COLD_DET, 4,
> +		       BD70528_INT_BATTSD_COLD_DET_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_BATTSD_HOT_RES, 4,
> +		       BD70528_INT_BATTSD_HOT_RES_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_BATTSD_HOT_DET, 4,
> +		       BD70528_INT_BATTSD_HOT_DET_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_CHG_TSD, 4, BD70528_INT_CHG_TSD_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_BAT_RMV, 5, BD70528_INT_BAT_RMV_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_BAT_DET, 5, BD70528_INT_BAT_DET_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_DCIN2_OV_RES, 5,
> +		       BD70528_INT_DCIN2_OV_RES_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_DCIN2_OV_DET, 5,
> +		       BD70528_INT_DCIN2_OV_DET_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_DCIN2_RMV, 5, BD70528_INT_DCIN2_RMV_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_DCIN2_DET, 5, BD70528_INT_DCIN2_DET_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_DCIN1_RMV, 5, BD70528_INT_DCIN1_RMV_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_DCIN1_DET, 5, BD70528_INT_DCIN1_DET_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_RTC_ALARM, 6, BD70528_INT_RTC_ALARM_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_ELPS_TIM, 6, BD70528_INT_ELPS_TIM_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_GPIO0, 7, BD70528_INT_GPIO0_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_GPIO1, 7, BD70528_INT_GPIO1_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_GPIO2, 7, BD70528_INT_GPIO2_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_GPIO3, 7, BD70528_INT_GPIO3_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_BUCK1_DVS_OPFAIL, 8,
> +		       BD70528_INT_BUCK1_DVS_OPFAIL_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_BUCK2_DVS_OPFAIL, 8,
> +		       BD70528_INT_BUCK2_DVS_OPFAIL_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_BUCK3_DVS_OPFAIL, 8,
> +		       BD70528_INT_BUCK3_DVS_OPFAIL_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_LED1_VOLT_OPFAIL, 8,
> +		       BD70528_INT_LED1_VOLT_OPFAIL_MASK),
> +	REGMAP_IRQ_REG(BD70528_INT_LED2_VOLT_OPFAIL, 8,
> +		       BD70528_INT_LED2_VOLT_OPFAIL_MASK),
> +};
> +
> +static struct regmap_irq_chip bd70528_irq_chip = {
> +	.name = "bd70528_irq",
> +	.main_status = BD70528_REG_INT_MAIN,
> +	.irqs = &irqs[0],
> +	.num_irqs = ARRAY_SIZE(irqs),
> +	.status_base = BD70528_REG_INT_SHDN,
> +	.mask_base = BD70528_REG_INT_SHDN_MASK,
> +	.ack_base = BD70528_REG_INT_SHDN,
> +	.type_base = BD70528_REG_GPIO1_IN,
> +	.init_ack_masked = true,
> +	.num_regs = 9,
> +	.num_main_regs = 1,
> +	.num_type_reg = 4,
> +	.sub_reg_offsets = &bd70528_sub_irq_offsets[0],
> +	.num_main_status_bits = 8,
> +	.irq_reg_stride = 1,
> +};
> +
> +#define WD_CTRL_MAGIC1 0x55
> +#define WD_CTRL_MAGIC2 0xAA
> +/**
> + * bd70528_wdt_set - arm or disarm watchdog timer
> + *
> + * @data:	device data for the PMIC instance we want to operate on
> + * @enable:	new state of WDT. zero to disable, non zero to enable
> + * @old_state:	previous state of WDT will be filled here
> + *
> + * Arm or disarm WDT on BD70528 PMIC. Expected to be called only by
> + * BD70528 RTC and BD70528 WDT drivers. The rtc_timer_lock must be taken
> + * by calling bd70528_wdt_lock before calling bd70528_wdt_set.
> + */
> +int bd70528_wdt_set(struct rohm_regmap_dev *data, int enable, int *old_state)

Why doesn't this reside in the watchdog driver?

> +{
> +	int ret, i;
> +	unsigned int tmp;
> +	struct pmic_data *bd70528 = container_of(data, struct pmic_data,
> +						 chip);
> +	u8 wd_ctrl_arr[3] = { WD_CTRL_MAGIC1, WD_CTRL_MAGIC2, 0 };
> +	u8 *wd_ctrl = &wd_ctrl_arr[2];
> +
> +	ret = regmap_read(bd70528->chip.regmap, BD70528_REG_WDT_CTRL, &tmp);
> +	if (ret)
> +		return ret;
> +
> +	*wd_ctrl = (u8)tmp;
> +
> +	if (old_state) {
> +		if (*wd_ctrl & BD70528_MASK_WDT_EN)
> +			*old_state |= BD70528_WDT_STATE_BIT;
> +		else
> +			*old_state &= ~BD70528_WDT_STATE_BIT;
> +		if ((!enable) == (!(*old_state & BD70528_WDT_STATE_BIT)))
> +			return 0;
> +	}
> +
> +	if (enable) {
> +		if (*wd_ctrl & BD70528_MASK_WDT_EN)
> +			return 0;
> +		*wd_ctrl |= BD70528_MASK_WDT_EN;
> +	} else {
> +		if (*wd_ctrl & BD70528_MASK_WDT_EN)
> +			*wd_ctrl &= ~BD70528_MASK_WDT_EN;
> +		else
> +			return 0;
> +	}
> +
> +	for (i = 0; i < 3; i++) {
> +		ret = regmap_write(bd70528->chip.regmap, BD70528_REG_WDT_CTRL,
> +				   wd_ctrl_arr[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = regmap_read(bd70528->chip.regmap, BD70528_REG_WDT_CTRL, &tmp);
> +	if ((tmp & BD70528_MASK_WDT_EN) != (*wd_ctrl & BD70528_MASK_WDT_EN)) {
> +		dev_err(bd70528->chip.dev,
> +			"Watchdog ctrl mismatch (hw) 0x%x (set) 0x%x\n",
> +			tmp, *wd_ctrl);
> +		ret = -EIO;
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(bd70528_wdt_set);
> +
> +/**
> + * bd70528_wdt_lock - take WDT lock
> + *
> + * @bd70528:	device data for the PMIC instance we want to operate on
> + *
> + * Lock WDT for arming/disarming in order to avoid race condition caused
> + * by WDT state changes initiated by WDT and RTC drivers.
> + */
> +void bd70528_wdt_lock(struct rohm_regmap_dev *data)
> +{
> +	struct pmic_data *bd70528 = container_of(data, struct pmic_data,
> +						 chip);
> +
> +	mutex_lock(&bd70528->rtc_timer_lock);
> +}
> +EXPORT_SYMBOL(bd70528_wdt_lock);
> +
> +/**
> + * bd70528_wdt_unlock - unlock WDT lock
> + *
> + * @bd70528:	device data for the PMIC instance we want to operate on
> + *
> + * Unlock WDT lock which has previously been taken by call to
> + * bd70528_wdt_lock.
> + */
> +void bd70528_wdt_unlock(struct rohm_regmap_dev *data)
> +{
> +	struct pmic_data *bd70528 = container_of(data, struct pmic_data,
> +						 chip);
> +
> +	mutex_unlock(&bd70528->rtc_timer_lock);
> +}
> +EXPORT_SYMBOL(bd70528_wdt_unlock);

Same goes for all of this Watchdog stuff.  The parent device really
shouldn't have to worry about all of this functionality.

> +#define BD70528_NUM_OF_GPIOS 4

Pop all defines at the top of the file.

> +static int bd70528_i2c_probe(struct i2c_client *i2c,
> +			    const struct i2c_device_id *id)
> +{
> +	struct pmic_data *bd70528;
> +	struct regmap_irq_chip_data *irq_data;
> +	int ret, i;
> +
> +	if (!i2c->irq) {
> +		dev_err(&i2c->dev, "No IRQ configured\n");
> +		return -EINVAL;
> +	}
> +
> +	bd70528 = devm_kzalloc(&i2c->dev, sizeof(*bd70528), GFP_KERNEL);
> +	if (!bd70528)
> +		return -ENOMEM;
> +
> +	mutex_init(&bd70528->rtc_timer_lock);
> +
> +	dev_set_drvdata(&i2c->dev, &bd70528->chip);
> +
> +	bd70528->chip.chip_type = ROHM_CHIP_TYPE_BD70528;
> +	bd70528->chip.regmap = devm_regmap_init_i2c(i2c, &bd70528_regmap);
> +	if (IS_ERR(bd70528->chip.regmap)) {
> +		dev_err(&i2c->dev, "regmap initialization failed\n");

Nit: "Regmap"

But better read as:

"Failed to initialise Regmap"

> +		return PTR_ERR(bd70528->chip.regmap);
> +	}
> +
> +	/*
> +	 * Disallow type setting for all IRQs by default as

Why the premature line feed?

> +	 * most of them do not support setting type.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(irqs); i++)
> +		irqs[i].type.types_supported = 0;
> +
> +	/*
> +	 * Set IRQ typesetting information for GPIO pins 0 - 3
> +	 */

Please format this as a one line comment.

> +	for (i = 0; i < BD70528_NUM_OF_GPIOS; i++) {
> +		struct regmap_irq_type *type;
> +
> +		type = &irqs[BD70528_INT_GPIO0 + i].type;
> +		type->type_reg_offset = 2 * i;
> +		type->type_rising_val = 0x20;
> +		type->type_falling_val = 0x10;
> +		type->type_level_high_val = 0x40;
> +		type->type_level_low_val = 0x50;
> +		type->types_supported = (IRQ_TYPE_EDGE_BOTH |
> +				IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
> +	}
> +
> +	ret = devm_regmap_add_irq_chip(&i2c->dev, bd70528->chip.regmap,
> +				       i2c->irq, IRQF_ONESHOT, 0,
> +				       &bd70528_irq_chip, &irq_data);
> +	if (ret) {
> +		dev_err(&i2c->dev, "Failed to add irq_chip\n");

"Failed to add IRQ chip"

> +		return ret;
> +	}
> +	dev_dbg(&i2c->dev, "Registered %d irqs for chip\n",

"IRQs"

> +			bd70528_irq_chip.num_irqs);
> +
> +	/*
> +	 * BD70528 IRQ controller is not touching the main mask register.
> +	 * So enable the GPIO block interrupts at main level. We can just
> +	 * leave them enabled as the IRQ controller should disable IRQs
> +	 * from sub-registers when IRQ is disabled or freed.
> +	 */
> +	ret = regmap_update_bits(bd70528->chip.regmap,
> +				 BD70528_REG_INT_MAIN_MASK,
> +				 BD70528_INT_GPIO_MASK, 0);
> +
> +	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
> +				   bd70528_mfd_cells,
> +				   ARRAY_SIZE(bd70528_mfd_cells), NULL, 0,
> +				   regmap_irq_get_domain(irq_data));
> +	if (ret)
> +		dev_err(&i2c->dev, "Failed to create subdevices\n");
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id bd70528_of_match[] = {
> +	{ .compatible = "rohm,bd70528", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, bd70528_of_match);
> +
> +static struct i2c_driver bd70528_drv = {
> +	.driver = {
> +		.name = "rohm-bd70528",
> +		.of_match_table = bd70528_of_match,
> +	},
> +	.probe = &bd70528_i2c_probe,
> +};
> +
> +module_i2c_driver(bd70528_drv);
> +
> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> +MODULE_DESCRIPTION("ROHM BD70528 Power Management IC driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/rohm-bd70528.h b/include/linux/mfd/rohm-bd70528.h
> new file mode 100644
> index 000000000000..155dc9c89e13
> --- /dev/null
> +++ b/include/linux/mfd/rohm-bd70528.h
> @@ -0,0 +1,383 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/* Copyright (C) 2018 ROHM Semiconductors */
> +
> +#ifndef __LINUX_MFD_BD70528_H__
> +#define __LINUX_MFD_BD70528_H__
> +
> +#include <linux/device.h>
> +#include <linux/mfd/rohm-generic.h>
> +#include <linux/regmap.h>
> +
> +enum {
> +	BD70528_BUCK1,
> +	BD70528_BUCK2,
> +	BD70528_BUCK3,
> +	BD70528_LDO1,
> +	BD70528_LDO2,
> +	BD70528_LDO3,
> +	BD70528_LED1,
> +	BD70528_LED2,
> +};
> +
> +#define BD70528_BUCK_VOLTS 17
> +#define BD70528_BUCK_VOLTS 17
> +#define BD70528_BUCK_VOLTS 17
> +#define BD70528_LDO_VOLTS 0x20
> +
> +#define BD70528_REG_BUCK1_EN	0x0F
> +#define BD70528_REG_BUCK1_VOLT	0x15
> +#define BD70528_REG_BUCK2_EN	0x10
> +#define BD70528_REG_BUCK2_VOLT	0x16
> +#define BD70528_REG_BUCK3_EN	0x11
> +#define BD70528_REG_BUCK3_VOLT	0x17
> +#define BD70528_REG_LDO1_EN	0x1b
> +#define BD70528_REG_LDO1_VOLT	0x1e
> +#define BD70528_REG_LDO2_EN	0x1c
> +#define BD70528_REG_LDO2_VOLT	0x1f
> +#define BD70528_REG_LDO3_EN	0x1d
> +#define BD70528_REG_LDO3_VOLT	0x20
> +#define BD70528_REG_LED_CTRL	0x2b
> +#define BD70528_REG_LED_VOLT	0x29
> +#define BD70528_REG_LED_EN	0x2a
> +
> +/* main irq registers */
> +#define BD70528_REG_INT_MAIN	0x7E
> +#define BD70528_REG_INT_MAIN_MASK 0x74
> +
> +/* 'sub irq' registers */
> +#define BD70528_REG_INT_SHDN	0x7F
> +#define BD70528_REG_INT_PWR_FLT	0x80
> +#define BD70528_REG_INT_VR_FLT	0x81
> +#define BD70528_REG_INT_MISC	0x82
> +#define BD70528_REG_INT_BAT1	0x83
> +#define BD70528_REG_INT_BAT2	0x84
> +#define BD70528_REG_INT_RTC	0x85
> +#define BD70528_REG_INT_GPIO	0x86
> +#define BD70528_REG_INT_OP_FAIL	0x87
> +
> +#define BD70528_REG_INT_SHDN_MASK	0x75
> +#define BD70528_REG_INT_PWR_FLT_MASK	0x76
> +#define BD70528_REG_INT_VR_FLT_MASK	0x77
> +#define BD70528_REG_INT_MISC_MASK	0x78
> +#define BD70528_REG_INT_BAT1_MASK	0x79
> +#define BD70528_REG_INT_BAT2_MASK	0x7a
> +#define BD70528_REG_INT_RTC_MASK	0x7b
> +#define BD70528_REG_INT_GPIO_MASK	0x7c
> +#define BD70528_REG_INT_OP_FAIL_MASK	0x7d
> +
> +/* Reset related 'magic' registers */
> +#define BD70528_REG_SHIPMODE	0x03
> +#define BD70528_REG_HWRESET	0x04
> +#define BD70528_REG_WARMRESET	0x05
> +#define BD70528_REG_STANDBY	0x06
> +
> +/* GPIO registers */
> +#define BD70528_REG_GPIO_STATE	0x8F
> +
> +#define BD70528_REG_GPIO1_IN	0x4d
> +#define BD70528_REG_GPIO2_IN	0x4f
> +#define BD70528_REG_GPIO3_IN	0x51
> +#define BD70528_REG_GPIO4_IN	0x53
> +#define BD70528_REG_GPIO1_OUT	0x4e
> +#define BD70528_REG_GPIO2_OUT	0x50
> +#define BD70528_REG_GPIO3_OUT	0x52
> +#define BD70528_REG_GPIO4_OUT	0x54
> +
> +/* clk control */
> +
> +#define BD70528_REG_CLK_OUT	0x2c
> +
> +/* RTC */
> +
> +#define BD70528_REG_RTC_COUNT_H		0x2d
> +#define BD70528_REG_RTC_COUNT_L		0x2e
> +#define BD70528_REG_RTC_SEC		0x2f
> +#define BD70528_REG_RTC_MINUTE		0x30
> +#define BD70528_REG_RTC_HOUR		0x31
> +#define BD70528_REG_RTC_WEEK		0x32
> +#define BD70528_REG_RTC_DAY		0x33
> +#define BD70528_REG_RTC_MONTH		0x34
> +#define BD70528_REG_RTC_YEAR		0x35
> +
> +#define BD70528_REG_RTC_ALM_SEC		0x36
> +#define BD70528_REG_RTC_ALM_START	BD70528_REG_RTC_ALM_SEC
> +#define BD70528_REG_RTC_ALM_MINUTE	0x37
> +#define BD70528_REG_RTC_ALM_HOUR	0x38
> +#define BD70528_REG_RTC_ALM_WEEK	0x39
> +#define BD70528_REG_RTC_ALM_DAY		0x3a
> +#define BD70528_REG_RTC_ALM_MONTH	0x3b
> +#define BD70528_REG_RTC_ALM_YEAR	0x3c
> +#define BD70528_REG_RTC_ALM_MASK	0x3d
> +#define BD70528_REG_RTC_ALM_REPEAT	0x3e
> +#define BD70528_REG_RTC_START		BD70528_REG_RTC_SEC
> +
> +#define BD70528_REG_RTC_WAKE_SEC	0x43
> +#define BD70528_REG_RTC_WAKE_START	BD70528_REG_RTC_WAKE_SEC
> +#define BD70528_REG_RTC_WAKE_MIN	0x44
> +#define BD70528_REG_RTC_WAKE_HOUR	0x45
> +#define BD70528_REG_RTC_WAKE_CTRL	0x46
> +
> +#define BD70528_REG_ELAPSED_TIMER_EN	0x42
> +#define BD70528_REG_WAKE_EN		0x46
> +
> +/* WDT registers */
> +#define BD70528_REG_WDT_CTRL		0x4A
> +#define BD70528_REG_WDT_HOUR		0x49
> +#define BD70528_REG_WDT_MINUTE		0x48
> +#define BD70528_REG_WDT_SEC		0x47
> +
> +/* Charger / Battery */
> +#define BD70528_REG_CHG_CURR_STAT	0x59
> +#define BD70528_REG_CHG_BAT_STAT	0x57
> +#define BD70528_REG_CHG_BAT_TEMP	0x58
> +#define BD70528_REG_CHG_IN_STAT		0x56
> +#define BD70528_REG_CHG_DCIN_ILIM	0x5d
> +#define BD70528_REG_CHG_CHG_CURR_WARM	0x61
> +#define BD70528_REG_CHG_CHG_CURR_COLD	0x62
> +
> +
> +/* Masks for main IRQ register bits */
> +enum {
> +	BD70528_INT_SHDN,
> +#define BD70528_INT_SHDN_MASK (1<<BD70528_INT_SHDN)
> +	BD70528_INT_PWR_FLT,
> +#define BD70528_INT_PWR_FLT_MASK (1<<BD70528_INT_PWR_FLT)
> +	BD70528_INT_VR_FLT,
> +#define BD70528_INT_VR_FLT_MASK (1<<BD70528_INT_VR_FLT)
> +	BD70528_INT_MISC,
> +#define BD70528_INT_MISC_MASK (1<<BD70528_INT_MISC)
> +	BD70528_INT_BAT1,
> +#define BD70528_INT_BAT1_MASK (1<<BD70528_INT_BAT1)
> +	BD70528_INT_RTC,
> +#define BD70528_INT_RTC_MASK (1<<BD70528_INT_RTC)
> +	BD70528_INT_GPIO,
> +#define BD70528_INT_GPIO_MASK (1<<BD70528_INT_GPIO)
> +	BD70528_INT_OP_FAIL,
> +#define BD70528_INT_OP_FAIL_MASK (1<<BD70528_INT_OP_FAIL)
> +};
> +
> +/* IRQs */
> +enum {
> +	/* Shutdown register IRQs */
> +	BD70528_INT_LONGPUSH,
> +	BD70528_INT_WDT,
> +	BD70528_INT_HWRESET,
> +	BD70528_INT_RSTB_FAULT,
> +	BD70528_INT_VBAT_UVLO,
> +	BD70528_INT_TSD,
> +	BD70528_INT_RSTIN,
> +	/* Power failure register IRQs */
> +	BD70528_INT_BUCK1_FAULT,
> +	BD70528_INT_BUCK2_FAULT,
> +	BD70528_INT_BUCK3_FAULT,
> +	BD70528_INT_LDO1_FAULT,
> +	BD70528_INT_LDO2_FAULT,
> +	BD70528_INT_LDO3_FAULT,
> +	BD70528_INT_LED1_FAULT,
> +	BD70528_INT_LED2_FAULT,
> +	/* VR FAULT register IRQs */
> +	BD70528_INT_BUCK1_OCP,
> +	BD70528_INT_BUCK2_OCP,
> +	BD70528_INT_BUCK3_OCP,
> +	BD70528_INT_LED1_OCP,
> +	BD70528_INT_LED2_OCP,
> +	BD70528_INT_BUCK1_FULLON,
> +	BD70528_INT_BUCK2_FULLON,
> +	/* PMU register interrupts */
> +	BD70528_INT_SHORTPUSH,
> +	BD70528_INT_AUTO_WAKEUP,
> +	BD70528_INT_STATE_CHANGE,
> +	/* Charger 1 register IRQs */
> +	BD70528_INT_BAT_OV_RES,
> +	BD70528_INT_BAT_OV_DET,
> +	BD70528_INT_DBAT_DET,
> +	BD70528_INT_BATTSD_COLD_RES,
> +	BD70528_INT_BATTSD_COLD_DET,
> +	BD70528_INT_BATTSD_HOT_RES,
> +	BD70528_INT_BATTSD_HOT_DET,
> +	BD70528_INT_CHG_TSD,
> +	/* Charger 2 register IRQs */
> +	BD70528_INT_BAT_RMV,
> +	BD70528_INT_BAT_DET,
> +	BD70528_INT_DCIN2_OV_RES,
> +	BD70528_INT_DCIN2_OV_DET,
> +	BD70528_INT_DCIN2_RMV,
> +	BD70528_INT_DCIN2_DET,
> +	BD70528_INT_DCIN1_RMV,
> +	BD70528_INT_DCIN1_DET,
> +	/* RTC register IRQs */
> +	BD70528_INT_RTC_ALARM,
> +	BD70528_INT_ELPS_TIM,
> +	/* GPIO register IRQs */
> +	BD70528_INT_GPIO0,
> +	BD70528_INT_GPIO1,
> +	BD70528_INT_GPIO2,
> +	BD70528_INT_GPIO3,
> +	/* Invalid operation register IRQs */
> +	BD70528_INT_BUCK1_DVS_OPFAIL,
> +	BD70528_INT_BUCK2_DVS_OPFAIL,
> +	BD70528_INT_BUCK3_DVS_OPFAIL,
> +	BD70528_INT_LED1_VOLT_OPFAIL,
> +	BD70528_INT_LED2_VOLT_OPFAIL,
> +};
> +
> +/* Masks */
> +#define BD70528_INT_LONGPUSH_MASK 0x1
> +#define BD70528_INT_WDT_MASK 0x2
> +#define BD70528_INT_HWRESET_MASK 0x4
> +#define BD70528_INT_RSTB_FAULT_MASK 0x8
> +#define BD70528_INT_VBAT_UVLO_MASK 0x10
> +#define BD70528_INT_TSD_MASK 0x20
> +#define BD70528_INT_RSTIN_MASK 0x40
> +
> +#define BD70528_INT_BUCK1_FAULT_MASK 0x1
> +#define BD70528_INT_BUCK2_FAULT_MASK 0x2
> +#define BD70528_INT_BUCK3_FAULT_MASK 0x4
> +#define BD70528_INT_LDO1_FAULT_MASK 0x8
> +#define BD70528_INT_LDO2_FAULT_MASK 0x10
> +#define BD70528_INT_LDO3_FAULT_MASK 0x20
> +#define BD70528_INT_LED1_FAULT_MASK 0x40
> +#define BD70528_INT_LED2_FAULT_MASK 0x80
> +
> +#define BD70528_INT_BUCK1_OCP_MASK 0x1
> +#define BD70528_INT_BUCK2_OCP_MASK 0x2
> +#define BD70528_INT_BUCK3_OCP_MASK 0x4
> +#define BD70528_INT_LED1_OCP_MASK 0x8
> +#define BD70528_INT_LED2_OCP_MASK 0x10
> +#define BD70528_INT_BUCK1_FULLON_MASK 0x20
> +#define BD70528_INT_BUCK2_FULLON_MASK 0x40
> +
> +#define BD70528_INT_SHORTPUSH_MASK 0x1
> +#define BD70528_INT_AUTO_WAKEUP_MASK 0x2
> +#define BD70528_INT_STATE_CHANGE_MASK 0x10
> +
> +#define BD70528_INT_BAT_OV_RES_MASK 0x1
> +#define BD70528_INT_BAT_OV_DET_MASK 0x2
> +#define BD70528_INT_DBAT_DET_MASK 0x4
> +#define BD70528_INT_BATTSD_COLD_RES_MASK 0x8
> +#define BD70528_INT_BATTSD_COLD_DET_MASK 0x10
> +#define BD70528_INT_BATTSD_HOT_RES_MASK 0x20
> +#define BD70528_INT_BATTSD_HOT_DET_MASK 0x40
> +#define BD70528_INT_CHG_TSD_MASK 0x80
> +
> +#define BD70528_INT_BAT_RMV_MASK 0x1
> +#define BD70528_INT_BAT_DET_MASK 0x2
> +#define BD70528_INT_DCIN2_OV_RES_MASK 0x4
> +#define BD70528_INT_DCIN2_OV_DET_MASK 0x8
> +#define BD70528_INT_DCIN2_RMV_MASK 0x10
> +#define BD70528_INT_DCIN2_DET_MASK 0x20
> +#define BD70528_INT_DCIN1_RMV_MASK 0x40
> +#define BD70528_INT_DCIN1_DET_MASK 0x80
> +
> +#define BD70528_INT_RTC_ALARM_MASK 0x1
> +#define BD70528_INT_ELPS_TIM_MASK 0x2
> +
> +#define BD70528_INT_GPIO0_MASK 0x1
> +#define BD70528_INT_GPIO1_MASK 0x2
> +#define BD70528_INT_GPIO2_MASK 0x4
> +#define BD70528_INT_GPIO3_MASK 0x8
> +
> +#define BD70528_INT_BUCK1_DVS_OPFAIL_MASK 0x1
> +#define BD70528_INT_BUCK2_DVS_OPFAIL_MASK 0x2
> +#define BD70528_INT_BUCK3_DVS_OPFAIL_MASK 0x4
> +#define BD70528_INT_LED1_VOLT_OPFAIL_MASK 0x10
> +#define BD70528_INT_LED2_VOLT_OPFAIL_MASK 0x20
> +
> +#define BD70528_DEBOUNCE_MASK 0x3
> +
> +#define BD70528_DEBOUNCE_DISABLE 0
> +#define BD70528_DEBOUNCE_15MS 1
> +#define BD70528_DEBOUNCE_30MS 2
> +#define BD70528_DEBOUNCE_50MS 3
> +
> +#define BD70528_GPIO_DRIVE_MASK 0x2
> +#define BD70528_GPIO_PUSH_PULL 0x0
> +#define BD70528_GPIO_OPEN_DRAIN 0x2
> +
> +#define BD70528_GPIO_OUT_EN_MASK 0x80
> +#define BD70528_GPIO_OUT_ENABLE 0x80
> +#define BD70528_GPIO_OUT_DISABLE 0x0
> +
> +#define BD70528_GPIO_OUT_HI 0x1
> +#define BD70528_GPIO_OUT_LO 0x0
> +#define BD70528_GPIO_OUT_MASK 0x1
> +
> +#define BD70528_GPIO_IN_STATE_BASE 1
> +
> +#define BD70528_CLK_OUT_EN_MASK 0x1
> +
> +/* RTC masks to mask out reserved bits */
> +
> +#define BD70528_MASK_RTC_SEC		0x7f
> +#define BD70528_MASK_RTC_MINUTE		0x7f
> +#define BD70528_MASK_RTC_HOUR_24H	0x80
> +#define BD70528_MASK_RTC_HOUR_PM	0x20
> +#define BD70528_MASK_RTC_HOUR		0x1f
> +#define BD70528_MASK_RTC_DAY		0x3f
> +#define BD70528_MASK_RTC_WEEK		0x07
> +#define BD70528_MASK_RTC_MONTH		0x1f
> +#define BD70528_MASK_RTC_YEAR		0xff
> +#define BD70528_MASK_RTC_COUNT_L	0x7f
> +
> +#define BD70528_MASK_ELAPSED_TIMER_EN	0x1
> +/* Mask second, min and hour fields
> + * HW would support ALM irq for over 24h
> + * (by setting day, month and year too)
> + * but as we wish to keep this same as for
> + * wake-up we limit ALM to 24H and only
> + * unmask sec, min and hour
> + */
> +#define BD70528_MASK_ALM_EN		0x7
> +#define BD70528_MASK_WAKE_EN		0x1
> +
> +/* WDT masks */
> +#define BD70528_MASK_WDT_EN		0x1
> +#define BD70528_MASK_WDT_HOUR		0x1
> +#define BD70528_MASK_WDT_MINUTE		0x7f
> +#define BD70528_MASK_WDT_SEC		0x7f
> +
> +#define BD70528_WDT_STATE_BIT		0x1
> +#define BD70528_ELAPSED_STATE_BIT	0x2
> +#define BD70528_WAKE_STATE_BIT		0x4
> +
> +/* Charger masks */
> +#define BD70528_MASK_CHG_STAT		0x7f
> +#define BD70528_MASK_CHG_BAT_TIMER	0x20
> +#define BD70528_MASK_CHG_BAT_OVERVOLT	0x10
> +#define BD70528_MASK_CHG_BAT_DETECT	0x1
> +#define BD70528_MASK_CHG_DCIN1_UVLO	0x1
> +#define BD70528_MASK_CHG_DCIN_ILIM	0x3f
> +#define BD70528_MASK_CHG_CHG_CURR	0x1f
> +#define BD70528_MASK_CHG_TRICKLE_CURR	0x10
> +
> +/*
> + * Note, external battery register is the lonely rider at
> + * address 0xc5. See how to stuff that in the regmap
> + */
> +#define BD70528_MAX_REGISTER 0x94
> +
> +/* Buck control masks */
> +#define BD70528_MASK_RUN_EN	0x4
> +#define BD70528_MASK_STBY_EN	0x2
> +#define BD70528_MASK_IDLE_EN	0x1
> +#define BD70528_MASK_LED1_EN	0x1
> +#define BD70528_MASK_LED2_EN	0x10
> +
> +#define BD70528_MASK_BUCK_VOLT	0xf
> +#define BD70528_MASK_LDO_VOLT	0x1f
> +#define BD70528_MASK_LED1_VOLT	0x1
> +#define BD70528_MASK_LED2_VOLT	0x10
> +
> +/* Misc irq masks */
> +#define BD70528_INT_MASK_SHORT_PUSH	1
> +#define BD70528_INT_MASK_AUTO_WAKE	2
> +#define BD70528_INT_MASK_POWER_STATE	4
> +
> +#define BD70528_MASK_BUCK_RAMP 0x10
> +#define BD70528_SIFT_BUCK_RAMP 4
> +
> +int bd70528_wdt_set(struct rohm_regmap_dev *data, int enable, int *old_state);
> +void bd70528_wdt_lock(struct rohm_regmap_dev *data);
> +void bd70528_wdt_unlock(struct rohm_regmap_dev *data);
> +
> +#endif /* __LINUX_MFD_BD70528_H__ */
Vaittinen, Matti April 3, 2019, 8:47 a.m. UTC | #2
Hello Lee,

Thanks for taking a look on this again =) I agree with most of the
comments and correct them at next version.

On Wed, Apr 03, 2019 at 08:31:52AM +0100, Lee Jones wrote:
> On Mon, 25 Mar 2019, Matti Vaittinen wrote:
> 
> > ROHM BD70528MWV is an ultra-low quiescent current general
> > purpose single-chip power management IC for battery-powered
> > portable devices.
> > 
> > Add MFD core which enables chip access for following subdevices:
> > 	- regulators/LED drivers
> > 	- battery-charger
> > 	- gpios
> > 	- 32.768kHz clk
> > 	- RTC
> > 	- watchdog
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > + * Mapping of main IRQ register bits to sub irq register offsets so
> 
> "sub-IRQ"
> 
> > + * that we can access corect sub IRQ registers based on bits that
> 
> "sub IRQ" is also fine, but please standardise.
> 
> I do prefer "sub-IRQ" though.

I'll go with "sub-IRQ" then

> > +
> > +#define WD_CTRL_MAGIC1 0x55
> > +#define WD_CTRL_MAGIC2 0xAA
> > +/**
> > + * bd70528_wdt_set - arm or disarm watchdog timer
> > + *
> > + * @data:	device data for the PMIC instance we want to operate on
> > + * @enable:	new state of WDT. zero to disable, non zero to enable
> > + * @old_state:	previous state of WDT will be filled here
> > + *
> > + * Arm or disarm WDT on BD70528 PMIC. Expected to be called only by
> > + * BD70528 RTC and BD70528 WDT drivers. The rtc_timer_lock must be taken
> > + * by calling bd70528_wdt_lock before calling bd70528_wdt_set.
> > + */
> > +int bd70528_wdt_set(struct rohm_regmap_dev *data, int enable, int *old_state)
> 
> Why doesn't this reside in the watchdog driver?

If my memory serves me right we shortly discussed this already during v8
review ;) Cant blame you though as I have seen some of the mail traffic
going through your inbox :D

The motivation to have the functions exported from MFD is to not create
sirect dependency between RTC and WDT. There may be cases where we want
to leave either RTC or WDT out of compilation. MFD is always needed so
the dependency from MFD to RTC/WDT does not harm.

(Here's some discussion necromancy if you are interested in re-reading
how we did end up with this implementation:
https://lore.kernel.org/lkml/20190212091723.GZ20638@dell/)

I hope you are still Ok with having the WDT control functions in MFD.

Best Regards
    Matti Vaittinen
Lee Jones April 3, 2019, 9:30 a.m. UTC | #3
On Wed, 03 Apr 2019, Matti Vaittinen wrote:

> Hello Lee,
> 
> Thanks for taking a look on this again =) I agree with most of the
> comments and correct them at next version.
> 
> On Wed, Apr 03, 2019 at 08:31:52AM +0100, Lee Jones wrote:
> > On Mon, 25 Mar 2019, Matti Vaittinen wrote:
> > 
> > > ROHM BD70528MWV is an ultra-low quiescent current general
> > > purpose single-chip power management IC for battery-powered
> > > portable devices.
> > > 
> > > Add MFD core which enables chip access for following subdevices:
> > > 	- regulators/LED drivers
> > > 	- battery-charger
> > > 	- gpios
> > > 	- 32.768kHz clk
> > > 	- RTC
> > > 	- watchdog
> > > 
> > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > > + * Mapping of main IRQ register bits to sub irq register offsets so
> > 
> > "sub-IRQ"
> > 
> > > + * that we can access corect sub IRQ registers based on bits that
> > 
> > "sub IRQ" is also fine, but please standardise.
> > 
> > I do prefer "sub-IRQ" though.
> 
> I'll go with "sub-IRQ" then
> 
> > > +
> > > +#define WD_CTRL_MAGIC1 0x55
> > > +#define WD_CTRL_MAGIC2 0xAA
> > > +/**
> > > + * bd70528_wdt_set - arm or disarm watchdog timer
> > > + *
> > > + * @data:	device data for the PMIC instance we want to operate on
> > > + * @enable:	new state of WDT. zero to disable, non zero to enable
> > > + * @old_state:	previous state of WDT will be filled here
> > > + *
> > > + * Arm or disarm WDT on BD70528 PMIC. Expected to be called only by
> > > + * BD70528 RTC and BD70528 WDT drivers. The rtc_timer_lock must be taken
> > > + * by calling bd70528_wdt_lock before calling bd70528_wdt_set.
> > > + */
> > > +int bd70528_wdt_set(struct rohm_regmap_dev *data, int enable, int *old_state)
> > 
> > Why doesn't this reside in the watchdog driver?
> 
> If my memory serves me right we shortly discussed this already during v8
> review ;) Cant blame you though as I have seen some of the mail traffic
> going through your inbox :D
> 
> The motivation to have the functions exported from MFD is to not create
> sirect dependency between RTC and WDT. There may be cases where we want
> to leave either RTC or WDT out of compilation. MFD is always needed so
> the dependency from MFD to RTC/WDT does not harm.
> 
> (Here's some discussion necromancy if you are interested in re-reading
> how we did end up with this implementation:
> https://lore.kernel.org/lkml/20190212091723.GZ20638@dell/)
> 
> I hope you are still Ok with having the WDT control functions in MFD.

OOI, why does the RTC need to control the WDT?
Vaittinen, Matti April 3, 2019, 10:10 a.m. UTC | #4
On Wed, Apr 03, 2019 at 10:30:15AM +0100, Lee Jones wrote:
> On Wed, 03 Apr 2019, Matti Vaittinen wrote:
> 
> > Hello Lee,
> > 
> > Thanks for taking a look on this again =) I agree with most of the
> > comments and correct them at next version.
> > 
> > On Wed, Apr 03, 2019 at 08:31:52AM +0100, Lee Jones wrote:
> > > On Mon, 25 Mar 2019, Matti Vaittinen wrote:
> > > 
> > > > ROHM BD70528MWV is an ultra-low quiescent current general
> > > > purpose single-chip power management IC for battery-powered
> > > > portable devices.
> > > > 
> > > > Add MFD core which enables chip access for following subdevices:
> > > > 	- regulators/LED drivers
> > > > 	- battery-charger
> > > > 	- gpios
> > > > 	- 32.768kHz clk
> > > > 	- RTC
> > > > 	- watchdog
> > > > 
> > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > > > + * Mapping of main IRQ register bits to sub irq register offsets so
> > > 
> > > "sub-IRQ"
> > > 
> > > > + * that we can access corect sub IRQ registers based on bits that
> > > 
> > > "sub IRQ" is also fine, but please standardise.
> > > 
> > > I do prefer "sub-IRQ" though.
> > 
> > I'll go with "sub-IRQ" then
> > 
> > > > +
> > > > +#define WD_CTRL_MAGIC1 0x55
> > > > +#define WD_CTRL_MAGIC2 0xAA
> > > > +/**
> > > > + * bd70528_wdt_set - arm or disarm watchdog timer
> > > > + *
> > > > + * @data:	device data for the PMIC instance we want to operate on
> > > > + * @enable:	new state of WDT. zero to disable, non zero to enable
> > > > + * @old_state:	previous state of WDT will be filled here
> > > > + *
> > > > + * Arm or disarm WDT on BD70528 PMIC. Expected to be called only by
> > > > + * BD70528 RTC and BD70528 WDT drivers. The rtc_timer_lock must be taken
> > > > + * by calling bd70528_wdt_lock before calling bd70528_wdt_set.
> > > > + */
> > > > +int bd70528_wdt_set(struct rohm_regmap_dev *data, int enable, int *old_state)
> > > 
> > > Why doesn't this reside in the watchdog driver?
> > 
> > If my memory serves me right we shortly discussed this already during v8
> > review ;) Cant blame you though as I have seen some of the mail traffic
> > going through your inbox :D
> > 
> > The motivation to have the functions exported from MFD is to not create
> > sirect dependency between RTC and WDT. There may be cases where we want
> > to leave either RTC or WDT out of compilation. MFD is always needed so
> > the dependency from MFD to RTC/WDT does not harm.
> > 
> > (Here's some discussion necromancy if you are interested in re-reading
> > how we did end up with this implementation:
> > https://lore.kernel.org/lkml/20190212091723.GZ20638@dell/)
> > 
> > I hope you are still Ok with having the WDT control functions in MFD.
> 
> OOI, why does the RTC need to control the WDT?

I thought I had a comment about this somewhere in code... O_o Must have
been in some development branch I had :/

Anyways, setting the RTC counter may cause watchdog to trigger. It is not
further explained why but I would guess watchdog uses RTC counter to check
if it should've been pinged already. So RTC needs to disable watch dog for
the duration of hwclock setting and enable it again after the new time is
set. I can add a comment about this to MFD driver if it helps :)

Br,
	Matti Vaittinen
Lee Jones April 3, 2019, 11:25 a.m. UTC | #5
On Wed, 03 Apr 2019, Matti Vaittinen wrote:

> On Wed, Apr 03, 2019 at 10:30:15AM +0100, Lee Jones wrote:
> > On Wed, 03 Apr 2019, Matti Vaittinen wrote:
> > 
> > > Hello Lee,
> > > 
> > > Thanks for taking a look on this again =) I agree with most of the
> > > comments and correct them at next version.
> > > 
> > > On Wed, Apr 03, 2019 at 08:31:52AM +0100, Lee Jones wrote:
> > > > On Mon, 25 Mar 2019, Matti Vaittinen wrote:
> > > > 
> > > > > ROHM BD70528MWV is an ultra-low quiescent current general
> > > > > purpose single-chip power management IC for battery-powered
> > > > > portable devices.
> > > > > 
> > > > > Add MFD core which enables chip access for following subdevices:
> > > > > 	- regulators/LED drivers
> > > > > 	- battery-charger
> > > > > 	- gpios
> > > > > 	- 32.768kHz clk
> > > > > 	- RTC
> > > > > 	- watchdog
> > > > > 
> > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > > > > + * Mapping of main IRQ register bits to sub irq register offsets so
> > > > 
> > > > "sub-IRQ"
> > > > 
> > > > > + * that we can access corect sub IRQ registers based on bits that
> > > > 
> > > > "sub IRQ" is also fine, but please standardise.
> > > > 
> > > > I do prefer "sub-IRQ" though.
> > > 
> > > I'll go with "sub-IRQ" then
> > > 
> > > > > +
> > > > > +#define WD_CTRL_MAGIC1 0x55
> > > > > +#define WD_CTRL_MAGIC2 0xAA
> > > > > +/**
> > > > > + * bd70528_wdt_set - arm or disarm watchdog timer
> > > > > + *
> > > > > + * @data:	device data for the PMIC instance we want to operate on
> > > > > + * @enable:	new state of WDT. zero to disable, non zero to enable
> > > > > + * @old_state:	previous state of WDT will be filled here
> > > > > + *
> > > > > + * Arm or disarm WDT on BD70528 PMIC. Expected to be called only by
> > > > > + * BD70528 RTC and BD70528 WDT drivers. The rtc_timer_lock must be taken
> > > > > + * by calling bd70528_wdt_lock before calling bd70528_wdt_set.
> > > > > + */
> > > > > +int bd70528_wdt_set(struct rohm_regmap_dev *data, int enable, int *old_state)
> > > > 
> > > > Why doesn't this reside in the watchdog driver?
> > > 
> > > If my memory serves me right we shortly discussed this already during v8
> > > review ;) Cant blame you though as I have seen some of the mail traffic
> > > going through your inbox :D
> > > 
> > > The motivation to have the functions exported from MFD is to not create
> > > sirect dependency between RTC and WDT. There may be cases where we want
> > > to leave either RTC or WDT out of compilation. MFD is always needed so
> > > the dependency from MFD to RTC/WDT does not harm.
> > > 
> > > (Here's some discussion necromancy if you are interested in re-reading
> > > how we did end up with this implementation:
> > > https://lore.kernel.org/lkml/20190212091723.GZ20638@dell/)
> > > 
> > > I hope you are still Ok with having the WDT control functions in MFD.
> > 
> > OOI, why does the RTC need to control the WDT?
> 
> I thought I had a comment about this somewhere in code... O_o Must have
> been in some development branch I had :/
> 
> Anyways, setting the RTC counter may cause watchdog to trigger. It is not
> further explained why but I would guess watchdog uses RTC counter to check
> if it should've been pinged already. So RTC needs to disable watch dog for
> the duration of hwclock setting and enable it again after the new time is
> set. I can add a comment about this to MFD driver if it helps :)

How does the user select between using the RTC and the WDT?

Or are the generally both enabled at the same time?
Vaittinen, Matti April 3, 2019, 11:45 a.m. UTC | #6
On Wed, 2019-04-03 at 12:25 +0100, Lee Jones wrote:
> On Wed, 03 Apr 2019, Matti Vaittinen wrote:
> 
> > On Wed, Apr 03, 2019 at 10:30:15AM +0100, Lee Jones wrote:
> > > On Wed, 03 Apr 2019, Matti Vaittinen wrote:
> > > 
> > > > Hello Lee,
> > > > 
> > > > Thanks for taking a look on this again =) I agree with most of
> > > > the
> > > > comments and correct them at next version.
> > > > 
> > > > On Wed, Apr 03, 2019 at 08:31:52AM +0100, Lee Jones wrote:
> > > > > On Mon, 25 Mar 2019, Matti Vaittinen wrote:
> > > > > 
> > > > > > ROHM BD70528MWV is an ultra-low quiescent current general
> > > > > > purpose single-chip power management IC for battery-powered
> > > > > > portable devices.
> > > > > > 
> > > > > > Add MFD core which enables chip access for following
> > > > > > subdevices:
> > > > > > 	- regulators/LED drivers
> > > > > > 	- battery-charger
> > > > > > 	- gpios
> > > > > > 	- 32.768kHz clk
> > > > > > 	- RTC
> > > > > > 	- watchdog
> > > > > > 
> > > > > > Signed-off-by: Matti Vaittinen <
> > > > > > matti.vaittinen@fi.rohmeurope.com>
> > > > > > + * Mapping of main IRQ register bits to sub irq register
> > > > > > offsets so
> > > > > 
> > > > > "sub-IRQ"
> > > > > 
> > > > > > + * that we can access corect sub IRQ registers based on
> > > > > > bits that
> > > > > 
> > > > > "sub IRQ" is also fine, but please standardise.
> > > > > 
> > > > > I do prefer "sub-IRQ" though.
> > > > 
> > > > I'll go with "sub-IRQ" then
> > > > 
> > > > > > +
> > > > > > +#define WD_CTRL_MAGIC1 0x55
> > > > > > +#define WD_CTRL_MAGIC2 0xAA
> > > > > > +/**
> > > > > > + * bd70528_wdt_set - arm or disarm watchdog timer
> > > > > > + *
> > > > > > + * @data:	device data for the PMIC instance we want to
> > > > > > operate on
> > > > > > + * @enable:	new state of WDT. zero to disable, non
> > > > > > zero to enable
> > > > > > + * @old_state:	previous state of WDT will be filled
> > > > > > here
> > > > > > + *
> > > > > > + * Arm or disarm WDT on BD70528 PMIC. Expected to be
> > > > > > called only by
> > > > > > + * BD70528 RTC and BD70528 WDT drivers. The rtc_timer_lock
> > > > > > must be taken
> > > > > > + * by calling bd70528_wdt_lock before calling
> > > > > > bd70528_wdt_set.
> > > > > > + */
> > > > > > +int bd70528_wdt_set(struct rohm_regmap_dev *data, int
> > > > > > enable, int *old_state)
> > > > > 
> > > > > Why doesn't this reside in the watchdog driver?
> > > > 
> > > > If my memory serves me right we shortly discussed this already
> > > > during v8
> > > > review ;) Cant blame you though as I have seen some of the mail
> > > > traffic
> > > > going through your inbox :D
> > > > 
> > > > The motivation to have the functions exported from MFD is to
> > > > not create
> > > > sirect dependency between RTC and WDT. There may be cases where
> > > > we want
> > > > to leave either RTC or WDT out of compilation. MFD is always
> > > > needed so
> > > > the dependency from MFD to RTC/WDT does not harm.
> > > > 
> > > > (Here's some discussion necromancy if you are interested in re-
> > > > reading
> > > > how we did end up with this implementation:
> > > > https://lore.kernel.org/lkml/20190212091723.GZ20638@dell/)
> > > > 
> > > > I hope you are still Ok with having the WDT control functions
> > > > in MFD.
> > > 
> > > OOI, why does the RTC need to control the WDT?
> > 
> > I thought I had a comment about this somewhere in code... O_o Must
> > have
> > been in some development branch I had :/
> > 
> > Anyways, setting the RTC counter may cause watchdog to trigger. It
> > is not
> > further explained why but I would guess watchdog uses RTC counter
> > to check
> > if it should've been pinged already. So RTC needs to disable watch
> > dog for
> > the duration of hwclock setting and enable it again after the new
> > time is
> > set. I can add a comment about this to MFD driver if it helps :)
> 
> How does the user select between using the RTC and the WDT?
> 
> Or are the generally both enabled at the same time?
> 

Both RTC and WDT can be enabled at the same time. But they are not
required to be used. When WDT is enabled, it uses current RTC time as
'base' (and RTC time is running no matter if we have the RTC driver
here or not) - and time-out gets scheduled to specified amount of time
into future. (Same setting timeout into the future happens when WDT is
pinged).

When we set RTC, we disable WDT (if it was enabled), set clock and re-
enable WDT. This causes the previously used time-out value to be set to
WDT again. This works Ok because BD70528 does not support 'short ping
detection'. Only side-effect will be one 'prolonged' WDT feeding period
when RTC is set. (absolute time when RTC was set minus absolute time
when previous WD ping or enable was done) longer than reqular period.

So user should not need to care about this 'dependency'. Basically the
only possible problem I see is that someone could accidentally hang the
system with something that keeps setting the RTC time - this would then
prevent watch dog from doing the reset. This, I believe, is a corner
case which I don't consider now - and if this is considered to be an
issue then such a system may disable the RTC driver and do RTC setting
in a what-ever-manner sees practical.

I'm not sure if I answered to question you asked though =)

Br,
	Matti Vaittinen
Lee Jones April 4, 2019, 2:52 a.m. UTC | #7
On Wed, 03 Apr 2019, Vaittinen, Matti wrote:

> On Wed, 2019-04-03 at 12:25 +0100, Lee Jones wrote:
> > On Wed, 03 Apr 2019, Matti Vaittinen wrote:
> > 
> > > On Wed, Apr 03, 2019 at 10:30:15AM +0100, Lee Jones wrote:
> > > > On Wed, 03 Apr 2019, Matti Vaittinen wrote:
> > > > 
> > > > > Hello Lee,
> > > > > 
> > > > > Thanks for taking a look on this again =) I agree with most of
> > > > > the
> > > > > comments and correct them at next version.
> > > > > 
> > > > > On Wed, Apr 03, 2019 at 08:31:52AM +0100, Lee Jones wrote:
> > > > > > On Mon, 25 Mar 2019, Matti Vaittinen wrote:
> > > > > > 
> > > > > > > ROHM BD70528MWV is an ultra-low quiescent current general
> > > > > > > purpose single-chip power management IC for battery-powered
> > > > > > > portable devices.
> > > > > > > 
> > > > > > > Add MFD core which enables chip access for following
> > > > > > > subdevices:
> > > > > > > 	- regulators/LED drivers
> > > > > > > 	- battery-charger
> > > > > > > 	- gpios
> > > > > > > 	- 32.768kHz clk
> > > > > > > 	- RTC
> > > > > > > 	- watchdog
> > > > > > > 
> > > > > > > Signed-off-by: Matti Vaittinen <
> > > > > > > matti.vaittinen@fi.rohmeurope.com>
> > > > > > > + * Mapping of main IRQ register bits to sub irq register
> > > > > > > offsets so
> > > > > > 
> > > > > > "sub-IRQ"
> > > > > > 
> > > > > > > + * that we can access corect sub IRQ registers based on
> > > > > > > bits that
> > > > > > 
> > > > > > "sub IRQ" is also fine, but please standardise.
> > > > > > 
> > > > > > I do prefer "sub-IRQ" though.
> > > > > 
> > > > > I'll go with "sub-IRQ" then
> > > > > 
> > > > > > > +
> > > > > > > +#define WD_CTRL_MAGIC1 0x55
> > > > > > > +#define WD_CTRL_MAGIC2 0xAA
> > > > > > > +/**
> > > > > > > + * bd70528_wdt_set - arm or disarm watchdog timer
> > > > > > > + *
> > > > > > > + * @data:	device data for the PMIC instance we want to
> > > > > > > operate on
> > > > > > > + * @enable:	new state of WDT. zero to disable, non
> > > > > > > zero to enable
> > > > > > > + * @old_state:	previous state of WDT will be filled
> > > > > > > here
> > > > > > > + *
> > > > > > > + * Arm or disarm WDT on BD70528 PMIC. Expected to be
> > > > > > > called only by
> > > > > > > + * BD70528 RTC and BD70528 WDT drivers. The rtc_timer_lock
> > > > > > > must be taken
> > > > > > > + * by calling bd70528_wdt_lock before calling
> > > > > > > bd70528_wdt_set.
> > > > > > > + */
> > > > > > > +int bd70528_wdt_set(struct rohm_regmap_dev *data, int
> > > > > > > enable, int *old_state)
> > > > > > 
> > > > > > Why doesn't this reside in the watchdog driver?
> > > > > 
> > > > > If my memory serves me right we shortly discussed this already
> > > > > during v8
> > > > > review ;) Cant blame you though as I have seen some of the mail
> > > > > traffic
> > > > > going through your inbox :D
> > > > > 
> > > > > The motivation to have the functions exported from MFD is to
> > > > > not create
> > > > > sirect dependency between RTC and WDT. There may be cases where
> > > > > we want
> > > > > to leave either RTC or WDT out of compilation. MFD is always
> > > > > needed so
> > > > > the dependency from MFD to RTC/WDT does not harm.
> > > > > 
> > > > > (Here's some discussion necromancy if you are interested in re-
> > > > > reading
> > > > > how we did end up with this implementation:
> > > > > https://lore.kernel.org/lkml/20190212091723.GZ20638@dell/)
> > > > > 
> > > > > I hope you are still Ok with having the WDT control functions
> > > > > in MFD.
> > > > 
> > > > OOI, why does the RTC need to control the WDT?
> > > 
> > > I thought I had a comment about this somewhere in code... O_o Must
> > > have
> > > been in some development branch I had :/
> > > 
> > > Anyways, setting the RTC counter may cause watchdog to trigger. It
> > > is not
> > > further explained why but I would guess watchdog uses RTC counter
> > > to check
> > > if it should've been pinged already. So RTC needs to disable watch
> > > dog for
> > > the duration of hwclock setting and enable it again after the new
> > > time is
> > > set. I can add a comment about this to MFD driver if it helps :)
> > 
> > How does the user select between using the RTC and the WDT?
> > 
> > Or are the generally both enabled at the same time?
> > 
> 
> Both RTC and WDT can be enabled at the same time. But they are not
> required to be used. When WDT is enabled, it uses current RTC time as
> 'base' (and RTC time is running no matter if we have the RTC driver
> here or not) - and time-out gets scheduled to specified amount of time
> into future. (Same setting timeout into the future happens when WDT is
> pinged).
> 
> When we set RTC, we disable WDT (if it was enabled), set clock and re-
> enable WDT. This causes the previously used time-out value to be set to
> WDT again. This works Ok because BD70528 does not support 'short ping
> detection'. Only side-effect will be one 'prolonged' WDT feeding period
> when RTC is set. (absolute time when RTC was set minus absolute time
> when previous WD ping or enable was done) longer than reqular period.
> 
> So user should not need to care about this 'dependency'. Basically the
> only possible problem I see is that someone could accidentally hang the
> system with something that keeps setting the RTC time - this would then
> prevent watch dog from doing the reset. This, I believe, is a corner
> case which I don't consider now - and if this is considered to be an
> issue then such a system may disable the RTC driver and do RTC setting
> in a what-ever-manner sees practical.
> 
> I'm not sure if I answered to question you asked though =)

I think you answered it just fine.

So my suggestion is to have the RTC depend on the WRT via Kconfig, and
place this WRT code in the WRT driver where it belongs.  These
functions can be exported from the WRT driver and the RTC can call
into them in the same way it calls into the MFD driver currently.

Avoiding a dependency on the WRT would seem strange, because there is
one.
Vaittinen, Matti April 4, 2019, 5:57 a.m. UTC | #8
On Thu, 2019-04-04 at 03:52 +0100, Lee Jones wrote:
> On Wed, 03 Apr 2019, Vaittinen, Matti wrote:
> 
> > On Wed, 2019-04-03 at 12:25 +0100, Lee Jones wrote:
> > > On Wed, 03 Apr 2019, Matti Vaittinen wrote:
> > > 
> > > > On Wed, Apr 03, 2019 at 10:30:15AM +0100, Lee Jones wrote:
> > > > > On Wed, 03 Apr 2019, Matti Vaittinen wrote:
> > > > > 
> > > > > > Hello Lee,
> > > > > > 
> > > > > > Thanks for taking a look on this again =) I agree with most
> > > > > > of
> > > > > > the
> > > > > > comments and correct them at next version.
> > > > > > 
> > > > > > On Wed, Apr 03, 2019 at 08:31:52AM +0100, Lee Jones wrote:
> > > > > > > On Mon, 25 Mar 2019, Matti Vaittinen wrote:
> > > > > > > 
> > > > > > > > ROHM BD70528MWV is an ultra-low quiescent current
> > > > > > > > general
> > > > > > > > purpose single-chip power management IC for battery-
> > > > > > > > powered
> > > > > > > > portable devices.
> > > > > > > > 
> > > > > > > > Add MFD core which enables chip access for following
> > > > > > > > subdevices:
> > > > > > > > 	- regulators/LED drivers
> > > > > > > > 	- battery-charger
> > > > > > > > 	- gpios
> > > > > > > > 	- 32.768kHz clk
> > > > > > > > 	- RTC
> > > > > > > > 	- watchdog
> > > > > > > > 
> > > > > > > > Signed-off-by: Matti Vaittinen <
> > > > > > > > matti.vaittinen@fi.rohmeurope.com>
> > > > > > > > + * Mapping of main IRQ register bits to sub irq
> > > > > > > > register
> > > > > > > > offsets so
> > > > > > > 
> > > > > > > "sub-IRQ"
> > > > > > > 
> > > > > > > > + * that we can access corect sub IRQ registers based
> > > > > > > > on
> > > > > > > > bits that
> > > > > > > 
> > > > > > > "sub IRQ" is also fine, but please standardise.
> > > > > > > 
> > > > > > > I do prefer "sub-IRQ" though.
> > > > > > 
> > > > > > I'll go with "sub-IRQ" then
> > > > > > 
> > > > > > > > +
> > > > > > > > +#define WD_CTRL_MAGIC1 0x55
> > > > > > > > +#define WD_CTRL_MAGIC2 0xAA
> > > > > > > > +/**
> > > > > > > > + * bd70528_wdt_set - arm or disarm watchdog timer
> > > > > > > > + *
> > > > > > > > + * @data:	device data for the PMIC instance we
> > > > > > > > want to
> > > > > > > > operate on
> > > > > > > > + * @enable:	new state of WDT. zero to disable, non
> > > > > > > > zero to enable
> > > > > > > > + * @old_state:	previous state of WDT will be filled
> > > > > > > > here
> > > > > > > > + *
> > > > > > > > + * Arm or disarm WDT on BD70528 PMIC. Expected to be
> > > > > > > > called only by
> > > > > > > > + * BD70528 RTC and BD70528 WDT drivers. The
> > > > > > > > rtc_timer_lock
> > > > > > > > must be taken
> > > > > > > > + * by calling bd70528_wdt_lock before calling
> > > > > > > > bd70528_wdt_set.
> > > > > > > > + */
> > > > > > > > +int bd70528_wdt_set(struct rohm_regmap_dev *data, int
> > > > > > > > enable, int *old_state)
> > > > > > > 
> > > > > > > Why doesn't this reside in the watchdog driver?
> > > > > > 
> > > > > > If my memory serves me right we shortly discussed this
> > > > > > already
> > > > > > during v8
> > > > > > review ;) Cant blame you though as I have seen some of the
> > > > > > mail
> > > > > > traffic
> > > > > > going through your inbox :D
> > > > > > 
> > > > > > The motivation to have the functions exported from MFD is
> > > > > > to
> > > > > > not create
> > > > > > sirect dependency between RTC and WDT. There may be cases
> > > > > > where
> > > > > > we want
> > > > > > to leave either RTC or WDT out of compilation. MFD is
> > > > > > always
> > > > > > needed so
> > > > > > the dependency from MFD to RTC/WDT does not harm.
> > > > > > 
> > > > > > (Here's some discussion necromancy if you are interested in
> > > > > > re-
> > > > > > reading
> > > > > > how we did end up with this implementation:
> > > > > > https://lore.kernel.org/lkml/20190212091723.GZ20638@dell/)
> > > > > > 
> > > > > > I hope you are still Ok with having the WDT control
> > > > > > functions
> > > > > > in MFD.
> > > > > 
> > > > > OOI, why does the RTC need to control the WDT?
> > > > 
> > > > I thought I had a comment about this somewhere in code... O_o
> > > > Must
> > > > have
> > > > been in some development branch I had :/
> > > > 
> > > > Anyways, setting the RTC counter may cause watchdog to trigger.
> > > > It
> > > > is not
> > > > further explained why but I would guess watchdog uses RTC
> > > > counter
> > > > to check
> > > > if it should've been pinged already. So RTC needs to disable
> > > > watch
> > > > dog for
> > > > the duration of hwclock setting and enable it again after the
> > > > new
> > > > time is
> > > > set. I can add a comment about this to MFD driver if it helps
> > > > :)
> > > 
> > > How does the user select between using the RTC and the WDT?
> > > 
> > > Or are the generally both enabled at the same time?
> > > 
> > 
> > Both RTC and WDT can be enabled at the same time. But they are not
> > required to be used. When WDT is enabled, it uses current RTC time
> > as
> > 'base' (and RTC time is running no matter if we have the RTC driver
> > here or not) - and time-out gets scheduled to specified amount of
> > time
> > into future. (Same setting timeout into the future happens when WDT
> > is
> > pinged).
> > 
> > When we set RTC, we disable WDT (if it was enabled), set clock and
> > re-
> > enable WDT. This causes the previously used time-out value to be
> > set to
> > WDT again. This works Ok because BD70528 does not support 'short
> > ping
> > detection'. Only side-effect will be one 'prolonged' WDT feeding
> > period
> > when RTC is set. (absolute time when RTC was set minus absolute
> > time
> > when previous WD ping or enable was done) longer than reqular
> > period.
> > 
> > So user should not need to care about this 'dependency'. Basically
> > the
> > only possible problem I see is that someone could accidentally hang
> > the
> > system with something that keeps setting the RTC time - this would
> > then
> > prevent watch dog from doing the reset. This, I believe, is a
> > corner
> > case which I don't consider now - and if this is considered to be
> > an
> > issue then such a system may disable the RTC driver and do RTC
> > setting
> > in a what-ever-manner sees practical.
> > 
> > I'm not sure if I answered to question you asked though =)
> 
> I think you answered it just fine.
> 
> So my suggestion is to have the RTC depend on the WRT via Kconfig,
> and
> place this WRT code in the WRT driver where it belongs.  These
> functions can be exported from the WRT driver and the RTC can call
> into them in the same way it calls into the MFD driver currently.

Yes, we could. But then we need to always compile the watch dog driver
when we want to use RTC. It is not a huge driver though so it probably
won't matter. So, I don't like this but I can do it so if you insist :]

> Avoiding a dependency on the WRT would seem strange, because there is
> one.

The dependency is artificial. It's caused by the current driver design.
If watchdog is not used, then RTC has no reason to touch the watchdog
block. RTC works just fine without watchdog. So from HW point there is
no dependency.

Actually, now that I thik of it the right way to do this would have
been the function pointer in parent data as was done in original patch
set. HW-colleagues tend to re-use HW blocks, and we like to re-use our
drivers. If the next PMIC from ROHM uses same RTC block but does not
provide watchdog - then it is cleanest solution to fall back to
function pointer and leave it to NULL when there is no WDT or when WDT
is unused. Another option is to export dummy function - which is not so
nice.

Additional benefit from function pointer would have been that the
function pointer can be only used by drivers which have acces to it.
This exported function is globally visible. The WDT disable/enable is
very specific procedure and it actually would be nicer design to not
have it visible globally. It is not intended to be used by anything
else besides the WDT and RTC here.

But I can't say there will be PMIC with same RTC and no WDT coming from
ROHM. Also, I am not terribly excited about the option of changing this
back to function-pointer as I already removed the pointer from parent
data and this changed parent data is already adapted to all sub drivers
- so this is all just babbling. Maybe it is just my huge ego shouting
there - 'I was right, I must have the final say'.

As a side note, I already did submit v12 with other styling fixes but
which left the WDT function in MFD. If you still see the WDT functions
should be exported from WDT - then please ignore the v12. I'll do v13
at the afternoon (my time, which is only a bit after noon your time I
guess) which will export these functions from WDT. (Well, I had to try
once more :D)

Br,
    Matti Vaittinen
Lee Jones April 4, 2019, 6:54 a.m. UTC | #9
On Thu, 04 Apr 2019, Vaittinen, Matti wrote:

> On Thu, 2019-04-04 at 03:52 +0100, Lee Jones wrote:
> > On Wed, 03 Apr 2019, Vaittinen, Matti wrote:
> > 
> > > On Wed, 2019-04-03 at 12:25 +0100, Lee Jones wrote:
> > > > On Wed, 03 Apr 2019, Matti Vaittinen wrote:
> > > > 
> > > > > On Wed, Apr 03, 2019 at 10:30:15AM +0100, Lee Jones wrote:
> > > > > > On Wed, 03 Apr 2019, Matti Vaittinen wrote:
> > > > > > 
> > > > > > > Hello Lee,
> > > > > > > 
> > > > > > > Thanks for taking a look on this again =) I agree with most
> > > > > > > of
> > > > > > > the
> > > > > > > comments and correct them at next version.
> > > > > > > 
> > > > > > > On Wed, Apr 03, 2019 at 08:31:52AM +0100, Lee Jones wrote:
> > > > > > > > On Mon, 25 Mar 2019, Matti Vaittinen wrote:
> > > > > > > > 
> > > > > > > > > ROHM BD70528MWV is an ultra-low quiescent current
> > > > > > > > > general
> > > > > > > > > purpose single-chip power management IC for battery-
> > > > > > > > > powered
> > > > > > > > > portable devices.
> > > > > > > > > 
> > > > > > > > > Add MFD core which enables chip access for following
> > > > > > > > > subdevices:
> > > > > > > > > 	- regulators/LED drivers
> > > > > > > > > 	- battery-charger
> > > > > > > > > 	- gpios
> > > > > > > > > 	- 32.768kHz clk
> > > > > > > > > 	- RTC
> > > > > > > > > 	- watchdog
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Matti Vaittinen <
> > > > > > > > > matti.vaittinen@fi.rohmeurope.com>
> > > > > > > > > + * Mapping of main IRQ register bits to sub irq
> > > > > > > > > register
> > > > > > > > > offsets so
> > > > > > > > 
> > > > > > > > "sub-IRQ"
> > > > > > > > 
> > > > > > > > > + * that we can access corect sub IRQ registers based
> > > > > > > > > on
> > > > > > > > > bits that
> > > > > > > > 
> > > > > > > > "sub IRQ" is also fine, but please standardise.
> > > > > > > > 
> > > > > > > > I do prefer "sub-IRQ" though.
> > > > > > > 
> > > > > > > I'll go with "sub-IRQ" then
> > > > > > > 
> > > > > > > > > +
> > > > > > > > > +#define WD_CTRL_MAGIC1 0x55
> > > > > > > > > +#define WD_CTRL_MAGIC2 0xAA
> > > > > > > > > +/**
> > > > > > > > > + * bd70528_wdt_set - arm or disarm watchdog timer
> > > > > > > > > + *
> > > > > > > > > + * @data:	device data for the PMIC instance we
> > > > > > > > > want to
> > > > > > > > > operate on
> > > > > > > > > + * @enable:	new state of WDT. zero to disable, non
> > > > > > > > > zero to enable
> > > > > > > > > + * @old_state:	previous state of WDT will be filled
> > > > > > > > > here
> > > > > > > > > + *
> > > > > > > > > + * Arm or disarm WDT on BD70528 PMIC. Expected to be
> > > > > > > > > called only by
> > > > > > > > > + * BD70528 RTC and BD70528 WDT drivers. The
> > > > > > > > > rtc_timer_lock
> > > > > > > > > must be taken
> > > > > > > > > + * by calling bd70528_wdt_lock before calling
> > > > > > > > > bd70528_wdt_set.
> > > > > > > > > + */
> > > > > > > > > +int bd70528_wdt_set(struct rohm_regmap_dev *data, int
> > > > > > > > > enable, int *old_state)
> > > > > > > > 
> > > > > > > > Why doesn't this reside in the watchdog driver?
> > > > > > > 
> > > > > > > If my memory serves me right we shortly discussed this
> > > > > > > already
> > > > > > > during v8
> > > > > > > review ;) Cant blame you though as I have seen some of the
> > > > > > > mail
> > > > > > > traffic
> > > > > > > going through your inbox :D
> > > > > > > 
> > > > > > > The motivation to have the functions exported from MFD is
> > > > > > > to
> > > > > > > not create
> > > > > > > sirect dependency between RTC and WDT. There may be cases
> > > > > > > where
> > > > > > > we want
> > > > > > > to leave either RTC or WDT out of compilation. MFD is
> > > > > > > always
> > > > > > > needed so
> > > > > > > the dependency from MFD to RTC/WDT does not harm.
> > > > > > > 
> > > > > > > (Here's some discussion necromancy if you are interested in
> > > > > > > re-
> > > > > > > reading
> > > > > > > how we did end up with this implementation:
> > > > > > > https://lore.kernel.org/lkml/20190212091723.GZ20638@dell/)
> > > > > > > 
> > > > > > > I hope you are still Ok with having the WDT control
> > > > > > > functions
> > > > > > > in MFD.
> > > > > > 
> > > > > > OOI, why does the RTC need to control the WDT?
> > > > > 
> > > > > I thought I had a comment about this somewhere in code... O_o
> > > > > Must
> > > > > have
> > > > > been in some development branch I had :/
> > > > > 
> > > > > Anyways, setting the RTC counter may cause watchdog to trigger.
> > > > > It
> > > > > is not
> > > > > further explained why but I would guess watchdog uses RTC
> > > > > counter
> > > > > to check
> > > > > if it should've been pinged already. So RTC needs to disable
> > > > > watch
> > > > > dog for
> > > > > the duration of hwclock setting and enable it again after the
> > > > > new
> > > > > time is
> > > > > set. I can add a comment about this to MFD driver if it helps
> > > > > :)
> > > > 
> > > > How does the user select between using the RTC and the WDT?
> > > > 
> > > > Or are the generally both enabled at the same time?
> > > > 
> > > 
> > > Both RTC and WDT can be enabled at the same time. But they are not
> > > required to be used. When WDT is enabled, it uses current RTC time
> > > as
> > > 'base' (and RTC time is running no matter if we have the RTC driver
> > > here or not) - and time-out gets scheduled to specified amount of
> > > time
> > > into future. (Same setting timeout into the future happens when WDT
> > > is
> > > pinged).
> > > 
> > > When we set RTC, we disable WDT (if it was enabled), set clock and
> > > re-
> > > enable WDT. This causes the previously used time-out value to be
> > > set to
> > > WDT again. This works Ok because BD70528 does not support 'short
> > > ping
> > > detection'. Only side-effect will be one 'prolonged' WDT feeding
> > > period
> > > when RTC is set. (absolute time when RTC was set minus absolute
> > > time
> > > when previous WD ping or enable was done) longer than reqular
> > > period.
> > > 
> > > So user should not need to care about this 'dependency'. Basically
> > > the
> > > only possible problem I see is that someone could accidentally hang
> > > the
> > > system with something that keeps setting the RTC time - this would
> > > then
> > > prevent watch dog from doing the reset. This, I believe, is a
> > > corner
> > > case which I don't consider now - and if this is considered to be
> > > an
> > > issue then such a system may disable the RTC driver and do RTC
> > > setting
> > > in a what-ever-manner sees practical.
> > > 
> > > I'm not sure if I answered to question you asked though =)
> > 
> > I think you answered it just fine.
> > 
> > So my suggestion is to have the RTC depend on the WRT via Kconfig,
> > and
> > place this WRT code in the WRT driver where it belongs.  These
> > functions can be exported from the WRT driver and the RTC can call
> > into them in the same way it calls into the MFD driver currently.
> 
> Yes, we could. But then we need to always compile the watch dog driver
> when we want to use RTC. It is not a huge driver though so it probably
> won't matter. So, I don't like this but I can do it so if you insist :]
> 
> > Avoiding a dependency on the WRT would seem strange, because there is
> > one.
> 
> The dependency is artificial. It's caused by the current driver design.
> If watchdog is not used, then RTC has no reason to touch the watchdog
> block. RTC works just fine without watchdog. So from HW point there is
> no dependency.

Great.

> Actually, now that I thik of it the right way to do this would have
> been the function pointer in parent data as was done in original patch
> set. HW-colleagues tend to re-use HW blocks, and we like to re-use our
> drivers. If the next PMIC from ROHM uses same RTC block but does not
> provide watchdog - then it is cleanest solution to fall back to
> function pointer and leave it to NULL when there is no WDT or when WDT
> is unused. Another option is to export dummy function - which is not so
> nice.

I think the converse is true.

Pointers to functions outside of a subsystem API context are generally
horrible.  It's much nicer to call a function which can be easily
stubbed out in a header file based on a Kconfig option.  It's how most
kernel APIs work.

Have a look for yourself how many there are:

 git grep -A5 inline -- include/linux/

> Additional benefit from function pointer would have been that the
> function pointer can be only used by drivers which have acces to it.
> This exported function is globally visible. The WDT disable/enable is
> very specific procedure and it actually would be nicer design to not
> have it visible globally. It is not intended to be used by anything
> else besides the WDT and RTC here.

Why would anything else try to use it?

There are 1000's of exported functions in the kernel.  If it's
properly namespaced a consumer would have to purposely call it, which
if they really wanted to, they could do anyway.  I don't really see
your point.

> But I can't say there will be PMIC with same RTC and no WDT coming from
> ROHM. Also, I am not terribly excited about the option of changing this
> back to function-pointer as I already removed the pointer from parent
> data and this changed parent data is already adapted to all sub drivers
> - so this is all just babbling. Maybe it is just my huge ego shouting
> there - 'I was right, I must have the final say'.

No, a call-back function would be a back-step.

Ego or no ego, you're wrong. =:-D

> As a side note, I already did submit v12 with other styling fixes but
> which left the WDT function in MFD. If you still see the WDT functions
> should be exported from WDT - then please ignore the v12. I'll do v13
> at the afternoon (my time, which is only a bit after noon your time I
> guess) which will export these functions from WDT. (Well, I had to try
> once more :D)

Please keep the WDT code in the WDT driver.  Create a little stub for
the cases where the WDT driver is not enabled - job done.
Vaittinen, Matti April 4, 2019, 7:24 a.m. UTC | #10
On Thu, Apr 04, 2019 at 07:54:52AM +0100, Lee Jones wrote:
> On Thu, 04 Apr 2019, Vaittinen, Matti wrote:
> 
> > Actually, now that I thik of it the right way to do this would have
> > been the function pointer in parent data as was done in original patch
> > set. HW-colleagues tend to re-use HW blocks, and we like to re-use our
> > drivers. If the next PMIC from ROHM uses same RTC block but does not
> > provide watchdog - then it is cleanest solution to fall back to
> > function pointer and leave it to NULL when there is no WDT or when WDT
> > is unused. Another option is to export dummy function - which is not so
> > nice.
> 
> I think the converse is true.
> 
> Pointers to functions outside of a subsystem API context are generally
> horrible.  It's much nicer to call a function which can be easily
> stubbed out in a header file based on a Kconfig option.  It's how most
> kernel APIs work.

I hate to admit but I see your point. This nicely solves any issues in
syncronizing the startup for driver providing function pointer and for
driver using it.

> > Additional benefit from function pointer would have been that the
> > function pointer can be only used by drivers which have acces to it.
> > This exported function is globally visible. The WDT disable/enable is
> > very specific procedure and it actually would be nicer design to not
> > have it visible globally. It is not intended to be used by anything
> > else besides the WDT and RTC here.
> 
> Why would anything else try to use it?
> 
> There are 1000's of exported functions in the kernel.  If it's
> properly namespaced a consumer would have to purposely call it, which
> if they really wanted to, they could do anyway.  I don't really see
> your point.

I could still argue on this. It _is_ less obvous that an exported function
is not meant to be publicly used than it is for function pointers. But
as you say, this is not a strong enough point to see the trouble in
synchronizing the WDT/RTC startup.

> > But I can't say there will be PMIC with same RTC and no WDT coming from
> > ROHM. Also, I am not terribly excited about the option of changing this
> > back to function-pointer as I already removed the pointer from parent
> > data and this changed parent data is already adapted to all sub drivers
> > - so this is all just babbling. Maybe it is just my huge ego shouting
> > there - 'I was right, I must have the final say'.
> 
> No, a call-back function would be a back-step.

You are probably right.

> Ego or no ego, you're wrong. =:-D

I'd rephrase that as "It's not that I am wrong, but you are right." =)

> > As a side note, I already did submit v12 with other styling fixes but
> > which left the WDT function in MFD. If you still see the WDT functions
> > should be exported from WDT - then please ignore the v12. I'll do v13
> > at the afternoon (my time, which is only a bit after noon your time I
> > guess) which will export these functions from WDT. (Well, I had to try
> > once more :D)
> 
> Please keep the WDT code in the WDT driver.  Create a little stub for
> the cases where the WDT driver is not enabled - job done.

Yes Sir.

Br,
	Matti Vaittinen
Alexandre Belloni April 4, 2019, 7:56 a.m. UTC | #11
On 04/04/2019 10:24:49+0300, Matti Vaittinen wrote:
> On Thu, Apr 04, 2019 at 07:54:52AM +0100, Lee Jones wrote:
> > On Thu, 04 Apr 2019, Vaittinen, Matti wrote:
> > 
> > > Actually, now that I thik of it the right way to do this would have
> > > been the function pointer in parent data as was done in original patch
> > > set. HW-colleagues tend to re-use HW blocks, and we like to re-use our
> > > drivers. If the next PMIC from ROHM uses same RTC block but does not
> > > provide watchdog - then it is cleanest solution to fall back to
> > > function pointer and leave it to NULL when there is no WDT or when WDT
> > > is unused. Another option is to export dummy function - which is not so
> > > nice.
> > 
> > I think the converse is true.
> > 
> > Pointers to functions outside of a subsystem API context are generally
> > horrible.  It's much nicer to call a function which can be easily
> > stubbed out in a header file based on a Kconfig option.  It's how most
> > kernel APIs work.
> 
> I hate to admit but I see your point. This nicely solves any issues in
> syncronizing the startup for driver providing function pointer and for
> driver using it.
> 

Wouldn't it be easier to register the watchdog driver as part of the RTC
driver?

As I see it, the wdt is just a glorified RTC alarm.
Lee Jones April 4, 2019, 8:06 a.m. UTC | #12
> > > ROHM. Also, I am not terribly excited about the option of changing this
> > > back to function-pointer as I already removed the pointer from parent
> > > data and this changed parent data is already adapted to all sub drivers
> > > - so this is all just babbling. Maybe it is just my huge ego shouting
> > > there - 'I was right, I must have the final say'.
> > 
> > No, a call-back function would be a back-step.
> 
> You are probably right.
> 
> > Ego or no ego, you're wrong. =:-D
> 
> I'd rephrase that as "It's not that I am wrong, but you are right." =)

Works for me.

> > > As a side note, I already did submit v12 with other styling fixes but
> > > which left the WDT function in MFD. If you still see the WDT functions
> > > should be exported from WDT - then please ignore the v12. I'll do v13
> > > at the afternoon (my time, which is only a bit after noon your time I
> > > guess) which will export these functions from WDT. (Well, I had to try
> > > once more :D)
> > 
> > Please keep the WDT code in the WDT driver.  Create a little stub for
> > the cases where the WDT driver is not enabled - job done.
> 
> Yes Sir.

=;-)
Vaittinen, Matti April 4, 2019, 8:10 a.m. UTC | #13
Hello Alexandre,

On Thu, 2019-04-04 at 09:56 +0200, Alexandre Belloni wrote:
> On 04/04/2019 10:24:49+0300, Matti Vaittinen wrote:
> > On Thu, Apr 04, 2019 at 07:54:52AM +0100, Lee Jones wrote:
> > > On Thu, 04 Apr 2019, Vaittinen, Matti wrote:
> > > 
> > > > Actually, now that I thik of it the right way to do this would
> > > > have
> > > > been the function pointer in parent data as was done in
> > > > original patch
> > > > set. HW-colleagues tend to re-use HW blocks, and we like to re-
> > > > use our
> > > > drivers. If the next PMIC from ROHM uses same RTC block but
> > > > does not
> > > > provide watchdog - then it is cleanest solution to fall back to
> > > > function pointer and leave it to NULL when there is no WDT or
> > > > when WDT
> > > > is unused. Another option is to export dummy function - which
> > > > is not so
> > > > nice.
> > > 
> > > I think the converse is true.
> > > 
> > > Pointers to functions outside of a subsystem API context are
> > > generally
> > > horrible.  It's much nicer to call a function which can be easily
> > > stubbed out in a header file based on a Kconfig option.  It's how
> > > most
> > > kernel APIs work.
> > 
> > I hate to admit but I see your point. This nicely solves any issues
> > in
> > syncronizing the startup for driver providing function pointer and
> > for
> > driver using it.
> > 
> 
> Wouldn't it be easier to register the watchdog driver as part of the
> RTC
> driver?
> 
> As I see it, the wdt is just a glorified RTC alarm.

Do you suggest me to put all the stuff now placed in
drivers/watchdog/bd70528_wdt.c into rtc driver? It would be doable -
but I'd rather kept the WDT independent module so that it can be left
out of config if WDT needs not to be used. And same with RTC. Also, re-
use of RTC driver in HW which does not include WDT is easier when WDT
is a separate module. To me it looks much cleaner to have the WDT as
own module than polluting the RTC driver with config ifdefs.

But from HW perspective you are correct. The WDT in BD70528 seems to be
kind of RTC alarm which shuts of the PMIC if triggered.

Br,
	Matti Vaittinen
Lee Jones April 4, 2019, 8:21 a.m. UTC | #14
On Thu, 04 Apr 2019, Vaittinen, Matti wrote:

> Hello Alexandre,
> 
> On Thu, 2019-04-04 at 09:56 +0200, Alexandre Belloni wrote:
> > On 04/04/2019 10:24:49+0300, Matti Vaittinen wrote:
> > > On Thu, Apr 04, 2019 at 07:54:52AM +0100, Lee Jones wrote:
> > > > On Thu, 04 Apr 2019, Vaittinen, Matti wrote:
> > > > 
> > > > > Actually, now that I thik of it the right way to do this would
> > > > > have
> > > > > been the function pointer in parent data as was done in
> > > > > original patch
> > > > > set. HW-colleagues tend to re-use HW blocks, and we like to re-
> > > > > use our
> > > > > drivers. If the next PMIC from ROHM uses same RTC block but
> > > > > does not
> > > > > provide watchdog - then it is cleanest solution to fall back to
> > > > > function pointer and leave it to NULL when there is no WDT or
> > > > > when WDT
> > > > > is unused. Another option is to export dummy function - which
> > > > > is not so
> > > > > nice.
> > > > 
> > > > I think the converse is true.
> > > > 
> > > > Pointers to functions outside of a subsystem API context are
> > > > generally
> > > > horrible.  It's much nicer to call a function which can be easily
> > > > stubbed out in a header file based on a Kconfig option.  It's how
> > > > most
> > > > kernel APIs work.
> > > 
> > > I hate to admit but I see your point. This nicely solves any issues
> > > in
> > > syncronizing the startup for driver providing function pointer and
> > > for
> > > driver using it.
> > > 
> > 
> > Wouldn't it be easier to register the watchdog driver as part of the
> > RTC
> > driver?
> > 
> > As I see it, the wdt is just a glorified RTC alarm.
> 
> Do you suggest me to put all the stuff now placed in
> drivers/watchdog/bd70528_wdt.c into rtc driver? It would be doable -
> but I'd rather kept the WDT independent module so that it can be left
> out of config if WDT needs not to be used. And same with RTC. Also, re-
> use of RTC driver in HW which does not include WDT is easier when WDT
> is a separate module. To me it looks much cleaner to have the WDT as
> own module than polluting the RTC driver with config ifdefs.

I haven't looked at the code, but I agree with this in principle.

I'm a firm believer of having functionality in the most appropriate
subsystem.  IMHO, if a device can be neatly split 9/10 it should be.

> But from HW perspective you are correct. The WDT in BD70528 seems to be
> kind of RTC alarm which shuts of the PMIC if triggered.
> 
> Br,
> 	Matti Vaittinen
diff mbox series

Patch

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 0ce2d8dfc5f1..2fbd6ed14329 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1866,6 +1866,23 @@  config MFD_ROHM_BD718XX
 	  NXP i.MX8. It contains 8 BUCK outputs and 7 LDOs, voltage monitoring
 	  and emergency shut down as well as 32,768KHz clock output.
 
+config MFD_ROHM_BD70528
+	tristate "ROHM BD70528 Power Management IC"
+	depends on I2C=y
+	depends on OF
+	select REGMAP_I2C
+	select REGMAP_IRQ
+	select MFD_CORE
+	help
+	  Select this option to get support for the ROHM BD70528 Power
+	  Management IC. BD71837 is general purpose single-chip power
+	  management IC for battery-powered portable devices. It contains
+	  3 ultra-low current consumption buck converters, 3 LDOs and 2 LED
+	  Drivers. Also included are 4 GPIOs, a real-time clock (RTC), a 32kHz
+	  crystal oscillator, high-accuracy VREF for use with an external ADC,
+	  10 bits SAR ADC for battery temperature monitor and 1S battery
+	  charger.
+
 config MFD_STM32_LPTIMER
 	tristate "Support for STM32 Low-Power Timer"
 	depends on (ARCH_STM32 && OF) || COMPILE_TEST
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index b4569ed7f3f3..dc1c9431c8d9 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -246,4 +246,5 @@  obj-$(CONFIG_MFD_MXS_LRADC)     += mxs-lradc.o
 obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
 obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
 obj-$(CONFIG_MFD_ROHM_BD718XX)	+= rohm-bd718x7.o
+obj-$(CONFIG_MFD_ROHM_BD70528)	+= rohm-bd70528.o
 
diff --git a/drivers/mfd/rohm-bd70528.c b/drivers/mfd/rohm-bd70528.c
new file mode 100644
index 000000000000..a46bbcdefce0
--- /dev/null
+++ b/drivers/mfd/rohm-bd70528.c
@@ -0,0 +1,438 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+//
+// Copyright (C) 2019 ROHM Semiconductors
+//
+// ROHM BD70528 PMIC driver
+
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/ioport.h>
+#include <linux/irq.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/rohm-bd70528.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+struct pmic_data {
+	struct rohm_regmap_dev chip;
+	struct mutex rtc_timer_lock;
+};
+
+static const struct resource rtc_irqs[] = {
+	DEFINE_RES_IRQ_NAMED(BD70528_INT_RTC_ALARM, "bd70528-rtc-alm"),
+	DEFINE_RES_IRQ_NAMED(BD70528_INT_ELPS_TIM, "bd70528-elapsed-timer"),
+};
+
+static const struct resource charger_irqs[] = {
+	DEFINE_RES_IRQ_NAMED(BD70528_INT_BAT_OV_RES, "bd70528-bat-ov-res"),
+	DEFINE_RES_IRQ_NAMED(BD70528_INT_BAT_OV_DET, "bd70528-bat-ov-det"),
+	DEFINE_RES_IRQ_NAMED(BD70528_INT_DBAT_DET, "bd70528-bat-dead"),
+	DEFINE_RES_IRQ_NAMED(BD70528_INT_BATTSD_COLD_RES, "bd70528-bat-warmed"),
+	DEFINE_RES_IRQ_NAMED(BD70528_INT_BATTSD_COLD_DET, "bd70528-bat-cold"),
+	DEFINE_RES_IRQ_NAMED(BD70528_INT_BATTSD_HOT_RES, "bd70528-bat-cooled"),
+	DEFINE_RES_IRQ_NAMED(BD70528_INT_BATTSD_HOT_DET, "bd70528-bat-hot"),
+	DEFINE_RES_IRQ_NAMED(BD70528_INT_CHG_TSD, "bd70528-chg-tshd"),
+	DEFINE_RES_IRQ_NAMED(BD70528_INT_BAT_RMV, "bd70528-bat-removed"),
+	DEFINE_RES_IRQ_NAMED(BD70528_INT_BAT_DET, "bd70528-bat-detected"),
+	DEFINE_RES_IRQ_NAMED(BD70528_INT_DCIN2_OV_RES, "bd70528-dcin2-ov-res"),
+	DEFINE_RES_IRQ_NAMED(BD70528_INT_DCIN2_OV_DET, "bd70528-dcin2-ov-det"),
+	DEFINE_RES_IRQ_NAMED(BD70528_INT_DCIN2_RMV, "bd70528-dcin2-removed"),
+	DEFINE_RES_IRQ_NAMED(BD70528_INT_DCIN2_DET, "bd70528-dcin2-detected"),
+	DEFINE_RES_IRQ_NAMED(BD70528_INT_DCIN1_RMV, "bd70528-dcin1-removed"),
+	DEFINE_RES_IRQ_NAMED(BD70528_INT_DCIN1_DET, "bd70528-dcin1-detected"),
+};
+
+static struct mfd_cell bd70528_mfd_cells[] = {
+	{ .name = "bd70528-pmic", },
+	{ .name = "bd70528-gpio", },
+	/*
+	 * We use BD71837 driver to drive the clk block. Only differences to
+	 * BD70528 clock gate are the register address and mask.
+	 */
+	{ .name = "bd718xx-clk", },
+	{ .name = "bd70528-wdt", },
+	{
+		.name = "bd70528-power",
+		.resources = charger_irqs,
+		.num_resources = ARRAY_SIZE(charger_irqs),
+	},
+	{
+		.name = "bd70528-rtc",
+		.resources = rtc_irqs,
+		.num_resources = ARRAY_SIZE(rtc_irqs),
+	},
+};
+
+static const struct regmap_range volatile_ranges[] = {
+	/* IRQ regs */
+	{
+		.range_min = BD70528_REG_INT_MAIN,
+		.range_max = BD70528_REG_INT_OP_FAIL,
+	},
+	/* RTC regs */
+	{
+		.range_min = BD70528_REG_RTC_COUNT_H,
+		.range_max = BD70528_REG_RTC_ALM_REPEAT,
+	},
+	/*
+	 * WDT control reg is special. Magic values must be
+	 * written to it in order to change the control. Should
+	 * not be cached.
+	 */
+	{
+		.range_min = BD70528_REG_WDT_CTRL,
+		.range_max = BD70528_REG_WDT_CTRL,
+	},
+	/*
+	 * BD70528 also contains a few other registers which require
+	 * magic sequences to be written in order to update the value.
+	 * At least SHIPMODE, HWRESET, WARMRESET,and STANDBY
+	 */
+	{
+		.range_min = BD70528_REG_SHIPMODE,
+		.range_max = BD70528_REG_STANDBY,
+	},
+};
+
+static const struct regmap_access_table volatile_regs = {
+	.yes_ranges = &volatile_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(volatile_ranges),
+};
+
+static struct regmap_config bd70528_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.volatile_table = &volatile_regs,
+	.max_register = BD70528_MAX_REGISTER,
+	.cache_type = REGCACHE_RBTREE,
+};
+
+/*
+ * Mapping of main IRQ register bits to sub irq register offsets so
+ * that we can access corect sub IRQ registers based on bits that
+ * are set in main IRQ register.
+ */
+
+/* bit [0] - Shutdown register */
+unsigned int bit0_offsets[] = {0};
+/* bit [1] - Power failure register */
+unsigned int bit1_offsets[] = {1};
+/* bit [2] - VR FAULT register */
+unsigned int bit2_offsets[] = {2};
+/* bit [3] - PMU register interrupts */
+unsigned int bit3_offsets[] = {3};
+/* bit [4] - Charger 1 and Charger 2 registers */
+unsigned int bit4_offsets[] = {4, 5};
+/* bit [5] - RTC register */
+unsigned int bit5_offsets[] = {6};
+/* bit [6] - GPIO register */
+unsigned int bit6_offsets[] = {7};
+/* bit [7] - Invalid operation register */
+unsigned int bit7_offsets[] = {8};
+
+static struct regmap_irq_sub_irq_map bd70528_sub_irq_offsets[] = {
+	REGMAP_IRQ_MAIN_REG_OFFSET(bit0_offsets),
+	REGMAP_IRQ_MAIN_REG_OFFSET(bit1_offsets),
+	REGMAP_IRQ_MAIN_REG_OFFSET(bit2_offsets),
+	REGMAP_IRQ_MAIN_REG_OFFSET(bit3_offsets),
+	REGMAP_IRQ_MAIN_REG_OFFSET(bit4_offsets),
+	REGMAP_IRQ_MAIN_REG_OFFSET(bit5_offsets),
+	REGMAP_IRQ_MAIN_REG_OFFSET(bit6_offsets),
+	REGMAP_IRQ_MAIN_REG_OFFSET(bit7_offsets),
+};
+
+static struct regmap_irq irqs[] = {
+	REGMAP_IRQ_REG(BD70528_INT_LONGPUSH, 0, BD70528_INT_LONGPUSH_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_WDT, 0, BD70528_INT_WDT_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_HWRESET, 0, BD70528_INT_HWRESET_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_RSTB_FAULT, 0, BD70528_INT_RSTB_FAULT_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_VBAT_UVLO, 0, BD70528_INT_VBAT_UVLO_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_TSD, 0, BD70528_INT_TSD_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_RSTIN, 0, BD70528_INT_RSTIN_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_BUCK1_FAULT, 1,
+		       BD70528_INT_BUCK1_FAULT_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_BUCK2_FAULT, 1,
+		       BD70528_INT_BUCK2_FAULT_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_BUCK3_FAULT, 1,
+		       BD70528_INT_BUCK3_FAULT_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_LDO1_FAULT, 1, BD70528_INT_LDO1_FAULT_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_LDO2_FAULT, 1, BD70528_INT_LDO2_FAULT_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_LDO3_FAULT, 1, BD70528_INT_LDO3_FAULT_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_LED1_FAULT, 1, BD70528_INT_LED1_FAULT_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_LED2_FAULT, 1, BD70528_INT_LED2_FAULT_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_BUCK1_OCP, 2, BD70528_INT_BUCK1_OCP_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_BUCK2_OCP, 2, BD70528_INT_BUCK2_OCP_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_BUCK3_OCP, 2, BD70528_INT_BUCK3_OCP_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_LED1_OCP, 2, BD70528_INT_LED1_OCP_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_LED2_OCP, 2, BD70528_INT_LED2_OCP_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_BUCK1_FULLON, 2,
+		       BD70528_INT_BUCK1_FULLON_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_BUCK2_FULLON, 2,
+		       BD70528_INT_BUCK2_FULLON_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_SHORTPUSH, 3, BD70528_INT_SHORTPUSH_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_AUTO_WAKEUP, 3,
+		       BD70528_INT_AUTO_WAKEUP_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_STATE_CHANGE, 3,
+		       BD70528_INT_STATE_CHANGE_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_BAT_OV_RES, 4, BD70528_INT_BAT_OV_RES_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_BAT_OV_DET, 4, BD70528_INT_BAT_OV_DET_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_DBAT_DET, 4, BD70528_INT_DBAT_DET_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_BATTSD_COLD_RES, 4,
+		       BD70528_INT_BATTSD_COLD_RES_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_BATTSD_COLD_DET, 4,
+		       BD70528_INT_BATTSD_COLD_DET_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_BATTSD_HOT_RES, 4,
+		       BD70528_INT_BATTSD_HOT_RES_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_BATTSD_HOT_DET, 4,
+		       BD70528_INT_BATTSD_HOT_DET_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_CHG_TSD, 4, BD70528_INT_CHG_TSD_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_BAT_RMV, 5, BD70528_INT_BAT_RMV_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_BAT_DET, 5, BD70528_INT_BAT_DET_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_DCIN2_OV_RES, 5,
+		       BD70528_INT_DCIN2_OV_RES_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_DCIN2_OV_DET, 5,
+		       BD70528_INT_DCIN2_OV_DET_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_DCIN2_RMV, 5, BD70528_INT_DCIN2_RMV_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_DCIN2_DET, 5, BD70528_INT_DCIN2_DET_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_DCIN1_RMV, 5, BD70528_INT_DCIN1_RMV_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_DCIN1_DET, 5, BD70528_INT_DCIN1_DET_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_RTC_ALARM, 6, BD70528_INT_RTC_ALARM_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_ELPS_TIM, 6, BD70528_INT_ELPS_TIM_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_GPIO0, 7, BD70528_INT_GPIO0_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_GPIO1, 7, BD70528_INT_GPIO1_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_GPIO2, 7, BD70528_INT_GPIO2_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_GPIO3, 7, BD70528_INT_GPIO3_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_BUCK1_DVS_OPFAIL, 8,
+		       BD70528_INT_BUCK1_DVS_OPFAIL_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_BUCK2_DVS_OPFAIL, 8,
+		       BD70528_INT_BUCK2_DVS_OPFAIL_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_BUCK3_DVS_OPFAIL, 8,
+		       BD70528_INT_BUCK3_DVS_OPFAIL_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_LED1_VOLT_OPFAIL, 8,
+		       BD70528_INT_LED1_VOLT_OPFAIL_MASK),
+	REGMAP_IRQ_REG(BD70528_INT_LED2_VOLT_OPFAIL, 8,
+		       BD70528_INT_LED2_VOLT_OPFAIL_MASK),
+};
+
+static struct regmap_irq_chip bd70528_irq_chip = {
+	.name = "bd70528_irq",
+	.main_status = BD70528_REG_INT_MAIN,
+	.irqs = &irqs[0],
+	.num_irqs = ARRAY_SIZE(irqs),
+	.status_base = BD70528_REG_INT_SHDN,
+	.mask_base = BD70528_REG_INT_SHDN_MASK,
+	.ack_base = BD70528_REG_INT_SHDN,
+	.type_base = BD70528_REG_GPIO1_IN,
+	.init_ack_masked = true,
+	.num_regs = 9,
+	.num_main_regs = 1,
+	.num_type_reg = 4,
+	.sub_reg_offsets = &bd70528_sub_irq_offsets[0],
+	.num_main_status_bits = 8,
+	.irq_reg_stride = 1,
+};
+
+#define WD_CTRL_MAGIC1 0x55
+#define WD_CTRL_MAGIC2 0xAA
+/**
+ * bd70528_wdt_set - arm or disarm watchdog timer
+ *
+ * @data:	device data for the PMIC instance we want to operate on
+ * @enable:	new state of WDT. zero to disable, non zero to enable
+ * @old_state:	previous state of WDT will be filled here
+ *
+ * Arm or disarm WDT on BD70528 PMIC. Expected to be called only by
+ * BD70528 RTC and BD70528 WDT drivers. The rtc_timer_lock must be taken
+ * by calling bd70528_wdt_lock before calling bd70528_wdt_set.
+ */
+int bd70528_wdt_set(struct rohm_regmap_dev *data, int enable, int *old_state)
+{
+	int ret, i;
+	unsigned int tmp;
+	struct pmic_data *bd70528 = container_of(data, struct pmic_data,
+						 chip);
+	u8 wd_ctrl_arr[3] = { WD_CTRL_MAGIC1, WD_CTRL_MAGIC2, 0 };
+	u8 *wd_ctrl = &wd_ctrl_arr[2];
+
+	ret = regmap_read(bd70528->chip.regmap, BD70528_REG_WDT_CTRL, &tmp);
+	if (ret)
+		return ret;
+
+	*wd_ctrl = (u8)tmp;
+
+	if (old_state) {
+		if (*wd_ctrl & BD70528_MASK_WDT_EN)
+			*old_state |= BD70528_WDT_STATE_BIT;
+		else
+			*old_state &= ~BD70528_WDT_STATE_BIT;
+		if ((!enable) == (!(*old_state & BD70528_WDT_STATE_BIT)))
+			return 0;
+	}
+
+	if (enable) {
+		if (*wd_ctrl & BD70528_MASK_WDT_EN)
+			return 0;
+		*wd_ctrl |= BD70528_MASK_WDT_EN;
+	} else {
+		if (*wd_ctrl & BD70528_MASK_WDT_EN)
+			*wd_ctrl &= ~BD70528_MASK_WDT_EN;
+		else
+			return 0;
+	}
+
+	for (i = 0; i < 3; i++) {
+		ret = regmap_write(bd70528->chip.regmap, BD70528_REG_WDT_CTRL,
+				   wd_ctrl_arr[i]);
+		if (ret)
+			return ret;
+	}
+
+	ret = regmap_read(bd70528->chip.regmap, BD70528_REG_WDT_CTRL, &tmp);
+	if ((tmp & BD70528_MASK_WDT_EN) != (*wd_ctrl & BD70528_MASK_WDT_EN)) {
+		dev_err(bd70528->chip.dev,
+			"Watchdog ctrl mismatch (hw) 0x%x (set) 0x%x\n",
+			tmp, *wd_ctrl);
+		ret = -EIO;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(bd70528_wdt_set);
+
+/**
+ * bd70528_wdt_lock - take WDT lock
+ *
+ * @bd70528:	device data for the PMIC instance we want to operate on
+ *
+ * Lock WDT for arming/disarming in order to avoid race condition caused
+ * by WDT state changes initiated by WDT and RTC drivers.
+ */
+void bd70528_wdt_lock(struct rohm_regmap_dev *data)
+{
+	struct pmic_data *bd70528 = container_of(data, struct pmic_data,
+						 chip);
+
+	mutex_lock(&bd70528->rtc_timer_lock);
+}
+EXPORT_SYMBOL(bd70528_wdt_lock);
+
+/**
+ * bd70528_wdt_unlock - unlock WDT lock
+ *
+ * @bd70528:	device data for the PMIC instance we want to operate on
+ *
+ * Unlock WDT lock which has previously been taken by call to
+ * bd70528_wdt_lock.
+ */
+void bd70528_wdt_unlock(struct rohm_regmap_dev *data)
+{
+	struct pmic_data *bd70528 = container_of(data, struct pmic_data,
+						 chip);
+
+	mutex_unlock(&bd70528->rtc_timer_lock);
+}
+EXPORT_SYMBOL(bd70528_wdt_unlock);
+
+#define BD70528_NUM_OF_GPIOS 4
+
+static int bd70528_i2c_probe(struct i2c_client *i2c,
+			    const struct i2c_device_id *id)
+{
+	struct pmic_data *bd70528;
+	struct regmap_irq_chip_data *irq_data;
+	int ret, i;
+
+	if (!i2c->irq) {
+		dev_err(&i2c->dev, "No IRQ configured\n");
+		return -EINVAL;
+	}
+
+	bd70528 = devm_kzalloc(&i2c->dev, sizeof(*bd70528), GFP_KERNEL);
+	if (!bd70528)
+		return -ENOMEM;
+
+	mutex_init(&bd70528->rtc_timer_lock);
+
+	dev_set_drvdata(&i2c->dev, &bd70528->chip);
+
+	bd70528->chip.chip_type = ROHM_CHIP_TYPE_BD70528;
+	bd70528->chip.regmap = devm_regmap_init_i2c(i2c, &bd70528_regmap);
+	if (IS_ERR(bd70528->chip.regmap)) {
+		dev_err(&i2c->dev, "regmap initialization failed\n");
+		return PTR_ERR(bd70528->chip.regmap);
+	}
+
+	/*
+	 * Disallow type setting for all IRQs by default as
+	 * most of them do not support setting type.
+	 */
+	for (i = 0; i < ARRAY_SIZE(irqs); i++)
+		irqs[i].type.types_supported = 0;
+
+	/*
+	 * Set IRQ typesetting information for GPIO pins 0 - 3
+	 */
+	for (i = 0; i < BD70528_NUM_OF_GPIOS; i++) {
+		struct regmap_irq_type *type;
+
+		type = &irqs[BD70528_INT_GPIO0 + i].type;
+		type->type_reg_offset = 2 * i;
+		type->type_rising_val = 0x20;
+		type->type_falling_val = 0x10;
+		type->type_level_high_val = 0x40;
+		type->type_level_low_val = 0x50;
+		type->types_supported = (IRQ_TYPE_EDGE_BOTH |
+				IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
+	}
+
+	ret = devm_regmap_add_irq_chip(&i2c->dev, bd70528->chip.regmap,
+				       i2c->irq, IRQF_ONESHOT, 0,
+				       &bd70528_irq_chip, &irq_data);
+	if (ret) {
+		dev_err(&i2c->dev, "Failed to add irq_chip\n");
+		return ret;
+	}
+	dev_dbg(&i2c->dev, "Registered %d irqs for chip\n",
+			bd70528_irq_chip.num_irqs);
+
+	/*
+	 * BD70528 IRQ controller is not touching the main mask register.
+	 * So enable the GPIO block interrupts at main level. We can just
+	 * leave them enabled as the IRQ controller should disable IRQs
+	 * from sub-registers when IRQ is disabled or freed.
+	 */
+	ret = regmap_update_bits(bd70528->chip.regmap,
+				 BD70528_REG_INT_MAIN_MASK,
+				 BD70528_INT_GPIO_MASK, 0);
+
+	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
+				   bd70528_mfd_cells,
+				   ARRAY_SIZE(bd70528_mfd_cells), NULL, 0,
+				   regmap_irq_get_domain(irq_data));
+	if (ret)
+		dev_err(&i2c->dev, "Failed to create subdevices\n");
+
+	return ret;
+}
+
+static const struct of_device_id bd70528_of_match[] = {
+	{ .compatible = "rohm,bd70528", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, bd70528_of_match);
+
+static struct i2c_driver bd70528_drv = {
+	.driver = {
+		.name = "rohm-bd70528",
+		.of_match_table = bd70528_of_match,
+	},
+	.probe = &bd70528_i2c_probe,
+};
+
+module_i2c_driver(bd70528_drv);
+
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_DESCRIPTION("ROHM BD70528 Power Management IC driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/rohm-bd70528.h b/include/linux/mfd/rohm-bd70528.h
new file mode 100644
index 000000000000..155dc9c89e13
--- /dev/null
+++ b/include/linux/mfd/rohm-bd70528.h
@@ -0,0 +1,383 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/* Copyright (C) 2018 ROHM Semiconductors */
+
+#ifndef __LINUX_MFD_BD70528_H__
+#define __LINUX_MFD_BD70528_H__
+
+#include <linux/device.h>
+#include <linux/mfd/rohm-generic.h>
+#include <linux/regmap.h>
+
+enum {
+	BD70528_BUCK1,
+	BD70528_BUCK2,
+	BD70528_BUCK3,
+	BD70528_LDO1,
+	BD70528_LDO2,
+	BD70528_LDO3,
+	BD70528_LED1,
+	BD70528_LED2,
+};
+
+#define BD70528_BUCK_VOLTS 17
+#define BD70528_BUCK_VOLTS 17
+#define BD70528_BUCK_VOLTS 17
+#define BD70528_LDO_VOLTS 0x20
+
+#define BD70528_REG_BUCK1_EN	0x0F
+#define BD70528_REG_BUCK1_VOLT	0x15
+#define BD70528_REG_BUCK2_EN	0x10
+#define BD70528_REG_BUCK2_VOLT	0x16
+#define BD70528_REG_BUCK3_EN	0x11
+#define BD70528_REG_BUCK3_VOLT	0x17
+#define BD70528_REG_LDO1_EN	0x1b
+#define BD70528_REG_LDO1_VOLT	0x1e
+#define BD70528_REG_LDO2_EN	0x1c
+#define BD70528_REG_LDO2_VOLT	0x1f
+#define BD70528_REG_LDO3_EN	0x1d
+#define BD70528_REG_LDO3_VOLT	0x20
+#define BD70528_REG_LED_CTRL	0x2b
+#define BD70528_REG_LED_VOLT	0x29
+#define BD70528_REG_LED_EN	0x2a
+
+/* main irq registers */
+#define BD70528_REG_INT_MAIN	0x7E
+#define BD70528_REG_INT_MAIN_MASK 0x74
+
+/* 'sub irq' registers */
+#define BD70528_REG_INT_SHDN	0x7F
+#define BD70528_REG_INT_PWR_FLT	0x80
+#define BD70528_REG_INT_VR_FLT	0x81
+#define BD70528_REG_INT_MISC	0x82
+#define BD70528_REG_INT_BAT1	0x83
+#define BD70528_REG_INT_BAT2	0x84
+#define BD70528_REG_INT_RTC	0x85
+#define BD70528_REG_INT_GPIO	0x86
+#define BD70528_REG_INT_OP_FAIL	0x87
+
+#define BD70528_REG_INT_SHDN_MASK	0x75
+#define BD70528_REG_INT_PWR_FLT_MASK	0x76
+#define BD70528_REG_INT_VR_FLT_MASK	0x77
+#define BD70528_REG_INT_MISC_MASK	0x78
+#define BD70528_REG_INT_BAT1_MASK	0x79
+#define BD70528_REG_INT_BAT2_MASK	0x7a
+#define BD70528_REG_INT_RTC_MASK	0x7b
+#define BD70528_REG_INT_GPIO_MASK	0x7c
+#define BD70528_REG_INT_OP_FAIL_MASK	0x7d
+
+/* Reset related 'magic' registers */
+#define BD70528_REG_SHIPMODE	0x03
+#define BD70528_REG_HWRESET	0x04
+#define BD70528_REG_WARMRESET	0x05
+#define BD70528_REG_STANDBY	0x06
+
+/* GPIO registers */
+#define BD70528_REG_GPIO_STATE	0x8F
+
+#define BD70528_REG_GPIO1_IN	0x4d
+#define BD70528_REG_GPIO2_IN	0x4f
+#define BD70528_REG_GPIO3_IN	0x51
+#define BD70528_REG_GPIO4_IN	0x53
+#define BD70528_REG_GPIO1_OUT	0x4e
+#define BD70528_REG_GPIO2_OUT	0x50
+#define BD70528_REG_GPIO3_OUT	0x52
+#define BD70528_REG_GPIO4_OUT	0x54
+
+/* clk control */
+
+#define BD70528_REG_CLK_OUT	0x2c
+
+/* RTC */
+
+#define BD70528_REG_RTC_COUNT_H		0x2d
+#define BD70528_REG_RTC_COUNT_L		0x2e
+#define BD70528_REG_RTC_SEC		0x2f
+#define BD70528_REG_RTC_MINUTE		0x30
+#define BD70528_REG_RTC_HOUR		0x31
+#define BD70528_REG_RTC_WEEK		0x32
+#define BD70528_REG_RTC_DAY		0x33
+#define BD70528_REG_RTC_MONTH		0x34
+#define BD70528_REG_RTC_YEAR		0x35
+
+#define BD70528_REG_RTC_ALM_SEC		0x36
+#define BD70528_REG_RTC_ALM_START	BD70528_REG_RTC_ALM_SEC
+#define BD70528_REG_RTC_ALM_MINUTE	0x37
+#define BD70528_REG_RTC_ALM_HOUR	0x38
+#define BD70528_REG_RTC_ALM_WEEK	0x39
+#define BD70528_REG_RTC_ALM_DAY		0x3a
+#define BD70528_REG_RTC_ALM_MONTH	0x3b
+#define BD70528_REG_RTC_ALM_YEAR	0x3c
+#define BD70528_REG_RTC_ALM_MASK	0x3d
+#define BD70528_REG_RTC_ALM_REPEAT	0x3e
+#define BD70528_REG_RTC_START		BD70528_REG_RTC_SEC
+
+#define BD70528_REG_RTC_WAKE_SEC	0x43
+#define BD70528_REG_RTC_WAKE_START	BD70528_REG_RTC_WAKE_SEC
+#define BD70528_REG_RTC_WAKE_MIN	0x44
+#define BD70528_REG_RTC_WAKE_HOUR	0x45
+#define BD70528_REG_RTC_WAKE_CTRL	0x46
+
+#define BD70528_REG_ELAPSED_TIMER_EN	0x42
+#define BD70528_REG_WAKE_EN		0x46
+
+/* WDT registers */
+#define BD70528_REG_WDT_CTRL		0x4A
+#define BD70528_REG_WDT_HOUR		0x49
+#define BD70528_REG_WDT_MINUTE		0x48
+#define BD70528_REG_WDT_SEC		0x47
+
+/* Charger / Battery */
+#define BD70528_REG_CHG_CURR_STAT	0x59
+#define BD70528_REG_CHG_BAT_STAT	0x57
+#define BD70528_REG_CHG_BAT_TEMP	0x58
+#define BD70528_REG_CHG_IN_STAT		0x56
+#define BD70528_REG_CHG_DCIN_ILIM	0x5d
+#define BD70528_REG_CHG_CHG_CURR_WARM	0x61
+#define BD70528_REG_CHG_CHG_CURR_COLD	0x62
+
+
+/* Masks for main IRQ register bits */
+enum {
+	BD70528_INT_SHDN,
+#define BD70528_INT_SHDN_MASK (1<<BD70528_INT_SHDN)
+	BD70528_INT_PWR_FLT,
+#define BD70528_INT_PWR_FLT_MASK (1<<BD70528_INT_PWR_FLT)
+	BD70528_INT_VR_FLT,
+#define BD70528_INT_VR_FLT_MASK (1<<BD70528_INT_VR_FLT)
+	BD70528_INT_MISC,
+#define BD70528_INT_MISC_MASK (1<<BD70528_INT_MISC)
+	BD70528_INT_BAT1,
+#define BD70528_INT_BAT1_MASK (1<<BD70528_INT_BAT1)
+	BD70528_INT_RTC,
+#define BD70528_INT_RTC_MASK (1<<BD70528_INT_RTC)
+	BD70528_INT_GPIO,
+#define BD70528_INT_GPIO_MASK (1<<BD70528_INT_GPIO)
+	BD70528_INT_OP_FAIL,
+#define BD70528_INT_OP_FAIL_MASK (1<<BD70528_INT_OP_FAIL)
+};
+
+/* IRQs */
+enum {
+	/* Shutdown register IRQs */
+	BD70528_INT_LONGPUSH,
+	BD70528_INT_WDT,
+	BD70528_INT_HWRESET,
+	BD70528_INT_RSTB_FAULT,
+	BD70528_INT_VBAT_UVLO,
+	BD70528_INT_TSD,
+	BD70528_INT_RSTIN,
+	/* Power failure register IRQs */
+	BD70528_INT_BUCK1_FAULT,
+	BD70528_INT_BUCK2_FAULT,
+	BD70528_INT_BUCK3_FAULT,
+	BD70528_INT_LDO1_FAULT,
+	BD70528_INT_LDO2_FAULT,
+	BD70528_INT_LDO3_FAULT,
+	BD70528_INT_LED1_FAULT,
+	BD70528_INT_LED2_FAULT,
+	/* VR FAULT register IRQs */
+	BD70528_INT_BUCK1_OCP,
+	BD70528_INT_BUCK2_OCP,
+	BD70528_INT_BUCK3_OCP,
+	BD70528_INT_LED1_OCP,
+	BD70528_INT_LED2_OCP,
+	BD70528_INT_BUCK1_FULLON,
+	BD70528_INT_BUCK2_FULLON,
+	/* PMU register interrupts */
+	BD70528_INT_SHORTPUSH,
+	BD70528_INT_AUTO_WAKEUP,
+	BD70528_INT_STATE_CHANGE,
+	/* Charger 1 register IRQs */
+	BD70528_INT_BAT_OV_RES,
+	BD70528_INT_BAT_OV_DET,
+	BD70528_INT_DBAT_DET,
+	BD70528_INT_BATTSD_COLD_RES,
+	BD70528_INT_BATTSD_COLD_DET,
+	BD70528_INT_BATTSD_HOT_RES,
+	BD70528_INT_BATTSD_HOT_DET,
+	BD70528_INT_CHG_TSD,
+	/* Charger 2 register IRQs */
+	BD70528_INT_BAT_RMV,
+	BD70528_INT_BAT_DET,
+	BD70528_INT_DCIN2_OV_RES,
+	BD70528_INT_DCIN2_OV_DET,
+	BD70528_INT_DCIN2_RMV,
+	BD70528_INT_DCIN2_DET,
+	BD70528_INT_DCIN1_RMV,
+	BD70528_INT_DCIN1_DET,
+	/* RTC register IRQs */
+	BD70528_INT_RTC_ALARM,
+	BD70528_INT_ELPS_TIM,
+	/* GPIO register IRQs */
+	BD70528_INT_GPIO0,
+	BD70528_INT_GPIO1,
+	BD70528_INT_GPIO2,
+	BD70528_INT_GPIO3,
+	/* Invalid operation register IRQs */
+	BD70528_INT_BUCK1_DVS_OPFAIL,
+	BD70528_INT_BUCK2_DVS_OPFAIL,
+	BD70528_INT_BUCK3_DVS_OPFAIL,
+	BD70528_INT_LED1_VOLT_OPFAIL,
+	BD70528_INT_LED2_VOLT_OPFAIL,
+};
+
+/* Masks */
+#define BD70528_INT_LONGPUSH_MASK 0x1
+#define BD70528_INT_WDT_MASK 0x2
+#define BD70528_INT_HWRESET_MASK 0x4
+#define BD70528_INT_RSTB_FAULT_MASK 0x8
+#define BD70528_INT_VBAT_UVLO_MASK 0x10
+#define BD70528_INT_TSD_MASK 0x20
+#define BD70528_INT_RSTIN_MASK 0x40
+
+#define BD70528_INT_BUCK1_FAULT_MASK 0x1
+#define BD70528_INT_BUCK2_FAULT_MASK 0x2
+#define BD70528_INT_BUCK3_FAULT_MASK 0x4
+#define BD70528_INT_LDO1_FAULT_MASK 0x8
+#define BD70528_INT_LDO2_FAULT_MASK 0x10
+#define BD70528_INT_LDO3_FAULT_MASK 0x20
+#define BD70528_INT_LED1_FAULT_MASK 0x40
+#define BD70528_INT_LED2_FAULT_MASK 0x80
+
+#define BD70528_INT_BUCK1_OCP_MASK 0x1
+#define BD70528_INT_BUCK2_OCP_MASK 0x2
+#define BD70528_INT_BUCK3_OCP_MASK 0x4
+#define BD70528_INT_LED1_OCP_MASK 0x8
+#define BD70528_INT_LED2_OCP_MASK 0x10
+#define BD70528_INT_BUCK1_FULLON_MASK 0x20
+#define BD70528_INT_BUCK2_FULLON_MASK 0x40
+
+#define BD70528_INT_SHORTPUSH_MASK 0x1
+#define BD70528_INT_AUTO_WAKEUP_MASK 0x2
+#define BD70528_INT_STATE_CHANGE_MASK 0x10
+
+#define BD70528_INT_BAT_OV_RES_MASK 0x1
+#define BD70528_INT_BAT_OV_DET_MASK 0x2
+#define BD70528_INT_DBAT_DET_MASK 0x4
+#define BD70528_INT_BATTSD_COLD_RES_MASK 0x8
+#define BD70528_INT_BATTSD_COLD_DET_MASK 0x10
+#define BD70528_INT_BATTSD_HOT_RES_MASK 0x20
+#define BD70528_INT_BATTSD_HOT_DET_MASK 0x40
+#define BD70528_INT_CHG_TSD_MASK 0x80
+
+#define BD70528_INT_BAT_RMV_MASK 0x1
+#define BD70528_INT_BAT_DET_MASK 0x2
+#define BD70528_INT_DCIN2_OV_RES_MASK 0x4
+#define BD70528_INT_DCIN2_OV_DET_MASK 0x8
+#define BD70528_INT_DCIN2_RMV_MASK 0x10
+#define BD70528_INT_DCIN2_DET_MASK 0x20
+#define BD70528_INT_DCIN1_RMV_MASK 0x40
+#define BD70528_INT_DCIN1_DET_MASK 0x80
+
+#define BD70528_INT_RTC_ALARM_MASK 0x1
+#define BD70528_INT_ELPS_TIM_MASK 0x2
+
+#define BD70528_INT_GPIO0_MASK 0x1
+#define BD70528_INT_GPIO1_MASK 0x2
+#define BD70528_INT_GPIO2_MASK 0x4
+#define BD70528_INT_GPIO3_MASK 0x8
+
+#define BD70528_INT_BUCK1_DVS_OPFAIL_MASK 0x1
+#define BD70528_INT_BUCK2_DVS_OPFAIL_MASK 0x2
+#define BD70528_INT_BUCK3_DVS_OPFAIL_MASK 0x4
+#define BD70528_INT_LED1_VOLT_OPFAIL_MASK 0x10
+#define BD70528_INT_LED2_VOLT_OPFAIL_MASK 0x20
+
+#define BD70528_DEBOUNCE_MASK 0x3
+
+#define BD70528_DEBOUNCE_DISABLE 0
+#define BD70528_DEBOUNCE_15MS 1
+#define BD70528_DEBOUNCE_30MS 2
+#define BD70528_DEBOUNCE_50MS 3
+
+#define BD70528_GPIO_DRIVE_MASK 0x2
+#define BD70528_GPIO_PUSH_PULL 0x0
+#define BD70528_GPIO_OPEN_DRAIN 0x2
+
+#define BD70528_GPIO_OUT_EN_MASK 0x80
+#define BD70528_GPIO_OUT_ENABLE 0x80
+#define BD70528_GPIO_OUT_DISABLE 0x0
+
+#define BD70528_GPIO_OUT_HI 0x1
+#define BD70528_GPIO_OUT_LO 0x0
+#define BD70528_GPIO_OUT_MASK 0x1
+
+#define BD70528_GPIO_IN_STATE_BASE 1
+
+#define BD70528_CLK_OUT_EN_MASK 0x1
+
+/* RTC masks to mask out reserved bits */
+
+#define BD70528_MASK_RTC_SEC		0x7f
+#define BD70528_MASK_RTC_MINUTE		0x7f
+#define BD70528_MASK_RTC_HOUR_24H	0x80
+#define BD70528_MASK_RTC_HOUR_PM	0x20
+#define BD70528_MASK_RTC_HOUR		0x1f
+#define BD70528_MASK_RTC_DAY		0x3f
+#define BD70528_MASK_RTC_WEEK		0x07
+#define BD70528_MASK_RTC_MONTH		0x1f
+#define BD70528_MASK_RTC_YEAR		0xff
+#define BD70528_MASK_RTC_COUNT_L	0x7f
+
+#define BD70528_MASK_ELAPSED_TIMER_EN	0x1
+/* Mask second, min and hour fields
+ * HW would support ALM irq for over 24h
+ * (by setting day, month and year too)
+ * but as we wish to keep this same as for
+ * wake-up we limit ALM to 24H and only
+ * unmask sec, min and hour
+ */
+#define BD70528_MASK_ALM_EN		0x7
+#define BD70528_MASK_WAKE_EN		0x1
+
+/* WDT masks */
+#define BD70528_MASK_WDT_EN		0x1
+#define BD70528_MASK_WDT_HOUR		0x1
+#define BD70528_MASK_WDT_MINUTE		0x7f
+#define BD70528_MASK_WDT_SEC		0x7f
+
+#define BD70528_WDT_STATE_BIT		0x1
+#define BD70528_ELAPSED_STATE_BIT	0x2
+#define BD70528_WAKE_STATE_BIT		0x4
+
+/* Charger masks */
+#define BD70528_MASK_CHG_STAT		0x7f
+#define BD70528_MASK_CHG_BAT_TIMER	0x20
+#define BD70528_MASK_CHG_BAT_OVERVOLT	0x10
+#define BD70528_MASK_CHG_BAT_DETECT	0x1
+#define BD70528_MASK_CHG_DCIN1_UVLO	0x1
+#define BD70528_MASK_CHG_DCIN_ILIM	0x3f
+#define BD70528_MASK_CHG_CHG_CURR	0x1f
+#define BD70528_MASK_CHG_TRICKLE_CURR	0x10
+
+/*
+ * Note, external battery register is the lonely rider at
+ * address 0xc5. See how to stuff that in the regmap
+ */
+#define BD70528_MAX_REGISTER 0x94
+
+/* Buck control masks */
+#define BD70528_MASK_RUN_EN	0x4
+#define BD70528_MASK_STBY_EN	0x2
+#define BD70528_MASK_IDLE_EN	0x1
+#define BD70528_MASK_LED1_EN	0x1
+#define BD70528_MASK_LED2_EN	0x10
+
+#define BD70528_MASK_BUCK_VOLT	0xf
+#define BD70528_MASK_LDO_VOLT	0x1f
+#define BD70528_MASK_LED1_VOLT	0x1
+#define BD70528_MASK_LED2_VOLT	0x10
+
+/* Misc irq masks */
+#define BD70528_INT_MASK_SHORT_PUSH	1
+#define BD70528_INT_MASK_AUTO_WAKE	2
+#define BD70528_INT_MASK_POWER_STATE	4
+
+#define BD70528_MASK_BUCK_RAMP 0x10
+#define BD70528_SIFT_BUCK_RAMP 4
+
+int bd70528_wdt_set(struct rohm_regmap_dev *data, int enable, int *old_state);
+void bd70528_wdt_lock(struct rohm_regmap_dev *data);
+void bd70528_wdt_unlock(struct rohm_regmap_dev *data);
+
+#endif /* __LINUX_MFD_BD70528_H__ */