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.
The driver aims to not limit the usage of PMIC. Thus the buck and LDO
naming is generic and not tied to any specific purposes. However there
is following limitations which make it mostly suitable for use cases
where the processor where PMIC driver is running is powered by the PMIC:
- The PMIC is not re-initialized if it resets. PMIC may reset as a
result of voltage monitoring (over/under voltage) or due to reset
request. Driver is only initializing PMIC at probe. This is not
problem as long as processor controlling PMIC is powered by PMIC.
- The PMIC internal state machine is ignored by driver. Driver assumes
the PMIC is wired so that it is always in "run" state when controlled
by the driver.
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 and regulator subsystems.
Patches 2, 3 and 4:
device tree binding documents.
Patch 5:
clock subsystem and support for gated clock.
Patch 6:
regulator driver supporting 8 bucks and 7 LDOs.
This patch series is based on for-mfd-next
---
Matti Vaittinen (6):
mfd: bd71837: mfd driver for ROHM BD71837 PMIC
mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
regulator: bd71837: Devicetree bindings for BD71837 regulators
clk: bd71837: Devicetree bindings for ROHM BD71837 PMIC
clk: bd71837: Add driver for BD71837 PMIC clock
regulator: bd71837: BD71837 PMIC regulator driver
.../bindings/clock/rohm,bd71837-clock.txt | 37 ++
.../devicetree/bindings/mfd/rohm,bd71837-pmic.txt | 52 ++
.../bindings/regulator/rohm,bd71837-regulator.txt | 126 ++++
drivers/clk/Kconfig | 9 +
drivers/clk/Makefile | 1 +
drivers/clk/clk-bd71837.c | 151 +++++
drivers/mfd/Kconfig | 13 +
drivers/mfd/Makefile | 1 +
drivers/mfd/bd71837.c | 222 +++++++
drivers/regulator/Kconfig | 11 +
drivers/regulator/Makefile | 1 +
drivers/regulator/bd71837-regulator.c | 640 +++++++++++++++++++++
include/linux/mfd/bd71837.h | 288 ++++++++++
13 files changed, 1552 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt
create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
create mode 100644 Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt
create mode 100644 drivers/clk/clk-bd71837.c
create mode 100644 drivers/mfd/bd71837.c
create mode 100644 drivers/regulator/bd71837-regulator.c
create mode 100644 include/linux/mfd/bd71837.h
--
2.14.3
ROHM BD71837 PMIC MFD driver providing interrupts and support
for two subsystems:
- clk
- Regulators
Signed-off-by: Matti Vaittinen <[email protected]>
---
drivers/mfd/Kconfig | 13 ++
drivers/mfd/Makefile | 1 +
drivers/mfd/bd71837.c | 222 ++++++++++++++++++++++++++++++++++
include/linux/mfd/bd71837.h | 288 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 524 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..7aa05fc9ed8e 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
+ bool "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..93525fb36259
--- /dev/null
+++ b/drivers/mfd/bd71837.c
@@ -0,0 +1,222 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 ROHM Semiconductors
+// bd71837.c -- ROHM BD71837MWV mfd driver
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/gpio.h>
+#include <linux/regmap.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/bd71837.h>
+
+/* bd71837 multi function cells */
+static struct mfd_cell bd71837_mfd_cells[] = {
+ {
+ .name = "bd71837-clk",
+ .of_compatible = "rohm,bd71837-clk",
+ }, {
+ .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,
+ .mask_invert = false,
+};
+
+static int bd71837_irq_exit(struct bd71837 *bd71837)
+{
+ if (bd71837->chip_irq > 0)
+ regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);
+ return 0;
+}
+
+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,
+};
+
+#ifdef CONFIG_OF
+static const struct of_device_id bd71837_of_match[] = {
+ { .compatible = "rohm,bd71837", .data = (void *)0},
+ { },
+};
+MODULE_DEVICE_TABLE(of, bd71837_of_match);
+
+static int bd71837_parse_dt(struct i2c_client *client, struct bd71837_board **b)
+{
+ struct device_node *np = client->dev.of_node;
+ struct bd71837_board *board_info;
+ unsigned int prop;
+ int r;
+ int rv = -ENOMEM;
+
+ board_info = devm_kzalloc(&client->dev, sizeof(*board_info),
+ GFP_KERNEL);
+ if (!board_info)
+ goto err_out;
+
+ if (client->irq) {
+ dev_dbg(&client->dev, "Got irq %d\n", client->irq);
+ board_info->gpio_intr = client->irq;
+ } else {
+ dev_err(&client->dev, "no pmic intr pin available\n");
+ rv = -ENOENT;
+ goto err_intr;
+ }
+ r = of_property_read_u32(np, "irq_base", &prop);
+ if (!r)
+ board_info->irq_base = prop;
+ else
+ board_info->irq_base = 0;
+
+ rv = 0;
+ *b = board_info;
+ if (0) {
+err_intr:
+ devm_kfree(&client->dev, board_info);
+err_out:
+ dev_err(&client->dev, "failed to parse device-tree (%d)\n", rv);
+ }
+ return rv;
+}
+#endif //CONFIG_OF
+
+static int bd71837_i2c_probe(struct i2c_client *i2c,
+ const struct i2c_device_id *id)
+{
+ struct bd71837 *bd71837;
+ struct bd71837_board *pmic_plat_data;
+ int ret = -EINVAL;
+
+ pmic_plat_data = dev_get_platdata(&i2c->dev);
+
+ if (!pmic_plat_data && i2c->dev.of_node)
+ ret = bd71837_parse_dt(i2c, &pmic_plat_data);
+
+ if (!pmic_plat_data)
+ return ret;
+
+ bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
+ if (bd71837 == NULL)
+ return -ENOMEM;
+
+ bd71837->of_plat_data = pmic_plat_data;
+ i2c_set_clientdata(i2c, bd71837);
+ bd71837->dev = &i2c->dev;
+ bd71837->i2c_client = i2c;
+ bd71837->chip_irq = pmic_plat_data->gpio_intr;
+
+ 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: %d\n", ret);
+ goto err_out;
+ }
+
+ ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
+ if (ret < 0) {
+ dev_err(bd71837->dev,
+ "%s(): Read BD71837_REG_DEVICE failed!\n", __func__);
+ goto err_out;
+ }
+
+ ret = regmap_add_irq_chip(bd71837->regmap, bd71837->chip_irq,
+ IRQF_ONESHOT, 0,
+ &bd71837_irq_chip, &bd71837->irq_data);
+ if (ret < 0) {
+ dev_err(bd71837->dev, "Failed to add irq_chip %d\n", ret);
+ goto err_out;
+ }
+
+ ret = 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)
+ regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);
+err_out:
+
+ return ret;
+}
+
+static int bd71837_i2c_remove(struct i2c_client *i2c)
+{
+ struct bd71837 *bd71837 = i2c_get_clientdata(i2c);
+
+ bd71837_irq_exit(bd71837);
+ mfd_remove_devices(bd71837->dev);
+
+ return 0;
+}
+
+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",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(bd71837_of_match),
+ },
+ .probe = bd71837_i2c_probe,
+ .remove = bd71837_i2c_remove,
+ .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 <[email protected]>");
+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..641b7dba3076
--- /dev/null
+++ b/include/linux/mfd/bd71837.h
@@ -0,0 +1,288 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* 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
+
+struct bd71837;
+struct bd71837_pmic;
+struct bd71837_clk;
+
+/*
+ * Board platform data may be used to initialize regulators.
+ */
+
+struct bd71837_board {
+ struct regulator_init_data *init_data[BD71837_REGULATOR_CNT];
+ int gpio_intr;
+ int irq_base;
+};
+
+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 bd71837_board *of_plat_data;
+};
+
+/*
+ * 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
Document devicetree bindings for ROHM BD71837 PMIC clock output.
Signed-off-by: Matti Vaittinen <[email protected]>
---
.../bindings/clock/rohm,bd71837-clock.txt | 37 ++++++++++++++++++++++
1 file changed, 37 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt
diff --git a/Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt b/Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt
new file mode 100644
index 000000000000..9759fd145781
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt
@@ -0,0 +1,37 @@
+ROHM BD71837 Power Management Integrated Circuit clock bindings
+
+BD71837 clock node should be sub node of the BD71837 MFD node.
+See BD71837 MFD bindings at
+Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
+
+Required properties:
+- compatible : Should be "rohm,bd71837-clk"
+- clock-frequency : Should be 32768
+- #clock-cells : Should be 0
+
+Optional properties:
+- clock-output-names : Should contain name for output clock.
+ Name "bd71837-32k-out" is used if this is not given.
+
+
+Example:
+
+pmic: bd71837@4b {
+ compatible = "rohm,bd71837";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x4b>;
+ interrupt-parent = <&gpio1>;
+ interrupts = <29 GPIO_ACTIVE_LOW>;
+ interrupt-names = "irq";
+ #interrupt-cells = <1>;
+ interrupt-controller;
+
+ /* Clock node */
+ clk: bd71837-32k-out {
+ compatible = "rohm,bd71837-clk";
+ #clock-cells = <0>;
+ clock-frequency = <32768>;
+ clock-output-names = "bd71837-32k-out";
+ };
+};
--
2.14.3
Document devicetree bindings for ROHM BD71837 PMIC MFD.
Signed-off-by: Matti Vaittinen <[email protected]>
---
.../devicetree/bindings/mfd/rohm,bd71837-pmic.txt | 52 ++++++++++++++++++++++
1 file changed, 52 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..bbc46d38b162
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
@@ -0,0 +1,52 @@
+* 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.
+
+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.
+ - interrupt-controller : Marks the device node as an interrupt controller.
+ - #interrupt-cells : The number of cells to describe an IRQ, should be 1 or 2.
+ The first cell is the IRQ number.
+ The second cell if present is the flags, encoded as trigger
+ masks from ../interrupt-controller/interrupts.txt.
+ - regulators: : List of child nodes that specify the regulators
+ Please see ../regulator/rohm,bd71837-regulator.txt
+ - clock: : Please see ../clock/rohm,bd71837-clock.txt
+
+Example:
+
+ pmic: bd71837@4b {
+ compatible = "rohm,bd71837";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x4b>;
+ interrupt-parent = <&gpio1>;
+ interrupts = <29 GPIO_ACTIVE_LOW>;
+ interrupt-names = "irq";
+ #interrupt-cells = <1>;
+ interrupt-controller;
+
+ regulators {
+ buck1: BUCK1 {
+ regulator-name = "buck1";
+ regulator-min-microvolt = <700000>;
+ regulator-max-microvolt = <1300000>;
+ regulator-boot-on;
+ regulator-ramp-delay = <1250>;
+ };
+ ...
+ };
+ clk: bd71837-32k-out {
+ compatible = "rohm,bd71837-clk";
+ #clock-cells = <0>;
+ clock-frequency = <32768>;
+ clock-output-names = "bd71837-32k-out";
+ };
+ };
--
2.14.3
Support BD71837 gateable 32768 Hz clock.
Signed-off-by: Matti Vaittinen <[email protected]>
---
drivers/clk/Kconfig | 9 +++
drivers/clk/Makefile | 1 +
drivers/clk/clk-bd71837.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 161 insertions(+)
create mode 100644 drivers/clk/clk-bd71837.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 41492e980ef4..4b045699bb5e 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -279,6 +279,15 @@ config COMMON_CLK_STM32H7
---help---
Support for stm32h7 SoC family clocks
+config COMMON_CLK_BD71837
+ tristate "Clock driver for ROHM BD71837 PMIC MFD"
+ depends on MFD_BD71837
+ depends on I2C=y
+ depends on OF
+ help
+ This driver supports ROHM BD71837 PMIC clock.
+
+
source "drivers/clk/bcm/Kconfig"
source "drivers/clk/hisilicon/Kconfig"
source "drivers/clk/imgtec/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index de6d06ac790b..8393c4af7d5a 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -21,6 +21,7 @@ endif
obj-$(CONFIG_MACH_ASM9260) += clk-asm9260.o
obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN) += clk-axi-clkgen.o
obj-$(CONFIG_ARCH_AXXIA) += clk-axm5516.o
+obj-$(CONFIG_COMMON_CLK_BD71837) += clk-bd71837.o
obj-$(CONFIG_COMMON_CLK_CDCE706) += clk-cdce706.o
obj-$(CONFIG_COMMON_CLK_CDCE925) += clk-cdce925.o
obj-$(CONFIG_ARCH_CLPS711X) += clk-clps711x.o
diff --git a/drivers/clk/clk-bd71837.c b/drivers/clk/clk-bd71837.c
new file mode 100644
index 000000000000..91456d1077ac
--- /dev/null
+++ b/drivers/clk/clk-bd71837.c
@@ -0,0 +1,151 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 ROHM Semiconductors
+// bd71837.c -- ROHM BD71837MWV clock driver
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/mfd/bd71837.h>
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+
+static int bd71837_clk_enable(struct clk_hw *hw);
+static void bd71837_clk_disable(struct clk_hw *hw);
+static int bd71837_clk_is_enabled(struct clk_hw *hw);
+
+struct bd71837_clk {
+ struct clk_hw hw;
+ uint8_t reg;
+ uint8_t mask;
+ unsigned long rate;
+ struct platform_device *pdev;
+ struct bd71837 *mfd;
+};
+
+static unsigned long bd71837_clk_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate);
+
+static const struct clk_ops bd71837_clk_ops = {
+ .recalc_rate = &bd71837_clk_recalc_rate,
+ .prepare = &bd71837_clk_enable,
+ .unprepare = &bd71837_clk_disable,
+ .is_prepared = &bd71837_clk_is_enabled,
+};
+
+static int bd71837_clk_set(struct clk_hw *hw, int status)
+{
+ struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
+
+ return bd71837_update_bits(c->mfd, c->reg, c->mask, status);
+}
+
+static void bd71837_clk_disable(struct clk_hw *hw)
+{
+ int rv;
+ struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
+
+ rv = bd71837_clk_set(hw, 0);
+ if (rv)
+ dev_err(&c->pdev->dev, "Failed to disable 32K clk (%d)\n", rv);
+}
+
+static int bd71837_clk_enable(struct clk_hw *hw)
+{
+ return bd71837_clk_set(hw, 1);
+}
+
+static int bd71837_clk_is_enabled(struct clk_hw *hw)
+{
+ struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
+
+ return c->mask & bd71837_reg_read(c->mfd, c->reg);
+}
+
+static unsigned long bd71837_clk_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
+
+ return c->rate;
+}
+
+static int bd71837_clk_probe(struct platform_device *pdev)
+{
+ struct bd71837_clk *c;
+ int rval = -ENOMEM;
+ struct bd71837 *mfd = dev_get_drvdata(pdev->dev.parent);
+ const char *errstr = "memory allocation for bd71837 data failed";
+ struct clk_init_data init = {
+ .name = "bd71837-32k-out",
+ .ops = &bd71837_clk_ops,
+ };
+
+ c = kzalloc(sizeof(struct bd71837_clk), GFP_KERNEL);
+ if (!c)
+ goto err_out;
+
+ c->reg = BD71837_REG_OUT32K;
+ c->mask = BD71837_OUT32K_EN;
+ c->rate = BD71837_CLK_RATE;
+ c->mfd = mfd;
+ c->pdev = pdev;
+
+ if (pdev->dev.of_node)
+ of_property_read_string_index(pdev->dev.of_node,
+ "clock-output-names", 0,
+ &init.name);
+
+ c->hw.init = &init;
+
+ errstr = "failed to register 32K clk";
+ rval = clk_hw_register(&pdev->dev, &c->hw);
+ if (rval)
+ goto err_free;
+
+ errstr = "failed to register clkdev for bd71837";
+ rval = clk_hw_register_clkdev(&c->hw, init.name, NULL);
+ if (rval)
+ goto err_unregister;
+
+ platform_set_drvdata(pdev, c);
+ dev_dbg(&pdev->dev, "bd71837_clk successfully probed\n");
+
+ return 0;
+
+err_unregister:
+ clk_hw_unregister(&c->hw);
+err_free:
+ kfree(c);
+err_out:
+ dev_err(&pdev->dev, "%s\n", errstr);
+ return rval;
+}
+
+static int bd71837_clk_remove(struct platform_device *pdev)
+{
+ struct bd71837_clk *c = platform_get_drvdata(pdev);
+
+ if (c) {
+ clk_hw_unregister(&c->hw);
+ kfree(c);
+ platform_set_drvdata(pdev, NULL);
+ }
+ return 0;
+}
+
+static struct platform_driver bd71837_clk = {
+ .driver = {
+ .name = "bd71837-clk",
+ },
+ .probe = bd71837_clk_probe,
+ .remove = bd71837_clk_remove,
+};
+
+module_platform_driver(bd71837_clk);
+
+MODULE_AUTHOR("Matti Vaittinen <[email protected]>");
+MODULE_DESCRIPTION("BD71837 chip clk driver");
+MODULE_LICENSE("GPL");
--
2.14.3
Support for controlling the 8 bucks and 7 LDOs the PMIC contains.
Signed-off-by: Matti Vaittinen <[email protected]>
---
drivers/regulator/Kconfig | 11 +
drivers/regulator/Makefile | 1 +
drivers/regulator/bd71837-regulator.c | 640 ++++++++++++++++++++++++++++++++++
3 files changed, 652 insertions(+)
create mode 100644 drivers/regulator/bd71837-regulator.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 097f61784a7d..139f4b53fea0 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -180,6 +180,17 @@ config REGULATOR_BCM590XX
BCM590xx PMUs. This will enable support for the software
controllable LDO/Switching regulators.
+config REGULATOR_BD71837
+ tristate "ROHM BD71837 Power Regulator"
+ depends on MFD_BD71837
+ help
+ This driver supports voltage regulators on ROHM BD71837 PMIC.
+ This will enable support for the software controllable buck
+ and LDO regulators.
+
+ This driver can also be built as a module. If so, the module
+ will be called bd71837-regulator.
+
config REGULATOR_BD9571MWV
tristate "ROHM BD9571MWV Regulators"
depends on MFD_BD9571MWV
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 590674fbecd7..1b4d8ec416c2 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_REGULATOR_AS3711) += as3711-regulator.o
obj-$(CONFIG_REGULATOR_AS3722) += as3722-regulator.o
obj-$(CONFIG_REGULATOR_AXP20X) += axp20x-regulator.o
obj-$(CONFIG_REGULATOR_BCM590XX) += bcm590xx-regulator.o
+obj-$(CONFIG_REGULATOR_BD71837) += bd71837-regulator.o
obj-$(CONFIG_REGULATOR_BD9571MWV) += bd9571mwv-regulator.o
obj-$(CONFIG_REGULATOR_DA903X) += da903x.o
obj-$(CONFIG_REGULATOR_DA9052) += da9052-regulator.o
diff --git a/drivers/regulator/bd71837-regulator.c b/drivers/regulator/bd71837-regulator.c
new file mode 100644
index 000000000000..6eae4d0432a2
--- /dev/null
+++ b/drivers/regulator/bd71837-regulator.c
@@ -0,0 +1,640 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 ROHM Semiconductors
+// bd71837-regulator.c ROHM BD71837MWV regulator driver
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <linux/gpio.h>
+#include <linux/mfd/bd71837.h>
+#include <linux/regulator/of_regulator.h>
+
+struct bd71837_pmic {
+ struct regulator_desc descs[BD71837_REGULATOR_CNT];
+ struct bd71837 *mfd;
+ struct platform_device *pdev;
+ struct regulator_dev *rdev[BD71837_REGULATOR_CNT];
+};
+
+/*
+ * BUCK1/2/3/4
+ * BUCK1RAMPRATE[1:0] BUCK1 DVS ramp rate setting
+ * 00: 10.00mV/usec 10mV 1uS
+ * 01: 5.00mV/usec 10mV 2uS
+ * 10: 2.50mV/usec 10mV 4uS
+ * 11: 1.25mV/usec 10mV 8uS
+ */
+static int bd71837_buck1234_set_ramp_delay(struct regulator_dev *rdev,
+ int ramp_delay)
+{
+ struct bd71837_pmic *pmic = rdev_get_drvdata(rdev);
+ struct bd71837 *mfd = pmic->mfd;
+ int id = rdev->desc->id;
+ unsigned int ramp_value = BUCK_RAMPRATE_10P00MV;
+
+ dev_dbg(&(pmic->pdev->dev), "Buck[%d] Set Ramp = %d\n", id + 1,
+ ramp_delay);
+ switch (ramp_delay) {
+ case 1 ... 1250:
+ ramp_value = BUCK_RAMPRATE_1P25MV;
+ break;
+ case 1251 ... 2500:
+ ramp_value = BUCK_RAMPRATE_2P50MV;
+ break;
+ case 2501 ... 5000:
+ ramp_value = BUCK_RAMPRATE_5P00MV;
+ break;
+ case 5001 ... 10000:
+ ramp_value = BUCK_RAMPRATE_10P00MV;
+ break;
+ default:
+ ramp_value = BUCK_RAMPRATE_10P00MV;
+ dev_err(&pmic->pdev->dev,
+ "%s: ramp_delay: %d not supported, setting 10000mV//us\n",
+ rdev->desc->name, ramp_delay);
+ }
+
+ return regmap_update_bits(mfd->regmap, BD71837_REG_BUCK1_CTRL + id,
+ BUCK_RAMPRATE_MASK, ramp_value << 6);
+}
+
+/* Bucks 1 to 4 support DVS. PWM mode is used when voltage is changed.
+ * Bucks 5 to 8 and LDOs can use PFM and must be disabled when voltage
+ * is changed. Hence we return -EBUSY for these if voltage is changed
+ * when BUCK/LDO is enabled.
+ */
+static int bd71837_set_voltage_sel_restricted(struct regulator_dev *rdev,
+ unsigned int sel)
+{
+ int ret;
+
+ ret = regulator_is_enabled_regmap(rdev);
+ if (!ret)
+ ret = regulator_set_voltage_sel_regmap(rdev, sel);
+ else if (ret == 1)
+ ret = -EBUSY;
+ return ret;
+}
+
+static struct regulator_ops bd71837_ldo_regulator_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .list_voltage = regulator_list_voltage_linear_range,
+ .set_voltage_sel = bd71837_set_voltage_sel_restricted,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+};
+
+static struct regulator_ops bd71837_ldo_regulator_nolinear_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .list_voltage = regulator_list_voltage_table,
+ .set_voltage_sel = bd71837_set_voltage_sel_restricted,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+};
+
+static struct regulator_ops bd71837_buck_regulator_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .list_voltage = regulator_list_voltage_linear_range,
+ .set_voltage_sel = bd71837_set_voltage_sel_restricted,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+ .set_voltage_time_sel = regulator_set_voltage_time_sel,
+};
+
+static struct regulator_ops bd71837_buck_regulator_nolinear_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .list_voltage = regulator_list_voltage_table,
+ .set_voltage_sel = bd71837_set_voltage_sel_restricted,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+ .set_voltage_time_sel = regulator_set_voltage_time_sel,
+};
+
+static struct regulator_ops bd71837_buck1234_regulator_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .list_voltage = regulator_list_voltage_linear_range,
+ .set_voltage_sel = regulator_set_voltage_sel_regmap,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+ .set_voltage_time_sel = regulator_set_voltage_time_sel,
+ .set_ramp_delay = bd71837_buck1234_set_ramp_delay,
+};
+
+/*
+ * BUCK1/2/3/4
+ * 0.70 to 1.30V (10mV step)
+ */
+static const struct regulator_linear_range bd71837_buck1234_voltage_ranges[] = {
+ REGULATOR_LINEAR_RANGE(700000, 0x00, 0x3C, 10000),
+ REGULATOR_LINEAR_RANGE(1300000, 0x3D, 0x3F, 0),
+};
+
+/*
+ * BUCK5
+ * 0.9V to 1.35V ()
+ */
+static const struct regulator_linear_range bd71837_buck5_voltage_ranges[] = {
+ REGULATOR_LINEAR_RANGE(700000, 0x00, 0x03, 100000),
+ REGULATOR_LINEAR_RANGE(1050000, 0x04, 0x05, 50000),
+ REGULATOR_LINEAR_RANGE(1200000, 0x06, 0x07, 150000),
+};
+
+/*
+ * BUCK6
+ * 3.0V to 3.3V (step 100mV)
+ */
+static const struct regulator_linear_range bd71837_buck6_voltage_ranges[] = {
+ REGULATOR_LINEAR_RANGE(3000000, 0x00, 0x03, 100000),
+};
+
+/*
+ * BUCK7
+ * 000 = 1.605V
+ * 001 = 1.695V
+ * 010 = 1.755V
+ * 011 = 1.8V (Initial)
+ * 100 = 1.845V
+ * 101 = 1.905V
+ * 110 = 1.95V
+ * 111 = 1.995V
+ */
+static const unsigned int buck_7_volts[] = {
+ 1605000, 1695000, 1755000, 1800000, 1845000, 1905000, 1950000, 1995000
+};
+
+/*
+ * BUCK8
+ * 0.8V to 1.40V (step 10mV)
+ */
+static const struct regulator_linear_range bd71837_buck8_voltage_ranges[] = {
+ REGULATOR_LINEAR_RANGE(800000, 0x00, 0x3C, 10000),
+ REGULATOR_LINEAR_RANGE(1400000, 0x3D, 0x3F, 0),
+};
+
+/*
+ * LDO1
+ * 3.0 to 3.3V (100mV step)
+ */
+static const struct regulator_linear_range bd71837_ldo1_voltage_ranges[] = {
+ REGULATOR_LINEAR_RANGE(3000000, 0x00, 0x03, 100000),
+};
+
+/*
+ * LDO2
+ * 0.8 or 0.9V
+ */
+const unsigned int ldo_2_volts[] = {
+ 900000, 800000
+};
+
+/*
+ * LDO3
+ * 1.8 to 3.3V (100mV step)
+ */
+static const struct regulator_linear_range bd71837_ldo3_voltage_ranges[] = {
+ REGULATOR_LINEAR_RANGE(1800000, 0x00, 0x0F, 100000),
+};
+
+/*
+ * LDO4
+ * 0.9 to 1.8V (100mV step)
+ */
+static const struct regulator_linear_range bd71837_ldo4_voltage_ranges[] = {
+ REGULATOR_LINEAR_RANGE(900000, 0x00, 0x09, 100000),
+ REGULATOR_LINEAR_RANGE(1800000, 0x0A, 0x0F, 0),
+};
+
+/*
+ * LDO5
+ * 1.8 to 3.3V (100mV step)
+ */
+static const struct regulator_linear_range bd71837_ldo5_voltage_ranges[] = {
+ REGULATOR_LINEAR_RANGE(1800000, 0x00, 0x0F, 100000),
+};
+
+/*
+ * LDO6
+ * 0.9 to 1.8V (100mV step)
+ */
+static const struct regulator_linear_range bd71837_ldo6_voltage_ranges[] = {
+ REGULATOR_LINEAR_RANGE(900000, 0x00, 0x09, 100000),
+ REGULATOR_LINEAR_RANGE(1800000, 0x0A, 0x0F, 0),
+};
+
+/*
+ * LDO7
+ * 1.8 to 3.3V (100mV step)
+ */
+static const struct regulator_linear_range bd71837_ldo7_voltage_ranges[] = {
+ REGULATOR_LINEAR_RANGE(1800000, 0x00, 0x0F, 100000),
+};
+
+static const struct regulator_desc bd71837_regulators[] = {
+ {
+ .name = "buck1",
+ .of_match = of_match_ptr("BUCK1"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_BUCK1,
+ .ops = &bd71837_buck1234_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_BUCK1_VOLTAGE_NUM,
+ .linear_ranges = bd71837_buck1234_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_buck1234_voltage_ranges),
+ .vsel_reg = BD71837_REG_BUCK1_VOLT_RUN,
+ .vsel_mask = BUCK1_RUN_MASK,
+ .enable_reg = BD71837_REG_BUCK1_CTRL,
+ .enable_mask = BD71837_BUCK_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "buck2",
+ .of_match = of_match_ptr("BUCK2"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_BUCK2,
+ .ops = &bd71837_buck1234_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_BUCK2_VOLTAGE_NUM,
+ .linear_ranges = bd71837_buck1234_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_buck1234_voltage_ranges),
+ .vsel_reg = BD71837_REG_BUCK2_VOLT_RUN,
+ .vsel_mask = BUCK2_RUN_MASK,
+ .enable_reg = BD71837_REG_BUCK2_CTRL,
+ .enable_mask = BD71837_BUCK_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "buck3",
+ .of_match = of_match_ptr("BUCK3"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_BUCK3,
+ .ops = &bd71837_buck1234_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_BUCK3_VOLTAGE_NUM,
+ .linear_ranges = bd71837_buck1234_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_buck1234_voltage_ranges),
+ .vsel_reg = BD71837_REG_BUCK3_VOLT_RUN,
+ .vsel_mask = BUCK3_RUN_MASK,
+ .enable_reg = BD71837_REG_BUCK3_CTRL,
+ .enable_mask = BD71837_BUCK_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "buck4",
+ .of_match = of_match_ptr("BUCK4"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_BUCK4,
+ .ops = &bd71837_buck1234_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_BUCK4_VOLTAGE_NUM,
+ .linear_ranges = bd71837_buck1234_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_buck1234_voltage_ranges),
+ .vsel_reg = BD71837_REG_BUCK4_VOLT_RUN,
+ .vsel_mask = BUCK4_RUN_MASK,
+ .enable_reg = BD71837_REG_BUCK4_CTRL,
+ .enable_mask = BD71837_BUCK_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "buck5",
+ .of_match = of_match_ptr("BUCK5"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_BUCK5,
+ .ops = &bd71837_buck_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_BUCK5_VOLTAGE_NUM,
+ .linear_ranges = bd71837_buck5_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_buck5_voltage_ranges),
+ .vsel_reg = BD71837_REG_BUCK5_VOLT,
+ .vsel_mask = BUCK5_MASK,
+ .enable_reg = BD71837_REG_BUCK5_CTRL,
+ .enable_mask = BD71837_BUCK_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "buck6",
+ .of_match = of_match_ptr("BUCK6"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_BUCK6,
+ .ops = &bd71837_buck_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_BUCK6_VOLTAGE_NUM,
+ .linear_ranges = bd71837_buck6_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_buck6_voltage_ranges),
+ .vsel_reg = BD71837_REG_BUCK6_VOLT,
+ .vsel_mask = BUCK6_MASK,
+ .enable_reg = BD71837_REG_BUCK6_CTRL,
+ .enable_mask = BD71837_BUCK_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "buck7",
+ .of_match = of_match_ptr("BUCK7"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_BUCK7,
+ .ops = &bd71837_buck_regulator_nolinear_ops,
+ .type = REGULATOR_VOLTAGE,
+ .volt_table = &buck_7_volts[0],
+ .n_voltages = ARRAY_SIZE(buck_7_volts),
+ .vsel_reg = BD71837_REG_BUCK7_VOLT,
+ .vsel_mask = BUCK7_MASK,
+ .enable_reg = BD71837_REG_BUCK7_CTRL,
+ .enable_mask = BD71837_BUCK_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "buck8",
+ .of_match = of_match_ptr("BUCK8"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_BUCK8,
+ .ops = &bd71837_buck_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_BUCK8_VOLTAGE_NUM,
+ .linear_ranges = bd71837_buck8_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_buck8_voltage_ranges),
+ .vsel_reg = BD71837_REG_BUCK8_VOLT,
+ .vsel_mask = BUCK8_MASK,
+ .enable_reg = BD71837_REG_BUCK8_CTRL,
+ .enable_mask = BD71837_BUCK_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "ldo1",
+ .of_match = of_match_ptr("LDO1"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_LDO1,
+ .ops = &bd71837_ldo_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_LDO1_VOLTAGE_NUM,
+ .linear_ranges = bd71837_ldo1_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_ldo1_voltage_ranges),
+ .vsel_reg = BD71837_REG_LDO1_VOLT,
+ .vsel_mask = LDO1_MASK,
+ .enable_reg = BD71837_REG_LDO1_VOLT,
+ .enable_mask = BD71837_LDO_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "ldo2",
+ .of_match = of_match_ptr("LDO2"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_LDO2,
+ .ops = &bd71837_ldo_regulator_nolinear_ops,
+ .type = REGULATOR_VOLTAGE,
+ .volt_table = &ldo_2_volts[0],
+ .vsel_reg = BD71837_REG_LDO2_VOLT,
+ .vsel_mask = LDO2_MASK,
+ .n_voltages = ARRAY_SIZE(ldo_2_volts),
+ .n_voltages = BD71837_LDO2_VOLTAGE_NUM,
+ .enable_reg = BD71837_REG_LDO2_VOLT,
+ .enable_mask = BD71837_LDO_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "ldo3",
+ .of_match = of_match_ptr("LDO3"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_LDO3,
+ .ops = &bd71837_ldo_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_LDO3_VOLTAGE_NUM,
+ .linear_ranges = bd71837_ldo3_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_ldo3_voltage_ranges),
+ .vsel_reg = BD71837_REG_LDO3_VOLT,
+ .vsel_mask = LDO3_MASK,
+ .enable_reg = BD71837_REG_LDO3_VOLT,
+ .enable_mask = BD71837_LDO_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "ldo4",
+ .of_match = of_match_ptr("LDO4"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_LDO4,
+ .ops = &bd71837_ldo_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_LDO4_VOLTAGE_NUM,
+ .linear_ranges = bd71837_ldo4_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_ldo4_voltage_ranges),
+ .vsel_reg = BD71837_REG_LDO4_VOLT,
+ .vsel_mask = LDO4_MASK,
+ .enable_reg = BD71837_REG_LDO4_VOLT,
+ .enable_mask = BD71837_LDO_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "ldo5",
+ .of_match = of_match_ptr("LDO5"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_LDO5,
+ .ops = &bd71837_ldo_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_LDO5_VOLTAGE_NUM,
+ .linear_ranges = bd71837_ldo5_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_ldo5_voltage_ranges),
+ /* LDO5 is supplied by buck6 */
+ .supply_name = "buck6",
+ .vsel_reg = BD71837_REG_LDO5_VOLT,
+ .vsel_mask = LDO5_MASK,
+ .enable_reg = BD71837_REG_LDO5_VOLT,
+ .enable_mask = BD71837_LDO_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "ldo6",
+ .of_match = of_match_ptr("LDO6"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_LDO6,
+ .ops = &bd71837_ldo_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_LDO6_VOLTAGE_NUM,
+ .linear_ranges = bd71837_ldo6_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_ldo6_voltage_ranges),
+ /* LDO6 is supplied by buck7 */
+ .supply_name = "buck7",
+ .vsel_reg = BD71837_REG_LDO6_VOLT,
+ .vsel_mask = LDO6_MASK,
+ .enable_reg = BD71837_REG_LDO6_VOLT,
+ .enable_mask = BD71837_LDO_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "ldo7",
+ .of_match = of_match_ptr("LDO7"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_LDO7,
+ .ops = &bd71837_ldo_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_LDO7_VOLTAGE_NUM,
+ .linear_ranges = bd71837_ldo7_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_ldo7_voltage_ranges),
+ .vsel_reg = BD71837_REG_LDO7_VOLT,
+ .vsel_mask = LDO7_MASK,
+ .enable_reg = BD71837_REG_LDO7_VOLT,
+ .enable_mask = BD71837_LDO_EN,
+ .owner = THIS_MODULE,
+ },
+};
+
+struct reg_init {
+ unsigned int reg;
+ unsigned int mask;
+};
+
+static int bd71837_probe(struct platform_device *pdev)
+{
+ struct bd71837_pmic *pmic;
+ struct bd71837_board *pdata;
+ struct regulator_config config = { 0 };
+ struct reg_init pmic_regulator_inits[] = {
+ {
+ .reg = BD71837_REG_BUCK1_CTRL,
+ .mask = BD71837_BUCK_SEL,
+ }, {
+ .reg = BD71837_REG_BUCK2_CTRL,
+ .mask = BD71837_BUCK_SEL,
+ }, {
+ .reg = BD71837_REG_BUCK3_CTRL,
+ .mask = BD71837_BUCK_SEL,
+ }, {
+ .reg = BD71837_REG_BUCK4_CTRL,
+ .mask = BD71837_BUCK_SEL,
+ }, {
+ .reg = BD71837_REG_BUCK5_CTRL,
+ .mask = BD71837_BUCK_SEL,
+ }, {
+ .reg = BD71837_REG_BUCK6_CTRL,
+ .mask = BD71837_BUCK_SEL,
+ }, {
+ .reg = BD71837_REG_BUCK7_CTRL,
+ .mask = BD71837_BUCK_SEL,
+ }, {
+ .reg = BD71837_REG_BUCK8_CTRL,
+ .mask = BD71837_BUCK_SEL,
+ }, {
+ .reg = BD71837_REG_LDO1_VOLT,
+ .mask = BD71837_LDO_SEL,
+ }, {
+ .reg = BD71837_REG_LDO2_VOLT,
+ .mask = BD71837_LDO_SEL,
+ }, {
+ .reg = BD71837_REG_LDO3_VOLT,
+ .mask = BD71837_LDO_SEL,
+ }, {
+ .reg = BD71837_REG_LDO4_VOLT,
+ .mask = BD71837_LDO_SEL,
+ }, {
+ .reg = BD71837_REG_LDO5_VOLT,
+ .mask = BD71837_LDO_SEL,
+ }, {
+ .reg = BD71837_REG_LDO6_VOLT,
+ .mask = BD71837_LDO_SEL,
+ }, {
+ .reg = BD71837_REG_LDO7_VOLT,
+ .mask = BD71837_LDO_SEL,
+ }
+ };
+
+ int i, err;
+
+ pmic = devm_kzalloc(&pdev->dev, sizeof(struct bd71837_pmic),
+ GFP_KERNEL);
+ if (!pmic)
+ return -ENOMEM;
+
+ memcpy(pmic->descs, bd71837_regulators, sizeof(pmic->descs));
+
+ pmic->pdev = pdev;
+ pmic->mfd = dev_get_drvdata(pdev->dev.parent);
+
+ if (!pmic->mfd) {
+ dev_err(&pdev->dev, "No MFD driver data\n");
+ err = -EINVAL;
+ goto err;
+ }
+ platform_set_drvdata(pdev, pmic);
+ pdata = dev_get_platdata(pmic->mfd->dev);
+
+ /* Register LOCK release */
+ err = regmap_update_bits(pmic->mfd->regmap, BD71837_REG_REGLOCK,
+ (REGLOCK_PWRSEQ | REGLOCK_VREG), 0);
+ if (err) {
+ dev_err(&pmic->pdev->dev, "Failed to unlock PMIC (%d)\n", err);
+ goto err;
+ } else {
+ dev_dbg(&pmic->pdev->dev, "%s: Unlocked lock register 0x%x\n",
+ __func__, BD71837_REG_REGLOCK);
+ }
+
+ for (i = 0; i < ARRAY_SIZE(pmic_regulator_inits); i++) {
+
+ struct regulator_desc *desc;
+ struct regulator_dev *rdev;
+
+ desc = &pmic->descs[i];
+
+ if (pdata)
+ config.init_data = pdata->init_data[i];
+
+ config.dev = pdev->dev.parent;
+ config.driver_data = pmic;
+ config.regmap = pmic->mfd->regmap;
+
+ rdev = devm_regulator_register(&pdev->dev, desc, &config);
+ if (IS_ERR(rdev)) {
+ dev_err(pmic->mfd->dev,
+ "failed to register %s regulator\n",
+ desc->name);
+ err = PTR_ERR(rdev);
+ goto err;
+ }
+ /* Regulator register gets the regulator constraints and
+ * applies them (set_machine_constraints). This should have
+ * turned the control register(s) to correct values and we
+ * can now switch the control from PMIC state machine to the
+ * register interface
+ */
+ err = regmap_update_bits(pmic->mfd->regmap,
+ pmic_regulator_inits[i].reg,
+ pmic_regulator_inits[i].mask,
+ 0xFFFFFFFF);
+ if (err) {
+ dev_err(&pmic->pdev->dev,
+ "Failed to write BUCK/LDO SEL bit for (%s)\n",
+ desc->name);
+ goto err;
+ }
+
+ pmic->rdev[i] = rdev;
+ }
+
+ return 0;
+
+err:
+ return err;
+}
+
+static struct platform_driver bd71837_regulator = {
+ .driver = {
+ .name = "bd71837-pmic",
+ .owner = THIS_MODULE,
+ },
+ .probe = bd71837_probe,
+};
+
+module_platform_driver(bd71837_regulator);
+
+MODULE_AUTHOR("Matti Vaittinen <[email protected]>");
+MODULE_DESCRIPTION("BD71837 voltage regulator driver");
+MODULE_LICENSE("GPL");
--
2.14.3
Document devicetree bindings for ROHM BD71837 PMIC regulators.
Signed-off-by: Matti Vaittinen <[email protected]>
---
.../bindings/regulator/rohm,bd71837-regulator.txt | 126 +++++++++++++++++++++
1 file changed, 126 insertions(+)
create mode 100644 Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt
diff --git a/Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt b/Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt
new file mode 100644
index 000000000000..4edf3137d9f7
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt
@@ -0,0 +1,126 @@
+ROHM BD71837 Power Management Integrated Circuit (PMIC) regulator bindings
+
+BD71837MWV is a programmable Power Management
+IC (PMIC) 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.
+
+Required properties:
+ - regulator-name: should be "buck1", ..., "buck8" and "ldo1", ..., "ldo7"
+
+List of regulators provided by this controller. BD71837 regulators node
+should be sub node of the BD71837 MFD node. See BD71837 MFD bindings at
+Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
+Regulator nodes should be named to BUCK_<number> and LDO_<number>. The
+definition for each of these nodes is defined using the standard
+binding for regulators at
+Documentation/devicetree/bindings/regulator/regulator.txt.
+Note that if BD71837 starts at RUN state you probably want to use
+regulator-boot-on at least for BUCK6 and BUCK7 so that those are not
+disabled by driver at startup. LDO5 and LDO6 are supplied by those and
+if they are disabled at startup the voltage monitoring for LDO5/LDO6 will
+cause PMIC to reset.
+
+The valid names for regulator nodes are:
+BUCK1, BUCK2, BUCK3, BUCK4, BUCK5, BUCK6, BUCK7, BUCK8
+LDO1, LDO2, LDO3, LDO4, LDO5, LDO6, LDO7
+
+Optional properties:
+- Any optional property defined in bindings/regulator/regulator.txt
+
+Example:
+regulators {
+ buck1: BUCK1 {
+ regulator-name = "buck1";
+ regulator-min-microvolt = <700000>;
+ regulator-max-microvolt = <1300000>;
+ regulator-boot-on;
+ regulator-ramp-delay = <1250>;
+ };
+ buck2: BUCK2 {
+ regulator-name = "buck2";
+ regulator-min-microvolt = <700000>;
+ regulator-max-microvolt = <1300000>;
+ regulator-boot-on;
+ regulator-always-on;
+ regulator-ramp-delay = <1250>;
+ };
+ buck3: BUCK3 {
+ regulator-name = "buck3";
+ regulator-min-microvolt = <700000>;
+ regulator-max-microvolt = <1300000>;
+ regulator-boot-on;
+ };
+ buck4: BUCK4 {
+ regulator-name = "buck4";
+ regulator-min-microvolt = <700000>;
+ regulator-max-microvolt = <1300000>;
+ regulator-boot-on;
+ };
+ buck5: BUCK5 {
+ regulator-name = "buck5";
+ regulator-min-microvolt = <700000>;
+ regulator-max-microvolt = <1350000>;
+ regulator-boot-on;
+ };
+ buck6: BUCK6 {
+ regulator-name = "buck6";
+ regulator-min-microvolt = <3000000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-boot-on;
+ };
+ buck7: BUCK7 {
+ regulator-name = "buck7";
+ regulator-min-microvolt = <1605000>;
+ regulator-max-microvolt = <1995000>;
+ regulator-boot-on;
+ };
+ buck8: BUCK8 {
+ regulator-name = "buck8";
+ regulator-min-microvolt = <800000>;
+ regulator-max-microvolt = <1400000>;
+ };
+
+ ldo1: LDO1 {
+ regulator-name = "ldo1";
+ regulator-min-microvolt = <3000000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-boot-on;
+ };
+ ldo2: LDO2 {
+ regulator-name = "ldo2";
+ regulator-min-microvolt = <900000>;
+ regulator-max-microvolt = <900000>;
+ regulator-boot-on;
+ };
+ ldo3: LDO3 {
+ regulator-name = "ldo3";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <3300000>;
+ };
+ ldo4: LDO4 {
+ regulator-name = "ldo4";
+ regulator-min-microvolt = <900000>;
+ regulator-max-microvolt = <1800000>;
+ };
+ ldo5: LDO5 {
+ regulator-name = "ldo5";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <3300000>;
+ };
+ ldo6: LDO6 {
+ regulator-name = "ldo6";
+ regulator-min-microvolt = <900000>;
+ regulator-max-microvolt = <1800000>;
+ };
+ ldo7_reg: LDO7 {
+ regulator-name = "ldo7";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <3300000>;
+ };
+};
+
+
--
2.14.3
Hello All,
I would like to ask for an educated opinion from more experienced
regulator driver developers.
On Wed, May 30, 2018 at 11:41:13AM +0300, Matti Vaittinen wrote:
> 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.
>
> The driver aims to not limit the usage of PMIC. Thus the buck and LDO
> naming is generic and not tied to any specific purposes. However there
> is following limitations which make it mostly suitable for use cases
> where the processor where PMIC driver is running is powered by the PMIC:
>
> - The PMIC is not re-initialized if it resets. PMIC may reset as a
> result of voltage monitoring (over/under voltage) or due to reset
> request. Driver is only initializing PMIC at probe. This is not
> problem as long as processor controlling PMIC is powered by PMIC.
>
> - The PMIC internal state machine is ignored by driver. Driver assumes
> the PMIC is wired so that it is always in "run" state when controlled
> by the driver.
>
> 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
This chip has 8 bucks. 4 of then support DVS (dynamic voltage scaling).
Other 4 can be used on PWM or PFM switching mode. When PWM is used
voltages can be changed without disabling regulator. On PFM this should
not be done. These latter 4 regulators can be forced to PWM mode via
control bit in register. This driver does not support controlling this
mode though. So this driver version just checks if regulator is enabled
before changing the voltage and if it is the voltage change fails with
-EBUSY
My question is whether it would be good idea to also read the 'force
PWM' bit when voltage is changed and allow the change if PWM mode is
forced to be used? Problem is that the check and voltage change can't be
atomic so there is a chance someone changes the mode (bypassing the
driver and regulator core) after this check but before we get to modify
the voltage. Furthermore, I doubt the 'force PWM' is widely used (but I
can't say for sure as I can't imagine all use cases) as it is not so
power efficient.
But as I am really inexperienced on this area I would like to get
opinions from people who know this field of industry better...
Br,
Matti Vaittinen
On Wed, May 30, 2018 at 12:05:12PM +0300, Matti Vaittinen wrote:
> Other 4 can be used on PWM or PFM switching mode. When PWM is used
> voltages can be changed without disabling regulator. On PFM this should
> not be done. These latter 4 regulators can be forced to PWM mode via
> control bit in register. This driver does not support controlling this
> mode though. So this driver version just checks if regulator is enabled
> before changing the voltage and if it is the voltage change fails with
> -EBUSY
It probably should support setting modes (especially if the device isn't
smart enough to automatically shift which sounds like the case) but
that's a separate thing.
> My question is whether it would be good idea to also read the 'force
> PWM' bit when voltage is changed and allow the change if PWM mode is
> forced to be used? Problem is that the check and voltage change can't be
> atomic so there is a chance someone changes the mode (bypassing the
> driver and regulator core) after this check but before we get to modify
> the voltage. Furthermore, I doubt the 'force PWM' is widely used (but I
> can't say for sure as I can't imagine all use cases) as it is not so
> power efficient.
Why would anything else be changing the mode configuration of the
regulator while the system is running? That sounds like a bad idea.
In any case if the hardware really needs to be manually put into force
PWM to change voltage then the simplest thing would be to just move it
into force PWM mode, do the change, then change back if it wasn't
already in force PWM mode.
The tradeoff with forced PWM mode is that the quality of regulation will
be a lot better, especially if the load changes suddenly (as things like
CPUs often do). Most hardware that's at all current is able respond to
changes in load and switch modes automatically when it's appropriate,
except possibly in some very low power modes.
The patch
regulator: bd71837: BD71837 PMIC regulator driver
has been applied to the regulator tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
From ba08799e90b5935a3df20766a73b5841046f6832 Mon Sep 17 00:00:00 2001
From: Matti Vaittinen <[email protected]>
Date: Wed, 30 May 2018 11:43:43 +0300
Subject: [PATCH] regulator: bd71837: BD71837 PMIC regulator driver
Support for controlling the 8 bucks and 7 LDOs the PMIC contains.
Signed-off-by: Matti Vaittinen <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
drivers/regulator/Kconfig | 11 +
drivers/regulator/Makefile | 1 +
drivers/regulator/bd71837-regulator.c | 640 ++++++++++++++++++++++++++
3 files changed, 652 insertions(+)
create mode 100644 drivers/regulator/bd71837-regulator.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 4efae3b7e746..5dbccf5f3037 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -180,6 +180,17 @@ config REGULATOR_BCM590XX
BCM590xx PMUs. This will enable support for the software
controllable LDO/Switching regulators.
+config REGULATOR_BD71837
+ tristate "ROHM BD71837 Power Regulator"
+ depends on MFD_BD71837
+ help
+ This driver supports voltage regulators on ROHM BD71837 PMIC.
+ This will enable support for the software controllable buck
+ and LDO regulators.
+
+ This driver can also be built as a module. If so, the module
+ will be called bd71837-regulator.
+
config REGULATOR_BD9571MWV
tristate "ROHM BD9571MWV Regulators"
depends on MFD_BD9571MWV
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index d81fb02bd6e9..bd818ceb7c72 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_REGULATOR_AS3711) += as3711-regulator.o
obj-$(CONFIG_REGULATOR_AS3722) += as3722-regulator.o
obj-$(CONFIG_REGULATOR_AXP20X) += axp20x-regulator.o
obj-$(CONFIG_REGULATOR_BCM590XX) += bcm590xx-regulator.o
+obj-$(CONFIG_REGULATOR_BD71837) += bd71837-regulator.o
obj-$(CONFIG_REGULATOR_BD9571MWV) += bd9571mwv-regulator.o
obj-$(CONFIG_REGULATOR_DA903X) += da903x.o
obj-$(CONFIG_REGULATOR_DA9052) += da9052-regulator.o
diff --git a/drivers/regulator/bd71837-regulator.c b/drivers/regulator/bd71837-regulator.c
new file mode 100644
index 000000000000..6eae4d0432a2
--- /dev/null
+++ b/drivers/regulator/bd71837-regulator.c
@@ -0,0 +1,640 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 ROHM Semiconductors
+// bd71837-regulator.c ROHM BD71837MWV regulator driver
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <linux/gpio.h>
+#include <linux/mfd/bd71837.h>
+#include <linux/regulator/of_regulator.h>
+
+struct bd71837_pmic {
+ struct regulator_desc descs[BD71837_REGULATOR_CNT];
+ struct bd71837 *mfd;
+ struct platform_device *pdev;
+ struct regulator_dev *rdev[BD71837_REGULATOR_CNT];
+};
+
+/*
+ * BUCK1/2/3/4
+ * BUCK1RAMPRATE[1:0] BUCK1 DVS ramp rate setting
+ * 00: 10.00mV/usec 10mV 1uS
+ * 01: 5.00mV/usec 10mV 2uS
+ * 10: 2.50mV/usec 10mV 4uS
+ * 11: 1.25mV/usec 10mV 8uS
+ */
+static int bd71837_buck1234_set_ramp_delay(struct regulator_dev *rdev,
+ int ramp_delay)
+{
+ struct bd71837_pmic *pmic = rdev_get_drvdata(rdev);
+ struct bd71837 *mfd = pmic->mfd;
+ int id = rdev->desc->id;
+ unsigned int ramp_value = BUCK_RAMPRATE_10P00MV;
+
+ dev_dbg(&(pmic->pdev->dev), "Buck[%d] Set Ramp = %d\n", id + 1,
+ ramp_delay);
+ switch (ramp_delay) {
+ case 1 ... 1250:
+ ramp_value = BUCK_RAMPRATE_1P25MV;
+ break;
+ case 1251 ... 2500:
+ ramp_value = BUCK_RAMPRATE_2P50MV;
+ break;
+ case 2501 ... 5000:
+ ramp_value = BUCK_RAMPRATE_5P00MV;
+ break;
+ case 5001 ... 10000:
+ ramp_value = BUCK_RAMPRATE_10P00MV;
+ break;
+ default:
+ ramp_value = BUCK_RAMPRATE_10P00MV;
+ dev_err(&pmic->pdev->dev,
+ "%s: ramp_delay: %d not supported, setting 10000mV//us\n",
+ rdev->desc->name, ramp_delay);
+ }
+
+ return regmap_update_bits(mfd->regmap, BD71837_REG_BUCK1_CTRL + id,
+ BUCK_RAMPRATE_MASK, ramp_value << 6);
+}
+
+/* Bucks 1 to 4 support DVS. PWM mode is used when voltage is changed.
+ * Bucks 5 to 8 and LDOs can use PFM and must be disabled when voltage
+ * is changed. Hence we return -EBUSY for these if voltage is changed
+ * when BUCK/LDO is enabled.
+ */
+static int bd71837_set_voltage_sel_restricted(struct regulator_dev *rdev,
+ unsigned int sel)
+{
+ int ret;
+
+ ret = regulator_is_enabled_regmap(rdev);
+ if (!ret)
+ ret = regulator_set_voltage_sel_regmap(rdev, sel);
+ else if (ret == 1)
+ ret = -EBUSY;
+ return ret;
+}
+
+static struct regulator_ops bd71837_ldo_regulator_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .list_voltage = regulator_list_voltage_linear_range,
+ .set_voltage_sel = bd71837_set_voltage_sel_restricted,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+};
+
+static struct regulator_ops bd71837_ldo_regulator_nolinear_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .list_voltage = regulator_list_voltage_table,
+ .set_voltage_sel = bd71837_set_voltage_sel_restricted,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+};
+
+static struct regulator_ops bd71837_buck_regulator_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .list_voltage = regulator_list_voltage_linear_range,
+ .set_voltage_sel = bd71837_set_voltage_sel_restricted,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+ .set_voltage_time_sel = regulator_set_voltage_time_sel,
+};
+
+static struct regulator_ops bd71837_buck_regulator_nolinear_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .list_voltage = regulator_list_voltage_table,
+ .set_voltage_sel = bd71837_set_voltage_sel_restricted,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+ .set_voltage_time_sel = regulator_set_voltage_time_sel,
+};
+
+static struct regulator_ops bd71837_buck1234_regulator_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .list_voltage = regulator_list_voltage_linear_range,
+ .set_voltage_sel = regulator_set_voltage_sel_regmap,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+ .set_voltage_time_sel = regulator_set_voltage_time_sel,
+ .set_ramp_delay = bd71837_buck1234_set_ramp_delay,
+};
+
+/*
+ * BUCK1/2/3/4
+ * 0.70 to 1.30V (10mV step)
+ */
+static const struct regulator_linear_range bd71837_buck1234_voltage_ranges[] = {
+ REGULATOR_LINEAR_RANGE(700000, 0x00, 0x3C, 10000),
+ REGULATOR_LINEAR_RANGE(1300000, 0x3D, 0x3F, 0),
+};
+
+/*
+ * BUCK5
+ * 0.9V to 1.35V ()
+ */
+static const struct regulator_linear_range bd71837_buck5_voltage_ranges[] = {
+ REGULATOR_LINEAR_RANGE(700000, 0x00, 0x03, 100000),
+ REGULATOR_LINEAR_RANGE(1050000, 0x04, 0x05, 50000),
+ REGULATOR_LINEAR_RANGE(1200000, 0x06, 0x07, 150000),
+};
+
+/*
+ * BUCK6
+ * 3.0V to 3.3V (step 100mV)
+ */
+static const struct regulator_linear_range bd71837_buck6_voltage_ranges[] = {
+ REGULATOR_LINEAR_RANGE(3000000, 0x00, 0x03, 100000),
+};
+
+/*
+ * BUCK7
+ * 000 = 1.605V
+ * 001 = 1.695V
+ * 010 = 1.755V
+ * 011 = 1.8V (Initial)
+ * 100 = 1.845V
+ * 101 = 1.905V
+ * 110 = 1.95V
+ * 111 = 1.995V
+ */
+static const unsigned int buck_7_volts[] = {
+ 1605000, 1695000, 1755000, 1800000, 1845000, 1905000, 1950000, 1995000
+};
+
+/*
+ * BUCK8
+ * 0.8V to 1.40V (step 10mV)
+ */
+static const struct regulator_linear_range bd71837_buck8_voltage_ranges[] = {
+ REGULATOR_LINEAR_RANGE(800000, 0x00, 0x3C, 10000),
+ REGULATOR_LINEAR_RANGE(1400000, 0x3D, 0x3F, 0),
+};
+
+/*
+ * LDO1
+ * 3.0 to 3.3V (100mV step)
+ */
+static const struct regulator_linear_range bd71837_ldo1_voltage_ranges[] = {
+ REGULATOR_LINEAR_RANGE(3000000, 0x00, 0x03, 100000),
+};
+
+/*
+ * LDO2
+ * 0.8 or 0.9V
+ */
+const unsigned int ldo_2_volts[] = {
+ 900000, 800000
+};
+
+/*
+ * LDO3
+ * 1.8 to 3.3V (100mV step)
+ */
+static const struct regulator_linear_range bd71837_ldo3_voltage_ranges[] = {
+ REGULATOR_LINEAR_RANGE(1800000, 0x00, 0x0F, 100000),
+};
+
+/*
+ * LDO4
+ * 0.9 to 1.8V (100mV step)
+ */
+static const struct regulator_linear_range bd71837_ldo4_voltage_ranges[] = {
+ REGULATOR_LINEAR_RANGE(900000, 0x00, 0x09, 100000),
+ REGULATOR_LINEAR_RANGE(1800000, 0x0A, 0x0F, 0),
+};
+
+/*
+ * LDO5
+ * 1.8 to 3.3V (100mV step)
+ */
+static const struct regulator_linear_range bd71837_ldo5_voltage_ranges[] = {
+ REGULATOR_LINEAR_RANGE(1800000, 0x00, 0x0F, 100000),
+};
+
+/*
+ * LDO6
+ * 0.9 to 1.8V (100mV step)
+ */
+static const struct regulator_linear_range bd71837_ldo6_voltage_ranges[] = {
+ REGULATOR_LINEAR_RANGE(900000, 0x00, 0x09, 100000),
+ REGULATOR_LINEAR_RANGE(1800000, 0x0A, 0x0F, 0),
+};
+
+/*
+ * LDO7
+ * 1.8 to 3.3V (100mV step)
+ */
+static const struct regulator_linear_range bd71837_ldo7_voltage_ranges[] = {
+ REGULATOR_LINEAR_RANGE(1800000, 0x00, 0x0F, 100000),
+};
+
+static const struct regulator_desc bd71837_regulators[] = {
+ {
+ .name = "buck1",
+ .of_match = of_match_ptr("BUCK1"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_BUCK1,
+ .ops = &bd71837_buck1234_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_BUCK1_VOLTAGE_NUM,
+ .linear_ranges = bd71837_buck1234_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_buck1234_voltage_ranges),
+ .vsel_reg = BD71837_REG_BUCK1_VOLT_RUN,
+ .vsel_mask = BUCK1_RUN_MASK,
+ .enable_reg = BD71837_REG_BUCK1_CTRL,
+ .enable_mask = BD71837_BUCK_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "buck2",
+ .of_match = of_match_ptr("BUCK2"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_BUCK2,
+ .ops = &bd71837_buck1234_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_BUCK2_VOLTAGE_NUM,
+ .linear_ranges = bd71837_buck1234_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_buck1234_voltage_ranges),
+ .vsel_reg = BD71837_REG_BUCK2_VOLT_RUN,
+ .vsel_mask = BUCK2_RUN_MASK,
+ .enable_reg = BD71837_REG_BUCK2_CTRL,
+ .enable_mask = BD71837_BUCK_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "buck3",
+ .of_match = of_match_ptr("BUCK3"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_BUCK3,
+ .ops = &bd71837_buck1234_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_BUCK3_VOLTAGE_NUM,
+ .linear_ranges = bd71837_buck1234_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_buck1234_voltage_ranges),
+ .vsel_reg = BD71837_REG_BUCK3_VOLT_RUN,
+ .vsel_mask = BUCK3_RUN_MASK,
+ .enable_reg = BD71837_REG_BUCK3_CTRL,
+ .enable_mask = BD71837_BUCK_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "buck4",
+ .of_match = of_match_ptr("BUCK4"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_BUCK4,
+ .ops = &bd71837_buck1234_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_BUCK4_VOLTAGE_NUM,
+ .linear_ranges = bd71837_buck1234_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_buck1234_voltage_ranges),
+ .vsel_reg = BD71837_REG_BUCK4_VOLT_RUN,
+ .vsel_mask = BUCK4_RUN_MASK,
+ .enable_reg = BD71837_REG_BUCK4_CTRL,
+ .enable_mask = BD71837_BUCK_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "buck5",
+ .of_match = of_match_ptr("BUCK5"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_BUCK5,
+ .ops = &bd71837_buck_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_BUCK5_VOLTAGE_NUM,
+ .linear_ranges = bd71837_buck5_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_buck5_voltage_ranges),
+ .vsel_reg = BD71837_REG_BUCK5_VOLT,
+ .vsel_mask = BUCK5_MASK,
+ .enable_reg = BD71837_REG_BUCK5_CTRL,
+ .enable_mask = BD71837_BUCK_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "buck6",
+ .of_match = of_match_ptr("BUCK6"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_BUCK6,
+ .ops = &bd71837_buck_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_BUCK6_VOLTAGE_NUM,
+ .linear_ranges = bd71837_buck6_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_buck6_voltage_ranges),
+ .vsel_reg = BD71837_REG_BUCK6_VOLT,
+ .vsel_mask = BUCK6_MASK,
+ .enable_reg = BD71837_REG_BUCK6_CTRL,
+ .enable_mask = BD71837_BUCK_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "buck7",
+ .of_match = of_match_ptr("BUCK7"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_BUCK7,
+ .ops = &bd71837_buck_regulator_nolinear_ops,
+ .type = REGULATOR_VOLTAGE,
+ .volt_table = &buck_7_volts[0],
+ .n_voltages = ARRAY_SIZE(buck_7_volts),
+ .vsel_reg = BD71837_REG_BUCK7_VOLT,
+ .vsel_mask = BUCK7_MASK,
+ .enable_reg = BD71837_REG_BUCK7_CTRL,
+ .enable_mask = BD71837_BUCK_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "buck8",
+ .of_match = of_match_ptr("BUCK8"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_BUCK8,
+ .ops = &bd71837_buck_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_BUCK8_VOLTAGE_NUM,
+ .linear_ranges = bd71837_buck8_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_buck8_voltage_ranges),
+ .vsel_reg = BD71837_REG_BUCK8_VOLT,
+ .vsel_mask = BUCK8_MASK,
+ .enable_reg = BD71837_REG_BUCK8_CTRL,
+ .enable_mask = BD71837_BUCK_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "ldo1",
+ .of_match = of_match_ptr("LDO1"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_LDO1,
+ .ops = &bd71837_ldo_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_LDO1_VOLTAGE_NUM,
+ .linear_ranges = bd71837_ldo1_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_ldo1_voltage_ranges),
+ .vsel_reg = BD71837_REG_LDO1_VOLT,
+ .vsel_mask = LDO1_MASK,
+ .enable_reg = BD71837_REG_LDO1_VOLT,
+ .enable_mask = BD71837_LDO_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "ldo2",
+ .of_match = of_match_ptr("LDO2"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_LDO2,
+ .ops = &bd71837_ldo_regulator_nolinear_ops,
+ .type = REGULATOR_VOLTAGE,
+ .volt_table = &ldo_2_volts[0],
+ .vsel_reg = BD71837_REG_LDO2_VOLT,
+ .vsel_mask = LDO2_MASK,
+ .n_voltages = ARRAY_SIZE(ldo_2_volts),
+ .n_voltages = BD71837_LDO2_VOLTAGE_NUM,
+ .enable_reg = BD71837_REG_LDO2_VOLT,
+ .enable_mask = BD71837_LDO_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "ldo3",
+ .of_match = of_match_ptr("LDO3"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_LDO3,
+ .ops = &bd71837_ldo_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_LDO3_VOLTAGE_NUM,
+ .linear_ranges = bd71837_ldo3_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_ldo3_voltage_ranges),
+ .vsel_reg = BD71837_REG_LDO3_VOLT,
+ .vsel_mask = LDO3_MASK,
+ .enable_reg = BD71837_REG_LDO3_VOLT,
+ .enable_mask = BD71837_LDO_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "ldo4",
+ .of_match = of_match_ptr("LDO4"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_LDO4,
+ .ops = &bd71837_ldo_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_LDO4_VOLTAGE_NUM,
+ .linear_ranges = bd71837_ldo4_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_ldo4_voltage_ranges),
+ .vsel_reg = BD71837_REG_LDO4_VOLT,
+ .vsel_mask = LDO4_MASK,
+ .enable_reg = BD71837_REG_LDO4_VOLT,
+ .enable_mask = BD71837_LDO_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "ldo5",
+ .of_match = of_match_ptr("LDO5"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_LDO5,
+ .ops = &bd71837_ldo_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_LDO5_VOLTAGE_NUM,
+ .linear_ranges = bd71837_ldo5_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_ldo5_voltage_ranges),
+ /* LDO5 is supplied by buck6 */
+ .supply_name = "buck6",
+ .vsel_reg = BD71837_REG_LDO5_VOLT,
+ .vsel_mask = LDO5_MASK,
+ .enable_reg = BD71837_REG_LDO5_VOLT,
+ .enable_mask = BD71837_LDO_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "ldo6",
+ .of_match = of_match_ptr("LDO6"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_LDO6,
+ .ops = &bd71837_ldo_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_LDO6_VOLTAGE_NUM,
+ .linear_ranges = bd71837_ldo6_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_ldo6_voltage_ranges),
+ /* LDO6 is supplied by buck7 */
+ .supply_name = "buck7",
+ .vsel_reg = BD71837_REG_LDO6_VOLT,
+ .vsel_mask = LDO6_MASK,
+ .enable_reg = BD71837_REG_LDO6_VOLT,
+ .enable_mask = BD71837_LDO_EN,
+ .owner = THIS_MODULE,
+ },
+ {
+ .name = "ldo7",
+ .of_match = of_match_ptr("LDO7"),
+ .regulators_node = of_match_ptr("regulators"),
+ .id = BD71837_LDO7,
+ .ops = &bd71837_ldo_regulator_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = BD71837_LDO7_VOLTAGE_NUM,
+ .linear_ranges = bd71837_ldo7_voltage_ranges,
+ .n_linear_ranges = ARRAY_SIZE(bd71837_ldo7_voltage_ranges),
+ .vsel_reg = BD71837_REG_LDO7_VOLT,
+ .vsel_mask = LDO7_MASK,
+ .enable_reg = BD71837_REG_LDO7_VOLT,
+ .enable_mask = BD71837_LDO_EN,
+ .owner = THIS_MODULE,
+ },
+};
+
+struct reg_init {
+ unsigned int reg;
+ unsigned int mask;
+};
+
+static int bd71837_probe(struct platform_device *pdev)
+{
+ struct bd71837_pmic *pmic;
+ struct bd71837_board *pdata;
+ struct regulator_config config = { 0 };
+ struct reg_init pmic_regulator_inits[] = {
+ {
+ .reg = BD71837_REG_BUCK1_CTRL,
+ .mask = BD71837_BUCK_SEL,
+ }, {
+ .reg = BD71837_REG_BUCK2_CTRL,
+ .mask = BD71837_BUCK_SEL,
+ }, {
+ .reg = BD71837_REG_BUCK3_CTRL,
+ .mask = BD71837_BUCK_SEL,
+ }, {
+ .reg = BD71837_REG_BUCK4_CTRL,
+ .mask = BD71837_BUCK_SEL,
+ }, {
+ .reg = BD71837_REG_BUCK5_CTRL,
+ .mask = BD71837_BUCK_SEL,
+ }, {
+ .reg = BD71837_REG_BUCK6_CTRL,
+ .mask = BD71837_BUCK_SEL,
+ }, {
+ .reg = BD71837_REG_BUCK7_CTRL,
+ .mask = BD71837_BUCK_SEL,
+ }, {
+ .reg = BD71837_REG_BUCK8_CTRL,
+ .mask = BD71837_BUCK_SEL,
+ }, {
+ .reg = BD71837_REG_LDO1_VOLT,
+ .mask = BD71837_LDO_SEL,
+ }, {
+ .reg = BD71837_REG_LDO2_VOLT,
+ .mask = BD71837_LDO_SEL,
+ }, {
+ .reg = BD71837_REG_LDO3_VOLT,
+ .mask = BD71837_LDO_SEL,
+ }, {
+ .reg = BD71837_REG_LDO4_VOLT,
+ .mask = BD71837_LDO_SEL,
+ }, {
+ .reg = BD71837_REG_LDO5_VOLT,
+ .mask = BD71837_LDO_SEL,
+ }, {
+ .reg = BD71837_REG_LDO6_VOLT,
+ .mask = BD71837_LDO_SEL,
+ }, {
+ .reg = BD71837_REG_LDO7_VOLT,
+ .mask = BD71837_LDO_SEL,
+ }
+ };
+
+ int i, err;
+
+ pmic = devm_kzalloc(&pdev->dev, sizeof(struct bd71837_pmic),
+ GFP_KERNEL);
+ if (!pmic)
+ return -ENOMEM;
+
+ memcpy(pmic->descs, bd71837_regulators, sizeof(pmic->descs));
+
+ pmic->pdev = pdev;
+ pmic->mfd = dev_get_drvdata(pdev->dev.parent);
+
+ if (!pmic->mfd) {
+ dev_err(&pdev->dev, "No MFD driver data\n");
+ err = -EINVAL;
+ goto err;
+ }
+ platform_set_drvdata(pdev, pmic);
+ pdata = dev_get_platdata(pmic->mfd->dev);
+
+ /* Register LOCK release */
+ err = regmap_update_bits(pmic->mfd->regmap, BD71837_REG_REGLOCK,
+ (REGLOCK_PWRSEQ | REGLOCK_VREG), 0);
+ if (err) {
+ dev_err(&pmic->pdev->dev, "Failed to unlock PMIC (%d)\n", err);
+ goto err;
+ } else {
+ dev_dbg(&pmic->pdev->dev, "%s: Unlocked lock register 0x%x\n",
+ __func__, BD71837_REG_REGLOCK);
+ }
+
+ for (i = 0; i < ARRAY_SIZE(pmic_regulator_inits); i++) {
+
+ struct regulator_desc *desc;
+ struct regulator_dev *rdev;
+
+ desc = &pmic->descs[i];
+
+ if (pdata)
+ config.init_data = pdata->init_data[i];
+
+ config.dev = pdev->dev.parent;
+ config.driver_data = pmic;
+ config.regmap = pmic->mfd->regmap;
+
+ rdev = devm_regulator_register(&pdev->dev, desc, &config);
+ if (IS_ERR(rdev)) {
+ dev_err(pmic->mfd->dev,
+ "failed to register %s regulator\n",
+ desc->name);
+ err = PTR_ERR(rdev);
+ goto err;
+ }
+ /* Regulator register gets the regulator constraints and
+ * applies them (set_machine_constraints). This should have
+ * turned the control register(s) to correct values and we
+ * can now switch the control from PMIC state machine to the
+ * register interface
+ */
+ err = regmap_update_bits(pmic->mfd->regmap,
+ pmic_regulator_inits[i].reg,
+ pmic_regulator_inits[i].mask,
+ 0xFFFFFFFF);
+ if (err) {
+ dev_err(&pmic->pdev->dev,
+ "Failed to write BUCK/LDO SEL bit for (%s)\n",
+ desc->name);
+ goto err;
+ }
+
+ pmic->rdev[i] = rdev;
+ }
+
+ return 0;
+
+err:
+ return err;
+}
+
+static struct platform_driver bd71837_regulator = {
+ .driver = {
+ .name = "bd71837-pmic",
+ .owner = THIS_MODULE,
+ },
+ .probe = bd71837_probe,
+};
+
+module_platform_driver(bd71837_regulator);
+
+MODULE_AUTHOR("Matti Vaittinen <[email protected]>");
+MODULE_DESCRIPTION("BD71837 voltage regulator driver");
+MODULE_LICENSE("GPL");
--
2.17.0
Hello All,
On Wed, May 30, 2018 at 12:02:56PM +0100, Mark Brown wrote:
> The patch
>
> regulator: bd71837: BD71837 PMIC regulator driver
>
> has been applied to the regulator tree at
>
> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git
Does this mean this single patch was applied? I am sorry if I did not
follow correct policy/way of informing the dependencies - but there is a
dependency. The patch 1/6 contains the header file
include/linux/mfd/bd71837.h with bunch of definitions this patch is
requiring.
So If Mark can apply all of these patches (or at least the MFD parts
which is patch 1/6) to his tree - then we are Ok. But if the MFD needs
to go via Lee's tree - then I need some process guidance in order to
understand how this goes and what is expected from me =)
Br,
Matti Vaittinen
>
> All being well this means that it will be integrated into the linux-next
> tree (usually sometime in the next 24 hours) and sent to Linus during
> the next merge window (or sooner if it is a bug fix), however if
> problems are discovered then the patch may be dropped or reverted.
>
> You may get further e-mails resulting from automated or manual testing
> and review of the tree, please engage with people reporting problems and
> send followup patches addressing any issues that are reported if needed.
>
> If any updates are required or you are submitting further changes they
> should be sent as incremental updates against current git, existing
> patches will not be replaced.
>
> Please add any relevant lists and maintainers to the CCs when replying
> to this mail.
>
> Thanks,
> Mark
>
> From ba08799e90b5935a3df20766a73b5841046f6832 Mon Sep 17 00:00:00 2001
> From: Matti Vaittinen <[email protected]>
> Date: Wed, 30 May 2018 11:43:43 +0300
> Subject: [PATCH] regulator: bd71837: BD71837 PMIC regulator driver
>
> Support for controlling the 8 bucks and 7 LDOs the PMIC contains.
>
> Signed-off-by: Matti Vaittinen <[email protected]>
> Signed-off-by: Mark Brown <[email protected]>
> ---
> drivers/regulator/Kconfig | 11 +
> drivers/regulator/Makefile | 1 +
> drivers/regulator/bd71837-regulator.c | 640 ++++++++++++++++++++++++++
> 3 files changed, 652 insertions(+)
> create mode 100644 drivers/regulator/bd71837-regulator.c
>
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 4efae3b7e746..5dbccf5f3037 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -180,6 +180,17 @@ config REGULATOR_BCM590XX
> BCM590xx PMUs. This will enable support for the software
> controllable LDO/Switching regulators.
>
> +config REGULATOR_BD71837
> + tristate "ROHM BD71837 Power Regulator"
> + depends on MFD_BD71837
> + help
> + This driver supports voltage regulators on ROHM BD71837 PMIC.
> + This will enable support for the software controllable buck
> + and LDO regulators.
> +
> + This driver can also be built as a module. If so, the module
> + will be called bd71837-regulator.
> +
> config REGULATOR_BD9571MWV
> tristate "ROHM BD9571MWV Regulators"
> depends on MFD_BD9571MWV
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index d81fb02bd6e9..bd818ceb7c72 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_REGULATOR_AS3711) += as3711-regulator.o
> obj-$(CONFIG_REGULATOR_AS3722) += as3722-regulator.o
> obj-$(CONFIG_REGULATOR_AXP20X) += axp20x-regulator.o
> obj-$(CONFIG_REGULATOR_BCM590XX) += bcm590xx-regulator.o
> +obj-$(CONFIG_REGULATOR_BD71837) += bd71837-regulator.o
> obj-$(CONFIG_REGULATOR_BD9571MWV) += bd9571mwv-regulator.o
> obj-$(CONFIG_REGULATOR_DA903X) += da903x.o
> obj-$(CONFIG_REGULATOR_DA9052) += da9052-regulator.o
> diff --git a/drivers/regulator/bd71837-regulator.c b/drivers/regulator/bd71837-regulator.c
> new file mode 100644
> index 000000000000..6eae4d0432a2
> --- /dev/null
> +++ b/drivers/regulator/bd71837-regulator.c
> @@ -0,0 +1,640 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2018 ROHM Semiconductors
> +// bd71837-regulator.c ROHM BD71837MWV regulator driver
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +#include <linux/delay.h>
> +#include <linux/slab.h>
> +#include <linux/gpio.h>
> +#include <linux/mfd/bd71837.h>
> +#include <linux/regulator/of_regulator.h>
> +
> +struct bd71837_pmic {
> + struct regulator_desc descs[BD71837_REGULATOR_CNT];
> + struct bd71837 *mfd;
> + struct platform_device *pdev;
> + struct regulator_dev *rdev[BD71837_REGULATOR_CNT];
> +};
> +
> +/*
> + * BUCK1/2/3/4
> + * BUCK1RAMPRATE[1:0] BUCK1 DVS ramp rate setting
> + * 00: 10.00mV/usec 10mV 1uS
> + * 01: 5.00mV/usec 10mV 2uS
> + * 10: 2.50mV/usec 10mV 4uS
> + * 11: 1.25mV/usec 10mV 8uS
> + */
> +static int bd71837_buck1234_set_ramp_delay(struct regulator_dev *rdev,
> + int ramp_delay)
> +{
> + struct bd71837_pmic *pmic = rdev_get_drvdata(rdev);
> + struct bd71837 *mfd = pmic->mfd;
> + int id = rdev->desc->id;
> + unsigned int ramp_value = BUCK_RAMPRATE_10P00MV;
> +
> + dev_dbg(&(pmic->pdev->dev), "Buck[%d] Set Ramp = %d\n", id + 1,
> + ramp_delay);
> + switch (ramp_delay) {
> + case 1 ... 1250:
> + ramp_value = BUCK_RAMPRATE_1P25MV;
> + break;
> + case 1251 ... 2500:
> + ramp_value = BUCK_RAMPRATE_2P50MV;
> + break;
> + case 2501 ... 5000:
> + ramp_value = BUCK_RAMPRATE_5P00MV;
> + break;
> + case 5001 ... 10000:
> + ramp_value = BUCK_RAMPRATE_10P00MV;
> + break;
> + default:
> + ramp_value = BUCK_RAMPRATE_10P00MV;
> + dev_err(&pmic->pdev->dev,
> + "%s: ramp_delay: %d not supported, setting 10000mV//us\n",
> + rdev->desc->name, ramp_delay);
> + }
> +
> + return regmap_update_bits(mfd->regmap, BD71837_REG_BUCK1_CTRL + id,
> + BUCK_RAMPRATE_MASK, ramp_value << 6);
> +}
> +
> +/* Bucks 1 to 4 support DVS. PWM mode is used when voltage is changed.
> + * Bucks 5 to 8 and LDOs can use PFM and must be disabled when voltage
> + * is changed. Hence we return -EBUSY for these if voltage is changed
> + * when BUCK/LDO is enabled.
> + */
> +static int bd71837_set_voltage_sel_restricted(struct regulator_dev *rdev,
> + unsigned int sel)
> +{
> + int ret;
> +
> + ret = regulator_is_enabled_regmap(rdev);
> + if (!ret)
> + ret = regulator_set_voltage_sel_regmap(rdev, sel);
> + else if (ret == 1)
> + ret = -EBUSY;
> + return ret;
> +}
> +
> +static struct regulator_ops bd71837_ldo_regulator_ops = {
> + .enable = regulator_enable_regmap,
> + .disable = regulator_disable_regmap,
> + .is_enabled = regulator_is_enabled_regmap,
> + .list_voltage = regulator_list_voltage_linear_range,
> + .set_voltage_sel = bd71837_set_voltage_sel_restricted,
> + .get_voltage_sel = regulator_get_voltage_sel_regmap,
> +};
> +
> +static struct regulator_ops bd71837_ldo_regulator_nolinear_ops = {
> + .enable = regulator_enable_regmap,
> + .disable = regulator_disable_regmap,
> + .is_enabled = regulator_is_enabled_regmap,
> + .list_voltage = regulator_list_voltage_table,
> + .set_voltage_sel = bd71837_set_voltage_sel_restricted,
> + .get_voltage_sel = regulator_get_voltage_sel_regmap,
> +};
> +
> +static struct regulator_ops bd71837_buck_regulator_ops = {
> + .enable = regulator_enable_regmap,
> + .disable = regulator_disable_regmap,
> + .is_enabled = regulator_is_enabled_regmap,
> + .list_voltage = regulator_list_voltage_linear_range,
> + .set_voltage_sel = bd71837_set_voltage_sel_restricted,
> + .get_voltage_sel = regulator_get_voltage_sel_regmap,
> + .set_voltage_time_sel = regulator_set_voltage_time_sel,
> +};
> +
> +static struct regulator_ops bd71837_buck_regulator_nolinear_ops = {
> + .enable = regulator_enable_regmap,
> + .disable = regulator_disable_regmap,
> + .is_enabled = regulator_is_enabled_regmap,
> + .list_voltage = regulator_list_voltage_table,
> + .set_voltage_sel = bd71837_set_voltage_sel_restricted,
> + .get_voltage_sel = regulator_get_voltage_sel_regmap,
> + .set_voltage_time_sel = regulator_set_voltage_time_sel,
> +};
> +
> +static struct regulator_ops bd71837_buck1234_regulator_ops = {
> + .enable = regulator_enable_regmap,
> + .disable = regulator_disable_regmap,
> + .is_enabled = regulator_is_enabled_regmap,
> + .list_voltage = regulator_list_voltage_linear_range,
> + .set_voltage_sel = regulator_set_voltage_sel_regmap,
> + .get_voltage_sel = regulator_get_voltage_sel_regmap,
> + .set_voltage_time_sel = regulator_set_voltage_time_sel,
> + .set_ramp_delay = bd71837_buck1234_set_ramp_delay,
> +};
> +
> +/*
> + * BUCK1/2/3/4
> + * 0.70 to 1.30V (10mV step)
> + */
> +static const struct regulator_linear_range bd71837_buck1234_voltage_ranges[] = {
> + REGULATOR_LINEAR_RANGE(700000, 0x00, 0x3C, 10000),
> + REGULATOR_LINEAR_RANGE(1300000, 0x3D, 0x3F, 0),
> +};
> +
> +/*
> + * BUCK5
> + * 0.9V to 1.35V ()
> + */
> +static const struct regulator_linear_range bd71837_buck5_voltage_ranges[] = {
> + REGULATOR_LINEAR_RANGE(700000, 0x00, 0x03, 100000),
> + REGULATOR_LINEAR_RANGE(1050000, 0x04, 0x05, 50000),
> + REGULATOR_LINEAR_RANGE(1200000, 0x06, 0x07, 150000),
> +};
> +
> +/*
> + * BUCK6
> + * 3.0V to 3.3V (step 100mV)
> + */
> +static const struct regulator_linear_range bd71837_buck6_voltage_ranges[] = {
> + REGULATOR_LINEAR_RANGE(3000000, 0x00, 0x03, 100000),
> +};
> +
> +/*
> + * BUCK7
> + * 000 = 1.605V
> + * 001 = 1.695V
> + * 010 = 1.755V
> + * 011 = 1.8V (Initial)
> + * 100 = 1.845V
> + * 101 = 1.905V
> + * 110 = 1.95V
> + * 111 = 1.995V
> + */
> +static const unsigned int buck_7_volts[] = {
> + 1605000, 1695000, 1755000, 1800000, 1845000, 1905000, 1950000, 1995000
> +};
> +
> +/*
> + * BUCK8
> + * 0.8V to 1.40V (step 10mV)
> + */
> +static const struct regulator_linear_range bd71837_buck8_voltage_ranges[] = {
> + REGULATOR_LINEAR_RANGE(800000, 0x00, 0x3C, 10000),
> + REGULATOR_LINEAR_RANGE(1400000, 0x3D, 0x3F, 0),
> +};
> +
> +/*
> + * LDO1
> + * 3.0 to 3.3V (100mV step)
> + */
> +static const struct regulator_linear_range bd71837_ldo1_voltage_ranges[] = {
> + REGULATOR_LINEAR_RANGE(3000000, 0x00, 0x03, 100000),
> +};
> +
> +/*
> + * LDO2
> + * 0.8 or 0.9V
> + */
> +const unsigned int ldo_2_volts[] = {
> + 900000, 800000
> +};
> +
> +/*
> + * LDO3
> + * 1.8 to 3.3V (100mV step)
> + */
> +static const struct regulator_linear_range bd71837_ldo3_voltage_ranges[] = {
> + REGULATOR_LINEAR_RANGE(1800000, 0x00, 0x0F, 100000),
> +};
> +
> +/*
> + * LDO4
> + * 0.9 to 1.8V (100mV step)
> + */
> +static const struct regulator_linear_range bd71837_ldo4_voltage_ranges[] = {
> + REGULATOR_LINEAR_RANGE(900000, 0x00, 0x09, 100000),
> + REGULATOR_LINEAR_RANGE(1800000, 0x0A, 0x0F, 0),
> +};
> +
> +/*
> + * LDO5
> + * 1.8 to 3.3V (100mV step)
> + */
> +static const struct regulator_linear_range bd71837_ldo5_voltage_ranges[] = {
> + REGULATOR_LINEAR_RANGE(1800000, 0x00, 0x0F, 100000),
> +};
> +
> +/*
> + * LDO6
> + * 0.9 to 1.8V (100mV step)
> + */
> +static const struct regulator_linear_range bd71837_ldo6_voltage_ranges[] = {
> + REGULATOR_LINEAR_RANGE(900000, 0x00, 0x09, 100000),
> + REGULATOR_LINEAR_RANGE(1800000, 0x0A, 0x0F, 0),
> +};
> +
> +/*
> + * LDO7
> + * 1.8 to 3.3V (100mV step)
> + */
> +static const struct regulator_linear_range bd71837_ldo7_voltage_ranges[] = {
> + REGULATOR_LINEAR_RANGE(1800000, 0x00, 0x0F, 100000),
> +};
> +
> +static const struct regulator_desc bd71837_regulators[] = {
> + {
> + .name = "buck1",
> + .of_match = of_match_ptr("BUCK1"),
> + .regulators_node = of_match_ptr("regulators"),
> + .id = BD71837_BUCK1,
> + .ops = &bd71837_buck1234_regulator_ops,
> + .type = REGULATOR_VOLTAGE,
> + .n_voltages = BD71837_BUCK1_VOLTAGE_NUM,
> + .linear_ranges = bd71837_buck1234_voltage_ranges,
> + .n_linear_ranges = ARRAY_SIZE(bd71837_buck1234_voltage_ranges),
> + .vsel_reg = BD71837_REG_BUCK1_VOLT_RUN,
> + .vsel_mask = BUCK1_RUN_MASK,
> + .enable_reg = BD71837_REG_BUCK1_CTRL,
> + .enable_mask = BD71837_BUCK_EN,
> + .owner = THIS_MODULE,
> + },
> + {
> + .name = "buck2",
> + .of_match = of_match_ptr("BUCK2"),
> + .regulators_node = of_match_ptr("regulators"),
> + .id = BD71837_BUCK2,
> + .ops = &bd71837_buck1234_regulator_ops,
> + .type = REGULATOR_VOLTAGE,
> + .n_voltages = BD71837_BUCK2_VOLTAGE_NUM,
> + .linear_ranges = bd71837_buck1234_voltage_ranges,
> + .n_linear_ranges = ARRAY_SIZE(bd71837_buck1234_voltage_ranges),
> + .vsel_reg = BD71837_REG_BUCK2_VOLT_RUN,
> + .vsel_mask = BUCK2_RUN_MASK,
> + .enable_reg = BD71837_REG_BUCK2_CTRL,
> + .enable_mask = BD71837_BUCK_EN,
> + .owner = THIS_MODULE,
> + },
> + {
> + .name = "buck3",
> + .of_match = of_match_ptr("BUCK3"),
> + .regulators_node = of_match_ptr("regulators"),
> + .id = BD71837_BUCK3,
> + .ops = &bd71837_buck1234_regulator_ops,
> + .type = REGULATOR_VOLTAGE,
> + .n_voltages = BD71837_BUCK3_VOLTAGE_NUM,
> + .linear_ranges = bd71837_buck1234_voltage_ranges,
> + .n_linear_ranges = ARRAY_SIZE(bd71837_buck1234_voltage_ranges),
> + .vsel_reg = BD71837_REG_BUCK3_VOLT_RUN,
> + .vsel_mask = BUCK3_RUN_MASK,
> + .enable_reg = BD71837_REG_BUCK3_CTRL,
> + .enable_mask = BD71837_BUCK_EN,
> + .owner = THIS_MODULE,
> + },
> + {
> + .name = "buck4",
> + .of_match = of_match_ptr("BUCK4"),
> + .regulators_node = of_match_ptr("regulators"),
> + .id = BD71837_BUCK4,
> + .ops = &bd71837_buck1234_regulator_ops,
> + .type = REGULATOR_VOLTAGE,
> + .n_voltages = BD71837_BUCK4_VOLTAGE_NUM,
> + .linear_ranges = bd71837_buck1234_voltage_ranges,
> + .n_linear_ranges = ARRAY_SIZE(bd71837_buck1234_voltage_ranges),
> + .vsel_reg = BD71837_REG_BUCK4_VOLT_RUN,
> + .vsel_mask = BUCK4_RUN_MASK,
> + .enable_reg = BD71837_REG_BUCK4_CTRL,
> + .enable_mask = BD71837_BUCK_EN,
> + .owner = THIS_MODULE,
> + },
> + {
> + .name = "buck5",
> + .of_match = of_match_ptr("BUCK5"),
> + .regulators_node = of_match_ptr("regulators"),
> + .id = BD71837_BUCK5,
> + .ops = &bd71837_buck_regulator_ops,
> + .type = REGULATOR_VOLTAGE,
> + .n_voltages = BD71837_BUCK5_VOLTAGE_NUM,
> + .linear_ranges = bd71837_buck5_voltage_ranges,
> + .n_linear_ranges = ARRAY_SIZE(bd71837_buck5_voltage_ranges),
> + .vsel_reg = BD71837_REG_BUCK5_VOLT,
> + .vsel_mask = BUCK5_MASK,
> + .enable_reg = BD71837_REG_BUCK5_CTRL,
> + .enable_mask = BD71837_BUCK_EN,
> + .owner = THIS_MODULE,
> + },
> + {
> + .name = "buck6",
> + .of_match = of_match_ptr("BUCK6"),
> + .regulators_node = of_match_ptr("regulators"),
> + .id = BD71837_BUCK6,
> + .ops = &bd71837_buck_regulator_ops,
> + .type = REGULATOR_VOLTAGE,
> + .n_voltages = BD71837_BUCK6_VOLTAGE_NUM,
> + .linear_ranges = bd71837_buck6_voltage_ranges,
> + .n_linear_ranges = ARRAY_SIZE(bd71837_buck6_voltage_ranges),
> + .vsel_reg = BD71837_REG_BUCK6_VOLT,
> + .vsel_mask = BUCK6_MASK,
> + .enable_reg = BD71837_REG_BUCK6_CTRL,
> + .enable_mask = BD71837_BUCK_EN,
> + .owner = THIS_MODULE,
> + },
> + {
> + .name = "buck7",
> + .of_match = of_match_ptr("BUCK7"),
> + .regulators_node = of_match_ptr("regulators"),
> + .id = BD71837_BUCK7,
> + .ops = &bd71837_buck_regulator_nolinear_ops,
> + .type = REGULATOR_VOLTAGE,
> + .volt_table = &buck_7_volts[0],
> + .n_voltages = ARRAY_SIZE(buck_7_volts),
> + .vsel_reg = BD71837_REG_BUCK7_VOLT,
> + .vsel_mask = BUCK7_MASK,
> + .enable_reg = BD71837_REG_BUCK7_CTRL,
> + .enable_mask = BD71837_BUCK_EN,
> + .owner = THIS_MODULE,
> + },
> + {
> + .name = "buck8",
> + .of_match = of_match_ptr("BUCK8"),
> + .regulators_node = of_match_ptr("regulators"),
> + .id = BD71837_BUCK8,
> + .ops = &bd71837_buck_regulator_ops,
> + .type = REGULATOR_VOLTAGE,
> + .n_voltages = BD71837_BUCK8_VOLTAGE_NUM,
> + .linear_ranges = bd71837_buck8_voltage_ranges,
> + .n_linear_ranges = ARRAY_SIZE(bd71837_buck8_voltage_ranges),
> + .vsel_reg = BD71837_REG_BUCK8_VOLT,
> + .vsel_mask = BUCK8_MASK,
> + .enable_reg = BD71837_REG_BUCK8_CTRL,
> + .enable_mask = BD71837_BUCK_EN,
> + .owner = THIS_MODULE,
> + },
> + {
> + .name = "ldo1",
> + .of_match = of_match_ptr("LDO1"),
> + .regulators_node = of_match_ptr("regulators"),
> + .id = BD71837_LDO1,
> + .ops = &bd71837_ldo_regulator_ops,
> + .type = REGULATOR_VOLTAGE,
> + .n_voltages = BD71837_LDO1_VOLTAGE_NUM,
> + .linear_ranges = bd71837_ldo1_voltage_ranges,
> + .n_linear_ranges = ARRAY_SIZE(bd71837_ldo1_voltage_ranges),
> + .vsel_reg = BD71837_REG_LDO1_VOLT,
> + .vsel_mask = LDO1_MASK,
> + .enable_reg = BD71837_REG_LDO1_VOLT,
> + .enable_mask = BD71837_LDO_EN,
> + .owner = THIS_MODULE,
> + },
> + {
> + .name = "ldo2",
> + .of_match = of_match_ptr("LDO2"),
> + .regulators_node = of_match_ptr("regulators"),
> + .id = BD71837_LDO2,
> + .ops = &bd71837_ldo_regulator_nolinear_ops,
> + .type = REGULATOR_VOLTAGE,
> + .volt_table = &ldo_2_volts[0],
> + .vsel_reg = BD71837_REG_LDO2_VOLT,
> + .vsel_mask = LDO2_MASK,
> + .n_voltages = ARRAY_SIZE(ldo_2_volts),
> + .n_voltages = BD71837_LDO2_VOLTAGE_NUM,
> + .enable_reg = BD71837_REG_LDO2_VOLT,
> + .enable_mask = BD71837_LDO_EN,
> + .owner = THIS_MODULE,
> + },
> + {
> + .name = "ldo3",
> + .of_match = of_match_ptr("LDO3"),
> + .regulators_node = of_match_ptr("regulators"),
> + .id = BD71837_LDO3,
> + .ops = &bd71837_ldo_regulator_ops,
> + .type = REGULATOR_VOLTAGE,
> + .n_voltages = BD71837_LDO3_VOLTAGE_NUM,
> + .linear_ranges = bd71837_ldo3_voltage_ranges,
> + .n_linear_ranges = ARRAY_SIZE(bd71837_ldo3_voltage_ranges),
> + .vsel_reg = BD71837_REG_LDO3_VOLT,
> + .vsel_mask = LDO3_MASK,
> + .enable_reg = BD71837_REG_LDO3_VOLT,
> + .enable_mask = BD71837_LDO_EN,
> + .owner = THIS_MODULE,
> + },
> + {
> + .name = "ldo4",
> + .of_match = of_match_ptr("LDO4"),
> + .regulators_node = of_match_ptr("regulators"),
> + .id = BD71837_LDO4,
> + .ops = &bd71837_ldo_regulator_ops,
> + .type = REGULATOR_VOLTAGE,
> + .n_voltages = BD71837_LDO4_VOLTAGE_NUM,
> + .linear_ranges = bd71837_ldo4_voltage_ranges,
> + .n_linear_ranges = ARRAY_SIZE(bd71837_ldo4_voltage_ranges),
> + .vsel_reg = BD71837_REG_LDO4_VOLT,
> + .vsel_mask = LDO4_MASK,
> + .enable_reg = BD71837_REG_LDO4_VOLT,
> + .enable_mask = BD71837_LDO_EN,
> + .owner = THIS_MODULE,
> + },
> + {
> + .name = "ldo5",
> + .of_match = of_match_ptr("LDO5"),
> + .regulators_node = of_match_ptr("regulators"),
> + .id = BD71837_LDO5,
> + .ops = &bd71837_ldo_regulator_ops,
> + .type = REGULATOR_VOLTAGE,
> + .n_voltages = BD71837_LDO5_VOLTAGE_NUM,
> + .linear_ranges = bd71837_ldo5_voltage_ranges,
> + .n_linear_ranges = ARRAY_SIZE(bd71837_ldo5_voltage_ranges),
> + /* LDO5 is supplied by buck6 */
> + .supply_name = "buck6",
> + .vsel_reg = BD71837_REG_LDO5_VOLT,
> + .vsel_mask = LDO5_MASK,
> + .enable_reg = BD71837_REG_LDO5_VOLT,
> + .enable_mask = BD71837_LDO_EN,
> + .owner = THIS_MODULE,
> + },
> + {
> + .name = "ldo6",
> + .of_match = of_match_ptr("LDO6"),
> + .regulators_node = of_match_ptr("regulators"),
> + .id = BD71837_LDO6,
> + .ops = &bd71837_ldo_regulator_ops,
> + .type = REGULATOR_VOLTAGE,
> + .n_voltages = BD71837_LDO6_VOLTAGE_NUM,
> + .linear_ranges = bd71837_ldo6_voltage_ranges,
> + .n_linear_ranges = ARRAY_SIZE(bd71837_ldo6_voltage_ranges),
> + /* LDO6 is supplied by buck7 */
> + .supply_name = "buck7",
> + .vsel_reg = BD71837_REG_LDO6_VOLT,
> + .vsel_mask = LDO6_MASK,
> + .enable_reg = BD71837_REG_LDO6_VOLT,
> + .enable_mask = BD71837_LDO_EN,
> + .owner = THIS_MODULE,
> + },
> + {
> + .name = "ldo7",
> + .of_match = of_match_ptr("LDO7"),
> + .regulators_node = of_match_ptr("regulators"),
> + .id = BD71837_LDO7,
> + .ops = &bd71837_ldo_regulator_ops,
> + .type = REGULATOR_VOLTAGE,
> + .n_voltages = BD71837_LDO7_VOLTAGE_NUM,
> + .linear_ranges = bd71837_ldo7_voltage_ranges,
> + .n_linear_ranges = ARRAY_SIZE(bd71837_ldo7_voltage_ranges),
> + .vsel_reg = BD71837_REG_LDO7_VOLT,
> + .vsel_mask = LDO7_MASK,
> + .enable_reg = BD71837_REG_LDO7_VOLT,
> + .enable_mask = BD71837_LDO_EN,
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +struct reg_init {
> + unsigned int reg;
> + unsigned int mask;
> +};
> +
> +static int bd71837_probe(struct platform_device *pdev)
> +{
> + struct bd71837_pmic *pmic;
> + struct bd71837_board *pdata;
> + struct regulator_config config = { 0 };
> + struct reg_init pmic_regulator_inits[] = {
> + {
> + .reg = BD71837_REG_BUCK1_CTRL,
> + .mask = BD71837_BUCK_SEL,
> + }, {
> + .reg = BD71837_REG_BUCK2_CTRL,
> + .mask = BD71837_BUCK_SEL,
> + }, {
> + .reg = BD71837_REG_BUCK3_CTRL,
> + .mask = BD71837_BUCK_SEL,
> + }, {
> + .reg = BD71837_REG_BUCK4_CTRL,
> + .mask = BD71837_BUCK_SEL,
> + }, {
> + .reg = BD71837_REG_BUCK5_CTRL,
> + .mask = BD71837_BUCK_SEL,
> + }, {
> + .reg = BD71837_REG_BUCK6_CTRL,
> + .mask = BD71837_BUCK_SEL,
> + }, {
> + .reg = BD71837_REG_BUCK7_CTRL,
> + .mask = BD71837_BUCK_SEL,
> + }, {
> + .reg = BD71837_REG_BUCK8_CTRL,
> + .mask = BD71837_BUCK_SEL,
> + }, {
> + .reg = BD71837_REG_LDO1_VOLT,
> + .mask = BD71837_LDO_SEL,
> + }, {
> + .reg = BD71837_REG_LDO2_VOLT,
> + .mask = BD71837_LDO_SEL,
> + }, {
> + .reg = BD71837_REG_LDO3_VOLT,
> + .mask = BD71837_LDO_SEL,
> + }, {
> + .reg = BD71837_REG_LDO4_VOLT,
> + .mask = BD71837_LDO_SEL,
> + }, {
> + .reg = BD71837_REG_LDO5_VOLT,
> + .mask = BD71837_LDO_SEL,
> + }, {
> + .reg = BD71837_REG_LDO6_VOLT,
> + .mask = BD71837_LDO_SEL,
> + }, {
> + .reg = BD71837_REG_LDO7_VOLT,
> + .mask = BD71837_LDO_SEL,
> + }
> + };
> +
> + int i, err;
> +
> + pmic = devm_kzalloc(&pdev->dev, sizeof(struct bd71837_pmic),
> + GFP_KERNEL);
> + if (!pmic)
> + return -ENOMEM;
> +
> + memcpy(pmic->descs, bd71837_regulators, sizeof(pmic->descs));
> +
> + pmic->pdev = pdev;
> + pmic->mfd = dev_get_drvdata(pdev->dev.parent);
> +
> + if (!pmic->mfd) {
> + dev_err(&pdev->dev, "No MFD driver data\n");
> + err = -EINVAL;
> + goto err;
> + }
> + platform_set_drvdata(pdev, pmic);
> + pdata = dev_get_platdata(pmic->mfd->dev);
> +
> + /* Register LOCK release */
> + err = regmap_update_bits(pmic->mfd->regmap, BD71837_REG_REGLOCK,
> + (REGLOCK_PWRSEQ | REGLOCK_VREG), 0);
> + if (err) {
> + dev_err(&pmic->pdev->dev, "Failed to unlock PMIC (%d)\n", err);
> + goto err;
> + } else {
> + dev_dbg(&pmic->pdev->dev, "%s: Unlocked lock register 0x%x\n",
> + __func__, BD71837_REG_REGLOCK);
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(pmic_regulator_inits); i++) {
> +
> + struct regulator_desc *desc;
> + struct regulator_dev *rdev;
> +
> + desc = &pmic->descs[i];
> +
> + if (pdata)
> + config.init_data = pdata->init_data[i];
> +
> + config.dev = pdev->dev.parent;
> + config.driver_data = pmic;
> + config.regmap = pmic->mfd->regmap;
> +
> + rdev = devm_regulator_register(&pdev->dev, desc, &config);
> + if (IS_ERR(rdev)) {
> + dev_err(pmic->mfd->dev,
> + "failed to register %s regulator\n",
> + desc->name);
> + err = PTR_ERR(rdev);
> + goto err;
> + }
> + /* Regulator register gets the regulator constraints and
> + * applies them (set_machine_constraints). This should have
> + * turned the control register(s) to correct values and we
> + * can now switch the control from PMIC state machine to the
> + * register interface
> + */
> + err = regmap_update_bits(pmic->mfd->regmap,
> + pmic_regulator_inits[i].reg,
> + pmic_regulator_inits[i].mask,
> + 0xFFFFFFFF);
> + if (err) {
> + dev_err(&pmic->pdev->dev,
> + "Failed to write BUCK/LDO SEL bit for (%s)\n",
> + desc->name);
> + goto err;
> + }
> +
> + pmic->rdev[i] = rdev;
> + }
> +
> + return 0;
> +
> +err:
> + return err;
> +}
> +
> +static struct platform_driver bd71837_regulator = {
> + .driver = {
> + .name = "bd71837-pmic",
> + .owner = THIS_MODULE,
> + },
> + .probe = bd71837_probe,
> +};
> +
> +module_platform_driver(bd71837_regulator);
> +
> +MODULE_AUTHOR("Matti Vaittinen <[email protected]>");
> +MODULE_DESCRIPTION("BD71837 voltage regulator driver");
> +MODULE_LICENSE("GPL");
> --
> 2.17.0
>
On Wed, May 30, 2018 at 02:14:25PM +0300, Matti Vaittinen wrote:
> Does this mean this single patch was applied? I am sorry if I did not
> follow correct policy/way of informing the dependencies - but there is a
> dependency. The patch 1/6 contains the header file
> include/linux/mfd/bd71837.h with bunch of definitions this patch is
> requiring.
Your driver won't actually get built until the MFD parts end up in the
same tree since the regulator driver depends on the MFD so there's no
problem, the code will just sit there and not cause any problems.
On Wed, May 30, 2018 at 12:00:00PM +0100, Mark Brown wrote:
> On Wed, May 30, 2018 at 12:05:12PM +0300, Matti Vaittinen wrote:
>
> > Other 4 can be used on PWM or PFM switching mode. When PWM is used
> > voltages can be changed without disabling regulator. On PFM this should
> > not be done. These latter 4 regulators can be forced to PWM mode via
> > control bit in register. This driver does not support controlling this
> > mode though. So this driver version just checks if regulator is enabled
> > before changing the voltage and if it is the voltage change fails with
> > -EBUSY
>
> It probably should support setting modes (especially if the device isn't
> smart enough to automatically shift which sounds like the case) but
> that's a separate thing.
Thanks for the guidance! The first 4 BUCKs (which support DVS) do change
the mode automatically. Latter 4 do not. I however didn't find example
on how to allow doing this mode change via regulator framework and thus
the driver is not allowing mode change. For me it would sound good to
allow setting the mode via device-tree property - and via in-kernel API
or sysfs - but as I said I didn't yet find a nice example on that. (But
I assume this device is not first one allowing mode change and thus
there should be an example and I probably should not invent own DT
properties or sysfs entries for this. Guess I just need to search harder).
>
> > My question is whether it would be good idea to also read the 'force
> > PWM' bit when voltage is changed and allow the change if PWM mode is
> > forced to be used? Problem is that the check and voltage change can't be
> > atomic so there is a chance someone changes the mode (bypassing the
> > driver and regulator core) after this check but before we get to modify
> > the voltage. Furthermore, I doubt the 'force PWM' is widely used (but I
> > can't say for sure as I can't imagine all use cases) as it is not so
> > power efficient.
>
> Why would anything else be changing the mode configuration of the
> regulator while the system is running? That sounds like a bad idea.
This was also my initial thought. Still with my lack of experience on this
area (and with my experience on people inventing strange solutions) I
felt unsure.
> In any case if the hardware really needs to be manually put into force
> PWM to change voltage then the simplest thing would be to just move it
> into force PWM mode, do the change, then change back if it wasn't
> already in force PWM mode.
I see 4 options for these last 4 bucks which can't switch to PWM
automatically when voltage is changed.
1. prohibit voltage change if regulator is enabled (safest? current
approach)
2. allow voltage change if regulator is enabled and force PWM is used
3. switch to 'force PWM' when changing voltage (as suggested by Mark)
4. disable regulator for duration of voltage change (discarded option)
So option 4 was discarded. Option 1 is the current approach. I will ask
from ROHM PMIC HW colleagues what they think of Mark's suggestion
(option 3) and implement it if HW behaves Ok. I guess option 2 is not
worth the hassle.
>
> The tradeoff with forced PWM mode is that the quality of regulation will
> be a lot better, especially if the load changes suddenly (as things like
> CPUs often do). Most hardware that's at all current is able respond to
> changes in load and switch modes automatically when it's appropriate,
> except possibly in some very low power modes.
Yes. The mode switching is automatic. But there is this control bit for
disabling automatic mode switching and forcing the PWM. Problem with
these 4 last bucks is just that if regulator is in PFM (and it may be
if not forced to PWM - due to this automatic switching) then the voltage
change is not behaving well.
Thanks for all the support Mark!
Br,
Matti Vaittinen
On Wed, May 30, 2018 at 12:17:14PM +0100, Mark Brown wrote:
> On Wed, May 30, 2018 at 02:14:25PM +0300, Matti Vaittinen wrote:
>
> > The patch 1/6 contains the header file
> > include/linux/mfd/bd71837.h with bunch of definitions this patch is
> > requiring.
>
> Your driver won't actually get built until the MFD parts end up in the
> same tree since the regulator driver depends on the MFD so there's no
> problem, the code will just sit there and not cause any problems.
Oh, right. Thanks! So MFD and clk portions should go through the other
trees then.
Br,
Matti Vaittinen
On Wed, May 30, 2018 at 03:58:28PM +0300, Matti Vaittinen wrote:
> Oh, right. Thanks! So MFD and clk portions should go through the other
> trees then.
That's what I'd expect, yes.
On Wed, May 30, 2018 at 03:56:34PM +0300, Matti Vaittinen wrote:
> On Wed, May 30, 2018 at 12:00:00PM +0100, Mark Brown wrote:
> > The tradeoff with forced PWM mode is that the quality of regulation will
> > be a lot better, especially if the load changes suddenly (as things like
> > CPUs often do). Most hardware that's at all current is able respond to
> > changes in load and switch modes automatically when it's appropriate,
> > except possibly in some very low power modes.
> Yes. The mode switching is automatic. But there is this control bit for
> disabling automatic mode switching and forcing the PWM. Problem with
> these 4 last bucks is just that if regulator is in PFM (and it may be
> if not forced to PWM - due to this automatic switching) then the voltage
> change is not behaving well.
That sounds like the mode switching just isn't very good and needs a bit
of help so forcing the mode is probably going to do the right thing.
On Wed, May 30, 2018 at 11:42:03AM +0300, Matti Vaittinen wrote:
> Document devicetree bindings for ROHM BD71837 PMIC MFD.
>
> Signed-off-by: Matti Vaittinen <[email protected]>
> ---
> .../devicetree/bindings/mfd/rohm,bd71837-pmic.txt | 52 ++++++++++++++++++++++
> 1 file changed, 52 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..bbc46d38b162
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
> @@ -0,0 +1,52 @@
> +* 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.
> +
> +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.
> + - interrupt-controller : Marks the device node as an interrupt controller.
What sub blocks have interrupts?
> + - #interrupt-cells : The number of cells to describe an IRQ, should be 1 or 2.
> + The first cell is the IRQ number.
> + The second cell if present is the flags, encoded as trigger
> + masks from ../interrupt-controller/interrupts.txt.
> + - regulators: : List of child nodes that specify the regulators
> + Please see ../regulator/rohm,bd71837-regulator.txt
> + - clock: : Please see ../clock/rohm,bd71837-clock.txt
> +
> +Example:
> +
> + pmic: bd71837@4b {
Node names should be generic ideally. So "pmic@4b"
> + compatible = "rohm,bd71837";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x4b>;
> + interrupt-parent = <&gpio1>;
> + interrupts = <29 GPIO_ACTIVE_LOW>;
> + interrupt-names = "irq";
> + #interrupt-cells = <1>;
> + interrupt-controller;
> +
> + regulators {
> + buck1: BUCK1 {
> + regulator-name = "buck1";
> + regulator-min-microvolt = <700000>;
> + regulator-max-microvolt = <1300000>;
> + regulator-boot-on;
> + regulator-ramp-delay = <1250>;
> + };
> + ...
> + };
> + clk: bd71837-32k-out {
clock-controller {
> + compatible = "rohm,bd71837-clk";
> + #clock-cells = <0>;
> + clock-frequency = <32768>;
Can this be anything else?
> + clock-output-names = "bd71837-32k-out";
> + };
> + };
> --
> 2.14.3
>
On Wed, May 30, 2018 at 11:42:32AM +0300, Matti Vaittinen wrote:
> Document devicetree bindings for ROHM BD71837 PMIC regulators.
>
> Signed-off-by: Matti Vaittinen <[email protected]>
> ---
> .../bindings/regulator/rohm,bd71837-regulator.txt | 126 +++++++++++++++++++++
> 1 file changed, 126 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt
>
> diff --git a/Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt b/Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt
> new file mode 100644
> index 000000000000..4edf3137d9f7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt
> @@ -0,0 +1,126 @@
> +ROHM BD71837 Power Management Integrated Circuit (PMIC) regulator bindings
> +
> +BD71837MWV is a programmable Power Management
> +IC (PMIC) 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.
Why duplicate this from the core binding?
Otherwise,
Reviewed-by: Rob Herring <[email protected]>
> +
> +Required properties:
> + - regulator-name: should be "buck1", ..., "buck8" and "ldo1", ..., "ldo7"
> +
> +List of regulators provided by this controller. BD71837 regulators node
> +should be sub node of the BD71837 MFD node. See BD71837 MFD bindings at
> +Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
> +Regulator nodes should be named to BUCK_<number> and LDO_<number>. The
> +definition for each of these nodes is defined using the standard
> +binding for regulators at
> +Documentation/devicetree/bindings/regulator/regulator.txt.
> +Note that if BD71837 starts at RUN state you probably want to use
> +regulator-boot-on at least for BUCK6 and BUCK7 so that those are not
> +disabled by driver at startup. LDO5 and LDO6 are supplied by those and
> +if they are disabled at startup the voltage monitoring for LDO5/LDO6 will
> +cause PMIC to reset.
> +
> +The valid names for regulator nodes are:
> +BUCK1, BUCK2, BUCK3, BUCK4, BUCK5, BUCK6, BUCK7, BUCK8
> +LDO1, LDO2, LDO3, LDO4, LDO5, LDO6, LDO7
> +
> +Optional properties:
> +- Any optional property defined in bindings/regulator/regulator.txt
> +
> +Example:
> +regulators {
> + buck1: BUCK1 {
> + regulator-name = "buck1";
> + regulator-min-microvolt = <700000>;
> + regulator-max-microvolt = <1300000>;
> + regulator-boot-on;
> + regulator-ramp-delay = <1250>;
> + };
> + buck2: BUCK2 {
> + regulator-name = "buck2";
> + regulator-min-microvolt = <700000>;
> + regulator-max-microvolt = <1300000>;
> + regulator-boot-on;
> + regulator-always-on;
> + regulator-ramp-delay = <1250>;
> + };
> + buck3: BUCK3 {
> + regulator-name = "buck3";
> + regulator-min-microvolt = <700000>;
> + regulator-max-microvolt = <1300000>;
> + regulator-boot-on;
> + };
> + buck4: BUCK4 {
> + regulator-name = "buck4";
> + regulator-min-microvolt = <700000>;
> + regulator-max-microvolt = <1300000>;
> + regulator-boot-on;
> + };
> + buck5: BUCK5 {
> + regulator-name = "buck5";
> + regulator-min-microvolt = <700000>;
> + regulator-max-microvolt = <1350000>;
> + regulator-boot-on;
> + };
> + buck6: BUCK6 {
> + regulator-name = "buck6";
> + regulator-min-microvolt = <3000000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-boot-on;
> + };
> + buck7: BUCK7 {
> + regulator-name = "buck7";
> + regulator-min-microvolt = <1605000>;
> + regulator-max-microvolt = <1995000>;
> + regulator-boot-on;
> + };
> + buck8: BUCK8 {
> + regulator-name = "buck8";
> + regulator-min-microvolt = <800000>;
> + regulator-max-microvolt = <1400000>;
> + };
> +
> + ldo1: LDO1 {
> + regulator-name = "ldo1";
> + regulator-min-microvolt = <3000000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-boot-on;
> + };
> + ldo2: LDO2 {
> + regulator-name = "ldo2";
> + regulator-min-microvolt = <900000>;
> + regulator-max-microvolt = <900000>;
> + regulator-boot-on;
> + };
> + ldo3: LDO3 {
> + regulator-name = "ldo3";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <3300000>;
> + };
> + ldo4: LDO4 {
> + regulator-name = "ldo4";
> + regulator-min-microvolt = <900000>;
> + regulator-max-microvolt = <1800000>;
> + };
> + ldo5: LDO5 {
> + regulator-name = "ldo5";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <3300000>;
> + };
> + ldo6: LDO6 {
> + regulator-name = "ldo6";
> + regulator-min-microvolt = <900000>;
> + regulator-max-microvolt = <1800000>;
> + };
> + ldo7_reg: LDO7 {
> + regulator-name = "ldo7";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <3300000>;
> + };
> +};
> +
> +
> --
> 2.14.3
>
On Wed, May 30, 2018 at 11:42:55AM +0300, Matti Vaittinen wrote:
> Document devicetree bindings for ROHM BD71837 PMIC clock output.
Comments from the core binding apply here too.
>
> Signed-off-by: Matti Vaittinen <[email protected]>
> ---
> .../bindings/clock/rohm,bd71837-clock.txt | 37 ++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt
>
> diff --git a/Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt b/Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt
> new file mode 100644
> index 000000000000..9759fd145781
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt
> @@ -0,0 +1,37 @@
> +ROHM BD71837 Power Management Integrated Circuit clock bindings
> +
> +BD71837 clock node should be sub node of the BD71837 MFD node.
> +See BD71837 MFD bindings at
> +Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
> +
> +Required properties:
> +- compatible : Should be "rohm,bd71837-clk"
> +- clock-frequency : Should be 32768
> +- #clock-cells : Should be 0
> +
> +Optional properties:
> +- clock-output-names : Should contain name for output clock.
> + Name "bd71837-32k-out" is used if this is not given.
> +
> +
> +Example:
> +
> +pmic: bd71837@4b {
> + compatible = "rohm,bd71837";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x4b>;
> + interrupt-parent = <&gpio1>;
> + interrupts = <29 GPIO_ACTIVE_LOW>;
> + interrupt-names = "irq";
> + #interrupt-cells = <1>;
> + interrupt-controller;
> +
> + /* Clock node */
> + clk: bd71837-32k-out {
> + compatible = "rohm,bd71837-clk";
> + #clock-cells = <0>;
> + clock-frequency = <32768>;
> + clock-output-names = "bd71837-32k-out";
> + };
> +};
> --
> 2.14.3
>
Hello Rob,
Thanks for the review!
On Wed, May 30, 2018 at 10:01:29PM -0500, Rob Herring wrote:
> On Wed, May 30, 2018 at 11:42:03AM +0300, Matti Vaittinen wrote:
> > Document devicetree bindings for ROHM BD71837 PMIC MFD.
> > + - interrupts : The interrupt line the device is connected to.
> > + - interrupt-controller : Marks the device node as an interrupt controller.
>
> What sub blocks have interrupts?
The PMIC can generate interrupts from events which cause it to reset.
Eg, irq from watchdog line change, power button pushes, reset request
via register interface etc. I don't know any generic handling for these
interrupts. In "normal" use-case this PMIC is powering the processor
where driver is running and I do not see reasonable handling because
power-reset is going to follow the irq.
This IRQ might be relevant if use for PMIC is such that it is not
powering the processor where the driver is runninng. Then the controlling
processor can get the notification that chips powered by PMIC are
resetting. But handling for this must be use-case specific, right? So in
short - none of the current sub-devices use the IRQs - they are there
for specific use-cases which some boards may implement. Any suggestions
how to document the available interrupts? (power button line, sw reset
etc). My current assumption has been that one who is interested in using
these irqs should really also see the data-sheet for IRQs. But I admit
that documenting available interrupts here would be helpful. I will just
cook up some explanation and send it as a patch if no suggestions on how
to document those.
Patches 3/6 and 6/6 from the series were already applied to Mark's tree.
So how should I send further patches? Should I still send the whole series
(including already applied patches 3/6 and 6/6) or only the ones I change?
> > +Example:
> > +
> > + pmic: bd71837@4b {
>
> Node names should be generic ideally. So "pmic@4b"
I'll change that.
> > + clk: bd71837-32k-out {
>
> clock-controller {
And I'll change that too.
>
> > + compatible = "rohm,bd71837-clk";
> > + #clock-cells = <0>;
> > + clock-frequency = <32768>;
>
> Can this be anything else?
Not so that I know. Frequency is fixed. Is there a problem with this?
Br,
Matti Vaittinen
On Wed, May 30, 2018 at 10:04:36PM -0500, Rob Herring wrote:
> On Wed, May 30, 2018 at 11:42:32AM +0300, Matti Vaittinen wrote:
> > Document devicetree bindings for ROHM BD71837 PMIC regulators.
> > +ROHM BD71837 Power Management Integrated Circuit (PMIC) regulator bindings
> > +
> > +BD71837MWV is a programmable Power Management
> > +IC (PMIC) 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.
>
> Why duplicate this from the core binding?
I can remove this. I just thought it is nice to see what this chip is
doing even without opening the MFD binding doc. Just same question as in
the other patch - how should I deliver the change? This was already
applied to Mark's tree - should I do new patch on top of the Mark's tree
- or do patch against tree which does not yet contain this change? If I
do it on top of Mark's tree then Mark should apply it, right? If I do
it against some other three, thene there will be merge conflict with
Mark, right?
>
> Otherwise,
>
> Reviewed-by: Rob Herring <[email protected]>
Thanks!
Br,
Matti Vaittinen
On Thu, May 31, 2018 at 10:17:17AM +0300, Matti Vaittinen wrote:
> Hello Rob,
>
> Thanks for the review!
>
> On Wed, May 30, 2018 at 10:01:29PM -0500, Rob Herring wrote:
> > On Wed, May 30, 2018 at 11:42:03AM +0300, Matti Vaittinen wrote:
> > > Document devicetree bindings for ROHM BD71837 PMIC MFD.
> > > + - interrupts : The interrupt line the device is connected to.
> > > + - interrupt-controller : Marks the device node as an interrupt controller.
> >
> > What sub blocks have interrupts?
>
> The PMIC can generate interrupts from events which cause it to reset.
> Eg, irq from watchdog line change, power button pushes, reset request
> via register interface etc. I don't know any generic handling for these
> interrupts. In "normal" use-case this PMIC is powering the processor
> where driver is running and I do not see reasonable handling because
> power-reset is going to follow the irq.
>
Oh, but when reading this I understand that the interrupt-controller
property should at least be optional.
Br,
Matti Vaittinen
On Thu, May 31, 2018 at 2:21 AM, Matti Vaittinen
<[email protected]> wrote:
> On Wed, May 30, 2018 at 10:04:36PM -0500, Rob Herring wrote:
>> On Wed, May 30, 2018 at 11:42:32AM +0300, Matti Vaittinen wrote:
>> > Document devicetree bindings for ROHM BD71837 PMIC regulators.
>> > +ROHM BD71837 Power Management Integrated Circuit (PMIC) regulator bindings
>> > +
>> > +BD71837MWV is a programmable Power Management
>> > +IC (PMIC) 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.
>>
>> Why duplicate this from the core binding?
>
> I can remove this. I just thought it is nice to see what this chip is
> doing even without opening the MFD binding doc. Just same question as in
> the other patch - how should I deliver the change? This was already
> applied to Mark's tree - should I do new patch on top of the Mark's tree
> - or do patch against tree which does not yet contain this change? If I
> do it on top of Mark's tree then Mark should apply it, right? If I do
> it against some other three, thene there will be merge conflict with
> Mark, right?
I thought he only applied the regulator driver part. If not, then
don't worry about it. (But the right answer would be to do an
incremental patch on top of his tree).
Rob
On Thu, May 31, 2018 at 5:23 AM, Matti Vaittinen
<[email protected]> wrote:
> On Thu, May 31, 2018 at 10:17:17AM +0300, Matti Vaittinen wrote:
>> Hello Rob,
>>
>> Thanks for the review!
>>
>> On Wed, May 30, 2018 at 10:01:29PM -0500, Rob Herring wrote:
>> > On Wed, May 30, 2018 at 11:42:03AM +0300, Matti Vaittinen wrote:
>> > > Document devicetree bindings for ROHM BD71837 PMIC MFD.
>> > > + - interrupts : The interrupt line the device is connected to.
>> > > + - interrupt-controller : Marks the device node as an interrupt controller.
>> >
>> > What sub blocks have interrupts?
>>
>> The PMIC can generate interrupts from events which cause it to reset.
>> Eg, irq from watchdog line change, power button pushes, reset request
>> via register interface etc. I don't know any generic handling for these
>> interrupts. In "normal" use-case this PMIC is powering the processor
>> where driver is running and I do not see reasonable handling because
>> power-reset is going to follow the irq.
>>
>
> Oh, but when reading this I understand that the interrupt-controller
> property should at least be optional.
I don't think it should. The h/w either has an interrupt controller or
it doesn't. My concern is you added it but nothing uses it which tells
me your binding is incomplete. I'd rather see complete bindings even
if you don't have drivers. For example, as-is, there's not really any
need for the clocks child node. You can just make the parent a clock
provider. But we need a complete picture of the h/w to make that
determination.
Rob
Quoting Rob Herring (2018-05-31 07:07:24)
> On Thu, May 31, 2018 at 5:23 AM, Matti Vaittinen
> <[email protected]> wrote:
> > On Thu, May 31, 2018 at 10:17:17AM +0300, Matti Vaittinen wrote:
> >> Hello Rob,
> >>
> >> Thanks for the review!
> >>
> >> On Wed, May 30, 2018 at 10:01:29PM -0500, Rob Herring wrote:
> >> > On Wed, May 30, 2018 at 11:42:03AM +0300, Matti Vaittinen wrote:
> >> > > Document devicetree bindings for ROHM BD71837 PMIC MFD.
> >> > > + - interrupts : The interrupt line the device is connected to.
> >> > > + - interrupt-controller : Marks the device node as an interrupt controller.
> >> >
> >> > What sub blocks have interrupts?
> >>
> >> The PMIC can generate interrupts from events which cause it to reset.
> >> Eg, irq from watchdog line change, power button pushes, reset request
> >> via register interface etc. I don't know any generic handling for these
> >> interrupts. In "normal" use-case this PMIC is powering the processor
> >> where driver is running and I do not see reasonable handling because
> >> power-reset is going to follow the irq.
> >>
> >
> > Oh, but when reading this I understand that the interrupt-controller
> > property should at least be optional.
>
> I don't think it should. The h/w either has an interrupt controller or
> it doesn't. My concern is you added it but nothing uses it which tells
> me your binding is incomplete. I'd rather see complete bindings even
> if you don't have drivers. For example, as-is, there's not really any
> need for the clocks child node. You can just make the parent a clock
> provider. But we need a complete picture of the h/w to make that
> determination.
>
I don't see a reason to have the clk subnode either.
Quoting Matti Vaittinen (2018-05-30 01:43:19)
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 41492e980ef4..4b045699bb5e 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -279,6 +279,15 @@ config COMMON_CLK_STM32H7
> ---help---
> Support for stm32h7 SoC family clocks
>
> +config COMMON_CLK_BD71837
> + tristate "Clock driver for ROHM BD71837 PMIC MFD"
> + depends on MFD_BD71837
> + depends on I2C=y
Why depend on I2C=y?
> + depends on OF
Why depend on OF?
> + help
> + This driver supports ROHM BD71837 PMIC clock.
> +
> +
> source "drivers/clk/bcm/Kconfig"
> source "drivers/clk/hisilicon/Kconfig"
> source "drivers/clk/imgtec/Kconfig"
> diff --git a/drivers/clk/clk-bd71837.c b/drivers/clk/clk-bd71837.c
> new file mode 100644
> index 000000000000..91456d1077ac
> --- /dev/null
> +++ b/drivers/clk/clk-bd71837.c
> @@ -0,0 +1,151 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2018 ROHM Semiconductors
> +// bd71837.c -- ROHM BD71837MWV clock driver
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/mfd/bd71837.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>
> +
> +static int bd71837_clk_enable(struct clk_hw *hw);
> +static void bd71837_clk_disable(struct clk_hw *hw);
> +static int bd71837_clk_is_enabled(struct clk_hw *hw);
> +
> +struct bd71837_clk {
> + struct clk_hw hw;
> + uint8_t reg;
> + uint8_t mask;
> + unsigned long rate;
> + struct platform_device *pdev;
> + struct bd71837 *mfd;
> +};
> +
> +static unsigned long bd71837_clk_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate);
> +
> +static const struct clk_ops bd71837_clk_ops = {
> + .recalc_rate = &bd71837_clk_recalc_rate,
> + .prepare = &bd71837_clk_enable,
> + .unprepare = &bd71837_clk_disable,
> + .is_prepared = &bd71837_clk_is_enabled,
> +};
Move this structure after the function definitions. And drop the forward
declared functions.
> +
> +static int bd71837_clk_set(struct clk_hw *hw, int status)
> +{
> + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> +
> + return bd71837_update_bits(c->mfd, c->reg, c->mask, status);
> +}
> +
> +static void bd71837_clk_disable(struct clk_hw *hw)
> +{
> + int rv;
> + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> +
> + rv = bd71837_clk_set(hw, 0);
> + if (rv)
> + dev_err(&c->pdev->dev, "Failed to disable 32K clk (%d)\n", rv);
Why is a disable error message more important than an enable error
message? Please drop this message and rely on callers to indicate if
enabling their clk didn't work for some reason.
> +}
> +
> +static int bd71837_clk_enable(struct clk_hw *hw)
> +{
> + return bd71837_clk_set(hw, 1);
> +}
> +
> +static int bd71837_clk_is_enabled(struct clk_hw *hw)
> +{
> + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> +
> + return c->mask & bd71837_reg_read(c->mfd, c->reg);
Please put this on two or more lines so we know what the type of
bd71837_reg_read() returns:
u32 reg = bd71837_reg_read(....)
return c->mask & reg;
> +}
> +
> +static unsigned long bd71837_clk_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> +
> + return c->rate;
Recalc rate should read the hardware instead of returning a cached rate
unless it can't actually read hardware.
> +}
> +
> +static int bd71837_clk_probe(struct platform_device *pdev)
> +{
> + struct bd71837_clk *c;
> + int rval = -ENOMEM;
> + struct bd71837 *mfd = dev_get_drvdata(pdev->dev.parent);
> + const char *errstr = "memory allocation for bd71837 data failed";
We don't need allocation error messages.
> + struct clk_init_data init = {
> + .name = "bd71837-32k-out",
> + .ops = &bd71837_clk_ops,
> + };
> +
> + c = kzalloc(sizeof(struct bd71837_clk), GFP_KERNEL);
Use sizeof(*c) instead so we don't have to worry about type mismatches.
> + if (!c)
> + goto err_out;
> +
> + c->reg = BD71837_REG_OUT32K;
> + c->mask = BD71837_OUT32K_EN;
> + c->rate = BD71837_CLK_RATE;
Is the plan to have more clks supported by this driver in the future?
Because right now it seems to make a structure up and then hardcode the
members for a single clk so I'm not sure why those registers aren't just
hardcoded in the clk_ops functions that use them.
> + c->mfd = mfd;
> + c->pdev = pdev;
> +
> + if (pdev->dev.of_node)
If there isn't an of_node it would be NULL, and then calling the
function below would cause it to not update the init name? Seems like it
could be called unconditionally.
> + of_property_read_string_index(pdev->dev.of_node,
> + "clock-output-names", 0,
> + &init.name);
> +
> + c->hw.init = &init;
> +
> + errstr = "failed to register 32K clk";
The 'errstr' thing is not standard. Please just call dev_err() directly
with the string you want to print. And consider not printing anything at
all.
> + rval = clk_hw_register(&pdev->dev, &c->hw);
> + if (rval)
> + goto err_free;
> +
> + errstr = "failed to register clkdev for bd71837";
> + rval = clk_hw_register_clkdev(&c->hw, init.name, NULL);
Are you using the clkdev lookup? Or this is just added for backup
purposes? If this is a mostly DT driver then please drop this part of
the patch and rely on of_clk_hw_add_provider() to handle the lookup
instead.
> + if (rval)
> + goto err_unregister;
> +
> + platform_set_drvdata(pdev, c);
> + dev_dbg(&pdev->dev, "bd71837_clk successfully probed\n");
There's a pr_debug() in really_probe() for this.
> +
> + return 0;
> +
> +err_unregister:
> + clk_hw_unregister(&c->hw);
> +err_free:
> + kfree(c);
> +err_out:
> + dev_err(&pdev->dev, "%s\n", errstr);
> + return rval;
> +}
> +
> +static int bd71837_clk_remove(struct platform_device *pdev)
> +{
> + struct bd71837_clk *c = platform_get_drvdata(pdev);
> +
> + if (c) {
> + clk_hw_unregister(&c->hw);
Use devm to register clks.
> + kfree(c);
and devm_kzalloc()
> + platform_set_drvdata(pdev, NULL);
This doesn't need to be done. Drop it.
> + }
> + return 0;
> +}
> +
On Thu, May 31, 2018 at 09:07:24AM -0500, Rob Herring wrote:
> On Thu, May 31, 2018 at 5:23 AM, Matti Vaittinen
> <[email protected]> wrote:
> > On Thu, May 31, 2018 at 10:17:17AM +0300, Matti Vaittinen wrote:
> >> On Wed, May 30, 2018 at 10:01:29PM -0500, Rob Herring wrote:
> >> > On Wed, May 30, 2018 at 11:42:03AM +0300, Matti Vaittinen wrote:
> >> > > Document devicetree bindings for ROHM BD71837 PMIC MFD.
> >> > > + - interrupts : The interrupt line the device is connected to.
> >> > > + - interrupt-controller : Marks the device node as an interrupt controller.
> >> >
> >> > What sub blocks have interrupts?
> >>
> >> The PMIC can generate interrupts from events which cause it to reset.
> >> Eg, irq from watchdog line change, power button pushes, reset request
> >> via register interface etc. I don't know any generic handling for these
> >> interrupts. In "normal" use-case this PMIC is powering the processor
> >> where driver is running and I do not see reasonable handling because
> >> power-reset is going to follow the irq.
> >>
> >
> > Oh, but when reading this I understand that the interrupt-controller
> > property should at least be optional.
>
> I don't think it should. The h/w either has an interrupt controller or
> it doesn't.
I hope this explains why I did this interrupt controller - please tell
me if this is legitimate use-case and what you think of following:
+Optional properties:
+ - interrupt-controller : Marks the device node as an interrupt controller.
+ BD71837MWV can report different power state change
+ events to other devices. Different events can be seen
+ as separate BD71837 domain interrupts.
+ - #interrupt-cells : The number of cells to describe an IRQ should be 1.
+ The first cell is the IRQ number.
+ masks from ../interrupt-controller/interrupts.txt.
> My concern is you added it but nothing uses it which tells
> me your binding is incomplete. I'd rather see complete bindings even
> if you don't have drivers.
So this makes me wonder if my use-case for interrupt controller is
valid. I thought making this PMIC as interrupt controller is a nice way
of hiding the irq register and i2c access from other potential drivers
using these interrupts. But as I don't know what could be the potential
user for these irqs, I don't know how to complete binding. This is why I
also thought of making this optional, so that the potential for using
the interrupts would be there but it was not required when interrupts
are not needed.
> For example, as-is, there's not really any
> need for the clocks child node. You can just make the parent a clock
> provider.
This sounds correct. I just lack of knowledge on how to handle clocks
in "standard way" using the clock framework and this was a result of
my first attempt. (Funny, I have written clk / synchronization drivers
for work in the past but still I have no idea on how to do this in
"standard way").
> But we need a complete picture of the h/w to make that
> determination.
My attempt is to create generic driver for this PMIC. I would rather not
limit it's use to any particular board/soc. The example binding is based
on my test environment where I simply connected this PMIC break out
board to beagle bone black. (I do also have a test board where i.MX8 and
peripherials are powered by this PMIC but I rather not limit this driver
to such single setup. Besides the linux running on that board is not
'standard')
The PMIC itself just has this 32.768 KHz clock output. Clock can be
enabled/disabled via register interface. I think this is intended to be
used for RTC but I thought this driver does not need to care about that.
I thought it is just a good idea to provide control via clk subsystem
and to not make assumptions on use-cases in this driver.
Best Regards
Matti Vaittinen
First of all - Thanks for looking at this!
On Thu, May 31, 2018 at 08:10:39AM -0700, Stephen Boyd wrote:
> Quoting Matti Vaittinen (2018-05-30 01:43:19)
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > index 41492e980ef4..4b045699bb5e 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -279,6 +279,15 @@ config COMMON_CLK_STM32H7
> > ---help---
> > Support for stm32h7 SoC family clocks
> >
> > +config COMMON_CLK_BD71837
> > + tristate "Clock driver for ROHM BD71837 PMIC MFD"
> > + depends on MFD_BD71837
> > + depends on I2C=y
>
> Why depend on I2C=y?
I added this because the PMIC is connected via i2c. But as you ask this
- and as you probably knew my answer - I guess this is not correct. So
I guess depends on MFD_BD71837 should be sufficient and the MFD portion
should hide the fact we sit on I2C? If this is the case - I will remove
this.
>
> > + depends on OF
>
> Why depend on OF?
You're right. This is not needed. I'll remove this.
>
> > + help
> > + This driver supports ROHM BD71837 PMIC clock.
> > +
> > +
> > source "drivers/clk/bcm/Kconfig"
> > source "drivers/clk/hisilicon/Kconfig"
> > source "drivers/clk/imgtec/Kconfig"
> > diff --git a/drivers/clk/clk-bd71837.c b/drivers/clk/clk-bd71837.c
> > new file mode 100644
> > index 000000000000..91456d1077ac
> > --- /dev/null
> > +++ b/drivers/clk/clk-bd71837.c
> > @@ -0,0 +1,151 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (C) 2018 ROHM Semiconductors
> > +// bd71837.c -- ROHM BD71837MWV clock driver
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/err.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/mfd/bd71837.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/clkdev.h>
> > +
> > +static int bd71837_clk_enable(struct clk_hw *hw);
> > +static void bd71837_clk_disable(struct clk_hw *hw);
> > +static int bd71837_clk_is_enabled(struct clk_hw *hw);
> > +
> > +struct bd71837_clk {
> > + struct clk_hw hw;
> > + uint8_t reg;
> > + uint8_t mask;
> > + unsigned long rate;
> > + struct platform_device *pdev;
> > + struct bd71837 *mfd;
> > +};
> > +
> > +static unsigned long bd71837_clk_recalc_rate(struct clk_hw *hw,
> > + unsigned long parent_rate);
> > +
> > +static const struct clk_ops bd71837_clk_ops = {
> > + .recalc_rate = &bd71837_clk_recalc_rate,
> > + .prepare = &bd71837_clk_enable,
> > + .unprepare = &bd71837_clk_disable,
> > + .is_prepared = &bd71837_clk_is_enabled,
> > +};
>
> Move this structure after the function definitions. And drop the forward
> declared functions.
Allright.
>
> > +
> > +static int bd71837_clk_set(struct clk_hw *hw, int status)
> > +{
> > + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> > +
> > + return bd71837_update_bits(c->mfd, c->reg, c->mask, status);
> > +}
> > +
> > +static void bd71837_clk_disable(struct clk_hw *hw)
> > +{
> > + int rv;
> > + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> > +
> > + rv = bd71837_clk_set(hw, 0);
> > + if (rv)
> > + dev_err(&c->pdev->dev, "Failed to disable 32K clk (%d)\n", rv);
>
> Why is a disable error message more important than an enable error
> message? Please drop this message and rely on callers to indicate if
> enabling their clk didn't work for some reason.
Reason is that the disable function is not returning error. So if I drop
the print there will be no indication of error, right?
>
> > +}
> > +
> > +static int bd71837_clk_enable(struct clk_hw *hw)
> > +{
> > + return bd71837_clk_set(hw, 1);
> > +}
> > +
> > +static int bd71837_clk_is_enabled(struct clk_hw *hw)
> > +{
> > + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> > +
> > + return c->mask & bd71837_reg_read(c->mfd, c->reg);
>
> Please put this on two or more lines so we know what the type of
> bd71837_reg_read() returns:
>
> u32 reg = bd71837_reg_read(....)
>
> return c->mask & reg;
>
Right. I'll do this.
>
> > +}
> > +
> > +static unsigned long bd71837_clk_recalc_rate(struct clk_hw *hw,
> > + unsigned long parent_rate)
> > +{
> > + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> > +
> > + return c->rate;
>
> Recalc rate should read the hardware instead of returning a cached rate
> unless it can't actually read hardware.
We can't read the rate from HW. And actually, as this is fixed rate
clock generator - is the recalc_rate needed at all?
>
> > +}
> > +
> > +static int bd71837_clk_probe(struct platform_device *pdev)
> > +{
> > + struct bd71837_clk *c;
> > + int rval = -ENOMEM;
> > + struct bd71837 *mfd = dev_get_drvdata(pdev->dev.parent);
> > + const char *errstr = "memory allocation for bd71837 data failed";
>
> We don't need allocation error messages.
Correct. I'll remove.
>
> > + struct clk_init_data init = {
> > + .name = "bd71837-32k-out",
> > + .ops = &bd71837_clk_ops,
> > + };
> > +
> > + c = kzalloc(sizeof(struct bd71837_clk), GFP_KERNEL);
>
> Use sizeof(*c) instead so we don't have to worry about type mismatches.
Ok.
>
> > + if (!c)
> > + goto err_out;
> > +
> > + c->reg = BD71837_REG_OUT32K;
> > + c->mask = BD71837_OUT32K_EN;
> > + c->rate = BD71837_CLK_RATE;
>
> Is the plan to have more clks supported by this driver in the future?
> Because right now it seems to make a structure up and then hardcode the
> members for a single clk so I'm not sure why those registers aren't just
> hardcoded in the clk_ops functions that use them.
(At least) one more chip from this series is coming and I am planning to
support it with this driver. I am not sure if the registers will stay
the same. Hence I rather not hardcode the register values.
>
> > + c->mfd = mfd;
> > + c->pdev = pdev;
> > +
> > + if (pdev->dev.of_node)
>
> If there isn't an of_node it would be NULL, and then calling the
> function below would cause it to not update the init name? Seems like it
> could be called unconditionally.
Right.
>
> > + of_property_read_string_index(pdev->dev.of_node,
> > + "clock-output-names", 0,
> > + &init.name);
> > +
> > + c->hw.init = &init;
> > +
> > + errstr = "failed to register 32K clk";
>
> The 'errstr' thing is not standard. Please just call dev_err() directly
> with the string you want to print. And consider not printing anything at
> all.
I think this technique is actually used in many places at net side. I
guess i have adobted this habit from some netlink message handling code.
I can change this to in-place prints if it fits environment better
though.
>
> > + rval = clk_hw_register(&pdev->dev, &c->hw);
> > + if (rval)
> > + goto err_free;
> > +
> > + errstr = "failed to register clkdev for bd71837";
> > + rval = clk_hw_register_clkdev(&c->hw, init.name, NULL);
>
> Are you using the clkdev lookup? Or this is just added for backup
> purposes? If this is a mostly DT driver then please drop this part of
> the patch and rely on of_clk_hw_add_provider() to handle the lookup
> instead.
I did this because this is how I get the clk_get to work in my test
driver. Problem is that I don't know how this clock is going to be used
- and I lack of knowledge how these things are usually done. I don't
know the clock consumer code so this was best I could think of. But if
this is not how things are usually handled I can remove the clkdev and
rely on of_clk_add_hw_provider. Would this be correct then:
>
> > + if (rval)
> > + goto err_unregister;
> > +
> > + platform_set_drvdata(pdev, c);
> > + dev_dbg(&pdev->dev, "bd71837_clk successfully probed\n");
>
> There's a pr_debug() in really_probe() for this.
Oh, thanks. I'll remove this debug.
>
> > +
> > + return 0;
> > +
> > +err_unregister:
> > + clk_hw_unregister(&c->hw);
> > +err_free:
> > + kfree(c);
> > +err_out:
> > + dev_err(&pdev->dev, "%s\n", errstr);
> > + return rval;
> > +}
> > +
> > +static int bd71837_clk_remove(struct platform_device *pdev)
> > +{
> > + struct bd71837_clk *c = platform_get_drvdata(pdev);
> > +
> > + if (c) {
> > + clk_hw_unregister(&c->hw);
>
> Use devm to register clks.
>
> > + kfree(c);
>
> and devm_kzalloc()
Right. Thanks - devm makes this simpler.
>
> > + platform_set_drvdata(pdev, NULL);
>
> This doesn't need to be done. Drop it.
Allright. Will drop.
>
> > + }
> > + return 0;
> > +}
> > +
Br,
Matti Vaittinen
On Thu, May 31, 2018 at 07:57:53AM -0700, Stephen Boyd wrote:
> Quoting Rob Herring (2018-05-31 07:07:24)
> > On Thu, May 31, 2018 at 5:23 AM, Matti Vaittinen
> > <[email protected]> wrote:
> > > On Thu, May 31, 2018 at 10:17:17AM +0300, Matti Vaittinen wrote:
> > >> Hello Rob,
> > >>
> > >> Thanks for the review!
> > >>
> > >> On Wed, May 30, 2018 at 10:01:29PM -0500, Rob Herring wrote:
> > >> > On Wed, May 30, 2018 at 11:42:03AM +0300, Matti Vaittinen wrote:
> > >> > > Document devicetree bindings for ROHM BD71837 PMIC MFD.
> > >> > > + - interrupts : The interrupt line the device is connected to.
> > >> > > + - interrupt-controller : Marks the device node as an interrupt controller.
> > >> >
> > >> > What sub blocks have interrupts?
> > >>
> > >> The PMIC can generate interrupts from events which cause it to reset.
> > >> Eg, irq from watchdog line change, power button pushes, reset request
> > >> via register interface etc. I don't know any generic handling for these
> > >> interrupts. In "normal" use-case this PMIC is powering the processor
> > >> where driver is running and I do not see reasonable handling because
> > >> power-reset is going to follow the irq.
> > >>
> > >
> > > Oh, but when reading this I understand that the interrupt-controller
> > > property should at least be optional.
> >
> > I don't think it should. The h/w either has an interrupt controller or
> > it doesn't. My concern is you added it but nothing uses it which tells
> > me your binding is incomplete. I'd rather see complete bindings even
> > if you don't have drivers. For example, as-is, there's not really any
> > need for the clocks child node. You can just make the parent a clock
> > provider. But we need a complete picture of the h/w to make that
> > determination.
> >
>
> I don't see a reason to have the clk subnode either.
After some pondering - do you mean I could:
1. remove clk binfing document and clk node.
2. add clock-output-names etc to pmic node (and describe them in pmic
node binding document)
3. use parent DT node in clk driver and do something like:
if (parent->of_node)
ret = of_clk_add_hw_provider(parent->of_node, of_clk_hw_simple_get,
hw);
4. remove the clkdev
I will cook new set of patches with all suggested changes but it may be I don't
get it ready for posting untill next week.
Br,
Matti Vaittinen
Quoting Matti Vaittinen (2018-06-01 00:31:56)
> First of all - Thanks for looking at this!
>
> On Thu, May 31, 2018 at 08:10:39AM -0700, Stephen Boyd wrote:
> > Quoting Matti Vaittinen (2018-05-30 01:43:19)
> > > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > > index 41492e980ef4..4b045699bb5e 100644
> > > --- a/drivers/clk/Kconfig
> > > +++ b/drivers/clk/Kconfig
> > > @@ -279,6 +279,15 @@ config COMMON_CLK_STM32H7
> > > ---help---
> > > Support for stm32h7 SoC family clocks
> > >
> > > +config COMMON_CLK_BD71837
> > > + tristate "Clock driver for ROHM BD71837 PMIC MFD"
> > > + depends on MFD_BD71837
> > > + depends on I2C=y
> >
> > Why depend on I2C=y?
>
> I added this because the PMIC is connected via i2c. But as you ask this
> - and as you probably knew my answer - I guess this is not correct. So
> I guess depends on MFD_BD71837 should be sufficient and the MFD portion
> should hide the fact we sit on I2C? If this is the case - I will remove
> this.
Sounds right.
> >
> > > +
> > > +static int bd71837_clk_set(struct clk_hw *hw, int status)
> > > +{
> > > + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> > > +
> > > + return bd71837_update_bits(c->mfd, c->reg, c->mask, status);
> > > +}
> > > +
> > > +static void bd71837_clk_disable(struct clk_hw *hw)
> > > +{
> > > + int rv;
> > > + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> > > +
> > > + rv = bd71837_clk_set(hw, 0);
> > > + if (rv)
> > > + dev_err(&c->pdev->dev, "Failed to disable 32K clk (%d)\n", rv);
> >
> > Why is a disable error message more important than an enable error
> > message? Please drop this message and rely on callers to indicate if
> > enabling their clk didn't work for some reason.
>
> Reason is that the disable function is not returning error. So if I drop
> the print there will be no indication of error, right?
I'm not sure anyone cares if disable fails, which is why we have it
marked as void. The end user probably can't do anything about it, and it
isn't actually causing some sort of problem besides lost power so if you
really want to keep it please move it to dev_dbg(), or just remove it
and add it in when you're debugging.
>
> >
> > > +}
> > > +
> > > +static unsigned long bd71837_clk_recalc_rate(struct clk_hw *hw,
> > > + unsigned long parent_rate)
> > > +{
> > > + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> > > +
> > > + return c->rate;
> >
> > Recalc rate should read the hardware instead of returning a cached rate
> > unless it can't actually read hardware.
>
> We can't read the rate from HW. And actually, as this is fixed rate
> clock generator - is the recalc_rate needed at all?
Yes recalc_rate is still needed in this case. Thanks for pointing it
out. This is fine.
>
> >
> > > + if (!c)
> > > + goto err_out;
> > > +
> > > + c->reg = BD71837_REG_OUT32K;
> > > + c->mask = BD71837_OUT32K_EN;
> > > + c->rate = BD71837_CLK_RATE;
> >
> > Is the plan to have more clks supported by this driver in the future?
> > Because right now it seems to make a structure up and then hardcode the
> > members for a single clk so I'm not sure why those registers aren't just
> > hardcoded in the clk_ops functions that use them.
>
> (At least) one more chip from this series is coming and I am planning to
> support it with this driver. I am not sure if the registers will stay
> the same. Hence I rather not hardcode the register values.
Ok.
>
> >
> > > + of_property_read_string_index(pdev->dev.of_node,
> > > + "clock-output-names", 0,
> > > + &init.name);
> > > +
> > > + c->hw.init = &init;
> > > +
> > > + errstr = "failed to register 32K clk";
> >
> > The 'errstr' thing is not standard. Please just call dev_err() directly
> > with the string you want to print. And consider not printing anything at
> > all.
>
> I think this technique is actually used in many places at net side. I
> guess i have adobted this habit from some netlink message handling code.
> I can change this to in-place prints if it fits environment better
> though.
Ok.
>
> >
> > > + rval = clk_hw_register(&pdev->dev, &c->hw);
> > > + if (rval)
> > > + goto err_free;
> > > +
> > > + errstr = "failed to register clkdev for bd71837";
> > > + rval = clk_hw_register_clkdev(&c->hw, init.name, NULL);
> >
> > Are you using the clkdev lookup? Or this is just added for backup
> > purposes? If this is a mostly DT driver then please drop this part of
> > the patch and rely on of_clk_hw_add_provider() to handle the lookup
> > instead.
>
> I did this because this is how I get the clk_get to work in my test
> driver. Problem is that I don't know how this clock is going to be used
> - and I lack of knowledge how these things are usually done. I don't
> know the clock consumer code so this was best I could think of. But if
> this is not how things are usually handled I can remove the clkdev and
> rely on of_clk_add_hw_provider. Would this be correct then:
There isn't any example code inserted, but yes, usually clkdev is used
when a non-DT enabled platform wants to use the clk. That typically only
happens on x86 platforms because we don't have a way in ACPI to express
clk handles. If this is never used with an x86 platform then clkdev is
not needed. Either way, adding an OF provider would be good to support
DT platforms. Having a clkdev lookup would also be good to support
non-DT platforms.
On Fri, Jun 1, 2018 at 1:25 AM, Matti Vaittinen
<[email protected]> wrote:
> On Thu, May 31, 2018 at 09:07:24AM -0500, Rob Herring wrote:
>> On Thu, May 31, 2018 at 5:23 AM, Matti Vaittinen
>> <[email protected]> wrote:
>> > On Thu, May 31, 2018 at 10:17:17AM +0300, Matti Vaittinen wrote:
>> >> On Wed, May 30, 2018 at 10:01:29PM -0500, Rob Herring wrote:
>> >> > On Wed, May 30, 2018 at 11:42:03AM +0300, Matti Vaittinen wrote:
>> >> > > Document devicetree bindings for ROHM BD71837 PMIC MFD.
>> >> > > + - interrupts : The interrupt line the device is connected to.
>> >> > > + - interrupt-controller : Marks the device node as an interrupt controller.
>> >> >
>> >> > What sub blocks have interrupts?
>> >>
>> >> The PMIC can generate interrupts from events which cause it to reset.
>> >> Eg, irq from watchdog line change, power button pushes, reset request
>> >> via register interface etc. I don't know any generic handling for these
>> >> interrupts. In "normal" use-case this PMIC is powering the processor
>> >> where driver is running and I do not see reasonable handling because
>> >> power-reset is going to follow the irq.
>> >>
>> >
>> > Oh, but when reading this I understand that the interrupt-controller
>> > property should at least be optional.
>>
>> I don't think it should. The h/w either has an interrupt controller or
>> it doesn't.
>
> I hope this explains why I did this interrupt controller - please tell
> me if this is legitimate use-case and what you think of following:
>
> +Optional properties:
> + - interrupt-controller : Marks the device node as an interrupt controller.
> + BD71837MWV can report different power state change
> + events to other devices. Different events can be seen
> + as separate BD71837 domain interrupts.
To what other devices?
> + - #interrupt-cells : The number of cells to describe an IRQ should be 1.
> + The first cell is the IRQ number.
> + masks from ../interrupt-controller/interrupts.txt.
I'm still not clear. Generally in a PMIC, you'd define an interrupt
controller when there's a common set of registers to manage sub-block
interrupts (typical mask/unmask, ack regs) and the subblocks
themselves have control of masking/unmasking interrupts. If there's
not a need to have these 2 levels of interrupt handling, then you
don't really need to define an interrupt controller.
>
>> My concern is you added it but nothing uses it which tells
>> me your binding is incomplete. I'd rather see complete bindings even
>> if you don't have drivers.
>
> So this makes me wonder if my use-case for interrupt controller is
> valid. I thought making this PMIC as interrupt controller is a nice way
> of hiding the irq register and i2c access from other potential drivers
> using these interrupts. But as I don't know what could be the potential
> user for these irqs, I don't know how to complete binding. This is why I
> also thought of making this optional, so that the potential for using
> the interrupts would be there but it was not required when interrupts
> are not needed.
The only drivers getting these interrupts would be drivers for this
PMIC. Interrupts are handled by the driver owning the h/w that
generated the interrupt. I think that is the disconnect here.
Take a power button. We don't create a generic power button interrupt
and then have some generic power button interrupt handler. We have a
handler for specifically for that device and then it generates a power
button press event.
>> For example, as-is, there's not really any
>> need for the clocks child node. You can just make the parent a clock
>> provider.
>
> This sounds correct. I just lack of knowledge on how to handle clocks
> in "standard way" using the clock framework and this was a result of
> my first attempt. (Funny, I have written clk / synchronization drivers
> for work in the past but still I have no idea on how to do this in
> "standard way").
>
>> But we need a complete picture of the h/w to make that
>> determination.
>
> My attempt is to create generic driver for this PMIC. I would rather not
> limit it's use to any particular board/soc. The example binding is based
> on my test environment where I simply connected this PMIC break out
> board to beagle bone black. (I do also have a test board where i.MX8 and
> peripherials are powered by this PMIC but I rather not limit this driver
> to such single setup. Besides the linux running on that board is not
> 'standard')
That's how we design all the PMIC drivers. All the PMIC functions
should be exposed thru standard class APIs. Though many PMICs are
pretty tightly coupled to particular SoCs either by design or just
there's not a lot of boards to create any sort of matrix of
combinations.
> The PMIC itself just has this 32.768 KHz clock output. Clock can be
> enabled/disabled via register interface. I think this is intended to be
> used for RTC but I thought this driver does not need to care about that.
> I thought it is just a good idea to provide control via clk subsystem
> and to not make assumptions on use-cases in this driver.
Sure that is fine. No one is saying you shouldn't do that.
Rob
Quoting Matti Vaittinen (2018-06-01 03:51:05)
> On Thu, May 31, 2018 at 07:57:53AM -0700, Stephen Boyd wrote:
> > Quoting Rob Herring (2018-05-31 07:07:24)
> > >
> > > I don't think it should. The h/w either has an interrupt controller or
> > > it doesn't. My concern is you added it but nothing uses it which tells
> > > me your binding is incomplete. I'd rather see complete bindings even
> > > if you don't have drivers. For example, as-is, there's not really any
> > > need for the clocks child node. You can just make the parent a clock
> > > provider. But we need a complete picture of the h/w to make that
> > > determination.
> > >
> >
> > I don't see a reason to have the clk subnode either.
>
> After some pondering - do you mean I could:
> 1. remove clk binfing document and clk node.
> 2. add clock-output-names etc to pmic node (and describe them in pmic
> node binding document)
> 3. use parent DT node in clk driver and do something like:
> if (parent->of_node)
> ret = of_clk_add_hw_provider(parent->of_node, of_clk_hw_simple_get,
> hw);
> 4. remove the clkdev
>
This sounds ok to me. As Rob said, a more complete picture of the
hardware would make this easier.
On Fri, Jun 01, 2018 at 12:32:16PM -0500, Rob Herring wrote:
> On Fri, Jun 1, 2018 at 1:25 AM, Matti Vaittinen
> <[email protected]> wrote:
> > On Thu, May 31, 2018 at 09:07:24AM -0500, Rob Herring wrote:
> >> On Thu, May 31, 2018 at 5:23 AM, Matti Vaittinen
> >> <[email protected]> wrote:
> >> > On Thu, May 31, 2018 at 10:17:17AM +0300, Matti Vaittinen wrote:
> >> >> On Wed, May 30, 2018 at 10:01:29PM -0500, Rob Herring wrote:
> >> >> > On Wed, May 30, 2018 at 11:42:03AM +0300, Matti Vaittinen wrote:
> >> >> > > Document devicetree bindings for ROHM BD71837 PMIC MFD.
> >> >> > > + - interrupts : The interrupt line the device is connected to.
> >> >> > > + - interrupt-controller : Marks the device node as an interrupt controller.
> >> >> >
> >> >> > What sub blocks have interrupts?
> >> >>
> >> >> The PMIC can generate interrupts from events which cause it to reset.
> >> >> Eg, irq from watchdog line change, power button pushes, reset request
> >> >> via register interface etc. I don't know any generic handling for these
> >> >> interrupts. In "normal" use-case this PMIC is powering the processor
> >> >> where driver is running and I do not see reasonable handling because
> >> >> power-reset is going to follow the irq.
> >> >>
> >> >
> >> > Oh, but when reading this I understand that the interrupt-controller
> >> > property should at least be optional.
> >>
> >> I don't think it should. The h/w either has an interrupt controller or
> >> it doesn't.
> >
> > I hope this explains why I did this interrupt controller - please tell
> > me if this is legitimate use-case and what you think of following:
> >
> > +Optional properties:
> > + - interrupt-controller : Marks the device node as an interrupt controller.
> > + BD71837MWV can report different power state change
> > + events to other devices. Different events can be seen
> > + as separate BD71837 domain interrupts.
>
> To what other devices?
Would it be better if I wrote "other drivers" instead? I think I've seen
examples where MFD driver is just providing the interrupts for other
drivers - like power-button input driver. Currently I have no such "irq
consumer" drivers written. Still I would like to expose these interrupts
so that they are ready for using if any platform using PMIC needs them.
I think there are other similar drivers in tree. For example TPS6591x
driver seems to be doing this. (Has MFD driver exposing the interrupts
but no driver handling those). Maybe explanation like this would help:
"The BD71837 driver only provides the infrastructure for the IRQs. The
users can write his own driver to convert the IRQ into the event they
wish. The IRQ can be used with the standard
request_irq/enable_irq/disable_irq API inside the kernel." (I found this
text from NXP forums and ruthlessly copied and modified it over here)
If this is not feasible, then I will remove the irq handling from MFD
(or leave code there but remove the binding information?) as I don't
know what the irq handles should do in generic case.
>
> > + - #interrupt-cells : The number of cells to describe an IRQ should be 1.
> > + The first cell is the IRQ number.
> > + masks from ../interrupt-controller/interrupts.txt.
Sorry this "masks from ../interrupt-controller/interrupts.txt." was
accidentally pasted here. I should have deleted it.
> I'm still not clear. Generally in a PMIC, you'd define an interrupt
> controller when there's a common set of registers to manage sub-block
> interrupts (typical mask/unmask, ack regs) and the subblocks
> themselves have control of masking/unmasking interrupts. If there's
> not a need to have these 2 levels of interrupt handling, then you
> don't really need to define an interrupt controller.
And to clarify - the PMIC can generate irq via one irq line. This is
typical ACTIVE_LOW irq with 8 bit "write 1 to clear" status register and
8 bit mask register. The role of interrupt-controller code here is just
to allow these 8 irq reasons to be seen as individual BD71837 domain
interrupts. I just don't have the driver(s) for handling these
interrupts.
> >> My concern is you added it but nothing uses it which tells
> >> me your binding is incomplete. I'd rather see complete bindings even
> >> if you don't have drivers.
> >
> > So this makes me wonder if my use-case for interrupt controller is
> > valid. I thought making this PMIC as interrupt controller is a nice way
> > of hiding the irq register and i2c access from other potential drivers
> > using these interrupts. But as I don't know what could be the potential
> > user for these irqs, I don't know how to complete binding. This is why I
> > also thought of making this optional, so that the potential for using
> > the interrupts would be there but it was not required when interrupts
> > are not needed.
>
> The only drivers getting these interrupts would be drivers for this
> PMIC. Interrupts are handled by the driver owning the h/w that
> generated the interrupt. I think that is the disconnect here.
>
> Take a power button. We don't create a generic power button interrupt
> and then have some generic power button interrupt handler. We have a
> handler for specifically for that device and then it generates a power
> button press event.
I think I understand this. Here we also have a 'power button' interrupt
from PMIC (as one of the interrupts) here. But what happens when button
is pressed depends on PMIC configuration. I guess we might want a power
button driver here - and this power button driver might be correct user
for the irq. So are you stating that I should write the power button
driver (or some other "IRQ consumer") before adding the
interrupt-controller property to bindings for MFD? Or should I just
somehow state that irq X in BD71837 is a "power button short push"
event and power button driver should be the consumer for it?
Rest of the interrupts are not so obvious. I have no idea how I should
handle rest of the interrupts. Those are interrupts which cause the PMIC
to reset and cut the powers from most of the regulators too. I can
easily think setup where one processor is controlling PMIC which powers
for the other processor. And getting IRQ if for example watchdog reset
the other processor would probably be very usefull. But doing any
'de-facto' handler for this is hard. Only generally usefull thing would
be notifying the user-space but I don't think I should invent any new
kernelspace - userspace interfaces for this. I believe that when such
are needed those should be implemented by ones knowing the platform.
So please bear with me but do you mean I should
a) document what conditions generate which IRQ
or
b) should I tell what kind of driver is needed for handling the IRQs
or
c) should I first write code using IRQs before addinf MFD binding?
a) I can do.
b) I think I can't do this for most of the irqs as in normal use-case
the processor won't catch these as it is going to be powered down.
c) I might be able to do this for power button IRQ but it might not be
what user wants in the end.
> >> My concern is you added it but nothing uses it which tells
> >> me your binding is incomplete. I'd rather see complete bindings even
> >> if you don't have drivers.
Can you please tell if I misunderstood this?
Br,
Matti Vaittinen
On Mon, Jun 4, 2018 at 6:32 AM, Matti Vaittinen
<[email protected]> wrote:
> On Fri, Jun 01, 2018 at 12:32:16PM -0500, Rob Herring wrote:
>> On Fri, Jun 1, 2018 at 1:25 AM, Matti Vaittinen
>> <[email protected]> wrote:
>> > On Thu, May 31, 2018 at 09:07:24AM -0500, Rob Herring wrote:
>> >> On Thu, May 31, 2018 at 5:23 AM, Matti Vaittinen
>> >> <[email protected]> wrote:
>> >> > On Thu, May 31, 2018 at 10:17:17AM +0300, Matti Vaittinen wrote:
>> >> >> On Wed, May 30, 2018 at 10:01:29PM -0500, Rob Herring wrote:
>> >> >> > On Wed, May 30, 2018 at 11:42:03AM +0300, Matti Vaittinen wrote:
>> >> >> > > Document devicetree bindings for ROHM BD71837 PMIC MFD.
>> >> >> > > + - interrupts : The interrupt line the device is connected to.
>> >> >> > > + - interrupt-controller : Marks the device node as an interrupt controller.
>> >> >> >
>> >> >> > What sub blocks have interrupts?
>> >> >>
>> >> >> The PMIC can generate interrupts from events which cause it to reset.
>> >> >> Eg, irq from watchdog line change, power button pushes, reset request
>> >> >> via register interface etc. I don't know any generic handling for these
>> >> >> interrupts. In "normal" use-case this PMIC is powering the processor
>> >> >> where driver is running and I do not see reasonable handling because
>> >> >> power-reset is going to follow the irq.
>> >> >>
>> >> >
>> >> > Oh, but when reading this I understand that the interrupt-controller
>> >> > property should at least be optional.
>> >>
>> >> I don't think it should. The h/w either has an interrupt controller or
>> >> it doesn't.
>> >
>> > I hope this explains why I did this interrupt controller - please tell
>> > me if this is legitimate use-case and what you think of following:
>> >
>> > +Optional properties:
>> > + - interrupt-controller : Marks the device node as an interrupt controller.
>> > + BD71837MWV can report different power state change
>> > + events to other devices. Different events can be seen
>> > + as separate BD71837 domain interrupts.
>>
>> To what other devices?
>
> Would it be better if I wrote "other drivers" instead? I think I've seen
> examples where MFD driver is just providing the interrupts for other
> drivers - like power-button input driver. Currently I have no such "irq
> consumer" drivers written. Still I would like to expose these interrupts
> so that they are ready for using if any platform using PMIC needs them.
No, worse. Interrupt binding describes interrupt connections between a
controller and devices (which could be sub-blocks in a device), not to
drivers.
I'm just curious as to what sub-blocks/devices there are. You don't
have to have a driver (yet) to define the devices.
>
> I think there are other similar drivers in tree. For example TPS6591x
> driver seems to be doing this. (Has MFD driver exposing the interrupts
> but no driver handling those). Maybe explanation like this would help:
>
> "The BD71837 driver only provides the infrastructure for the IRQs. The
> users can write his own driver to convert the IRQ into the event they
> wish. The IRQ can be used with the standard
> request_irq/enable_irq/disable_irq API inside the kernel." (I found this
> text from NXP forums and ruthlessly copied and modified it over here)
That's all OS details that have nothing to do with the binding. The
binding describes the h/w.
> If this is not feasible, then I will remove the irq handling from MFD
> (or leave code there but remove the binding information?) as I don't
> know what the irq handles should do in generic case.
I don't understand what you mean by generic. An IRQ has to be wired to
something. The only generic IRQs are GPIOs.
>> > + - #interrupt-cells : The number of cells to describe an IRQ should be 1.
>> > + The first cell is the IRQ number.
>> > + masks from ../interrupt-controller/interrupts.txt.
>
> Sorry this "masks from ../interrupt-controller/interrupts.txt." was
> accidentally pasted here. I should have deleted it.
>
>> I'm still not clear. Generally in a PMIC, you'd define an interrupt
>> controller when there's a common set of registers to manage sub-block
>> interrupts (typical mask/unmask, ack regs) and the subblocks
>> themselves have control of masking/unmasking interrupts. If there's
>> not a need to have these 2 levels of interrupt handling, then you
>> don't really need to define an interrupt controller.
>
> And to clarify - the PMIC can generate irq via one irq line. This is
> typical ACTIVE_LOW irq with 8 bit "write 1 to clear" status register and
> 8 bit mask register. The role of interrupt-controller code here is just
> to allow these 8 irq reasons to be seen as individual BD71837 domain
> interrupts. I just don't have the driver(s) for handling these
> interrupts.
If what I'm asking for above is still not clear, what are the 8 bits
defined as or what are those 8 lines connected to?
>
>> >> My concern is you added it but nothing uses it which tells
>> >> me your binding is incomplete. I'd rather see complete bindings even
>> >> if you don't have drivers.
>> >
>> > So this makes me wonder if my use-case for interrupt controller is
>> > valid. I thought making this PMIC as interrupt controller is a nice way
>> > of hiding the irq register and i2c access from other potential drivers
>> > using these interrupts. But as I don't know what could be the potential
>> > user for these irqs, I don't know how to complete binding. This is why I
>> > also thought of making this optional, so that the potential for using
>> > the interrupts would be there but it was not required when interrupts
>> > are not needed.
>>
>> The only drivers getting these interrupts would be drivers for this
>> PMIC. Interrupts are handled by the driver owning the h/w that
>> generated the interrupt. I think that is the disconnect here.
>>
>> Take a power button. We don't create a generic power button interrupt
>> and then have some generic power button interrupt handler. We have a
>> handler for specifically for that device and then it generates a power
>> button press event.
>
> I think I understand this. Here we also have a 'power button' interrupt
> from PMIC (as one of the interrupts) here. But what happens when button
> is pressed depends on PMIC configuration. I guess we might want a power
> button driver here - and this power button driver might be correct user
> for the irq. So are you stating that I should write the power button
> driver (or some other "IRQ consumer") before adding the
> interrupt-controller property to bindings for MFD?
No. Bindings come before drivers.
> Or should I just
> somehow state that irq X in BD71837 is a "power button short push"
> event and power button driver should be the consumer for it?
Yes, at least, but who is the consumer is an OS detail that is not
relevant to the binding. Ideally, you would describe the node with the
interrupts property for "irq X".
> Rest of the interrupts are not so obvious. I have no idea how I should
> handle rest of the interrupts. Those are interrupts which cause the PMIC
> to reset and cut the powers from most of the regulators too. I can
> easily think setup where one processor is controlling PMIC which powers
> for the other processor. And getting IRQ if for example watchdog reset
> the other processor would probably be very usefull. But doing any
> 'de-facto' handler for this is hard. Only generally usefull thing would
> be notifying the user-space but I don't think I should invent any new
> kernelspace - userspace interfaces for this. I believe that when such
> are needed those should be implemented by ones knowing the platform.
Don't think about the OS or driver details. Think about sub-blocks of
the hardware and the connections between them (like irqs) and to board
that need to be described in DT.
If you can't describe that, then you just probably shouldn't have
sub-nodes in DT (ever). Though adding later is easier than trying to
remove them.
>
> So please bear with me but do you mean I should
> a) document what conditions generate which IRQ
> or
> b) should I tell what kind of driver is needed for handling the IRQs
> or
> c) should I first write code using IRQs before addinf MFD binding?
A.
> a) I can do.
> b) I think I can't do this for most of the irqs as in normal use-case
> the processor won't catch these as it is going to be powered down.
> c) I might be able to do this for power button IRQ but it might not be
> what user wants in the end.
>
>> >> My concern is you added it but nothing uses it which tells
>> >> me your binding is incomplete. I'd rather see complete bindings even
>> >> if you don't have drivers.
>
> Can you please tell if I misunderstood this?
>
> Br,
> Matti Vaittinen
>
On Tue, Jun 05, 2018 at 10:46:14AM -0500, Rob Herring wrote:
> On Mon, Jun 4, 2018 at 6:32 AM, Matti Vaittinen
> <[email protected]> wrote:
> > On Fri, Jun 01, 2018 at 12:32:16PM -0500, Rob Herring wrote:
> >> On Fri, Jun 1, 2018 at 1:25 AM, Matti Vaittinen
> >> <[email protected]> wrote:
> >> > On Thu, May 31, 2018 at 09:07:24AM -0500, Rob Herring wrote:
> >> >> On Thu, May 31, 2018 at 5:23 AM, Matti Vaittinen
> >> >> <[email protected]> wrote:
> >> >> > On Thu, May 31, 2018 at 10:17:17AM +0300, Matti Vaittinen wrote:
> >> >> >> On Wed, May 30, 2018 at 10:01:29PM -0500, Rob Herring wrote:
> >> >> >> > On Wed, May 30, 2018 at 11:42:03AM +0300, Matti Vaittinen wrote:
> >> >> >> > > Document devicetree bindings for ROHM BD71837 PMIC MFD.
> >> >> >> > > + - interrupts : The interrupt line the device is connected to.
> >> >> >> > > + - interrupt-controller : Marks the device node as an interrupt controller.
> >> >> >> >
> >> >> >> > What sub blocks have interrupts?
> >> >> >>
> >> >> >> The PMIC can generate interrupts from events which cause it to reset.
> >> >> >> Eg, irq from watchdog line change, power button pushes, reset request
> >> >> >> via register interface etc. I don't know any generic handling for these
> >> >> >> interrupts. In "normal" use-case this PMIC is powering the processor
> >> >> >> where driver is running and I do not see reasonable handling because
> >> >> >> power-reset is going to follow the irq.
> >> >> >>
> >> >> >
> >> >> > Oh, but when reading this I understand that the interrupt-controller
> >> >> > property should at least be optional.
> >> >>
> >> >> I don't think it should. The h/w either has an interrupt controller or
> >> >> it doesn't.
> >> >
> >> > I hope this explains why I did this interrupt controller - please tell
> >> > me if this is legitimate use-case and what you think of following:
> >> >
> >> > +Optional properties:
> >> > + - interrupt-controller : Marks the device node as an interrupt controller.
> >> > + BD71837MWV can report different power state change
> >> > + events to other devices. Different events can be seen
> >> > + as separate BD71837 domain interrupts.
> >>
> >> To what other devices?
> >
> > Would it be better if I wrote "other drivers" instead? I think I've seen
> > examples where MFD driver is just providing the interrupts for other
> > drivers - like power-button input driver. Currently I have no such "irq
> > consumer" drivers written. Still I would like to expose these interrupts
> > so that they are ready for using if any platform using PMIC needs them.
>
> No, worse. Interrupt binding describes interrupt connections between a
> controller and devices (which could be sub-blocks in a device), not to
> drivers.
Ok.
> I'm just curious as to what sub-blocks/devices there are. You don't
> have to have a driver (yet) to define the devices.
Right. I should have done this from the start. I just thought everyone
is busy with other things and pushing people to read data sheets might
be considered as lazines. "Go and read from data sheet what my driver
does, I am too lazy to try to explain what I am doing" - type of thing
you know... But as people asked me for more information about HW:
Datasheet:
https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
(Would it be good idea to add this link to comments in MFD driver or to
binding document(s)?) Page 8 contains roughly the same picture I drew
below. Page 69 shows the interrupt registers. And for interested ones,
HW state transitions are described on page 24.
+--------------------------------------------------+
| |
VSYS +-----------------+ +-----------+ |
| | | | |
| +-------+ | | BUCKS 1-4 +-------->
| | | | | | |
I2C IF +-------> H | +--------------------+ DVS +-------->
| | O | | | Support | |
PWRON_B +-------> S | | | +-------->
| | T | | | | |
PMIC_STBY_REQ +-------> | | | +-------->
| | I | | | | |
PMIC_ON_REQ +-------> / | | +-----------+ |
| | F | | |
WDOG_B +-------> | | +-----------+ |
| | | | | +-------->
| | | +--------------------+ BUCKS 5,8 | |
| | | | | +-------->
| | | | +-----------+ |
| | | | |
| | | | +----------+ |
IRQ_OUT <-------+ | | | | |
| | | +---------------------+ LDO1 +-------->
C32K_OUT <-------+ | | | | |
| | | | +----------+ |
| | | | |
| | | | +----------+ |
| | | | | | |
| | | +---------------------+ LDO2 +-------->
| | | | | | |
| | | | +----------+ |
| | | | |
| | | | +----------+ |
| | | | | | |
| | | +---------------------+ LDO7 +-------->
| +-------+ | | | |
| | +----------+ |
| | |
| | +----------+ +-------------------------->
| | | | | |
| +-+ BUCK6 +-+ +----------+ |
| | | | | | | |
| | +----------+ +------> LDO5 +-------->
| | | | | |
| | | +----------+ |
| | | |
| +-------+ | | +----------+ |
| | | | o | | |
XIN +---------+ 32K | | \-----> LDO3 +-------->
| |Crystal| +--------------o | | |
| |Driver | | +---------+ +----------+ |
XOUT +---------+ | | | | |
| | | +--+ BUCK7 +-+-------------------------->
| +-------+ | | | | |
| | +---------+ | +-----------+ |
| | | | | |
| | +------> LDO6 +------->
| | | | | |
| | | +-----------+ |
| | | |
| | | +-----------+ |
| | o | | |
| | \-----> LDO4 +------->
| +--------------o | | |
| +-----------+ |
| |
+--------------------------------------------------+
On the left we see input lines to PMIC. PWRON_B intended to be connected
to power button. PMIC_STBY_REQ and PMIC_ON_REQ lines for controlling HW
state machine PMIC has. And WDOG_B from watch dog.
PMIC has control register for controlling what happens to BUCK/LDO
outputs when input line states change. PMIC reports change in input
lines via the IRQ_OUT line and IRQ status register.
So HW mapping for interrup(s) from PMIC would be:
(HW) event => BD71837 domain IRQ
PMIC_STBY_REQ line level change => 0
PMIC_ON_REQ line level change => 1
PMIC_WDOG_B line level change => 2
PMIC_PWRON_B line level change => 3
PMIC_PWRON_B line/long push detected => 4
PMIC_PWRON_B line/short push detected => 5
SWRESET register on PMIC written => 6
> > "The BD71837 driver only provides the infrastructure for the IRQs. The
> > users can write his own driver to convert the IRQ into the event they
> > wish. The IRQ can be used with the standard
> > request_irq/enable_irq/disable_irq API inside the kernel." (I found this
> > text from NXP forums and ruthlessly copied and modified it over here)
>
> That's all OS details that have nothing to do with the binding. The
> binding describes the h/w.
Right. I'll drop it.
>
> > If this is not feasible, then I will remove the irq handling from MFD
> > (or leave code there but remove the binding information?) as I don't
> > know what the irq handles should do in generic case.
>
> I don't understand what you mean by generic. An IRQ has to be wired to
> something. The only generic IRQs are GPIOs.
By generic case I mean for example when PMIC_WDOG_B line changes. In
example use-case I have seen, this would be cutting the power from
processor. But this is not necessarily the case. This can be configured
from PMIC register so that action can be warm or cold reset, or no
action. Finally, I'd rather not expect that the BUCKs are supplying
power to processor which is controlling the PMIC. Thus I do not know how
to do generic _handler_ for these interrupts.
So from PMIC HW point of view I know that the interrupt is tied to
PMIC_WDOG_B line change. And this can be described in binding document.
(I tried doing this to v5 patch). Still from system/SW point of view I
don't know what action should be taken (or by which driver) when such
change happens. Hence I liked the idea of hiding the irq register
details in MFD driver by declaring it as interrupt controller - and
leaving the interrupts to be used by what ever drivers need the change
information is system the PMIC is used.
> >> > + - #interrupt-cells : The number of cells to describe an IRQ should be 1.
> >> > + The first cell is the IRQ number.
> >> > + masks from ../interrupt-controller/interrupts.txt.
> >
> > Sorry this "masks from ../interrupt-controller/interrupts.txt." was
> > accidentally pasted here. I should have deleted it.
> >
> >> I'm still not clear. Generally in a PMIC, you'd define an interrupt
> >> controller when there's a common set of registers to manage sub-block
> >> interrupts (typical mask/unmask, ack regs) and the subblocks
> >> themselves have control of masking/unmasking interrupts. If there's
> >> not a need to have these 2 levels of interrupt handling, then you
> >> don't really need to define an interrupt controller.
> >
> > And to clarify - the PMIC can generate irq via one irq line. This is
> > typical ACTIVE_LOW irq with 8 bit "write 1 to clear" status register and
> > 8 bit mask register. The role of interrupt-controller code here is just
> > to allow these 8 irq reasons to be seen as individual BD71837 domain
> > interrupts. I just don't have the driver(s) for handling these
> > interrupts.
>
> If what I'm asking for above is still not clear, what are the 8 bits
> defined as or what are those 8 lines connected to?
I am sorry - there were only 7 bits. One bit was unused. I hope my
explanation abowe did clarify this.
> > Or should I just
> > somehow state that irq X in BD71837 is a "power button short push"
> > event and power button driver should be the consumer for it?
>
> Yes, at least, but who is the consumer is an OS detail that is not
> relevant to the binding. Ideally, you would describe the node with the
> interrupts property for "irq X".
I think I need to try changing my mind set. I tend to think the DT nodes
are for drivers so that drivers can get the information they need. An as
I don't know what kind of driver would be handling the irq, I don't know
what kind of DT node would be good for it. Hence I would rather leave
constructing the node who consumes the IRQ to someone who knows what
they want to do with this IRQ information.
>
> > Rest of the interrupts are not so obvious. I have no idea how I should
> > handle rest of the interrupts. Those are interrupts which cause the PMIC
> > to reset and cut the powers from most of the regulators too. I can
> > easily think setup where one processor is controlling PMIC which powers
> > for the other processor. And getting IRQ if for example watchdog reset
> > the other processor would probably be very usefull. But doing any
> > 'de-facto' handler for this is hard. Only generally usefull thing would
> > be notifying the user-space but I don't think I should invent any new
> > kernelspace - userspace interfaces for this. I believe that when such
> > are needed those should be implemented by ones knowing the platform.
>
> Don't think about the OS or driver details. Think about sub-blocks of
> the hardware and the connections between them (like irqs) and to board
> that need to be described in DT.
>
> If you can't describe that, then you just probably shouldn't have
> sub-nodes in DT (ever).
This is why I did not add any "irq consumer" nodes in example DT. But I
believe someone can think of a board/setup where such are needed. Thus I
liked the idea of providing the interrupt-controller.
> >
> > So please bear with me but do you mean I should
> > a) document what conditions generate which IRQ
> > or
> > b) should I tell what kind of driver is needed for handling the IRQs
> > or
> > c) should I first write code using IRQs before addinf MFD binding?
>
> A.
So what do you think of this:
+Optional properties:
+ - interrupt-controller : Marks the device node as an interrupt controller.
+ BD71837MWV can report input line state change and SW
+ reset events via interrupts. Different events can be seen
+ as separate BD71837 domain interrupts.
+ - #interrupt-cells : The number of cells to describe an IRQ should be 1.
+ The value in cell is the IRQ number.
+ Meaningfull numbers are:
+ 0 => PMIC_STBY_REQ level change
+ 1 => PMIC_ON_REQ level change
+ 2 => WDOG_B level change
+ 3 => Power Button level change
+ 4 => Power Button Long Push
+ 5 => Power Button Short Push
+ 6 => SWRESET register is written 1
Would this be getting closer to what is needed from binding document?
Oh, and thanks for the patience =)
Br,
Matti Vaittinen
On Wed, Jun 6, 2018 at 2:34 AM, Matti Vaittinen
<[email protected]> wrote:
> On Tue, Jun 05, 2018 at 10:46:14AM -0500, Rob Herring wrote:
>> On Mon, Jun 4, 2018 at 6:32 AM, Matti Vaittinen
>> <[email protected]> wrote:
>> > On Fri, Jun 01, 2018 at 12:32:16PM -0500, Rob Herring wrote:
>> >> On Fri, Jun 1, 2018 at 1:25 AM, Matti Vaittinen
>> >> <[email protected]> wrote:
>> >> > On Thu, May 31, 2018 at 09:07:24AM -0500, Rob Herring wrote:
>> >> >> On Thu, May 31, 2018 at 5:23 AM, Matti Vaittinen
>> >> >> <[email protected]> wrote:
>> >> >> > On Thu, May 31, 2018 at 10:17:17AM +0300, Matti Vaittinen wrote:
>> >> >> >> On Wed, May 30, 2018 at 10:01:29PM -0500, Rob Herring wrote:
>> >> >> >> > On Wed, May 30, 2018 at 11:42:03AM +0300, Matti Vaittinen wrote:
>> >> >> >> > > Document devicetree bindings for ROHM BD71837 PMIC MFD.
>> >> >> >> > > + - interrupts : The interrupt line the device is connected to.
>> >> >> >> > > + - interrupt-controller : Marks the device node as an interrupt controller.
>> >> >> >> >
>> >> >> >> > What sub blocks have interrupts?
>> >> >> >>
>> >> >> >> The PMIC can generate interrupts from events which cause it to reset.
>> >> >> >> Eg, irq from watchdog line change, power button pushes, reset request
>> >> >> >> via register interface etc. I don't know any generic handling for these
>> >> >> >> interrupts. In "normal" use-case this PMIC is powering the processor
>> >> >> >> where driver is running and I do not see reasonable handling because
>> >> >> >> power-reset is going to follow the irq.
>> >> >> >>
>> >> >> >
>> >> >> > Oh, but when reading this I understand that the interrupt-controller
>> >> >> > property should at least be optional.
>> >> >>
>> >> >> I don't think it should. The h/w either has an interrupt controller or
>> >> >> it doesn't.
>> >> >
>> >> > I hope this explains why I did this interrupt controller - please tell
>> >> > me if this is legitimate use-case and what you think of following:
>> >> >
>> >> > +Optional properties:
>> >> > + - interrupt-controller : Marks the device node as an interrupt controller.
>> >> > + BD71837MWV can report different power state change
>> >> > + events to other devices. Different events can be seen
>> >> > + as separate BD71837 domain interrupts.
>> >>
>> >> To what other devices?
>> >
>> > Would it be better if I wrote "other drivers" instead? I think I've seen
>> > examples where MFD driver is just providing the interrupts for other
>> > drivers - like power-button input driver. Currently I have no such "irq
>> > consumer" drivers written. Still I would like to expose these interrupts
>> > so that they are ready for using if any platform using PMIC needs them.
>>
>> No, worse. Interrupt binding describes interrupt connections between a
>> controller and devices (which could be sub-blocks in a device), not to
>> drivers.
>
> Ok.
>
>> I'm just curious as to what sub-blocks/devices there are. You don't
>> have to have a driver (yet) to define the devices.
>
> Right. I should have done this from the start. I just thought everyone
> is busy with other things and pushing people to read data sheets might
> be considered as lazines. "Go and read from data sheet what my driver
> does, I am too lazy to try to explain what I am doing" - type of thing
> you know... But as people asked me for more information about HW:
>
> Datasheet:
> https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
> (Would it be good idea to add this link to comments in MFD driver or to
> binding document(s)?)
Yes, it would.
> Page 8 contains roughly the same picture I drew
> below. Page 69 shows the interrupt registers. And for interested ones,
> HW state transitions are described on page 24.
>
> +--------------------------------------------------+
> | |
> VSYS +-----------------+ +-----------+ |
> | | | | |
> | +-------+ | | BUCKS 1-4 +-------->
> | | | | | | |
> I2C IF +-------> H | +--------------------+ DVS +-------->
> | | O | | | Support | |
> PWRON_B +-------> S | | | +-------->
> | | T | | | | |
> PMIC_STBY_REQ +-------> | | | +-------->
> | | I | | | | |
> PMIC_ON_REQ +-------> / | | +-----------+ |
> | | F | | |
> WDOG_B +-------> | | +-----------+ |
> | | | | | +-------->
> | | | +--------------------+ BUCKS 5,8 | |
> | | | | | +-------->
> | | | | +-----------+ |
> | | | | |
> | | | | +----------+ |
> IRQ_OUT <-------+ | | | | |
> | | | +---------------------+ LDO1 +-------->
> C32K_OUT <-------+ | | | | |
> | | | | +----------+ |
> | | | | |
> | | | | +----------+ |
> | | | | | | |
> | | | +---------------------+ LDO2 +-------->
> | | | | | | |
> | | | | +----------+ |
> | | | | |
> | | | | +----------+ |
> | | | | | | |
> | | | +---------------------+ LDO7 +-------->
> | +-------+ | | | |
> | | +----------+ |
> | | |
> | | +----------+ +-------------------------->
> | | | | | |
> | +-+ BUCK6 +-+ +----------+ |
> | | | | | | | |
> | | +----------+ +------> LDO5 +-------->
> | | | | | |
> | | | +----------+ |
> | | | |
> | +-------+ | | +----------+ |
> | | | | o | | |
> XIN +---------+ 32K | | \-----> LDO3 +-------->
> | |Crystal| +--------------o | | |
> | |Driver | | +---------+ +----------+ |
> XOUT +---------+ | | | | |
> | | | +--+ BUCK7 +-+-------------------------->
> | +-------+ | | | | |
> | | +---------+ | +-----------+ |
> | | | | | |
> | | +------> LDO6 +------->
> | | | | | |
> | | | +-----------+ |
> | | | |
> | | | +-----------+ |
> | | o | | |
> | | \-----> LDO4 +------->
> | +--------------o | | |
> | +-----------+ |
> | |
> +--------------------------------------------------+
>
> On the left we see input lines to PMIC. PWRON_B intended to be connected
> to power button. PMIC_STBY_REQ and PMIC_ON_REQ lines for controlling HW
> state machine PMIC has. And WDOG_B from watch dog.
>
> PMIC has control register for controlling what happens to BUCK/LDO
> outputs when input line states change. PMIC reports change in input
> lines via the IRQ_OUT line and IRQ status register.
So it looks like this is just regulators with a few other signals to handle.
> So HW mapping for interrup(s) from PMIC would be:
>
> (HW) event => BD71837 domain IRQ
>
> PMIC_STBY_REQ line level change => 0
> PMIC_ON_REQ line level change => 1
I'm not really clear how these would have s/w handling. They look like
handshake signals for the processor to enter and exit standby/suspend.
H/w designers don't always know what to do with things and may have
just said lets have an interrupt for every input signal. I'd think you
would just want DT properties to configure what action to take (and
perhaps polarity?).
> PMIC_WDOG_B line level change => 2
Ah, this is an input signal, not a watchdog block within the PMIC. I
think this should just be handled by the core driver. If you need to
configure what this does, then we can add a property to handle that.
> PMIC_PWRON_B line level change => 3
> PMIC_PWRON_B line/long push detected => 4
> PMIC_PWRON_B line/short push detected => 5
So you need a power button driver (or maybe not even a separate
driver) for this, but I don't think that warrants a child node. I
think some PMIC drivers just generate a power key press (which just
gets punted to userspace), but some will do an actual power down. Or
maybe a combination of those based on short/long press.
I think there's already some DT properties defined to control the
behavior as well.
> SWRESET register on PMIC written => 6
Probably this can be handled within the core driver. Seems like you'd
only use this if you have separate entities that write and read this.
If you don't know, then just ignore it for now.
>> > "The BD71837 driver only provides the infrastructure for the IRQs. The
>> > users can write his own driver to convert the IRQ into the event they
>> > wish. The IRQ can be used with the standard
>> > request_irq/enable_irq/disable_irq API inside the kernel." (I found this
>> > text from NXP forums and ruthlessly copied and modified it over here)
>>
>> That's all OS details that have nothing to do with the binding. The
>> binding describes the h/w.
>
> Right. I'll drop it.
>
>>
>> > If this is not feasible, then I will remove the irq handling from MFD
>> > (or leave code there but remove the binding information?) as I don't
>> > know what the irq handles should do in generic case.
>>
>> I don't understand what you mean by generic. An IRQ has to be wired to
>> something. The only generic IRQs are GPIOs.
>
> By generic case I mean for example when PMIC_WDOG_B line changes. In
> example use-case I have seen, this would be cutting the power from
> processor. But this is not necessarily the case. This can be configured
> from PMIC register so that action can be warm or cold reset, or no
> action. Finally, I'd rather not expect that the BUCKs are supplying
> power to processor which is controlling the PMIC. Thus I do not know how
> to do generic _handler_ for these interrupts.
>
> So from PMIC HW point of view I know that the interrupt is tied to
> PMIC_WDOG_B line change. And this can be described in binding document.
> (I tried doing this to v5 patch). Still from system/SW point of view I
> don't know what action should be taken (or by which driver) when such
> change happens. Hence I liked the idea of hiding the irq register
> details in MFD driver by declaring it as interrupt controller - and
> leaving the interrupts to be used by what ever drivers need the change
> information is system the PMIC is used.
This can still be done but doesn't have to be in DT.
>> >> > + - #interrupt-cells : The number of cells to describe an IRQ should be 1.
>> >> > + The first cell is the IRQ number.
>> >> > + masks from ../interrupt-controller/interrupts.txt.
>> >
>> > Sorry this "masks from ../interrupt-controller/interrupts.txt." was
>> > accidentally pasted here. I should have deleted it.
>> >
>> >> I'm still not clear. Generally in a PMIC, you'd define an interrupt
>> >> controller when there's a common set of registers to manage sub-block
>> >> interrupts (typical mask/unmask, ack regs) and the subblocks
>> >> themselves have control of masking/unmasking interrupts. If there's
>> >> not a need to have these 2 levels of interrupt handling, then you
>> >> don't really need to define an interrupt controller.
>> >
>> > And to clarify - the PMIC can generate irq via one irq line. This is
>> > typical ACTIVE_LOW irq with 8 bit "write 1 to clear" status register and
>> > 8 bit mask register. The role of interrupt-controller code here is just
>> > to allow these 8 irq reasons to be seen as individual BD71837 domain
>> > interrupts. I just don't have the driver(s) for handling these
>> > interrupts.
>>
>> If what I'm asking for above is still not clear, what are the 8 bits
>> defined as or what are those 8 lines connected to?
>
> I am sorry - there were only 7 bits. One bit was unused. I hope my
> explanation abowe did clarify this.
>
>> > Or should I just
>> > somehow state that irq X in BD71837 is a "power button short push"
>> > event and power button driver should be the consumer for it?
>>
>> Yes, at least, but who is the consumer is an OS detail that is not
>> relevant to the binding. Ideally, you would describe the node with the
>> interrupts property for "irq X".
>
> I think I need to try changing my mind set. I tend to think the DT nodes
> are for drivers so that drivers can get the information they need. An as
> I don't know what kind of driver would be handling the irq, I don't know
> what kind of DT node would be good for it. Hence I would rather leave
> constructing the node who consumes the IRQ to someone who knows what
> they want to do with this IRQ information.
>
>>
>> > Rest of the interrupts are not so obvious. I have no idea how I should
>> > handle rest of the interrupts. Those are interrupts which cause the PMIC
>> > to reset and cut the powers from most of the regulators too. I can
>> > easily think setup where one processor is controlling PMIC which powers
>> > for the other processor. And getting IRQ if for example watchdog reset
>> > the other processor would probably be very usefull. But doing any
>> > 'de-facto' handler for this is hard. Only generally usefull thing would
>> > be notifying the user-space but I don't think I should invent any new
>> > kernelspace - userspace interfaces for this. I believe that when such
>> > are needed those should be implemented by ones knowing the platform.
>>
>> Don't think about the OS or driver details. Think about sub-blocks of
>> the hardware and the connections between them (like irqs) and to board
>> that need to be described in DT.
>>
>> If you can't describe that, then you just probably shouldn't have
>> sub-nodes in DT (ever).
>
> This is why I did not add any "irq consumer" nodes in example DT. But I
> believe someone can think of a board/setup where such are needed. Thus I
> liked the idea of providing the interrupt-controller.
>
>> >
>> > So please bear with me but do you mean I should
>> > a) document what conditions generate which IRQ
>> > or
>> > b) should I tell what kind of driver is needed for handling the IRQs
>> > or
>> > c) should I first write code using IRQs before addinf MFD binding?
>>
>> A.
>
>
> So what do you think of this:
>
> +Optional properties:
> + - interrupt-controller : Marks the device node as an interrupt controller.
> + BD71837MWV can report input line state change and SW
> + reset events via interrupts. Different events can be seen
> + as separate BD71837 domain interrupts.
> + - #interrupt-cells : The number of cells to describe an IRQ should be 1.
> + The value in cell is the IRQ number.
> + Meaningfull numbers are:
> + 0 => PMIC_STBY_REQ level change
> + 1 => PMIC_ON_REQ level change
> + 2 => WDOG_B level change
> + 3 => Power Button level change
> + 4 => Power Button Long Push
> + 5 => Power Button Short Push
> + 6 => SWRESET register is written 1
>
> Would this be getting closer to what is needed from binding document?
I don't think any of this needs to live in DT. All you really need a
separate driver (and hence irq) for is really just the power button.
You can just define the interrupts within the kernel.
Rob
On Wed, Jun 06, 2018 at 10:16:37AM -0500, Rob Herring wrote:
> On Wed, Jun 6, 2018 at 2:34 AM, Matti Vaittinen
> <[email protected]> wrote:
> > On Tue, Jun 05, 2018 at 10:46:14AM -0500, Rob Herring wrote:
> >> On Mon, Jun 4, 2018 at 6:32 AM, Matti Vaittinen
> >> <[email protected]> wrote:
> >> > On Fri, Jun 01, 2018 at 12:32:16PM -0500, Rob Herring wrote:
> >> >> On Fri, Jun 1, 2018 at 1:25 AM, Matti Vaittinen
> >> >> <[email protected]> wrote:
> >> >> > On Thu, May 31, 2018 at 09:07:24AM -0500, Rob Herring wrote:
> >> >> >> On Thu, May 31, 2018 at 5:23 AM, Matti Vaittinen
> >> >> >> <[email protected]> wrote:
> >> >> >> > On Thu, May 31, 2018 at 10:17:17AM +0300, Matti Vaittinen wrote:
> >> >> >> >> On Wed, May 30, 2018 at 10:01:29PM -0500, Rob Herring wrote:
> >> >> >> >> > On Wed, May 30, 2018 at 11:42:03AM +0300, Matti Vaittinen wrote:
> > Datasheet:
> > https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
> > (Would it be good idea to add this link to comments in MFD driver or to
> > binding document(s)?)
>
> Yes, it would.
I will add this then. I don't really like adding links like this as urls
tend to change and links become dead - but I guess this is something we
just must live with.
> > +--------------------------------------------------+
> > | |
> > VSYS +-----------------+ +-----------+ |
> > | | | | |
> > | +-------+ | | BUCKS 1-4 +-------->
> > | | | | | | |
> > I2C IF +-------> H | +--------------------+ DVS +-------->
> > | | O | | | Support | |
> > PWRON_B +-------> S | | | +-------->
> > | | T | | | | |
> > PMIC_STBY_REQ +-------> | | | +-------->
Do you think this ASCII art picture in MFD or regulator driver comments would
be an overkill?
> > On the left we see input lines to PMIC. PWRON_B intended to be connected
> > to power button. PMIC_STBY_REQ and PMIC_ON_REQ lines for controlling HW
> > state machine PMIC has. And WDOG_B from watch dog.
> >
> > PMIC has control register for controlling what happens to BUCK/LDO
> > outputs when input line states change. PMIC reports change in input
> > lines via the IRQ_OUT line and IRQ status register.
>
> So it looks like this is just regulators with a few other signals to handle.
Yes. Regulators and the HW state machine which is currently mostly
bypassed by drivers. And while we are at it - is there some standard
device-tree properties for describing the voltages for different PMIC states
(idle, run, standby) so that the driver could configure voltages the
bucks should use when PMIC state is changed. Or do you think I should
use custom ones like:
bd71837,pmic-buck1-dvs-voltage = <900000>, <850000>, <800000>; /* VDD_SOC: Run-Idle-Suspend */
bd71837,pmic-buck2-dvs-voltage = <1000000>, <900000>, <0>; /* VDD_ARM: Run-Idle */
bd71837,pmic-buck3-dvs-voltage = <1000000>, <0>, <0>; /* VDD_GPU: Run */
bd71837,pmic-buck4-dvs-voltage = <1000000>, <0>, <0>; /* VDD_VPU: Run */
I think this is not the only PMIC with configurable voltages for
different states so someone has probably already invented a way to
provide this information - is the DT correct place to search for this?
>
> > So HW mapping for interrup(s) from PMIC would be:
> >
> > (HW) event => BD71837 domain IRQ
> >
> > PMIC_STBY_REQ line level change => 0
> > PMIC_ON_REQ line level change => 1
>
> I'm not really clear how these would have s/w handling. They look like
> handshake signals for the processor to enter and exit standby/suspend.
> H/w designers don't always know what to do with things and may have
> just said lets have an interrupt for every input signal.
The PMIC_ON_REQ can be used to switch the PMIC from 'READY' to 'SNVS'
and from 'SNVS' to 'RUN' states. But same transitions can be configured
to be done by power button presses too.
I think I in some earlier mail said that I can't think how to handle
other but power button interrupts - when PMIC is used to power the
processor controlling PMIC. But I also mentioned another use case
which I can think of - but I am not sure if it is relevant or not. In
this other use case we have a 'slave processor' powered by PMIC doing
some specific tasks - and a 'control processor' which is not powered by
this PMIC - but which may be controlling it (and thus also controls power
for the slave). In such setup all these interrupts mmight become
meaningfull - but handling should be platform specific then. (The
control processor could for example detect slave processor resets or
wake-ups via these interrupts and initiate any platform specific
activities based on this). I have no system design experience so I don't
know if this sounds reasonable or not - but thats why I liked the idea
of providing access to all interrupts via this interrupt-controller.
> I'd think you
> would just want DT properties to configure what action to take (and
> perhaps polarity?).
Currently the driver is not configuring what happens when state of these
signals change. But it could as you say. And it would be nice to get the
configuration from DT. I think the polarity is fixed.
> > PMIC_WDOG_B line level change => 2
>
> Ah, this is an input signal, not a watchdog block within the PMIC. I
> think this should just be handled by the core driver. If you need to
> configure what this does, then we can add a property to handle that.
Action taken when WDOG_B is asserted is indeed configurable. PMIC can do
cold reset, warm reset of take no action. But here I again fail to think
how this should be handled - especially because HW default for action is
cold reset. But as I said abowe - it may be there is use case for this
interrupt even though I can't provide generic one. ...and this is why I
liked the idea of having it available via DT... :)
>
> > PMIC_PWRON_B line level change => 3
> > PMIC_PWRON_B line/long push detected => 4
> > PMIC_PWRON_B line/short push detected => 5
>
> So you need a power button driver (or maybe not even a separate
> driver) for this, but I don't think that warrants a child node. I
> think some PMIC drivers just generate a power key press (which just
> gets punted to userspace), but some will do an actual power down. Or
> maybe a combination of those based on short/long press.
I agree with you on this. A power button driver could consume these
IRQs. And yes, (input?) event to user-space sounds reasonable. Maybe
also a power-off handler if "system-power-controller" property is given.
Whether this is something crying for own DT node is not my decision. I
just know that having own sub nodes and relying on irq-controller xlate
callbacks eould make driver side really simple.
>
> I think there's already some DT properties defined to control the
> behavior as well.
>
> > SWRESET register on PMIC written => 6
>
> Probably this can be handled within the core driver. Seems like you'd
> only use this if you have separate entities that write and read this.
> If you don't know, then just ignore it for now.
Yep. My plan was not to implement handling for any of these interrupts
for now. I just wanted to make them easily available for any future use.
> > Hence I liked the idea of hiding the irq register
> > details in MFD driver by declaring it as interrupt controller - and
> > leaving the interrupts to be used by what ever drivers need the change
> > information is system the PMIC is used.
>
> This can still be done but doesn't have to be in DT.
I think this is my weak spot. I don't know how to do this easily without
DT. How do I bring the irq to power-button driver or to WDOG
irq handler (or XXX-driver invented by one who needs these IRQs) without
DT. Options I can think of are:
1. getting the GPIO irq (from MFD parent) and making the IRQ registers
visible to power button/XXX drivers. Possibly ending up with shared
IRQ hanlder.
2. Keeping the IRQ register accesses in MFD driver and providing some
handler callback registration interface from MFD driver to power
button/XXX drivers
3. Using this interrupt-controller approach.
I dislike option 1 because all drivers using IRQs need to be aware of
IRQ registers.
I dislike option 2 because inventing such IRQ callbacks just feel wrong.
There must be better way - I just don't know it =)
I would like to use option 3 - but I don't know how to do it without DT?
Specifically I don't know how to provide the irq number and interrupt
parent to power-button/XXX drivers if they are not brought from DT. And
why to invent some parallel mechanism for this when DT usage already
provides this. I would be really gratefull for any tips how to do this
correctly if DT is not used.
> > So what do you think of this:
> >
> > +Optional properties:
> > + - interrupt-controller : Marks the device node as an interrupt controller.
> > + BD71837MWV can report input line state change and SW
> > + reset events via interrupts. Different events can be seen
> > + as separate BD71837 domain interrupts.
> > + - #interrupt-cells : The number of cells to describe an IRQ should be 1.
> > + The value in cell is the IRQ number.
> > + Meaningfull numbers are:
> > + 0 => PMIC_STBY_REQ level change
> > + 1 => PMIC_ON_REQ level change
> > + 2 => WDOG_B level change
> > + 3 => Power Button level change
> > + 4 => Power Button Long Push
> > + 5 => Power Button Short Push
> > + 6 => SWRESET register is written 1
> >
> > Would this be getting closer to what is needed from binding document?
>
> I don't think any of this needs to live in DT. All you really need a
> separate driver (and hence irq) for is really just the power button.
> You can just define the interrupts within the kernel.
So I tried to argue that all of the IRQs should be usable and we should
not limit the HW capabilities by design (I like the idea of exposing all
HW functionality because I only rarely know what should be usable better
than the user). Are you sure my dual processor use-case is not
convincing you we should provide these interrupts via DT?
If so - as I already said, I would be gratefull for any suggestions on
how to provide access to irqs without DT. I just can't help feeling that
DT would have been the best way for cleanest and most
understandable/usable end result.
Meanwhile I will prepare a patch where the interrupt handling is
completely ripped from MFD and binding documents as I don't have power
button driver tested (I drafted something though). Maybe such a patch
could be acceptable in order to enable regulator usage (and
compilation). Is that Ok?
Best Regards
Matti Vaittinen
On Thu, Jun 07, 2018 at 02:12:18PM +0300, Matti Vaittinen wrote:
> On Wed, Jun 06, 2018 at 10:16:37AM -0500, Rob Herring wrote:
> > On Wed, Jun 6, 2018 at 2:34 AM, Matti Vaittinen
> > <[email protected]> wrote:
> > > On Tue, Jun 05, 2018 at 10:46:14AM -0500, Rob Herring wrote:
> > >> On Mon, Jun 4, 2018 at 6:32 AM, Matti Vaittinen
> > >> <[email protected]> wrote:
> > >> > On Fri, Jun 01, 2018 at 12:32:16PM -0500, Rob Herring wrote:
> > >> >> On Fri, Jun 1, 2018 at 1:25 AM, Matti Vaittinen
> > >> >> <[email protected]> wrote:
> > >> >> > On Thu, May 31, 2018 at 09:07:24AM -0500, Rob Herring wrote:
> > >> >> >> On Thu, May 31, 2018 at 5:23 AM, Matti Vaittinen
> > >> >> >> <[email protected]> wrote:
> > >> >> >> > On Thu, May 31, 2018 at 10:17:17AM +0300, Matti Vaittinen wrote:
> > >> >> >> >> On Wed, May 30, 2018 at 10:01:29PM -0500, Rob Herring wrote:
> > >> >> >> >> > On Wed, May 30, 2018 at 11:42:03AM +0300, Matti Vaittinen wrote:
> > So it looks like this is just regulators with a few other signals to handle.
>
> Yes. Regulators and the HW state machine which is currently mostly
> bypassed by drivers. And while we are at it - is there some standard
> device-tree properties for describing the voltages for different PMIC states
> (idle, run, standby) so that the driver could configure voltages the
> bucks should use when PMIC state is changed. Or do you think I should
> use custom ones like:
>
> bd71837,pmic-buck1-dvs-voltage = <900000>, <850000>, <800000>; /* VDD_SOC: Run-Idle-Suspend */
> bd71837,pmic-buck2-dvs-voltage = <1000000>, <900000>, <0>; /* VDD_ARM: Run-Idle */
> bd71837,pmic-buck3-dvs-voltage = <1000000>, <0>, <0>; /* VDD_GPU: Run */
> bd71837,pmic-buck4-dvs-voltage = <1000000>, <0>, <0>; /* VDD_VPU: Run */
>
> I think this is not the only PMIC with configurable voltages for
> different states so someone has probably already invented a way to
> provide this information - is the DT correct place to search for this?
I will leave this out for now. Guess this can be added later.
>
> >
> > > So HW mapping for interrup(s) from PMIC would be:
> > >
> > > (HW) event => BD71837 domain IRQ
> > >
> > > PMIC_STBY_REQ line level change => 0
> > > PMIC_ON_REQ line level change => 1
> >
> > I'm not really clear how these would have s/w handling. They look like
> > handshake signals for the processor to enter and exit standby/suspend.
> > H/w designers don't always know what to do with things and may have
> > just said lets have an interrupt for every input signal.
>
Well, I will only handle the power button irq as you suggested for now.
> > > PMIC_PWRON_B line level change => 3
> > > PMIC_PWRON_B line/long push detected => 4
> > > PMIC_PWRON_B line/short push detected => 5
> >
> > So you need a power button driver (or maybe not even a separate
> > driver) for this, but I don't think that warrants a child node. I
> > think some PMIC drivers just generate a power key press (which just
> > gets punted to userspace), but some will do an actual power down. Or
> > maybe a combination of those based on short/long press.
I add power button driver (and input subsystem people) tp next patch
set. I think I will later also add power/reset driver because PMIC can
do reset for the system. Unfortunately the PMIC can't provide power-off.
But I leave this out from this series.
> > I think there's already some DT properties defined to control the
> > behavior as well.
> >
> > > SWRESET register on PMIC written => 6
> >
> > Probably this can be handled within the core driver. Seems like you'd
> > only use this if you have separate entities that write and read this.
> > If you don't know, then just ignore it for now.
I planned to use SWRESET for power/reset driver - but irq handling is
not needed there.
> > > Hence I liked the idea of hiding the irq register
> > > details in MFD driver by declaring it as interrupt controller - and
> > > leaving the interrupts to be used by what ever drivers need the change
> > > information is system the PMIC is used.
> >
> > This can still be done but doesn't have to be in DT.
>
> I think this is my weak spot. I don't know how to do this easily without
> DT.
I think I found the correct way - I'll send the patch v6 soon. I hope it
addresses this issue correctly.
Best Regards
Matti Vaittinen