[PATCH v1 0/3] TPS68470 PMIC drivers

[PATCH v1 0/3] TPS68470 PMIC drivers

[Rajmohan Mani]

This is the patch series for TPS68470 PMIC that works as a camera PMIC.

The patch series provide the following 3 drivers.

TPS68470 MFD driver:
This is the multi function driver that initializes the TPS68470
PMIC and supports the GPIO and Op Region functions.

TPS68470 GPIO driver:
This is the PMIC GPIO driver that will be used by the OS GPIO layer,
when the BIOS / firmware triggered GPIO access is done.

TPS68470 Op Region driver:
This is the driver that will be invoked, when the BIOS / firmware
configures the voltage / clock for the sensors / vcm devices
connected to the PMIC.

---

Rajmohan Mani (3):
  mfd: Add new mfd device TPS68470
  gpio: Add support for TPS68470 GPIOs
  ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

 drivers/acpi/Kconfig              |  12 +
 drivers/acpi/Makefile             |   2 +
 drivers/acpi/pmic/pmic_tps68470.c | 454 ++++++++++++++++++++++++++++++++++++++
 drivers/gpio/Kconfig              |  10 +
 drivers/gpio/Makefile             |   1 +
 drivers/gpio/gpio-tps68470.c      | 185 ++++++++++++++++
 drivers/mfd/Kconfig               |  12 +
 drivers/mfd/Makefile              |   1 +
 drivers/mfd/tps68470.c            | 227 +++++++++++++++++++
 include/linux/mfd/tps68470.h      | 167 ++++++++++++++
 10 files changed, 1071 insertions(+)
 create mode 100644 drivers/acpi/pmic/pmic_tps68470.c
 create mode 100644 drivers/gpio/gpio-tps68470.c
 create mode 100644 drivers/mfd/tps68470.c
 create mode 100644 include/linux/mfd/tps68470.h

-- 
1.9.1

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

[Rajmohan Mani]

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

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

[Heikki Krogerus]

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

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

[Mani, Rajmohan]

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

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

[Andy Shevchenko]

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

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

[Sakari Ailus]

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

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

[Mani, Rajmohan]

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

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

[Lee Jones]

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

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

[Mani, Rajmohan]

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

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

[Mani, Rajmohan]

Hi Lee,

> > 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 for your patience.
This is fixed with v4 of this series.

Raj

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

[Lee Jones]

On Wed, 19 Jul 2017, Mani, Rajmohan wrote:

> Hi Lee,
> 
> > > 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 for your patience.
> This is fixed with v4 of this series.

Great, thanks.

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

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

[Mani, Rajmohan]

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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2230 bytes --]

Hi Rajmohan,

[auto build test WARNING on ljones-mfd/for-mfd-next]
[also build test WARNING on v4.12-rc4 next-20170606]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Rajmohan-Mani/mfd-Add-new-mfd-device-TPS68470/20170607-080306
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=ia64 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/mfd/tps68470.c: In function 'tps68470_probe':
>> drivers/mfd/tps68470.c:175:3: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
      dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/ret +175 drivers/mfd/tps68470.c

   159	
   160	static int tps68470_probe(struct i2c_client *client)
   161	{
   162		struct tps68470 *tps;
   163		int ret;
   164	
   165		tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
   166		if (!tps)
   167			return -ENOMEM;
   168	
   169		mutex_init(&tps->lock);
   170		i2c_set_clientdata(client, tps);
   171		tps->dev = &client->dev;
   172	
   173		tps->regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
   174		if (IS_ERR(tps->regmap)) {
 > 175			dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
   176			return PTR_ERR(tps->regmap);
   177		}
   178	
   179		ret = mfd_add_devices(tps->dev, -1, tps68470s,
   180				      ARRAY_SIZE(tps68470s), NULL, 0, NULL);
   181		if (ret < 0) {
   182			dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
   183			return ret;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 49287 bytes --]

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1990 bytes --]

Hi Rajmohan,

[auto build test ERROR on ljones-mfd/for-mfd-next]
[also build test ERROR on v4.12-rc4 next-20170607]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Rajmohan-Mani/mfd-Add-new-mfd-device-TPS68470/20170607-080306
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=alpha 

All error/warnings (new ones prefixed by >>):

>> drivers/mfd/tps68470.c:217:1: warning: data definition has no type or storage class
    MODULE_DEVICE_TABLE(acpi, tps68470_acpi_ids);
    ^~~~~~~~~~~~~~~~~~~
>> drivers/mfd/tps68470.c:217:1: error: type defaults to 'int' in declaration of 'MODULE_DEVICE_TABLE' [-Werror=implicit-int]
>> drivers/mfd/tps68470.c:217:1: warning: parameter names (without types) in function declaration
   drivers/mfd/tps68470.c: In function 'tps68470_probe':
   drivers/mfd/tps68470.c:175:3: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
      dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +217 drivers/mfd/tps68470.c

   211	
   212	static const struct acpi_device_id tps68470_acpi_ids[] = {
   213		{"INT3472"},
   214		{},
   215	};
   216	
 > 217	MODULE_DEVICE_TABLE(acpi, tps68470_acpi_ids);
   218	
   219	static struct i2c_driver tps68470_driver = {
   220		.driver = {

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 51469 bytes --]

[PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Rajmohan Mani]

This patch adds support for TPS68470 GPIOs.
There are 7 GPIOs and a few sensor related GPIOs.
These GPIOs can be requested and configured as
appropriate.

Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
---
 drivers/gpio/Kconfig         |  10 +++
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-tps68470.c | 185 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 196 insertions(+)
 create mode 100644 drivers/gpio/gpio-tps68470.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 23ca51e..ed34f9b 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1041,6 +1041,16 @@ config GPIO_TPS65912
 	help
 	  This driver supports TPS65912 gpio chip
 
+config GPIO_TPS68470
+	bool "TPS68470 GPIO"
+	depends on MFD_TPS68470
+	help
+	  Select this option to enable GPIO driver for the TPS68470
+	  chip family.
+	  There are 7 GPIOs and few sensor related GPIOs supported
+	  by the TPS68470. These GPIOs can be requested and
+	  configured as appropriate, with this driver.
+
 config GPIO_TWL4030
 	tristate "TWL4030, TWL5030, and TPS659x0 GPIOs"
 	depends on TWL4030_CORE
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 68b9627..a2659cb 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -120,6 +120,7 @@ obj-$(CONFIG_GPIO_TPS65218)	+= gpio-tps65218.o
 obj-$(CONFIG_GPIO_TPS6586X)	+= gpio-tps6586x.o
 obj-$(CONFIG_GPIO_TPS65910)	+= gpio-tps65910.o
 obj-$(CONFIG_GPIO_TPS65912)	+= gpio-tps65912.o
+obj-$(CONFIG_GPIO_TPS68470)	+= gpio-tps68470.o
 obj-$(CONFIG_GPIO_TS4800)	+= gpio-ts4800.o
 obj-$(CONFIG_GPIO_TS4900)	+= gpio-ts4900.o
 obj-$(CONFIG_GPIO_TS5500)	+= gpio-ts5500.o
diff --git a/drivers/gpio/gpio-tps68470.c b/drivers/gpio/gpio-tps68470.c
new file mode 100644
index 0000000..b599a66
--- /dev/null
+++ b/drivers/gpio/gpio-tps68470.c
@@ -0,0 +1,185 @@
+/*
+ * GPIO driver for TPS68470 PMIC
+ *
+ * Copyright (C) 2017 Intel Corporation
+ * Authors:
+ * Antti Laakso <antti.laakso@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/gpio.h>
+#include <linux/gpio/machine.h>
+#include <linux/mfd/tps68470.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define TPS68470_N_LOGIC_OUTPUT	3
+#define TPS68470_N_REGULAR_GPIO	7
+#define TPS68470_N_GPIO	(TPS68470_N_LOGIC_OUTPUT + TPS68470_N_REGULAR_GPIO)
+
+struct tps68470_gpio_data {
+	struct tps68470 *tps68470;
+	struct gpio_chip gc;
+};
+
+static inline struct tps68470_gpio_data *to_gpio_data(struct gpio_chip *gpiochp)
+{
+	return container_of(gpiochp, struct tps68470_gpio_data, gc);
+}
+
+static int tps68470_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct tps68470_gpio_data *tps68470_gpio = to_gpio_data(gc);
+	struct tps68470 *tps = tps68470_gpio->tps68470;
+	unsigned int reg = TPS68470_REG_GPDO;
+	int val, ret;
+
+	if (offset >= TPS68470_N_REGULAR_GPIO) {
+		offset -= TPS68470_N_REGULAR_GPIO;
+		reg = TPS68470_REG_SGPO;
+	}
+
+	ret = tps68470_reg_read(tps, reg, &val);
+	if (ret) {
+		dev_err(tps->dev, "reg 0x%x read failed\n", TPS68470_REG_SGPO);
+		return ret;
+	}
+	return !!(val & BIT(offset));
+}
+
+static void tps68470_gpio_set(struct gpio_chip *gc, unsigned int offset,
+				int value)
+{
+	struct tps68470_gpio_data *tps68470_gpio = to_gpio_data(gc);
+	struct tps68470 *tps = tps68470_gpio->tps68470;
+	unsigned int reg = TPS68470_REG_GPDO;
+
+	if (offset >= TPS68470_N_REGULAR_GPIO) {
+		reg = TPS68470_REG_SGPO;
+		offset -= TPS68470_N_REGULAR_GPIO;
+	}
+
+	tps68470_update_bits(tps, reg, BIT(offset), value ? BIT(offset) : 0);
+}
+
+static int tps68470_gpio_output(struct gpio_chip *gc, unsigned int offset,
+				int value)
+{
+	struct tps68470_gpio_data *tps68470_gpio = to_gpio_data(gc);
+	struct tps68470 *tps = tps68470_gpio->tps68470;
+
+	/* rest are always outputs */
+	if (offset >= TPS68470_N_REGULAR_GPIO)
+		return 0;
+
+	/* Set the initial value */
+	tps68470_gpio_set(gc, offset, value);
+
+	return tps68470_update_bits(tps, TPS68470_GPIO_CTL_REG_A(offset),
+				 TPS68470_GPIO_MODE_MASK,
+				 TPS68470_GPIO_MODE_OUT_CMOS);
+}
+
+static int tps68470_gpio_input(struct gpio_chip *gc, unsigned int offset)
+{
+	struct tps68470_gpio_data *tps68470_gpio = to_gpio_data(gc);
+	struct tps68470 *tps = tps68470_gpio->tps68470;
+
+	/* rest are always outputs */
+	if (offset >= TPS68470_N_REGULAR_GPIO)
+		return -EINVAL;
+
+	return tps68470_update_bits(tps, TPS68470_GPIO_CTL_REG_A(offset),
+				   TPS68470_GPIO_MODE_MASK, 0x00);
+}
+
+struct gpiod_lookup_table gpios_table = {
+	.dev_id = NULL,
+	.table = {
+		  GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0", GPIO_ACTIVE_HIGH),
+		  GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1", GPIO_ACTIVE_HIGH),
+		  GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2", GPIO_ACTIVE_HIGH),
+		  GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3", GPIO_ACTIVE_HIGH),
+		  GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4", GPIO_ACTIVE_HIGH),
+		  GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5", GPIO_ACTIVE_HIGH),
+		  GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6", GPIO_ACTIVE_HIGH),
+		  GPIO_LOOKUP("tps68470-gpio", 7, "s_enable", GPIO_ACTIVE_HIGH),
+		  GPIO_LOOKUP("tps68470-gpio", 8, "s_idle", GPIO_ACTIVE_HIGH),
+		  GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn", GPIO_ACTIVE_HIGH),
+		  {},
+	},
+};
+
+static int tps68470_gpio_probe(struct platform_device *pdev)
+{
+	struct tps68470 *tps68470 = dev_get_drvdata(pdev->dev.parent);
+	struct tps68470_gpio_data *tps68470_gpio;
+	int i, ret;
+
+	tps68470_gpio = devm_kzalloc(&pdev->dev, sizeof(*tps68470_gpio),
+				     GFP_KERNEL);
+	if (!tps68470_gpio)
+		return -ENOMEM;
+
+	tps68470_gpio->tps68470 = tps68470;
+	tps68470_gpio->gc.label = "tps68470-gpio";
+	tps68470_gpio->gc.owner = THIS_MODULE;
+	tps68470_gpio->gc.direction_input = tps68470_gpio_input;
+	tps68470_gpio->gc.direction_output = tps68470_gpio_output;
+	tps68470_gpio->gc.get = tps68470_gpio_get;
+	tps68470_gpio->gc.set = tps68470_gpio_set;
+	tps68470_gpio->gc.can_sleep = true;
+	tps68470_gpio->gc.ngpio = TPS68470_N_GPIO;
+	tps68470_gpio->gc.base = -1;
+	tps68470_gpio->gc.parent = &pdev->dev;
+
+	ret = gpiochip_add(&tps68470_gpio->gc);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to register gpio_chip: %d\n", ret);
+		return ret;
+	}
+
+	gpiod_add_lookup_table(&gpios_table);
+
+	platform_set_drvdata(pdev, tps68470_gpio);
+
+	/*
+	 * Initialize all the GPIOs to 0, just to make sure all
+	 * GPIOs start with known default values. This protects against
+	 * any GPIOs getting set with a value of 1, after TPS68470 reset
+	 */
+	for (i = 0; i < tps68470_gpio->gc.ngpio; i++)
+		tps68470_gpio_set(&tps68470_gpio->gc, i, 0);
+
+	return ret;
+}
+
+static int tps68470_gpio_remove(struct platform_device *pdev)
+{
+	struct tps68470_gpio_data *tps68470_gpio = platform_get_drvdata(pdev);
+
+	gpiod_remove_lookup_table(&gpios_table);
+	gpiochip_remove(&tps68470_gpio->gc);
+
+	return 0;
+}
+
+static struct platform_driver tps68470_gpio_driver = {
+	.driver = {
+		   .name = "tps68470-gpio",
+	},
+	.probe = tps68470_gpio_probe,
+	.remove = tps68470_gpio_remove,
+};
+
+builtin_platform_driver(tps68470_gpio_driver)
-- 
1.9.1

Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Andy Shevchenko]

On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.mani@intel.com> wrote:
> This patch adds support for TPS68470 GPIOs.
> There are 7 GPIOs and a few sensor related GPIOs.
> These GPIOs can be requested and configured as
> appropriate.

Besides my below comments, just put it here that I recommended earlier
to provide 2 GPIO chips (one per bank of GPIOs).
It's up to Linus to decide since you didn't follow the recommendation.

> +#include <linux/gpio.h>
> +#include <linux/gpio/machine.h>

These shouldn't be in the driver.
Instead use
#include <linux/gpio/driver.h>

> +#include <linux/mfd/tps68470.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>

> +       if (offset >= TPS68470_N_REGULAR_GPIO) {
> +               offset -= TPS68470_N_REGULAR_GPIO;
> +               reg = TPS68470_REG_SGPO;
> +       }

Two GPIO chips makes this gone.

> +struct gpiod_lookup_table gpios_table = {
> +       .dev_id = NULL,
> +       .table = {
> +                 GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0", GPIO_ACTIVE_HIGH),
> +                 GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1", GPIO_ACTIVE_HIGH),
> +                 GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2", GPIO_ACTIVE_HIGH),
> +                 GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3", GPIO_ACTIVE_HIGH),
> +                 GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4", GPIO_ACTIVE_HIGH),
> +                 GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5", GPIO_ACTIVE_HIGH),
> +                 GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6", GPIO_ACTIVE_HIGH),
> +                 GPIO_LOOKUP("tps68470-gpio", 7, "s_enable", GPIO_ACTIVE_HIGH),
> +                 GPIO_LOOKUP("tps68470-gpio", 8, "s_idle", GPIO_ACTIVE_HIGH),
> +                 GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn", GPIO_ACTIVE_HIGH),
> +                 {},
> +       },
> +};

This doesn't belong to the driver.

> +static int tps68470_gpio_probe(struct platform_device *pdev)
> +{
> +       struct tps68470 *tps68470 = dev_get_drvdata(pdev->dev.parent);
> +       struct tps68470_gpio_data *tps68470_gpio;

> +       int i, ret;

unsingned int i;

> +       ret = gpiochip_add(&tps68470_gpio->gc);

devm_ ?

> +       gpiod_add_lookup_table(&gpios_table);

Doesn't belong to the driver either.
I suppose it's a part of MFD (patch 1)

> +       /*
> +        * Initialize all the GPIOs to 0, just to make sure all
> +        * GPIOs start with known default values. This protects against
> +        * any GPIOs getting set with a value of 1, after TPS68470 reset

So, this is hardware bug. Right? Or misconfiguration of the chip we may avoid?

> +        */
> +       for (i = 0; i < tps68470_gpio->gc.ngpio; i++)
> +               tps68470_gpio_set(&tps68470_gpio->gc, i, 0);
> +
> +       return ret;
> +}

> +
> +static int tps68470_gpio_remove(struct platform_device *pdev)
> +{
> +       struct tps68470_gpio_data *tps68470_gpio = platform_get_drvdata(pdev);
> +
> +       gpiod_remove_lookup_table(&gpios_table);
> +       gpiochip_remove(&tps68470_gpio->gc);
> +
> +       return 0;
> +}

Should gone after devm_ in use.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Andy,

Thanks for the reviews and patience.

> 
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.mani@intel.com>
> wrote:
> > This patch adds support for TPS68470 GPIOs.
> > There are 7 GPIOs and a few sensor related GPIOs.
> > These GPIOs can be requested and configured as appropriate.
> 
> Besides my below comments, just put it here that I recommended earlier to
> provide 2 GPIO chips (one per bank of GPIOs).
> It's up to Linus to decide since you didn't follow the recommendation.
> 

Ack.
Did you mean to add this in Kconfig or this source file?

Here's some more details on these GPIOs.
Each of these 7 GPIOs has 2 registers to control the mode, level, drive strength, polarity, hysteresis control among other things. Also there are GPDI and GPDO registers to control the input and output values of these 7 GPIOs. These GPIOs are numbered 0 through 6.
The remaining 3 GPIOs are more of special purpose GPIOs that are output only, with one register to control all of their output values and drive strengths. These GPIOs are named with a special purpose (ENABLE, IDLE and RESET of the sensor).

> > +#include <linux/gpio.h>
> > +#include <linux/gpio/machine.h>
> 
> These shouldn't be in the driver.
> Instead use
> #include <linux/gpio/driver.h>
> 

Ack

> > +#include <linux/mfd/tps68470.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> 
> > +       if (offset >= TPS68470_N_REGULAR_GPIO) {
> > +               offset -= TPS68470_N_REGULAR_GPIO;
> > +               reg = TPS68470_REG_SGPO;
> > +       }
> 
> Two GPIO chips makes this gone.
> 

Ack.
On a closer look, creating two GPIO chips makes things clearer. 
But, this comes at the cost of a new set of gpio_get/set, 
gpio_output/input functions for the new GPIO chip. This results in 
new code for the second GPIO chip, which is pretty much 
going to be the copy of first GPIO chip, except for initializing 
the "reg" variable with GPDO or SGPO register. If we decide to 
reuse the existing code of the first GPIO chip for the new/second 
GPIO chip, we would still need to add a check, which would be 
effectively the same as the existing code, with the only advantage 
of not having to initialize the "offset" variable (which holds the 
GPIO offset). Given the above, it seems ok to retain the existing 
model of a single chip with the adjustments for offset, reg 
variables per the GPIO offset, to keep the whole picture simple.

> > +struct gpiod_lookup_table gpios_table = {
> > +       .dev_id = NULL,
> > +       .table = {
> > +                 GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 7, "s_enable",
> GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 8, "s_idle", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn",
> GPIO_ACTIVE_HIGH),
> > +                 {},
> > +       },
> > +};
> 
> This doesn't belong to the driver.
> 

Ack
I have moved this code to the MFD driver

> > +static int tps68470_gpio_probe(struct platform_device *pdev) {
> > +       struct tps68470 *tps68470 = dev_get_drvdata(pdev->dev.parent);
> > +       struct tps68470_gpio_data *tps68470_gpio;
> 
> > +       int i, ret;
> 
> unsingned int i;
> 
> > +       ret = gpiochip_add(&tps68470_gpio->gc);
> 
> devm_ ?
> 

Ack

> > +       gpiod_add_lookup_table(&gpios_table);
> 
> Doesn't belong to the driver either.
> I suppose it's a part of MFD (patch 1)
> 

Ack

> > +       /*
> > +        * Initialize all the GPIOs to 0, just to make sure all
> > +        * GPIOs start with known default values. This protects against
> > +        * any GPIOs getting set with a value of 1, after TPS68470
> > + reset
> 
> So, this is hardware bug. Right? Or misconfiguration of the chip we may avoid?
> 

The tps68470 PMIC upon reset, does not have the "power on reset" values.
We just initialize the GPIOs with their known default values.

> > +        */
> > +       for (i = 0; i < tps68470_gpio->gc.ngpio; i++)
> > +               tps68470_gpio_set(&tps68470_gpio->gc, i, 0);
> > +
> > +       return ret;
> > +}
> 
> > +
> > +static int tps68470_gpio_remove(struct platform_device *pdev) {
> > +       struct tps68470_gpio_data *tps68470_gpio =
> > +platform_get_drvdata(pdev);
> > +
> > +       gpiod_remove_lookup_table(&gpios_table);
> > +       gpiochip_remove(&tps68470_gpio->gc);
> > +
> > +       return 0;
> > +}
> 
> Should gone after devm_ in use.
> 

Ack

Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Sakari Ailus]

Hi Rajmohan and Andy,

