| Message ID | 1530803657-17684-1-git-send-email-p.paillet@st.com |
|---|---|
| Headers | show |
| Series | Introduce STPMU1 PMIC Driver | expand |
On Thu, Jul 05, 2018 at 03:14:23PM +0000, Pascal PAILLET-LME wrote: > obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o > obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o > obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o > +obj-$(CONFIG_PROTECTION_CONSUMER) += protection-consumer.o > > obj-$(CONFIG_REGULATOR_88PG86X) += 88pg86x.o > obj-$(CONFIG_REGULATOR_88PM800) += 88pm800.o Looks like this got included by mistake... > --- /dev/null > +++ b/drivers/regulator/stpmu1_regulator.c > @@ -0,0 +1,919 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) STMicroelectronics 2018 - All Rights Reserved All rights reserved and GPL :) Please also make the entire comment a C++ one so the block looks more intentional. > +static unsigned int stpmu1_map_mode(unsigned int mode) > +{ > + return mode == REGULATOR_MODE_STANDBY ? > + REGULATOR_MODE_STANDBY : REGULATOR_MODE_NORMAL; > +} This looks confused - what's going on here? Normally a map_mode() operation would be translating values in DT but this translates everything that isn't standby into normal which suggests there's an error checking problem. > +static int stpmu1_ldo3_list_voltage(struct regulator_dev *rdev, > + unsigned int sel) > +{ > + struct stpmu1_regulator *regul = rdev_get_drvdata(rdev); > + > + if (sel == 0) > + return regulator_list_voltage_linear_range(rdev, 1); > + > + if (sel < 31) > + return regulator_list_voltage_linear_range(rdev, sel); > + > + if (sel == 31) > + return stpmu1_get_voltage_regmap(regul->voltage_ref_reg) / 2; > + > + return -EINVAL; > +} The only thing that's going on here that's odd and that couldn't be represented with a helper is selector 31 which looks to be some sort of divided bypass mode - can you explain what this represents in hardware terms please? > +static int stpmu1_ldo3_get_voltage(struct regulator_dev *rdev) > +{ > + int sel = regulator_get_voltage_sel_regmap(rdev); > + > + if (sel < 0) > + return -EINVAL; > + > + return stpmu1_ldo3_list_voltage(rdev, (unsigned int)sel); > +} This is just the standard core behaviour, no need for this. > +static int stpmu1_fixed_regul_get_voltage(struct regulator_dev *rdev) > +{ > + struct stpmu1_regulator *regul = rdev_get_drvdata(rdev); > + int id = rdev_get_id(rdev); > + > + /* VREF_DDR voltage is equal to Buck2/2 */ > + if (id == STPMU1_VREF_DDR) > + return stpmu1_get_voltage_regmap(regul->voltage_ref_reg) / 2; > + > + /* For all other case , rely on fixed value defined by Hw settings */ > + return regul->cfg->desc.fixed_uV; > +} It'd be better to just use a separate set of operations for the DDR regulator rather than have a conditional here, less code and makes it clearer that this one is a special case. > +static void update_regulator_constraints(int index, > + struct regulator_init_data *init_data) > +{ > + struct stpmu1_regulator_cfg *cfg = &stpmu1_regulator_cfgs[index]; > + struct regulator_desc *desc = &cfg->desc; > + > + init_data->constraints.valid_ops_mask |= > + cfg->valid_ops_mask; > + init_data->constraints.valid_modes_mask |= > + cfg->valid_modes_mask; Drivers should never be modifying their constraints, this is a no no. > + /* > + * if all constraints are not specified in DT , ensure Hw > + * constraints are met > + */ > + if (desc->n_voltages > 1) { Drivers shouldn't be doing this either. The API will not allow any modifications of hardware state without constraints so unless the bootloader is leaving things in a broken state we should be fine. > + if (!init_data->constraints.ramp_delay) > + init_data->constraints.ramp_delay = PMIC_RAMP_SLOPE_UV_PER_US; > + > + if (!init_data->constraints.enable_time) > + init_data->constraints.enable_time = PMIC_ENABLE_TIME_US; If these are hard constraints we know from the chip design they should be being set in the descriptor. The constraints are there to override if either they're board dependent or the board needs something longer than the chip default. > + /* LDO3 and VREF_DDR can use buck2 as reference voltage */ > + if (regul->regul_id == STPMU1_LDO3 || > + regul->regul_id == STPMU1_VREF_DDR) { > + if (!buck2) { > + dev_err(&pdev->dev, > + "Error in PMIC regulator settings: Buck2 is not defined prior to LDO3 or VREF_DDR regulators\n" > + ); > + return ERR_PTR(-EINVAL); > + } > + regul->voltage_ref_reg = buck2; > + } Can or do? Usually this would be hooked up in the DT (with the regulator specifying a supply name in the desc). > + np = pdev->dev.of_node; > + if (!np) { > + dev_err(&pdev->dev, "regulators node not found\n"); > + return -EINVAL; > + } > + May as well let the driver probe? > + /* Register all defined (from DT) regulators to Regulator Framework */ > + for (i = 0; i < ARRAY_SIZE(stpmu1_regulator_cfgs); i++) { > + /* Parse DT & find regulators to register */ > + init_data = stpmu1_regulators_matches[i].init_data; You should register everything that's physically there unconditionally, there's no harm in having a regulator registered that's not used and it might be useful for debugging purposes. > +static const struct of_device_id of_pmic_regulator_match[] = { > + { .compatible = "st,stpmu1-regulators" }, > + { }, > +}; This is part of a MFD for a single chip, why do we need a specific compatible string here rather than just enumerating from the base registration of the device?
On 07/05/2018 08:14 AM, Pascal PAILLET-LME wrote: > From: pascal paillet <p.paillet@st.com> > > The stpmu1 PMIC embeds a watchdog which is disabled by default. As soon > as the watchdog is started, it must be refreshed periodically otherwise > the PMIC goes off. > > Signed-off-by: pascal paillet <p.paillet@st.com> > --- > drivers/watchdog/Kconfig | 12 +++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/stpmu1_wdt.c | 177 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 190 insertions(+) > create mode 100644 drivers/watchdog/stpmu1_wdt.c > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 9af07fd..2155f4d 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -796,6 +796,18 @@ config STM32_WATCHDOG > To compile this driver as a module, choose M here: the > module will be called stm32_iwdg. > > +config STPMU1_WATCHDOG > + tristate "STPMU1 PMIC watchdog support" > + depends on MFD_STPMU1 > + select WATCHDOG_CORE > + help > + Say Y here to include watchdog support embedded into STPMU1 PMIC. > + If the watchdog timer expires, stpmu1 shut-down all its power > + supplies. > + > + To compile this driver as a module, choose M here: the > + module will be called spmu1_wdt. > + > config UNIPHIER_WATCHDOG > tristate "UniPhier watchdog support" > depends on ARCH_UNIPHIER || COMPILE_TEST > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 1d3c6b0..c9eba94 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -216,3 +216,4 @@ obj-$(CONFIG_ZIIRAVE_WATCHDOG) += ziirave_wdt.o > obj-$(CONFIG_SOFT_WATCHDOG) += softdog.o > obj-$(CONFIG_MENF21BMC_WATCHDOG) += menf21bmc_wdt.o > obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o > +obj-$(CONFIG_STPMU1_WATCHDOG) += stpmu1_wdt.o > diff --git a/drivers/watchdog/stpmu1_wdt.c b/drivers/watchdog/stpmu1_wdt.c > new file mode 100644 > index 0000000..57e0afa > --- /dev/null > +++ b/drivers/watchdog/stpmu1_wdt.c > @@ -0,0 +1,177 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) STMicroelectronics 2018 - All Rights Reserved > + * Author: Philippe Peurichard <philippe.peurichard@st.com>, > + * Pascal Paillet <p.paillet@st.com> for STMicroelectronics. > + */ > + > +#include <linux/kernel.h> > +#include <linux/mfd/stpmu1.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/of.h> > +#include <linux/regmap.h> > +#include <linux/reboot.h> > +#include <linux/slab.h> > +#include <linux/watchdog.h> > + > +/* WATCHDOG CONTROL REGISTER bit */ > +#define WDT_START BIT(0) > +#define WDT_PING BIT(1) > +#define WDT_START_MASK BIT(0) > +#define WDT_PING_MASK BIT(1) > + > +#define PMIC_WDT_MIN_TIMEOUT 1 > +#define PMIC_WDT_MAX_TIMEOUT 256 > + > +struct stpmu1_wdt { > + struct stpmu1_dev *pmic; > + struct watchdog_device wdtdev; > + struct notifier_block restart_handler; > +}; > + > +static int pmic_wdt_start(struct watchdog_device *wdd) > +{ > + struct stpmu1_wdt *wdt = watchdog_get_drvdata(wdd); > + > + return regmap_update_bits(wdt->pmic->regmap, > + WCHDG_CR, WDT_START_MASK, WDT_START); > +} > + > +static int pmic_wdt_stop(struct watchdog_device *wdd) > +{ > + struct stpmu1_wdt *wdt = watchdog_get_drvdata(wdd); > + > + return regmap_update_bits(wdt->pmic->regmap, > + WCHDG_CR, WDT_START_MASK, ~WDT_START); > +} > + > +static int pmic_wdt_ping(struct watchdog_device *wdd) > +{ > + struct stpmu1_wdt *wdt = watchdog_get_drvdata(wdd); > + int ret; > + > + return regmap_update_bits(wdt->pmic->regmap, > + WCHDG_CR, WDT_PING_MASK, WDT_PING); > + return ret; > +} > + > +static int pmic_wdt_set_timeout(struct watchdog_device *wdd, > + unsigned int timeout) > +{ > + struct stpmu1_wdt *wdt = watchdog_get_drvdata(wdd); > + int ret; > + > + ret = regmap_write(wdt->pmic->regmap, WCHDG_TIMER_CR, timeout); > + if (ret) > + dev_err(wdt->pmic->dev, > + "Failed to set watchdog timeout (err = %d)\n", ret); > + else > + wdd->timeout = PMIC_WDT_MAX_TIMEOUT; First the requested timeout is set, then the caller is notified that the timeout was set to the maximum possible value ? That doesn't make sense. If that is really intentional, I would expect a detailed explanation, and I would expect that the value written into the chip register matches the value reported back to the user. > + > + return ret; > +} > + > +static int pmic_wdt_restart_handler(struct notifier_block *this, > + unsigned long mode, void *cmd) > +{ > + struct stpmu1_wdt *wdt = container_of(this, > + struct stpmu1_wdt, > + restart_handler); > + > + dev_info(wdt->pmic->dev, > + "PMIC Watchdog Elapsed (timeout %d), shutdown of PMIC initiated\n", > + wdt->wdtdev.timeout); > + Register a restart handler just to issue a message ? That is quite pointless. A restart handler is supposed to restart the system. Besides, the message is highly misleading; there is no reason to believe that it will be called after the watchdog expired. This function should restart the system. If it doesn't, drop it. > + return NOTIFY_DONE; > +} > + > +static const struct watchdog_info pmic_watchdog_info = { > + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING, > + .identity = "STPMU1 PMIC Watchdog", > +}; > + > +static const struct watchdog_ops pmic_watchdog_ops = { > + .owner = THIS_MODULE, > + .start = pmic_wdt_start, > + .stop = pmic_wdt_stop, > + .ping = pmic_wdt_ping, > + .set_timeout = pmic_wdt_set_timeout, > +}; > + > +static int pmic_wdt_probe(struct platform_device *pdev) > +{ > + int ret; > + struct stpmu1_dev *pmic; > + struct stpmu1_wdt *wdt; > + > + if (!pdev->dev.parent) > + return -EINVAL; > + > + pmic = dev_get_drvdata(pdev->dev.parent); > + if (!pmic) > + return -EINVAL; > + > + wdt = devm_kzalloc(&pdev->dev, sizeof(struct stpmu1_wdt), GFP_KERNEL); > + if (!wdt) > + return -ENOMEM; > + > + wdt->pmic = pmic; > + > + wdt->wdtdev.info = &pmic_watchdog_info; > + wdt->wdtdev.ops = &pmic_watchdog_ops; > + wdt->wdtdev.min_timeout = PMIC_WDT_MIN_TIMEOUT; > + wdt->wdtdev.max_timeout = PMIC_WDT_MAX_TIMEOUT; > + wdt->wdtdev.timeout = PMIC_WDT_MAX_TIMEOUT; 256 seconds default timeout ? Unusual, just making sure that this is what you want. > + > + wdt->wdtdev.status = WATCHDOG_NOWAYOUT_INIT_STATUS; > + > + watchdog_set_drvdata(&wdt->wdtdev, wdt); > + dev_set_drvdata(&pdev->dev, wdt); > + > + ret = watchdog_register_device(&wdt->wdtdev); > + if (ret) > + return ret; > + > + wdt->restart_handler.notifier_call = pmic_wdt_restart_handler; > + wdt->restart_handler.priority = 128; > + ret = register_restart_handler(&wdt->restart_handler); Why is the restart handler provided by the watchdog core not sufficient ? > + if (ret) { > + dev_err(wdt->pmic->dev, "failed to register restart handler\n"); > + return ret; > + } > + > + dev_dbg(wdt->pmic->dev, "PMIC Watchdog driver probed\n"); The only reasons not to use the devm_ function to register the watchdog are the restart handler, which 1) doesn't do anything and 2) should use the watchdog core, and this debug message. I would suggest to use the devm_ function to register the watchdog instead. > + return 0; > +} > + > +static int pmic_wdt_remove(struct platform_device *pdev) > +{ > + struct stpmu1_wdt *wdt = dev_get_drvdata(&pdev->dev); > + > + unregister_restart_handler(&wdt->restart_handler); > + watchdog_unregister_device(&wdt->wdtdev); > + > + return 0; > +} > + > +static const struct of_device_id of_pmic_wdt_match[] = { > + { .compatible = "st,stpmu1-wdt" }, > + { }, > +}; > + > +MODULE_DEVICE_TABLE(of, of_pmic_wdt_match); > + > +static struct platform_driver stpmu1_wdt_driver = { > + .probe = pmic_wdt_probe, > + .remove = pmic_wdt_remove, > + .driver = { > + .name = "stpmu1-wdt", > + .of_match_table = of_pmic_wdt_match, > + }, > +}; > +module_platform_driver(stpmu1_wdt_driver); > + > +MODULE_AUTHOR("philippe.peurichard@st.com>"); > +MODULE_DESCRIPTION("Watchdog driver for STPMU1 device"); > +MODULE_LICENSE("GPL"); > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Pascal, On Thu, Jul 05, 2018 at 03:14:24PM +0000, Pascal PAILLET-LME wrote: > From: pascal paillet <p.paillet@st.com> > > The stpmu1 pmic is able to manage an onkey button. This driver exposes > the stpmu1 onkey as an input device. It can also be configured to > shut-down the power supplies on a long key-press with an adjustable > duration. > > Signed-off-by: pascal paillet <p.paillet@st.com> > --- > drivers/input/misc/Kconfig | 11 ++ > drivers/input/misc/Makefile | 2 + > drivers/input/misc/stpmu1_onkey.c | 321 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 334 insertions(+) > create mode 100644 drivers/input/misc/stpmu1_onkey.c > > diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig > index c25606e..d020971 100644 > --- a/drivers/input/misc/Kconfig > +++ b/drivers/input/misc/Kconfig > @@ -841,4 +841,15 @@ config INPUT_RAVE_SP_PWRBUTTON > To compile this driver as a module, choose M here: the > module will be called rave-sp-pwrbutton. > > +config INPUT_STPMU1_ONKEY > + tristate "STPMU1 PMIC Onkey support" > + depends on MFD_STPMU1 > + help > + Say Y to enable support of onkey embedded into STPMU1 PMIC. onkey > + can be used to wakeup from low power modes and force a shut-down on > + long press. > + > + To compile this driver as a module, choose M here: the > + module will be called stpmu1_onkey. > + > endif > diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile > index 72cde28..cc60dc1 100644 > --- a/drivers/input/misc/Makefile > +++ b/drivers/input/misc/Makefile > @@ -70,6 +70,7 @@ obj-$(CONFIG_INPUT_SGI_BTNS) += sgi_btns.o > obj-$(CONFIG_INPUT_SIRFSOC_ONKEY) += sirfsoc-onkey.o > obj-$(CONFIG_INPUT_SOC_BUTTON_ARRAY) += soc_button_array.o > obj-$(CONFIG_INPUT_SPARCSPKR) += sparcspkr.o > +obj-$(CONFIG_INPUT_STPMU1_ONKEY) += stpmu1_onkey.o > obj-$(CONFIG_INPUT_TPS65218_PWRBUTTON) += tps65218-pwrbutton.o > obj-$(CONFIG_INPUT_TWL4030_PWRBUTTON) += twl4030-pwrbutton.o > obj-$(CONFIG_INPUT_TWL4030_VIBRA) += twl4030-vibra.o > @@ -80,3 +81,4 @@ obj-$(CONFIG_INPUT_WM831X_ON) += wm831x-on.o > obj-$(CONFIG_INPUT_XEN_KBDDEV_FRONTEND) += xen-kbdfront.o > obj-$(CONFIG_INPUT_YEALINK) += yealink.o > obj-$(CONFIG_INPUT_IDEAPAD_SLIDEBAR) += ideapad_slidebar.o > + > diff --git a/drivers/input/misc/stpmu1_onkey.c b/drivers/input/misc/stpmu1_onkey.c > new file mode 100644 > index 0000000..084a31f > --- /dev/null > +++ b/drivers/input/misc/stpmu1_onkey.c > @@ -0,0 +1,321 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) STMicroelectronics 2018 - All Rights Reserved > + * Author: Philippe Peurichard <philippe.peurichard@st.com>, > + * Pascal Paillet <p.paillet@st.com> for STMicroelectronics. > + */ > + > +#include <linux/input.h> > +#include <linux/interrupt.h> > +#include <linux/mfd/stpmu1.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +/** > + * struct stpmu1_onkey - OnKey data > + * @pmic: pointer to STPMU1 PMIC device > + * @input_dev: pointer to input device > + * @irq_falling: irq that we are hooked on to > + * @irq_rising: irq that we are hooked on to > + */ > +struct stpmu1_onkey { > + struct stpmu1_dev *pmic; > + struct input_dev *input_dev; > + int irq_falling; > + int irq_rising; > +}; > + > +/** > + * struct pmic_onkey_config - configuration of pmic PONKEYn > + * @turnoff_enabled: value to enable turnoff condition > + * @cc_flag_clear: value to clear CC flag in case of PowerOff > + * trigger by longkey press > + * @onkey_pullup_val: value of PONKEY PullUp (active or inactive) > + * @long_press_time_val: value for long press h/w shutdown event > + */ > +struct pmic_onkey_config { > + bool turnoff_enabled; > + bool cc_flag_clear; > + u8 onkey_pullup_val; > + u8 long_press_time_val; > +}; > + > +/** > + * onkey_falling_irq() - button press isr > + * @irq: irq > + * @pmic_onkey: onkey device > + * > + * Return: IRQ_HANDLED > + */ This is internal driver functions, I do not see the need for kernel-doc style comments (or any comments for this one to be honest). > +static irqreturn_t onkey_falling_irq(int irq, void *ponkey) > +{ > + struct stpmu1_onkey *onkey = ponkey; > + struct input_dev *input_dev = onkey->input_dev; > + > + input_report_key(input_dev, KEY_POWER, 1); > + pm_wakeup_event(input_dev->dev.parent, 0); > + input_sync(input_dev); > + > + dev_dbg(&input_dev->dev, "Pwr Onkey Falling Interrupt received\n"); > + > + return IRQ_HANDLED; > +} > + > +/** > + * onkey_rising_irq() - button released isr > + * @irq: irq > + * @pmic_onkey: onkey device > + * > + * Return: IRQ_HANDLED > + */ > +static irqreturn_t onkey_rising_irq(int irq, void *ponkey) > +{ > + struct stpmu1_onkey *onkey = ponkey; > + struct input_dev *input_dev = onkey->input_dev; > + > + input_report_key(input_dev, KEY_POWER, 0); > + pm_wakeup_event(input_dev->dev.parent, 0); > + input_sync(input_dev); > + > + dev_dbg(&input_dev->dev, "Pwr Onkey Rising Interrupt received\n"); > + > + return IRQ_HANDLED; > +} > + > +/** > + * stpmu1_onkey_dt_params() - device tree parameter parser > + * @pdev: platform device for the PONKEY > + * @onkey: pointer to onkey driver data > + * @config: configuration params to be filled up > + */ > +static int stpmu1_onkey_dt_params(struct platform_device *pdev, > + struct stpmu1_onkey *onkey, > + struct pmic_onkey_config *config) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np; > + u32 val; > + > + np = dev->of_node; > + if (!np) > + return -EINVAL; Is this possible? > + > + onkey->irq_falling = platform_get_irq_byname(pdev, "onkey-falling"); > + if (onkey->irq_falling < 0) { > + dev_err(dev, "failed: request IRQ onkey-falling %d", Some of your messages use \n, some don't. I'd rather they all did. > + onkey->irq_falling); > + return onkey->irq_falling; > + } > + > + onkey->irq_rising = platform_get_irq_byname(pdev, "onkey-rising"); > + if (onkey->irq_rising < 0) { > + dev_err(dev, "failed: request IRQ onkey-rising %d", > + onkey->irq_rising); > + return onkey->irq_rising; > + } > + > + if (!of_property_read_u32(np, "st,onkey-long-press-seconds", &val)) { > + if (val >= 1 && val <= 16) { > + config->long_press_time_val = (16 - val); > + } else { > + dev_warn(dev, > + "Invalid range of long key pressed timer %d (<16)\n\r", Why \n\r? > + val); > + } > + } > + if (of_get_property(np, "st,onkey-pwroff-enabled", NULL)) > + config->turnoff_enabled = true; > + > + if (of_get_property(np, "st,onkey-clear-cc-flag", NULL)) > + config->cc_flag_clear = true; > + > + if (of_get_property(np, "st,onkey-pu-inactive", NULL)) > + config->onkey_pullup_val = PONKEY_PU_ACTIVE; Even though the driver is only used in OF systems, I wonder if we should not be using generic device property API. > + > + dev_dbg(dev, "onkey-switch-off duration=%d seconds\n", > + config->long_press_time_val); > + > + return 0; > +} > + > +/** > + * stpmu1_onkey_probe() - probe > + * @pdev: platform device for the PONKEY > + * > + * Return: 0 for successful probe else appropriate error > + */ > +static int stpmu1_onkey_probe(struct platform_device *pdev) > +{ > + struct stpmu1_dev *pmic = dev_get_drvdata(pdev->dev.parent); > + struct device *dev = &pdev->dev; > + struct input_dev *input_dev; > + struct stpmu1_onkey *onkey; > + struct pmic_onkey_config config; > + unsigned int val = 0; > + int ret; Call this variable "error" please. > + > + onkey = devm_kzalloc(dev, sizeof(*onkey), GFP_KERNEL); > + if (!onkey) > + return -ENOMEM; > + > + memset(&config, 0, sizeof(struct pmic_onkey_config)); > + ret = stpmu1_onkey_dt_params(pdev, onkey, &config); > + if (ret < 0) stpmu1_onkey_dt_params() does not return positives (good) so: if (error) return error; > + goto err; > + > + input_dev = devm_input_allocate_device(dev); > + if (!input_dev) { > + dev_err(dev, "Can't allocate Pwr Onkey Input Device\n"); > + ret = -ENOMEM; > + goto err; > + } > + > + input_dev->name = "pmic_onkey"; > + input_dev->phys = "pmic_onkey/input0"; > + input_dev->dev.parent = dev; This is already set by devm_input_allocate_device(). > + > + input_set_capability(input_dev, EV_KEY, KEY_POWER); > + > + /* Setup Power Onkey Hardware parameters (long key press)*/ > + val = (config.long_press_time_val & PONKEY_TURNOFF_TIMER_MASK); > + if (config.turnoff_enabled) > + val |= PONKEY_PWR_OFF; > + if (config.cc_flag_clear) > + val |= PONKEY_CC_FLAG_CLEAR; > + ret = regmap_update_bits(pmic->regmap, PKEY_TURNOFF_CR, > + PONKEY_TURNOFF_MASK, val); > + if (ret) { > + dev_err(dev, "LONG_PRESS_KEY_UPDATE failed: %d\n", ret); > + goto err; You are using all managed resources, so "return error" directly, no need to just to error unwinding path. > + } > + > + ret = regmap_update_bits(pmic->regmap, PADS_PULL_CR, > + PONKEY_PU_ACTIVE, > + config.onkey_pullup_val); > + > + if (ret) { > + dev_err(dev, "ONKEY Pads configuration failed: %d\n", ret); > + goto err; > + } > + > + onkey->pmic = pmic; > + onkey->input_dev = input_dev; > + > + ret = devm_request_threaded_irq(dev, onkey->irq_falling, NULL, Why does this need to be threaded? The isr does not seem to be calling any APIs that may wait. > + onkey_falling_irq, > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > + dev_name(dev), onkey); > + if (ret) { > + dev_err(dev, "Can't get IRQ for Onkey Falling edge: %d\n", ret); > + goto err; > + } > + > + ret = devm_request_threaded_irq(dev, onkey->irq_rising, NULL, > + onkey_rising_irq, > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > + dev_name(dev), onkey); > + if (ret) { > + dev_err(dev, "Can't get IRQ for Onkey Rising edge: %d\n", ret); > + goto err; > + } > + > + ret = input_register_device(input_dev); > + if (ret) { > + dev_err(dev, "Can't register power button: %d\n", ret); > + goto err; > + } > + > + platform_set_drvdata(pdev, onkey); > + device_init_wakeup(dev, true); > + > + dev_dbg(dev, "PMIC Pwr Onkey driver probed\n"); I'd rather dropped this. Input core will print when registering device already. > + > +err: > + return ret; > +} > + > +/** > + * stpmu1_onkey_remove() - Cleanup on removal > + * @pdev: platform device for the button > + * > + * Return: 0 > + */ > +static int stpmu1_onkey_remove(struct platform_device *pdev) > +{ > + struct stpmu1_onkey *onkey = platform_get_drvdata(pdev); > + > + input_unregister_device(onkey->input_dev); You are using managed input device, so this call is not needed. You should be able to remove entire stpmu1_onkey_remove(). > + return 0; > +} > + > +#ifdef CONFIG_PM_SLEEP You annotated suspend/resume methods with __maybe_unused, so this guard is not needed. > + > +/** > + * pmic_onkey_suspend() - suspend handler > + * @dev: power button device > + * > + * Cancel all pending work items for the power button, setup irq for wakeup > + * > + * Return: 0 > + */ > +static int __maybe_unused stpmu1_onkey_suspend(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct stpmu1_onkey *onkey = platform_get_drvdata(pdev); > + > + if (device_may_wakeup(dev)) { > + enable_irq_wake(onkey->irq_falling); > + enable_irq_wake(onkey->irq_rising); > + } > + return 0; > +} > + > +/** > + * pmic_onkey_resume() - resume handler > + * @dev: power button device > + * > + * Just disable the wakeup capability of irq here. > + * > + * Return: 0 > + */ > +static int __maybe_unused stpmu1_onkey_resume(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct stpmu1_onkey *onkey = platform_get_drvdata(pdev); > + > + if (device_may_wakeup(dev)) { > + disable_irq_wake(onkey->irq_falling); > + disable_irq_wake(onkey->irq_rising); > + } > + return 0; > +} > + > +#endif > + > +static SIMPLE_DEV_PM_OPS(stpmu1_onkey_pm, > + stpmu1_onkey_suspend, > + stpmu1_onkey_resume); > + > +static const struct of_device_id of_stpmu1_onkey_match[] = { > + { .compatible = "st,stpmu1-onkey" }, > + { }, > +}; > + > +MODULE_DEVICE_TABLE(of, of_stpmu1_onkey_match); > + > +static struct platform_driver stpmu1_onkey_driver = { > + .probe = stpmu1_onkey_probe, > + .remove = stpmu1_onkey_remove, > + .driver = { > + .name = "stpmu1_onkey", > + .of_match_table = of_match_ptr(of_stpmu1_onkey_match), > + .pm = &stpmu1_onkey_pm, > + }, > +}; > +module_platform_driver(stpmu1_onkey_driver); > + > +MODULE_DESCRIPTION("Onkey driver for STPMU1"); > +MODULE_LICENSE("GPL"); To match your SPDX notice this should be MODULE_LICENSE("GPL v2"). > +MODULE_AUTHOR("philippe.peurichard@st.com>"); > -- > 1.9.1 Thanks.
From: pascal paillet <p.paillet@st.com> The goal of this patch-set is to propose a driver for the STPMU1 PMIC from ST Microelectronics. The STPMU1 regulators supply power to an application processor as well as to external system peripherals such as DDR, Flash memories and system devices. It also features onkey button input and an hardware watchdog. The STPMU1 is controlled via I2C. Main driver is drivers/mfd/stpmu1 that handle I2C regmap configuration and irqchip. stpmu1_regulator, stpmu1_onkey and stpmu1_wdt need stpmu1 mfd as parent. stpmu1 mfd regulator drivers maybe mandatory at boot time. *** BLURB HERE *** pascal paillet (8): dt-bindings: mfd: document stpmu1 pmic mfd: stpmu1: add stpmu1 pmic driver dt-bindings: regulator: document stpmu1 pmic regulators regulator: stpmu1: add stpmu1 regulator driver dt-bindings: input: document stpmu1 pmic onkey input: stpmu1: add stpmu1 onkey driver dt-bindings: watchdog: document stpmu1 pmic watchdog watchdog: stpmu1: add stpmu1 watchdog driver .../devicetree/bindings/input/st,stpmu1-onkey.txt | 31 + .../devicetree/bindings/mfd/st,stpmu1.txt | 138 ++++ .../bindings/regulator/st,stpmu1-regulator.txt | 72 ++ .../devicetree/bindings/watchdog/st,stpmu1-wdt.txt | 11 + drivers/input/misc/Kconfig | 11 + drivers/input/misc/Makefile | 2 + drivers/input/misc/stpmu1_onkey.c | 321 +++++++ drivers/mfd/Kconfig | 14 + drivers/mfd/Makefile | 1 + drivers/mfd/stpmu1.c | 490 +++++++++++ drivers/regulator/Kconfig | 12 + drivers/regulator/Makefile | 2 + drivers/regulator/stpmu1_regulator.c | 919 +++++++++++++++++++++ drivers/watchdog/Kconfig | 12 + drivers/watchdog/Makefile | 1 + drivers/watchdog/stpmu1_wdt.c | 177 ++++ include/dt-bindings/mfd/st,stpmu1.h | 46 ++ include/linux/mfd/stpmu1.h | 220 +++++ 18 files changed, 2480 insertions(+) create mode 100644 Documentation/devicetree/bindings/input/st,stpmu1-onkey.txt create mode 100644 Documentation/devicetree/bindings/mfd/st,stpmu1.txt create mode 100644 Documentation/devicetree/bindings/regulator/st,stpmu1-regulator.txt create mode 100644 Documentation/devicetree/bindings/watchdog/st,stpmu1-wdt.txt create mode 100644 drivers/input/misc/stpmu1_onkey.c create mode 100644 drivers/mfd/stpmu1.c create mode 100644 drivers/regulator/stpmu1_regulator.c create mode 100644 drivers/watchdog/stpmu1_wdt.c create mode 100644 include/dt-bindings/mfd/st,stpmu1.h create mode 100644 include/linux/mfd/stpmu1.h