[PATCH v9 0/2] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver

[PATCH v9 0/2] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver

[Matti Vaittinen]

Patch series adding support for ROHM BD71837 PMIC.

BD71837 is a programmable Power Management IC for powering single-core,
dual-core, and quad-core SoC’s such as NXP-i.MX 8M. It is optimized for
low BOM cost and compact solution footprint. It integrates 8 buck
regulators and 7 LDO’s to provide all the power rails required by the
SoC and the commonly used peripherals.

This is reduced set of patches containing only the MFD and devicetree
bindings. This enables alrady applied regulator part and power button
support using gpio-keys. Clock and reset support will be sent as separate
set of patches - possibly only after my vacations. Sorry for longish delay
which will follow.

Changelog v9
Fixes initiated by feedback from Enric Balletbo Serra, Lee and Dmitry.
- Simplified control flow for probe
- Removed accidentally left squash commit message
- Some comma presence toggling after last member in an array =)
- Styling of multi line comments.

Changelog v8
- Dropped clk-bd71837 from series (will send later)
- Dropped bd718xx-pwrkey driver and used gpio_keys instead.
- Added power-button short/long press duration configuratio to MFD
- Cleaned MFD driver according to comments from Enric Balletbo Serra.
  (used devm, removed unnecessary header inclusions, removed redundant
  assignment, styling issues, allow building MFD part as module, fixed
  license mismatch).

Changelog v7
- patch 1: Cleaned MFD probe since MFD no longer directly reads DT
  properties.
- patch 1/4: Moved power-key related definitions from powerkey patch (4)
  to MFD patch (1) so that powerkey can be applied independently
- Patch 2 is unchanged.
- patch 3: Added missing allocation check back to clk probe

Changelog v6
- Added power-key input driver
Based on feedback from Rob Herring and Stephen Boyd
- Added link to datasheet
- Removed interrupt-controller from DT and fixed binding document
- clk styling fixes
- remove clkdev usage
- add clk bindings to MFD documentation
- removed clk binding document

Changelog v5
- dropped regulator patches which are already applied to Mark's tree
Based on feedback from Rob Herring and Stephen Boyd
- mfd bindings: explain why this can be interrupt-controller
- mfd bindings: describe interrupts better
- mfd bindings: require one cell interrupt specifier
- mfd bindings: use generic node names in example
- mfd driver:   ack masked interrupt once at init
- clk bindings: use generic node names in example
- clk driver:   use devm
- clk driver:   use of_clk_add_hw_provider
- clk driver:   change severity of print and how prints are emitted at
                probe error path.
- clk driver:   dropped forward declared functions
- clk configs:  drop unnecessary dependencies
- clk driver:   other styling issues
- mfd/clk DT:   drop clk node.

Changelog v4
- remove mutex from regulator state check as core prevents simultaneous
  accesses
- allow voltage change for bucks 1 to 4 when regulator is enabled
- fix indentiation problems
- properly correct SPDX comments

Changelog v3
- kill unused variable
- kill unused definitions
- use REGMAP_IRQ_REG

Changelog v2
Based on feedback from Mark Brown
- Squashed code and buildfile changes to same patch
- Fixed some styling issues
- Changed SPDX comments to CPP style
- Error out if voltage is changed when regulator is enabled instead of
  Disabling the regulator for duration of change
- Use devm_regulator_register
- Remove compatible usage from regulators - use parent dev for config
- Add a note about using regulator-boot-on for BUCK6 and 7
- fixed warnings from kbuild test robot

patch 1:
        MFD driver and definitions bringing interrupt support and
        enabling clk, regulator and input subsystems.
patch 2:
        MFD driver DT bindings

This patch series is based on for-mfd-next

---

Matti Vaittinen (2):
  mfd: bd71837: mfd driver for ROHM BD71837 PMIC
  mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC

 .../devicetree/bindings/mfd/rohm,bd71837-pmic.txt  |  67 ++++
 drivers/mfd/Kconfig                                |  13 +
 drivers/mfd/Makefile                               |   1 +
 drivers/mfd/bd71837.c                              | 215 +++++++++++
 include/linux/mfd/bd71837.h                        | 391 +++++++++++++++++++++
 5 files changed, 687 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
 create mode 100644 drivers/mfd/bd71837.c
 create mode 100644 include/linux/mfd/bd71837.h

-- 
2.14.3

[PATCH v9 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC

[Matti Vaittinen]

ROHM BD71837 PMIC MFD driver providing interrupts and support
for three subsystems:
- clk
- Regulators
- input/power-key

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/mfd/Kconfig         |  13 ++
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/bd71837.c       | 215 ++++++++++++++++++++++++
 include/linux/mfd/bd71837.h | 391 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 620 insertions(+)
 create mode 100644 drivers/mfd/bd71837.c
 create mode 100644 include/linux/mfd/bd71837.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index b860eb5aa194..d01a279f5e4a 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1787,6 +1787,19 @@ config MFD_STW481X
 	  in various ST Microelectronics and ST-Ericsson embedded
 	  Nomadik series.
 
+config MFD_BD71837
+	tristate "BD71837 Power Management chip"
+	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 BD71837
+	  Power Management chips. BD71837 is designed to power processors like
+	  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_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 e9fd20dba18d..09dc9eb3782c 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -227,4 +227,5 @@ obj-$(CONFIG_MFD_STM32_TIMERS) 	+= stm32-timers.o
 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_BD71837)	+= bd71837.o
 
