[PATCH v1] mfd: Add support for Merrifield Basin Cove PMIC

[PATCH v1] mfd: Add support for Merrifield Basin Cove PMIC

[Andy Shevchenko]

Add an mfd driver for Intel Merrifield Basin Cove PMIC.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mfd/Kconfig                      |  11 ++
 drivers/mfd/Makefile                     |   1 +
 drivers/mfd/intel_soc_pmic_mrfld.c       | 157 +++++++++++++++++++++++
 include/linux/mfd/intel_soc_pmic_mrfld.h |  81 ++++++++++++
 4 files changed, 250 insertions(+)
 create mode 100644 drivers/mfd/intel_soc_pmic_mrfld.c
 create mode 100644 include/linux/mfd/intel_soc_pmic_mrfld.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 0ce2d8dfc5f1..2adf9d393029 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -572,6 +572,17 @@ config INTEL_SOC_PMIC_CHTDC_TI
 	  Select this option for supporting Dollar Cove (TI version) PMIC
 	  device that is found on some Intel Cherry Trail systems.
 
+config INTEL_SOC_PMIC_MRFLD
+	tristate "Support for Intel Merrifield Basin Cove PMIC"
+	depends on GPIOLIB
+	depends on ACPI
+	depends on INTEL_SCU_IPC
+	select MFD_CORE
+	select REGMAP_IRQ
+	help
+	  Select this option for supporting Basin Cove PMIC device
+	  that is found on Intel Merrifield systems.
+
 config MFD_INTEL_LPSS
 	tristate
 	select COMMON_CLK
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index b4569ed7f3f3..1b746bd01ac5 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -234,6 +234,7 @@ obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
 obj-$(CONFIG_INTEL_SOC_PMIC_BXTWC)	+= intel_soc_pmic_bxtwc.o
 obj-$(CONFIG_INTEL_SOC_PMIC_CHTWC)	+= intel_soc_pmic_chtwc.o
 obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI)	+= intel_soc_pmic_chtdc_ti.o
+obj-$(CONFIG_INTEL_SOC_PMIC_MRFLD)	+= intel_soc_pmic_mrfld.o
 obj-$(CONFIG_MFD_MT6397)	+= mt6397-core.o
 
 obj-$(CONFIG_MFD_ALTERA_A10SR)	+= altera-a10sr.o
diff --git a/drivers/mfd/intel_soc_pmic_mrfld.c b/drivers/mfd/intel_soc_pmic_mrfld.c
new file mode 100644
index 000000000000..bbee89c0c25b
--- /dev/null
+++ b/drivers/mfd/intel_soc_pmic_mrfld.c
@@ -0,0 +1,157 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device access for Basin Cove PMIC
+ *
+ * Copyright (c) 2018, Intel Corporation.
+ * Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
+ */
+
+#include <linux/acpi.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include <linux/mfd/intel_soc_pmic_mrfld.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include <asm/intel_scu_ipc.h>
+
+/*
+ * Level 2 IRQs
+ *
+ * Firmware on the systems with Basin Cove PMIC services Level 1 IRQs
+ * without an assistance. Thus, each of the Level 1 IRQ is represented
+ * as a separate RTE in IOAPIC.
+ */
+static struct resource irq_level2_resources[] = {
+	DEFINE_RES_IRQ(0), /* power button */
+	DEFINE_RES_IRQ(0), /* TMU */
+	DEFINE_RES_IRQ(0), /* thermal */
+	DEFINE_RES_IRQ(0), /* BCU */
+	DEFINE_RES_IRQ(0), /* ADC */
+	DEFINE_RES_IRQ(0), /* charger */
+	DEFINE_RES_IRQ(0), /* GPIO */
+};
+
+static const struct mfd_cell bcove_dev[] = {
+	{
+		.name = "mrfld_bcove_pwrbtn",
+		.num_resources = 1,
+		.resources = &irq_level2_resources[0],
+	}, {
+		.name = "mrfld_bcove_tmu",
+		.num_resources = 1,
+		.resources = &irq_level2_resources[1],
+	}, {
+		.name = "mrfld_bcove_thermal",
+		.num_resources = 1,
+		.resources = &irq_level2_resources[2],
+	}, {
+		.name = "mrfld_bcove_bcu",
+		.num_resources = 1,
+		.resources = &irq_level2_resources[3],
+	}, {
+		.name = "mrfld_bcove_adc",
+		.num_resources = 1,
+		.resources = &irq_level2_resources[4],
+	}, {
+		.name = "mrfld_bcove_charger",
+		.num_resources = 1,
+		.resources = &irq_level2_resources[5],
+	}, {
+		.name = "mrfld_bcove_extcon",
+		.num_resources = 1,
+		.resources = &irq_level2_resources[5],
+	}, {
+		.name = "mrfld_bcove_gpio",
+		.num_resources = 1,
+		.resources = &irq_level2_resources[6],
+	},
+	{	.name = "mrfld_bcove_region", },
+};
+
+static int regmap_ipc_byte_reg_read(void *context, unsigned int reg,
+				    unsigned int *val)
+{
+	u8 ipc_out;
+	int ret;
+
+	ret = intel_scu_ipc_ioread8(reg, &ipc_out);
+	if (ret)
+		return ret;
+
+	*val = ipc_out;
+	return 0;
+}
+
+static int regmap_ipc_byte_reg_write(void *context, unsigned int reg,
+				     unsigned int val)
+{
+	u8 ipc_in = val;
+	int ret;
+
+	ret = intel_scu_ipc_iowrite8(reg, ipc_in);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct regmap_config bcove_regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 8,
+	.max_register = 0xff,
+	.reg_write = regmap_ipc_byte_reg_write,
+	.reg_read = regmap_ipc_byte_reg_read,
+};
+
+static int bcove_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct intel_soc_pmic *pmic;
+	unsigned int i;
+	int ret;
+
+	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
+	if (!pmic)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, pmic);
+	pmic->dev = &pdev->dev;
+
+	pmic->regmap = devm_regmap_init(dev, NULL, pmic, &bcove_regmap_config);
+	if (IS_ERR(pmic->regmap))
+		return PTR_ERR(pmic->regmap);
+
+	for (i = 0; i < ARRAY_SIZE(irq_level2_resources); i++) {
+		ret = platform_get_irq(pdev, i);
+		if (ret < 0)
+			return ret;
+
+		irq_level2_resources[i].start = ret;
+		irq_level2_resources[i].end = ret;
+	}
+
+	return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
+				    bcove_dev, ARRAY_SIZE(bcove_dev),
+				    NULL, 0, NULL);
+}
+
+static const struct acpi_device_id bcove_acpi_ids[] = {
+	{ "INTC100E" },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, bcove_acpi_ids);
+
+static struct platform_driver bcove_driver = {
+	.driver = {
+		.name = "intel_soc_pmic_mrfld",
+		.acpi_match_table = bcove_acpi_ids,
+	},
+	.probe = bcove_probe,
+};
+module_platform_driver(bcove_driver);
+
+MODULE_DESCRIPTION("IPC driver for Intel SoC Basin Cove PMIC");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/intel_soc_pmic_mrfld.h b/include/linux/mfd/intel_soc_pmic_mrfld.h
new file mode 100644
index 000000000000..00feed7c2787
--- /dev/null
+++ b/include/linux/mfd/intel_soc_pmic_mrfld.h
@@ -0,0 +1,81 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for Intel Merrifield Basin Cove PMIC
+ *
+ * Copyright (C) 2018 Intel Corporation. All rights reserved.
+ */
+
+#ifndef __INTEL_SOC_PMIC_MRFLD_H__
+#define __INTEL_SOC_PMIC_MRFLD_H__
+
+#include <linux/bits.h>
+
+#define BCOVE_ID		0x00
+
+#define BCOVE_ID_MINREV0	GENMASK(2, 0)
+#define BCOVE_ID_MAJREV0	GENMASK(5, 3)
+#define BCOVE_ID_VENDID0	GENMASK(7, 6)
+
+#define BCOVE_MINOR(x)		(unsigned int)(((x) & BCOVE_ID_MINREV0) >> 0)
+#define BCOVE_MAJOR(x)		(unsigned int)(((x) & BCOVE_ID_MAJREV0) >> 3)
+#define BCOVE_VENDOR(x)		(unsigned int)(((x) & BCOVE_ID_VENDID0) >> 6)
+
+#define BCOVE_IRQLVL1		0x01
+
+#define BCOVE_PBIRQ		0x02
+#define BCOVE_TMUIRQ		0x03
+#define BCOVE_THRMIRQ		0x04
+#define BCOVE_BCUIRQ		0x05
+#define BCOVE_ADCIRQ		0x06
+#define BCOVE_CHGRIRQ0		0x07
+#define BCOVE_CHGRIRQ1		0x08
+#define BCOVE_GPIOIRQ		0x09
+#define BCOVE_CRITIRQ		0x0B
+
+#define BCOVE_MIRQLVL1		0x0C
+
+#define BCOVE_MPBIRQ		0x0D
+#define BCOVE_MTMUIRQ		0x0E
+#define BCOVE_MTHRMIRQ		0x0F
+#define BCOVE_MBCUIRQ		0x10
+#define BCOVE_MADCIRQ		0x11
+#define BCOVE_MCHGRIRQ0		0x12
+#define BCOVE_MCHGRIRQ1		0x13
+#define BCOVE_MGPIOIRQ		0x14
+#define BCOVE_MCRITIRQ		0x16
+
+#define BCOVE_SCHGRIRQ0		0x4E
+#define BCOVE_SCHGRIRQ1		0x4F
+
+/* Level 1 IRQs */
+#define BCOVE_LVL1_PWRBTN	BIT(0)	/* power button */
+#define BCOVE_LVL1_TMU		BIT(1)	/* time management unit */
+#define BCOVE_LVL1_THRM		BIT(2)	/* thermal */
+#define BCOVE_LVL1_BCU		BIT(3)	/* burst control unit */
+#define BCOVE_LVL1_ADC		BIT(4)	/* ADC */
+#define BCOVE_LVL1_CHGR		BIT(5)	/* charger */
+#define BCOVE_LVL1_GPIO		BIT(6)	/* GPIO */
+#define BCOVE_LVL1_CRIT		BIT(7)	/* critical event */
+
+/* Level 2 IRQs: power button */
+#define BCOVE_PBIRQ_PBTN	BIT(0)
+#define BCOVE_PBIRQ_UBTN	BIT(1)
+
+/* Level 2 IRQs: ADC */
+#define BCOVE_ADCIRQ_BATTEMP	BIT(2)
+#define BCOVE_ADCIRQ_SYSTEMP	BIT(3)
+#define BCOVE_ADCIRQ_BATTID	BIT(4)
+#define BCOVE_ADCIRQ_VIBATT	BIT(5)
+#define BCOVE_ADCIRQ_CCTICK	BIT(7)
+
+/* Level 2 IRQs: charger */
+#define BCOVE_CHGRIRQ_BAT0ALRT	BIT(4)
+#define BCOVE_CHGRIRQ_BAT1ALRT	BIT(5)
+#define BCOVE_CHGRIRQ_BATCRIT	BIT(6)
+
+#define BCOVE_CHGRIRQ_VBUSDET	BIT(0)
+#define BCOVE_CHGRIRQ_DCDET	BIT(1)
+#define BCOVE_CHGRIRQ_BATTDET	BIT(2)
+#define BCOVE_CHGRIRQ_USBIDDET	BIT(3)
+
+#endif	/* __INTEL_SOC_PMIC_MRFLD_H__ */
-- 
2.20.1

