Message ID | cover.1611037866.git.matti.vaittinen@fi.rohmeurope.com |
---|---|
Headers | show |
Series | Support ROHM BD71815 PMIC | expand |
On Tue, Jan 19, 2021 at 8:23 AM Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> wrote: > > Support GPO(s) found from ROHM BD71815 power management IC. The IC has two > GPO pins but only one is properly documented in data-sheet. The driver > exposes by default only the documented GPO. The second GPO is connected to > E5 pin and is marked as GND in data-sheet. Control for this undocumented > pin can be enabled using a special DT property. > > This driver is derived from work by Peter Yang <yanglsh@embest-tech.com> > although not so much of original is left. > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> Hi Matti, looks great, just a couple nits. > --- > Changes since v1: > - removed unneeded headers > - clarified dev/parent->dev usage > - removed forgotten #define DEBUG > > drivers/gpio/Kconfig | 10 +++ > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-bd71815.c | 171 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 182 insertions(+) > create mode 100644 drivers/gpio/gpio-bd71815.c > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index c70f46e80a3b..fd7283af858d 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -1096,6 +1096,16 @@ config GPIO_BD70528 > This driver can also be built as a module. If so, the module > will be called gpio-bd70528. > > +config GPIO_BD71815 > + tristate "ROHM BD71815 PMIC GPIO support" > + depends on MFD_ROHM_BD71828 > + help > + Support for GPO(s) on ROHM BD71815 PMIC. There are two GPOs > + available on the ROHM PMIC. > + > + This driver can also be built as a module. If so, the module > + will be called gpio-bd71815. > + > config GPIO_BD71828 > tristate "ROHM BD71828 GPIO support" > depends on MFD_ROHM_BD71828 > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > index 35e3b6026665..86bb680522a6 100644 > --- a/drivers/gpio/Makefile > +++ b/drivers/gpio/Makefile > @@ -39,6 +39,7 @@ obj-$(CONFIG_GPIO_ATH79) += gpio-ath79.o > obj-$(CONFIG_GPIO_BCM_KONA) += gpio-bcm-kona.o > obj-$(CONFIG_GPIO_BCM_XGS_IPROC) += gpio-xgs-iproc.o > obj-$(CONFIG_GPIO_BD70528) += gpio-bd70528.o > +obj-$(CONFIG_GPIO_BD71815) += gpio-bd71815.o > obj-$(CONFIG_GPIO_BD71828) += gpio-bd71828.o > obj-$(CONFIG_GPIO_BD9571MWV) += gpio-bd9571mwv.o > obj-$(CONFIG_GPIO_BRCMSTB) += gpio-brcmstb.o > diff --git a/drivers/gpio/gpio-bd71815.c b/drivers/gpio/gpio-bd71815.c > new file mode 100644 > index 000000000000..664de5f69bf1 > --- /dev/null > +++ b/drivers/gpio/gpio-bd71815.c > @@ -0,0 +1,171 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Support to GPOs on ROHM BD71815 > + */ Newline here. > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/irq.h> > +#include <linux/gpio/driver.h> > +#include <linux/platform_device.h> > +#include <linux/of.h> > +/* For the BD71815 register definitions */ > +#include <linux/mfd/rohm-bd71815.h> > + Please arrange headers alphabetically. > +struct bd71815_gpio { > + struct gpio_chip chip; > + struct device *dev; > + struct regmap *regmap; > + /* > + * Sigh. The BD71815 and BD71817 were originally designed to support two > + * GPO pins. At some point it was noticed the second GPO pin which is > + * the E5 pin located at the center of IC is hard to use on PCB (due to > + * the location). It was decided to not promote this second GPO and pin > + * is marked as GND on the data-sheet. The functionality is still there > + * though! I guess driving GPO connected to ground is a bad idea. Thus > + * we do not support it by default. OTOH - the original driver written > + * by colleagues at Embest did support controlling this second GPO. It > + * is thus possible this is used in some of the products. > + * > + * This driver does not by default support configuring this second GPO > + * but allows using it by providing the DT property > + * "rohm,enable-hidden-gpo". > + */ > + bool e5_pin_is_gpo; > +}; > + > +static int bd71815gpo_get(struct gpio_chip *chip, unsigned int offset) > +{ > + struct bd71815_gpio *bd71815 = gpiochip_get_data(chip); > + int ret = 0; > + int val; > + > + ret = regmap_read(bd71815->regmap, BD71815_REG_GPO, &val); > + if (ret) > + return ret; > + > + return (val >> offset) & 1; > +} > + > +static void bd71815gpo_set(struct gpio_chip *chip, unsigned int offset, > + int value) > +{ > + struct bd71815_gpio *bd71815 = gpiochip_get_data(chip); > + int ret, val, mask; > + > + if (!bd71815->e5_pin_is_gpo && offset) > + return; > + > + mask = BIT(offset); > + val = value ? mask : 0; Maybe use regmap_set/clear_bits() here? > + ret = regmap_update_bits(bd71815->regmap, BD71815_REG_GPO, mask, val); > + if (ret) > + dev_warn(bd71815->dev, "failed to toggle GPO\n"); > +} > + > +static int bd71815_gpio_set_config(struct gpio_chip *chip, unsigned int offset, > + unsigned long config) > +{ > + struct bd71815_gpio *bdgpio = gpiochip_get_data(chip); > + > + if (!bdgpio->e5_pin_is_gpo && offset) > + return -EOPNOTSUPP; > + > + switch (pinconf_to_config_param(config)) { > + case PIN_CONFIG_DRIVE_OPEN_DRAIN: > + return regmap_update_bits(bdgpio->regmap, > + BD71815_REG_GPO, > + BD71815_GPIO_DRIVE_MASK << offset, > + BD71815_GPIO_OPEN_DRAIN << offset); > + case PIN_CONFIG_DRIVE_PUSH_PULL: > + return regmap_update_bits(bdgpio->regmap, > + BD71815_REG_GPO, > + BD71815_GPIO_DRIVE_MASK << offset, > + BD71815_GPIO_CMOS << offset); > + default: > + break; > + } > + return -EOPNOTSUPP; > +} > + > +/* BD71815 GPIO is actually GPO */ > +static int bd71815gpo_direction_get(struct gpio_chip *gc, unsigned int offset) > +{ > + return GPIO_LINE_DIRECTION_OUT; > +} > + > +/* Template for GPIO chip */ So let's make it const? > +static struct gpio_chip bd71815gpo_chip = { > + .label = "bd71815", > + .owner = THIS_MODULE, > + .get = bd71815gpo_get, > + .get_direction = bd71815gpo_direction_get, > + .set = bd71815gpo_set, > + .set_config = bd71815_gpio_set_config, > + .can_sleep = 1, > +}; > + > +static int gpo_bd71815_probe(struct platform_device *pdev) > +{ > + int ret; > + struct bd71815_gpio *g; > + struct device *dev; > + struct device *parent; > + > + /* > + * Bind devm lifetime to this platform device => use dev for devm. > + * also the prints should originate from this device. > + */ > + dev = &pdev->dev; > + /* The device-tree and regmap come from MFD => use parent for that */ > + parent = dev->parent; > + > + g = devm_kzalloc(dev, sizeof(*g), GFP_KERNEL); > + if (!g) > + return -ENOMEM; > + > + g->e5_pin_is_gpo = of_property_read_bool(parent->of_node, > + "rohm,enable-hidden-gpo"); > + g->chip = bd71815gpo_chip; > + g->chip.base = -1; > + > + if (g->e5_pin_is_gpo) > + g->chip.ngpio = 2; > + else > + g->chip.ngpio = 1; > + > + g->chip.parent = parent; > + g->chip.of_node = parent->of_node; > + g->regmap = dev_get_regmap(parent, NULL); > + g->dev = dev; > + > + ret = devm_gpiochip_add_data(dev, &g->chip, g); > + if (ret < 0) { > + dev_err(dev, "could not register gpiochip, %d\n", ret); > + return ret; > + } > + > + return ret; > +} > +static const struct platform_device_id bd7181x_gpo_id[] = { > + { "bd71815-gpo" }, > + { }, > +}; > +MODULE_DEVICE_TABLE(platform, bd7181x_gpo_id); > + > +static struct platform_driver gpo_bd71815_driver = { > + .driver = { > + .name = "bd71815-gpo", > + .owner = THIS_MODULE, > + }, > + .probe = gpo_bd71815_probe, > + .id_table = bd7181x_gpo_id, > +}; > + > +module_platform_driver(gpo_bd71815_driver); > + > +/* Note: this hardware lives inside an I2C-based multi-function device. */ > +MODULE_ALIAS("platform:bd71815-gpo"); > + > +MODULE_AUTHOR("Peter Yang <yanglsh@embest-tech.com>"); > +MODULE_DESCRIPTION("GPO interface for BD71815"); > +MODULE_LICENSE("GPL"); > -- > 2.25.4 > Bartosz > > -- > Matti Vaittinen, Linux device drivers > ROHM Semiconductors, Finland SWDC > Kiviharjunlenkki 1E > 90220 OULU > FINLAND > > ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ > Simon says - in Latin please. > ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~ > Thanks to Simon Glass for the translation =]
Hi Bartosz, On Tue, 2021-01-19 at 12:07 +0100, Bartosz Golaszewski wrote: > On Tue, Jan 19, 2021 at 8:23 AM Matti Vaittinen > <matti.vaittinen@fi.rohmeurope.com> wrote: > > Support GPO(s) found from ROHM BD71815 power management IC. The IC > > has two > > GPO pins but only one is properly documented in data-sheet. The > > driver > > exposes by default only the documented GPO. The second GPO is > > connected to > > E5 pin and is marked as GND in data-sheet. Control for this > > undocumented > > pin can be enabled using a special DT property. > > > > This driver is derived from work by Peter Yang < > > yanglsh@embest-tech.com> > > although not so much of original is left. > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > Hi Matti, > > looks great, just a couple nits. > Thanks for the review! I'll store your finsings and fix them when I respin this. I think all of your points were valid. As I know this is largish series (and as I know I accidentally sent first 10 v2 patches to all recipients no matter what subsystem was impacted) I'll wait for a while before resending (at least a week). Besides I don't expect the dependencies to be merged before next kernel release so this is not urgent :) - but thanks! Br, Matti
On 19/01/2021 09:14:45+0200, Matti Vaittinen wrote: > The ROHM BD71828 and BD71815 RTC drivers only need the regmap > pointer from parent. Regmap can be obtained via dev_get_regmap() > so do not require parent to populate driver data for that. > > BD70528 on the other hand requires parent data to access the > watchdog so leave the parent data for BD70528 here for now. > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > --- > > Please note that this same change has been sent individually: > https://lore.kernel.org/lkml/20210105152350.GA3714833@localhost.localdomain/ > It is present in this series only because some patches depend on it. > Then I think it is best to have it as part of this series. > ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ I've alway wanted to comment on that, should he have to say "I don't think" to vanish ? Because saying "I don't think so," means that he thinks this is not so. > Simon says - in Latin please. > ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~ And I guess this should be simply "non cogito" ;)
On Tue, 19 Jan 2021, Matti Vaittinen wrote: > Most ROHM PMIC sub-devices only use the regmap pointer from > parent device. They can obtain this by dev_get_regamap so in > most cases the MFD device does not need to allocate and populate > the driver data. Simplify drivers by removing this. > > The BD70528 still needs the access to watchdog mutex so keep > rohm_regmap_dev in use on BD70528 RTC and WDG drivers for now. > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > --- > > No changes since v1 > > drivers/mfd/rohm-bd718x7.c | 43 ++++++++++++-------------------- > include/linux/mfd/rohm-bd718x7.h | 13 ---------- > 2 files changed, 16 insertions(+), 40 deletions(-) For my own reference (apply this as-is to your sign-off block): Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
On Tue, 19 Jan 2021, Matti Vaittinen wrote: > Add chip ID for ROHM BD71815 and PMIC so that drivers can identify > this IC. > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > --- > No changes since v1. > > include/linux/mfd/rohm-generic.h | 1 + > 1 file changed, 1 insertion(+) For my own reference (apply this as-is to your sign-off block): Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
On Tue, 19 Jan 2021, Matti Vaittinen wrote: > Add core support for ROHM BD71815 Power Management IC. > > The IC integrates regulators, a battery charger with a coulomb counter, > a real-time clock (RTC), clock gate and general-purpose outputs (GPO). > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > --- > Changes since v1: > - Used BIT() for better readability > - removed some unused definitions > > drivers/mfd/Kconfig | 15 +- > drivers/mfd/rohm-bd71828.c | 416 +++++++++++++++++++++-- > include/linux/mfd/rohm-bd71815.h | 561 +++++++++++++++++++++++++++++++ > include/linux/mfd/rohm-bd71828.h | 3 + > 4 files changed, 952 insertions(+), 43 deletions(-) > create mode 100644 include/linux/mfd/rohm-bd71815.h > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index bdfce7b15621..59bfacb91898 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -1984,19 +1984,20 @@ config MFD_ROHM_BD70528 > charger. > > config MFD_ROHM_BD71828 > - tristate "ROHM BD71828 Power Management IC" > + tristate "ROHM BD71828 and BD71815 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 BD71828 Power > - Management IC. BD71828GW is a single-chip power management IC for > - battery-powered portable devices. The IC integrates 7 buck > - converters, 7 LDOs, and a 1500 mA single-cell linear charger. > - Also included is a Coulomb counter, a real-time clock (RTC), and > - a 32.768 kHz clock gate. > + Select this option to get support for the ROHM BD71828 and BD71815 > + Power Management ICs. BD71828GW and BD71815AGW are single-chip power > + management ICs mainly for battery-powered portable devices. > + The BD71828 integrates 7 buck converters and 7 LDOs. The BD71815 > + has 5 bucks, 7 LDOs, and a boost for driving LEDs. Both ICs provide > + also a single-cell linear charger, a Coulomb counter, a real-time > + clock (RTC), GPIOs and a 32.768 kHz clock gate. > > config MFD_STM32_LPTIMER > tristate "Support for STM32 Low-Power Timer" > diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm-bd71828.c > index 210261d026f2..28b82477ce4c 100644 > --- a/drivers/mfd/rohm-bd71828.c > +++ b/drivers/mfd/rohm-bd71828.c > @@ -2,7 +2,7 @@ > // > // Copyright (C) 2019 ROHM Semiconductors > // > -// ROHM BD71828 PMIC driver > +// ROHM BD71828/BD71815 PMIC driver > > #include <linux/gpio_keys.h> > #include <linux/i2c.h> > @@ -11,7 +11,9 @@ > #include <linux/ioport.h> > #include <linux/irq.h> > #include <linux/mfd/core.h> > +#include <linux/mfd/rohm-bd71815.h> > #include <linux/mfd/rohm-bd71828.h> > +#include <linux/mfd/rohm-generic.h> > #include <linux/module.h> > #include <linux/of_device.h> > #include <linux/regmap.h> > @@ -29,12 +31,102 @@ static struct gpio_keys_platform_data bd71828_powerkey_data = { > .name = "bd71828-pwrkey", > }; > > -static const struct resource rtc_irqs[] = { > +static const struct resource bd71815_rtc_irqs[] = { > + DEFINE_RES_IRQ_NAMED(BD71815_INT_RTC0, "bd71815-rtc-alm-0"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_RTC1, "bd71815-rtc-alm-1"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_RTC2, "bd71815-rtc-alm-2"), > +}; > + > +static const struct resource bd71828_rtc_irqs[] = { > DEFINE_RES_IRQ_NAMED(BD71828_INT_RTC0, "bd71828-rtc-alm-0"), > DEFINE_RES_IRQ_NAMED(BD71828_INT_RTC1, "bd71828-rtc-alm-1"), > DEFINE_RES_IRQ_NAMED(BD71828_INT_RTC2, "bd71828-rtc-alm-2"), > }; > > +static struct resource bd71815_power_irqs[] = { > + DEFINE_RES_IRQ_NAMED(BD71815_INT_DCIN_RMV, "bd71815-dcin-rmv"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CLPS_OUT, "bd71815-clps-out"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CLPS_IN, "bd71815-clps-in"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_DCIN_OVP_RES, "bd71815-dcin-ovp-res"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_DCIN_OVP_DET, "bd71815-dcin-ovp-det"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_DCIN_MON_RES, "bd71815-dcin-mon-res"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_DCIN_MON_DET, "bd71815-dcin-mon-det"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_VSYS_UV_RES, "bd71815-vsys-uv-res"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_VSYS_UV_DET, "bd71815-vsys-uv-det"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_VSYS_LOW_RES, "bd71815-vsys-low-res"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_VSYS_LOW_DET, "bd71815-vsys-low-det"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_VSYS_MON_RES, "bd71815-vsys-mon-res"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_VSYS_MON_RES, "bd71815-vsys-mon-det"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CHG_WDG_TEMP, "bd71815-chg-wdg-temp"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CHG_WDG_TIME, "bd71815-chg-wdg"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CHG_RECHARGE_RES, "bd71815-rechg-res"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CHG_RECHARGE_DET, "bd71815-rechg-det"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CHG_RANGED_TEMP_TRANSITION, > + "bd71815-ranged-temp-transit"), The new line limit is 100. Feel free to run these out. > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CHG_STATE_TRANSITION, > + "bd71815-chg-state-change"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_TEMP_NORMAL, > + "bd71815-bat-temp-normal"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_TEMP_ERANGE, > + "bd71815-bat-temp-erange"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_REMOVED, "bd71815-bat-rmv"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_DETECTED, "bd71815-bat-det"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_THERM_REMOVED, "bd71815-therm-rmv"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_THERM_DETECTED, "bd71815-therm-det"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_DEAD, "bd71815-bat-dead"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_SHORTC_RES, > + "bd71815-bat-short-res"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_SHORTC_DET, > + "bd71815-bat-short-det"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_LOW_VOLT_RES, > + "bd71815-bat-low-res"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_LOW_VOLT_DET, > + "bd71815-bat-low-det"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_OVER_VOLT_RES, > + "bd71815-bat-over-res"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_OVER_VOLT_DET, > + "bd71815-bat-over-det"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_MON_RES, "bd71815-bat-mon-res"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_MON_DET, "bd71815-bat-mon-det"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_CC_MON1, "bd71815-bat-cc-mon1"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_CC_MON2, "bd71815-bat-cc-mon2"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_CC_MON3, "bd71815-bat-cc-mon3"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_OVER_CURR_1_RES, > + "bd71815-bat-oc1-res"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_OVER_CURR_1_DET, > + "bd71815-bat-oc1-det"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_OVER_CURR_2_RES, > + "bd71815-bat-oc2-res"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_OVER_CURR_2_DET, > + "bd71815-bat-oc2-det"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_OVER_CURR_3_RES, > + "bd71815-bat-oc3-res"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_BAT_OVER_CURR_3_DET, > + "bd71815-bat-oc3-det"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_TEMP_BAT_LOW_RES, > + "bd71815-bat-low-res"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_TEMP_BAT_LOW_DET, > + "bd71815-bat-low-det"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_TEMP_BAT_HI_RES, "bd71815-bat-hi-res"), > + DEFINE_RES_IRQ_NAMED(BD71815_INT_TEMP_BAT_HI_DET, "bd71815-bat-hi-det"), > +}; [...] > +static const struct regmap_irq bd71815_irqs[] = { [...] > + REGMAP_IRQ_REG(BD71815_INT_TEMP_CHIP_OVER_VF_DET, 10, > + BD71815_INT_TEMP_CHIP_OVER_VF_DET_MASK), As above. > + /* RTC Alarm */ > + REGMAP_IRQ_REG(BD71815_INT_RTC0, 11, BD71815_INT_RTC0_MASK), > + REGMAP_IRQ_REG(BD71815_INT_RTC1, 11, BD71815_INT_RTC1_MASK), > + REGMAP_IRQ_REG(BD71815_INT_RTC2, 11, BD71815_INT_RTC2_MASK), > +}; [...] > +static int set_clk_mode(struct device *dev, struct regmap *regmap, > + int clkmode_reg) > +{ > + int ret; > + const char *mode; > + > + ret = of_property_read_string_index(dev->of_node, "rohm,clkout-mode", 0, > + &mode); > + if (ret) { > + if (ret == -EINVAL) > + return 0; > + return ret; > + } > + if (!strncmp(mode, "open-drain", 10)) { > + dev_dbg(dev, "configuring clk32kout as open-drain"); Do you *really* need these? > + ret = regmap_update_bits(regmap, clkmode_reg, OUT32K_MODE, > + OUT32K_MODE_OPEN_DRAIN); > + } else if (!strncmp(mode, "cmos", 4)) { > + dev_dbg(dev, "configuring clk32kout as cmos"); > + ret = regmap_update_bits(regmap, clkmode_reg, OUT32K_MODE, > + OUT32K_MODE_CMOS); > + } else { > + dev_err(dev, "bad clk32kout mode configuration"); > + return -EINVAL; > + } > + return ret; > +} > + > static int bd71828_i2c_probe(struct i2c_client *i2c) > { > - struct rohm_regmap_dev *chip; > struct regmap_irq_chip_data *irq_data; > int ret; > + struct regmap *regmap; > + const struct regmap_config *regmap_config; > + struct regmap_irq_chip *irqchip; > + unsigned int chip_type; > + struct mfd_cell *mfd; > + int cells; > + int button_irq; > + int clkmode_reg; > > if (!i2c->irq) { > dev_err(&i2c->dev, "No IRQ configured\n"); > return -EINVAL; > } > > - chip = devm_kzalloc(&i2c->dev, sizeof(*chip), GFP_KERNEL); > - if (!chip) > - return -ENOMEM; > - > - dev_set_drvdata(&i2c->dev, chip); > + chip_type = (unsigned int)(uintptr_t) > + of_device_get_match_data(&i2c->dev); > + switch (chip_type) { > + case ROHM_CHIP_TYPE_BD71828: > + mfd = bd71828_mfd_cells; > + cells = ARRAY_SIZE(bd71828_mfd_cells); > + regmap_config = &bd71828_regmap; > + irqchip = &bd71828_irq_chip; > + clkmode_reg = BD71815_REG_OUT32K; > + button_irq = BD71828_INT_SHORTPUSH; > + dev_info(&i2c->dev, "BD71828 found\n"); > + break; > + case ROHM_CHIP_TYPE_BD71815: > + mfd = bd71815_mfd_cells; > + cells = ARRAY_SIZE(bd71815_mfd_cells); > + regmap_config = &bd71815_regmap; > + irqchip = &bd71815_irq_chip; > + clkmode_reg = BD71828_REG_OUT32K; > + /* > + * If BD71817 support is needed we should be able to handle it > + * with proper DT configs + BD71815 drivers + power-button. > + * BD71815 data-sheet does not list the power-button IRQ so we > + * don't use it. > + */ > + button_irq = 0; > + dev_info(&i2c->dev, "BD71815 found\n"); Again, are these *really* useful to you and/or your users? Besides, this device not *found* i.e. scanned/read and instantiated, it has simply been matched from the local DTB. It can still be wrong. You can probably omit them. [...] > diff --git a/include/linux/mfd/rohm-bd71815.h b/include/linux/mfd/rohm-bd71815.h > new file mode 100644 > index 000000000000..8ee5874a5b73 > --- /dev/null > +++ b/include/linux/mfd/rohm-bd71815.h > @@ -0,0 +1,561 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright 2014 Embest Technology Co. Ltd. Inc. Jeeze! Is this for the Amiga? > + * Author: yanglsh@embest-tech.com > + * > + * 2020, 2021 Heavily modified by: You should probably add a proper copyright. > + * Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > + */ > + > +#ifndef _MFD_BD71815_H > +#define _MFD_BD71815_H > + > +#include <linux/regmap.h> [...]
Hello Lee, Thanks again for the review! On Mon, 2021-01-25 at 14:10 +0000, Lee Jones wrote: > On Tue, 19 Jan 2021, Matti Vaittinen wrote: > > > Add core support for ROHM BD71815 Power Management IC. > > > > The IC integrates regulators, a battery charger with a coulomb > > counter, > > a real-time clock (RTC), clock gate and general-purpose outputs > > (GPO). > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > --- > > Changes since v1: > > - Used BIT() for better readability > > - removed some unused definitions > > > > drivers/mfd/Kconfig | 15 +- > > drivers/mfd/rohm-bd71828.c | 416 +++++++++++++++++++++-- > > include/linux/mfd/rohm-bd71815.h | 561 > > +++++++++++++++++++++++++++++++ > > include/linux/mfd/rohm-bd71828.h | 3 + > > 4 files changed, 952 insertions(+), 43 deletions(-) > > create mode 100644 include/linux/mfd/rohm-bd71815.h > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > index bdfce7b15621..59bfacb91898 100644 > > --- a/drivers/mfd/Kconfig > > +++ b/drivers/mfd/Kconfig > > @@ -1984,19 +1984,20 @@ config MFD_ROHM_BD70528 > > charger. > > > > config MFD_ROHM_BD71828 > > - tristate "ROHM BD71828 Power Management IC" > > + tristate "ROHM BD71828 and BD71815 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 BD71828 Power > > - Management IC. BD71828GW is a single-chip power management IC > > for > > - battery-powered portable devices. The IC integrates 7 buck > > - converters, 7 LDOs, and a 1500 mA single-cell linear charger. > > - Also included is a Coulomb counter, a real-time clock (RTC), > > and > > - a 32.768 kHz clock gate. > > + Select this option to get support for the ROHM BD71828 and > > BD71815 > > + Power Management ICs. BD71828GW and BD71815AGW are single- > > chip power > > + management ICs mainly for battery-powered portable devices. > > + The BD71828 integrates 7 buck converters and 7 LDOs. The > > BD71815 > > + has 5 bucks, 7 LDOs, and a boost for driving LEDs. Both ICs > > provide > > + also a single-cell linear charger, a Coulomb counter, a real- > > time > > + clock (RTC), GPIOs and a 32.768 kHz clock gate. > > > > config MFD_STM32_LPTIMER > > tristate "Support for STM32 Low-Power Timer" > > diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm- > > bd71828.c > > index 210261d026f2..28b82477ce4c 100644 > > --- a/drivers/mfd/rohm-bd71828.c > > +++ b/drivers/mfd/rohm-bd71828.c > > @@ -2,7 +2,7 @@ > > // > > // Copyright (C) 2019 ROHM Semiconductors > > // > > -// ROHM BD71828 PMIC driver > > +// ROHM BD71828/BD71815 PMIC driver > > > > #include <linux/gpio_keys.h> > > #include <linux/i2c.h> > > @@ -11,7 +11,9 @@ > > #include <linux/ioport.h> > > #include <linux/irq.h> > > #include <linux/mfd/core.h> > > +#include <linux/mfd/rohm-bd71815.h> > > #include <linux/mfd/rohm-bd71828.h> > > +#include <linux/mfd/rohm-generic.h> > > #include <linux/module.h> > > #include <linux/of_device.h> > > #include <linux/regmap.h> > > @@ -29,12 +31,102 @@ static struct gpio_keys_platform_data > > bd71828_powerkey_data = { > > .name = "bd71828-pwrkey", > > }; > > > > -static const struct resource rtc_irqs[] = { > > +static const struct resource bd71815_rtc_irqs[] = { > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_RTC0, "bd71815-rtc-alm-0"), > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_RTC1, "bd71815-rtc-alm-1"), > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_RTC2, "bd71815-rtc-alm-2"), > > +}; > > + > > +static const struct resource bd71828_rtc_irqs[] = { > > DEFINE_RES_IRQ_NAMED(BD71828_INT_RTC0, "bd71828-rtc-alm-0"), > > DEFINE_RES_IRQ_NAMED(BD71828_INT_RTC1, "bd71828-rtc-alm-1"), > > DEFINE_RES_IRQ_NAMED(BD71828_INT_RTC2, "bd71828-rtc-alm-2"), > > }; > > > > +static struct resource bd71815_power_irqs[] = { > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_DCIN_RMV, "bd71815-dcin-rmv"), > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CLPS_OUT, "bd71815-clps-out"), > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CLPS_IN, "bd71815-clps-in"), > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_DCIN_OVP_RES, "bd71815-dcin- > > ovp-res"), > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_DCIN_OVP_DET, "bd71815-dcin- > > ovp-det"), > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_DCIN_MON_RES, "bd71815-dcin- > > mon-res"), > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_DCIN_MON_DET, "bd71815-dcin- > > mon-det"), > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_VSYS_UV_RES, "bd71815-vsys-uv- > > res"), > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_VSYS_UV_DET, "bd71815-vsys-uv- > > det"), > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_VSYS_LOW_RES, "bd71815-vsys- > > low-res"), > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_VSYS_LOW_DET, "bd71815-vsys- > > low-det"), > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_VSYS_MON_RES, "bd71815-vsys- > > mon-res"), > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_VSYS_MON_RES, "bd71815-vsys- > > mon-det"), > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CHG_WDG_TEMP, "bd71815-chg- > > wdg-temp"), > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CHG_WDG_TIME, "bd71815-chg- > > wdg"), > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CHG_RECHARGE_RES, "bd71815- > > rechg-res"), > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CHG_RECHARGE_DET, "bd71815- > > rechg-det"), > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CHG_RANGED_TEMP_TRANSITION, > > + "bd71815-ranged-temp-transit"), > > The new line limit is 100. Feel free to run these out. I learn new things every day it seems. This change is more than welcome! > > + if (!strncmp(mode, "open-drain", 10)) { > > + dev_dbg(dev, "configuring clk32kout as open-drain"); > > Do you *really* need these? No. development leftover. Thanks for pointing that. > > > > + button_irq = 0; > > + dev_info(&i2c->dev, "BD71815 found\n"); > > Again, are these *really* useful to you and/or your users? > > Besides, this device not *found* i.e. scanned/read and instantiated, > it has simply been matched from the local DTB. It can still be > wrong. You can probably omit them. You're right. One can check the DT contents from /proc if he wants to check what IC compatible was used. Thanks. > > [...] > > > diff --git a/include/linux/mfd/rohm-bd71815.h > > b/include/linux/mfd/rohm-bd71815.h > > new file mode 100644 > > index 000000000000..8ee5874a5b73 > > --- /dev/null > > +++ b/include/linux/mfd/rohm-bd71815.h > > @@ -0,0 +1,561 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * Copyright 2014 Embest Technology Co. Ltd. Inc. > > Jeeze! Is this for the Amiga? Nah. Long live NXP SOCs! ;) I think BD71815 was _originally_ developed for i.MX6. > > > + * Author: yanglsh@embest-tech.com > > + * > > + * 2020, 2021 Heavily modified by: > > You should probably add a proper copyright. Ok. I guess I can do so. Thanks! > > + * Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > + */ > > + > > +#ifndef _MFD_BD71815_H > > +#define _MFD_BD71815_H > > + > > +#include <linux/regmap.h> Best Regards -- Matti Vaittinen
On Mon, 25 Jan 2021, Matti Vaittinen wrote: > Hello Lee, > > Thanks again for the review! > > On Mon, 2021-01-25 at 14:10 +0000, Lee Jones wrote: > > On Tue, 19 Jan 2021, Matti Vaittinen wrote: > > > > > Add core support for ROHM BD71815 Power Management IC. > > > > > > The IC integrates regulators, a battery charger with a coulomb > > > counter, > > > a real-time clock (RTC), clock gate and general-purpose outputs > > > (GPO). > > > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > > --- > > > Changes since v1: > > > - Used BIT() for better readability > > > - removed some unused definitions > > > > > > drivers/mfd/Kconfig | 15 +- > > > drivers/mfd/rohm-bd71828.c | 416 +++++++++++++++++++++-- > > > include/linux/mfd/rohm-bd71815.h | 561 > > > +++++++++++++++++++++++++++++++ > > > include/linux/mfd/rohm-bd71828.h | 3 + > > > 4 files changed, 952 insertions(+), 43 deletions(-) > > > create mode 100644 include/linux/mfd/rohm-bd71815.h > > > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > > index bdfce7b15621..59bfacb91898 100644 > > > --- a/drivers/mfd/Kconfig > > > +++ b/drivers/mfd/Kconfig > > > @@ -1984,19 +1984,20 @@ config MFD_ROHM_BD70528 > > > charger. > > > > > > config MFD_ROHM_BD71828 > > > - tristate "ROHM BD71828 Power Management IC" > > > + tristate "ROHM BD71828 and BD71815 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 BD71828 Power > > > - Management IC. BD71828GW is a single-chip power management IC > > > for > > > - battery-powered portable devices. The IC integrates 7 buck > > > - converters, 7 LDOs, and a 1500 mA single-cell linear charger. > > > - Also included is a Coulomb counter, a real-time clock (RTC), > > > and > > > - a 32.768 kHz clock gate. > > > + Select this option to get support for the ROHM BD71828 and > > > BD71815 > > > + Power Management ICs. BD71828GW and BD71815AGW are single- > > > chip power > > > + management ICs mainly for battery-powered portable devices. > > > + The BD71828 integrates 7 buck converters and 7 LDOs. The > > > BD71815 > > > + has 5 bucks, 7 LDOs, and a boost for driving LEDs. Both ICs > > > provide > > > + also a single-cell linear charger, a Coulomb counter, a real- > > > time > > > + clock (RTC), GPIOs and a 32.768 kHz clock gate. > > > > > > config MFD_STM32_LPTIMER > > > tristate "Support for STM32 Low-Power Timer" > > > diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm- > > > bd71828.c > > > index 210261d026f2..28b82477ce4c 100644 > > > --- a/drivers/mfd/rohm-bd71828.c > > > +++ b/drivers/mfd/rohm-bd71828.c > > > @@ -2,7 +2,7 @@ > > > // > > > // Copyright (C) 2019 ROHM Semiconductors > > > // > > > -// ROHM BD71828 PMIC driver > > > +// ROHM BD71828/BD71815 PMIC driver > > > > > > #include <linux/gpio_keys.h> > > > #include <linux/i2c.h> > > > @@ -11,7 +11,9 @@ > > > #include <linux/ioport.h> > > > #include <linux/irq.h> > > > #include <linux/mfd/core.h> > > > +#include <linux/mfd/rohm-bd71815.h> > > > #include <linux/mfd/rohm-bd71828.h> > > > +#include <linux/mfd/rohm-generic.h> > > > #include <linux/module.h> > > > #include <linux/of_device.h> > > > #include <linux/regmap.h> > > > @@ -29,12 +31,102 @@ static struct gpio_keys_platform_data > > > bd71828_powerkey_data = { > > > .name = "bd71828-pwrkey", > > > }; > > > > > > -static const struct resource rtc_irqs[] = { > > > +static const struct resource bd71815_rtc_irqs[] = { > > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_RTC0, "bd71815-rtc-alm-0"), > > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_RTC1, "bd71815-rtc-alm-1"), > > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_RTC2, "bd71815-rtc-alm-2"), > > > +}; > > > + > > > +static const struct resource bd71828_rtc_irqs[] = { > > > DEFINE_RES_IRQ_NAMED(BD71828_INT_RTC0, "bd71828-rtc-alm-0"), > > > DEFINE_RES_IRQ_NAMED(BD71828_INT_RTC1, "bd71828-rtc-alm-1"), > > > DEFINE_RES_IRQ_NAMED(BD71828_INT_RTC2, "bd71828-rtc-alm-2"), > > > }; > > > > > > +static struct resource bd71815_power_irqs[] = { > > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_DCIN_RMV, "bd71815-dcin-rmv"), > > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CLPS_OUT, "bd71815-clps-out"), > > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CLPS_IN, "bd71815-clps-in"), > > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_DCIN_OVP_RES, "bd71815-dcin- > > > ovp-res"), > > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_DCIN_OVP_DET, "bd71815-dcin- > > > ovp-det"), > > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_DCIN_MON_RES, "bd71815-dcin- > > > mon-res"), > > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_DCIN_MON_DET, "bd71815-dcin- > > > mon-det"), > > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_VSYS_UV_RES, "bd71815-vsys-uv- > > > res"), > > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_VSYS_UV_DET, "bd71815-vsys-uv- > > > det"), > > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_VSYS_LOW_RES, "bd71815-vsys- > > > low-res"), > > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_VSYS_LOW_DET, "bd71815-vsys- > > > low-det"), > > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_VSYS_MON_RES, "bd71815-vsys- > > > mon-res"), > > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_VSYS_MON_RES, "bd71815-vsys- > > > mon-det"), > > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CHG_WDG_TEMP, "bd71815-chg- > > > wdg-temp"), > > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CHG_WDG_TIME, "bd71815-chg- > > > wdg"), > > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CHG_RECHARGE_RES, "bd71815- > > > rechg-res"), > > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CHG_RECHARGE_DET, "bd71815- > > > rechg-det"), > > > + DEFINE_RES_IRQ_NAMED(BD71815_INT_CHG_RANGED_TEMP_TRANSITION, > > > + "bd71815-ranged-temp-transit"), > > > > The new line limit is 100. Feel free to run these out. > > I learn new things every day it seems. This change is more than > welcome! Please see: bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column warning") ... for a more complete description.
On Tue, 2021-01-19 at 12:07 +0100, Bartosz Golaszewski wrote: > On Tue, Jan 19, 2021 at 8:23 AM Matti Vaittinen > <matti.vaittinen@fi.rohmeurope.com> wrote: > > Support GPO(s) found from ROHM BD71815 power management IC. The IC > > has two > > GPO pins but only one is properly documented in data-sheet. The > > driver > > exposes by default only the documented GPO. The second GPO is > > connected to > > E5 pin and is marked as GND in data-sheet. Control for this > > undocumented > > pin can be enabled using a special DT property. > > > > This driver is derived from work by Peter Yang < > > yanglsh@embest-tech.com> > > although not so much of original is left. > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > Hi Matti, > > looks great, just a couple nits. Hello Bartosz, I think fixed all the nits to v3. Can I translate this to an ack? (I will respin the series as I guess the regulator part may have fallen through the cracks so I'd like to add the relevant acks :] ) Best Regards Matti Vaittinen
On Tue, Mar 23, 2021 at 10:57 AM Vaittinen, Matti <Matti.Vaittinen@fi.rohmeurope.com> wrote: > > > On Tue, 2021-01-19 at 12:07 +0100, Bartosz Golaszewski wrote: > > On Tue, Jan 19, 2021 at 8:23 AM Matti Vaittinen > > <matti.vaittinen@fi.rohmeurope.com> wrote: > > > Support GPO(s) found from ROHM BD71815 power management IC. The IC > > > has two > > > GPO pins but only one is properly documented in data-sheet. The > > > driver > > > exposes by default only the documented GPO. The second GPO is > > > connected to > > > E5 pin and is marked as GND in data-sheet. Control for this > > > undocumented > > > pin can be enabled using a special DT property. > > > > > > This driver is derived from work by Peter Yang < > > > yanglsh@embest-tech.com> > > > although not so much of original is left. > > > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > > > Hi Matti, > > > > looks great, just a couple nits. > > Hello Bartosz, > > I think fixed all the nits to v3. Can I translate this to an ack? (I > will respin the series as I guess the regulator part may have fallen > through the cracks so I'd like to add the relevant acks :] ) > > Best Regards > Matti Vaittinen Yes: Acked-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>