diff --git a/drivers/mfd/bd71837.c b/drivers/mfd/bd71837.c
new file mode 100644
index 000000000000..677097bcea17
--- /dev/null
+++ b/drivers/mfd/bd71837.c
@@ -0,0 +1,215 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright (C) 2018 ROHM Semiconductors
+// bd71837.c -- ROHM BD71837MWV mfd driver
+//
+// Datasheet available from
+// https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
+
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/bd71837.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/gpio_keys.h>
+
+static struct gpio_keys_button btns[] = {
+	{
+		.code = KEY_POWER,
+		.gpio = -1,
+		.type = EV_KEY,
+	},
+};
+
+static struct gpio_keys_platform_data bd718xx_powerkey_data = {
+	.buttons = &btns[0],
+	.nbuttons = ARRAY_SIZE(btns),
+	.name = "bd718xx-pwrkey",
+};
+
+static struct mfd_cell bd71837_mfd_cells[] = {
+	{
+		.name = "bd71837-clk",
+	}, {
+		.name = "gpio-keys",
+		.platform_data = &bd718xx_powerkey_data,
+		.pdata_size = sizeof(bd718xx_powerkey_data),
+	}, {
+		.name = "bd71837-pmic",
+	},
+};
+
+static const struct regmap_irq bd71837_irqs[] = {
+	REGMAP_IRQ_REG(BD71837_INT_SWRST, 0, BD71837_INT_SWRST_MASK),
+	REGMAP_IRQ_REG(BD71837_INT_PWRBTN_S, 0, BD71837_INT_PWRBTN_S_MASK),
+	REGMAP_IRQ_REG(BD71837_INT_PWRBTN_L, 0, BD71837_INT_PWRBTN_L_MASK),
+	REGMAP_IRQ_REG(BD71837_INT_PWRBTN, 0, BD71837_INT_PWRBTN_MASK),
+	REGMAP_IRQ_REG(BD71837_INT_WDOG, 0, BD71837_INT_WDOG_MASK),
+	REGMAP_IRQ_REG(BD71837_INT_ON_REQ, 0, BD71837_INT_ON_REQ_MASK),
+	REGMAP_IRQ_REG(BD71837_INT_STBY_REQ, 0, BD71837_INT_STBY_REQ_MASK),
+};
+
+static struct regmap_irq_chip bd71837_irq_chip = {
+	.name = "bd71837-irq",
+	.irqs = bd71837_irqs,
+	.num_irqs = ARRAY_SIZE(bd71837_irqs),
+	.num_regs = 1,
+	.irq_reg_stride = 1,
+	.status_base = BD71837_REG_IRQ,
+	.mask_base = BD71837_REG_MIRQ,
+	.ack_base = BD71837_REG_IRQ,
+	.init_ack_masked = true,
+	.mask_invert = false,
+};
+
+static const struct regmap_range pmic_status_range = {
+	.range_min = BD71837_REG_IRQ,
+	.range_max = BD71837_REG_POW_STATE,
+};
+
+static const struct regmap_access_table volatile_regs = {
+	.yes_ranges = &pmic_status_range,
+	.n_yes_ranges = 1,
+};
+
+static const struct regmap_config bd71837_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.volatile_table = &volatile_regs,
+	.max_register = BD71837_MAX_REGISTER - 1,
+	.cache_type = REGCACHE_RBTREE,
+};
+
+static const struct of_device_id bd71837_of_match[] = {
+	{ .compatible = "rohm,bd71837", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, bd71837_of_match);
+
+static int bd71837_i2c_probe(struct i2c_client *i2c,
+			    const struct i2c_device_id *id)
+{
+	struct bd71837 *bd71837;
+	struct bd71837_board *board_info;
+	int ret = -EINVAL;
+
+	bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
+
+	if (!bd71837)
+		return -ENOMEM;
+
+	board_info = dev_get_platdata(&i2c->dev);
+
+	if (!board_info)
+		bd71837->chip_irq = i2c->irq;
+	else
+		bd71837->chip_irq = board_info->gpio_intr;
+
+	if (!bd71837->chip_irq) {
+		dev_err(&i2c->dev, "No IRQ configured\n");
+		goto err_out;
+	}
+
+	bd71837->dev = &i2c->dev;
+	bd71837->i2c_client = i2c;
+	i2c_set_clientdata(i2c, bd71837);
+
+	bd71837->regmap = devm_regmap_init_i2c(i2c, &bd71837_regmap_config);
+	if (IS_ERR(bd71837->regmap)) {
+		ret = PTR_ERR(bd71837->regmap);
+		dev_err(&i2c->dev, "regmap initialization failed\n");
+		goto err_out;
+	}
+
+	ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
+	if (ret < 0) {
+		dev_err(&i2c->dev, "Read BD71837_REG_DEVICE failed\n");
+		goto err_out;
+	}
+
+	ret = devm_regmap_add_irq_chip(&i2c->dev, bd71837->regmap,
+				       bd71837->chip_irq, IRQF_ONESHOT, 0,
+				       &bd71837_irq_chip, &bd71837->irq_data);
+	if (ret < 0) {
+		dev_err(&i2c->dev, "Failed to add irq_chip\n");
+		goto err_out;
+	}
+
+	ret = regmap_update_bits(bd71837->regmap,
+				 BD71837_REG_PWRONCONFIG0,
+				 BD718XX_PWRBTN_PRESS_DURATION_MASK,
+				 BD718XX_PWRBTN_SHORT_PRESS_10MS);
+	if (ret < 0) {
+		dev_err(&i2c->dev, "Failed to configure button short press timeout\n");
+		goto err_out;
+	}
+
+	/*
+	 * According to BD71847 datasheet the HW default for long press
+	 * detection is 10ms. So lets change it to 10 sec so we can actually
+	 * get the short push and allow gracefull shut down
+	 */
+	ret = regmap_update_bits(bd71837->regmap,
+				 BD71837_REG_PWRONCONFIG1,
+				 BD718XX_PWRBTN_PRESS_DURATION_MASK,
+				 BD718XX_PWRBTN_LONG_PRESS_10S);
+
+	if (ret < 0) {
+		dev_err(&i2c->dev, "Failed to configure button long press timeout\n");
+		goto err_out;
+	}
+
+	btns[0].irq = regmap_irq_get_virq(bd71837->irq_data,
+					  BD71837_INT_PWRBTN_S);
+
+	if (btns[0].irq < 0) {
+		ret = btns[0].irq;
+		dev_err(&i2c->dev, "Failed to get the IRQ\n");
+		goto err_out;
+	}
+
+	ret = devm_mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
+				   bd71837_mfd_cells,
+				   ARRAY_SIZE(bd71837_mfd_cells), NULL, 0,
+				   regmap_irq_get_domain(bd71837->irq_data));
+	if (ret)
+		dev_err(&i2c->dev, "Failed to create subdevices\n");
+err_out:
+
+	return ret;
+}
+
+static const struct i2c_device_id bd71837_i2c_id[] = {
+	{ .name = "bd71837", },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id);
+
+static struct i2c_driver bd71837_i2c_driver = {
+	.driver = {
+		.name = "bd71837-mfd",
+		.of_match_table = bd71837_of_match,
+	},
+	.probe = bd71837_i2c_probe,
+	.id_table = bd71837_i2c_id,
+};
+
+static int __init bd71837_i2c_init(void)
+{
+	return i2c_add_driver(&bd71837_i2c_driver);
+}
+/*
+ * init early so consumer devices can complete system boot
+ */
+subsys_initcall(bd71837_i2c_init);
+
+static void __exit bd71837_i2c_exit(void)
+{
+	i2c_del_driver(&bd71837_i2c_driver);
+}
+module_exit(bd71837_i2c_exit);
+
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_DESCRIPTION("BD71837 chip multi-function driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/bd71837.h b/include/linux/mfd/bd71837.h
new file mode 100644
index 000000000000..10b0dfe90f27
--- /dev/null
+++ b/include/linux/mfd/bd71837.h
@@ -0,0 +1,391 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/* Copyright (C) 2018 ROHM Semiconductors */
+
+/*
+ * ROHM BD71837MWV header file
+ */
+
+#ifndef __LINUX_MFD_BD71837_H__
+#define __LINUX_MFD_BD71837_H__
+
+#include <linux/regmap.h>
+
+enum {
+	BD71837_BUCK1	=	0,
+	BD71837_BUCK2,
+	BD71837_BUCK3,
+	BD71837_BUCK4,
+	BD71837_BUCK5,
+	BD71837_BUCK6,
+	BD71837_BUCK7,
+	BD71837_BUCK8,
+	BD71837_LDO1,
+	BD71837_LDO2,
+	BD71837_LDO3,
+	BD71837_LDO4,
+	BD71837_LDO5,
+	BD71837_LDO6,
+	BD71837_LDO7,
+	BD71837_REGULATOR_CNT,
+};
+
+#define BD71837_BUCK1_VOLTAGE_NUM	0x40
+#define BD71837_BUCK2_VOLTAGE_NUM	0x40
+#define BD71837_BUCK3_VOLTAGE_NUM	0x40
+#define BD71837_BUCK4_VOLTAGE_NUM	0x40
+
+#define BD71837_BUCK5_VOLTAGE_NUM	0x08
+#define BD71837_BUCK6_VOLTAGE_NUM	0x04
+#define BD71837_BUCK7_VOLTAGE_NUM	0x08
+#define BD71837_BUCK8_VOLTAGE_NUM	0x40
+
+#define BD71837_LDO1_VOLTAGE_NUM	0x04
+#define BD71837_LDO2_VOLTAGE_NUM	0x02
+#define BD71837_LDO3_VOLTAGE_NUM	0x10
+#define BD71837_LDO4_VOLTAGE_NUM	0x10
+#define BD71837_LDO5_VOLTAGE_NUM	0x10
+#define BD71837_LDO6_VOLTAGE_NUM	0x10
+#define BD71837_LDO7_VOLTAGE_NUM	0x10
+
+enum {
+	BD71837_REG_REV                = 0x00,
+	BD71837_REG_SWRESET            = 0x01,
+	BD71837_REG_I2C_DEV            = 0x02,
+	BD71837_REG_PWRCTRL0           = 0x03,
+	BD71837_REG_PWRCTRL1           = 0x04,
+	BD71837_REG_BUCK1_CTRL         = 0x05,
+	BD71837_REG_BUCK2_CTRL         = 0x06,
+	BD71837_REG_BUCK3_CTRL         = 0x07,
+	BD71837_REG_BUCK4_CTRL         = 0x08,
+	BD71837_REG_BUCK5_CTRL         = 0x09,
+	BD71837_REG_BUCK6_CTRL         = 0x0A,
+	BD71837_REG_BUCK7_CTRL         = 0x0B,
+	BD71837_REG_BUCK8_CTRL         = 0x0C,
+	BD71837_REG_BUCK1_VOLT_RUN     = 0x0D,
+	BD71837_REG_BUCK1_VOLT_IDLE    = 0x0E,
+	BD71837_REG_BUCK1_VOLT_SUSP    = 0x0F,
+	BD71837_REG_BUCK2_VOLT_RUN     = 0x10,
+	BD71837_REG_BUCK2_VOLT_IDLE    = 0x11,
+	BD71837_REG_BUCK3_VOLT_RUN     = 0x12,
+	BD71837_REG_BUCK4_VOLT_RUN     = 0x13,
+	BD71837_REG_BUCK5_VOLT         = 0x14,
+	BD71837_REG_BUCK6_VOLT         = 0x15,
+	BD71837_REG_BUCK7_VOLT         = 0x16,
+	BD71837_REG_BUCK8_VOLT         = 0x17,
+	BD71837_REG_LDO1_VOLT          = 0x18,
+	BD71837_REG_LDO2_VOLT          = 0x19,
+	BD71837_REG_LDO3_VOLT          = 0x1A,
+	BD71837_REG_LDO4_VOLT          = 0x1B,
+	BD71837_REG_LDO5_VOLT          = 0x1C,
+	BD71837_REG_LDO6_VOLT          = 0x1D,
+	BD71837_REG_LDO7_VOLT          = 0x1E,
+	BD71837_REG_TRANS_COND0        = 0x1F,
+	BD71837_REG_TRANS_COND1        = 0x20,
+	BD71837_REG_VRFAULTEN          = 0x21,
+	BD71837_REG_MVRFLTMASK0        = 0x22,
+	BD71837_REG_MVRFLTMASK1        = 0x23,
+	BD71837_REG_MVRFLTMASK2        = 0x24,
+	BD71837_REG_RCVCFG             = 0x25,
+	BD71837_REG_RCVNUM             = 0x26,
+	BD71837_REG_PWRONCONFIG0       = 0x27,
+	BD71837_REG_PWRONCONFIG1       = 0x28,
+	BD71837_REG_RESETSRC           = 0x29,
+	BD71837_REG_MIRQ               = 0x2A,
+	BD71837_REG_IRQ                = 0x2B,
+	BD71837_REG_IN_MON             = 0x2C,
+	BD71837_REG_POW_STATE          = 0x2D,
+	BD71837_REG_OUT32K             = 0x2E,
+	BD71837_REG_REGLOCK            = 0x2F,
+	BD71837_REG_OTPVER             = 0xFF,
+	BD71837_MAX_REGISTER           = 0x100,
+};
+
+#define REGLOCK_PWRSEQ	0x1
+#define REGLOCK_VREG	0x10
+
+/* Generic BUCK control masks */
+#define BD71837_BUCK_SEL	0x02
+#define BD71837_BUCK_EN		0x01
+#define BD71837_BUCK_RUN_ON	0x04
+
+/* Generic LDO masks */
+#define BD71837_LDO_SEL		0x80
+#define BD71837_LDO_EN		0x40
+
+/* BD71837 BUCK ramp rate CTRL reg bits */
+#define BUCK_RAMPRATE_MASK	0xC0
+#define BUCK_RAMPRATE_10P00MV	0x0
+#define BUCK_RAMPRATE_5P00MV	0x1
+#define BUCK_RAMPRATE_2P50MV	0x2
+#define BUCK_RAMPRATE_1P25MV	0x3
+
+/* BD71837_REG_BUCK1_VOLT_RUN bits */
+#define BUCK1_RUN_MASK		0x3F
+#define BUCK1_RUN_DEFAULT	0x14
+
+/* BD71837_REG_BUCK1_VOLT_SUSP bits */
+#define BUCK1_SUSP_MASK		0x3F
+#define BUCK1_SUSP_DEFAULT	0x14
+
+/* BD71837_REG_BUCK1_VOLT_IDLE bits */
+#define BUCK1_IDLE_MASK		0x3F
+#define BUCK1_IDLE_DEFAULT	0x14
+
+/* BD71837_REG_BUCK2_VOLT_RUN bits */
+#define BUCK2_RUN_MASK		0x3F
+#define BUCK2_RUN_DEFAULT	0x1E
+
+/* BD71837_REG_BUCK2_VOLT_IDLE bits */
+#define BUCK2_IDLE_MASK		0x3F
+#define BUCK2_IDLE_DEFAULT	0x14
+
+/* BD71837_REG_BUCK3_VOLT_RUN bits */
+#define BUCK3_RUN_MASK		0x3F
+#define BUCK3_RUN_DEFAULT	0x1E
+
+/* BD71837_REG_BUCK4_VOLT_RUN bits */
+#define BUCK4_RUN_MASK		0x3F
+#define BUCK4_RUN_DEFAULT	0x1E
+
+/* BD71837_REG_BUCK5_VOLT bits */
+#define BUCK5_MASK		0x07
+#define BUCK5_DEFAULT		0x02
+
+/* BD71837_REG_BUCK6_VOLT bits */
+#define BUCK6_MASK		0x03
+#define BUCK6_DEFAULT		0x03
+
+/* BD71837_REG_BUCK7_VOLT bits */
+#define BUCK7_MASK		0x07
+#define BUCK7_DEFAULT		0x03
+
+/* BD71837_REG_BUCK8_VOLT bits */
+#define BUCK8_MASK		0x3F
+#define BUCK8_DEFAULT		0x1E
+
+/* BD71837_REG_IRQ bits */
+#define IRQ_SWRST		0x40
+#define IRQ_PWRON_S		0x20
+#define IRQ_PWRON_L		0x10
+#define IRQ_PWRON		0x08
+#define IRQ_WDOG		0x04
+#define IRQ_ON_REQ		0x02
+#define IRQ_STBY_REQ		0x01
+
+/* BD71837_REG_OUT32K bits */
+#define BD71837_OUT32K_EN	0x01
+
+/* BD71837 gated clock rate */
+#define BD71837_CLK_RATE 32768
+
+/* BD71837 irqs */
+enum {
+	BD71837_INT_STBY_REQ,
+	BD71837_INT_ON_REQ,
+	BD71837_INT_WDOG,
+	BD71837_INT_PWRBTN,
+	BD71837_INT_PWRBTN_L,
+	BD71837_INT_PWRBTN_S,
+	BD71837_INT_SWRST
+};
+
+/* BD71837 interrupt masks */
+#define BD71837_INT_SWRST_MASK		0x40
+#define BD71837_INT_PWRBTN_S_MASK	0x20
+#define BD71837_INT_PWRBTN_L_MASK	0x10
+#define BD71837_INT_PWRBTN_MASK		0x8
+#define BD71837_INT_WDOG_MASK		0x4
+#define BD71837_INT_ON_REQ_MASK		0x2
+#define BD71837_INT_STBY_REQ_MASK	0x1
+
+/* BD71837_REG_LDO1_VOLT bits */
+#define LDO1_MASK		0x03
+
+/* BD71837_REG_LDO1_VOLT bits */
+#define LDO2_MASK		0x20
+
+/* BD71837_REG_LDO3_VOLT bits */
+#define LDO3_MASK		0x0F
+
+/* BD71837_REG_LDO4_VOLT bits */
+#define LDO4_MASK		0x0F
+
+/* BD71837_REG_LDO5_VOLT bits */
+#define LDO5_MASK		0x0F
+
+/* BD71837_REG_LDO6_VOLT bits */
+#define LDO6_MASK		0x0F
+
+/* BD71837_REG_LDO7_VOLT bits */
+#define LDO7_MASK		0x0F
+
+/* Register write induced reset settings */
+
+/* Even though the bit zero is not SWRESET type we still want to write zero
+ * to it when changing type. Bit zero is 'SWRESET' trigger bit and if we
+ * write 1 to it we will trigger the action. So always write 0 to it when
+ * changning SWRESET action - no matter what we read from it.
+ */
+#define BD71837_SWRESET_TYPE_MASK 7
+#define BD71837_SWRESET_TYPE_DISABLED 0
+#define BD71837_SWRESET_TYPE_COLD 4
+#define BD71837_SWRESET_TYPE_WARM 6
+
+#define BD71837_SWRESET_RESET_MASK 1
+#define BD71837_SWRESET_RESET 1
+
+/* Poweroff state transition conditions */
+
+#define BD718XX_ON_REQ_POWEROFF_MASK 1
+#define BD718XX_SWRESET_POWEROFF_MASK 2
+#define BD718XX_WDOG_POWEROFF_MASK 4
+#define BD718XX_KEY_L_POWEROFF_MASK 8
+
+#define BD718XX_POWOFF_TO_SNVS 0
+#define BD718XX_POWOFF_TO_RDY 0xF
+
+#define BD718XX_POWOFF_TIME_MASK 0xF0
+enum {
+	BD718XX_POWOFF_TIME_5MS = 0,
+	BD718XX_POWOFF_TIME_10MS,
+	BD718XX_POWOFF_TIME_15MS,
+	BD718XX_POWOFF_TIME_20MS,
+	BD718XX_POWOFF_TIME_25MS,
+	BD718XX_POWOFF_TIME_30MS,
+	BD718XX_POWOFF_TIME_35MS,
+	BD718XX_POWOFF_TIME_40MS,
+	BD718XX_POWOFF_TIME_45MS,
+	BD718XX_POWOFF_TIME_50MS,
+	BD718XX_POWOFF_TIME_75MS,
+	BD718XX_POWOFF_TIME_100MS,
+	BD718XX_POWOFF_TIME_250MS,
+	BD718XX_POWOFF_TIME_500MS,
+	BD718XX_POWOFF_TIME_750MS,
+	BD718XX_POWOFF_TIME_1500MS
+};
+
+/* Poweron sequence state transition conditions */
+
+#define BD718XX_RDY_TO_SNVS_MASK 0xF
+#define BD718XX_SNVS_TO_RUN_MASK 0xF0
+
+#define BD718XX_PWR_TRIG_KEY_L 1
+#define BD718XX_PWR_TRIG_KEY_S 2
+#define BD718XX_PWR_TRIG_PMIC_ON 4
+#define BD718XX_PWR_TRIG_VSYS_UVLO 8
+#define BD718XX_RDY_TO_SNVS_SIFT 0
+#define BD718XX_SNVS_TO_RUN_SIFT 4
+
+
+#define BD718XX_PWRBTN_PRESS_DURATION_MASK 0xF
+
+/* Timeout value for detecting short press */
+
+enum {
+	BD718XX_PWRBTN_SHORT_PRESS_10MS = 0,
+	BD718XX_PWRBTN_SHORT_PRESS_500MS,
+	BD718XX_PWRBTN_SHORT_PRESS_1000MS,
+	BD718XX_PWRBTN_SHORT_PRESS_1500MS,
+	BD718XX_PWRBTN_SHORT_PRESS_2000MS,
+	BD718XX_PWRBTN_SHORT_PRESS_2500MS,
+	BD718XX_PWRBTN_SHORT_PRESS_3000MS,
+	BD718XX_PWRBTN_SHORT_PRESS_3500MS,
+	BD718XX_PWRBTN_SHORT_PRESS_4000MS,
+	BD718XX_PWRBTN_SHORT_PRESS_4500MS,
+	BD718XX_PWRBTN_SHORT_PRESS_5000MS,
+	BD718XX_PWRBTN_SHORT_PRESS_5500MS,
+	BD718XX_PWRBTN_SHORT_PRESS_6000MS,
+	BD718XX_PWRBTN_SHORT_PRESS_6500MS,
+	BD718XX_PWRBTN_SHORT_PRESS_7000MS,
+	BD718XX_PWRBTN_SHORT_PRESS_7500MS
+};
+
+/* Timeout value for detecting LONG press */
+
+enum {
+	BD718XX_PWRBTN_LONG_PRESS_10MS = 0,
+	BD718XX_PWRBTN_LONG_PRESS_1S,
+	BD718XX_PWRBTN_LONG_PRESS_2S,
+	BD718XX_PWRBTN_LONG_PRESS_3S,
+	BD718XX_PWRBTN_LONG_PRESS_4S,
+	BD718XX_PWRBTN_LONG_PRESS_5S,
+	BD718XX_PWRBTN_LONG_PRESS_6S,
+	BD718XX_PWRBTN_LONG_PRESS_7S,
+	BD718XX_PWRBTN_LONG_PRESS_8S,
+	BD718XX_PWRBTN_LONG_PRESS_9S,
+	BD718XX_PWRBTN_LONG_PRESS_10S,
+	BD718XX_PWRBTN_LONG_PRESS_11S,
+	BD718XX_PWRBTN_LONG_PRESS_12S,
+	BD718XX_PWRBTN_LONG_PRESS_13S,
+	BD718XX_PWRBTN_LONG_PRESS_14S,
+	BD718XX_PWRBTN_LONG_PRESS_15S
+};
+
+
+
+struct bd71837;
+struct bd71837_pmic;
+struct bd71837_clk;
+struct bd718xx_pwrkey;
+
+/*
+ * Board platform data may be used to initialize regulators.
+ */
+
+struct bd71837_board {
+	struct regulator_init_data *init_data[BD71837_REGULATOR_CNT];
+	int	gpio_intr;
+};
+
+struct bd71837 {
+	struct device *dev;
+	struct i2c_client *i2c_client;
+	struct regmap *regmap;
+	unsigned long int id;
+
+	int chip_irq;
+	struct regmap_irq_chip_data *irq_data;
+
+	struct bd71837_pmic *pmic;
+	struct bd71837_clk *clk;
+	struct bd718xx_pwrkey *pwrkey;
+};
+
+/*
+ * bd71837 sub-driver chip access routines
+ */
+
+static inline int bd71837_reg_read(struct bd71837 *bd71837, u8 reg)
+{
+	int r, val;
+
+	r = regmap_read(bd71837->regmap, reg, &val);
+	if (r < 0)
+		return r;
+	return val;
+}
+
+static inline int bd71837_reg_write(struct bd71837 *bd71837, u8 reg,
+				    unsigned int val)
+{
+	return regmap_write(bd71837->regmap, reg, val);
+}
+
+static inline int bd71837_set_bits(struct bd71837 *bd71837, u8 reg, u8 mask)
+{
+	return regmap_update_bits(bd71837->regmap, reg, mask, mask);
+}
+
+static inline int bd71837_clear_bits(struct bd71837 *bd71837, u8 reg,
+				     u8 mask)
+{
+	return regmap_update_bits(bd71837->regmap, reg, mask, 0);
+}
+
+static inline int bd71837_update_bits(struct bd71837 *bd71837, u8 reg,
+				      u8 mask, u8 val)
+{
+	return regmap_update_bits(bd71837->regmap, reg, mask, val);
+}
+
+#endif /* __LINUX_MFD_BD71837_H__ */
-- 
2.14.3

[PATCH v9 2/2] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC

[Matti Vaittinen]

Document devicetree bindings for ROHM BD71837 PMIC MFD.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/mfd/rohm,bd71837-pmic.txt  | 67 ++++++++++++++++++++++
 1 file changed, 67 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt

diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
new file mode 100644
index 000000000000..67f2616288d9
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
@@ -0,0 +1,67 @@
+* ROHM BD71837 Power Management Integrated Circuit bindings
+
+BD71837MWV is a programmable Power Management IC for powering single-core,
+dual-core, and quad-core SoC’s such as NXP-i.MX 8M. It is optimized for
+low BOM cost and compact solution footprint. It integrates 8 Buck
+egulators and 7 LDO’s to provide all the power rails required by the SoC and
+the commonly used peripherals.
+
+Datasheet for PMIC is available at:
+https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
+
+Required properties:
+ - compatible		: Should be "rohm,bd71837".
+ - reg			: I2C slave address.
+ - interrupt-parent	: Phandle to the parent interrupt controller.
+ - interrupts		: The interrupt line the device is connected to.
+ - clocks		: The parent clock connected to PMIC. If this is missng
+			  32768 KHz clock is assumed.
+ - #clock-cells		: Should be 0
+ - regulators:		: List of child nodes that specify the regulators
+			  Please see ../regulator/rohm,bd71837-regulator.txt
+
+Optional properties:
+- clock-output-names	: Should contain name for output clock.
+
+Example:
+
+	/* external oscillator node */
+	osc: oscillator {
+		compatible = "fixed-clock";
+		#clock-cells = <1>;
+		clock-frequency  = <32768>;
+		clock-output-names = "osc";
+	};
+
+	/* PMIC node */
+
+	pmic: pmic@4b {
+		compatible = "rohm,bd71837";
+		reg = <0x4b>;
+		interrupt-parent = <&gpio1>;
+		interrupts = <29 GPIO_ACTIVE_LOW>;
+		interrupt-names = "irq";
+		#clock-cells = <0>;
+		clocks = <&osc 0>;
+		clock-output-names = "bd71837-32k-out";
+
+		regulators {
+			buck1: BUCK1 {
+				regulator-name = "buck1";
+				regulator-min-microvolt = <700000>;
+				regulator-max-microvolt = <1300000>;
+				regulator-boot-on;
+				regulator-ramp-delay = <1250>;
+			};
+			/* ... */
+		};
+	};
+
+	/* Clock consumer node */
+
+	foo@0 {
+		compatible = "bar,foo";
+		/* ... */
+		clock-names = "my-clock";
+		clocks = <&pmic>;
+	};
-- 
2.14.3

Re: [PATCH v9 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC

[Enric Balletbo Serra]

Hi Matti,
Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
dia dj., 5 de jul. 2018 a les 9:46:
>
> ROHM BD71837 PMIC MFD driver providing interrupts and support
> for three subsystems:
> - clk
> - Regulators
> - input/power-key
>
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
>  drivers/mfd/Kconfig         |  13 ++
>  drivers/mfd/Makefile        |   1 +
>  drivers/mfd/bd71837.c       | 215 ++++++++++++++++++++++++
>  include/linux/mfd/bd71837.h | 391 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 620 insertions(+)
>  create mode 100644 drivers/mfd/bd71837.c
>  create mode 100644 include/linux/mfd/bd71837.h
>

(snip)

Looks good to me.
Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Best Regards,
 Enric

Re: [PATCH v9 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC

[Lee Jones]

On Thu, 05 Jul 2018, Matti Vaittinen wrote:

> ROHM BD71837 PMIC MFD driver providing interrupts and support
> for three subsystems:
> - clk
> - Regulators
> - input/power-key
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
>  drivers/mfd/Kconfig         |  13 ++
>  drivers/mfd/Makefile        |   1 +
>  drivers/mfd/bd71837.c       | 215 ++++++++++++++++++++++++
>  include/linux/mfd/bd71837.h | 391 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 620 insertions(+)
>  create mode 100644 drivers/mfd/bd71837.c
>  create mode 100644 include/linux/mfd/bd71837.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b860eb5aa194..d01a279f5e4a 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1787,6 +1787,19 @@ config MFD_STW481X
>  	  in various ST Microelectronics and ST-Ericsson embedded
>  	  Nomadik series.
>  
> +config MFD_BD71837

It's be nice if you prefix this with the manufacturer.  Rohm?

> +	tristate "BD71837 Power Management chip"

Again, missing manufacturer.

This should also be "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 BD71837
> +	  Power Management chips. BD71837 is designed to power processors like

"chips" is slang.  Should be "ICs".

> +	  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_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 e9fd20dba18d..09dc9eb3782c 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -227,4 +227,5 @@ obj-$(CONFIG_MFD_STM32_TIMERS) 	+= stm32-timers.o
>  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_BD71837)	+= bd71837.o
>  
> diff --git a/drivers/mfd/bd71837.c b/drivers/mfd/bd71837.c

rohm-bd71837.c would be nicer.

> new file mode 100644
> index 000000000000..677097bcea17
> --- /dev/null
> +++ b/drivers/mfd/bd71837.c
> @@ -0,0 +1,215 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later

'\n' here please.

> +// Copyright (C) 2018 ROHM Semiconductors

'\n' here please.

> +// bd71837.c -- ROHM BD71837MWV mfd driver

Remove the filename -- they have a habit of becoming incorrect.

Also, and 'MFD driver' isn't a thing -- this is a PMIC driver.

> +// Datasheet available from
> +// https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
> +
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/bd71837.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/gpio_keys.h>

Alphabetical please.

> +static struct gpio_keys_button btns[] = {
> +	{
> +		.code = KEY_POWER,
> +		.gpio = -1,
> +		.type = EV_KEY,
> +	},
> +};

Does this need to be an array?

> +static struct gpio_keys_platform_data bd718xx_powerkey_data = {
> +	.buttons = &btns[0],

Why not just "btns"?

Also, I don't see any reason not to just call this 'buttons'.

Or even, 'button' if there will only ever be one of them.

> +	.nbuttons = ARRAY_SIZE(btns),
> +	.name = "bd718xx-pwrkey",
> +};
> +
> +static struct mfd_cell bd71837_mfd_cells[] = {
> +	{
> +		.name = "bd71837-clk",
> +	}, {
> +		.name = "gpio-keys",
> +		.platform_data = &bd718xx_powerkey_data,
> +		.pdata_size = sizeof(bd718xx_powerkey_data),
> +	}, {
> +		.name = "bd71837-pmic",
> +	},
> +};

Place the one line entries on one line, like this:

	{ .name = "bd71837-pmic", },

... and if ordering is not important, place them at the bottom.

> +static const struct regmap_irq bd71837_irqs[] = {
> +	REGMAP_IRQ_REG(BD71837_INT_SWRST, 0, BD71837_INT_SWRST_MASK),
> +	REGMAP_IRQ_REG(BD71837_INT_PWRBTN_S, 0, BD71837_INT_PWRBTN_S_MASK),
> +	REGMAP_IRQ_REG(BD71837_INT_PWRBTN_L, 0, BD71837_INT_PWRBTN_L_MASK),
> +	REGMAP_IRQ_REG(BD71837_INT_PWRBTN, 0, BD71837_INT_PWRBTN_MASK),
> +	REGMAP_IRQ_REG(BD71837_INT_WDOG, 0, BD71837_INT_WDOG_MASK),
> +	REGMAP_IRQ_REG(BD71837_INT_ON_REQ, 0, BD71837_INT_ON_REQ_MASK),
> +	REGMAP_IRQ_REG(BD71837_INT_STBY_REQ, 0, BD71837_INT_STBY_REQ_MASK),
> +};
> +
> +static struct regmap_irq_chip bd71837_irq_chip = {
> +	.name = "bd71837-irq",
> +	.irqs = bd71837_irqs,
> +	.num_irqs = ARRAY_SIZE(bd71837_irqs),
> +	.num_regs = 1,
> +	.irq_reg_stride = 1,
> +	.status_base = BD71837_REG_IRQ,
> +	.mask_base = BD71837_REG_MIRQ,
> +	.ack_base = BD71837_REG_IRQ,
> +	.init_ack_masked = true,
> +	.mask_invert = false,
> +};
> +
> +static const struct regmap_range pmic_status_range = {
> +	.range_min = BD71837_REG_IRQ,
> +	.range_max = BD71837_REG_POW_STATE,
> +};
> +
> +static const struct regmap_access_table volatile_regs = {
> +	.yes_ranges = &pmic_status_range,
> +	.n_yes_ranges = 1,
> +};
> +
> +static const struct regmap_config bd71837_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.volatile_table = &volatile_regs,
> +	.max_register = BD71837_MAX_REGISTER - 1,
> +	.cache_type = REGCACHE_RBTREE,
> +};
> +
> +static const struct of_device_id bd71837_of_match[] = {
> +	{ .compatible = "rohm,bd71837", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, bd71837_of_match);
> +
> +static int bd71837_i2c_probe(struct i2c_client *i2c,
> +			    const struct i2c_device_id *id)
> +{
> +	struct bd71837 *bd71837;
> +	struct bd71837_board *board_info;
> +	int ret = -EINVAL;

I'd prefer it if you set the error at the location it happened for
readability.

> +	bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
> +

Drop this line.

> +	if (!bd71837)
> +		return -ENOMEM;
> +
> +	board_info = dev_get_platdata(&i2c->dev);
> +
> +	if (!board_info)
> +		bd71837->chip_irq = i2c->irq;
> +	else
> +		bd71837->chip_irq = board_info->gpio_intr;
> +
> +	if (!bd71837->chip_irq) {
> +		dev_err(&i2c->dev, "No IRQ configured\n");
> +		goto err_out;
> +	}
> +
> +	bd71837->dev = &i2c->dev;
> +	bd71837->i2c_client = i2c;
> +	i2c_set_clientdata(i2c, bd71837);
> +
> +	bd71837->regmap = devm_regmap_init_i2c(i2c, &bd71837_regmap_config);
> +	if (IS_ERR(bd71837->regmap)) {
> +		ret = PTR_ERR(bd71837->regmap);
> +		dev_err(&i2c->dev, "regmap initialization failed\n");
> +		goto err_out;
> +	}
> +
> +	ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
> +	if (ret < 0) {

Is 0 return a successful read?  I suggest that it isn't.

> +		dev_err(&i2c->dev, "Read BD71837_REG_DEVICE failed\n");
> +		goto err_out;
> +	}
> +
> +	ret = devm_regmap_add_irq_chip(&i2c->dev, bd71837->regmap,
> +				       bd71837->chip_irq, IRQF_ONESHOT, 0,
> +				       &bd71837_irq_chip, &bd71837->irq_data);
> +	if (ret < 0) {

Is a >0 return valid?  If not, perhaps "if (ret)" would be better.

> +		dev_err(&i2c->dev, "Failed to add irq_chip\n");
> +		goto err_out;
> +	}
> +
> +	ret = regmap_update_bits(bd71837->regmap,
> +				 BD71837_REG_PWRONCONFIG0,
> +				 BD718XX_PWRBTN_PRESS_DURATION_MASK,
> +				 BD718XX_PWRBTN_SHORT_PRESS_10MS);
> +	if (ret < 0) {

As above.

> +		dev_err(&i2c->dev, "Failed to configure button short press timeout\n");
> +		goto err_out;
> +	}
> +
> +	/*
> +	 * According to BD71847 datasheet the HW default for long press
> +	 * detection is 10ms. So lets change it to 10 sec so we can actually
> +	 * get the short push and allow gracefull shut down
> +	 */

I don't think this comment is necessary.

If anything it only needs to say "Configure long press to 10 seconds".

And if you do that, please add one for short press too.

> +	ret = regmap_update_bits(bd71837->regmap,
> +				 BD71837_REG_PWRONCONFIG1,
> +				 BD718XX_PWRBTN_PRESS_DURATION_MASK,
> +				 BD718XX_PWRBTN_LONG_PRESS_10S);
> +
> +	if (ret < 0) {
> +		dev_err(&i2c->dev, "Failed to configure button long press timeout\n");
> +		goto err_out;
> +	}
> +
> +	btns[0].irq = regmap_irq_get_virq(bd71837->irq_data,
> +					  BD71837_INT_PWRBTN_S);
> +
> +	if (btns[0].irq < 0) {
> +		ret = btns[0].irq;
> +		dev_err(&i2c->dev, "Failed to get the IRQ\n");
> +		goto err_out;

No unwinding to be done, just return directly and save yourself an
superflouous goto.

> +	}
> +
> +	ret = devm_mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
> +				   bd71837_mfd_cells,
> +				   ARRAY_SIZE(bd71837_mfd_cells), NULL, 0,
> +				   regmap_irq_get_domain(bd71837->irq_data));
> +	if (ret)
> +		dev_err(&i2c->dev, "Failed to create subdevices\n");
> +err_out:
> +
> +	return ret;
> +}
> +
> +static const struct i2c_device_id bd71837_i2c_id[] = {
> +	{ .name = "bd71837", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id);

You shouldn't need this table anymore.

See da10c06a044b ("i2c: Make I2C ID tables non-mandatory for DT'ed devices")

> +static struct i2c_driver bd71837_i2c_driver = {
> +	.driver = {
> +		.name = "bd71837-mfd",

And "MFD" isn't really a thing.  It's more common to just mention the
board name, or something like 'rohm-bd7183'.

> +		.of_match_table = bd71837_of_match,
> +	},
> +	.probe = bd71837_i2c_probe,
> +	.id_table = bd71837_i2c_id,
> +};
> +
> +static int __init bd71837_i2c_init(void)
> +{
> +	return i2c_add_driver(&bd71837_i2c_driver);
> +}
> +/*
> + * init early so consumer devices can complete system boot
> + */

This is a single line commit.  Please use the correct format.

  /* Single line comment */

> +subsys_initcall(bd71837_i2c_init);
> +
> +static void __exit bd71837_i2c_exit(void)
> +{
> +	i2c_del_driver(&bd71837_i2c_driver);
> +}
> +module_exit(bd71837_i2c_exit);
> +
> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> +MODULE_DESCRIPTION("BD71837 chip multi-function driver");

'PMIC' or 'Power Management IC' rather than MFD.

> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/bd71837.h b/include/linux/mfd/bd71837.h
> new file mode 100644
> index 000000000000..10b0dfe90f27
> --- /dev/null
> +++ b/include/linux/mfd/bd71837.h
> @@ -0,0 +1,391 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */

I thought these were meant to use C++ (//) comments?

> +/* Copyright (C) 2018 ROHM Semiconductors */
> +
> +/*
> + * ROHM BD71837MWV header file
> + */

If you prefix the mentions of (BD,bd)71837 with (ROHM,rohm) then this
comment becomes redundant and you can remove it.

> +#ifndef __LINUX_MFD_BD71837_H__
> +#define __LINUX_MFD_BD71837_H__
> +
> +#include <linux/regmap.h>
> +
> +enum {
> +	BD71837_BUCK1	=	0,
> +	BD71837_BUCK2,
> +	BD71837_BUCK3,
> +	BD71837_BUCK4,
> +	BD71837_BUCK5,
> +	BD71837_BUCK6,
> +	BD71837_BUCK7,
> +	BD71837_BUCK8,
> +	BD71837_LDO1,
> +	BD71837_LDO2,
> +	BD71837_LDO3,
> +	BD71837_LDO4,
> +	BD71837_LDO5,
> +	BD71837_LDO6,
> +	BD71837_LDO7,
> +	BD71837_REGULATOR_CNT,
> +};
> +
> +#define BD71837_BUCK1_VOLTAGE_NUM	0x40
> +#define BD71837_BUCK2_VOLTAGE_NUM	0x40
> +#define BD71837_BUCK3_VOLTAGE_NUM	0x40
> +#define BD71837_BUCK4_VOLTAGE_NUM	0x40
> +
> +#define BD71837_BUCK5_VOLTAGE_NUM	0x08
> +#define BD71837_BUCK6_VOLTAGE_NUM	0x04
> +#define BD71837_BUCK7_VOLTAGE_NUM	0x08
> +#define BD71837_BUCK8_VOLTAGE_NUM	0x40
> +
> +#define BD71837_LDO1_VOLTAGE_NUM	0x04
> +#define BD71837_LDO2_VOLTAGE_NUM	0x02
> +#define BD71837_LDO3_VOLTAGE_NUM	0x10
> +#define BD71837_LDO4_VOLTAGE_NUM	0x10
> +#define BD71837_LDO5_VOLTAGE_NUM	0x10
> +#define BD71837_LDO6_VOLTAGE_NUM	0x10
> +#define BD71837_LDO7_VOLTAGE_NUM	0x10
> +
> +enum {
> +	BD71837_REG_REV                = 0x00,
> +	BD71837_REG_SWRESET            = 0x01,
> +	BD71837_REG_I2C_DEV            = 0x02,
> +	BD71837_REG_PWRCTRL0           = 0x03,
> +	BD71837_REG_PWRCTRL1           = 0x04,
> +	BD71837_REG_BUCK1_CTRL         = 0x05,
> +	BD71837_REG_BUCK2_CTRL         = 0x06,
> +	BD71837_REG_BUCK3_CTRL         = 0x07,
> +	BD71837_REG_BUCK4_CTRL         = 0x08,
> +	BD71837_REG_BUCK5_CTRL         = 0x09,
> +	BD71837_REG_BUCK6_CTRL         = 0x0A,
> +	BD71837_REG_BUCK7_CTRL         = 0x0B,
> +	BD71837_REG_BUCK8_CTRL         = 0x0C,
> +	BD71837_REG_BUCK1_VOLT_RUN     = 0x0D,
> +	BD71837_REG_BUCK1_VOLT_IDLE    = 0x0E,
> +	BD71837_REG_BUCK1_VOLT_SUSP    = 0x0F,
> +	BD71837_REG_BUCK2_VOLT_RUN     = 0x10,
> +	BD71837_REG_BUCK2_VOLT_IDLE    = 0x11,
> +	BD71837_REG_BUCK3_VOLT_RUN     = 0x12,
> +	BD71837_REG_BUCK4_VOLT_RUN     = 0x13,
> +	BD71837_REG_BUCK5_VOLT         = 0x14,
> +	BD71837_REG_BUCK6_VOLT         = 0x15,
> +	BD71837_REG_BUCK7_VOLT         = 0x16,
> +	BD71837_REG_BUCK8_VOLT         = 0x17,
> +	BD71837_REG_LDO1_VOLT          = 0x18,
> +	BD71837_REG_LDO2_VOLT          = 0x19,
> +	BD71837_REG_LDO3_VOLT          = 0x1A,
> +	BD71837_REG_LDO4_VOLT          = 0x1B,
> +	BD71837_REG_LDO5_VOLT          = 0x1C,
> +	BD71837_REG_LDO6_VOLT          = 0x1D,
> +	BD71837_REG_LDO7_VOLT          = 0x1E,
> +	BD71837_REG_TRANS_COND0        = 0x1F,
> +	BD71837_REG_TRANS_COND1        = 0x20,
> +	BD71837_REG_VRFAULTEN          = 0x21,
> +	BD71837_REG_MVRFLTMASK0        = 0x22,
> +	BD71837_REG_MVRFLTMASK1        = 0x23,
> +	BD71837_REG_MVRFLTMASK2        = 0x24,
> +	BD71837_REG_RCVCFG             = 0x25,
> +	BD71837_REG_RCVNUM             = 0x26,
> +	BD71837_REG_PWRONCONFIG0       = 0x27,
> +	BD71837_REG_PWRONCONFIG1       = 0x28,
> +	BD71837_REG_RESETSRC           = 0x29,
> +	BD71837_REG_MIRQ               = 0x2A,
> +	BD71837_REG_IRQ                = 0x2B,
> +	BD71837_REG_IN_MON             = 0x2C,
> +	BD71837_REG_POW_STATE          = 0x2D,
> +	BD71837_REG_OUT32K             = 0x2E,
> +	BD71837_REG_REGLOCK            = 0x2F,
> +	BD71837_REG_OTPVER             = 0xFF,
> +	BD71837_MAX_REGISTER           = 0x100,
> +};
> +
> +#define REGLOCK_PWRSEQ	0x1
> +#define REGLOCK_VREG	0x10
> +
> +/* Generic BUCK control masks */
> +#define BD71837_BUCK_SEL	0x02
> +#define BD71837_BUCK_EN		0x01
> +#define BD71837_BUCK_RUN_ON	0x04
> +
> +/* Generic LDO masks */
> +#define BD71837_LDO_SEL		0x80
> +#define BD71837_LDO_EN		0x40
> +
> +/* BD71837 BUCK ramp rate CTRL reg bits */
> +#define BUCK_RAMPRATE_MASK	0xC0
> +#define BUCK_RAMPRATE_10P00MV	0x0
> +#define BUCK_RAMPRATE_5P00MV	0x1
> +#define BUCK_RAMPRATE_2P50MV	0x2
> +#define BUCK_RAMPRATE_1P25MV	0x3
> +
> +/* BD71837_REG_BUCK1_VOLT_RUN bits */
> +#define BUCK1_RUN_MASK		0x3F
> +#define BUCK1_RUN_DEFAULT	0x14
> +
> +/* BD71837_REG_BUCK1_VOLT_SUSP bits */
> +#define BUCK1_SUSP_MASK		0x3F
> +#define BUCK1_SUSP_DEFAULT	0x14
> +
> +/* BD71837_REG_BUCK1_VOLT_IDLE bits */
> +#define BUCK1_IDLE_MASK		0x3F
> +#define BUCK1_IDLE_DEFAULT	0x14
> +
> +/* BD71837_REG_BUCK2_VOLT_RUN bits */
> +#define BUCK2_RUN_MASK		0x3F
> +#define BUCK2_RUN_DEFAULT	0x1E
> +
> +/* BD71837_REG_BUCK2_VOLT_IDLE bits */
> +#define BUCK2_IDLE_MASK		0x3F
> +#define BUCK2_IDLE_DEFAULT	0x14
> +
> +/* BD71837_REG_BUCK3_VOLT_RUN bits */
> +#define BUCK3_RUN_MASK		0x3F
> +#define BUCK3_RUN_DEFAULT	0x1E
> +
> +/* BD71837_REG_BUCK4_VOLT_RUN bits */
> +#define BUCK4_RUN_MASK		0x3F
> +#define BUCK4_RUN_DEFAULT	0x1E
> +
> +/* BD71837_REG_BUCK5_VOLT bits */
> +#define BUCK5_MASK		0x07
> +#define BUCK5_DEFAULT		0x02
> +
> +/* BD71837_REG_BUCK6_VOLT bits */
> +#define BUCK6_MASK		0x03
> +#define BUCK6_DEFAULT		0x03
> +
> +/* BD71837_REG_BUCK7_VOLT bits */
> +#define BUCK7_MASK		0x07
> +#define BUCK7_DEFAULT		0x03
> +
> +/* BD71837_REG_BUCK8_VOLT bits */
> +#define BUCK8_MASK		0x3F
> +#define BUCK8_DEFAULT		0x1E
> +
> +/* BD71837_REG_IRQ bits */
> +#define IRQ_SWRST		0x40
> +#define IRQ_PWRON_S		0x20
> +#define IRQ_PWRON_L		0x10
> +#define IRQ_PWRON		0x08
> +#define IRQ_WDOG		0x04
> +#define IRQ_ON_REQ		0x02
> +#define IRQ_STBY_REQ		0x01
> +
> +/* BD71837_REG_OUT32K bits */
> +#define BD71837_OUT32K_EN	0x01
> +
> +/* BD71837 gated clock rate */
> +#define BD71837_CLK_RATE 32768
> +
> +/* BD71837 irqs */
> +enum {
> +	BD71837_INT_STBY_REQ,
> +	BD71837_INT_ON_REQ,
> +	BD71837_INT_WDOG,
> +	BD71837_INT_PWRBTN,
> +	BD71837_INT_PWRBTN_L,
> +	BD71837_INT_PWRBTN_S,
> +	BD71837_INT_SWRST
> +};
> +
> +/* BD71837 interrupt masks */
> +#define BD71837_INT_SWRST_MASK		0x40
> +#define BD71837_INT_PWRBTN_S_MASK	0x20
> +#define BD71837_INT_PWRBTN_L_MASK	0x10
> +#define BD71837_INT_PWRBTN_MASK		0x8
> +#define BD71837_INT_WDOG_MASK		0x4
> +#define BD71837_INT_ON_REQ_MASK		0x2
> +#define BD71837_INT_STBY_REQ_MASK	0x1
> +
> +/* BD71837_REG_LDO1_VOLT bits */
> +#define LDO1_MASK		0x03
> +
> +/* BD71837_REG_LDO1_VOLT bits */
> +#define LDO2_MASK		0x20
> +
> +/* BD71837_REG_LDO3_VOLT bits */
> +#define LDO3_MASK		0x0F
> +
> +/* BD71837_REG_LDO4_VOLT bits */
> +#define LDO4_MASK		0x0F
> +
> +/* BD71837_REG_LDO5_VOLT bits */
> +#define LDO5_MASK		0x0F
> +
> +/* BD71837_REG_LDO6_VOLT bits */
> +#define LDO6_MASK		0x0F
> +
> +/* BD71837_REG_LDO7_VOLT bits */
> +#define LDO7_MASK		0x0F
> +
> +/* Register write induced reset settings */
> +
> +/* Even though the bit zero is not SWRESET type we still want to write zero
> + * to it when changing type. Bit zero is 'SWRESET' trigger bit and if we
> + * write 1 to it we will trigger the action. So always write 0 to it when
> + * changning SWRESET action - no matter what we read from it.
> + */

Please use the correct format for multi-line comments.

Hint: The first line should be empty.

> +#define BD71837_SWRESET_TYPE_MASK 7
> +#define BD71837_SWRESET_TYPE_DISABLED 0
> +#define BD71837_SWRESET_TYPE_COLD 4
> +#define BD71837_SWRESET_TYPE_WARM 6
> +
> +#define BD71837_SWRESET_RESET_MASK 1
> +#define BD71837_SWRESET_RESET 1
> +
> +/* Poweroff state transition conditions */
> +
> +#define BD718XX_ON_REQ_POWEROFF_MASK 1
> +#define BD718XX_SWRESET_POWEROFF_MASK 2
> +#define BD718XX_WDOG_POWEROFF_MASK 4
> +#define BD718XX_KEY_L_POWEROFF_MASK 8
> +
> +#define BD718XX_POWOFF_TO_SNVS 0
> +#define BD718XX_POWOFF_TO_RDY 0xF

Please align each of the values with tabs.

> +#define BD718XX_POWOFF_TIME_MASK 0xF0
> +enum {
> +	BD718XX_POWOFF_TIME_5MS = 0,
> +	BD718XX_POWOFF_TIME_10MS,
> +	BD718XX_POWOFF_TIME_15MS,
> +	BD718XX_POWOFF_TIME_20MS,
> +	BD718XX_POWOFF_TIME_25MS,
> +	BD718XX_POWOFF_TIME_30MS,
> +	BD718XX_POWOFF_TIME_35MS,
> +	BD718XX_POWOFF_TIME_40MS,
> +	BD718XX_POWOFF_TIME_45MS,
> +	BD718XX_POWOFF_TIME_50MS,
> +	BD718XX_POWOFF_TIME_75MS,
> +	BD718XX_POWOFF_TIME_100MS,
> +	BD718XX_POWOFF_TIME_250MS,
> +	BD718XX_POWOFF_TIME_500MS,
> +	BD718XX_POWOFF_TIME_750MS,
> +	BD718XX_POWOFF_TIME_1500MS
> +};
> +
> +/* Poweron sequence state transition conditions */
> +
> +#define BD718XX_RDY_TO_SNVS_MASK 0xF
> +#define BD718XX_SNVS_TO_RUN_MASK 0xF0
> +
> +#define BD718XX_PWR_TRIG_KEY_L 1
> +#define BD718XX_PWR_TRIG_KEY_S 2
> +#define BD718XX_PWR_TRIG_PMIC_ON 4
> +#define BD718XX_PWR_TRIG_VSYS_UVLO 8
> +#define BD718XX_RDY_TO_SNVS_SIFT 0
> +#define BD718XX_SNVS_TO_RUN_SIFT 4

As above.

> +
> +

No need for 2 '\n'.

> +#define BD718XX_PWRBTN_PRESS_DURATION_MASK 0xF
> +
> +/* Timeout value for detecting short press */
> +
> +enum {
> +	BD718XX_PWRBTN_SHORT_PRESS_10MS = 0,
> +	BD718XX_PWRBTN_SHORT_PRESS_500MS,
> +	BD718XX_PWRBTN_SHORT_PRESS_1000MS,
> +	BD718XX_PWRBTN_SHORT_PRESS_1500MS,
> +	BD718XX_PWRBTN_SHORT_PRESS_2000MS,
> +	BD718XX_PWRBTN_SHORT_PRESS_2500MS,
> +	BD718XX_PWRBTN_SHORT_PRESS_3000MS,
> +	BD718XX_PWRBTN_SHORT_PRESS_3500MS,
> +	BD718XX_PWRBTN_SHORT_PRESS_4000MS,
> +	BD718XX_PWRBTN_SHORT_PRESS_4500MS,
> +	BD718XX_PWRBTN_SHORT_PRESS_5000MS,
> +	BD718XX_PWRBTN_SHORT_PRESS_5500MS,
> +	BD718XX_PWRBTN_SHORT_PRESS_6000MS,
> +	BD718XX_PWRBTN_SHORT_PRESS_6500MS,
> +	BD718XX_PWRBTN_SHORT_PRESS_7000MS,
> +	BD718XX_PWRBTN_SHORT_PRESS_7500MS
> +};
> +
> +/* Timeout value for detecting LONG press */
> +
> +enum {
> +	BD718XX_PWRBTN_LONG_PRESS_10MS = 0,
> +	BD718XX_PWRBTN_LONG_PRESS_1S,
> +	BD718XX_PWRBTN_LONG_PRESS_2S,
> +	BD718XX_PWRBTN_LONG_PRESS_3S,
> +	BD718XX_PWRBTN_LONG_PRESS_4S,
> +	BD718XX_PWRBTN_LONG_PRESS_5S,
> +	BD718XX_PWRBTN_LONG_PRESS_6S,
> +	BD718XX_PWRBTN_LONG_PRESS_7S,
> +	BD718XX_PWRBTN_LONG_PRESS_8S,
> +	BD718XX_PWRBTN_LONG_PRESS_9S,
> +	BD718XX_PWRBTN_LONG_PRESS_10S,
> +	BD718XX_PWRBTN_LONG_PRESS_11S,
> +	BD718XX_PWRBTN_LONG_PRESS_12S,
> +	BD718XX_PWRBTN_LONG_PRESS_13S,
> +	BD718XX_PWRBTN_LONG_PRESS_14S,
> +	BD718XX_PWRBTN_LONG_PRESS_15S
> +};
> +
> +
> +

Certainly no need for 3 '\n'.

> +struct bd71837;

Why is this necessary?

> +struct bd71837_pmic;
> +struct bd71837_clk;
> +struct bd718xx_pwrkey;

Where are these forward declared from?

> +/*
> + * Board platform data may be used to initialize regulators.
> + */
> +
> +struct bd71837_board {
> +	struct regulator_init_data *init_data[BD71837_REGULATOR_CNT];
> +	int	gpio_intr;
> +};

Where is this populated?

> +struct bd71837 {
> +	struct device *dev;
> +	struct i2c_client *i2c_client;
> +	struct regmap *regmap;
> +	unsigned long int id;
> +
> +	int chip_irq;
> +	struct regmap_irq_chip_data *irq_data;
> +
> +	struct bd71837_pmic *pmic;
> +	struct bd71837_clk *clk;
> +	struct bd718xx_pwrkey *pwrkey;
> +};
> +
> +/*
> + * bd71837 sub-driver chip access routines
> + */
> +
> +static inline int bd71837_reg_read(struct bd71837 *bd71837, u8 reg)
> +{
> +	int r, val;
> +
> +	r = regmap_read(bd71837->regmap, reg, &val);
> +	if (r < 0)
> +		return r;
> +	return val;
> +}
> +
> +static inline int bd71837_reg_write(struct bd71837 *bd71837, u8 reg,
> +				    unsigned int val)
> +{
> +	return regmap_write(bd71837->regmap, reg, val);
> +}
> +
> +static inline int bd71837_set_bits(struct bd71837 *bd71837, u8 reg, u8 mask)
> +{
> +	return regmap_update_bits(bd71837->regmap, reg, mask, mask);
> +}
> +
> +static inline int bd71837_clear_bits(struct bd71837 *bd71837, u8 reg,
> +				     u8 mask)
> +{
> +	return regmap_update_bits(bd71837->regmap, reg, mask, 0);
> +}
> +
> +static inline int bd71837_update_bits(struct bd71837 *bd71837, u8 reg,
> +				      u8 mask, u8 val)
> +{
> +	return regmap_update_bits(bd71837->regmap, reg, mask, val);
> +}

Please don't abstract APIs for no reason.

Use the regmap_* API directly instead.

> +#endif /* __LINUX_MFD_BD71837_H__ */

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v9 2/2] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC

[Lee Jones]

On Thu, 05 Jul 2018, Matti Vaittinen wrote:

> Document devicetree bindings for ROHM BD71837 PMIC MFD.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>  .../devicetree/bindings/mfd/rohm,bd71837-pmic.txt  | 67 ++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
> new file mode 100644
> index 000000000000..67f2616288d9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
> @@ -0,0 +1,67 @@
> +* ROHM BD71837 Power Management Integrated Circuit bindings
> +
> +BD71837MWV is a programmable Power Management IC for powering single-core,
> +dual-core, and quad-core SoC’s such as NXP-i.MX 8M. It is optimized for

Nit: s/SOC's/SOCs/

> +low BOM cost and compact solution footprint. It integrates 8 Buck
> +egulators and 7 LDO’s to provide all the power rails required by the SoC and

Nit: s/LDO's/LDOs/

> +the commonly used peripherals.
> +
> +Datasheet for PMIC is available at:
> +https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
> +
> +Required properties:
> + - compatible		: Should be "rohm,bd71837".
> + - reg			: I2C slave address.
> + - interrupt-parent	: Phandle to the parent interrupt controller.
> + - interrupts		: The interrupt line the device is connected to.
> + - clocks		: The parent clock connected to PMIC. If this is missng

s/missng/missing/

> +			  32768 KHz clock is assumed.
> + - #clock-cells		: Should be 0
> + - regulators:		: List of child nodes that specify the regulators

By your convention, the bottom two entries are missing '.'s.

> +			  Please see ../regulator/rohm,bd71837-regulator.txt
> +
> +Optional properties:
> +- clock-output-names	: Should contain name for output clock.
> +
> +Example:
> +
> +	/* external oscillator node */

Uppercase character to start a sentence.

> +	osc: oscillator {
> +		compatible = "fixed-clock";
> +		#clock-cells = <1>;
> +		clock-frequency  = <32768>;
> +		clock-output-names = "osc";
> +	};
> +
> +	/* PMIC node */

Is this comment really necessary?  The node is called "pmic".

> +	pmic: pmic@4b {
> +		compatible = "rohm,bd71837";
> +		reg = <0x4b>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <29 GPIO_ACTIVE_LOW>;
> +		interrupt-names = "irq";
> +		#clock-cells = <0>;
> +		clocks = <&osc 0>;
> +		clock-output-names = "bd71837-32k-out";
> +
> +		regulators {
> +			buck1: BUCK1 {
> +				regulator-name = "buck1";
> +				regulator-min-microvolt = <700000>;
> +				regulator-max-microvolt = <1300000>;
> +				regulator-boot-on;
> +				regulator-ramp-delay = <1250>;
> +			};
> +			/* ... */
> +		};
> +	};
> +
> +	/* Clock consumer node */
> +
> +	foo@0 {
> +		compatible = "bar,foo";
> +		/* ... */

No need for this line.  Additional properties are always expected.

> +		clock-names = "my-clock";
> +		clocks = <&pmic>;
> +	};

Do you have a real example to give?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v9 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC

[Matti Vaittinen]

Thanks for taking the time to check this =)

On Thu, Jul 05, 2018 at 10:18:13AM +0100, Lee Jones wrote:
> On Thu, 05 Jul 2018, Matti Vaittinen wrote:
> 
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1787,6 +1787,19 @@ config MFD_STW481X
> >  	  in various ST Microelectronics and ST-Ericsson embedded
> >  	  Nomadik series.
> >  
> > +config MFD_BD71837
> 
> It's be nice if you prefix this with the manufacturer.  Rohm?

Like MFD_ROHM_BD71837, right? How important this is? The already applied
regulator part has depends_on for MFD_BD71837 in Kconfig so changing
this will require change in regulator tree too. I guess it's doable if
this is important though =)
> 
> > +	tristate "BD71837 Power Management chip"
> 
> Again, missing manufacturer.
> 
> This should also be "Power Management IC"

