Hi Heikki,

Thanks for the reviews and patience.

-----Original Message-----
From: Heikki Krogerus [mailto:heikki.krogerus@linux.intel.com]
Sent: Tuesday, June 06, 2017 5:49 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

Hi Rajmohan,

On Tue, Jun 06, 2017 at 04:55:16AM -0700, Rajmohan Mani wrote:

quoted

+/*
+ * tps68470_reg_read: Read a single tps68470 register.
+ *
+ * @tps: Device to read from.
+ * @reg: Register to read.
+ * @val: Contains the value
+ */
+int tps68470_reg_read(struct tps68470 *tps, unsigned int reg,
+			unsigned int *val)
+{
+	int ret;
+
+	mutex_lock(&tps->lock);
+	ret = regmap_read(tps->regmap, reg, val);
+	mutex_unlock(&tps->lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(tps68470_reg_read);
+
+/*
+ * tps68470_reg_write: Write a single tps68470 register.
+ *
+ * @tps68470: Device to write to.
+ * @reg: Register to write to.
+ * @val: Value to write.
+ */
+int tps68470_reg_write(struct tps68470 *tps, unsigned int reg,
+			unsigned int val)
+{
+	int ret;
+
+	mutex_lock(&tps->lock);
+	ret = regmap_write(tps->regmap, reg, val);
+	mutex_unlock(&tps->lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(tps68470_reg_write);
+
+/*
+ * tps68470_update_bits: Modify bits w.r.t mask and val.
+ *
+ * @tps68470: Device to write to.
+ * @reg: Register to read-write to.
+ * @mask: Mask.
+ * @val: Value to write.
+ */
+int tps68470_update_bits(struct tps68470 *tps, unsigned int reg,
+				unsigned int mask, unsigned int val) {
+	int ret;
+
+	mutex_lock(&tps->lock);
+	ret = regmap_update_bits(tps->regmap, reg, mask, val);
+	mutex_unlock(&tps->lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(tps68470_update_bits);

I'm not sure you need those above wrappers at all, regmap is handling locking in
any case.

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

+static const struct regmap_config tps68470_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = TPS68470_REG_MAX,
+};
+
+static int tps68470_chip_init(struct tps68470 *tps) {
+	unsigned int version;
+	int ret;
+
+	ret = tps68470_reg_read(tps, TPS68470_REG_REVID, &version);
+	if (ret < 0) {
+		dev_err(tps->dev,
+			"Failed to read revision register: %d\n", ret);
+		return ret;
+	}
+
+	dev_info(tps->dev, "TPS68470 REVID: 0x%x\n", version);
+
+	ret = tps68470_reg_write(tps, TPS68470_REG_RESET, 0xff);
+	if (ret < 0)
+		return ret;
+
+	/* FIXME: configure these dynamically */
+	/* Enable Daisy Chain LDO and configure relevant GPIOs as output */
+	ret = tps68470_reg_write(tps, TPS68470_REG_S_I2C_CTL, 2);
+	if (ret < 0)
+		return ret;
+
+	ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL4A, 2);
+	if (ret < 0)
+		return ret;
+
+	ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL5A, 2);
+	if (ret < 0)
+		return ret;
+
+	ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL6A, 2);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * When SDA and SCL are routed to GPIO1 and GPIO2, the mode
+	 * for these GPIOs must be configured using their respective
+	 * GPCTLxA registers as inputs with no pull-ups.
+	 */
+	ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL1A, 0);
+	if (ret < 0)
+		return ret;
+
+	ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL2A, 0);
+	if (ret < 0)
+		return ret;
+
+	/* Enable daisy chain */
+	ret = tps68470_update_bits(tps, TPS68470_REG_S_I2C_CTL, 1, 1);
+	if (ret < 0)
+		return ret;
+
+	usleep_range(TPS68470_DAISY_CHAIN_DELAY_US,
+			TPS68470_DAISY_CHAIN_DELAY_US + 10);
+	return 0;
+}
+
+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);
+	}
+
+	ret = mfd_add_devices(tps->dev, -1, tps68470s,
+			      ARRAY_SIZE(tps68470s), NULL, 0, NULL);
+	if (ret < 0) {
+		dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
+		return ret;
+	}

devm_mfd_add_devices()?

Ack

quoted

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

Why do you need to lock here?

Same as explained above (to address Alan's comments)

quoted

+	mfd_remove_devices(tps->dev);
+	mutex_unlock(&tps->lock);
+
+	return ret;
+}
+
+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);
+
+	return 0;
+}
+
+static const struct acpi_device_id tps68470_acpi_ids[] = {
+	{"INT3472"},
+	{},
+};
+
+MODULE_DEVICE_TABLE(acpi, tps68470_acpi_ids);
+
+static struct i2c_driver tps68470_driver = {
+	.driver = {
+		   .name = "tps68470",
+		   .acpi_match_table = ACPI_PTR(tps68470_acpi_ids),
+	},
+	.probe_new = tps68470_probe,
+	.remove = tps68470_remove,
+};

<snip>

quoted

+/**
+ * struct tps68470 - tps68470 sub-driver chip access routines
+ *
+ * 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
+	 */
+	struct mutex lock;

Is this lock really necessary at all? Actually, you probable don't even need this
structure at all if you just rely on regmap functions in the drivers.

Ack
I am looking into this and will get back with v2.

Thanks,

--
heikki