Re: [PATCH] regmap: add virtual PMIC IPC bus support

Re: [PATCH] regmap: add virtual PMIC IPC bus support

[Mark Brown]

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

On Tue, Apr 21, 2015 at 07:38:58AM +0800, qipeng.zha wrote:
> From: "qipeng.zha" <qipeng.zha@intel.com>
> 
> There defined Whiskey Cove PMIC controller on one Intel platform,
> and all registers on this PMIC are accessed by IPC commands.
> This patch is to utilize regmap framework to access PMIC registers
> for PMIC core and device drivers.

Why is this being added to the regmap core rather than being implemented
in using the reg_write() and reg_read() callbacks in the core driver for
the PMIC?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH] regmap: add virtual PMIC IPC bus support

[qipeng.zha]

From: "qipeng.zha" <qipeng.zha@intel.com>

There defined Whiskey Cove PMIC controller on one Intel platform,
and all registers on this PMIC are accessed by IPC commands.
This patch is to utilize regmap framework to access PMIC registers
for PMIC core and device drivers.

Signed-off-by: qipeng.zha <qipeng.zha@intel.com>
---
 drivers/base/regmap/Kconfig           |   5 +-
 drivers/base/regmap/Makefile          |   1 +
 drivers/base/regmap/regmap-pmic-ipc.c | 128 ++++++++++++++++++++++++++++++++++
 include/linux/mfd/intel_soc_pmic.h    |   2 +
 include/linux/regmap.h                |   5 ++
 5 files changed, 140 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/regmap/regmap-pmic-ipc.c

diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index db9d00c3..c6ab5a9 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -3,7 +3,7 @@
 # subsystems should select the appropriate symbols.
 
 config REGMAP
-	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ)
+	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ || REGMAP_PMIC_IPC)
 	select LZO_COMPRESS
 	select LZO_DECOMPRESS
 	select IRQ_DOMAIN if REGMAP_IRQ
@@ -29,3 +29,6 @@ config REGMAP_MMIO
 
 config REGMAP_IRQ
 	bool
+
+config REGMAP_PMIC_IPC
+	bool
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index 609e4c8..fb859c0 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_REGMAP_SPI) += regmap-spi.o
 obj-$(CONFIG_REGMAP_SPMI) += regmap-spmi.o
 obj-$(CONFIG_REGMAP_MMIO) += regmap-mmio.o
 obj-$(CONFIG_REGMAP_IRQ) += regmap-irq.o
