[PATCH 1/2] mfd: pm8921: add support to pm8821

[PATCH 1/2] mfd: pm8921: add support to pm8821

[Srinivas Kandagatla]

This patch adds support to PM8821 PMIC and interrupt support.
PM8821 is companion device that supplements primary PMIC PM8921 IC.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
Tested this patch for MPP and IRQ functionality on IFC6410 and SD600 EVAL
board with mpps PM8821 and PM8921.

 .../devicetree/bindings/mfd/qcom-pm8xxx.txt        |   1 +
 drivers/mfd/pm8921-core.c                          | 368 +++++++++++++++++++--
 2 files changed, 340 insertions(+), 29 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt b/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
index 37a088f..8f1b4ec 100644
--- a/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
+++ b/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
@@ -11,6 +11,7 @@ voltages and other various functionality to Qualcomm SoCs.
 	Definition: must be one of:
 		    "qcom,pm8058"
 		    "qcom,pm8921"
+		    "qcom,pm8821"
 
 - #address-cells:
 	Usage: required
diff --git a/drivers/mfd/pm8921-core.c b/drivers/mfd/pm8921-core.c
index 0e3a2ea..28c2470 100644
--- a/drivers/mfd/pm8921-core.c
+++ b/drivers/mfd/pm8921-core.c
@@ -28,16 +28,26 @@
 #include <linux/mfd/core.h>
 
 #define	SSBI_REG_ADDR_IRQ_BASE		0x1BB
-
-#define	SSBI_REG_ADDR_IRQ_ROOT		(SSBI_REG_ADDR_IRQ_BASE + 0)
-#define	SSBI_REG_ADDR_IRQ_M_STATUS1	(SSBI_REG_ADDR_IRQ_BASE + 1)
-#define	SSBI_REG_ADDR_IRQ_M_STATUS2	(SSBI_REG_ADDR_IRQ_BASE + 2)
-#define	SSBI_REG_ADDR_IRQ_M_STATUS3	(SSBI_REG_ADDR_IRQ_BASE + 3)
-#define	SSBI_REG_ADDR_IRQ_M_STATUS4	(SSBI_REG_ADDR_IRQ_BASE + 4)
-#define	SSBI_REG_ADDR_IRQ_BLK_SEL	(SSBI_REG_ADDR_IRQ_BASE + 5)
-#define	SSBI_REG_ADDR_IRQ_IT_STATUS	(SSBI_REG_ADDR_IRQ_BASE + 6)
-#define	SSBI_REG_ADDR_IRQ_CONFIG	(SSBI_REG_ADDR_IRQ_BASE + 7)
-#define	SSBI_REG_ADDR_IRQ_RT_STATUS	(SSBI_REG_ADDR_IRQ_BASE + 8)
+#define	SSBI_PM8821_REG_ADDR_IRQ_BASE	0x100
+
+#define	SSBI_REG_ADDR_IRQ_ROOT		(0)
+#define	SSBI_REG_ADDR_IRQ_M_STATUS1	(1)
+#define	SSBI_REG_ADDR_IRQ_M_STATUS2	(2)
+#define	SSBI_REG_ADDR_IRQ_M_STATUS3	(3)
+#define	SSBI_REG_ADDR_IRQ_M_STATUS4	(4)
+#define	SSBI_REG_ADDR_IRQ_BLK_SEL	(5)
+#define	SSBI_REG_ADDR_IRQ_IT_STATUS	(6)
+#define	SSBI_REG_ADDR_IRQ_CONFIG	(7)
+#define	SSBI_REG_ADDR_IRQ_RT_STATUS	(8)
+
+#define	PM8821_TOTAL_IRQ_MASTERS	2
+#define	PM8821_BLOCKS_PER_MASTER	7
+#define	PM8821_IRQ_MASTER1_SET		0x01
+#define	PM8821_IRQ_CLEAR_OFFSET		0x01
+#define	PM8821_IRQ_RT_STATUS_OFFSET	0x0f
+#define	PM8821_IRQ_MASK_REG_OFFSET	0x08
+#define	SSBI_REG_ADDR_IRQ_MASTER0	0x30
+#define	SSBI_REG_ADDR_IRQ_MASTER1	0xb0
 
 #define	PM_IRQF_LVL_SEL			0x01	/* level select */
 #define	PM_IRQF_MASK_FE			0x02	/* mask falling edge */
@@ -54,30 +64,41 @@
 #define REG_HWREV_2		0x0E8  /* PMIC4 revision 2 */
 
 #define PM8921_NR_IRQS		256
+#define PM8821_NR_IRQS		112
 
 struct pm_irq_chip {
 	struct regmap		*regmap;
 	spinlock_t		pm_irq_lock;
 	struct irq_domain	*irqdomain;
+	unsigned int		irq_reg_base;
 	unsigned int		num_irqs;
 	unsigned int		num_blocks;
 	unsigned int		num_masters;
 	u8			config[0];
 };
 
+struct pm8xxx_data {
+	int num_irqs;
+	unsigned int		irq_reg_base;
+	const struct irq_domain_ops  *irq_domain_ops;
+	void (*irq_handler)(struct irq_desc *desc);
+};
+
 static int pm8xxx_read_block_irq(struct pm_irq_chip *chip, unsigned int bp,
 				 unsigned int *ip)
 {
 	int	rc;
 
 	spin_lock(&chip->pm_irq_lock);
-	rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, bp);
+	rc = regmap_write(chip->regmap,
+			  chip->irq_reg_base + SSBI_REG_ADDR_IRQ_BLK_SEL, bp);
 	if (rc) {
 		pr_err("Failed Selecting Block %d rc=%d\n", bp, rc);
 		goto bail;
 	}
 
-	rc = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_IT_STATUS, ip);
+	rc = regmap_read(chip->regmap,
+			 chip->irq_reg_base + SSBI_REG_ADDR_IRQ_IT_STATUS, ip);
 	if (rc)
 		pr_err("Failed Reading Status rc=%d\n", rc);
 bail:
@@ -91,14 +112,16 @@ pm8xxx_config_irq(struct pm_irq_chip *chip, unsigned int bp, unsigned int cp)
 	int	rc;
 
 	spin_lock(&chip->pm_irq_lock);
-	rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, bp);
+	rc = regmap_write(chip->regmap,
+			  chip->irq_reg_base + SSBI_REG_ADDR_IRQ_BLK_SEL, bp);
 	if (rc) {
 		pr_err("Failed Selecting Block %d rc=%d\n", bp, rc);
 		goto bail;
 	}
 
 	cp |= PM_IRQF_WRITE;
-	rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_CONFIG, cp);
+	rc = regmap_write(chip->regmap,
+			  chip->irq_reg_base + SSBI_REG_ADDR_IRQ_CONFIG, cp);
 	if (rc)
 		pr_err("Failed Configuring IRQ rc=%d\n", rc);
 bail:
@@ -137,8 +160,8 @@ static int pm8xxx_irq_master_handler(struct pm_irq_chip *chip, int master)
 	unsigned int blockbits;
 	int block_number, i, ret = 0;
 
-	ret = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_M_STATUS1 + master,
-			  &blockbits);
+	ret = regmap_read(chip->regmap, chip->irq_reg_base +
+			  SSBI_REG_ADDR_IRQ_M_STATUS1 + master, &blockbits);
 	if (ret) {
 		pr_err("Failed to read master %d ret=%d\n", master, ret);
 		return ret;
@@ -165,7 +188,8 @@ static void pm8xxx_irq_handler(struct irq_desc *desc)
 
 	chained_irq_enter(irq_chip, desc);
 
-	ret = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_ROOT, &root);
+	ret = regmap_read(chip->regmap,
+			  chip->irq_reg_base + SSBI_REG_ADDR_IRQ_ROOT, &root);
 	if (ret) {
 		pr_err("Can't read root status ret=%d\n", ret);
 		return;
@@ -182,6 +206,122 @@ static void pm8xxx_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(irq_chip, desc);
 }
 