On Sun, Jun 11, 2017 at 03:49:18AM +0000, Mani, Rajmohan wrote:
> Hi Andy,
> 
> Thanks for the reviews and patience.
> 
> > 
> > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.mani@intel.com>
> > wrote:
> > > This patch adds support for TPS68470 GPIOs.
> > > There are 7 GPIOs and a few sensor related GPIOs.
> > > These GPIOs can be requested and configured as appropriate.
> > 
> > Besides my below comments, just put it here that I recommended earlier to
> > provide 2 GPIO chips (one per bank of GPIOs).
> > It's up to Linus to decide since you didn't follow the recommendation.
> > 
> 
> Ack.
> Did you mean to add this in Kconfig or this source file?
> 
> Here's some more details on these GPIOs.
> Each of these 7 GPIOs has 2 registers to control the mode, level, drive strength, polarity, hysteresis control among other things. Also there are GPDI and GPDO registers to control the input and output values of these 7 GPIOs. These GPIOs are numbered 0 through 6.
> The remaining 3 GPIOs are more of special purpose GPIOs that are output only, with one register to control all of their output values and drive strengths. These GPIOs are named with a special purpose (ENABLE, IDLE and RESET of the sensor).
> 
> > > +#include <linux/gpio.h>
> > > +#include <linux/gpio/machine.h>
> > 
> > These shouldn't be in the driver.
> > Instead use
> > #include <linux/gpio/driver.h>
> > 
> 
> Ack
> 
> > > +#include <linux/mfd/tps68470.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > 
> > > +       if (offset >= TPS68470_N_REGULAR_GPIO) {
> > > +               offset -= TPS68470_N_REGULAR_GPIO;
> > > +               reg = TPS68470_REG_SGPO;
> > > +       }
> > 
> > Two GPIO chips makes this gone.

Again, I'm not really worried about this driver, but the ACPI tables. How
does the difference show there?

The outputs (s_enable, s_idle and s_resetn) are not numbered in the
documentation. There grouped, though, but the order in that grouping varies.

> > > +struct gpiod_lookup_table gpios_table = {
> > > +       .dev_id = NULL,
> > > +       .table = {
> > > +                 GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0", GPIO_ACTIVE_HIGH),
> > > +                 GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1", GPIO_ACTIVE_HIGH),
> > > +                 GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2", GPIO_ACTIVE_HIGH),
> > > +                 GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3", GPIO_ACTIVE_HIGH),
> > > +                 GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4", GPIO_ACTIVE_HIGH),
> > > +                 GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5", GPIO_ACTIVE_HIGH),
> > > +                 GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6", GPIO_ACTIVE_HIGH),
> > > +                 GPIO_LOOKUP("tps68470-gpio", 7, "s_enable",
> > GPIO_ACTIVE_HIGH),
> > > +                 GPIO_LOOKUP("tps68470-gpio", 8, "s_idle", GPIO_ACTIVE_HIGH),
> > > +                 GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn",
> > GPIO_ACTIVE_HIGH),
> > > +                 {},
> > > +       },
> > > +};
> > 
> > This doesn't belong to the driver.
> > 
> 
> Ack
> I have moved this code to the MFD driver

This information should come from the ACPI tables, it should not be present
in drivers. Why do you need it?

> > > +       /*
> > > +        * Initialize all the GPIOs to 0, just to make sure all
> > > +        * GPIOs start with known default values. This protects against
> > > +        * any GPIOs getting set with a value of 1, after TPS68470
> > > + reset
> > 
> > So, this is hardware bug. Right? Or misconfiguration of the chip we may avoid?
> > 
> 
> The tps68470 PMIC upon reset, does not have the "power on reset" values.
> We just initialize the GPIOs with their known default values.

If that was the case, I bet you could expect interesting behaviour from the
hardware connected to these pins.

For what it's worth, the chip documentation states that the reset value for
the SGPO and GPDO registers is zero, as well as that GPIOs are configured as
input and the reset value of the s_* outputs is low.

In other words, I don't think that explicitly setting the values to zero has
an effect.

-- 
Regards,

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

Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Andy Shevchenko]

On Sun, Jun 11, 2017 at 2:30 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> On Sun, Jun 11, 2017 at 03:49:18AM +0000, Mani, Rajmohan wrote:
>> > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.mani@intel.com>
>> > wrote:

>> > Besides my below comments, just put it here that I recommended earlier to
>> > provide 2 GPIO chips (one per bank of GPIOs).
>> > It's up to Linus to decide since you didn't follow the recommendation.

>> Did you mean to add this in Kconfig or this source file?
>>
>> Here's some more details on these GPIOs.
>> Each of these 7 GPIOs has 2 registers to control the mode, level, drive strength, polarity, hysteresis control among other things. Also there are GPDI and GPDO registers to control the input and output values of these 7 GPIOs. These GPIOs are numbered 0 through 6.
>> The remaining 3 GPIOs are more of special purpose GPIOs that are output only, with one register to control all of their output values and drive strengths. These GPIOs are named with a special purpose (ENABLE, IDLE and RESET of the sensor).

>> > > +#include <linux/mfd/tps68470.h>
>> > > +#include <linux/module.h>
>> > > +#include <linux/platform_device.h>
>> >
>> > > +       if (offset >= TPS68470_N_REGULAR_GPIO) {
>> > > +               offset -= TPS68470_N_REGULAR_GPIO;
>> > > +               reg = TPS68470_REG_SGPO;
>> > > +       }
>> >
>> > Two GPIO chips makes this gone.
>
> Again, I'm not really worried about this driver, but the ACPI tables. How
> does the difference show there?

Same way. You will have common numbering over the chip [0, 9]. It will
be just an abstraction inside the driver.

> The outputs (s_enable, s_idle and s_resetn) are not numbered in the
> documentation. There grouped, though, but the order in that grouping varies.

I don't get this. You are telling that the property of "always output"
can be assigned to any 3 out of 10?
Above states the opposite, so, it's clear to me that abstraction of 2
GPIO chips over 1 can be utilized here.

>> > > +struct gpiod_lookup_table gpios_table = {
>> > > +       .dev_id = NULL,
>> > > +       .table = {
>> > > +                 GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0", GPIO_ACTIVE_HIGH),
>> > > +                 GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1", GPIO_ACTIVE_HIGH),
>> > > +                 GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2", GPIO_ACTIVE_HIGH),
>> > > +                 GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3", GPIO_ACTIVE_HIGH),
>> > > +                 GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4", GPIO_ACTIVE_HIGH),
>> > > +                 GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5", GPIO_ACTIVE_HIGH),
>> > > +                 GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6", GPIO_ACTIVE_HIGH),
>> > > +                 GPIO_LOOKUP("tps68470-gpio", 7, "s_enable",
>> > GPIO_ACTIVE_HIGH),
>> > > +                 GPIO_LOOKUP("tps68470-gpio", 8, "s_idle", GPIO_ACTIVE_HIGH),
>> > > +                 GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn",
>> > GPIO_ACTIVE_HIGH),
>> > > +                 {},
>> > > +       },
>> > > +};
>> >
>> > This doesn't belong to the driver.

>> I have moved this code to the MFD driver
>
> This information should come from the ACPI tables, it should not be present
> in drivers. Why do you need it?

+1 (asked same in review of new version)

>> > > +       /*
>> > > +        * Initialize all the GPIOs to 0, just to make sure all
>> > > +        * GPIOs start with known default values. This protects against
>> > > +        * any GPIOs getting set with a value of 1, after TPS68470
>> > > + reset
>> >
>> > So, this is hardware bug. Right? Or misconfiguration of the chip we may avoid?

>> The tps68470 PMIC upon reset, does not have the "power on reset" values.
>> We just initialize the GPIOs with their known default values.
>
> If that was the case, I bet you could expect interesting behaviour from the
> hardware connected to these pins.

+1. I don't believe the hardware / firmware doesn't care about clear
and always predictable state.

> For what it's worth, the chip documentation states that the reset value for
> the SGPO and GPDO registers is zero, as well as that GPIOs are configured as
> input and the reset value of the s_* outputs is low.
>
> In other words, I don't think that explicitly setting the values to zero has
> an effect.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Sakari Ailus]

Hi Andy,

On Sun, Jun 11, 2017 at 04:40:16PM +0300, Andy Shevchenko wrote:
> On Sun, Jun 11, 2017 at 2:30 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > On Sun, Jun 11, 2017 at 03:49:18AM +0000, Mani, Rajmohan wrote:
> >> > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.mani@intel.com>
> >> > wrote:
> 
> >> > Besides my below comments, just put it here that I recommended earlier to
> >> > provide 2 GPIO chips (one per bank of GPIOs).
> >> > It's up to Linus to decide since you didn't follow the recommendation.
> 
> >> Did you mean to add this in Kconfig or this source file?
> >>
> >> Here's some more details on these GPIOs.
> >> Each of these 7 GPIOs has 2 registers to control the mode, level, drive strength, polarity, hysteresis control among other things. Also there are GPDI and GPDO registers to control the input and output values of these 7 GPIOs. These GPIOs are numbered 0 through 6.
> >> The remaining 3 GPIOs are more of special purpose GPIOs that are output only, with one register to control all of their output values and drive strengths. These GPIOs are named with a special purpose (ENABLE, IDLE and RESET of the sensor).
> 
> >> > > +#include <linux/mfd/tps68470.h>
> >> > > +#include <linux/module.h>
> >> > > +#include <linux/platform_device.h>
> >> >
> >> > > +       if (offset >= TPS68470_N_REGULAR_GPIO) {
> >> > > +               offset -= TPS68470_N_REGULAR_GPIO;
> >> > > +               reg = TPS68470_REG_SGPO;
> >> > > +       }
> >> >
> >> > Two GPIO chips makes this gone.
> >
> > Again, I'm not really worried about this driver, but the ACPI tables. How
> > does the difference show there?
> 
> Same way. You will have common numbering over the chip [0, 9]. It will
> be just an abstraction inside the driver.

Oh, in that case that should be a non-issue.

> 
> > The outputs (s_enable, s_idle and s_resetn) are not numbered in the
> > documentation. There grouped, though, but the order in that grouping varies.
> 
> I don't get this. You are telling that the property of "always output"
> can be assigned to any 3 out of 10?

No, I'm telling you that the three (s_enable, s_idle and s_resetn) cannot be
configured as inputs --- instead they're always outputs. That's how the
hardware is implemented.

> Above states the opposite, so, it's clear to me that abstraction of 2
> GPIO chips over 1 can be utilized here.

Sounds fine to me, taken that this does not add complications to ACPI
tables.

-- 
Regards,

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

Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Andy Shevchenko]

On Sun, Jun 11, 2017 at 7:50 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> On Sun, Jun 11, 2017 at 04:40:16PM +0300, Andy Shevchenko wrote:
>> On Sun, Jun 11, 2017 at 2:30 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:

>> > Again, I'm not really worried about this driver, but the ACPI tables. How
>> > does the difference show there?
>>
>> Same way. You will have common numbering over the chip [0, 9]. It will
>> be just an abstraction inside the driver.
>
> Oh, in that case that should be a non-issue.

>> Above states the opposite, so, it's clear to me that abstraction of 2
>> GPIO chips over 1 can be utilized here.
>
> Sounds fine to me, taken that this does not add complications to ACPI
> tables.

They just need to share the same ACPI_HANDLE (it might require to do
this in generic way in gpiolib) and have a continuous numbering (easy
to achieve with carefully chosen bases).

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Andy, Sakari,

> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs
> 
> On Sun, Jun 11, 2017 at 7:50 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > On Sun, Jun 11, 2017 at 04:40:16PM +0300, Andy Shevchenko wrote:
> >> On Sun, Jun 11, 2017 at 2:30 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> 
> >> > Again, I'm not really worried about this driver, but the ACPI
> >> > tables. How does the difference show there?
> >>
> >> Same way. You will have common numbering over the chip [0, 9]. It
> >> will be just an abstraction inside the driver.
> >
> > Oh, in that case that should be a non-issue.
> 
> >> Above states the opposite, so, it's clear to me that abstraction of 2
> >> GPIO chips over 1 can be utilized here.
> >
> > Sounds fine to me, taken that this does not add complications to ACPI
> > tables.
> 
> They just need to share the same ACPI_HANDLE (it might require to do this in
> generic way in gpiolib) and have a continuous numbering (easy to achieve with
> carefully chosen bases).
> 

Few clarifications...

Are you implying new kernel changes are needed in gpiolib to accommodate 2 GPIO chips?
Does it need changes in platform firmware or is it expected to work just with the gpiolib changes that you described above?

Thanks
Raj

Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Andy Shevchenko]

On Mon, Jun 12, 2017 at 12:17 PM, Mani, Rajmohan
<rajmohan.mani@intel.com> wrote:
>> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs
>> On Sun, Jun 11, 2017 at 7:50 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>> > On Sun, Jun 11, 2017 at 04:40:16PM +0300, Andy Shevchenko wrote:
>> >> On Sun, Jun 11, 2017 at 2:30 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:

>> >> > Again, I'm not really worried about this driver, but the ACPI
>> >> > tables. How does the difference show there?
>> >>
>> >> Same way. You will have common numbering over the chip [0, 9]. It
>> >> will be just an abstraction inside the driver.

>> > Sounds fine to me, taken that this does not add complications to ACPI
>> > tables.
>>
>> They just need to share the same ACPI_HANDLE (it might require to do this in
>> generic way in gpiolib) and have a continuous numbering (easy to achieve with
>> carefully chosen bases).

> Few clarifications...
>
> Are you implying new kernel changes are needed in gpiolib to accommodate 2 GPIO chips?

Might be needed. It should be investigated first.
In any case it would be somelike oneliner.

> Does it need changes in platform firmware or is it expected to work just with the gpiolib changes that you described above?

No firmware changes are implied.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Sakari,

> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs
> 
> Hi Rajmohan and Andy,
> 
> On Sun, Jun 11, 2017 at 03:49:18AM +0000, Mani, Rajmohan wrote:
> > Hi Andy,
> >
> > Thanks for the reviews and patience.
> >
> > >
> > > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani
> > > <rajmohan.mani@intel.com>
> > > wrote:
> > > > This patch adds support for TPS68470 GPIOs.
> > > > There are 7 GPIOs and a few sensor related GPIOs.
> > > > These GPIOs can be requested and configured as appropriate.
> > >
> > > Besides my below comments, just put it here that I recommended
> > > earlier to provide 2 GPIO chips (one per bank of GPIOs).
> > > It's up to Linus to decide since you didn't follow the recommendation.
> > >
> >
> > Ack.
> > Did you mean to add this in Kconfig or this source file?
> >
> > Here's some more details on these GPIOs.
> > Each of these 7 GPIOs has 2 registers to control the mode, level, drive
> strength, polarity, hysteresis control among other things. Also there are GPDI
> and GPDO registers to control the input and output values of these 7 GPIOs.
> These GPIOs are numbered 0 through 6.
> > The remaining 3 GPIOs are more of special purpose GPIOs that are output
> only, with one register to control all of their output values and drive strengths.
> These GPIOs are named with a special purpose (ENABLE, IDLE and RESET of the
> sensor).
> >
> > > > +#include <linux/gpio.h>
> > > > +#include <linux/gpio/machine.h>
> > >
> > > These shouldn't be in the driver.
> > > Instead use
> > > #include <linux/gpio/driver.h>
> > >
> >
> > Ack
> >
> > > > +#include <linux/mfd/tps68470.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/platform_device.h>
> > >
> > > > +       if (offset >= TPS68470_N_REGULAR_GPIO) {
> > > > +               offset -= TPS68470_N_REGULAR_GPIO;
> > > > +               reg = TPS68470_REG_SGPO;
> > > > +       }
> > >
> > > Two GPIO chips makes this gone.
> 
> Again, I'm not really worried about this driver, but the ACPI tables. How does
> the difference show there?
> 
> The outputs (s_enable, s_idle and s_resetn) are not numbered in the
> documentation. There grouped, though, but the order in that grouping varies.
> 
> > > > +struct gpiod_lookup_table gpios_table = {
> > > > +       .dev_id = NULL,
> > > > +       .table = {
> > > > +                 GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0",
> GPIO_ACTIVE_HIGH),
> > > > +                 GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1",
> GPIO_ACTIVE_HIGH),
> > > > +                 GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2",
> GPIO_ACTIVE_HIGH),
> > > > +                 GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3",
> GPIO_ACTIVE_HIGH),
> > > > +                 GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4",
> GPIO_ACTIVE_HIGH),
> > > > +                 GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5",
> GPIO_ACTIVE_HIGH),
> > > > +                 GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6",
> GPIO_ACTIVE_HIGH),
> > > > +                 GPIO_LOOKUP("tps68470-gpio", 7, "s_enable",
> > > GPIO_ACTIVE_HIGH),
> > > > +                 GPIO_LOOKUP("tps68470-gpio", 8, "s_idle",
> GPIO_ACTIVE_HIGH),
> > > > +                 GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn",
> > > GPIO_ACTIVE_HIGH),
> > > > +                 {},
> > > > +       },
> > > > +};
> > >
> > > This doesn't belong to the driver.
> > >
> >
> > Ack
> > I have moved this code to the MFD driver
> 
> This information should come from the ACPI tables, it should not be present in
> drivers. Why do you need it?
> 

I have removed the GPIO lookup table code for now.

> > > > +       /*
> > > > +        * Initialize all the GPIOs to 0, just to make sure all
> > > > +        * GPIOs start with known default values. This protects against
> > > > +        * any GPIOs getting set with a value of 1, after TPS68470
> > > > + reset
> > >
> > > So, this is hardware bug. Right? Or misconfiguration of the chip we may
> avoid?
> > >
> >
> > The tps68470 PMIC upon reset, does not have the "power on reset" values.
> > We just initialize the GPIOs with their known default values.
> 
> If that was the case, I bet you could expect interesting behaviour from the
> hardware connected to these pins.
> 
> For what it's worth, the chip documentation states that the reset value for the
> SGPO and GPDO registers is zero, as well as that GPIOs are configured as input
> and the reset value of the s_* outputs is low.
> 
> In other words, I don't think that explicitly setting the values to zero has an
> effect.
>

For now, I have removed this code that sets the GPIOs to zeros. If there are enough findings / reasons, this code may come back.

Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Linus Walleij]

On Tue, Jun 6, 2017 at 1:55 PM, Rajmohan Mani <rajmohan.mani@intel.com> wrote:

> This patch adds support for TPS68470 GPIOs.
> There are 7 GPIOs and a few sensor related GPIOs.
> These GPIOs can be requested and configured as
> appropriate.
>
> Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>

Same comments as Andy already sent, plus:

> +static inline struct tps68470_gpio_data *to_gpio_data(struct gpio_chip *gpiochp)
> +{
> +       return container_of(gpiochp, struct tps68470_gpio_data, gc);
> +}

Please use the data pointe inside gpio_chip.

struct tps68470_gpio_data *foo = gpiochip_get_data(gc);

> +       ret = tps68470_reg_read(tps, reg, &val);
(...)
> +       tps68470_update_bits(tps, reg, BIT(offset), value ? BIT(offset) : 0);
(...)
> +       return tps68470_update_bits(tps, TPS68470_GPIO_CTL_REG_A(offset),
> +                                TPS68470_GPIO_MODE_MASK,
> +                                TPS68470_GPIO_MODE_OUT_CMOS);
(...)
> +       return tps68470_update_bits(tps, TPS68470_GPIO_CTL_REG_A(offset),
> +                                  TPS68470_GPIO_MODE_MASK, 0x00);

This looks like a reimplementation of regmap. Is it not possible
to create a regmap in the MFD driver and pass that around instead?

> +struct gpiod_lookup_table gpios_table = {
> +       .dev_id = NULL,
> +       .table = {
> +                 GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0", GPIO_ACTIVE_HIGH),
> +                 GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1", GPIO_ACTIVE_HIGH),
> +                 GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2", GPIO_ACTIVE_HIGH),
> +                 GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3", GPIO_ACTIVE_HIGH),
> +                 GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4", GPIO_ACTIVE_HIGH),
> +                 GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5", GPIO_ACTIVE_HIGH),
> +                 GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6", GPIO_ACTIVE_HIGH),
> +                 GPIO_LOOKUP("tps68470-gpio", 7, "s_enable", GPIO_ACTIVE_HIGH),
> +                 GPIO_LOOKUP("tps68470-gpio", 8, "s_idle", GPIO_ACTIVE_HIGH),
> +                 GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn", GPIO_ACTIVE_HIGH),
> +                 {},
> +       },
> +};

Hm that's why you include <linux/gpio/machine.h> I guess.

This warrants a big comment above it explaining why this is done like
that and what the use is inside any system with this chip mounted.

> +       tps68470_gpio->gc.label = "tps68470-gpio";
> +       tps68470_gpio->gc.owner = THIS_MODULE;
> +       tps68470_gpio->gc.direction_input = tps68470_gpio_input;
> +       tps68470_gpio->gc.direction_output = tps68470_gpio_output;

Please implement .get_direction()

> +       tps68470_gpio->gc.get = tps68470_gpio_get;
> +       tps68470_gpio->gc.set = tps68470_gpio_set;
> +       tps68470_gpio->gc.can_sleep = true;
> +       tps68470_gpio->gc.ngpio = TPS68470_N_GPIO;
> +       tps68470_gpio->gc.base = -1;
> +       tps68470_gpio->gc.parent = &pdev->dev;

Consider assigning the .names() array some sensible line names.

> +static int tps68470_gpio_remove(struct platform_device *pdev)
> +{
> +       struct tps68470_gpio_data *tps68470_gpio = platform_get_drvdata(pdev);
> +
> +       gpiod_remove_lookup_table(&gpios_table);
> +       gpiochip_remove(&tps68470_gpio->gc);

You can't use devm_gpiochip_add()?

Yours,
Linus Walleij

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Linus,

Thanks for the reviews.

> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: Friday, June 09, 2017 4:15 AM
> To: Mani, Rajmohan <rajmohan.mani@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; ACPI Devel
> Maling List <linux-acpi@vger.kernel.org>; Lee Jones <lee.jones@linaro.org>;
> Alexandre Courbot <gnurou@gmail.com>; Rafael J. Wysocki
> <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>
> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs
> 
> On Tue, Jun 6, 2017 at 1:55 PM, Rajmohan Mani <rajmohan.mani@intel.com>
> wrote:
> 
> > This patch adds support for TPS68470 GPIOs.
> > There are 7 GPIOs and a few sensor related GPIOs.
> > These GPIOs can be requested and configured as appropriate.
> >
> > Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
> 
> Same comments as Andy already sent, plus:
> 
> > +static inline struct tps68470_gpio_data *to_gpio_data(struct
> > +gpio_chip *gpiochp) {
> > +       return container_of(gpiochp, struct tps68470_gpio_data, gc); }
> 
> Please use the data pointe inside gpio_chip.
> 

This is not clear to me.
The driver already gets the data pointer inside gpio_chip (which is gpiochp)
Is the name gpiochp misleading? 
I had to get rid of "i" in gpiochip, to meet 80 char limit.

> struct tps68470_gpio_data *foo = gpiochip_get_data(gc);
> 
> > +       ret = tps68470_reg_read(tps, reg, &val);
> (...)
> > +       tps68470_update_bits(tps, reg, BIT(offset), value ?
> > + BIT(offset) : 0);
> (...)
> > +       return tps68470_update_bits(tps, TPS68470_GPIO_CTL_REG_A(offset),
> > +                                TPS68470_GPIO_MODE_MASK,
> > +                                TPS68470_GPIO_MODE_OUT_CMOS);
> (...)
> > +       return tps68470_update_bits(tps, TPS68470_GPIO_CTL_REG_A(offset),
> > +                                  TPS68470_GPIO_MODE_MASK, 0x00);
> 
> This looks like a reimplementation of regmap. Is it not possible to create a
> regmap in the MFD driver and pass that around instead?
> 

Ack
Will be addressed in v2 of this patch series.

> > +struct gpiod_lookup_table gpios_table = {
> > +       .dev_id = NULL,
> > +       .table = {
> > +                 GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 7, "s_enable",
> GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 8, "s_idle", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn",
> GPIO_ACTIVE_HIGH),
> > +                 {},
> > +       },
> > +};
> 
> Hm that's why you include <linux/gpio/machine.h> I guess.
> 

Ack

> This warrants a big comment above it explaining why this is done like that and
> what the use is inside any system with this chip mounted.
> 

Ack
I have added comments in the MFD driver, inside which the lookup table has moved.

> > +       tps68470_gpio->gc.label = "tps68470-gpio";
> > +       tps68470_gpio->gc.owner = THIS_MODULE;
> > +       tps68470_gpio->gc.direction_input = tps68470_gpio_input;
> > +       tps68470_gpio->gc.direction_output = tps68470_gpio_output;
> 
> Please implement .get_direction()
> 

Ack

> > +       tps68470_gpio->gc.get = tps68470_gpio_get;
> > +       tps68470_gpio->gc.set = tps68470_gpio_set;
> > +       tps68470_gpio->gc.can_sleep = true;
> > +       tps68470_gpio->gc.ngpio = TPS68470_N_GPIO;
> > +       tps68470_gpio->gc.base = -1;
> > +       tps68470_gpio->gc.parent = &pdev->dev;
> 
> Consider assigning the .names() array some sensible line names.
> 

Ack

> > +static int tps68470_gpio_remove(struct platform_device *pdev) {
> > +       struct tps68470_gpio_data *tps68470_gpio =
> > +platform_get_drvdata(pdev);
> > +
> > +       gpiod_remove_lookup_table(&gpios_table);
> > +       gpiochip_remove(&tps68470_gpio->gc);
> 
> You can't use devm_gpiochip_add()?
> 

Originally I couldn't, because the driver can not remove the lookup table upon exit/removal. I moved this code inside the MFD driver, which is modified to use .shutdown() to remove the lookup table, so it can use dev_mfd_* call.

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Linus,

> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

> > +static inline struct tps68470_gpio_data *to_gpio_data(struct
> > +gpio_chip *gpiochp) {
> > +       return container_of(gpiochp, struct tps68470_gpio_data, gc); }
> 
> Please use the data pointe inside gpio_chip.
> 

My bad. I completely missed the following line. Will fix it v3.

> struct tps68470_gpio_data *foo = gpiochip_get_data(gc);
> 

[PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Rajmohan Mani]

The Kabylake platform coreboot (Chrome OS equivalent of
BIOS) has defined 4 operation regions for the TI TPS68470 PMIC.
These operation regions are to enable/disable voltage
regulators, configure voltage regulators, enable/disable
clocks and to configure clocks.

This config adds ACPI operation region support for TI TPS68470 PMIC.
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 driver enables ACPI operation region support to control voltage
regulators and clocks for the TPS68470 PMIC.

Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
---
 drivers/acpi/Kconfig              |  12 +
 drivers/acpi/Makefile             |   2 +
 drivers/acpi/pmic/pmic_tps68470.c | 454 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 468 insertions(+)
 create mode 100644 drivers/acpi/pmic/pmic_tps68470.c

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 1ce52f8..218d22d 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -535,4 +535,16 @@ if ARM64
 source "drivers/acpi/arm64/Kconfig"
 endif
 
+config TPS68470_PMIC_OPREGION
+	bool "ACPI operation region support for TPS68470 PMIC"
+	help
+	  This config adds ACPI operation region support for TI TPS68470 PMIC.
+	  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 driver enables ACPI operation region support control voltage
+	  regulators and clocks.
+
+
 endif	# ACPI
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index b1aacfc..7113d05 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -106,6 +106,8 @@ obj-$(CONFIG_CHT_WC_PMIC_OPREGION) += pmic/intel_pmic_chtwc.o
 
 obj-$(CONFIG_ACPI_CONFIGFS)	+= acpi_configfs.o
 
+obj-$(CONFIG_TPS68470_PMIC_OPREGION)	+= pmic/pmic_tps68470.o
+
 video-objs			+= acpi_video.o video_detect.o
 obj-y				+= dptf/
 
diff --git a/drivers/acpi/pmic/pmic_tps68470.c b/drivers/acpi/pmic/pmic_tps68470.c
new file mode 100644
index 0000000..b2d608b
--- /dev/null
+++ b/drivers/acpi/pmic/pmic_tps68470.c
@@ -0,0 +1,454 @@
+/*
+ * TI TPS68470 PMIC operation region driver
+ *
+ * Copyright (C) 2017 Intel Corporation. All rights reserved.
+ * Author: Rajmohan Mani <rajmohan.mani@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * Based on drivers/acpi/pmic/intel_pmic* drivers
+ *
+ */
+
+#include <linux/acpi.h>
+#include <linux/mfd/tps68470.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+struct ti_pmic_table {
+	u32 address;		/* operation region address */
+	u32 reg;		/* corresponding register */
+	u32 bitmask;		/* bit mask for power, clock */
+};
+
+#define TI_PMIC_POWER_OPREGION_ID		0xB0
+#define TI_PMIC_VR_VAL_OPREGION_ID		0xB1
+#define TI_PMIC_CLOCK_OPREGION_ID		0xB2
+#define TI_PMIC_CLKFREQ_OPREGION_ID		0xB3
+
+struct ti_pmic_opregion {
+	struct mutex lock;
+	struct regmap *regmap;
+};
+
+#define S_IO_I2C_EN	(BIT(0) | BIT(1))
+
+static const struct ti_pmic_table power_table[] = {
+	{
+		.address = 0x00,
+		.reg = TPS68470_REG_S_I2C_CTL,
+		.bitmask = S_IO_I2C_EN,
+		/* S_I2C_CTL */
+	},
+	{
+		.address = 0x04,
+		.reg = TPS68470_REG_VCMCTL,
+		.bitmask = BIT(0),
+		/* VCMCTL */
+	},
+	{
+		.address = 0x08,
+		.reg = TPS68470_REG_VAUX1CTL,
+		.bitmask = BIT(0),
+		/* VAUX1_CTL */
+	},
+	{
+		.address = 0x0C,
+		.reg = TPS68470_REG_VAUX2CTL,
+		.bitmask = BIT(0),
+		/* VAUX2CTL */
+	},
+	{
+		.address = 0x10,
+		.reg = TPS68470_REG_VACTL,
+		.bitmask = BIT(0),
+		/* VACTL */
+	},
+	{
+		.address = 0x14,
+		.reg = TPS68470_REG_VDCTL,
+		.bitmask = BIT(0),
+		/* VDCTL */
+	},
+};
+
+/* Table to set voltage regulator value */
+static const struct ti_pmic_table vr_val_table[] = {
+	{
+		.address = 0x00,
+		.reg = TPS68470_REG_VSIOVAL,
+		.bitmask = TPS68470_VSIOVAL_IOVOLT_MASK,
+		/* TPS68470_REG_VSIOVAL */
+	},
+	{
+		.address = 0x04,
+		.reg = TPS68470_REG_VIOVAL,
+		.bitmask = TPS68470_VIOVAL_IOVOLT_MASK,
+		/* TPS68470_REG_VIOVAL */
+	},
+	{
+		.address = 0x08,
+		.reg = TPS68470_REG_VCMVAL,
+		.bitmask = TPS68470_VCMVAL_VCVOLT_MASK,
+		/* TPS68470_REG_VCMVAL */
+	},
+	{
+		.address = 0x0C,
+		.reg = TPS68470_REG_VAUX1VAL,
+		.bitmask = TPS68470_VAUX1VAL_AUX1VOLT_MASK,
+		/* TPS68470_REG_VAUX1VAL */
+	},
+	{
+		.address = 0x10,
+		.reg = TPS68470_REG_VAUX2VAL,
+		.bitmask = TPS68470_VAUX2VAL_AUX2VOLT_MASK,
+		/* TPS68470_REG_VAUX2VAL */
+	},
+	{
+		.address = 0x14,
+		.reg = TPS68470_REG_VAVAL,
+		.bitmask = TPS68470_VAVAL_AVOLT_MASK,
+		/* TPS68470_REG_VAVAL */
+	},
+	{
+		.address = 0x18,
+		.reg = TPS68470_REG_VDVAL,
+		.bitmask = TPS68470_VDVAL_DVOLT_MASK,
+		/* TPS68470_REG_VDVAL */
+	},
+};
+
+/* Table to configure clock frequency */
+static const struct ti_pmic_table clk_freq_table[] = {
+	{
+		.address = 0x00,
+		.reg = TPS68470_REG_POSTDIV2,
+		.bitmask = BIT(0) | BIT(1),
+		/* TPS68470_REG_POSTDIV2 */
+	},
+	{
+		.address = 0x04,
+		.reg = TPS68470_REG_BOOSTDIV,
+		.bitmask = 0x1F,
+		/* TPS68470_REG_BOOSTDIV */
+	},
+	{
+		.address = 0x08,
+		.reg = TPS68470_REG_BUCKDIV,
+		.bitmask = 0x0F,
+		/* TPS68470_REG_BUCKDIV */
+	},
+	{
+		.address = 0x0C,
+		.reg = TPS68470_REG_PLLSWR,
+		.bitmask = 0x13,
+		/* TPS68470_REG_PLLSWR */
+	},
+	{
+		.address = 0x10,
+		.reg = TPS68470_REG_XTALDIV,
+		.bitmask = 0xFF,
+		/* TPS68470_REG_XTALDIV */
+	},
+	{
+		.address = 0x14,
+		.reg = TPS68470_REG_PLLDIV,
+		.bitmask = 0xFF,
+		/* TPS68470_REG_PLLDIV */
+	},
+	{
+		.address = 0x18,
+		.reg = TPS68470_REG_POSTDIV,
+		.bitmask = 0x83,
+		/* TPS68470_REG_POSTDIV */
+	},
+};
+
+/* Table to configure and enable clocks */
+static const struct ti_pmic_table clk_table[] = {
+	{
+		.address = 0x00,
+		.reg = TPS68470_REG_PLLCTL,
+		.bitmask = 0xF5,
+		/* TPS68470_REG_PLLCTL */
+	},
+	{
+		.address = 0x04,
+		.reg = TPS68470_REG_PLLCTL2,
+		.bitmask = BIT(0),
+		/* TPS68470_REG_PLLCTL2 */
+	},
+	{
+		.address = 0x08,
+		.reg = TPS68470_REG_CLKCFG1,
+		.bitmask = TPS68470_CLKCFG1_MODE_A_MASK |
+			TPS68470_CLKCFG1_MODE_B_MASK,
+		/* TPS68470_REG_CLKCFG1 */
+	},
+	{
+		.address = 0x0C,
+		.reg = TPS68470_REG_CLKCFG2,
+		.bitmask = TPS68470_CLKCFG1_MODE_A_MASK |
+			TPS68470_CLKCFG1_MODE_B_MASK,
+		/* TPS68470_REG_CLKCFG2 */
+	},
+};
+
+static int pmic_get_reg_bit(u64 address, struct ti_pmic_table *table,
+			    int count, int *reg, int *bitmask)
+{
+	u64 i;
+
+	i = address / 4;
+
+	if (i >= count)
+		return -ENOENT;
+
+	if (!reg || !bitmask)
+		return -EINVAL;
+
+	*reg = table[i].reg;
+	*bitmask = table[i].bitmask;
+
+	return 0;
+}
+
+static int ti_tps68470_pmic_get_power(struct regmap *regmap, int reg,
+				       int bitmask, u64 *value)
+{
+	int data;
+
+	if (regmap_read(regmap, reg, &data))
+		return -EIO;
+
+	*value = (data & bitmask) ? 1 : 0;
+	return 0;
+}
+
+static int ti_tps68470_pmic_get_vr_val(struct regmap *regmap, int reg,
+				       int bitmask, u64 *value)
+{
+	int data;
+
+	if (regmap_read(regmap, reg, &data))
+		return -EIO;
+
+	*value = data & bitmask;
+	return 0;
+}
+
+static int ti_tps68470_pmic_get_clk(struct regmap *regmap, int reg,
+				       int bitmask, u64 *value)
+{
+	int data;
+
+	if (regmap_read(regmap, reg, &data))
+		return -EIO;
+
+	*value = (data & bitmask) ? 1 : 0;
+	return 0;
+}
+
+static int ti_tps68470_pmic_get_clk_freq(struct regmap *regmap, int reg,
+				       int bitmask, u64 *value)
+{
+	int data;
+
+	if (regmap_read(regmap, reg, &data))
+		return -EIO;
+
+	*value = data & bitmask;
+	return 0;
+}
+
+static int ti_tps68470_regmap_update_bits(struct regmap *regmap, int reg,
+					int bitmask, u64 value)
+{
+	return regmap_update_bits(regmap, reg, bitmask, value);
+}
+
+static acpi_status ti_pmic_common_handler(u32 function,
+					  acpi_physical_address address,
+					  u32 bits, u64 *value,
+					  void *handler_context,
+					  void *region_context,
+					  int (*get)(struct regmap *,
+						     int, int, u64 *),
+					  int (*update)(struct regmap *,
+							int, int, u64),
+					  struct ti_pmic_table *table,
+					  int table_size)
+{
+	struct ti_pmic_opregion *opregion = region_context;
+	struct regmap *regmap = opregion->regmap;
+	int reg, ret, bitmask;
+
+	if (bits != 32)
+		return AE_BAD_PARAMETER;
+
+	ret = pmic_get_reg_bit(address, table,
+				  table_size, &reg, &bitmask);
+	if (ret < 0)
+		return AE_BAD_PARAMETER;
+
+	if (function == ACPI_WRITE && (*value > bitmask))
+		return AE_BAD_PARAMETER;
+
+	mutex_lock(&opregion->lock);
+
+	ret = (function == ACPI_READ) ?
+		get(regmap, reg, bitmask, value) :
+		update(regmap, reg, bitmask, *value);
+
+	mutex_unlock(&opregion->lock);
+
+	return ret ? AE_ERROR : AE_OK;
+}
+
+static acpi_status ti_pmic_clk_freq_handler(u32 function,
+					    acpi_physical_address address,
+					    u32 bits, u64 *value,
+					    void *handler_context,
+					    void *region_context)
+{
+	return ti_pmic_common_handler(function, address, bits, value,
+				handler_context, region_context,
+				ti_tps68470_pmic_get_clk_freq,
+				ti_tps68470_regmap_update_bits,
+				(struct ti_pmic_table *) &clk_freq_table,
+				ARRAY_SIZE(clk_freq_table));
+}
+
+static acpi_status ti_pmic_clk_handler(u32 function,
+				       acpi_physical_address address, u32 bits,
+				       u64 *value, void *handler_context,
+				       void *region_context)
+{
+	return ti_pmic_common_handler(function, address, bits, value,
+				handler_context, region_context,
+				ti_tps68470_pmic_get_clk,
+				ti_tps68470_regmap_update_bits,
+				(struct ti_pmic_table *) &clk_table,
+				 ARRAY_SIZE(clk_table));
+}
+
+static acpi_status ti_pmic_vr_val_handler(u32 function,
+					  acpi_physical_address address,
+					  u32 bits, u64 *value,
+					  void *handler_context,
+					  void *region_context)
+{
+	return ti_pmic_common_handler(function, address, bits, value,
+				handler_context, region_context,
+				ti_tps68470_pmic_get_vr_val,
+				ti_tps68470_regmap_update_bits,
+				(struct ti_pmic_table *) &vr_val_table,
+				ARRAY_SIZE(vr_val_table));
+}
+
+static acpi_status ti_pmic_power_handler(u32 function,
+					 acpi_physical_address address,
+					 u32 bits, u64 *value,
+					 void *handler_context,
+					 void *region_context)
+{
+	if (bits != 32)
+		return AE_BAD_PARAMETER;
+
+	/* set/clear for bit 0, bits 0 and 1 together */
+	if (function == ACPI_WRITE &&
+	    !(*value == 0 || *value == 1 || *value == 3)) {
+		return AE_BAD_PARAMETER;
+	}
+
+	return ti_pmic_common_handler(function, address, bits, value,
+				handler_context, region_context,
+				ti_tps68470_pmic_get_power,
+				ti_tps68470_regmap_update_bits,
+				(struct ti_pmic_table *) &power_table,
+				ARRAY_SIZE(power_table));
+}
+
+static int ti_tps68470_pmic_opregion_probe(struct platform_device *pdev)
+{
+	struct tps68470 *pmic = dev_get_drvdata(pdev->dev.parent);
+	acpi_handle handle = ACPI_HANDLE(pdev->dev.parent);
+	struct device *dev = &pdev->dev;
+	struct ti_pmic_opregion *opregion;
+	acpi_status status;
+
+	if (!dev || !pmic->regmap) {
+		WARN(1, "dev or regmap is NULL\n");
+		return -EINVAL;
+	}
+
+	if (!handle) {
+		WARN(1, "acpi handle is NULL\n");
+		return -ENODEV;
+	}
+
+	opregion = devm_kzalloc(dev, sizeof(*opregion), GFP_KERNEL);
+	if (!opregion)
+		return -ENOMEM;
+
+	mutex_init(&opregion->lock);
+	opregion->regmap = pmic->regmap;
+
+	status = acpi_install_address_space_handler(handle,
+						    TI_PMIC_POWER_OPREGION_ID,
+						    ti_pmic_power_handler,
+						    NULL, opregion);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	status = acpi_install_address_space_handler(handle,
+						    TI_PMIC_VR_VAL_OPREGION_ID,
+						    ti_pmic_vr_val_handler,
+						    NULL, opregion);
+	if (ACPI_FAILURE(status))
+		goto out_remove_power_handler;
+
+	status = acpi_install_address_space_handler(handle,
+						    TI_PMIC_CLOCK_OPREGION_ID,
+						    ti_pmic_clk_handler,
+						    NULL, opregion);
+	if (ACPI_FAILURE(status))
+		goto out_remove_vr_val_handler;
+
+	status = acpi_install_address_space_handler(handle,
+						    TI_PMIC_CLKFREQ_OPREGION_ID,
+						    ti_pmic_clk_freq_handler,
+						    NULL, opregion);
+	if (ACPI_FAILURE(status))
+		goto out_remove_clk_handler;
+
+	return 0;
+
+out_remove_clk_handler:
+	acpi_remove_address_space_handler(handle, TI_PMIC_CLOCK_OPREGION_ID,
+					  ti_pmic_clk_handler);
+out_remove_vr_val_handler:
+	acpi_remove_address_space_handler(handle, TI_PMIC_VR_VAL_OPREGION_ID,
+					  ti_pmic_vr_val_handler);
+out_remove_power_handler:
+	acpi_remove_address_space_handler(handle, TI_PMIC_POWER_OPREGION_ID,
+					  ti_pmic_power_handler);
+	return -ENODEV;
+}
+
+static struct platform_driver ti_tps68470_pmic_opregion_driver = {
+	.probe = ti_tps68470_pmic_opregion_probe,
+	.driver = {
+		.name = "tps68470_pmic_opregion",
+	},
+};
+
+builtin_platform_driver(ti_tps68470_pmic_opregion_driver)
-- 
1.9.1

Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Andy Shevchenko]