Re: [PATCH v1] mfd: Add support for Merrifield Basin Cove PMIC

[Andy Shevchenko]

On Mon, Mar 18, 2019 at 12:53:16PM +0300, Andy Shevchenko wrote:
> Add an mfd driver for Intel Merrifield Basin Cove PMIC.

While I waiting for comments here, please, don't apply it either yet.

The individual driver names might be changed. So, I will update them in v2 when
all parties will be satisfied with the names.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/mfd/Kconfig                      |  11 ++
>  drivers/mfd/Makefile                     |   1 +
>  drivers/mfd/intel_soc_pmic_mrfld.c       | 157 +++++++++++++++++++++++
>  include/linux/mfd/intel_soc_pmic_mrfld.h |  81 ++++++++++++
>  4 files changed, 250 insertions(+)
>  create mode 100644 drivers/mfd/intel_soc_pmic_mrfld.c
>  create mode 100644 include/linux/mfd/intel_soc_pmic_mrfld.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 0ce2d8dfc5f1..2adf9d393029 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -572,6 +572,17 @@ config INTEL_SOC_PMIC_CHTDC_TI
>  	  Select this option for supporting Dollar Cove (TI version) PMIC
>  	  device that is found on some Intel Cherry Trail systems.
>  
> +config INTEL_SOC_PMIC_MRFLD
> +	tristate "Support for Intel Merrifield Basin Cove PMIC"
> +	depends on GPIOLIB
> +	depends on ACPI
> +	depends on INTEL_SCU_IPC
> +	select MFD_CORE
> +	select REGMAP_IRQ
> +	help
> +	  Select this option for supporting Basin Cove PMIC device
> +	  that is found on Intel Merrifield systems.
> +
>  config MFD_INTEL_LPSS
>  	tristate
>  	select COMMON_CLK
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index b4569ed7f3f3..1b746bd01ac5 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -234,6 +234,7 @@ obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
>  obj-$(CONFIG_INTEL_SOC_PMIC_BXTWC)	+= intel_soc_pmic_bxtwc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC_CHTWC)	+= intel_soc_pmic_chtwc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI)	+= intel_soc_pmic_chtdc_ti.o
> +obj-$(CONFIG_INTEL_SOC_PMIC_MRFLD)	+= intel_soc_pmic_mrfld.o
>  obj-$(CONFIG_MFD_MT6397)	+= mt6397-core.o
>  
>  obj-$(CONFIG_MFD_ALTERA_A10SR)	+= altera-a10sr.o
> diff --git a/drivers/mfd/intel_soc_pmic_mrfld.c b/drivers/mfd/intel_soc_pmic_mrfld.c
> new file mode 100644
> index 000000000000..bbee89c0c25b
> --- /dev/null
> +++ b/drivers/mfd/intel_soc_pmic_mrfld.c
> @@ -0,0 +1,157 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device access for Basin Cove PMIC
> + *
> + * Copyright (c) 2018, Intel Corporation.
> + * Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/intel_soc_pmic.h>
> +#include <linux/mfd/intel_soc_pmic_mrfld.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <asm/intel_scu_ipc.h>
> +
> +/*
> + * Level 2 IRQs
> + *
> + * Firmware on the systems with Basin Cove PMIC services Level 1 IRQs
> + * without an assistance. Thus, each of the Level 1 IRQ is represented
> + * as a separate RTE in IOAPIC.
> + */
> +static struct resource irq_level2_resources[] = {
> +	DEFINE_RES_IRQ(0), /* power button */
> +	DEFINE_RES_IRQ(0), /* TMU */
> +	DEFINE_RES_IRQ(0), /* thermal */
> +	DEFINE_RES_IRQ(0), /* BCU */
> +	DEFINE_RES_IRQ(0), /* ADC */
> +	DEFINE_RES_IRQ(0), /* charger */
> +	DEFINE_RES_IRQ(0), /* GPIO */
> +};
> +
> +static const struct mfd_cell bcove_dev[] = {
> +	{
> +		.name = "mrfld_bcove_pwrbtn",
> +		.num_resources = 1,
> +		.resources = &irq_level2_resources[0],
> +	}, {
> +		.name = "mrfld_bcove_tmu",
> +		.num_resources = 1,
> +		.resources = &irq_level2_resources[1],
> +	}, {
> +		.name = "mrfld_bcove_thermal",
> +		.num_resources = 1,
> +		.resources = &irq_level2_resources[2],
> +	}, {
> +		.name = "mrfld_bcove_bcu",
> +		.num_resources = 1,
> +		.resources = &irq_level2_resources[3],
> +	}, {
> +		.name = "mrfld_bcove_adc",
> +		.num_resources = 1,
> +		.resources = &irq_level2_resources[4],
> +	}, {
> +		.name = "mrfld_bcove_charger",
> +		.num_resources = 1,
> +		.resources = &irq_level2_resources[5],
> +	}, {
> +		.name = "mrfld_bcove_extcon",
> +		.num_resources = 1,
> +		.resources = &irq_level2_resources[5],
> +	}, {
> +		.name = "mrfld_bcove_gpio",
> +		.num_resources = 1,
> +		.resources = &irq_level2_resources[6],
> +	},
> +	{	.name = "mrfld_bcove_region", },
> +};
> +
> +static int regmap_ipc_byte_reg_read(void *context, unsigned int reg,
> +				    unsigned int *val)
> +{
> +	u8 ipc_out;
> +	int ret;
> +
> +	ret = intel_scu_ipc_ioread8(reg, &ipc_out);
> +	if (ret)
> +		return ret;
> +
> +	*val = ipc_out;
> +	return 0;
> +}
> +
> +static int regmap_ipc_byte_reg_write(void *context, unsigned int reg,
> +				     unsigned int val)
> +{
> +	u8 ipc_in = val;
> +	int ret;
> +
> +	ret = intel_scu_ipc_iowrite8(reg, ipc_in);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static const struct regmap_config bcove_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 8,
> +	.max_register = 0xff,
> +	.reg_write = regmap_ipc_byte_reg_write,
> +	.reg_read = regmap_ipc_byte_reg_read,
> +};
> +
> +static int bcove_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct intel_soc_pmic *pmic;
> +	unsigned int i;
> +	int ret;
> +
> +	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
> +	if (!pmic)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, pmic);
> +	pmic->dev = &pdev->dev;
> +
> +	pmic->regmap = devm_regmap_init(dev, NULL, pmic, &bcove_regmap_config);
> +	if (IS_ERR(pmic->regmap))
> +		return PTR_ERR(pmic->regmap);
> +
> +	for (i = 0; i < ARRAY_SIZE(irq_level2_resources); i++) {
> +		ret = platform_get_irq(pdev, i);
> +		if (ret < 0)
> +			return ret;
> +
> +		irq_level2_resources[i].start = ret;
> +		irq_level2_resources[i].end = ret;
> +	}
> +
> +	return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
> +				    bcove_dev, ARRAY_SIZE(bcove_dev),
> +				    NULL, 0, NULL);
> +}
> +
> +static const struct acpi_device_id bcove_acpi_ids[] = {
> +	{ "INTC100E" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, bcove_acpi_ids);
> +
> +static struct platform_driver bcove_driver = {
> +	.driver = {
> +		.name = "intel_soc_pmic_mrfld",
> +		.acpi_match_table = bcove_acpi_ids,
> +	},
> +	.probe = bcove_probe,
> +};
> +module_platform_driver(bcove_driver);
> +
> +MODULE_DESCRIPTION("IPC driver for Intel SoC Basin Cove PMIC");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/intel_soc_pmic_mrfld.h b/include/linux/mfd/intel_soc_pmic_mrfld.h
> new file mode 100644
> index 000000000000..00feed7c2787
> --- /dev/null
> +++ b/include/linux/mfd/intel_soc_pmic_mrfld.h
> @@ -0,0 +1,81 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Header file for Intel Merrifield Basin Cove PMIC
> + *
> + * Copyright (C) 2018 Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef __INTEL_SOC_PMIC_MRFLD_H__
> +#define __INTEL_SOC_PMIC_MRFLD_H__
> +
> +#include <linux/bits.h>
> +
> +#define BCOVE_ID		0x00
> +
> +#define BCOVE_ID_MINREV0	GENMASK(2, 0)
> +#define BCOVE_ID_MAJREV0	GENMASK(5, 3)
> +#define BCOVE_ID_VENDID0	GENMASK(7, 6)
> +
> +#define BCOVE_MINOR(x)		(unsigned int)(((x) & BCOVE_ID_MINREV0) >> 0)
> +#define BCOVE_MAJOR(x)		(unsigned int)(((x) & BCOVE_ID_MAJREV0) >> 3)
> +#define BCOVE_VENDOR(x)		(unsigned int)(((x) & BCOVE_ID_VENDID0) >> 6)
> +
> +#define BCOVE_IRQLVL1		0x01
> +
> +#define BCOVE_PBIRQ		0x02
> +#define BCOVE_TMUIRQ		0x03
> +#define BCOVE_THRMIRQ		0x04
> +#define BCOVE_BCUIRQ		0x05
> +#define BCOVE_ADCIRQ		0x06
> +#define BCOVE_CHGRIRQ0		0x07
> +#define BCOVE_CHGRIRQ1		0x08
> +#define BCOVE_GPIOIRQ		0x09
> +#define BCOVE_CRITIRQ		0x0B
> +
> +#define BCOVE_MIRQLVL1		0x0C
> +
> +#define BCOVE_MPBIRQ		0x0D
> +#define BCOVE_MTMUIRQ		0x0E
> +#define BCOVE_MTHRMIRQ		0x0F
> +#define BCOVE_MBCUIRQ		0x10
> +#define BCOVE_MADCIRQ		0x11
> +#define BCOVE_MCHGRIRQ0		0x12
> +#define BCOVE_MCHGRIRQ1		0x13
> +#define BCOVE_MGPIOIRQ		0x14
> +#define BCOVE_MCRITIRQ		0x16
> +
> +#define BCOVE_SCHGRIRQ0		0x4E
> +#define BCOVE_SCHGRIRQ1		0x4F
> +
> +/* Level 1 IRQs */
> +#define BCOVE_LVL1_PWRBTN	BIT(0)	/* power button */
> +#define BCOVE_LVL1_TMU		BIT(1)	/* time management unit */
> +#define BCOVE_LVL1_THRM		BIT(2)	/* thermal */
> +#define BCOVE_LVL1_BCU		BIT(3)	/* burst control unit */
> +#define BCOVE_LVL1_ADC		BIT(4)	/* ADC */
> +#define BCOVE_LVL1_CHGR		BIT(5)	/* charger */
> +#define BCOVE_LVL1_GPIO		BIT(6)	/* GPIO */
> +#define BCOVE_LVL1_CRIT		BIT(7)	/* critical event */
> +
> +/* Level 2 IRQs: power button */
> +#define BCOVE_PBIRQ_PBTN	BIT(0)
> +#define BCOVE_PBIRQ_UBTN	BIT(1)
> +
> +/* Level 2 IRQs: ADC */
> +#define BCOVE_ADCIRQ_BATTEMP	BIT(2)
> +#define BCOVE_ADCIRQ_SYSTEMP	BIT(3)
> +#define BCOVE_ADCIRQ_BATTID	BIT(4)
> +#define BCOVE_ADCIRQ_VIBATT	BIT(5)
> +#define BCOVE_ADCIRQ_CCTICK	BIT(7)
> +
> +/* Level 2 IRQs: charger */
> +#define BCOVE_CHGRIRQ_BAT0ALRT	BIT(4)
> +#define BCOVE_CHGRIRQ_BAT1ALRT	BIT(5)
> +#define BCOVE_CHGRIRQ_BATCRIT	BIT(6)
> +
> +#define BCOVE_CHGRIRQ_VBUSDET	BIT(0)
> +#define BCOVE_CHGRIRQ_DCDET	BIT(1)
> +#define BCOVE_CHGRIRQ_BATTDET	BIT(2)
> +#define BCOVE_CHGRIRQ_USBIDDET	BIT(3)
> +
> +#endif	/* __INTEL_SOC_PMIC_MRFLD_H__ */
> -- 
> 2.20.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] mfd: Add support for Merrifield Basin Cove PMIC

