Hi Matti,

Thanks for the patch, a few comments below, some are feedback I
received when I sent some patches to this subsystem.

Missatge de Matti Vaittinen [off-list ref] del
dia dt., 19 de juny 2018 a les 12:57:

I know that some drivers need to be built-in, is this really a
requirement for this driver? Or should work as a module too.

There is a mismatch between what SPDX says and MODULE_LICENSE says.

GPL-2.0 = GPL v2 only
MODULE_LICENSE(GPL) = GPL v2 or later.

 You'd like to change the SPDX identifier to GPL-2.0-or-later or set
module license to "GPL v2".

+// Copyright (C) 2018 ROHM Semiconductors
+// bd71837.c -- ROHM BD71837MWV mfd driver
+//
+// Datasheet available from
+// https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>

This include is not required.

+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>

ditto

+#include <linux/irqdomain.h>

ditto

+#include <linux/gpio.h>

ditto

+#include <linux/regmap.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>

ditto

+#include <linux/mfd/core.h>
+#include <linux/mfd/bd71837.h>
+

Please review the needed includes.

+static const struct resource irqs[] = {
+       {
+               .start = BD71837_INT_PWRBTN,
+               .end = BD71837_INT_PWRBTN,
+               .flags = IORESOURCE_IRQ,
+               .name = "pwr-btn",
+       }, {
+               .start = BD71837_INT_PWRBTN_L,
+               .end = BD71837_INT_PWRBTN_L,
+               .flags = IORESOURCE_IRQ,
+               .name = "pwr-btn-l",
+       }, {
+               .start = BD71837_INT_PWRBTN_S,
+               .end = BD71837_INT_PWRBTN_S,
+               .flags = IORESOURCE_IRQ,
+               .name = "pwr-btn-s",
+       },

nit: no comma at the end

+};
+
+/* bd71837 multi function cells */
+static struct mfd_cell bd71837_mfd_cells[] = {
+       {
+               .name = "bd71837-clk",
+       }, {
+               .name = "bd718xx-pwrkey",
+               .resources = &irqs[0],
+               .num_resources = ARRAY_SIZE(irqs),
+       }, {
+               .name = "bd71837-pmic",
+       },

nit: no comma at the end

+};
+
+static const struct regmap_irq bd71837_irqs[] = {
+       REGMAP_IRQ_REG(BD71837_INT_SWRST, 0, BD71837_INT_SWRST_MASK),
+       REGMAP_IRQ_REG(BD71837_INT_PWRBTN_S, 0, BD71837_INT_PWRBTN_S_MASK),
+       REGMAP_IRQ_REG(BD71837_INT_PWRBTN_L, 0, BD71837_INT_PWRBTN_L_MASK),
+       REGMAP_IRQ_REG(BD71837_INT_PWRBTN, 0, BD71837_INT_PWRBTN_MASK),
+       REGMAP_IRQ_REG(BD71837_INT_WDOG, 0, BD71837_INT_WDOG_MASK),
+       REGMAP_IRQ_REG(BD71837_INT_ON_REQ, 0, BD71837_INT_ON_REQ_MASK),
+       REGMAP_IRQ_REG(BD71837_INT_STBY_REQ, 0, BD71837_INT_STBY_REQ_MASK),
+};
+
+static struct regmap_irq_chip bd71837_irq_chip = {
+       .name = "bd71837-irq",
+       .irqs = bd71837_irqs,
+       .num_irqs = ARRAY_SIZE(bd71837_irqs),
+       .num_regs = 1,
+       .irq_reg_stride = 1,
+       .status_base = BD71837_REG_IRQ,
+       .mask_base = BD71837_REG_MIRQ,
+       .ack_base = BD71837_REG_IRQ,
+       .init_ack_masked = true,
+       .mask_invert = false,
+};
+
+static int bd71837_irq_exit(struct bd71837 *bd71837)
+{
+       if (bd71837->chip_irq > 0)
+               regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);
+       return 0;
+}
+
+static const struct regmap_range pmic_status_range = {
+       .range_min = BD71837_REG_IRQ,
+       .range_max = BD71837_REG_POW_STATE,
+};
+
+static const struct regmap_access_table volatile_regs = {
+       .yes_ranges = &pmic_status_range,
+       .n_yes_ranges = 1,
+};
+
+static const struct regmap_config bd71837_regmap_config = {
+       .reg_bits = 8,
+       .val_bits = 8,
+       .volatile_table = &volatile_regs,
+       .max_register = BD71837_MAX_REGISTER - 1,
+       .cache_type = REGCACHE_RBTREE,
+};
+
+#ifdef CONFIG_OF