+Cc Hans (that's why didn't delete anything from original mail, just
adding my comments).

Hans, if you have few minutes it would be appreciated to glance on the
below for some issues if any since you did pass quite a good quest
with other PMIC drivers.

On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.mani@intel.com> wrote:
> The Kabylake platform coreboot (Chrome OS equivalent of
> BIOS) has defined 4 operation regions for the TI TPS68470 PMIC.
> These operation regions are to enable/disable voltage
> regulators, configure voltage regulators, enable/disable
> clocks and to configure clocks.
>
> This config adds ACPI operation region support for TI TPS68470 PMIC.
> 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 driver enables ACPI operation region support to control voltage
> regulators and clocks for the TPS68470 PMIC.
>
> Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
> ---
>  drivers/acpi/Kconfig              |  12 +
>  drivers/acpi/Makefile             |   2 +

>  drivers/acpi/pmic/pmic_tps68470.c | 454 ++++++++++++++++++++++++++++++++++++++

Follow the pattern, please, I suppose
ti_pmic_tps68470.c

>  3 files changed, 468 insertions(+)
>  create mode 100644 drivers/acpi/pmic/pmic_tps68470.c
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 1ce52f8..218d22d 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -535,4 +535,16 @@ if ARM64
>  source "drivers/acpi/arm64/Kconfig"
>  endif
>
> +config TPS68470_PMIC_OPREGION
> +       bool "ACPI operation region support for TPS68470 PMIC"
> +       help
> +         This config adds ACPI operation region support for TI TPS68470 PMIC.
> +         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 driver enables ACPI operation region support control voltage
> +         regulators and clocks.
> +

> +

Extra line, remove.

