Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 19 May 2020 11:33:55 +0100
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, tglx@linutronix.de,
        jason@lakedaemon.net, maz@kernel.org
Subject: Re: [PATCH 2/2] mfd: Introduce QTI I2C PMIC controller
Message-ID: <20200519103355.GU271301@dell>
References: <cover.1588037638.git.gurus@codeaurora.org>
 <041dd80e01d1bf86da2edc2747e922c473b50509.1588037638.git.gurus@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <041dd80e01d1bf86da2edc2747e922c473b50509.1588037638.git.gurus@codeaurora.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

[Adding IRQ chaps]

Seeing as a vast portion of this driver is IRQ domain related, I see
good reason to separate it out into a driver in its own right.  At the
very least it will require an IRQ *-by tag.

On Mon, 27 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 ++++++++++++++++++++++++++++++++++++++++++++
>  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

"I2C PMIC" is too generic.

> +	tristate "QTI I2C PMIC support"

I don't see QTI used very often.

What's the difference between "QTI" and "QCOM"?

> +	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 out of date.

No author tag?

> + */
> +
> +#define pr_fmt(fmt) "I2C PMIC: %s: " fmt, __func__

Outside of in-development code I don't see a reason for adding the
function name to system log entries.  IMHO, it just dirties the log on
production/released H/W.  Please considering removing this
altogether in from plain device drivers.

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

I'm going to skip all of the IRQ code until we know what we're doing
with it.

 #########
<-- IRQ -->
 #########