I'll fix this

> > +	select MFD_CORE
> > +	help
> > +	  Select this option to get support for the ROHM BD71837
> > +	  Power Management chips. BD71837 is designed to power processors like
> 
> "chips" is slang.  Should be "ICs".

I'll fix this
 
> > +obj-$(CONFIG_MFD_BD71837)	+= bd71837.o
> >  
> > diff --git a/drivers/mfd/bd71837.c b/drivers/mfd/bd71837.c
> 
> rohm-bd71837.c would be nicer.

I'll fix this

> > new file mode 100644
> > index 000000000000..677097bcea17
> > --- /dev/null
> > +++ b/drivers/mfd/bd71837.c
> > @@ -0,0 +1,215 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> 
> '\n' here please.
> > +// Copyright (C) 2018 ROHM Semiconductors
> 
> '\n' here please.

As an empty line after licence line? Ok.

> > +// bd71837.c -- ROHM BD71837MWV mfd driver
> 
> Remove the filename -- they have a habit of becoming incorrect.
> 
> Also, and 'MFD driver' isn't a thing -- this is a PMIC driver. 

I'll fix this

> > +// Datasheet available from
> > +// https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
> > +
> > +#include <linux/i2c.h>
> > +#include <linux/input.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mfd/bd71837.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/gpio_keys.h>
> 
> Alphabetical please.
> 