>  endif  # ACPI
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index b1aacfc..7113d05 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -106,6 +106,8 @@ obj-$(CONFIG_CHT_WC_PMIC_OPREGION) += pmic/intel_pmic_chtwc.o
>
>  obj-$(CONFIG_ACPI_CONFIGFS)    += acpi_configfs.o
>
> +obj-$(CONFIG_TPS68470_PMIC_OPREGION)   += pmic/pmic_tps68470.o
> +
>  video-objs                     += acpi_video.o video_detect.o
>  obj-y                          += dptf/
>
> diff --git a/drivers/acpi/pmic/pmic_tps68470.c b/drivers/acpi/pmic/pmic_tps68470.c
> new file mode 100644
> index 0000000..b2d608b
> --- /dev/null
> +++ b/drivers/acpi/pmic/pmic_tps68470.c
> @@ -0,0 +1,454 @@
> +/*
> + * TI TPS68470 PMIC operation region driver
> + *
> + * Copyright (C) 2017 Intel Corporation. All rights reserved.
> + * Author: Rajmohan Mani <rajmohan.mani@intel.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version
> + * 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * Based on drivers/acpi/pmic/intel_pmic* drivers
> + *
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/mfd/tps68470.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +struct ti_pmic_table {
> +       u32 address;            /* operation region address */
> +       u32 reg;                /* corresponding register */
> +       u32 bitmask;            /* bit mask for power, clock */
> +};
> +
> +#define TI_PMIC_POWER_OPREGION_ID              0xB0
> +#define TI_PMIC_VR_VAL_OPREGION_ID             0xB1
> +#define TI_PMIC_CLOCK_OPREGION_ID              0xB2
> +#define TI_PMIC_CLKFREQ_OPREGION_ID            0xB3
> +
> +struct ti_pmic_opregion {
> +       struct mutex lock;
> +       struct regmap *regmap;
> +};
> +
> +#define S_IO_I2C_EN    (BIT(0) | BIT(1))
> +
> +static const struct ti_pmic_table power_table[] = {
> +       {
> +               .address = 0x00,
> +               .reg = TPS68470_REG_S_I2C_CTL,
> +               .bitmask = S_IO_I2C_EN,
> +               /* S_I2C_CTL */
> +       },
> +       {
> +               .address = 0x04,
> +               .reg = TPS68470_REG_VCMCTL,
> +               .bitmask = BIT(0),
> +               /* VCMCTL */
> +       },
> +       {
> +               .address = 0x08,
> +               .reg = TPS68470_REG_VAUX1CTL,
> +               .bitmask = BIT(0),
> +               /* VAUX1_CTL */
> +       },
> +       {
> +               .address = 0x0C,
> +               .reg = TPS68470_REG_VAUX2CTL,
> +               .bitmask = BIT(0),
> +               /* VAUX2CTL */
> +       },
> +       {
> +               .address = 0x10,
> +               .reg = TPS68470_REG_VACTL,
> +               .bitmask = BIT(0),
> +               /* VACTL */
> +       },
> +       {
> +               .address = 0x14,
> +               .reg = TPS68470_REG_VDCTL,
> +               .bitmask = BIT(0),
> +               /* VDCTL */
> +       },
> +};
> +
> +/* Table to set voltage regulator value */
> +static const struct ti_pmic_table vr_val_table[] = {
> +       {
> +               .address = 0x00,
> +               .reg = TPS68470_REG_VSIOVAL,
> +               .bitmask = TPS68470_VSIOVAL_IOVOLT_MASK,
> +               /* TPS68470_REG_VSIOVAL */
> +       },
> +       {
> +               .address = 0x04,
> +               .reg = TPS68470_REG_VIOVAL,
> +               .bitmask = TPS68470_VIOVAL_IOVOLT_MASK,
> +               /* TPS68470_REG_VIOVAL */
> +       },
> +       {
> +               .address = 0x08,
> +               .reg = TPS68470_REG_VCMVAL,
> +               .bitmask = TPS68470_VCMVAL_VCVOLT_MASK,
> +               /* TPS68470_REG_VCMVAL */
> +       },
> +       {
> +               .address = 0x0C,
> +               .reg = TPS68470_REG_VAUX1VAL,
> +               .bitmask = TPS68470_VAUX1VAL_AUX1VOLT_MASK,
> +               /* TPS68470_REG_VAUX1VAL */
> +       },
> +       {
> +               .address = 0x10,
> +               .reg = TPS68470_REG_VAUX2VAL,
> +               .bitmask = TPS68470_VAUX2VAL_AUX2VOLT_MASK,
> +               /* TPS68470_REG_VAUX2VAL */
> +       },
> +       {
> +               .address = 0x14,
> +               .reg = TPS68470_REG_VAVAL,
> +               .bitmask = TPS68470_VAVAL_AVOLT_MASK,
> +               /* TPS68470_REG_VAVAL */
> +       },
> +       {
> +               .address = 0x18,
> +               .reg = TPS68470_REG_VDVAL,
> +               .bitmask = TPS68470_VDVAL_DVOLT_MASK,
> +               /* TPS68470_REG_VDVAL */
> +       },
> +};
> +
> +/* Table to configure clock frequency */
> +static const struct ti_pmic_table clk_freq_table[] = {
> +       {
> +               .address = 0x00,
> +               .reg = TPS68470_REG_POSTDIV2,
> +               .bitmask = BIT(0) | BIT(1),
> +               /* TPS68470_REG_POSTDIV2 */
> +       },
> +       {
> +               .address = 0x04,
> +               .reg = TPS68470_REG_BOOSTDIV,
> +               .bitmask = 0x1F,
> +               /* TPS68470_REG_BOOSTDIV */
> +       },
> +       {
> +               .address = 0x08,
> +               .reg = TPS68470_REG_BUCKDIV,
> +               .bitmask = 0x0F,
> +               /* TPS68470_REG_BUCKDIV */
> +       },
> +       {
> +               .address = 0x0C,
> +               .reg = TPS68470_REG_PLLSWR,
> +               .bitmask = 0x13,
> +               /* TPS68470_REG_PLLSWR */
> +       },
> +       {
> +               .address = 0x10,
> +               .reg = TPS68470_REG_XTALDIV,
> +               .bitmask = 0xFF,
> +               /* TPS68470_REG_XTALDIV */
> +       },
> +       {
> +               .address = 0x14,
> +               .reg = TPS68470_REG_PLLDIV,
> +               .bitmask = 0xFF,
> +               /* TPS68470_REG_PLLDIV */
> +       },
> +       {
> +               .address = 0x18,
> +               .reg = TPS68470_REG_POSTDIV,
> +               .bitmask = 0x83,
> +               /* TPS68470_REG_POSTDIV */
> +       },
> +};
> +
> +/* Table to configure and enable clocks */
> +static const struct ti_pmic_table clk_table[] = {
> +       {
> +               .address = 0x00,
> +               .reg = TPS68470_REG_PLLCTL,
> +               .bitmask = 0xF5,
> +               /* TPS68470_REG_PLLCTL */
> +       },
> +       {
> +               .address = 0x04,
> +               .reg = TPS68470_REG_PLLCTL2,
> +               .bitmask = BIT(0),
> +               /* TPS68470_REG_PLLCTL2 */
> +       },
> +       {
> +               .address = 0x08,
> +               .reg = TPS68470_REG_CLKCFG1,
> +               .bitmask = TPS68470_CLKCFG1_MODE_A_MASK |
> +                       TPS68470_CLKCFG1_MODE_B_MASK,
> +               /* TPS68470_REG_CLKCFG1 */
> +       },
> +       {
> +               .address = 0x0C,
> +               .reg = TPS68470_REG_CLKCFG2,
> +               .bitmask = TPS68470_CLKCFG1_MODE_A_MASK |
> +                       TPS68470_CLKCFG1_MODE_B_MASK,
> +               /* TPS68470_REG_CLKCFG2 */
> +       },
> +};
> +
> +static int pmic_get_reg_bit(u64 address, struct ti_pmic_table *table,
> +                           int count, int *reg, int *bitmask)
> +{
> +       u64 i;
> +

> +       i = address / 4;


> +

Remove this line.

> +       if (i >= count)
> +               return -ENOENT;
> +
> +       if (!reg || !bitmask)
> +               return -EINVAL;
> +
> +       *reg = table[i].reg;
> +       *bitmask = table[i].bitmask;
> +
> +       return 0;
> +}
> +
> +static int ti_tps68470_pmic_get_power(struct regmap *regmap, int reg,
> +                                      int bitmask, u64 *value)
> +{
> +       int data;
> +
> +       if (regmap_read(regmap, reg, &data))
> +               return -EIO;
> +
> +       *value = (data & bitmask) ? 1 : 0;
> +       return 0;
> +}
> +
> +static int ti_tps68470_pmic_get_vr_val(struct regmap *regmap, int reg,
> +                                      int bitmask, u64 *value)
> +{
> +       int data;
> +
> +       if (regmap_read(regmap, reg, &data))
> +               return -EIO;
> +
> +       *value = data & bitmask;
> +       return 0;
> +}
> +
> +static int ti_tps68470_pmic_get_clk(struct regmap *regmap, int reg,
> +                                      int bitmask, u64 *value)
> +{
> +       int data;
> +
> +       if (regmap_read(regmap, reg, &data))
> +               return -EIO;
> +
> +       *value = (data & bitmask) ? 1 : 0;
> +       return 0;
> +}
> +
> +static int ti_tps68470_pmic_get_clk_freq(struct regmap *regmap, int reg,
> +                                      int bitmask, u64 *value)
> +{
> +       int data;
> +
> +       if (regmap_read(regmap, reg, &data))
> +               return -EIO;
> +
> +       *value = data & bitmask;
> +       return 0;
> +}
> +
> +static int ti_tps68470_regmap_update_bits(struct regmap *regmap, int reg,
> +                                       int bitmask, u64 value)
> +{
> +       return regmap_update_bits(regmap, reg, bitmask, value);
> +}
> +
> +static acpi_status ti_pmic_common_handler(u32 function,
> +                                         acpi_physical_address address,
> +                                         u32 bits, u64 *value,
> +                                         void *handler_context,
> +                                         void *region_context,
> +                                         int (*get)(struct regmap *,
> +                                                    int, int, u64 *),
> +                                         int (*update)(struct regmap *,
> +                                                       int, int, u64),
> +                                         struct ti_pmic_table *table,
> +                                         int table_size)
> +{
> +       struct ti_pmic_opregion *opregion = region_context;
> +       struct regmap *regmap = opregion->regmap;
> +       int reg, ret, bitmask;
> +
> +       if (bits != 32)
> +               return AE_BAD_PARAMETER;
> +
> +       ret = pmic_get_reg_bit(address, table,
> +                                 table_size, &reg, &bitmask);
> +       if (ret < 0)
> +               return AE_BAD_PARAMETER;
> +
> +       if (function == ACPI_WRITE && (*value > bitmask))
> +               return AE_BAD_PARAMETER;
> +
> +       mutex_lock(&opregion->lock);
> +
> +       ret = (function == ACPI_READ) ?
> +               get(regmap, reg, bitmask, value) :
> +               update(regmap, reg, bitmask, *value);
> +
> +       mutex_unlock(&opregion->lock);
> +
> +       return ret ? AE_ERROR : AE_OK;
> +}
> +
> +static acpi_status ti_pmic_clk_freq_handler(u32 function,
> +                                           acpi_physical_address address,
> +                                           u32 bits, u64 *value,
> +                                           void *handler_context,
> +                                           void *region_context)
> +{
> +       return ti_pmic_common_handler(function, address, bits, value,
> +                               handler_context, region_context,
> +                               ti_tps68470_pmic_get_clk_freq,
> +                               ti_tps68470_regmap_update_bits,
> +                               (struct ti_pmic_table *) &clk_freq_table,
> +                               ARRAY_SIZE(clk_freq_table));
> +}
> +
> +static acpi_status ti_pmic_clk_handler(u32 function,
> +                                      acpi_physical_address address, u32 bits,
> +                                      u64 *value, void *handler_context,
> +                                      void *region_context)
> +{
> +       return ti_pmic_common_handler(function, address, bits, value,
> +                               handler_context, region_context,
> +                               ti_tps68470_pmic_get_clk,
> +                               ti_tps68470_regmap_update_bits,
> +                               (struct ti_pmic_table *) &clk_table,
> +                                ARRAY_SIZE(clk_table));
> +}
> +
> +static acpi_status ti_pmic_vr_val_handler(u32 function,
> +                                         acpi_physical_address address,
> +                                         u32 bits, u64 *value,
> +                                         void *handler_context,
> +                                         void *region_context)
> +{
> +       return ti_pmic_common_handler(function, address, bits, value,
> +                               handler_context, region_context,
> +                               ti_tps68470_pmic_get_vr_val,
> +                               ti_tps68470_regmap_update_bits,
> +                               (struct ti_pmic_table *) &vr_val_table,
> +                               ARRAY_SIZE(vr_val_table));
> +}
> +
> +static acpi_status ti_pmic_power_handler(u32 function,
> +                                        acpi_physical_address address,
> +                                        u32 bits, u64 *value,
> +                                        void *handler_context,
> +                                        void *region_context)
> +{
> +       if (bits != 32)
> +               return AE_BAD_PARAMETER;
> +
> +       /* set/clear for bit 0, bits 0 and 1 together */
> +       if (function == ACPI_WRITE &&
> +           !(*value == 0 || *value == 1 || *value == 3)) {
> +               return AE_BAD_PARAMETER;
> +       }
> +
> +       return ti_pmic_common_handler(function, address, bits, value,
> +                               handler_context, region_context,
> +                               ti_tps68470_pmic_get_power,
> +                               ti_tps68470_regmap_update_bits,
> +                               (struct ti_pmic_table *) &power_table,
> +                               ARRAY_SIZE(power_table));
> +}
> +
> +static int ti_tps68470_pmic_opregion_probe(struct platform_device *pdev)
> +{
> +       struct tps68470 *pmic = dev_get_drvdata(pdev->dev.parent);
> +       acpi_handle handle = ACPI_HANDLE(pdev->dev.parent);
> +       struct device *dev = &pdev->dev;
> +       struct ti_pmic_opregion *opregion;
> +       acpi_status status;
> +

> +       if (!dev || !pmic->regmap) {
> +               WARN(1, "dev or regmap is NULL\n");
> +               return -EINVAL;
> +       }
> +
> +       if (!handle) {
> +               WARN(1, "acpi handle is NULL\n");
> +               return -ENODEV;
> +       }

I dunno if WARNs make user experience any better.
Besides that I would double check you may have such cases.

> +
> +       opregion = devm_kzalloc(dev, sizeof(*opregion), GFP_KERNEL);
> +       if (!opregion)
> +               return -ENOMEM;
> +
> +       mutex_init(&opregion->lock);
> +       opregion->regmap = pmic->regmap;
> +
> +       status = acpi_install_address_space_handler(handle,
> +                                                   TI_PMIC_POWER_OPREGION_ID,
> +                                                   ti_pmic_power_handler,
> +                                                   NULL, opregion);
> +       if (ACPI_FAILURE(status))
> +               return -ENODEV;
> +
> +       status = acpi_install_address_space_handler(handle,
> +                                                   TI_PMIC_VR_VAL_OPREGION_ID,
> +                                                   ti_pmic_vr_val_handler,
> +                                                   NULL, opregion);
> +       if (ACPI_FAILURE(status))
> +               goto out_remove_power_handler;
> +
> +       status = acpi_install_address_space_handler(handle,
> +                                                   TI_PMIC_CLOCK_OPREGION_ID,
> +                                                   ti_pmic_clk_handler,
> +                                                   NULL, opregion);
> +       if (ACPI_FAILURE(status))
> +               goto out_remove_vr_val_handler;
> +
> +       status = acpi_install_address_space_handler(handle,
> +                                                   TI_PMIC_CLKFREQ_OPREGION_ID,
> +                                                   ti_pmic_clk_freq_handler,
> +                                                   NULL, opregion);
> +       if (ACPI_FAILURE(status))
> +               goto out_remove_clk_handler;
> +
> +       return 0;
> +
> +out_remove_clk_handler:
> +       acpi_remove_address_space_handler(handle, TI_PMIC_CLOCK_OPREGION_ID,
> +                                         ti_pmic_clk_handler);
> +out_remove_vr_val_handler:
> +       acpi_remove_address_space_handler(handle, TI_PMIC_VR_VAL_OPREGION_ID,
> +                                         ti_pmic_vr_val_handler);
> +out_remove_power_handler:
> +       acpi_remove_address_space_handler(handle, TI_PMIC_POWER_OPREGION_ID,
> +                                         ti_pmic_power_handler);
> +       return -ENODEV;
> +}
> +
> +static struct platform_driver ti_tps68470_pmic_opregion_driver = {
> +       .probe = ti_tps68470_pmic_opregion_probe,
> +       .driver = {
> +               .name = "tps68470_pmic_opregion",
> +       },
> +};
> +
> +builtin_platform_driver(ti_tps68470_pmic_opregion_driver)
> --
> 1.9.1
>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Hans de Goede]

Hi,

On 06/06/2017 04:23 PM, Andy Shevchenko wrote:
> +Cc Hans (that's why didn't delete anything from original mail, just
> adding my comments).
> 
> Hans, if you have few minutes it would be appreciated to glance on the
> below for some issues if any since you did pass quite a good quest
> with other PMIC drivers.

I've gone over this driver, nothing stands out in a bad way to me,
IOW this seems like a normal PMIC OpRegion handler to me.

Regards,

Hans



