* [PATCH v7 0/4] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver
@ 2018-06-19 10:55 Matti Vaittinen
2018-06-19 10:55 ` [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC Matti Vaittinen
` (4 more replies)
0 siblings, 5 replies; 45+ messages in thread
From: Matti Vaittinen @ 2018-06-19 10:55 UTC (permalink / raw)
To: mturquette, sboyd, robh+dt, mark.rutland, lee.jones, lgirdwood,
broonie, mazziesaccount, arnd, dmitry.torokhov, sre, chenjh,
andrew.smirnov, linus.walleij, kstewart, heiko, gregkh
Cc: 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.
Rewgulator part is already applied. This is reduced set of patches
containing the MFD part, clock gate for 32.768 KHz clock buffer, input
driver for power button and device-tree bindings for MFD.
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 and regulator subsystems.
patch 2:
MFD driver DT bindings
patch 3:
clock driver for BD71837 clock output
patch 4:
power button driver for power button connected to BD71837
This patch series is based on for-mfd-next
---
Matti Vaittinen (4):
mfd: bd71837: mfd driver for ROHM BD71837 PMIC
mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
clk: bd71837: Add driver for BD71837 PMIC clock
input/power: Add driver for BD71837/BD71847 PMIC power button
.../devicetree/bindings/mfd/rohm,bd71837-pmic.txt | 67 ++++
drivers/clk/Kconfig | 6 +
drivers/clk/Makefile | 1 +
drivers/clk/clk-bd71837.c | 146 ++++++++
drivers/input/misc/Kconfig | 10 +
drivers/input/misc/Makefile | 1 +
drivers/input/misc/bd718xx-pwrkey.c | 90 +++++
drivers/mfd/Kconfig | 13 +
drivers/mfd/Makefile | 1 +
drivers/mfd/bd71837.c | 221 +++++++++++++
include/linux/mfd/bd71837.h | 367 +++++++++++++++++++++
11 files changed, 923 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
create mode 100644 drivers/clk/clk-bd71837.c
create mode 100644 drivers/input/misc/bd718xx-pwrkey.c
create mode 100644 drivers/mfd/bd71837.c
create mode 100644 include/linux/mfd/bd71837.h
--
2.14.3
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-06-19 10:55 [PATCH v7 0/4] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver Matti Vaittinen
@ 2018-06-19 10:55 ` Matti Vaittinen
2018-06-26 9:06 ` Enric Balletbo Serra
2018-06-19 10:56 ` [PATCH v7 2/4] mfd: bd71837: Devicetree bindings " Matti Vaittinen
` (3 subsequent siblings)
4 siblings, 1 reply; 45+ messages in thread
From: Matti Vaittinen @ 2018-06-19 10:55 UTC (permalink / raw)
To: mturquette, sboyd, robh+dt, mark.rutland, lee.jones, lgirdwood,
broonie, mazziesaccount, arnd, dmitry.torokhov, sre, chenjh,
andrew.smirnov, linus.walleij, kstewart, heiko, gregkh
Cc: linux-clk, devicetree, linux-kernel, linux-input, mikko.mutanen,
heikki.haikola
ROHM BD71837 PMIC MFD driver providing interrupts and support
for two subsystems:
- clk
- Regulators
Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
drivers/mfd/Kconfig | 13 ++
drivers/mfd/Makefile | 1 +
drivers/mfd/bd71837.c | 221 ++++++++++++++++++++++++++
include/linux/mfd/bd71837.h | 367 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 602 insertions(+)
create mode 100644 drivers/mfd/bd71837.c
create mode 100644 include/linux/mfd/bd71837.h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index b860eb5aa194..7aa05fc9ed8e 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1787,6 +1787,19 @@ config MFD_STW481X
in various ST Microelectronics and ST-Ericsson embedded
Nomadik series.
+config MFD_BD71837
+ bool "BD71837 Power Management chip"
+ depends on I2C=y
+ depends on OF
+ select REGMAP_I2C
+ select REGMAP_IRQ
+ select MFD_CORE
+ help
+ Select this option to get support for the ROHM BD71837
+ Power Management chips. BD71837 is designed to power processors like
+ NXP i.MX8. It contains 8 BUCK outputs and 7 LDOs, voltage monitoring
+ and emergency shut down as well as 32,768KHz clock output.
+
config MFD_STM32_LPTIMER
tristate "Support for STM32 Low-Power Timer"
depends on (ARCH_STM32 && OF) || COMPILE_TEST
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index e9fd20dba18d..09dc9eb3782c 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -227,4 +227,5 @@ obj-$(CONFIG_MFD_STM32_TIMERS) += stm32-timers.o
obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o
obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o
obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
+obj-$(CONFIG_MFD_BD71837) += bd71837.o
diff --git a/drivers/mfd/bd71837.c b/drivers/mfd/bd71837.c
new file mode 100644
index 000000000000..0f0361d6cad6
--- /dev/null
+++ b/drivers/mfd/bd71837.c
@@ -0,0 +1,221 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 ROHM Semiconductors
+// bd71837.c -- ROHM BD71837MWV mfd driver
+//
+// Datasheet available from
+// https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/gpio.h>
+#include <linux/regmap.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/bd71837.h>
+
+static const struct resource irqs[] = {
+ {
+ .start = BD71837_INT_PWRBTN,
+ .end = BD71837_INT_PWRBTN,
+ .flags = IORESOURCE_IRQ,
+ .name = "pwr-btn",
+ }, {
+ .start = BD71837_INT_PWRBTN_L,
+ .end = BD71837_INT_PWRBTN_L,
+ .flags = IORESOURCE_IRQ,
+ .name = "pwr-btn-l",
+ }, {
+ .start = BD71837_INT_PWRBTN_S,
+ .end = BD71837_INT_PWRBTN_S,
+ .flags = IORESOURCE_IRQ,
+ .name = "pwr-btn-s",
+ },
+};
+
+/* bd71837 multi function cells */
+static struct mfd_cell bd71837_mfd_cells[] = {
+ {
+ .name = "bd71837-clk",
+ }, {
+ .name = "bd718xx-pwrkey",
+ .resources = &irqs[0],
+ .num_resources = ARRAY_SIZE(irqs),
+ }, {
+ .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 int bd71837_irq_exit(struct bd71837 *bd71837)
+{
+ if (bd71837->chip_irq > 0)
+ regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);
+ return 0;
+}
+
+static const struct regmap_range pmic_status_range = {
+ .range_min = BD71837_REG_IRQ,
+ .range_max = BD71837_REG_POW_STATE,
+};
+
+static const struct regmap_access_table volatile_regs = {
+ .yes_ranges = &pmic_status_range,
+ .n_yes_ranges = 1,
+};
+
+static const struct regmap_config bd71837_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .volatile_table = &volatile_regs,
+ .max_register = BD71837_MAX_REGISTER - 1,
+ .cache_type = REGCACHE_RBTREE,
+};
+
+#ifdef CONFIG_OF
+static const struct of_device_id bd71837_of_match[] = {
+ { .compatible = "rohm,bd71837", .data = (void *)0},
+ { },
+};
+MODULE_DEVICE_TABLE(of, bd71837_of_match);
+#endif //CONFIG_OF
+
+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 = regmap_add_irq_chip(bd71837->regmap, bd71837->chip_irq,
+ IRQF_ONESHOT, 0,
+ &bd71837_irq_chip, &bd71837->irq_data);
+ if (ret < 0) {
+ dev_err(bd71837->dev, "Failed to add irq_chip %d\n", ret);
+ goto err_out;
+ }
+
+ ret = mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
+ bd71837_mfd_cells, ARRAY_SIZE(bd71837_mfd_cells),
+ NULL, 0,
+ regmap_irq_get_domain(bd71837->irq_data));
+ if (ret)
+ regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);
+err_out:
+
+ return ret;
+}
+
+static int bd71837_i2c_remove(struct i2c_client *i2c)
+{
+ struct bd71837 *bd71837 = i2c_get_clientdata(i2c);
+
+ bd71837_irq_exit(bd71837);
+ mfd_remove_devices(bd71837->dev);
+
+ return 0;
+}
+
+static const struct i2c_device_id bd71837_i2c_id[] = {
+ { .name = "bd71837", },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id);
+
+static struct i2c_driver bd71837_i2c_driver = {
+ .driver = {
+ .name = "bd71837-mfd",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(bd71837_of_match),
+ },
+ .probe = bd71837_i2c_probe,
+ .remove = bd71837_i2c_remove,
+ .id_table = bd71837_i2c_id,
+};
+
+static int __init bd71837_i2c_init(void)
+{
+ return i2c_add_driver(&bd71837_i2c_driver);
+}
+/* init early so consumer devices can complete system boot */
+subsys_initcall(bd71837_i2c_init);
+
+static void __exit bd71837_i2c_exit(void)
+{
+ i2c_del_driver(&bd71837_i2c_driver);
+}
+module_exit(bd71837_i2c_exit);
+
+MODULE_AUTHOR("Matti Vaittinen <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..125c7478ec29
--- /dev/null
+++ b/include/linux/mfd/bd71837.h
@@ -0,0 +1,367 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2018 ROHM Semiconductors */
+
+/*
+ * ROHM BD71837MWV header file
+ */
+
+#ifndef __LINUX_MFD_BD71837_H__
+#define __LINUX_MFD_BD71837_H__
+
+#include <linux/regmap.h>
+
+enum {
+ BD71837_BUCK1 = 0,
+ BD71837_BUCK2,
+ BD71837_BUCK3,
+ BD71837_BUCK4,
+ BD71837_BUCK5,
+ BD71837_BUCK6,
+ BD71837_BUCK7,
+ BD71837_BUCK8,
+ BD71837_LDO1,
+ BD71837_LDO2,
+ BD71837_LDO3,
+ BD71837_LDO4,
+ BD71837_LDO5,
+ BD71837_LDO6,
+ BD71837_LDO7,
+ BD71837_REGULATOR_CNT,
+};
+
+#define BD71837_BUCK1_VOLTAGE_NUM 0x40
+#define BD71837_BUCK2_VOLTAGE_NUM 0x40
+#define BD71837_BUCK3_VOLTAGE_NUM 0x40
+#define BD71837_BUCK4_VOLTAGE_NUM 0x40
+
+#define BD71837_BUCK5_VOLTAGE_NUM 0x08
+#define BD71837_BUCK6_VOLTAGE_NUM 0x04
+#define BD71837_BUCK7_VOLTAGE_NUM 0x08
+#define BD71837_BUCK8_VOLTAGE_NUM 0x40
+
+#define BD71837_LDO1_VOLTAGE_NUM 0x04
+#define BD71837_LDO2_VOLTAGE_NUM 0x02
+#define BD71837_LDO3_VOLTAGE_NUM 0x10
+#define BD71837_LDO4_VOLTAGE_NUM 0x10
+#define BD71837_LDO5_VOLTAGE_NUM 0x10
+#define BD71837_LDO6_VOLTAGE_NUM 0x10
+#define BD71837_LDO7_VOLTAGE_NUM 0x10
+
+enum {
+ BD71837_REG_REV = 0x00,
+ BD71837_REG_SWRESET = 0x01,
+ BD71837_REG_I2C_DEV = 0x02,
+ BD71837_REG_PWRCTRL0 = 0x03,
+ BD71837_REG_PWRCTRL1 = 0x04,
+ BD71837_REG_BUCK1_CTRL = 0x05,
+ BD71837_REG_BUCK2_CTRL = 0x06,
+ BD71837_REG_BUCK3_CTRL = 0x07,
+ BD71837_REG_BUCK4_CTRL = 0x08,
+ BD71837_REG_BUCK5_CTRL = 0x09,
+ BD71837_REG_BUCK6_CTRL = 0x0A,
+ BD71837_REG_BUCK7_CTRL = 0x0B,
+ BD71837_REG_BUCK8_CTRL = 0x0C,
+ BD71837_REG_BUCK1_VOLT_RUN = 0x0D,
+ BD71837_REG_BUCK1_VOLT_IDLE = 0x0E,
+ BD71837_REG_BUCK1_VOLT_SUSP = 0x0F,
+ BD71837_REG_BUCK2_VOLT_RUN = 0x10,
+ BD71837_REG_BUCK2_VOLT_IDLE = 0x11,
+ BD71837_REG_BUCK3_VOLT_RUN = 0x12,
+ BD71837_REG_BUCK4_VOLT_RUN = 0x13,
+ BD71837_REG_BUCK5_VOLT = 0x14,
+ BD71837_REG_BUCK6_VOLT = 0x15,
+ BD71837_REG_BUCK7_VOLT = 0x16,
+ BD71837_REG_BUCK8_VOLT = 0x17,
+ BD71837_REG_LDO1_VOLT = 0x18,
+ BD71837_REG_LDO2_VOLT = 0x19,
+ BD71837_REG_LDO3_VOLT = 0x1A,
+ BD71837_REG_LDO4_VOLT = 0x1B,
+ BD71837_REG_LDO5_VOLT = 0x1C,
+ BD71837_REG_LDO6_VOLT = 0x1D,
+ BD71837_REG_LDO7_VOLT = 0x1E,
+ BD71837_REG_TRANS_COND0 = 0x1F,
+ BD71837_REG_TRANS_COND1 = 0x20,
+ BD71837_REG_VRFAULTEN = 0x21,
+ BD71837_REG_MVRFLTMASK0 = 0x22,
+ BD71837_REG_MVRFLTMASK1 = 0x23,
+ BD71837_REG_MVRFLTMASK2 = 0x24,
+ BD71837_REG_RCVCFG = 0x25,
+ BD71837_REG_RCVNUM = 0x26,
+ BD71837_REG_PWRONCONFIG0 = 0x27,
+ BD71837_REG_PWRONCONFIG1 = 0x28,
+ BD71837_REG_RESETSRC = 0x29,
+ BD71837_REG_MIRQ = 0x2A,
+ BD71837_REG_IRQ = 0x2B,
+ BD71837_REG_IN_MON = 0x2C,
+ BD71837_REG_POW_STATE = 0x2D,
+ BD71837_REG_OUT32K = 0x2E,
+ BD71837_REG_REGLOCK = 0x2F,
+ BD71837_REG_OTPVER = 0xFF,
+ BD71837_MAX_REGISTER = 0x100,
+};
+
+#define REGLOCK_PWRSEQ 0x1
+#define REGLOCK_VREG 0x10
+
+/* Generic BUCK control masks */
+#define BD71837_BUCK_SEL 0x02
+#define BD71837_BUCK_EN 0x01
+#define BD71837_BUCK_RUN_ON 0x04
+
+/* Generic LDO masks */
+#define BD71837_LDO_SEL 0x80
+#define BD71837_LDO_EN 0x40
+
+/* BD71837 BUCK ramp rate CTRL reg bits */
+#define BUCK_RAMPRATE_MASK 0xC0
+#define BUCK_RAMPRATE_10P00MV 0x0
+#define BUCK_RAMPRATE_5P00MV 0x1
+#define BUCK_RAMPRATE_2P50MV 0x2
+#define BUCK_RAMPRATE_1P25MV 0x3
+
+/* BD71837_REG_BUCK1_VOLT_RUN bits */
+#define BUCK1_RUN_MASK 0x3F
+#define BUCK1_RUN_DEFAULT 0x14
+
+/* BD71837_REG_BUCK1_VOLT_SUSP bits */
+#define BUCK1_SUSP_MASK 0x3F
+#define BUCK1_SUSP_DEFAULT 0x14
+
+/* BD71837_REG_BUCK1_VOLT_IDLE bits */
+#define BUCK1_IDLE_MASK 0x3F
+#define BUCK1_IDLE_DEFAULT 0x14
+
+/* BD71837_REG_BUCK2_VOLT_RUN bits */
+#define BUCK2_RUN_MASK 0x3F
+#define BUCK2_RUN_DEFAULT 0x1E
+
+/* BD71837_REG_BUCK2_VOLT_IDLE bits */
+#define BUCK2_IDLE_MASK 0x3F
+#define BUCK2_IDLE_DEFAULT 0x14
+
+/* BD71837_REG_BUCK3_VOLT_RUN bits */
+#define BUCK3_RUN_MASK 0x3F
+#define BUCK3_RUN_DEFAULT 0x1E
+
+/* BD71837_REG_BUCK4_VOLT_RUN bits */
+#define BUCK4_RUN_MASK 0x3F
+#define BUCK4_RUN_DEFAULT 0x1E
+
+/* BD71837_REG_BUCK5_VOLT bits */
+#define BUCK5_MASK 0x07
+#define BUCK5_DEFAULT 0x02
+
+/* BD71837_REG_BUCK6_VOLT bits */
+#define BUCK6_MASK 0x03
+#define BUCK6_DEFAULT 0x03
+
+/* BD71837_REG_BUCK7_VOLT bits */
+#define BUCK7_MASK 0x07
+#define BUCK7_DEFAULT 0x03
+
+/* BD71837_REG_BUCK8_VOLT bits */
+#define BUCK8_MASK 0x3F
+#define BUCK8_DEFAULT 0x1E
+
+/* BD71837_REG_IRQ bits */
+#define IRQ_SWRST 0x40
+#define IRQ_PWRON_S 0x20
+#define IRQ_PWRON_L 0x10
+#define IRQ_PWRON 0x08
+#define IRQ_WDOG 0x04
+#define IRQ_ON_REQ 0x02
+#define IRQ_STBY_REQ 0x01
+
+/* BD71837_REG_OUT32K bits */
+#define BD71837_OUT32K_EN 0x01
+
+/* BD71837 gated clock rate */
+#define BD71837_CLK_RATE 32768
+
+/* BD71837 irqs */
+enum {
+ BD71837_INT_STBY_REQ,
+ BD71837_INT_ON_REQ,
+ BD71837_INT_WDOG,
+ BD71837_INT_PWRBTN,
+ BD71837_INT_PWRBTN_L,
+ BD71837_INT_PWRBTN_S,
+ BD71837_INT_SWRST
+};
+
+/* BD71837 interrupt masks */
+#define BD71837_INT_SWRST_MASK 0x40
+#define BD71837_INT_PWRBTN_S_MASK 0x20
+#define BD71837_INT_PWRBTN_L_MASK 0x10
+#define BD71837_INT_PWRBTN_MASK 0x8
+#define BD71837_INT_WDOG_MASK 0x4
+#define BD71837_INT_ON_REQ_MASK 0x2
+#define BD71837_INT_STBY_REQ_MASK 0x1
+
+/* BD71837_REG_LDO1_VOLT bits */
+#define LDO1_MASK 0x03
+
+/* BD71837_REG_LDO1_VOLT bits */
+#define LDO2_MASK 0x20
+
+/* BD71837_REG_LDO3_VOLT bits */
+#define LDO3_MASK 0x0F
+
+/* BD71837_REG_LDO4_VOLT bits */
+#define LDO4_MASK 0x0F
+
+/* BD71837_REG_LDO5_VOLT bits */
+#define LDO5_MASK 0x0F
+
+/* BD71837_REG_LDO6_VOLT bits */
+#define LDO6_MASK 0x0F
+
+/* BD71837_REG_LDO7_VOLT bits */
+#define LDO7_MASK 0x0F
+
+/* 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. Biz 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
+
+/* Timeout value for detecting short press */
+
+#define BD718XX_PWRBTN_SHORT_PRESS_MASK 0xF
+
+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
+};
+
+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] 45+ messages in thread
* [PATCH v7 2/4] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
2018-06-19 10:55 [PATCH v7 0/4] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver Matti Vaittinen
2018-06-19 10:55 ` [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC Matti Vaittinen
@ 2018-06-19 10:56 ` Matti Vaittinen
2018-06-19 10:56 ` [PATCH v7 3/4] clk: bd71837: Add driver for BD71837 PMIC clock Matti Vaittinen
` (2 subsequent siblings)
4 siblings, 0 replies; 45+ messages in thread
From: Matti Vaittinen @ 2018-06-19 10:56 UTC (permalink / raw)
To: mturquette, sboyd, robh+dt, mark.rutland, lee.jones, lgirdwood,
broonie, mazziesaccount, arnd, dmitry.torokhov, sre, chenjh,
andrew.smirnov, linus.walleij, kstewart, heiko, gregkh
Cc: 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] 45+ messages in thread
* [PATCH v7 3/4] clk: bd71837: Add driver for BD71837 PMIC clock
2018-06-19 10:55 [PATCH v7 0/4] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver Matti Vaittinen
2018-06-19 10:55 ` [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC Matti Vaittinen
2018-06-19 10:56 ` [PATCH v7 2/4] mfd: bd71837: Devicetree bindings " Matti Vaittinen
@ 2018-06-19 10:56 ` Matti Vaittinen
2018-06-19 10:57 ` [PATCH v7 4/4] input/power: Add driver for BD71837/BD71847 PMIC power button Matti Vaittinen
2018-06-21 10:34 ` [PATCH v7 0/4] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver Matti Vaittinen
4 siblings, 0 replies; 45+ messages in thread
From: Matti Vaittinen @ 2018-06-19 10:56 UTC (permalink / raw)
To: mturquette, sboyd, robh+dt, mark.rutland, lee.jones, lgirdwood,
broonie, mazziesaccount, arnd, dmitry.torokhov, sre, chenjh,
andrew.smirnov, linus.walleij, kstewart, heiko, gregkh
Cc: linux-clk, devicetree, linux-kernel, linux-input, mikko.mutanen,
heikki.haikola
Support BD71837 gateable 32768 Hz clock.
Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
drivers/clk/Kconfig | 6 ++
drivers/clk/Makefile | 1 +
drivers/clk/clk-bd71837.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 153 insertions(+)
create mode 100644 drivers/clk/clk-bd71837.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 41492e980ef4..065421a9eb22 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -279,6 +279,12 @@ config COMMON_CLK_STM32H7
---help---
Support for stm32h7 SoC family clocks
+config COMMON_CLK_BD71837
+ tristate "Clock driver for ROHM BD71837 PMIC MFD"
+ depends on MFD_BD71837
+ help
+ This driver supports ROHM BD71837 PMIC clock.
+
source "drivers/clk/bcm/Kconfig"
source "drivers/clk/hisilicon/Kconfig"
source "drivers/clk/imgtec/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index de6d06ac790b..8393c4af7d5a 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -21,6 +21,7 @@ endif
obj-$(CONFIG_MACH_ASM9260) += clk-asm9260.o
obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN) += clk-axi-clkgen.o
obj-$(CONFIG_ARCH_AXXIA) += clk-axm5516.o
+obj-$(CONFIG_COMMON_CLK_BD71837) += clk-bd71837.o
obj-$(CONFIG_COMMON_CLK_CDCE706) += clk-cdce706.o
obj-$(CONFIG_COMMON_CLK_CDCE925) += clk-cdce925.o
obj-$(CONFIG_ARCH_CLPS711X) += clk-clps711x.o
diff --git a/drivers/clk/clk-bd71837.c b/drivers/clk/clk-bd71837.c
new file mode 100644
index 000000000000..f5768039b5c1
--- /dev/null
+++ b/drivers/clk/clk-bd71837.c
@@ -0,0 +1,146 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 ROHM Semiconductors
+// bd71837.c -- ROHM BD71837MWV clock driver
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/mfd/bd71837.h>
+#include <linux/clk-provider.h>
+
+struct bd71837_clk {
+ struct clk_hw hw;
+ u8 reg;
+ u8 mask;
+ unsigned long rate;
+ struct platform_device *pdev;
+ struct bd71837 *mfd;
+};
+
+static int bd71837_clk_set(struct clk_hw *hw, int status)
+{
+ struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
+
+ return bd71837_update_bits(c->mfd, c->reg, c->mask, status);
+}
+
+static void bd71837_clk_disable(struct clk_hw *hw)
+{
+ int rv;
+ struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
+
+ rv = bd71837_clk_set(hw, 0);
+ if (rv)
+ dev_dbg(&c->pdev->dev, "Failed to disable 32K clk (%d)\n", rv);
+}
+
+static int bd71837_clk_enable(struct clk_hw *hw)
+{
+ return bd71837_clk_set(hw, 1);
+}
+
+static int bd71837_clk_is_enabled(struct clk_hw *hw)
+{
+ int enabled;
+ struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
+
+ enabled = c->mask;
+ enabled &= bd71837_reg_read(c->mfd, c->reg);
+
+ return enabled;
+}
+static unsigned long bd71837_clk_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
+
+ return c->rate;
+}
+
+static struct clk_ops bd71837_clk_ops = {
+ .prepare = &bd71837_clk_enable,
+ .unprepare = &bd71837_clk_disable,
+ .is_prepared = &bd71837_clk_is_enabled,
+};
+
+static int bd71837_clk_probe(struct platform_device *pdev)
+{
+ struct bd71837_clk *c;
+ int rval = -ENOMEM;
+ const char *parent_clk;
+ struct device *parent = pdev->dev.parent;
+ struct bd71837 *mfd = dev_get_drvdata(parent);
+ struct clk_init_data init = {
+ .name = "bd71837-32k-out",
+ .ops = &bd71837_clk_ops,
+ };
+
+ c = devm_kzalloc(&pdev->dev, sizeof(*c), GFP_KERNEL);
+ if (!c)
+ return -ENOMEM;
+
+ parent_clk = of_clk_get_parent_name(parent->of_node, 0);
+
+ init.parent_names = &parent_clk;
+ if (parent_clk) {
+ init.num_parents = 1;
+ } else {
+ /* If parent is not given from DT we assume the typical
+ * use-case with 32.768 KHz oscillator for RTC (Maybe we
+ * should just error out here and require parent?)
+ */
+ c->rate = BD71837_CLK_RATE;
+ bd71837_clk_ops.recalc_rate = &bd71837_clk_recalc_rate;
+ dev_warn(&pdev->dev, "No parent clk found - assuming 32,768 KHz\n");
+ }
+
+ c->reg = BD71837_REG_OUT32K;
+ c->mask = BD71837_OUT32K_EN;
+ c->mfd = mfd;
+ c->pdev = pdev;
+ c->hw.init = &init;
+
+ of_property_read_string_index(parent->of_node,
+ "clock-output-names", 0,
+ &init.name);
+
+ rval = devm_clk_hw_register(&pdev->dev, &c->hw);
+ if (!rval) {
+ if (parent->of_node) {
+ rval = of_clk_add_hw_provider(parent->of_node,
+ of_clk_hw_simple_get,
+ &c->hw);
+ if (rval)
+ dev_err(&pdev->dev,
+ "adding clk provider failed\n");
+ }
+ } else {
+ dev_err(&pdev->dev, "failed to register 32K clk");
+ }
+
+ return rval;
+}
+
+static int bd71837_clk_remove(struct platform_device *pdev)
+{
+ if (pdev->dev.parent->of_node)
+ of_clk_del_provider(pdev->dev.parent->of_node);
+ return 0;
+}
+
+static struct platform_driver bd71837_clk = {
+ .driver = {
+ .name = "bd71837-clk",
+ },
+ .probe = bd71837_clk_probe,
+ .remove = bd71837_clk_remove,
+};
+
+module_platform_driver(bd71837_clk);
+
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_DESCRIPTION("BD71837 chip clk driver");
+MODULE_LICENSE("GPL");
--
2.14.3
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v7 4/4] input/power: Add driver for BD71837/BD71847 PMIC power button
2018-06-19 10:55 [PATCH v7 0/4] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver Matti Vaittinen
` (2 preceding siblings ...)
2018-06-19 10:56 ` [PATCH v7 3/4] clk: bd71837: Add driver for BD71837 PMIC clock Matti Vaittinen
@ 2018-06-19 10:57 ` Matti Vaittinen
2018-06-19 17:50 ` Dmitry Torokhov
2018-06-21 10:34 ` [PATCH v7 0/4] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver Matti Vaittinen
4 siblings, 1 reply; 45+ messages in thread
From: Matti Vaittinen @ 2018-06-19 10:57 UTC (permalink / raw)
To: mturquette, sboyd, robh+dt, mark.rutland, lee.jones, lgirdwood,
broonie, mazziesaccount, arnd, dmitry.torokhov, sre, chenjh,
andrew.smirnov, linus.walleij, kstewart, heiko, gregkh
Cc: linux-clk, devicetree, linux-kernel, linux-input, mikko.mutanen,
heikki.haikola
ROHM BD71837 PMIC power button driver providing power-key press
information to user-space.
Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
drivers/input/misc/Kconfig | 10 +++++
drivers/input/misc/Makefile | 1 +
drivers/input/misc/bd718xx-pwrkey.c | 90 +++++++++++++++++++++++++++++++++++++
3 files changed, 101 insertions(+)
create mode 100644 drivers/input/misc/bd718xx-pwrkey.c
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 572b15fa18c2..694c05d3f9fb 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -96,6 +96,16 @@ config INPUT_ATMEL_CAPTOUCH
To compile this driver as a module, choose M here: the
module will be called atmel_captouch.
+config INPUT_BD718XX_PWRKEY
+ tristate "ROHM BD71837/BD71847 power key support"
+ depends on MFD_BD71837
+ help
+ Say Y here if you want support for the power key usually found
+ on boards using a ROHM BD71837/BD71847 compatible PMIC.
+
+ To compile this driver as a module, choose M here: the
+ module will be called bd718xx-pwrkey.
+
config INPUT_BMA150
tristate "BMA150/SMB380 acceleration sensor support"
depends on I2C
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 72cde28649e2..ea5b81cbf2bf 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_INPUT_ATI_REMOTE2) += ati_remote2.o
obj-$(CONFIG_INPUT_ATLAS_BTNS) += atlas_btns.o
obj-$(CONFIG_INPUT_ATMEL_CAPTOUCH) += atmel_captouch.o
obj-$(CONFIG_INPUT_BMA150) += bma150.o
+obj-$(CONFIG_INPUT_BD718XX_PWRKEY) += bd718xx-pwrkey.o
obj-$(CONFIG_INPUT_CM109) += cm109.o
obj-$(CONFIG_INPUT_CMA3000) += cma3000_d0x.o
obj-$(CONFIG_INPUT_CMA3000_I2C) += cma3000_d0x_i2c.o
diff --git a/drivers/input/misc/bd718xx-pwrkey.c b/drivers/input/misc/bd718xx-pwrkey.c
new file mode 100644
index 000000000000..e8ac9475c3cf
--- /dev/null
+++ b/drivers/input/misc/bd718xx-pwrkey.c
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 ROHM Semiconductors
+// bd718xx-pwrkey.c -- ROHM BD71837MWV and BD71847 power button driver
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/mfd/bd71837.h>
+
+struct bd718xx_pwrkey {
+ struct input_dev *idev;
+ struct bd71837 *mfd;
+ int irq;
+};
+
+static irqreturn_t button_irq(int irq, void *_priv)
+{
+ struct input_dev *idev = (struct input_dev *)_priv;
+
+ input_report_key(idev, KEY_POWER, 1);
+ input_sync(idev);
+ input_report_key(idev, KEY_POWER, 0);
+ input_sync(idev);
+
+ return IRQ_HANDLED;
+}
+
+static int bd718xx_pwr_btn_probe(struct platform_device *pdev)
+{
+ int err = -ENOMEM;
+ struct bd718xx_pwrkey *pk;
+
+ pk = devm_kzalloc(&pdev->dev, sizeof(*pk), GFP_KERNEL);
+ if (!pk)
+ goto err_out;
+
+ pk->mfd = dev_get_drvdata(pdev->dev.parent);
+
+ pk->idev = devm_input_allocate_device(&pdev->dev);
+ if (!pk->idev)
+ goto err_out;
+
+ pk->idev->name = "bd718xx-pwrkey";
+ pk->idev->phys = "bd718xx-pwrkey/input0";
+ pk->idev->dev.parent = &pdev->dev;
+
+ input_set_capability(pk->idev, EV_KEY, KEY_POWER);
+
+ err = platform_get_irq_byname(pdev, "pwr-btn-s");
+ if (err < 0) {
+ dev_err(&pdev->dev, "could not get power key interrupt\n");
+ goto err_out;
+ }
+
+ pk->irq = err;
+ err = devm_request_threaded_irq(&pdev->dev, pk->irq, NULL, &button_irq,
+ 0, "bd718xx-pwrkey", pk);
+ if (err)
+ goto err_out;
+
+ platform_set_drvdata(pdev, pk);
+ err = regmap_update_bits(pk->mfd->regmap,
+ BD71837_REG_PWRONCONFIG0,
+ BD718XX_PWRBTN_SHORT_PRESS_MASK,
+ BD718XX_PWRBTN_SHORT_PRESS_10MS);
+ if (err)
+ goto err_out;
+
+ err = input_register_device(pk->idev);
+
+err_out:
+
+ return err;
+}
+
+static struct platform_driver bd718xx_pwr_btn_driver = {
+ .probe = bd718xx_pwr_btn_probe,
+ .driver = {
+ .name = "bd718xx-pwrkey",
+ },
+};
+module_platform_driver(bd718xx_pwr_btn_driver);
+MODULE_DESCRIPTION("Power button driver for buttons connected to ROHM bd71837/bd71847 PMIC");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+
--
2.14.3
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH v7 4/4] input/power: Add driver for BD71837/BD71847 PMIC power button
2018-06-19 10:57 ` [PATCH v7 4/4] input/power: Add driver for BD71837/BD71847 PMIC power button Matti Vaittinen
@ 2018-06-19 17:50 ` Dmitry Torokhov
2018-06-20 6:43 ` Matti Vaittinen
0 siblings, 1 reply; 45+ messages in thread
From: Dmitry Torokhov @ 2018-06-19 17:50 UTC (permalink / raw)
To: Matti Vaittinen
Cc: mturquette, sboyd, robh+dt, mark.rutland, lee.jones, lgirdwood,
broonie, mazziesaccount, arnd, sre, chenjh, andrew.smirnov,
linus.walleij, kstewart, heiko, gregkh, linux-clk, devicetree,
linux-kernel, linux-input, mikko.mutanen, heikki.haikola
Hi Matti,
On Tue, Jun 19, 2018 at 01:57:09PM +0300, Matti Vaittinen wrote:
> ROHM BD71837 PMIC power button driver providing power-key press
> information to user-space.
>
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
> drivers/input/misc/Kconfig | 10 +++++
> drivers/input/misc/Makefile | 1 +
> drivers/input/misc/bd718xx-pwrkey.c | 90 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 101 insertions(+)
> create mode 100644 drivers/input/misc/bd718xx-pwrkey.c
>
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 572b15fa18c2..694c05d3f9fb 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -96,6 +96,16 @@ config INPUT_ATMEL_CAPTOUCH
> To compile this driver as a module, choose M here: the
> module will be called atmel_captouch.
>
> +config INPUT_BD718XX_PWRKEY
> + tristate "ROHM BD71837/BD71847 power key support"
> + depends on MFD_BD71837
> + help
> + Say Y here if you want support for the power key usually found
> + on boards using a ROHM BD71837/BD71847 compatible PMIC.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called bd718xx-pwrkey.
> +
> config INPUT_BMA150
> tristate "BMA150/SMB380 acceleration sensor support"
> depends on I2C
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 72cde28649e2..ea5b81cbf2bf 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_INPUT_ATI_REMOTE2) += ati_remote2.o
> obj-$(CONFIG_INPUT_ATLAS_BTNS) += atlas_btns.o
> obj-$(CONFIG_INPUT_ATMEL_CAPTOUCH) += atmel_captouch.o
> obj-$(CONFIG_INPUT_BMA150) += bma150.o
> +obj-$(CONFIG_INPUT_BD718XX_PWRKEY) += bd718xx-pwrkey.o
> obj-$(CONFIG_INPUT_CM109) += cm109.o
> obj-$(CONFIG_INPUT_CMA3000) += cma3000_d0x.o
> obj-$(CONFIG_INPUT_CMA3000_I2C) += cma3000_d0x_i2c.o
> diff --git a/drivers/input/misc/bd718xx-pwrkey.c b/drivers/input/misc/bd718xx-pwrkey.c
> new file mode 100644
> index 000000000000..e8ac9475c3cf
> --- /dev/null
> +++ b/drivers/input/misc/bd718xx-pwrkey.c
> @@ -0,0 +1,90 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2018 ROHM Semiconductors
> +// bd718xx-pwrkey.c -- ROHM BD71837MWV and BD71847 power button driver
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/mfd/bd71837.h>
> +
> +struct bd718xx_pwrkey {
> + struct input_dev *idev;
> + struct bd71837 *mfd;
> + int irq;
> +};
> +
> +static irqreturn_t button_irq(int irq, void *_priv)
> +{
> + struct input_dev *idev = (struct input_dev *)_priv;
> +
> + input_report_key(idev, KEY_POWER, 1);
> + input_sync(idev);
> + input_report_key(idev, KEY_POWER, 0);
> + input_sync(idev);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int bd718xx_pwr_btn_probe(struct platform_device *pdev)
> +{
> + int err = -ENOMEM;
> + struct bd718xx_pwrkey *pk;
> +
> + pk = devm_kzalloc(&pdev->dev, sizeof(*pk), GFP_KERNEL);
> + if (!pk)
> + goto err_out;
> +
> + pk->mfd = dev_get_drvdata(pdev->dev.parent);
> +
> + pk->idev = devm_input_allocate_device(&pdev->dev);
> + if (!pk->idev)
> + goto err_out;
> +
> + pk->idev->name = "bd718xx-pwrkey";
> + pk->idev->phys = "bd718xx-pwrkey/input0";
> + pk->idev->dev.parent = &pdev->dev;
> +
> + input_set_capability(pk->idev, EV_KEY, KEY_POWER);
> +
> + err = platform_get_irq_byname(pdev, "pwr-btn-s");
> + if (err < 0) {
> + dev_err(&pdev->dev, "could not get power key interrupt\n");
> + goto err_out;
> + }
> +
> + pk->irq = err;
> + err = devm_request_threaded_irq(&pdev->dev, pk->irq, NULL, &button_irq,
> + 0, "bd718xx-pwrkey", pk);
> + if (err)
> + goto err_out;
> +
> + platform_set_drvdata(pdev, pk);
> + err = regmap_update_bits(pk->mfd->regmap,
> + BD71837_REG_PWRONCONFIG0,
> + BD718XX_PWRBTN_SHORT_PRESS_MASK,
> + BD718XX_PWRBTN_SHORT_PRESS_10MS);
This seems to be the only custom bit of set up in the driver, the rest I
think can easily be handled by gpio-keys.c in interrupt-only mode. Maybe
we could move this into MFD piece and drop this driver?
> + if (err)
> + goto err_out;
> +
> + err = input_register_device(pk->idev);
> +
> +err_out:
> +
> + return err;
> +}
> +
> +static struct platform_driver bd718xx_pwr_btn_driver = {
> + .probe = bd718xx_pwr_btn_probe,
> + .driver = {
> + .name = "bd718xx-pwrkey",
> + },
> +};
> +module_platform_driver(bd718xx_pwr_btn_driver);
> +MODULE_DESCRIPTION("Power button driver for buttons connected to ROHM bd71837/bd71847 PMIC");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> +
> --
> 2.14.3
>
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 4/4] input/power: Add driver for BD71837/BD71847 PMIC power button
2018-06-19 17:50 ` Dmitry Torokhov
@ 2018-06-20 6:43 ` Matti Vaittinen
2018-06-21 10:25 ` Matti Vaittinen
0 siblings, 1 reply; 45+ messages in thread
From: Matti Vaittinen @ 2018-06-20 6:43 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: mturquette, sboyd, robh+dt, mark.rutland, lee.jones, lgirdwood,
broonie, mazziesaccount, arnd, sre, chenjh, andrew.smirnov,
linus.walleij, kstewart, heiko, gregkh, linux-clk, devicetree,
linux-kernel, linux-input, mikko.mutanen, heikki.haikola
Hello Dmitry,
First of all - thanks for taking the time to review the patch =)
On Tue, Jun 19, 2018 at 10:50:28AM -0700, Dmitry Torokhov wrote:
> Hi Matti,
>
> On Tue, Jun 19, 2018 at 01:57:09PM +0300, Matti Vaittinen wrote:
> > ROHM BD71837 PMIC power button driver providing power-key press
> > information to user-space.
> >
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > ---
> > drivers/input/misc/Kconfig | 10 +++++
> > drivers/input/misc/Makefile | 1 +
> > drivers/input/misc/bd718xx-pwrkey.c | 90 +++++++++++++++++++++++++++++++++++++
> > 3 files changed, 101 insertions(+)
> > create mode 100644 drivers/input/misc/bd718xx-pwrkey.c
> >
> > + platform_set_drvdata(pdev, pk);
> > + err = regmap_update_bits(pk->mfd->regmap,
> > + BD71837_REG_PWRONCONFIG0,
> > + BD718XX_PWRBTN_SHORT_PRESS_MASK,
> > + BD718XX_PWRBTN_SHORT_PRESS_10MS);
>
> This seems to be the only custom bit of set up in the driver, the rest I
> think can easily be handled by gpio-keys.c in interrupt-only mode. Maybe
> we could move this into MFD piece and drop this driver?
I looked at the gpio_keys.c (for first time so please bear with me if I
ask something you consider as obvious). This is also the first time I am
dealing with input subsystem drivers. So this patch was kind of "RFC"
because I am unsure if this is the best way...
HW we are dealing with is a PMIC which can hace a power-button attached.
HW can generate 3 different types of interrupts for power button
presses:
1. interrupt when button is pressed or released. (Eg. if someone just
hits the button we get two interrupts of this type). We get no 'position
information' from PMIC - just the irqs. Hence it is difficult to know if
buttown was pressed or released. This is the reason why I decided not to
use this IRQ (at least not for now).
2. interrupt when button is pressed for 'short time'. Short time is
configurable and IRQ is generated when button is released based on the
duration it was held down. The limit for 'short time' can be configured.
By default if button is pressed longer than 3 seconds but less than 10
seconds the the PMIC detects 'short push'.
3. interrupt when button is pressed for 'long time'. Mechanism is same
as with short push. Default time is button held over 10 seconds. This
interrupt is not handled if PMIC provides power to processor as PMIC
will cut the power when long push is detected.
So the custom piece is setting the 'short push detection' time from
t > 3 sec to t > 10 msec. Driver is then using short push irq.
This means that we don't detect button press if it is shorter than 10ms.
But we don't need any button state information in driver either. This is
why I decided to use the short push irq - is it Ok?
After this explanation - the gpio_keys_irq_isr seems to be doing exactly
what is needed for short push handling (as far as I can tell). Now it
boils down to question how we should bundle the MFD and gpio_keys
together?
Should I just fill the gpio_keys_platform_data for gpio_keys in MFD
driver? After my short browsing of existing MFD drivers I did not see
any other drivers doing that. This is why I wonder if this is a correct
approach?
Still if MFD is configuring the button presses the gpio_keys for this chip
should only be used if MFD is used, right? Hence the gpio_keys driver
should be instantiaed from MFD, right?
Another option would be using DT and adding gpio_keys node to MFD node
or to simple-bus node. But I have an idea that this would make Rob
unhappy :) I had lenghty discussion with him about declaring the PMIC
as interrupt-controller in device-tree - and I was kindly educated that
it was not the way to go :) I'd rather not started this discussion
again.
Finally, there may be cases when power button is not attached to PMIC
or is needing different configuration for 'short push'. This is why I
would prefer having own Kconfig option for this power-key driver. I am
not sure if it is easily doable if we use gpio_keys?
Can you please give me some further pointer on how I could use the
gpio_keys from MFD?
Best Regards
Matti Vaittinen
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 4/4] input/power: Add driver for BD71837/BD71847 PMIC power button
2018-06-20 6:43 ` Matti Vaittinen
@ 2018-06-21 10:25 ` Matti Vaittinen
2018-06-27 0:21 ` Dmitry Torokhov
0 siblings, 1 reply; 45+ messages in thread
From: Matti Vaittinen @ 2018-06-21 10:25 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: mturquette, sboyd, robh+dt, mark.rutland, lee.jones, lgirdwood,
broonie, mazziesaccount, arnd, sre, chenjh, andrew.smirnov,
linus.walleij, kstewart, heiko, gregkh, linux-clk, devicetree,
linux-kernel, linux-input, mikko.mutanen, heikki.haikola
On Wed, Jun 20, 2018 at 09:43:16AM +0300, Matti Vaittinen wrote:
> On Tue, Jun 19, 2018 at 10:50:28AM -0700, Dmitry Torokhov wrote:
> > Hi Matti,
> >
> > On Tue, Jun 19, 2018 at 01:57:09PM +0300, Matti Vaittinen wrote:
> > > ROHM BD71837 PMIC power button driver providing power-key press
> > > information to user-space.
> > >
> > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > > ---
> > > drivers/input/misc/Kconfig | 10 +++++
> > > drivers/input/misc/Makefile | 1 +
> > > drivers/input/misc/bd718xx-pwrkey.c | 90 +++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 101 insertions(+)
> > > create mode 100644 drivers/input/misc/bd718xx-pwrkey.c
> > >
> > > + platform_set_drvdata(pdev, pk);
> > > + err = regmap_update_bits(pk->mfd->regmap,
> > > + BD71837_REG_PWRONCONFIG0,
> > > + BD718XX_PWRBTN_SHORT_PRESS_MASK,
> > > + BD718XX_PWRBTN_SHORT_PRESS_10MS);
> >
> > This seems to be the only custom bit of set up in the driver, the rest I
> > think can easily be handled by gpio-keys.c in interrupt-only mode. Maybe
> > we could move this into MFD piece and drop this driver?
I did following in MFD driver - is this what you suggested:
+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),
+ }, {
//snip
+static int bd71837_i2c_probe(struct i2c_client *i2c,
+ const struct i2c_device_id *id)
+{
// snip
+ ret = regmap_add_irq_chip(bd71837->regmap, bd71837->chip_irq,
+ IRQF_ONESHOT, 0,
+ &bd71837_irq_chip, &bd71837->irq_data);
+ if (ret < 0) {
+ dev_err(bd71837->dev, "Failed to add irq_chip %d\n", ret);
+ goto err_out;
+ }
+ /* I think this should be done conditionally and only when pwrkey is used
+ * What would be the correct way to decide if we want to touch rhw button
+ * press detection times?
+ */
+ 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 letch 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 = 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 looks is Ok I will send new patch with this approach at next week -
unless I get lost during the traditional midsummer festival here in
Finland.
> Finally, there may be cases when power button is not attached to PMIC
> or is needing different configuration for 'short push'. This is why I
> would prefer having own Kconfig option for this power-key driver. I am
> not sure if it is easily doable if we use gpio_keys?
What would be the preferred mechanism for skipping the button push duration
configurations (time it takes for PMIC to detect short or long push)? Or
setting the durations to values user(s) would prefer? To me this sounds again
something we could configure from DT. Would adding propereties
rohm,short-press-ms and rohm,long-press-ms sound reasonable? I will send
the first version with no option to skip/specify the configuration
(fixed 10ms for short press, 10 sec for long press) but I would like to add
support for specifying the duration as next step.
Br,
Matti Vaittinen
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 0/4] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver
2018-06-19 10:55 [PATCH v7 0/4] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver Matti Vaittinen
` (3 preceding siblings ...)
2018-06-19 10:57 ` [PATCH v7 4/4] input/power: Add driver for BD71837/BD71847 PMIC power button Matti Vaittinen
@ 2018-06-21 10:34 ` Matti Vaittinen
2018-07-03 7:02 ` Lee Jones
4 siblings, 1 reply; 45+ messages in thread
From: Matti Vaittinen @ 2018-06-21 10:34 UTC (permalink / raw)
To: mturquette, sboyd, robh+dt, mark.rutland, lee.jones, lgirdwood,
broonie, mazziesaccount, arnd, dmitry.torokhov, sre, chenjh,
andrew.smirnov, linus.walleij, kstewart, heiko, gregkh
Cc: linux-clk, devicetree, linux-kernel, linux-input, mikko.mutanen,
heikki.haikola
On Tue, Jun 19, 2018 at 01:55:31PM +0300, Matti Vaittinen wrote:
> Patch series adding support for ROHM BD71837 PMIC.
>
What is the preferred way when I send updated patches:
1. always resend _all_ unapplied patches even if there is no changes to
some of them. (patch-vN mail thread contains _all_ unapplied patches)
2. only resend changed patches (patch-vN mail thread contains only
patches that were changed from patch-vN-1)
I have currently used approach 1 - so that no patches would be
accidentally forgotten - but downside is that people need to check if
they have already reviewed some of the patches. I'd rather not caused
any extra work. What is the most convenient way for you guys?
Br,
Matti Vaittinen
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-06-19 10:55 ` [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC Matti Vaittinen
@ 2018-06-26 9:06 ` Enric Balletbo Serra
0 siblings, 0 replies; 45+ messages in thread
From: Enric Balletbo Serra @ 2018-06-26 9:06 UTC (permalink / raw)
To: matti.vaittinen
Cc: Michael Turquette, sboyd, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, mazziesaccount, Arnd Bergmann,
Dmitry Torokhov, Sebastian Reichel, chenjh, andrew.smirnov,
Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree, linux-kernel,
linux-input, mikko.mutanen, heikki.haikola
Hi Matti,
Thanks for the patch, a few comments below, some are feedback I
received when I sent some patches to this subsystem.
Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
dia dt., 19 de juny 2018 a les 12:57:
>
> ROHM BD71837 PMIC MFD driver providing interrupts and support
> for two subsystems:
> - clk
> - Regulators
>
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
> drivers/mfd/Kconfig | 13 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/bd71837.c | 221 ++++++++++++++++++++++++++
> include/linux/mfd/bd71837.h | 367 ++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 602 insertions(+)
> create mode 100644 drivers/mfd/bd71837.c
> create mode 100644 include/linux/mfd/bd71837.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b860eb5aa194..7aa05fc9ed8e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1787,6 +1787,19 @@ config MFD_STW481X
> in various ST Microelectronics and ST-Ericsson embedded
> Nomadik series.
>
> +config MFD_BD71837
> + bool "BD71837 Power Management chip"
I know that some drivers need to be built-in, is this really a
requirement for this driver? Or should work as a module too.
> + 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..0f0361d6cad6
> --- /dev/null
> +++ b/drivers/mfd/bd71837.c
> @@ -0,0 +1,221 @@
> +// SPDX-License-Identifier: GPL-2.0
There is a mismatch between what SPDX says and MODULE_LICENSE says.
GPL-2.0 = GPL v2 only
MODULE_LICENSE(GPL) = GPL v2 or later.
You'd like to change the SPDX identifier to GPL-2.0-or-later or set
module license to "GPL v2".
> +// Copyright (C) 2018 ROHM Semiconductors
> +// bd71837.c -- ROHM BD71837MWV mfd driver
> +//
> +// Datasheet available from
> +// https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
> +
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
This include is not required.
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
ditto
> +#include <linux/irqdomain.h>
ditto
> +#include <linux/gpio.h>
ditto
> +#include <linux/regmap.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
ditto
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/bd71837.h>
> +
Please review the needed includes.
> +static const struct resource irqs[] = {
> + {
> + .start = BD71837_INT_PWRBTN,
> + .end = BD71837_INT_PWRBTN,
> + .flags = IORESOURCE_IRQ,
> + .name = "pwr-btn",
> + }, {
> + .start = BD71837_INT_PWRBTN_L,
> + .end = BD71837_INT_PWRBTN_L,
> + .flags = IORESOURCE_IRQ,
> + .name = "pwr-btn-l",
> + }, {
> + .start = BD71837_INT_PWRBTN_S,
> + .end = BD71837_INT_PWRBTN_S,
> + .flags = IORESOURCE_IRQ,
> + .name = "pwr-btn-s",
> + },
nit: no comma at the end
> +};
> +
> +/* bd71837 multi function cells */
> +static struct mfd_cell bd71837_mfd_cells[] = {
> + {
> + .name = "bd71837-clk",
> + }, {
> + .name = "bd718xx-pwrkey",
> + .resources = &irqs[0],
> + .num_resources = ARRAY_SIZE(irqs),
> + }, {
> + .name = "bd71837-pmic",
> + },
nit: no comma at the end
> +};
> +
> +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 int bd71837_irq_exit(struct bd71837 *bd71837)
> +{
> + if (bd71837->chip_irq > 0)
> + regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);
> + return 0;
> +}
> +
> +static const struct regmap_range pmic_status_range = {
> + .range_min = BD71837_REG_IRQ,
> + .range_max = BD71837_REG_POW_STATE,
> +};
> +
> +static const struct regmap_access_table volatile_regs = {
> + .yes_ranges = &pmic_status_range,
> + .n_yes_ranges = 1,
> +};
> +
> +static const struct regmap_config bd71837_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .volatile_table = &volatile_regs,
> + .max_register = BD71837_MAX_REGISTER - 1,
> + .cache_type = REGCACHE_RBTREE,
> +};
> +
> +#ifdef CONFIG_OF
The driver is DT-only and depends on OF so you don't need to protect this.
> +static const struct of_device_id bd71837_of_match[] = {
> + { .compatible = "rohm,bd71837", .data = (void *)0},
> + { },
nit: { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, bd71837_of_match);
> +#endif //CONFIG_OF
> +
> +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 = regmap_add_irq_chip(bd71837->regmap, bd71837->chip_irq,
> + IRQF_ONESHOT, 0,
> + &bd71837_irq_chip, &bd71837->irq_data);
I think that you can use 'devm_regmap_add_irq_chip' here and remove
the code to delete the irq chip
> + if (ret < 0) {
> + dev_err(bd71837->dev, "Failed to add irq_chip %d\n", ret);
> + goto err_out;
> + }
> +
> + ret = mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
> + bd71837_mfd_cells, ARRAY_SIZE(bd71837_mfd_cells),
> + NULL, 0,
> + regmap_irq_get_domain(bd71837->irq_data));
Same here, I think you can use the devm_mfd_add_devices.
> + if (ret)
> + regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);
If you use devm_ functions you can remove this.
> +err_out:
> +
> + return ret;
> +}
> +
> +static int bd71837_i2c_remove(struct i2c_client *i2c)
> +{
> + struct bd71837 *bd71837 = i2c_get_clientdata(i2c);
> +
> + bd71837_irq_exit(bd71837);
> + mfd_remove_devices(bd71837->dev);
All this can go away if you use the devm_ calls.
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id bd71837_i2c_id[] = {
> + { .name = "bd71837", },
> + { }
nit: { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id);
> +
> +static struct i2c_driver bd71837_i2c_driver = {
> + .driver = {
> + .name = "bd71837-mfd",
> + .owner = THIS_MODULE,
Remove this, it is not needed, the core does it for you.
> + .of_match_table = of_match_ptr(bd71837_of_match),
The driver is DT-only, don't need to call of_match_ptr.
> + },
> + .probe = bd71837_i2c_probe,
> + .remove = bd71837_i2c_remove,
> + .id_table = bd71837_i2c_id,
> +};
> +
> +static int __init bd71837_i2c_init(void)
> +{
> + return i2c_add_driver(&bd71837_i2c_driver);
> +}
> +/* init early so consumer devices can complete system boot */
> +subsys_initcall(bd71837_i2c_init);
> +
> +static void __exit bd71837_i2c_exit(void)
> +{
> + i2c_del_driver(&bd71837_i2c_driver);
> +}
> +module_exit(bd71837_i2c_exit);
> +
Can't you use module_i2c_driver here and get rid of init/exit calls?
> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> +MODULE_DESCRIPTION("BD71837 chip multi-function driver");
> +MODULE_LICENSE("GPL");
License mismatch.
> diff --git a/include/linux/mfd/bd71837.h b/include/linux/mfd/bd71837.h
> new file mode 100644
> index 000000000000..125c7478ec29
> --- /dev/null
> +++ b/include/linux/mfd/bd71837.h
> @@ -0,0 +1,367 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2018 ROHM Semiconductors */
> +
> +/*
> + * ROHM BD71837MWV header file
> + */
> +
> +#ifndef __LINUX_MFD_BD71837_H__
> +#define __LINUX_MFD_BD71837_H__
> +
> +#include <linux/regmap.h>
> +
> +enum {
> + BD71837_BUCK1 = 0,
> + BD71837_BUCK2,
> + BD71837_BUCK3,
> + BD71837_BUCK4,
> + BD71837_BUCK5,
> + BD71837_BUCK6,
> + BD71837_BUCK7,
> + BD71837_BUCK8,
> + BD71837_LDO1,
> + BD71837_LDO2,
> + BD71837_LDO3,
> + BD71837_LDO4,
> + BD71837_LDO5,
> + BD71837_LDO6,
> + BD71837_LDO7,
> + BD71837_REGULATOR_CNT,
> +};
> +
> +#define BD71837_BUCK1_VOLTAGE_NUM 0x40
> +#define BD71837_BUCK2_VOLTAGE_NUM 0x40
> +#define BD71837_BUCK3_VOLTAGE_NUM 0x40
> +#define BD71837_BUCK4_VOLTAGE_NUM 0x40
> +
> +#define BD71837_BUCK5_VOLTAGE_NUM 0x08
> +#define BD71837_BUCK6_VOLTAGE_NUM 0x04
> +#define BD71837_BUCK7_VOLTAGE_NUM 0x08
> +#define BD71837_BUCK8_VOLTAGE_NUM 0x40
> +
> +#define BD71837_LDO1_VOLTAGE_NUM 0x04
> +#define BD71837_LDO2_VOLTAGE_NUM 0x02
> +#define BD71837_LDO3_VOLTAGE_NUM 0x10
> +#define BD71837_LDO4_VOLTAGE_NUM 0x10
> +#define BD71837_LDO5_VOLTAGE_NUM 0x10
> +#define BD71837_LDO6_VOLTAGE_NUM 0x10
> +#define BD71837_LDO7_VOLTAGE_NUM 0x10
> +
> +enum {
> + BD71837_REG_REV = 0x00,
> + BD71837_REG_SWRESET = 0x01,
> + BD71837_REG_I2C_DEV = 0x02,
> + BD71837_REG_PWRCTRL0 = 0x03,
> + BD71837_REG_PWRCTRL1 = 0x04,
> + BD71837_REG_BUCK1_CTRL = 0x05,
> + BD71837_REG_BUCK2_CTRL = 0x06,
> + BD71837_REG_BUCK3_CTRL = 0x07,
> + BD71837_REG_BUCK4_CTRL = 0x08,
> + BD71837_REG_BUCK5_CTRL = 0x09,
> + BD71837_REG_BUCK6_CTRL = 0x0A,
> + BD71837_REG_BUCK7_CTRL = 0x0B,
> + BD71837_REG_BUCK8_CTRL = 0x0C,
> + BD71837_REG_BUCK1_VOLT_RUN = 0x0D,
> + BD71837_REG_BUCK1_VOLT_IDLE = 0x0E,
> + BD71837_REG_BUCK1_VOLT_SUSP = 0x0F,
> + BD71837_REG_BUCK2_VOLT_RUN = 0x10,
> + BD71837_REG_BUCK2_VOLT_IDLE = 0x11,
> + BD71837_REG_BUCK3_VOLT_RUN = 0x12,
> + BD71837_REG_BUCK4_VOLT_RUN = 0x13,
> + BD71837_REG_BUCK5_VOLT = 0x14,
> + BD71837_REG_BUCK6_VOLT = 0x15,
> + BD71837_REG_BUCK7_VOLT = 0x16,
> + BD71837_REG_BUCK8_VOLT = 0x17,
> + BD71837_REG_LDO1_VOLT = 0x18,
> + BD71837_REG_LDO2_VOLT = 0x19,
> + BD71837_REG_LDO3_VOLT = 0x1A,
> + BD71837_REG_LDO4_VOLT = 0x1B,
> + BD71837_REG_LDO5_VOLT = 0x1C,
> + BD71837_REG_LDO6_VOLT = 0x1D,
> + BD71837_REG_LDO7_VOLT = 0x1E,
> + BD71837_REG_TRANS_COND0 = 0x1F,
> + BD71837_REG_TRANS_COND1 = 0x20,
> + BD71837_REG_VRFAULTEN = 0x21,
> + BD71837_REG_MVRFLTMASK0 = 0x22,
> + BD71837_REG_MVRFLTMASK1 = 0x23,
> + BD71837_REG_MVRFLTMASK2 = 0x24,
> + BD71837_REG_RCVCFG = 0x25,
> + BD71837_REG_RCVNUM = 0x26,
> + BD71837_REG_PWRONCONFIG0 = 0x27,
> + BD71837_REG_PWRONCONFIG1 = 0x28,
> + BD71837_REG_RESETSRC = 0x29,
> + BD71837_REG_MIRQ = 0x2A,
> + BD71837_REG_IRQ = 0x2B,
> + BD71837_REG_IN_MON = 0x2C,
> + BD71837_REG_POW_STATE = 0x2D,
> + BD71837_REG_OUT32K = 0x2E,
> + BD71837_REG_REGLOCK = 0x2F,
> + BD71837_REG_OTPVER = 0xFF,
> + BD71837_MAX_REGISTER = 0x100,
> +};
> +
> +#define REGLOCK_PWRSEQ 0x1
> +#define REGLOCK_VREG 0x10
> +
> +/* Generic BUCK control masks */
> +#define BD71837_BUCK_SEL 0x02
> +#define BD71837_BUCK_EN 0x01
> +#define BD71837_BUCK_RUN_ON 0x04
> +
> +/* Generic LDO masks */
> +#define BD71837_LDO_SEL 0x80
> +#define BD71837_LDO_EN 0x40
> +
> +/* BD71837 BUCK ramp rate CTRL reg bits */
> +#define BUCK_RAMPRATE_MASK 0xC0
> +#define BUCK_RAMPRATE_10P00MV 0x0
> +#define BUCK_RAMPRATE_5P00MV 0x1
> +#define BUCK_RAMPRATE_2P50MV 0x2
> +#define BUCK_RAMPRATE_1P25MV 0x3
> +
> +/* BD71837_REG_BUCK1_VOLT_RUN bits */
> +#define BUCK1_RUN_MASK 0x3F
> +#define BUCK1_RUN_DEFAULT 0x14
> +
> +/* BD71837_REG_BUCK1_VOLT_SUSP bits */
> +#define BUCK1_SUSP_MASK 0x3F
> +#define BUCK1_SUSP_DEFAULT 0x14
> +
> +/* BD71837_REG_BUCK1_VOLT_IDLE bits */
> +#define BUCK1_IDLE_MASK 0x3F
> +#define BUCK1_IDLE_DEFAULT 0x14
> +
> +/* BD71837_REG_BUCK2_VOLT_RUN bits */
> +#define BUCK2_RUN_MASK 0x3F
> +#define BUCK2_RUN_DEFAULT 0x1E
> +
> +/* BD71837_REG_BUCK2_VOLT_IDLE bits */
> +#define BUCK2_IDLE_MASK 0x3F
> +#define BUCK2_IDLE_DEFAULT 0x14
> +
> +/* BD71837_REG_BUCK3_VOLT_RUN bits */
> +#define BUCK3_RUN_MASK 0x3F
> +#define BUCK3_RUN_DEFAULT 0x1E
> +
> +/* BD71837_REG_BUCK4_VOLT_RUN bits */
> +#define BUCK4_RUN_MASK 0x3F
> +#define BUCK4_RUN_DEFAULT 0x1E
> +
> +/* BD71837_REG_BUCK5_VOLT bits */
> +#define BUCK5_MASK 0x07
> +#define BUCK5_DEFAULT 0x02
> +
> +/* BD71837_REG_BUCK6_VOLT bits */
> +#define BUCK6_MASK 0x03
> +#define BUCK6_DEFAULT 0x03
> +
> +/* BD71837_REG_BUCK7_VOLT bits */
> +#define BUCK7_MASK 0x07
> +#define BUCK7_DEFAULT 0x03
> +
> +/* BD71837_REG_BUCK8_VOLT bits */
> +#define BUCK8_MASK 0x3F
> +#define BUCK8_DEFAULT 0x1E
> +
> +/* BD71837_REG_IRQ bits */
> +#define IRQ_SWRST 0x40
> +#define IRQ_PWRON_S 0x20
> +#define IRQ_PWRON_L 0x10
> +#define IRQ_PWRON 0x08
> +#define IRQ_WDOG 0x04
> +#define IRQ_ON_REQ 0x02
> +#define IRQ_STBY_REQ 0x01
> +
> +/* BD71837_REG_OUT32K bits */
> +#define BD71837_OUT32K_EN 0x01
> +
> +/* BD71837 gated clock rate */
> +#define BD71837_CLK_RATE 32768
> +
> +/* BD71837 irqs */
> +enum {
> + BD71837_INT_STBY_REQ,
> + BD71837_INT_ON_REQ,
> + BD71837_INT_WDOG,
> + BD71837_INT_PWRBTN,
> + BD71837_INT_PWRBTN_L,
> + BD71837_INT_PWRBTN_S,
> + BD71837_INT_SWRST
> +};
> +
> +/* BD71837 interrupt masks */
> +#define BD71837_INT_SWRST_MASK 0x40
> +#define BD71837_INT_PWRBTN_S_MASK 0x20
> +#define BD71837_INT_PWRBTN_L_MASK 0x10
> +#define BD71837_INT_PWRBTN_MASK 0x8
> +#define BD71837_INT_WDOG_MASK 0x4
> +#define BD71837_INT_ON_REQ_MASK 0x2
> +#define BD71837_INT_STBY_REQ_MASK 0x1
> +
> +/* BD71837_REG_LDO1_VOLT bits */
> +#define LDO1_MASK 0x03
> +
> +/* BD71837_REG_LDO1_VOLT bits */
> +#define LDO2_MASK 0x20
> +
> +/* BD71837_REG_LDO3_VOLT bits */
> +#define LDO3_MASK 0x0F
> +
> +/* BD71837_REG_LDO4_VOLT bits */
> +#define LDO4_MASK 0x0F
> +
> +/* BD71837_REG_LDO5_VOLT bits */
> +#define LDO5_MASK 0x0F
> +
> +/* BD71837_REG_LDO6_VOLT bits */
> +#define LDO6_MASK 0x0F
> +
> +/* BD71837_REG_LDO7_VOLT bits */
> +#define LDO7_MASK 0x0F
> +
> +/* register write induced reset settings */
/* Register ...
> +
> +/* even though the bit zero is not SWRESET type we still want to write zero
/*
* Even
> + * to it when changing type. Biz zero is 'SWRESET' trigger bit and if we
s/Biz/Bit/
> + * 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
> +
> +/* Timeout value for detecting short press */
> +
> +#define BD718XX_PWRBTN_SHORT_PRESS_MASK 0xF
> +
> +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
> +};
> +
> +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
> + */
> +
All these access routines only hide the regmap_* calls, so why not use
the regmap calls directly in the driver and remove all this?
> +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] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-06-26 9:06 ` Enric Balletbo Serra
0 siblings, 0 replies; 45+ messages in thread
From: Enric Balletbo Serra @ 2018-06-26 9:06 UTC (permalink / raw)
To: matti.vaittinen
Cc: Michael Turquette, sboyd, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, mazziesaccount, Arnd Bergmann,
Dmitry Torokhov, Sebastian Reichel, chenjh, andrew.smirnov,
Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree, linux-kernel,
linux-input
Hi Matti,
Thanks for the patch, a few comments below, some are feedback I
received when I sent some patches to this subsystem.
Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
dia dt., 19 de juny 2018 a les 12:57:
>
> ROHM BD71837 PMIC MFD driver providing interrupts and support
> for two subsystems:
> - clk
> - Regulators
>
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
> drivers/mfd/Kconfig | 13 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/bd71837.c | 221 ++++++++++++++++++++++++++
> include/linux/mfd/bd71837.h | 367 ++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 602 insertions(+)
> create mode 100644 drivers/mfd/bd71837.c
> create mode 100644 include/linux/mfd/bd71837.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b860eb5aa194..7aa05fc9ed8e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1787,6 +1787,19 @@ config MFD_STW481X
> in various ST Microelectronics and ST-Ericsson embedded
> Nomadik series.
>
> +config MFD_BD71837
> + bool "BD71837 Power Management chip"
I know that some drivers need to be built-in, is this really a
requirement for this driver? Or should work as a module too.
> + 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..0f0361d6cad6
> --- /dev/null
> +++ b/drivers/mfd/bd71837.c
> @@ -0,0 +1,221 @@
> +// SPDX-License-Identifier: GPL-2.0
There is a mismatch between what SPDX says and MODULE_LICENSE says.
GPL-2.0 = GPL v2 only
MODULE_LICENSE(GPL) = GPL v2 or later.
You'd like to change the SPDX identifier to GPL-2.0-or-later or set
module license to "GPL v2".
> +// Copyright (C) 2018 ROHM Semiconductors
> +// bd71837.c -- ROHM BD71837MWV mfd driver
> +//
> +// Datasheet available from
> +// https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
> +
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
This include is not required.
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
ditto
> +#include <linux/irqdomain.h>
ditto
> +#include <linux/gpio.h>
ditto
> +#include <linux/regmap.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
ditto
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/bd71837.h>
> +
Please review the needed includes.
> +static const struct resource irqs[] = {
> + {
> + .start = BD71837_INT_PWRBTN,
> + .end = BD71837_INT_PWRBTN,
> + .flags = IORESOURCE_IRQ,
> + .name = "pwr-btn",
> + }, {
> + .start = BD71837_INT_PWRBTN_L,
> + .end = BD71837_INT_PWRBTN_L,
> + .flags = IORESOURCE_IRQ,
> + .name = "pwr-btn-l",
> + }, {
> + .start = BD71837_INT_PWRBTN_S,
> + .end = BD71837_INT_PWRBTN_S,
> + .flags = IORESOURCE_IRQ,
> + .name = "pwr-btn-s",
> + },
nit: no comma at the end
> +};
> +
> +/* bd71837 multi function cells */
> +static struct mfd_cell bd71837_mfd_cells[] = {
> + {
> + .name = "bd71837-clk",
> + }, {
> + .name = "bd718xx-pwrkey",
> + .resources = &irqs[0],
> + .num_resources = ARRAY_SIZE(irqs),
> + }, {
> + .name = "bd71837-pmic",
> + },
nit: no comma at the end
> +};
> +
> +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 int bd71837_irq_exit(struct bd71837 *bd71837)
> +{
> + if (bd71837->chip_irq > 0)
> + regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);
> + return 0;
> +}
> +
> +static const struct regmap_range pmic_status_range = {
> + .range_min = BD71837_REG_IRQ,
> + .range_max = BD71837_REG_POW_STATE,
> +};
> +
> +static const struct regmap_access_table volatile_regs = {
> + .yes_ranges = &pmic_status_range,
> + .n_yes_ranges = 1,
> +};
> +
> +static const struct regmap_config bd71837_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .volatile_table = &volatile_regs,
> + .max_register = BD71837_MAX_REGISTER - 1,
> + .cache_type = REGCACHE_RBTREE,
> +};
> +
> +#ifdef CONFIG_OF
The driver is DT-only and depends on OF so you don't need to protect this.
> +static const struct of_device_id bd71837_of_match[] = {
> + { .compatible = "rohm,bd71837", .data = (void *)0},
> + { },
nit: { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, bd71837_of_match);
> +#endif //CONFIG_OF
> +
> +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 = regmap_add_irq_chip(bd71837->regmap, bd71837->chip_irq,
> + IRQF_ONESHOT, 0,
> + &bd71837_irq_chip, &bd71837->irq_data);
I think that you can use 'devm_regmap_add_irq_chip' here and remove
the code to delete the irq chip
> + if (ret < 0) {
> + dev_err(bd71837->dev, "Failed to add irq_chip %d\n", ret);
> + goto err_out;
> + }
> +
> + ret = mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
> + bd71837_mfd_cells, ARRAY_SIZE(bd71837_mfd_cells),
> + NULL, 0,
> + regmap_irq_get_domain(bd71837->irq_data));
Same here, I think you can use the devm_mfd_add_devices.
> + if (ret)
> + regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);
If you use devm_ functions you can remove this.
> +err_out:
> +
> + return ret;
> +}
> +
> +static int bd71837_i2c_remove(struct i2c_client *i2c)
> +{
> + struct bd71837 *bd71837 = i2c_get_clientdata(i2c);
> +
> + bd71837_irq_exit(bd71837);
> + mfd_remove_devices(bd71837->dev);
All this can go away if you use the devm_ calls.
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id bd71837_i2c_id[] = {
> + { .name = "bd71837", },
> + { }
nit: { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id);
> +
> +static struct i2c_driver bd71837_i2c_driver = {
> + .driver = {
> + .name = "bd71837-mfd",
> + .owner = THIS_MODULE,
Remove this, it is not needed, the core does it for you.
> + .of_match_table = of_match_ptr(bd71837_of_match),
The driver is DT-only, don't need to call of_match_ptr.
> + },
> + .probe = bd71837_i2c_probe,
> + .remove = bd71837_i2c_remove,
> + .id_table = bd71837_i2c_id,
> +};
> +
> +static int __init bd71837_i2c_init(void)
> +{
> + return i2c_add_driver(&bd71837_i2c_driver);
> +}
> +/* init early so consumer devices can complete system boot */
> +subsys_initcall(bd71837_i2c_init);
> +
> +static void __exit bd71837_i2c_exit(void)
> +{
> + i2c_del_driver(&bd71837_i2c_driver);
> +}
> +module_exit(bd71837_i2c_exit);
> +
Can't you use module_i2c_driver here and get rid of init/exit calls?
> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> +MODULE_DESCRIPTION("BD71837 chip multi-function driver");
> +MODULE_LICENSE("GPL");
License mismatch.
> diff --git a/include/linux/mfd/bd71837.h b/include/linux/mfd/bd71837.h
> new file mode 100644
> index 000000000000..125c7478ec29
> --- /dev/null
> +++ b/include/linux/mfd/bd71837.h
> @@ -0,0 +1,367 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2018 ROHM Semiconductors */
> +
> +/*
> + * ROHM BD71837MWV header file
> + */
> +
> +#ifndef __LINUX_MFD_BD71837_H__
> +#define __LINUX_MFD_BD71837_H__
> +
> +#include <linux/regmap.h>
> +
> +enum {
> + BD71837_BUCK1 = 0,
> + BD71837_BUCK2,
> + BD71837_BUCK3,
> + BD71837_BUCK4,
> + BD71837_BUCK5,
> + BD71837_BUCK6,
> + BD71837_BUCK7,
> + BD71837_BUCK8,
> + BD71837_LDO1,
> + BD71837_LDO2,
> + BD71837_LDO3,
> + BD71837_LDO4,
> + BD71837_LDO5,
> + BD71837_LDO6,
> + BD71837_LDO7,
> + BD71837_REGULATOR_CNT,
> +};
> +
> +#define BD71837_BUCK1_VOLTAGE_NUM 0x40
> +#define BD71837_BUCK2_VOLTAGE_NUM 0x40
> +#define BD71837_BUCK3_VOLTAGE_NUM 0x40
> +#define BD71837_BUCK4_VOLTAGE_NUM 0x40
> +
> +#define BD71837_BUCK5_VOLTAGE_NUM 0x08
> +#define BD71837_BUCK6_VOLTAGE_NUM 0x04
> +#define BD71837_BUCK7_VOLTAGE_NUM 0x08
> +#define BD71837_BUCK8_VOLTAGE_NUM 0x40
> +
> +#define BD71837_LDO1_VOLTAGE_NUM 0x04
> +#define BD71837_LDO2_VOLTAGE_NUM 0x02
> +#define BD71837_LDO3_VOLTAGE_NUM 0x10
> +#define BD71837_LDO4_VOLTAGE_NUM 0x10
> +#define BD71837_LDO5_VOLTAGE_NUM 0x10
> +#define BD71837_LDO6_VOLTAGE_NUM 0x10
> +#define BD71837_LDO7_VOLTAGE_NUM 0x10
> +
> +enum {
> + BD71837_REG_REV = 0x00,
> + BD71837_REG_SWRESET = 0x01,
> + BD71837_REG_I2C_DEV = 0x02,
> + BD71837_REG_PWRCTRL0 = 0x03,
> + BD71837_REG_PWRCTRL1 = 0x04,
> + BD71837_REG_BUCK1_CTRL = 0x05,
> + BD71837_REG_BUCK2_CTRL = 0x06,
> + BD71837_REG_BUCK3_CTRL = 0x07,
> + BD71837_REG_BUCK4_CTRL = 0x08,
> + BD71837_REG_BUCK5_CTRL = 0x09,
> + BD71837_REG_BUCK6_CTRL = 0x0A,
> + BD71837_REG_BUCK7_CTRL = 0x0B,
> + BD71837_REG_BUCK8_CTRL = 0x0C,
> + BD71837_REG_BUCK1_VOLT_RUN = 0x0D,
> + BD71837_REG_BUCK1_VOLT_IDLE = 0x0E,
> + BD71837_REG_BUCK1_VOLT_SUSP = 0x0F,
> + BD71837_REG_BUCK2_VOLT_RUN = 0x10,
> + BD71837_REG_BUCK2_VOLT_IDLE = 0x11,
> + BD71837_REG_BUCK3_VOLT_RUN = 0x12,
> + BD71837_REG_BUCK4_VOLT_RUN = 0x13,
> + BD71837_REG_BUCK5_VOLT = 0x14,
> + BD71837_REG_BUCK6_VOLT = 0x15,
> + BD71837_REG_BUCK7_VOLT = 0x16,
> + BD71837_REG_BUCK8_VOLT = 0x17,
> + BD71837_REG_LDO1_VOLT = 0x18,
> + BD71837_REG_LDO2_VOLT = 0x19,
> + BD71837_REG_LDO3_VOLT = 0x1A,
> + BD71837_REG_LDO4_VOLT = 0x1B,
> + BD71837_REG_LDO5_VOLT = 0x1C,
> + BD71837_REG_LDO6_VOLT = 0x1D,
> + BD71837_REG_LDO7_VOLT = 0x1E,
> + BD71837_REG_TRANS_COND0 = 0x1F,
> + BD71837_REG_TRANS_COND1 = 0x20,
> + BD71837_REG_VRFAULTEN = 0x21,
> + BD71837_REG_MVRFLTMASK0 = 0x22,
> + BD71837_REG_MVRFLTMASK1 = 0x23,
> + BD71837_REG_MVRFLTMASK2 = 0x24,
> + BD71837_REG_RCVCFG = 0x25,
> + BD71837_REG_RCVNUM = 0x26,
> + BD71837_REG_PWRONCONFIG0 = 0x27,
> + BD71837_REG_PWRONCONFIG1 = 0x28,
> + BD71837_REG_RESETSRC = 0x29,
> + BD71837_REG_MIRQ = 0x2A,
> + BD71837_REG_IRQ = 0x2B,
> + BD71837_REG_IN_MON = 0x2C,
> + BD71837_REG_POW_STATE = 0x2D,
> + BD71837_REG_OUT32K = 0x2E,
> + BD71837_REG_REGLOCK = 0x2F,
> + BD71837_REG_OTPVER = 0xFF,
> + BD71837_MAX_REGISTER = 0x100,
> +};
> +
> +#define REGLOCK_PWRSEQ 0x1
> +#define REGLOCK_VREG 0x10
> +
> +/* Generic BUCK control masks */
> +#define BD71837_BUCK_SEL 0x02
> +#define BD71837_BUCK_EN 0x01
> +#define BD71837_BUCK_RUN_ON 0x04
> +
> +/* Generic LDO masks */
> +#define BD71837_LDO_SEL 0x80
> +#define BD71837_LDO_EN 0x40
> +
> +/* BD71837 BUCK ramp rate CTRL reg bits */
> +#define BUCK_RAMPRATE_MASK 0xC0
> +#define BUCK_RAMPRATE_10P00MV 0x0
> +#define BUCK_RAMPRATE_5P00MV 0x1
> +#define BUCK_RAMPRATE_2P50MV 0x2
> +#define BUCK_RAMPRATE_1P25MV 0x3
> +
> +/* BD71837_REG_BUCK1_VOLT_RUN bits */
> +#define BUCK1_RUN_MASK 0x3F
> +#define BUCK1_RUN_DEFAULT 0x14
> +
> +/* BD71837_REG_BUCK1_VOLT_SUSP bits */
> +#define BUCK1_SUSP_MASK 0x3F
> +#define BUCK1_SUSP_DEFAULT 0x14
> +
> +/* BD71837_REG_BUCK1_VOLT_IDLE bits */
> +#define BUCK1_IDLE_MASK 0x3F
> +#define BUCK1_IDLE_DEFAULT 0x14
> +
> +/* BD71837_REG_BUCK2_VOLT_RUN bits */
> +#define BUCK2_RUN_MASK 0x3F
> +#define BUCK2_RUN_DEFAULT 0x1E
> +
> +/* BD71837_REG_BUCK2_VOLT_IDLE bits */
> +#define BUCK2_IDLE_MASK 0x3F
> +#define BUCK2_IDLE_DEFAULT 0x14
> +
> +/* BD71837_REG_BUCK3_VOLT_RUN bits */
> +#define BUCK3_RUN_MASK 0x3F
> +#define BUCK3_RUN_DEFAULT 0x1E
> +
> +/* BD71837_REG_BUCK4_VOLT_RUN bits */
> +#define BUCK4_RUN_MASK 0x3F
> +#define BUCK4_RUN_DEFAULT 0x1E
> +
> +/* BD71837_REG_BUCK5_VOLT bits */
> +#define BUCK5_MASK 0x07
> +#define BUCK5_DEFAULT 0x02
> +
> +/* BD71837_REG_BUCK6_VOLT bits */
> +#define BUCK6_MASK 0x03
> +#define BUCK6_DEFAULT 0x03
> +
> +/* BD71837_REG_BUCK7_VOLT bits */
> +#define BUCK7_MASK 0x07
> +#define BUCK7_DEFAULT 0x03
> +
> +/* BD71837_REG_BUCK8_VOLT bits */
> +#define BUCK8_MASK 0x3F
> +#define BUCK8_DEFAULT 0x1E
> +
> +/* BD71837_REG_IRQ bits */
> +#define IRQ_SWRST 0x40
> +#define IRQ_PWRON_S 0x20
> +#define IRQ_PWRON_L 0x10
> +#define IRQ_PWRON 0x08
> +#define IRQ_WDOG 0x04
> +#define IRQ_ON_REQ 0x02
> +#define IRQ_STBY_REQ 0x01
> +
> +/* BD71837_REG_OUT32K bits */
> +#define BD71837_OUT32K_EN 0x01
> +
> +/* BD71837 gated clock rate */
> +#define BD71837_CLK_RATE 32768
> +
> +/* BD71837 irqs */
> +enum {
> + BD71837_INT_STBY_REQ,
> + BD71837_INT_ON_REQ,
> + BD71837_INT_WDOG,
> + BD71837_INT_PWRBTN,
> + BD71837_INT_PWRBTN_L,
> + BD71837_INT_PWRBTN_S,
> + BD71837_INT_SWRST
> +};
> +
> +/* BD71837 interrupt masks */
> +#define BD71837_INT_SWRST_MASK 0x40
> +#define BD71837_INT_PWRBTN_S_MASK 0x20
> +#define BD71837_INT_PWRBTN_L_MASK 0x10
> +#define BD71837_INT_PWRBTN_MASK 0x8
> +#define BD71837_INT_WDOG_MASK 0x4
> +#define BD71837_INT_ON_REQ_MASK 0x2
> +#define BD71837_INT_STBY_REQ_MASK 0x1
> +
> +/* BD71837_REG_LDO1_VOLT bits */
> +#define LDO1_MASK 0x03
> +
> +/* BD71837_REG_LDO1_VOLT bits */
> +#define LDO2_MASK 0x20
> +
> +/* BD71837_REG_LDO3_VOLT bits */
> +#define LDO3_MASK 0x0F
> +
> +/* BD71837_REG_LDO4_VOLT bits */
> +#define LDO4_MASK 0x0F
> +
> +/* BD71837_REG_LDO5_VOLT bits */
> +#define LDO5_MASK 0x0F
> +
> +/* BD71837_REG_LDO6_VOLT bits */
> +#define LDO6_MASK 0x0F
> +
> +/* BD71837_REG_LDO7_VOLT bits */
> +#define LDO7_MASK 0x0F
> +
> +/* register write induced reset settings */
/* Register ...
> +
> +/* even though the bit zero is not SWRESET type we still want to write zero
/*
* Even
> + * to it when changing type. Biz zero is 'SWRESET' trigger bit and if we
s/Biz/Bit/
> + * 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
> +
> +/* Timeout value for detecting short press */
> +
> +#define BD718XX_PWRBTN_SHORT_PRESS_MASK 0xF
> +
> +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
> +};
> +
> +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
> + */
> +
All these access routines only hide the regmap_* calls, so why not use
the regmap calls directly in the driver and remove all this?
> +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] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-06-26 9:06 ` Enric Balletbo Serra
@ 2018-06-26 11:24 ` Matti Vaittinen
-1 siblings, 0 replies; 45+ messages in thread
From: Matti Vaittinen @ 2018-06-26 11:24 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Michael Turquette, sboyd, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, mazziesaccount, Arnd Bergmann,
Dmitry Torokhov, Sebastian Reichel, chenjh, andrew.smirnov,
Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree, linux-kernel,
linux-input, mikko.mutanen, heikki.haikola
Hello Eric,
Thanks for the comments! I'll be addressing these in patch series v8
- except the regmap wrapper one which will be taken care of by another
patch set.
On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> Hi Matti,
>
> Thanks for the patch, a few comments below, some are feedback I
> received when I sent some patches to this subsystem.
>
> Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> dia dt., 19 de juny 2018 a les 12:57:
> >
> > ROHM BD71837 PMIC MFD driver providing interrupts and support
> > for two subsystems:
> > - clk
> > - Regulators
> >
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > ---
> > drivers/mfd/Kconfig | 13 ++
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/bd71837.c | 221 ++++++++++++++++++++++++++
> > include/linux/mfd/bd71837.h | 367 ++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 602 insertions(+)
> > create mode 100644 drivers/mfd/bd71837.c
> > create mode 100644 include/linux/mfd/bd71837.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index b860eb5aa194..7aa05fc9ed8e 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1787,6 +1787,19 @@ config MFD_STW481X
> > in various ST Microelectronics and ST-Ericsson embedded
> > Nomadik series.
> >
> > +config MFD_BD71837
> > + bool "BD71837 Power Management chip"
>
> I know that some drivers need to be built-in, is this really a
> requirement for this driver? Or should work as a module too.
>
I have been writing power/reset driver for this PMIC. I thought that the
modules would be unload before reset hook is ran - which seems to be
not true on platform where I tested this. So you are correct, at least
on cases where reset is not used via PMIC - or where the platform is not
unloading modules prior reset - these drivers can indeed be modules. So
it is fair to allow building them as modules. Users who want to build
this in kernel can still do so. => I'll change this.
> > + 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..0f0361d6cad6
> > --- /dev/null
> > +++ b/drivers/mfd/bd71837.c
> > @@ -0,0 +1,221 @@
> > +// SPDX-License-Identifier: GPL-2.0
>
> There is a mismatch between what SPDX says and MODULE_LICENSE says.
>
> GPL-2.0 = GPL v2 only
> MODULE_LICENSE(GPL) = GPL v2 or later.
>
> You'd like to change the SPDX identifier to GPL-2.0-or-later or set
> module license to "GPL v2".
Right. Thanks for pointing that out.
>
> > +// Copyright (C) 2018 ROHM Semiconductors
> > +// bd71837.c -- ROHM BD71837MWV mfd driver
> > +//
> > +// Datasheet available from
> > +// https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
> > +
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> This include is not required.
>
> > +#include <linux/init.h>
> > +#include <linux/slab.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> ditto
>
> > +#include <linux/irqdomain.h>
> ditto
>
> > +#include <linux/gpio.h>
> ditto
>
> > +#include <linux/regmap.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_gpio.h>
> ditto
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/bd71837.h>
> > +
>
> Please review the needed includes.
I'll do that, thanks.
> > +static const struct resource irqs[] = {
> > + {
> > + .start = BD71837_INT_PWRBTN,
> > + .end = BD71837_INT_PWRBTN,
> > + .flags = IORESOURCE_IRQ,
> > + .name = "pwr-btn",
> > + }, {
> > + .start = BD71837_INT_PWRBTN_L,
> > + .end = BD71837_INT_PWRBTN_L,
> > + .flags = IORESOURCE_IRQ,
> > + .name = "pwr-btn-l",
> > + }, {
> > + .start = BD71837_INT_PWRBTN_S,
> > + .end = BD71837_INT_PWRBTN_S,
> > + .flags = IORESOURCE_IRQ,
> > + .name = "pwr-btn-s",
> > + },
>
> nit: no comma at the end
Whole struct will be removed. I will use gpio-keys and remove the
bd718xx-pwrkey driver as suggested by Dmitry Torokhov.
> > +};
> > +
> > +/* bd71837 multi function cells */
> > +static struct mfd_cell bd71837_mfd_cells[] = {
> > + {
> > + .name = "bd71837-clk",
> > + }, {
> > + .name = "bd718xx-pwrkey",
> > + .resources = &irqs[0],
> > + .num_resources = ARRAY_SIZE(irqs),
> > + }, {
> > + .name = "bd71837-pmic",
> > + },
> nit: no comma at the end
Ok.
> > +};
> > +
> > +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 int bd71837_irq_exit(struct bd71837 *bd71837)
> > +{
> > + if (bd71837->chip_irq > 0)
> > + regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);
> > + return 0;
> > +}
> > +
> > +static const struct regmap_range pmic_status_range = {
> > + .range_min = BD71837_REG_IRQ,
> > + .range_max = BD71837_REG_POW_STATE,
> > +};
> > +
> > +static const struct regmap_access_table volatile_regs = {
> > + .yes_ranges = &pmic_status_range,
> > + .n_yes_ranges = 1,
> > +};
> > +
> > +static const struct regmap_config bd71837_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .volatile_table = &volatile_regs,
> > + .max_register = BD71837_MAX_REGISTER - 1,
> > + .cache_type = REGCACHE_RBTREE,
> > +};
> > +
> > +#ifdef CONFIG_OF
> The driver is DT-only and depends on OF so you don't need to protect this.
True. I'll remove this
> > +static const struct of_device_id bd71837_of_match[] = {
> > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > + { },
>
> nit: { /* sentinel */ }
I am sorry but I didn't quite get the point here. Could you please
explain what did you expect to be added here?
> > +};
> > +MODULE_DEVICE_TABLE(of, bd71837_of_match);
> > +#endif //CONFIG_OF
> > +
> > +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 = regmap_add_irq_chip(bd71837->regmap, bd71837->chip_irq,
> > + IRQF_ONESHOT, 0,
> > + &bd71837_irq_chip, &bd71837->irq_data);
>
> I think that you can use 'devm_regmap_add_irq_chip' here and remove
> the code to delete the irq chip
Right. Good point. Makes this lot leaner and cleaner.
> > + if (ret < 0) {
> > + dev_err(bd71837->dev, "Failed to add irq_chip %d\n", ret);
> > + goto err_out;
> > + }
> > +
> > + ret = mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
> > + bd71837_mfd_cells, ARRAY_SIZE(bd71837_mfd_cells),
> > + NULL, 0,
> > + regmap_irq_get_domain(bd71837->irq_data));
>
> Same here, I think you can use the devm_mfd_add_devices.
Right. Good point. Makes this lot leaner and cleaner.
> > +
> > + return 0;
> > +}
> > +
> > +static const struct i2c_device_id bd71837_i2c_id[] = {
> > + { .name = "bd71837", },
> > + { }
>
> nit: { /* sentinel */ }
I am sorry but I didn't quite get the point here. Could you please
explain what did you expect to be added here?
> > +};
> > +MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id);
> > +
> > +static struct i2c_driver bd71837_i2c_driver = {
> > + .driver = {
> > + .name = "bd71837-mfd",
> > + .owner = THIS_MODULE,
>
> Remove this, it is not needed, the core does it for you.
True. Thanks.
> > + .of_match_table = of_match_ptr(bd71837_of_match),
>
> The driver is DT-only, don't need to call of_match_ptr.
Ok.
> > + },
> > + .probe = bd71837_i2c_probe,
> > + .remove = bd71837_i2c_remove,
> > + .id_table = bd71837_i2c_id,
> > +};
> > +
> > +static int __init bd71837_i2c_init(void)
> > +{
> > + return i2c_add_driver(&bd71837_i2c_driver);
> > +}
> > +/* init early so consumer devices can complete system boot */
> > +subsys_initcall(bd71837_i2c_init);
> > +
> > +static void __exit bd71837_i2c_exit(void)
> > +{
> > + i2c_del_driver(&bd71837_i2c_driver);
> > +}
> > +module_exit(bd71837_i2c_exit);
> > +
>
> Can't you use module_i2c_driver here and get rid of init/exit calls?
I did this because of subsys_initcall() and how other drivers had used
that.
> > +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> > +MODULE_DESCRIPTION("BD71837 chip multi-function driver");
> > +MODULE_LICENSE("GPL");
>
> License mismatch.
Thanks again
> > diff --git a/include/linux/mfd/bd71837.h b/include/linux/mfd/bd71837.h
> > new file mode 100644
> > index 000000000000..125c7478ec29
> > --- /dev/null
> > +++ b/include/linux/mfd/bd71837.h
> > @@ -0,0 +1,367 @@
> > +
> > +/* register write induced reset settings */
>
> /* Register ...
Ok.
> > +
> > +/* even though the bit zero is not SWRESET type we still want to write zero
>
> /*
> * Even
Ok.
> > + * to it when changing type. Biz zero is 'SWRESET' trigger bit and if we
>
> s/Biz/Bit/
Ok.
> > +/*
> > + * bd71837 sub-driver chip access routines
> > + */
> > +
>
> All these access routines only hide the regmap_* calls, so why not use
> the regmap calls directly in the driver and remove all this?
This was also discussed with Stephen during the review for clk patches:
https://lore.kernel.org/lkml/152997029783.143105.16692843405899913246@swboyd.mtv.corp.google.com/
I will later send a new patch set which only removes these wrappers from
this MFD and also the submodules. I like to introduce that as a separate
change set after the whole driver is applied as it impacts to some already
applied parts. I don't want any mismatches which jump out as compile errors
when MFD gets applied and config dependencies get solved.
But yes, you are correct - the write/read to BD71837 is plain regmap access
and same simple access seems to be working with the BD71847 when it comes.
So these are not needed and will be removed.
Br,
Matti Vaittinen
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-06-26 11:24 ` Matti Vaittinen
0 siblings, 0 replies; 45+ messages in thread
From: Matti Vaittinen @ 2018-06-26 11:24 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Michael Turquette, sboyd, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, mazziesaccount, Arnd Bergmann,
Dmitry Torokhov, Sebastian Reichel, chenjh, andrew.smirnov,
Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree, linux-kernel,
linux-input
Hello Eric,
Thanks for the comments! I'll be addressing these in patch series v8
- except the regmap wrapper one which will be taken care of by another
patch set.
On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> Hi Matti,
>
> Thanks for the patch, a few comments below, some are feedback I
> received when I sent some patches to this subsystem.
>
> Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> dia dt., 19 de juny 2018 a les 12:57:
> >
> > ROHM BD71837 PMIC MFD driver providing interrupts and support
> > for two subsystems:
> > - clk
> > - Regulators
> >
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > ---
> > drivers/mfd/Kconfig | 13 ++
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/bd71837.c | 221 ++++++++++++++++++++++++++
> > include/linux/mfd/bd71837.h | 367 ++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 602 insertions(+)
> > create mode 100644 drivers/mfd/bd71837.c
> > create mode 100644 include/linux/mfd/bd71837.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index b860eb5aa194..7aa05fc9ed8e 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1787,6 +1787,19 @@ config MFD_STW481X
> > in various ST Microelectronics and ST-Ericsson embedded
> > Nomadik series.
> >
> > +config MFD_BD71837
> > + bool "BD71837 Power Management chip"
>
> I know that some drivers need to be built-in, is this really a
> requirement for this driver? Or should work as a module too.
>
I have been writing power/reset driver for this PMIC. I thought that the
modules would be unload before reset hook is ran - which seems to be
not true on platform where I tested this. So you are correct, at least
on cases where reset is not used via PMIC - or where the platform is not
unloading modules prior reset - these drivers can indeed be modules. So
it is fair to allow building them as modules. Users who want to build
this in kernel can still do so. => I'll change this.
> > + 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..0f0361d6cad6
> > --- /dev/null
> > +++ b/drivers/mfd/bd71837.c
> > @@ -0,0 +1,221 @@
> > +// SPDX-License-Identifier: GPL-2.0
>
> There is a mismatch between what SPDX says and MODULE_LICENSE says.
>
> GPL-2.0 = GPL v2 only
> MODULE_LICENSE(GPL) = GPL v2 or later.
>
> You'd like to change the SPDX identifier to GPL-2.0-or-later or set
> module license to "GPL v2".
Right. Thanks for pointing that out.
>
> > +// Copyright (C) 2018 ROHM Semiconductors
> > +// bd71837.c -- ROHM BD71837MWV mfd driver
> > +//
> > +// Datasheet available from
> > +// https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
> > +
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> This include is not required.
>
> > +#include <linux/init.h>
> > +#include <linux/slab.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> ditto
>
> > +#include <linux/irqdomain.h>
> ditto
>
> > +#include <linux/gpio.h>
> ditto
>
> > +#include <linux/regmap.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_gpio.h>
> ditto
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/bd71837.h>
> > +
>
> Please review the needed includes.
I'll do that, thanks.
> > +static const struct resource irqs[] = {
> > + {
> > + .start = BD71837_INT_PWRBTN,
> > + .end = BD71837_INT_PWRBTN,
> > + .flags = IORESOURCE_IRQ,
> > + .name = "pwr-btn",
> > + }, {
> > + .start = BD71837_INT_PWRBTN_L,
> > + .end = BD71837_INT_PWRBTN_L,
> > + .flags = IORESOURCE_IRQ,
> > + .name = "pwr-btn-l",
> > + }, {
> > + .start = BD71837_INT_PWRBTN_S,
> > + .end = BD71837_INT_PWRBTN_S,
> > + .flags = IORESOURCE_IRQ,
> > + .name = "pwr-btn-s",
> > + },
>
> nit: no comma at the end
Whole struct will be removed. I will use gpio-keys and remove the
bd718xx-pwrkey driver as suggested by Dmitry Torokhov.
> > +};
> > +
> > +/* bd71837 multi function cells */
> > +static struct mfd_cell bd71837_mfd_cells[] = {
> > + {
> > + .name = "bd71837-clk",
> > + }, {
> > + .name = "bd718xx-pwrkey",
> > + .resources = &irqs[0],
> > + .num_resources = ARRAY_SIZE(irqs),
> > + }, {
> > + .name = "bd71837-pmic",
> > + },
> nit: no comma at the end
Ok.
> > +};
> > +
> > +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 int bd71837_irq_exit(struct bd71837 *bd71837)
> > +{
> > + if (bd71837->chip_irq > 0)
> > + regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);
> > + return 0;
> > +}
> > +
> > +static const struct regmap_range pmic_status_range = {
> > + .range_min = BD71837_REG_IRQ,
> > + .range_max = BD71837_REG_POW_STATE,
> > +};
> > +
> > +static const struct regmap_access_table volatile_regs = {
> > + .yes_ranges = &pmic_status_range,
> > + .n_yes_ranges = 1,
> > +};
> > +
> > +static const struct regmap_config bd71837_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .volatile_table = &volatile_regs,
> > + .max_register = BD71837_MAX_REGISTER - 1,
> > + .cache_type = REGCACHE_RBTREE,
> > +};
> > +
> > +#ifdef CONFIG_OF
> The driver is DT-only and depends on OF so you don't need to protect this.
True. I'll remove this
> > +static const struct of_device_id bd71837_of_match[] = {
> > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > + { },
>
> nit: { /* sentinel */ }
I am sorry but I didn't quite get the point here. Could you please
explain what did you expect to be added here?
> > +};
> > +MODULE_DEVICE_TABLE(of, bd71837_of_match);
> > +#endif //CONFIG_OF
> > +
> > +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 = regmap_add_irq_chip(bd71837->regmap, bd71837->chip_irq,
> > + IRQF_ONESHOT, 0,
> > + &bd71837_irq_chip, &bd71837->irq_data);
>
> I think that you can use 'devm_regmap_add_irq_chip' here and remove
> the code to delete the irq chip
Right. Good point. Makes this lot leaner and cleaner.
> > + if (ret < 0) {
> > + dev_err(bd71837->dev, "Failed to add irq_chip %d\n", ret);
> > + goto err_out;
> > + }
> > +
> > + ret = mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
> > + bd71837_mfd_cells, ARRAY_SIZE(bd71837_mfd_cells),
> > + NULL, 0,
> > + regmap_irq_get_domain(bd71837->irq_data));
>
> Same here, I think you can use the devm_mfd_add_devices.
Right. Good point. Makes this lot leaner and cleaner.
> > +
> > + return 0;
> > +}
> > +
> > +static const struct i2c_device_id bd71837_i2c_id[] = {
> > + { .name = "bd71837", },
> > + { }
>
> nit: { /* sentinel */ }
I am sorry but I didn't quite get the point here. Could you please
explain what did you expect to be added here?
> > +};
> > +MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id);
> > +
> > +static struct i2c_driver bd71837_i2c_driver = {
> > + .driver = {
> > + .name = "bd71837-mfd",
> > + .owner = THIS_MODULE,
>
> Remove this, it is not needed, the core does it for you.
True. Thanks.
> > + .of_match_table = of_match_ptr(bd71837_of_match),
>
> The driver is DT-only, don't need to call of_match_ptr.
Ok.
> > + },
> > + .probe = bd71837_i2c_probe,
> > + .remove = bd71837_i2c_remove,
> > + .id_table = bd71837_i2c_id,
> > +};
> > +
> > +static int __init bd71837_i2c_init(void)
> > +{
> > + return i2c_add_driver(&bd71837_i2c_driver);
> > +}
> > +/* init early so consumer devices can complete system boot */
> > +subsys_initcall(bd71837_i2c_init);
> > +
> > +static void __exit bd71837_i2c_exit(void)
> > +{
> > + i2c_del_driver(&bd71837_i2c_driver);
> > +}
> > +module_exit(bd71837_i2c_exit);
> > +
>
> Can't you use module_i2c_driver here and get rid of init/exit calls?
I did this because of subsys_initcall() and how other drivers had used
that.
> > +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> > +MODULE_DESCRIPTION("BD71837 chip multi-function driver");
> > +MODULE_LICENSE("GPL");
>
> License mismatch.
Thanks again
> > diff --git a/include/linux/mfd/bd71837.h b/include/linux/mfd/bd71837.h
> > new file mode 100644
> > index 000000000000..125c7478ec29
> > --- /dev/null
> > +++ b/include/linux/mfd/bd71837.h
> > @@ -0,0 +1,367 @@
> > +
> > +/* register write induced reset settings */
>
> /* Register ...
Ok.
> > +
> > +/* even though the bit zero is not SWRESET type we still want to write zero
>
> /*
> * Even
Ok.
> > + * to it when changing type. Biz zero is 'SWRESET' trigger bit and if we
>
> s/Biz/Bit/
Ok.
> > +/*
> > + * bd71837 sub-driver chip access routines
> > + */
> > +
>
> All these access routines only hide the regmap_* calls, so why not use
> the regmap calls directly in the driver and remove all this?
This was also discussed with Stephen during the review for clk patches:
https://lore.kernel.org/lkml/152997029783.143105.16692843405899913246@swboyd.mtv.corp.google.com/
I will later send a new patch set which only removes these wrappers from
this MFD and also the submodules. I like to introduce that as a separate
change set after the whole driver is applied as it impacts to some already
applied parts. I don't want any mismatches which jump out as compile errors
when MFD gets applied and config dependencies get solved.
But yes, you are correct - the write/read to BD71837 is plain regmap access
and same simple access seems to be working with the BD71847 when it comes.
So these are not needed and will be removed.
Br,
Matti Vaittinen
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-06-26 11:24 ` Matti Vaittinen
@ 2018-06-26 11:40 ` Enric Balletbo Serra
-1 siblings, 0 replies; 45+ messages in thread
From: Enric Balletbo Serra @ 2018-06-26 11:40 UTC (permalink / raw)
To: matti.vaittinen
Cc: Michael Turquette, sboyd, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, mazziesaccount, Arnd Bergmann,
Dmitry Torokhov, Sebastian Reichel, chenjh, andrew.smirnov,
Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree, linux-kernel,
linux-input, mikko.mutanen, heikki.haikola
Hi Matti,
Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
dia dt., 26 de juny 2018 a les 13:25:
>
> Hello Eric,
>
> Thanks for the comments! I'll be addressing these in patch series v8
> - except the regmap wrapper one which will be taken care of by another
> patch set.
>
> On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> > Hi Matti,
> >
> > Thanks for the patch, a few comments below, some are feedback I
> > received when I sent some patches to this subsystem.
> >
> > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > dia dt., 19 de juny 2018 a les 12:57:
> > >
> > > ROHM BD71837 PMIC MFD driver providing interrupts and support
> > > for two subsystems:
> > > - clk
> > > - Regulators
> > >
> > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > > ---
> > > drivers/mfd/Kconfig | 13 ++
> > > drivers/mfd/Makefile | 1 +
> > > drivers/mfd/bd71837.c | 221 ++++++++++++++++++++++++++
> > > include/linux/mfd/bd71837.h | 367 ++++++++++++++++++++++++++++++++++++++++++++
> > > 4 files changed, 602 insertions(+)
> > > create mode 100644 drivers/mfd/bd71837.c
> > > create mode 100644 include/linux/mfd/bd71837.h
> > >
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index b860eb5aa194..7aa05fc9ed8e 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -1787,6 +1787,19 @@ config MFD_STW481X
> > > in various ST Microelectronics and ST-Ericsson embedded
> > > Nomadik series.
> > >
> > > +config MFD_BD71837
> > > + bool "BD71837 Power Management chip"
> >
> > I know that some drivers need to be built-in, is this really a
> > requirement for this driver? Or should work as a module too.
> >
>
> I have been writing power/reset driver for this PMIC. I thought that the
> modules would be unload before reset hook is ran - which seems to be
> not true on platform where I tested this. So you are correct, at least
> on cases where reset is not used via PMIC - or where the platform is not
> unloading modules prior reset - these drivers can indeed be modules. So
> it is fair to allow building them as modules. Users who want to build
> this in kernel can still do so. => I'll change this.
>
> > > + 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..0f0361d6cad6
> > > --- /dev/null
> > > +++ b/drivers/mfd/bd71837.c
> > > @@ -0,0 +1,221 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> >
> > There is a mismatch between what SPDX says and MODULE_LICENSE says.
> >
> > GPL-2.0 = GPL v2 only
> > MODULE_LICENSE(GPL) = GPL v2 or later.
> >
> > You'd like to change the SPDX identifier to GPL-2.0-or-later or set
> > module license to "GPL v2".
>
> Right. Thanks for pointing that out.
>
> >
> > > +// Copyright (C) 2018 ROHM Semiconductors
> > > +// bd71837.c -- ROHM BD71837MWV mfd driver
> > > +//
> > > +// Datasheet available from
> > > +// https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/moduleparam.h>
> > This include is not required.
> >
> > > +#include <linux/init.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/irq.h>
> > ditto
> >
> > > +#include <linux/irqdomain.h>
> > ditto
> >
> > > +#include <linux/gpio.h>
> > ditto
> >
> > > +#include <linux/regmap.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/of_gpio.h>
> > ditto
> > > +#include <linux/mfd/core.h>
> > > +#include <linux/mfd/bd71837.h>
> > > +
> >
> > Please review the needed includes.
>
> I'll do that, thanks.
>
> > > +static const struct resource irqs[] = {
> > > + {
> > > + .start = BD71837_INT_PWRBTN,
> > > + .end = BD71837_INT_PWRBTN,
> > > + .flags = IORESOURCE_IRQ,
> > > + .name = "pwr-btn",
> > > + }, {
> > > + .start = BD71837_INT_PWRBTN_L,
> > > + .end = BD71837_INT_PWRBTN_L,
> > > + .flags = IORESOURCE_IRQ,
> > > + .name = "pwr-btn-l",
> > > + }, {
> > > + .start = BD71837_INT_PWRBTN_S,
> > > + .end = BD71837_INT_PWRBTN_S,
> > > + .flags = IORESOURCE_IRQ,
> > > + .name = "pwr-btn-s",
> > > + },
> >
> > nit: no comma at the end
>
> Whole struct will be removed. I will use gpio-keys and remove the
> bd718xx-pwrkey driver as suggested by Dmitry Torokhov.
>
> > > +};
> > > +
> > > +/* bd71837 multi function cells */
> > > +static struct mfd_cell bd71837_mfd_cells[] = {
> > > + {
> > > + .name = "bd71837-clk",
> > > + }, {
> > > + .name = "bd718xx-pwrkey",
> > > + .resources = &irqs[0],
> > > + .num_resources = ARRAY_SIZE(irqs),
> > > + }, {
> > > + .name = "bd71837-pmic",
> > > + },
> > nit: no comma at the end
>
> Ok.
>
> > > +};
> > > +
> > > +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 int bd71837_irq_exit(struct bd71837 *bd71837)
> > > +{
> > > + if (bd71837->chip_irq > 0)
> > > + regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);
> > > + return 0;
> > > +}
> > > +
> > > +static const struct regmap_range pmic_status_range = {
> > > + .range_min = BD71837_REG_IRQ,
> > > + .range_max = BD71837_REG_POW_STATE,
> > > +};
> > > +
> > > +static const struct regmap_access_table volatile_regs = {
> > > + .yes_ranges = &pmic_status_range,
> > > + .n_yes_ranges = 1,
> > > +};
> > > +
> > > +static const struct regmap_config bd71837_regmap_config = {
> > > + .reg_bits = 8,
> > > + .val_bits = 8,
> > > + .volatile_table = &volatile_regs,
> > > + .max_register = BD71837_MAX_REGISTER - 1,
> > > + .cache_type = REGCACHE_RBTREE,
> > > +};
> > > +
> > > +#ifdef CONFIG_OF
>
> > The driver is DT-only and depends on OF so you don't need to protect this.
> True. I'll remove this
>
> > > +static const struct of_device_id bd71837_of_match[] = {
> > > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > > + { },
> >
> > nit: { /* sentinel */ }
>
> I am sorry but I didn't quite get the point here. Could you please
> explain what did you expect to be added here?
>
It's a nit but It is a good practice to specify that the last entry is
a sentinel. Just this.
+static const struct of_device_id bd71837_of_match[] = {
+ { .compatible = "rohm,bd71837", .data = (void *)0},
+ { /* sentinel */ }
+};
And just noticed, is .data = (void *)0 really required?
> > > +};
> > > +MODULE_DEVICE_TABLE(of, bd71837_of_match);
> > > +#endif //CONFIG_OF
> > > +
> > > +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 = regmap_add_irq_chip(bd71837->regmap, bd71837->chip_irq,
> > > + IRQF_ONESHOT, 0,
> > > + &bd71837_irq_chip, &bd71837->irq_data);
> >
> > I think that you can use 'devm_regmap_add_irq_chip' here and remove
> > the code to delete the irq chip
>
> Right. Good point. Makes this lot leaner and cleaner.
>
> > > + if (ret < 0) {
> > > + dev_err(bd71837->dev, "Failed to add irq_chip %d\n", ret);
> > > + goto err_out;
> > > + }
> > > +
> > > + ret = mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
> > > + bd71837_mfd_cells, ARRAY_SIZE(bd71837_mfd_cells),
> > > + NULL, 0,
> > > + regmap_irq_get_domain(bd71837->irq_data));
> >
> > Same here, I think you can use the devm_mfd_add_devices.
>
> Right. Good point. Makes this lot leaner and cleaner.
>
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct i2c_device_id bd71837_i2c_id[] = {
> > > + { .name = "bd71837", },
> > > + { }
> >
> > nit: { /* sentinel */ }
>
> I am sorry but I didn't quite get the point here. Could you please
> explain what did you expect to be added here?
>
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id);
> > > +
> > > +static struct i2c_driver bd71837_i2c_driver = {
> > > + .driver = {
> > > + .name = "bd71837-mfd",
> > > + .owner = THIS_MODULE,
> >
> > Remove this, it is not needed, the core does it for you.
>
> True. Thanks.
>
> > > + .of_match_table = of_match_ptr(bd71837_of_match),
> >
> > The driver is DT-only, don't need to call of_match_ptr.
>
> Ok.
>
> > > + },
> > > + .probe = bd71837_i2c_probe,
> > > + .remove = bd71837_i2c_remove,
> > > + .id_table = bd71837_i2c_id,
> > > +};
> > > +
> > > +static int __init bd71837_i2c_init(void)
> > > +{
> > > + return i2c_add_driver(&bd71837_i2c_driver);
> > > +}
> > > +/* init early so consumer devices can complete system boot */
> > > +subsys_initcall(bd71837_i2c_init);
> > > +
> > > +static void __exit bd71837_i2c_exit(void)
> > > +{
> > > + i2c_del_driver(&bd71837_i2c_driver);
> > > +}
> > > +module_exit(bd71837_i2c_exit);
> > > +
> >
> > Can't you use module_i2c_driver here and get rid of init/exit calls?
>
> I did this because of subsys_initcall() and how other drivers had used
> that.
>
> > > +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> > > +MODULE_DESCRIPTION("BD71837 chip multi-function driver");
> > > +MODULE_LICENSE("GPL");
> >
> > License mismatch.
>
> Thanks again
>
> > > diff --git a/include/linux/mfd/bd71837.h b/include/linux/mfd/bd71837.h
> > > new file mode 100644
> > > index 000000000000..125c7478ec29
> > > --- /dev/null
> > > +++ b/include/linux/mfd/bd71837.h
> > > @@ -0,0 +1,367 @@
> > > +
> > > +/* register write induced reset settings */
> >
> > /* Register ...
>
> Ok.
>
> > > +
> > > +/* even though the bit zero is not SWRESET type we still want to write zero
> >
> > /*
> > * Even
>
> Ok.
>
> > > + * to it when changing type. Biz zero is 'SWRESET' trigger bit and if we
> >
> > s/Biz/Bit/
>
> Ok.
>
> > > +/*
> > > + * bd71837 sub-driver chip access routines
> > > + */
> > > +
> >
> > All these access routines only hide the regmap_* calls, so why not use
> > the regmap calls directly in the driver and remove all this?
>
> This was also discussed with Stephen during the review for clk patches:
> https://lore.kernel.org/lkml/152997029783.143105.16692843405899913246@swboyd.mtv.corp.google.com/
>
> I will later send a new patch set which only removes these wrappers from
> this MFD and also the submodules. I like to introduce that as a separate
> change set after the whole driver is applied as it impacts to some already
> applied parts. I don't want any mismatches which jump out as compile errors
> when MFD gets applied and config dependencies get solved.
>
> But yes, you are correct - the write/read to BD71837 is plain regmap access
> and same simple access seems to be working with the BD71847 when it comes.
> So these are not needed and will be removed.
>
> Br,
> Matti Vaittinen
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-06-26 11:40 ` Enric Balletbo Serra
0 siblings, 0 replies; 45+ messages in thread
From: Enric Balletbo Serra @ 2018-06-26 11:40 UTC (permalink / raw)
To: matti.vaittinen
Cc: Michael Turquette, sboyd, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, mazziesaccount, Arnd Bergmann,
Dmitry Torokhov, Sebastian Reichel, chenjh, andrew.smirnov,
Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree, linux-kernel,
linux-input
Hi Matti,
Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
dia dt., 26 de juny 2018 a les 13:25:
>
> Hello Eric,
>
> Thanks for the comments! I'll be addressing these in patch series v8
> - except the regmap wrapper one which will be taken care of by another
> patch set.
>
> On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> > Hi Matti,
> >
> > Thanks for the patch, a few comments below, some are feedback I
> > received when I sent some patches to this subsystem.
> >
> > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > dia dt., 19 de juny 2018 a les 12:57:
> > >
> > > ROHM BD71837 PMIC MFD driver providing interrupts and support
> > > for two subsystems:
> > > - clk
> > > - Regulators
> > >
> > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > > ---
> > > drivers/mfd/Kconfig | 13 ++
> > > drivers/mfd/Makefile | 1 +
> > > drivers/mfd/bd71837.c | 221 ++++++++++++++++++++++++++
> > > include/linux/mfd/bd71837.h | 367 ++++++++++++++++++++++++++++++++++++++++++++
> > > 4 files changed, 602 insertions(+)
> > > create mode 100644 drivers/mfd/bd71837.c
> > > create mode 100644 include/linux/mfd/bd71837.h
> > >
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index b860eb5aa194..7aa05fc9ed8e 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -1787,6 +1787,19 @@ config MFD_STW481X
> > > in various ST Microelectronics and ST-Ericsson embedded
> > > Nomadik series.
> > >
> > > +config MFD_BD71837
> > > + bool "BD71837 Power Management chip"
> >
> > I know that some drivers need to be built-in, is this really a
> > requirement for this driver? Or should work as a module too.
> >
>
> I have been writing power/reset driver for this PMIC. I thought that the
> modules would be unload before reset hook is ran - which seems to be
> not true on platform where I tested this. So you are correct, at least
> on cases where reset is not used via PMIC - or where the platform is not
> unloading modules prior reset - these drivers can indeed be modules. So
> it is fair to allow building them as modules. Users who want to build
> this in kernel can still do so. => I'll change this.
>
> > > + 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..0f0361d6cad6
> > > --- /dev/null
> > > +++ b/drivers/mfd/bd71837.c
> > > @@ -0,0 +1,221 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> >
> > There is a mismatch between what SPDX says and MODULE_LICENSE says.
> >
> > GPL-2.0 = GPL v2 only
> > MODULE_LICENSE(GPL) = GPL v2 or later.
> >
> > You'd like to change the SPDX identifier to GPL-2.0-or-later or set
> > module license to "GPL v2".
>
> Right. Thanks for pointing that out.
>
> >
> > > +// Copyright (C) 2018 ROHM Semiconductors
> > > +// bd71837.c -- ROHM BD71837MWV mfd driver
> > > +//
> > > +// Datasheet available from
> > > +// https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/moduleparam.h>
> > This include is not required.
> >
> > > +#include <linux/init.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/irq.h>
> > ditto
> >
> > > +#include <linux/irqdomain.h>
> > ditto
> >
> > > +#include <linux/gpio.h>
> > ditto
> >
> > > +#include <linux/regmap.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/of_gpio.h>
> > ditto
> > > +#include <linux/mfd/core.h>
> > > +#include <linux/mfd/bd71837.h>
> > > +
> >
> > Please review the needed includes.
>
> I'll do that, thanks.
>
> > > +static const struct resource irqs[] = {
> > > + {
> > > + .start = BD71837_INT_PWRBTN,
> > > + .end = BD71837_INT_PWRBTN,
> > > + .flags = IORESOURCE_IRQ,
> > > + .name = "pwr-btn",
> > > + }, {
> > > + .start = BD71837_INT_PWRBTN_L,
> > > + .end = BD71837_INT_PWRBTN_L,
> > > + .flags = IORESOURCE_IRQ,
> > > + .name = "pwr-btn-l",
> > > + }, {
> > > + .start = BD71837_INT_PWRBTN_S,
> > > + .end = BD71837_INT_PWRBTN_S,
> > > + .flags = IORESOURCE_IRQ,
> > > + .name = "pwr-btn-s",
> > > + },
> >
> > nit: no comma at the end
>
> Whole struct will be removed. I will use gpio-keys and remove the
> bd718xx-pwrkey driver as suggested by Dmitry Torokhov.
>
> > > +};
> > > +
> > > +/* bd71837 multi function cells */
> > > +static struct mfd_cell bd71837_mfd_cells[] = {
> > > + {
> > > + .name = "bd71837-clk",
> > > + }, {
> > > + .name = "bd718xx-pwrkey",
> > > + .resources = &irqs[0],
> > > + .num_resources = ARRAY_SIZE(irqs),
> > > + }, {
> > > + .name = "bd71837-pmic",
> > > + },
> > nit: no comma at the end
>
> Ok.
>
> > > +};
> > > +
> > > +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 int bd71837_irq_exit(struct bd71837 *bd71837)
> > > +{
> > > + if (bd71837->chip_irq > 0)
> > > + regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);
> > > + return 0;
> > > +}
> > > +
> > > +static const struct regmap_range pmic_status_range = {
> > > + .range_min = BD71837_REG_IRQ,
> > > + .range_max = BD71837_REG_POW_STATE,
> > > +};
> > > +
> > > +static const struct regmap_access_table volatile_regs = {
> > > + .yes_ranges = &pmic_status_range,
> > > + .n_yes_ranges = 1,
> > > +};
> > > +
> > > +static const struct regmap_config bd71837_regmap_config = {
> > > + .reg_bits = 8,
> > > + .val_bits = 8,
> > > + .volatile_table = &volatile_regs,
> > > + .max_register = BD71837_MAX_REGISTER - 1,
> > > + .cache_type = REGCACHE_RBTREE,
> > > +};
> > > +
> > > +#ifdef CONFIG_OF
>
> > The driver is DT-only and depends on OF so you don't need to protect this.
> True. I'll remove this
>
> > > +static const struct of_device_id bd71837_of_match[] = {
> > > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > > + { },
> >
> > nit: { /* sentinel */ }
>
> I am sorry but I didn't quite get the point here. Could you please
> explain what did you expect to be added here?
>
It's a nit but It is a good practice to specify that the last entry is
a sentinel. Just this.
+static const struct of_device_id bd71837_of_match[] = {
+ { .compatible = "rohm,bd71837", .data = (void *)0},
+ { /* sentinel */ }
+};
And just noticed, is .data = (void *)0 really required?
> > > +};
> > > +MODULE_DEVICE_TABLE(of, bd71837_of_match);
> > > +#endif //CONFIG_OF
> > > +
> > > +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 = regmap_add_irq_chip(bd71837->regmap, bd71837->chip_irq,
> > > + IRQF_ONESHOT, 0,
> > > + &bd71837_irq_chip, &bd71837->irq_data);
> >
> > I think that you can use 'devm_regmap_add_irq_chip' here and remove
> > the code to delete the irq chip
>
> Right. Good point. Makes this lot leaner and cleaner.
>
> > > + if (ret < 0) {
> > > + dev_err(bd71837->dev, "Failed to add irq_chip %d\n", ret);
> > > + goto err_out;
> > > + }
> > > +
> > > + ret = mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
> > > + bd71837_mfd_cells, ARRAY_SIZE(bd71837_mfd_cells),
> > > + NULL, 0,
> > > + regmap_irq_get_domain(bd71837->irq_data));
> >
> > Same here, I think you can use the devm_mfd_add_devices.
>
> Right. Good point. Makes this lot leaner and cleaner.
>
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct i2c_device_id bd71837_i2c_id[] = {
> > > + { .name = "bd71837", },
> > > + { }
> >
> > nit: { /* sentinel */ }
>
> I am sorry but I didn't quite get the point here. Could you please
> explain what did you expect to be added here?
>
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id);
> > > +
> > > +static struct i2c_driver bd71837_i2c_driver = {
> > > + .driver = {
> > > + .name = "bd71837-mfd",
> > > + .owner = THIS_MODULE,
> >
> > Remove this, it is not needed, the core does it for you.
>
> True. Thanks.
>
> > > + .of_match_table = of_match_ptr(bd71837_of_match),
> >
> > The driver is DT-only, don't need to call of_match_ptr.
>
> Ok.
>
> > > + },
> > > + .probe = bd71837_i2c_probe,
> > > + .remove = bd71837_i2c_remove,
> > > + .id_table = bd71837_i2c_id,
> > > +};
> > > +
> > > +static int __init bd71837_i2c_init(void)
> > > +{
> > > + return i2c_add_driver(&bd71837_i2c_driver);
> > > +}
> > > +/* init early so consumer devices can complete system boot */
> > > +subsys_initcall(bd71837_i2c_init);
> > > +
> > > +static void __exit bd71837_i2c_exit(void)
> > > +{
> > > + i2c_del_driver(&bd71837_i2c_driver);
> > > +}
> > > +module_exit(bd71837_i2c_exit);
> > > +
> >
> > Can't you use module_i2c_driver here and get rid of init/exit calls?
>
> I did this because of subsys_initcall() and how other drivers had used
> that.
>
> > > +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> > > +MODULE_DESCRIPTION("BD71837 chip multi-function driver");
> > > +MODULE_LICENSE("GPL");
> >
> > License mismatch.
>
> Thanks again
>
> > > diff --git a/include/linux/mfd/bd71837.h b/include/linux/mfd/bd71837.h
> > > new file mode 100644
> > > index 000000000000..125c7478ec29
> > > --- /dev/null
> > > +++ b/include/linux/mfd/bd71837.h
> > > @@ -0,0 +1,367 @@
> > > +
> > > +/* register write induced reset settings */
> >
> > /* Register ...
>
> Ok.
>
> > > +
> > > +/* even though the bit zero is not SWRESET type we still want to write zero
> >
> > /*
> > * Even
>
> Ok.
>
> > > + * to it when changing type. Biz zero is 'SWRESET' trigger bit and if we
> >
> > s/Biz/Bit/
>
> Ok.
>
> > > +/*
> > > + * bd71837 sub-driver chip access routines
> > > + */
> > > +
> >
> > All these access routines only hide the regmap_* calls, so why not use
> > the regmap calls directly in the driver and remove all this?
>
> This was also discussed with Stephen during the review for clk patches:
> https://lore.kernel.org/lkml/152997029783.143105.16692843405899913246@swboyd.mtv.corp.google.com/
>
> I will later send a new patch set which only removes these wrappers from
> this MFD and also the submodules. I like to introduce that as a separate
> change set after the whole driver is applied as it impacts to some already
> applied parts. I don't want any mismatches which jump out as compile errors
> when MFD gets applied and config dependencies get solved.
>
> But yes, you are correct - the write/read to BD71837 is plain regmap access
> and same simple access seems to be working with the BD71847 when it comes.
> So these are not needed and will be removed.
>
> Br,
> Matti Vaittinen
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-06-26 11:40 ` Enric Balletbo Serra
@ 2018-06-26 12:03 ` Matti Vaittinen
-1 siblings, 0 replies; 45+ messages in thread
From: Matti Vaittinen @ 2018-06-26 12:03 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Michael Turquette, sboyd, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, mazziesaccount, Arnd Bergmann,
Dmitry Torokhov, Sebastian Reichel, chenjh, andrew.smirnov,
Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree, linux-kernel,
linux-input, mikko.mutanen, heikki.haikola
Hello Again Eric,
On Tue, Jun 26, 2018 at 01:40:40PM +0200, Enric Balletbo Serra wrote:
> Hi Matti,
> Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> dia dt., 26 de juny 2018 a les 13:25:
> > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > dia dt., 19 de juny 2018 a les 12:57:
> >
> > > > +static const struct of_device_id bd71837_of_match[] = {
> > > > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > > > + { },
> > >
> > > nit: { /* sentinel */ }
> >
> > I am sorry but I didn't quite get the point here. Could you please
> > explain what did you expect to be added here?
> >
>
> It's a nit but It is a good practice to specify that the last entry is
> a sentinel. Just this.
>
> +static const struct of_device_id bd71837_of_match[] = {
> + { .compatible = "rohm,bd71837", .data = (void *)0},
> + { /* sentinel */ }
> +};
Oh, I see. Finally something I can disagree =) I quickly opened few
random drivers which declare match table. None of them practiced this
good practice. So I guess it is not such a standard after all. And I
guess the meaning of last entry in match table should be quite obvious.
Adding the comment /* sentinel */ sounds like stating the obvious at
best - at worst it gets one just to wonder what the "sentinel" means =)
>
> And just noticed, is .data = (void *)0 really required?
As static structs should be initialized to zero I'd say it is not
required. Will remove this. Thanks for pointing this out.
Br,
Matti Vaittinen
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-06-26 12:03 ` Matti Vaittinen
0 siblings, 0 replies; 45+ messages in thread
From: Matti Vaittinen @ 2018-06-26 12:03 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Michael Turquette, sboyd, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, mazziesaccount, Arnd Bergmann,
Dmitry Torokhov, Sebastian Reichel, chenjh, andrew.smirnov,
Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree, linux-kernel,
linux-input
Hello Again Eric,
On Tue, Jun 26, 2018 at 01:40:40PM +0200, Enric Balletbo Serra wrote:
> Hi Matti,
> Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> dia dt., 26 de juny 2018 a les 13:25:
> > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > dia dt., 19 de juny 2018 a les 12:57:
> >
> > > > +static const struct of_device_id bd71837_of_match[] = {
> > > > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > > > + { },
> > >
> > > nit: { /* sentinel */ }
> >
> > I am sorry but I didn't quite get the point here. Could you please
> > explain what did you expect to be added here?
> >
>
> It's a nit but It is a good practice to specify that the last entry is
> a sentinel. Just this.
>
> +static const struct of_device_id bd71837_of_match[] = {
> + { .compatible = "rohm,bd71837", .data = (void *)0},
> + { /* sentinel */ }
> +};
Oh, I see. Finally something I can disagree =) I quickly opened few
random drivers which declare match table. None of them practiced this
good practice. So I guess it is not such a standard after all. And I
guess the meaning of last entry in match table should be quite obvious.
Adding the comment /* sentinel */ sounds like stating the obvious at
best - at worst it gets one just to wonder what the "sentinel" means =)
>
> And just noticed, is .data = (void *)0 really required?
As static structs should be initialized to zero I'd say it is not
required. Will remove this. Thanks for pointing this out.
Br,
Matti Vaittinen
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-06-26 12:03 ` Matti Vaittinen
@ 2018-06-26 14:24 ` Enric Balletbo Serra
-1 siblings, 0 replies; 45+ messages in thread
From: Enric Balletbo Serra @ 2018-06-26 14:24 UTC (permalink / raw)
To: matti.vaittinen
Cc: Michael Turquette, sboyd, 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, linux-clk, devicetree, linux-kernel,
linux-input, mikko.mutanen, heikki.haikola
Hi Matti,
Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
dia dt., 26 de juny 2018 a les 14:03:
>
> Hello Again Eric,
>
> On Tue, Jun 26, 2018 at 01:40:40PM +0200, Enric Balletbo Serra wrote:
> > Hi Matti,
> > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > dia dt., 26 de juny 2018 a les 13:25:
> > > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> > > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > > dia dt., 19 de juny 2018 a les 12:57:
> > >
> > > > > +static const struct of_device_id bd71837_of_match[] = {
> > > > > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > > > > + { },
> > > >
> > > > nit: { /* sentinel */ }
> > >
> > > I am sorry but I didn't quite get the point here. Could you please
> > > explain what did you expect to be added here?
> > >
> >
> > It's a nit but It is a good practice to specify that the last entry is
> > a sentinel. Just this.
> >
> > +static const struct of_device_id bd71837_of_match[] = {
> > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > + { /* sentinel */ }
> > +};
>
> Oh, I see. Finally something I can disagree =) I quickly opened few
> random drivers which declare match table. None of them practiced this
> good practice. So I guess it is not such a standard after all. And I
> guess the meaning of last entry in match table should be quite obvious.
> Adding the comment /* sentinel */ sounds like stating the obvious at
> best - at worst it gets one just to wonder what the "sentinel" means =)
>
git grep "/\* sentinel \*/"
Anyway, I marked this change as a nit, so you don't need to change. I
just commented because at some point I received a "complain" when I
didn't add it. But this is up to the maintainer and I am not sure if
the "complain" was received in this subsystem :)
Cheers,
Enric
> >
> > And just noticed, is .data = (void *)0 really required?
>
> As static structs should be initialized to zero I'd say it is not
> required. Will remove this. Thanks for pointing this out.
>
> Br,
> Matti Vaittinen
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-06-26 14:24 ` Enric Balletbo Serra
0 siblings, 0 replies; 45+ messages in thread
From: Enric Balletbo Serra @ 2018-06-26 14:24 UTC (permalink / raw)
To: matti.vaittinen
Cc: Michael Turquette, sboyd, 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, linux-clk, devicetree, linux-kernel
Hi Matti,
Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
dia dt., 26 de juny 2018 a les 14:03:
>
> Hello Again Eric,
>
> On Tue, Jun 26, 2018 at 01:40:40PM +0200, Enric Balletbo Serra wrote:
> > Hi Matti,
> > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > dia dt., 26 de juny 2018 a les 13:25:
> > > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> > > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > > dia dt., 19 de juny 2018 a les 12:57:
> > >
> > > > > +static const struct of_device_id bd71837_of_match[] = {
> > > > > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > > > > + { },
> > > >
> > > > nit: { /* sentinel */ }
> > >
> > > I am sorry but I didn't quite get the point here. Could you please
> > > explain what did you expect to be added here?
> > >
> >
> > It's a nit but It is a good practice to specify that the last entry is
> > a sentinel. Just this.
> >
> > +static const struct of_device_id bd71837_of_match[] = {
> > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > + { /* sentinel */ }
> > +};
>
> Oh, I see. Finally something I can disagree =) I quickly opened few
> random drivers which declare match table. None of them practiced this
> good practice. So I guess it is not such a standard after all. And I
> guess the meaning of last entry in match table should be quite obvious.
> Adding the comment /* sentinel */ sounds like stating the obvious at
> best - at worst it gets one just to wonder what the "sentinel" means =)
>
git grep "/\* sentinel \*/"
Anyway, I marked this change as a nit, so you don't need to change. I
just commented because at some point I received a "complain" when I
didn't add it. But this is up to the maintainer and I am not sure if
the "complain" was received in this subsystem :)
Cheers,
Enric
> >
> > And just noticed, is .data = (void *)0 really required?
>
> As static structs should be initialized to zero I'd say it is not
> required. Will remove this. Thanks for pointing this out.
>
> Br,
> Matti Vaittinen
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 4/4] input/power: Add driver for BD71837/BD71847 PMIC power button
2018-06-21 10:25 ` Matti Vaittinen
@ 2018-06-27 0:21 ` Dmitry Torokhov
0 siblings, 0 replies; 45+ messages in thread
From: Dmitry Torokhov @ 2018-06-27 0:21 UTC (permalink / raw)
To: Matti Vaittinen
Cc: mturquette, sboyd, robh+dt, mark.rutland, lee.jones, lgirdwood,
broonie, mazziesaccount, arnd, sre, chenjh, andrew.smirnov,
linus.walleij, kstewart, heiko, gregkh, linux-clk, devicetree,
linux-kernel, linux-input, mikko.mutanen, heikki.haikola
Hi Matti,
On Thu, Jun 21, 2018 at 01:25:47PM +0300, Matti Vaittinen wrote:
> On Wed, Jun 20, 2018 at 09:43:16AM +0300, Matti Vaittinen wrote:
> > On Tue, Jun 19, 2018 at 10:50:28AM -0700, Dmitry Torokhov wrote:
> > > Hi Matti,
> > >
> > > On Tue, Jun 19, 2018 at 01:57:09PM +0300, Matti Vaittinen wrote:
> > > > ROHM BD71837 PMIC power button driver providing power-key press
> > > > information to user-space.
> > > >
> > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > > > ---
> > > > drivers/input/misc/Kconfig | 10 +++++
> > > > drivers/input/misc/Makefile | 1 +
> > > > drivers/input/misc/bd718xx-pwrkey.c | 90 +++++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 101 insertions(+)
> > > > create mode 100644 drivers/input/misc/bd718xx-pwrkey.c
> > > >
> > > > + platform_set_drvdata(pdev, pk);
> > > > + err = regmap_update_bits(pk->mfd->regmap,
> > > > + BD71837_REG_PWRONCONFIG0,
> > > > + BD718XX_PWRBTN_SHORT_PRESS_MASK,
> > > > + BD718XX_PWRBTN_SHORT_PRESS_10MS);
> > >
> > > This seems to be the only custom bit of set up in the driver, the rest I
> > > think can easily be handled by gpio-keys.c in interrupt-only mode. Maybe
> > > we could move this into MFD piece and drop this driver?
>
> I did following in MFD driver - is this what you suggested:
> +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),
> + }, {
Yes, something like this should work. It will not expose the internal
kernel structure (the fact that we are using gpio-keys dirver) to handle
power button of this PMIC, which Rob would appreciate. And we can change
it later if you want to create a fancier driver.
>
> //snip
>
> +static int bd71837_i2c_probe(struct i2c_client *i2c,
> + const struct i2c_device_id *id)
> +{
>
> // snip
>
> + ret = regmap_add_irq_chip(bd71837->regmap, bd71837->chip_irq,
> + IRQF_ONESHOT, 0,
> + &bd71837_irq_chip, &bd71837->irq_data);
> + if (ret < 0) {
> + dev_err(bd71837->dev, "Failed to add irq_chip %d\n", ret);
> + goto err_out;
> + }
> + /* I think this should be done conditionally and only when pwrkey is used
> + * What would be the correct way to decide if we want to touch rhw button
> + * press detection times?
> + */
> + 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 letch 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 = 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 looks is Ok I will send new patch with this approach at next week -
> unless I get lost during the traditional midsummer festival here in
> Finland.
>
> > Finally, there may be cases when power button is not attached to PMIC
> > or is needing different configuration for 'short push'. This is why I
> > would prefer having own Kconfig option for this power-key driver. I am
> > not sure if it is easily doable if we use gpio_keys?
>
> What would be the preferred mechanism for skipping the button push duration
> configurations (time it takes for PMIC to detect short or long push)? Or
> setting the durations to values user(s) would prefer? To me this sounds again
> something we could configure from DT. Would adding propereties
> rohm,short-press-ms and rohm,long-press-ms sound reasonable? I will send
> the first version with no option to skip/specify the configuration
> (fixed 10ms for short press, 10 sec for long press) but I would like to add
> support for specifying the duration as next step.
This sounds OK to be but you'd need to get Rob's buy in here. The
properties should probably be in a sub-node of the PMIC device node.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-06-26 11:40 ` Enric Balletbo Serra
@ 2018-07-03 6:53 ` Lee Jones
-1 siblings, 0 replies; 45+ messages in thread
From: Lee Jones @ 2018-07-03 6:53 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: matti.vaittinen, Michael Turquette, sboyd, Rob Herring,
Mark Rutland, Liam Girdwood, Mark Brown, mazziesaccount,
Arnd Bergmann, Dmitry Torokhov, Sebastian Reichel, chenjh,
andrew.smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree, linux-kernel,
linux-input, mikko.mutanen, heikki.haikola
On Tue, 26 Jun 2018, Enric Balletbo Serra wrote:
> Hi Matti,
> Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> dia dt., 26 de juny 2018 a les 13:25:
> >
> > Hello Eric,
> >
> > Thanks for the comments! I'll be addressing these in patch series v8
> > - except the regmap wrapper one which will be taken care of by another
> > patch set.
> >
> > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> > > Hi Matti,
> > >
> > > Thanks for the patch, a few comments below, some are feedback I
> > > received when I sent some patches to this subsystem.
> > >
> > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > dia dt., 19 de juny 2018 a les 12:57:
> > > >
> > > > ROHM BD71837 PMIC MFD driver providing interrupts and support
> > > > for two subsystems:
> > > > - clk
> > > > - Regulators
> > > >
> > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > > > ---
> > > > drivers/mfd/Kconfig | 13 ++
> > > > drivers/mfd/Makefile | 1 +
> > > > drivers/mfd/bd71837.c | 221 ++++++++++++++++++++++++++
> > > > include/linux/mfd/bd71837.h | 367 ++++++++++++++++++++++++++++++++++++++++++++
> > > > 4 files changed, 602 insertions(+)
> > > > create mode 100644 drivers/mfd/bd71837.c
> > > > create mode 100644 include/linux/mfd/bd71837.h
[...]
> > > > +static const struct of_device_id bd71837_of_match[] = {
> > > > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > > > + { },
> > >
> > > nit: { /* sentinel */ }
> >
> > I am sorry but I didn't quite get the point here. Could you please
> > explain what did you expect to be added here?
> >
>
> It's a nit but It is a good practice to specify that the last entry is
> a sentinel. Just this.
>
> +static const struct of_device_id bd71837_of_match[] = {
> + { .compatible = "rohm,bd71837", .data = (void *)0},
> + { /* sentinel */ }
> +};
I would not say this is "good practice" at all.
We all know what empty brackets mean. No need for obvious comments.
When authors put this, I let it go because "sentinel" is a cool word -
and nothing more! ;)
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-07-03 6:53 ` Lee Jones
0 siblings, 0 replies; 45+ messages in thread
From: Lee Jones @ 2018-07-03 6:53 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: matti.vaittinen, Michael Turquette, sboyd, Rob Herring,
Mark Rutland, Liam Girdwood, Mark Brown, mazziesaccount,
Arnd Bergmann, Dmitry Torokhov, Sebastian Reichel, chenjh,
andrew.smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree, linux-kernel,
linux-input
On Tue, 26 Jun 2018, Enric Balletbo Serra wrote:
> Hi Matti,
> Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> dia dt., 26 de juny 2018 a les 13:25:
> >
> > Hello Eric,
> >
> > Thanks for the comments! I'll be addressing these in patch series v8
> > - except the regmap wrapper one which will be taken care of by another
> > patch set.
> >
> > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> > > Hi Matti,
> > >
> > > Thanks for the patch, a few comments below, some are feedback I
> > > received when I sent some patches to this subsystem.
> > >
> > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > dia dt., 19 de juny 2018 a les 12:57:
> > > >
> > > > ROHM BD71837 PMIC MFD driver providing interrupts and support
> > > > for two subsystems:
> > > > - clk
> > > > - Regulators
> > > >
> > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > > > ---
> > > > drivers/mfd/Kconfig | 13 ++
> > > > drivers/mfd/Makefile | 1 +
> > > > drivers/mfd/bd71837.c | 221 ++++++++++++++++++++++++++
> > > > include/linux/mfd/bd71837.h | 367 ++++++++++++++++++++++++++++++++++++++++++++
> > > > 4 files changed, 602 insertions(+)
> > > > create mode 100644 drivers/mfd/bd71837.c
> > > > create mode 100644 include/linux/mfd/bd71837.h
[...]
> > > > +static const struct of_device_id bd71837_of_match[] = {
> > > > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > > > + { },
> > >
> > > nit: { /* sentinel */ }
> >
> > I am sorry but I didn't quite get the point here. Could you please
> > explain what did you expect to be added here?
> >
>
> It's a nit but It is a good practice to specify that the last entry is
> a sentinel. Just this.
>
> +static const struct of_device_id bd71837_of_match[] = {
> + { .compatible = "rohm,bd71837", .data = (void *)0},
> + { /* sentinel */ }
> +};
I would not say this is "good practice" at all.
We all know what empty brackets mean. No need for obvious comments.
When authors put this, I let it go because "sentinel" is a cool word -
and nothing more! ;)
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-06-26 14:24 ` Enric Balletbo Serra
@ 2018-07-03 6:56 ` Lee Jones
-1 siblings, 0 replies; 45+ messages in thread
From: Lee Jones @ 2018-07-03 6:56 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: matti.vaittinen, Michael Turquette, sboyd, 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, linux-clk, devicetree, linux-kernel,
linux-input, mikko.mutanen, heikki.haikola
On Tue, 26 Jun 2018, Enric Balletbo Serra wrote:
> Hi Matti,
>
> Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> dia dt., 26 de juny 2018 a les 14:03:
> >
> > Hello Again Eric,
> >
> > On Tue, Jun 26, 2018 at 01:40:40PM +0200, Enric Balletbo Serra wrote:
> > > Hi Matti,
> > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > dia dt., 26 de juny 2018 a les 13:25:
> > > > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> > > > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > > > dia dt., 19 de juny 2018 a les 12:57:
> > > >
> > > > > > +static const struct of_device_id bd71837_of_match[] = {
> > > > > > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > > > > > + { },
> > > > >
> > > > > nit: { /* sentinel */ }
> > > >
> > > > I am sorry but I didn't quite get the point here. Could you please
> > > > explain what did you expect to be added here?
> > > >
> > >
> > > It's a nit but It is a good practice to specify that the last entry is
> > > a sentinel. Just this.
> > >
> > > +static const struct of_device_id bd71837_of_match[] = {
> > > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > > + { /* sentinel */ }
> > > +};
> >
> > Oh, I see. Finally something I can disagree =) I quickly opened few
> > random drivers which declare match table. None of them practiced this
> > good practice. So I guess it is not such a standard after all. And I
> > guess the meaning of last entry in match table should be quite obvious.
> > Adding the comment /* sentinel */ sounds like stating the obvious at
> > best - at worst it gets one just to wonder what the "sentinel" means =)
> >
>
> git grep "/\* sentinel \*/"
>
> Anyway, I marked this change as a nit, so you don't need to change. I
> just commented because at some point I received a "complain" when I
> didn't add it. But this is up to the maintainer and I am not sure if
> the "complain" was received in this subsystem :)
Certainly not from me. I don't agree with the practice.
I'd like to know who is enforcing this!
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-07-03 6:56 ` Lee Jones
0 siblings, 0 replies; 45+ messages in thread
From: Lee Jones @ 2018-07-03 6:56 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: matti.vaittinen, Michael Turquette, sboyd, 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, linux-clk, devicetree
On Tue, 26 Jun 2018, Enric Balletbo Serra wrote:
> Hi Matti,
>
> Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> dia dt., 26 de juny 2018 a les 14:03:
> >
> > Hello Again Eric,
> >
> > On Tue, Jun 26, 2018 at 01:40:40PM +0200, Enric Balletbo Serra wrote:
> > > Hi Matti,
> > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > dia dt., 26 de juny 2018 a les 13:25:
> > > > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> > > > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > > > dia dt., 19 de juny 2018 a les 12:57:
> > > >
> > > > > > +static const struct of_device_id bd71837_of_match[] = {
> > > > > > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > > > > > + { },
> > > > >
> > > > > nit: { /* sentinel */ }
> > > >
> > > > I am sorry but I didn't quite get the point here. Could you please
> > > > explain what did you expect to be added here?
> > > >
> > >
> > > It's a nit but It is a good practice to specify that the last entry is
> > > a sentinel. Just this.
> > >
> > > +static const struct of_device_id bd71837_of_match[] = {
> > > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > > + { /* sentinel */ }
> > > +};
> >
> > Oh, I see. Finally something I can disagree =) I quickly opened few
> > random drivers which declare match table. None of them practiced this
> > good practice. So I guess it is not such a standard after all. And I
> > guess the meaning of last entry in match table should be quite obvious.
> > Adding the comment /* sentinel */ sounds like stating the obvious at
> > best - at worst it gets one just to wonder what the "sentinel" means =)
> >
>
> git grep "/\* sentinel \*/"
>
> Anyway, I marked this change as a nit, so you don't need to change. I
> just commented because at some point I received a "complain" when I
> didn't add it. But this is up to the maintainer and I am not sure if
> the "complain" was received in this subsystem :)
Certainly not from me. I don't agree with the practice.
I'd like to know who is enforcing this!
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 0/4] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver
2018-06-21 10:34 ` [PATCH v7 0/4] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver Matti Vaittinen
@ 2018-07-03 7:02 ` Lee Jones
2018-07-04 8:47 ` Matti Vaittinen
0 siblings, 1 reply; 45+ messages in thread
From: Lee Jones @ 2018-07-03 7:02 UTC (permalink / raw)
To: Matti Vaittinen
Cc: mturquette, sboyd, robh+dt, mark.rutland, lgirdwood, broonie,
mazziesaccount, arnd, dmitry.torokhov, sre, chenjh,
andrew.smirnov, linus.walleij, kstewart, heiko, gregkh,
linux-clk, devicetree, linux-kernel, linux-input, mikko.mutanen,
heikki.haikola
On Thu, 21 Jun 2018, Matti Vaittinen wrote:
> On Tue, Jun 19, 2018 at 01:55:31PM +0300, Matti Vaittinen wrote:
> > Patch series adding support for ROHM BD71837 PMIC.
> >
> What is the preferred way when I send updated patches:
>
> 1. always resend _all_ unapplied patches even if there is no changes to
> some of them. (patch-vN mail thread contains _all_ unapplied patches)
> 2. only resend changed patches (patch-vN mail thread contains only
> patches that were changed from patch-vN-1)
>
> I have currently used approach 1 - so that no patches would be
> accidentally forgotten - but downside is that people need to check if
> they have already reviewed some of the patches. I'd rather not caused
> any extra work. What is the most convenient way for you guys?
Option 1 is preferred.
Just ensure you apply any tags you have collected so reviewers can see
which patches are pending a review. It's also a good idea to keep a
succinct change log between the "--" marker and the diff stat where
you can state "v4: No change" or the like.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-07-03 6:56 ` Lee Jones
(?)
@ 2018-07-03 8:09 ` Enric Balletbo Serra
-1 siblings, 0 replies; 45+ messages in thread
From: Enric Balletbo Serra @ 2018-07-03 8:09 UTC (permalink / raw)
To: Lee Jones
Cc: matti.vaittinen, Michael Turquette, sboyd, 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, linux-clk, devicetree, linux-kernel,
linux-input, mikko.mutanen, heikki.haikola
Hi,
Missatge de Lee Jones <lee.jones@linaro.org> del dia dt., 3 de jul.
2018 a les 8:56:
>
> On Tue, 26 Jun 2018, Enric Balletbo Serra wrote:
>
> > Hi Matti,
> >
> > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > dia dt., 26 de juny 2018 a les 14:03:
> > >
> > > Hello Again Eric,
> > >
> > > On Tue, Jun 26, 2018 at 01:40:40PM +0200, Enric Balletbo Serra wrote:
> > > > Hi Matti,
> > > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > > dia dt., 26 de juny 2018 a les 13:25:
> > > > > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> > > > > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > > > > dia dt., 19 de juny 2018 a les 12:57:
> > > > >
> > > > > > > +static const struct of_device_id bd71837_of_match[] = {
> > > > > > > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > > > > > > + { },
> > > > > >
> > > > > > nit: { /* sentinel */ }
> > > > >
> > > > > I am sorry but I didn't quite get the point here. Could you please
> > > > > explain what did you expect to be added here?
> > > > >
> > > >
> > > > It's a nit but It is a good practice to specify that the last entry is
> > > > a sentinel. Just this.
> > > >
> > > > +static const struct of_device_id bd71837_of_match[] = {
> > > > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > > > + { /* sentinel */ }
> > > > +};
> > >
> > > Oh, I see. Finally something I can disagree =) I quickly opened few
> > > random drivers which declare match table. None of them practiced this
> > > good practice. So I guess it is not such a standard after all. And I
> > > guess the meaning of last entry in match table should be quite obvious.
> > > Adding the comment /* sentinel */ sounds like stating the obvious at
> > > best - at worst it gets one just to wonder what the "sentinel" means =)
> > >
> >
> > git grep "/\* sentinel \*/"
> >
> > Anyway, I marked this change as a nit, so you don't need to change. I
> > just commented because at some point I received a "complain" when I
> > didn't add it. But this is up to the maintainer and I am not sure if
> > the "complain" was received in this subsystem :)
>
> Certainly not from me. I don't agree with the practice.
>
Ok, Removed from my notes.
> I'd like to know who is enforcing this!
I tried to find from where my note comes from but I did not find it.
Anyway, it's clear now that you don't like it, so I'll remove from the
pending patches I have and see if someone complains about it again.
Thanks,
Enric
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-07-03 8:09 ` Enric Balletbo Serra
0 siblings, 0 replies; 45+ messages in thread
From: Enric Balletbo Serra @ 2018-07-03 8:09 UTC (permalink / raw)
To: Lee Jones
Cc: matti.vaittinen, Michael Turquette, sboyd, 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, linux-clk, devicetree
Hi,
Missatge de Lee Jones <lee.jones@linaro.org> del dia dt., 3 de jul.
2018 a les 8:56:
>
> On Tue, 26 Jun 2018, Enric Balletbo Serra wrote:
>
> > Hi Matti,
> >
> > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > dia dt., 26 de juny 2018 a les 14:03:
> > >
> > > Hello Again Eric,
> > >
> > > On Tue, Jun 26, 2018 at 01:40:40PM +0200, Enric Balletbo Serra wrote:
> > > > Hi Matti,
> > > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > > dia dt., 26 de juny 2018 a les 13:25:
> > > > > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> > > > > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > > > > dia dt., 19 de juny 2018 a les 12:57:
> > > > >
> > > > > > > +static const struct of_device_id bd71837_of_match[] = {
> > > > > > > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > > > > > > + { },
> > > > > >
> > > > > > nit: { /* sentinel */ }
> > > > >
> > > > > I am sorry but I didn't quite get the point here. Could you please
> > > > > explain what did you expect to be added here?
> > > > >
> > > >
> > > > It's a nit but It is a good practice to specify that the last entry is
> > > > a sentinel. Just this.
> > > >
> > > > +static const struct of_device_id bd71837_of_match[] = {
> > > > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > > > + { /* sentinel */ }
> > > > +};
> > >
> > > Oh, I see. Finally something I can disagree =) I quickly opened few
> > > random drivers which declare match table. None of them practiced this
> > > good practice. So I guess it is not such a standard after all. And I
> > > guess the meaning of last entry in match table should be quite obvious.
> > > Adding the comment /* sentinel */ sounds like stating the obvious at
> > > best - at worst it gets one just to wonder what the "sentinel" means =)
> > >
> >
> > git grep "/\* sentinel \*/"
> >
> > Anyway, I marked this change as a nit, so you don't need to change. I
> > just commented because at some point I received a "complain" when I
> > didn't add it. But this is up to the maintainer and I am not sure if
> > the "complain" was received in this subsystem :)
>
> Certainly not from me. I don't agree with the practice.
>
Ok, Removed from my notes.
> I'd like to know who is enforcing this!
I tried to find from where my note comes from but I did not find it.
Anyway, it's clear now that you don't like it, so I'll remove from the
pending patches I have and see if someone complains about it again.
Thanks,
Enric
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-07-03 8:09 ` Enric Balletbo Serra
0 siblings, 0 replies; 45+ messages in thread
From: Enric Balletbo Serra @ 2018-07-03 8:09 UTC (permalink / raw)
To: Lee Jones
Cc: matti.vaittinen, Michael Turquette, sboyd, 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, linux-clk, devicetree, linux-kernel,
linux-input, mikko.mutanen, heikki.haikola
Hi,
Missatge de Lee Jones <lee.jones@linaro.org> del dia dt., 3 de jul.
2018 a les 8:56:
>
> On Tue, 26 Jun 2018, Enric Balletbo Serra wrote:
>
> > Hi Matti,
> >
> > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > dia dt., 26 de juny 2018 a les 14:03:
> > >
> > > Hello Again Eric,
> > >
> > > On Tue, Jun 26, 2018 at 01:40:40PM +0200, Enric Balletbo Serra wrote:
> > > > Hi Matti,
> > > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > > dia dt., 26 de juny 2018 a les 13:25:
> > > > > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wr=
ote:
> > > > > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>=
del
> > > > > > dia dt., 19 de juny 2018 a les 12:57:
> > > > >
> > > > > > > +static const struct of_device_id bd71837_of_match[] =3D {
> > > > > > > + { .compatible =3D "rohm,bd71837", .data =3D (void *)0=
},
> > > > > > > + { },
> > > > > >
> > > > > > nit: { /* sentinel */ }
> > > > >
> > > > > I am sorry but I didn't quite get the point here. Could you pleas=
e
> > > > > explain what did you expect to be added here?
> > > > >
> > > >
> > > > It's a nit but It is a good practice to specify that the last entry=
is
> > > > a sentinel. Just this.
> > > >
> > > > +static const struct of_device_id bd71837_of_match[] =3D {
> > > > + { .compatible =3D "rohm,bd71837", .data =3D (void *)0},
> > > > + { /* sentinel */ }
> > > > +};
> > >
> > > Oh, I see. Finally something I can disagree =3D) I quickly opened few
> > > random drivers which declare match table. None of them practiced this
> > > good practice. So I guess it is not such a standard after all. And I
> > > guess the meaning of last entry in match table should be quite obviou=
s.
> > > Adding the comment /* sentinel */ sounds like stating the obvious at
> > > best - at worst it gets one just to wonder what the "sentinel" means =
=3D)
> > >
> >
> > git grep "/\* sentinel \*/"
> >
> > Anyway, I marked this change as a nit, so you don't need to change. I
> > just commented because at some point I received a "complain" when I
> > didn't add it. But this is up to the maintainer and I am not sure if
> > the "complain" was received in this subsystem :)
>
> Certainly not from me. I don't agree with the practice.
>
Ok, Removed from my notes.
> I'd like to know who is enforcing this!
I tried to find from where my note comes from but I did not find it.
Anyway, it's clear now that you don't like it, so I'll remove from the
pending patches I have and see if someone complains about it again.
Thanks,
Enric
>
> --
> Lee Jones [=E6=9D=8E=E7=90=BC=E6=96=AF]
> Linaro Services Technical Lead
> Linaro.org =E2=94=82 Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 0/4] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver
2018-07-03 7:02 ` Lee Jones
@ 2018-07-04 8:47 ` Matti Vaittinen
2018-07-04 9:21 ` Lee Jones
0 siblings, 1 reply; 45+ messages in thread
From: Matti Vaittinen @ 2018-07-04 8:47 UTC (permalink / raw)
To: Lee Jones
Cc: mturquette, sboyd, robh+dt, mark.rutland, lgirdwood, broonie,
mazziesaccount, arnd, dmitry.torokhov, sre, chenjh,
andrew.smirnov, linus.walleij, kstewart, heiko, gregkh,
linux-clk, devicetree, linux-kernel, linux-input, mikko.mutanen,
heikki.haikola
On Tue, Jul 03, 2018 at 08:02:00AM +0100, Lee Jones wrote:
> On Thu, 21 Jun 2018, Matti Vaittinen wrote:
>
> > On Tue, Jun 19, 2018 at 01:55:31PM +0300, Matti Vaittinen wrote:
> > > Patch series adding support for ROHM BD71837 PMIC.
> > >
> > What is the preferred way when I send updated patches:
> >
> > 1. always resend _all_ unapplied patches even if there is no changes to
> > some of them. (patch-vN mail thread contains _all_ unapplied patches)
> > 2. only resend changed patches (patch-vN mail thread contains only
> > patches that were changed from patch-vN-1)
> >
> > I have currently used approach 1 - so that no patches would be
> > accidentally forgotten - but downside is that people need to check if
> > they have already reviewed some of the patches. I'd rather not caused
> > any extra work. What is the most convenient way for you guys?
>
> Option 1 is preferred.
>
> Just ensure you apply any tags you have collected so reviewers can see
> which patches are pending a review. It's also a good idea to keep a
> succinct change log between the "--" marker and the diff stat where
> you can state "v4: No change" or the like.
Right. Thanks. Just one question - what if I get reviewed-by for a
patch which I later rework? Like this MFD patch where I got reviewed-by
from Linus Walleij for v6 - but which I reworked due to comments from
Enric and Dmitry. I have not kept the reviewed-by as the patch is not
exactly the same Linus was originally reviewing. I guess the tags should
be only kept for patches which are unchanged, right?
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 0/4] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver
2018-07-04 8:47 ` Matti Vaittinen
@ 2018-07-04 9:21 ` Lee Jones
0 siblings, 0 replies; 45+ messages in thread
From: Lee Jones @ 2018-07-04 9:21 UTC (permalink / raw)
To: Matti Vaittinen
Cc: mturquette, sboyd, robh+dt, mark.rutland, lgirdwood, broonie,
mazziesaccount, arnd, dmitry.torokhov, sre, chenjh,
andrew.smirnov, linus.walleij, kstewart, heiko, gregkh,
linux-clk, devicetree, linux-kernel, linux-input, mikko.mutanen,
heikki.haikola
On Wed, 04 Jul 2018, Matti Vaittinen wrote:
> On Tue, Jul 03, 2018 at 08:02:00AM +0100, Lee Jones wrote:
> > On Thu, 21 Jun 2018, Matti Vaittinen wrote:
> >
> > > On Tue, Jun 19, 2018 at 01:55:31PM +0300, Matti Vaittinen wrote:
> > > > Patch series adding support for ROHM BD71837 PMIC.
> > > >
> > > What is the preferred way when I send updated patches:
> > >
> > > 1. always resend _all_ unapplied patches even if there is no changes to
> > > some of them. (patch-vN mail thread contains _all_ unapplied patches)
> > > 2. only resend changed patches (patch-vN mail thread contains only
> > > patches that were changed from patch-vN-1)
> > >
> > > I have currently used approach 1 - so that no patches would be
> > > accidentally forgotten - but downside is that people need to check if
> > > they have already reviewed some of the patches. I'd rather not caused
> > > any extra work. What is the most convenient way for you guys?
> >
> > Option 1 is preferred.
> >
> > Just ensure you apply any tags you have collected so reviewers can see
> > which patches are pending a review. It's also a good idea to keep a
> > succinct change log between the "--" marker and the diff stat where
> > you can state "v4: No change" or the like.
>
> Right. Thanks. Just one question - what if I get reviewed-by for a
> patch which I later rework? Like this MFD patch where I got reviewed-by
> from Linus Walleij for v6 - but which I reworked due to comments from
> Enric and Dmitry. I have not kept the reviewed-by as the patch is not
> exactly the same Linus was originally reviewing. I guess the tags should
> be only kept for patches which are unchanged, right?
That is the $64,000 question. The answer is "it depends". You should
use your common sense. Did your re-work taint the code that your
reviewer provided his tag for? If so, drop it. If not, keep it.
There are no hard and fast rules about these kinds of things.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-06-26 9:06 ` Enric Balletbo Serra
@ 2018-07-04 14:56 ` Dmitry Torokhov
-1 siblings, 0 replies; 45+ messages in thread
From: Dmitry Torokhov @ 2018-07-04 14:56 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: matti.vaittinen, Michael Turquette, sboyd, Rob Herring,
Mark Rutland, Lee Jones, Liam Girdwood, Mark Brown,
mazziesaccount, Arnd Bergmann, Sebastian Reichel, chenjh,
andrew.smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree, linux-kernel,
linux-input, mikko.mutanen, heikki.haikola
Hi Enric,
On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> Hi Matti,
>
> Thanks for the patch, a few comments below, some are feedback I
> received when I sent some patches to this subsystem.
>
> Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> dia dt., 19 de juny 2018 a les 12:57:
> > +};
> > +
> > +/* bd71837 multi function cells */
> > +static struct mfd_cell bd71837_mfd_cells[] = {
> > + {
> > + .name = "bd71837-clk",
> > + }, {
> > + .name = "bd718xx-pwrkey",
> > + .resources = &irqs[0],
> > + .num_resources = ARRAY_SIZE(irqs),
> > + }, {
> > + .name = "bd71837-pmic",
> > + },
> nit: no comma at the end
Actually, trailing comma is preferred on structures/arrays without
sentinels, because if one needs to add a new entry/new member, then in
the diff there will have only one new line added, instead of one line
being changed (adding now necessary comma) and one added.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-07-04 14:56 ` Dmitry Torokhov
0 siblings, 0 replies; 45+ messages in thread
From: Dmitry Torokhov @ 2018-07-04 14:56 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: matti.vaittinen, Michael Turquette, sboyd, Rob Herring,
Mark Rutland, Lee Jones, Liam Girdwood, Mark Brown,
mazziesaccount, Arnd Bergmann, Sebastian Reichel, chenjh,
andrew.smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree, linux-kernel,
linux-input
Hi Enric,
On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> Hi Matti,
>
> Thanks for the patch, a few comments below, some are feedback I
> received when I sent some patches to this subsystem.
>
> Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> dia dt., 19 de juny 2018 a les 12:57:
> > +};
> > +
> > +/* bd71837 multi function cells */
> > +static struct mfd_cell bd71837_mfd_cells[] = {
> > + {
> > + .name = "bd71837-clk",
> > + }, {
> > + .name = "bd718xx-pwrkey",
> > + .resources = &irqs[0],
> > + .num_resources = ARRAY_SIZE(irqs),
> > + }, {
> > + .name = "bd71837-pmic",
> > + },
> nit: no comma at the end
Actually, trailing comma is preferred on structures/arrays without
sentinels, because if one needs to add a new entry/new member, then in
the diff there will have only one new line added, instead of one line
being changed (adding now necessary comma) and one added.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-07-04 14:56 ` Dmitry Torokhov
@ 2018-07-04 16:57 ` Enric Balletbo Serra
-1 siblings, 0 replies; 45+ messages in thread
From: Enric Balletbo Serra @ 2018-07-04 16:57 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: matti.vaittinen, Michael Turquette, sboyd, Rob Herring,
Mark Rutland, Lee Jones, Liam Girdwood, Mark Brown,
Matti Vaittinen, Arnd Bergmann, Sebastian Reichel, chenjh,
Andrey Smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree, linux-kernel,
linux-input, mikko.mutanen, heikki.haikola
Missatge de Dmitry Torokhov <dmitry.torokhov@gmail.com> del dia dc., 4
de jul. 2018 a les 17:10:
>
> Hi Enric,
>
> On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> > Hi Matti,
> >
> > Thanks for the patch, a few comments below, some are feedback I
> > received when I sent some patches to this subsystem.
> >
> > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > dia dt., 19 de juny 2018 a les 12:57:
> > > +};
> > > +
> > > +/* bd71837 multi function cells */
> > > +static struct mfd_cell bd71837_mfd_cells[] = {
> > > + {
> > > + .name = "bd71837-clk",
> > > + }, {
> > > + .name = "bd718xx-pwrkey",
> > > + .resources = &irqs[0],
> > > + .num_resources = ARRAY_SIZE(irqs),
> > > + }, {
> > > + .name = "bd71837-pmic",
> > > + },
> > nit: no comma at the end
>
> Actually, trailing comma is preferred on structures/arrays without
> sentinels, because if one needs to add a new entry/new member, then in
> the diff there will have only one new line added, instead of one line
> being changed (adding now necessary comma) and one added.
>
Many thanks for sharing your knowledge! That looks to me a good
reason. I did this comment because at some point I received this
comment. I try to mark this kind of things as nitpicks because
sometimes there are differences between maintainers. E.g: some
maintainers prefer 'if (something == NULL)', others 'if (!something)';
others are fine using goto even there is nothing to unwind, other
prefer plain returns, etc.
Matti, I don't want to beat about the bush with these nitpicks. It is
not my intention. So I'd say, do what the maintainer wants :)
Dmitry, I really appreciate this kind of comments, especially when
there is a justification, so I can learn more and more to do better
patches.
Thanks,
Enric
> Thanks.
>
> --
> Dmitry
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-07-04 16:57 ` Enric Balletbo Serra
0 siblings, 0 replies; 45+ messages in thread
From: Enric Balletbo Serra @ 2018-07-04 16:57 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: matti.vaittinen, Michael Turquette, sboyd, Rob Herring,
Mark Rutland, Lee Jones, Liam Girdwood, Mark Brown,
Matti Vaittinen, Arnd Bergmann, Sebastian Reichel, chenjh,
Andrey Smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree, linux-kernel
Missatge de Dmitry Torokhov <dmitry.torokhov@gmail.com> del dia dc., 4
de jul. 2018 a les 17:10:
>
> Hi Enric,
>
> On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> > Hi Matti,
> >
> > Thanks for the patch, a few comments below, some are feedback I
> > received when I sent some patches to this subsystem.
> >
> > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > dia dt., 19 de juny 2018 a les 12:57:
> > > +};
> > > +
> > > +/* bd71837 multi function cells */
> > > +static struct mfd_cell bd71837_mfd_cells[] = {
> > > + {
> > > + .name = "bd71837-clk",
> > > + }, {
> > > + .name = "bd718xx-pwrkey",
> > > + .resources = &irqs[0],
> > > + .num_resources = ARRAY_SIZE(irqs),
> > > + }, {
> > > + .name = "bd71837-pmic",
> > > + },
> > nit: no comma at the end
>
> Actually, trailing comma is preferred on structures/arrays without
> sentinels, because if one needs to add a new entry/new member, then in
> the diff there will have only one new line added, instead of one line
> being changed (adding now necessary comma) and one added.
>
Many thanks for sharing your knowledge! That looks to me a good
reason. I did this comment because at some point I received this
comment. I try to mark this kind of things as nitpicks because
sometimes there are differences between maintainers. E.g: some
maintainers prefer 'if (something == NULL)', others 'if (!something)';
others are fine using goto even there is nothing to unwind, other
prefer plain returns, etc.
Matti, I don't want to beat about the bush with these nitpicks. It is
not my intention. So I'd say, do what the maintainer wants :)
Dmitry, I really appreciate this kind of comments, especially when
there is a justification, so I can learn more and more to do better
patches.
Thanks,
Enric
> Thanks.
>
> --
> Dmitry
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-07-04 16:57 ` Enric Balletbo Serra
@ 2018-07-05 5:52 ` Lee Jones
-1 siblings, 0 replies; 45+ messages in thread
From: Lee Jones @ 2018-07-05 5:52 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Dmitry Torokhov, matti.vaittinen, Michael Turquette, sboyd,
Rob Herring, Mark Rutland, Liam Girdwood, Mark Brown,
Matti Vaittinen, Arnd Bergmann, Sebastian Reichel, chenjh,
Andrey Smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree, linux-kernel,
linux-input, mikko.mutanen, heikki.haikola
On Wed, 04 Jul 2018, Enric Balletbo Serra wrote:
> Missatge de Dmitry Torokhov <dmitry.torokhov@gmail.com> del dia dc., 4
> de jul. 2018 a les 17:10:
> >
> > Hi Enric,
> >
> > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> > > Hi Matti,
> > >
> > > Thanks for the patch, a few comments below, some are feedback I
> > > received when I sent some patches to this subsystem.
> > >
> > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > dia dt., 19 de juny 2018 a les 12:57:
> > > > +};
> > > > +
> > > > +/* bd71837 multi function cells */
> > > > +static struct mfd_cell bd71837_mfd_cells[] = {
> > > > + {
> > > > + .name = "bd71837-clk",
> > > > + }, {
> > > > + .name = "bd718xx-pwrkey",
> > > > + .resources = &irqs[0],
> > > > + .num_resources = ARRAY_SIZE(irqs),
> > > > + }, {
> > > > + .name = "bd71837-pmic",
> > > > + },
> > > nit: no comma at the end
> >
> > Actually, trailing comma is preferred on structures/arrays without
> > sentinels, because if one needs to add a new entry/new member, then in
> > the diff there will have only one new line added, instead of one line
> > being changed (adding now necessary comma) and one added.
[...]
> Dmitry, I really appreciate this kind of comments, especially when
> there is a justification, so I can learn more and more to do better
> patches.
+1
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-07-05 5:52 ` Lee Jones
0 siblings, 0 replies; 45+ messages in thread
From: Lee Jones @ 2018-07-05 5:52 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Dmitry Torokhov, matti.vaittinen, Michael Turquette, sboyd,
Rob Herring, Mark Rutland, Liam Girdwood, Mark Brown,
Matti Vaittinen, Arnd Bergmann, Sebastian Reichel, chenjh,
Andrey Smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree
On Wed, 04 Jul 2018, Enric Balletbo Serra wrote:
> Missatge de Dmitry Torokhov <dmitry.torokhov@gmail.com> del dia dc., 4
> de jul. 2018 a les 17:10:
> >
> > Hi Enric,
> >
> > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> > > Hi Matti,
> > >
> > > Thanks for the patch, a few comments below, some are feedback I
> > > received when I sent some patches to this subsystem.
> > >
> > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > dia dt., 19 de juny 2018 a les 12:57:
> > > > +};
> > > > +
> > > > +/* bd71837 multi function cells */
> > > > +static struct mfd_cell bd71837_mfd_cells[] = {
> > > > + {
> > > > + .name = "bd71837-clk",
> > > > + }, {
> > > > + .name = "bd718xx-pwrkey",
> > > > + .resources = &irqs[0],
> > > > + .num_resources = ARRAY_SIZE(irqs),
> > > > + }, {
> > > > + .name = "bd71837-pmic",
> > > > + },
> > > nit: no comma at the end
> >
> > Actually, trailing comma is preferred on structures/arrays without
> > sentinels, because if one needs to add a new entry/new member, then in
> > the diff there will have only one new line added, instead of one line
> > being changed (adding now necessary comma) and one added.
[...]
> Dmitry, I really appreciate this kind of comments, especially when
> there is a justification, so I can learn more and more to do better
> patches.
+1
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-07-04 16:57 ` Enric Balletbo Serra
@ 2018-07-05 7:56 ` Matti Vaittinen
-1 siblings, 0 replies; 45+ messages in thread
From: Matti Vaittinen @ 2018-07-05 7:56 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Dmitry Torokhov, Michael Turquette, sboyd, Rob Herring,
Mark Rutland, Lee Jones, Liam Girdwood, Mark Brown,
Matti Vaittinen, Arnd Bergmann, Sebastian Reichel, chenjh,
Andrey Smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree, linux-kernel,
linux-input, mikko.mutanen, heikki.haikola
On Wed, Jul 04, 2018 at 06:57:39PM +0200, Enric Balletbo Serra wrote:
> Missatge de Dmitry Torokhov <dmitry.torokhov@gmail.com> del dia dc., 4
> de jul. 2018 a les 17:10:
> >
> > Hi Enric,
> >
> > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> > > Hi Matti,
> > >
> > > Thanks for the patch, a few comments below, some are feedback I
> > > received when I sent some patches to this subsystem.
> > >
> > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > dia dt., 19 de juny 2018 a les 12:57:
> > > > +};
> > > > +
> > > > +/* bd71837 multi function cells */
> > > > +static struct mfd_cell bd71837_mfd_cells[] = {
> > > > + {
> > > > + .name = "bd71837-clk",
> > > > + }, {
> > > > + .name = "bd718xx-pwrkey",
> > > > + .resources = &irqs[0],
> > > > + .num_resources = ARRAY_SIZE(irqs),
> > > > + }, {
> > > > + .name = "bd71837-pmic",
> > > > + },
> > > nit: no comma at the end
> >
> > Actually, trailing comma is preferred on structures/arrays without
> > sentinels, because if one needs to add a new entry/new member, then in
> > the diff there will have only one new line added, instead of one line
> > being changed (adding now necessary comma) and one added.
> >
>
> Many thanks for sharing your knowledge! That looks to me a good
> reason.
So in this specific ecample leaving the comma does not help. The opening
brace for new array element would be added to same line where the comma
is, right? In any case - this was educating and makes perfect sense for
arrays with simple items. Thanks.
> Matti, I don't want to beat about the bush with these nitpicks. It is
> not my intention. So I'd say, do what the maintainer wants :)
>
No problem.
Br,
Matti Vaittinen
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-07-05 7:56 ` Matti Vaittinen
0 siblings, 0 replies; 45+ messages in thread
From: Matti Vaittinen @ 2018-07-05 7:56 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Dmitry Torokhov, Michael Turquette, sboyd, Rob Herring,
Mark Rutland, Lee Jones, Liam Girdwood, Mark Brown,
Matti Vaittinen, Arnd Bergmann, Sebastian Reichel, chenjh,
Andrey Smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree
On Wed, Jul 04, 2018 at 06:57:39PM +0200, Enric Balletbo Serra wrote:
> Missatge de Dmitry Torokhov <dmitry.torokhov@gmail.com> del dia dc., 4
> de jul. 2018 a les 17:10:
> >
> > Hi Enric,
> >
> > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> > > Hi Matti,
> > >
> > > Thanks for the patch, a few comments below, some are feedback I
> > > received when I sent some patches to this subsystem.
> > >
> > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > dia dt., 19 de juny 2018 a les 12:57:
> > > > +};
> > > > +
> > > > +/* bd71837 multi function cells */
> > > > +static struct mfd_cell bd71837_mfd_cells[] = {
> > > > + {
> > > > + .name = "bd71837-clk",
> > > > + }, {
> > > > + .name = "bd718xx-pwrkey",
> > > > + .resources = &irqs[0],
> > > > + .num_resources = ARRAY_SIZE(irqs),
> > > > + }, {
> > > > + .name = "bd71837-pmic",
> > > > + },
> > > nit: no comma at the end
> >
> > Actually, trailing comma is preferred on structures/arrays without
> > sentinels, because if one needs to add a new entry/new member, then in
> > the diff there will have only one new line added, instead of one line
> > being changed (adding now necessary comma) and one added.
> >
>
> Many thanks for sharing your knowledge! That looks to me a good
> reason.
So in this specific ecample leaving the comma does not help. The opening
brace for new array element would be added to same line where the comma
is, right? In any case - this was educating and makes perfect sense for
arrays with simple items. Thanks.
> Matti, I don't want to beat about the bush with these nitpicks. It is
> not my intention. So I'd say, do what the maintainer wants :)
>
No problem.
Br,
Matti Vaittinen
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-07-05 7:56 ` Matti Vaittinen
(?)
@ 2018-07-06 6:38 ` Dmitry Torokhov
-1 siblings, 0 replies; 45+ messages in thread
From: Dmitry Torokhov @ 2018-07-06 6:38 UTC (permalink / raw)
To: Matti Vaittinen, Enric Balletbo Serra
Cc: Michael Turquette, sboyd, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, Matti Vaittinen, Arnd Bergmann,
Sebastian Reichel, chenjh, Andrey Smirnov, Linus Walleij,
Kate Stewart, Heiko Stübner, Greg Kroah-Hartman, linux-clk,
devicetree, linux-kernel, linux-input, mikko.mutanen,
heikki.haikola
On July 5, 2018 12:56:50 AM PDT, Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> wrote:
>On Wed, Jul 04, 2018 at 06:57:39PM +0200, Enric Balletbo Serra wrote:
>> Missatge de Dmitry Torokhov <dmitry.torokhov@gmail.com> del dia dc.,
>4
>> de jul. 2018 a les 17:10:
>> >
>> > Hi Enric,
>> >
>> > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra
>wrote:
>> > > Hi Matti,
>> > >
>> > > Thanks for the patch, a few comments below, some are feedback I
>> > > received when I sent some patches to this subsystem.
>> > >
>> > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
>del
>> > > dia dt., 19 de juny 2018 a les 12:57:
>> > > > +};
>> > > > +
>> > > > +/* bd71837 multi function cells */
>> > > > +static struct mfd_cell bd71837_mfd_cells[] = {
>> > > > + {
>> > > > + .name = "bd71837-clk",
>> > > > + }, {
>> > > > + .name = "bd718xx-pwrkey",
>> > > > + .resources = &irqs[0],
>> > > > + .num_resources = ARRAY_SIZE(irqs),
>> > > > + }, {
>> > > > + .name = "bd71837-pmic",
>> > > > + },
>> > > nit: no comma at the end
>> >
>> > Actually, trailing comma is preferred on structures/arrays without
>> > sentinels, because if one needs to add a new entry/new member, then
>in
>> > the diff there will have only one new line added, instead of one
>line
>> > being changed (adding now necessary comma) and one added.
>> >
>>
>> Many thanks for sharing your knowledge! That looks to me a good
>> reason.
>
>So in this specific ecample leaving the comma does not help. The
>opening
>brace for new array element would be added to same line where the comma
>is, right?
Ah, yes, you are right. We usually have either:
{ /* element 1 */ },
{ / *element 2 */ },
...
or:
{
/* element 1 */
},
{
/* element 2 */
},
but I do not think that it is codified in the CodingStyle.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-07-06 6:38 ` Dmitry Torokhov
0 siblings, 0 replies; 45+ messages in thread
From: Dmitry Torokhov @ 2018-07-06 6:38 UTC (permalink / raw)
To: Matti Vaittinen, Enric Balletbo Serra
Cc: Michael Turquette, sboyd, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, Matti Vaittinen, Arnd Bergmann,
Sebastian Reichel, chenjh, Andrey Smirnov, Linus Walleij,
Kate Stewart, Heiko Stübner, Greg Kroah-Hartman, linux-clk,
devicetree, linux-kernel, linux-input
On July 5, 2018 12:56:50 AM PDT, Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> wrote:
>On Wed, Jul 04, 2018 at 06:57:39PM +0200, Enric Balletbo Serra wrote:
>> Missatge de Dmitry Torokhov <dmitry.torokhov@gmail.com> del dia dc.,
>4
>> de jul. 2018 a les 17:10:
>> >
>> > Hi Enric,
>> >
>> > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra
>wrote:
>> > > Hi Matti,
>> > >
>> > > Thanks for the patch, a few comments below, some are feedback I
>> > > received when I sent some patches to this subsystem.
>> > >
>> > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
>del
>> > > dia dt., 19 de juny 2018 a les 12:57:
>> > > > +};
>> > > > +
>> > > > +/* bd71837 multi function cells */
>> > > > +static struct mfd_cell bd71837_mfd_cells[] = {
>> > > > + {
>> > > > + .name = "bd71837-clk",
>> > > > + }, {
>> > > > + .name = "bd718xx-pwrkey",
>> > > > + .resources = &irqs[0],
>> > > > + .num_resources = ARRAY_SIZE(irqs),
>> > > > + }, {
>> > > > + .name = "bd71837-pmic",
>> > > > + },
>> > > nit: no comma at the end
>> >
>> > Actually, trailing comma is preferred on structures/arrays without
>> > sentinels, because if one needs to add a new entry/new member, then
>in
>> > the diff there will have only one new line added, instead of one
>line
>> > being changed (adding now necessary comma) and one added.
>> >
>>
>> Many thanks for sharing your knowledge! That looks to me a good
>> reason.
>
>So in this specific ecample leaving the comma does not help. The
>opening
>brace for new array element would be added to same line where the comma
>is, right?
Ah, yes, you are right. We usually have either:
{ /* element 1 */ },
{ / *element 2 */ },
...
or:
{
/* element 1 */
},
{
/* element 2 */
},
but I do not think that it is codified in the CodingStyle.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-07-06 6:38 ` Dmitry Torokhov
0 siblings, 0 replies; 45+ messages in thread
From: Dmitry Torokhov @ 2018-07-06 6:38 UTC (permalink / raw)
To: Matti Vaittinen, Enric Balletbo Serra
Cc: Michael Turquette, sboyd, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, Matti Vaittinen, Arnd Bergmann,
Sebastian Reichel, chenjh, Andrey Smirnov, Linus Walleij,
Kate Stewart, Heiko Stübner, Greg Kroah-Hartman, linux-clk,
devicetree, linux-kernel, linux-input, mikko.mutanen,
heikki.haikola
On July 5, 2018 12:56:50 AM PDT, Matti Vaittinen <matti=2Evaittinen@fi=2Ero=
hmeurope=2Ecom> wrote:
>On Wed, Jul 04, 2018 at 06:57:39PM +0200, Enric Balletbo Serra wrote:
>> Missatge de Dmitry Torokhov <dmitry=2Etorokhov@gmail=2Ecom> del dia dc=
=2E,
>4
>> de jul=2E 2018 a les 17:10:
>> >
>> > Hi Enric,
>> >
>> > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra
>wrote:
>> > > Hi Matti,
>> > >
>> > > Thanks for the patch, a few comments below, some are feedback I
>> > > received when I sent some patches to this subsystem=2E
>> > >
>> > > Missatge de Matti Vaittinen <matti=2Evaittinen@fi=2Erohmeurope=2Eco=
m>
>del
>> > > dia dt=2E, 19 de juny 2018 a les 12:57:
>> > > > +};
>> > > > +
>> > > > +/* bd71837 multi function cells */
>> > > > +static struct mfd_cell bd71837_mfd_cells[] =3D {
>> > > > + {
>> > > > + =2Ename =3D "bd71837-clk",
>> > > > + }, {
>> > > > + =2Ename =3D "bd718xx-pwrkey",
>> > > > + =2Eresources =3D &irqs[0],
>> > > > + =2Enum_resources =3D ARRAY_SIZE(irqs),
>> > > > + }, {
>> > > > + =2Ename =3D "bd71837-pmic",
>> > > > + },
>> > > nit: no comma at the end
>> >
>> > Actually, trailing comma is preferred on structures/arrays without
>> > sentinels, because if one needs to add a new entry/new member, then
>in
>> > the diff there will have only one new line added, instead of one
>line
>> > being changed (adding now necessary comma) and one added=2E
>> >
>>=20
>> Many thanks for sharing your knowledge! That looks to me a good
>> reason=2E
>
>So in this specific ecample leaving the comma does not help=2E The
>opening
>brace for new array element would be added to same line where the comma
>is, right?
Ah, yes, you are right=2E We usually have either:
{ /* element 1 */ },
{ / *element 2 */ },
=2E=2E=2E
or:
{
/* element 1 */
},
{
/* element 2 */
},
but I do not think that it is codified in the CodingStyle=2E
Thanks=2E
--=20
Dmitry
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-07-06 6:38 ` Dmitry Torokhov
@ 2018-07-06 7:05 ` Lee Jones
-1 siblings, 0 replies; 45+ messages in thread
From: Lee Jones @ 2018-07-06 7:05 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Matti Vaittinen, Enric Balletbo Serra, Michael Turquette, sboyd,
Rob Herring, Mark Rutland, Liam Girdwood, Mark Brown,
Matti Vaittinen, Arnd Bergmann, Sebastian Reichel, chenjh,
Andrey Smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree, linux-kernel,
linux-input, mikko.mutanen, heikki.haikola
On Thu, 05 Jul 2018, Dmitry Torokhov wrote:
> On July 5, 2018 12:56:50 AM PDT, Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> wrote:
> >On Wed, Jul 04, 2018 at 06:57:39PM +0200, Enric Balletbo Serra wrote:
> >> Missatge de Dmitry Torokhov <dmitry.torokhov@gmail.com> del dia dc.,
> >4
> >> de jul. 2018 a les 17:10:
> >> >
> >> > Hi Enric,
> >> >
> >> > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra
> >wrote:
> >> > > Hi Matti,
> >> > >
> >> > > Thanks for the patch, a few comments below, some are feedback I
> >> > > received when I sent some patches to this subsystem.
> >> > >
> >> > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> >del
> >> > > dia dt., 19 de juny 2018 a les 12:57:
> >> > > > +};
> >> > > > +
> >> > > > +/* bd71837 multi function cells */
> >> > > > +static struct mfd_cell bd71837_mfd_cells[] = {
> >> > > > + {
> >> > > > + .name = "bd71837-clk",
> >> > > > + }, {
> >> > > > + .name = "bd718xx-pwrkey",
> >> > > > + .resources = &irqs[0],
> >> > > > + .num_resources = ARRAY_SIZE(irqs),
> >> > > > + }, {
> >> > > > + .name = "bd71837-pmic",
> >> > > > + },
> >> > > nit: no comma at the end
> >> >
> >> > Actually, trailing comma is preferred on structures/arrays without
> >> > sentinels, because if one needs to add a new entry/new member, then
> >in
> >> > the diff there will have only one new line added, instead of one
> >line
> >> > being changed (adding now necessary comma) and one added.
> >> >
> >>
> >> Many thanks for sharing your knowledge! That looks to me a good
> >> reason.
> >
> >So in this specific ecample leaving the comma does not help. The
> >opening
> >brace for new array element would be added to same line where the comma
> >is, right?
>
> Ah, yes, you are right. We usually have either:
>
> { /* element 1 */ },
> { / *element 2 */ },
> ...
>
> or:
>
> {
> /* element 1 */
> },
> {
> /* element 2 */
> },
>
> but I do not think that it is codified in the CodingStyle.
FWIW, my *strong* preference for single line entries in the
aforementioned single line format. Then Dmitry's explanation rings
true.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-07-06 7:05 ` Lee Jones
0 siblings, 0 replies; 45+ messages in thread
From: Lee Jones @ 2018-07-06 7:05 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Matti Vaittinen, Enric Balletbo Serra, Michael Turquette, sboyd,
Rob Herring, Mark Rutland, Liam Girdwood, Mark Brown,
Matti Vaittinen, Arnd Bergmann, Sebastian Reichel, chenjh,
Andrey Smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree
On Thu, 05 Jul 2018, Dmitry Torokhov wrote:
> On July 5, 2018 12:56:50 AM PDT, Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> wrote:
> >On Wed, Jul 04, 2018 at 06:57:39PM +0200, Enric Balletbo Serra wrote:
> >> Missatge de Dmitry Torokhov <dmitry.torokhov@gmail.com> del dia dc.,
> >4
> >> de jul. 2018 a les 17:10:
> >> >
> >> > Hi Enric,
> >> >
> >> > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra
> >wrote:
> >> > > Hi Matti,
> >> > >
> >> > > Thanks for the patch, a few comments below, some are feedback I
> >> > > received when I sent some patches to this subsystem.
> >> > >
> >> > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> >del
> >> > > dia dt., 19 de juny 2018 a les 12:57:
> >> > > > +};
> >> > > > +
> >> > > > +/* bd71837 multi function cells */
> >> > > > +static struct mfd_cell bd71837_mfd_cells[] = {
> >> > > > + {
> >> > > > + .name = "bd71837-clk",
> >> > > > + }, {
> >> > > > + .name = "bd718xx-pwrkey",
> >> > > > + .resources = &irqs[0],
> >> > > > + .num_resources = ARRAY_SIZE(irqs),
> >> > > > + }, {
> >> > > > + .name = "bd71837-pmic",
> >> > > > + },
> >> > > nit: no comma at the end
> >> >
> >> > Actually, trailing comma is preferred on structures/arrays without
> >> > sentinels, because if one needs to add a new entry/new member, then
> >in
> >> > the diff there will have only one new line added, instead of one
> >line
> >> > being changed (adding now necessary comma) and one added.
> >> >
> >>
> >> Many thanks for sharing your knowledge! That looks to me a good
> >> reason.
> >
> >So in this specific ecample leaving the comma does not help. The
> >opening
> >brace for new array element would be added to same line where the comma
> >is, right?
>
> Ah, yes, you are right. We usually have either:
>
> { /* element 1 */ },
> { / *element 2 */ },
> ...
>
> or:
>
> {
> /* element 1 */
> },
> {
> /* element 2 */
> },
>
> but I do not think that it is codified in the CodingStyle.
FWIW, my *strong* preference for single line entries in the
aforementioned single line format. Then Dmitry's explanation rings
true.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
2018-07-06 7:05 ` Lee Jones
@ 2018-07-06 7:49 ` Matti Vaittinen
-1 siblings, 0 replies; 45+ messages in thread
From: Matti Vaittinen @ 2018-07-06 7:49 UTC (permalink / raw)
To: Lee Jones
Cc: Dmitry Torokhov, Enric Balletbo Serra, Michael Turquette, sboyd,
Rob Herring, Mark Rutland, Liam Girdwood, Mark Brown,
Matti Vaittinen, Arnd Bergmann, Sebastian Reichel, chenjh,
Andrey Smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree, linux-kernel,
linux-input, mikko.mutanen, heikki.haikola
On Fri, Jul 06, 2018 at 08:05:59AM +0100, Lee Jones wrote:
> On Thu, 05 Jul 2018, Dmitry Torokhov wrote:
>
> > On July 5, 2018 12:56:50 AM PDT, Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> wrote:
> > >On Wed, Jul 04, 2018 at 06:57:39PM +0200, Enric Balletbo Serra wrote:
> > >> Missatge de Dmitry Torokhov <dmitry.torokhov@gmail.com> del dia dc.,
> > >4
> > >> de jul. 2018 a les 17:10:
> > >> >
> > >> > Hi Enric,
> > >> >
> > >> > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra
> > >wrote:
> > >> > > > +static struct mfd_cell bd71837_mfd_cells[] = {
> > >> > > > + {
> > >> > > > + .name = "bd71837-clk",
> > >> > > > + }, {
> > >> > > > + .name = "bd718xx-pwrkey",
> > >> > > > + .resources = &irqs[0],
> > >> > > > + .num_resources = ARRAY_SIZE(irqs),
> > >> > > > + }, {
> > >> > > > + .name = "bd71837-pmic",
> > >> > > > + },
> > >> > > nit: no comma at the end
> > >> >
> > >> > Actually, trailing comma is preferred on structures/arrays without
> > >> > sentinels, because if one needs to add a new entry/new member, then
> > >in
> > >> > the diff there will have only one new line added, instead of one
> > >line
> > >> > being changed (adding now necessary comma) and one added.
> > >> >
> > >>
> > >> Many thanks for sharing your knowledge! That looks to me a good
> > >> reason.
> > >
> > >So in this specific ecample leaving the comma does not help. The
> > >opening
> > >brace for new array element would be added to same line where the comma
> > >is, right?
> >
> > Ah, yes, you are right. We usually have either:
> >
> > { /* element 1 */ },
> > { / *element 2 */ },
> > ...
> >
> > or:
> >
> > {
> > /* element 1 */
> > },
> > {
> > /* element 2 */
> > },
> >
> > but I do not think that it is codified in the CodingStyle.
>
> FWIW, my *strong* preference for single line entries in the
> aforementioned single line format. Then Dmitry's explanation rings
> true.
The reasoning given by Dmitry makes perfect sense. And to my eyes:
{
/* element 1 */
},
{
/* element 2 */
},
actually looks better than:
> {
/* element 1 */
}, {
/* element 2 */
},
So if first one is not enforced in order to minimize almost empty lines
- then I will try to be using the latter in the future. (In such cases
where element consists of more than one value).
Thanks for this little lesson =)
Br,
Matti Vaittinen
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
@ 2018-07-06 7:49 ` Matti Vaittinen
0 siblings, 0 replies; 45+ messages in thread
From: Matti Vaittinen @ 2018-07-06 7:49 UTC (permalink / raw)
To: Lee Jones
Cc: Dmitry Torokhov, Enric Balletbo Serra, Michael Turquette, sboyd,
Rob Herring, Mark Rutland, Liam Girdwood, Mark Brown,
Matti Vaittinen, Arnd Bergmann, Sebastian Reichel, chenjh,
Andrey Smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree
On Fri, Jul 06, 2018 at 08:05:59AM +0100, Lee Jones wrote:
> On Thu, 05 Jul 2018, Dmitry Torokhov wrote:
>
> > On July 5, 2018 12:56:50 AM PDT, Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> wrote:
> > >On Wed, Jul 04, 2018 at 06:57:39PM +0200, Enric Balletbo Serra wrote:
> > >> Missatge de Dmitry Torokhov <dmitry.torokhov@gmail.com> del dia dc.,
> > >4
> > >> de jul. 2018 a les 17:10:
> > >> >
> > >> > Hi Enric,
> > >> >
> > >> > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra
> > >wrote:
> > >> > > > +static struct mfd_cell bd71837_mfd_cells[] = {
> > >> > > > + {
> > >> > > > + .name = "bd71837-clk",
> > >> > > > + }, {
> > >> > > > + .name = "bd718xx-pwrkey",
> > >> > > > + .resources = &irqs[0],
> > >> > > > + .num_resources = ARRAY_SIZE(irqs),
> > >> > > > + }, {
> > >> > > > + .name = "bd71837-pmic",
> > >> > > > + },
> > >> > > nit: no comma at the end
> > >> >
> > >> > Actually, trailing comma is preferred on structures/arrays without
> > >> > sentinels, because if one needs to add a new entry/new member, then
> > >in
> > >> > the diff there will have only one new line added, instead of one
> > >line
> > >> > being changed (adding now necessary comma) and one added.
> > >> >
> > >>
> > >> Many thanks for sharing your knowledge! That looks to me a good
> > >> reason.
> > >
> > >So in this specific ecample leaving the comma does not help. The
> > >opening
> > >brace for new array element would be added to same line where the comma
> > >is, right?
> >
> > Ah, yes, you are right. We usually have either:
> >
> > { /* element 1 */ },
> > { / *element 2 */ },
> > ...
> >
> > or:
> >
> > {
> > /* element 1 */
> > },
> > {
> > /* element 2 */
> > },
> >
> > but I do not think that it is codified in the CodingStyle.
>
> FWIW, my *strong* preference for single line entries in the
> aforementioned single line format. Then Dmitry's explanation rings
> true.
The reasoning given by Dmitry makes perfect sense. And to my eyes:
{
/* element 1 */
},
{
/* element 2 */
},
actually looks better than:
> {
/* element 1 */
}, {
/* element 2 */
},
So if first one is not enforced in order to minimize almost empty lines
- then I will try to be using the latter in the future. (In such cases
where element consists of more than one value).
Thanks for this little lesson =)
Br,
Matti Vaittinen
^ permalink raw reply [flat|nested] 45+ messages in thread
end of thread, other threads:[~2018-07-06 7:49 UTC | newest]
Thread overview: 45+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-19 10:55 [PATCH v7 0/4] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver Matti Vaittinen
2018-06-19 10:55 ` [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC Matti Vaittinen
2018-06-26 9:06 ` Enric Balletbo Serra
2018-06-26 9:06 ` Enric Balletbo Serra
2018-06-26 11:24 ` Matti Vaittinen
2018-06-26 11:24 ` Matti Vaittinen
2018-06-26 11:40 ` Enric Balletbo Serra
2018-06-26 11:40 ` Enric Balletbo Serra
2018-06-26 12:03 ` Matti Vaittinen
2018-06-26 12:03 ` Matti Vaittinen
2018-06-26 14:24 ` Enric Balletbo Serra
2018-06-26 14:24 ` Enric Balletbo Serra
2018-07-03 6:56 ` Lee Jones
2018-07-03 6:56 ` Lee Jones
2018-07-03 8:09 ` Enric Balletbo Serra
2018-07-03 8:09 ` Enric Balletbo Serra
2018-07-03 8:09 ` Enric Balletbo Serra
2018-07-03 6:53 ` Lee Jones
2018-07-03 6:53 ` Lee Jones
2018-07-04 14:56 ` Dmitry Torokhov
2018-07-04 14:56 ` Dmitry Torokhov
2018-07-04 16:57 ` Enric Balletbo Serra
2018-07-04 16:57 ` Enric Balletbo Serra
2018-07-05 5:52 ` Lee Jones
2018-07-05 5:52 ` Lee Jones
2018-07-05 7:56 ` Matti Vaittinen
2018-07-05 7:56 ` Matti Vaittinen
2018-07-06 6:38 ` Dmitry Torokhov
2018-07-06 6:38 ` Dmitry Torokhov
2018-07-06 6:38 ` Dmitry Torokhov
2018-07-06 7:05 ` Lee Jones
2018-07-06 7:05 ` Lee Jones
2018-07-06 7:49 ` Matti Vaittinen
2018-07-06 7:49 ` Matti Vaittinen
2018-06-19 10:56 ` [PATCH v7 2/4] mfd: bd71837: Devicetree bindings " Matti Vaittinen
2018-06-19 10:56 ` [PATCH v7 3/4] clk: bd71837: Add driver for BD71837 PMIC clock Matti Vaittinen
2018-06-19 10:57 ` [PATCH v7 4/4] input/power: Add driver for BD71837/BD71847 PMIC power button Matti Vaittinen
2018-06-19 17:50 ` Dmitry Torokhov
2018-06-20 6:43 ` Matti Vaittinen
2018-06-21 10:25 ` Matti Vaittinen
2018-06-27 0:21 ` Dmitry Torokhov
2018-06-21 10:34 ` [PATCH v7 0/4] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver Matti Vaittinen
2018-07-03 7:02 ` Lee Jones
2018-07-04 8:47 ` Matti Vaittinen
2018-07-04 9:21 ` Lee Jones
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.