| Message ID | a304164cc3c1b31c7c6b62f8e233ce321a82cdd6.1530776326.git.matti.vaittinen@fi.rohmeurope.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Matti, Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del dia dj., 5 de jul. 2018 a les 9:46: > > ROHM BD71837 PMIC MFD driver providing interrupts and support > for three subsystems: > - clk > - Regulators > - input/power-key > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > --- > drivers/mfd/Kconfig | 13 ++ > drivers/mfd/Makefile | 1 + > drivers/mfd/bd71837.c | 215 ++++++++++++++++++++++++ > include/linux/mfd/bd71837.h | 391 ++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 620 insertions(+) > create mode 100644 drivers/mfd/bd71837.c > create mode 100644 include/linux/mfd/bd71837.h > (snip) Looks good to me. Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> Best Regards, Enric -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 05 Jul 2018, Matti Vaittinen wrote: > ROHM BD71837 PMIC MFD driver providing interrupts and support > for three subsystems: > - clk > - Regulators > - input/power-key > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > --- > drivers/mfd/Kconfig | 13 ++ > drivers/mfd/Makefile | 1 + > drivers/mfd/bd71837.c | 215 ++++++++++++++++++++++++ > include/linux/mfd/bd71837.h | 391 ++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 620 insertions(+) > create mode 100644 drivers/mfd/bd71837.c > create mode 100644 include/linux/mfd/bd71837.h > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index b860eb5aa194..d01a279f5e4a 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -1787,6 +1787,19 @@ config MFD_STW481X > in various ST Microelectronics and ST-Ericsson embedded > Nomadik series. > > +config MFD_BD71837 It's be nice if you prefix this with the manufacturer. Rohm? > + tristate "BD71837 Power Management chip" Again, missing manufacturer. This should also be "Power Management IC" > + depends on I2C=y > + depends on OF > + select REGMAP_I2C > + select REGMAP_IRQ > + select MFD_CORE > + help > + Select this option to get support for the ROHM BD71837 > + Power Management chips. BD71837 is designed to power processors like "chips" is slang. Should be "ICs". > + NXP i.MX8. It contains 8 BUCK outputs and 7 LDOs, voltage monitoring > + and emergency shut down as well as 32,768KHz clock output. > + > config MFD_STM32_LPTIMER > tristate "Support for STM32 Low-Power Timer" > depends on (ARCH_STM32 && OF) || COMPILE_TEST > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index e9fd20dba18d..09dc9eb3782c 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -227,4 +227,5 @@ obj-$(CONFIG_MFD_STM32_TIMERS) += stm32-timers.o > obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o > obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o > obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o > +obj-$(CONFIG_MFD_BD71837) += bd71837.o > > diff --git a/drivers/mfd/bd71837.c b/drivers/mfd/bd71837.c rohm-bd71837.c would be nicer. > new file mode 100644 > index 000000000000..677097bcea17 > --- /dev/null > +++ b/drivers/mfd/bd71837.c > @@ -0,0 +1,215 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later '\n' here please. > +// Copyright (C) 2018 ROHM Semiconductors '\n' here please. > +// bd71837.c -- ROHM BD71837MWV mfd driver Remove the filename -- they have a habit of becoming incorrect. Also, and 'MFD driver' isn't a thing -- this is a PMIC driver. > +// Datasheet available from > +// https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e > + > +#include <linux/i2c.h> > +#include <linux/input.h> > +#include <linux/interrupt.h> > +#include <linux/mfd/bd71837.h> > +#include <linux/mfd/core.h> > +#include <linux/module.h> > +#include <linux/regmap.h> > +#include <linux/gpio_keys.h> Alphabetical please. > +static struct gpio_keys_button btns[] = { > + { > + .code = KEY_POWER, > + .gpio = -1, > + .type = EV_KEY, > + }, > +}; Does this need to be an array? > +static struct gpio_keys_platform_data bd718xx_powerkey_data = { > + .buttons = &btns[0], Why not just "btns"? Also, I don't see any reason not to just call this 'buttons'. Or even, 'button' if there will only ever be one of them. > + .nbuttons = ARRAY_SIZE(btns), > + .name = "bd718xx-pwrkey", > +}; > + > +static struct mfd_cell bd71837_mfd_cells[] = { > + { > + .name = "bd71837-clk", > + }, { > + .name = "gpio-keys", > + .platform_data = &bd718xx_powerkey_data, > + .pdata_size = sizeof(bd718xx_powerkey_data), > + }, { > + .name = "bd71837-pmic", > + }, > +}; Place the one line entries on one line, like this: { .name = "bd71837-pmic", }, ... and if ordering is not important, place them at the bottom. > +static const struct regmap_irq bd71837_irqs[] = { > + REGMAP_IRQ_REG(BD71837_INT_SWRST, 0, BD71837_INT_SWRST_MASK), > + REGMAP_IRQ_REG(BD71837_INT_PWRBTN_S, 0, BD71837_INT_PWRBTN_S_MASK), > + REGMAP_IRQ_REG(BD71837_INT_PWRBTN_L, 0, BD71837_INT_PWRBTN_L_MASK), > + REGMAP_IRQ_REG(BD71837_INT_PWRBTN, 0, BD71837_INT_PWRBTN_MASK), > + REGMAP_IRQ_REG(BD71837_INT_WDOG, 0, BD71837_INT_WDOG_MASK), > + REGMAP_IRQ_REG(BD71837_INT_ON_REQ, 0, BD71837_INT_ON_REQ_MASK), > + REGMAP_IRQ_REG(BD71837_INT_STBY_REQ, 0, BD71837_INT_STBY_REQ_MASK), > +}; > + > +static struct regmap_irq_chip bd71837_irq_chip = { > + .name = "bd71837-irq", > + .irqs = bd71837_irqs, > + .num_irqs = ARRAY_SIZE(bd71837_irqs), > + .num_regs = 1, > + .irq_reg_stride = 1, > + .status_base = BD71837_REG_IRQ, > + .mask_base = BD71837_REG_MIRQ, > + .ack_base = BD71837_REG_IRQ, > + .init_ack_masked = true, > + .mask_invert = false, > +}; > + > +static const struct regmap_range pmic_status_range = { > + .range_min = BD71837_REG_IRQ, > + .range_max = BD71837_REG_POW_STATE, > +}; > + > +static const struct regmap_access_table volatile_regs = { > + .yes_ranges = &pmic_status_range, > + .n_yes_ranges = 1, > +}; > + > +static const struct regmap_config bd71837_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .volatile_table = &volatile_regs, > + .max_register = BD71837_MAX_REGISTER - 1, > + .cache_type = REGCACHE_RBTREE, > +}; > + > +static const struct of_device_id bd71837_of_match[] = { > + { .compatible = "rohm,bd71837", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, bd71837_of_match); > + > +static int bd71837_i2c_probe(struct i2c_client *i2c, > + const struct i2c_device_id *id) > +{ > + struct bd71837 *bd71837; > + struct bd71837_board *board_info; > + int ret = -EINVAL; I'd prefer it if you set the error at the location it happened for readability. > + bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL); > + Drop this line. > + if (!bd71837) > + return -ENOMEM; > + > + board_info = dev_get_platdata(&i2c->dev); > + > + if (!board_info) > + bd71837->chip_irq = i2c->irq; > + else > + bd71837->chip_irq = board_info->gpio_intr; > + > + if (!bd71837->chip_irq) { > + dev_err(&i2c->dev, "No IRQ configured\n"); > + goto err_out; > + } > + > + bd71837->dev = &i2c->dev; > + bd71837->i2c_client = i2c; > + i2c_set_clientdata(i2c, bd71837); > + > + bd71837->regmap = devm_regmap_init_i2c(i2c, &bd71837_regmap_config); > + if (IS_ERR(bd71837->regmap)) { > + ret = PTR_ERR(bd71837->regmap); > + dev_err(&i2c->dev, "regmap initialization failed\n"); > + goto err_out; > + } > + > + ret = bd71837_reg_read(bd71837, BD71837_REG_REV); > + if (ret < 0) { Is 0 return a successful read? I suggest that it isn't. > + dev_err(&i2c->dev, "Read BD71837_REG_DEVICE failed\n"); > + goto err_out; > + } > + > + ret = devm_regmap_add_irq_chip(&i2c->dev, bd71837->regmap, > + bd71837->chip_irq, IRQF_ONESHOT, 0, > + &bd71837_irq_chip, &bd71837->irq_data); > + if (ret < 0) { Is a >0 return valid? If not, perhaps "if (ret)" would be better. > + dev_err(&i2c->dev, "Failed to add irq_chip\n"); > + goto err_out; > + } > + > + ret = regmap_update_bits(bd71837->regmap, > + BD71837_REG_PWRONCONFIG0, > + BD718XX_PWRBTN_PRESS_DURATION_MASK, > + BD718XX_PWRBTN_SHORT_PRESS_10MS); > + if (ret < 0) { As above. > + dev_err(&i2c->dev, "Failed to configure button short press timeout\n"); > + goto err_out; > + } > + > + /* > + * According to BD71847 datasheet the HW default for long press > + * detection is 10ms. So lets change it to 10 sec so we can actually > + * get the short push and allow gracefull shut down > + */ I don't think this comment is necessary. If anything it only needs to say "Configure long press to 10 seconds". And if you do that, please add one for short press too. > + ret = regmap_update_bits(bd71837->regmap, > + BD71837_REG_PWRONCONFIG1, > + BD718XX_PWRBTN_PRESS_DURATION_MASK, > + BD718XX_PWRBTN_LONG_PRESS_10S); > + > + if (ret < 0) { > + dev_err(&i2c->dev, "Failed to configure button long press timeout\n"); > + goto err_out; > + } > + > + btns[0].irq = regmap_irq_get_virq(bd71837->irq_data, > + BD71837_INT_PWRBTN_S); > + > + if (btns[0].irq < 0) { > + ret = btns[0].irq; > + dev_err(&i2c->dev, "Failed to get the IRQ\n"); > + goto err_out; No unwinding to be done, just return directly and save yourself an superflouous goto. > + } > + > + ret = devm_mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO, > + bd71837_mfd_cells, > + ARRAY_SIZE(bd71837_mfd_cells), NULL, 0, > + regmap_irq_get_domain(bd71837->irq_data)); > + if (ret) > + dev_err(&i2c->dev, "Failed to create subdevices\n"); > +err_out: > + > + return ret; > +} > + > +static const struct i2c_device_id bd71837_i2c_id[] = { > + { .name = "bd71837", }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id); You shouldn't need this table anymore. See da10c06a044b ("i2c: Make I2C ID tables non-mandatory for DT'ed devices") > +static struct i2c_driver bd71837_i2c_driver = { > + .driver = { > + .name = "bd71837-mfd", And "MFD" isn't really a thing. It's more common to just mention the board name, or something like 'rohm-bd7183'. > + .of_match_table = bd71837_of_match, > + }, > + .probe = bd71837_i2c_probe, > + .id_table = bd71837_i2c_id, > +}; > + > +static int __init bd71837_i2c_init(void) > +{ > + return i2c_add_driver(&bd71837_i2c_driver); > +} > +/* > + * init early so consumer devices can complete system boot > + */ This is a single line commit. Please use the correct format. /* Single line comment */ > +subsys_initcall(bd71837_i2c_init); > + > +static void __exit bd71837_i2c_exit(void) > +{ > + i2c_del_driver(&bd71837_i2c_driver); > +} > +module_exit(bd71837_i2c_exit); > + > +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>"); > +MODULE_DESCRIPTION("BD71837 chip multi-function driver"); 'PMIC' or 'Power Management IC' rather than MFD. > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/mfd/bd71837.h b/include/linux/mfd/bd71837.h > new file mode 100644 > index 000000000000..10b0dfe90f27 > --- /dev/null > +++ b/include/linux/mfd/bd71837.h > @@ -0,0 +1,391 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ I thought these were meant to use C++ (//) comments? > +/* Copyright (C) 2018 ROHM Semiconductors */ > + > +/* > + * ROHM BD71837MWV header file > + */ If you prefix the mentions of (BD,bd)71837 with (ROHM,rohm) then this comment becomes redundant and you can remove it. > +#ifndef __LINUX_MFD_BD71837_H__ > +#define __LINUX_MFD_BD71837_H__ > + > +#include <linux/regmap.h> > + > +enum { > + BD71837_BUCK1 = 0, > + BD71837_BUCK2, > + BD71837_BUCK3, > + BD71837_BUCK4, > + BD71837_BUCK5, > + BD71837_BUCK6, > + BD71837_BUCK7, > + BD71837_BUCK8, > + BD71837_LDO1, > + BD71837_LDO2, > + BD71837_LDO3, > + BD71837_LDO4, > + BD71837_LDO5, > + BD71837_LDO6, > + BD71837_LDO7, > + BD71837_REGULATOR_CNT, > +}; > + > +#define BD71837_BUCK1_VOLTAGE_NUM 0x40 > +#define BD71837_BUCK2_VOLTAGE_NUM 0x40 > +#define BD71837_BUCK3_VOLTAGE_NUM 0x40 > +#define BD71837_BUCK4_VOLTAGE_NUM 0x40 > + > +#define BD71837_BUCK5_VOLTAGE_NUM 0x08 > +#define BD71837_BUCK6_VOLTAGE_NUM 0x04 > +#define BD71837_BUCK7_VOLTAGE_NUM 0x08 > +#define BD71837_BUCK8_VOLTAGE_NUM 0x40 > + > +#define BD71837_LDO1_VOLTAGE_NUM 0x04 > +#define BD71837_LDO2_VOLTAGE_NUM 0x02 > +#define BD71837_LDO3_VOLTAGE_NUM 0x10 > +#define BD71837_LDO4_VOLTAGE_NUM 0x10 > +#define BD71837_LDO5_VOLTAGE_NUM 0x10 > +#define BD71837_LDO6_VOLTAGE_NUM 0x10 > +#define BD71837_LDO7_VOLTAGE_NUM 0x10 > + > +enum { > + BD71837_REG_REV = 0x00, > + BD71837_REG_SWRESET = 0x01, > + BD71837_REG_I2C_DEV = 0x02, > + BD71837_REG_PWRCTRL0 = 0x03, > + BD71837_REG_PWRCTRL1 = 0x04, > + BD71837_REG_BUCK1_CTRL = 0x05, > + BD71837_REG_BUCK2_CTRL = 0x06, > + BD71837_REG_BUCK3_CTRL = 0x07, > + BD71837_REG_BUCK4_CTRL = 0x08, > + BD71837_REG_BUCK5_CTRL = 0x09, > + BD71837_REG_BUCK6_CTRL = 0x0A, > + BD71837_REG_BUCK7_CTRL = 0x0B, > + BD71837_REG_BUCK8_CTRL = 0x0C, > + BD71837_REG_BUCK1_VOLT_RUN = 0x0D, > + BD71837_REG_BUCK1_VOLT_IDLE = 0x0E, > + BD71837_REG_BUCK1_VOLT_SUSP = 0x0F, > + BD71837_REG_BUCK2_VOLT_RUN = 0x10, > + BD71837_REG_BUCK2_VOLT_IDLE = 0x11, > + BD71837_REG_BUCK3_VOLT_RUN = 0x12, > + BD71837_REG_BUCK4_VOLT_RUN = 0x13, > + BD71837_REG_BUCK5_VOLT = 0x14, > + BD71837_REG_BUCK6_VOLT = 0x15, > + BD71837_REG_BUCK7_VOLT = 0x16, > + BD71837_REG_BUCK8_VOLT = 0x17, > + BD71837_REG_LDO1_VOLT = 0x18, > + BD71837_REG_LDO2_VOLT = 0x19, > + BD71837_REG_LDO3_VOLT = 0x1A, > + BD71837_REG_LDO4_VOLT = 0x1B, > + BD71837_REG_LDO5_VOLT = 0x1C, > + BD71837_REG_LDO6_VOLT = 0x1D, > + BD71837_REG_LDO7_VOLT = 0x1E, > + BD71837_REG_TRANS_COND0 = 0x1F, > + BD71837_REG_TRANS_COND1 = 0x20, > + BD71837_REG_VRFAULTEN = 0x21, > + BD71837_REG_MVRFLTMASK0 = 0x22, > + BD71837_REG_MVRFLTMASK1 = 0x23, > + BD71837_REG_MVRFLTMASK2 = 0x24, > + BD71837_REG_RCVCFG = 0x25, > + BD71837_REG_RCVNUM = 0x26, > + BD71837_REG_PWRONCONFIG0 = 0x27, > + BD71837_REG_PWRONCONFIG1 = 0x28, > + BD71837_REG_RESETSRC = 0x29, > + BD71837_REG_MIRQ = 0x2A, > + BD71837_REG_IRQ = 0x2B, > + BD71837_REG_IN_MON = 0x2C, > + BD71837_REG_POW_STATE = 0x2D, > + BD71837_REG_OUT32K = 0x2E, > + BD71837_REG_REGLOCK = 0x2F, > + BD71837_REG_OTPVER = 0xFF, > + BD71837_MAX_REGISTER = 0x100, > +}; > + > +#define REGLOCK_PWRSEQ 0x1 > +#define REGLOCK_VREG 0x10 > + > +/* Generic BUCK control masks */ > +#define BD71837_BUCK_SEL 0x02 > +#define BD71837_BUCK_EN 0x01 > +#define BD71837_BUCK_RUN_ON 0x04 > + > +/* Generic LDO masks */ > +#define BD71837_LDO_SEL 0x80 > +#define BD71837_LDO_EN 0x40 > + > +/* BD71837 BUCK ramp rate CTRL reg bits */ > +#define BUCK_RAMPRATE_MASK 0xC0 > +#define BUCK_RAMPRATE_10P00MV 0x0 > +#define BUCK_RAMPRATE_5P00MV 0x1 > +#define BUCK_RAMPRATE_2P50MV 0x2 > +#define BUCK_RAMPRATE_1P25MV 0x3 > + > +/* BD71837_REG_BUCK1_VOLT_RUN bits */ > +#define BUCK1_RUN_MASK 0x3F > +#define BUCK1_RUN_DEFAULT 0x14 > + > +/* BD71837_REG_BUCK1_VOLT_SUSP bits */ > +#define BUCK1_SUSP_MASK 0x3F > +#define BUCK1_SUSP_DEFAULT 0x14 > + > +/* BD71837_REG_BUCK1_VOLT_IDLE bits */ > +#define BUCK1_IDLE_MASK 0x3F > +#define BUCK1_IDLE_DEFAULT 0x14 > + > +/* BD71837_REG_BUCK2_VOLT_RUN bits */ > +#define BUCK2_RUN_MASK 0x3F > +#define BUCK2_RUN_DEFAULT 0x1E > + > +/* BD71837_REG_BUCK2_VOLT_IDLE bits */ > +#define BUCK2_IDLE_MASK 0x3F > +#define BUCK2_IDLE_DEFAULT 0x14 > + > +/* BD71837_REG_BUCK3_VOLT_RUN bits */ > +#define BUCK3_RUN_MASK 0x3F > +#define BUCK3_RUN_DEFAULT 0x1E > + > +/* BD71837_REG_BUCK4_VOLT_RUN bits */ > +#define BUCK4_RUN_MASK 0x3F > +#define BUCK4_RUN_DEFAULT 0x1E > + > +/* BD71837_REG_BUCK5_VOLT bits */ > +#define BUCK5_MASK 0x07 > +#define BUCK5_DEFAULT 0x02 > + > +/* BD71837_REG_BUCK6_VOLT bits */ > +#define BUCK6_MASK 0x03 > +#define BUCK6_DEFAULT 0x03 > + > +/* BD71837_REG_BUCK7_VOLT bits */ > +#define BUCK7_MASK 0x07 > +#define BUCK7_DEFAULT 0x03 > + > +/* BD71837_REG_BUCK8_VOLT bits */ > +#define BUCK8_MASK 0x3F > +#define BUCK8_DEFAULT 0x1E > + > +/* BD71837_REG_IRQ bits */ > +#define IRQ_SWRST 0x40 > +#define IRQ_PWRON_S 0x20 > +#define IRQ_PWRON_L 0x10 > +#define IRQ_PWRON 0x08 > +#define IRQ_WDOG 0x04 > +#define IRQ_ON_REQ 0x02 > +#define IRQ_STBY_REQ 0x01 > + > +/* BD71837_REG_OUT32K bits */ > +#define BD71837_OUT32K_EN 0x01 > + > +/* BD71837 gated clock rate */ > +#define BD71837_CLK_RATE 32768 > + > +/* BD71837 irqs */ > +enum { > + BD71837_INT_STBY_REQ, > + BD71837_INT_ON_REQ, > + BD71837_INT_WDOG, > + BD71837_INT_PWRBTN, > + BD71837_INT_PWRBTN_L, > + BD71837_INT_PWRBTN_S, > + BD71837_INT_SWRST > +}; > + > +/* BD71837 interrupt masks */ > +#define BD71837_INT_SWRST_MASK 0x40 > +#define BD71837_INT_PWRBTN_S_MASK 0x20 > +#define BD71837_INT_PWRBTN_L_MASK 0x10 > +#define BD71837_INT_PWRBTN_MASK 0x8 > +#define BD71837_INT_WDOG_MASK 0x4 > +#define BD71837_INT_ON_REQ_MASK 0x2 > +#define BD71837_INT_STBY_REQ_MASK 0x1 > + > +/* BD71837_REG_LDO1_VOLT bits */ > +#define LDO1_MASK 0x03 > + > +/* BD71837_REG_LDO1_VOLT bits */ > +#define LDO2_MASK 0x20 > + > +/* BD71837_REG_LDO3_VOLT bits */ > +#define LDO3_MASK 0x0F > + > +/* BD71837_REG_LDO4_VOLT bits */ > +#define LDO4_MASK 0x0F > + > +/* BD71837_REG_LDO5_VOLT bits */ > +#define LDO5_MASK 0x0F > + > +/* BD71837_REG_LDO6_VOLT bits */ > +#define LDO6_MASK 0x0F > + > +/* BD71837_REG_LDO7_VOLT bits */ > +#define LDO7_MASK 0x0F > + > +/* Register write induced reset settings */ > + > +/* Even though the bit zero is not SWRESET type we still want to write zero > + * to it when changing type. Bit zero is 'SWRESET' trigger bit and if we > + * write 1 to it we will trigger the action. So always write 0 to it when > + * changning SWRESET action - no matter what we read from it. > + */ Please use the correct format for multi-line comments. Hint: The first line should be empty. > +#define BD71837_SWRESET_TYPE_MASK 7 > +#define BD71837_SWRESET_TYPE_DISABLED 0 > +#define BD71837_SWRESET_TYPE_COLD 4 > +#define BD71837_SWRESET_TYPE_WARM 6 > + > +#define BD71837_SWRESET_RESET_MASK 1 > +#define BD71837_SWRESET_RESET 1 > + > +/* Poweroff state transition conditions */ > + > +#define BD718XX_ON_REQ_POWEROFF_MASK 1 > +#define BD718XX_SWRESET_POWEROFF_MASK 2 > +#define BD718XX_WDOG_POWEROFF_MASK 4 > +#define BD718XX_KEY_L_POWEROFF_MASK 8 > + > +#define BD718XX_POWOFF_TO_SNVS 0 > +#define BD718XX_POWOFF_TO_RDY 0xF Please align each of the values with tabs. > +#define BD718XX_POWOFF_TIME_MASK 0xF0 > +enum { > + BD718XX_POWOFF_TIME_5MS = 0, > + BD718XX_POWOFF_TIME_10MS, > + BD718XX_POWOFF_TIME_15MS, > + BD718XX_POWOFF_TIME_20MS, > + BD718XX_POWOFF_TIME_25MS, > + BD718XX_POWOFF_TIME_30MS, > + BD718XX_POWOFF_TIME_35MS, > + BD718XX_POWOFF_TIME_40MS, > + BD718XX_POWOFF_TIME_45MS, > + BD718XX_POWOFF_TIME_50MS, > + BD718XX_POWOFF_TIME_75MS, > + BD718XX_POWOFF_TIME_100MS, > + BD718XX_POWOFF_TIME_250MS, > + BD718XX_POWOFF_TIME_500MS, > + BD718XX_POWOFF_TIME_750MS, > + BD718XX_POWOFF_TIME_1500MS > +}; > + > +/* Poweron sequence state transition conditions */ > + > +#define BD718XX_RDY_TO_SNVS_MASK 0xF > +#define BD718XX_SNVS_TO_RUN_MASK 0xF0 > + > +#define BD718XX_PWR_TRIG_KEY_L 1 > +#define BD718XX_PWR_TRIG_KEY_S 2 > +#define BD718XX_PWR_TRIG_PMIC_ON 4 > +#define BD718XX_PWR_TRIG_VSYS_UVLO 8 > +#define BD718XX_RDY_TO_SNVS_SIFT 0 > +#define BD718XX_SNVS_TO_RUN_SIFT 4 As above. > + > + No need for 2 '\n'. > +#define BD718XX_PWRBTN_PRESS_DURATION_MASK 0xF > + > +/* Timeout value for detecting short press */ > + > +enum { > + BD718XX_PWRBTN_SHORT_PRESS_10MS = 0, > + BD718XX_PWRBTN_SHORT_PRESS_500MS, > + BD718XX_PWRBTN_SHORT_PRESS_1000MS, > + BD718XX_PWRBTN_SHORT_PRESS_1500MS, > + BD718XX_PWRBTN_SHORT_PRESS_2000MS, > + BD718XX_PWRBTN_SHORT_PRESS_2500MS, > + BD718XX_PWRBTN_SHORT_PRESS_3000MS, > + BD718XX_PWRBTN_SHORT_PRESS_3500MS, > + BD718XX_PWRBTN_SHORT_PRESS_4000MS, > + BD718XX_PWRBTN_SHORT_PRESS_4500MS, > + BD718XX_PWRBTN_SHORT_PRESS_5000MS, > + BD718XX_PWRBTN_SHORT_PRESS_5500MS, > + BD718XX_PWRBTN_SHORT_PRESS_6000MS, > + BD718XX_PWRBTN_SHORT_PRESS_6500MS, > + BD718XX_PWRBTN_SHORT_PRESS_7000MS, > + BD718XX_PWRBTN_SHORT_PRESS_7500MS > +}; > + > +/* Timeout value for detecting LONG press */ > + > +enum { > + BD718XX_PWRBTN_LONG_PRESS_10MS = 0, > + BD718XX_PWRBTN_LONG_PRESS_1S, > + BD718XX_PWRBTN_LONG_PRESS_2S, > + BD718XX_PWRBTN_LONG_PRESS_3S, > + BD718XX_PWRBTN_LONG_PRESS_4S, > + BD718XX_PWRBTN_LONG_PRESS_5S, > + BD718XX_PWRBTN_LONG_PRESS_6S, > + BD718XX_PWRBTN_LONG_PRESS_7S, > + BD718XX_PWRBTN_LONG_PRESS_8S, > + BD718XX_PWRBTN_LONG_PRESS_9S, > + BD718XX_PWRBTN_LONG_PRESS_10S, > + BD718XX_PWRBTN_LONG_PRESS_11S, > + BD718XX_PWRBTN_LONG_PRESS_12S, > + BD718XX_PWRBTN_LONG_PRESS_13S, > + BD718XX_PWRBTN_LONG_PRESS_14S, > + BD718XX_PWRBTN_LONG_PRESS_15S > +}; > + > + > + Certainly no need for 3 '\n'. > +struct bd71837; Why is this necessary? > +struct bd71837_pmic; > +struct bd71837_clk; > +struct bd718xx_pwrkey; Where are these forward declared from? > +/* > + * Board platform data may be used to initialize regulators. > + */ > + > +struct bd71837_board { > + struct regulator_init_data *init_data[BD71837_REGULATOR_CNT]; > + int gpio_intr; > +}; Where is this populated? > +struct bd71837 { > + struct device *dev; > + struct i2c_client *i2c_client; > + struct regmap *regmap; > + unsigned long int id; > + > + int chip_irq; > + struct regmap_irq_chip_data *irq_data; > + > + struct bd71837_pmic *pmic; > + struct bd71837_clk *clk; > + struct bd718xx_pwrkey *pwrkey; > +}; > + > +/* > + * bd71837 sub-driver chip access routines > + */ > + > +static inline int bd71837_reg_read(struct bd71837 *bd71837, u8 reg) > +{ > + int r, val; > + > + r = regmap_read(bd71837->regmap, reg, &val); > + if (r < 0) > + return r; > + return val; > +} > + > +static inline int bd71837_reg_write(struct bd71837 *bd71837, u8 reg, > + unsigned int val) > +{ > + return regmap_write(bd71837->regmap, reg, val); > +} > + > +static inline int bd71837_set_bits(struct bd71837 *bd71837, u8 reg, u8 mask) > +{ > + return regmap_update_bits(bd71837->regmap, reg, mask, mask); > +} > + > +static inline int bd71837_clear_bits(struct bd71837 *bd71837, u8 reg, > + u8 mask) > +{ > + return regmap_update_bits(bd71837->regmap, reg, mask, 0); > +} > + > +static inline int bd71837_update_bits(struct bd71837 *bd71837, u8 reg, > + u8 mask, u8 val) > +{ > + return regmap_update_bits(bd71837->regmap, reg, mask, val); > +} Please don't abstract APIs for no reason. Use the regmap_* API directly instead. > +#endif /* __LINUX_MFD_BD71837_H__ */
Thanks for taking the time to check this =) On Thu, Jul 05, 2018 at 10:18:13AM +0100, Lee Jones wrote: > On Thu, 05 Jul 2018, Matti Vaittinen wrote: > > > --- a/drivers/mfd/Kconfig > > +++ b/drivers/mfd/Kconfig > > @@ -1787,6 +1787,19 @@ config MFD_STW481X > > in various ST Microelectronics and ST-Ericsson embedded > > Nomadik series. > > > > +config MFD_BD71837 > > It's be nice if you prefix this with the manufacturer. Rohm? Like MFD_ROHM_BD71837, right? How important this is? The already applied regulator part has depends_on for MFD_BD71837 in Kconfig so changing this will require change in regulator tree too. I guess it's doable if this is important though =) > > > + tristate "BD71837 Power Management chip" > > Again, missing manufacturer. > > This should also be "Power Management IC" I'll fix this > > + select MFD_CORE > > + help > > + Select this option to get support for the ROHM BD71837 > > + Power Management chips. BD71837 is designed to power processors like > > "chips" is slang. Should be "ICs". I'll fix this > > +obj-$(CONFIG_MFD_BD71837) += bd71837.o > > > > diff --git a/drivers/mfd/bd71837.c b/drivers/mfd/bd71837.c > > rohm-bd71837.c would be nicer. I'll fix this > > new file mode 100644 > > index 000000000000..677097bcea17 > > --- /dev/null > > +++ b/drivers/mfd/bd71837.c > > @@ -0,0 +1,215 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > '\n' here please. > > +// Copyright (C) 2018 ROHM Semiconductors > > '\n' here please. As an empty line after licence line? Ok. > > +// bd71837.c -- ROHM BD71837MWV mfd driver > > Remove the filename -- they have a habit of becoming incorrect. > > Also, and 'MFD driver' isn't a thing -- this is a PMIC driver. I'll fix this > > +// Datasheet available from > > +// https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e > > + > > +#include <linux/i2c.h> > > +#include <linux/input.h> > > +#include <linux/interrupt.h> > > +#include <linux/mfd/bd71837.h> > > +#include <linux/mfd/core.h> > > +#include <linux/module.h> > > +#include <linux/regmap.h> > > +#include <linux/gpio_keys.h> > > Alphabetical please. > There is a problem with gpio_key.h - it uses bool but does not include the header defining this type. So I get comple error when compiling this using my GCC arm cross-compiler. I've submitted a patch to fix this but as it is not yet accepted. https://lore.kernel.org/lkml/20180627073458.GA27800@localhost.localdomain/ Hence the gpio_keys.h is last one. > > +static struct gpio_keys_button btns[] = { > > + { > > + .code = KEY_POWER, > > + .gpio = -1, > > + .type = EV_KEY, > > + }, > > +}; > > Does this need to be an array? https://elixir.bootlin.com/linux/v4.17-rc1/source/include/linux/gpio_keys.h#L39 gpio_keys_platform_data documentation states the buttons to be a pointer to an array. Thus I did an array. Do you think I should change this to single struct? > > +static struct gpio_keys_platform_data bd718xx_powerkey_data = { > > + .buttons = &btns[0], > > Why not just "btns"? > > Also, I don't see any reason not to just call this 'buttons'. > > Or even, 'button' if there will only ever be one of them. > I can rename this - and if I make the array to not be an array - well, then the &foo[0] gets cleared too =) > > +static struct mfd_cell bd71837_mfd_cells[] = { > > + { > > + .name = "bd71837-clk", > > + }, { > > + .name = "gpio-keys", > > + .platform_data = &bd718xx_powerkey_data, > > + .pdata_size = sizeof(bd718xx_powerkey_data), > > + }, { > > + .name = "bd71837-pmic", > > + }, > > +}; > > Place the one line entries on one line, like this: > > { .name = "bd71837-pmic", }, > > ... and if ordering is not important, place them at the bottom. I'll fix this > > +static int bd71837_i2c_probe(struct i2c_client *i2c, > > + const struct i2c_device_id *id) > > +{ > > + struct bd71837 *bd71837; > > + struct bd71837_board *board_info; > > + int ret = -EINVAL; > > I'd prefer it if you set the error at the location it happened for > readability. I'll fix this > > + bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL); > > + > > Drop this line. I'll fix this > > + ret = bd71837_reg_read(bd71837, BD71837_REG_REV); > > + if (ret < 0) { > > Is 0 return a successful read? I suggest that it isn't. > > > + ret = devm_regmap_add_irq_chip(&i2c->dev, bd71837->regmap, > > + bd71837->chip_irq, IRQF_ONESHOT, 0, > > + &bd71837_irq_chip, &bd71837->irq_data); > > + if (ret < 0) { > > Is a >0 return valid? If not, perhaps "if (ret)" would be better. > > > + ret = regmap_update_bits(bd71837->regmap, > > + BD71837_REG_PWRONCONFIG0, > > + BD718XX_PWRBTN_PRESS_DURATION_MASK, > > + BD718XX_PWRBTN_SHORT_PRESS_10MS); > > + if (ret < 0) { > > As above. > I will check these. Thanks. > > + > > + /* > > + * According to BD71847 datasheet the HW default for long press > > + * detection is 10ms. So lets change it to 10 sec so we can actually > > + * get the short push and allow gracefull shut down > > + */ > > I don't think this comment is necessary. > > If anything it only needs to say "Configure long press to 10 seconds". > > And if you do that, please add one for short press too. Difference between short and long push is that if PMIC detects log push it will kill the power without any gracefull handling. And the chip I am supporting right now is BD71837 - which has more sane default value (10 seconds). But I am going to add support for BD71847 - which was shipped to me yesterday - and according the draft data-sheet the default on it is 10ms - which makes short push unusable. So comment is intended to point out _why_ I configure the long push here. Also someone might want to use the HW default (I guess such default was selected for some specific reason) so comment helps on removing this configuration for specific use-cases. But I guess I can reduce the comment to "Configure long press to 10 seconds" as you suggested - or do you think I should make difference between ICs BD71837 and BD71847 more visible here? > > > + ret = regmap_update_bits(bd71837->regmap, > > + BD71837_REG_PWRONCONFIG1, > > + BD718XX_PWRBTN_PRESS_DURATION_MASK, > > + BD718XX_PWRBTN_LONG_PRESS_10S); > > + > > + if (ret < 0) { > > + dev_err(&i2c->dev, "Failed to configure button long press timeout\n"); > > + goto err_out; > > + } > > + > > + btns[0].irq = regmap_irq_get_virq(bd71837->irq_data, > > + BD71837_INT_PWRBTN_S); > > + > > + if (btns[0].irq < 0) { > > + ret = btns[0].irq; > > + dev_err(&i2c->dev, "Failed to get the IRQ\n"); > > + goto err_out; > > No unwinding to be done, just return directly and save yourself an > superflouous goto. So should I replace _all_ the gotos by in-place returns as Enric suggested yesterday? As I explained yesterday, my preference is to have only one point of exit but I can change gotos to returns if needed. > > +static const struct i2c_device_id bd71837_i2c_id[] = { > > + { .name = "bd71837", }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id); > > You shouldn't need this table anymore. > > See da10c06a044b ("i2c: Make I2C ID tables non-mandatory for DT'ed devices") So if I read this correctly the > > + .of_match_table = bd71837_of_match, is now sufficient. Thanks. > > +static struct i2c_driver bd71837_i2c_driver = { > > + .driver = { > > + .name = "bd71837-mfd", > > And "MFD" isn't really a thing. It's more common to just mention the > board name, or something like 'rohm-bd7183'. I'll fix this > > +/* > > + * init early so consumer devices can complete system boot > > + */ > > This is a single line commit. Please use the correct format. > > /* Single line comment */ I'll fix this. I misinterpreted your comment yesterday. > > +MODULE_DESCRIPTION("BD71837 chip multi-function driver"); > > 'PMIC' or 'Power Management IC' rather than MFD. I'll fix this > > +MODULE_LICENSE("GPL"); > > diff --git a/include/linux/mfd/bd71837.h b/include/linux/mfd/bd71837.h > > new file mode 100644 > > index 000000000000..10b0dfe90f27 > > --- /dev/null > > +++ b/include/linux/mfd/bd71837.h > > @@ -0,0 +1,391 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > I thought these were meant to use C++ (//) comments? The checkpatch.pl did complain if I used // comment on SPDX line for header file. OTOH for c-file it required // comments and complained about /* */ - version. So I did as it suggested and used > > +/* Copyright (C) 2018 ROHM Semiconductors */ > > + > > +/* > > + * ROHM BD71837MWV header file > > + */ > > If you prefix the mentions of (BD,bd)71837 with (ROHM,rohm) then this > comment becomes redundant and you can remove it. I am sorry but I am not sure what you suggest here. Do you mean I should add ROHM or rohm to all definitions here? I think that would make definitions pretty long so I would certinly need to split more lines. Such cange would also impact already applied patches. So maybe I misinterpreted your comment? And in case I did not - can this prefixing of types - be done as a separate patch set as it impacts to regulators and clk too? (clk is not yet applied so that is easy to change though). > > +/* Even though the bit zero is not SWRESET type we still want to write zero > > + * to it when changing type. Bit zero is 'SWRESET' trigger bit and if we > > + * write 1 to it we will trigger the action. So always write 0 to it when > > + * changning SWRESET action - no matter what we read from it. > > + */ > > Please use the correct format for multi-line comments. > > Hint: The first line should be empty. I'll fix this > > +#define BD71837_SWRESET_TYPE_MASK 7 > > +#define BD71837_SWRESET_TYPE_DISABLED 0 > > +#define BD71837_SWRESET_TYPE_COLD 4 > > +#define BD71837_SWRESET_TYPE_WARM 6 > > + > > +#define BD71837_SWRESET_RESET_MASK 1 > > +#define BD71837_SWRESET_RESET 1 > > + > > +/* Poweroff state transition conditions */ > > + > > +#define BD718XX_ON_REQ_POWEROFF_MASK 1 > > +#define BD718XX_SWRESET_POWEROFF_MASK 2 > > +#define BD718XX_WDOG_POWEROFF_MASK 4 > > +#define BD718XX_KEY_L_POWEROFF_MASK 8 > > + > > +#define BD718XX_POWOFF_TO_SNVS 0 > > +#define BD718XX_POWOFF_TO_RDY 0xF > > Please align each of the values with tabs. I'll fix this > > +#define BD718XX_PWR_TRIG_KEY_L 1 > > +#define BD718XX_PWR_TRIG_KEY_S 2 > > +#define BD718XX_PWR_TRIG_PMIC_ON 4 > > +#define BD718XX_PWR_TRIG_VSYS_UVLO 8 > > +#define BD718XX_RDY_TO_SNVS_SIFT 0 > > +#define BD718XX_SNVS_TO_RUN_SIFT 4 > > As above. I'll fix this > > + > > + > > No need for 2 '\n'. I'll fix this > > + BD718XX_PWRBTN_LONG_PRESS_15S > > +}; > > + > > + > > + > > Certainly no need for 3 '\n'. I'll fix this > > +struct bd71837; > > Why is this necessary? > I think it's not. I'll remove it. > > +struct bd71837_pmic; > > +struct bd71837_clk; > > +struct bd718xx_pwrkey; > > Where are these forward declared from? bd71837_pmic is in drivers/regulator/bd71837-regulator.c clk will be in clk subdevice patch that is not yet applied. It is pending some fixes. Pwrkey must be dropped as the power-key driver I originally wrote was replaced using gpio_keys instead. Should there be some comment explaining this? Suggestions on how to do this without referring to file names which are subject to change... Should I just say they are defined in subdevice drivers? > > +/* > > + * Board platform data may be used to initialize regulators. > > + */ > > + > > +struct bd71837_board { > > + struct regulator_init_data *init_data[BD71837_REGULATOR_CNT]; > > + int gpio_intr; > > +}; > > Where is this populated? > Currently nowhere as I use device-tree for getting the regulator/irq config. This is for architectures which do not use DT - but as I don't have one for testing I did leave the depends_on OF. If it was populated I would expect it to be done in some setup code. > > +/* > > + * bd71837 sub-driver chip access routines > > + */ > > + > > Please don't abstract APIs for no reason. > > Use the regmap_* API directly instead. > Yes. This was already pointed out by Stephen Boyd. But again, as part of the patches (reguators) were already applied using the wrappers - I asked if I can remove these in separate patch set after getting this initial version out. > > +#endif /* __LINUX_MFD_BD71837_H__ */ > > -- > Lee Jones [李琼斯] > Linaro Services Technical Lead > Linaro.org │ Open source software for ARM SoCs > Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 05 Jul 2018, Matti Vaittinen wrote: > Thanks for taking the time to check this =) > > On Thu, Jul 05, 2018 at 10:18:13AM +0100, Lee Jones wrote: > > On Thu, 05 Jul 2018, Matti Vaittinen wrote: > > > > > --- a/drivers/mfd/Kconfig > > > +++ b/drivers/mfd/Kconfig > > > @@ -1787,6 +1787,19 @@ config MFD_STW481X > > > in various ST Microelectronics and ST-Ericsson embedded > > > Nomadik series. > > > > > > +config MFD_BD71837 > > > > It's be nice if you prefix this with the manufacturer. Rohm? > > Like MFD_ROHM_BD71837, right? How important this is? The already applied > regulator part has depends_on for MFD_BD71837 in Kconfig so changing > this will require change in regulator tree too. I guess it's doable if > this is important though =) How can it depend on something that doesn't exist. ;) It's better, then similar devices can be grouped. [...] > > > new file mode 100644 > > > index 000000000000..677097bcea17 > > > --- /dev/null > > > +++ b/drivers/mfd/bd71837.c > > > @@ -0,0 +1,215 @@ > > > +// SPDX-License-Identifier: GPL-2.0-or-later > > > > '\n' here please. > > > +// Copyright (C) 2018 ROHM Semiconductors > > > > '\n' here please. > > As an empty line after licence line? Ok. Just '//', yes. > > > +#include <linux/i2c.h> > > > +#include <linux/input.h> > > > +#include <linux/interrupt.h> > > > +#include <linux/mfd/bd71837.h> > > > +#include <linux/mfd/core.h> > > > +#include <linux/module.h> > > > +#include <linux/regmap.h> > > > +#include <linux/gpio_keys.h> > > > > Alphabetical please. > > > > There is a problem with gpio_key.h - it uses bool but does not include > the header defining this type. So I get comple error when compiling this > using my GCC arm cross-compiler. I've submitted a patch to fix this but > as it is not yet accepted. > https://lore.kernel.org/lkml/20180627073458.GA27800@localhost.localdomain/ > Hence the gpio_keys.h is last one. If the ordering is important, separate it from the pack and place a little comment in to say why you've done so. That way no well meaning contributor will attempt to tidy it up in the future. If this will be fixed anyway, perhaps the point is moot? > > > +static struct gpio_keys_button btns[] = { > > > + { > > > + .code = KEY_POWER, > > > + .gpio = -1, > > > + .type = EV_KEY, > > > + }, > > > +}; > > > > Does this need to be an array? > > https://elixir.bootlin.com/linux/v4.17-rc1/source/include/linux/gpio_keys.h#L39 > gpio_keys_platform_data documentation states the buttons to be a pointer > to an array. Thus I did an array. Do you think I should change this to > single struct? I can't think of any reason why not. > > > +static struct gpio_keys_platform_data bd718xx_powerkey_data = { > > > + .buttons = &btns[0], > > > > Why not just "btns"? > > > > Also, I don't see any reason not to just call this 'buttons'. > > > > Or even, 'button' if there will only ever be one of them. > > I can rename this - and if I make the array to not be an array - well, > then the &foo[0] gets cleared too =) Exactly. > > > +static struct mfd_cell bd71837_mfd_cells[] = { > > > + { > > > + .name = "bd71837-clk", > > > + }, { > > > + .name = "gpio-keys", > > > + .platform_data = &bd718xx_powerkey_data, > > > + .pdata_size = sizeof(bd718xx_powerkey_data), > > > + }, { > > > + .name = "bd71837-pmic", > > > + }, > > > +}; > > > > Place the one line entries on one line, like this: > > > > { .name = "bd71837-pmic", }, > > > > ... and if ordering is not important, place them at the bottom. > > I'll fix this It's best to simply snip all the parts you agree with. [...] > > > + /* > > > + * According to BD71847 datasheet the HW default for long press > > > + * detection is 10ms. So lets change it to 10 sec so we can actually > > > + * get the short push and allow gracefull shut down > > > + */ > > > > I don't think this comment is necessary. > > > > If anything it only needs to say "Configure long press to 10 seconds". > > > > And if you do that, please add one for short press too. > Difference between short and long push is that if PMIC detects log push > it will kill the power without any gracefull handling. And the chip I am > supporting right now is BD71837 - which has more sane default value (10 > seconds). But I am going to add support for BD71847 - which was shipped > to me yesterday - and according the draft data-sheet the default on it > is 10ms - which makes short push unusable. So comment is intended to > point out _why_ I configure the long push here. Also someone might want > to use the HW default (I guess such default was selected for some > specific reason) so comment helps on removing this configuration for > specific use-cases. But I guess I can reduce the comment to "Configure > long press to 10 seconds" as you suggested - or do you think I should > make difference between ICs BD71837 and BD71847 more visible here? Honestly, I don't think it's required. If anyone were to try to change it, I would hope that they too would have read the datasheet and understand what they are doing. > > > + ret = regmap_update_bits(bd71837->regmap, > > > + BD71837_REG_PWRONCONFIG1, > > > + BD718XX_PWRBTN_PRESS_DURATION_MASK, > > > + BD718XX_PWRBTN_LONG_PRESS_10S); > > > + > > > + if (ret < 0) { > > > + dev_err(&i2c->dev, "Failed to configure button long press timeout\n"); > > > + goto err_out; > > > + } > > > + > > > + btns[0].irq = regmap_irq_get_virq(bd71837->irq_data, > > > + BD71837_INT_PWRBTN_S); > > > + > > > + if (btns[0].irq < 0) { > > > + ret = btns[0].irq; > > > + dev_err(&i2c->dev, "Failed to get the IRQ\n"); > > > + goto err_out; > > > > No unwinding to be done, just return directly and save yourself an > > superflouous goto. > > So should I replace _all_ the gotos by in-place returns as Enric suggested > yesterday? As I explained yesterday, my preference is to have only one > point of exit but I can change gotos to returns if needed. I'm not sure where the 'one point of exit' thing stems from and I can't think of any good reason for it. The main reason for using a goto in the exit path is to save lines of code and code duplication. Your method *adds* lines of code by firstly setting 'ret', then calling the goto. If nothing needs to be done before returning, just return the error value with one line. > > > +static const struct i2c_device_id bd71837_i2c_id[] = { > > > + { .name = "bd71837", }, > > > + { } > > > +}; > > > +MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id); > > > > You shouldn't need this table anymore. > > > > See da10c06a044b ("i2c: Make I2C ID tables non-mandatory for DT'ed devices") > > So if I read this correctly the > > > + .of_match_table = bd71837_of_match, > is now sufficient. Thanks. Right. > > > +++ b/include/linux/mfd/bd71837.h > > > @@ -0,0 +1,391 @@ > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > > I thought these were meant to use C++ (//) comments? > > The checkpatch.pl did complain if I used // comment on SPDX line for > header file. OTOH for c-file it required // comments and complained > about /* */ - version. So I did as it suggested and used Well that's just dandy -- who comes up with this stuff? > > > +/* Copyright (C) 2018 ROHM Semiconductors */ > > > + > > > +/* > > > + * ROHM BD71837MWV header file > > > + */ > > > > If you prefix the mentions of (BD,bd)71837 with (ROHM,rohm) then this > > comment becomes redundant and you can remove it. > > I am sorry but I am not sure what you suggest here. Do you mean I should > add ROHM or rohm to all definitions here? I think that would make > definitions pretty long so I would certinly need to split more lines. > Such cange would also impact already applied patches. So maybe I > misinterpreted your comment? And in case I did not - can this prefixing of > types - be done as a separate patch set as it impacts to regulators and > clk too? (clk is not yet applied so that is easy to change though). Any lines which are already long, you can justify as not having to do this, however I think for the filenames and function names it would be nice if they were prefixed. Filenames are particularly important. That way all of the Rohm drivers will be grouped. Unless you can be assured that all Rohm devices will be prefixed by 'bd', then the point is slightly mooted, however since 'bd' doesn't really correlate with 'rohm' it's still difficult to assume that bd* drivers are associated with Rohm -- if you catch my drift. For instance - 'wm' for Wolfson Microelectonics - 'max' for Maxim - 'intel' for, well, you know Etc. [...] > > > +struct bd71837_pmic; > > > +struct bd71837_clk; > > > +struct bd718xx_pwrkey; > > > > Where are these forward declared from? > > bd71837_pmic is in drivers/regulator/bd71837-regulator.c > clk will be in clk subdevice patch that is not yet applied. > It is pending some fixes. Pwrkey must be dropped as the power-key driver > I originally wrote was replaced using gpio_keys instead. Should there be > some comment explaining this? Suggestions on how to do this without > referring to file names which are subject to change... Should I just say > they are defined in subdevice drivers? I just wanted you to think about if they were really necessary. > > > +/* > > > + * Board platform data may be used to initialize regulators. > > > + */ > > > + > > > +struct bd71837_board { > > > + struct regulator_init_data *init_data[BD71837_REGULATOR_CNT]; > > > + int gpio_intr; > > > +}; > > > > Where is this populated? > > > Currently nowhere as I use device-tree for getting the regulator/irq > config. This is for architectures which do not use DT - but as I don't > have one for testing I did leave the depends_on OF. If it was populated > I would expect it to be done in some setup code. Please don't add code for 'what ifs'. Please remove it and add it back when you need it. > > > +/* > > > + * bd71837 sub-driver chip access routines > > > + */ > > > + > > > > Please don't abstract APIs for no reason. > > > > Use the regmap_* API directly instead. > > > > Yes. This was already pointed out by Stephen Boyd. But again, as part of > the patches (reguators) were already applied using the wrappers - I asked > if I can remove these in separate patch set after getting this initial > version out. That is one of the issues with applying related patches without each of them being reviewed first. Applying unsuitable code is not the correct thing to do, sorry.
On Thu, Jul 05, 2018 at 12:17:05PM +0100, Lee Jones wrote: > On Thu, 05 Jul 2018, Matti Vaittinen wrote: > > > Thanks for taking the time to check this =) > > > > On Thu, Jul 05, 2018 at 10:18:13AM +0100, Lee Jones wrote: > > > On Thu, 05 Jul 2018, Matti Vaittinen wrote: > > > > > > > --- a/drivers/mfd/Kconfig > > > > +++ b/drivers/mfd/Kconfig > > > > @@ -1787,6 +1787,19 @@ config MFD_STW481X > > > > in various ST Microelectronics and ST-Ericsson embedded > > > > Nomadik series. > > > > > > > > +config MFD_BD71837 > > > > > > It's be nice if you prefix this with the manufacturer. Rohm? > > > > Like MFD_ROHM_BD71837, right? How important this is? The already applied > > regulator part has depends_on for MFD_BD71837 in Kconfig so changing > > this will require change in regulator tree too. I guess it's doable if > > this is important though =) > > How can it depend on something that doesn't exist. ;) > > It's better, then similar devices can be grouped. > > > > > +#include <linux/i2c.h> > > > > +#include <linux/input.h> > > > > +#include <linux/interrupt.h> > > > > +#include <linux/mfd/bd71837.h> > > > > +#include <linux/mfd/core.h> > > > > +#include <linux/module.h> > > > > +#include <linux/regmap.h> > > > > +#include <linux/gpio_keys.h> > > > > > > Alphabetical please. > > > > > > > There is a problem with gpio_key.h - it uses bool but does not include > > the header defining this type. So I get comple error when compiling this > > using my GCC arm cross-compiler. I've submitted a patch to fix this but > > as it is not yet accepted. > > https://lore.kernel.org/lkml/20180627073458.GA27800@localhost.localdomain/ > > Hence the gpio_keys.h is last one. > > If the ordering is important, separate it from the pack and place a > little comment in to say why you've done so. That way no well meaning > contributor will attempt to tidy it up in the future. If this will be > fixed anyway, perhaps the point is moot? I have no idea if my patch to add missing include is going to be applied. I've not heard anything from it in a week or so. So I'll just do as you suggested and drop a comment on why it is left last. > > > > + /* > > > > + * According to BD71847 datasheet the HW default for long press > > > > + * detection is 10ms. So lets change it to 10 sec so we can actually > > > > + * get the short push and allow gracefull shut down > > > > + */ > > > > > > I don't think this comment is necessary. > > > > > > If anything it only needs to say "Configure long press to 10 seconds". > > > > > > And if you do that, please add one for short press too. > > > Difference between short and long push is that if PMIC detects log push > > it will kill the power without any gracefull handling. And the chip I am > > supporting right now is BD71837 - which has more sane default value (10 > > seconds). But I am going to add support for BD71847 - which was shipped > > to me yesterday - and according the draft data-sheet the default on it > > is 10ms - which makes short push unusable. So comment is intended to > > point out _why_ I configure the long push here. Also someone might want > > to use the HW default (I guess such default was selected for some > > specific reason) so comment helps on removing this configuration for > > specific use-cases. But I guess I can reduce the comment to "Configure > > long press to 10 seconds" as you suggested - or do you think I should > > make difference between ICs BD71837 and BD71847 more visible here? > > Honestly, I don't think it's required. If anyone were to try to > change it, I would hope that they too would have read the datasheet > and understand what they are doing. You still have the hope and believe in good developers? ;) Allright, I'll drop the comment. > > > > + ret = regmap_update_bits(bd71837->regmap, > > > > + BD71837_REG_PWRONCONFIG1, > > > > + BD718XX_PWRBTN_PRESS_DURATION_MASK, > > > > + BD718XX_PWRBTN_LONG_PRESS_10S); > > > > + > > > > + if (ret < 0) { > > > > + dev_err(&i2c->dev, "Failed to configure button long press timeout\n"); > > > > + goto err_out; > > > > + } > > > > + > > > > + btns[0].irq = regmap_irq_get_virq(bd71837->irq_data, > > > > + BD71837_INT_PWRBTN_S); > > > > + > > > > + if (btns[0].irq < 0) { > > > > + ret = btns[0].irq; > > > > + dev_err(&i2c->dev, "Failed to get the IRQ\n"); > > > > + goto err_out; > > > > > > No unwinding to be done, just return directly and save yourself an > > > superflouous goto. > > > > So should I replace _all_ the gotos by in-place returns as Enric suggested > > yesterday? As I explained yesterday, my preference is to have only one > > point of exit but I can change gotos to returns if needed. > > I'm not sure where the 'one point of exit' thing stems from and I > can't think of any good reason for it. The main reason for using a > goto in the exit path is to save lines of code and code duplication. > Your method *adds* lines of code by firstly setting 'ret', then > calling the goto. If nothing needs to be done before returning, just > return the error value with one line. The point stems from my personal experiences =) I've once spent almost two years cleaning/fixing up a code which left resources/locks leaking from various function exits at error branches. Back then I sweared I would always build just one clean-up sequence at the end of function and never return from the middle. So this is my personal fixation. But as I said, I will replace gotos with in place returns - and cross my fingers no one adds a (non devm) resource allocation or locking in the probe =) > > > > +++ b/include/linux/mfd/bd71837.h > > > > @@ -0,0 +1,391 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > > > > I thought these were meant to use C++ (//) comments? > > > > The checkpatch.pl did complain if I used // comment on SPDX line for > > header file. OTOH for c-file it required // comments and complained > > about /* */ - version. So I did as it suggested and used > > Well that's just dandy -- who comes up with this stuff? Engineers I bet =) > > > > +/* Copyright (C) 2018 ROHM Semiconductors */ > > > > + > > > > +/* > > > > + * ROHM BD71837MWV header file > > > > + */ > > > > > > If you prefix the mentions of (BD,bd)71837 with (ROHM,rohm) then this > > > comment becomes redundant and you can remove it. > > > > I am sorry but I am not sure what you suggest here. Do you mean I should > > add ROHM or rohm to all definitions here? I think that would make > > definitions pretty long so I would certinly need to split more lines. > > Such cange would also impact already applied patches. So maybe I > > misinterpreted your comment? And in case I did not - can this prefixing of > > types - be done as a separate patch set as it impacts to regulators and > > clk too? (clk is not yet applied so that is easy to change though). > > Any lines which are already long, you can justify as not having to do > this, however I think for the filenames and function names it would > be nice if they were prefixed. Right. For file names this should be easily doable. And when the regmap wrappers are removed there are no public functions left. And I think the name of file containing the functions is sufficient for grouping functions under "Rohm", right? > Filenames are particularly important. That way all of the Rohm > drivers will be grouped. Unless you can be assured that all Rohm > devices will be prefixed by 'bd', then the point is slightly mooted, > however since 'bd' doesn't really correlate with 'rohm' it's still > difficult to assume that bd* drivers are associated with Rohm -- if > you catch my drift. I guess I do catch it. And no, all ROHM stuff will definitely not be prefixed with bd - which is the name of the power chip - I mean power IC series. > > > > +struct bd71837_pmic; > > > > +struct bd71837_clk; > > > > +struct bd718xx_pwrkey; > > > > > > Where are these forward declared from? > > > > bd71837_pmic is in drivers/regulator/bd71837-regulator.c > > clk will be in clk subdevice patch that is not yet applied. > > It is pending some fixes. Pwrkey must be dropped as the power-key driver > > I originally wrote was replaced using gpio_keys instead. Should there be > > some comment explaining this? Suggestions on how to do this without > > referring to file names which are subject to change... Should I just say > > they are defined in subdevice drivers? > > I just wanted you to think about if they were really necessary. Right =) Well done on that=) > > > > +/* > > > > + * Board platform data may be used to initialize regulators. > > > > + */ > > > > + > > > > +struct bd71837_board { > > > > + struct regulator_init_data *init_data[BD71837_REGULATOR_CNT]; > > > > + int gpio_intr; > > > > +}; > > > > > > Where is this populated? > > > > > Currently nowhere as I use device-tree for getting the regulator/irq > > config. This is for architectures which do not use DT - but as I don't > > have one for testing I did leave the depends_on OF. If it was populated > > I would expect it to be done in some setup code. > > Please don't add code for 'what ifs'. > > Please remove it and add it back when you need it. Allright. Although this will also break the regulator part then... > > > > +/* > > > > + * bd71837 sub-driver chip access routines > > > > + */ > > > > + > > > > > > Please don't abstract APIs for no reason. > > > > > > Use the regmap_* API directly instead. > > > > > > > Yes. This was already pointed out by Stephen Boyd. But again, as part of > > the patches (reguators) were already applied using the wrappers - I asked > > if I can remove these in separate patch set after getting this initial > > version out. > > That is one of the issues with applying related patches without each > of them being reviewed first. Applying unsuitable code is not the > correct thing to do, sorry. So I assume you are not Ok with adding the wrappers and removing them with later set of patches? I'll do following workaround then: 1. Change MFD Kconfig option name => current regulator code will not be compiled even if the MFD would be applied. 2. Change MFD according to this discussion (and break the compatibility with applied regulator code) 3. Fix the regulator code with further patches to Mark 4. Fix the depends_on Kconfig option in regulator tree to match the new one suggested here. Does this sound reasonable? Best Regards Matti Vaittinen > -- > Lee Jones [李琼斯] > Linaro Services Technical Lead > Linaro.org │ Open source software for ARM SoCs > Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > > > +++ b/include/linux/mfd/bd71837.h > > > > > @@ -0,0 +1,391 @@ > > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > > > > > > I thought these were meant to use C++ (//) comments? > > > > > > The checkpatch.pl did complain if I used // comment on SPDX line for > > > header file. OTOH for c-file it required // comments and complained > > > about /* */ - version. So I did as it suggested and used > > > > Well that's just dandy -- who comes up with this stuff? > > Engineers I bet =) Ones who do not suffer from OCD, clearly! > > > > > +/* Copyright (C) 2018 ROHM Semiconductors */ > > > > > + > > > > > +/* > > > > > + * ROHM BD71837MWV header file > > > > > + */ > > > > > > > > If you prefix the mentions of (BD,bd)71837 with (ROHM,rohm) then this > > > > comment becomes redundant and you can remove it. > > > > > > I am sorry but I am not sure what you suggest here. Do you mean I should > > > add ROHM or rohm to all definitions here? I think that would make > > > definitions pretty long so I would certinly need to split more lines. > > > Such cange would also impact already applied patches. So maybe I > > > misinterpreted your comment? And in case I did not - can this prefixing of > > > types - be done as a separate patch set as it impacts to regulators and > > > clk too? (clk is not yet applied so that is easy to change though). > > > > Any lines which are already long, you can justify as not having to do > > this, however I think for the filenames and function names it would > > be nice if they were prefixed. > > Right. For file names this should be easily doable. And when the regmap > wrappers are removed there are no public functions left. And I think the > name of file containing the functions is sufficient for grouping > functions under "Rohm", right? That's fine. > > Filenames are particularly important. That way all of the Rohm > > drivers will be grouped. Unless you can be assured that all Rohm > > devices will be prefixed by 'bd', then the point is slightly mooted, > > however since 'bd' doesn't really correlate with 'rohm' it's still > > difficult to assume that bd* drivers are associated with Rohm -- if > > you catch my drift. > > I guess I do catch it. And no, all ROHM stuff will definitely not be > prefixed with bd - which is the name of the power chip > I mean power IC series. Now you're getting it! ;) > > > > > +struct bd71837_board { > > > > > + struct regulator_init_data *init_data[BD71837_REGULATOR_CNT]; > > > > > + int gpio_intr; > > > > > +}; > > > > > > > > Where is this populated? > > > > > > > Currently nowhere as I use device-tree for getting the regulator/irq > > > config. This is for architectures which do not use DT - but as I don't > > > have one for testing I did leave the depends_on OF. If it was populated > > > I would expect it to be done in some setup code. > > > > Please don't add code for 'what ifs'. > > > > Please remove it and add it back when you need it. > > Allright. Although this will also break the regulator part then... Well, it's broken anyway ... > > > > > +/* > > > > > + * bd71837 sub-driver chip access routines > > > > > + */ > > > > > + > > > > > > > > Please don't abstract APIs for no reason. > > > > > > > > Use the regmap_* API directly instead. > > > > > > > > > > Yes. This was already pointed out by Stephen Boyd. But again, as part of > > > the patches (reguators) were already applied using the wrappers - I asked > > > if I can remove these in separate patch set after getting this initial > > > version out. > > > > That is one of the issues with applying related patches without each > > of them being reviewed first. Applying unsuitable code is not the > > correct thing to do, sorry. > > So I assume you are not Ok with adding the wrappers and removing them > with later set of patches? I'll do following workaround then: No, I'm not okay with that at all. :| > 1. Change MFD Kconfig option name => current regulator code will not be > compiled even if the MFD would be applied. > 2. Change MFD according to this discussion (and break the compatibility > with applied regulator code) > 3. Fix the regulator code with further patches to Mark > 4. Fix the depends_on Kconfig option in regulator tree to match the new > one suggested here. > > Does this sound reasonable? That's how I would do it.
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index b860eb5aa194..d01a279f5e4a 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -1787,6 +1787,19 @@ config MFD_STW481X in various ST Microelectronics and ST-Ericsson embedded Nomadik series. +config MFD_BD71837 + tristate "BD71837 Power Management chip" + depends on I2C=y + depends on OF + select REGMAP_I2C + select REGMAP_IRQ + select MFD_CORE + help + Select this option to get support for the ROHM BD71837 + Power Management chips. BD71837 is designed to power processors like + NXP i.MX8. It contains 8 BUCK outputs and 7 LDOs, voltage monitoring + and emergency shut down as well as 32,768KHz clock output. + config MFD_STM32_LPTIMER tristate "Support for STM32 Low-Power Timer" depends on (ARCH_STM32 && OF) || COMPILE_TEST diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index e9fd20dba18d..09dc9eb3782c 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -227,4 +227,5 @@ obj-$(CONFIG_MFD_STM32_TIMERS) += stm32-timers.o obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o +obj-$(CONFIG_MFD_BD71837) += bd71837.o diff --git a/drivers/mfd/bd71837.c b/drivers/mfd/bd71837.c new file mode 100644 index 000000000000..677097bcea17 --- /dev/null +++ b/drivers/mfd/bd71837.c @@ -0,0 +1,215 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +// Copyright (C) 2018 ROHM Semiconductors +// bd71837.c -- ROHM BD71837MWV mfd driver +// +// Datasheet available from +// https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e + +#include <linux/i2c.h> +#include <linux/input.h> +#include <linux/interrupt.h> +#include <linux/mfd/bd71837.h> +#include <linux/mfd/core.h> +#include <linux/module.h> +#include <linux/regmap.h> +#include <linux/gpio_keys.h> + +static struct gpio_keys_button btns[] = { + { + .code = KEY_POWER, + .gpio = -1, + .type = EV_KEY, + }, +}; + +static struct gpio_keys_platform_data bd718xx_powerkey_data = { + .buttons = &btns[0], + .nbuttons = ARRAY_SIZE(btns), + .name = "bd718xx-pwrkey", +}; + +static struct mfd_cell bd71837_mfd_cells[] = { + { + .name = "bd71837-clk", + }, { + .name = "gpio-keys", + .platform_data = &bd718xx_powerkey_data, + .pdata_size = sizeof(bd718xx_powerkey_data), + }, { + .name = "bd71837-pmic", + }, +}; + +static const struct regmap_irq bd71837_irqs[] = { + REGMAP_IRQ_REG(BD71837_INT_SWRST, 0, BD71837_INT_SWRST_MASK), + REGMAP_IRQ_REG(BD71837_INT_PWRBTN_S, 0, BD71837_INT_PWRBTN_S_MASK), + REGMAP_IRQ_REG(BD71837_INT_PWRBTN_L, 0, BD71837_INT_PWRBTN_L_MASK), + REGMAP_IRQ_REG(BD71837_INT_PWRBTN, 0, BD71837_INT_PWRBTN_MASK), + REGMAP_IRQ_REG(BD71837_INT_WDOG, 0, BD71837_INT_WDOG_MASK), + REGMAP_IRQ_REG(BD71837_INT_ON_REQ, 0, BD71837_INT_ON_REQ_MASK), + REGMAP_IRQ_REG(BD71837_INT_STBY_REQ, 0, BD71837_INT_STBY_REQ_MASK), +}; + +static struct regmap_irq_chip bd71837_irq_chip = { + .name = "bd71837-irq", + .irqs = bd71837_irqs, + .num_irqs = ARRAY_SIZE(bd71837_irqs), + .num_regs = 1, + .irq_reg_stride = 1, + .status_base = BD71837_REG_IRQ, + .mask_base = BD71837_REG_MIRQ, + .ack_base = BD71837_REG_IRQ, + .init_ack_masked = true, + .mask_invert = false, +}; + +static const struct regmap_range pmic_status_range = { + .range_min = BD71837_REG_IRQ, + .range_max = BD71837_REG_POW_STATE, +}; + +static const struct regmap_access_table volatile_regs = { + .yes_ranges = &pmic_status_range, + .n_yes_ranges = 1, +}; + +static const struct regmap_config bd71837_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .volatile_table = &volatile_regs, + .max_register = BD71837_MAX_REGISTER - 1, + .cache_type = REGCACHE_RBTREE, +}; + +static const struct of_device_id bd71837_of_match[] = { + { .compatible = "rohm,bd71837", }, + { } +}; +MODULE_DEVICE_TABLE(of, bd71837_of_match); + +static int bd71837_i2c_probe(struct i2c_client *i2c, + const struct i2c_device_id *id) +{ + struct bd71837 *bd71837; + struct bd71837_board *board_info; + int ret = -EINVAL; + + bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL); + + if (!bd71837) + return -ENOMEM; + + board_info = dev_get_platdata(&i2c->dev); + + if (!board_info) + bd71837->chip_irq = i2c->irq; + else + bd71837->chip_irq = board_info->gpio_intr; + + if (!bd71837->chip_irq) { + dev_err(&i2c->dev, "No IRQ configured\n"); + goto err_out; + } + + bd71837->dev = &i2c->dev; + bd71837->i2c_client = i2c; + i2c_set_clientdata(i2c, bd71837); + + bd71837->regmap = devm_regmap_init_i2c(i2c, &bd71837_regmap_config); + if (IS_ERR(bd71837->regmap)) { + ret = PTR_ERR(bd71837->regmap); + dev_err(&i2c->dev, "regmap initialization failed\n"); + goto err_out; + } + + ret = bd71837_reg_read(bd71837, BD71837_REG_REV); + if (ret < 0) { + dev_err(&i2c->dev, "Read BD71837_REG_DEVICE failed\n"); + goto err_out; + } + + ret = devm_regmap_add_irq_chip(&i2c->dev, bd71837->regmap, + bd71837->chip_irq, IRQF_ONESHOT, 0, + &bd71837_irq_chip, &bd71837->irq_data); + if (ret < 0) { + dev_err(&i2c->dev, "Failed to add irq_chip\n"); + goto err_out; + } + + ret = regmap_update_bits(bd71837->regmap, + BD71837_REG_PWRONCONFIG0, + BD718XX_PWRBTN_PRESS_DURATION_MASK, + BD718XX_PWRBTN_SHORT_PRESS_10MS); + if (ret < 0) { + dev_err(&i2c->dev, "Failed to configure button short press timeout\n"); + goto err_out; + } + + /* + * According to BD71847 datasheet the HW default for long press + * detection is 10ms. So lets change it to 10 sec so we can actually + * get the short push and allow gracefull shut down + */ + ret = regmap_update_bits(bd71837->regmap, + BD71837_REG_PWRONCONFIG1, + BD718XX_PWRBTN_PRESS_DURATION_MASK, + BD718XX_PWRBTN_LONG_PRESS_10S); + + if (ret < 0) { + dev_err(&i2c->dev, "Failed to configure button long press timeout\n"); + goto err_out; + } + + btns[0].irq = regmap_irq_get_virq(bd71837->irq_data, + BD71837_INT_PWRBTN_S); + + if (btns[0].irq < 0) { + ret = btns[0].irq; + dev_err(&i2c->dev, "Failed to get the IRQ\n"); + goto err_out; + } + + ret = devm_mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO, + bd71837_mfd_cells, + ARRAY_SIZE(bd71837_mfd_cells), NULL, 0, + regmap_irq_get_domain(bd71837->irq_data)); + if (ret) + dev_err(&i2c->dev, "Failed to create subdevices\n"); +err_out: + + return ret; +} + +static const struct i2c_device_id bd71837_i2c_id[] = { + { .name = "bd71837", }, + { } +}; +MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id); + +static struct i2c_driver bd71837_i2c_driver = { + .driver = { + .name = "bd71837-mfd", + .of_match_table = bd71837_of_match, + }, + .probe = bd71837_i2c_probe, + .id_table = bd71837_i2c_id, +}; + +static int __init bd71837_i2c_init(void) +{ + return i2c_add_driver(&bd71837_i2c_driver); +} +/* + * init early so consumer devices can complete system boot + */ +subsys_initcall(bd71837_i2c_init); + +static void __exit bd71837_i2c_exit(void) +{ + i2c_del_driver(&bd71837_i2c_driver); +} +module_exit(bd71837_i2c_exit); + +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>"); +MODULE_DESCRIPTION("BD71837 chip multi-function driver"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/mfd/bd71837.h b/include/linux/mfd/bd71837.h new file mode 100644 index 000000000000..10b0dfe90f27 --- /dev/null +++ b/include/linux/mfd/bd71837.h @@ -0,0 +1,391 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* Copyright (C) 2018 ROHM Semiconductors */ + +/* + * ROHM BD71837MWV header file + */ + +#ifndef __LINUX_MFD_BD71837_H__ +#define __LINUX_MFD_BD71837_H__ + +#include <linux/regmap.h> + +enum { + BD71837_BUCK1 = 0, + BD71837_BUCK2, + BD71837_BUCK3, + BD71837_BUCK4, + BD71837_BUCK5, + BD71837_BUCK6, + BD71837_BUCK7, + BD71837_BUCK8, + BD71837_LDO1, + BD71837_LDO2, + BD71837_LDO3, + BD71837_LDO4, + BD71837_LDO5, + BD71837_LDO6, + BD71837_LDO7, + BD71837_REGULATOR_CNT, +}; + +#define BD71837_BUCK1_VOLTAGE_NUM 0x40 +#define BD71837_BUCK2_VOLTAGE_NUM 0x40 +#define BD71837_BUCK3_VOLTAGE_NUM 0x40 +#define BD71837_BUCK4_VOLTAGE_NUM 0x40 + +#define BD71837_BUCK5_VOLTAGE_NUM 0x08 +#define BD71837_BUCK6_VOLTAGE_NUM 0x04 +#define BD71837_BUCK7_VOLTAGE_NUM 0x08 +#define BD71837_BUCK8_VOLTAGE_NUM 0x40 + +#define BD71837_LDO1_VOLTAGE_NUM 0x04 +#define BD71837_LDO2_VOLTAGE_NUM 0x02 +#define BD71837_LDO3_VOLTAGE_NUM 0x10 +#define BD71837_LDO4_VOLTAGE_NUM 0x10 +#define BD71837_LDO5_VOLTAGE_NUM 0x10 +#define BD71837_LDO6_VOLTAGE_NUM 0x10 +#define BD71837_LDO7_VOLTAGE_NUM 0x10 + +enum { + BD71837_REG_REV = 0x00, + BD71837_REG_SWRESET = 0x01, + BD71837_REG_I2C_DEV = 0x02, + BD71837_REG_PWRCTRL0 = 0x03, + BD71837_REG_PWRCTRL1 = 0x04, + BD71837_REG_BUCK1_CTRL = 0x05, + BD71837_REG_BUCK2_CTRL = 0x06, + BD71837_REG_BUCK3_CTRL = 0x07, + BD71837_REG_BUCK4_CTRL = 0x08, + BD71837_REG_BUCK5_CTRL = 0x09, + BD71837_REG_BUCK6_CTRL = 0x0A, + BD71837_REG_BUCK7_CTRL = 0x0B, + BD71837_REG_BUCK8_CTRL = 0x0C, + BD71837_REG_BUCK1_VOLT_RUN = 0x0D, + BD71837_REG_BUCK1_VOLT_IDLE = 0x0E, + BD71837_REG_BUCK1_VOLT_SUSP = 0x0F, + BD71837_REG_BUCK2_VOLT_RUN = 0x10, + BD71837_REG_BUCK2_VOLT_IDLE = 0x11, + BD71837_REG_BUCK3_VOLT_RUN = 0x12, + BD71837_REG_BUCK4_VOLT_RUN = 0x13, + BD71837_REG_BUCK5_VOLT = 0x14, + BD71837_REG_BUCK6_VOLT = 0x15, + BD71837_REG_BUCK7_VOLT = 0x16, + BD71837_REG_BUCK8_VOLT = 0x17, + BD71837_REG_LDO1_VOLT = 0x18, + BD71837_REG_LDO2_VOLT = 0x19, + BD71837_REG_LDO3_VOLT = 0x1A, + BD71837_REG_LDO4_VOLT = 0x1B, + BD71837_REG_LDO5_VOLT = 0x1C, + BD71837_REG_LDO6_VOLT = 0x1D, + BD71837_REG_LDO7_VOLT = 0x1E, + BD71837_REG_TRANS_COND0 = 0x1F, + BD71837_REG_TRANS_COND1 = 0x20, + BD71837_REG_VRFAULTEN = 0x21, + BD71837_REG_MVRFLTMASK0 = 0x22, + BD71837_REG_MVRFLTMASK1 = 0x23, + BD71837_REG_MVRFLTMASK2 = 0x24, + BD71837_REG_RCVCFG = 0x25, + BD71837_REG_RCVNUM = 0x26, + BD71837_REG_PWRONCONFIG0 = 0x27, + BD71837_REG_PWRONCONFIG1 = 0x28, + BD71837_REG_RESETSRC = 0x29, + BD71837_REG_MIRQ = 0x2A, + BD71837_REG_IRQ = 0x2B, + BD71837_REG_IN_MON = 0x2C, + BD71837_REG_POW_STATE = 0x2D, + BD71837_REG_OUT32K = 0x2E, + BD71837_REG_REGLOCK = 0x2F, + BD71837_REG_OTPVER = 0xFF, + BD71837_MAX_REGISTER = 0x100, +}; + +#define REGLOCK_PWRSEQ 0x1 +#define REGLOCK_VREG 0x10 + +/* Generic BUCK control masks */ +#define BD71837_BUCK_SEL 0x02 +#define BD71837_BUCK_EN 0x01 +#define BD71837_BUCK_RUN_ON 0x04 + +/* Generic LDO masks */ +#define BD71837_LDO_SEL 0x80 +#define BD71837_LDO_EN 0x40 + +/* BD71837 BUCK ramp rate CTRL reg bits */ +#define BUCK_RAMPRATE_MASK 0xC0 +#define BUCK_RAMPRATE_10P00MV 0x0 +#define BUCK_RAMPRATE_5P00MV 0x1 +#define BUCK_RAMPRATE_2P50MV 0x2 +#define BUCK_RAMPRATE_1P25MV 0x3 + +/* BD71837_REG_BUCK1_VOLT_RUN bits */ +#define BUCK1_RUN_MASK 0x3F +#define BUCK1_RUN_DEFAULT 0x14 + +/* BD71837_REG_BUCK1_VOLT_SUSP bits */ +#define BUCK1_SUSP_MASK 0x3F +#define BUCK1_SUSP_DEFAULT 0x14 + +/* BD71837_REG_BUCK1_VOLT_IDLE bits */ +#define BUCK1_IDLE_MASK 0x3F +#define BUCK1_IDLE_DEFAULT 0x14 + +/* BD71837_REG_BUCK2_VOLT_RUN bits */ +#define BUCK2_RUN_MASK 0x3F +#define BUCK2_RUN_DEFAULT 0x1E + +/* BD71837_REG_BUCK2_VOLT_IDLE bits */ +#define BUCK2_IDLE_MASK 0x3F +#define BUCK2_IDLE_DEFAULT 0x14 + +/* BD71837_REG_BUCK3_VOLT_RUN bits */ +#define BUCK3_RUN_MASK 0x3F +#define BUCK3_RUN_DEFAULT 0x1E + +/* BD71837_REG_BUCK4_VOLT_RUN bits */ +#define BUCK4_RUN_MASK 0x3F +#define BUCK4_RUN_DEFAULT 0x1E + +/* BD71837_REG_BUCK5_VOLT bits */ +#define BUCK5_MASK 0x07 +#define BUCK5_DEFAULT 0x02 + +/* BD71837_REG_BUCK6_VOLT bits */ +#define BUCK6_MASK 0x03 +#define BUCK6_DEFAULT 0x03 + +/* BD71837_REG_BUCK7_VOLT bits */ +#define BUCK7_MASK 0x07 +#define BUCK7_DEFAULT 0x03 + +/* BD71837_REG_BUCK8_VOLT bits */ +#define BUCK8_MASK 0x3F +#define BUCK8_DEFAULT 0x1E + +/* BD71837_REG_IRQ bits */ +#define IRQ_SWRST 0x40 +#define IRQ_PWRON_S 0x20 +#define IRQ_PWRON_L 0x10 +#define IRQ_PWRON 0x08 +#define IRQ_WDOG 0x04 +#define IRQ_ON_REQ 0x02 +#define IRQ_STBY_REQ 0x01 + +/* BD71837_REG_OUT32K bits */ +#define BD71837_OUT32K_EN 0x01 + +/* BD71837 gated clock rate */ +#define BD71837_CLK_RATE 32768 + +/* BD71837 irqs */ +enum { + BD71837_INT_STBY_REQ, + BD71837_INT_ON_REQ, + BD71837_INT_WDOG, + BD71837_INT_PWRBTN, + BD71837_INT_PWRBTN_L, + BD71837_INT_PWRBTN_S, + BD71837_INT_SWRST +}; + +/* BD71837 interrupt masks */ +#define BD71837_INT_SWRST_MASK 0x40 +#define BD71837_INT_PWRBTN_S_MASK 0x20 +#define BD71837_INT_PWRBTN_L_MASK 0x10 +#define BD71837_INT_PWRBTN_MASK 0x8 +#define BD71837_INT_WDOG_MASK 0x4 +#define BD71837_INT_ON_REQ_MASK 0x2 +#define BD71837_INT_STBY_REQ_MASK 0x1 + +/* BD71837_REG_LDO1_VOLT bits */ +#define LDO1_MASK 0x03 + +/* BD71837_REG_LDO1_VOLT bits */ +#define LDO2_MASK 0x20 + +/* BD71837_REG_LDO3_VOLT bits */ +#define LDO3_MASK 0x0F + +/* BD71837_REG_LDO4_VOLT bits */ +#define LDO4_MASK 0x0F + +/* BD71837_REG_LDO5_VOLT bits */ +#define LDO5_MASK 0x0F + +/* BD71837_REG_LDO6_VOLT bits */ +#define LDO6_MASK 0x0F + +/* BD71837_REG_LDO7_VOLT bits */ +#define LDO7_MASK 0x0F + +/* Register write induced reset settings */ + +/* Even though the bit zero is not SWRESET type we still want to write zero + * to it when changing type. Bit zero is 'SWRESET' trigger bit and if we + * write 1 to it we will trigger the action. So always write 0 to it when + * changning SWRESET action - no matter what we read from it. + */ +#define BD71837_SWRESET_TYPE_MASK 7 +#define BD71837_SWRESET_TYPE_DISABLED 0 +#define BD71837_SWRESET_TYPE_COLD 4 +#define BD71837_SWRESET_TYPE_WARM 6 + +#define BD71837_SWRESET_RESET_MASK 1 +#define BD71837_SWRESET_RESET 1 + +/* Poweroff state transition conditions */ + +#define BD718XX_ON_REQ_POWEROFF_MASK 1 +#define BD718XX_SWRESET_POWEROFF_MASK 2 +#define BD718XX_WDOG_POWEROFF_MASK 4 +#define BD718XX_KEY_L_POWEROFF_MASK 8 + +#define BD718XX_POWOFF_TO_SNVS 0 +#define BD718XX_POWOFF_TO_RDY 0xF + +#define BD718XX_POWOFF_TIME_MASK 0xF0 +enum { + BD718XX_POWOFF_TIME_5MS = 0, + BD718XX_POWOFF_TIME_10MS, + BD718XX_POWOFF_TIME_15MS, + BD718XX_POWOFF_TIME_20MS, + BD718XX_POWOFF_TIME_25MS, + BD718XX_POWOFF_TIME_30MS, + BD718XX_POWOFF_TIME_35MS, + BD718XX_POWOFF_TIME_40MS, + BD718XX_POWOFF_TIME_45MS, + BD718XX_POWOFF_TIME_50MS, + BD718XX_POWOFF_TIME_75MS, + BD718XX_POWOFF_TIME_100MS, + BD718XX_POWOFF_TIME_250MS, + BD718XX_POWOFF_TIME_500MS, + BD718XX_POWOFF_TIME_750MS, + BD718XX_POWOFF_TIME_1500MS +}; + +/* Poweron sequence state transition conditions */ + +#define BD718XX_RDY_TO_SNVS_MASK 0xF +#define BD718XX_SNVS_TO_RUN_MASK 0xF0 + +#define BD718XX_PWR_TRIG_KEY_L 1 +#define BD718XX_PWR_TRIG_KEY_S 2 +#define BD718XX_PWR_TRIG_PMIC_ON 4 +#define BD718XX_PWR_TRIG_VSYS_UVLO 8 +#define BD718XX_RDY_TO_SNVS_SIFT 0 +#define BD718XX_SNVS_TO_RUN_SIFT 4 + + +#define BD718XX_PWRBTN_PRESS_DURATION_MASK 0xF + +/* Timeout value for detecting short press */ + +enum { + BD718XX_PWRBTN_SHORT_PRESS_10MS = 0, + BD718XX_PWRBTN_SHORT_PRESS_500MS, + BD718XX_PWRBTN_SHORT_PRESS_1000MS, + BD718XX_PWRBTN_SHORT_PRESS_1500MS, + BD718XX_PWRBTN_SHORT_PRESS_2000MS, + BD718XX_PWRBTN_SHORT_PRESS_2500MS, + BD718XX_PWRBTN_SHORT_PRESS_3000MS, + BD718XX_PWRBTN_SHORT_PRESS_3500MS, + BD718XX_PWRBTN_SHORT_PRESS_4000MS, + BD718XX_PWRBTN_SHORT_PRESS_4500MS, + BD718XX_PWRBTN_SHORT_PRESS_5000MS, + BD718XX_PWRBTN_SHORT_PRESS_5500MS, + BD718XX_PWRBTN_SHORT_PRESS_6000MS, + BD718XX_PWRBTN_SHORT_PRESS_6500MS, + BD718XX_PWRBTN_SHORT_PRESS_7000MS, + BD718XX_PWRBTN_SHORT_PRESS_7500MS +}; + +/* Timeout value for detecting LONG press */ + +enum { + BD718XX_PWRBTN_LONG_PRESS_10MS = 0, + BD718XX_PWRBTN_LONG_PRESS_1S, + BD718XX_PWRBTN_LONG_PRESS_2S, + BD718XX_PWRBTN_LONG_PRESS_3S, + BD718XX_PWRBTN_LONG_PRESS_4S, + BD718XX_PWRBTN_LONG_PRESS_5S, + BD718XX_PWRBTN_LONG_PRESS_6S, + BD718XX_PWRBTN_LONG_PRESS_7S, + BD718XX_PWRBTN_LONG_PRESS_8S, + BD718XX_PWRBTN_LONG_PRESS_9S, + BD718XX_PWRBTN_LONG_PRESS_10S, + BD718XX_PWRBTN_LONG_PRESS_11S, + BD718XX_PWRBTN_LONG_PRESS_12S, + BD718XX_PWRBTN_LONG_PRESS_13S, + BD718XX_PWRBTN_LONG_PRESS_14S, + BD718XX_PWRBTN_LONG_PRESS_15S +}; + + + +struct bd71837; +struct bd71837_pmic; +struct bd71837_clk; +struct bd718xx_pwrkey; + +/* + * Board platform data may be used to initialize regulators. + */ + +struct bd71837_board { + struct regulator_init_data *init_data[BD71837_REGULATOR_CNT]; + int gpio_intr; +}; + +struct bd71837 { + struct device *dev; + struct i2c_client *i2c_client; + struct regmap *regmap; + unsigned long int id; + + int chip_irq; + struct regmap_irq_chip_data *irq_data; + + struct bd71837_pmic *pmic; + struct bd71837_clk *clk; + struct bd718xx_pwrkey *pwrkey; +}; + +/* + * bd71837 sub-driver chip access routines + */ + +static inline int bd71837_reg_read(struct bd71837 *bd71837, u8 reg) +{ + int r, val; + + r = regmap_read(bd71837->regmap, reg, &val); + if (r < 0) + return r; + return val; +} + +static inline int bd71837_reg_write(struct bd71837 *bd71837, u8 reg, + unsigned int val) +{ + return regmap_write(bd71837->regmap, reg, val); +} + +static inline int bd71837_set_bits(struct bd71837 *bd71837, u8 reg, u8 mask) +{ + return regmap_update_bits(bd71837->regmap, reg, mask, mask); +} + +static inline int bd71837_clear_bits(struct bd71837 *bd71837, u8 reg, + u8 mask) +{ + return regmap_update_bits(bd71837->regmap, reg, mask, 0); +} + +static inline int bd71837_update_bits(struct bd71837 *bd71837, u8 reg, + u8 mask, u8 val) +{ + return regmap_update_bits(bd71837->regmap, reg, mask, val); +} + +#endif /* __LINUX_MFD_BD71837_H__ */
ROHM BD71837 PMIC MFD driver providing interrupts and support for three subsystems: - clk - Regulators - input/power-key Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> --- drivers/mfd/Kconfig | 13 ++ drivers/mfd/Makefile | 1 + drivers/mfd/bd71837.c | 215 ++++++++++++++++++++++++ include/linux/mfd/bd71837.h | 391 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 620 insertions(+) create mode 100644 drivers/mfd/bd71837.c create mode 100644 include/linux/mfd/bd71837.h