> 
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.mani@intel.com> wrote:
>> The Kabylake platform coreboot (Chrome OS equivalent of
>> BIOS) has defined 4 operation regions for the TI TPS68470 PMIC.
>> These operation regions are to enable/disable voltage
>> regulators, configure voltage regulators, enable/disable
>> clocks and to configure clocks.
>>
>> This config adds ACPI operation region support for TI TPS68470 PMIC.
>> 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 driver enables ACPI operation region support to control voltage
>> regulators and clocks for the TPS68470 PMIC.
>>
>> Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
>> ---
>>   drivers/acpi/Kconfig              |  12 +
>>   drivers/acpi/Makefile             |   2 +
> 
>>   drivers/acpi/pmic/pmic_tps68470.c | 454 ++++++++++++++++++++++++++++++++++++++
> 
> Follow the pattern, please, I suppose
> ti_pmic_tps68470.c
> 
>>   3 files changed, 468 insertions(+)
>>   create mode 100644 drivers/acpi/pmic/pmic_tps68470.c
>>
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index 1ce52f8..218d22d 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -535,4 +535,16 @@ if ARM64
>>   source "drivers/acpi/arm64/Kconfig"
>>   endif
>>
>> +config TPS68470_PMIC_OPREGION
>> +       bool "ACPI operation region support for TPS68470 PMIC"
>> +       help
>> +         This config adds ACPI operation region support for TI TPS68470 PMIC.
>> +         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 driver enables ACPI operation region support control voltage
>> +         regulators and clocks.
>> +
> 
>> +
> 
> Extra line, remove.
> 
>>   endif  # ACPI
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index b1aacfc..7113d05 100644
>> --- a/drivers/acpi/Makefile
>> +++ b/drivers/acpi/Makefile
>> @@ -106,6 +106,8 @@ obj-$(CONFIG_CHT_WC_PMIC_OPREGION) += pmic/intel_pmic_chtwc.o
>>
>>   obj-$(CONFIG_ACPI_CONFIGFS)    += acpi_configfs.o
>>
>> +obj-$(CONFIG_TPS68470_PMIC_OPREGION)   += pmic/pmic_tps68470.o
>> +
>>   video-objs                     += acpi_video.o video_detect.o
>>   obj-y                          += dptf/
>>
>> diff --git a/drivers/acpi/pmic/pmic_tps68470.c b/drivers/acpi/pmic/pmic_tps68470.c
>> new file mode 100644
>> index 0000000..b2d608b
>> --- /dev/null
>> +++ b/drivers/acpi/pmic/pmic_tps68470.c
>> @@ -0,0 +1,454 @@
>> +/*
>> + * TI TPS68470 PMIC operation region driver
>> + *
>> + * Copyright (C) 2017 Intel Corporation. All rights reserved.
>> + * Author: Rajmohan Mani <rajmohan.mani@intel.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License version
>> + * 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * Based on drivers/acpi/pmic/intel_pmic* drivers
>> + *
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/mfd/tps68470.h>
>> +#include <linux/init.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +struct ti_pmic_table {
>> +       u32 address;            /* operation region address */
>> +       u32 reg;                /* corresponding register */
>> +       u32 bitmask;            /* bit mask for power, clock */
>> +};
>> +
>> +#define TI_PMIC_POWER_OPREGION_ID              0xB0
>> +#define TI_PMIC_VR_VAL_OPREGION_ID             0xB1
>> +#define TI_PMIC_CLOCK_OPREGION_ID              0xB2
>> +#define TI_PMIC_CLKFREQ_OPREGION_ID            0xB3
>> +
>> +struct ti_pmic_opregion {
>> +       struct mutex lock;
>> +       struct regmap *regmap;
>> +};
>> +
>> +#define S_IO_I2C_EN    (BIT(0) | BIT(1))
>> +
>> +static const struct ti_pmic_table power_table[] = {
>> +       {
>> +               .address = 0x00,
>> +               .reg = TPS68470_REG_S_I2C_CTL,
>> +               .bitmask = S_IO_I2C_EN,
>> +               /* S_I2C_CTL */
>> +       },
>> +       {
>> +               .address = 0x04,
>> +               .reg = TPS68470_REG_VCMCTL,
>> +               .bitmask = BIT(0),
>> +               /* VCMCTL */
>> +       },
>> +       {
>> +               .address = 0x08,
>> +               .reg = TPS68470_REG_VAUX1CTL,
>> +               .bitmask = BIT(0),
>> +               /* VAUX1_CTL */
>> +       },
>> +       {
>> +               .address = 0x0C,
>> +               .reg = TPS68470_REG_VAUX2CTL,
>> +               .bitmask = BIT(0),
>> +               /* VAUX2CTL */
>> +       },
>> +       {
>> +               .address = 0x10,
>> +               .reg = TPS68470_REG_VACTL,
>> +               .bitmask = BIT(0),
>> +               /* VACTL */
>> +       },
>> +       {
>> +               .address = 0x14,
>> +               .reg = TPS68470_REG_VDCTL,
>> +               .bitmask = BIT(0),
>> +               /* VDCTL */
>> +       },
>> +};
>> +
>> +/* Table to set voltage regulator value */
>> +static const struct ti_pmic_table vr_val_table[] = {
>> +       {
>> +               .address = 0x00,
>> +               .reg = TPS68470_REG_VSIOVAL,
>> +               .bitmask = TPS68470_VSIOVAL_IOVOLT_MASK,
>> +               /* TPS68470_REG_VSIOVAL */
>> +       },
>> +       {
>> +               .address = 0x04,
>> +               .reg = TPS68470_REG_VIOVAL,
>> +               .bitmask = TPS68470_VIOVAL_IOVOLT_MASK,
>> +               /* TPS68470_REG_VIOVAL */
>> +       },
>> +       {
>> +               .address = 0x08,
>> +               .reg = TPS68470_REG_VCMVAL,
>> +               .bitmask = TPS68470_VCMVAL_VCVOLT_MASK,
>> +               /* TPS68470_REG_VCMVAL */
>> +       },
>> +       {
>> +               .address = 0x0C,
>> +               .reg = TPS68470_REG_VAUX1VAL,
>> +               .bitmask = TPS68470_VAUX1VAL_AUX1VOLT_MASK,
>> +               /* TPS68470_REG_VAUX1VAL */
>> +       },
>> +       {
>> +               .address = 0x10,
>> +               .reg = TPS68470_REG_VAUX2VAL,
>> +               .bitmask = TPS68470_VAUX2VAL_AUX2VOLT_MASK,
>> +               /* TPS68470_REG_VAUX2VAL */
>> +       },
>> +       {
>> +               .address = 0x14,
>> +               .reg = TPS68470_REG_VAVAL,
>> +               .bitmask = TPS68470_VAVAL_AVOLT_MASK,
>> +               /* TPS68470_REG_VAVAL */
>> +       },
>> +       {
>> +               .address = 0x18,
>> +               .reg = TPS68470_REG_VDVAL,
>> +               .bitmask = TPS68470_VDVAL_DVOLT_MASK,
>> +               /* TPS68470_REG_VDVAL */
>> +       },
>> +};
>> +
>> +/* Table to configure clock frequency */
>> +static const struct ti_pmic_table clk_freq_table[] = {
>> +       {
>> +               .address = 0x00,
>> +               .reg = TPS68470_REG_POSTDIV2,
>> +               .bitmask = BIT(0) | BIT(1),
>> +               /* TPS68470_REG_POSTDIV2 */
>> +       },
>> +       {
>> +               .address = 0x04,
>> +               .reg = TPS68470_REG_BOOSTDIV,
>> +               .bitmask = 0x1F,
>> +               /* TPS68470_REG_BOOSTDIV */
>> +       },
>> +       {
>> +               .address = 0x08,
>> +               .reg = TPS68470_REG_BUCKDIV,
>> +               .bitmask = 0x0F,
>> +               /* TPS68470_REG_BUCKDIV */
>> +       },
>> +       {
>> +               .address = 0x0C,
>> +               .reg = TPS68470_REG_PLLSWR,
>> +               .bitmask = 0x13,
>> +               /* TPS68470_REG_PLLSWR */
>> +       },
>> +       {
>> +               .address = 0x10,
>> +               .reg = TPS68470_REG_XTALDIV,
>> +               .bitmask = 0xFF,
>> +               /* TPS68470_REG_XTALDIV */
>> +       },
>> +       {
>> +               .address = 0x14,
>> +               .reg = TPS68470_REG_PLLDIV,
>> +               .bitmask = 0xFF,
>> +               /* TPS68470_REG_PLLDIV */
>> +       },
>> +       {
>> +               .address = 0x18,
>> +               .reg = TPS68470_REG_POSTDIV,
>> +               .bitmask = 0x83,
>> +               /* TPS68470_REG_POSTDIV */
>> +       },
>> +};
>> +
>> +/* Table to configure and enable clocks */
>> +static const struct ti_pmic_table clk_table[] = {
>> +       {
>> +               .address = 0x00,
>> +               .reg = TPS68470_REG_PLLCTL,
>> +               .bitmask = 0xF5,
>> +               /* TPS68470_REG_PLLCTL */
>> +       },
>> +       {
>> +               .address = 0x04,
>> +               .reg = TPS68470_REG_PLLCTL2,
>> +               .bitmask = BIT(0),
>> +               /* TPS68470_REG_PLLCTL2 */
>> +       },
>> +       {
>> +               .address = 0x08,
>> +               .reg = TPS68470_REG_CLKCFG1,
>> +               .bitmask = TPS68470_CLKCFG1_MODE_A_MASK |
>> +                       TPS68470_CLKCFG1_MODE_B_MASK,
>> +               /* TPS68470_REG_CLKCFG1 */
>> +       },
>> +       {
>> +               .address = 0x0C,
>> +               .reg = TPS68470_REG_CLKCFG2,
>> +               .bitmask = TPS68470_CLKCFG1_MODE_A_MASK |
>> +                       TPS68470_CLKCFG1_MODE_B_MASK,
>> +               /* TPS68470_REG_CLKCFG2 */
>> +       },
>> +};
>> +
>> +static int pmic_get_reg_bit(u64 address, struct ti_pmic_table *table,
>> +                           int count, int *reg, int *bitmask)
>> +{
>> +       u64 i;
>> +
> 
>> +       i = address / 4;
> 
> 
>> +
> 
> Remove this line.
> 
>> +       if (i >= count)
>> +               return -ENOENT;
>> +
>> +       if (!reg || !bitmask)
>> +               return -EINVAL;
>> +
>> +       *reg = table[i].reg;
>> +       *bitmask = table[i].bitmask;
>> +
>> +       return 0;
>> +}
>> +
>> +static int ti_tps68470_pmic_get_power(struct regmap *regmap, int reg,
>> +                                      int bitmask, u64 *value)
>> +{
>> +       int data;
>> +
>> +       if (regmap_read(regmap, reg, &data))
>> +               return -EIO;
>> +
>> +       *value = (data & bitmask) ? 1 : 0;
>> +       return 0;
>> +}
>> +
>> +static int ti_tps68470_pmic_get_vr_val(struct regmap *regmap, int reg,
>> +                                      int bitmask, u64 *value)
>> +{
>> +       int data;
>> +
>> +       if (regmap_read(regmap, reg, &data))
>> +               return -EIO;
>> +
>> +       *value = data & bitmask;
>> +       return 0;
>> +}
>> +
>> +static int ti_tps68470_pmic_get_clk(struct regmap *regmap, int reg,
>> +                                      int bitmask, u64 *value)
>> +{
>> +       int data;
>> +
>> +       if (regmap_read(regmap, reg, &data))
>> +               return -EIO;
>> +
>> +       *value = (data & bitmask) ? 1 : 0;
>> +       return 0;
>> +}
>> +
>> +static int ti_tps68470_pmic_get_clk_freq(struct regmap *regmap, int reg,
>> +                                      int bitmask, u64 *value)
>> +{
>> +       int data;
>> +
>> +       if (regmap_read(regmap, reg, &data))
>> +               return -EIO;
>> +
>> +       *value = data & bitmask;
>> +       return 0;
>> +}
>> +
>> +static int ti_tps68470_regmap_update_bits(struct regmap *regmap, int reg,
>> +                                       int bitmask, u64 value)
>> +{
>> +       return regmap_update_bits(regmap, reg, bitmask, value);
>> +}
>> +
>> +static acpi_status ti_pmic_common_handler(u32 function,
>> +                                         acpi_physical_address address,
>> +                                         u32 bits, u64 *value,
>> +                                         void *handler_context,
>> +                                         void *region_context,
>> +                                         int (*get)(struct regmap *,
>> +                                                    int, int, u64 *),
>> +                                         int (*update)(struct regmap *,
>> +                                                       int, int, u64),
>> +                                         struct ti_pmic_table *table,
>> +                                         int table_size)
>> +{
>> +       struct ti_pmic_opregion *opregion = region_context;
>> +       struct regmap *regmap = opregion->regmap;
>> +       int reg, ret, bitmask;
>> +
>> +       if (bits != 32)
>> +               return AE_BAD_PARAMETER;
>> +
>> +       ret = pmic_get_reg_bit(address, table,
>> +                                 table_size, &reg, &bitmask);
>> +       if (ret < 0)
>> +               return AE_BAD_PARAMETER;
>> +
>> +       if (function == ACPI_WRITE && (*value > bitmask))
>> +               return AE_BAD_PARAMETER;
>> +
>> +       mutex_lock(&opregion->lock);
>> +
>> +       ret = (function == ACPI_READ) ?
>> +               get(regmap, reg, bitmask, value) :
>> +               update(regmap, reg, bitmask, *value);
>> +
>> +       mutex_unlock(&opregion->lock);
>> +
>> +       return ret ? AE_ERROR : AE_OK;
>> +}
>> +
>> +static acpi_status ti_pmic_clk_freq_handler(u32 function,
>> +                                           acpi_physical_address address,
>> +                                           u32 bits, u64 *value,
>> +                                           void *handler_context,
>> +                                           void *region_context)
>> +{
>> +       return ti_pmic_common_handler(function, address, bits, value,
>> +                               handler_context, region_context,
>> +                               ti_tps68470_pmic_get_clk_freq,
>> +                               ti_tps68470_regmap_update_bits,
>> +                               (struct ti_pmic_table *) &clk_freq_table,
>> +                               ARRAY_SIZE(clk_freq_table));
>> +}
>> +
>> +static acpi_status ti_pmic_clk_handler(u32 function,
>> +                                      acpi_physical_address address, u32 bits,
>> +                                      u64 *value, void *handler_context,
>> +                                      void *region_context)
>> +{
>> +       return ti_pmic_common_handler(function, address, bits, value,
>> +                               handler_context, region_context,
>> +                               ti_tps68470_pmic_get_clk,
>> +                               ti_tps68470_regmap_update_bits,
>> +                               (struct ti_pmic_table *) &clk_table,
>> +                                ARRAY_SIZE(clk_table));
>> +}
>> +
>> +static acpi_status ti_pmic_vr_val_handler(u32 function,
>> +                                         acpi_physical_address address,
>> +                                         u32 bits, u64 *value,
>> +                                         void *handler_context,
>> +                                         void *region_context)
>> +{
>> +       return ti_pmic_common_handler(function, address, bits, value,
>> +                               handler_context, region_context,
>> +                               ti_tps68470_pmic_get_vr_val,
>> +                               ti_tps68470_regmap_update_bits,
>> +                               (struct ti_pmic_table *) &vr_val_table,
>> +                               ARRAY_SIZE(vr_val_table));
>> +}
>> +
>> +static acpi_status ti_pmic_power_handler(u32 function,
>> +                                        acpi_physical_address address,
>> +                                        u32 bits, u64 *value,
>> +                                        void *handler_context,
>> +                                        void *region_context)
>> +{
>> +       if (bits != 32)
>> +               return AE_BAD_PARAMETER;
>> +
>> +       /* set/clear for bit 0, bits 0 and 1 together */
>> +       if (function == ACPI_WRITE &&
>> +           !(*value == 0 || *value == 1 || *value == 3)) {
>> +               return AE_BAD_PARAMETER;
>> +       }
>> +
>> +       return ti_pmic_common_handler(function, address, bits, value,
>> +                               handler_context, region_context,
>> +                               ti_tps68470_pmic_get_power,
>> +                               ti_tps68470_regmap_update_bits,
>> +                               (struct ti_pmic_table *) &power_table,
>> +                               ARRAY_SIZE(power_table));
>> +}
>> +
>> +static int ti_tps68470_pmic_opregion_probe(struct platform_device *pdev)
>> +{
>> +       struct tps68470 *pmic = dev_get_drvdata(pdev->dev.parent);
>> +       acpi_handle handle = ACPI_HANDLE(pdev->dev.parent);
>> +       struct device *dev = &pdev->dev;
>> +       struct ti_pmic_opregion *opregion;
>> +       acpi_status status;
>> +
> 
>> +       if (!dev || !pmic->regmap) {
>> +               WARN(1, "dev or regmap is NULL\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (!handle) {
>> +               WARN(1, "acpi handle is NULL\n");
>> +               return -ENODEV;
>> +       }
> 
> I dunno if WARNs make user experience any better.
> Besides that I would double check you may have such cases.
> 
>> +
>> +       opregion = devm_kzalloc(dev, sizeof(*opregion), GFP_KERNEL);
>> +       if (!opregion)
>> +               return -ENOMEM;
>> +
>> +       mutex_init(&opregion->lock);
>> +       opregion->regmap = pmic->regmap;
>> +
>> +       status = acpi_install_address_space_handler(handle,
>> +                                                   TI_PMIC_POWER_OPREGION_ID,
>> +                                                   ti_pmic_power_handler,
>> +                                                   NULL, opregion);
>> +       if (ACPI_FAILURE(status))
>> +               return -ENODEV;
>> +
>> +       status = acpi_install_address_space_handler(handle,
>> +                                                   TI_PMIC_VR_VAL_OPREGION_ID,
>> +                                                   ti_pmic_vr_val_handler,
>> +                                                   NULL, opregion);
>> +       if (ACPI_FAILURE(status))
>> +               goto out_remove_power_handler;
>> +
>> +       status = acpi_install_address_space_handler(handle,
>> +                                                   TI_PMIC_CLOCK_OPREGION_ID,
>> +                                                   ti_pmic_clk_handler,
>> +                                                   NULL, opregion);
>> +       if (ACPI_FAILURE(status))
>> +               goto out_remove_vr_val_handler;
>> +
>> +       status = acpi_install_address_space_handler(handle,
>> +                                                   TI_PMIC_CLKFREQ_OPREGION_ID,
>> +                                                   ti_pmic_clk_freq_handler,
>> +                                                   NULL, opregion);
>> +       if (ACPI_FAILURE(status))
>> +               goto out_remove_clk_handler;
>> +
>> +       return 0;
>> +
>> +out_remove_clk_handler:
>> +       acpi_remove_address_space_handler(handle, TI_PMIC_CLOCK_OPREGION_ID,
>> +                                         ti_pmic_clk_handler);
>> +out_remove_vr_val_handler:
>> +       acpi_remove_address_space_handler(handle, TI_PMIC_VR_VAL_OPREGION_ID,
>> +                                         ti_pmic_vr_val_handler);
>> +out_remove_power_handler:
>> +       acpi_remove_address_space_handler(handle, TI_PMIC_POWER_OPREGION_ID,
>> +                                         ti_pmic_power_handler);
>> +       return -ENODEV;
>> +}
>> +
>> +static struct platform_driver ti_tps68470_pmic_opregion_driver = {
>> +       .probe = ti_tps68470_pmic_opregion_probe,
>> +       .driver = {
>> +               .name = "tps68470_pmic_opregion",
>> +       },
>> +};
>> +
>> +builtin_platform_driver(ti_tps68470_pmic_opregion_driver)
>> --
>> 1.9.1
>>
> 

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Hans,

> -----Original Message-----
> From: Hans de Goede [mailto:hdegoede@redhat.com]
> Sent: Tuesday, June 06, 2017 8:22 AM
> To: Andy Shevchenko <andy.shevchenko@gmail.com>; 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 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> Hi,
> 
> On 06/06/2017 04:23 PM, Andy Shevchenko wrote:
> > +Cc Hans (that's why didn't delete anything from original mail, just
> > adding my comments).
> >
> > Hans, if you have few minutes it would be appreciated to glance on the
> > below for some issues if any since you did pass quite a good quest
> > with other PMIC drivers.
> 
> I've gone over this driver, nothing stands out in a bad way to me, IOW this
> seems like a normal PMIC OpRegion handler to me.
> 

Thanks for the reviews and time.

Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Sakari Ailus]

On Tue, Jun 06, 2017 at 05:23:56PM +0300, Andy Shevchenko wrote:
> Follow the pattern, please, I suppose
> ti_pmic_tps68470.c

This pattern is weird. "ti" in front of the file name is redundant, and in
very few places the vendor prefix is used anyway. Especially when the chip
has a proper name --- as this one does.

I assume for the Intel PMICs it could be there for a couple of reasons which
are

1) lack of a clearly unique chip ID and

2) the use of common frameworklet for Intel PMICs.

There are also no other PMIC chips supported currently.

The pmic_tps68470 naming is in line with the GPIO driver (apart from the
dash / underscore difference).

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

Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Andy Shevchenko]

On Wed, Jun 7, 2017 at 3:15 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> On Tue, Jun 06, 2017 at 05:23:56PM +0300, Andy Shevchenko wrote:
>> Follow the pattern, please, I suppose
>> ti_pmic_tps68470.c
>
> This pattern is weird. "ti" in front of the file name is redundant, and in
> very few places the vendor prefix is used anyway. Especially when the chip
> has a proper name --- as this one does.
>
> I assume for the Intel PMICs it could be there for a couple of reasons which
> are
>
> 1) lack of a clearly unique chip ID and
>
> 2) the use of common frameworklet for Intel PMICs.
>
> There are also no other PMIC chips supported currently.
>
> The pmic_tps68470 naming is in line with the GPIO driver (apart from the
> dash / underscore difference).

Since

% git ls-files *pmic*

returns somewhat interesting results, I would even go further and use

tps68470.c here

and

s/ti_pmic/tps6840/g

inside the file.

Would it work for you?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Sakari Ailus]

Hi Andy,

On Wed, Jun 07, 2017 at 04:40:13PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 7, 2017 at 3:15 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > On Tue, Jun 06, 2017 at 05:23:56PM +0300, Andy Shevchenko wrote:
> >> Follow the pattern, please, I suppose
> >> ti_pmic_tps68470.c
> >
> > This pattern is weird. "ti" in front of the file name is redundant, and in
> > very few places the vendor prefix is used anyway. Especially when the chip
> > has a proper name --- as this one does.
> >
> > I assume for the Intel PMICs it could be there for a couple of reasons which
> > are
> >
> > 1) lack of a clearly unique chip ID and
> >
> > 2) the use of common frameworklet for Intel PMICs.
> >
> > There are also no other PMIC chips supported currently.
> >
> > The pmic_tps68470 naming is in line with the GPIO driver (apart from the
> > dash / underscore difference).
> 
> Since
> 
> % git ls-files *pmic*
> 
> returns somewhat interesting results, I would even go further and use
> 
> tps68470.c here
> 
> and
> 
> s/ti_pmic/tps6840/g
> 
> inside the file.
> 
> Would it work for you?

This is still a different driver from the tps68470 driver which is an MFD
driver. For clarity, I'd keep pmic as part of the name (and I'd use
tps68470_pmic_ prefix for internal symbols, too).

As PMICs are typically linked to the kernel (vs. being modules), there's no
issue with the module name. I would suppose few if any PMICs will be
compiled as modules in general.

It's not a big deal though. I'm fine either way.

-- 
Regards,

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

Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Andy Shevchenko]

On Wed, Jun 7, 2017 at 11:10 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> On Wed, Jun 07, 2017 at 04:40:13PM +0300, Andy Shevchenko wrote:
>> On Wed, Jun 7, 2017 at 3:15 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>> > On Tue, Jun 06, 2017 at 05:23:56PM +0300, Andy Shevchenko wrote:

>> >> Follow the pattern, please, I suppose
>> >> ti_pmic_tps68470.c
>>
>> Would it work for you?
>
> This is still a different driver from the tps68470 driver which is an MFD
> driver. For clarity, I'd keep pmic as part of the name (and I'd use
> tps68470_pmic_ prefix for internal symbols, too).
>
> As PMICs are typically linked to the kernel (vs. being modules), there's no
> issue with the module name. I would suppose few if any PMICs will be
> compiled as modules in general.
>
> It's not a big deal though. I'm fine either way.

Okay, let's agree on  tps68470_pmic for internal prefix and for file name?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Sakari Ailus]

On Wed, Jun 07, 2017 at 11:40:27PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 7, 2017 at 11:10 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > On Wed, Jun 07, 2017 at 04:40:13PM +0300, Andy Shevchenko wrote:
> >> On Wed, Jun 7, 2017 at 3:15 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >> > On Tue, Jun 06, 2017 at 05:23:56PM +0300, Andy Shevchenko wrote:
> 
> >> >> Follow the pattern, please, I suppose
> >> >> ti_pmic_tps68470.c
> >>
> >> Would it work for you?
> >
> > This is still a different driver from the tps68470 driver which is an MFD
> > driver. For clarity, I'd keep pmic as part of the name (and I'd use
> > tps68470_pmic_ prefix for internal symbols, too).
> >
> > As PMICs are typically linked to the kernel (vs. being modules), there's no
> > issue with the module name. I would suppose few if any PMICs will be
> > compiled as modules in general.
> >
> > It's not a big deal though. I'm fine either way.
> 
> Okay, let's agree on  tps68470_pmic for internal prefix and for file name?

Ack. Thanks!

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

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Sakari, Andy,

> -----Original Message-----
> From: Sakari Ailus [mailto:sakari.ailus@iki.fi]
> Sent: Wednesday, June 07, 2017 2:13 PM
> To: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Mani, Rajmohan <rajmohan.mani@intel.com>; Hans de Goede
> <hdegoede@redhat.com>; 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 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> On Wed, Jun 07, 2017 at 11:40:27PM +0300, Andy Shevchenko wrote:
> > On Wed, Jun 7, 2017 at 11:10 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > > On Wed, Jun 07, 2017 at 04:40:13PM +0300, Andy Shevchenko wrote:
> > >> On Wed, Jun 7, 2017 at 3:15 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > >> > On Tue, Jun 06, 2017 at 05:23:56PM +0300, Andy Shevchenko wrote:
> >
> > >> >> Follow the pattern, please, I suppose ti_pmic_tps68470.c
> > >>
> > >> Would it work for you?
> > >
> > > This is still a different driver from the tps68470 driver which is
> > > an MFD driver. For clarity, I'd keep pmic as part of the name (and
> > > I'd use tps68470_pmic_ prefix for internal symbols, too).
> > >
> > > As PMICs are typically linked to the kernel (vs. being modules),
> > > there's no issue with the module name. I would suppose few if any
> > > PMICs will be compiled as modules in general.
> > >
> > > It's not a big deal though. I'm fine either way.
> >
> > Okay, let's agree on  tps68470_pmic for internal prefix and for file name?
> 
> Ack. Thanks!
> 

Ack

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Sakari, Andy,

> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> On Wed, Jun 07, 2017 at 11:40:27PM +0300, Andy Shevchenko wrote:
> > On Wed, Jun 7, 2017 at 11:10 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > > On Wed, Jun 07, 2017 at 04:40:13PM +0300, Andy Shevchenko wrote:
> > >> On Wed, Jun 7, 2017 at 3:15 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > >> > On Tue, Jun 06, 2017 at 05:23:56PM +0300, Andy Shevchenko wrote:
> >
> > >> >> Follow the pattern, please, I suppose ti_pmic_tps68470.c
> > >>
> > >> Would it work for you?
> > >
> > > This is still a different driver from the tps68470 driver which is
> > > an MFD driver. For clarity, I'd keep pmic as part of the name (and
> > > I'd use tps68470_pmic_ prefix for internal symbols, too).
> > >
> > > As PMICs are typically linked to the kernel (vs. being modules),
> > > there's no issue with the module name. I would suppose few if any
> > > PMICs will be compiled as modules in general.
> > >
> > > It's not a big deal though. I'm fine either way.
> >
> > Okay, let's agree on  tps68470_pmic for internal prefix and for file name?
> 
> Ack. Thanks!
> 

