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 [off-list ref] del dia dj., 5 de
jul. 2018 a les 17:17:

quoted

From: pascal paillet <redacted>

stpmu1 is a pmic from STMicroelectronics. The stpmu1 integrates 10
regulators and 3 switches with various capabilities.

Signed-off-by: pascal paillet <redacted>
---
  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.

quoted

  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

quoted

+/*
+ * Copyright (C) STMicroelectronics 2018 - All Rights Reserved
+ * Author: Philippe Peurichard [off-list ref],
+ * Pascal Paillet [off-list ref] for STMicroelectronics.
+ */
+

I think that Lee, like Linus, prefers the C++ style here

quoted

+#include <linux/err.h>

That this include is not needed.

quoted

+#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

quoted

+#include <linux/pm_runtime.h>

ditto

quoted

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

[snip]

quoted

+
+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.

quoted

+       int ret = 0;

You don't need to initialize ret to 0, anyway will be overwritten.

quoted

+       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.

quoted

+               return irq ? irq : -ENODEV;

nit: return irq ?: -ENODEV;

quoted

+       }
+       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;

quoted

+       }
+       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 ...

quoted

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

and just return PTR_ERR(pmic_dev->regmap);

quoted

+       }
+
+       ret = stpmu1_configure_from_dt(pmic_dev);
+       if (ret < 0) {

Is ret >0 return valid?  If not, perhaps "if (ret)" would be better.

quoted

+               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.

quoted

+               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?

quoted

+
+       /* 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.

quoted

+               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.

quoted

+};
+

Remove this line

quoted

+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 ...

quoted

+
+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 ...

quoted

+
+       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.

quoted

+               goto err;

return ret;

quoted

+
+       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() ?

quoted

+       dev_dbg(dev, "stpmu1 driver probed\n");

That message looks redundant to me. I'd remove it.

quoted

+err:

And you can remove this label.

quoted

+       return ret;

And this

quoted

+}
+
+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")

quoted

+
+#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.

quoted

+
+       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.

quoted

+                  .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.

quoted

+                  },
+       .probe = stpmu1_probe,
+       .remove = stpmu1_remove,

Now you can remove this

quoted

+       .id_table = stpmu1_id,

And you can remove this also.

quoted

+};
+
+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.

quoted

+MODULE_AUTHOR("[off-list ref]");

Use "Name <email>" or just "Name"

quoted

+MODULE_LICENSE("GPL");

As I told you there is a license mismatch with SPDX.

[snip]

Best regards,
  Enric

Best Regards,
pascal