[Lee Jones]

On Mon, 18 Mar 2019, Andy Shevchenko wrote:

> Add an mfd driver for Intel Merrifield Basin Cove PMIC.

Nit: s/mfd/MFD/

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/mfd/Kconfig                      |  11 ++
>  drivers/mfd/Makefile                     |   1 +
>  drivers/mfd/intel_soc_pmic_mrfld.c       | 157 +++++++++++++++++++++++
>  include/linux/mfd/intel_soc_pmic_mrfld.h |  81 ++++++++++++
>  4 files changed, 250 insertions(+)
>  create mode 100644 drivers/mfd/intel_soc_pmic_mrfld.c
>  create mode 100644 include/linux/mfd/intel_soc_pmic_mrfld.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 0ce2d8dfc5f1..2adf9d393029 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -572,6 +572,17 @@ config INTEL_SOC_PMIC_CHTDC_TI
>  	  Select this option for supporting Dollar Cove (TI version) PMIC
>  	  device that is found on some Intel Cherry Trail systems.
>  
> +config INTEL_SOC_PMIC_MRFLD
> +	tristate "Support for Intel Merrifield Basin Cove PMIC"
> +	depends on GPIOLIB
> +	depends on ACPI
> +	depends on INTEL_SCU_IPC
> +	select MFD_CORE
> +	select REGMAP_IRQ
> +	help
> +	  Select this option for supporting Basin Cove PMIC device
> +	  that is found on Intel Merrifield systems.
> +
>  config MFD_INTEL_LPSS
>  	tristate
>  	select COMMON_CLK
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index b4569ed7f3f3..1b746bd01ac5 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -234,6 +234,7 @@ obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
>  obj-$(CONFIG_INTEL_SOC_PMIC_BXTWC)	+= intel_soc_pmic_bxtwc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC_CHTWC)	+= intel_soc_pmic_chtwc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI)	+= intel_soc_pmic_chtdc_ti.o
> +obj-$(CONFIG_INTEL_SOC_PMIC_MRFLD)	+= intel_soc_pmic_mrfld.o
>  obj-$(CONFIG_MFD_MT6397)	+= mt6397-core.o
>  
>  obj-$(CONFIG_MFD_ALTERA_A10SR)	+= altera-a10sr.o
> diff --git a/drivers/mfd/intel_soc_pmic_mrfld.c b/drivers/mfd/intel_soc_pmic_mrfld.c
> new file mode 100644
> index 000000000000..bbee89c0c25b
> --- /dev/null
> +++ b/drivers/mfd/intel_soc_pmic_mrfld.c
> @@ -0,0 +1,157 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device access for Basin Cove PMIC
> + *
> + * Copyright (c) 2018, Intel Corporation.
> + * Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/intel_soc_pmic.h>
> +#include <linux/mfd/intel_soc_pmic_mrfld.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <asm/intel_scu_ipc.h>
> +
> +/*
> + * Level 2 IRQs
> + *
> + * Firmware on the systems with Basin Cove PMIC services Level 1 IRQs
> + * without an assistance. Thus, each of the Level 1 IRQ is represented
> + * as a separate RTE in IOAPIC.
> + */
> +static struct resource irq_level2_resources[] = {
> +	DEFINE_RES_IRQ(0), /* power button */
> +	DEFINE_RES_IRQ(0), /* TMU */
> +	DEFINE_RES_IRQ(0), /* thermal */
> +	DEFINE_RES_IRQ(0), /* BCU */
> +	DEFINE_RES_IRQ(0), /* ADC */
> +	DEFINE_RES_IRQ(0), /* charger */
> +	DEFINE_RES_IRQ(0), /* GPIO */
> +};
> +
> +static const struct mfd_cell bcove_dev[] = {
> +	{
> +		.name = "mrfld_bcove_pwrbtn",
> +		.num_resources = 1,
> +		.resources = &irq_level2_resources[0],
> +	}, {
> +		.name = "mrfld_bcove_tmu",
> +		.num_resources = 1,
> +		.resources = &irq_level2_resources[1],
> +	}, {
> +		.name = "mrfld_bcove_thermal",
> +		.num_resources = 1,
> +		.resources = &irq_level2_resources[2],
> +	}, {
> +		.name = "mrfld_bcove_bcu",
> +		.num_resources = 1,
> +		.resources = &irq_level2_resources[3],
> +	}, {
> +		.name = "mrfld_bcove_adc",
> +		.num_resources = 1,
> +		.resources = &irq_level2_resources[4],
> +	}, {
> +		.name = "mrfld_bcove_charger",
> +		.num_resources = 1,
> +		.resources = &irq_level2_resources[5],
> +	}, {
> +		.name = "mrfld_bcove_extcon",
> +		.num_resources = 1,
> +		.resources = &irq_level2_resources[5],
> +	}, {
> +		.name = "mrfld_bcove_gpio",
> +		.num_resources = 1,
> +		.resources = &irq_level2_resources[6],
> +	},
> +	{	.name = "mrfld_bcove_region", },
> +};
> +
> +static int regmap_ipc_byte_reg_read(void *context, unsigned int reg,

Prefixing these with regmap is pretty confusing, since this it not
part of the Regmap API.  Better to provide them with local names
instead.

  bcove_ipc_byte_reg_read()

> +				    unsigned int *val)
> +{
> +	u8 ipc_out;
> +	int ret;
> +
> +	ret = intel_scu_ipc_ioread8(reg, &ipc_out);
> +	if (ret)
> +		return ret;
> +
> +	*val = ipc_out;
> +	return 0;
> +}
> +
> +static int regmap_ipc_byte_reg_write(void *context, unsigned int reg,
> +				     unsigned int val)
> +{
> +	u8 ipc_in = val;
> +	int ret;
> +
> +	ret = intel_scu_ipc_iowrite8(reg, ipc_in);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static const struct regmap_config bcove_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 8,
> +	.max_register = 0xff,
> +	.reg_write = regmap_ipc_byte_reg_write,
> +	.reg_read = regmap_ipc_byte_reg_read,
> +};
> +
> +static int bcove_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct intel_soc_pmic *pmic;
> +	unsigned int i;
> +	int ret;
> +
> +	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
> +	if (!pmic)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, pmic);
> +	pmic->dev = &pdev->dev;
> +
> +	pmic->regmap = devm_regmap_init(dev, NULL, pmic, &bcove_regmap_config);
> +	if (IS_ERR(pmic->regmap))
> +		return PTR_ERR(pmic->regmap);
> +
> +	for (i = 0; i < ARRAY_SIZE(irq_level2_resources); i++) {
> +		ret = platform_get_irq(pdev, i);
> +		if (ret < 0)
> +			return ret;
> +
> +		irq_level2_resources[i].start = ret;
> +		irq_level2_resources[i].end = ret;
> +	}