Ack

Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Hans de Goede]

Hi,

On 07-06-17 22:10, Sakari Ailus wrote:
> Hi Andy,
> 
> On Wed, Jun 07, 2017 at 04:40:13PM +0300, Andy Shevchenko wrote:
>> On Wed, Jun 7, 2017 at 3:15 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>>> On Tue, Jun 06, 2017 at 05:23:56PM +0300, Andy Shevchenko wrote:
>>>> Follow the pattern, please, I suppose
>>>> ti_pmic_tps68470.c
>>>
>>> This pattern is weird. "ti" in front of the file name is redundant, and in
>>> very few places the vendor prefix is used anyway. Especially when the chip
>>> has a proper name --- as this one does.
>>>
>>> I assume for the Intel PMICs it could be there for a couple of reasons which
>>> are
>>>
>>> 1) lack of a clearly unique chip ID and
>>>
>>> 2) the use of common frameworklet for Intel PMICs.
>>>
>>> There are also no other PMIC chips supported currently.
>>>
>>> The pmic_tps68470 naming is in line with the GPIO driver (apart from the
>>> dash / underscore difference).
>>
>> Since
>>
>> % git ls-files *pmic*
>>
>> returns somewhat interesting results, I would even go further and use
>>
>> tps68470.c here
>>
>> and
>>
>> s/ti_pmic/tps6840/g
>>
>> inside the file.
>>
>> Would it work for you?
> 
> This is still a different driver from the tps68470 driver which is an MFD
> driver. For clarity, I'd keep pmic as part of the name (and I'd use
> tps68470_pmic_ prefix for internal symbols, too).
> 
> As PMICs are typically linked to the kernel (vs. being modules), there's no
> issue with the module name. I would suppose few if any PMICs will be
> compiled as modules in general.

Good point about the OpRegion driver usually being built-in, in my experience
it MUST always be built-in, so the Kconfig option should be a bool. Note this
is useless unless the mfd driver is also a bool (I would advice to go that
route) and the mfd driver's Kconfig should select the right i2c bus driver
to make sure that is built-in too, see for example:

https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for-mfd-next&id=2f91ded5f8f4fdd67d8daae514b0d434c98ab1e0
https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for-mfd-next&id=c5065d8625ebdc164199b99d838ac0636faa7f0b
https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for-mfd-next&id=5f125f1f570568a29edf783fba1ebb606d5c6b24

Which are all recent commits from me dealing with making the mfd driver
built-in / selecting the i2c bus driver.

Regards,

Hans

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Hans,

> >
> > As PMICs are typically linked to the kernel (vs. being modules),
> > there's no issue with the module name. I would suppose few if any
> > PMICs will be compiled as modules in general.
> 
> Good point about the OpRegion driver usually being built-in, in my experience it
> MUST always be built-in, so the Kconfig option should be a bool. Note this is
> useless unless the mfd driver is also a bool (I would advice to go that
> route) and the mfd driver's Kconfig should select the right i2c bus driver to
> make sure that is built-in too, see for example:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for-
> mfd-next&id=2f91ded5f8f4fdd67d8daae514b0d434c98ab1e0
> https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for-
> mfd-next&id=c5065d8625ebdc164199b99d838ac0636faa7f0b
> https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for-
> mfd-next&id=5f125f1f570568a29edf783fba1ebb606d5c6b24
> 
> Which are all recent commits from me dealing with making the mfd driver built-
> in / selecting the i2c bus driver.
> 

Thanks for these links.
I will update the Kconfig and commit messages with relevant description around this.

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Andy,

Thanks for the reviews and patience.

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Tuesday, June 06, 2017 7:24 AM
> To: Mani, Rajmohan <rajmohan.mani@intel.com>; Hans de Goede
> <hdegoede@redhat.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 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> +Cc Hans (that's why didn't delete anything from original mail, just
> adding my comments).
> 
> Hans, if you have few minutes it would be appreciated to glance on the below
> for some issues if any since you did pass quite a good quest with other PMIC
> drivers.
> 
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.mani@intel.com>
> wrote:
> > The Kabylake platform coreboot (Chrome OS equivalent of
> > BIOS) has defined 4 operation regions for the TI TPS68470 PMIC.
> > These operation regions are to enable/disable voltage regulators,
> > configure voltage regulators, enable/disable clocks and to configure
> > clocks.
> >
> > This config adds ACPI operation region support for TI TPS68470 PMIC.
> > 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 driver enables ACPI operation region support to control voltage
> > regulators and clocks for the TPS68470 PMIC.
> >
> > Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
> > ---
> >  drivers/acpi/Kconfig              |  12 +
> >  drivers/acpi/Makefile             |   2 +
> 
> >  drivers/acpi/pmic/pmic_tps68470.c | 454
> > ++++++++++++++++++++++++++++++++++++++
> 
> Follow the pattern, please, I suppose
> ti_pmic_tps68470.c
> 

I will reply to this, on the later threads on the same subject

> >  3 files changed, 468 insertions(+)
> >  create mode 100644 drivers/acpi/pmic/pmic_tps68470.c
> >
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index
> > 1ce52f8..218d22d 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -535,4 +535,16 @@ if ARM64
> >  source "drivers/acpi/arm64/Kconfig"
> >  endif
> >
> > +config TPS68470_PMIC_OPREGION
> > +       bool "ACPI operation region support for TPS68470 PMIC"
> > +       help
> > +         This config adds ACPI operation region support for TI TPS68470 PMIC.
> > +         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 driver enables ACPI operation region support control voltage
> > +         regulators and clocks.
> > +
> 
> > +
> 
> Extra line, remove.
> 

Ack

> >  endif  # ACPI
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index
> > b1aacfc..7113d05 100644
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -106,6 +106,8 @@ obj-$(CONFIG_CHT_WC_PMIC_OPREGION) +=
> > pmic/intel_pmic_chtwc.o
> >
> >  obj-$(CONFIG_ACPI_CONFIGFS)    += acpi_configfs.o
> >
> > +obj-$(CONFIG_TPS68470_PMIC_OPREGION)   += pmic/pmic_tps68470.o
> > +
> >  video-objs                     += acpi_video.o video_detect.o
> >  obj-y                          += dptf/
> >
> > diff --git a/drivers/acpi/pmic/pmic_tps68470.c
> > b/drivers/acpi/pmic/pmic_tps68470.c
> > new file mode 100644
> > index 0000000..b2d608b
> > --- /dev/null
> > +++ b/drivers/acpi/pmic/pmic_tps68470.c
> > @@ -0,0 +1,454 @@
> > +/*
> > + * TI TPS68470 PMIC operation region driver
> > + *
> > + * Copyright (C) 2017 Intel Corporation. All rights reserved.
> > + * Author: Rajmohan Mani <rajmohan.mani@intel.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > +version
> > + * 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * Based on drivers/acpi/pmic/intel_pmic* drivers
> > + *
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/mfd/tps68470.h>
> > +#include <linux/init.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +
> > +struct ti_pmic_table {
> > +       u32 address;            /* operation region address */
> > +       u32 reg;                /* corresponding register */
> > +       u32 bitmask;            /* bit mask for power, clock */
> > +};
> > +
> > +#define TI_PMIC_POWER_OPREGION_ID              0xB0
> > +#define TI_PMIC_VR_VAL_OPREGION_ID             0xB1
> > +#define TI_PMIC_CLOCK_OPREGION_ID              0xB2
> > +#define TI_PMIC_CLKFREQ_OPREGION_ID            0xB3
> > +
> > +struct ti_pmic_opregion {
> > +       struct mutex lock;
> > +       struct regmap *regmap;
> > +};
> > +
> > +#define S_IO_I2C_EN    (BIT(0) | BIT(1))
> > +
> > +static const struct ti_pmic_table power_table[] = {
> > +       {
> > +               .address = 0x00,
> > +               .reg = TPS68470_REG_S_I2C_CTL,
> > +               .bitmask = S_IO_I2C_EN,
> > +               /* S_I2C_CTL */
> > +       },
> > +       {
> > +               .address = 0x04,
> > +               .reg = TPS68470_REG_VCMCTL,
> > +               .bitmask = BIT(0),
> > +               /* VCMCTL */
> > +       },
> > +       {
> > +               .address = 0x08,
> > +               .reg = TPS68470_REG_VAUX1CTL,
> > +               .bitmask = BIT(0),
> > +               /* VAUX1_CTL */
> > +       },
> > +       {
> > +               .address = 0x0C,
> > +               .reg = TPS68470_REG_VAUX2CTL,
> > +               .bitmask = BIT(0),
> > +               /* VAUX2CTL */
> > +       },
> > +       {
> > +               .address = 0x10,
> > +               .reg = TPS68470_REG_VACTL,
> > +               .bitmask = BIT(0),
> > +               /* VACTL */
> > +       },
> > +       {
> > +               .address = 0x14,
> > +               .reg = TPS68470_REG_VDCTL,
> > +               .bitmask = BIT(0),
> > +               /* VDCTL */
> > +       },
> > +};
> > +
> > +/* Table to set voltage regulator value */ static const struct
> > +ti_pmic_table vr_val_table[] = {
> > +       {
> > +               .address = 0x00,
> > +               .reg = TPS68470_REG_VSIOVAL,
> > +               .bitmask = TPS68470_VSIOVAL_IOVOLT_MASK,
> > +               /* TPS68470_REG_VSIOVAL */
> > +       },
> > +       {
> > +               .address = 0x04,
> > +               .reg = TPS68470_REG_VIOVAL,
> > +               .bitmask = TPS68470_VIOVAL_IOVOLT_MASK,
> > +               /* TPS68470_REG_VIOVAL */
> > +       },
> > +       {
> > +               .address = 0x08,
> > +               .reg = TPS68470_REG_VCMVAL,
> > +               .bitmask = TPS68470_VCMVAL_VCVOLT_MASK,
> > +               /* TPS68470_REG_VCMVAL */
> > +       },
> > +       {
> > +               .address = 0x0C,
> > +               .reg = TPS68470_REG_VAUX1VAL,
> > +               .bitmask = TPS68470_VAUX1VAL_AUX1VOLT_MASK,
> > +               /* TPS68470_REG_VAUX1VAL */
> > +       },
> > +       {
> > +               .address = 0x10,
> > +               .reg = TPS68470_REG_VAUX2VAL,
> > +               .bitmask = TPS68470_VAUX2VAL_AUX2VOLT_MASK,
> > +               /* TPS68470_REG_VAUX2VAL */
> > +       },
> > +       {
> > +               .address = 0x14,
> > +               .reg = TPS68470_REG_VAVAL,
> > +               .bitmask = TPS68470_VAVAL_AVOLT_MASK,
> > +               /* TPS68470_REG_VAVAL */
> > +       },
> > +       {
> > +               .address = 0x18,
> > +               .reg = TPS68470_REG_VDVAL,
> > +               .bitmask = TPS68470_VDVAL_DVOLT_MASK,
> > +               /* TPS68470_REG_VDVAL */
> > +       },
> > +};
> > +
> > +/* Table to configure clock frequency */ static const struct
> > +ti_pmic_table clk_freq_table[] = {
> > +       {
> > +               .address = 0x00,
> > +               .reg = TPS68470_REG_POSTDIV2,
> > +               .bitmask = BIT(0) | BIT(1),
> > +               /* TPS68470_REG_POSTDIV2 */
> > +       },
> > +       {
> > +               .address = 0x04,
> > +               .reg = TPS68470_REG_BOOSTDIV,
> > +               .bitmask = 0x1F,
> > +               /* TPS68470_REG_BOOSTDIV */
> > +       },
> > +       {
> > +               .address = 0x08,
> > +               .reg = TPS68470_REG_BUCKDIV,
> > +               .bitmask = 0x0F,
> > +               /* TPS68470_REG_BUCKDIV */
> > +       },
> > +       {
> > +               .address = 0x0C,
> > +               .reg = TPS68470_REG_PLLSWR,
> > +               .bitmask = 0x13,
> > +               /* TPS68470_REG_PLLSWR */
> > +       },
> > +       {
> > +               .address = 0x10,
> > +               .reg = TPS68470_REG_XTALDIV,
> > +               .bitmask = 0xFF,
> > +               /* TPS68470_REG_XTALDIV */
> > +       },
> > +       {
> > +               .address = 0x14,
> > +               .reg = TPS68470_REG_PLLDIV,
> > +               .bitmask = 0xFF,
> > +               /* TPS68470_REG_PLLDIV */
> > +       },
> > +       {
> > +               .address = 0x18,
> > +               .reg = TPS68470_REG_POSTDIV,
> > +               .bitmask = 0x83,
> > +               /* TPS68470_REG_POSTDIV */
> > +       },
> > +};
> > +
> > +/* Table to configure and enable clocks */ static const struct
> > +ti_pmic_table clk_table[] = {
> > +       {
> > +               .address = 0x00,
> > +               .reg = TPS68470_REG_PLLCTL,
> > +               .bitmask = 0xF5,
> > +               /* TPS68470_REG_PLLCTL */
> > +       },
> > +       {
> > +               .address = 0x04,
> > +               .reg = TPS68470_REG_PLLCTL2,
> > +               .bitmask = BIT(0),
> > +               /* TPS68470_REG_PLLCTL2 */
> > +       },
> > +       {
> > +               .address = 0x08,
> > +               .reg = TPS68470_REG_CLKCFG1,
> > +               .bitmask = TPS68470_CLKCFG1_MODE_A_MASK |
> > +                       TPS68470_CLKCFG1_MODE_B_MASK,
> > +               /* TPS68470_REG_CLKCFG1 */
> > +       },
> > +       {
> > +               .address = 0x0C,
> > +               .reg = TPS68470_REG_CLKCFG2,
> > +               .bitmask = TPS68470_CLKCFG1_MODE_A_MASK |
> > +                       TPS68470_CLKCFG1_MODE_B_MASK,
> > +               /* TPS68470_REG_CLKCFG2 */
> > +       },
> > +};
> > +
> > +static int pmic_get_reg_bit(u64 address, struct ti_pmic_table *table,
> > +                           int count, int *reg, int *bitmask) {
> > +       u64 i;
> > +
> 
> > +       i = address / 4;
> 
> 
> > +
> 
> Remove this line.
> 

Ack

> > +       if (i >= count)
> > +               return -ENOENT;
> > +
> > +       if (!reg || !bitmask)
> > +               return -EINVAL;
> > +
> > +       *reg = table[i].reg;
> > +       *bitmask = table[i].bitmask;
> > +
> > +       return 0;
> > +}
> > +
> > +static int ti_tps68470_pmic_get_power(struct regmap *regmap, int reg,
> > +                                      int bitmask, u64 *value) {
> > +       int data;
> > +
> > +       if (regmap_read(regmap, reg, &data))
> > +               return -EIO;
> > +
> > +       *value = (data & bitmask) ? 1 : 0;
> > +       return 0;
> > +}
> > +
> > +static int ti_tps68470_pmic_get_vr_val(struct regmap *regmap, int reg,
> > +                                      int bitmask, u64 *value) {
> > +       int data;
> > +
> > +       if (regmap_read(regmap, reg, &data))
> > +               return -EIO;
> > +
> > +       *value = data & bitmask;
> > +       return 0;
> > +}
> > +
> > +static int ti_tps68470_pmic_get_clk(struct regmap *regmap, int reg,
> > +                                      int bitmask, u64 *value) {
> > +       int data;
> > +
> > +       if (regmap_read(regmap, reg, &data))
> > +               return -EIO;
> > +
> > +       *value = (data & bitmask) ? 1 : 0;
> > +       return 0;
> > +}
> > +
> > +static int ti_tps68470_pmic_get_clk_freq(struct regmap *regmap, int reg,
> > +                                      int bitmask, u64 *value) {
> > +       int data;
> > +
> > +       if (regmap_read(regmap, reg, &data))
> > +               return -EIO;
> > +
> > +       *value = data & bitmask;
> > +       return 0;
> > +}
> > +
> > +static int ti_tps68470_regmap_update_bits(struct regmap *regmap, int reg,
> > +                                       int bitmask, u64 value) {
> > +       return regmap_update_bits(regmap, reg, bitmask, value); }
> > +
> > +static acpi_status ti_pmic_common_handler(u32 function,
> > +                                         acpi_physical_address address,
> > +                                         u32 bits, u64 *value,
> > +                                         void *handler_context,
> > +                                         void *region_context,
> > +                                         int (*get)(struct regmap *,
> > +                                                    int, int, u64 *),
> > +                                         int (*update)(struct regmap *,
> > +                                                       int, int, u64),
> > +                                         struct ti_pmic_table *table,
> > +                                         int table_size) {
> > +       struct ti_pmic_opregion *opregion = region_context;
> > +       struct regmap *regmap = opregion->regmap;
> > +       int reg, ret, bitmask;
> > +
> > +       if (bits != 32)
> > +               return AE_BAD_PARAMETER;
> > +
> > +       ret = pmic_get_reg_bit(address, table,
> > +                                 table_size, &reg, &bitmask);
> > +       if (ret < 0)
> > +               return AE_BAD_PARAMETER;
> > +
> > +       if (function == ACPI_WRITE && (*value > bitmask))
> > +               return AE_BAD_PARAMETER;
> > +
> > +       mutex_lock(&opregion->lock);
> > +
> > +       ret = (function == ACPI_READ) ?
> > +               get(regmap, reg, bitmask, value) :
> > +               update(regmap, reg, bitmask, *value);
> > +
> > +       mutex_unlock(&opregion->lock);
> > +
> > +       return ret ? AE_ERROR : AE_OK; }
> > +
> > +static acpi_status ti_pmic_clk_freq_handler(u32 function,
> > +                                           acpi_physical_address address,
> > +                                           u32 bits, u64 *value,
> > +                                           void *handler_context,
> > +                                           void *region_context) {
> > +       return ti_pmic_common_handler(function, address, bits, value,
> > +                               handler_context, region_context,
> > +                               ti_tps68470_pmic_get_clk_freq,
> > +                               ti_tps68470_regmap_update_bits,
> > +                               (struct ti_pmic_table *) &clk_freq_table,
> > +                               ARRAY_SIZE(clk_freq_table)); }
> > +
> > +static acpi_status ti_pmic_clk_handler(u32 function,
> > +                                      acpi_physical_address address, u32 bits,
> > +                                      u64 *value, void *handler_context,
> > +                                      void *region_context) {
> > +       return ti_pmic_common_handler(function, address, bits, value,
> > +                               handler_context, region_context,
> > +                               ti_tps68470_pmic_get_clk,
> > +                               ti_tps68470_regmap_update_bits,
> > +                               (struct ti_pmic_table *) &clk_table,
> > +                                ARRAY_SIZE(clk_table)); }
> > +
> > +static acpi_status ti_pmic_vr_val_handler(u32 function,
> > +                                         acpi_physical_address address,
> > +                                         u32 bits, u64 *value,
> > +                                         void *handler_context,
> > +                                         void *region_context) {
> > +       return ti_pmic_common_handler(function, address, bits, value,
> > +                               handler_context, region_context,
> > +                               ti_tps68470_pmic_get_vr_val,
> > +                               ti_tps68470_regmap_update_bits,
> > +                               (struct ti_pmic_table *) &vr_val_table,
> > +                               ARRAY_SIZE(vr_val_table)); }
> > +
> > +static acpi_status ti_pmic_power_handler(u32 function,
> > +                                        acpi_physical_address address,
> > +                                        u32 bits, u64 *value,
> > +                                        void *handler_context,
> > +                                        void *region_context) {
> > +       if (bits != 32)
> > +               return AE_BAD_PARAMETER;
> > +
> > +       /* set/clear for bit 0, bits 0 and 1 together */
> > +       if (function == ACPI_WRITE &&
> > +           !(*value == 0 || *value == 1 || *value == 3)) {
> > +               return AE_BAD_PARAMETER;
> > +       }
> > +
> > +       return ti_pmic_common_handler(function, address, bits, value,
> > +                               handler_context, region_context,
> > +                               ti_tps68470_pmic_get_power,
> > +                               ti_tps68470_regmap_update_bits,
> > +                               (struct ti_pmic_table *) &power_table,
> > +                               ARRAY_SIZE(power_table)); }
> > +
> > +static int ti_tps68470_pmic_opregion_probe(struct platform_device
> > +*pdev) {
> > +       struct tps68470 *pmic = dev_get_drvdata(pdev->dev.parent);
> > +       acpi_handle handle = ACPI_HANDLE(pdev->dev.parent);
> > +       struct device *dev = &pdev->dev;
> > +       struct ti_pmic_opregion *opregion;
> > +       acpi_status status;
> > +
> 
> > +       if (!dev || !pmic->regmap) {
> > +               WARN(1, "dev or regmap is NULL\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (!handle) {
> > +               WARN(1, "acpi handle is NULL\n");
> > +               return -ENODEV;
> > +       }
> 
> I dunno if WARNs make user experience any better.
> Besides that I would double check you may have such cases.
> 

Ack
Since the caller is from within the same file and we know the parameters will be set properly, I can change this to dev_warn()

> > +
> > +       opregion = devm_kzalloc(dev, sizeof(*opregion), GFP_KERNEL);
> > +       if (!opregion)
> > +               return -ENOMEM;
> > +
> > +       mutex_init(&opregion->lock);
> > +       opregion->regmap = pmic->regmap;
> > +
> > +       status = acpi_install_address_space_handler(handle,
> > +                                                   TI_PMIC_POWER_OPREGION_ID,
> > +                                                   ti_pmic_power_handler,
> > +                                                   NULL, opregion);
> > +       if (ACPI_FAILURE(status))
> > +               return -ENODEV;
> > +
> > +       status = acpi_install_address_space_handler(handle,
> > +                                                   TI_PMIC_VR_VAL_OPREGION_ID,
> > +                                                   ti_pmic_vr_val_handler,
> > +                                                   NULL, opregion);
> > +       if (ACPI_FAILURE(status))
> > +               goto out_remove_power_handler;
> > +
> > +       status = acpi_install_address_space_handler(handle,
> > +                                                   TI_PMIC_CLOCK_OPREGION_ID,
> > +                                                   ti_pmic_clk_handler,
> > +                                                   NULL, opregion);
> > +       if (ACPI_FAILURE(status))
> > +               goto out_remove_vr_val_handler;
> > +
> > +       status = acpi_install_address_space_handler(handle,
> > +                                                   TI_PMIC_CLKFREQ_OPREGION_ID,
> > +                                                   ti_pmic_clk_freq_handler,
> > +                                                   NULL, opregion);
> > +       if (ACPI_FAILURE(status))
> > +               goto out_remove_clk_handler;
> > +
> > +       return 0;
> > +
> > +out_remove_clk_handler:
> > +       acpi_remove_address_space_handler(handle,
> TI_PMIC_CLOCK_OPREGION_ID,
> > +                                         ti_pmic_clk_handler);
> > +out_remove_vr_val_handler:
> > +       acpi_remove_address_space_handler(handle,
> TI_PMIC_VR_VAL_OPREGION_ID,
> > +                                         ti_pmic_vr_val_handler);
> > +out_remove_power_handler:
> > +       acpi_remove_address_space_handler(handle,
> TI_PMIC_POWER_OPREGION_ID,
> > +                                         ti_pmic_power_handler);
> > +       return -ENODEV;
> > +}
> > +
> > +static struct platform_driver ti_tps68470_pmic_opregion_driver = {
> > +       .probe = ti_tps68470_pmic_opregion_probe,
> > +       .driver = {
> > +               .name = "tps68470_pmic_opregion",
> > +       },
> > +};
> > +
> > +builtin_platform_driver(ti_tps68470_pmic_opregion_driver)
> > --
> > 1.9.1
> >
> 
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Sakari Ailus]

