From: Lee Jones <lee.jones@linaro.org>
To: Guru Das Srinagesh <gurus@codeaurora.org>
Cc: devicetree@vger.kernel.org,
linux-arm-msm <linux-arm-msm@vger.kernel.org>,
Rob Herring <robh+dt@kernel.org>,
Subbaraman Narayanamurthy <subbaram@codeaurora.org>,
David Collins <collinsd@codeaurora.org>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 2/2] mfd: Introduce QTI I2C PMIC controller
Date: Wed, 29 Apr 2020 08:50:10 +0100 [thread overview]
Message-ID: <20200429075010.GX3559@dell> (raw)
In-Reply-To: <5644dea146f8b49a5b827c56392ff916bfb343e9.1588115326.git.gurus@codeaurora.org>
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@codeaurora.org>
> ---
> 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
next prev parent reply other threads:[~2020-04-29 7:50 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-29 0:30 [PATCH v1 0/2] Introduce QTI I2C PMIC controller Guru Das Srinagesh
2020-04-29 0:30 ` [PATCH v1 1/2] dt-bindings: mfd: Document " Guru Das Srinagesh
2020-04-29 7:51 ` Lee Jones
2020-04-29 21:01 ` Rob Herring
2020-04-29 0:30 ` [PATCH v1 2/2] mfd: Introduce " Guru Das Srinagesh
2020-04-29 7:50 ` Lee Jones [this message]
2020-05-01 1:13 ` Guru Das Srinagesh
2020-05-01 1:18 ` Joe Perches
2020-05-01 1:28 ` Guru Das Srinagesh
2020-05-15 10:45 ` Lee Jones
2020-05-19 18:57 ` Guru Das Srinagesh
2020-05-20 6:39 ` Lee Jones
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200429075010.GX3559@dell \
--to=lee.jones@linaro.org \
--cc=collinsd@codeaurora.org \
--cc=devicetree@vger.kernel.org \
--cc=gurus@codeaurora.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=subbaram@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).