There is a problem with gpio_key.h - it uses bool but does not include
the header defining this type. So I get comple error when compiling this
using my GCC arm cross-compiler. I've submitted a patch to fix this but
as it is not yet accepted.
https://lore.kernel.org/lkml/20180627073458.GA27800@localhost.localdomain/
 Hence the gpio_keys.h is last one.

> > +static struct gpio_keys_button btns[] = {
> > +	{
> > +		.code = KEY_POWER,
> > +		.gpio = -1,
> > +		.type = EV_KEY,
> > +	},
> > +};
> 
> Does this need to be an array?

https://elixir.bootlin.com/linux/v4.17-rc1/source/include/linux/gpio_keys.h#L39
gpio_keys_platform_data documentation states the buttons to be a pointer
to an array. Thus I did an array. Do you think I should change this to
single struct?

> > +static struct gpio_keys_platform_data bd718xx_powerkey_data = {
> > +	.buttons = &btns[0],
> 
> Why not just "btns"?
> 
> Also, I don't see any reason not to just call this 'buttons'.
> 
> Or even, 'button' if there will only ever be one of them.
> 

I can rename this - and if I make the array to not be an array - well,
then the &foo[0] gets cleared too =)

> > +static struct mfd_cell bd71837_mfd_cells[] = {
> > +	{
> > +		.name = "bd71837-clk",
> > +	}, {
> > +		.name = "gpio-keys",
> > +		.platform_data = &bd718xx_powerkey_data,
> > +		.pdata_size = sizeof(bd718xx_powerkey_data),
> > +	}, {
> > +		.name = "bd71837-pmic",
> > +	},
> > +};
> 
> Place the one line entries on one line, like this:
> 
> 	{ .name = "bd71837-pmic", },
> 
> ... and if ordering is not important, place them at the bottom.

