| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 10 Jul 2018 00:38:36 +0200
From: Enric Balletbo Serra <eballetbo@...il.com>
To: p.paillet@...com
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>,
Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Lee Jones <lee.jones@...aro.org>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>, wim@...ux-watchdog.org,
Guenter Roeck <linux@...ck-us.net>,
linux-input@...r.kernel.org,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
linux-watchdog@...r.kernel.org, benjamin.gaignard@...aro.org
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 <p.paillet@...com> del dia dj., 5 de
jul. 2018 a les 17:17:
>
> From: pascal paillet <p.paillet@...com>
>
> stpmu1 is a pmic from STMicroelectronics. The stpmu1 integrates 10
> regulators and 3 switches with various capabilities.
>
> Signed-off-by: pascal paillet <p.paillet@...com>
> ---
> 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 <philippe.peurichard@...com>,
> + * Pascal Paillet <p.paillet@...com> 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", ®)) {
> + 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", ®)) {
> + 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", ®)) {
> + 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", ®)) {
> + 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("<philippe.peurichard@...com>");
Use "Name <email>" or just "Name"
> +MODULE_LICENSE("GPL");
As I told you there is a license mismatch with SPDX.
[snip]
Best regards,
Enric
Powered by blists - more mailing lists