Although succinct, dragging values from one platform device into
another doesn't sound that neat.  Also, since the ordering of the
devices is critical in this implementation, it also comes across as
fragile.

Any reason why ACPI can't register all of the child devices, or for
the child devices to obtain their IRQ directly from the tables?

> +	return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
> +				    bcove_dev, ARRAY_SIZE(bcove_dev),
> +				    NULL, 0, NULL);
> +}
> +
> +static const struct acpi_device_id bcove_acpi_ids[] = {
> +	{ "INTC100E" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, bcove_acpi_ids);
> +
> +static struct platform_driver bcove_driver = {
> +	.driver = {
> +		.name = "intel_soc_pmic_mrfld",
> +		.acpi_match_table = bcove_acpi_ids,
> +	},
> +	.probe = bcove_probe,
> +};
> +module_platform_driver(bcove_driver);
> +
> +MODULE_DESCRIPTION("IPC driver for Intel SoC Basin Cove PMIC");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/intel_soc_pmic_mrfld.h b/include/linux/mfd/intel_soc_pmic_mrfld.h
> new file mode 100644
> index 000000000000..00feed7c2787
> --- /dev/null
> +++ b/include/linux/mfd/intel_soc_pmic_mrfld.h
> @@ -0,0 +1,81 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Header file for Intel Merrifield Basin Cove PMIC
> + *
> + * Copyright (C) 2018 Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef __INTEL_SOC_PMIC_MRFLD_H__
> +#define __INTEL_SOC_PMIC_MRFLD_H__
> +
> +#include <linux/bits.h>
> +
> +#define BCOVE_ID		0x00
> +
> +#define BCOVE_ID_MINREV0	GENMASK(2, 0)
> +#define BCOVE_ID_MAJREV0	GENMASK(5, 3)
> +#define BCOVE_ID_VENDID0	GENMASK(7, 6)
> +
> +#define BCOVE_MINOR(x)		(unsigned int)(((x) & BCOVE_ID_MINREV0) >> 0)
> +#define BCOVE_MAJOR(x)		(unsigned int)(((x) & BCOVE_ID_MAJREV0) >> 3)
> +#define BCOVE_VENDOR(x)		(unsigned int)(((x) & BCOVE_ID_VENDID0) >> 6)
> +
> +#define BCOVE_IRQLVL1		0x01
> +
> +#define BCOVE_PBIRQ		0x02
> +#define BCOVE_TMUIRQ		0x03
> +#define BCOVE_THRMIRQ		0x04
> +#define BCOVE_BCUIRQ		0x05
> +#define BCOVE_ADCIRQ		0x06
> +#define BCOVE_CHGRIRQ0		0x07
> +#define BCOVE_CHGRIRQ1		0x08
> +#define BCOVE_GPIOIRQ		0x09
> +#define BCOVE_CRITIRQ		0x0B
> +
> +#define BCOVE_MIRQLVL1		0x0C
> +
> +#define BCOVE_MPBIRQ		0x0D
> +#define BCOVE_MTMUIRQ		0x0E
> +#define BCOVE_MTHRMIRQ		0x0F
> +#define BCOVE_MBCUIRQ		0x10
> +#define BCOVE_MADCIRQ		0x11
> +#define BCOVE_MCHGRIRQ0		0x12
> +#define BCOVE_MCHGRIRQ1		0x13
> +#define BCOVE_MGPIOIRQ		0x14
> +#define BCOVE_MCRITIRQ		0x16
> +
> +#define BCOVE_SCHGRIRQ0		0x4E
> +#define BCOVE_SCHGRIRQ1		0x4F
> +
> +/* Level 1 IRQs */
> +#define BCOVE_LVL1_PWRBTN	BIT(0)	/* power button */
> +#define BCOVE_LVL1_TMU		BIT(1)	/* time management unit */
> +#define BCOVE_LVL1_THRM		BIT(2)	/* thermal */
> +#define BCOVE_LVL1_BCU		BIT(3)	/* burst control unit */
> +#define BCOVE_LVL1_ADC		BIT(4)	/* ADC */
> +#define BCOVE_LVL1_CHGR		BIT(5)	/* charger */
> +#define BCOVE_LVL1_GPIO		BIT(6)	/* GPIO */
> +#define BCOVE_LVL1_CRIT		BIT(7)	/* critical event */
> +
> +/* Level 2 IRQs: power button */
> +#define BCOVE_PBIRQ_PBTN	BIT(0)
> +#define BCOVE_PBIRQ_UBTN	BIT(1)
> +
> +/* Level 2 IRQs: ADC */
> +#define BCOVE_ADCIRQ_BATTEMP	BIT(2)
> +#define BCOVE_ADCIRQ_SYSTEMP	BIT(3)
> +#define BCOVE_ADCIRQ_BATTID	BIT(4)
> +#define BCOVE_ADCIRQ_VIBATT	BIT(5)
> +#define BCOVE_ADCIRQ_CCTICK	BIT(7)
> +
> +/* Level 2 IRQs: charger */
> +#define BCOVE_CHGRIRQ_BAT0ALRT	BIT(4)
> +#define BCOVE_CHGRIRQ_BAT1ALRT	BIT(5)
> +#define BCOVE_CHGRIRQ_BATCRIT	BIT(6)
> +
> +#define BCOVE_CHGRIRQ_VBUSDET	BIT(0)
> +#define BCOVE_CHGRIRQ_DCDET	BIT(1)
> +#define BCOVE_CHGRIRQ_BATTDET	BIT(2)
> +#define BCOVE_CHGRIRQ_USBIDDET	BIT(3)
> +
> +#endif	/* __INTEL_SOC_PMIC_MRFLD_H__ */

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v1] mfd: Add support for Merrifield Basin Cove PMIC