I'll fix this

> > +static int bd71837_i2c_probe(struct i2c_client *i2c,
> > +			    const struct i2c_device_id *id)
> > +{
> > +	struct bd71837 *bd71837;
> > +	struct bd71837_board *board_info;
> > +	int ret = -EINVAL;
> 
> I'd prefer it if you set the error at the location it happened for
> readability.

I'll fix this

> > +	bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
> > +
> 
> Drop this line.

I'll fix this

> > +	ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
> > +	if (ret < 0) {
> 
> Is 0 return a successful read?  I suggest that it isn't.
> 
> > +	ret = devm_regmap_add_irq_chip(&i2c->dev, bd71837->regmap,
> > +				       bd71837->chip_irq, IRQF_ONESHOT, 0,
> > +				       &bd71837_irq_chip, &bd71837->irq_data);
> > +	if (ret < 0) {
> 
> Is a >0 return valid?  If not, perhaps "if (ret)" would be better.
> 
> > +	ret = regmap_update_bits(bd71837->regmap,
> > +				 BD71837_REG_PWRONCONFIG0,
> > +				 BD718XX_PWRBTN_PRESS_DURATION_MASK,
> > +				 BD718XX_PWRBTN_SHORT_PRESS_10MS);
> > +	if (ret < 0) {
> 
> As above.
> 

I will check these. Thanks.

> > +
> > +	/*
> > +	 * According to BD71847 datasheet the HW default for long press
> > +	 * detection is 10ms. So lets change it to 10 sec so we can actually
> > +	 * get the short push and allow gracefull shut down
> > +	 */
> 
> I don't think this comment is necessary.
> 
> If anything it only needs to say "Configure long press to 10 seconds".
> 
> And if you do that, please add one for short press too.
Difference between short and long push is that if PMIC detects log push
it will kill the power without any gracefull handling. And the chip I am
supporting right now is BD71837 - which has more sane default value (10
seconds). But I am going to add support for BD71847 - which was shipped
to me yesterday - and according the draft data-sheet the default on it
is 10ms - which makes short push unusable. So comment is intended to
point out _why_ I configure the long push here. Also someone might want
to use the HW default (I guess such default was selected for some
specific reason) so comment helps on removing this configuration for
specific use-cases. But I guess I can reduce the comment to "Configure
long press to 10 seconds" as you suggested - or do you think I should
make difference between ICs BD71837 and BD71847 more visible here?
> 
> > +	ret = regmap_update_bits(bd71837->regmap,
> > +				 BD71837_REG_PWRONCONFIG1,
> > +				 BD718XX_PWRBTN_PRESS_DURATION_MASK,
> > +				 BD718XX_PWRBTN_LONG_PRESS_10S);
> > +
> > +	if (ret < 0) {
> > +		dev_err(&i2c->dev, "Failed to configure button long press timeout\n");
> > +		goto err_out;
> > +	}
> > +
> > +	btns[0].irq = regmap_irq_get_virq(bd71837->irq_data,
> > +					  BD71837_INT_PWRBTN_S);
> > +
> > +	if (btns[0].irq < 0) {
> > +		ret = btns[0].irq;
> > +		dev_err(&i2c->dev, "Failed to get the IRQ\n");
> > +		goto err_out;
> 
> No unwinding to be done, just return directly and save yourself an
> superflouous goto.

So should I replace _all_ the gotos by in-place returns as Enric suggested
yesterday? As I explained yesterday, my preference is to have only one
point of exit but I can change gotos to returns if needed.

> > +static const struct i2c_device_id bd71837_i2c_id[] = {
> > +	{ .name = "bd71837", },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id);
> 
> You shouldn't need this table anymore.
> 
> See da10c06a044b ("i2c: Make I2C ID tables non-mandatory for DT'ed devices")

So if I read this correctly the 
> > +		.of_match_table = bd71837_of_match,
is now sufficient. Thanks.

> > +static struct i2c_driver bd71837_i2c_driver = {
> > +	.driver = {
> > +		.name = "bd71837-mfd",
> 
> And "MFD" isn't really a thing.  It's more common to just mention the
> board name, or something like 'rohm-bd7183'.

I'll fix this

> > +/*
> > + * init early so consumer devices can complete system boot
> > + */
> 
> This is a single line commit.  Please use the correct format.
> 
>   /* Single line comment */

I'll fix this. I misinterpreted your comment yesterday.

> > +MODULE_DESCRIPTION("BD71837 chip multi-function driver");
> 
> 'PMIC' or 'Power Management IC' rather than MFD.

I'll fix this

> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/bd71837.h b/include/linux/mfd/bd71837.h
> > new file mode 100644
> > index 000000000000..10b0dfe90f27
> > --- /dev/null
> > +++ b/include/linux/mfd/bd71837.h
> > @@ -0,0 +1,391 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> 
> I thought these were meant to use C++ (//) comments?

The checkpatch.pl did complain if I used // comment on SPDX line for
header file. OTOH for c-file it required // comments and complained
about /* */ - version. So I did as it suggested and used 

> > +/* Copyright (C) 2018 ROHM Semiconductors */
> > +
> > +/*
> > + * ROHM BD71837MWV header file
> > + */
> 
> If you prefix the mentions of (BD,bd)71837 with (ROHM,rohm) then this
> comment becomes redundant and you can remove it.

I am sorry but I am not sure what you suggest here. Do you mean I should
add ROHM or rohm to all definitions here? I think that would make
definitions pretty long so I would certinly need to split more lines.
Such cange would also impact already applied patches. So maybe I
misinterpreted your comment? And in case I did not - can this prefixing of
types - be done as a separate patch set as it impacts to regulators and
clk too? (clk is not yet applied so that is easy to change though).

> > +/* Even though the bit zero is not SWRESET type we still want to write zero
> > + * to it when changing type. Bit zero is 'SWRESET' trigger bit and if we
> > + * write 1 to it we will trigger the action. So always write 0 to it when
> > + * changning SWRESET action - no matter what we read from it.
> > + */
> 
> Please use the correct format for multi-line comments.
> 
> Hint: The first line should be empty.

I'll fix this

> > +#define BD71837_SWRESET_TYPE_MASK 7
> > +#define BD71837_SWRESET_TYPE_DISABLED 0
> > +#define BD71837_SWRESET_TYPE_COLD 4
> > +#define BD71837_SWRESET_TYPE_WARM 6
> > +
> > +#define BD71837_SWRESET_RESET_MASK 1
> > +#define BD71837_SWRESET_RESET 1
> > +
> > +/* Poweroff state transition conditions */
> > +
> > +#define BD718XX_ON_REQ_POWEROFF_MASK 1
> > +#define BD718XX_SWRESET_POWEROFF_MASK 2
> > +#define BD718XX_WDOG_POWEROFF_MASK 4
> > +#define BD718XX_KEY_L_POWEROFF_MASK 8
> > +
> > +#define BD718XX_POWOFF_TO_SNVS 0
> > +#define BD718XX_POWOFF_TO_RDY 0xF
> 
> Please align each of the values with tabs.

I'll fix this

> > +#define BD718XX_PWR_TRIG_KEY_L 1
> > +#define BD718XX_PWR_TRIG_KEY_S 2
> > +#define BD718XX_PWR_TRIG_PMIC_ON 4
> > +#define BD718XX_PWR_TRIG_VSYS_UVLO 8
> > +#define BD718XX_RDY_TO_SNVS_SIFT 0
> > +#define BD718XX_SNVS_TO_RUN_SIFT 4
> 
> As above.

I'll fix this

> > +
> > +
> 
> No need for 2 '\n'.

I'll fix this

> > +	BD718XX_PWRBTN_LONG_PRESS_15S
> > +};
> > +
> > +
> > +
> 
> Certainly no need for 3 '\n'.

I'll fix this

> > +struct bd71837;
> 
> Why is this necessary?
> 

I think it's not. I'll remove it.

> > +struct bd71837_pmic;
> > +struct bd71837_clk;
> > +struct bd718xx_pwrkey;
> 
> Where are these forward declared from?

bd71837_pmic is in drivers/regulator/bd71837-regulator.c
clk will be in clk subdevice patch that is not yet applied.
It is pending some fixes. Pwrkey must be dropped as the power-key driver
I originally wrote was replaced using gpio_keys instead. Should there be
some comment explaining this? Suggestions on how to do this without
referring to file names which are subject to change... Should I just say
they are defined in subdevice drivers? 

> > +/*
> > + * Board platform data may be used to initialize regulators.
> > + */
> > +
> > +struct bd71837_board {
> > +	struct regulator_init_data *init_data[BD71837_REGULATOR_CNT];
> > +	int	gpio_intr;
> > +};
> 
> Where is this populated?
> 
Currently nowhere as I use device-tree for getting the regulator/irq
config. This is for architectures which do not use DT - but as I don't
have one for testing I did leave the depends_on OF. If it was populated
I would expect it to be done in some setup code.

> > +/*
> > + * bd71837 sub-driver chip access routines
> > + */
> > +
> 
> Please don't abstract APIs for no reason.
> 
> Use the regmap_* API directly instead.
> 

Yes. This was already pointed out by Stephen Boyd. But again, as part of
the patches (reguators) were already applied using the wrappers - I asked
if I can remove these in separate patch set after getting this initial
version out.

> > +#endif /* __LINUX_MFD_BD71837_H__ */
> 
> -- 
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v9 2/2] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC

[Matti Vaittinen]

And thanks again Lee!

On Thu, Jul 05, 2018 at 10:24:44AM +0100, Lee Jones wrote:
> On Thu, 05 Jul 2018, Matti Vaittinen wrote:
> 
> > Document devicetree bindings for ROHM BD71837 PMIC MFD.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > ---
> >  .../devicetree/bindings/mfd/rohm,bd71837-pmic.txt  | 67 ++++++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
> > new file mode 100644
> > index 000000000000..67f2616288d9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
> > @@ -0,0 +1,67 @@
> > +* ROHM BD71837 Power Management Integrated Circuit bindings
> > +
> > +BD71837MWV is a programmable Power Management IC for powering single-core,
> > +dual-core, and quad-core SoC’s such as NXP-i.MX 8M. It is optimized for
> 
> Nit: s/SOC's/SOCs/

I'll change this.

> > +low BOM cost and compact solution footprint. It integrates 8 Buck
> > +egulators and 7 LDO’s to provide all the power rails required by the SoC and
> 
> Nit: s/LDO's/LDOs/

I'll change this.

> > +the commonly used peripherals.
> > +
> > +Datasheet for PMIC is available at:
> > +https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
> > +
> > +Required properties:
> > + - compatible		: Should be "rohm,bd71837".
> > + - reg			: I2C slave address.
> > + - interrupt-parent	: Phandle to the parent interrupt controller.
> > + - interrupts		: The interrupt line the device is connected to.
> > + - clocks		: The parent clock connected to PMIC. If this is missng
> 
> s/missng/missing/

I'll change this.

> > +			  32768 KHz clock is assumed.
> > + - #clock-cells		: Should be 0
> > + - regulators:		: List of child nodes that specify the regulators
> 
> By your convention, the bottom two entries are missing '.'s.

I'll change this.

> > +			  Please see ../regulator/rohm,bd71837-regulator.txt
> > +
> > +Optional properties:
> > +- clock-output-names	: Should contain name for output clock.
> > +
> > +Example:
> > +
> > +	/* external oscillator node */
> 
> Uppercase character to start a sentence.

I'll change this.

> > +	osc: oscillator {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <1>;
> > +		clock-frequency  = <32768>;
> > +		clock-output-names = "osc";
> > +	};
> > +
> > +	/* PMIC node */
> 
> Is this comment really necessary?  The node is called "pmic".

I'll remove this.

> > +	pmic: pmic@4b {
> > +		compatible = "rohm,bd71837";
> > +		reg = <0x4b>;
> > +		interrupt-parent = <&gpio1>;
> > +		interrupts = <29 GPIO_ACTIVE_LOW>;
> > +		interrupt-names = "irq";
> > +		#clock-cells = <0>;
> > +		clocks = <&osc 0>;
> > +		clock-output-names = "bd71837-32k-out";
> > +
> > +		regulators {
> > +			buck1: BUCK1 {
> > +				regulator-name = "buck1";
> > +				regulator-min-microvolt = <700000>;
> > +				regulator-max-microvolt = <1300000>;
> > +				regulator-boot-on;
> > +				regulator-ramp-delay = <1250>;
> > +			};
> > +			/* ... */
> > +		};
> > +	};
> > +
> > +	/* Clock consumer node */
> > +
> > +	foo@0 {
> > +		compatible = "bar,foo";
> > +		/* ... */
> 
> No need for this line.  Additional properties are always expected.

I'll remove this.

> > +		clock-names = "my-clock";
> > +		clocks = <&pmic>;
> > +	};
> 
> Do you have a real example to give?

For clock consumer? Sorry, no I don't.

Br,
	Matti Vaittinen

> -- 
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v9 2/2] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC

[Lee Jones]

On Thu, 05 Jul 2018, Matti Vaittinen wrote:
> On Thu, Jul 05, 2018 at 10:24:44AM +0100, Lee Jones wrote:
> > On Thu, 05 Jul 2018, Matti Vaittinen wrote:
> > 
> > > Document devicetree bindings for ROHM BD71837 PMIC MFD.
> > > 
> > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > ---
> > >  .../devicetree/bindings/mfd/rohm,bd71837-pmic.txt  | 67 ++++++++++++++++++++++
> > >  1 file changed, 67 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
> > > +		clock-names = "my-clock";
> > > +		clocks = <&pmic>;
> > > +	};
> > 
> > Do you have a real example to give?
> 
> For clock consumer? Sorry, no I don't.

Might be better to drop it for the time being then.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v9 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC

[Lee Jones]

On Thu, 05 Jul 2018, Matti Vaittinen wrote:

> Thanks for taking the time to check this =)
> 
> On Thu, Jul 05, 2018 at 10:18:13AM +0100, Lee Jones wrote:
> > On Thu, 05 Jul 2018, Matti Vaittinen wrote:
> > 
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -1787,6 +1787,19 @@ config MFD_STW481X
> > >  	  in various ST Microelectronics and ST-Ericsson embedded
> > >  	  Nomadik series.
> > >  
> > > +config MFD_BD71837
> > 
> > It's be nice if you prefix this with the manufacturer.  Rohm?
> 
> Like MFD_ROHM_BD71837, right? How important this is? The already applied
> regulator part has depends_on for MFD_BD71837 in Kconfig so changing
> this will require change in regulator tree too. I guess it's doable if
> this is important though =)

How can it depend on something that doesn't exist. ;)

It's better, then similar devices can be grouped.

[...]

> > > new file mode 100644
> > > index 000000000000..677097bcea17
> > > --- /dev/null
> > > +++ b/drivers/mfd/bd71837.c
> > > @@ -0,0 +1,215 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > 
> > '\n' here please.
> > > +// Copyright (C) 2018 ROHM Semiconductors
> > 
> > '\n' here please.
> 
> As an empty line after licence line? Ok.

Just '//', yes.

> > > +#include <linux/i2c.h>
> > > +#include <linux/input.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/mfd/bd71837.h>
> > > +#include <linux/mfd/core.h>
> > > +#include <linux/module.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/gpio_keys.h>
> > 
> > Alphabetical please.
> > 
> 
> There is a problem with gpio_key.h - it uses bool but does not include
> the header defining this type. So I get comple error when compiling this
> using my GCC arm cross-compiler. I've submitted a patch to fix this but
> as it is not yet accepted.
> https://lore.kernel.org/lkml/20180627073458.GA27800@localhost.localdomain/
>  Hence the gpio_keys.h is last one.

If the ordering is important, separate it from the pack and place a
little comment in to say why you've done so.  That way no well meaning
contributor will attempt to tidy it up in the future.  If this will be
fixed anyway, perhaps the point is moot?

> > > +static struct gpio_keys_button btns[] = {
> > > +	{
> > > +		.code = KEY_POWER,
> > > +		.gpio = -1,
> > > +		.type = EV_KEY,
> > > +	},
> > > +};
> > 
> > Does this need to be an array?
> 
> https://elixir.bootlin.com/linux/v4.17-rc1/source/include/linux/gpio_keys.h#L39
> gpio_keys_platform_data documentation states the buttons to be a pointer
> to an array. Thus I did an array. Do you think I should change this to
> single struct?

I can't think of any reason why not.

> > > +static struct gpio_keys_platform_data bd718xx_powerkey_data = {
> > > +	.buttons = &btns[0],
> > 
> > Why not just "btns"?
> > 
> > Also, I don't see any reason not to just call this 'buttons'.
> > 
> > Or even, 'button' if there will only ever be one of them.
> 
> I can rename this - and if I make the array to not be an array - well,
> then the &foo[0] gets cleared too =)

Exactly.

> > > +static struct mfd_cell bd71837_mfd_cells[] = {
> > > +	{
> > > +		.name = "bd71837-clk",
> > > +	}, {
> > > +		.name = "gpio-keys",
> > > +		.platform_data = &bd718xx_powerkey_data,
> > > +		.pdata_size = sizeof(bd718xx_powerkey_data),
> > > +	}, {
> > > +		.name = "bd71837-pmic",
> > > +	},
> > > +};
> > 
> > Place the one line entries on one line, like this:
> > 
> > 	{ .name = "bd71837-pmic", },
> > 
> > ... and if ordering is not important, place them at the bottom.
> 
> I'll fix this

It's best to simply snip all the parts you agree with.

[...]

> > > +	/*
> > > +	 * According to BD71847 datasheet the HW default for long press
> > > +	 * detection is 10ms. So lets change it to 10 sec so we can actually
> > > +	 * get the short push and allow gracefull shut down
> > > +	 */
> > 
> > I don't think this comment is necessary.
> > 
> > If anything it only needs to say "Configure long press to 10 seconds".
> > 
> > And if you do that, please add one for short press too.

> Difference between short and long push is that if PMIC detects log push
> it will kill the power without any gracefull handling. And the chip I am
> supporting right now is BD71837 - which has more sane default value (10
> seconds). But I am going to add support for BD71847 - which was shipped
> to me yesterday - and according the draft data-sheet the default on it
> is 10ms - which makes short push unusable. So comment is intended to
> point out _why_ I configure the long push here. Also someone might want
> to use the HW default (I guess such default was selected for some
> specific reason) so comment helps on removing this configuration for
> specific use-cases. But I guess I can reduce the comment to "Configure
> long press to 10 seconds" as you suggested - or do you think I should
> make difference between ICs BD71837 and BD71847 more visible here?

Honestly, I don't think it's required.  If anyone were to try to
change it, I would hope that they too would have read the datasheet
and understand what they are doing.

> > > +	ret = regmap_update_bits(bd71837->regmap,
> > > +				 BD71837_REG_PWRONCONFIG1,
> > > +				 BD718XX_PWRBTN_PRESS_DURATION_MASK,
> > > +				 BD718XX_PWRBTN_LONG_PRESS_10S);
> > > +
> > > +	if (ret < 0) {
> > > +		dev_err(&i2c->dev, "Failed to configure button long press timeout\n");
> > > +		goto err_out;
> > > +	}
> > > +
> > > +	btns[0].irq = regmap_irq_get_virq(bd71837->irq_data,
> > > +					  BD71837_INT_PWRBTN_S);
> > > +
> > > +	if (btns[0].irq < 0) {
> > > +		ret = btns[0].irq;
> > > +		dev_err(&i2c->dev, "Failed to get the IRQ\n");
> > > +		goto err_out;
> > 
> > No unwinding to be done, just return directly and save yourself an
> > superflouous goto.
> 
> So should I replace _all_ the gotos by in-place returns as Enric suggested
> yesterday? As I explained yesterday, my preference is to have only one
> point of exit but I can change gotos to returns if needed.

I'm not sure where the 'one point of exit' thing stems from and I
can't think of any good reason for it.  The main reason for using a
goto in the exit path is to save lines of code and code duplication.
Your method *adds* lines of code by firstly setting 'ret', then
calling the goto.  If nothing needs to be done before returning, just
return the error value with one line.

> > > +static const struct i2c_device_id bd71837_i2c_id[] = {
> > > +	{ .name = "bd71837", },
> > > +	{ }
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id);
> > 
> > You shouldn't need this table anymore.
> > 
> > See da10c06a044b ("i2c: Make I2C ID tables non-mandatory for DT'ed devices")
> 
> So if I read this correctly the 
> > > +		.of_match_table = bd71837_of_match,
> is now sufficient. Thanks.

Right.

> > > +++ b/include/linux/mfd/bd71837.h
> > > @@ -0,0 +1,391 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > 
> > I thought these were meant to use C++ (//) comments?
> 
> The checkpatch.pl did complain if I used // comment on SPDX line for
> header file. OTOH for c-file it required // comments and complained
> about /* */ - version. So I did as it suggested and used 

Well that's just dandy -- who comes up with this stuff?

> > > +/* Copyright (C) 2018 ROHM Semiconductors */
> > > +
> > > +/*
> > > + * ROHM BD71837MWV header file
> > > + */
> > 
> > If you prefix the mentions of (BD,bd)71837 with (ROHM,rohm) then this
> > comment becomes redundant and you can remove it.
> 
> I am sorry but I am not sure what you suggest here. Do you mean I should
> add ROHM or rohm to all definitions here? I think that would make
> definitions pretty long so I would certinly need to split more lines.
> Such cange would also impact already applied patches. So maybe I
> misinterpreted your comment? And in case I did not - can this prefixing of
> types - be done as a separate patch set as it impacts to regulators and
> clk too? (clk is not yet applied so that is easy to change though).

Any lines which are already long, you can justify as not having to do
this, however I think for the filenames and function names it would
be nice if they were prefixed.

Filenames are particularly important.  That way all of the Rohm
drivers will be grouped.  Unless you can be assured that all Rohm
devices will be prefixed by 'bd', then the point is slightly mooted,
however since 'bd' doesn't really correlate with 'rohm' it's still
difficult to assume that bd* drivers are associated with Rohm -- if
you catch my drift.

For instance
 - 'wm' for Wolfson Microelectonics 
 - 'max' for Maxim
 - 'intel' for, well, you know
Etc.
 
[...]

> > > +struct bd71837_pmic;
> > > +struct bd71837_clk;
> > > +struct bd718xx_pwrkey;
> > 
> > Where are these forward declared from?
> 
> bd71837_pmic is in drivers/regulator/bd71837-regulator.c
> clk will be in clk subdevice patch that is not yet applied.
> It is pending some fixes. Pwrkey must be dropped as the power-key driver
> I originally wrote was replaced using gpio_keys instead. Should there be
> some comment explaining this? Suggestions on how to do this without
> referring to file names which are subject to change... Should I just say
> they are defined in subdevice drivers? 

I just wanted you to think about if they were really necessary.

> > > +/*
> > > + * Board platform data may be used to initialize regulators.
> > > + */
> > > +
> > > +struct bd71837_board {
> > > +	struct regulator_init_data *init_data[BD71837_REGULATOR_CNT];
> > > +	int	gpio_intr;
> > > +};
> > 
> > Where is this populated?
> > 
> Currently nowhere as I use device-tree for getting the regulator/irq
> config. This is for architectures which do not use DT - but as I don't
> have one for testing I did leave the depends_on OF. If it was populated
> I would expect it to be done in some setup code.

Please don't add code for 'what ifs'.

Please remove it and add it back when you need it.

> > > +/*
> > > + * bd71837 sub-driver chip access routines
> > > + */
> > > +
> > 
> > Please don't abstract APIs for no reason.
> > 
> > Use the regmap_* API directly instead.
> > 
> 
> Yes. This was already pointed out by Stephen Boyd. But again, as part of
> the patches (reguators) were already applied using the wrappers - I asked
> if I can remove these in separate patch set after getting this initial
> version out.

That is one of the issues with applying related patches without each
of them being reviewed first.  Applying unsuitable code is not the
correct thing to do, sorry.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v9 2/2] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC

[Matti Vaittinen]

On Thu, Jul 05, 2018 at 11:49:19AM +0100, Lee Jones wrote:
> On Thu, 05 Jul 2018, Matti Vaittinen wrote:
> > On Thu, Jul 05, 2018 at 10:24:44AM +0100, Lee Jones wrote:
> > > On Thu, 05 Jul 2018, Matti Vaittinen wrote:
> > > 
> > > > Document devicetree bindings for ROHM BD71837 PMIC MFD.
> > > > 
> > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > > ---
> > > >  .../devicetree/bindings/mfd/rohm,bd71837-pmic.txt  | 67 ++++++++++++++++++++++
> > > >  1 file changed, 67 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
> > > > +		clock-names = "my-clock";
> > > > +		clocks = <&pmic>;
> > > > +	};
> > > 
> > > Do you have a real example to give?
> > 
> > For clock consumer? Sorry, no I don't.
> 
> Might be better to drop it for the time being then.

I have tested the clk driver using this dummy consumer. So in a sense it
"works" and can be used as an example on how to write a real clock
consumer node. Thus I see some value in this example node - even if it
does not match to any real world HW. If I had to use the clk from this
PMIC and write HW description I would appreciate this dummy exaple. I
can drop it if you insist - but I would at least like to hear what is
the downside on having it here?

Best regards,
    Matti Vaittinen

Re: [PATCH v9 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC

[Matti Vaittinen]

On Thu, Jul 05, 2018 at 12:17:05PM +0100, Lee Jones wrote:
> On Thu, 05 Jul 2018, Matti Vaittinen wrote:
> 
> > Thanks for taking the time to check this =)
> > 
> > On Thu, Jul 05, 2018 at 10:18:13AM +0100, Lee Jones wrote:
> > > On Thu, 05 Jul 2018, Matti Vaittinen wrote:
> > > 
> > > > --- a/drivers/mfd/Kconfig
> > > > +++ b/drivers/mfd/Kconfig
> > > > @@ -1787,6 +1787,19 @@ config MFD_STW481X
> > > >  	  in various ST Microelectronics and ST-Ericsson embedded
> > > >  	  Nomadik series.
> > > >  
> > > > +config MFD_BD71837
> > > 
> > > It's be nice if you prefix this with the manufacturer.  Rohm?
> > 
> > Like MFD_ROHM_BD71837, right? How important this is? The already applied
> > regulator part has depends_on for MFD_BD71837 in Kconfig so changing
> > this will require change in regulator tree too. I guess it's doable if
> > this is important though =)
> 
> How can it depend on something that doesn't exist. ;)
> 
> It's better, then similar devices can be grouped.
> 
> > > > +#include <linux/i2c.h>
> > > > +#include <linux/input.h>
> > > > +#include <linux/interrupt.h>
> > > > +#include <linux/mfd/bd71837.h>
> > > > +#include <linux/mfd/core.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/regmap.h>
> > > > +#include <linux/gpio_keys.h>
> > > 
> > > Alphabetical please.
> > > 
> > 
> > There is a problem with gpio_key.h - it uses bool but does not include
> > the header defining this type. So I get comple error when compiling this
> > using my GCC arm cross-compiler. I've submitted a patch to fix this but
> > as it is not yet accepted.
> > https://lore.kernel.org/lkml/20180627073458.GA27800@localhost.localdomain/
> >  Hence the gpio_keys.h is last one.
> 
> If the ordering is important, separate it from the pack and place a
> little comment in to say why you've done so.  That way no well meaning
> contributor will attempt to tidy it up in the future.  If this will be
> fixed anyway, perhaps the point is moot?

I have no idea if my patch to add missing include is going to be
applied. I've not heard anything from it in a week or so. So I'll just
do as you suggested and drop a comment on why it is left last.

> > > > +	/*
> > > > +	 * According to BD71847 datasheet the HW default for long press
> > > > +	 * detection is 10ms. So lets change it to 10 sec so we can actually
> > > > +	 * get the short push and allow gracefull shut down
> > > > +	 */
> > > 
> > > I don't think this comment is necessary.
> > > 
> > > If anything it only needs to say "Configure long press to 10 seconds".
> > > 
> > > And if you do that, please add one for short press too.
> 
> > Difference between short and long push is that if PMIC detects log push
> > it will kill the power without any gracefull handling. And the chip I am
> > supporting right now is BD71837 - which has more sane default value (10
> > seconds). But I am going to add support for BD71847 - which was shipped
> > to me yesterday - and according the draft data-sheet the default on it
> > is 10ms - which makes short push unusable. So comment is intended to
> > point out _why_ I configure the long push here. Also someone might want
> > to use the HW default (I guess such default was selected for some
> > specific reason) so comment helps on removing this configuration for
> > specific use-cases. But I guess I can reduce the comment to "Configure
> > long press to 10 seconds" as you suggested - or do you think I should
> > make difference between ICs BD71837 and BD71847 more visible here?
> 
> Honestly, I don't think it's required.  If anyone were to try to
> change it, I would hope that they too would have read the datasheet
> and understand what they are doing.

You still have the hope and believe in good developers? ;) Allright,
I'll drop the comment.