Hi Rajmohan,

Thanks for removing the redundant struct definition. A couple more comments
below. Not really necessarily bugs but a few things to clean things up a
bit.

On Tue, Jun 06, 2017 at 04:55:18AM -0700, Rajmohan Mani wrote:
> The Kabylake platform coreboot (Chrome OS equivalent of
> BIOS) has defined 4 operation regions for the TI TPS68470 PMIC.
> These operation regions are to enable/disable voltage
> regulators, configure voltage regulators, enable/disable
> clocks and to configure clocks.
> 
> This config adds ACPI operation region support for TI TPS68470 PMIC.
> 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 driver enables ACPI operation region support to control voltage
> regulators and clocks for the TPS68470 PMIC.
> 
> Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
> ---
>  drivers/acpi/Kconfig              |  12 +
>  drivers/acpi/Makefile             |   2 +
>  drivers/acpi/pmic/pmic_tps68470.c | 454 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 468 insertions(+)
>  create mode 100644 drivers/acpi/pmic/pmic_tps68470.c
> 
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 1ce52f8..218d22d 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -535,4 +535,16 @@ if ARM64
>  source "drivers/acpi/arm64/Kconfig"
>  endif
>  
> +config TPS68470_PMIC_OPREGION
> +	bool "ACPI operation region support for TPS68470 PMIC"
> +	help
> +	  This config adds ACPI operation region support for TI TPS68470 PMIC.
> +	  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 driver enables ACPI operation region support control voltage
> +	  regulators and clocks.
> +

Extra newline.

> +
>  endif	# ACPI
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index b1aacfc..7113d05 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -106,6 +106,8 @@ obj-$(CONFIG_CHT_WC_PMIC_OPREGION) += pmic/intel_pmic_chtwc.o
>  
>  obj-$(CONFIG_ACPI_CONFIGFS)	+= acpi_configfs.o
>  
> +obj-$(CONFIG_TPS68470_PMIC_OPREGION)	+= pmic/pmic_tps68470.o
> +
>  video-objs			+= acpi_video.o video_detect.o
>  obj-y				+= dptf/
>  
> diff --git a/drivers/acpi/pmic/pmic_tps68470.c b/drivers/acpi/pmic/pmic_tps68470.c
> new file mode 100644
> index 0000000..b2d608b
> --- /dev/null
> +++ b/drivers/acpi/pmic/pmic_tps68470.c
> @@ -0,0 +1,454 @@
> +/*
> + * TI TPS68470 PMIC operation region driver
> + *
> + * Copyright (C) 2017 Intel Corporation. All rights reserved.
> + * Author: Rajmohan Mani <rajmohan.mani@intel.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version
> + * 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * Based on drivers/acpi/pmic/intel_pmic* drivers
> + *
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/mfd/tps68470.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +struct ti_pmic_table {
> +	u32 address;		/* operation region address */
> +	u32 reg;		/* corresponding register */
> +	u32 bitmask;		/* bit mask for power, clock */
> +};
> +
> +#define TI_PMIC_POWER_OPREGION_ID		0xB0
> +#define TI_PMIC_VR_VAL_OPREGION_ID		0xB1
> +#define TI_PMIC_CLOCK_OPREGION_ID		0xB2
> +#define TI_PMIC_CLKFREQ_OPREGION_ID		0xB3
> +
> +struct ti_pmic_opregion {
> +	struct mutex lock;
> +	struct regmap *regmap;
> +};
> +
> +#define S_IO_I2C_EN	(BIT(0) | BIT(1))
> +
> +static const struct ti_pmic_table power_table[] = {
> +	{
> +		.address = 0x00,
> +		.reg = TPS68470_REG_S_I2C_CTL,
> +		.bitmask = S_IO_I2C_EN,
> +		/* S_I2C_CTL */
> +	},
> +	{
> +		.address = 0x04,
> +		.reg = TPS68470_REG_VCMCTL,
> +		.bitmask = BIT(0),
> +		/* VCMCTL */
> +	},
> +	{
> +		.address = 0x08,
> +		.reg = TPS68470_REG_VAUX1CTL,
> +		.bitmask = BIT(0),
> +		/* VAUX1_CTL */
> +	},
> +	{
> +		.address = 0x0C,
> +		.reg = TPS68470_REG_VAUX2CTL,
> +		.bitmask = BIT(0),
> +		/* VAUX2CTL */
> +	},
> +	{
> +		.address = 0x10,
> +		.reg = TPS68470_REG_VACTL,
> +		.bitmask = BIT(0),
> +		/* VACTL */
> +	},
> +	{
> +		.address = 0x14,
> +		.reg = TPS68470_REG_VDCTL,
> +		.bitmask = BIT(0),
> +		/* VDCTL */
> +	},
> +};
> +
> +/* Table to set voltage regulator value */
> +static const struct ti_pmic_table vr_val_table[] = {
> +	{
> +		.address = 0x00,
> +		.reg = TPS68470_REG_VSIOVAL,
> +		.bitmask = TPS68470_VSIOVAL_IOVOLT_MASK,
> +		/* TPS68470_REG_VSIOVAL */
> +	},
> +	{
> +		.address = 0x04,
> +		.reg = TPS68470_REG_VIOVAL,
> +		.bitmask = TPS68470_VIOVAL_IOVOLT_MASK,
> +		/* TPS68470_REG_VIOVAL */
> +	},
> +	{
> +		.address = 0x08,
> +		.reg = TPS68470_REG_VCMVAL,
> +		.bitmask = TPS68470_VCMVAL_VCVOLT_MASK,
> +		/* TPS68470_REG_VCMVAL */
> +	},
> +	{
> +		.address = 0x0C,
> +		.reg = TPS68470_REG_VAUX1VAL,
> +		.bitmask = TPS68470_VAUX1VAL_AUX1VOLT_MASK,
> +		/* TPS68470_REG_VAUX1VAL */
> +	},
> +	{
> +		.address = 0x10,
> +		.reg = TPS68470_REG_VAUX2VAL,
> +		.bitmask = TPS68470_VAUX2VAL_AUX2VOLT_MASK,
> +		/* TPS68470_REG_VAUX2VAL */
> +	},
> +	{
> +		.address = 0x14,
> +		.reg = TPS68470_REG_VAVAL,
> +		.bitmask = TPS68470_VAVAL_AVOLT_MASK,
> +		/* TPS68470_REG_VAVAL */
> +	},
> +	{
> +		.address = 0x18,
> +		.reg = TPS68470_REG_VDVAL,
> +		.bitmask = TPS68470_VDVAL_DVOLT_MASK,
> +		/* TPS68470_REG_VDVAL */
> +	},
> +};
> +
> +/* Table to configure clock frequency */
> +static const struct ti_pmic_table clk_freq_table[] = {
> +	{
> +		.address = 0x00,
> +		.reg = TPS68470_REG_POSTDIV2,
> +		.bitmask = BIT(0) | BIT(1),
> +		/* TPS68470_REG_POSTDIV2 */
> +	},
> +	{
> +		.address = 0x04,
> +		.reg = TPS68470_REG_BOOSTDIV,
> +		.bitmask = 0x1F,
> +		/* TPS68470_REG_BOOSTDIV */
> +	},
> +	{
> +		.address = 0x08,
> +		.reg = TPS68470_REG_BUCKDIV,
> +		.bitmask = 0x0F,
> +		/* TPS68470_REG_BUCKDIV */
> +	},
> +	{
> +		.address = 0x0C,
> +		.reg = TPS68470_REG_PLLSWR,
> +		.bitmask = 0x13,
> +		/* TPS68470_REG_PLLSWR */
> +	},
> +	{
> +		.address = 0x10,
> +		.reg = TPS68470_REG_XTALDIV,
> +		.bitmask = 0xFF,
> +		/* TPS68470_REG_XTALDIV */
> +	},
> +	{
> +		.address = 0x14,
> +		.reg = TPS68470_REG_PLLDIV,
> +		.bitmask = 0xFF,
> +		/* TPS68470_REG_PLLDIV */
> +	},
> +	{
> +		.address = 0x18,
> +		.reg = TPS68470_REG_POSTDIV,
> +		.bitmask = 0x83,
> +		/* TPS68470_REG_POSTDIV */
> +	},
> +};
> +
> +/* Table to configure and enable clocks */
> +static const struct ti_pmic_table clk_table[] = {
> +	{
> +		.address = 0x00,
> +		.reg = TPS68470_REG_PLLCTL,
> +		.bitmask = 0xF5,
> +		/* TPS68470_REG_PLLCTL */
> +	},
> +	{
> +		.address = 0x04,
> +		.reg = TPS68470_REG_PLLCTL2,
> +		.bitmask = BIT(0),
> +		/* TPS68470_REG_PLLCTL2 */
> +	},
> +	{
> +		.address = 0x08,
> +		.reg = TPS68470_REG_CLKCFG1,
> +		.bitmask = TPS68470_CLKCFG1_MODE_A_MASK |
> +			TPS68470_CLKCFG1_MODE_B_MASK,
> +		/* TPS68470_REG_CLKCFG1 */
> +	},
> +	{
> +		.address = 0x0C,
> +		.reg = TPS68470_REG_CLKCFG2,
> +		.bitmask = TPS68470_CLKCFG1_MODE_A_MASK |
> +			TPS68470_CLKCFG1_MODE_B_MASK,
> +		/* TPS68470_REG_CLKCFG2 */
> +	},
> +};
> +
> +static int pmic_get_reg_bit(u64 address, struct ti_pmic_table *table,
> +			    int count, int *reg, int *bitmask)

unsigned int count?

> +{
> +	u64 i;
> +
> +	i = address / 4;
> +
> +	if (i >= count)
> +		return -ENOENT;
> +
> +	if (!reg || !bitmask)
> +		return -EINVAL;
> +
> +	*reg = table[i].reg;
> +	*bitmask = table[i].bitmask;
> +
> +	return 0;
> +}
> +
> +static int ti_tps68470_pmic_get_power(struct regmap *regmap, int reg,
> +				       int bitmask, u64 *value)
> +{
> +	int data;

Shouldn't you use unsigned int here? Same in the functions below.

> +
> +	if (regmap_read(regmap, reg, &data))
> +		return -EIO;
> +
> +	*value = (data & bitmask) ? 1 : 0;
> +	return 0;
> +}
> +
> +static int ti_tps68470_pmic_get_vr_val(struct regmap *regmap, int reg,
> +				       int bitmask, u64 *value)
> +{
> +	int data;
> +
> +	if (regmap_read(regmap, reg, &data))
> +		return -EIO;
> +
> +	*value = data & bitmask;
> +	return 0;
> +}
> +
> +static int ti_tps68470_pmic_get_clk(struct regmap *regmap, int reg,
> +				       int bitmask, u64 *value)
> +{
> +	int data;
> +
> +	if (regmap_read(regmap, reg, &data))
> +		return -EIO;
> +
> +	*value = (data & bitmask) ? 1 : 0;
> +	return 0;
> +}
> +
> +static int ti_tps68470_pmic_get_clk_freq(struct regmap *regmap, int reg,
> +				       int bitmask, u64 *value)
> +{
> +	int data;
> +
> +	if (regmap_read(regmap, reg, &data))
> +		return -EIO;
> +
> +	*value = data & bitmask;
> +	return 0;
> +}
> +
> +static int ti_tps68470_regmap_update_bits(struct regmap *regmap, int reg,
> +					int bitmask, u64 value)
> +{
> +	return regmap_update_bits(regmap, reg, bitmask, value);
> +}
> +
> +static acpi_status ti_pmic_common_handler(u32 function,
> +					  acpi_physical_address address,
> +					  u32 bits, u64 *value,
> +					  void *handler_context,

handler_context is unused.

> +					  void *region_context,
> +					  int (*get)(struct regmap *,
> +						     int, int, u64 *),
> +					  int (*update)(struct regmap *,
> +							int, int, u64),
> +					  struct ti_pmic_table *table,
> +					  int table_size)

unsigned int. (Or size_t or whatever.)

> +{
> +	struct ti_pmic_opregion *opregion = region_context;
> +	struct regmap *regmap = opregion->regmap;
> +	int reg, ret, bitmask;
> +
> +	if (bits != 32)
> +		return AE_BAD_PARAMETER;
> +
> +	ret = pmic_get_reg_bit(address, table,
> +				  table_size, &reg, &bitmask);
> +	if (ret < 0)
> +		return AE_BAD_PARAMETER;
> +
> +	if (function == ACPI_WRITE && (*value > bitmask))

Extra parentheses.

> +		return AE_BAD_PARAMETER;
> +
> +	mutex_lock(&opregion->lock);
> +
> +	ret = (function == ACPI_READ) ?
> +		get(regmap, reg, bitmask, value) :
> +		update(regmap, reg, bitmask, *value);
> +
> +	mutex_unlock(&opregion->lock);
> +
> +	return ret ? AE_ERROR : AE_OK;
> +}
> +
> +static acpi_status ti_pmic_clk_freq_handler(u32 function,
> +					    acpi_physical_address address,
> +					    u32 bits, u64 *value,
> +					    void *handler_context,
> +					    void *region_context)
> +{
> +	return ti_pmic_common_handler(function, address, bits, value,
> +				handler_context, region_context,
> +				ti_tps68470_pmic_get_clk_freq,
> +				ti_tps68470_regmap_update_bits,
> +				(struct ti_pmic_table *) &clk_freq_table,

You shouldn't use an explicit cast here. Instead make the function argument
const as well and you're fine.

> +				ARRAY_SIZE(clk_freq_table));
> +}
> +
> +static acpi_status ti_pmic_clk_handler(u32 function,
> +				       acpi_physical_address address, u32 bits,
> +				       u64 *value, void *handler_context,
> +				       void *region_context)
> +{
> +	return ti_pmic_common_handler(function, address, bits, value,
> +				handler_context, region_context,
> +				ti_tps68470_pmic_get_clk,
> +				ti_tps68470_regmap_update_bits,
> +				(struct ti_pmic_table *) &clk_table,
> +				 ARRAY_SIZE(clk_table));
> +}
> +
> +static acpi_status ti_pmic_vr_val_handler(u32 function,
> +					  acpi_physical_address address,
> +					  u32 bits, u64 *value,
> +					  void *handler_context,
> +					  void *region_context)
> +{
> +	return ti_pmic_common_handler(function, address, bits, value,
> +				handler_context, region_context,
> +				ti_tps68470_pmic_get_vr_val,
> +				ti_tps68470_regmap_update_bits,
> +				(struct ti_pmic_table *) &vr_val_table,
> +				ARRAY_SIZE(vr_val_table));
> +}
> +
> +static acpi_status ti_pmic_power_handler(u32 function,
> +					 acpi_physical_address address,
> +					 u32 bits, u64 *value,
> +					 void *handler_context,
> +					 void *region_context)
> +{
> +	if (bits != 32)
> +		return AE_BAD_PARAMETER;
> +
> +	/* set/clear for bit 0, bits 0 and 1 together */
> +	if (function == ACPI_WRITE &&
> +	    !(*value == 0 || *value == 1 || *value == 3)) {
> +		return AE_BAD_PARAMETER;
> +	}
> +
> +	return ti_pmic_common_handler(function, address, bits, value,
> +				handler_context, region_context,
> +				ti_tps68470_pmic_get_power,
> +				ti_tps68470_regmap_update_bits,
> +				(struct ti_pmic_table *) &power_table,
> +				ARRAY_SIZE(power_table));
> +}
> +
> +static int ti_tps68470_pmic_opregion_probe(struct platform_device *pdev)
> +{
> +	struct tps68470 *pmic = dev_get_drvdata(pdev->dev.parent);
> +	acpi_handle handle = ACPI_HANDLE(pdev->dev.parent);
> +	struct device *dev = &pdev->dev;
> +	struct ti_pmic_opregion *opregion;
> +	acpi_status status;
> +
> +	if (!dev || !pmic->regmap) {
> +		WARN(1, "dev or regmap is NULL\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!handle) {
> +		WARN(1, "acpi handle is NULL\n");
> +		return -ENODEV;
> +	}
> +
> +	opregion = devm_kzalloc(dev, sizeof(*opregion), GFP_KERNEL);
> +	if (!opregion)
> +		return -ENOMEM;
> +
> +	mutex_init(&opregion->lock);
> +	opregion->regmap = pmic->regmap;
> +
> +	status = acpi_install_address_space_handler(handle,
> +						    TI_PMIC_POWER_OPREGION_ID,
> +						    ti_pmic_power_handler,
> +						    NULL, opregion);
> +	if (ACPI_FAILURE(status))

mutex_destroy() after mutex_init() --- please add a label for this.

> +		return -ENODEV;
> +
> +	status = acpi_install_address_space_handler(handle,
> +						    TI_PMIC_VR_VAL_OPREGION_ID,
> +						    ti_pmic_vr_val_handler,
> +						    NULL, opregion);
> +	if (ACPI_FAILURE(status))
> +		goto out_remove_power_handler;
> +
> +	status = acpi_install_address_space_handler(handle,
> +						    TI_PMIC_CLOCK_OPREGION_ID,
> +						    ti_pmic_clk_handler,
> +						    NULL, opregion);
> +	if (ACPI_FAILURE(status))
> +		goto out_remove_vr_val_handler;
> +
> +	status = acpi_install_address_space_handler(handle,
> +						    TI_PMIC_CLKFREQ_OPREGION_ID,
> +						    ti_pmic_clk_freq_handler,
> +						    NULL, opregion);
> +	if (ACPI_FAILURE(status))
> +		goto out_remove_clk_handler;
> +
> +	return 0;
> +
> +out_remove_clk_handler:
> +	acpi_remove_address_space_handler(handle, TI_PMIC_CLOCK_OPREGION_ID,
> +					  ti_pmic_clk_handler);
> +out_remove_vr_val_handler:
> +	acpi_remove_address_space_handler(handle, TI_PMIC_VR_VAL_OPREGION_ID,
> +					  ti_pmic_vr_val_handler);
> +out_remove_power_handler:
> +	acpi_remove_address_space_handler(handle, TI_PMIC_POWER_OPREGION_ID,
> +					  ti_pmic_power_handler);
> +	return -ENODEV;
> +}
> +
> +static struct platform_driver ti_tps68470_pmic_opregion_driver = {
> +	.probe = ti_tps68470_pmic_opregion_probe,
> +	.driver = {
> +		.name = "tps68470_pmic_opregion",
> +	},
> +};
> +
> +builtin_platform_driver(ti_tps68470_pmic_opregion_driver)

-- 
Kind regards,

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

Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Andy Shevchenko]

On Wed, Jun 7, 2017 at 3:07 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:

>> +static int ti_tps68470_pmic_get_power(struct regmap *regmap, int reg,
>> +                                    int bitmask, u64 *value)
>> +{
>> +     int data;
>
> Shouldn't you use unsigned int here? Same in the functions below.

+1, regmap_read() returns unsigned int.

>> +static acpi_status ti_pmic_common_handler(u32 function,
> +                                       acpi_physical_address address,
> +                                       u32 bits, u64 *value,
> +                                       void *handler_context,

> handler_context is unused.

>> +                                                  int, int, u64 *),
>> +                                       int (*update)(struct regmap *,
>> +                                                     int, int, u64),
>> +                                       struct ti_pmic_table *table,
>> +                                       int table_size)

I would even split this to have separate update() and get() paths
instead of having such a monster of parameters.

>> +static acpi_status ti_pmic_clk_freq_handler(u32 function,
>> +                                         acpi_physical_address address,
>> +                                         u32 bits, u64 *value,
>> +                                         void *handler_context,
>> +                                         void *region_context)
>> +{
>> +     return ti_pmic_common_handler(function, address, bits, value,
>> +                             handler_context, region_context,
>> +                             ti_tps68470_pmic_get_clk_freq,
>> +                             ti_tps68470_regmap_update_bits,
>> +                             (struct ti_pmic_table *) &clk_freq_table,
>
> You shouldn't use an explicit cast here. Instead make the function argument
> const as well and you're fine.

+1.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Sakari Ailus]

On Wed, Jun 07, 2017 at 04:37:12PM +0300, Andy Shevchenko wrote:
> >> +static acpi_status ti_pmic_common_handler(u32 function,
> > +                                       acpi_physical_address address,
> > +                                       u32 bits, u64 *value,
> > +                                       void *handler_context,
> 
> > handler_context is unused.
> 
> >> +                                                  int, int, u64 *),
> >> +                                       int (*update)(struct regmap *,
> >> +                                                     int, int, u64),
> >> +                                       struct ti_pmic_table *table,
> >> +                                       int table_size)
> 
> I would even split this to have separate update() and get() paths
> instead of having such a monster of parameters.

I'm not really worried about the two callbacks --- you have the compexity,
which is agruably rather manageable, split into a number of caller
functions. I'd rather keep it as-is.

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

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Sakari, Andy,

> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> On Wed, Jun 07, 2017 at 04:37:12PM +0300, Andy Shevchenko wrote:
> > >> +static acpi_status ti_pmic_common_handler(u32 function,
> > > +                                       acpi_physical_address address,
> > > +                                       u32 bits, u64 *value,
> > > +                                       void *handler_context,
> >
> > > handler_context is unused.
> >
> > >> +                                                  int, int, u64 *),
> > >> +                                       int (*update)(struct regmap *,
> > >> +                                                     int, int, u64),
> > >> +                                       struct ti_pmic_table *table,
> > >> +                                       int table_size)
> >
> > I would even split this to have separate update() and get() paths
> > instead of having such a monster of parameters.
> 
> I'm not really worried about the two callbacks --- you have the compexity,
> which is agruably rather manageable, split into a number of caller functions. I'd
> rather keep it as-is.
> 

Ack

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Andy,

Thanks for the reviews and patience.

> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> On Wed, Jun 7, 2017 at 3:07 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> 
> >> +static int ti_tps68470_pmic_get_power(struct regmap *regmap, int reg,
> >> +                                    int bitmask, u64 *value) {
> >> +     int data;
> >
> > Shouldn't you use unsigned int here? Same in the functions below.
> 
> +1, regmap_read() returns unsigned int.
> 

Ack

> >> +static acpi_status ti_pmic_common_handler(u32 function,
> > +                                       acpi_physical_address address,
> > +                                       u32 bits, u64 *value,
> > +                                       void *handler_context,
> 
> > handler_context is unused.
> 

Ack

> >> +                                                  int, int, u64 *),
> >> +                                       int (*update)(struct regmap *,
> >> +                                                     int, int, u64),
> >> +                                       struct ti_pmic_table *table,
> >> +                                       int table_size)
> 
> I would even split this to have separate update() and get() paths instead of
> having such a monster of parameters.
> 

I have responded on top of Sakari's response.

> >> +static acpi_status ti_pmic_clk_freq_handler(u32 function,
> >> +                                         acpi_physical_address address,
> >> +                                         u32 bits, u64 *value,
> >> +                                         void *handler_context,
> >> +                                         void *region_context) {
> >> +     return ti_pmic_common_handler(function, address, bits, value,
> >> +                             handler_context, region_context,
> >> +                             ti_tps68470_pmic_get_clk_freq,
> >> +                             ti_tps68470_regmap_update_bits,
> >> +                             (struct ti_pmic_table *)
> >> +&clk_freq_table,
> >
> > You shouldn't use an explicit cast here. Instead make the function
> > argument const as well and you're fine.
> 
> +1.
> 

Ack

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Sakari,

Thanks for the reviews.

> -----Original Message-----
> From: Sakari Ailus [mailto:sakari.ailus@iki.fi]
> Sent: Wednesday, June 07, 2017 5:08 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 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> Hi Rajmohan,
> 
> Thanks for removing the redundant struct definition. A couple more comments
> below. Not really necessarily bugs but a few things to clean things up a bit.
> 

Ack

> On Tue, Jun 06, 2017 at 04:55:18AM -0700, Rajmohan Mani wrote:
> > The Kabylake platform coreboot (Chrome OS equivalent of
> > BIOS) has defined 4 operation regions for the TI TPS68470 PMIC.
> > These operation regions are to enable/disable voltage regulators,
> > configure voltage regulators, enable/disable clocks and to configure
> > clocks.
> >
> > This config adds ACPI operation region support for TI TPS68470 PMIC.
> > 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 driver enables ACPI operation region support to control voltage
> > regulators and clocks for the TPS68470 PMIC.
> >
> > Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
> > ---
> >  drivers/acpi/Kconfig              |  12 +
> >  drivers/acpi/Makefile             |   2 +
> >  drivers/acpi/pmic/pmic_tps68470.c | 454
> > ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 468 insertions(+)
> >  create mode 100644 drivers/acpi/pmic/pmic_tps68470.c
> >
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index
> > 1ce52f8..218d22d 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -535,4 +535,16 @@ if ARM64
> >  source "drivers/acpi/arm64/Kconfig"
> >  endif
> >
> > +config TPS68470_PMIC_OPREGION
> > +	bool "ACPI operation region support for TPS68470 PMIC"
> > +	help
> > +	  This config adds ACPI operation region support for TI TPS68470 PMIC.
> > +	  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 driver enables ACPI operation region support control voltage
> > +	  regulators and clocks.
> > +
> 
> Extra newline.
> 

Ack

> > +
> >  endif	# ACPI
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index
> > b1aacfc..7113d05 100644
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -106,6 +106,8 @@ obj-$(CONFIG_CHT_WC_PMIC_OPREGION) +=
> > pmic/intel_pmic_chtwc.o
> >
> >  obj-$(CONFIG_ACPI_CONFIGFS)	+= acpi_configfs.o
> >
> > +obj-$(CONFIG_TPS68470_PMIC_OPREGION)	+= pmic/pmic_tps68470.o
> > +
> >  video-objs			+= acpi_video.o video_detect.o
> >  obj-y				+= dptf/
> >
> > diff --git a/drivers/acpi/pmic/pmic_tps68470.c
> > b/drivers/acpi/pmic/pmic_tps68470.c
> > new file mode 100644
> > index 0000000..b2d608b
> > --- /dev/null
> > +++ b/drivers/acpi/pmic/pmic_tps68470.c
> > @@ -0,0 +1,454 @@
> > +/*
> > + * TI TPS68470 PMIC operation region driver
> > + *
> > + * Copyright (C) 2017 Intel Corporation. All rights reserved.
> > + * Author: Rajmohan Mani <rajmohan.mani@intel.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > +version
> > + * 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * Based on drivers/acpi/pmic/intel_pmic* drivers
> > + *
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/mfd/tps68470.h>
> > +#include <linux/init.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +
> > +struct ti_pmic_table {
> > +	u32 address;		/* operation region address */
> > +	u32 reg;		/* corresponding register */
> > +	u32 bitmask;		/* bit mask for power, clock */
> > +};
> > +
> > +#define TI_PMIC_POWER_OPREGION_ID		0xB0
> > +#define TI_PMIC_VR_VAL_OPREGION_ID		0xB1
> > +#define TI_PMIC_CLOCK_OPREGION_ID		0xB2
> > +#define TI_PMIC_CLKFREQ_OPREGION_ID		0xB3
> > +
> > +struct ti_pmic_opregion {
> > +	struct mutex lock;
> > +	struct regmap *regmap;
> > +};
> > +
> > +#define S_IO_I2C_EN	(BIT(0) | BIT(1))
> > +
> > +static const struct ti_pmic_table power_table[] = {
> > +	{
> > +		.address = 0x00,
> > +		.reg = TPS68470_REG_S_I2C_CTL,
> > +		.bitmask = S_IO_I2C_EN,
> > +		/* S_I2C_CTL */
> > +	},
> > +	{
> > +		.address = 0x04,
> > +		.reg = TPS68470_REG_VCMCTL,
> > +		.bitmask = BIT(0),
> > +		/* VCMCTL */
> > +	},
> > +	{
> > +		.address = 0x08,
> > +		.reg = TPS68470_REG_VAUX1CTL,
> > +		.bitmask = BIT(0),
> > +		/* VAUX1_CTL */
> > +	},
> > +	{
> > +		.address = 0x0C,
> > +		.reg = TPS68470_REG_VAUX2CTL,
> > +		.bitmask = BIT(0),
> > +		/* VAUX2CTL */
> > +	},
> > +	{
> > +		.address = 0x10,
> > +		.reg = TPS68470_REG_VACTL,
> > +		.bitmask = BIT(0),
> > +		/* VACTL */
> > +	},
> > +	{
> > +		.address = 0x14,
> > +		.reg = TPS68470_REG_VDCTL,
> > +		.bitmask = BIT(0),
> > +		/* VDCTL */
> > +	},
> > +};
> > +
> > +/* Table to set voltage regulator value */ static const struct
> > +ti_pmic_table vr_val_table[] = {
> > +	{
> > +		.address = 0x00,
> > +		.reg = TPS68470_REG_VSIOVAL,
> > +		.bitmask = TPS68470_VSIOVAL_IOVOLT_MASK,
> > +		/* TPS68470_REG_VSIOVAL */
> > +	},
> > +	{
> > +		.address = 0x04,
> > +		.reg = TPS68470_REG_VIOVAL,
> > +		.bitmask = TPS68470_VIOVAL_IOVOLT_MASK,
> > +		/* TPS68470_REG_VIOVAL */
> > +	},
> > +	{
> > +		.address = 0x08,
> > +		.reg = TPS68470_REG_VCMVAL,
> > +		.bitmask = TPS68470_VCMVAL_VCVOLT_MASK,
> > +		/* TPS68470_REG_VCMVAL */
> > +	},
> > +	{
> > +		.address = 0x0C,
> > +		.reg = TPS68470_REG_VAUX1VAL,
> > +		.bitmask = TPS68470_VAUX1VAL_AUX1VOLT_MASK,
> > +		/* TPS68470_REG_VAUX1VAL */
> > +	},
> > +	{
> > +		.address = 0x10,
> > +		.reg = TPS68470_REG_VAUX2VAL,
> > +		.bitmask = TPS68470_VAUX2VAL_AUX2VOLT_MASK,
> > +		/* TPS68470_REG_VAUX2VAL */
> > +	},
> > +	{
> > +		.address = 0x14,
> > +		.reg = TPS68470_REG_VAVAL,
> > +		.bitmask = TPS68470_VAVAL_AVOLT_MASK,
> > +		/* TPS68470_REG_VAVAL */
> > +	},
> > +	{
> > +		.address = 0x18,
> > +		.reg = TPS68470_REG_VDVAL,
> > +		.bitmask = TPS68470_VDVAL_DVOLT_MASK,
> > +		/* TPS68470_REG_VDVAL */
> > +	},
> > +};
> > +
> > +/* Table to configure clock frequency */ static const struct
> > +ti_pmic_table clk_freq_table[] = {
> > +	{
> > +		.address = 0x00,
> > +		.reg = TPS68470_REG_POSTDIV2,
> > +		.bitmask = BIT(0) | BIT(1),
> > +		/* TPS68470_REG_POSTDIV2 */
> > +	},
> > +	{
> > +		.address = 0x04,
> > +		.reg = TPS68470_REG_BOOSTDIV,
> > +		.bitmask = 0x1F,
> > +		/* TPS68470_REG_BOOSTDIV */
> > +	},
> > +	{
> > +		.address = 0x08,
> > +		.reg = TPS68470_REG_BUCKDIV,
> > +		.bitmask = 0x0F,
> > +		/* TPS68470_REG_BUCKDIV */
> > +	},
> > +	{
> > +		.address = 0x0C,
> > +		.reg = TPS68470_REG_PLLSWR,
> > +		.bitmask = 0x13,
> > +		/* TPS68470_REG_PLLSWR */
> > +	},
> > +	{
> > +		.address = 0x10,
> > +		.reg = TPS68470_REG_XTALDIV,
> > +		.bitmask = 0xFF,
> > +		/* TPS68470_REG_XTALDIV */
> > +	},
> > +	{
> > +		.address = 0x14,
> > +		.reg = TPS68470_REG_PLLDIV,
> > +		.bitmask = 0xFF,
> > +		/* TPS68470_REG_PLLDIV */
> > +	},
> > +	{
> > +		.address = 0x18,
> > +		.reg = TPS68470_REG_POSTDIV,
> > +		.bitmask = 0x83,
> > +		/* TPS68470_REG_POSTDIV */
> > +	},
> > +};
> > +
> > +/* Table to configure and enable clocks */ static const struct
> > +ti_pmic_table clk_table[] = {
> > +	{
> > +		.address = 0x00,
> > +		.reg = TPS68470_REG_PLLCTL,
> > +		.bitmask = 0xF5,
> > +		/* TPS68470_REG_PLLCTL */
> > +	},
> > +	{
> > +		.address = 0x04,
> > +		.reg = TPS68470_REG_PLLCTL2,
> > +		.bitmask = BIT(0),
> > +		/* TPS68470_REG_PLLCTL2 */
> > +	},
> > +	{
> > +		.address = 0x08,
> > +		.reg = TPS68470_REG_CLKCFG1,
> > +		.bitmask = TPS68470_CLKCFG1_MODE_A_MASK |
> > +			TPS68470_CLKCFG1_MODE_B_MASK,
> > +		/* TPS68470_REG_CLKCFG1 */
> > +	},
> > +	{
> > +		.address = 0x0C,
> > +		.reg = TPS68470_REG_CLKCFG2,
> > +		.bitmask = TPS68470_CLKCFG1_MODE_A_MASK |
> > +			TPS68470_CLKCFG1_MODE_B_MASK,
> > +		/* TPS68470_REG_CLKCFG2 */
> > +	},
> > +};
> > +
> > +static int pmic_get_reg_bit(u64 address, struct ti_pmic_table *table,
> > +			    int count, int *reg, int *bitmask)
> 
> unsigned int count?
> 

Ack
I will also rename the variable to be table_size, so it's more meaningful.

> > +{
> > +	u64 i;
> > +
> > +	i = address / 4;
> > +
> > +	if (i >= count)
> > +		return -ENOENT;
> > +
> > +	if (!reg || !bitmask)
> > +		return -EINVAL;
> > +
> > +	*reg = table[i].reg;
> > +	*bitmask = table[i].bitmask;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ti_tps68470_pmic_get_power(struct regmap *regmap, int reg,
> > +				       int bitmask, u64 *value)
> > +{
> > +	int data;
> 
> Shouldn't you use unsigned int here? Same in the functions below.
> 

Ack

> > +
> > +	if (regmap_read(regmap, reg, &data))
> > +		return -EIO;
> > +
> > +	*value = (data & bitmask) ? 1 : 0;
> > +	return 0;
> > +}
> > +
> > +static int ti_tps68470_pmic_get_vr_val(struct regmap *regmap, int reg,
> > +				       int bitmask, u64 *value)
> > +{
> > +	int data;
> > +
> > +	if (regmap_read(regmap, reg, &data))
> > +		return -EIO;
> > +
> > +	*value = data & bitmask;
> > +	return 0;
> > +}
> > +
> > +static int ti_tps68470_pmic_get_clk(struct regmap *regmap, int reg,
> > +				       int bitmask, u64 *value)
> > +{
> > +	int data;
> > +
> > +	if (regmap_read(regmap, reg, &data))
> > +		return -EIO;
> > +
> > +	*value = (data & bitmask) ? 1 : 0;
> > +	return 0;
> > +}
> > +
> > +static int ti_tps68470_pmic_get_clk_freq(struct regmap *regmap, int reg,
> > +				       int bitmask, u64 *value)
> > +{
> > +	int data;
> > +
> > +	if (regmap_read(regmap, reg, &data))
> > +		return -EIO;
> > +
> > +	*value = data & bitmask;
> > +	return 0;
> > +}
> > +
> > +static int ti_tps68470_regmap_update_bits(struct regmap *regmap, int reg,
> > +					int bitmask, u64 value)
> > +{
> > +	return regmap_update_bits(regmap, reg, bitmask, value); }
> > +
> > +static acpi_status ti_pmic_common_handler(u32 function,
> > +					  acpi_physical_address address,
> > +					  u32 bits, u64 *value,
> > +					  void *handler_context,
> 
> handler_context is unused.
> 

Ack

> > +					  void *region_context,
> > +					  int (*get)(struct regmap *,
> > +						     int, int, u64 *),
> > +					  int (*update)(struct regmap *,
> > +							int, int, u64),
> > +					  struct ti_pmic_table *table,
> > +					  int table_size)
> 
> unsigned int. (Or size_t or whatever.)
> 

Ack

> > +{
> > +	struct ti_pmic_opregion *opregion = region_context;
> > +	struct regmap *regmap = opregion->regmap;
> > +	int reg, ret, bitmask;
> > +
> > +	if (bits != 32)
> > +		return AE_BAD_PARAMETER;
> > +
> > +	ret = pmic_get_reg_bit(address, table,
> > +				  table_size, &reg, &bitmask);
> > +	if (ret < 0)
> > +		return AE_BAD_PARAMETER;
> > +
> > +	if (function == ACPI_WRITE && (*value > bitmask))
> 
> Extra parentheses.
> 

Ack

> > +		return AE_BAD_PARAMETER;
> > +
> > +	mutex_lock(&opregion->lock);
> > +
> > +	ret = (function == ACPI_READ) ?
> > +		get(regmap, reg, bitmask, value) :
> > +		update(regmap, reg, bitmask, *value);
> > +
> > +	mutex_unlock(&opregion->lock);
> > +
> > +	return ret ? AE_ERROR : AE_OK;
> > +}
> > +
> > +static acpi_status ti_pmic_clk_freq_handler(u32 function,
> > +					    acpi_physical_address address,
> > +					    u32 bits, u64 *value,
> > +					    void *handler_context,
> > +					    void *region_context)
> > +{
> > +	return ti_pmic_common_handler(function, address, bits, value,
> > +				handler_context, region_context,
> > +				ti_tps68470_pmic_get_clk_freq,
> > +				ti_tps68470_regmap_update_bits,
> > +				(struct ti_pmic_table *) &clk_freq_table,
> 
> You shouldn't use an explicit cast here. Instead make the function argument
> const as well and you're fine.
> 

Ack

> > +				ARRAY_SIZE(clk_freq_table));
> > +}
> > +
> > +static acpi_status ti_pmic_clk_handler(u32 function,
> > +				       acpi_physical_address address, u32 bits,
> > +				       u64 *value, void *handler_context,
> > +				       void *region_context)
> > +{
> > +	return ti_pmic_common_handler(function, address, bits, value,
> > +				handler_context, region_context,
> > +				ti_tps68470_pmic_get_clk,
> > +				ti_tps68470_regmap_update_bits,
> > +				(struct ti_pmic_table *) &clk_table,
> > +				 ARRAY_SIZE(clk_table));
> > +}
> > +
> > +static acpi_status ti_pmic_vr_val_handler(u32 function,
> > +					  acpi_physical_address address,
> > +					  u32 bits, u64 *value,
> > +					  void *handler_context,
> > +					  void *region_context)
> > +{
> > +	return ti_pmic_common_handler(function, address, bits, value,
> > +				handler_context, region_context,
> > +				ti_tps68470_pmic_get_vr_val,
> > +				ti_tps68470_regmap_update_bits,
> > +				(struct ti_pmic_table *) &vr_val_table,
> > +				ARRAY_SIZE(vr_val_table));
> > +}
> > +
> > +static acpi_status ti_pmic_power_handler(u32 function,
> > +					 acpi_physical_address address,
> > +					 u32 bits, u64 *value,
> > +					 void *handler_context,
> > +					 void *region_context)
> > +{
> > +	if (bits != 32)
> > +		return AE_BAD_PARAMETER;
> > +
> > +	/* set/clear for bit 0, bits 0 and 1 together */
> > +	if (function == ACPI_WRITE &&
> > +	    !(*value == 0 || *value == 1 || *value == 3)) {
> > +		return AE_BAD_PARAMETER;
> > +	}
> > +
> > +	return ti_pmic_common_handler(function, address, bits, value,
> > +				handler_context, region_context,
> > +				ti_tps68470_pmic_get_power,
> > +				ti_tps68470_regmap_update_bits,
> > +				(struct ti_pmic_table *) &power_table,
> > +				ARRAY_SIZE(power_table));
> > +}
> > +
> > +static int ti_tps68470_pmic_opregion_probe(struct platform_device
> > +*pdev) {
> > +	struct tps68470 *pmic = dev_get_drvdata(pdev->dev.parent);
> > +	acpi_handle handle = ACPI_HANDLE(pdev->dev.parent);
> > +	struct device *dev = &pdev->dev;
> > +	struct ti_pmic_opregion *opregion;
> > +	acpi_status status;
> > +
> > +	if (!dev || !pmic->regmap) {
> > +		WARN(1, "dev or regmap is NULL\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (!handle) {
> > +		WARN(1, "acpi handle is NULL\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	opregion = devm_kzalloc(dev, sizeof(*opregion), GFP_KERNEL);
> > +	if (!opregion)
> > +		return -ENOMEM;
> > +
> > +	mutex_init(&opregion->lock);
> > +	opregion->regmap = pmic->regmap;
> > +
> > +	status = acpi_install_address_space_handler(handle,
> > +
> TI_PMIC_POWER_OPREGION_ID,
> > +						    ti_pmic_power_handler,
> > +						    NULL, opregion);
> > +	if (ACPI_FAILURE(status))
> 
> mutex_destroy() after mutex_init() --- please add a label for this.
> 
> > +		return -ENODEV;
> > +
> > +	status = acpi_install_address_space_handler(handle,
> > +
> TI_PMIC_VR_VAL_OPREGION_ID,
> > +						    ti_pmic_vr_val_handler,
> > +						    NULL, opregion);
> > +	if (ACPI_FAILURE(status))
> > +		goto out_remove_power_handler;
> > +
> > +	status = acpi_install_address_space_handler(handle,
> > +
> TI_PMIC_CLOCK_OPREGION_ID,
> > +						    ti_pmic_clk_handler,
> > +						    NULL, opregion);
> > +	if (ACPI_FAILURE(status))
> > +		goto out_remove_vr_val_handler;
> > +
> > +	status = acpi_install_address_space_handler(handle,
> > +
> TI_PMIC_CLKFREQ_OPREGION_ID,
> > +						    ti_pmic_clk_freq_handler,
> > +						    NULL, opregion);
> > +	if (ACPI_FAILURE(status))
> > +		goto out_remove_clk_handler;
> > +
> > +	return 0;
> > +
> > +out_remove_clk_handler:
> > +	acpi_remove_address_space_handler(handle,
> TI_PMIC_CLOCK_OPREGION_ID,
> > +					  ti_pmic_clk_handler);
> > +out_remove_vr_val_handler:
> > +	acpi_remove_address_space_handler(handle,
> TI_PMIC_VR_VAL_OPREGION_ID,
> > +					  ti_pmic_vr_val_handler);
> > +out_remove_power_handler:
> > +	acpi_remove_address_space_handler(handle,
> TI_PMIC_POWER_OPREGION_ID,
> > +					  ti_pmic_power_handler);
> > +	return -ENODEV;
> > +}
> > +
> > +static struct platform_driver ti_tps68470_pmic_opregion_driver = {
> > +	.probe = ti_tps68470_pmic_opregion_probe,
> > +	.driver = {
> > +		.name = "tps68470_pmic_opregion",
> > +	},
> > +};
> > +
> > +builtin_platform_driver(ti_tps68470_pmic_opregion_driver)
> 
> --
> Kind regards,
> 
> Sakari Ailus
> e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk