| Message ID | 7e1888375c979accc402e9ddd7316e528b2ac52c.1529404894.git.matti.vaittinen@fi.rohmeurope.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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 > -- 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
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 -- 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
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 -- 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
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 -- 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
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 > -- 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 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! ;)
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!
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 -- 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
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.
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 -- 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 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
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 -- 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 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.
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.
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 -- 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
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__ */
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