Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1658685 > unrolled thread

[PATCH v1 1/3] mfd: Add new mfd device TPS68470

Started byRajmohan Mani <rajmohan.mani@intel.com>
First post2017-06-06 14:10 +0200
Last post2017-06-10 00:10 +0200
Articles 9 — 6 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v1 1/3] mfd: Add new mfd device TPS68470 Rajmohan Mani <rajmohan.mani@intel.com> - 2017-06-06 14:10 +0200
    Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470 Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2017-06-06 15:00 +0200
      RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470 "Mani, Rajmohan" <rajmohan.mani@intel.com> - 2017-06-10 00:10 +0200
    Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470 Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-06 15:10 +0200
      Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470 Sakari Ailus <sakari.ailus@iki.fi> - 2017-06-07 14:00 +0200
        RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470 "Mani, Rajmohan" <rajmohan.mani@intel.com> - 2017-06-10 00:20 +0200
          Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470 Lee Jones <lee.jones@linaro.org> - 2017-06-12 10:30 +0200
            RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470 "Mani, Rajmohan" <rajmohan.mani@intel.com> - 2017-06-12 11:30 +0200
      RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470 "Mani, Rajmohan" <rajmohan.mani@intel.com> - 2017-06-10 00:10 +0200

#1658685 — [PATCH v1 1/3] mfd: Add new mfd device TPS68470

FromRajmohan Mani <rajmohan.mani@intel.com>
Date2017-06-06 14:10 +0200
Subject[PATCH v1 1/3] mfd: Add new mfd device TPS68470
Message-ID<tPlt8-aF-7@gated-at.bofh.it>
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.

Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
---
 drivers/mfd/Kconfig          |  12 +++
 drivers/mfd/Makefile         |   1 +
 drivers/mfd/tps68470.c       | 227 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/tps68470.h | 167 +++++++++++++++++++++++++++++++
 4 files changed, 407 insertions(+)
 create mode 100644 drivers/mfd/tps68470.c
 create mode 100644 include/linux/mfd/tps68470.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 3eb5c93..c5e51bc 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1311,6 +1311,18 @@ config MFD_TPS65217
 	  This driver can also be built as a module.  If so, the module
 	  will be called tps65217.
 
+config MFD_TPS68470
+	bool "TI TPS68470 Power Management / LED chips"
+	depends on I2C
+	select MFD_CORE
+	select REGMAP_I2C
+	help
+	  If you say yes here you get support for the TPS68470 series of
+	  Power Management / LED chips.
+
+	  These include voltage regulators, led and other features
+	  that are often used in portable devices.
+
 config MFD_TI_LP873X
 	tristate "TI LP873X Power Management IC"
 	depends on I2C
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index c16bf1e..6dd2b94 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -82,6 +82,7 @@ obj-$(CONFIG_MFD_TPS65910)	+= tps65910.o
 obj-$(CONFIG_MFD_TPS65912)	+= tps65912-core.o
 obj-$(CONFIG_MFD_TPS65912_I2C)	+= tps65912-i2c.o
 obj-$(CONFIG_MFD_TPS65912_SPI)  += tps65912-spi.o
+obj-$(CONFIG_MFD_TPS68470)	+= tps68470.o
 obj-$(CONFIG_MFD_TPS80031)	+= tps80031.o
 obj-$(CONFIG_MENELAUS)		+= menelaus.o
 