The driver is DT-only and depends on OF so you don't need to protect this.

+static const struct of_device_id bd71837_of_match[] = {
+       { .compatible = "rohm,bd71837", .data = (void *)0},
+       { },

nit: { /* sentinel */ }

+};
+MODULE_DEVICE_TABLE(of, bd71837_of_match);
+#endif //CONFIG_OF
+
+static int bd71837_i2c_probe(struct i2c_client *i2c,
+                           const struct i2c_device_id *id)
+{
+       struct bd71837 *bd71837;
+       struct bd71837_board *board_info;
+       int ret = -EINVAL;
+
+       board_info = dev_get_platdata(&i2c->dev);
+
+       if (!board_info) {
+               board_info = devm_kzalloc(&i2c->dev, sizeof(*board_info),
+                                         GFP_KERNEL);
+               if (!board_info) {
+                       ret = -ENOMEM;
+                       goto err_out;
+               } else if (i2c->irq) {
+                       board_info->gpio_intr = i2c->irq;
+               } else {
+                       ret = -ENOENT;
+                       goto err_out;
+               }
+       }
+
+       if (!board_info)
+               goto err_out;
+
+       bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
+       if (bd71837 == NULL)
+               return -ENOMEM;
+
+       i2c_set_clientdata(i2c, bd71837);
+       bd71837->dev = &i2c->dev;
+       bd71837->i2c_client = i2c;
+       bd71837->chip_irq = board_info->gpio_intr;
+
+       bd71837->regmap = devm_regmap_init_i2c(i2c, &bd71837_regmap_config);
+       if (IS_ERR(bd71837->regmap)) {
+               ret = PTR_ERR(bd71837->regmap);
+               dev_err(&i2c->dev, "regmap initialization failed: %d\n", ret);
+               goto err_out;
+       }
+
+       ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
+       if (ret < 0) {
+               dev_err(bd71837->dev,
+                       "%s(): Read BD71837_REG_DEVICE failed!\n", __func__);
+               goto err_out;
+       }
+
+       ret = regmap_add_irq_chip(bd71837->regmap, bd71837->chip_irq,
+               IRQF_ONESHOT, 0,
+               &bd71837_irq_chip, &bd71837->irq_data);

I think that you can use 'devm_regmap_add_irq_chip' here and remove
the code to delete the irq chip

+       if (ret < 0) {
+               dev_err(bd71837->dev, "Failed to add irq_chip %d\n", ret);
+               goto err_out;
+       }
+
+       ret = mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
+                             bd71837_mfd_cells, ARRAY_SIZE(bd71837_mfd_cells),
+                             NULL, 0,
+                             regmap_irq_get_domain(bd71837->irq_data));

Same here, I think you can use the devm_mfd_add_devices.

+       if (ret)
+               regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);

If you use devm_ functions you can remove this.