> > > > +	ret = regmap_update_bits(bd71837->regmap,
> > > > +				 BD71837_REG_PWRONCONFIG1,
> > > > +				 BD718XX_PWRBTN_PRESS_DURATION_MASK,
> > > > +				 BD718XX_PWRBTN_LONG_PRESS_10S);
> > > > +
> > > > +	if (ret < 0) {
> > > > +		dev_err(&i2c->dev, "Failed to configure button long press timeout\n");
> > > > +		goto err_out;
> > > > +	}
> > > > +
> > > > +	btns[0].irq = regmap_irq_get_virq(bd71837->irq_data,
> > > > +					  BD71837_INT_PWRBTN_S);
> > > > +
> > > > +	if (btns[0].irq < 0) {
> > > > +		ret = btns[0].irq;
> > > > +		dev_err(&i2c->dev, "Failed to get the IRQ\n");
> > > > +		goto err_out;
> > > 
> > > No unwinding to be done, just return directly and save yourself an
> > > superflouous goto.
> > 
> > So should I replace _all_ the gotos by in-place returns as Enric suggested
> > yesterday? As I explained yesterday, my preference is to have only one
> > point of exit but I can change gotos to returns if needed.
> 
> I'm not sure where the 'one point of exit' thing stems from and I
> can't think of any good reason for it.  The main reason for using a
> goto in the exit path is to save lines of code and code duplication.
> Your method *adds* lines of code by firstly setting 'ret', then
> calling the goto.  If nothing needs to be done before returning, just
> return the error value with one line.

The point stems from my personal experiences =) I've once spent almost
two years cleaning/fixing up a code which left resources/locks leaking
from various function exits at error branches. Back then I sweared I
would always build just one clean-up sequence at the end of function and
never return from the middle. So this is my personal fixation. But as I
said, I will replace gotos with in place returns - and cross my fingers
no one adds a (non devm) resource allocation or locking in the probe =)