[Andy Shevchenko]

On Tue, Apr 02, 2019 at 06:12:11AM +0100, Lee Jones wrote:
> On Mon, 18 Mar 2019, Andy Shevchenko wrote:
> 
> > Add an mfd driver for Intel Merrifield Basin Cove PMIC.
> 
> Nit: s/mfd/MFD/

Noted. And changed for v2.

> > +static const struct mfd_cell bcove_dev[] = {
> > +	{
> > +		.name = "mrfld_bcove_pwrbtn",
> > +		.num_resources = 1,
> > +		.resources = &irq_level2_resources[0],
> > +	}, {
> > +		.name = "mrfld_bcove_tmu",
> > +		.num_resources = 1,
> > +		.resources = &irq_level2_resources[1],
> > +	}, {
> > +		.name = "mrfld_bcove_thermal",
> > +		.num_resources = 1,
> > +		.resources = &irq_level2_resources[2],
> > +	}, {
> > +		.name = "mrfld_bcove_bcu",
> > +		.num_resources = 1,
> > +		.resources = &irq_level2_resources[3],
> > +	}, {
> > +		.name = "mrfld_bcove_adc",
> > +		.num_resources = 1,
> > +		.resources = &irq_level2_resources[4],
> > +	}, {
> > +		.name = "mrfld_bcove_charger",
> > +		.num_resources = 1,
> > +		.resources = &irq_level2_resources[5],
> > +	}, {
> > +		.name = "mrfld_bcove_extcon",
> > +		.num_resources = 1,
> > +		.resources = &irq_level2_resources[5],
> > +	}, {
> > +		.name = "mrfld_bcove_gpio",
> > +		.num_resources = 1,
> > +		.resources = &irq_level2_resources[6],
> > +	},
> > +	{	.name = "mrfld_bcove_region", },
> > +};

> > +static int regmap_ipc_byte_reg_read(void *context, unsigned int reg,
> 
> Prefixing these with regmap is pretty confusing, since this it not
> part of the Regmap API.  Better to provide them with local names
> instead.
> 
>   bcove_ipc_byte_reg_read()

Good point. And changed for v2.

> > +	for (i = 0; i < ARRAY_SIZE(irq_level2_resources); i++) {
> > +		ret = platform_get_irq(pdev, i);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		irq_level2_resources[i].start = ret;
> > +		irq_level2_resources[i].end = ret;
> > +	}
> 
> Although succinct, dragging values from one platform device into
> another doesn't sound that neat.

So, how to split resources given in one _physical_ multi-functional device to
several of them?  Isn't it what MFD framework for?

Any other approach here? I'm all ears!

> Also, since the ordering of the
> devices is critical in this implementation, it also comes across as
> fragile.

How fragile? In ACPI we don't have IRQ labeling scheme. Index is used for that.

> Any reason why ACPI can't register all of the child devices, or for
> the child devices to obtain their IRQ directly from the tables?

And how are we supposed to enumerated them taking into consideration single
ACPI ID given?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] mfd: Add support for Merrifield Basin Cove PMIC

[Andy Shevchenko]

On Tue, Apr 02, 2019 at 03:20:01PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 02, 2019 at 06:12:11AM +0100, Lee Jones wrote:

> Noted. And changed for v2.

> Good point. And changed for v2.

s/v2/v3/g

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] mfd: Add support for Merrifield Basin Cove PMIC

[Lee Jones]

On Tue, 02 Apr 2019, Andy Shevchenko wrote:

> On Tue, Apr 02, 2019 at 06:12:11AM +0100, Lee Jones wrote:
> > On Mon, 18 Mar 2019, Andy Shevchenko wrote:
> > 
> > > Add an mfd driver for Intel Merrifield Basin Cove PMIC.
> > 
> > Nit: s/mfd/MFD/
> 
> Noted. And changed for v2.
> 
> > > +static const struct mfd_cell bcove_dev[] = {
> > > +	{
> > > +		.name = "mrfld_bcove_pwrbtn",
> > > +		.num_resources = 1,
> > > +		.resources = &irq_level2_resources[0],
> > > +	}, {
> > > +		.name = "mrfld_bcove_tmu",
> > > +		.num_resources = 1,
> > > +		.resources = &irq_level2_resources[1],
> > > +	}, {
> > > +		.name = "mrfld_bcove_thermal",
> > > +		.num_resources = 1,
> > > +		.resources = &irq_level2_resources[2],
> > > +	}, {
> > > +		.name = "mrfld_bcove_bcu",
> > > +		.num_resources = 1,
> > > +		.resources = &irq_level2_resources[3],
> > > +	}, {
> > > +		.name = "mrfld_bcove_adc",
> > > +		.num_resources = 1,
> > > +		.resources = &irq_level2_resources[4],
> > > +	}, {
> > > +		.name = "mrfld_bcove_charger",
> > > +		.num_resources = 1,
> > > +		.resources = &irq_level2_resources[5],
> > > +	}, {
> > > +		.name = "mrfld_bcove_extcon",
> > > +		.num_resources = 1,
> > > +		.resources = &irq_level2_resources[5],
> > > +	}, {
> > > +		.name = "mrfld_bcove_gpio",
> > > +		.num_resources = 1,
> > > +		.resources = &irq_level2_resources[6],
> > > +	},
> > > +	{	.name = "mrfld_bcove_region", },
> > > +};
> 
> > > +static int regmap_ipc_byte_reg_read(void *context, unsigned int reg,
> > 
> > Prefixing these with regmap is pretty confusing, since this it not
> > part of the Regmap API.  Better to provide them with local names
> > instead.
> > 
> >   bcove_ipc_byte_reg_read()
> 
> Good point. And changed for v2.
> 
> > > +	for (i = 0; i < ARRAY_SIZE(irq_level2_resources); i++) {
> > > +		ret = platform_get_irq(pdev, i);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > > +		irq_level2_resources[i].start = ret;
> > > +		irq_level2_resources[i].end = ret;
> > > +	}
> > 
> > Although succinct, dragging values from one platform device into
> > another doesn't sound that neat.
> 
> So, how to split resources given in one _physical_ multi-functional device to
> several of them?  Isn't it what MFD framework for?
> 
> Any other approach here? I'm all ears!

From the child:

  platform_get_irq(dev->parent, CLIENT_ID);

> > Also, since the ordering of the
> > devices is critical in this implementation, it also comes across as
> > fragile.
> 
> How fragile? In ACPI we don't have IRQ labeling scheme. Index is used for that.
> 
> > Any reason why ACPI can't register all of the child devices, or for
> > the child devices to obtain their IRQ directly from the tables?
> 
> And how are we supposed to enumerated them taking into consideration single
> ACPI ID given?

This question was a little whimsical, since I have no idea how the
ACPI tables you're working with are laid out.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v1] mfd: Add support for Merrifield Basin Cove PMIC

[Lee Jones]

On Thu, 04 Apr 2019, Lee Jones wrote:

> On Tue, 02 Apr 2019, Andy Shevchenko wrote:
> 
> > On Tue, Apr 02, 2019 at 06:12:11AM +0100, Lee Jones wrote:
> > > On Mon, 18 Mar 2019, Andy Shevchenko wrote:
> > > 
> > > > Add an mfd driver for Intel Merrifield Basin Cove PMIC.
> > > 
> > > Nit: s/mfd/MFD/
> > 
> > Noted. And changed for v2.
> > 
> > > > +static const struct mfd_cell bcove_dev[] = {
> > > > +	{
> > > > +		.name = "mrfld_bcove_pwrbtn",
> > > > +		.num_resources = 1,
> > > > +		.resources = &irq_level2_resources[0],
> > > > +	}, {
> > > > +		.name = "mrfld_bcove_tmu",
> > > > +		.num_resources = 1,
> > > > +		.resources = &irq_level2_resources[1],
> > > > +	}, {
> > > > +		.name = "mrfld_bcove_thermal",
> > > > +		.num_resources = 1,
> > > > +		.resources = &irq_level2_resources[2],
> > > > +	}, {
> > > > +		.name = "mrfld_bcove_bcu",
> > > > +		.num_resources = 1,
> > > > +		.resources = &irq_level2_resources[3],
> > > > +	}, {
> > > > +		.name = "mrfld_bcove_adc",
> > > > +		.num_resources = 1,
> > > > +		.resources = &irq_level2_resources[4],
> > > > +	}, {
> > > > +		.name = "mrfld_bcove_charger",
> > > > +		.num_resources = 1,
> > > > +		.resources = &irq_level2_resources[5],
> > > > +	}, {
> > > > +		.name = "mrfld_bcove_extcon",
> > > > +		.num_resources = 1,
> > > > +		.resources = &irq_level2_resources[5],
> > > > +	}, {
> > > > +		.name = "mrfld_bcove_gpio",
> > > > +		.num_resources = 1,
> > > > +		.resources = &irq_level2_resources[6],
> > > > +	},
> > > > +	{	.name = "mrfld_bcove_region", },
> > > > +};
> > 
> > > > +static int regmap_ipc_byte_reg_read(void *context, unsigned int reg,
> > > 
> > > Prefixing these with regmap is pretty confusing, since this it not
> > > part of the Regmap API.  Better to provide them with local names
> > > instead.
> > > 
> > >   bcove_ipc_byte_reg_read()
> > 
> > Good point. And changed for v2.
> > 
> > > > +	for (i = 0; i < ARRAY_SIZE(irq_level2_resources); i++) {
> > > > +		ret = platform_get_irq(pdev, i);
> > > > +		if (ret < 0)
> > > > +			return ret;
> > > > +
> > > > +		irq_level2_resources[i].start = ret;
> > > > +		irq_level2_resources[i].end = ret;
> > > > +	}
> > > 
> > > Although succinct, dragging values from one platform device into
> > > another doesn't sound that neat.
> > 
> > So, how to split resources given in one _physical_ multi-functional device to
> > several of them?  Isn't it what MFD framework for?
> > 
> > Any other approach here? I'm all ears!
> 
> From the child:
> 
>   platform_get_irq(dev->parent, CLIENT_ID);

If you set the .id of the cell properly you could do:

  platform_get_irq(dev->parent, dev->id);

> > > Also, since the ordering of the
> > > devices is critical in this implementation, it also comes across as
> > > fragile.
> > 
> > How fragile? In ACPI we don't have IRQ labeling scheme. Index is used for that.
> > 
> > > Any reason why ACPI can't register all of the child devices, or for
> > > the child devices to obtain their IRQ directly from the tables?
> > 
> > And how are we supposed to enumerated them taking into consideration single
> > ACPI ID given?
> 
> This question was a little whimsical, since I have no idea how the
> ACPI tables you're working with are laid out.
> 

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v1] mfd: Add support for Merrifield Basin Cove PMIC

[Andy Shevchenko]

On Thu, Apr 04, 2019 at 08:03:57AM +0100, Lee Jones wrote:
> On Thu, 04 Apr 2019, Lee Jones wrote:
> > On Tue, 02 Apr 2019, Andy Shevchenko wrote:
> > > On Tue, Apr 02, 2019 at 06:12:11AM +0100, Lee Jones wrote:
> > > > On Mon, 18 Mar 2019, Andy Shevchenko wrote:

> > > > > +static const struct mfd_cell bcove_dev[] = {
> > > > > +	{
> > > > > +		.name = "mrfld_bcove_pwrbtn",
> > > > > +		.num_resources = 1,
> > > > > +		.resources = &irq_level2_resources[0],
> > > > > +	}, {
> > > > > +		.name = "mrfld_bcove_tmu",
> > > > > +		.num_resources = 1,
> > > > > +		.resources = &irq_level2_resources[1],
> > > > > +	}, {
> > > > > +		.name = "mrfld_bcove_thermal",
> > > > > +		.num_resources = 1,
> > > > > +		.resources = &irq_level2_resources[2],
> > > > > +	}, {
> > > > > +		.name = "mrfld_bcove_bcu",
> > > > > +		.num_resources = 1,
> > > > > +		.resources = &irq_level2_resources[3],
> > > > > +	}, {
> > > > > +		.name = "mrfld_bcove_adc",
> > > > > +		.num_resources = 1,
> > > > > +		.resources = &irq_level2_resources[4],
> > > > > +	}, {
> > > > > +		.name = "mrfld_bcove_charger",
> > > > > +		.num_resources = 1,
> > > > > +		.resources = &irq_level2_resources[5],
> > > > > +	}, {
> > > > > +		.name = "mrfld_bcove_extcon",
> > > > > +		.num_resources = 1,
> > > > > +		.resources = &irq_level2_resources[5],
> > > > > +	}, {
> > > > > +		.name = "mrfld_bcove_gpio",
> > > > > +		.num_resources = 1,
> > > > > +		.resources = &irq_level2_resources[6],
> > > > > +	},
> > > > > +	{	.name = "mrfld_bcove_region", },
> > > > > +};

> > > > > +	for (i = 0; i < ARRAY_SIZE(irq_level2_resources); i++) {
> > > > > +		ret = platform_get_irq(pdev, i);
> > > > > +		if (ret < 0)
> > > > > +			return ret;
> > > > > +
> > > > > +		irq_level2_resources[i].start = ret;
> > > > > +		irq_level2_resources[i].end = ret;
> > > > > +	}
> > > > 
> > > > Although succinct, dragging values from one platform device into
> > > > another doesn't sound that neat.
> > > 
> > > So, how to split resources given in one _physical_ multi-functional device to
> > > several of them?  Isn't it what MFD framework for?
> > > 
> > > Any other approach here? I'm all ears!
> > 
> > From the child:
> > 
> >   platform_get_irq(dev->parent, CLIENT_ID);

So, instead of keeping a fragile approach in one driver, we will spread this
to all of them.

> If you set the .id of the cell properly you could do:
> 
>   platform_get_irq(dev->parent, dev->id);

This will bring a confuse, ID is used to form an instance name, for now
we don't have several instances of any of the devices from PMIC.

On top of above, some of the resources (one already, others might be a case in
the future) is split between two drivers, which would bring even more confusion
to the entire picture.

> > > > Also, since the ordering of the
> > > > devices is critical in this implementation, it also comes across as
> > > > fragile.
> > > 
> > > How fragile? In ACPI we don't have IRQ labeling scheme. Index is used for that.
> > > 
> > > > Any reason why ACPI can't register all of the child devices, or for
> > > > the child devices to obtain their IRQ directly from the tables?
> > > 
> > > And how are we supposed to enumerated them taking into consideration single
> > > ACPI ID given?
> > 
> > This question was a little whimsical, since I have no idea how the
> > ACPI tables you're working with are laid out.

There is one device node with several IRQ and other resources.
In pseudo code:

	device node {
		device ID,
		IRQ 0,
		IRQ 1,
		...
		MMIO 0,
		...
	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] mfd: Add support for Merrifield Basin Cove PMIC

[Lee Jones]

On Thu, 04 Apr 2019, Andy Shevchenko wrote:

> On Thu, Apr 04, 2019 at 08:03:57AM +0100, Lee Jones wrote:
> > On Thu, 04 Apr 2019, Lee Jones wrote:
> > > On Tue, 02 Apr 2019, Andy Shevchenko wrote:
> > > > On Tue, Apr 02, 2019 at 06:12:11AM +0100, Lee Jones wrote:
> > > > > On Mon, 18 Mar 2019, Andy Shevchenko wrote:
> 
> > > > > > +static const struct mfd_cell bcove_dev[] = {
> > > > > > +	{
> > > > > > +		.name = "mrfld_bcove_pwrbtn",
> > > > > > +		.num_resources = 1,
> > > > > > +		.resources = &irq_level2_resources[0],
> > > > > > +	}, {
> > > > > > +		.name = "mrfld_bcove_tmu",
> > > > > > +		.num_resources = 1,
> > > > > > +		.resources = &irq_level2_resources[1],
> > > > > > +	}, {
> > > > > > +		.name = "mrfld_bcove_thermal",
> > > > > > +		.num_resources = 1,
> > > > > > +		.resources = &irq_level2_resources[2],
> > > > > > +	}, {
> > > > > > +		.name = "mrfld_bcove_bcu",
> > > > > > +		.num_resources = 1,
> > > > > > +		.resources = &irq_level2_resources[3],
> > > > > > +	}, {
> > > > > > +		.name = "mrfld_bcove_adc",
> > > > > > +		.num_resources = 1,
> > > > > > +		.resources = &irq_level2_resources[4],
> > > > > > +	}, {
> > > > > > +		.name = "mrfld_bcove_charger",
> > > > > > +		.num_resources = 1,
> > > > > > +		.resources = &irq_level2_resources[5],
> > > > > > +	}, {
> > > > > > +		.name = "mrfld_bcove_extcon",
> > > > > > +		.num_resources = 1,
> > > > > > +		.resources = &irq_level2_resources[5],
> > > > > > +	}, {
> > > > > > +		.name = "mrfld_bcove_gpio",
> > > > > > +		.num_resources = 1,
> > > > > > +		.resources = &irq_level2_resources[6],
> > > > > > +	},
> > > > > > +	{	.name = "mrfld_bcove_region", },
> > > > > > +};
> 
> > > > > > +	for (i = 0; i < ARRAY_SIZE(irq_level2_resources); i++) {
> > > > > > +		ret = platform_get_irq(pdev, i);
> > > > > > +		if (ret < 0)
> > > > > > +			return ret;
> > > > > > +
> > > > > > +		irq_level2_resources[i].start = ret;
> > > > > > +		irq_level2_resources[i].end = ret;
> > > > > > +	}
> > > > > 
> > > > > Although succinct, dragging values from one platform device into
> > > > > another doesn't sound that neat.
> > > > 
> > > > So, how to split resources given in one _physical_ multi-functional device to
> > > > several of them?  Isn't it what MFD framework for?
> > > > 
> > > > Any other approach here? I'm all ears!
> > > 
> > > From the child:
> > > 
> > >   platform_get_irq(dev->parent, CLIENT_ID);
> 
> So, instead of keeping a fragile approach in one driver, we will spread this
> to all of them.

No, the fragileness goes away with implicit definitions of IDs.

> > If you set the .id of the cell properly you could do:
> > 
> >   platform_get_irq(dev->parent, dev->id);
> 
> This will bring a confuse, ID is used to form an instance name, for now
> we don't have several instances of any of the devices from PMIC.

That is true.  It is probably an abuse of the API. :)

