On Tue, 28 Apr 2020, Guru Das Srinagesh wrote:
> The Qualcomm Technologies, Inc. I2C PMIC Controller is used by
> multi-function PMIC devices which communicate over the I2C bus. The
> controller enumerates all child nodes as platform devices, and
> instantiates a regmap interface for them to communicate over the I2C
> bus.
>
> The controller also controls interrupts for all of the children platform
> devices. The controller handles the summary interrupt by deciphering
> which peripheral triggered the interrupt, and which of the peripheral
> interrupts were triggered. Finally, it calls the interrupt handlers for
> each of the virtual interrupts that were registered.
>
> Nicholas Troast is the original author of this driver.
>
> Signed-off-by: Guru Das Srinagesh <gurus@xxxxxxxxxxxxxx>
> ---
> drivers/mfd/Kconfig | 11 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/qcom-i2c-pmic.c | 737 ++++++++++++++++++++++++++++++++++++++++++++
The vast majority of this driver deals with IRQ handling. Why can't
this be split out into its own IRQ Chip driver and moved to
drivers/irqchip?
> 3 files changed, 749 insertions(+)
> create mode 100644 drivers/mfd/qcom-i2c-pmic.c
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 54b6aa4..bf112eb 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1002,6 +1002,17 @@ config MFD_PM8XXX
> Say M here if you want to include support for PM8xxx chips as a
> module. This will build a module called "pm8xxx-core".
>
> +config MFD_I2C_PMIC
Too generic. This should identify the vendor too.
> + tristate "QTI I2C PMIC support"
Why aren't you using QCOM?
Actually, this should be expanded here anyway.
> + depends on I2C && OF
> + select IRQ_DOMAIN
> + select REGMAP_I2C
> + help
> + This enables support for controlling Qualcomm Technologies, Inc.
> + PMICs over I2C. The driver controls interrupts, and provides register
> + access for all of the device's peripherals. Some QTI PMIC chips
> + support communication over both I2C and SPMI.
> +
> config MFD_QCOM_RPM
> tristate "Qualcomm Resource Power Manager (RPM)"
> depends on ARCH_QCOM && OF
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 7761f84..26f0b80 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -199,6 +199,7 @@ obj-$(CONFIG_MFD_SI476X_CORE) += si476x-core.o
> obj-$(CONFIG_MFD_CS5535) += cs5535-mfd.o
> obj-$(CONFIG_MFD_OMAP_USB_HOST) += omap-usb-host.o omap-usb-tll.o
> obj-$(CONFIG_MFD_PM8XXX) += qcom-pm8xxx.o ssbi.o
> +obj-$(CONFIG_MFD_I2C_PMIC) += qcom-i2c-pmic.o
> obj-$(CONFIG_MFD_QCOM_RPM) += qcom_rpm.o
> obj-$(CONFIG_MFD_SPMI_PMIC) += qcom-spmi-pmic.o
> obj-$(CONFIG_TPS65911_COMPARATOR) += tps65911-comparator.o
> diff --git a/drivers/mfd/qcom-i2c-pmic.c b/drivers/mfd/qcom-i2c-pmic.c
> new file mode 100644
> index 0000000..d0f600a
> --- /dev/null
> +++ b/drivers/mfd/qcom-i2c-pmic.c
> @@ -0,0 +1,737 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
This is very out of date.
> + */
> +
> +#define pr_fmt(fmt) "I2C PMIC: %s: " fmt, __func__
Please don't role your own debug helpers.
The ones the kernel provides are suitably proficient.
> +#include <linux/bitops.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#define I2C_INTR_STATUS_BASE 0x0550
> +#define INT_RT_STS_OFFSET 0x10
> +#define INT_SET_TYPE_OFFSET 0x11
> +#define INT_POL_HIGH_OFFSET 0x12
> +#define INT_POL_LOW_OFFSET 0x13
> +#define INT_LATCHED_CLR_OFFSET 0x14
> +#define INT_EN_SET_OFFSET 0x15
> +#define INT_EN_CLR_OFFSET 0x16
> +#define INT_LATCHED_STS_OFFSET 0x18
> +#define INT_PENDING_STS_OFFSET 0x19
> +#define INT_MID_SEL_OFFSET 0x1A
> +#define INT_MID_SEL_MASK GENMASK(1, 0)
> +#define INT_PRIORITY_OFFSET 0x1B
> +#define INT_PRIORITY_BIT BIT(0)
> +
> +enum {
> + IRQ_SET_TYPE = 0,
> + IRQ_POL_HIGH,
> + IRQ_POL_LOW,
> + IRQ_LATCHED_CLR, /* not needed but makes life easy */
"Not"
It doesn't matter if the value is not used.
I think you can drop the comment.
> + IRQ_EN_SET,
> + IRQ_MAX_REGS,
> +};
Going to stop here for a second, as the vast majority of the remainder
of the driver appears to surround IRQ management.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