diff --git a/drivers/mfd/tps68470.c b/drivers/mfd/tps68470.c
new file mode 100644
index 0000000..ca174fb
--- /dev/null
+++ b/drivers/mfd/tps68470.c
@@ -0,0 +1,227 @@
+/*
+ * TPS68470 chip family multi-function driver
+ *
+ * Copyright (C) 2017 Intel Corporation
+ * Authors:
+ * Rajmohan Mani <rajmohan.mani@intel.com>
+ * Tianshu Qiu <tian.shu.qiu@intel.com>
+ * Jian Xu Zheng <jian.xu.zheng@intel.com>
+ * Yuning Pu <yuning.pu@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/acpi.h>
+#include <linux/delay.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/tps68470.h>
+#include <linux/init.h>
+#include <linux/regmap.h>
+
+static const struct mfd_cell tps68470s[] = {
+	{
+		.name = "tps68470-gpio",
+	},
+	{
+		.name = "tps68470_pmic_opregion",
+	},
+};
+
+/*
+ * 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);
+
+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);
+	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;
+	}
+
+	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);
+	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,
+};
+builtin_i2c_driver(tps68470_driver);
diff --git a/include/linux/mfd/tps68470.h b/include/linux/mfd/tps68470.h
new file mode 100644
index 0000000..440c802
--- /dev/null
+++ b/include/linux/mfd/tps68470.h
@@ -0,0 +1,167 @@
+/*
+ * Copyright (c) 2017 Intel Corporation
+ *
+ * Functions to access TPS68470 power management chip.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __LINUX_MFD_TPS68470_H
+#define __LINUX_MFD_TPS68470_H
+
+#include <linux/i2c.h>
+
+/* All register addresses */
+#define TPS68470_REG_GSTAT		0x01
+#define TPS68470_REG_VRSTA		0x02
+#define TPS68470_REG_VRSHORT		0x03
+#define TPS68470_REG_INTMASK		0x04
+#define TPS68470_REG_VCOSPEED		0x05
+#define TPS68470_REG_POSTDIV2		0x06
+#define TPS68470_REG_BOOSTDIV		0x07
+#define TPS68470_REG_BUCKDIV		0x08
+#define TPS68470_REG_PLLSWR		0x09
+#define TPS68470_REG_XTALDIV		0x0A
+#define TPS68470_REG_PLLDIV		0x0B
+#define TPS68470_REG_POSTDIV		0x0C
+#define TPS68470_REG_PLLCTL		0x0D
+#define TPS68470_REG_PLLCTL2		0x0E
+#define TPS68470_REG_CLKCFG1		0x0F
+#define TPS68470_REG_CLKCFG2		0x10
+#define TPS68470_REG_GPCTL0A		0x14
+#define TPS68470_REG_GPCTL0B		0x15
+#define TPS68470_REG_GPCTL1A		0x16
+#define TPS68470_REG_GPCTL1B		0x17
+#define TPS68470_REG_GPCTL2A		0x18
+#define TPS68470_REG_GPCTL2B		0x19
+#define TPS68470_REG_GPCTL3A		0x1A
+#define TPS68470_REG_GPCTL3B		0x1B
+#define TPS68470_REG_GPCTL4A		0x1C
+#define TPS68470_REG_GPCTL4B		0x1D
+#define TPS68470_REG_GPCTL5A		0x1E
+#define TPS68470_REG_GPCTL5B		0x1F
+#define TPS68470_REG_GPCTL6A		0x20
+#define TPS68470_REG_GPCTL6B		0x21
+#define TPS68470_REG_SGPO		0x22
+#define TPS68470_REG_PITCTL		0x23
+#define TPS68470_REG_WAKECFG		0x24
+#define TPS68470_REG_IOWAKESTAT		0x25
+#define TPS68470_REG_GPDI		0x26
+#define TPS68470_REG_GPDO		0x27
+#define TPS68470_REG_ILEDCTL		0x28
+#define TPS68470_REG_WLEDSTAT		0x29
+#define TPS68470_REG_VWLEDILIM		0x2A
+#define TPS68470_REG_VWLEDVAL		0x2B
+#define TPS68470_REG_WLEDMAXRER		0x2C
+#define TPS68470_REG_WLEDMAXT		0x2D
+#define TPS68470_REG_WLEDMAXAF		0x2E
+#define TPS68470_REG_WLEDMAXF		0x2F
+#define TPS68470_REG_WLEDTO		0x30
+#define TPS68470_REG_VWLEDCTL		0x31
+#define TPS68470_REG_WLEDTIMER_MSB	0x32
+#define TPS68470_REG_WLEDTIMER_LSB	0x33
+#define TPS68470_REG_WLEDC1		0x34
+#define TPS68470_REG_WLEDC2		0x35
+#define TPS68470_REG_WLEDCTL		0x36
+#define TPS68470_REG_VCMVAL		0x3C
+#define TPS68470_REG_VAUX1VAL		0x3D
+#define TPS68470_REG_VAUX2VAL		0x3E
+#define TPS68470_REG_VIOVAL		0x3F
+#define TPS68470_REG_VSIOVAL		0x40
+#define TPS68470_REG_VAVAL		0x41
+#define TPS68470_REG_VDVAL		0x42
+#define TPS68470_REG_S_I2C_CTL		0x43
+#define TPS68470_REG_VCMCTL		0x44
+#define TPS68470_REG_VAUX1CTL		0x45
+#define TPS68470_REG_VAUX2CTL		0x46
+#define TPS68470_REG_VACTL		0x47
+#define TPS68470_REG_VDCTL		0x48
+#define TPS68470_REG_RESET		0x50
+#define TPS68470_REG_REVID		0xFF
+
+#define TPS68470_REG_MAX		TPS68470_REG_REVID
+
+/* Register field definitions */
+
+#define TPS68470_VAVAL_AVOLT_MASK	GENMASK(6, 0)
+
+#define TPS68470_VDVAL_DVOLT_MASK	GENMASK(5, 0)
+#define TPS68470_VCMVAL_VCVOLT_MASK	GENMASK(6, 0)
+#define TPS68470_VIOVAL_IOVOLT_MASK	GENMASK(6, 0)
+#define TPS68470_VSIOVAL_IOVOLT_MASK	GENMASK(6, 0)
+#define TPS68470_VAUX1VAL_AUX1VOLT_MASK	GENMASK(6, 0)
+#define TPS68470_VAUX2VAL_AUX2VOLT_MASK	GENMASK(6, 0)
+
+#define TPS68470_VACTL_EN_MASK		GENMASK(0, 0)
+#define TPS68470_VDCTL_EN_MASK		GENMASK(0, 0)
+#define TPS68470_VCMCTL_EN_MASK		GENMASK(0, 0)
+#define TPS68470_S_I2C_CTL_EN_MASK	GENMASK(1, 0)
+#define TPS68470_VAUX1CTL_EN_MASK	GENMASK(0, 0)
+#define TPS68470_VAUX2CTL_EN_MASK	GENMASK(0, 0)
+#define TPS68470_PLL_EN_MASK		GENMASK(0, 0)
+
+#define TPS68470_OSC_EXT_CAP_SHIFT	4
+#define TPS68470_OSC_EXT_CAP_DEFAULT	0x05	/* 10pf */
+
+#define TPS68470_CLK_SRC_SHIFT		7
+#define TPS68470_CLK_SRC_GPIO3		0
+#define TPS68470_CLK_SRC_XTAL		1
+
+#define TPS68470_DRV_STR_1MA		0
+#define TPS68470_DRV_STR_2MA		1
+#define TPS68470_DRV_STR_4MA		2
+#define TPS68470_DRV_STR_8MA		3
+#define TPS68470_DRV_STR_A_SHIFT	0
+#define TPS68470_DRV_STR_B_SHIFT	2
+
+#define TPS68470_OUTPUT_XTAL_BUFFERED	1
+#define TPS68470_PLL_OUTPUT_ENABLE	2
+#define TPS68470_PLL_OUTPUT_SS_ENABLE	3
+#define TPS68470_OUTPUT_A_SHIFT		0
+#define TPS68470_OUTPUT_B_SHIFT		2
+
+#define TPS68470_CLKCFG1_MODE_A_MASK	GENMASK(1, 0)
+#define TPS68470_CLKCFG1_MODE_B_MASK	GENMASK(3, 2)
+
+#define TPS68470_GPIO_CTL_REG_A(x)	(TPS68470_REG_GPCTL0A + (x) * 2)
+#define TPS68470_GPIO_CTL_REG_B(x)	(TPS68470_REG_GPCTL0B + (x) * 2)
+#define TPS68470_GPIO_MODE_MASK		GENMASK(1, 0)
+#define TPS68470_GPIO_MODE_IN		0
+#define TPS68470_GPIO_MODE_IN_PULLUP	1
+#define TPS68470_GPIO_MODE_OUT_CMOS	2
+#define TPS68470_GPIO_MODE_OUT_ODRAIN	3
+
+#define TPS68470_PLL_STARTUP_DELAY_US	1000
+#define TPS68470_DAISY_CHAIN_DELAY_US	3000
+
+/**
+ * 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;
+};
+
+int tps68470_reg_read(struct tps68470 *tps, unsigned int reg,
+		      unsigned int *val);
+int tps68470_reg_write(struct tps68470 *tps, unsigned int reg,
+		       unsigned int val);
+int tps68470_update_bits(struct tps68470 *tps, unsigned int reg,
+		      unsigned int mask, unsigned int val);
+
+#endif /* __LINUX_MFD_TPS68470_H */
-- 
1.9.1