> > > > +++ b/include/linux/mfd/bd71837.h
> > > > @@ -0,0 +1,391 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > 
> > > I thought these were meant to use C++ (//) comments?
> > 
> > The checkpatch.pl did complain if I used // comment on SPDX line for
> > header file. OTOH for c-file it required // comments and complained
> > about /* */ - version. So I did as it suggested and used 
> 
> Well that's just dandy -- who comes up with this stuff?

Engineers I bet =)

> > > > +/* Copyright (C) 2018 ROHM Semiconductors */
> > > > +
> > > > +/*
> > > > + * ROHM BD71837MWV header file
> > > > + */
> > > 
> > > If you prefix the mentions of (BD,bd)71837 with (ROHM,rohm) then this
> > > comment becomes redundant and you can remove it.
> > 
> > I am sorry but I am not sure what you suggest here. Do you mean I should
> > add ROHM or rohm to all definitions here? I think that would make
> > definitions pretty long so I would certinly need to split more lines.
> > Such cange would also impact already applied patches. So maybe I
> > misinterpreted your comment? And in case I did not - can this prefixing of
> > types - be done as a separate patch set as it impacts to regulators and
> > clk too? (clk is not yet applied so that is easy to change though).
> 
> Any lines which are already long, you can justify as not having to do
> this, however I think for the filenames and function names it would
> be nice if they were prefixed.