+static int pm8821_read_master_irq(const struct pm_irq_chip *chip,
+				  int m, unsigned int *master)
+{
+	unsigned int base;
+
+	if (!m)
+		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
+	else
+		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
+
+	return regmap_read(chip->regmap, base, master);
+}
+
+static int pm8821_read_block_irq(struct pm_irq_chip *chip, int master,
+				 u8 block, unsigned int *bits)
+{
+	int rc;
+
+	unsigned int base;
+
+	if (!master)
+		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
+	else
+		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
+
+	spin_lock(&chip->pm_irq_lock);
+
+	rc = regmap_read(chip->regmap, base + block, bits);
+	if (rc)
+		pr_err("Failed Reading Status rc=%d\n", rc);
+
+	spin_unlock(&chip->pm_irq_lock);
+	return rc;
+}
+
+static int pm8821_irq_block_handler(struct pm_irq_chip *chip,
+				    int master_number, int block)
+{
+	int pmirq, irq, i, ret;
+	unsigned int bits;
+
+	ret = pm8821_read_block_irq(chip, master_number, block, &bits);
+	if (ret) {
+		pr_err("Failed reading %d block ret=%d", block, ret);
+		return ret;
+	}
+	if (!bits) {
+		pr_err("block bit set in master but no irqs: %d", block);
+		return 0;
+	}
+
+	/* Convert block offset to global block number */
+	block += (master_number * PM8821_BLOCKS_PER_MASTER) - 1;
+
+	/* Check IRQ bits */
+	for (i = 0; i < 8; i++) {
+		if (bits & BIT(i)) {
+			pmirq = block * 8 + i;
+			irq = irq_find_mapping(chip->irqdomain, pmirq);
+			generic_handle_irq(irq);
+		}
+	}
+
+	return 0;
+}
+
+static int pm8821_irq_read_master(struct pm_irq_chip *chip,
+				int master_number, u8 master_val)
+{
+	int ret = 0;
+	int block;
+
+	for (block = 1; block < 8; block++) {
+		if (master_val & BIT(block)) {
+			ret |= pm8821_irq_block_handler(chip,
+					master_number, block);
+		}
+	}
+
+	return ret;
+}
+
+static void pm8821_irq_handler(struct irq_desc *desc)
+{
+	struct pm_irq_chip *chip = irq_desc_get_handler_data(desc);
+	struct irq_chip *irq_chip = irq_desc_get_chip(desc);
+	int ret;
+	unsigned int master;
+
+	chained_irq_enter(irq_chip, desc);
+	/* check master 0 */
+	ret = pm8821_read_master_irq(chip, 0, &master);
+	if (ret) {
+		pr_err("Failed to re:Qad master 0 ret=%d\n", ret);
+		return;
+	}
+
+	if (master & ~PM8821_IRQ_MASTER1_SET)
+		pm8821_irq_read_master(chip, 0, master);
+
+	/* check master 1 */
+	if (!(master & PM8821_IRQ_MASTER1_SET))
+		goto done;
+
+	ret = pm8821_read_master_irq(chip, 1, &master);
+	if (ret) {
+		pr_err("Failed to read master 1 ret=%d\n", ret);
+		return;
+	}
+
+	pm8821_irq_read_master(chip, 1, master);
+
+done:
+	chained_irq_exit(irq_chip, desc);
+}
+
 static void pm8xxx_irq_mask_ack(struct irq_data *d)
 {
 	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
@@ -254,13 +394,15 @@ static int pm8xxx_irq_get_irqchip_state(struct irq_data *d,
 	irq_bit = pmirq % 8;
 
 	spin_lock(&chip->pm_irq_lock);
-	rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, block);
+	rc = regmap_write(chip->regmap, chip->irq_reg_base +
+			  SSBI_REG_ADDR_IRQ_BLK_SEL, block);
 	if (rc) {
 		pr_err("Failed Selecting Block %d rc=%d\n", block, rc);
 		goto bail;
 	}
 
-	rc = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
+	rc = regmap_read(chip->regmap, chip->irq_reg_base +
+			 SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
 	if (rc) {
 		pr_err("Failed Reading Status rc=%d\n", rc);
 		goto bail;
@@ -299,6 +441,151 @@ static const struct irq_domain_ops pm8xxx_irq_domain_ops = {
 	.map = pm8xxx_irq_domain_map,
 };
 
+static void pm8821_irq_mask_ack(struct irq_data *d)
+{
+	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
+	unsigned int base, pmirq = irqd_to_hwirq(d);
+	u8 block, master;
+	int irq_bit, rc;
+
+	block = pmirq / 8;
+	master = block / PM8821_BLOCKS_PER_MASTER;
+	irq_bit = pmirq % 8;
+	block %= PM8821_BLOCKS_PER_MASTER;
+
+	if (!master)
+		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
+	else
+		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
+
+	spin_lock(&chip->pm_irq_lock);
+	rc = regmap_update_bits(chip->regmap,
+				base + PM8821_IRQ_MASK_REG_OFFSET + block,
+				BIT(irq_bit), BIT(irq_bit));
+
+	if (rc) {
+		pr_err("Failed to read/write mask IRQ:%d rc=%d\n", pmirq, rc);
+		goto fail;
+	}
+
+	rc = regmap_update_bits(chip->regmap,
+				base + PM8821_IRQ_CLEAR_OFFSET + block,
+				BIT(irq_bit), BIT(irq_bit));
+
+	if (rc) {
+		pr_err("Failed to read/write IT_CLEAR IRQ:%d rc=%d\n",
+								pmirq, rc);
+	}
+
+fail:
+	spin_unlock(&chip->pm_irq_lock);
+}
+
+static void pm8821_irq_unmask(struct irq_data *d)
+{
+	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
+	unsigned int base, pmirq = irqd_to_hwirq(d);
+	int irq_bit, rc;
+	u8 block, master;
+
+	block = pmirq / 8;
+	master = block / PM8821_BLOCKS_PER_MASTER;
+	irq_bit = pmirq % 8;
+	block %= PM8821_BLOCKS_PER_MASTER;
+
+	if (!master)
+		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
+	else
+		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
+
+	spin_lock(&chip->pm_irq_lock);
+
+	rc = regmap_update_bits(chip->regmap,
+				base + PM8821_IRQ_MASK_REG_OFFSET + block,
+				BIT(irq_bit), ~BIT(irq_bit));
+
+	if (rc)
+		pr_err("Failed to read/write unmask IRQ:%d rc=%d\n", pmirq, rc);
+
+	spin_unlock(&chip->pm_irq_lock);
+}
+
+static int pm8821_irq_set_type(struct irq_data *d, unsigned int flow_type)
+{
+
+	/*
+	 * PM8821 IRQ controller does not have explicit software support for
+	 * IRQ flow type.
+	 */
+	return 0;
+}
+
+static int pm8821_irq_get_irqchip_state(struct irq_data *d,
+					enum irqchip_irq_state which,
+					bool *state)
+{
+	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
+	int pmirq, rc;
+	u8 block, irq_bit, master;
+	unsigned int bits;
+	unsigned int base;
+	unsigned long flags;
+
+	pmirq = irqd_to_hwirq(d);
+
+	block = pmirq / 8;
+	master = block / PM8821_BLOCKS_PER_MASTER;
+	irq_bit = pmirq % 8;
+	block %= PM8821_BLOCKS_PER_MASTER;
+
+	if (!master)
+		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
+	else
+		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
+
+	spin_lock_irqsave(&chip->pm_irq_lock, flags);
+
+	rc = regmap_read(chip->regmap,
+		base + PM8821_IRQ_RT_STATUS_OFFSET + block, &bits);
+	if (rc) {
+		pr_err("Failed Reading Status rc=%d\n", rc);
+		goto bail_out;
+	}
+
+	*state = !!(bits & BIT(irq_bit));
+
+bail_out:
+	spin_unlock_irqrestore(&chip->pm_irq_lock, flags);
+
+	return rc;
+}
+
+static struct irq_chip pm8821_irq_chip = {
+	.name		= "pm8821",
+	.irq_mask_ack	= pm8821_irq_mask_ack,
+	.irq_unmask	= pm8821_irq_unmask,
+	.irq_set_type	= pm8821_irq_set_type,
+	.irq_get_irqchip_state = pm8821_irq_get_irqchip_state,
+	.flags		= IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
+};
+
+static int pm8821_irq_domain_map(struct irq_domain *d, unsigned int irq,
+				   irq_hw_number_t hwirq)
+{
+	struct pm_irq_chip *chip = d->host_data;
+
+	irq_set_chip_and_handler(irq, &pm8821_irq_chip, handle_level_irq);
+	irq_set_chip_data(irq, chip);
+	irq_set_noprobe(irq);
+
+	return 0;
+}
+
+static const struct irq_domain_ops pm8821_irq_domain_ops = {
+	.xlate = irq_domain_xlate_twocell,
+	.map = pm8821_irq_domain_map,
+};
+
 static const struct regmap_config ssbi_regmap_config = {
 	.reg_bits = 16,
 	.val_bits = 8,
@@ -308,10 +595,25 @@ static const struct regmap_config ssbi_regmap_config = {
 	.reg_write = ssbi_reg_write
 };
 
+static const struct pm8xxx_data pm8xxx_data = {
+	.num_irqs = PM8921_NR_IRQS,
+	.irq_reg_base = SSBI_REG_ADDR_IRQ_BASE,
+	.irq_domain_ops = &pm8xxx_irq_domain_ops,
+	.irq_handler = pm8xxx_irq_handler,
+};
+
+static const struct pm8xxx_data pm8821_data = {
+	.num_irqs = PM8821_NR_IRQS,
+	.irq_reg_base = SSBI_PM8821_REG_ADDR_IRQ_BASE,
+	.irq_domain_ops = &pm8821_irq_domain_ops,
+	.irq_handler = pm8821_irq_handler,
+};
+
 static const struct of_device_id pm8921_id_table[] = {
-	{ .compatible = "qcom,pm8018", },
-	{ .compatible = "qcom,pm8058", },
-	{ .compatible = "qcom,pm8921", },
+	{ .compatible = "qcom,pm8018", .data = &pm8xxx_data},
+	{ .compatible = "qcom,pm8058", .data = &pm8xxx_data},
+	{ .compatible = "qcom,pm8821", .data = &pm8821_data},
+	{ .compatible = "qcom,pm8921", .data = &pm8xxx_data},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, pm8921_id_table);
@@ -319,11 +621,17 @@ MODULE_DEVICE_TABLE(of, pm8921_id_table);
 static int pm8921_probe(struct platform_device *pdev)
 {
 	struct regmap *regmap;
+	const struct pm8xxx_data *data;
 	int irq, rc;
 	unsigned int val;
 	u32 rev;
 	struct pm_irq_chip *chip;
-	unsigned int nirqs = PM8921_NR_IRQS;
+
+	data = of_match_node(pm8921_id_table, pdev->dev.of_node)->data;
+	if (!data) {
+		dev_err(&pdev->dev, "No matching driver data found\n");
+		return -EINVAL;
+	}
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
@@ -354,25 +662,27 @@ static int pm8921_probe(struct platform_device *pdev)
 	rev |= val << BITS_PER_BYTE;
 
 	chip = devm_kzalloc(&pdev->dev, sizeof(*chip) +
-					sizeof(chip->config[0]) * nirqs,
-					GFP_KERNEL);
+			    sizeof(chip->config[0]) * data->num_irqs,
+			    GFP_KERNEL);
 	if (!chip)
 		return -ENOMEM;
 
 	platform_set_drvdata(pdev, chip);
 	chip->regmap = regmap;
-	chip->num_irqs = nirqs;
+	chip->num_irqs = data->num_irqs;
+	chip->irq_reg_base = data->irq_reg_base;
 	chip->num_blocks = DIV_ROUND_UP(chip->num_irqs, 8);
 	chip->num_masters = DIV_ROUND_UP(chip->num_blocks, 8);
 	spin_lock_init(&chip->pm_irq_lock);
 
-	chip->irqdomain = irq_domain_add_linear(pdev->dev.of_node, nirqs,
-						&pm8xxx_irq_domain_ops,
+	chip->irqdomain = irq_domain_add_linear(pdev->dev.of_node,
+						data->num_irqs,
+						data->irq_domain_ops,
 						chip);
 	if (!chip->irqdomain)
 		return -ENODEV;
 
-	irq_set_chained_handler_and_data(irq, pm8xxx_irq_handler, chip);
+	irq_set_chained_handler_and_data(irq, data->irq_handler, chip);
 	irq_set_irq_wake(irq, 1);
 
 	rc = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
-- 
2.10.1

[PATCH 2/2] ARM: dts: apq8064: add support to pm8821

[Srinivas Kandagatla]

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 arch/arm/boot/dts/qcom-apq8064.dtsi | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi
index 1dbe697..fde006c 100644
--- a/arch/arm/boot/dts/qcom-apq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-apq8064.dtsi
@@ -627,6 +627,34 @@
 			clock-names = "core";
 		};
 
+		qcom,ssbi@c00000 {
+			compatible = "qcom,ssbi";
+			reg = <0x00c00000 0x1000>;
+			qcom,controller-type = "pmic-arbiter";
+
+			pmicintc2: pmic@1 {
+				compatible = "qcom,pm8821";
+				interrupt-parent = <&tlmm_pinmux>;
+				interrupts = <76 8>;
+				#interrupt-cells = <2>;
+				interrupt-controller;
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				pm8821_mpps: mpps@50 {
+
+					compatible = "qcom,pm8821-mpp", "qcom,ssbi-mpp";
+					reg = <0x50>;
+
+					interrupts = <24 1>, <25 1>, <26 1>,
+						     <27 1>;
+
+					gpio-controller;
+					#gpio-cells = <2>;
+		                };
+			};
+		};
+
 		qcom,ssbi@500000 {
 			compatible = "qcom,ssbi";
 			reg = <0x00500000 0x1000>;
-- 
2.10.1

Re: [PATCH 1/2] mfd: pm8921: add support to pm8821

[Bjorn Andersson]

On Tue 08 Nov 08:29 PST 2016, Srinivas Kandagatla wrote:

> This patch adds support to PM8821 PMIC and interrupt support.
> PM8821 is companion device that supplements primary PMIC PM8921 IC.
> 

Linus Walleij has a patch out for renaming a lot of things in this file,
so we should probably make sure that lands and then rebase this ontop.

> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> Tested this patch for MPP and IRQ functionality on IFC6410 and SD600 EVAL
> board with mpps PM8821 and PM8921.
> 
>  .../devicetree/bindings/mfd/qcom-pm8xxx.txt        |   1 +
>  drivers/mfd/pm8921-core.c                          | 368 +++++++++++++++++++--
>  2 files changed, 340 insertions(+), 29 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt b/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
> index 37a088f..8f1b4ec 100644
> --- a/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
> +++ b/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
> @@ -11,6 +11,7 @@ voltages and other various functionality to Qualcomm SoCs.
>  	Definition: must be one of:
>  		    "qcom,pm8058"
>  		    "qcom,pm8921"
> +		    "qcom,pm8821"
>  
>  - #address-cells:
>  	Usage: required
> diff --git a/drivers/mfd/pm8921-core.c b/drivers/mfd/pm8921-core.c
> index 0e3a2ea..28c2470 100644
> --- a/drivers/mfd/pm8921-core.c
> +++ b/drivers/mfd/pm8921-core.c
> @@ -28,16 +28,26 @@
>  #include <linux/mfd/core.h>
>  
>  #define	SSBI_REG_ADDR_IRQ_BASE		0x1BB
> -
> -#define	SSBI_REG_ADDR_IRQ_ROOT		(SSBI_REG_ADDR_IRQ_BASE + 0)
> -#define	SSBI_REG_ADDR_IRQ_M_STATUS1	(SSBI_REG_ADDR_IRQ_BASE + 1)
> -#define	SSBI_REG_ADDR_IRQ_M_STATUS2	(SSBI_REG_ADDR_IRQ_BASE + 2)
> -#define	SSBI_REG_ADDR_IRQ_M_STATUS3	(SSBI_REG_ADDR_IRQ_BASE + 3)
> -#define	SSBI_REG_ADDR_IRQ_M_STATUS4	(SSBI_REG_ADDR_IRQ_BASE + 4)
> -#define	SSBI_REG_ADDR_IRQ_BLK_SEL	(SSBI_REG_ADDR_IRQ_BASE + 5)
> -#define	SSBI_REG_ADDR_IRQ_IT_STATUS	(SSBI_REG_ADDR_IRQ_BASE + 6)
> -#define	SSBI_REG_ADDR_IRQ_CONFIG	(SSBI_REG_ADDR_IRQ_BASE + 7)
> -#define	SSBI_REG_ADDR_IRQ_RT_STATUS	(SSBI_REG_ADDR_IRQ_BASE + 8)

Keep these (per argumentation that follows), but try to name them
appropriately.

> +#define	SSBI_PM8821_REG_ADDR_IRQ_BASE	0x100
> +
> +#define	SSBI_REG_ADDR_IRQ_ROOT		(0)
> +#define	SSBI_REG_ADDR_IRQ_M_STATUS1	(1)
> +#define	SSBI_REG_ADDR_IRQ_M_STATUS2	(2)
> +#define	SSBI_REG_ADDR_IRQ_M_STATUS3	(3)
> +#define	SSBI_REG_ADDR_IRQ_M_STATUS4	(4)
> +#define	SSBI_REG_ADDR_IRQ_BLK_SEL	(5)
> +#define	SSBI_REG_ADDR_IRQ_IT_STATUS	(6)
> +#define	SSBI_REG_ADDR_IRQ_CONFIG	(7)
> +#define	SSBI_REG_ADDR_IRQ_RT_STATUS	(8)

Unnecessary parenthesis.

> +
> +#define	PM8821_TOTAL_IRQ_MASTERS	2

Unused.

> +#define	PM8821_BLOCKS_PER_MASTER	7
> +#define	PM8821_IRQ_MASTER1_SET		0x01

BIT(0), but I would prefer that you just inline this with a comment.

> +#define	PM8821_IRQ_CLEAR_OFFSET		0x01

Rather than having a single define for this and add in the base and
block numbers I think you should split it into a master0 and master1
define. (And it's not a offset as much as a register)

> +#define	PM8821_IRQ_RT_STATUS_OFFSET	0x0f

Dito

> +#define	PM8821_IRQ_MASK_REG_OFFSET	0x08

Dito

> +#define	SSBI_REG_ADDR_IRQ_MASTER0	0x30
> +#define	SSBI_REG_ADDR_IRQ_MASTER1	0xb0

Fold these two into the registers above.

>  
>  #define	PM_IRQF_LVL_SEL			0x01	/* level select */
>  #define	PM_IRQF_MASK_FE			0x02	/* mask falling edge */
> @@ -54,30 +64,41 @@
>  #define REG_HWREV_2		0x0E8  /* PMIC4 revision 2 */
>  
>  #define PM8921_NR_IRQS		256
> +#define PM8821_NR_IRQS		112
>  
>  struct pm_irq_chip {
>  	struct regmap		*regmap;
>  	spinlock_t		pm_irq_lock;
>  	struct irq_domain	*irqdomain;
> +	unsigned int		irq_reg_base;
>  	unsigned int		num_irqs;
>  	unsigned int		num_blocks;
>  	unsigned int		num_masters;
>  	u8			config[0];
>  };
>  
> +struct pm8xxx_data {
> +	int num_irqs;
> +	unsigned int		irq_reg_base;

As far as I can see this is always SSBI_PM8821_REG_ADDR_IRQ_BASE in the
8821 functions and SSBI_REG_ADDR_IRQ_BASE in the pm8xxx functions. If
you have disjunct code paths I think it's better to not obscure this
with a variable.

Try renaming the constants appropriately instead. This also has the
benefit of reducing the size of the patch slightly.

> +	const struct irq_domain_ops  *irq_domain_ops;
> +	void (*irq_handler)(struct irq_desc *desc);
> +};
> +
>  static int pm8xxx_read_block_irq(struct pm_irq_chip *chip, unsigned int bp,
>  				 unsigned int *ip)
>  {
>  	int	rc;
>  
>  	spin_lock(&chip->pm_irq_lock);
> -	rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, bp);
> +	rc = regmap_write(chip->regmap,
> +			  chip->irq_reg_base + SSBI_REG_ADDR_IRQ_BLK_SEL, bp);
>  	if (rc) {
>  		pr_err("Failed Selecting Block %d rc=%d\n", bp, rc);
>  		goto bail;
>  	}
>  
> -	rc = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_IT_STATUS, ip);
> +	rc = regmap_read(chip->regmap,
> +			 chip->irq_reg_base + SSBI_REG_ADDR_IRQ_IT_STATUS, ip);
>  	if (rc)
>  		pr_err("Failed Reading Status rc=%d\n", rc);
>  bail:
> @@ -91,14 +112,16 @@ pm8xxx_config_irq(struct pm_irq_chip *chip, unsigned int bp, unsigned int cp)
>  	int	rc;
>  
>  	spin_lock(&chip->pm_irq_lock);
> -	rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, bp);
> +	rc = regmap_write(chip->regmap,
> +			  chip->irq_reg_base + SSBI_REG_ADDR_IRQ_BLK_SEL, bp);
>  	if (rc) {
>  		pr_err("Failed Selecting Block %d rc=%d\n", bp, rc);
>  		goto bail;
>  	}
>  
>  	cp |= PM_IRQF_WRITE;
> -	rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_CONFIG, cp);
> +	rc = regmap_write(chip->regmap,
> +			  chip->irq_reg_base + SSBI_REG_ADDR_IRQ_CONFIG, cp);
>  	if (rc)
>  		pr_err("Failed Configuring IRQ rc=%d\n", rc);
>  bail:
> @@ -137,8 +160,8 @@ static int pm8xxx_irq_master_handler(struct pm_irq_chip *chip, int master)
>  	unsigned int blockbits;
>  	int block_number, i, ret = 0;
>  
> -	ret = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_M_STATUS1 + master,
> -			  &blockbits);
> +	ret = regmap_read(chip->regmap, chip->irq_reg_base +
> +			  SSBI_REG_ADDR_IRQ_M_STATUS1 + master, &blockbits);
>  	if (ret) {
>  		pr_err("Failed to read master %d ret=%d\n", master, ret);
>  		return ret;
> @@ -165,7 +188,8 @@ static void pm8xxx_irq_handler(struct irq_desc *desc)
>  
>  	chained_irq_enter(irq_chip, desc);
>  
> -	ret = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_ROOT, &root);
> +	ret = regmap_read(chip->regmap,
> +			  chip->irq_reg_base + SSBI_REG_ADDR_IRQ_ROOT, &root);
>  	if (ret) {
>  		pr_err("Can't read root status ret=%d\n", ret);
>  		return;
> @@ -182,6 +206,122 @@ static void pm8xxx_irq_handler(struct irq_desc *desc)
>  	chained_irq_exit(irq_chip, desc);
>  }
>  
> +static int pm8821_read_master_irq(const struct pm_irq_chip *chip,
> +				  int m, unsigned int *master)
> +{

I think you should inline this, as you already have the calls unrolled
in pm8821_irq_handler().

> +	unsigned int base;
> +
> +	if (!m)
> +		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
> +	else
> +		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
> +
> +	return regmap_read(chip->regmap, base, master);
> +}
> +
> +static int pm8821_read_block_irq(struct pm_irq_chip *chip, int master,
> +				 u8 block, unsigned int *bits)
> +{
> +	int rc;
> +
> +	unsigned int base;

Odd empty line between rc and base. (And btw, sorting your local
variables in descending length make things pretty).

> +
> +	if (!master)
> +		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
> +	else
> +		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
> +
> +	spin_lock(&chip->pm_irq_lock);

The reason why this is done under a lock in the other case is because
the status register is paged, so you shouldn't need it here.

With this updated I think you can favorably inline this into
pm8821_irq_block_handler().

> +
> +	rc = regmap_read(chip->regmap, base + block, bits);
> +	if (rc)
> +		pr_err("Failed Reading Status rc=%d\n", rc);
> +
> +	spin_unlock(&chip->pm_irq_lock);
> +	return rc;
> +}
> +
> +static int pm8821_irq_block_handler(struct pm_irq_chip *chip,
> +				    int master_number, int block)
> +{
> +	int pmirq, irq, i, ret;
> +	unsigned int bits;
> +
> +	ret = pm8821_read_block_irq(chip, master_number, block, &bits);
> +	if (ret) {
> +		pr_err("Failed reading %d block ret=%d", block, ret);
> +		return ret;
> +	}
> +	if (!bits) {
> +		pr_err("block bit set in master but no irqs: %d", block);
> +		return 0;
> +	}
> +
> +	/* Convert block offset to global block number */
> +	block += (master_number * PM8821_BLOCKS_PER_MASTER) - 1;

So this is block -= 1 for master 0 and block += 6 for master 1, is the
latter correct?

> +
> +	/* Check IRQ bits */
> +	for (i = 0; i < 8; i++) {
> +		if (bits & BIT(i)) {
> +			pmirq = block * 8 + i;
> +			irq = irq_find_mapping(chip->irqdomain, pmirq);
> +			generic_handle_irq(irq);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int pm8821_irq_read_master(struct pm_irq_chip *chip,
> +				int master_number, u8 master_val)

This isn't so much a matter of "reading master X" as "handle master X".

Also, you don't care about the return value, so no need to return one...

> +{
> +	int ret = 0;
> +	int block;
> +
> +	for (block = 1; block < 8; block++) {
> +		if (master_val & BIT(block)) {
> +			ret |= pm8821_irq_block_handler(chip,
> +					master_number, block);
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static void pm8821_irq_handler(struct irq_desc *desc)
> +{
> +	struct pm_irq_chip *chip = irq_desc_get_handler_data(desc);
> +	struct irq_chip *irq_chip = irq_desc_get_chip(desc);
> +	int ret;
> +	unsigned int master;
> +
> +	chained_irq_enter(irq_chip, desc);
> +	/* check master 0 */
> +	ret = pm8821_read_master_irq(chip, 0, &master);
> +	if (ret) {
> +		pr_err("Failed to re:Qad master 0 ret=%d\n", ret);
> +		return;
> +	}
> +
> +	if (master & ~PM8821_IRQ_MASTER1_SET)

Rather than having a define for MASTER1_SET use BIT(0) here and write a
comment like:

"bits 1 through 7 marks the first 7 blocks"

> +		pm8821_irq_read_master(chip, 0, master);
> +

and then

"bit 0 is set if second master contains any bits"

Or just skip this optimization and check the two masters unconditionally
in a loop.

> +	/* check master 1 */
> +	if (!(master & PM8821_IRQ_MASTER1_SET))
> +		goto done;
> +
> +	ret = pm8821_read_master_irq(chip, 1, &master);
> +	if (ret) {
> +		pr_err("Failed to read master 1 ret=%d\n", ret);
> +		return;
> +	}
> +
> +	pm8821_irq_read_master(chip, 1, master);
> +
> +done:
> +	chained_irq_exit(irq_chip, desc);
> +}
> +
>  static void pm8xxx_irq_mask_ack(struct irq_data *d)
>  {
>  	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> @@ -254,13 +394,15 @@ static int pm8xxx_irq_get_irqchip_state(struct irq_data *d,
>  	irq_bit = pmirq % 8;
>  
>  	spin_lock(&chip->pm_irq_lock);
> -	rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, block);
> +	rc = regmap_write(chip->regmap, chip->irq_reg_base +
> +			  SSBI_REG_ADDR_IRQ_BLK_SEL, block);
>  	if (rc) {
>  		pr_err("Failed Selecting Block %d rc=%d\n", block, rc);
>  		goto bail;
>  	}
>  
> -	rc = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
> +	rc = regmap_read(chip->regmap, chip->irq_reg_base +
> +			 SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
>  	if (rc) {
>  		pr_err("Failed Reading Status rc=%d\n", rc);
>  		goto bail;
> @@ -299,6 +441,151 @@ static const struct irq_domain_ops pm8xxx_irq_domain_ops = {
>  	.map = pm8xxx_irq_domain_map,
>  };
>  
> +static void pm8821_irq_mask_ack(struct irq_data *d)
> +{
> +	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> +	unsigned int base, pmirq = irqd_to_hwirq(d);
> +	u8 block, master;
> +	int irq_bit, rc;
> +
> +	block = pmirq / 8;
> +	master = block / PM8821_BLOCKS_PER_MASTER;
> +	irq_bit = pmirq % 8;
> +	block %= PM8821_BLOCKS_PER_MASTER;

You can deobfuscate this somewhat by instead of testing for !master
below you just do:

if (block < PM8821_BLOCKS_PER_MASTER) {
	base = 
} else {
	base = 
	block -= PM8821_BLOCKS_PER_MASTER;
}

> +
> +	if (!master)
> +		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
> +	else
> +		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
> +
> +	spin_lock(&chip->pm_irq_lock);

The irqchip code grabs a lock on the irq_desc, so this can't race with
unmask - and the regmap_update_bits() is internally protecting the
read/write cycle.

So you shouldn't need to lock around this section.

> +	rc = regmap_update_bits(chip->regmap,
> +				base + PM8821_IRQ_MASK_REG_OFFSET + block,
> +				BIT(irq_bit), BIT(irq_bit));
> +
> +	if (rc) {
> +		pr_err("Failed to read/write mask IRQ:%d rc=%d\n", pmirq, rc);
> +		goto fail;
> +	}
> +
> +	rc = regmap_update_bits(chip->regmap,
> +				base + PM8821_IRQ_CLEAR_OFFSET + block,
> +				BIT(irq_bit), BIT(irq_bit));
> +
> +	if (rc) {
> +		pr_err("Failed to read/write IT_CLEAR IRQ:%d rc=%d\n",
> +								pmirq, rc);
> +	}
> +
> +fail:
> +	spin_unlock(&chip->pm_irq_lock);
> +}
> +
> +static void pm8821_irq_unmask(struct irq_data *d)
> +{
> +	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> +	unsigned int base, pmirq = irqd_to_hwirq(d);
> +	int irq_bit, rc;
> +	u8 block, master;
> +
> +	block = pmirq / 8;
> +	master = block / PM8821_BLOCKS_PER_MASTER;
> +	irq_bit = pmirq % 8;
> +	block %= PM8821_BLOCKS_PER_MASTER;

As mask().

> +
> +	if (!master)
> +		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
> +	else
> +		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
> +
> +	spin_lock(&chip->pm_irq_lock);

As mask().

> +
> +	rc = regmap_update_bits(chip->regmap,
> +				base + PM8821_IRQ_MASK_REG_OFFSET + block,
> +				BIT(irq_bit), ~BIT(irq_bit));
> +
> +	if (rc)
> +		pr_err("Failed to read/write unmask IRQ:%d rc=%d\n", pmirq, rc);
> +
> +	spin_unlock(&chip->pm_irq_lock);
> +}
> +
> +static int pm8821_irq_set_type(struct irq_data *d, unsigned int flow_type)
> +{
> +
> +	/*
> +	 * PM8821 IRQ controller does not have explicit software support for
> +	 * IRQ flow type.
> +	 */

Is returning "success" here the right thing to do? Shouldn't we just
omit the function? Or did you perhaps hit some clients that wouldn't
deal with that?

> +	return 0;
> +}
> +
> +static int pm8821_irq_get_irqchip_state(struct irq_data *d,
> +					enum irqchip_irq_state which,
> +					bool *state)
> +{
> +	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> +	int pmirq, rc;
> +	u8 block, irq_bit, master;
> +	unsigned int bits;
> +	unsigned int base;
> +	unsigned long flags;
> +
> +	pmirq = irqd_to_hwirq(d);
> +
> +	block = pmirq / 8;
> +	master = block / PM8821_BLOCKS_PER_MASTER;
> +	irq_bit = pmirq % 8;
> +	block %= PM8821_BLOCKS_PER_MASTER;
> +

Simplify as in mask().

> +	if (!master)
> +		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
> +	else
> +		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
> +
> +	spin_lock_irqsave(&chip->pm_irq_lock, flags);

No need to lock here as we're just reading a single register.

> +
> +	rc = regmap_read(chip->regmap,
> +		base + PM8821_IRQ_RT_STATUS_OFFSET + block, &bits);
> +	if (rc) {
> +		pr_err("Failed Reading Status rc=%d\n", rc);
> +		goto bail_out;
> +	}
> +
> +	*state = !!(bits & BIT(irq_bit));
> +
> +bail_out:
> +	spin_unlock_irqrestore(&chip->pm_irq_lock, flags);
> +
> +	return rc;
> +}
> +
> +static struct irq_chip pm8821_irq_chip = {
> +	.name		= "pm8821",
> +	.irq_mask_ack	= pm8821_irq_mask_ack,
> +	.irq_unmask	= pm8821_irq_unmask,
> +	.irq_set_type	= pm8821_irq_set_type,
> +	.irq_get_irqchip_state = pm8821_irq_get_irqchip_state,
> +	.flags		= IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
> +};
> +

Regards,
Bjorn

Re: [PATCH 2/2] ARM: dts: apq8064: add support to pm8821

[Bjorn Andersson]

On Tue 08 Nov 08:29 PST 2016, Srinivas Kandagatla wrote:

> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  arch/arm/boot/dts/qcom-apq8064.dtsi | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi
> index 1dbe697..fde006c 100644
> --- a/arch/arm/boot/dts/qcom-apq8064.dtsi
> +++ b/arch/arm/boot/dts/qcom-apq8064.dtsi
> @@ -627,6 +627,34 @@
>  			clock-names = "core";
>  		};
>  
> +		qcom,ssbi@c00000 {

No "qcom," in the node name.

> +			compatible = "qcom,ssbi";
> +			reg = <0x00c00000 0x1000>;
> +			qcom,controller-type = "pmic-arbiter";
> +
> +			pmicintc2: pmic@1 {

I think we should follow Linus' lead and label this "pm8821".

> +				compatible = "qcom,pm8821";
> +				interrupt-parent = <&tlmm_pinmux>;
> +				interrupts = <76 8>;

Please spell out IRQ_TYPE_LEVEL_LOW.

And interrupts-extended = <&tlmm_pinmux 76 IRQ_TYPE_LEVEL_LOW> combines
the two lines nicely.

> +				#interrupt-cells = <2>;
> +				interrupt-controller;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				pm8821_mpps: mpps@50 {
> +

Extra newline.

> +					compatible = "qcom,pm8821-mpp", "qcom,ssbi-mpp";
> +					reg = <0x50>;
> +
> +					interrupts = <24 1>, <25 1>, <26 1>,
> +						     <27 1>;

I think these should be IRQ_TYPE_NONE per the discussion on how to share
interrupts between the gpio/mpp driver and other clients.

On the other hand, per the pm8821 driver we only support LEVEL_LOW
(high?).

> +
> +					gpio-controller;
> +					#gpio-cells = <2>;
> +		                };
> +			};
> +		};
> +

Regards,
Bjorn

Re: [PATCH 1/2] mfd: pm8921: add support to pm8821

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1764 bytes --]

Hi Srinivas,

[auto build test ERROR on ljones-mfd/for-mfd-next]
[also build test ERROR on v4.9-rc4 next-20161110]
[cannot apply to robh/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Srinivas-Kandagatla/mfd-pm8921-add-support-to-pm8821/20161109-013248
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: arm-pxa_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All error/warnings (new ones prefixed by >>):

   drivers/mfd/pm8921-core.c: In function 'pm8921_probe':
>> drivers/mfd/pm8921-core.c:630:58: warning: dereferencing 'void *' pointer
     data = of_match_node(pm8921_id_table, pdev->dev.of_node)->data;
                                                             ^~
>> drivers/mfd/pm8921-core.c:630:58: error: request for member 'data' in something not a structure or union

vim +/data +630 drivers/mfd/pm8921-core.c

   624		const struct pm8xxx_data *data;
   625		int irq, rc;
   626		unsigned int val;
   627		u32 rev;
   628		struct pm_irq_chip *chip;
   629	
 > 630		data = of_match_node(pm8921_id_table, pdev->dev.of_node)->data;
   631		if (!data) {
   632			dev_err(&pdev->dev, "No matching driver data found\n");
   633			return -EINVAL;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30088 bytes --]

Re: [PATCH 1/2] mfd: pm8921: add support to pm8821

[Rob Herring]

On Tue, Nov 08, 2016 at 04:29:36PM +0000, Srinivas Kandagatla wrote:
> This patch adds support to PM8821 PMIC and interrupt support.
> PM8821 is companion device that supplements primary PMIC PM8921 IC.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> Tested this patch for MPP and IRQ functionality on IFC6410 and SD600 EVAL
> board with mpps PM8821 and PM8921.
> 
>  .../devicetree/bindings/mfd/qcom-pm8xxx.txt        |   1 +

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/mfd/pm8921-core.c                          | 368 +++++++++++++++++++--
>  2 files changed, 340 insertions(+), 29 deletions(-)

Re: [PATCH 2/2] ARM: dts: apq8064: add support to pm8821

[Srinivas Kandagatla]

Thanks Bjorn for review comments.


On 08/11/16 19:13, Bjorn Andersson wrote:
> On Tue 08 Nov 08:29 PST 2016, Srinivas Kandagatla wrote:
>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>  arch/arm/boot/dts/qcom-apq8064.dtsi | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi
>> index 1dbe697..fde006c 100644
>> --- a/arch/arm/boot/dts/qcom-apq8064.dtsi
>> +++ b/arch/arm/boot/dts/qcom-apq8064.dtsi
>> @@ -627,6 +627,34 @@
>>  			clock-names = "core";
>>  		};
>>
>> +		qcom,ssbi@c00000 {
>
> No "qcom," in the node name.
Will fix it in next version,  I agree with rest of the comments too.
All of them will be fixed in next version.
>
>> +			compatible = "qcom,ssbi";
>> +			reg = <0x00c00000 0x1000>;
>> +			qcom,controller-type = "pmic-arbiter";
>> +
>> +			pmicintc2: pmic@1 {
>
> I think we should follow Linus' lead and label this "pm8821".
>
>> +				compatible = "qcom,pm8821";
>> +				interrupt-parent = <&tlmm_pinmux>;
>> +				interrupts = <76 8>;
>
> Please spell out IRQ_TYPE_LEVEL_LOW.
>
> And interrupts-extended = <&tlmm_pinmux 76 IRQ_TYPE_LEVEL_LOW> combines
> the two lines nicely.
>
>> +				#interrupt-cells = <2>;
>> +				interrupt-controller;
>> +				#address-cells = <1>;
>> +				#size-cells = <0>;
>> +
>> +				pm8821_mpps: mpps@50 {
>> +
>
> Extra newline.
>
>> +					compatible = "qcom,pm8821-mpp", "qcom,ssbi-mpp";
>> +					reg = <0x50>;
>> +
>> +					interrupts = <24 1>, <25 1>, <26 1>,
>> +						     <27 1>;
>
> I think these should be IRQ_TYPE_NONE per the discussion on how to share
> interrupts between the gpio/mpp driver and other clients.
>
> On the other hand, per the pm8821 driver we only support LEVEL_LOW
> (high?).
>
>> +
>> +					gpio-controller;
>> +					#gpio-cells = <2>;
>> +		                };
>> +			};
>> +		};
>> +
>
> Regards,
> Bjorn
>

Re: [PATCH 1/2] mfd: pm8921: add support to pm8821

[Srinivas Kandagatla]

Thanks Bjorn for review comments.

On 08/11/16 19:07, Bjorn Andersson wrote:
> On Tue 08 Nov 08:29 PST 2016, Srinivas Kandagatla wrote:
>
>> This patch adds support to PM8821 PMIC and interrupt support.
>> PM8821 is companion device that supplements primary PMIC PM8921 IC.
>>
>
> Linus Walleij has a patch out for renaming a lot of things in this file,
> so we should probably make sure that lands and then rebase this ontop.
>
Yep, Will rebase on top of it.

>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>> Tested this patch for MPP and IRQ functionality on IFC6410 and SD600 EVAL
>> board with mpps PM8821 and PM8921.
>>
>>  .../devicetree/bindings/mfd/qcom-pm8xxx.txt        |   1 +
>>  drivers/mfd/pm8921-core.c                          | 368 +++++++++++++++++++--
>>  2 files changed, 340 insertions(+), 29 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt b/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
>> index 37a088f..8f1b4ec 100644
>> --- a/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
>> +++ b/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
>> @@ -11,6 +11,7 @@ voltages and other various functionality to Qualcomm SoCs.
>>  	Definition: must be one of:
>>  		    "qcom,pm8058"
>>  		    "qcom,pm8921"
>> +		    "qcom,pm8821"
>>
>>  - #address-cells:
>>  	Usage: required
>> diff --git a/drivers/mfd/pm8921-core.c b/drivers/mfd/pm8921-core.c
>> index 0e3a2ea..28c2470 100644
>> --- a/drivers/mfd/pm8921-core.c
>> +++ b/drivers/mfd/pm8921-core.c
>> @@ -28,16 +28,26 @@
>>  #include <linux/mfd/core.h>
>>
>>  #define	SSBI_REG_ADDR_IRQ_BASE		0x1BB
>> -
>> -#define	SSBI_REG_ADDR_IRQ_ROOT		(SSBI_REG_ADDR_IRQ_BASE + 0)
>> -#define	SSBI_REG_ADDR_IRQ_M_STATUS1	(SSBI_REG_ADDR_IRQ_BASE + 1)
>> -#define	SSBI_REG_ADDR_IRQ_M_STATUS2	(SSBI_REG_ADDR_IRQ_BASE + 2)
>> -#define	SSBI_REG_ADDR_IRQ_M_STATUS3	(SSBI_REG_ADDR_IRQ_BASE + 3)
>> -#define	SSBI_REG_ADDR_IRQ_M_STATUS4	(SSBI_REG_ADDR_IRQ_BASE + 4)
>> -#define	SSBI_REG_ADDR_IRQ_BLK_SEL	(SSBI_REG_ADDR_IRQ_BASE + 5)
>> -#define	SSBI_REG_ADDR_IRQ_IT_STATUS	(SSBI_REG_ADDR_IRQ_BASE + 6)
>> -#define	SSBI_REG_ADDR_IRQ_CONFIG	(SSBI_REG_ADDR_IRQ_BASE + 7)
>> -#define	SSBI_REG_ADDR_IRQ_RT_STATUS	(SSBI_REG_ADDR_IRQ_BASE + 8)
>
> Keep these (per argumentation that follows), but try to name them
> appropriately.
>
Yes, I agree, I will address all the comments related to register 
defines in next version.
...
>
>>
>>  #define	PM_IRQF_LVL_SEL			0x01	/* level select */
>>  #define	PM_IRQF_MASK_FE			0x02	/* mask falling edge */
>> @@ -54,30 +64,41 @@
>>  #define REG_HWREV_2		0x0E8  /* PMIC4 revision 2 */
>>
>>  #define PM8921_NR_IRQS		256
>> +#define PM8821_NR_IRQS		112
>>
>>  struct pm_irq_chip {
>>  	struct regmap		*regmap;
>>  	spinlock_t		pm_irq_lock;
>>  	struct irq_domain	*irqdomain;
>> +	unsigned int		irq_reg_base;
>>  	unsigned int		num_irqs;
>>  	unsigned int		num_blocks;
>>  	unsigned int		num_masters;
>>  	u8			config[0];
>>  };
>>
>> +struct pm8xxx_data {
>> +	int num_irqs;
>> +	unsigned int		irq_reg_base;
>
> As far as I can see this is always SSBI_PM8821_REG_ADDR_IRQ_BASE in the
> 8821 functions and SSBI_REG_ADDR_IRQ_BASE in the pm8xxx functions. If
> you have disjunct code paths I think it's better to not obscure this
> with a variable.
>
> Try renaming the constants appropriately instead. This also has the
> benefit of reducing the size of the patch slightly.
>
Yep, will remove reg_base variable.

>>

...
>>
>> +static int pm8821_read_master_irq(const struct pm_irq_chip *chip,
>> +				  int m, unsigned int *master)
>> +{
>
> I think you should inline this, as you already have the calls unrolled
> in pm8821_irq_handler().

We can just call regmap_read directly from the caller function, and get 
rid of this function all together.
>
>> +	unsigned int base;
>> +
>> +	if (!m)
>> +		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
>> +	else
>> +		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
>> +
>> +	return regmap_read(chip->regmap, base, master);
>> +}
>> +
>> +static int pm8821_read_block_irq(struct pm_irq_chip *chip, int master,
>> +				 u8 block, unsigned int *bits)
>> +{
>> +	int rc;
>> +
>> +	unsigned int base;
>
> Odd empty line between rc and base. (And btw, sorting your local
> variables in descending length make things pretty).
Yep, will fix it in next version.

>
>> +
>> +	if (!master)
>> +		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
>> +	else
>> +		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
>> +
>> +	spin_lock(&chip->pm_irq_lock);
>
> The reason why this is done under a lock in the other case is because
> the status register is paged, so you shouldn't need it here.
>
Thanks for the info, will remove it.

> With this updated I think you can favorably inline this into
> pm8821_irq_block_handler().
>
>> +
>> +	rc = regmap_read(chip->regmap, base + block, bits);
>> +	if (rc)
>> +		pr_err("Failed Reading Status rc=%d\n", rc);
>> +
>> +	spin_unlock(&chip->pm_irq_lock);
>> +	return rc;
>> +}
>> +
>> +static int pm8821_irq_block_handler(struct pm_irq_chip *chip,
>> +				    int master_number, int block)
>> +{
>> +	int pmirq, irq, i, ret;
>> +	unsigned int bits;
>> +
>> +	ret = pm8821_read_block_irq(chip, master_number, block, &bits);
>> +	if (ret) {
>> +		pr_err("Failed reading %d block ret=%d", block, ret);
>> +		return ret;
>> +	}
>> +	if (!bits) {
>> +		pr_err("block bit set in master but no irqs: %d", block);
>> +		return 0;
>> +	}
>> +
>> +	/* Convert block offset to global block number */
>> +	block += (master_number * PM8821_BLOCKS_PER_MASTER) - 1;
>
> So this is block -= 1 for master 0 and block += 6 for master 1, is the
> latter correct?
>
Yes, both of them are correct.

for master 0 which has block numbers from 1-7 should translate to 0-6 in 
linear space.
for master 1 which has block numbers from 1-7 should translate to 7-13 
in linear space.

so for master0 it is -=1 and and for master1 it is +=6 seems correct.

>> +
>> +	/* Check IRQ bits */
>> +	for (i = 0; i < 8; i++) {
>> +		if (bits & BIT(i)) {
>> +			pmirq = block * 8 + i;
>> +			irq = irq_find_mapping(chip->irqdomain, pmirq);
>> +			generic_handle_irq(irq);
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int pm8821_irq_read_master(struct pm_irq_chip *chip,
>> +				int master_number, u8 master_val)
>
> This isn't so much a matter of "reading master X" as "handle master X".
>
Agreed, it would be more consistent with pm8xxx too.
> Also, you don't care about the return value, so no need to return one...
>
Yep will fix it.
>> +{
>> +	int ret = 0;
>> +	int block;
>> +
>> +	for (block = 1; block < 8; block++) {
>> +		if (master_val & BIT(block)) {
>> +			ret |= pm8821_irq_block_handler(chip,
>> +					master_number, block);
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static void pm8821_irq_handler(struct irq_desc *desc)
>> +{
>> +	struct pm_irq_chip *chip = irq_desc_get_handler_data(desc);
>> +	struct irq_chip *irq_chip = irq_desc_get_chip(desc);
>> +	int ret;
>> +	unsigned int master;
>> +
>> +	chained_irq_enter(irq_chip, desc);
>> +	/* check master 0 */
>> +	ret = pm8821_read_master_irq(chip, 0, &master);
>> +	if (ret) {
>> +		pr_err("Failed to re:Qad master 0 ret=%d\n", ret);
>> +		return;
>> +	}
>> +
>> +	if (master & ~PM8821_IRQ_MASTER1_SET)
>
> Rather than having a define for MASTER1_SET use BIT(0) here and write a
> comment like:
>
Yep, I will add some comments in this area.
> "bits 1 through 7 marks the first 7 blocks"
>
>> +		pm8821_irq_read_master(chip, 0, master);
>> +
>
> and then
>
> "bit 0 is set if second master contains any bits"
>
> Or just skip this optimization and check the two masters unconditionally
> in a loop.
>
>> +	/* check master 1 */
>> +	if (!(master & PM8821_IRQ_MASTER1_SET))
>> +		goto done;
>> +
>> +	ret = pm8821_read_master_irq(chip, 1, &master);
>> +	if (ret) {
>> +		pr_err("Failed to read master 1 ret=%d\n", ret);
>> +		return;
>> +	}
>> +
>> +	pm8821_irq_read_master(chip, 1, master);
>> +
>> +done:
>> +	chained_irq_exit(irq_chip, desc);
>> +}
>> +
>>  static void pm8xxx_irq_mask_ack(struct irq_data *d)
>>  {
>>  	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> @@ -254,13 +394,15 @@ static int pm8xxx_irq_get_irqchip_state(struct irq_data *d,
>>  	irq_bit = pmirq % 8;
>>
>>  	spin_lock(&chip->pm_irq_lock);
>> -	rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, block);
>> +	rc = regmap_write(chip->regmap, chip->irq_reg_base +
>> +			  SSBI_REG_ADDR_IRQ_BLK_SEL, block);
>>  	if (rc) {
>>  		pr_err("Failed Selecting Block %d rc=%d\n", block, rc);
>>  		goto bail;
>>  	}
>>
>> -	rc = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
>> +	rc = regmap_read(chip->regmap, chip->irq_reg_base +
>> +			 SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
>>  	if (rc) {
>>  		pr_err("Failed Reading Status rc=%d\n", rc);
>>  		goto bail;
>> @@ -299,6 +441,151 @@ static const struct irq_domain_ops pm8xxx_irq_domain_ops = {
>>  	.map = pm8xxx_irq_domain_map,
>>  };
>>
>> +static void pm8821_irq_mask_ack(struct irq_data *d)
>> +{
>> +	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> +	unsigned int base, pmirq = irqd_to_hwirq(d);
>> +	u8 block, master;
>> +	int irq_bit, rc;
>> +
>> +	block = pmirq / 8;
>> +	master = block / PM8821_BLOCKS_PER_MASTER;
>> +	irq_bit = pmirq % 8;
>> +	block %= PM8821_BLOCKS_PER_MASTER;
>
> You can deobfuscate this somewhat by instead of testing for !master
> below you just do:
>
> if (block < PM8821_BLOCKS_PER_MASTER) {
> 	base =
> } else {
> 	base =
> 	block -= PM8821_BLOCKS_PER_MASTER;
> }
>
Done some cleanup in register defines which avoids this totally.
>> +
>> +	if (!master)
>> +		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
>> +	else
>> +		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
>> +
>> +	spin_lock(&chip->pm_irq_lock);
>
> The irqchip code grabs a lock on the irq_desc, so this can't race with
> unmask - and the regmap_update_bits() is internally protecting the
> read/write cycle.
>
> So you shouldn't need to lock around this section.
>
Yep.
>> +	rc = regmap_update_bits(chip->regmap,
>> +				base + PM8821_IRQ_MASK_REG_OFFSET + block,
>> +				BIT(irq_bit), BIT(irq_bit));
>> +
>> +	if (rc) {
>> +		pr_err("Failed to read/write mask IRQ:%d rc=%d\n", pmirq, rc);
>> +		goto fail;
>> +	}
>> +
>> +	rc = regmap_update_bits(chip->regmap,
>> +				base + PM8821_IRQ_CLEAR_OFFSET + block,
>> +				BIT(irq_bit), BIT(irq_bit));
>> +
>> +	if (rc) {
>> +		pr_err("Failed to read/write IT_CLEAR IRQ:%d rc=%d\n",
>> +								pmirq, rc);
>> +	}
>> +
>> +fail:
>> +	spin_unlock(&chip->pm_irq_lock);
>> +}
>> +
>> +static void pm8821_irq_unmask(struct irq_data *d)
>> +{
>> +	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> +	unsigned int base, pmirq = irqd_to_hwirq(d);
>> +	int irq_bit, rc;
>> +	u8 block, master;
>> +
>> +	block = pmirq / 8;
>> +	master = block / PM8821_BLOCKS_PER_MASTER;
>> +	irq_bit = pmirq % 8;
>> +	block %= PM8821_BLOCKS_PER_MASTER;
>
> As mask().
>
>> +
>> +	if (!master)
>> +		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
>> +	else
>> +		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
>> +
>> +	spin_lock(&chip->pm_irq_lock);
>
> As mask().
>
>> +
>> +	rc = regmap_update_bits(chip->regmap,
>> +				base + PM8821_IRQ_MASK_REG_OFFSET + block,
>> +				BIT(irq_bit), ~BIT(irq_bit));
>> +
>> +	if (rc)
>> +		pr_err("Failed to read/write unmask IRQ:%d rc=%d\n", pmirq, rc);
>> +
>> +	spin_unlock(&chip->pm_irq_lock);
>> +}
>> +
>> +static int pm8821_irq_set_type(struct irq_data *d, unsigned int flow_type)
>> +{
>> +
>> +	/*
>> +	 * PM8821 IRQ controller does not have explicit software support for
>> +	 * IRQ flow type.
>> +	 */
>
> Is returning "success" here the right thing to do? Shouldn't we just
> omit the function? Or did you perhaps hit some clients that wouldn't
> deal with that?
>
Will remove this totally.
>> +	return 0;
>> +}
>> +
>> +static int pm8821_irq_get_irqchip_state(struct irq_data *d,
>> +					enum irqchip_irq_state which,
>> +					bool *state)
>> +{
>> +	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> +	int pmirq, rc;
>> +	u8 block, irq_bit, master;
>> +	unsigned int bits;
>> +	unsigned int base;
>> +	unsigned long flags;
>> +
>> +	pmirq = irqd_to_hwirq(d);
>> +
>> +	block = pmirq / 8;
>> +	master = block / PM8821_BLOCKS_PER_MASTER;
>> +	irq_bit = pmirq % 8;
>> +	block %= PM8821_BLOCKS_PER_MASTER;
>> +
>
> Simplify as in mask().
taken care by new register defines.
>
>> +	if (!master)
>> +		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
>> +	else
>> +		base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
>> +
>> +	spin_lock_irqsave(&chip->pm_irq_lock, flags);
>
> No need to lock here as we're just reading a single register.
>
yep done.

>> +
>> +	rc = regmap_read(chip->regmap,
>> +		base + PM8821_IRQ_RT_STATUS_OFFSET + block, &bits);
>> +	if (rc) {
>> +		pr_err("Failed Reading Status rc=%d\n", rc);
>> +		goto bail_out;
>> +	}
>> +
>> +	*state = !!(bits & BIT(irq_bit));
>> +
>> +bail_out:
>> +	spin_unlock_irqrestore(&chip->pm_irq_lock, flags);
>> +
>> +	return rc;
>> +}
>> +
>> +static struct irq_chip pm8821_irq_chip = {
>> +	.name		= "pm8821",
>> +	.irq_mask_ack	= pm8821_irq_mask_ack,
>> +	.irq_unmask	= pm8821_irq_unmask,
>> +	.irq_set_type	= pm8821_irq_set_type,
>> +	.irq_get_irqchip_state = pm8821_irq_get_irqchip_state,
>> +	.flags		= IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
>> +};
>> +
>
> Regards,
> Bjorn
>

Re: [PATCH 1/2] mfd: pm8921: add support to pm8821

[Bjorn Andersson]

On Mon 14 Nov 09:33 PST 2016, Srinivas Kandagatla wrote:

[..]
> >>+static int pm8821_irq_block_handler(struct pm_irq_chip *chip,
> >>+				    int master_number, int block)
> >>+{
> >>+	int pmirq, irq, i, ret;
> >>+	unsigned int bits;
> >>+
> >>+	ret = pm8821_read_block_irq(chip, master_number, block, &bits);
> >>+	if (ret) {
> >>+		pr_err("Failed reading %d block ret=%d", block, ret);
> >>+		return ret;
> >>+	}
> >>+	if (!bits) {
> >>+		pr_err("block bit set in master but no irqs: %d", block);
> >>+		return 0;
> >>+	}
> >>+
> >>+	/* Convert block offset to global block number */
> >>+	block += (master_number * PM8821_BLOCKS_PER_MASTER) - 1;
> >
> >So this is block -= 1 for master 0 and block += 6 for master 1, is the
> >latter correct?
> >
> Yes, both of them are correct.
> 
> for master 0 which has block numbers from 1-7 should translate to 0-6 in
> linear space.
> for master 1 which has block numbers from 1-7 should translate to 7-13 in
> linear space.
> 
> so for master0 it is -=1 and and for master1 it is +=6 seems correct.
> 

Ahh, because block is 1-indexed when we enter, so have to switch base
and then calculate the global number, like:

  block = block - 1 + (master * PER_MASTER) + 7

but we cancel out the subtraction. I agree that this looks correct then.

I would prefer less of a mixture between 0-indexing and 1-indexing, but
I don't have any good ideas on how to restructure it to make it better.

Regards,
Bjorn