[toc] | [next] | [standalone]

#1658749

FromHeikki Krogerus <heikki.krogerus@linux.intel.com>
Date2017-06-06 15:00 +0200
Message-ID<tPmfv-vX-5@gated-at.bofh.it>
In reply to#1658685
Hi Rajmohan,

On Tue, Jun 06, 2017 at 04:55:16AM -0700, Rajmohan Mani wrote:
> +/*
> + * 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.

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

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

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

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


Thanks,

-- 
heikki

[toc] | [prev] | [next] | [standalone]

#1662777

From"Mani, Rajmohan" <rajmohan.mani@intel.com>
Date2017-06-10 00:10 +0200
Message-ID<tQAgp-78J-9@gated-at.bofh.it>
In reply to#1658749
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 <rajmohan.mani@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; linux-
> acpi@vger.kernel.org; Lee Jones <lee.jones@linaro.org>; Linus Walleij
> <linus.walleij@linaro.org>; Alexandre Courbot <gnurou@gmail.com>; Rafael J.
> Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>
> 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:
> > +/*
> > + * 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.

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

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

> > +	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>
> 
> > +/**
> > + * 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

[toc] | [prev] | [next] | [standalone]

#1658755

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2017-06-06 15:10 +0200
Message-ID<tPmpd-OH-47@gated-at.bofh.it>
In reply to#1658685
On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.mani@intel.com> 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

[toc] | [prev] | [next] | [standalone]

#1659723

FromSakari Ailus <sakari.ailus@iki.fi>
Date2017-06-07 14:00 +0200
Message-ID<tPHN1-68d-41@gated-at.bofh.it>
In reply to#1658755
Hi Andy,

On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.mani@intel.com> 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?

Nothing I suppose. They're however not needed right now and can be
implemented later on if they're ever needed.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [next] | [standalone]

#1662782

From"Mani, Rajmohan" <rajmohan.mani@intel.com>
Date2017-06-10 00:20 +0200
Message-ID<tQAq6-7c1-5@gated-at.bofh.it>
In reply to#1659723
Hi Andy,

> On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani
> <rajmohan.mani@intel.com> 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?
> 
> Nothing I suppose. They're however not needed right now and can be
> implemented later on if they're ever needed.
> 

Ack

[toc] | [prev] | [next] | [standalone]

#1663322

FromLee Jones <lee.jones@linaro.org>
Date2017-06-12 10:30 +0200
Message-ID<tRsTx-8al-21@gated-at.bofh.it>
In reply to#1662782
On Fri, 09 Jun 2017, Mani, Rajmohan wrote:

> Hi Andy,
> 
> > On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani
> > <rajmohan.mani@intel.com> 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?
> > 
> > Nothing I suppose. They're however not needed right now and can be
> > implemented later on if they're ever needed.
> > 
> 
> Ack

What does this mean?  Is the plan to fix it or not?  I don't want
FIXMEs in the code that a) can be fixed right away or b) might never
be fixed.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1663361

From"Mani, Rajmohan" <rajmohan.mani@intel.com>
Date2017-06-12 11:30 +0200
Message-ID<tRtPA-iC-5@gated-at.bofh.it>
In reply to#1663322
Hi Lee,

> Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470
> 
> On Fri, 09 Jun 2017, Mani, Rajmohan wrote:
> 
> > Hi Andy,
> >
> > > On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani
> > > <rajmohan.mani@intel.com> 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?
> > >
> > > Nothing I suppose. They're however not needed right now and can be
> > > implemented later on if they're ever needed.
> > >
> >
> > Ack
> 
> What does this mean?  Is the plan to fix it or not?  I don't want FIXMEs in the
> code that a) can be fixed right away or b) might never be fixed.
> 

I meant that this can be implemented later on, if there's a need.
I will look into this and see how this can be fixed.

Thanks
Raj

[toc] | [prev] | [next] | [standalone]

#1662772

From"Mani, Rajmohan" <rajmohan.mani@intel.com>
Date2017-06-10 00:10 +0200
Message-ID<tQAgp-78J-3@gated-at.bofh.it>
In reply to#1658755
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 <rajmohan.mani@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; linux-
> acpi@vger.kernel.org; Lee Jones <lee.jones@linaro.org>; Linus Walleij
> <linus.walleij@linaro.org>; Alexandre Courbot <gnurou@gmail.com>; Rafael J.
> Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>
> Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470
> 
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.mani@intel.com>
> 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.
> 

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

> > +static int tps68470_chip_init(struct tps68470 *tps) {
> > +       unsigned int version;
> > +       int ret;
> 
> > +       /* FIXME: configure these dynamically */
> 
> So, what prevents you to fix this?
> 

I will respond on top of Sakari's response.

> > +       /* 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_?
> 

Ack

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

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.

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

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

Same as above

> > +       return 0;
> > +}
> 
> > +/**
> > + * 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.

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

Same as above

> > +       struct mutex lock;
> > +};
> 
> --
> With Best Regards,
> Andy Shevchenko

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web