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

From: Lee Jones
Date: Mon Aug 12 2019 - 05:53:45 EST


On Thu, 01 Aug 2019, Andy Shevchenko wrote:

> Add an MFD driver for Intel Merrifield Basin Cove PMIC.
>
> Firmware on the platforms which are using Basin Cove PMIC is "smarter"
> than on the rest supported by vanilla kernel. It handles first level
> of interrupt itself, while others do it on OS level.
>
> The driver is done in the same way as the rest of Intel PMIC MFD drivers
> in the kernel to support the initial design. The design allows to use
> one driver among few PMICs without knowing implementation details of
> the each hardware version or generation.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
> v4: elaborate in the commit message the design choice
> 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 f129f9678940..adf178ad5e7b 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -597,6 +597,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 f026ada68f6a..637ecf6b12de 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -241,6 +241,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..26a1551c5faf
> --- /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) 2019, Intel Corporation.
> + * Author: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> + */
> +
> +#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_pwrsrc",
> + .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 bcove_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 bcove_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;
> +}

Is intel_scu_ipc_iowrite8() going to be used with Regmap again? If
so, it's probably best to have these as part of the API. Probably
supplied as an inline function.

> +static const struct regmap_config bcove_regmap_config = {
> + .reg_bits = 16,
> + .val_bits = 8,
> + .max_register = 0xff,
> + .reg_write = bcove_ipc_byte_reg_write,
> + .reg_read = bcove_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;
> + }

I still get the same feeling that this is a waste of a driver!

ACPI hackery, if you will. :(

It's just a shame that I can't think of a better way to do it.

> + 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..4daecd682275
> --- /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) 2019 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