+err_out:
+
+       return ret;
+}
+
+static int bd71837_i2c_remove(struct i2c_client *i2c)
+{
+       struct bd71837 *bd71837 = i2c_get_clientdata(i2c);
+
+       bd71837_irq_exit(bd71837);
+       mfd_remove_devices(bd71837->dev);

All this can go away if you use the devm_ calls.

+
+       return 0;
+}
+
+static const struct i2c_device_id bd71837_i2c_id[] = {
+       { .name = "bd71837", },
+       { }

nit: { /* sentinel */ }

+};
+MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id);
+
+static struct i2c_driver bd71837_i2c_driver = {
+       .driver = {
+               .name = "bd71837-mfd",
+               .owner = THIS_MODULE,

Remove this, it is not needed, the core does it for you.

+               .of_match_table = of_match_ptr(bd71837_of_match),

The driver is DT-only, don't need to call of_match_ptr.

+       },
+       .probe = bd71837_i2c_probe,
+       .remove = bd71837_i2c_remove,
+       .id_table = bd71837_i2c_id,
+};
+
+static int __init bd71837_i2c_init(void)
+{
+       return i2c_add_driver(&bd71837_i2c_driver);
+}
+/* init early so consumer devices can complete system boot */
+subsys_initcall(bd71837_i2c_init);
+
+static void __exit bd71837_i2c_exit(void)
+{
+       i2c_del_driver(&bd71837_i2c_driver);
+}
+module_exit(bd71837_i2c_exit);
+

Can't you use module_i2c_driver here and get rid of init/exit calls?

+MODULE_AUTHOR("Matti Vaittinen [off-list ref]");
+MODULE_DESCRIPTION("BD71837 chip multi-function driver");
+MODULE_LICENSE("GPL");

License mismatch.

/* Register ...

+
+/* even though the bit zero is not SWRESET type we still want to write zero

/*
 * Even

+ * to it when changing type. Biz zero is 'SWRESET' trigger bit and if we

s/Biz/Bit/

+ * write 1 to it we will trigger the action. So always write 0 to it when
+ * changning SWRESET action - no matter what we read from it.
+ */
+#define BD71837_SWRESET_TYPE_MASK 7
+#define BD71837_SWRESET_TYPE_DISABLED 0
+#define BD71837_SWRESET_TYPE_COLD 4
+#define BD71837_SWRESET_TYPE_WARM 6
+
+#define BD71837_SWRESET_RESET_MASK 1
+#define BD71837_SWRESET_RESET 1
+
+/* Poweroff state transition conditions */
+
+#define BD718XX_ON_REQ_POWEROFF_MASK 1
+#define BD718XX_SWRESET_POWEROFF_MASK 2
+#define BD718XX_WDOG_POWEROFF_MASK 4
+#define BD718XX_KEY_L_POWEROFF_MASK 8
+
+#define BD718XX_POWOFF_TO_SNVS 0
+#define BD718XX_POWOFF_TO_RDY 0xF
+
+#define BD718XX_POWOFF_TIME_MASK 0xF0
+enum {
+       BD718XX_POWOFF_TIME_5MS = 0,
+       BD718XX_POWOFF_TIME_10MS,
+       BD718XX_POWOFF_TIME_15MS,
+       BD718XX_POWOFF_TIME_20MS,
+       BD718XX_POWOFF_TIME_25MS,
+       BD718XX_POWOFF_TIME_30MS,
+       BD718XX_POWOFF_TIME_35MS,
+       BD718XX_POWOFF_TIME_40MS,
+       BD718XX_POWOFF_TIME_45MS,
+       BD718XX_POWOFF_TIME_50MS,
+       BD718XX_POWOFF_TIME_75MS,
+       BD718XX_POWOFF_TIME_100MS,
+       BD718XX_POWOFF_TIME_250MS,
+       BD718XX_POWOFF_TIME_500MS,
+       BD718XX_POWOFF_TIME_750MS,
+       BD718XX_POWOFF_TIME_1500MS
+};
+
+/* Poweron sequence state transition conditions */
+
+#define BD718XX_RDY_TO_SNVS_MASK 0xF
+#define BD718XX_SNVS_TO_RUN_MASK 0xF0
+
+#define BD718XX_PWR_TRIG_KEY_L 1
+#define BD718XX_PWR_TRIG_KEY_S 2
+#define BD718XX_PWR_TRIG_PMIC_ON 4
+#define BD718XX_PWR_TRIG_VSYS_UVLO 8
+#define BD718XX_RDY_TO_SNVS_SIFT 0
+#define BD718XX_SNVS_TO_RUN_SIFT 4
+
+/* Timeout value for detecting short press */
+
+#define BD718XX_PWRBTN_SHORT_PRESS_MASK 0xF
+
+enum {
+       BD718XX_PWRBTN_SHORT_PRESS_10MS = 0,
+       BD718XX_PWRBTN_SHORT_PRESS_500MS,
+       BD718XX_PWRBTN_SHORT_PRESS_1000MS,
+       BD718XX_PWRBTN_SHORT_PRESS_1500MS,
+       BD718XX_PWRBTN_SHORT_PRESS_2000MS,
+       BD718XX_PWRBTN_SHORT_PRESS_2500MS,
+       BD718XX_PWRBTN_SHORT_PRESS_3000MS,
+       BD718XX_PWRBTN_SHORT_PRESS_3500MS,
+       BD718XX_PWRBTN_SHORT_PRESS_4000MS,
+       BD718XX_PWRBTN_SHORT_PRESS_4500MS,
+       BD718XX_PWRBTN_SHORT_PRESS_5000MS,
+       BD718XX_PWRBTN_SHORT_PRESS_5500MS,
+       BD718XX_PWRBTN_SHORT_PRESS_6000MS,
+       BD718XX_PWRBTN_SHORT_PRESS_6500MS,
+       BD718XX_PWRBTN_SHORT_PRESS_7000MS,
+       BD718XX_PWRBTN_SHORT_PRESS_7500MS
+};
+
+struct bd71837;
+struct bd71837_pmic;
+struct bd71837_clk;
+struct bd718xx_pwrkey;
+
+/*
+ * Board platform data may be used to initialize regulators.
+ */
+
+struct bd71837_board {
+       struct regulator_init_data *init_data[BD71837_REGULATOR_CNT];
+       int     gpio_intr;
+};
+
+struct bd71837 {
+       struct device *dev;
+       struct i2c_client *i2c_client;
+       struct regmap *regmap;
+       unsigned long int id;
+
+       int chip_irq;
+       struct regmap_irq_chip_data *irq_data;
+
+       struct bd71837_pmic *pmic;
+       struct bd71837_clk *clk;
+       struct bd718xx_pwrkey *pwrkey;
+};
+
+/*
+ * bd71837 sub-driver chip access routines
+ */
+

All these access routines only hide the regmap_* calls, so why not use
the regmap calls directly in the driver and remove all this?

+static inline int bd71837_reg_read(struct bd71837 *bd71837, u8 reg)
+{
+       int r, val;
+
+       r = regmap_read(bd71837->regmap, reg, &val);
+       if (r < 0)
+               return r;
+       return val;
+}
+
+static inline int bd71837_reg_write(struct bd71837 *bd71837, u8 reg,
+                                   unsigned int val)
+{
+       return regmap_write(bd71837->regmap, reg, val);
+}
+
+static inline int bd71837_set_bits(struct bd71837 *bd71837, u8 reg, u8 mask)
+{
+       return regmap_update_bits(bd71837->regmap, reg, mask, mask);
+}
+
+static inline int bd71837_clear_bits(struct bd71837 *bd71837, u8 reg,
+                                    u8 mask)
+{
+       return regmap_update_bits(bd71837->regmap, reg, mask, 0);
+}
+
+static inline int bd71837_update_bits(struct bd71837 *bd71837, u8 reg,
+                                     u8 mask, u8 val)
+{
+       return regmap_update_bits(bd71837->regmap, reg, mask, val);
+}
+
+#endif /* __LINUX_MFD_BD71837_H__ */
--
2.14.3