Hi Andy,

Thanks for the reviews and patience.

-----Original Message-----
From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
Sent: Tuesday, June 06, 2017 6:00 AM
To: Mani, Rajmohan <redacted>
Cc: linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; linux-
acpi@vger.kernel.org; Lee Jones [off-list ref]; Linus Walleij
[off-list ref]; Alexandre Courbot [off-list ref]; Rafael J.
Wysocki [off-list ref]; Len Brown [off-list ref]
Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470

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

quoted

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.

We decided to go with the submission of these drivers for upstream review sooner rather than later.

quoted

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

quoted

+       /* FIXME: configure these dynamically */

So, what prevents you to fix this?

I will respond on top of Sakari's response.

quoted

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

quoted

+}

quoted

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

quoted

+       if (IS_ERR(tps->regmap)) {
+               dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
+               return PTR_ERR(tps->regmap);
+       }
+

quoted

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

devm_?

Ack

quoted

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

quoted

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

Repeating the response to Heikki here

I had this following question from Alan Cox on the original code without these wrappers.

"What is the model for insuring that no interrupt or thread of a driver is not in parallel issuing a tps68470_ operation when the device goes away (eg if I down the i2c controller) ?"

To address the above concerns, I got extra cautious and implemented locks around the regmap_* calls.
Now, I have been asked from more than one reviewer about the necessity of the same.
With the use of devm_* calls, tps68470_remove() goes away and leaves the driver just with regmap_* calls.
Unless I hear from Alan or other reviewers otherwise, I will drop these wrappers around regmap_* calls.

quoted

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

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

Same as above.

quoted

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

quoted

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

Ditto.

Same as above

quoted

+       return 0;
+}

quoted

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

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

Ack
It looks like this structure will go away, once I implement the feedback from Heikki.

quoted

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

quoted

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

Move it to kernel-doc above.

Same as above

quoted

+       struct mutex lock;
+};

--
With Best Regards,
Andy Shevchenko