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

[Pascal PAILLET-LME <p.paillet@st.com>]

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 <p.paillet@st.com> del dia dj., 5 de
> jul. 2018 a les 17:17:
>> From: pascal paillet <p.paillet@st.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@st.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@st.com>,
>> + * Pascal Paillet <p.paillet@st.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", &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("<philippe.peurichard@st.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
>

Best Regards,
pascal