+obj-$(CONFIG_REGMAP_IRQ) += regmap-pmic-ipc.o
diff --git a/drivers/base/regmap/regmap-pmic-ipc.c b/drivers/base/regmap/regmap-pmic-ipc.c
new file mode 100644
index 0000000..2895d22
--- /dev/null
+++ b/drivers/base/regmap/regmap-pmic-ipc.c
@@ -0,0 +1,128 @@
+/*
+ * Register map access API - Intel PMIC IPC support
+ *
+ * (C) Copyright 2015 Intel Corporation
+ *
+ * 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
+ * of the License.
+ *
+ */
+
+#include <linux/regmap.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include <asm/intel_pmc_ipc.h>
+#include "internal.h"
+
+#define REG_ADDR_MASK		0xFF00
+#define REG_ADDR_SHIFT		8
+#define REG_OFFSET_MASK		0xFF
+
+static int regmap_ipc_byte_reg_read(void *context, unsigned int reg,
+				      unsigned int *val)
+{
+	int ret;
+	int i2c_addr;
+	u8 ipc_in[2];
+	u8 ipc_out[4];
+	struct intel_soc_pmic *pmic = context;
+
+	if (reg & REG_ADDR_MASK)
+		i2c_addr = (reg & REG_ADDR_MASK) >> REG_ADDR_SHIFT;
+	else {
+		i2c_addr = pmic->default_i2c_addr;
+		if (!i2c_addr) {
+			dev_err(pmic->dev, "Wrong register addr to read\n");
+			return -EINVAL;
+		}
+	}
+	reg &= REG_OFFSET_MASK;
+
+	ipc_in[0] = reg;
+	ipc_in[1] = i2c_addr;
+	ret = intel_pmc_ipc_command(PMC_IPC_PMIC_ACCESS,
+			PMC_IPC_PMIC_ACCESS_READ,
+			ipc_in, sizeof(ipc_in), (u32 *)ipc_out, 1);
+	if (ret) {
+		dev_err(pmic->dev, "Err: ipc read pmic\n");
+		return ret;
+	}
+	*val = ipc_out[0];
+	return 0;
+}
+
+static int regmap_ipc_byte_reg_write(void *context, unsigned int reg,
+				       unsigned int val)
+{
+	int ret;
+	int i2c_addr;
+	u8 ipc_in[3];
+	struct intel_soc_pmic *pmic = context;
+
+	if (reg & REG_ADDR_MASK)
+		i2c_addr = (reg & REG_ADDR_MASK) >> REG_ADDR_SHIFT;
+	else {
+		i2c_addr = pmic->default_i2c_addr;
+		if (!i2c_addr) {
+			dev_err(pmic->dev, "Wrong register addr to write\n");
+			return -EINVAL;
+		}
+	}
+	reg &= REG_OFFSET_MASK;
+
+	ipc_in[0] = reg;
+	ipc_in[1] = i2c_addr;
+	ipc_in[2] = val;
+	ret = intel_pmc_ipc_command(PMC_IPC_PMIC_ACCESS,
+			PMC_IPC_PMIC_ACCESS_WRITE,
+			ipc_in, sizeof(ipc_in), NULL, 0);
+	if (ret) {
+		dev_err(pmic->dev, "Err: ipc write pmic\n");
+		return ret;
+	}
+	return 0;
+}
+
+static struct regmap_bus ipc_regmap_bus = {
+	.reg_write = regmap_ipc_byte_reg_write,
+	.reg_read = regmap_ipc_byte_reg_read,
+};
+
+/**
+ * regmap_init_pmic_ipc(): Initialise register map
+ *
+ * @pmic: Device that will be interacted with
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer to
+ * a struct regmap.
+ */
+struct regmap *regmap_init_pmic_ipc(struct intel_soc_pmic *pmic,
+			       const struct regmap_config *config)
+{
+	return regmap_init(pmic->dev, &ipc_regmap_bus, pmic, config);
+}
+EXPORT_SYMBOL_GPL(regmap_init_pmic_ipc);
+
+/**
+ * devm_regmap_init_pmic_ipc(): Initialise managed register map
+ *
+ * @pmic: Device that will be interacted with
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap.  The regmap will be automatically freed by the
+ * device management code.
+ */
+struct regmap *devm_regmap_init_pmic_ipc(struct intel_soc_pmic *pmic,
+				    const struct regmap_config *config)
+{
+	return devm_regmap_init(pmic->dev, &ipc_regmap_bus, pmic, config);
+}
+EXPORT_SYMBOL_GPL(devm_regmap_init_pmic_ipc);
+
+MODULE_AUTHOR("qipeng.zha@intel.com");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/intel_soc_pmic.h b/include/linux/mfd/intel_soc_pmic.h
index abcbfcf..74ee3d2 100644
--- a/include/linux/mfd/intel_soc_pmic.h
+++ b/include/linux/mfd/intel_soc_pmic.h
@@ -25,6 +25,8 @@ struct intel_soc_pmic {
 	int irq;
 	struct regmap *regmap;
 	struct regmap_irq_chip_data *irq_chip_data;
+	struct device *dev;
+	int default_i2c_addr;
 };
 
 #endif	/* __INTEL_SOC_PMIC_H__ */
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 116655d..11bbf1d 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -28,6 +28,7 @@ struct regmap;
 struct regmap_range_cfg;
 struct regmap_field;
 struct snd_ac97;
+struct intel_soc_pmic;
 
 /* An enum of all the supported cache types */
 enum regcache_type {
@@ -343,6 +344,8 @@ struct regmap *regmap_init_mmio_clk(struct device *dev, const char *clk_id,
 				    const struct regmap_config *config);
 struct regmap *regmap_init_ac97(struct snd_ac97 *ac97,
 				const struct regmap_config *config);
+struct regmap *regmap_init_pmic_ipc(struct intel_soc_pmic *pmic,
+			       const struct regmap_config *config);
 
 struct regmap *devm_regmap_init(struct device *dev,
 				const struct regmap_bus *bus,
@@ -361,6 +364,8 @@ struct regmap *devm_regmap_init_mmio_clk(struct device *dev, const char *clk_id,
 					 const struct regmap_config *config);
 struct regmap *devm_regmap_init_ac97(struct snd_ac97 *ac97,
 				     const struct regmap_config *config);
+struct regmap *devm_regmap_init_pmic_ipc(struct intel_soc_pmic *pmic,
+				    const struct regmap_config *config);
 
 bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
 
-- 
1.8.3.2

RE: [PATCH] regmap: add virtual PMIC IPC bus support

[Zha, Qipeng]

Hi Broonie

Can you help comment on this patch ?
We do really want to leverage current regmap framework to acess intel pmic devices.
So please evaluate this driver, thanks.




Best wishes
Qipeng

-----Original Message-----
From: Zha, Qipeng 
Sent: Tuesday, April 21, 2015 7:39 AM
To: linux-kernel@vger.kernel.org
Cc: broonie@kernel.org; Yang, Fei; Zhong, Huiquan; Chen, Jason CJ; Zheng, Qi; Zha, Qipeng
Subject: [PATCH] regmap: add virtual PMIC IPC bus support

From: "qipeng.zha" <qipeng.zha@intel.com>

There defined Whiskey Cove PMIC controller on one Intel platform, and all registers on this PMIC are accessed by IPC commands.
This patch is to utilize regmap framework to access PMIC registers for PMIC core and device drivers.

Signed-off-by: qipeng.zha <qipeng.zha@intel.com>
---
 drivers/base/regmap/Kconfig           |   5 +-
 drivers/base/regmap/Makefile          |   1 +
 drivers/base/regmap/regmap-pmic-ipc.c | 128 ++++++++++++++++++++++++++++++++++
 include/linux/mfd/intel_soc_pmic.h    |   2 +
 include/linux/regmap.h                |   5 ++
 5 files changed, 140 insertions(+), 1 deletion(-)  create mode 100644 drivers/base/regmap/regmap-pmic-ipc.c

diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig index db9d00c3..c6ab5a9 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -3,7 +3,7 @@
 # subsystems should select the appropriate symbols.
 
 config REGMAP
-	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ)
+	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_AC97 
+|| REGMAP_MMIO || REGMAP_IRQ || REGMAP_PMIC_IPC)
 	select LZO_COMPRESS
 	select LZO_DECOMPRESS
 	select IRQ_DOMAIN if REGMAP_IRQ
@@ -29,3 +29,6 @@ config REGMAP_MMIO
 
 config REGMAP_IRQ
 	bool
+
+config REGMAP_PMIC_IPC
+	bool
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile index 609e4c8..fb859c0 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_REGMAP_SPI) += regmap-spi.o
 obj-$(CONFIG_REGMAP_SPMI) += regmap-spmi.o
 obj-$(CONFIG_REGMAP_MMIO) += regmap-mmio.o
 obj-$(CONFIG_REGMAP_IRQ) += regmap-irq.o
+obj-$(CONFIG_REGMAP_IRQ) += regmap-pmic-ipc.o
diff --git a/drivers/base/regmap/regmap-pmic-ipc.c b/drivers/base/regmap/regmap-pmic-ipc.c
new file mode 100644
index 0000000..2895d22
--- /dev/null
+++ b/drivers/base/regmap/regmap-pmic-ipc.c
@@ -0,0 +1,128 @@
+/*
+ * Register map access API - Intel PMIC IPC support
+ *
+ * (C) Copyright 2015 Intel Corporation
+ *
+ * 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
+ * of the License.
+ *
+ */
+
+#include <linux/regmap.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include <asm/intel_pmc_ipc.h>
+#include "internal.h"
+
+#define REG_ADDR_MASK		0xFF00
+#define REG_ADDR_SHIFT		8
+#define REG_OFFSET_MASK		0xFF
+
+static int regmap_ipc_byte_reg_read(void *context, unsigned int reg,
+				      unsigned int *val)
+{
+	int ret;
+	int i2c_addr;
+	u8 ipc_in[2];
+	u8 ipc_out[4];
+	struct intel_soc_pmic *pmic = context;
+
+	if (reg & REG_ADDR_MASK)
+		i2c_addr = (reg & REG_ADDR_MASK) >> REG_ADDR_SHIFT;
+	else {
+		i2c_addr = pmic->default_i2c_addr;
+		if (!i2c_addr) {
+			dev_err(pmic->dev, "Wrong register addr to read\n");
+			return -EINVAL;
+		}
+	}
+	reg &= REG_OFFSET_MASK;
+
+	ipc_in[0] = reg;
+	ipc_in[1] = i2c_addr;
+	ret = intel_pmc_ipc_command(PMC_IPC_PMIC_ACCESS,
+			PMC_IPC_PMIC_ACCESS_READ,
+			ipc_in, sizeof(ipc_in), (u32 *)ipc_out, 1);
+	if (ret) {
+		dev_err(pmic->dev, "Err: ipc read pmic\n");
+		return ret;
+	}
+	*val = ipc_out[0];
+	return 0;
+}
+
+static int regmap_ipc_byte_reg_write(void *context, unsigned int reg,
+				       unsigned int val)
+{
+	int ret;
+	int i2c_addr;
+	u8 ipc_in[3];
+	struct intel_soc_pmic *pmic = context;
+
+	if (reg & REG_ADDR_MASK)
+		i2c_addr = (reg & REG_ADDR_MASK) >> REG_ADDR_SHIFT;
+	else {
+		i2c_addr = pmic->default_i2c_addr;
+		if (!i2c_addr) {
+			dev_err(pmic->dev, "Wrong register addr to write\n");
+			return -EINVAL;
+		}
+	}
+	reg &= REG_OFFSET_MASK;
+
+	ipc_in[0] = reg;
+	ipc_in[1] = i2c_addr;
+	ipc_in[2] = val;
+	ret = intel_pmc_ipc_command(PMC_IPC_PMIC_ACCESS,
+			PMC_IPC_PMIC_ACCESS_WRITE,
+			ipc_in, sizeof(ipc_in), NULL, 0);
+	if (ret) {
+		dev_err(pmic->dev, "Err: ipc write pmic\n");
+		return ret;
+	}
+	return 0;
+}
+
+static struct regmap_bus ipc_regmap_bus = {
+	.reg_write = regmap_ipc_byte_reg_write,
+	.reg_read = regmap_ipc_byte_reg_read,
+};
+
+/**
+ * regmap_init_pmic_ipc(): Initialise register map
+ *
+ * @pmic: Device that will be interacted with
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer to
+ * a struct regmap.
+ */
+struct regmap *regmap_init_pmic_ipc(struct intel_soc_pmic *pmic,
+			       const struct regmap_config *config) {
+	return regmap_init(pmic->dev, &ipc_regmap_bus, pmic, config); } 
+EXPORT_SYMBOL_GPL(regmap_init_pmic_ipc);
+
+/**
+ * devm_regmap_init_pmic_ipc(): Initialise managed register map
+ *
+ * @pmic: Device that will be interacted with
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap.  The regmap will be automatically freed by the
+ * device management code.
+ */
+struct regmap *devm_regmap_init_pmic_ipc(struct intel_soc_pmic *pmic,
+				    const struct regmap_config *config) {
+	return devm_regmap_init(pmic->dev, &ipc_regmap_bus, pmic, config); } 
+EXPORT_SYMBOL_GPL(devm_regmap_init_pmic_ipc);
+
+MODULE_AUTHOR("qipeng.zha@intel.com");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/intel_soc_pmic.h b/include/linux/mfd/intel_soc_pmic.h
index abcbfcf..74ee3d2 100644
--- a/include/linux/mfd/intel_soc_pmic.h
+++ b/include/linux/mfd/intel_soc_pmic.h
@@ -25,6 +25,8 @@ struct intel_soc_pmic {
 	int irq;
 	struct regmap *regmap;
 	struct regmap_irq_chip_data *irq_chip_data;
+	struct device *dev;
+	int default_i2c_addr;
 };
 
 #endif	/* __INTEL_SOC_PMIC_H__ */
diff --git a/include/linux/regmap.h b/include/linux/regmap.h index 116655d..11bbf1d 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -28,6 +28,7 @@ struct regmap;
 struct regmap_range_cfg;
 struct regmap_field;
 struct snd_ac97;
+struct intel_soc_pmic;
 
 /* An enum of all the supported cache types */  enum regcache_type { @@ -343,6 +344,8 @@ struct regmap *regmap_init_mmio_clk(struct device *dev, const char *clk_id,
 				    const struct regmap_config *config);  struct regmap *regmap_init_ac97(struct snd_ac97 *ac97,
 				const struct regmap_config *config);
+struct regmap *regmap_init_pmic_ipc(struct intel_soc_pmic *pmic,
+			       const struct regmap_config *config);
 
 struct regmap *devm_regmap_init(struct device *dev,
 				const struct regmap_bus *bus,
@@ -361,6 +364,8 @@ struct regmap *devm_regmap_init_mmio_clk(struct device *dev, const char *clk_id,
 					 const struct regmap_config *config);  struct regmap *devm_regmap_init_ac97(struct snd_ac97 *ac97,
 				     const struct regmap_config *config);
+struct regmap *devm_regmap_init_pmic_ipc(struct intel_soc_pmic *pmic,
+				    const struct regmap_config *config);
 
 bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
 
--
1.8.3.2

Re: [PATCH] regmap: add virtual PMIC IPC bus support

[Mark Brown]

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

On Mon, May 18, 2015 at 08:10:04AM +0000, Zha, Qipeng wrote:
> Hi Broonie
> 
> Can you help comment on this patch ?
> We do really want to leverage current regmap framework to acess intel pmic devices.
> So please evaluate this driver, thanks.

Please don't send contentless pings, and please refer to my original
response to one of the copies you sent:

https://lkml.org/lkml/2015/4/20/425

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

RE: [PATCH] regmap: add virtual PMIC IPC bus support

[Zha, Qipeng]

Hi

>Why is this being added to the regmap core rather than being implemented
>in using the reg_write() and reg_read() callbacks in the core driver for
>the PMIC?

Thanks. I don't receive this message before in eamil.
Current regmap only support register access  using several bus:  i2c, spi, ac97 ...
But register access for intel pmic are done by hardware ipc, so we add such support in regmap core,
To make pmic subdevice drivers can use generic regmap r/w API to access registers.

 
Best wishes
Qipeng

-----Original Message-----
From: Mark Brown [mailto:broonie@kernel.org] 
Sent: Monday, May 18, 2015 7:21 PM
To: Zha, Qipeng
Cc: linux-kernel@vger.kernel.org; Yang, Fei; Zhong, Huiquan; Chen, Jason CJ; Zheng, Qi
Subject: Re: [PATCH] regmap: add virtual PMIC IPC bus support

On Mon, May 18, 2015 at 08:10:04AM +0000, Zha, Qipeng wrote:
> Hi Broonie
> 
> Can you help comment on this patch ?
> We do really want to leverage current regmap framework to acess intel pmic devices.
> So please evaluate this driver, thanks.

Please don't send contentless pings, and please refer to my original response to one of the copies you sent:

https://lkml.org/lkml/2015/4/20/425

Re: [PATCH] regmap: add virtual PMIC IPC bus support

[Mark Brown]

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

On Tue, May 19, 2015 at 04:57:00AM +0000, Zha, Qipeng wrote:

> >Why is this being added to the regmap core rather than being implemented
> >in using the reg_write() and reg_read() callbacks in the core driver for
> >the PMIC?

> Thanks. I don't receive this message before in eamil.
> Current regmap only support register access  using several bus:  i2c, spi, ac97 ...
> But register access for intel pmic are done by hardware ipc, so we add such support in regmap core,
> To make pmic subdevice drivers can use generic regmap r/w API to access registers.

That doesn't really address my concern - we already have the reg_read()
and reg_write() interface so devices can use regmap without a bus in the
core.  The question is if this is something that's going to be used by
many devices so there's code to share or if it's something that's only
going to be used by this PMIC in which case keeping it in the driver
should be easier.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

RE: [PATCH] regmap: add virtual PMIC IPC bus support

[Zha, Qipeng]

Hi 

Not sure what's your mean using regmap api without bus suport for this case
Confirm many device driver will use this api.

The detail is
Beside pmic core driver itself, there are many pmic function device dirvers, 
such as gpio, theremal, charger, bcu ..., will use this regmap api to access registers.

Using regmap, we can keep transparent for register access in these sub pmic device drivers ,
Since we do also have different pmic hw which is controlled by i2c, not ipc here
If don't add such regmap core support here, we need wrap a similar layer as regmap core for these sub device drivers.

Best wishes
Qipeng

-----Original Message-----
From: Mark Brown [mailto:broonie@kernel.org] 
Sent: Tuesday, May 19, 2015 8:09 PM
To: Zha, Qipeng
Cc: linux-kernel@vger.kernel.org; Yang, Fei; Zhong, Huiquan; Chen, Jason CJ; Zheng, Qi
Subject: Re: [PATCH] regmap: add virtual PMIC IPC bus support

On Tue, May 19, 2015 at 04:57:00AM +0000, Zha, Qipeng wrote:

> >Why is this being added to the regmap core rather than being 
> >implemented in using the reg_write() and reg_read() callbacks in the 
> >core driver for the PMIC?

> Thanks. I don't receive this message before in eamil.
> Current regmap only support register access  using several bus:  i2c, spi, ac97 ...
> But register access for intel pmic are done by hardware ipc, so we add 
> such support in regmap core, To make pmic subdevice drivers can use generic regmap r/w API to access registers.

That doesn't really address my concern - we already have the reg_read() and reg_write() interface so devices can use regmap without a bus in the core.  The question is if this is something that's going to be used by many devices so there's code to share or if it's something that's only going to be used by this PMIC in which case keeping it in the driver should be easier.

Re: [PATCH] regmap: add virtual PMIC IPC bus support

[Mark Brown]

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

On Wed, May 20, 2015 at 01:02:49AM +0000, Zha, Qipeng wrote:

Please don't top post.

> Not sure what's your mean using regmap api without bus suport for this case
> Confirm many device driver will use this api.

Please take a look at the code, particularly the reg_read() and
reg_write() interface which I've mentioned in each of my previous
replies and their users.  If you have queries about these please ask.

> The detail is
> Beside pmic core driver itself, there are many pmic function device dirvers, 
> such as gpio, theremal, charger, bcu ..., will use this regmap api to access registers.

This sounds like this is a single device, not a generic bus.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

RE: [PATCH] regmap: add virtual PMIC IPC bus support

[Yang, Fei]

>> The detail is
>> Beside pmic core driver itself, there are many pmic function device 
>> dirvers, such as gpio, theremal, charger, bcu ..., will use this regmap api to access registers.

> This sounds like this is a single device, not a generic bus.
Well, yes this is more like a bridge to the PMIC instead of a generic bus. In the latest Intel SoC design,
the IA core doesn't have direct access to the I2C bus which PMIC is connected to. Instead, there is a PMC
controlling the I2C host and talking to PMIC directly. In the meantime PMC runs a firmware providing IPC
interface to the IA core. So for IA to access PMIC registers, it has to go through these IPC commands.
In order to fit the regmap architecture, the IPC is treated as a bus here so that the PMIC drivers can be
Implemented in the same way as others.

RE: [PATCH] regmap: add virtual PMIC IPC bus support

[Yang, Fei]

> > >Why is this being added to the regmap core rather than being 
> > >implemented in using the reg_write() and reg_read() callbacks in the 
> > >core driver for the PMIC?

> > Thanks. I don't receive this message before in eamil.
> > Current regmap only support register access  using several bus:  i2c, spi, ac97 ...
> > But register access for intel pmic are done by hardware ipc, so we add 
> > such support in regmap core, To make pmic subdevice drivers can use generic regmap r/w API to access registers.

> That doesn't really address my concern - we already have the reg_read() and reg_write() interface so devices can use
> regmap without a bus in the core.  The question is if this is something that's going to be used by many devices so there's
> code to share or if it's something that's only going to be used by this PMIC in which case keeping it in the driver should be easier.

The IPC code is actually independent of PMIC, it will evolve together with the PMC firmware instead of PMIC. Maintaining the IPC
code inside a PMIC driver would be weird because PMIC doesn't really know/care anything about IPC.
In other words, the same PMIC driver could run on different platforms with either i2c or IPC access without knowing which mechanism is
running underneath.