I'm just floating ideas.

> On top of above, some of the resources (one already, others might be a case in
> the future) is split between two drivers, which would bring even more confusion
> to the entire picture.

I don't see any indication in the code that 2 platform devices can't
share the same .id value.  But again, this is probably academic since
abusing the API should probably be avoided in general.

> > > > > Also, since the ordering of the
> > > > > devices is critical in this implementation, it also comes across as
> > > > > fragile.
> > > > 
> > > > How fragile? In ACPI we don't have IRQ labeling scheme. Index is used for that.
> > > > 
> > > > > Any reason why ACPI can't register all of the child devices, or for
> > > > > the child devices to obtain their IRQ directly from the tables?
> > > > 
> > > > And how are we supposed to enumerated them taking into consideration single
> > > > ACPI ID given?
> > > 
> > > This question was a little whimsical, since I have no idea how the
> > > ACPI tables you're working with are laid out.
> 
> There is one device node with several IRQ and other resources.
> In pseudo code:
> 
> 	device node {
> 		device ID,
> 		IRQ 0,
> 		IRQ 1,
> 		...
> 		MMIO 0,
> 		...
> 	}

Sure.  Thanks for the explanation.

Very well.  I guess it's not too bad as it is.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v1] mfd: Add support for Merrifield Basin Cove PMIC

