* [PATCH v8 0/2] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver
@ 2018-06-29 8:20 Matti Vaittinen
2018-06-29 8:21 ` [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC Matti Vaittinen
2018-06-29 8:23 ` [PATCH v8 2/2] mfd: bd71837: Devicetree bindings " Matti Vaittinen
0 siblings, 2 replies; 16+ messages in thread
From: Matti Vaittinen @ 2018-06-29 8:20 UTC (permalink / raw)
To: mturquette, robh+dt, mark.rutland, lee.jones, lgirdwood, broonie,
mazziesaccount, arnd, dmitry.torokhov, sre, chenjh,
andrew.smirnov, linus.walleij, kstewart, heiko, gregkh
Cc: sboyd, linux-clk, devicetree, linux-kernel, linux-input,
mikko.mutanen, heikki.haikola
Patch series adding support for ROHM BD71837 PMIC.
BD71837 is a programmable Power Management IC for powering single-core,
dual-core, and quad-core SoC’s such as NXP-i.MX 8M. It is optimized for
low BOM cost and compact solution footprint. It integrates 8 buck
regulators and 7 LDO’s to provide all the power rails required by the
SoC and the commonly used peripherals.
This is reduced set of patches containing only the MFD and devicetree
bindings. Devicetree bindings patch (2) is not changed. The MFD patch
enables alrady applied regulator part and power button support using
gpio-keys. Clock and reset support will be sent as separate set of
patches - possibly only after my vacations. Sorry for longish delay
which will follow.
Changelog v8
- Dropped clk-bd71837 from series (will send later)
- Patch 2 is unchanged.
- Dropped bd718xx-pwrkey driver and used gpio_keys instead.
- Added power-button short/long press duration configuratio to MFD
- Cleaned MFD driver according to comments from Enric Balletbo Serra.
(used devm, removed unnecessary header inclusions, removed redundant
assignment, styling issues, allow building MFD part as module, fixed
license mismatch).
Changelog v7
- patch 1: Cleaned MFD probe since MFD no longer directly reads DT
properties.
- patch 1/4: Moved power-key related definitions from powerkey patch (4)
to MFD patch (1) so that powerkey can be applied independently
- Patch 2 is unchanged.
- patch 3: Added missing allocation check back to clk probe
Changelog v6
- Added power-key input driver
Based on feedback from Rob Herring and Stephen Boyd
- Added link to datasheet
- Removed interrupt-controller from DT and fixed binding document
- clk styling fixes
- remove clkdev usage
- add clk bindings to MFD documentation
- removed clk binding document
Changelog v5
- dropped regulator patches which are already applied to Mark's tree
Based on feedback from Rob Herring and Stephen Boyd
- mfd bindings: explain why this can be interrupt-controller
- mfd bindings: describe interrupts better
- mfd bindings: require one cell interrupt specifier
- mfd bindings: use generic node names in example
- mfd driver: ack masked interrupt once at init
- clk bindings: use generic node names in example
- clk driver: use devm
- clk driver: use of_clk_add_hw_provider
- clk driver: change severity of print and how prints are emitted at
probe error path.
- clk driver: dropped forward declared functions
- clk configs: drop unnecessary dependencies
- clk driver: other styling issues
- mfd/clk DT: drop clk node.
Changelog v4
- remove mutex from regulator state check as core prevents simultaneous
accesses
- allow voltage change for bucks 1 to 4 when regulator is enabled
- fix indentiation problems
- properly correct SPDX comments
Changelog v3
- kill unused variable
- kill unused definitions
- use REGMAP_IRQ_REG
Changelog v2
Based on feedback from Mark Brown
- Squashed code and buildfile changes to same patch
- Fixed some styling issues
- Changed SPDX comments to CPP style
- Error out if voltage is changed when regulator is enabled instead of
Disabling the regulator for duration of change
- Use devm_regulator_register
- Remove compatible usage from regulators - use parent dev for config
- Add a note about using regulator-boot-on for BUCK6 and 7
- fixed warnings from kbuild test robot
patch 1:
MFD driver and definitions bringing interrupt support and
enabling clk, regulator and input subsystems.
patch 2:
MFD driver DT bindings
This patch series is based on for-mfd-next
---
Matti Vaittinen (2):
mfd: bd71837: mfd driver for ROHM BD71837 PMIC
mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
.../devicetree/bindings/mfd/rohm,bd71837-pmic.txt | 67 ++++
drivers/mfd/Kconfig | 13 +
drivers/mfd/Makefile | 1 +
drivers/mfd/bd71837.c | 220 ++++++++++++
include/linux/mfd/bd71837.h | 391 +++++++++++++++++++++
5 files changed, 692 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
create mode 100644 drivers/mfd/bd71837.c
create mode 100644 include/linux/mfd/bd71837.h
--
2.14.3
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-06-29 8:20 [PATCH v8 0/2] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver Matti Vaittinen
@ 2018-06-29 8:21 ` Matti Vaittinen
2018-07-03 9:26 ` Enric Balletbo Serra
2018-06-29 8:23 ` [PATCH v8 2/2] mfd: bd71837: Devicetree bindings " Matti Vaittinen
1 sibling, 1 reply; 16+ messages in thread
From: Matti Vaittinen @ 2018-06-29 8:21 UTC (permalink / raw)
To: mturquette, robh+dt, mark.rutland, lee.jones, lgirdwood, broonie,
mazziesaccount, arnd, dmitry.torokhov, sre, chenjh,
andrew.smirnov, linus.walleij, kstewart, heiko, gregkh
Cc: sboyd, linux-clk, devicetree, linux-kernel, linux-input,
mikko.mutanen, heikki.haikola
ROHM BD71837 PMIC MFD driver providing interrupts and support
for three subsystems:
- clk
- Regulators
- input/power-key
fix too long lines
Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
drivers/mfd/Kconfig | 13 ++
drivers/mfd/Makefile | 1 +
drivers/mfd/bd71837.c | 220 +++++++++++++++++++++++++
include/linux/mfd/bd71837.h | 391 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 625 insertions(+)
create mode 100644 drivers/mfd/bd71837.c
create mode 100644 include/linux/mfd/bd71837.h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index b860eb5aa194..d01a279f5e4a 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1787,6 +1787,19 @@ config MFD_STW481X
in various ST Microelectronics and ST-Ericsson embedded
Nomadik series.
+config MFD_BD71837
+ tristate "BD71837 Power Management chip"
+ depends on I2C=y
+ depends on OF
+ select REGMAP_I2C
+ select REGMAP_IRQ
+ select MFD_CORE
+ help
+ Select this option to get support for the ROHM BD71837
+ Power Management chips. BD71837 is designed to power processors like
+ NXP i.MX8. It contains 8 BUCK outputs and 7 LDOs, voltage monitoring
+ and emergency shut down as well as 32,768KHz clock output.
+
config MFD_STM32_LPTIMER
tristate "Support for STM32 Low-Power Timer"
depends on (ARCH_STM32 && OF) || COMPILE_TEST
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index e9fd20dba18d..09dc9eb3782c 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -227,4 +227,5 @@ obj-$(CONFIG_MFD_STM32_TIMERS) += stm32-timers.o
obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o
obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o
obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
+obj-$(CONFIG_MFD_BD71837) += bd71837.o
diff --git a/drivers/mfd/bd71837.c b/drivers/mfd/bd71837.c
new file mode 100644
index 000000000000..25f2de2a5bb8
--- /dev/null
+++ b/drivers/mfd/bd71837.c
@@ -0,0 +1,220 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright (C) 2018 ROHM Semiconductors
+// bd71837.c -- ROHM BD71837MWV mfd driver
+//
+// Datasheet available from
+// https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
+
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/bd71837.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/gpio_keys.h>
+
+static struct gpio_keys_button btns[] = {
+ {
+ .code = KEY_POWER,
+ .gpio = -1,
+ .type = EV_KEY,
+ }
+};
+
+static struct gpio_keys_platform_data bd718xx_powerkey_data = {
+ .buttons = &btns[0],
+ .nbuttons = ARRAY_SIZE(btns),
+ .name = "bd718xx-pwrkey",
+};
+
+/* bd71837 multi function cells */
+
+static struct mfd_cell bd71837_mfd_cells[] = {
+ {
+ .name = "bd71837-clk",
+ }, {
+ .name = "gpio-keys",
+ .platform_data = &bd718xx_powerkey_data,
+ .pdata_size = sizeof(bd718xx_powerkey_data),
+ }, {
+ .name = "bd71837-pmic",
+ }
+};
+
+static const struct regmap_irq bd71837_irqs[] = {
+ REGMAP_IRQ_REG(BD71837_INT_SWRST, 0, BD71837_INT_SWRST_MASK),
+ REGMAP_IRQ_REG(BD71837_INT_PWRBTN_S, 0, BD71837_INT_PWRBTN_S_MASK),
+ REGMAP_IRQ_REG(BD71837_INT_PWRBTN_L, 0, BD71837_INT_PWRBTN_L_MASK),
+ REGMAP_IRQ_REG(BD71837_INT_PWRBTN, 0, BD71837_INT_PWRBTN_MASK),
+ REGMAP_IRQ_REG(BD71837_INT_WDOG, 0, BD71837_INT_WDOG_MASK),
+ REGMAP_IRQ_REG(BD71837_INT_ON_REQ, 0, BD71837_INT_ON_REQ_MASK),
+ REGMAP_IRQ_REG(BD71837_INT_STBY_REQ, 0, BD71837_INT_STBY_REQ_MASK),
+};
+
+static struct regmap_irq_chip bd71837_irq_chip = {
+ .name = "bd71837-irq",
+ .irqs = bd71837_irqs,
+ .num_irqs = ARRAY_SIZE(bd71837_irqs),
+ .num_regs = 1,
+ .irq_reg_stride = 1,
+ .status_base = BD71837_REG_IRQ,
+ .mask_base = BD71837_REG_MIRQ,
+ .ack_base = BD71837_REG_IRQ,
+ .init_ack_masked = true,
+ .mask_invert = false,
+};
+
+static const struct regmap_range pmic_status_range = {
+ .range_min = BD71837_REG_IRQ,
+ .range_max = BD71837_REG_POW_STATE,
+};
+
+static const struct regmap_access_table volatile_regs = {
+ .yes_ranges = &pmic_status_range,
+ .n_yes_ranges = 1,
+};
+
+static const struct regmap_config bd71837_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .volatile_table = &volatile_regs,
+ .max_register = BD71837_MAX_REGISTER - 1,
+ .cache_type = REGCACHE_RBTREE,
+};
+
+static const struct of_device_id bd71837_of_match[] = {
+ { .compatible = "rohm,bd71837", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, bd71837_of_match);
+
+static int bd71837_i2c_probe(struct i2c_client *i2c,
+ const struct i2c_device_id *id)
+{
+ struct bd71837 *bd71837;
+ struct bd71837_board *board_info;
+ int ret = -EINVAL;
+
+ board_info = dev_get_platdata(&i2c->dev);
+
+ if (!board_info) {
+ board_info = devm_kzalloc(&i2c->dev, sizeof(*board_info),
+ GFP_KERNEL);
+ if (!board_info) {
+ ret = -ENOMEM;
+ goto err_out;
+ } else if (i2c->irq) {
+ board_info->gpio_intr = i2c->irq;
+ } else {
+ ret = -ENOENT;
+ goto err_out;
+ }
+ }
+
+ if (!board_info)
+ goto err_out;
+
+ bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
+ if (bd71837 == NULL)
+ return -ENOMEM;
+
+ i2c_set_clientdata(i2c, bd71837);
+ bd71837->dev = &i2c->dev;
+ bd71837->i2c_client = i2c;
+ bd71837->chip_irq = board_info->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 = devm_regmap_add_irq_chip(&i2c->dev, bd71837->regmap,
+ bd71837->chip_irq, IRQF_ONESHOT, 0,
+ &bd71837_irq_chip, &bd71837->irq_data);
+ if (ret < 0) {
+ dev_err(bd71837->dev, "Failed to add irq_chip %d\n", ret);
+ goto err_out;
+ }
+
+ ret = regmap_update_bits(bd71837->regmap,
+ BD71837_REG_PWRONCONFIG0,
+ BD718XX_PWRBTN_PRESS_DURATION_MASK,
+ BD718XX_PWRBTN_SHORT_PRESS_10MS);
+ if (ret < 0) {
+ dev_err(bd71837->dev,
+ "Failed to configure button short press timeout %d\n",
+ ret);
+ goto err_out;
+ }
+ /* According to BD71847 datasheet the HW default for long press
+ * detection is 10ms. So lets change it to 10 sec so we can actually
+ * get the short push and allow gracefull shut down
+ */
+ ret = regmap_update_bits(bd71837->regmap,
+ BD71837_REG_PWRONCONFIG1,
+ BD718XX_PWRBTN_PRESS_DURATION_MASK,
+ BD718XX_PWRBTN_LONG_PRESS_10S);
+ if (ret < 0) {
+ dev_err(bd71837->dev,
+ "Failed to configure button long press timeout %d\n",
+ ret);
+ goto err_out;
+ }
+ btns[0].irq = regmap_irq_get_virq(bd71837->irq_data,
+ BD71837_INT_PWRBTN_S);
+
+ if (btns[0].irq < 0) {
+ ret = btns[0].irq;
+ goto err_out;
+ }
+
+ ret = devm_mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
+ bd71837_mfd_cells,
+ ARRAY_SIZE(bd71837_mfd_cells), NULL, 0,
+ regmap_irq_get_domain(bd71837->irq_data));
+err_out:
+
+ return ret;
+}
+
+static const struct i2c_device_id bd71837_i2c_id[] = {
+ { .name = "bd71837", },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id);
+
+static struct i2c_driver bd71837_i2c_driver = {
+ .driver = {
+ .name = "bd71837-mfd",
+ .of_match_table = bd71837_of_match,
+ },
+ .probe = bd71837_i2c_probe,
+ .id_table = bd71837_i2c_id,
+};
+
+static int __init bd71837_i2c_init(void)
+{
+ return i2c_add_driver(&bd71837_i2c_driver);
+}
+/* init early so consumer devices can complete system boot */
+subsys_initcall(bd71837_i2c_init);
+
+static void __exit bd71837_i2c_exit(void)
+{
+ i2c_del_driver(&bd71837_i2c_driver);
+}
+module_exit(bd71837_i2c_exit);
+
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_DESCRIPTION("BD71837 chip multi-function driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/bd71837.h b/include/linux/mfd/bd71837.h
new file mode 100644
index 000000000000..10b0dfe90f27
--- /dev/null
+++ b/include/linux/mfd/bd71837.h
@@ -0,0 +1,391 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/* Copyright (C) 2018 ROHM Semiconductors */
+
+/*
+ * ROHM BD71837MWV header file
+ */
+
+#ifndef __LINUX_MFD_BD71837_H__
+#define __LINUX_MFD_BD71837_H__
+
+#include <linux/regmap.h>
+
+enum {
+ BD71837_BUCK1 = 0,
+ BD71837_BUCK2,
+ BD71837_BUCK3,
+ BD71837_BUCK4,
+ BD71837_BUCK5,
+ BD71837_BUCK6,
+ BD71837_BUCK7,
+ BD71837_BUCK8,
+ BD71837_LDO1,
+ BD71837_LDO2,
+ BD71837_LDO3,
+ BD71837_LDO4,
+ BD71837_LDO5,
+ BD71837_LDO6,
+ BD71837_LDO7,
+ BD71837_REGULATOR_CNT,
+};
+
+#define BD71837_BUCK1_VOLTAGE_NUM 0x40
+#define BD71837_BUCK2_VOLTAGE_NUM 0x40
+#define BD71837_BUCK3_VOLTAGE_NUM 0x40
+#define BD71837_BUCK4_VOLTAGE_NUM 0x40
+
+#define BD71837_BUCK5_VOLTAGE_NUM 0x08
+#define BD71837_BUCK6_VOLTAGE_NUM 0x04
+#define BD71837_BUCK7_VOLTAGE_NUM 0x08
+#define BD71837_BUCK8_VOLTAGE_NUM 0x40
+
+#define BD71837_LDO1_VOLTAGE_NUM 0x04
+#define BD71837_LDO2_VOLTAGE_NUM 0x02
+#define BD71837_LDO3_VOLTAGE_NUM 0x10
+#define BD71837_LDO4_VOLTAGE_NUM 0x10
+#define BD71837_LDO5_VOLTAGE_NUM 0x10
+#define BD71837_LDO6_VOLTAGE_NUM 0x10
+#define BD71837_LDO7_VOLTAGE_NUM 0x10
+
+enum {
+ BD71837_REG_REV = 0x00,
+ BD71837_REG_SWRESET = 0x01,
+ BD71837_REG_I2C_DEV = 0x02,
+ BD71837_REG_PWRCTRL0 = 0x03,
+ BD71837_REG_PWRCTRL1 = 0x04,
+ BD71837_REG_BUCK1_CTRL = 0x05,
+ BD71837_REG_BUCK2_CTRL = 0x06,
+ BD71837_REG_BUCK3_CTRL = 0x07,
+ BD71837_REG_BUCK4_CTRL = 0x08,
+ BD71837_REG_BUCK5_CTRL = 0x09,
+ BD71837_REG_BUCK6_CTRL = 0x0A,
+ BD71837_REG_BUCK7_CTRL = 0x0B,
+ BD71837_REG_BUCK8_CTRL = 0x0C,
+ BD71837_REG_BUCK1_VOLT_RUN = 0x0D,
+ BD71837_REG_BUCK1_VOLT_IDLE = 0x0E,
+ BD71837_REG_BUCK1_VOLT_SUSP = 0x0F,
+ BD71837_REG_BUCK2_VOLT_RUN = 0x10,
+ BD71837_REG_BUCK2_VOLT_IDLE = 0x11,
+ BD71837_REG_BUCK3_VOLT_RUN = 0x12,
+ BD71837_REG_BUCK4_VOLT_RUN = 0x13,
+ BD71837_REG_BUCK5_VOLT = 0x14,
+ BD71837_REG_BUCK6_VOLT = 0x15,
+ BD71837_REG_BUCK7_VOLT = 0x16,
+ BD71837_REG_BUCK8_VOLT = 0x17,
+ BD71837_REG_LDO1_VOLT = 0x18,
+ BD71837_REG_LDO2_VOLT = 0x19,
+ BD71837_REG_LDO3_VOLT = 0x1A,
+ BD71837_REG_LDO4_VOLT = 0x1B,
+ BD71837_REG_LDO5_VOLT = 0x1C,
+ BD71837_REG_LDO6_VOLT = 0x1D,
+ BD71837_REG_LDO7_VOLT = 0x1E,
+ BD71837_REG_TRANS_COND0 = 0x1F,
+ BD71837_REG_TRANS_COND1 = 0x20,
+ BD71837_REG_VRFAULTEN = 0x21,
+ BD71837_REG_MVRFLTMASK0 = 0x22,
+ BD71837_REG_MVRFLTMASK1 = 0x23,
+ BD71837_REG_MVRFLTMASK2 = 0x24,
+ BD71837_REG_RCVCFG = 0x25,
+ BD71837_REG_RCVNUM = 0x26,
+ BD71837_REG_PWRONCONFIG0 = 0x27,
+ BD71837_REG_PWRONCONFIG1 = 0x28,
+ BD71837_REG_RESETSRC = 0x29,
+ BD71837_REG_MIRQ = 0x2A,
+ BD71837_REG_IRQ = 0x2B,
+ BD71837_REG_IN_MON = 0x2C,
+ BD71837_REG_POW_STATE = 0x2D,
+ BD71837_REG_OUT32K = 0x2E,
+ BD71837_REG_REGLOCK = 0x2F,
+ BD71837_REG_OTPVER = 0xFF,
+ BD71837_MAX_REGISTER = 0x100,
+};
+
+#define REGLOCK_PWRSEQ 0x1
+#define REGLOCK_VREG 0x10
+
+/* Generic BUCK control masks */
+#define BD71837_BUCK_SEL 0x02
+#define BD71837_BUCK_EN 0x01
+#define BD71837_BUCK_RUN_ON 0x04
+
+/* Generic LDO masks */
+#define BD71837_LDO_SEL 0x80
+#define BD71837_LDO_EN 0x40
+
+/* BD71837 BUCK ramp rate CTRL reg bits */
+#define BUCK_RAMPRATE_MASK 0xC0
+#define BUCK_RAMPRATE_10P00MV 0x0
+#define BUCK_RAMPRATE_5P00MV 0x1
+#define BUCK_RAMPRATE_2P50MV 0x2
+#define BUCK_RAMPRATE_1P25MV 0x3
+
+/* BD71837_REG_BUCK1_VOLT_RUN bits */
+#define BUCK1_RUN_MASK 0x3F
+#define BUCK1_RUN_DEFAULT 0x14
+
+/* BD71837_REG_BUCK1_VOLT_SUSP bits */
+#define BUCK1_SUSP_MASK 0x3F
+#define BUCK1_SUSP_DEFAULT 0x14
+
+/* BD71837_REG_BUCK1_VOLT_IDLE bits */
+#define BUCK1_IDLE_MASK 0x3F
+#define BUCK1_IDLE_DEFAULT 0x14
+
+/* BD71837_REG_BUCK2_VOLT_RUN bits */
+#define BUCK2_RUN_MASK 0x3F
+#define BUCK2_RUN_DEFAULT 0x1E
+
+/* BD71837_REG_BUCK2_VOLT_IDLE bits */
+#define BUCK2_IDLE_MASK 0x3F
+#define BUCK2_IDLE_DEFAULT 0x14
+
+/* BD71837_REG_BUCK3_VOLT_RUN bits */
+#define BUCK3_RUN_MASK 0x3F
+#define BUCK3_RUN_DEFAULT 0x1E
+
+/* BD71837_REG_BUCK4_VOLT_RUN bits */
+#define BUCK4_RUN_MASK 0x3F
+#define BUCK4_RUN_DEFAULT 0x1E
+
+/* BD71837_REG_BUCK5_VOLT bits */
+#define BUCK5_MASK 0x07
+#define BUCK5_DEFAULT 0x02
+
+/* BD71837_REG_BUCK6_VOLT bits */
+#define BUCK6_MASK 0x03
+#define BUCK6_DEFAULT 0x03
+
+/* BD71837_REG_BUCK7_VOLT bits */
+#define BUCK7_MASK 0x07
+#define BUCK7_DEFAULT 0x03
+
+/* BD71837_REG_BUCK8_VOLT bits */
+#define BUCK8_MASK 0x3F
+#define BUCK8_DEFAULT 0x1E
+
+/* BD71837_REG_IRQ bits */
+#define IRQ_SWRST 0x40
+#define IRQ_PWRON_S 0x20
+#define IRQ_PWRON_L 0x10
+#define IRQ_PWRON 0x08
+#define IRQ_WDOG 0x04
+#define IRQ_ON_REQ 0x02
+#define IRQ_STBY_REQ 0x01
+
+/* BD71837_REG_OUT32K bits */
+#define BD71837_OUT32K_EN 0x01
+
+/* BD71837 gated clock rate */
+#define BD71837_CLK_RATE 32768
+
+/* BD71837 irqs */
+enum {
+ BD71837_INT_STBY_REQ,
+ BD71837_INT_ON_REQ,
+ BD71837_INT_WDOG,
+ BD71837_INT_PWRBTN,
+ BD71837_INT_PWRBTN_L,
+ BD71837_INT_PWRBTN_S,
+ BD71837_INT_SWRST
+};
+
+/* BD71837 interrupt masks */
+#define BD71837_INT_SWRST_MASK 0x40
+#define BD71837_INT_PWRBTN_S_MASK 0x20
+#define BD71837_INT_PWRBTN_L_MASK 0x10
+#define BD71837_INT_PWRBTN_MASK 0x8
+#define BD71837_INT_WDOG_MASK 0x4
+#define BD71837_INT_ON_REQ_MASK 0x2
+#define BD71837_INT_STBY_REQ_MASK 0x1
+
+/* BD71837_REG_LDO1_VOLT bits */
+#define LDO1_MASK 0x03
+
+/* BD71837_REG_LDO1_VOLT bits */
+#define LDO2_MASK 0x20
+
+/* BD71837_REG_LDO3_VOLT bits */
+#define LDO3_MASK 0x0F
+
+/* BD71837_REG_LDO4_VOLT bits */
+#define LDO4_MASK 0x0F
+
+/* BD71837_REG_LDO5_VOLT bits */
+#define LDO5_MASK 0x0F
+
+/* BD71837_REG_LDO6_VOLT bits */
+#define LDO6_MASK 0x0F
+
+/* BD71837_REG_LDO7_VOLT bits */
+#define LDO7_MASK 0x0F
+
+/* Register write induced reset settings */
+
+/* Even though the bit zero is not SWRESET type we still want to write zero
+ * to it when changing type. Bit zero is 'SWRESET' trigger bit and if we
+ * write 1 to it we will trigger the action. So always write 0 to it when
+ * changning SWRESET action - no matter what we read from it.
+ */
+#define BD71837_SWRESET_TYPE_MASK 7
+#define BD71837_SWRESET_TYPE_DISABLED 0
+#define BD71837_SWRESET_TYPE_COLD 4
+#define BD71837_SWRESET_TYPE_WARM 6
+
+#define BD71837_SWRESET_RESET_MASK 1
+#define BD71837_SWRESET_RESET 1
+
+/* Poweroff state transition conditions */
+
+#define BD718XX_ON_REQ_POWEROFF_MASK 1
+#define BD718XX_SWRESET_POWEROFF_MASK 2
+#define BD718XX_WDOG_POWEROFF_MASK 4
+#define BD718XX_KEY_L_POWEROFF_MASK 8
+
+#define BD718XX_POWOFF_TO_SNVS 0
+#define BD718XX_POWOFF_TO_RDY 0xF
+
+#define BD718XX_POWOFF_TIME_MASK 0xF0
+enum {
+ BD718XX_POWOFF_TIME_5MS = 0,
+ BD718XX_POWOFF_TIME_10MS,
+ BD718XX_POWOFF_TIME_15MS,
+ BD718XX_POWOFF_TIME_20MS,
+ BD718XX_POWOFF_TIME_25MS,
+ BD718XX_POWOFF_TIME_30MS,
+ BD718XX_POWOFF_TIME_35MS,
+ BD718XX_POWOFF_TIME_40MS,
+ BD718XX_POWOFF_TIME_45MS,
+ BD718XX_POWOFF_TIME_50MS,
+ BD718XX_POWOFF_TIME_75MS,
+ BD718XX_POWOFF_TIME_100MS,
+ BD718XX_POWOFF_TIME_250MS,
+ BD718XX_POWOFF_TIME_500MS,
+ BD718XX_POWOFF_TIME_750MS,
+ BD718XX_POWOFF_TIME_1500MS
+};
+
+/* Poweron sequence state transition conditions */
+
+#define BD718XX_RDY_TO_SNVS_MASK 0xF
+#define BD718XX_SNVS_TO_RUN_MASK 0xF0
+
+#define BD718XX_PWR_TRIG_KEY_L 1
+#define BD718XX_PWR_TRIG_KEY_S 2
+#define BD718XX_PWR_TRIG_PMIC_ON 4
+#define BD718XX_PWR_TRIG_VSYS_UVLO 8
+#define BD718XX_RDY_TO_SNVS_SIFT 0
+#define BD718XX_SNVS_TO_RUN_SIFT 4
+
+
+#define BD718XX_PWRBTN_PRESS_DURATION_MASK 0xF
+
+/* Timeout value for detecting short press */
+
+enum {
+ BD718XX_PWRBTN_SHORT_PRESS_10MS = 0,
+ BD718XX_PWRBTN_SHORT_PRESS_500MS,
+ BD718XX_PWRBTN_SHORT_PRESS_1000MS,
+ BD718XX_PWRBTN_SHORT_PRESS_1500MS,
+ BD718XX_PWRBTN_SHORT_PRESS_2000MS,
+ BD718XX_PWRBTN_SHORT_PRESS_2500MS,
+ BD718XX_PWRBTN_SHORT_PRESS_3000MS,
+ BD718XX_PWRBTN_SHORT_PRESS_3500MS,
+ BD718XX_PWRBTN_SHORT_PRESS_4000MS,
+ BD718XX_PWRBTN_SHORT_PRESS_4500MS,
+ BD718XX_PWRBTN_SHORT_PRESS_5000MS,
+ BD718XX_PWRBTN_SHORT_PRESS_5500MS,
+ BD718XX_PWRBTN_SHORT_PRESS_6000MS,
+ BD718XX_PWRBTN_SHORT_PRESS_6500MS,
+ BD718XX_PWRBTN_SHORT_PRESS_7000MS,
+ BD718XX_PWRBTN_SHORT_PRESS_7500MS
+};
+
+/* Timeout value for detecting LONG press */
+
+enum {
+ BD718XX_PWRBTN_LONG_PRESS_10MS = 0,
+ BD718XX_PWRBTN_LONG_PRESS_1S,
+ BD718XX_PWRBTN_LONG_PRESS_2S,
+ BD718XX_PWRBTN_LONG_PRESS_3S,
+ BD718XX_PWRBTN_LONG_PRESS_4S,
+ BD718XX_PWRBTN_LONG_PRESS_5S,
+ BD718XX_PWRBTN_LONG_PRESS_6S,
+ BD718XX_PWRBTN_LONG_PRESS_7S,
+ BD718XX_PWRBTN_LONG_PRESS_8S,
+ BD718XX_PWRBTN_LONG_PRESS_9S,
+ BD718XX_PWRBTN_LONG_PRESS_10S,
+ BD718XX_PWRBTN_LONG_PRESS_11S,
+ BD718XX_PWRBTN_LONG_PRESS_12S,
+ BD718XX_PWRBTN_LONG_PRESS_13S,
+ BD718XX_PWRBTN_LONG_PRESS_14S,
+ BD718XX_PWRBTN_LONG_PRESS_15S
+};
+
+
+
+struct bd71837;
+struct bd71837_pmic;
+struct bd71837_clk;
+struct bd718xx_pwrkey;
+
+/*
+ * Board platform data may be used to initialize regulators.
+ */
+
+struct bd71837_board {
+ struct regulator_init_data *init_data[BD71837_REGULATOR_CNT];
+ int gpio_intr;
+};
+
+struct bd71837 {
+ struct device *dev;
+ struct i2c_client *i2c_client;
+ struct regmap *regmap;
+ unsigned long int id;
+
+ int chip_irq;
+ struct regmap_irq_chip_data *irq_data;
+
+ struct bd71837_pmic *pmic;
+ struct bd71837_clk *clk;
+ struct bd718xx_pwrkey *pwrkey;
+};
+
+/*
+ * bd71837 sub-driver chip access routines
+ */
+
+static inline int bd71837_reg_read(struct bd71837 *bd71837, u8 reg)
+{
+ int r, val;
+
+ r = regmap_read(bd71837->regmap, reg, &val);
+ if (r < 0)
+ return r;
+ return val;
+}
+
+static inline int bd71837_reg_write(struct bd71837 *bd71837, u8 reg,
+ unsigned int val)
+{
+ return regmap_write(bd71837->regmap, reg, val);
+}
+
+static inline int bd71837_set_bits(struct bd71837 *bd71837, u8 reg, u8 mask)
+{
+ return regmap_update_bits(bd71837->regmap, reg, mask, mask);
+}
+
+static inline int bd71837_clear_bits(struct bd71837 *bd71837, u8 reg,
+ u8 mask)
+{
+ return regmap_update_bits(bd71837->regmap, reg, mask, 0);
+}
+
+static inline int bd71837_update_bits(struct bd71837 *bd71837, u8 reg,
+ u8 mask, u8 val)
+{
+ return regmap_update_bits(bd71837->regmap, reg, mask, val);
+}
+
+#endif /* __LINUX_MFD_BD71837_H__ */
--
2.14.3
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v8 2/2] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
2018-06-29 8:20 [PATCH v8 0/2] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver Matti Vaittinen
2018-06-29 8:21 ` [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC Matti Vaittinen
@ 2018-06-29 8:23 ` Matti Vaittinen
2018-06-29 8:26 ` Matti Vaittinen
1 sibling, 1 reply; 16+ messages in thread
From: Matti Vaittinen @ 2018-06-29 8:23 UTC (permalink / raw)
To: mturquette, robh+dt, mark.rutland, lee.jones, lgirdwood, broonie,
mazziesaccount, arnd, dmitry.torokhov, sre, chenjh,
andrew.smirnov, linus.walleij, kstewart, heiko, gregkh
Cc: sboyd, linux-clk, devicetree, linux-kernel, linux-input,
mikko.mutanen, heikki.haikola
Document devicetree bindings for ROHM BD71837 PMIC MFD.
Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
.../devicetree/bindings/mfd/rohm,bd71837-pmic.txt | 67 ++++++++++++++++++++++
1 file changed, 67 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
new file mode 100644
index 000000000000..67f2616288d9
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
@@ -0,0 +1,67 @@
+* ROHM BD71837 Power Management Integrated Circuit bindings
+
+BD71837MWV is a programmable Power Management IC for powering single-core,
+dual-core, and quad-core SoC’s such as NXP-i.MX 8M. It is optimized for
+low BOM cost and compact solution footprint. It integrates 8 Buck
+egulators and 7 LDO’s to provide all the power rails required by the SoC and
+the commonly used peripherals.
+
+Datasheet for PMIC is available at:
+https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
+
+Required properties:
+ - compatible : Should be "rohm,bd71837".
+ - reg : I2C slave address.
+ - interrupt-parent : Phandle to the parent interrupt controller.
+ - interrupts : The interrupt line the device is connected to.
+ - clocks : The parent clock connected to PMIC. If this is missng
+ 32768 KHz clock is assumed.
+ - #clock-cells : Should be 0
+ - regulators: : List of child nodes that specify the regulators
+ Please see ../regulator/rohm,bd71837-regulator.txt
+
+Optional properties:
+- clock-output-names : Should contain name for output clock.
+
+Example:
+
+ /* external oscillator node */
+ osc: oscillator {
+ compatible = "fixed-clock";
+ #clock-cells = <1>;
+ clock-frequency = <32768>;
+ clock-output-names = "osc";
+ };
+
+ /* PMIC node */
+
+ pmic: pmic@4b {
+ compatible = "rohm,bd71837";
+ reg = <0x4b>;
+ interrupt-parent = <&gpio1>;
+ interrupts = <29 GPIO_ACTIVE_LOW>;
+ interrupt-names = "irq";
+ #clock-cells = <0>;
+ clocks = <&osc 0>;
+ clock-output-names = "bd71837-32k-out";
+
+ regulators {
+ buck1: BUCK1 {
+ regulator-name = "buck1";
+ regulator-min-microvolt = <700000>;
+ regulator-max-microvolt = <1300000>;
+ regulator-boot-on;
+ regulator-ramp-delay = <1250>;
+ };
+ /* ... */
+ };
+ };
+
+ /* Clock consumer node */
+
+ foo@0 {
+ compatible = "bar,foo";
+ /* ... */
+ clock-names = "my-clock";
+ clocks = <&pmic>;
+ };
--
2.14.3
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v8 2/2] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
2018-06-29 8:23 ` [PATCH v8 2/2] mfd: bd71837: Devicetree bindings " Matti Vaittinen
@ 2018-06-29 8:26 ` Matti Vaittinen
0 siblings, 0 replies; 16+ messages in thread
From: Matti Vaittinen @ 2018-06-29 8:26 UTC (permalink / raw)
To: mturquette, robh+dt, mark.rutland, lee.jones, lgirdwood, broonie,
mazziesaccount, arnd, dmitry.torokhov, sre, chenjh,
andrew.smirnov, linus.walleij, kstewart, heiko, gregkh
Cc: sboyd, linux-clk, devicetree, linux-kernel, linux-input,
mikko.mutanen, heikki.haikola
On Fri, Jun 29, 2018 at 11:23:42AM +0300, Matti Vaittinen wrote:
> Document devicetree bindings for ROHM BD71837 PMIC MFD.
>
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
I kept the Reviewed-by: tag here as there is no changes from patch v6
which was reviewed by Rob - hopefully this was not a violation of any
sort...
Best Regards
Matti Vaittinen
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-06-29 8:21 ` [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC Matti Vaittinen
@ 2018-07-03 9:26 ` Enric Balletbo Serra
0 siblings, 0 replies; 16+ messages in thread
From: Enric Balletbo Serra @ 2018-07-03 9:26 UTC (permalink / raw)
To: matti.vaittinen
Cc: Michael Turquette, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, Matti Vaittinen, Arnd Bergmann,
Dmitry Torokhov, Sebastian Reichel, chenjh, Andrey Smirnov,
Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, sboyd, linux-clk, devicetree, linux-kernel,
linux-input, mikko.mutanen, heikki.haikola
Hi Matti,
One doubt regarding the probe function and few comments.
Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
dia dv., 29 de juny 2018 a les 11:47:
>
> ROHM BD71837 PMIC MFD driver providing interrupts and support
> for three subsystems:
> - clk
> - Regulators
> - input/power-key
>
> fix too long lines
I guess that this message is not intended to be here.
>
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
> drivers/mfd/Kconfig | 13 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/bd71837.c | 220 +++++++++++++++++++++++++
> include/linux/mfd/bd71837.h | 391 ++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 625 insertions(+)
> create mode 100644 drivers/mfd/bd71837.c
> create mode 100644 include/linux/mfd/bd71837.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b860eb5aa194..d01a279f5e4a 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1787,6 +1787,19 @@ config MFD_STW481X
> in various ST Microelectronics and ST-Ericsson embedded
> Nomadik series.
>
> +config MFD_BD71837
> + tristate "BD71837 Power Management chip"
> + depends on I2C=y
> + depends on OF
> + select REGMAP_I2C
> + select REGMAP_IRQ
> + select MFD_CORE
> + help
> + Select this option to get support for the ROHM BD71837
> + Power Management chips. BD71837 is designed to power processors like
> + NXP i.MX8. It contains 8 BUCK outputs and 7 LDOs, voltage monitoring
> + and emergency shut down as well as 32,768KHz clock output.
> +
> config MFD_STM32_LPTIMER
> tristate "Support for STM32 Low-Power Timer"
> depends on (ARCH_STM32 && OF) || COMPILE_TEST
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index e9fd20dba18d..09dc9eb3782c 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -227,4 +227,5 @@ obj-$(CONFIG_MFD_STM32_TIMERS) += stm32-timers.o
> obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o
> obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o
> obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
> +obj-$(CONFIG_MFD_BD71837) += bd71837.o
>
> diff --git a/drivers/mfd/bd71837.c b/drivers/mfd/bd71837.c
> new file mode 100644
> index 000000000000..25f2de2a5bb8
> --- /dev/null
> +++ b/drivers/mfd/bd71837.c
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// Copyright (C) 2018 ROHM Semiconductors
> +// bd71837.c -- ROHM BD71837MWV mfd driver
> +//
> +// Datasheet available from
> +// https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
> +
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/bd71837.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/gpio_keys.h>
> +
> +static struct gpio_keys_button btns[] = {
> + {
> + .code = KEY_POWER,
> + .gpio = -1,
> + .type = EV_KEY,
> + }
> +};
> +
> +static struct gpio_keys_platform_data bd718xx_powerkey_data = {
> + .buttons = &btns[0],
> + .nbuttons = ARRAY_SIZE(btns),
> + .name = "bd718xx-pwrkey",
> +};
> +
> +/* bd71837 multi function cells */
> +
> +static struct mfd_cell bd71837_mfd_cells[] = {
> + {
> + .name = "bd71837-clk",
> + }, {
> + .name = "gpio-keys",
> + .platform_data = &bd718xx_powerkey_data,
> + .pdata_size = sizeof(bd718xx_powerkey_data),
> + }, {
> + .name = "bd71837-pmic",
> + }
> +};
> +
> +static const struct regmap_irq bd71837_irqs[] = {
> + REGMAP_IRQ_REG(BD71837_INT_SWRST, 0, BD71837_INT_SWRST_MASK),
> + REGMAP_IRQ_REG(BD71837_INT_PWRBTN_S, 0, BD71837_INT_PWRBTN_S_MASK),
> + REGMAP_IRQ_REG(BD71837_INT_PWRBTN_L, 0, BD71837_INT_PWRBTN_L_MASK),
> + REGMAP_IRQ_REG(BD71837_INT_PWRBTN, 0, BD71837_INT_PWRBTN_MASK),
> + REGMAP_IRQ_REG(BD71837_INT_WDOG, 0, BD71837_INT_WDOG_MASK),
> + REGMAP_IRQ_REG(BD71837_INT_ON_REQ, 0, BD71837_INT_ON_REQ_MASK),
> + REGMAP_IRQ_REG(BD71837_INT_STBY_REQ, 0, BD71837_INT_STBY_REQ_MASK),
> +};
> +
> +static struct regmap_irq_chip bd71837_irq_chip = {
> + .name = "bd71837-irq",
> + .irqs = bd71837_irqs,
> + .num_irqs = ARRAY_SIZE(bd71837_irqs),
> + .num_regs = 1,
> + .irq_reg_stride = 1,
> + .status_base = BD71837_REG_IRQ,
> + .mask_base = BD71837_REG_MIRQ,
> + .ack_base = BD71837_REG_IRQ,
> + .init_ack_masked = true,
> + .mask_invert = false,
> +};
> +
> +static const struct regmap_range pmic_status_range = {
> + .range_min = BD71837_REG_IRQ,
> + .range_max = BD71837_REG_POW_STATE,
> +};
> +
> +static const struct regmap_access_table volatile_regs = {
> + .yes_ranges = &pmic_status_range,
> + .n_yes_ranges = 1,
> +};
> +
> +static const struct regmap_config bd71837_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .volatile_table = &volatile_regs,
> + .max_register = BD71837_MAX_REGISTER - 1,
> + .cache_type = REGCACHE_RBTREE,
> +};
> +
> +static const struct of_device_id bd71837_of_match[] = {
> + { .compatible = "rohm,bd71837", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, bd71837_of_match);
> +
> +static int bd71837_i2c_probe(struct i2c_client *i2c,
> + const struct i2c_device_id *id)
> +{
> + struct bd71837 *bd71837;
> + struct bd71837_board *board_info;
> + int ret = -EINVAL;
> +
> + board_info = dev_get_platdata(&i2c->dev);
Sorry in advance if I am missing something, but isn't this always NULL?
> +
> + if (!board_info) {
then this check is not required
> + board_info = devm_kzalloc(&i2c->dev, sizeof(*board_info),
> + GFP_KERNEL);
> + if (!board_info) {
> + ret = -ENOMEM;
> + goto err_out;
Now that you use devm calls and you don't need to unwind things I
think is better to use plain returns. So,
return -ENOMEM;
> + } else if (i2c->irq) {
IMO this else is confusing, also maybe you want to warn about the
missing interrupt.
if (!i2c->irq) {
dev_err(&i2c->dev, "No IRQ configured!);
return -EINVAL;
}
> + board_info->gpio_intr = i2c->irq;
Is board_info really necessary?
> + } else {
> + ret = -ENOENT;
> + goto err_out;
> + }
and remove the else
> + }
> +
> + if (!board_info)
> + goto err_out;
> +
This is redundant.
> + bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
> + if (bd71837 == NULL)
if (!bd71837)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(i2c, bd71837);
> + bd71837->dev = &i2c->dev;
> + bd71837->i2c_client = i2c;
> + bd71837->chip_irq = board_info->gpio_intr;
bd71837->chip_irq = i2c->irq;
> +
> + 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;
dev_err(bd71837->dev, "regmap initialization failed: %d\n", ret);
return PTR_ERR(bd71837->regmap);
> + }
> +
> + ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
> + if (ret < 0) {
> + dev_err(bd71837->dev,
> + "%s(): Read BD71837_REG_DEVICE failed!\n", __func__);
__func__ part is redundant.
> + goto err_out;
return ret;
> + }
> +
> + ret = devm_regmap_add_irq_chip(&i2c->dev, bd71837->regmap,
> + bd71837->chip_irq, IRQF_ONESHOT, 0,
> + &bd71837_irq_chip, &bd71837->irq_data);
> + if (ret < 0) {
> + dev_err(bd71837->dev, "Failed to add irq_chip %d\n", ret);
> + goto err_out;
return ret;
> + }
> +
> + ret = regmap_update_bits(bd71837->regmap,
> + BD71837_REG_PWRONCONFIG0,
> + BD718XX_PWRBTN_PRESS_DURATION_MASK,
> + BD718XX_PWRBTN_SHORT_PRESS_10MS);
> + if (ret < 0) {
> + dev_err(bd71837->dev,
> + "Failed to configure button short press timeout %d\n",
> + ret);
> + goto err_out;
return ret;
> + }
> + /* According to BD71847 datasheet the HW default for long press
> + * detection is 10ms. So lets change it to 10 sec so we can actually
> + * get the short push and allow gracefull shut down
> + */
> + ret = regmap_update_bits(bd71837->regmap,
> + BD71837_REG_PWRONCONFIG1,
> + BD718XX_PWRBTN_PRESS_DURATION_MASK,
> + BD718XX_PWRBTN_LONG_PRESS_10S);
> + if (ret < 0) {
> + dev_err(bd71837->dev,
> + "Failed to configure button long press timeout %d\n",
> + ret);
> + goto err_out;
return ret;
> + }
> + btns[0].irq = regmap_irq_get_virq(bd71837->irq_data,
> + BD71837_INT_PWRBTN_S);
> +
> + if (btns[0].irq < 0) {
> + ret = btns[0].irq;
> + goto err_out;
return ret;
> + }
> +
> + ret = devm_mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
> + bd71837_mfd_cells,
> + ARRAY_SIZE(bd71837_mfd_cells), NULL, 0,
> + regmap_irq_get_domain(bd71837->irq_data));
> +err_out:
Remove the label.
> +
> + return ret;
> +}
> +
> +static const struct i2c_device_id bd71837_i2c_id[] = {
> + { .name = "bd71837", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id);
> +
> +static struct i2c_driver bd71837_i2c_driver = {
> + .driver = {
> + .name = "bd71837-mfd",
> + .of_match_table = bd71837_of_match,
> + },
> + .probe = bd71837_i2c_probe,
> + .id_table = bd71837_i2c_id,
> +};
> +
> +static int __init bd71837_i2c_init(void)
> +{
> + return i2c_add_driver(&bd71837_i2c_driver);
> +}
> +/* init early so consumer devices can complete system boot */
> +subsys_initcall(bd71837_i2c_init);
> +
> +static void __exit bd71837_i2c_exit(void)
> +{
> + i2c_del_driver(&bd71837_i2c_driver);
> +}
> +module_exit(bd71837_i2c_exit);
> +
> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> +MODULE_DESCRIPTION("BD71837 chip multi-function driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/bd71837.h b/include/linux/mfd/bd71837.h
> new file mode 100644
> index 000000000000..10b0dfe90f27
> --- /dev/null
> +++ b/include/linux/mfd/bd71837.h
> @@ -0,0 +1,391 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/* Copyright (C) 2018 ROHM Semiconductors */
> +
> +/*
> + * ROHM BD71837MWV header file
> + */
> +
> +#ifndef __LINUX_MFD_BD71837_H__
> +#define __LINUX_MFD_BD71837_H__
> +
> +#include <linux/regmap.h>
> +
> +enum {
> + BD71837_BUCK1 = 0,
> + BD71837_BUCK2,
> + BD71837_BUCK3,
> + BD71837_BUCK4,
> + BD71837_BUCK5,
> + BD71837_BUCK6,
> + BD71837_BUCK7,
> + BD71837_BUCK8,
> + BD71837_LDO1,
> + BD71837_LDO2,
> + BD71837_LDO3,
> + BD71837_LDO4,
> + BD71837_LDO5,
> + BD71837_LDO6,
> + BD71837_LDO7,
> + BD71837_REGULATOR_CNT,
> +};
> +
> +#define BD71837_BUCK1_VOLTAGE_NUM 0x40
> +#define BD71837_BUCK2_VOLTAGE_NUM 0x40
> +#define BD71837_BUCK3_VOLTAGE_NUM 0x40
> +#define BD71837_BUCK4_VOLTAGE_NUM 0x40
> +
> +#define BD71837_BUCK5_VOLTAGE_NUM 0x08
> +#define BD71837_BUCK6_VOLTAGE_NUM 0x04
> +#define BD71837_BUCK7_VOLTAGE_NUM 0x08
> +#define BD71837_BUCK8_VOLTAGE_NUM 0x40
> +
> +#define BD71837_LDO1_VOLTAGE_NUM 0x04
> +#define BD71837_LDO2_VOLTAGE_NUM 0x02
> +#define BD71837_LDO3_VOLTAGE_NUM 0x10
> +#define BD71837_LDO4_VOLTAGE_NUM 0x10
> +#define BD71837_LDO5_VOLTAGE_NUM 0x10
> +#define BD71837_LDO6_VOLTAGE_NUM 0x10
> +#define BD71837_LDO7_VOLTAGE_NUM 0x10
> +
> +enum {
> + BD71837_REG_REV = 0x00,
> + BD71837_REG_SWRESET = 0x01,
> + BD71837_REG_I2C_DEV = 0x02,
> + BD71837_REG_PWRCTRL0 = 0x03,
> + BD71837_REG_PWRCTRL1 = 0x04,
> + BD71837_REG_BUCK1_CTRL = 0x05,
> + BD71837_REG_BUCK2_CTRL = 0x06,
> + BD71837_REG_BUCK3_CTRL = 0x07,
> + BD71837_REG_BUCK4_CTRL = 0x08,
> + BD71837_REG_BUCK5_CTRL = 0x09,
> + BD71837_REG_BUCK6_CTRL = 0x0A,
> + BD71837_REG_BUCK7_CTRL = 0x0B,
> + BD71837_REG_BUCK8_CTRL = 0x0C,
> + BD71837_REG_BUCK1_VOLT_RUN = 0x0D,
> + BD71837_REG_BUCK1_VOLT_IDLE = 0x0E,
> + BD71837_REG_BUCK1_VOLT_SUSP = 0x0F,
> + BD71837_REG_BUCK2_VOLT_RUN = 0x10,
> + BD71837_REG_BUCK2_VOLT_IDLE = 0x11,
> + BD71837_REG_BUCK3_VOLT_RUN = 0x12,
> + BD71837_REG_BUCK4_VOLT_RUN = 0x13,
> + BD71837_REG_BUCK5_VOLT = 0x14,
> + BD71837_REG_BUCK6_VOLT = 0x15,
> + BD71837_REG_BUCK7_VOLT = 0x16,
> + BD71837_REG_BUCK8_VOLT = 0x17,
> + BD71837_REG_LDO1_VOLT = 0x18,
> + BD71837_REG_LDO2_VOLT = 0x19,
> + BD71837_REG_LDO3_VOLT = 0x1A,
> + BD71837_REG_LDO4_VOLT = 0x1B,
> + BD71837_REG_LDO5_VOLT = 0x1C,
> + BD71837_REG_LDO6_VOLT = 0x1D,
> + BD71837_REG_LDO7_VOLT = 0x1E,
> + BD71837_REG_TRANS_COND0 = 0x1F,
> + BD71837_REG_TRANS_COND1 = 0x20,
> + BD71837_REG_VRFAULTEN = 0x21,
> + BD71837_REG_MVRFLTMASK0 = 0x22,
> + BD71837_REG_MVRFLTMASK1 = 0x23,
> + BD71837_REG_MVRFLTMASK2 = 0x24,
> + BD71837_REG_RCVCFG = 0x25,
> + BD71837_REG_RCVNUM = 0x26,
> + BD71837_REG_PWRONCONFIG0 = 0x27,
> + BD71837_REG_PWRONCONFIG1 = 0x28,
> + BD71837_REG_RESETSRC = 0x29,
> + BD71837_REG_MIRQ = 0x2A,
> + BD71837_REG_IRQ = 0x2B,
> + BD71837_REG_IN_MON = 0x2C,
> + BD71837_REG_POW_STATE = 0x2D,
> + BD71837_REG_OUT32K = 0x2E,
> + BD71837_REG_REGLOCK = 0x2F,
> + BD71837_REG_OTPVER = 0xFF,
> + BD71837_MAX_REGISTER = 0x100,
> +};
> +
> +#define REGLOCK_PWRSEQ 0x1
> +#define REGLOCK_VREG 0x10
> +
> +/* Generic BUCK control masks */
> +#define BD71837_BUCK_SEL 0x02
> +#define BD71837_BUCK_EN 0x01
> +#define BD71837_BUCK_RUN_ON 0x04
> +
> +/* Generic LDO masks */
> +#define BD71837_LDO_SEL 0x80
> +#define BD71837_LDO_EN 0x40
> +
> +/* BD71837 BUCK ramp rate CTRL reg bits */
> +#define BUCK_RAMPRATE_MASK 0xC0
> +#define BUCK_RAMPRATE_10P00MV 0x0
> +#define BUCK_RAMPRATE_5P00MV 0x1
> +#define BUCK_RAMPRATE_2P50MV 0x2
> +#define BUCK_RAMPRATE_1P25MV 0x3
> +
> +/* BD71837_REG_BUCK1_VOLT_RUN bits */
> +#define BUCK1_RUN_MASK 0x3F
> +#define BUCK1_RUN_DEFAULT 0x14
> +
> +/* BD71837_REG_BUCK1_VOLT_SUSP bits */
> +#define BUCK1_SUSP_MASK 0x3F
> +#define BUCK1_SUSP_DEFAULT 0x14
> +
> +/* BD71837_REG_BUCK1_VOLT_IDLE bits */
> +#define BUCK1_IDLE_MASK 0x3F
> +#define BUCK1_IDLE_DEFAULT 0x14
> +
> +/* BD71837_REG_BUCK2_VOLT_RUN bits */
> +#define BUCK2_RUN_MASK 0x3F
> +#define BUCK2_RUN_DEFAULT 0x1E
> +
> +/* BD71837_REG_BUCK2_VOLT_IDLE bits */
> +#define BUCK2_IDLE_MASK 0x3F
> +#define BUCK2_IDLE_DEFAULT 0x14
> +
> +/* BD71837_REG_BUCK3_VOLT_RUN bits */
> +#define BUCK3_RUN_MASK 0x3F
> +#define BUCK3_RUN_DEFAULT 0x1E
> +
> +/* BD71837_REG_BUCK4_VOLT_RUN bits */
> +#define BUCK4_RUN_MASK 0x3F
> +#define BUCK4_RUN_DEFAULT 0x1E
> +
> +/* BD71837_REG_BUCK5_VOLT bits */
> +#define BUCK5_MASK 0x07
> +#define BUCK5_DEFAULT 0x02
> +
> +/* BD71837_REG_BUCK6_VOLT bits */
> +#define BUCK6_MASK 0x03
> +#define BUCK6_DEFAULT 0x03
> +
> +/* BD71837_REG_BUCK7_VOLT bits */
> +#define BUCK7_MASK 0x07
> +#define BUCK7_DEFAULT 0x03
> +
> +/* BD71837_REG_BUCK8_VOLT bits */
> +#define BUCK8_MASK 0x3F
> +#define BUCK8_DEFAULT 0x1E
> +
> +/* BD71837_REG_IRQ bits */
> +#define IRQ_SWRST 0x40
> +#define IRQ_PWRON_S 0x20
> +#define IRQ_PWRON_L 0x10
> +#define IRQ_PWRON 0x08
> +#define IRQ_WDOG 0x04
> +#define IRQ_ON_REQ 0x02
> +#define IRQ_STBY_REQ 0x01
> +
> +/* BD71837_REG_OUT32K bits */
> +#define BD71837_OUT32K_EN 0x01
> +
> +/* BD71837 gated clock rate */
> +#define BD71837_CLK_RATE 32768
> +
> +/* BD71837 irqs */
> +enum {
> + BD71837_INT_STBY_REQ,
> + BD71837_INT_ON_REQ,
> + BD71837_INT_WDOG,
> + BD71837_INT_PWRBTN,
> + BD71837_INT_PWRBTN_L,
> + BD71837_INT_PWRBTN_S,
> + BD71837_INT_SWRST
> +};
> +
> +/* BD71837 interrupt masks */
> +#define BD71837_INT_SWRST_MASK 0x40
> +#define BD71837_INT_PWRBTN_S_MASK 0x20
> +#define BD71837_INT_PWRBTN_L_MASK 0x10
> +#define BD71837_INT_PWRBTN_MASK 0x8
> +#define BD71837_INT_WDOG_MASK 0x4
> +#define BD71837_INT_ON_REQ_MASK 0x2
> +#define BD71837_INT_STBY_REQ_MASK 0x1
> +
> +/* BD71837_REG_LDO1_VOLT bits */
> +#define LDO1_MASK 0x03
> +
> +/* BD71837_REG_LDO1_VOLT bits */
> +#define LDO2_MASK 0x20
> +
> +/* BD71837_REG_LDO3_VOLT bits */
> +#define LDO3_MASK 0x0F
> +
> +/* BD71837_REG_LDO4_VOLT bits */
> +#define LDO4_MASK 0x0F
> +
> +/* BD71837_REG_LDO5_VOLT bits */
> +#define LDO5_MASK 0x0F
> +
> +/* BD71837_REG_LDO6_VOLT bits */
> +#define LDO6_MASK 0x0F
> +
> +/* BD71837_REG_LDO7_VOLT bits */
> +#define LDO7_MASK 0x0F
> +
> +/* Register write induced reset settings */
> +
> +/* Even though the bit zero is not SWRESET type we still want to write zero
> + * to it when changing type. Bit zero is 'SWRESET' trigger bit and if we
> + * write 1 to it we will trigger the action. So always write 0 to it when
> + * changning SWRESET action - no matter what we read from it.
> + */
> +#define BD71837_SWRESET_TYPE_MASK 7
> +#define BD71837_SWRESET_TYPE_DISABLED 0
> +#define BD71837_SWRESET_TYPE_COLD 4
> +#define BD71837_SWRESET_TYPE_WARM 6
> +
> +#define BD71837_SWRESET_RESET_MASK 1
> +#define BD71837_SWRESET_RESET 1
> +
> +/* Poweroff state transition conditions */
> +
> +#define BD718XX_ON_REQ_POWEROFF_MASK 1
> +#define BD718XX_SWRESET_POWEROFF_MASK 2
> +#define BD718XX_WDOG_POWEROFF_MASK 4
> +#define BD718XX_KEY_L_POWEROFF_MASK 8
> +
> +#define BD718XX_POWOFF_TO_SNVS 0
> +#define BD718XX_POWOFF_TO_RDY 0xF
> +
> +#define BD718XX_POWOFF_TIME_MASK 0xF0
> +enum {
> + BD718XX_POWOFF_TIME_5MS = 0,
> + BD718XX_POWOFF_TIME_10MS,
> + BD718XX_POWOFF_TIME_15MS,
> + BD718XX_POWOFF_TIME_20MS,
> + BD718XX_POWOFF_TIME_25MS,
> + BD718XX_POWOFF_TIME_30MS,
> + BD718XX_POWOFF_TIME_35MS,
> + BD718XX_POWOFF_TIME_40MS,
> + BD718XX_POWOFF_TIME_45MS,
> + BD718XX_POWOFF_TIME_50MS,
> + BD718XX_POWOFF_TIME_75MS,
> + BD718XX_POWOFF_TIME_100MS,
> + BD718XX_POWOFF_TIME_250MS,
> + BD718XX_POWOFF_TIME_500MS,
> + BD718XX_POWOFF_TIME_750MS,
> + BD718XX_POWOFF_TIME_1500MS
> +};
> +
> +/* Poweron sequence state transition conditions */
> +
> +#define BD718XX_RDY_TO_SNVS_MASK 0xF
> +#define BD718XX_SNVS_TO_RUN_MASK 0xF0
> +
> +#define BD718XX_PWR_TRIG_KEY_L 1
> +#define BD718XX_PWR_TRIG_KEY_S 2
> +#define BD718XX_PWR_TRIG_PMIC_ON 4
> +#define BD718XX_PWR_TRIG_VSYS_UVLO 8
> +#define BD718XX_RDY_TO_SNVS_SIFT 0
> +#define BD718XX_SNVS_TO_RUN_SIFT 4
> +
> +
> +#define BD718XX_PWRBTN_PRESS_DURATION_MASK 0xF
> +
> +/* Timeout value for detecting short press */
> +
> +enum {
> + BD718XX_PWRBTN_SHORT_PRESS_10MS = 0,
> + BD718XX_PWRBTN_SHORT_PRESS_500MS,
> + BD718XX_PWRBTN_SHORT_PRESS_1000MS,
> + BD718XX_PWRBTN_SHORT_PRESS_1500MS,
> + BD718XX_PWRBTN_SHORT_PRESS_2000MS,
> + BD718XX_PWRBTN_SHORT_PRESS_2500MS,
> + BD718XX_PWRBTN_SHORT_PRESS_3000MS,
> + BD718XX_PWRBTN_SHORT_PRESS_3500MS,
> + BD718XX_PWRBTN_SHORT_PRESS_4000MS,
> + BD718XX_PWRBTN_SHORT_PRESS_4500MS,
> + BD718XX_PWRBTN_SHORT_PRESS_5000MS,
> + BD718XX_PWRBTN_SHORT_PRESS_5500MS,
> + BD718XX_PWRBTN_SHORT_PRESS_6000MS,
> + BD718XX_PWRBTN_SHORT_PRESS_6500MS,
> + BD718XX_PWRBTN_SHORT_PRESS_7000MS,
> + BD718XX_PWRBTN_SHORT_PRESS_7500MS
> +};
> +
> +/* Timeout value for detecting LONG press */
> +
> +enum {
> + BD718XX_PWRBTN_LONG_PRESS_10MS = 0,
> + BD718XX_PWRBTN_LONG_PRESS_1S,
> + BD718XX_PWRBTN_LONG_PRESS_2S,
> + BD718XX_PWRBTN_LONG_PRESS_3S,
> + BD718XX_PWRBTN_LONG_PRESS_4S,
> + BD718XX_PWRBTN_LONG_PRESS_5S,
> + BD718XX_PWRBTN_LONG_PRESS_6S,
> + BD718XX_PWRBTN_LONG_PRESS_7S,
> + BD718XX_PWRBTN_LONG_PRESS_8S,
> + BD718XX_PWRBTN_LONG_PRESS_9S,
> + BD718XX_PWRBTN_LONG_PRESS_10S,
> + BD718XX_PWRBTN_LONG_PRESS_11S,
> + BD718XX_PWRBTN_LONG_PRESS_12S,
> + BD718XX_PWRBTN_LONG_PRESS_13S,
> + BD718XX_PWRBTN_LONG_PRESS_14S,
> + BD718XX_PWRBTN_LONG_PRESS_15S
> +};
> +
> +
> +
> +struct bd71837;
> +struct bd71837_pmic;
> +struct bd71837_clk;
> +struct bd718xx_pwrkey;
> +
> +/*
> + * Board platform data may be used to initialize regulators.
> + */
> +
> +struct bd71837_board {
> + struct regulator_init_data *init_data[BD71837_REGULATOR_CNT];
> + int gpio_intr;
> +};
> +
> +struct bd71837 {
> + struct device *dev;
> + struct i2c_client *i2c_client;
> + struct regmap *regmap;
> + unsigned long int id;
> +
> + int chip_irq;
> + struct regmap_irq_chip_data *irq_data;
> +
> + struct bd71837_pmic *pmic;
> + struct bd71837_clk *clk;
> + struct bd718xx_pwrkey *pwrkey;
> +};
> +
> +/*
> + * bd71837 sub-driver chip access routines
> + */
> +
> +static inline int bd71837_reg_read(struct bd71837 *bd71837, u8 reg)
> +{
> + int r, val;
> +
> + r = regmap_read(bd71837->regmap, reg, &val);
> + if (r < 0)
> + return r;
> + return val;
> +}
> +
> +static inline int bd71837_reg_write(struct bd71837 *bd71837, u8 reg,
> + unsigned int val)
> +{
> + return regmap_write(bd71837->regmap, reg, val);
> +}
> +
> +static inline int bd71837_set_bits(struct bd71837 *bd71837, u8 reg, u8 mask)
> +{
> + return regmap_update_bits(bd71837->regmap, reg, mask, mask);
> +}
> +
> +static inline int bd71837_clear_bits(struct bd71837 *bd71837, u8 reg,
> + u8 mask)
> +{
> + return regmap_update_bits(bd71837->regmap, reg, mask, 0);
> +}
> +
> +static inline int bd71837_update_bits(struct bd71837 *bd71837, u8 reg,
> + u8 mask, u8 val)
> +{
> + return regmap_update_bits(bd71837->regmap, reg, mask, val);
> +}
> +
> +#endif /* __LINUX_MFD_BD71837_H__ */
> --
> 2.14.3
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-07-03 9:26 ` Enric Balletbo Serra
0 siblings, 0 replies; 16+ messages in thread
From: Enric Balletbo Serra @ 2018-07-03 9:26 UTC (permalink / raw)
To: matti.vaittinen
Cc: Michael Turquette, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, Matti Vaittinen, Arnd Bergmann,
Dmitry Torokhov, Sebastian Reichel, chenjh, Andrey Smirnov,
Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, sboyd, linux-clk, devicetree, linux-kernel
Hi Matti,
One doubt regarding the probe function and few comments.
Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
dia dv., 29 de juny 2018 a les 11:47:
>
> ROHM BD71837 PMIC MFD driver providing interrupts and support
> for three subsystems:
> - clk
> - Regulators
> - input/power-key
>
> fix too long lines
I guess that this message is not intended to be here.
>
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
> drivers/mfd/Kconfig | 13 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/bd71837.c | 220 +++++++++++++++++++++++++
> include/linux/mfd/bd71837.h | 391 ++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 625 insertions(+)
> create mode 100644 drivers/mfd/bd71837.c
> create mode 100644 include/linux/mfd/bd71837.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b860eb5aa194..d01a279f5e4a 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1787,6 +1787,19 @@ config MFD_STW481X
> in various ST Microelectronics and ST-Ericsson embedded
> Nomadik series.
>
> +config MFD_BD71837
> + tristate "BD71837 Power Management chip"
> + depends on I2C=y
> + depends on OF
> + select REGMAP_I2C
> + select REGMAP_IRQ
> + select MFD_CORE
> + help
> + Select this option to get support for the ROHM BD71837
> + Power Management chips. BD71837 is designed to power processors like
> + NXP i.MX8. It contains 8 BUCK outputs and 7 LDOs, voltage monitoring
> + and emergency shut down as well as 32,768KHz clock output.
> +
> config MFD_STM32_LPTIMER
> tristate "Support for STM32 Low-Power Timer"
> depends on (ARCH_STM32 && OF) || COMPILE_TEST
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index e9fd20dba18d..09dc9eb3782c 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -227,4 +227,5 @@ obj-$(CONFIG_MFD_STM32_TIMERS) += stm32-timers.o
> obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o
> obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o
> obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
> +obj-$(CONFIG_MFD_BD71837) += bd71837.o
>
> diff --git a/drivers/mfd/bd71837.c b/drivers/mfd/bd71837.c
> new file mode 100644
> index 000000000000..25f2de2a5bb8
> --- /dev/null
> +++ b/drivers/mfd/bd71837.c
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// Copyright (C) 2018 ROHM Semiconductors
> +// bd71837.c -- ROHM BD71837MWV mfd driver
> +//
> +// Datasheet available from
> +// https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
> +
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/bd71837.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/gpio_keys.h>
> +
> +static struct gpio_keys_button btns[] = {
> + {
> + .code = KEY_POWER,
> + .gpio = -1,
> + .type = EV_KEY,
> + }
> +};
> +
> +static struct gpio_keys_platform_data bd718xx_powerkey_data = {
> + .buttons = &btns[0],
> + .nbuttons = ARRAY_SIZE(btns),
> + .name = "bd718xx-pwrkey",
> +};
> +
> +/* bd71837 multi function cells */
> +
> +static struct mfd_cell bd71837_mfd_cells[] = {
> + {
> + .name = "bd71837-clk",
> + }, {
> + .name = "gpio-keys",
> + .platform_data = &bd718xx_powerkey_data,
> + .pdata_size = sizeof(bd718xx_powerkey_data),
> + }, {
> + .name = "bd71837-pmic",
> + }
> +};
> +
> +static const struct regmap_irq bd71837_irqs[] = {
> + REGMAP_IRQ_REG(BD71837_INT_SWRST, 0, BD71837_INT_SWRST_MASK),
> + REGMAP_IRQ_REG(BD71837_INT_PWRBTN_S, 0, BD71837_INT_PWRBTN_S_MASK),
> + REGMAP_IRQ_REG(BD71837_INT_PWRBTN_L, 0, BD71837_INT_PWRBTN_L_MASK),
> + REGMAP_IRQ_REG(BD71837_INT_PWRBTN, 0, BD71837_INT_PWRBTN_MASK),
> + REGMAP_IRQ_REG(BD71837_INT_WDOG, 0, BD71837_INT_WDOG_MASK),
> + REGMAP_IRQ_REG(BD71837_INT_ON_REQ, 0, BD71837_INT_ON_REQ_MASK),
> + REGMAP_IRQ_REG(BD71837_INT_STBY_REQ, 0, BD71837_INT_STBY_REQ_MASK),
> +};
> +
> +static struct regmap_irq_chip bd71837_irq_chip = {
> + .name = "bd71837-irq",
> + .irqs = bd71837_irqs,
> + .num_irqs = ARRAY_SIZE(bd71837_irqs),
> + .num_regs = 1,
> + .irq_reg_stride = 1,
> + .status_base = BD71837_REG_IRQ,
> + .mask_base = BD71837_REG_MIRQ,
> + .ack_base = BD71837_REG_IRQ,
> + .init_ack_masked = true,
> + .mask_invert = false,
> +};
> +
> +static const struct regmap_range pmic_status_range = {
> + .range_min = BD71837_REG_IRQ,
> + .range_max = BD71837_REG_POW_STATE,
> +};
> +
> +static const struct regmap_access_table volatile_regs = {
> + .yes_ranges = &pmic_status_range,
> + .n_yes_ranges = 1,
> +};
> +
> +static const struct regmap_config bd71837_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .volatile_table = &volatile_regs,
> + .max_register = BD71837_MAX_REGISTER - 1,
> + .cache_type = REGCACHE_RBTREE,
> +};
> +
> +static const struct of_device_id bd71837_of_match[] = {
> + { .compatible = "rohm,bd71837", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, bd71837_of_match);
> +
> +static int bd71837_i2c_probe(struct i2c_client *i2c,
> + const struct i2c_device_id *id)
> +{
> + struct bd71837 *bd71837;
> + struct bd71837_board *board_info;
> + int ret = -EINVAL;
> +
> + board_info = dev_get_platdata(&i2c->dev);
Sorry in advance if I am missing something, but isn't this always NULL?
> +
> + if (!board_info) {
then this check is not required
> + board_info = devm_kzalloc(&i2c->dev, sizeof(*board_info),
> + GFP_KERNEL);
> + if (!board_info) {
> + ret = -ENOMEM;
> + goto err_out;
Now that you use devm calls and you don't need to unwind things I
think is better to use plain returns. So,
return -ENOMEM;
> + } else if (i2c->irq) {
IMO this else is confusing, also maybe you want to warn about the
missing interrupt.
if (!i2c->irq) {
dev_err(&i2c->dev, "No IRQ configured!);
return -EINVAL;
}
> + board_info->gpio_intr = i2c->irq;
Is board_info really necessary?
> + } else {
> + ret = -ENOENT;
> + goto err_out;
> + }
and remove the else
> + }
> +
> + if (!board_info)
> + goto err_out;
> +
This is redundant.
> + bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
> + if (bd71837 == NULL)
if (!bd71837)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(i2c, bd71837);
> + bd71837->dev = &i2c->dev;
> + bd71837->i2c_client = i2c;
> + bd71837->chip_irq = board_info->gpio_intr;
bd71837->chip_irq = i2c->irq;
> +
> + 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;
dev_err(bd71837->dev, "regmap initialization failed: %d\n", ret);
return PTR_ERR(bd71837->regmap);
> + }
> +
> + ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
> + if (ret < 0) {
> + dev_err(bd71837->dev,
> + "%s(): Read BD71837_REG_DEVICE failed!\n", __func__);
__func__ part is redundant.
> + goto err_out;
return ret;
> + }
> +
> + ret = devm_regmap_add_irq_chip(&i2c->dev, bd71837->regmap,
> + bd71837->chip_irq, IRQF_ONESHOT, 0,
> + &bd71837_irq_chip, &bd71837->irq_data);
> + if (ret < 0) {
> + dev_err(bd71837->dev, "Failed to add irq_chip %d\n", ret);
> + goto err_out;
return ret;
> + }
> +
> + ret = regmap_update_bits(bd71837->regmap,
> + BD71837_REG_PWRONCONFIG0,
> + BD718XX_PWRBTN_PRESS_DURATION_MASK,
> + BD718XX_PWRBTN_SHORT_PRESS_10MS);
> + if (ret < 0) {
> + dev_err(bd71837->dev,
> + "Failed to configure button short press timeout %d\n",
> + ret);
> + goto err_out;
return ret;
> + }
> + /* According to BD71847 datasheet the HW default for long press
> + * detection is 10ms. So lets change it to 10 sec so we can actually
> + * get the short push and allow gracefull shut down
> + */
> + ret = regmap_update_bits(bd71837->regmap,
> + BD71837_REG_PWRONCONFIG1,
> + BD718XX_PWRBTN_PRESS_DURATION_MASK,
> + BD718XX_PWRBTN_LONG_PRESS_10S);
> + if (ret < 0) {
> + dev_err(bd71837->dev,
> + "Failed to configure button long press timeout %d\n",
> + ret);
> + goto err_out;
return ret;
> + }
> + btns[0].irq = regmap_irq_get_virq(bd71837->irq_data,
> + BD71837_INT_PWRBTN_S);
> +
> + if (btns[0].irq < 0) {
> + ret = btns[0].irq;
> + goto err_out;
return ret;
> + }
> +
> + ret = devm_mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
> + bd71837_mfd_cells,
> + ARRAY_SIZE(bd71837_mfd_cells), NULL, 0,
> + regmap_irq_get_domain(bd71837->irq_data));
> +err_out:
Remove the label.
> +
> + return ret;
> +}
> +
> +static const struct i2c_device_id bd71837_i2c_id[] = {
> + { .name = "bd71837", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id);
> +
> +static struct i2c_driver bd71837_i2c_driver = {
> + .driver = {
> + .name = "bd71837-mfd",
> + .of_match_table = bd71837_of_match,
> + },
> + .probe = bd71837_i2c_probe,
> + .id_table = bd71837_i2c_id,
> +};
> +
> +static int __init bd71837_i2c_init(void)
> +{
> + return i2c_add_driver(&bd71837_i2c_driver);
> +}
> +/* init early so consumer devices can complete system boot */
> +subsys_initcall(bd71837_i2c_init);
> +
> +static void __exit bd71837_i2c_exit(void)
> +{
> + i2c_del_driver(&bd71837_i2c_driver);
> +}
> +module_exit(bd71837_i2c_exit);
> +
> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> +MODULE_DESCRIPTION("BD71837 chip multi-function driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/bd71837.h b/include/linux/mfd/bd71837.h
> new file mode 100644
> index 000000000000..10b0dfe90f27
> --- /dev/null
> +++ b/include/linux/mfd/bd71837.h
> @@ -0,0 +1,391 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/* Copyright (C) 2018 ROHM Semiconductors */
> +
> +/*
> + * ROHM BD71837MWV header file
> + */
> +
> +#ifndef __LINUX_MFD_BD71837_H__
> +#define __LINUX_MFD_BD71837_H__
> +
> +#include <linux/regmap.h>
> +
> +enum {
> + BD71837_BUCK1 = 0,
> + BD71837_BUCK2,
> + BD71837_BUCK3,
> + BD71837_BUCK4,
> + BD71837_BUCK5,
> + BD71837_BUCK6,
> + BD71837_BUCK7,
> + BD71837_BUCK8,
> + BD71837_LDO1,
> + BD71837_LDO2,
> + BD71837_LDO3,
> + BD71837_LDO4,
> + BD71837_LDO5,
> + BD71837_LDO6,
> + BD71837_LDO7,
> + BD71837_REGULATOR_CNT,
> +};
> +
> +#define BD71837_BUCK1_VOLTAGE_NUM 0x40
> +#define BD71837_BUCK2_VOLTAGE_NUM 0x40
> +#define BD71837_BUCK3_VOLTAGE_NUM 0x40
> +#define BD71837_BUCK4_VOLTAGE_NUM 0x40
> +
> +#define BD71837_BUCK5_VOLTAGE_NUM 0x08
> +#define BD71837_BUCK6_VOLTAGE_NUM 0x04
> +#define BD71837_BUCK7_VOLTAGE_NUM 0x08
> +#define BD71837_BUCK8_VOLTAGE_NUM 0x40
> +
> +#define BD71837_LDO1_VOLTAGE_NUM 0x04
> +#define BD71837_LDO2_VOLTAGE_NUM 0x02
> +#define BD71837_LDO3_VOLTAGE_NUM 0x10
> +#define BD71837_LDO4_VOLTAGE_NUM 0x10
> +#define BD71837_LDO5_VOLTAGE_NUM 0x10
> +#define BD71837_LDO6_VOLTAGE_NUM 0x10
> +#define BD71837_LDO7_VOLTAGE_NUM 0x10
> +
> +enum {
> + BD71837_REG_REV = 0x00,
> + BD71837_REG_SWRESET = 0x01,
> + BD71837_REG_I2C_DEV = 0x02,
> + BD71837_REG_PWRCTRL0 = 0x03,
> + BD71837_REG_PWRCTRL1 = 0x04,
> + BD71837_REG_BUCK1_CTRL = 0x05,
> + BD71837_REG_BUCK2_CTRL = 0x06,
> + BD71837_REG_BUCK3_CTRL = 0x07,
> + BD71837_REG_BUCK4_CTRL = 0x08,
> + BD71837_REG_BUCK5_CTRL = 0x09,
> + BD71837_REG_BUCK6_CTRL = 0x0A,
> + BD71837_REG_BUCK7_CTRL = 0x0B,
> + BD71837_REG_BUCK8_CTRL = 0x0C,
> + BD71837_REG_BUCK1_VOLT_RUN = 0x0D,
> + BD71837_REG_BUCK1_VOLT_IDLE = 0x0E,
> + BD71837_REG_BUCK1_VOLT_SUSP = 0x0F,
> + BD71837_REG_BUCK2_VOLT_RUN = 0x10,
> + BD71837_REG_BUCK2_VOLT_IDLE = 0x11,
> + BD71837_REG_BUCK3_VOLT_RUN = 0x12,
> + BD71837_REG_BUCK4_VOLT_RUN = 0x13,
> + BD71837_REG_BUCK5_VOLT = 0x14,
> + BD71837_REG_BUCK6_VOLT = 0x15,
> + BD71837_REG_BUCK7_VOLT = 0x16,
> + BD71837_REG_BUCK8_VOLT = 0x17,
> + BD71837_REG_LDO1_VOLT = 0x18,
> + BD71837_REG_LDO2_VOLT = 0x19,
> + BD71837_REG_LDO3_VOLT = 0x1A,
> + BD71837_REG_LDO4_VOLT = 0x1B,
> + BD71837_REG_LDO5_VOLT = 0x1C,
> + BD71837_REG_LDO6_VOLT = 0x1D,
> + BD71837_REG_LDO7_VOLT = 0x1E,
> + BD71837_REG_TRANS_COND0 = 0x1F,
> + BD71837_REG_TRANS_COND1 = 0x20,
> + BD71837_REG_VRFAULTEN = 0x21,
> + BD71837_REG_MVRFLTMASK0 = 0x22,
> + BD71837_REG_MVRFLTMASK1 = 0x23,
> + BD71837_REG_MVRFLTMASK2 = 0x24,
> + BD71837_REG_RCVCFG = 0x25,
> + BD71837_REG_RCVNUM = 0x26,
> + BD71837_REG_PWRONCONFIG0 = 0x27,
> + BD71837_REG_PWRONCONFIG1 = 0x28,
> + BD71837_REG_RESETSRC = 0x29,
> + BD71837_REG_MIRQ = 0x2A,
> + BD71837_REG_IRQ = 0x2B,
> + BD71837_REG_IN_MON = 0x2C,
> + BD71837_REG_POW_STATE = 0x2D,
> + BD71837_REG_OUT32K = 0x2E,
> + BD71837_REG_REGLOCK = 0x2F,
> + BD71837_REG_OTPVER = 0xFF,
> + BD71837_MAX_REGISTER = 0x100,
> +};
> +
> +#define REGLOCK_PWRSEQ 0x1
> +#define REGLOCK_VREG 0x10
> +
> +/* Generic BUCK control masks */
> +#define BD71837_BUCK_SEL 0x02
> +#define BD71837_BUCK_EN 0x01
> +#define BD71837_BUCK_RUN_ON 0x04
> +
> +/* Generic LDO masks */
> +#define BD71837_LDO_SEL 0x80
> +#define BD71837_LDO_EN 0x40
> +
> +/* BD71837 BUCK ramp rate CTRL reg bits */
> +#define BUCK_RAMPRATE_MASK 0xC0
> +#define BUCK_RAMPRATE_10P00MV 0x0
> +#define BUCK_RAMPRATE_5P00MV 0x1
> +#define BUCK_RAMPRATE_2P50MV 0x2
> +#define BUCK_RAMPRATE_1P25MV 0x3
> +
> +/* BD71837_REG_BUCK1_VOLT_RUN bits */
> +#define BUCK1_RUN_MASK 0x3F
> +#define BUCK1_RUN_DEFAULT 0x14
> +
> +/* BD71837_REG_BUCK1_VOLT_SUSP bits */
> +#define BUCK1_SUSP_MASK 0x3F
> +#define BUCK1_SUSP_DEFAULT 0x14
> +
> +/* BD71837_REG_BUCK1_VOLT_IDLE bits */
> +#define BUCK1_IDLE_MASK 0x3F
> +#define BUCK1_IDLE_DEFAULT 0x14
> +
> +/* BD71837_REG_BUCK2_VOLT_RUN bits */
> +#define BUCK2_RUN_MASK 0x3F
> +#define BUCK2_RUN_DEFAULT 0x1E
> +
> +/* BD71837_REG_BUCK2_VOLT_IDLE bits */
> +#define BUCK2_IDLE_MASK 0x3F
> +#define BUCK2_IDLE_DEFAULT 0x14
> +
> +/* BD71837_REG_BUCK3_VOLT_RUN bits */
> +#define BUCK3_RUN_MASK 0x3F
> +#define BUCK3_RUN_DEFAULT 0x1E
> +
> +/* BD71837_REG_BUCK4_VOLT_RUN bits */
> +#define BUCK4_RUN_MASK 0x3F
> +#define BUCK4_RUN_DEFAULT 0x1E
> +
> +/* BD71837_REG_BUCK5_VOLT bits */
> +#define BUCK5_MASK 0x07
> +#define BUCK5_DEFAULT 0x02
> +
> +/* BD71837_REG_BUCK6_VOLT bits */
> +#define BUCK6_MASK 0x03
> +#define BUCK6_DEFAULT 0x03
> +
> +/* BD71837_REG_BUCK7_VOLT bits */
> +#define BUCK7_MASK 0x07
> +#define BUCK7_DEFAULT 0x03
> +
> +/* BD71837_REG_BUCK8_VOLT bits */
> +#define BUCK8_MASK 0x3F
> +#define BUCK8_DEFAULT 0x1E
> +
> +/* BD71837_REG_IRQ bits */
> +#define IRQ_SWRST 0x40
> +#define IRQ_PWRON_S 0x20
> +#define IRQ_PWRON_L 0x10
> +#define IRQ_PWRON 0x08
> +#define IRQ_WDOG 0x04
> +#define IRQ_ON_REQ 0x02
> +#define IRQ_STBY_REQ 0x01
> +
> +/* BD71837_REG_OUT32K bits */
> +#define BD71837_OUT32K_EN 0x01
> +
> +/* BD71837 gated clock rate */
> +#define BD71837_CLK_RATE 32768
> +
> +/* BD71837 irqs */
> +enum {
> + BD71837_INT_STBY_REQ,
> + BD71837_INT_ON_REQ,
> + BD71837_INT_WDOG,
> + BD71837_INT_PWRBTN,
> + BD71837_INT_PWRBTN_L,
> + BD71837_INT_PWRBTN_S,
> + BD71837_INT_SWRST
> +};
> +
> +/* BD71837 interrupt masks */
> +#define BD71837_INT_SWRST_MASK 0x40
> +#define BD71837_INT_PWRBTN_S_MASK 0x20
> +#define BD71837_INT_PWRBTN_L_MASK 0x10
> +#define BD71837_INT_PWRBTN_MASK 0x8
> +#define BD71837_INT_WDOG_MASK 0x4
> +#define BD71837_INT_ON_REQ_MASK 0x2
> +#define BD71837_INT_STBY_REQ_MASK 0x1
> +
> +/* BD71837_REG_LDO1_VOLT bits */
> +#define LDO1_MASK 0x03
> +
> +/* BD71837_REG_LDO1_VOLT bits */
> +#define LDO2_MASK 0x20
> +
> +/* BD71837_REG_LDO3_VOLT bits */
> +#define LDO3_MASK 0x0F
> +
> +/* BD71837_REG_LDO4_VOLT bits */
> +#define LDO4_MASK 0x0F
> +
> +/* BD71837_REG_LDO5_VOLT bits */
> +#define LDO5_MASK 0x0F
> +
> +/* BD71837_REG_LDO6_VOLT bits */
> +#define LDO6_MASK 0x0F
> +
> +/* BD71837_REG_LDO7_VOLT bits */
> +#define LDO7_MASK 0x0F
> +
> +/* Register write induced reset settings */
> +
> +/* Even though the bit zero is not SWRESET type we still want to write zero
> + * to it when changing type. Bit zero is 'SWRESET' trigger bit and if we
> + * write 1 to it we will trigger the action. So always write 0 to it when
> + * changning SWRESET action - no matter what we read from it.
> + */
> +#define BD71837_SWRESET_TYPE_MASK 7
> +#define BD71837_SWRESET_TYPE_DISABLED 0
> +#define BD71837_SWRESET_TYPE_COLD 4
> +#define BD71837_SWRESET_TYPE_WARM 6
> +
> +#define BD71837_SWRESET_RESET_MASK 1
> +#define BD71837_SWRESET_RESET 1
> +
> +/* Poweroff state transition conditions */
> +
> +#define BD718XX_ON_REQ_POWEROFF_MASK 1
> +#define BD718XX_SWRESET_POWEROFF_MASK 2
> +#define BD718XX_WDOG_POWEROFF_MASK 4
> +#define BD718XX_KEY_L_POWEROFF_MASK 8
> +
> +#define BD718XX_POWOFF_TO_SNVS 0
> +#define BD718XX_POWOFF_TO_RDY 0xF
> +
> +#define BD718XX_POWOFF_TIME_MASK 0xF0
> +enum {
> + BD718XX_POWOFF_TIME_5MS = 0,
> + BD718XX_POWOFF_TIME_10MS,
> + BD718XX_POWOFF_TIME_15MS,
> + BD718XX_POWOFF_TIME_20MS,
> + BD718XX_POWOFF_TIME_25MS,
> + BD718XX_POWOFF_TIME_30MS,
> + BD718XX_POWOFF_TIME_35MS,
> + BD718XX_POWOFF_TIME_40MS,
> + BD718XX_POWOFF_TIME_45MS,
> + BD718XX_POWOFF_TIME_50MS,
> + BD718XX_POWOFF_TIME_75MS,
> + BD718XX_POWOFF_TIME_100MS,
> + BD718XX_POWOFF_TIME_250MS,
> + BD718XX_POWOFF_TIME_500MS,
> + BD718XX_POWOFF_TIME_750MS,
> + BD718XX_POWOFF_TIME_1500MS
> +};
> +
> +/* Poweron sequence state transition conditions */
> +
> +#define BD718XX_RDY_TO_SNVS_MASK 0xF
> +#define BD718XX_SNVS_TO_RUN_MASK 0xF0
> +
> +#define BD718XX_PWR_TRIG_KEY_L 1
> +#define BD718XX_PWR_TRIG_KEY_S 2
> +#define BD718XX_PWR_TRIG_PMIC_ON 4
> +#define BD718XX_PWR_TRIG_VSYS_UVLO 8
> +#define BD718XX_RDY_TO_SNVS_SIFT 0
> +#define BD718XX_SNVS_TO_RUN_SIFT 4
> +
> +
> +#define BD718XX_PWRBTN_PRESS_DURATION_MASK 0xF
> +
> +/* Timeout value for detecting short press */
> +
> +enum {
> + BD718XX_PWRBTN_SHORT_PRESS_10MS = 0,
> + BD718XX_PWRBTN_SHORT_PRESS_500MS,
> + BD718XX_PWRBTN_SHORT_PRESS_1000MS,
> + BD718XX_PWRBTN_SHORT_PRESS_1500MS,
> + BD718XX_PWRBTN_SHORT_PRESS_2000MS,
> + BD718XX_PWRBTN_SHORT_PRESS_2500MS,
> + BD718XX_PWRBTN_SHORT_PRESS_3000MS,
> + BD718XX_PWRBTN_SHORT_PRESS_3500MS,
> + BD718XX_PWRBTN_SHORT_PRESS_4000MS,
> + BD718XX_PWRBTN_SHORT_PRESS_4500MS,
> + BD718XX_PWRBTN_SHORT_PRESS_5000MS,
> + BD718XX_PWRBTN_SHORT_PRESS_5500MS,
> + BD718XX_PWRBTN_SHORT_PRESS_6000MS,
> + BD718XX_PWRBTN_SHORT_PRESS_6500MS,
> + BD718XX_PWRBTN_SHORT_PRESS_7000MS,
> + BD718XX_PWRBTN_SHORT_PRESS_7500MS
> +};
> +
> +/* Timeout value for detecting LONG press */
> +
> +enum {
> + BD718XX_PWRBTN_LONG_PRESS_10MS = 0,
> + BD718XX_PWRBTN_LONG_PRESS_1S,
> + BD718XX_PWRBTN_LONG_PRESS_2S,
> + BD718XX_PWRBTN_LONG_PRESS_3S,
> + BD718XX_PWRBTN_LONG_PRESS_4S,
> + BD718XX_PWRBTN_LONG_PRESS_5S,
> + BD718XX_PWRBTN_LONG_PRESS_6S,
> + BD718XX_PWRBTN_LONG_PRESS_7S,
> + BD718XX_PWRBTN_LONG_PRESS_8S,
> + BD718XX_PWRBTN_LONG_PRESS_9S,
> + BD718XX_PWRBTN_LONG_PRESS_10S,
> + BD718XX_PWRBTN_LONG_PRESS_11S,
> + BD718XX_PWRBTN_LONG_PRESS_12S,
> + BD718XX_PWRBTN_LONG_PRESS_13S,
> + BD718XX_PWRBTN_LONG_PRESS_14S,
> + BD718XX_PWRBTN_LONG_PRESS_15S
> +};
> +
> +
> +
> +struct bd71837;
> +struct bd71837_pmic;
> +struct bd71837_clk;
> +struct bd718xx_pwrkey;
> +
> +/*
> + * Board platform data may be used to initialize regulators.
> + */
> +
> +struct bd71837_board {
> + struct regulator_init_data *init_data[BD71837_REGULATOR_CNT];
> + int gpio_intr;
> +};
> +
> +struct bd71837 {
> + struct device *dev;
> + struct i2c_client *i2c_client;
> + struct regmap *regmap;
> + unsigned long int id;
> +
> + int chip_irq;
> + struct regmap_irq_chip_data *irq_data;
> +
> + struct bd71837_pmic *pmic;
> + struct bd71837_clk *clk;
> + struct bd718xx_pwrkey *pwrkey;
> +};
> +
> +/*
> + * bd71837 sub-driver chip access routines
> + */
> +
> +static inline int bd71837_reg_read(struct bd71837 *bd71837, u8 reg)
> +{
> + int r, val;
> +
> + r = regmap_read(bd71837->regmap, reg, &val);
> + if (r < 0)
> + return r;
> + return val;
> +}
> +
> +static inline int bd71837_reg_write(struct bd71837 *bd71837, u8 reg,
> + unsigned int val)
> +{
> + return regmap_write(bd71837->regmap, reg, val);
> +}
> +
> +static inline int bd71837_set_bits(struct bd71837 *bd71837, u8 reg, u8 mask)
> +{
> + return regmap_update_bits(bd71837->regmap, reg, mask, mask);
> +}
> +
> +static inline int bd71837_clear_bits(struct bd71837 *bd71837, u8 reg,
> + u8 mask)
> +{
> + return regmap_update_bits(bd71837->regmap, reg, mask, 0);
> +}
> +
> +static inline int bd71837_update_bits(struct bd71837 *bd71837, u8 reg,
> + u8 mask, u8 val)
> +{
> + return regmap_update_bits(bd71837->regmap, reg, mask, val);
> +}
> +
> +#endif /* __LINUX_MFD_BD71837_H__ */
> --
> 2.14.3
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-07-03 9:26 ` Enric Balletbo Serra
@ 2018-07-04 8:39 ` Matti Vaittinen
-1 siblings, 0 replies; 16+ messages in thread
From: Matti Vaittinen @ 2018-07-04 8:39 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Michael Turquette, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, Matti Vaittinen, Arnd Bergmann,
Dmitry Torokhov, Sebastian Reichel, chenjh, Andrey Smirnov,
Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, sboyd, linux-clk, devicetree, linux-kernel,
linux-input, mikko.mutanen, heikki.haikola
Hello Enric, Lee and others.
Thanks again for taking the time to review the patch! I do appreciate
the effort. Especially because I find reviewing to be quite hard work.
You spotted some obvious things to change but additionally commented on
things which I would rather not change. (Namely the platdata usage and
replacing gotos with in-pklace returns). I would like to get opinion
from Lee on these before implementing the changes. So I will cook new
åatch only after I know what changes are required. Please see my view on
suggested changes below.
On Tue, Jul 03, 2018 at 11:26:00AM +0200, Enric Balletbo Serra wrote:
> One doubt regarding the probe function and few comments.
>
> Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> dia dv., 29 de juny 2018 a les 11:47:
> >
> > ROHM BD71837 PMIC MFD driver providing interrupts and support
> > for three subsystems:
> > - clk
> > - Regulators
> > - input/power-key
> >
> > fix too long lines
>
> I guess that this message is not intended to be here.
Right. That's a leftover from squash commit. Good catch!
>
> > +static int bd71837_i2c_probe(struct i2c_client *i2c,
> > + const struct i2c_device_id *id)
> > +{
> > + struct bd71837 *bd71837;
> > + struct bd71837_board *board_info;
> > + int ret = -EINVAL;
> > +
> > + board_info = dev_get_platdata(&i2c->dev);
>
> Sorry in advance if I am missing something, but isn't this always NULL?
At the moment, yes. But the idea is that one using this PMIC could
relatively easily get rid of the "depends on OF" if the PMIC is controlled
for example using X86 family chips. So platdata is a mechanism that
could be used to bring in for example the irq information - or perhaps
the chip type when I add support to BD71847. I can remove the platdata
for now if it really bothers - but if it does not, then I would like to
keep it in.
>
> > +
> > + if (!board_info) {
>
> then this check is not required
Yes. But as I said, I would like to keep this so that platdata could be
used for giving the HW information to driver on certain architectures,
But as I said - if this is a problem it can be removed. Please let me
know if platdata usage is a "show stopper" here.
>
> > + board_info = devm_kzalloc(&i2c->dev, sizeof(*board_info),
> > + GFP_KERNEL);
> > + if (!board_info) {
> > + ret = -ENOMEM;
> > + goto err_out;
>
> Now that you use devm calls and you don't need to unwind things I
> think is better to use plain returns. So,
>
> return -ENOMEM;
I have never really understood why use of gotos in error handling is
discouraged. Personally I would always choose single point of exit from
a function when it is simple enough to achieve (like in this case). I've
written and fixed way too many functions which leak resources or
accidentally keep a lock when exiting from error branches. But I know
many colleagues like you who prefer not to have gotos but in place returns
instead. So I guess I'll leave the final call on this to the one who is
maintainer for this code. And it is true there is no things to unwind
now - which does not mean that next updater won't add such. But as I
said, I know plenty of people share your view - and even though I rather
maintain code with only one exit the final call is on subsystem maintainer
here.
>
> > + } else if (i2c->irq) {
>
> IMO this else is confusing, also maybe you want to warn about the
> missing interrupt.
Right. The else is completely unnecessary as we have goto in previous
if. Nicely spotted-
>
> if (!i2c->irq) {
> dev_err(&i2c->dev, "No IRQ configured!);
> return -EINVAL;
> }
>
> > + board_info->gpio_intr = i2c->irq;
>
> Is board_info really necessary?
>
As explained before the idea of board info is to be able to provide the
HW description without device-tree. It could be used for example to provide
the regulator information to sub device(s). I have not tested/used this
mechanism as my development setup relies on DT - but I like to keep this
as an option for those who need to work on archs which do not have DT
support.
> > + } else {
> > + ret = -ENOENT;
> > + goto err_out;
> > + }
>
> and remove the else
>
> > + }
> > +
> > + if (!board_info)
> > + goto err_out;
> > +
>
> This is redundant.
Right. We have alloc check abowe and goto error there.
> > + bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
> > + if (bd71837 == NULL)
>
> if (!bd71837)
>
Right.
> > + return -ENOMEM;
> > +
> > + i2c_set_clientdata(i2c, bd71837);
> > + bd71837->dev = &i2c->dev;
> > + bd71837->i2c_client = i2c;
> > + bd71837->chip_irq = board_info->gpio_intr;
>
> bd71837->chip_irq = i2c->irq;
>
Maybe not if we want to keep the platdata support, right?
> > +
> > + 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;
>
> dev_err(bd71837->dev, "regmap initialization failed: %d\n", ret);
> return PTR_ERR(bd71837->regmap);
>
This goes back to the discussion on whether to prefer single point of
exit or not.
> > + }
> > +
> > + ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
> > + if (ret < 0) {
> > + dev_err(bd71837->dev,
> > + "%s(): Read BD71837_REG_DEVICE failed!\n", __func__);
>
> __func__ part is redundant.
>
Indeed. Thanks.
> > + goto err_out;
> return ret;
Rest of the comments can be fixed if we choose to add multpile exit
points. But as I said, I would prefer having only one so let's wait for
another opinion, Ok?
Best Regards,
Matti Vaittinen
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-07-04 8:39 ` Matti Vaittinen
0 siblings, 0 replies; 16+ messages in thread
From: Matti Vaittinen @ 2018-07-04 8:39 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Michael Turquette, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, Matti Vaittinen, Arnd Bergmann,
Dmitry Torokhov, Sebastian Reichel, chenjh, Andrey Smirnov,
Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, sboyd, linux-clk, devicetree
Hello Enric, Lee and others.
Thanks again for taking the time to review the patch! I do appreciate
the effort. Especially because I find reviewing to be quite hard work.
You spotted some obvious things to change but additionally commented on
things which I would rather not change. (Namely the platdata usage and
replacing gotos with in-pklace returns). I would like to get opinion
from Lee on these before implementing the changes. So I will cook new
åatch only after I know what changes are required. Please see my view on
suggested changes below.
On Tue, Jul 03, 2018 at 11:26:00AM +0200, Enric Balletbo Serra wrote:
> One doubt regarding the probe function and few comments.
>
> Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> dia dv., 29 de juny 2018 a les 11:47:
> >
> > ROHM BD71837 PMIC MFD driver providing interrupts and support
> > for three subsystems:
> > - clk
> > - Regulators
> > - input/power-key
> >
> > fix too long lines
>
> I guess that this message is not intended to be here.
Right. That's a leftover from squash commit. Good catch!
>
> > +static int bd71837_i2c_probe(struct i2c_client *i2c,
> > + const struct i2c_device_id *id)
> > +{
> > + struct bd71837 *bd71837;
> > + struct bd71837_board *board_info;
> > + int ret = -EINVAL;
> > +
> > + board_info = dev_get_platdata(&i2c->dev);
>
> Sorry in advance if I am missing something, but isn't this always NULL?
At the moment, yes. But the idea is that one using this PMIC could
relatively easily get rid of the "depends on OF" if the PMIC is controlled
for example using X86 family chips. So platdata is a mechanism that
could be used to bring in for example the irq information - or perhaps
the chip type when I add support to BD71847. I can remove the platdata
for now if it really bothers - but if it does not, then I would like to
keep it in.
>
> > +
> > + if (!board_info) {
>
> then this check is not required
Yes. But as I said, I would like to keep this so that platdata could be
used for giving the HW information to driver on certain architectures,
But as I said - if this is a problem it can be removed. Please let me
know if platdata usage is a "show stopper" here.
>
> > + board_info = devm_kzalloc(&i2c->dev, sizeof(*board_info),
> > + GFP_KERNEL);
> > + if (!board_info) {
> > + ret = -ENOMEM;
> > + goto err_out;
>
> Now that you use devm calls and you don't need to unwind things I
> think is better to use plain returns. So,
>
> return -ENOMEM;
I have never really understood why use of gotos in error handling is
discouraged. Personally I would always choose single point of exit from
a function when it is simple enough to achieve (like in this case). I've
written and fixed way too many functions which leak resources or
accidentally keep a lock when exiting from error branches. But I know
many colleagues like you who prefer not to have gotos but in place returns
instead. So I guess I'll leave the final call on this to the one who is
maintainer for this code. And it is true there is no things to unwind
now - which does not mean that next updater won't add such. But as I
said, I know plenty of people share your view - and even though I rather
maintain code with only one exit the final call is on subsystem maintainer
here.
>
> > + } else if (i2c->irq) {
>
> IMO this else is confusing, also maybe you want to warn about the
> missing interrupt.
Right. The else is completely unnecessary as we have goto in previous
if. Nicely spotted-
>
> if (!i2c->irq) {
> dev_err(&i2c->dev, "No IRQ configured!);
> return -EINVAL;
> }
>
> > + board_info->gpio_intr = i2c->irq;
>
> Is board_info really necessary?
>
As explained before the idea of board info is to be able to provide the
HW description without device-tree. It could be used for example to provide
the regulator information to sub device(s). I have not tested/used this
mechanism as my development setup relies on DT - but I like to keep this
as an option for those who need to work on archs which do not have DT
support.
> > + } else {
> > + ret = -ENOENT;
> > + goto err_out;
> > + }
>
> and remove the else
>
> > + }
> > +
> > + if (!board_info)
> > + goto err_out;
> > +
>
> This is redundant.
Right. We have alloc check abowe and goto error there.
> > + bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
> > + if (bd71837 == NULL)
>
> if (!bd71837)
>
Right.
> > + return -ENOMEM;
> > +
> > + i2c_set_clientdata(i2c, bd71837);
> > + bd71837->dev = &i2c->dev;
> > + bd71837->i2c_client = i2c;
> > + bd71837->chip_irq = board_info->gpio_intr;
>
> bd71837->chip_irq = i2c->irq;
>
Maybe not if we want to keep the platdata support, right?
> > +
> > + 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;
>
> dev_err(bd71837->dev, "regmap initialization failed: %d\n", ret);
> return PTR_ERR(bd71837->regmap);
>
This goes back to the discussion on whether to prefer single point of
exit or not.
> > + }
> > +
> > + ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
> > + if (ret < 0) {
> > + dev_err(bd71837->dev,
> > + "%s(): Read BD71837_REG_DEVICE failed!\n", __func__);
>
> __func__ part is redundant.
>
Indeed. Thanks.
> > + goto err_out;
> return ret;
Rest of the comments can be fixed if we choose to add multpile exit
points. But as I said, I would prefer having only one so let's wait for
another opinion, Ok?
Best Regards,
Matti Vaittinen
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-07-04 8:39 ` Matti Vaittinen
@ 2018-07-04 9:52 ` Matti Vaittinen
-1 siblings, 0 replies; 16+ messages in thread
From: Matti Vaittinen @ 2018-07-04 9:52 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Michael Turquette, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, Matti Vaittinen, Arnd Bergmann,
Dmitry Torokhov, Sebastian Reichel, chenjh, Andrey Smirnov,
Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, sboyd, linux-clk, devicetree, linux-kernel,
linux-input, mikko.mutanen, heikki.haikola
On Wed, Jul 04, 2018 at 11:39:11AM +0300, Matti Vaittinen wrote:
> On Tue, Jul 03, 2018 at 11:26:00AM +0200, Enric Balletbo Serra wrote:
> > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > dia dv., 29 de juny 2018 a les 11:47:
> >
> > Now that you use devm calls and you don't need to unwind things I
> > think is better to use plain returns. So,
> >
> > return -ENOMEM;
>
> I have never really understood why use of gotos in error handling is
> discouraged. Personally I would always choose single point of exit from
> a function when it is simple enough to achieve (like in this case). I've
> written and fixed way too many functions which leak resources or
> accidentally keep a lock when exiting from error branches. But I know
> many colleagues like you who prefer not to have gotos but in place returns
> instead. So I guess I'll leave the final call on this to the one who is
> maintainer for this code. And it is true there is no things to unwind
> now - which does not mean that next updater won't add such. But as I
> said, I know plenty of people share your view - and even though I rather
> maintain code with only one exit the final call is on subsystem maintainer
> here.
Actually, If it was completely my call the probe would look something
like this:
+static int bd71837_i2c_probe(struct i2c_client *i2c,
+ const struct i2c_device_id *id)
+{
+ struct bd71837 *bd71837;
+ struct bd71837_board *board_info;
+ int gpio_intr = 0;
+
+ const char *errstr = "No IRQ configured";
+ int ret = -EINVAL;
+
+ bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
+
+ if (bd71837 == NULL)
+ return -ENOMEM;
+
+ board_info = dev_get_platdata(&i2c->dev);
+
+ if (!board_info)
+ gpio_intr = i2c->irq;
+ else
+ gpio_intr = board_info->gpio_intr;
+
+ if (!gpio_intr)
+ goto err_out;
+
+ i2c_set_clientdata(i2c, bd71837);
+ bd71837->dev = &i2c->dev;
+ bd71837->i2c_client = i2c;
+ bd71837->chip_irq = gpio_intr;
+
+ errstr = "regmap initialization failed";
+
+ bd71837->regmap = devm_regmap_init_i2c(i2c, &bd71837_regmap_config);
+ ret = PTR_ERR(bd71837->regmap);
+ if (IS_ERR(bd71837->regmap))
+ goto err_out;
+
+ errstr = "Read BD71837_REG_DEVICE failed";
+ ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
+ if (ret < 0)
+ goto err_out;
+
+ errstr = "Failed to add irq_chip";
+ ret = devm_regmap_add_irq_chip(&i2c->dev, bd71837->regmap,
+ bd71837->chip_irq, IRQF_ONESHOT, 0,
+ &bd71837_irq_chip, &bd71837->irq_data);
+ if (ret < 0)
+ goto err_out;
+
+ errstr = "Failed to configure button short press timeout";
+ ret = regmap_update_bits(bd71837->regmap,
+ BD71837_REG_PWRONCONFIG0,
+ BD718XX_PWRBTN_PRESS_DURATION_MASK,
+ BD718XX_PWRBTN_SHORT_PRESS_10MS);
+ if (ret < 0)
+ goto err_out;
+
+ /* According to BD71847 datasheet the HW default for long press
+ * detection is 10ms. So lets change it to 10 sec so we can actually
+ * get the short push and allow gracefull shut down
+ */
+ ret = regmap_update_bits(bd71837->regmap,
+ BD71837_REG_PWRONCONFIG1,
+ BD718XX_PWRBTN_PRESS_DURATION_MASK,
+ BD718XX_PWRBTN_LONG_PRESS_10S);
+
+ errstr = "Failed to configure button long press timeout";
+ if (ret < 0)
+ goto err_out;
+
+ btns[0].irq = regmap_irq_get_virq(bd71837->irq_data,
+ BD71837_INT_PWRBTN_S);
+
+ errstr = "Failed to get the IRQ";
+ ret = btns[0].irq;
+ if (btns[0].irq < 0)
+ goto err_out;
+
+ errstr = "Failed to create subdevices";
+ ret = devm_mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
+ bd71837_mfd_cells,
+ ARRAY_SIZE(bd71837_mfd_cells), NULL, 0,
+ regmap_irq_get_domain(bd71837->irq_data));
+ if (ret) {
+err_out:
+ if (errstr)
+ dev_err(&i2c->dev, "%s (%d)\n", errstr, ret);
+ }
+
+ return ret;
+}
What do you think of this? To my eye it is nice. It keeps single point of
exit and introduces only simple if-statements without the need of curly
brackets. And finally the error prints string works as a comment too.
I've seen bunch of constructs like this on the networking side but I
have no idea if this is frowned on this subsystem =) Oh, and probe abowe
is just to illustrate the idea, I did not even try compiling it yet.
Best Regards
Matti Vaittinen
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-07-04 9:52 ` Matti Vaittinen
0 siblings, 0 replies; 16+ messages in thread
From: Matti Vaittinen @ 2018-07-04 9:52 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Michael Turquette, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, Matti Vaittinen, Arnd Bergmann,
Dmitry Torokhov, Sebastian Reichel, chenjh, Andrey Smirnov,
Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, sboyd, linux-clk, devicetree
On Wed, Jul 04, 2018 at 11:39:11AM +0300, Matti Vaittinen wrote:
> On Tue, Jul 03, 2018 at 11:26:00AM +0200, Enric Balletbo Serra wrote:
> > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > dia dv., 29 de juny 2018 a les 11:47:
> >
> > Now that you use devm calls and you don't need to unwind things I
> > think is better to use plain returns. So,
> >
> > return -ENOMEM;
>
> I have never really understood why use of gotos in error handling is
> discouraged. Personally I would always choose single point of exit from
> a function when it is simple enough to achieve (like in this case). I've
> written and fixed way too many functions which leak resources or
> accidentally keep a lock when exiting from error branches. But I know
> many colleagues like you who prefer not to have gotos but in place returns
> instead. So I guess I'll leave the final call on this to the one who is
> maintainer for this code. And it is true there is no things to unwind
> now - which does not mean that next updater won't add such. But as I
> said, I know plenty of people share your view - and even though I rather
> maintain code with only one exit the final call is on subsystem maintainer
> here.
Actually, If it was completely my call the probe would look something
like this:
+static int bd71837_i2c_probe(struct i2c_client *i2c,
+ const struct i2c_device_id *id)
+{
+ struct bd71837 *bd71837;
+ struct bd71837_board *board_info;
+ int gpio_intr = 0;
+
+ const char *errstr = "No IRQ configured";
+ int ret = -EINVAL;
+
+ bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
+
+ if (bd71837 == NULL)
+ return -ENOMEM;
+
+ board_info = dev_get_platdata(&i2c->dev);
+
+ if (!board_info)
+ gpio_intr = i2c->irq;
+ else
+ gpio_intr = board_info->gpio_intr;
+
+ if (!gpio_intr)
+ goto err_out;
+
+ i2c_set_clientdata(i2c, bd71837);
+ bd71837->dev = &i2c->dev;
+ bd71837->i2c_client = i2c;
+ bd71837->chip_irq = gpio_intr;
+
+ errstr = "regmap initialization failed";
+
+ bd71837->regmap = devm_regmap_init_i2c(i2c, &bd71837_regmap_config);
+ ret = PTR_ERR(bd71837->regmap);
+ if (IS_ERR(bd71837->regmap))
+ goto err_out;
+
+ errstr = "Read BD71837_REG_DEVICE failed";
+ ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
+ if (ret < 0)
+ goto err_out;
+
+ errstr = "Failed to add irq_chip";
+ ret = devm_regmap_add_irq_chip(&i2c->dev, bd71837->regmap,
+ bd71837->chip_irq, IRQF_ONESHOT, 0,
+ &bd71837_irq_chip, &bd71837->irq_data);
+ if (ret < 0)
+ goto err_out;
+
+ errstr = "Failed to configure button short press timeout";
+ ret = regmap_update_bits(bd71837->regmap,
+ BD71837_REG_PWRONCONFIG0,
+ BD718XX_PWRBTN_PRESS_DURATION_MASK,
+ BD718XX_PWRBTN_SHORT_PRESS_10MS);
+ if (ret < 0)
+ goto err_out;
+
+ /* According to BD71847 datasheet the HW default for long press
+ * detection is 10ms. So lets change it to 10 sec so we can actually
+ * get the short push and allow gracefull shut down
+ */
+ ret = regmap_update_bits(bd71837->regmap,
+ BD71837_REG_PWRONCONFIG1,
+ BD718XX_PWRBTN_PRESS_DURATION_MASK,
+ BD718XX_PWRBTN_LONG_PRESS_10S);
+
+ errstr = "Failed to configure button long press timeout";
+ if (ret < 0)
+ goto err_out;
+
+ btns[0].irq = regmap_irq_get_virq(bd71837->irq_data,
+ BD71837_INT_PWRBTN_S);
+
+ errstr = "Failed to get the IRQ";
+ ret = btns[0].irq;
+ if (btns[0].irq < 0)
+ goto err_out;
+
+ errstr = "Failed to create subdevices";
+ ret = devm_mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
+ bd71837_mfd_cells,
+ ARRAY_SIZE(bd71837_mfd_cells), NULL, 0,
+ regmap_irq_get_domain(bd71837->irq_data));
+ if (ret) {
+err_out:
+ if (errstr)
+ dev_err(&i2c->dev, "%s (%d)\n", errstr, ret);
+ }
+
+ return ret;
+}
What do you think of this? To my eye it is nice. It keeps single point of
exit and introduces only simple if-statements without the need of curly
brackets. And finally the error prints string works as a comment too.
I've seen bunch of constructs like this on the networking side but I
have no idea if this is frowned on this subsystem =) Oh, and probe abowe
is just to illustrate the idea, I did not even try compiling it yet.
Best Regards
Matti Vaittinen
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-07-04 9:52 ` Matti Vaittinen
@ 2018-07-04 10:11 ` Lee Jones
-1 siblings, 0 replies; 16+ messages in thread
From: Lee Jones @ 2018-07-04 10:11 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Enric Balletbo Serra, Michael Turquette, Rob Herring,
Mark Rutland, Liam Girdwood, Mark Brown, Matti Vaittinen,
Arnd Bergmann, Dmitry Torokhov, Sebastian Reichel, chenjh,
Andrey Smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, sboyd, linux-clk, devicetree, linux-kernel,
linux-input, mikko.mutanen, heikki.haikola
On Wed, 04 Jul 2018, Matti Vaittinen wrote:
> On Wed, Jul 04, 2018 at 11:39:11AM +0300, Matti Vaittinen wrote:
> > On Tue, Jul 03, 2018 at 11:26:00AM +0200, Enric Balletbo Serra wrote:
> > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > dia dv., 29 de juny 2018 a les 11:47:
> > >
> > > Now that you use devm calls and you don't need to unwind things I
> > > think is better to use plain returns. So,
> > >
> > > return -ENOMEM;
> >
> > I have never really understood why use of gotos in error handling is
> > discouraged.
They're not.
> > Personally I would always choose single point of exit from
> > a function when it is simple enough to achieve (like in this case). I've
> > written and fixed way too many functions which leak resources or
> > accidentally keep a lock when exiting from error branches. But I know
> > many colleagues like you who prefer not to have gotos but in place returns
> > instead. So I guess I'll leave the final call on this to the one who is
> > maintainer for this code. And it is true there is no things to unwind
> > now - which does not mean that next updater won't add such. But as I
> > said, I know plenty of people share your view - and even though I rather
> > maintain code with only one exit the final call is on subsystem maintainer
> > here.
Please use gotos in the error path.
IMO, it's the nicest way to unwind (as you call it).
> Actually, If it was completely my call the probe would look something
> like this:
>
> +static int bd71837_i2c_probe(struct i2c_client *i2c,
> + const struct i2c_device_id *id)
> +{
> + struct bd71837 *bd71837;
> + struct bd71837_board *board_info;
> + int gpio_intr = 0;
> +
> + const char *errstr = "No IRQ configured";
> + int ret = -EINVAL;
> +
> + bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
> +
> + if (bd71837 == NULL)
> + return -ENOMEM;
> +
> + board_info = dev_get_platdata(&i2c->dev);
> +
> + if (!board_info)
> + gpio_intr = i2c->irq;
> + else
> + gpio_intr = board_info->gpio_intr;
> +
> + if (!gpio_intr)
> + goto err_out;
> +
> + i2c_set_clientdata(i2c, bd71837);
> + bd71837->dev = &i2c->dev;
> + bd71837->i2c_client = i2c;
> + bd71837->chip_irq = gpio_intr;
> +
> + errstr = "regmap initialization failed";
> +
> + bd71837->regmap = devm_regmap_init_i2c(i2c, &bd71837_regmap_config);
> + ret = PTR_ERR(bd71837->regmap);
> + if (IS_ERR(bd71837->regmap))
> + goto err_out;
> +
> + errstr = "Read BD71837_REG_DEVICE failed";
> + ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
> + if (ret < 0)
> + goto err_out;
> +
> + errstr = "Failed to add irq_chip";
> + ret = devm_regmap_add_irq_chip(&i2c->dev, bd71837->regmap,
> + bd71837->chip_irq, IRQF_ONESHOT, 0,
> + &bd71837_irq_chip, &bd71837->irq_data);
> + if (ret < 0)
> + goto err_out;
> +
> + errstr = "Failed to configure button short press timeout";
> + ret = regmap_update_bits(bd71837->regmap,
> + BD71837_REG_PWRONCONFIG0,
> + BD718XX_PWRBTN_PRESS_DURATION_MASK,
> + BD718XX_PWRBTN_SHORT_PRESS_10MS);
> + if (ret < 0)
> + goto err_out;
> +
> + /* According to BD71847 datasheet the HW default for long press
> + * detection is 10ms. So lets change it to 10 sec so we can actually
> + * get the short push and allow gracefull shut down
> + */
> + ret = regmap_update_bits(bd71837->regmap,
> + BD71837_REG_PWRONCONFIG1,
> + BD718XX_PWRBTN_PRESS_DURATION_MASK,
> + BD718XX_PWRBTN_LONG_PRESS_10S);
> +
> + errstr = "Failed to configure button long press timeout";
> + if (ret < 0)
> + goto err_out;
> +
> + btns[0].irq = regmap_irq_get_virq(bd71837->irq_data,
> + BD71837_INT_PWRBTN_S);
> +
> + errstr = "Failed to get the IRQ";
> + ret = btns[0].irq;
> + if (btns[0].irq < 0)
> + goto err_out;
> +
> + errstr = "Failed to create subdevices";
> + ret = devm_mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
> + bd71837_mfd_cells,
> + ARRAY_SIZE(bd71837_mfd_cells), NULL, 0,
> + regmap_irq_get_domain(bd71837->irq_data));
> + if (ret) {
> +err_out:
> + if (errstr)
> + dev_err(&i2c->dev, "%s (%d)\n", errstr, ret);
> + }
> +
> + return ret;
> +}
>
> What do you think of this? To my eye it is nice. It keeps single point of
> exit and introduces only simple if-statements without the need of curly
> brackets. And finally the error prints string works as a comment too.
> I've seen bunch of constructs like this on the networking side but I
> have no idea if this is frowned on this subsystem =) Oh, and probe abowe
> is just to illustrate the idea, I did not even try compiling it yet.
That is horrible. I nearly vomited on my keyboard. It doesn't flow
anywhere nearly as nicely has sorting out all of the error handling
*after* it has been detected. You're sacrificing readability to save
a single line and do not save any *actual* lines of code, only a brace.
Landing a goto in the middle of a statement is messy and unsightly.
What happens when you have some resources to free? The last few lines
will become very messy, very quickly.
Nit: "something == NULL" is better written as "!something".
Nit: Please use proper multi-line comments as per the Coding Style.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-07-04 10:11 ` Lee Jones
0 siblings, 0 replies; 16+ messages in thread
From: Lee Jones @ 2018-07-04 10:11 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Enric Balletbo Serra, Michael Turquette, Rob Herring,
Mark Rutland, Liam Girdwood, Mark Brown, Matti Vaittinen,
Arnd Bergmann, Dmitry Torokhov, Sebastian Reichel, chenjh,
Andrey Smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, sboyd, linux-clk, devicetree
On Wed, 04 Jul 2018, Matti Vaittinen wrote:
> On Wed, Jul 04, 2018 at 11:39:11AM +0300, Matti Vaittinen wrote:
> > On Tue, Jul 03, 2018 at 11:26:00AM +0200, Enric Balletbo Serra wrote:
> > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > dia dv., 29 de juny 2018 a les 11:47:
> > >
> > > Now that you use devm calls and you don't need to unwind things I
> > > think is better to use plain returns. So,
> > >
> > > return -ENOMEM;
> >
> > I have never really understood why use of gotos in error handling is
> > discouraged.
They're not.
> > Personally I would always choose single point of exit from
> > a function when it is simple enough to achieve (like in this case). I've
> > written and fixed way too many functions which leak resources or
> > accidentally keep a lock when exiting from error branches. But I know
> > many colleagues like you who prefer not to have gotos but in place returns
> > instead. So I guess I'll leave the final call on this to the one who is
> > maintainer for this code. And it is true there is no things to unwind
> > now - which does not mean that next updater won't add such. But as I
> > said, I know plenty of people share your view - and even though I rather
> > maintain code with only one exit the final call is on subsystem maintainer
> > here.
Please use gotos in the error path.
IMO, it's the nicest way to unwind (as you call it).
> Actually, If it was completely my call the probe would look something
> like this:
>
> +static int bd71837_i2c_probe(struct i2c_client *i2c,
> + const struct i2c_device_id *id)
> +{
> + struct bd71837 *bd71837;
> + struct bd71837_board *board_info;
> + int gpio_intr = 0;
> +
> + const char *errstr = "No IRQ configured";
> + int ret = -EINVAL;
> +
> + bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
> +
> + if (bd71837 == NULL)
> + return -ENOMEM;
> +
> + board_info = dev_get_platdata(&i2c->dev);
> +
> + if (!board_info)
> + gpio_intr = i2c->irq;
> + else
> + gpio_intr = board_info->gpio_intr;
> +
> + if (!gpio_intr)
> + goto err_out;
> +
> + i2c_set_clientdata(i2c, bd71837);
> + bd71837->dev = &i2c->dev;
> + bd71837->i2c_client = i2c;
> + bd71837->chip_irq = gpio_intr;
> +
> + errstr = "regmap initialization failed";
> +
> + bd71837->regmap = devm_regmap_init_i2c(i2c, &bd71837_regmap_config);
> + ret = PTR_ERR(bd71837->regmap);
> + if (IS_ERR(bd71837->regmap))
> + goto err_out;
> +
> + errstr = "Read BD71837_REG_DEVICE failed";
> + ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
> + if (ret < 0)
> + goto err_out;
> +
> + errstr = "Failed to add irq_chip";
> + ret = devm_regmap_add_irq_chip(&i2c->dev, bd71837->regmap,
> + bd71837->chip_irq, IRQF_ONESHOT, 0,
> + &bd71837_irq_chip, &bd71837->irq_data);
> + if (ret < 0)
> + goto err_out;
> +
> + errstr = "Failed to configure button short press timeout";
> + ret = regmap_update_bits(bd71837->regmap,
> + BD71837_REG_PWRONCONFIG0,
> + BD718XX_PWRBTN_PRESS_DURATION_MASK,
> + BD718XX_PWRBTN_SHORT_PRESS_10MS);
> + if (ret < 0)
> + goto err_out;
> +
> + /* According to BD71847 datasheet the HW default for long press
> + * detection is 10ms. So lets change it to 10 sec so we can actually
> + * get the short push and allow gracefull shut down
> + */
> + ret = regmap_update_bits(bd71837->regmap,
> + BD71837_REG_PWRONCONFIG1,
> + BD718XX_PWRBTN_PRESS_DURATION_MASK,
> + BD718XX_PWRBTN_LONG_PRESS_10S);
> +
> + errstr = "Failed to configure button long press timeout";
> + if (ret < 0)
> + goto err_out;
> +
> + btns[0].irq = regmap_irq_get_virq(bd71837->irq_data,
> + BD71837_INT_PWRBTN_S);
> +
> + errstr = "Failed to get the IRQ";
> + ret = btns[0].irq;
> + if (btns[0].irq < 0)
> + goto err_out;
> +
> + errstr = "Failed to create subdevices";
> + ret = devm_mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
> + bd71837_mfd_cells,
> + ARRAY_SIZE(bd71837_mfd_cells), NULL, 0,
> + regmap_irq_get_domain(bd71837->irq_data));
> + if (ret) {
> +err_out:
> + if (errstr)
> + dev_err(&i2c->dev, "%s (%d)\n", errstr, ret);
> + }
> +
> + return ret;
> +}
>
> What do you think of this? To my eye it is nice. It keeps single point of
> exit and introduces only simple if-statements without the need of curly
> brackets. And finally the error prints string works as a comment too.
> I've seen bunch of constructs like this on the networking side but I
> have no idea if this is frowned on this subsystem =) Oh, and probe abowe
> is just to illustrate the idea, I did not even try compiling it yet.
That is horrible. I nearly vomited on my keyboard. It doesn't flow
anywhere nearly as nicely has sorting out all of the error handling
*after* it has been detected. You're sacrificing readability to save
a single line and do not save any *actual* lines of code, only a brace.
Landing a goto in the middle of a statement is messy and unsightly.
What happens when you have some resources to free? The last few lines
will become very messy, very quickly.
Nit: "something == NULL" is better written as "!something".
Nit: Please use proper multi-line comments as per the Coding Style.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-07-04 10:11 ` Lee Jones
@ 2018-07-04 10:34 ` Matti Vaittinen
-1 siblings, 0 replies; 16+ messages in thread
From: Matti Vaittinen @ 2018-07-04 10:34 UTC (permalink / raw)
To: Lee Jones
Cc: Enric Balletbo Serra, Michael Turquette, Rob Herring,
Mark Rutland, Liam Girdwood, Mark Brown, Matti Vaittinen,
Arnd Bergmann, Dmitry Torokhov, Sebastian Reichel, chenjh,
Andrey Smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, sboyd, linux-clk, devicetree, linux-kernel,
linux-input, mikko.mutanen, heikki.haikola
On Wed, Jul 04, 2018 at 11:11:02AM +0100, Lee Jones wrote:
> On Wed, 04 Jul 2018, Matti Vaittinen wrote:
>
> > On Wed, Jul 04, 2018 at 11:39:11AM +0300, Matti Vaittinen wrote:
> > > On Tue, Jul 03, 2018 at 11:26:00AM +0200, Enric Balletbo Serra wrote:
> > > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > > dia dv., 29 de juny 2018 a les 11:47:
> > > >
> > > > Now that you use devm calls and you don't need to unwind things I
> > > > think is better to use plain returns. So,
> > > >
> > > > return -ENOMEM;
> > >
> > > I have never really understood why use of gotos in error handling is
> > > discouraged.
>
> They're not.
>
> > > Personally I would always choose single point of exit from
> > > a function when it is simple enough to achieve (like in this case). I've
> > > written and fixed way too many functions which leak resources or
> > > accidentally keep a lock when exiting from error branches. But I know
> > > many colleagues like you who prefer not to have gotos but in place returns
> > > instead. So I guess I'll leave the final call on this to the one who is
> > > maintainer for this code. And it is true there is no things to unwind
> > > now - which does not mean that next updater won't add such. But as I
> > > said, I know plenty of people share your view - and even though I rather
> > > maintain code with only one exit the final call is on subsystem maintainer
> > > here.
>
> Please use gotos in the error path.
>
> IMO, it's the nicest way to unwind (as you call it).
I'll keep the gotos but clean other stuff for patch v9 then.
>
> > Actually, If it was completely my call the probe would look something
> > like this:
> >
> > +static int bd71837_i2c_probe(struct i2c_client *i2c,
> > + const struct i2c_device_id *id)
> > +{
> > + struct bd71837 *bd71837;
> > + struct bd71837_board *board_info;
> > + int gpio_intr = 0;
> > +
> > + const char *errstr = "No IRQ configured";
> > + int ret = -EINVAL;
> > +
> > + bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
> > +
> > + if (bd71837 == NULL)
> > + return -ENOMEM;
> > +
> > + board_info = dev_get_platdata(&i2c->dev);
> > +
> > + if (!board_info)
> > + gpio_intr = i2c->irq;
> > + else
> > + gpio_intr = board_info->gpio_intr;
> > +
> > + if (!gpio_intr)
> > + goto err_out;
> > +
> > + i2c_set_clientdata(i2c, bd71837);
> > + bd71837->dev = &i2c->dev;
> > + bd71837->i2c_client = i2c;
> > + bd71837->chip_irq = gpio_intr;
> > +
> > + errstr = "regmap initialization failed";
> > +
> > + bd71837->regmap = devm_regmap_init_i2c(i2c, &bd71837_regmap_config);
> > + ret = PTR_ERR(bd71837->regmap);
> > + if (IS_ERR(bd71837->regmap))
> > + goto err_out;
> > +
> > + errstr = "Read BD71837_REG_DEVICE failed";
> > + ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
> > + if (ret < 0)
> > + goto err_out;
> > +
> > + errstr = "Failed to add irq_chip";
> > + ret = devm_regmap_add_irq_chip(&i2c->dev, bd71837->regmap,
> > + bd71837->chip_irq, IRQF_ONESHOT, 0,
> > + &bd71837_irq_chip, &bd71837->irq_data);
> > + if (ret < 0)
> > + goto err_out;
> > +
> > + errstr = "Failed to configure button short press timeout";
> > + ret = regmap_update_bits(bd71837->regmap,
> > + BD71837_REG_PWRONCONFIG0,
> > + BD718XX_PWRBTN_PRESS_DURATION_MASK,
> > + BD718XX_PWRBTN_SHORT_PRESS_10MS);
> > + if (ret < 0)
> > + goto err_out;
> > +
> > + /* According to BD71847 datasheet the HW default for long press
> > + * detection is 10ms. So lets change it to 10 sec so we can actually
> > + * get the short push and allow gracefull shut down
> > + */
> > + ret = regmap_update_bits(bd71837->regmap,
> > + BD71837_REG_PWRONCONFIG1,
> > + BD718XX_PWRBTN_PRESS_DURATION_MASK,
> > + BD718XX_PWRBTN_LONG_PRESS_10S);
> > +
> > + errstr = "Failed to configure button long press timeout";
> > + if (ret < 0)
> > + goto err_out;
> > +
> > + btns[0].irq = regmap_irq_get_virq(bd71837->irq_data,
> > + BD71837_INT_PWRBTN_S);
> > +
> > + errstr = "Failed to get the IRQ";
> > + ret = btns[0].irq;
> > + if (btns[0].irq < 0)
> > + goto err_out;
> > +
> > + errstr = "Failed to create subdevices";
> > + ret = devm_mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
> > + bd71837_mfd_cells,
> > + ARRAY_SIZE(bd71837_mfd_cells), NULL, 0,
> > + regmap_irq_get_domain(bd71837->irq_data));
> > + if (ret) {
> > +err_out:
> > + if (errstr)
> > + dev_err(&i2c->dev, "%s (%d)\n", errstr, ret);
> > + }
> > +
> > + return ret;
> > +}
> >
> > What do you think of this? To my eye it is nice. It keeps single point of
> > exit and introduces only simple if-statements without the need of curly
> > brackets. And finally the error prints string works as a comment too.
> > I've seen bunch of constructs like this on the networking side but I
> > have no idea if this is frowned on this subsystem =) Oh, and probe abowe
> > is just to illustrate the idea, I did not even try compiling it yet.
>
> That is horrible. I nearly vomited on my keyboard.
Note to self: Never to buy second hand keyboard from Lee =)
> It doesn't flow
> anywhere nearly as nicely has sorting out all of the error handling
> *after* it has been detected. You're sacrificing readability to save
> a single line and do not save any *actual* lines of code, only a brace.
I was expecting something like this comment =) But the truth is that one
gets used to reading this quickly. Well, this still sounds like I should
not try convincing you - so you can stay heretic ;)
>
> Landing a goto in the middle of a statement is messy and unsightly.
This is another thing one gets used to. I've actually seen plenty of
code using
if (0) {
error_label:
....
}
for error handling. But again - you can keep your view and I'll adopt to
it here =)
> What happens when you have some resources to free? The last few lines
> will become very messy, very quickly.
One can just build the usual clean-up sequence inside the last if (ret)
using different goto lables. Eg:
if (ret) {
err_unwind_X:
undo_x();
err_unwind_Y:
undo_y();
err_out:
dev_err(...);
}
>
> Nit: "something == NULL" is better written as "!something".
Oh, I personally liked the !foo more as Enric - but I will write the
NULL in case it won't make line too long. This is not a big deal to me.
> Nit: Please use proper multi-line comments as per the Coding Style.
Will do.
Thanks for quick reply! I will send new version today or tomorrow.
Best Regards
Matti Vaittinen
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-07-04 10:34 ` Matti Vaittinen
0 siblings, 0 replies; 16+ messages in thread
From: Matti Vaittinen @ 2018-07-04 10:34 UTC (permalink / raw)
To: Lee Jones
Cc: Enric Balletbo Serra, Michael Turquette, Rob Herring,
Mark Rutland, Liam Girdwood, Mark Brown, Matti Vaittinen,
Arnd Bergmann, Dmitry Torokhov, Sebastian Reichel, chenjh,
Andrey Smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, sboyd, linux-clk, devicetree
On Wed, Jul 04, 2018 at 11:11:02AM +0100, Lee Jones wrote:
> On Wed, 04 Jul 2018, Matti Vaittinen wrote:
>
> > On Wed, Jul 04, 2018 at 11:39:11AM +0300, Matti Vaittinen wrote:
> > > On Tue, Jul 03, 2018 at 11:26:00AM +0200, Enric Balletbo Serra wrote:
> > > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > > dia dv., 29 de juny 2018 a les 11:47:
> > > >
> > > > Now that you use devm calls and you don't need to unwind things I
> > > > think is better to use plain returns. So,
> > > >
> > > > return -ENOMEM;
> > >
> > > I have never really understood why use of gotos in error handling is
> > > discouraged.
>
> They're not.
>
> > > Personally I would always choose single point of exit from
> > > a function when it is simple enough to achieve (like in this case). I've
> > > written and fixed way too many functions which leak resources or
> > > accidentally keep a lock when exiting from error branches. But I know
> > > many colleagues like you who prefer not to have gotos but in place returns
> > > instead. So I guess I'll leave the final call on this to the one who is
> > > maintainer for this code. And it is true there is no things to unwind
> > > now - which does not mean that next updater won't add such. But as I
> > > said, I know plenty of people share your view - and even though I rather
> > > maintain code with only one exit the final call is on subsystem maintainer
> > > here.
>
> Please use gotos in the error path.
>
> IMO, it's the nicest way to unwind (as you call it).
I'll keep the gotos but clean other stuff for patch v9 then.
>
> > Actually, If it was completely my call the probe would look something
> > like this:
> >
> > +static int bd71837_i2c_probe(struct i2c_client *i2c,
> > + const struct i2c_device_id *id)
> > +{
> > + struct bd71837 *bd71837;
> > + struct bd71837_board *board_info;
> > + int gpio_intr = 0;
> > +
> > + const char *errstr = "No IRQ configured";
> > + int ret = -EINVAL;
> > +
> > + bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
> > +
> > + if (bd71837 == NULL)
> > + return -ENOMEM;
> > +
> > + board_info = dev_get_platdata(&i2c->dev);
> > +
> > + if (!board_info)
> > + gpio_intr = i2c->irq;
> > + else
> > + gpio_intr = board_info->gpio_intr;
> > +
> > + if (!gpio_intr)
> > + goto err_out;
> > +
> > + i2c_set_clientdata(i2c, bd71837);
> > + bd71837->dev = &i2c->dev;
> > + bd71837->i2c_client = i2c;
> > + bd71837->chip_irq = gpio_intr;
> > +
> > + errstr = "regmap initialization failed";
> > +
> > + bd71837->regmap = devm_regmap_init_i2c(i2c, &bd71837_regmap_config);
> > + ret = PTR_ERR(bd71837->regmap);
> > + if (IS_ERR(bd71837->regmap))
> > + goto err_out;
> > +
> > + errstr = "Read BD71837_REG_DEVICE failed";
> > + ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
> > + if (ret < 0)
> > + goto err_out;
> > +
> > + errstr = "Failed to add irq_chip";
> > + ret = devm_regmap_add_irq_chip(&i2c->dev, bd71837->regmap,
> > + bd71837->chip_irq, IRQF_ONESHOT, 0,
> > + &bd71837_irq_chip, &bd71837->irq_data);
> > + if (ret < 0)
> > + goto err_out;
> > +
> > + errstr = "Failed to configure button short press timeout";
> > + ret = regmap_update_bits(bd71837->regmap,
> > + BD71837_REG_PWRONCONFIG0,
> > + BD718XX_PWRBTN_PRESS_DURATION_MASK,
> > + BD718XX_PWRBTN_SHORT_PRESS_10MS);
> > + if (ret < 0)
> > + goto err_out;
> > +
> > + /* According to BD71847 datasheet the HW default for long press
> > + * detection is 10ms. So lets change it to 10 sec so we can actually
> > + * get the short push and allow gracefull shut down
> > + */
> > + ret = regmap_update_bits(bd71837->regmap,
> > + BD71837_REG_PWRONCONFIG1,
> > + BD718XX_PWRBTN_PRESS_DURATION_MASK,
> > + BD718XX_PWRBTN_LONG_PRESS_10S);
> > +
> > + errstr = "Failed to configure button long press timeout";
> > + if (ret < 0)
> > + goto err_out;
> > +
> > + btns[0].irq = regmap_irq_get_virq(bd71837->irq_data,
> > + BD71837_INT_PWRBTN_S);
> > +
> > + errstr = "Failed to get the IRQ";
> > + ret = btns[0].irq;
> > + if (btns[0].irq < 0)
> > + goto err_out;
> > +
> > + errstr = "Failed to create subdevices";
> > + ret = devm_mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
> > + bd71837_mfd_cells,
> > + ARRAY_SIZE(bd71837_mfd_cells), NULL, 0,
> > + regmap_irq_get_domain(bd71837->irq_data));
> > + if (ret) {
> > +err_out:
> > + if (errstr)
> > + dev_err(&i2c->dev, "%s (%d)\n", errstr, ret);
> > + }
> > +
> > + return ret;
> > +}
> >
> > What do you think of this? To my eye it is nice. It keeps single point of
> > exit and introduces only simple if-statements without the need of curly
> > brackets. And finally the error prints string works as a comment too.
> > I've seen bunch of constructs like this on the networking side but I
> > have no idea if this is frowned on this subsystem =) Oh, and probe abowe
> > is just to illustrate the idea, I did not even try compiling it yet.
>
> That is horrible. I nearly vomited on my keyboard.
Note to self: Never to buy second hand keyboard from Lee =)
> It doesn't flow
> anywhere nearly as nicely has sorting out all of the error handling
> *after* it has been detected. You're sacrificing readability to save
> a single line and do not save any *actual* lines of code, only a brace.
I was expecting something like this comment =) But the truth is that one
gets used to reading this quickly. Well, this still sounds like I should
not try convincing you - so you can stay heretic ;)
>
> Landing a goto in the middle of a statement is messy and unsightly.
This is another thing one gets used to. I've actually seen plenty of
code using
if (0) {
error_label:
....
}
for error handling. But again - you can keep your view and I'll adopt to
it here =)
> What happens when you have some resources to free? The last few lines
> will become very messy, very quickly.
One can just build the usual clean-up sequence inside the last if (ret)
using different goto lables. Eg:
if (ret) {
err_unwind_X:
undo_x();
err_unwind_Y:
undo_y();
err_out:
dev_err(...);
}
>
> Nit: "something == NULL" is better written as "!something".
Oh, I personally liked the !foo more as Enric - but I will write the
NULL in case it won't make line too long. This is not a big deal to me.
> Nit: Please use proper multi-line comments as per the Coding Style.
Will do.
Thanks for quick reply! I will send new version today or tomorrow.
Best Regards
Matti Vaittinen
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-07-04 10:34 ` Matti Vaittinen
@ 2018-07-04 10:53 ` Lee Jones
-1 siblings, 0 replies; 16+ messages in thread
From: Lee Jones @ 2018-07-04 10:53 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Enric Balletbo Serra, Michael Turquette, Rob Herring,
Mark Rutland, Liam Girdwood, Mark Brown, Matti Vaittinen,
Arnd Bergmann, Dmitry Torokhov, Sebastian Reichel, chenjh,
Andrey Smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, sboyd, linux-clk, devicetree, linux-kernel,
linux-input, mikko.mutanen, heikki.haikola
On Wed, 04 Jul 2018, Matti Vaittinen wrote:
> On Wed, Jul 04, 2018 at 11:11:02AM +0100, Lee Jones wrote:
> > On Wed, 04 Jul 2018, Matti Vaittinen wrote:
> >
> > > On Wed, Jul 04, 2018 at 11:39:11AM +0300, Matti Vaittinen wrote:
> > > > On Tue, Jul 03, 2018 at 11:26:00AM +0200, Enric Balletbo Serra wrote:
> > > > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > > > dia dv., 29 de juny 2018 a les 11:47:
> > > > >
> > > > > Now that you use devm calls and you don't need to unwind things I
> > > > > think is better to use plain returns. So,
> > > > >
> > > > > return -ENOMEM;
> > > >
> > > > I have never really understood why use of gotos in error handling is
> > > > discouraged.
> >
> > They're not.
> >
> > > > Personally I would always choose single point of exit from
> > > > a function when it is simple enough to achieve (like in this case). I've
> > > > written and fixed way too many functions which leak resources or
> > > > accidentally keep a lock when exiting from error branches. But I know
> > > > many colleagues like you who prefer not to have gotos but in place returns
> > > > instead. So I guess I'll leave the final call on this to the one who is
> > > > maintainer for this code. And it is true there is no things to unwind
> > > > now - which does not mean that next updater won't add such. But as I
> > > > said, I know plenty of people share your view - and even though I rather
> > > > maintain code with only one exit the final call is on subsystem maintainer
> > > > here.
> >
> > Please use gotos in the error path.
> >
> > IMO, it's the nicest way to unwind (as you call it).
>
> I'll keep the gotos but clean other stuff for patch v9 then.
Sounds good.
> > > Actually, If it was completely my call the probe would look something
> > > like this:
> > >
> > > +static int bd71837_i2c_probe(struct i2c_client *i2c,
> > > + const struct i2c_device_id *id)
> > > +{
[...]
> > > +}
> > >
> > > What do you think of this? To my eye it is nice. It keeps single point of
> > > exit and introduces only simple if-statements without the need of curly
> > > brackets. And finally the error prints string works as a comment too.
> > > I've seen bunch of constructs like this on the networking side but I
> > > have no idea if this is frowned on this subsystem =) Oh, and probe abowe
> > > is just to illustrate the idea, I did not even try compiling it yet.
> >
> > That is horrible. I nearly vomited on my keyboard.
>
> Note to self: Never to buy second hand keyboard from Lee =)
That is sound advice.
Not sure I would buy 2nd-hand keyboard from anyone - ewe! :\
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-07-04 10:53 ` Lee Jones
0 siblings, 0 replies; 16+ messages in thread
From: Lee Jones @ 2018-07-04 10:53 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Enric Balletbo Serra, Michael Turquette, Rob Herring,
Mark Rutland, Liam Girdwood, Mark Brown, Matti Vaittinen,
Arnd Bergmann, Dmitry Torokhov, Sebastian Reichel, chenjh,
Andrey Smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, sboyd, linux-clk, devicetree
On Wed, 04 Jul 2018, Matti Vaittinen wrote:
> On Wed, Jul 04, 2018 at 11:11:02AM +0100, Lee Jones wrote:
> > On Wed, 04 Jul 2018, Matti Vaittinen wrote:
> >
> > > On Wed, Jul 04, 2018 at 11:39:11AM +0300, Matti Vaittinen wrote:
> > > > On Tue, Jul 03, 2018 at 11:26:00AM +0200, Enric Balletbo Serra wrote:
> > > > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > > > dia dv., 29 de juny 2018 a les 11:47:
> > > > >
> > > > > Now that you use devm calls and you don't need to unwind things I
> > > > > think is better to use plain returns. So,
> > > > >
> > > > > return -ENOMEM;
> > > >
> > > > I have never really understood why use of gotos in error handling is
> > > > discouraged.
> >
> > They're not.
> >
> > > > Personally I would always choose single point of exit from
> > > > a function when it is simple enough to achieve (like in this case). I've
> > > > written and fixed way too many functions which leak resources or
> > > > accidentally keep a lock when exiting from error branches. But I know
> > > > many colleagues like you who prefer not to have gotos but in place returns
> > > > instead. So I guess I'll leave the final call on this to the one who is
> > > > maintainer for this code. And it is true there is no things to unwind
> > > > now - which does not mean that next updater won't add such. But as I
> > > > said, I know plenty of people share your view - and even though I rather
> > > > maintain code with only one exit the final call is on subsystem maintainer
> > > > here.
> >
> > Please use gotos in the error path.
> >
> > IMO, it's the nicest way to unwind (as you call it).
>
> I'll keep the gotos but clean other stuff for patch v9 then.
Sounds good.
> > > Actually, If it was completely my call the probe would look something
> > > like this:
> > >
> > > +static int bd71837_i2c_probe(struct i2c_client *i2c,
> > > + const struct i2c_device_id *id)
> > > +{
[...]
> > > +}
> > >
> > > What do you think of this? To my eye it is nice. It keeps single point of
> > > exit and introduces only simple if-statements without the need of curly
> > > brackets. And finally the error prints string works as a comment too.
> > > I've seen bunch of constructs like this on the networking side but I
> > > have no idea if this is frowned on this subsystem =) Oh, and probe abowe
> > > is just to illustrate the idea, I did not even try compiling it yet.
> >
> > That is horrible. I nearly vomited on my keyboard.
>
> Note to self: Never to buy second hand keyboard from Lee =)
That is sound advice.
Not sure I would buy 2nd-hand keyboard from anyone - ewe! :\
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-07-04 10:53 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-29 8:20 [PATCH v8 0/2] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver Matti Vaittinen
2018-06-29 8:21 ` [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC Matti Vaittinen
2018-07-03 9:26 ` Enric Balletbo Serra
2018-07-03 9:26 ` Enric Balletbo Serra
2018-07-04 8:39 ` Matti Vaittinen
2018-07-04 8:39 ` Matti Vaittinen
2018-07-04 9:52 ` Matti Vaittinen
2018-07-04 9:52 ` Matti Vaittinen
2018-07-04 10:11 ` Lee Jones
2018-07-04 10:11 ` Lee Jones
2018-07-04 10:34 ` Matti Vaittinen
2018-07-04 10:34 ` Matti Vaittinen
2018-07-04 10:53 ` Lee Jones
2018-07-04 10:53 ` Lee Jones
2018-06-29 8:23 ` [PATCH v8 2/2] mfd: bd71837: Devicetree bindings " Matti Vaittinen
2018-06-29 8:26 ` Matti Vaittinen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.