Right. For file names this should be easily doable. And when the regmap
wrappers are removed there are no public functions left. And I think the
name of file containing the functions is sufficient for grouping
functions under "Rohm", right? 

> Filenames are particularly important.  That way all of the Rohm
> drivers will be grouped.  Unless you can be assured that all Rohm
> devices will be prefixed by 'bd', then the point is slightly mooted,
> however since 'bd' doesn't really correlate with 'rohm' it's still
> difficult to assume that bd* drivers are associated with Rohm -- if
> you catch my drift.

I guess I do catch it. And no, all ROHM stuff will definitely not be
prefixed with bd - which is the name of the power chip - I mean power IC
series.

> > > > +struct bd71837_pmic;
> > > > +struct bd71837_clk;
> > > > +struct bd718xx_pwrkey;
> > > 
> > > Where are these forward declared from?
> > 
> > bd71837_pmic is in drivers/regulator/bd71837-regulator.c
> > clk will be in clk subdevice patch that is not yet applied.
> > It is pending some fixes. Pwrkey must be dropped as the power-key driver
> > I originally wrote was replaced using gpio_keys instead. Should there be
> > some comment explaining this? Suggestions on how to do this without
> > referring to file names which are subject to change... Should I just say
> > they are defined in subdevice drivers? 
> 
> I just wanted you to think about if they were really necessary.

Right =) Well done on that=)

> > > > +/*
> > > > + * Board platform data may be used to initialize regulators.
> > > > + */
> > > > +
> > > > +struct bd71837_board {
> > > > +	struct regulator_init_data *init_data[BD71837_REGULATOR_CNT];
> > > > +	int	gpio_intr;
> > > > +};
> > > 
> > > Where is this populated?
> > > 
> > Currently nowhere as I use device-tree for getting the regulator/irq
> > config. This is for architectures which do not use DT - but as I don't
> > have one for testing I did leave the depends_on OF. If it was populated
> > I would expect it to be done in some setup code.
> 
> Please don't add code for 'what ifs'.
> 
> Please remove it and add it back when you need it.

Allright. Although this will also break the regulator part then...

> > > > +/*
> > > > + * bd71837 sub-driver chip access routines
> > > > + */
> > > > +
> > > 
> > > Please don't abstract APIs for no reason.
> > > 
> > > Use the regmap_* API directly instead.
> > > 
> > 
> > Yes. This was already pointed out by Stephen Boyd. But again, as part of
> > the patches (reguators) were already applied using the wrappers - I asked
> > if I can remove these in separate patch set after getting this initial
> > version out.
> 
> That is one of the issues with applying related patches without each
> of them being reviewed first.  Applying unsuitable code is not the
> correct thing to do, sorry.

So I assume you are not Ok with adding the wrappers and removing them
with later set of patches? I'll do following workaround then:

1. Change MFD Kconfig option name => current regulator code will not be
compiled even if the MFD would be applied.
2. Change MFD according to this discussion (and break the compatibility
with applied regulator code)
3. Fix the regulator code with further patches to Mark
4. Fix the depends_on Kconfig option in regulator tree to match the new
one suggested here.

Does this sound reasonable?

Best Regards
    Matti Vaittinen

> -- 
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v9 2/2] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC

[Lee Jones]

On Thu, 05 Jul 2018, Matti Vaittinen wrote:

> On Thu, Jul 05, 2018 at 11:49:19AM +0100, Lee Jones wrote:
> > On Thu, 05 Jul 2018, Matti Vaittinen wrote:
> > > On Thu, Jul 05, 2018 at 10:24:44AM +0100, Lee Jones wrote:
> > > > On Thu, 05 Jul 2018, Matti Vaittinen wrote:
> > > > 
> > > > > Document devicetree bindings for ROHM BD71837 PMIC MFD.
> > > > > 
> > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > > > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > > > ---
> > > > >  .../devicetree/bindings/mfd/rohm,bd71837-pmic.txt  | 67 ++++++++++++++++++++++
> > > > >  1 file changed, 67 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
> > > > > +		clock-names = "my-clock";
> > > > > +		clocks = <&pmic>;
> > > > > +	};
> > > > 
> > > > Do you have a real example to give?
> > > 
> > > For clock consumer? Sorry, no I don't.
> > 
> > Might be better to drop it for the time being then.
> 
> I have tested the clk driver using this dummy consumer. So in a sense it
> "works" and can be used as an example on how to write a real clock
> consumer node. Thus I see some value in this example node - even if it
> does not match to any real world HW. If I had to use the clk from this
> PMIC and write HW description I would appreciate this dummy exaple. I
> can drop it if you insist - but I would at least like to hear what is
> the downside on having it here?

My suggestion then would be to make it look as authentic as possible.

It is only an example, so it doesn't *really* matter, but the current
foo,bar one just looks a bit crumby.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v9 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC

[Lee Jones]

> > > > > +++ b/include/linux/mfd/bd71837.h
> > > > > @@ -0,0 +1,391 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > 
> > > > I thought these were meant to use C++ (//) comments?
> > > 
> > > The checkpatch.pl did complain if I used // comment on SPDX line for
> > > header file. OTOH for c-file it required // comments and complained
> > > about /* */ - version. So I did as it suggested and used 
> > 
> > Well that's just dandy -- who comes up with this stuff?
> 
> Engineers I bet =)

Ones who do not suffer from OCD, clearly!

> > > > > +/* Copyright (C) 2018 ROHM Semiconductors */
> > > > > +
> > > > > +/*
> > > > > + * ROHM BD71837MWV header file
> > > > > + */
> > > > 
> > > > If you prefix the mentions of (BD,bd)71837 with (ROHM,rohm) then this
> > > > comment becomes redundant and you can remove it.
> > > 
> > > I am sorry but I am not sure what you suggest here. Do you mean I should
> > > add ROHM or rohm to all definitions here? I think that would make
> > > definitions pretty long so I would certinly need to split more lines.
> > > Such cange would also impact already applied patches. So maybe I
> > > misinterpreted your comment? And in case I did not - can this prefixing of
> > > types - be done as a separate patch set as it impacts to regulators and
> > > clk too? (clk is not yet applied so that is easy to change though).
> > 
> > Any lines which are already long, you can justify as not having to do
> > this, however I think for the filenames and function names it would
> > be nice if they were prefixed.
> 
> Right. For file names this should be easily doable. And when the regmap
> wrappers are removed there are no public functions left. And I think the
> name of file containing the functions is sufficient for grouping
> functions under "Rohm", right? 

That's fine.

> > Filenames are particularly important.  That way all of the Rohm
> > drivers will be grouped.  Unless you can be assured that all Rohm
> > devices will be prefixed by 'bd', then the point is slightly mooted,
> > however since 'bd' doesn't really correlate with 'rohm' it's still
> > difficult to assume that bd* drivers are associated with Rohm -- if
> > you catch my drift.
> 
> I guess I do catch it. And no, all ROHM stuff will definitely not be
> prefixed with bd - which is the name of the power chip

> I mean power IC series.

Now you're getting it! ;)

> > > > > +struct bd71837_board {
> > > > > +	struct regulator_init_data *init_data[BD71837_REGULATOR_CNT];
> > > > > +	int	gpio_intr;
> > > > > +};
> > > > 
> > > > Where is this populated?
> > > > 
> > > Currently nowhere as I use device-tree for getting the regulator/irq
> > > config. This is for architectures which do not use DT - but as I don't
> > > have one for testing I did leave the depends_on OF. If it was populated
> > > I would expect it to be done in some setup code.
> > 
> > Please don't add code for 'what ifs'.
> > 
> > Please remove it and add it back when you need it.
> 
> Allright. Although this will also break the regulator part then...

Well, it's broken anyway ...

> > > > > +/*
> > > > > + * bd71837 sub-driver chip access routines
> > > > > + */
> > > > > +
> > > > 
> > > > Please don't abstract APIs for no reason.
> > > > 
> > > > Use the regmap_* API directly instead.
> > > > 
> > > 
> > > Yes. This was already pointed out by Stephen Boyd. But again, as part of
> > > the patches (reguators) were already applied using the wrappers - I asked
> > > if I can remove these in separate patch set after getting this initial
> > > version out.
> > 
> > That is one of the issues with applying related patches without each
> > of them being reviewed first.  Applying unsuitable code is not the
> > correct thing to do, sorry.
> 
> So I assume you are not Ok with adding the wrappers and removing them
> with later set of patches? I'll do following workaround then:

No, I'm not okay with that at all. :|

> 1. Change MFD Kconfig option name => current regulator code will not be
> compiled even if the MFD would be applied.
> 2. Change MFD according to this discussion (and break the compatibility
> with applied regulator code)
> 3. Fix the regulator code with further patches to Mark
> 4. Fix the depends_on Kconfig option in regulator tree to match the new
> one suggested here.
> 
> Does this sound reasonable?

That's how I would do it.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog