On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani [off-list ref] wrote:

The TPS68470 device is an advanced power management
unit that powers a Compact Camera Module (CCM),
generates clocks for image sensors, drives a dual
LED for Flash and incorporates two LED drivers for
general purpose indicators.

This patch adds support for TPS68470 mfd device.

I dunno why you decide to send this out now, see my comments below.

+static int tps68470_chip_init(struct tps68470 *tps)
+{
+       unsigned int version;
+       int ret;

+       /* FIXME: configure these dynamically */

So, what prevents you to fix this?

+       /* Enable Daisy Chain LDO and configure relevant GPIOs as output */

+}

+static int tps68470_probe(struct i2c_client *client)
+{
+       struct tps68470 *tps;
+       int ret;
+
+       tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
+       if (!tps)
+               return -ENOMEM;
+
+       mutex_init(&tps->lock);
+       i2c_set_clientdata(client, tps);
+       tps->dev = &client->dev;
+
+       tps->regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
+       if (IS_ERR(tps->regmap)) {
+               dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
+               return PTR_ERR(tps->regmap);
+       }
+

+       ret = mfd_add_devices(tps->dev, -1, tps68470s,
+                             ARRAY_SIZE(tps68470s), NULL, 0, NULL);

devm_?

+       if (ret < 0) {
+               dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
+               return ret;
+       }
+
+       ret = tps68470_chip_init(tps);
+       if (ret < 0) {
+               dev_err(tps->dev, "TPS68470 Init Error %d\n", ret);
+               goto fail;
+       }
+
+       return 0;

+fail:
+       mutex_lock(&tps->lock);

I'm not sure you need this mutex to be held here.
Otherwise your code has a bug with locking.

+       mfd_remove_devices(tps->dev);
+       mutex_unlock(&tps->lock);
+
+       return ret;

Taking above into consideration I suggest to clarify your locking scheme.

+}
+
+static int tps68470_remove(struct i2c_client *client)
+{
+       struct tps68470 *tps = i2c_get_clientdata(client);
+

+       mutex_lock(&tps->lock);
+       mfd_remove_devices(tps->dev);
+       mutex_unlock(&tps->lock);

Ditto.

+       return 0;
+}

+/**
+ * struct tps68470 - tps68470 sub-driver chip access routines
+ *

kbuild bot will be unhappy. You need to file a description per field.

+ * Device data may be used to access the TPS68470 chip
+ */
+
+struct tps68470 {
+       struct device *dev;
+       struct regmap *regmap;

+       /*
+        * Used to synchronize access to tps68470_ operations
+        * and addition and removal of mfd devices
+        */

Move it to kernel-doc above.

+       struct mutex lock;
+};

-- 
With Best Regards,
Andy Shevchenko