> +#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 */
> +	IRQ_EN_SET,
> +	IRQ_MAX_REGS,
> +};
> +
> +struct i2c_pmic_periph {
> +	void		*data;
> +	u16		addr;
> +	u8		cached[IRQ_MAX_REGS];
> +	u8		synced[IRQ_MAX_REGS];
> +	u8		wake;
> +	struct mutex	lock;
> +};
> +
> +struct i2c_pmic {
> +	struct device		*dev;
> +	struct regmap		*regmap;
> +	struct irq_domain	*domain;
> +	struct i2c_pmic_periph	*periph;
> +	struct pinctrl		*pinctrl;
> +	struct mutex		irq_complete;
> +	const char		*pinctrl_name;
> +	int			num_periphs;
> +	int			summary_irq;
> +	bool			resume_completed;
> +	bool			irq_waiting;
> +};
> +
> +static void i2c_pmic_irq_bus_lock(struct irq_data *d)
> +{
> +	struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
> +
> +	mutex_lock(&periph->lock);
> +}
> +
> +static void i2c_pmic_sync_type_polarity(struct i2c_pmic *chip,
> +			       struct i2c_pmic_periph *periph)
> +{
> +	int rc;
> +
> +	/* did any irq type change? */
> +	if (periph->cached[IRQ_SET_TYPE] ^ periph->synced[IRQ_SET_TYPE]) {
> +		rc = regmap_write(chip->regmap,
> +				  periph->addr | INT_SET_TYPE_OFFSET,
> +				  periph->cached[IRQ_SET_TYPE]);
> +		if (rc < 0) {
> +			pr_err("Couldn't set periph 0x%04x irqs 0x%02x type rc=%d\n",
> +				periph->addr, periph->cached[IRQ_SET_TYPE], rc);
> +			return;
> +		}
> +
> +		periph->synced[IRQ_SET_TYPE] = periph->cached[IRQ_SET_TYPE];
> +	}
> +
> +	/* did any polarity high change? */
> +	if (periph->cached[IRQ_POL_HIGH] ^ periph->synced[IRQ_POL_HIGH]) {
> +		rc = regmap_write(chip->regmap,
> +				  periph->addr | INT_POL_HIGH_OFFSET,
> +				  periph->cached[IRQ_POL_HIGH]);
> +		if (rc < 0) {
> +			pr_err("Couldn't set periph 0x%04x irqs 0x%02x polarity high rc=%d\n",
> +				periph->addr, periph->cached[IRQ_POL_HIGH], rc);
> +			return;
> +		}
> +
> +		periph->synced[IRQ_POL_HIGH] = periph->cached[IRQ_POL_HIGH];
> +	}
> +
> +	/* did any polarity low change? */
> +	if (periph->cached[IRQ_POL_LOW] ^ periph->synced[IRQ_POL_LOW]) {
> +		rc = regmap_write(chip->regmap,
> +				  periph->addr | INT_POL_LOW_OFFSET,
> +				  periph->cached[IRQ_POL_LOW]);
> +		if (rc < 0) {
> +			pr_err("Couldn't set periph 0x%04x irqs 0x%02x polarity low rc=%d\n",
> +				periph->addr, periph->cached[IRQ_POL_LOW], rc);
> +			return;
> +		}
> +
> +		periph->synced[IRQ_POL_LOW] = periph->cached[IRQ_POL_LOW];
> +	}
> +}
> +
> +static void i2c_pmic_sync_enable(struct i2c_pmic *chip,
> +				 struct i2c_pmic_periph *periph)
> +{
> +	u8 en_set, en_clr;
> +	int rc;
> +
> +	/* determine which irqs were enabled and which were disabled */
> +	en_clr = periph->synced[IRQ_EN_SET] & ~periph->cached[IRQ_EN_SET];
> +	en_set = ~periph->synced[IRQ_EN_SET] & periph->cached[IRQ_EN_SET];
> +
> +	/* were any irqs disabled? */
> +	if (en_clr) {
> +		rc = regmap_write(chip->regmap,
> +				  periph->addr | INT_EN_CLR_OFFSET, en_clr);
> +		if (rc < 0) {
> +			pr_err("Couldn't disable periph 0x%04x irqs 0x%02x rc=%d\n",
> +				periph->addr, en_clr, rc);
> +			return;
> +		}
> +	}
> +
> +	/* were any irqs enabled? */
> +	if (en_set) {
> +		rc = regmap_write(chip->regmap,
> +				  periph->addr | INT_EN_SET_OFFSET, en_set);
> +		if (rc < 0) {
> +			pr_err("Couldn't enable periph 0x%04x irqs 0x%02x rc=%d\n",
> +				periph->addr, en_set, rc);
> +			return;
> +		}
> +	}
> +
> +	/* irq enabled status was written to hardware */
> +	periph->synced[IRQ_EN_SET] = periph->cached[IRQ_EN_SET];
> +}
> +
> +static void i2c_pmic_irq_bus_sync_unlock(struct irq_data *d)
> +{
> +	struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
> +	struct i2c_pmic *chip = periph->data;
> +
> +	i2c_pmic_sync_type_polarity(chip, periph);
> +	i2c_pmic_sync_enable(chip, periph);
> +	mutex_unlock(&periph->lock);
> +}
> +
> +static void i2c_pmic_irq_disable(struct irq_data *d)
> +{
> +	struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
> +
> +	periph->cached[IRQ_EN_SET] &= ~d->hwirq & 0xFF;
> +}
> +
> +static void i2c_pmic_irq_enable(struct irq_data *d)
> +{
> +	struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
> +
> +	periph->cached[IRQ_EN_SET] |= d->hwirq & 0xFF;
> +}
> +
> +static int i2c_pmic_irq_set_type(struct irq_data *d, unsigned int irq_type)
> +{
> +	struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
> +
> +	switch (irq_type) {
> +	case IRQ_TYPE_EDGE_RISING:
> +		periph->cached[IRQ_SET_TYPE] |= d->hwirq & 0xFF;
> +		periph->cached[IRQ_POL_HIGH] |= d->hwirq & 0xFF;
> +		periph->cached[IRQ_POL_LOW] &= ~d->hwirq & 0xFF;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		periph->cached[IRQ_SET_TYPE] |= d->hwirq & 0xFF;
> +		periph->cached[IRQ_POL_HIGH] &= ~d->hwirq & 0xFF;
> +		periph->cached[IRQ_POL_LOW] |= d->hwirq & 0xFF;
> +		break;
> +	case IRQ_TYPE_EDGE_BOTH:
> +		periph->cached[IRQ_SET_TYPE] |= d->hwirq & 0xFF;
> +		periph->cached[IRQ_POL_HIGH] |= d->hwirq & 0xFF;
> +		periph->cached[IRQ_POL_LOW] |= d->hwirq & 0xFF;
> +		break;
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		periph->cached[IRQ_SET_TYPE] &= ~d->hwirq & 0xFF;
> +		periph->cached[IRQ_POL_HIGH] |= d->hwirq & 0xFF;
> +		periph->cached[IRQ_POL_LOW] &= ~d->hwirq & 0xFF;
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		periph->cached[IRQ_SET_TYPE] &= ~d->hwirq & 0xFF;
> +		periph->cached[IRQ_POL_HIGH] &= ~d->hwirq & 0xFF;
> +		periph->cached[IRQ_POL_LOW] |= d->hwirq & 0xFF;
> +		break;
> +	default:
> +		pr_err("irq type 0x%04x is not supported\n", irq_type);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int i2c_pmic_irq_set_wake(struct irq_data *d, unsigned int on)
> +{
> +	struct i2c_pmic_periph *periph = irq_data_get_irq_chip_data(d);
> +
> +	if (on)
> +		periph->wake |= d->hwirq & 0xFF;
> +	else
> +		periph->wake &= ~d->hwirq & 0xFF;
> +
> +	return 0;
> +}
> +#else
> +#define i2c_pmic_irq_set_wake NULL
> +#endif
> +
> +static struct irq_chip i2c_pmic_irq_chip = {
> +	.name			= "i2c_pmic_irq_chip",
> +	.irq_bus_lock		= i2c_pmic_irq_bus_lock,
> +	.irq_bus_sync_unlock	= i2c_pmic_irq_bus_sync_unlock,
> +	.irq_disable		= i2c_pmic_irq_disable,
> +	.irq_enable		= i2c_pmic_irq_enable,
> +	.irq_set_type		= i2c_pmic_irq_set_type,
> +	.irq_set_wake		= i2c_pmic_irq_set_wake,
> +};
> +
> +static struct i2c_pmic_periph *i2c_pmic_find_periph(struct i2c_pmic *chip,
> +						    irq_hw_number_t hwirq)
> +{
> +	int i;
> +
> +	for (i = 0; i < chip->num_periphs; i++)
> +		if (chip->periph[i].addr == (hwirq & 0xFF00))
> +			return &chip->periph[i];
> +
> +	pr_err_ratelimited("Couldn't find periph struct for hwirq 0x%04lx\n",
> +			   hwirq);
> +	return NULL;
> +}
> +
> +static int i2c_pmic_domain_map(struct irq_domain *d, unsigned int virq,
> +			irq_hw_number_t hwirq)
> +{
> +	struct i2c_pmic *chip = d->host_data;
> +	struct i2c_pmic_periph *periph = i2c_pmic_find_periph(chip, hwirq);
> +
> +	if (!periph)
> +		return -ENODEV;
> +
> +	irq_set_chip_data(virq, periph);
> +	irq_set_chip_and_handler(virq, &i2c_pmic_irq_chip, handle_level_irq);
> +	irq_set_nested_thread(virq, 1);
> +	irq_set_noprobe(virq);
> +	return 0;
> +}
> +
> +static int i2c_pmic_domain_xlate(struct irq_domain *d,
> +				 struct device_node *ctrlr, const u32 *intspec,
> +				 unsigned int intsize, unsigned long *out_hwirq,
> +				 unsigned int *out_type)
> +{
> +	if (intsize != 3)
> +		return -EINVAL;
> +
> +	if (intspec[0] > 0xFF || intspec[1] > 0x7 ||
> +					intspec[2] > IRQ_TYPE_SENSE_MASK)
> +		return -EINVAL;
> +
> +	/*
> +	 * Interrupt specifiers are triplets
> +	 * <peripheral-address, irq-number, IRQ_TYPE_*>
> +	 *
> +	 * peripheral-address - The base address of the peripheral
> +	 * irq-number	      - The zero based bit position of the peripheral's
> +	 *			interrupt registers corresponding to the irq
> +	 *			where the LSB is 0 and the MSB is 7
> +	 * IRQ_TYPE_*	      - Please refer to linux/irq.h
> +	 */
> +	*out_hwirq = intspec[0] << 8 | BIT(intspec[1]);
> +	*out_type = intspec[2] & IRQ_TYPE_SENSE_MASK;
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops i2c_pmic_domain_ops = {
> +	.map	= i2c_pmic_domain_map,
> +	.xlate	= i2c_pmic_domain_xlate,
> +};
> +
> +static void i2c_pmic_irq_ack_now(struct i2c_pmic *chip, u16 hwirq)
> +{
> +	int rc;
> +
> +	rc = regmap_write(chip->regmap,
> +			  (hwirq & 0xFF00) | INT_LATCHED_CLR_OFFSET,
> +			  hwirq & 0xFF);
> +	if (rc < 0)
> +		pr_err_ratelimited("Couldn't ack 0x%04x rc=%d\n", hwirq, rc);
> +}
> +
> +static void i2c_pmic_irq_disable_now(struct i2c_pmic *chip, u16 hwirq)
> +{
> +	struct i2c_pmic_periph *periph = i2c_pmic_find_periph(chip, hwirq);
> +	int rc;
> +
> +	if (!periph)
> +		return;
> +
> +	mutex_lock(&periph->lock);
> +	periph->cached[IRQ_EN_SET] &= ~hwirq & 0xFF;
> +
> +	rc = regmap_write(chip->regmap,
> +			  (hwirq & 0xFF00) | INT_EN_CLR_OFFSET,
> +			  hwirq & 0xFF);
> +	if (rc < 0) {
> +		pr_err_ratelimited("Couldn't disable irq 0x%04x rc=%d\n",
> +				   hwirq, rc);
> +		goto unlock;
> +	}
> +
> +	periph->synced[IRQ_EN_SET] = periph->cached[IRQ_EN_SET];
> +
> +unlock:
> +	mutex_unlock(&periph->lock);
> +}
> +
> +static void i2c_pmic_periph_status_handler(struct i2c_pmic *chip,
> +					   u16 periph_address, u8 periph_status)
> +{
> +	unsigned int hwirq, virq;
> +	int i;
> +
> +	while (periph_status) {
> +		i = ffs(periph_status) - 1;
> +		periph_status &= ~BIT(i);
> +		hwirq = periph_address | BIT(i);
> +		virq = irq_find_mapping(chip->domain, hwirq);
> +		if (virq == 0) {
> +			pr_err_ratelimited("Couldn't find mapping; disabling 0x%04x\n",
> +					   hwirq);
> +			i2c_pmic_irq_disable_now(chip, hwirq);
> +			continue;
> +		}
> +
> +		handle_nested_irq(virq);
> +		i2c_pmic_irq_ack_now(chip, hwirq);
> +	}
> +}
> +
> +static void i2c_pmic_summary_status_handler(struct i2c_pmic *chip,
> +					    struct i2c_pmic_periph *periph,
> +					    u8 summary_status)
> +{
> +	unsigned int periph_status;
> +	int rc, i;
> +
> +	while (summary_status) {
> +		i = ffs(summary_status) - 1;
> +		summary_status &= ~BIT(i);
> +
> +		rc = regmap_read(chip->regmap,
> +				 periph[i].addr | INT_LATCHED_STS_OFFSET,
> +				 &periph_status);
> +		if (rc < 0) {
> +			pr_err_ratelimited("Couldn't read 0x%04x | INT_LATCHED_STS rc=%d\n",
> +					   periph[i].addr, rc);
> +			continue;
> +		}
> +
> +		i2c_pmic_periph_status_handler(chip, periph[i].addr,
> +					       periph_status);
> +	}
> +}
> +
> +static irqreturn_t i2c_pmic_irq_handler(int irq, void *dev_id)
> +{
> +	struct i2c_pmic *chip = dev_id;
> +	struct i2c_pmic_periph *periph;
> +	unsigned int summary_status;
> +	int rc, i;
> +
> +	mutex_lock(&chip->irq_complete);
> +	chip->irq_waiting = true;
> +	if (!chip->resume_completed) {
> +		pr_debug("IRQ triggered before device-resume\n");
> +		disable_irq_nosync(irq);
> +		mutex_unlock(&chip->irq_complete);
> +		return IRQ_HANDLED;
> +	}
> +	chip->irq_waiting = false;
> +
> +	for (i = 0; i < DIV_ROUND_UP(chip->num_periphs, BITS_PER_BYTE); i++) {
> +		rc = regmap_read(chip->regmap, I2C_INTR_STATUS_BASE + i,
> +				&summary_status);
> +		if (rc < 0) {
> +			pr_err_ratelimited("Couldn't read I2C_INTR_STATUS%d rc=%d\n",
> +					   i, rc);
> +			continue;
> +		}
> +
> +		if (summary_status == 0)
> +			continue;
> +
> +		periph = &chip->periph[i * 8];
> +		i2c_pmic_summary_status_handler(chip, periph, summary_status);
> +	}
> +
> +	mutex_unlock(&chip->irq_complete);
> +
> +	return IRQ_HANDLED;
> +}

 #########
<-- IRQ -->
 #########

> +static int i2c_pmic_parse_dt(struct i2c_pmic *chip)
> +{
> +	struct device_node *node = chip->dev->of_node;

s/node/np/

> +	int rc, i;
> +	u32 temp;

Temp is seldom good nomenclature.

"periph_addr"

> +	if (!node) {
> +		pr_err("missing device tree\n");

Why are you using pr_err() over dev_err()?

Please use proper grammar (less full-stops) in prints and comments.

The above goes for all - I won't mention it again.

> +		return -EINVAL;
> +	}

Can this happen.

> +	chip->num_periphs = of_property_count_u32_elems(node,
> +							"qcom,periph-map");

Prefer the break after the '='.

If you change 's/node/np' you don't need to break at all.

> +	if (chip->num_periphs < 0) {
> +		pr_err("missing qcom,periph-map property rc=%d\n",

Prefer it if you use plain English for user facing comments.

  "Peripheral map not defined"

> +			chip->num_periphs);
> +		return chip->num_periphs;
> +	}
> +
> +	if (chip->num_periphs == 0) {
> +		pr_err("qcom,periph-map must contain at least one address\n");

"Peripheral map is empty"

> +		return -EINVAL;
> +	}
> +
> +	chip->periph = devm_kcalloc(chip->dev, chip->num_periphs,
> +				     sizeof(*chip->periph), GFP_KERNEL);
> +	if (!chip->periph)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < chip->num_periphs; i++) {
> +		rc = of_property_read_u32_index(node, "qcom,periph-map",
> +						i, &temp);
> +		if (rc < 0) {
> +			pr_err("Couldn't read qcom,periph-map[%d]
> rc=%d\n",

"Peripheral map entry %d is invalid"

> +			       i, rc);
> +			return rc;
> +		}
> +
> +		chip->periph[i].addr = (u16)(temp << 8);

Worth a comment I think.

> +		chip->periph[i].data = chip;

'data' is also not a great variable name, if you can avoid it.

> +		mutex_init(&chip->periph[i].lock);
> +	}
> +
> +	of_property_read_string(node, "pinctrl-names", &chip->pinctrl_name);
> +
> +	return rc;

Why return rc here?

> +}
> +
> +#define MAX_I2C_RETRIES	3

Why 3?

> +static int i2c_pmic_read(struct regmap *map, unsigned int reg, void *val,
> +			size_t val_count)
> +{
> +	int rc, retries = 0;
> +
> +	do {
> +		rc = regmap_bulk_read(map, reg, val, val_count);
> +	} while (rc == -ENOTCONN && retries++ < MAX_I2C_RETRIES);
> +
> +	if (retries > 1)
> +		pr_err("i2c_pmic_read failed for %d retries, rc = %d\n",
> +			retries - 1, rc);

Is this always an error?

Why would the user care if it failed once, then succeeded?

> +	return rc;
> +}
> +
> +static int i2c_pmic_determine_initial_status(struct i2c_pmic *chip)
> +{
> +	int rc, i;
> +
> +	for (i = 0; i < chip->num_periphs; i++) {
> +		rc = i2c_pmic_read(chip->regmap,
> +				chip->periph[i].addr | INT_SET_TYPE_OFFSET,
> +				chip->periph[i].cached, IRQ_MAX_REGS);
> +		if (rc < 0) {
> +			pr_err("Couldn't read irq data rc=%d\n", rc);

"IRQ"

> +			return rc;
> +		}
> +
> +		memcpy(chip->periph[i].synced, chip->periph[i].cached,
> +		       IRQ_MAX_REGS * sizeof(*chip->periph[i].synced));
> +	}
> +
> +	return 0;
> +}
> +
> +static struct regmap_config i2c_pmic_regmap_config = {
> +	.reg_bits	= 16,
> +	.val_bits	= 8,
> +	.max_register	= 0xFFFF,
> +};
> +
> +static int i2c_pmic_probe(struct i2c_client *client,
> +			  const struct i2c_device_id *id)
> +{
> +	struct i2c_pmic *chip;

We usually call this ddata.  Especially relevant as you are storing it
in *driver_data for reclaim in the functions above.

> +	int rc = 0;

'ret' is by far the more common name for this.

$ git grep "int ret[;\|,]" | wc -l
39365
$ git grep "int rc[;\|,]" | wc -l
6761

Let's keep things as consistent as possible please.

> +	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	chip->dev = &client->dev;
> +	chip->regmap = devm_regmap_init_i2c(client, &i2c_pmic_regmap_config);
> +	if (!chip->regmap)
> +		return -ENODEV;
> +
> +	i2c_set_clientdata(client, chip);

'\n'

> +	if (!of_property_read_bool(chip->dev->of_node, "interrupt-controller"))
> +		goto probe_children;
> +
> +	chip->domain = irq_domain_add_tree(client->dev.of_node,
> +					   &i2c_pmic_domain_ops, chip);
> +	if (!chip->domain) {
> +		rc = -ENOMEM;
> +		goto cleanup;
> +	}
> +
> +	rc = i2c_pmic_parse_dt(chip);
> +	if (rc < 0) {

If you return an error or 0 here, you can change this to:

if (rc)

> +		pr_err("Couldn't parse device tree rc=%d\n", rc);

Looks like you already have prints in i2c_pmic_parse_dt().

No need for an additional one.

> +		goto cleanup;
> +	}
> +
> +	rc = i2c_pmic_determine_initial_status(chip);
> +	if (rc < 0) {
> +		pr_err("Couldn't determine initial status rc=%d\n", rc);

As above.

> +		goto cleanup;
> +	}
> +
> +	if (chip->pinctrl_name) {
> +		chip->pinctrl = devm_pinctrl_get_select(chip->dev,
> +							chip->pinctrl_name);
> +		if (IS_ERR(chip->pinctrl)) {
> +			pr_err("Couldn't select %s pinctrl rc=%ld\n",
> +				chip->pinctrl_name, PTR_ERR(chip->pinctrl));
> +			rc = PTR_ERR(chip->pinctrl);

Do this before the print, then you only need to do it once.

> +			goto cleanup;
> +		}
> +	}
> +
> +	chip->resume_completed = true;

Why can't you disable interrupts before suspending?

> +	mutex_init(&chip->irq_complete);
> +
> +	rc = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +				       i2c_pmic_irq_handler,
> +				       IRQF_ONESHOT | IRQF_SHARED,
> +				       "i2c_pmic_stat_irq", chip);
> +	if (rc < 0) {
> +		pr_err("Couldn't request irq %d rc=%d\n", client->irq, rc);

"IRQ"

> +		goto cleanup;
> +	}
> +
> +	chip->summary_irq = client->irq;
> +	enable_irq_wake(client->irq);
> +
> +probe_children:
> +	of_platform_populate(chip->dev->of_node, NULL, NULL, chip->dev);

devm_*?

> +	pr_info("I2C PMIC probe successful\n");

This is superfluous, please remove it.

'\n' here.

> +	return rc;
> +
> +cleanup:
> +	if (chip->domain)
> +		irq_domain_remove(chip->domain);
> +	i2c_set_clientdata(client, NULL);

'\n' here.

> +	return rc;
> +}
> +
> +static int i2c_pmic_remove(struct i2c_client *client)
> +{
> +	struct i2c_pmic *chip = i2c_get_clientdata(client);
> +
> +	of_platform_depopulate(chip->dev);

If you use devm_* this becomes superfluous.

> +	if (chip->domain)
> +		irq_domain_remove(chip->domain);

'\n' here.

> +	i2c_set_clientdata(client, NULL);

'\n' here.

> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int i2c_pmic_suspend_noirq(struct device *dev)
> +{
> +	struct i2c_pmic *chip = dev_get_drvdata(dev);
> +
> +	if (chip->irq_waiting) {
> +		pr_err_ratelimited("Aborting suspend, an interrupt was detected while suspending\n");
> +		return -EBUSY;

I haven't seen this before.  Why does this platform require it?

What happens when you return early from .suspend_noirq?

Does this system just resume?

> +	}

'\n' here.

> +	return 0;
> +}
> +
> +static int i2c_pmic_suspend(struct device *dev)
> +{
> +	struct i2c_pmic *chip = dev_get_drvdata(dev);
> +	struct i2c_pmic_periph *periph;
> +	int rc = 0, i;
> +
> +	for (i = 0; i < chip->num_periphs; i++) {
> +		periph = &chip->periph[i];
> +
> +		rc = regmap_write(chip->regmap,
> +				  periph->addr | INT_EN_CLR_OFFSET, 0xFF);
> +		if (rc < 0) {
> +			pr_err_ratelimited("Couldn't clear 0x%04x irqs rc=%d\n",

"IRQs"

Same goes for all the other mentions of it.

> +				periph->addr, rc);
> +			continue;
> +		}
> +
> +		rc = regmap_write(chip->regmap,
> +				  periph->addr | INT_EN_SET_OFFSET,
> +				  periph->wake);
> +		if (rc < 0)
> +			pr_err_ratelimited("Couldn't enable 0x%04x wake irqs 0x%02x rc=%d\n",
> +			       periph->addr, periph->wake, rc);
> +	}

'\n' here.

> +	if (!rc) {
> +		mutex_lock(&chip->irq_complete);
> +		chip->resume_completed = false;
> +		mutex_unlock(&chip->irq_complete);
> +	}
> +
> +	return rc;
> +}
> +
> +static int i2c_pmic_resume(struct device *dev)
> +{
> +	struct i2c_pmic *chip = dev_get_drvdata(dev);
> +	struct i2c_pmic_periph *periph;
> +	int rc = 0, i;
> +
> +	for (i = 0; i < chip->num_periphs; i++) {
> +		periph = &chip->periph[i];
> +
> +		rc = regmap_write(chip->regmap,
> +				  periph->addr | INT_EN_CLR_OFFSET, 0xFF);
> +		if (rc < 0) {
> +			pr_err("Couldn't clear 0x%04x irqs rc=%d\n",
> +				periph->addr, rc);
> +			continue;
> +		}
> +
> +		rc = regmap_write(chip->regmap,
> +				  periph->addr | INT_EN_SET_OFFSET,
> +				  periph->synced[IRQ_EN_SET]);
> +		if (rc < 0)
> +			pr_err("Couldn't restore 0x%04x synced irqs 0x%02x rc=%d\n",
> +			       periph->addr, periph->synced[IRQ_EN_SET], rc);
> +	}
> +
> +	mutex_lock(&chip->irq_complete);
> +	chip->resume_completed = true;

'\n' here.

> +	if (chip->irq_waiting) {
> +		mutex_unlock(&chip->irq_complete);

Move this above the if(), then you can remove it from the else too.

> +		/* irq was pending, call the handler */
> +		i2c_pmic_irq_handler(chip->summary_irq, chip);
> +		enable_irq(chip->summary_irq);
> +	} else {
> +		mutex_unlock(&chip->irq_complete);
> +	}
> +
> +	return rc;
> +}
> +#else
> +static int i2c_pmic_suspend(struct device *dev)
> +{
> +	return 0;
> +}
> +static int i2c_pmic_resume(struct device *dev)
> +{
> +	return 0;
> +}
> +static int i2c_pmic_suspend_noirq(struct device *dev)
> +{
> +	return 0
> +}
> +#endif
> +static const struct dev_pm_ops i2c_pmic_pm_ops = {
> +	.suspend	= i2c_pmic_suspend,
> +	.suspend_noirq	= i2c_pmic_suspend_noirq,
> +	.resume		= i2c_pmic_resume,
> +};

See how drivers/mfd/arizona-core.c removes all of this boilerplate.

> +static const struct of_device_id i2c_pmic_match_table[] = {
> +	{ .compatible = "qcom,i2c-pmic", },
> +	{ },
> +};
> +static const struct i2c_device_id i2c_pmic_id[] = {
> +	{ "i2c-pmic", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, i2c_pmic_id);

If you use probe_new, you can remove this table altogether.

> +static struct i2c_driver i2c_pmic_driver = {
> +	.driver		= {
> +		.name		= "i2c_pmic",
> +		.pm		= &i2c_pmic_pm_ops,
> +		.of_match_table	= i2c_pmic_match_table,
> +	},
> +	.probe		= i2c_pmic_probe,
> +	.remove		= i2c_pmic_remove,
> +	.id_table	= i2c_pmic_id,
> +};
> +

Remove this line.

> +module_i2c_driver(i2c_pmic_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("i2c:i2c_pmic");

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