2018-07-05 18:49:57

by Guenter Roeck

[permalink] [raw]

Subject: Re: [PATCH 8/8] watchdog: stpmu1: add stpmu1 watchdog driver

On 07/05/2018 08:14 AM, Pascal PAILLET-LME wrote:
> From: pascal paillet <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>,
> + * Pascal Paillet <[email protected]> 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("[email protected]>");
> +MODULE_DESCRIPTION("Watchdog driver for STPMU1 device");
> +MODULE_LICENSE("GPL");
>

2018-07-09 22:39:58

by Enric Balletbo Serra

[permalink] [raw]

Subject: Re: [PATCH 2/8] mfd: stpmu1: add stpmu1 pmic driver

Hi Pascal,

Thanks for the patch some comments below.

Missatge de Pascal PAILLET-LME <[email protected]> del dia dj., 5 de
jul. 2018 a les 17:17:
>
> From: pascal paillet <[email protected]>
>
> stpmu1 is a pmic from STMicroelectronics. The stpmu1 integrates 10
> regulators and 3 switches with various capabilities.
>
> Signed-off-by: pascal paillet <[email protected]>
> ---
> drivers/mfd/Kconfig | 14 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/stpmu1.c | 490 ++++++++++++++++++++++++++++++++++++
> include/dt-bindings/mfd/st,stpmu1.h | 46 ++++
> include/linux/mfd/stpmu1.h | 220 ++++++++++++++++
> 5 files changed, 771 insertions(+)
> create mode 100644 drivers/mfd/stpmu1.c
> create mode 100644 include/dt-bindings/mfd/st,stpmu1.h
> create mode 100644 include/linux/mfd/stpmu1.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b860eb5..e15140b 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1812,6 +1812,20 @@ config MFD_STM32_TIMERS
> for PWM and IIO Timer. This driver allow to share the
> registers between the others drivers.
>
> +config MFD_STPMU1
> + tristate "Support for STPMU1 PMIC"
> + depends on (I2C=y && OF)
> + select REGMAP_I2C
> + select REGMAP_IRQ
> + select MFD_CORE
> + help
> + Support for ST Microelectronics STPMU1 PMIC. Stpmu1 mfd driver is
> + the core driver for stpmu1 component that mainly handles interrupts.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called stpmu1.
> +
> +
Extra line not needed.

> menu "Multimedia Capabilities Port drivers"
> depends on ARCH_SA1100
>
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index e9fd20d..f1c4be1 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -220,6 +220,7 @@ obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI) += intel_soc_pmic_chtdc_ti.o
> obj-$(CONFIG_MFD_MT6397) += mt6397-core.o
>
> obj-$(CONFIG_MFD_ALTERA_A10SR) += altera-a10sr.o
> +obj-$(CONFIG_MFD_STPMU1) += stpmu1.o
> obj-$(CONFIG_MFD_SUN4I_GPADC) += sun4i-gpadc.o
>
> obj-$(CONFIG_MFD_STM32_LPTIMER) += stm32-lptimer.o
> diff --git a/drivers/mfd/stpmu1.c b/drivers/mfd/stpmu1.c
> new file mode 100644
> index 0000000..a284a3e
> --- /dev/null
> +++ b/drivers/mfd/stpmu1.c
> @@ -0,0 +1,490 @@
> +// SPDX-License-Identifier: GPL-2.0

There is a license mismatch between SPDX and MODULE_LICENSE. Or SPDX
identifier should be GPL-2.0-or-later or MODULE_LICENSE should be
("GPL v2")

See https://elixir.bootlin.com/linux/latest/source/include/linux/module.h#L175

> +/*
> + * Copyright (C) STMicroelectronics 2018 - All Rights Reserved
> + * Author: Philippe Peurichard <[email protected]>,
> + * Pascal Paillet <[email protected]> for STMicroelectronics.
> + */
> +
I think that Lee, like Linus, prefers the C++ style here

> +#include <linux/err.h>
That this include is not needed.

> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/stpmu1.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
ditto
> +#include <linux/pm_runtime.h>
ditto

> +#include <linux/pm_wakeirq.h>
> +#include <linux/regmap.h>
> +#include <dt-bindings/mfd/st,stpmu1.h>
> +

[snip]