[Andy Shevchenko]

On Thu, Apr 04, 2019 at 10:03:14AM +0100, Lee Jones wrote:
> On Thu, 04 Apr 2019, Andy Shevchenko wrote:
> > On Thu, Apr 04, 2019 at 08:03:57AM +0100, Lee Jones wrote:
> > > On Thu, 04 Apr 2019, Lee Jones wrote:
> > > > On Tue, 02 Apr 2019, Andy Shevchenko wrote:
> > > > > On Tue, Apr 02, 2019 at 06:12:11AM +0100, Lee Jones wrote:
> > > > > > On Mon, 18 Mar 2019, Andy Shevchenko wrote:

> > > > > > Although succinct, dragging values from one platform device into
> > > > > > another doesn't sound that neat.
> > > > > 
> > > > > So, how to split resources given in one _physical_ multi-functional device to
> > > > > several of them?  Isn't it what MFD framework for?
> > > > > 
> > > > > Any other approach here? I'm all ears!
> > > > 
> > > > From the child:
> > > > 
> > > >   platform_get_irq(dev->parent, CLIENT_ID);
> > 
> > So, instead of keeping a fragile approach in one driver, we will spread this
> > to all of them.
> 
> No, the fragileness goes away with implicit definitions of IDs.

Did you mean "explicit"?
Something like we need to have a shared map of those indices?

> > > > > > Also, since the ordering of the
> > > > > > devices is critical in this implementation, it also comes across as
> > > > > > fragile.
> > > > > 
> > > > > How fragile? In ACPI we don't have IRQ labeling scheme. Index is used for that.
> > > > > 
> > > > > > Any reason why ACPI can't register all of the child devices, or for
> > > > > > the child devices to obtain their IRQ directly from the tables?
> > > > > 
> > > > > And how are we supposed to enumerated them taking into consideration single
> > > > > ACPI ID given?
> > > > 
> > > > This question was a little whimsical, since I have no idea how the
> > > > ACPI tables you're working with are laid out.
> > 
> > There is one device node with several IRQ and other resources.
> > In pseudo code:
> > 
> > 	device node {
> > 		device ID,
> > 		IRQ 0,
> > 		IRQ 1,
> > 		...
> > 		MMIO 0,
> > 		...
> > 	}
> 
> Sure.  Thanks for the explanation.
> 
> Very well.  I guess it's not too bad as it is.

It represent real hardware 1:1.
Just out of curiosity how this case can be described in DT?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] mfd: Add support for Merrifield Basin Cove PMIC

[Lee Jones]

On Thu, 04 Apr 2019, Andy Shevchenko wrote:

> On Thu, Apr 04, 2019 at 10:03:14AM +0100, Lee Jones wrote:
> > On Thu, 04 Apr 2019, Andy Shevchenko wrote:
> > > On Thu, Apr 04, 2019 at 08:03:57AM +0100, Lee Jones wrote:
> > > > On Thu, 04 Apr 2019, Lee Jones wrote:
> > > > > On Tue, 02 Apr 2019, Andy Shevchenko wrote:
> > > > > > On Tue, Apr 02, 2019 at 06:12:11AM +0100, Lee Jones wrote:
> > > > > > > On Mon, 18 Mar 2019, Andy Shevchenko wrote:
> 
> > > > > > > Although succinct, dragging values from one platform device into
> > > > > > > another doesn't sound that neat.
> > > > > > 
> > > > > > So, how to split resources given in one _physical_ multi-functional device to
> > > > > > several of them?  Isn't it what MFD framework for?
> > > > > > 
> > > > > > Any other approach here? I'm all ears!
> > > > > 
> > > > > From the child:
> > > > > 
> > > > >   platform_get_irq(dev->parent, CLIENT_ID);
> > > 
> > > So, instead of keeping a fragile approach in one driver, we will spread this
> > > to all of them.
> > 
> > No, the fragileness goes away with implicit definitions of IDs.
> 
> Did you mean "explicit"?

Yes.  Thank you for correcting my English. :)

> Something like we need to have a shared map of those indices?

Defining the IDs of the devices would lead to a more robust
implementation, yes.

> > > > > > > Also, since the ordering of the
> > > > > > > devices is critical in this implementation, it also comes across as
> > > > > > > fragile.
> > > > > > 
> > > > > > How fragile? In ACPI we don't have IRQ labeling scheme. Index is used for that.
> > > > > > 
> > > > > > > Any reason why ACPI can't register all of the child devices, or for
> > > > > > > the child devices to obtain their IRQ directly from the tables?
> > > > > > 
> > > > > > And how are we supposed to enumerated them taking into consideration single
> > > > > > ACPI ID given?
> > > > > 
> > > > > This question was a little whimsical, since I have no idea how the
> > > > > ACPI tables you're working with are laid out.
> > > 
> > > There is one device node with several IRQ and other resources.
> > > In pseudo code:
> > > 
> > > 	device node {
> > > 		device ID,
> > > 		IRQ 0,
> > > 		IRQ 1,
> > > 		...
> > > 		MMIO 0,
> > > 		...
> > > 	}
> > 
> > Sure.  Thanks for the explanation.
> > 
> > Very well.  I guess it's not too bad as it is.
> 
> It represent real hardware 1:1.
> Just out of curiosity how this case can be described in DT?

In DT you can have a sub-node for each child which can contain the
IRQ.  Without a sub-node you would define the IRQs in this file.  If
these IRQs do not change, that option is still available to you.

I can't think of an example where all of the children's IRQs have been
listed in the parent's DT node in this way.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v1] mfd: Add support for Merrifield Basin Cove PMIC

[Andy Shevchenko]

On Thu, Apr 04, 2019 at 10:44:59AM +0100, Lee Jones wrote:
> On Thu, 04 Apr 2019, Andy Shevchenko wrote:
> > On Thu, Apr 04, 2019 at 10:03:14AM +0100, Lee Jones wrote:
> > > On Thu, 04 Apr 2019, Andy Shevchenko wrote:
> > > > On Thu, Apr 04, 2019 at 08:03:57AM +0100, Lee Jones wrote:
> > > > > On Thu, 04 Apr 2019, Lee Jones wrote:
> > > > > > On Tue, 02 Apr 2019, Andy Shevchenko wrote:

> > > > > > From the child:
> > > > > > 
> > > > > >   platform_get_irq(dev->parent, CLIENT_ID);
> > > > 
> > > > So, instead of keeping a fragile approach in one driver, we will spread this
> > > > to all of them.
> > > 
> > > No, the fragileness goes away with implicit definitions of IDs.
> > 
> > Did you mean "explicit"?
> 
> Yes.  Thank you for correcting my English. :)
> 
> > Something like we need to have a shared map of those indices?
> 
> Defining the IDs of the devices would lead to a more robust
> implementation, yes.

This would make children to know that they are springs of exact MFD parent driver
that makes them dependent and inflexible.

> > > > There is one device node with several IRQ and other resources.
> > > > In pseudo code:
> > > > 
> > > > 	device node {
> > > > 		device ID,
> > > > 		IRQ 0,
> > > > 		IRQ 1,
> > > > 		...
> > > > 		MMIO 0,
> > > > 		...
> > > > 	}
> > > 
> > > Sure.  Thanks for the explanation.
> > > 
> > > Very well.  I guess it's not too bad as it is.
> > 
> > It represent real hardware 1:1.
> > Just out of curiosity how this case can be described in DT?
> 
> In DT you can have a sub-node for each child which can contain the
> IRQ.  Without a sub-node you would define the IRQs in this file.  If
> these IRQs do not change, that option is still available to you.
> 
> I can't think of an example where all of the children's IRQs have been
> listed in the parent's DT node in this way.

I see. Something similar is done in ACPI table for Intel Galileo Gen2
(see intel_quark_i2c_gpio.c), though it's a PCI device with shared
interrupt line.

If it would be a case like above, MFD would have propagated IRQ resources from
them to the children anyway.

-- 
With Best Regards,
Andy Shevchenko