Re: [PATCH v1 2/2] mfd: Introduce QTI I2C PMIC controller

[Guru Das Srinagesh]

On Wed, Apr 29, 2020 at 08:50:10AM +0100, Lee Jones wrote:
> 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?

There appear to be quite a few in-tree MFD drivers that register IRQ
controllers, like this driver does:

$ grep --exclude-dir=.git -rnE "irq_domain_(add|create).+\(" drivers/mfd | wc -l
23

As a further example, drivers/mfd/stpmic1.c closely resembles this
driver in that it uses both devm_regmap_add_irq_chip() as well as
devm_of_platform_populate().

As such, it seems like this driver is in line with some of the
architectural choices that have been accepted in already-merged drivers.
Could you please elaborate on your concerns?

> 
> >  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.

Will change to MFD_QCOM_I2C_PMIC.

> 
> > +	tristate "QTI I2C PMIC support"
> 
> Why aren't you using QCOM?
> 
> Actually, this should be expanded here anyway.

Will expand this to "Qualcomm Technologies, Inc. I2C PMIC support".

> 
> > +	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.

It is internal company policy to update the copyright year only for
those years when a change was made to the driver. Since this driver hasn't
been modified since 2018, the copyright year has also been static since
then. Otherwise, the driver is very much current functionality-wise.

What modification would you suggest for the copyright year?

> 
> > + */
> > +
> > +#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.

Sure. Would this be acceptable instead, with the custom string replaced by a
macro that the kernel provides?

	#define pr_fmt(fmt) "%s: %s: " fmt, KBUILD_MODNAME, __func__

> 
> > +#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.

Done.

> 
> > +	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.

(Please see my first comment above.)

Thank you.

Guru Das.