> +
> +static int stpmu1_configure_from_dt(struct stpmu1_dev *pmic_dev)
> +{
> + struct device_node *np = pmic_dev->np;
> + u32 reg = 0;
You don't need to initialize reg to 0, anyway will be overwriten.

> + int ret = 0;
You don't need to initialize ret to 0, anyway will be overwritten.

> + int irq;
> +
> + irq = of_irq_get(np, 0);
> + if (irq <= 0) {
> + dev_err(pmic_dev->dev,
> + "Failed to get irq config: %d\n", irq);
This can be in one line.

> + return irq ? irq : -ENODEV;
nit: return irq ?: -ENODEV;

> + }
> + pmic_dev->irq = irq;
> +
> + irq = of_irq_get(np, 1);
> + if (irq <= 0) {
> + dev_err(pmic_dev->dev,
> + "Failed to get irq_wake config: %d\n", irq);
> + return irq ? irq : -ENODEV;
nit: return irq ?: -ENODEV;

> + }
> + pmic_dev->irq_wake = irq;
> +
> + device_init_wakeup(pmic_dev->dev, true);
> + ret = dev_pm_set_dedicated_wake_irq(pmic_dev->dev, pmic_dev->irq_wake);
> + if (ret)
> + dev_warn(pmic_dev->dev, "failed to set up wakeup irq");
> +
> + if (!of_property_read_u32(np, "st,main_control_register", &reg)) {
> + ret = regmap_update_bits(pmic_dev->regmap,
> + SWOFF_PWRCTRL_CR,
> + PWRCTRL_POLARITY_HIGH |
> + PWRCTRL_PIN_VALID |
> + RESTART_REQUEST_ENABLED,
> + reg);
> + if (ret) {
> + dev_err(pmic_dev->dev,
> + "Failed to update main control register: %d\n",
> + ret);
> + return ret;
> + }
> + }
> +
> + if (!of_property_read_u32(np, "st,pads_pull_register", &reg)) {
> + ret = regmap_update_bits(pmic_dev->regmap,
> + PADS_PULL_CR,
> + WAKEUP_DETECTOR_DISABLED |
> + PWRCTRL_PD_ACTIVE |
> + PWRCTRL_PU_ACTIVE |
> + WAKEUP_PD_ACTIVE,
> + reg);
> + if (ret) {
> + dev_err(pmic_dev->dev,
> + "Failed to update pads control register: %d\n",
> + ret);
> + return ret;
> + }
> + }
> +
> + if (!of_property_read_u32(np, "st,vin_control_register", &reg)) {
> + ret = regmap_update_bits(pmic_dev->regmap,
> + VBUS_DET_VIN_CR,
> + VINLOW_CTRL_REG_MASK,
> + reg);
> + if (ret) {
> + dev_err(pmic_dev->dev,
> + "Failed to update vin control register: %d\n",
> + ret);
> + return ret;
> + }
> + }
> +
> + if (!of_property_read_u32(np, "st,usb_control_register", &reg)) {
> + ret = regmap_update_bits(pmic_dev->regmap, BST_SW_CR,
> + BOOST_OVP_DISABLED |
> + VBUS_OTG_DETECTION_DISABLED |
> + SW_OUT_DISCHARGE |
> + VBUS_OTG_DISCHARGE |
> + OCP_LIMIT_HIGH,
> + reg);
> + if (ret) {
> + dev_err(pmic_dev->dev,
> + "Failed to update usb control register: %d\n",
> + ret);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +int stpmu1_device_init(struct stpmu1_dev *pmic_dev)
> +{
> + int ret;
> + unsigned int val;
> +
> + pmic_dev->regmap =
> + devm_regmap_init_i2c(pmic_dev->i2c, &stpmu1_regmap_config);
> +
> + if (IS_ERR(pmic_dev->regmap)) {
> + ret = PTR_ERR(pmic_dev->regmap);
You can remove this ...

> + dev_err(pmic_dev->dev, "Failed to allocate register map: %d\n",
> + ret);
> + return ret;
and just return PTR_ERR(pmic_dev->regmap);

> + }
> +
> + ret = stpmu1_configure_from_dt(pmic_dev);
> + if (ret < 0) {
Is ret >0 return valid? If not, perhaps "if (ret)" would be better.

> + dev_err(pmic_dev->dev,
> + "Unable to configure PMIC from Device Tree: %d\n", ret);
> + return ret;
> + }
> +
> + /* Read Version ID */
> + ret = regmap_read(pmic_dev->regmap, VERSION_SR, &val);
> + if (ret < 0) {
Is ret >0 return valid? If not, perhaps "if (ret)" would be better.

> + dev_err(pmic_dev->dev, "Unable to read pmic version\n");
> + return ret;
> + }
> + dev_dbg(pmic_dev->dev, "PMIC Chip Version: 0x%x\n", val);
nit: Maybe that should be dev_info instead of dev_dbg?

> +
> + /* Initialize PMIC IRQ Chip & IRQ domains associated */
> + ret = devm_regmap_add_irq_chip(pmic_dev->dev, pmic_dev->regmap,
> + pmic_dev->irq,
> + IRQF_ONESHOT | IRQF_SHARED,
> + 0, &stpmu1_regmap_irq_chip,
> + &pmic_dev->irq_data);
> + if (ret < 0) {
Is ret >0 return valid? If not, perhaps "if (ret)" would be better.

> + dev_err(pmic_dev->dev, "IRQ Chip registration failed: %d\n",
> + ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id stpmu1_dt_match[] = {
> + {.compatible = "st,stpmu1"},
> + {},
I'd rewrite this as
+ { .compatible = "st,stpmu1" },
+ { }
Space after/before brackets and no comma at the end. The sentinel
indicates the last item on structure/arrays so no need to add a comma
at the end.

> +};
> +
Remove this line

> +MODULE_DEVICE_TABLE(of, stpmu1_dt_match);
> +
> +static int stpmu1_remove(struct i2c_client *i2c)
> +{
> + struct stpmu1_dev *pmic_dev = i2c_get_clientdata(i2c);
> +
> + of_platform_depopulate(pmic_dev->dev);
> +
> + return 0;
> +}
You can remove this function, see below ...

> +
> +static int stpmu1_probe(struct i2c_client *i2c,
> + const struct i2c_device_id *id)
> +{
> + struct stpmu1_dev *pmic;
> + struct device *dev = &i2c->dev;
> + int ret = 0;
No need to initialize to 0 if ...

> +
> + pmic = devm_kzalloc(dev, sizeof(struct stpmu1_dev), GFP_KERNEL);
> + if (!pmic)
> + return -ENOMEM;
> +
> + pmic->np = dev->of_node;
> +
> + dev_set_drvdata(dev, pmic);
> + pmic->dev = dev;
> + pmic->i2c = i2c;
> +
> + ret = stpmu1_device_init(pmic);
> + if (ret < 0)
Is ret >0 return valid? If not, perhaps "if (ret)" would be better.
> + goto err;
return ret;

> +
> + ret = of_platform_populate(pmic->np, NULL, NULL, pmic->dev);
> +
ret = devm_of_platform_populate(pmic->dev);

or even better

return devm_of_platform_populate(pmic->dev);

And remove the stpmu1_remove function.

> + dev_dbg(dev, "stpmu1 driver probed\n");
That message looks redundant to me. I'd remove it.

> +err:
And you can remove this label.

> + return ret;
And this

> +}
> +
> +static const struct i2c_device_id stpmu1_id[] = {
> + {"stpmu1", 0},
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, stpmu1_id);
The above code shouldn't be needed anymore for DT-only devices. See
da10c06a044b ("i2c: Make I2C ID tables non-mandatory for DT'ed
devices")

> +
> +#ifdef CONFIG_PM_SLEEP
> +static int stpmu1_suspend(struct device *dev)
> +{
> + struct i2c_client *i2c = container_of(dev, struct i2c_client, dev);
> + struct stpmu1_dev *pmic_dev = i2c_get_clientdata(i2c);
> +
> + if (device_may_wakeup(dev))
> + enable_irq_wake(pmic_dev->irq_wake);
> +
> + disable_irq(pmic_dev->irq);
> + return 0;
> +}
> +
> +static int stpmu1_resume(struct device *dev)
> +{
> + struct i2c_client *i2c = container_of(dev, struct i2c_client, dev);
> + struct stpmu1_dev *pmic_dev = i2c_get_clientdata(i2c);
> +
> + regcache_sync(pmic_dev->regmap);
Maybe you would like to check for an error here.

> +
> + if (device_may_wakeup(dev))
> + disable_irq_wake(pmic_dev->irq_wake);
> +
> + enable_irq(pmic_dev->irq);
> + return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(stpmu1_pm, stpmu1_suspend, stpmu1_resume);
> +
> +static struct i2c_driver stpmu1_driver = {
> + .driver = {
> + .name = "stpmu1",
> + .owner = THIS_MODULE,
This is not needed, the core does it for you.

> + .pm = &stpmu1_pm,
> + .of_match_table = of_match_ptr(stpmu1_dt_match),
It is a DT-only device so no need the of_match_ptr.

> + },
> + .probe = stpmu1_probe,
> + .remove = stpmu1_remove,
Now you can remove this

> + .id_table = stpmu1_id,
And you can remove this also.

> +};
> +
> +module_i2c_driver(stpmu1_driver);
> +
> +MODULE_DESCRIPTION("STPMU1 PMIC I2C Client");
nit: PMIC I2C Client sounds weird to me, "STPMU1 PMIC driver" ? Note
that I am not english native so I could be wrong.

> +MODULE_AUTHOR("<[email protected]>");
Use "Name <email>" or just "Name"

> +MODULE_LICENSE("GPL");
As I told you there is a license mismatch with SPDX.

[snip]

Best regards,
Enric

2018-07-16 22:16:05

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH 1/8] dt-bindings: mfd: document stpmu1 pmic

On Thu, Jul 05, 2018 at 03:14:22PM +0000, Pascal PAILLET-LME wrote:
> From: pascal paillet <[email protected]>
>
> stpmu1 is a pmic from STMicroelectronics. The stpmu1 integrates 10
> regulators and 3 switches with various capabilities.
>
> Signed-off-by: pascal paillet <[email protected]>
> ---
> .../devicetree/bindings/mfd/st,stpmu1.txt | 138 +++++++++++++++++++++
> 1 file changed, 138 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/st,stpmu1.txt
>
> diff --git a/Documentation/devicetree/bindings/mfd/st,stpmu1.txt b/Documentation/devicetree/bindings/mfd/st,stpmu1.txt
> new file mode 100644
> index 0000000..53bdab4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/st,stpmu1.txt
> @@ -0,0 +1,138 @@
> +* STMicroelectronics STPMU1 Power Management IC
> +
> +Required parent device properties:
> +- compatible: "st,stpmu1"
> +- reg: the I2C slave address for the stpmu1 chip
> +- interrupts-extended: interrupt lines to use: second irq is for wakeup.
> +- #interrupt-cells: should be 2.
> +- interrupt-controller: describes the STPMU1 as an interrupt
> + controller (has its own domain). interrupt number are the following:
> + /* Interrupt Register 1 (0x50 for latch) */
> + IT_SWOUT_R=0
> + IT_SWOUT_F=1
> + IT_VBUS_OTG_R=2
> + IT_VBUS_OTG_F=3
> + IT_WAKEUP_R=4
> + IT_WAKEUP_F=5
> + IT_PONKEY_R=6
> + IT_PONKEY_F=7
> + /* Interrupt Register 2 (0x51 for latch) */
> + IT_OVP_BOOST=8
> + IT_OCP_BOOST=9
> + IT_OCP_SWOUT=10
> + IT_OCP_OTG=11
> + IT_CURLIM_BUCK4=12
> + IT_CURLIM_BUCK3=13
> + IT_CURLIM_BUCK2=14
> + IT_CURLIM_BUCK1=15
> + /* Interrupt Register 3 (0x52 for latch) */
> + IT_SHORT_SWOUT=16
> + IT_SHORT_SWOTG=17
> + IT_CURLIM_LDO6=18
> + IT_CURLIM_LDO5=19
> + IT_CURLIM_LDO4=20
> + IT_CURLIM_LDO3=21
> + IT_CURLIM_LDO2=22
> + IT_CURLIM_LDO1=23
> + /* Interrupt Register 3 (0x52 for latch) */
> + IT_SWIN_R=24
> + IT_SWIN_F=25
> + IT_RESERVED_1=26
> + IT_RESERVED_2=27
> + IT_VINLOW_R=28
> + IT_VINLOW_F=29
> + IT_TWARN_R=30
> + IT_TWARN_F=31
> +
> +Optional parent device properties:
> +- st,main_control_register:

s/_/-/

And elsewhere...

> + -bit 1: Power cycling will be performed on turn OFF condition
> + -bit 2: PWRCTRL is functional
> + -bit 3: PWRCTRL active high
> +- st,pads_pull_register:
> + -bit 1: WAKEUP pull down is not active
> + -bit 2: PWRCTRL pull up is active
> + -bit 3: PWRCTRL pull down is active
> + -bit 4: WAKEUP detector is disabled
> +- st,vin_control_register:
> + -bit 0: VINLOW monitoring is enabled
> + -bit [1...3]: VINLOW rising threshold
> + 000 VINOK_f + 50mV
> + 001 VINOK_f + 100mV
> + 010 VINOK_f + 150mV
> + 011 VINOK_f + 200mV
> + 100 VINOK_f + 250mV
> + 101 VINOK_f + 300mV
> + 110 VINOK_f + 350mV
> + 111 VINOK_f + 400mV
> + -bit [4...5]: VINLOW hyst
> + 00 100mV
> + 01 200mV
> + 10 300mV
> + 11 400mV
> + -bit 6: SW_OUT detector is disabled
> + -bit 7: SW_IN detector is enabled.
> +- st,usb_control_register:
> + -bit 3: SW_OUT current limit
> + 0: 600mA
> + 1: 1.1A
> + -bit 4: VBUS_OTG discharge is enabled
> + -bit 5: SW_OUT discharge is enabled
> + -bit 6: VBUS_OTG detection is enabled
> + -bit 7: BOOST_OVP is disabled

Just dumping register values into DT is not the greatest design.

> +
> +
> +stpmu1 consists is a varied group of sub-devices:
> +
> +Device Description
> +------ ------------
> +stpmu1-onkey : On key
> +stpmu1-regulators : Regulators
> +stpmu1-wdt : Watchdog

These should match the node name below.

> +
> +each sub-device bindings is be described in associated driver
> +documentation section.
> +
> +Example:
> +
> +pmic: stpmu1@33 {
> + compatible = "st,stpmu1";
> + reg = <0x33>;
> + interrupts = <0 2>;
> + interrupts-extended = <&intc GIC_SPI 149 IRQ_TYPE_NONE>,
> + <&exti 55 1>;
> + st,version_status = <0x10>;
> + st,main_control_register=<0x0c>;
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + onkey {
> + compatible = "st,stpmu1-onkey";
> + interrupt-parent = <&pmic>;
> + interrupts = <7 0>,<6 1>;

> + st,onkey-pwroff-enabled;
> + st,onkey-press-seconds = <10>;

IIRC, we have some standard properties for these.

> + };
> +
> + watchdog {
> + compatible = "st,stpmu1-wdt";
> + };
> +
> + regulators {
> + compatible = "st,stpmu1-regulators";
> +
> + vdd_core: regulator@0 {

unit-address without reg is not valid. regulator-buck1, etc. instead.

> + regulator-compatible = "buck1";
> + regulator-name = "vdd_core";
> + regulator-boot-on;
> + regulator-min-microvolt = <700000>;
> + regulator-max-microvolt = <1200000>;
> + };
> + vdd: regulator@1 {
> + regulator-compatible = "buck3";
> + regulator-name = "vdd";
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-boot-on;
> + regulator-pull-down;
> + };
> + };
> --
> 1.9.1

2018-07-16 22:16:48

by Rob Herring (Arm)

[permalink] [raw]

Hi,

Thanks a lot for the review ! I just have a question below regarding
populate method.

Le 07/10/2018 12:38 AM, Enric Balletbo Serra a écrit :
> Hi Pascal,
>
> Thanks for the patch some comments below.
>
> Missatge de Pascal PAILLET-LME <[email protected]> del dia dj., 5 de
> jul. 2018 a les 17:17:
>> From: pascal paillet <[email protected]>
>>
>> stpmu1 is a pmic from STMicroelectronics. The stpmu1 integrates 10
>> regulators and 3 switches with various capabilities.
>>
>> Signed-off-by: pascal paillet <[email protected]>
>> ---
>> drivers/mfd/Kconfig | 14 ++
>> drivers/mfd/Makefile | 1 +
>> drivers/mfd/stpmu1.c | 490 ++++++++++++++++++++++++++++++++++++
>> include/dt-bindings/mfd/st,stpmu1.h | 46 ++++
>> include/linux/mfd/stpmu1.h | 220 ++++++++++++++++
>> 5 files changed, 771 insertions(+)
>> create mode 100644 drivers/mfd/stpmu1.c
>> create mode 100644 include/dt-bindings/mfd/st,stpmu1.h
>> create mode 100644 include/linux/mfd/stpmu1.h
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index b860eb5..e15140b 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -1812,6 +1812,20 @@ config MFD_STM32_TIMERS
>> for PWM and IIO Timer. This driver allow to share the
>> registers between the others drivers.
>>
>> +config MFD_STPMU1
>> + tristate "Support for STPMU1 PMIC"
>> + depends on (I2C=y && OF)
>> + select REGMAP_I2C
>> + select REGMAP_IRQ
>> + select MFD_CORE
>> + help
>> + Support for ST Microelectronics STPMU1 PMIC. Stpmu1 mfd driver is
>> + the core driver for stpmu1 component that mainly handles interrupts.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called stpmu1.
>> +
>> +
> Extra line not needed.
>
>> menu "Multimedia Capabilities Port drivers"
>> depends on ARCH_SA1100
>>
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index e9fd20d..f1c4be1 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -220,6 +220,7 @@ obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI) += intel_soc_pmic_chtdc_ti.o
>> obj-$(CONFIG_MFD_MT6397) += mt6397-core.o
>>
>> obj-$(CONFIG_MFD_ALTERA_A10SR) += altera-a10sr.o
>> +obj-$(CONFIG_MFD_STPMU1) += stpmu1.o
>> obj-$(CONFIG_MFD_SUN4I_GPADC) += sun4i-gpadc.o
>>
>> obj-$(CONFIG_MFD_STM32_LPTIMER) += stm32-lptimer.o
>> diff --git a/drivers/mfd/stpmu1.c b/drivers/mfd/stpmu1.c
>> new file mode 100644
>> index 0000000..a284a3e
>> --- /dev/null
>> +++ b/drivers/mfd/stpmu1.c
>> @@ -0,0 +1,490 @@
>> +// SPDX-License-Identifier: GPL-2.0
> There is a license mismatch between SPDX and MODULE_LICENSE. Or SPDX
> identifier should be GPL-2.0-or-later or MODULE_LICENSE should be
> ("GPL v2")
>
> See https://elixir.bootlin.com/linux/latest/source/include/linux/module.h#L175
>
>> +/*
>> + * Copyright (C) STMicroelectronics 2018 - All Rights Reserved
>> + * Author: Philippe Peurichard <[email protected]>,
>> + * Pascal Paillet <[email protected]> for STMicroelectronics.
>> + */
>> +
> I think that Lee, like Linus, prefers the C++ style here
>
>> +#include <linux/err.h>
> That this include is not needed.
>
>> +#include <linux/i2c.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/stpmu1.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
> ditto
>> +#include <linux/pm_runtime.h>
> ditto
>
>> +#include <linux/pm_wakeirq.h>
>> +#include <linux/regmap.h>
>> +#include <dt-bindings/mfd/st,stpmu1.h>
>> +
> [snip]
>
>> +
>> +static int stpmu1_configure_from_dt(struct stpmu1_dev *pmic_dev)
>> +{
>> + struct device_node *np = pmic_dev->np;
>> + u32 reg = 0;
> You don't need to initialize reg to 0, anyway will be overwriten.
>
>> + int ret = 0;
> You don't need to initialize ret to 0, anyway will be overwritten.
>
>> + int irq;
>> +
>> + irq = of_irq_get(np, 0);
>> + if (irq <= 0) {
>> + dev_err(pmic_dev->dev,
>> + "Failed to get irq config: %d\n", irq);
> This can be in one line.
>
>> + return irq ? irq : -ENODEV;
> nit: return irq ?: -ENODEV;
>
>> + }
>> + pmic_dev->irq = irq;
>> +
>> + irq = of_irq_get(np, 1);
>> + if (irq <= 0) {
>> + dev_err(pmic_dev->dev,
>> + "Failed to get irq_wake config: %d\n", irq);
>> + return irq ? irq : -ENODEV;
> nit: return irq ?: -ENODEV;
>
>> + }
>> + pmic_dev->irq_wake = irq;
>> +
>> + device_init_wakeup(pmic_dev->dev, true);
>> + ret = dev_pm_set_dedicated_wake_irq(pmic_dev->dev, pmic_dev->irq_wake);
>> + if (ret)
>> + dev_warn(pmic_dev->dev, "failed to set up wakeup irq");
>> +
>> + if (!of_property_read_u32(np, "st,main_control_register", &reg)) {
>> + ret = regmap_update_bits(pmic_dev->regmap,
>> + SWOFF_PWRCTRL_CR,
>> + PWRCTRL_POLARITY_HIGH |
>> + PWRCTRL_PIN_VALID |
>> + RESTART_REQUEST_ENABLED,
>> + reg);
>> + if (ret) {
>> + dev_err(pmic_dev->dev,
>> + "Failed to update main control register: %d\n",
>> + ret);
>> + return ret;
>> + }
>> + }
>> +
>> + if (!of_property_read_u32(np, "st,pads_pull_register", &reg)) {
>> + ret = regmap_update_bits(pmic_dev->regmap,
>> + PADS_PULL_CR,
>> + WAKEUP_DETECTOR_DISABLED |
>> + PWRCTRL_PD_ACTIVE |
>> + PWRCTRL_PU_ACTIVE |
>> + WAKEUP_PD_ACTIVE,
>> + reg);
>> + if (ret) {
>> + dev_err(pmic_dev->dev,
>> + "Failed to update pads control register: %d\n",
>> + ret);
>> + return ret;
>> + }
>> + }
>> +
>> + if (!of_property_read_u32(np, "st,vin_control_register", &reg)) {
>> + ret = regmap_update_bits(pmic_dev->regmap,
>> + VBUS_DET_VIN_CR,
>> + VINLOW_CTRL_REG_MASK,
>> + reg);
>> + if (ret) {
>> + dev_err(pmic_dev->dev,
>> + "Failed to update vin control register: %d\n",
>> + ret);
>> + return ret;
>> + }
>> + }
>> +
>> + if (!of_property_read_u32(np, "st,usb_control_register", &reg)) {
>> + ret = regmap_update_bits(pmic_dev->regmap, BST_SW_CR,
>> + BOOST_OVP_DISABLED |
>> + VBUS_OTG_DETECTION_DISABLED |
>> + SW_OUT_DISCHARGE |
>> + VBUS_OTG_DISCHARGE |
>> + OCP_LIMIT_HIGH,
>> + reg);
>> + if (ret) {
>> + dev_err(pmic_dev->dev,
>> + "Failed to update usb control register: %d\n",
>> + ret);
>> + return ret;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int stpmu1_device_init(struct stpmu1_dev *pmic_dev)
>> +{
>> + int ret;
>> + unsigned int val;
>> +
>> + pmic_dev->regmap =
>> + devm_regmap_init_i2c(pmic_dev->i2c, &stpmu1_regmap_config);
>> +
>> + if (IS_ERR(pmic_dev->regmap)) {
>> + ret = PTR_ERR(pmic_dev->regmap);
> You can remove this ...
>
>> + dev_err(pmic_dev->dev, "Failed to allocate register map: %d\n",
>> + ret);
>> + return ret;
> and just return PTR_ERR(pmic_dev->regmap);
>
>> + }
>> +
>> + ret = stpmu1_configure_from_dt(pmic_dev);
>> + if (ret < 0) {
> Is ret >0 return valid? If not, perhaps "if (ret)" would be better.
>
>> + dev_err(pmic_dev->dev,
>> + "Unable to configure PMIC from Device Tree: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + /* Read Version ID */
>> + ret = regmap_read(pmic_dev->regmap, VERSION_SR, &val);
>> + if (ret < 0) {
> Is ret >0 return valid? If not, perhaps "if (ret)" would be better.
>
>> + dev_err(pmic_dev->dev, "Unable to read pmic version\n");
>> + return ret;
>> + }
>> + dev_dbg(pmic_dev->dev, "PMIC Chip Version: 0x%x\n", val);
> nit: Maybe that should be dev_info instead of dev_dbg?
>
>> +
>> + /* Initialize PMIC IRQ Chip & IRQ domains associated */
>> + ret = devm_regmap_add_irq_chip(pmic_dev->dev, pmic_dev->regmap,
>> + pmic_dev->irq,
>> + IRQF_ONESHOT | IRQF_SHARED,
>> + 0, &stpmu1_regmap_irq_chip,
>> + &pmic_dev->irq_data);
>> + if (ret < 0) {
> Is ret >0 return valid? If not, perhaps "if (ret)" would be better.
>
>> + dev_err(pmic_dev->dev, "IRQ Chip registration failed: %d\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id stpmu1_dt_match[] = {
>> + {.compatible = "st,stpmu1"},
>> + {},
> I'd rewrite this as
> + { .compatible = "st,stpmu1" },
> + { }
> Space after/before brackets and no comma at the end. The sentinel
> indicates the last item on structure/arrays so no need to add a comma
> at the end.
>
>> +};
>> +
> Remove this line
>
>> +MODULE_DEVICE_TABLE(of, stpmu1_dt_match);
>> +
>> +static int stpmu1_remove(struct i2c_client *i2c)
>> +{
>> + struct stpmu1_dev *pmic_dev = i2c_get_clientdata(i2c);
>> +
>> + of_platform_depopulate(pmic_dev->dev);
>> +
>> + return 0;
>> +}
> You can remove this function, see below ...
>
>> +
>> +static int stpmu1_probe(struct i2c_client *i2c,
>> + const struct i2c_device_id *id)
>> +{
>> + struct stpmu1_dev *pmic;
>> + struct device *dev = &i2c->dev;
>> + int ret = 0;
> No need to initialize to 0 if ...
>
>> +
>> + pmic = devm_kzalloc(dev, sizeof(struct stpmu1_dev), GFP_KERNEL);
>> + if (!pmic)
>> + return -ENOMEM;
>> +
>> + pmic->np = dev->of_node;
>> +
>> + dev_set_drvdata(dev, pmic);
>> + pmic->dev = dev;
>> + pmic->i2c = i2c;
>> +
>> + ret = stpmu1_device_init(pmic);
>> + if (ret < 0)
> Is ret >0 return valid? If not, perhaps "if (ret)" would be better.
>> + goto err;
> return ret;
>
>> +
>> + ret = of_platform_populate(pmic->np, NULL, NULL, pmic->dev);
>> +
> ret = devm_of_platform_populate(pmic->dev);
>
> or even better
>
> return devm_of_platform_populate(pmic->dev);
>
> And remove the stpmu1_remove function.
From the regulator driver review, Mark Brown suggest to use
mfd_add_devices() to remove the compatible strings in the child drivers.
This means adding struct mfd_cell with a list of devices to probe.
Is it ok if I switch to mfd_add_devices() ?

>> + dev_dbg(dev, "stpmu1 driver probed\n");
> That message looks redundant to me. I'd remove it.
>
>> +err:
> And you can remove this label.
>
>> + return ret;
> And this
>
>> +}
>> +
>> +static const struct i2c_device_id stpmu1_id[] = {
>> + {"stpmu1", 0},
>> + {}
>> +};
>> +
>> +MODULE_DEVICE_TABLE(i2c, stpmu1_id);
> The above code shouldn't be needed anymore for DT-only devices. See
> da10c06a044b ("i2c: Make I2C ID tables non-mandatory for DT'ed
> devices")
>
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int stpmu1_suspend(struct device *dev)
>> +{
>> + struct i2c_client *i2c = container_of(dev, struct i2c_client, dev);
>> + struct stpmu1_dev *pmic_dev = i2c_get_clientdata(i2c);
>> +
>> + if (device_may_wakeup(dev))
>> + enable_irq_wake(pmic_dev->irq_wake);
>> +
>> + disable_irq(pmic_dev->irq);
>> + return 0;
>> +}
>> +
>> +static int stpmu1_resume(struct device *dev)
>> +{
>> + struct i2c_client *i2c = container_of(dev, struct i2c_client, dev);
>> + struct stpmu1_dev *pmic_dev = i2c_get_clientdata(i2c);
>> +
>> + regcache_sync(pmic_dev->regmap);
> Maybe you would like to check for an error here.
>
>> +
>> + if (device_may_wakeup(dev))
>> + disable_irq_wake(pmic_dev->irq_wake);
>> +
>> + enable_irq(pmic_dev->irq);
>> + return 0;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(stpmu1_pm, stpmu1_suspend, stpmu1_resume);
>> +
>> +static struct i2c_driver stpmu1_driver = {
>> + .driver = {
>> + .name = "stpmu1",
>> + .owner = THIS_MODULE,
> This is not needed, the core does it for you.
>
>> + .pm = &stpmu1_pm,
>> + .of_match_table = of_match_ptr(stpmu1_dt_match),
> It is a DT-only device so no need the of_match_ptr.
>
>> + },
>> + .probe = stpmu1_probe,
>> + .remove = stpmu1_remove,
> Now you can remove this
>
>> + .id_table = stpmu1_id,
> And you can remove this also.
>
>> +};
>> +
>> +module_i2c_driver(stpmu1_driver);
>> +
>> +MODULE_DESCRIPTION("STPMU1 PMIC I2C Client");
> nit: PMIC I2C Client sounds weird to me, "STPMU1 PMIC driver" ? Note
> that I am not english native so I could be wrong.
>
>> +MODULE_AUTHOR("<[email protected]>");
> Use "Name <email>" or just "Name"
>
>> +MODULE_LICENSE("GPL");
> As I told you there is a license mismatch with SPDX.
>
> [snip]
>
> Best regards,
> Enric
>

Best Regards,
pascal