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

[PATCH v2 1/2] mfd: pm8xxx: 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>
Acked-by: Rob Herring <robh@kernel.org>
---
Changes from v1:
	- Removed unnessary locking spotted by Bjorn
	- updated register naming to reflect PM8821
	- lot of cleanups suggested by Bjorn
	- rebased on top of Linus Walleij's pm8xxx namespace
	 cleanup patch. 

 .../devicetree/bindings/mfd/qcom-pm8xxx.txt        |   1 +
 drivers/mfd/qcom-pm8xxx.c                          | 247 ++++++++++++++++++++-
 2 files changed, 238 insertions(+), 10 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/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
index 7f9620e..dc347d3 100644
--- a/drivers/mfd/qcom-pm8xxx.c
+++ b/drivers/mfd/qcom-pm8xxx.c
@@ -24,6 +24,7 @@
 #include <linux/err.h>
 #include <linux/ssbi.h>
 #include <linux/regmap.h>
+#include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/mfd/core.h>
 
@@ -39,6 +40,20 @@
 #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	PM8821_SSBI_REG_ADDR_IRQ_BASE	0x100
+#define	PM8821_SSBI_REG_ADDR_IRQ_MASTER0 (PM8821_SSBI_REG_ADDR_IRQ_BASE + 0x30)
+#define	PM8821_SSBI_REG_ADDR_IRQ_MASTER1 (PM8821_SSBI_REG_ADDR_IRQ_BASE + 0xb0)
+#define	PM8821_SSBI_REG(m, b, offset) \
+			((m == 0) ? \
+			(PM8821_SSBI_REG_ADDR_IRQ_MASTER0 + b + offset) : \
+			(PM8821_SSBI_REG_ADDR_IRQ_MASTER1 + b + offset))
+#define	PM8821_SSBI_ADDR_IRQ_ROOT(m, b)		PM8821_SSBI_REG(m, b, 0x0)
+#define	PM8821_SSBI_ADDR_IRQ_CLEAR(m, b)	PM8821_SSBI_REG(m, b, 0x01)
+#define	PM8821_SSBI_ADDR_IRQ_MASK(m, b)		PM8821_SSBI_REG(m, b, 0x08)
+#define	PM8821_SSBI_ADDR_IRQ_RT_STATUS(m, b)	PM8821_SSBI_REG(m, b, 0x0f)
+
+#define	PM8821_BLOCKS_PER_MASTER	7
+
 #define	PM_IRQF_LVL_SEL			0x01	/* level select */
 #define	PM_IRQF_MASK_FE			0x02	/* mask falling edge */
 #define	PM_IRQF_MASK_RE			0x04	/* mask rising edge */
@@ -54,6 +69,7 @@
 #define REG_HWREV_2		0x0E8  /* PMIC4 revision 2 */
 
 #define PM8XXX_NR_IRQS		256
+#define PM8821_NR_IRQS		112
 
 struct pm_irq_chip {
 	struct regmap		*regmap;
@@ -65,6 +81,12 @@ struct pm_irq_chip {
 	u8			config[0];
 };
 
+struct pm_irq_data {
+	int num_irqs;
+	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)
 {
@@ -182,6 +204,84 @@ static void pm8xxx_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(irq_chip, desc);
 }
 
+static void pm8821_irq_block_handler(struct pm_irq_chip *chip,
+				     int master, int block)
+{
+	int pmirq, irq, i, ret;
+	unsigned int bits;
+
+	ret = regmap_read(chip->regmap,
+			  PM8821_SSBI_ADDR_IRQ_ROOT(master, block), &bits);
+	if (ret) {
+		pr_err("Failed reading %d block ret=%d", block, ret);
+		return;
+	}
+	if (!bits) {
+		pr_err("block bit set in master but no irqs: %d", block);
+		return;
+	}
+
+	/* Convert block offset to global block number */
+	block += (master * 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);
+		}
+	}
+
+}
+
+static inline void pm8821_irq_master_handler(struct pm_irq_chip *chip,
+					     int master, u8 master_val)
+{
+	int block;
+
+	for (block = 1; block < 8; block++)
+		if (master_val & BIT(block))
+			pm8821_irq_block_handler(chip, master, block);
+
+}
+
+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);
+	unsigned int master;
+	int ret;
+
+	chained_irq_enter(irq_chip, desc);
+	ret = regmap_read(chip->regmap,
+			  PM8821_SSBI_REG_ADDR_IRQ_MASTER0, &master);
+	if (ret) {
+		pr_err("Failed to re:Qad master 0 ret=%d\n", ret);
+		return;
+	}
+
+	 /* bits 1 through 7 marks the first 7 blocks in master 0*/
+	if (master & GENMASK(7, 1))
+		pm8821_irq_master_handler(chip, 0, master);
+
+	 /* bit 0 marks if master 1 contains any bits */
+	if (!(master & BIT(0)))
+		goto done;
+
+	ret = regmap_read(chip->regmap,
+			  PM8821_SSBI_REG_ADDR_IRQ_MASTER1, &master);
+	if (ret) {
+		pr_err("Failed to read master 1 ret=%d\n", ret);
+		return;
+	}
+
+	pm8821_irq_master_handler(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);
@@ -299,6 +399,110 @@ 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 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;
+
+	rc = regmap_update_bits(chip->regmap,
+				PM8821_SSBI_ADDR_IRQ_MASK(master, block),
+				BIT(irq_bit), BIT(irq_bit));
+
+	if (rc) {
+		pr_err("Failed to read/write mask IRQ:%d rc=%d\n", pmirq, rc);
+		return;
+	}
+
+	rc = regmap_update_bits(chip->regmap,
+				PM8821_SSBI_ADDR_IRQ_CLEAR(master, block),
+				BIT(irq_bit), BIT(irq_bit));
+
+	if (rc) {
+		pr_err("Failed to read/write IT_CLEAR IRQ:%d rc=%d\n",
+								pmirq, rc);
+	}
+
+}
+
+static void pm8821_irq_unmask(struct irq_data *d)
+{
+	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
+	unsigned int 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;
+
+	rc = regmap_update_bits(chip->regmap,
+				PM8821_SSBI_ADDR_IRQ_MASK(master, block),
+				BIT(irq_bit), ~BIT(irq_bit));
+
+	if (rc)
+		pr_err("Failed to read/write unmask IRQ:%d rc=%d\n", pmirq, rc);
+
+}
+
+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 rc, pmirq = irqd_to_hwirq(d);
+	u8 block, irq_bit, master;
+	unsigned int bits;
+
+	block = pmirq / 8;
+	master = block / PM8821_BLOCKS_PER_MASTER;
+	irq_bit = pmirq % 8;
+	block %= PM8821_BLOCKS_PER_MASTER;
+
+	rc = regmap_read(chip->regmap,
+		PM8821_SSBI_ADDR_IRQ_RT_STATUS(master, block), &bits);
+	if (rc) {
+		pr_err("Failed Reading Status rc=%d\n", rc);
+		return rc;
+	}
+
+	*state = !!(bits & BIT(irq_bit));
+
+	return rc;
+}
+
+static struct irq_chip pm8821_irq_chip = {
+	.name		= "pm8821",
+	.irq_mask_ack	= pm8821_irq_mask_ack,
+	.irq_unmask	= pm8821_irq_unmask,
+	.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,22 +512,44 @@ static const struct regmap_config ssbi_regmap_config = {
 	.reg_write = ssbi_reg_write
 };
 
+static const struct pm_irq_data pm8xxx_data = {
+	.num_irqs = PM8XXX_NR_IRQS,
+	.irq_domain_ops = &pm8xxx_irq_domain_ops,
+	.irq_handler = pm8xxx_irq_handler,
+};
+
+static const struct pm_irq_data pm8821_data = {
+	.num_irqs = PM8821_NR_IRQS,
+	.irq_domain_ops = &pm8821_irq_domain_ops,
+	.irq_handler = pm8821_irq_handler,
+};
+
 static const struct of_device_id pm8xxx_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, pm8xxx_id_table);
 
 static int pm8xxx_probe(struct platform_device *pdev)
 {
+	const struct of_device_id *match;
+	const struct pm_irq_data *data;
 	struct regmap *regmap;
 	int irq, rc;
 	unsigned int val;
 	u32 rev;
 	struct pm_irq_chip *chip;
-	unsigned int nirqs = PM8XXX_NR_IRQS;
+
+	match = of_match_node(pm8xxx_id_table, pdev->dev.of_node);
+	if (!match) {
+		dev_err(&pdev->dev, "No matching driver data found\n");
+		return -EINVAL;
+	}
+
+	data = match->data;
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
@@ -354,25 +580,26 @@ static int pm8xxx_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->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 v2 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 | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi
index 268bd47..c61ba32 100644
--- a/arch/arm/boot/dts/qcom-apq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-apq8064.dtsi
@@ -627,6 +627,33 @@
 			clock-names = "core";
 		};
 
+		ssbi@c00000 {
+			compatible = "qcom,ssbi";
+			reg = <0x00c00000 0x1000>;
+			qcom,controller-type = "pmic-arbiter";
+
+			pm8821: pmic@1 {
+				compatible = "qcom,pm8821";
+				interrupt-parent = <&tlmm_pinmux>;
+				interrupts = <76 IRQ_TYPE_LEVEL_LOW>;
+				#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 IRQ_TYPE_NONE>,
+						     <25 IRQ_TYPE_NONE>,
+						     <26 IRQ_TYPE_NONE>,
+						     <27 IRQ_TYPE_NONE>;
+					gpio-controller;
+					#gpio-cells = <2>;
+		                };
+			};
+		};
+
 		qcom,ssbi@500000 {
 			compatible = "qcom,ssbi";
 			reg = <0x00500000 0x1000>;
-- 
2.10.1

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

[Stephen Boyd]

On 11/14/2016 09:52 AM, Srinivas Kandagatla wrote:
> diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
> index 7f9620e..dc347d3 100644
> --- a/drivers/mfd/qcom-pm8xxx.c
> +++ b/drivers/mfd/qcom-pm8xxx.c
> +
> +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);
> +	unsigned int master;
> +	int ret;
> +
> +	chained_irq_enter(irq_chip, desc);
> +	ret = regmap_read(chip->regmap,
> +			  PM8821_SSBI_REG_ADDR_IRQ_MASTER0, &master);
> +	if (ret) {
> +		pr_err("Failed to re:Qad master 0 ret=%d\n", ret);

Hm? vi?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Bjorn Andersson]

On Mon 14 Nov 09:52 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.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
> Changes from v1:
> 	- Removed unnessary locking spotted by Bjorn
> 	- updated register naming to reflect PM8821
> 	- lot of cleanups suggested by Bjorn
> 	- rebased on top of Linus Walleij's pm8xxx namespace
> 	 cleanup patch. 

Looks good, just some minor style nits below.

> 
>  .../devicetree/bindings/mfd/qcom-pm8xxx.txt        |   1 +
>  drivers/mfd/qcom-pm8xxx.c                          | 247 ++++++++++++++++++++-
>  2 files changed, 238 insertions(+), 10 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"

8 < 9, so move it one step up please.

>  
>  - #address-cells:
>  	Usage: required
> diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
[..]
> +#define	PM8821_SSBI_REG_ADDR_IRQ_BASE	0x100
> +#define	PM8821_SSBI_REG_ADDR_IRQ_MASTER0 (PM8821_SSBI_REG_ADDR_IRQ_BASE + 0x30)
> +#define	PM8821_SSBI_REG_ADDR_IRQ_MASTER1 (PM8821_SSBI_REG_ADDR_IRQ_BASE + 0xb0)
> +#define	PM8821_SSBI_REG(m, b, offset) \
> +			((m == 0) ? \
> +			(PM8821_SSBI_REG_ADDR_IRQ_MASTER0 + b + offset) : \
> +			(PM8821_SSBI_REG_ADDR_IRQ_MASTER1 + b + offset))
> +#define	PM8821_SSBI_ADDR_IRQ_ROOT(m, b)		PM8821_SSBI_REG(m, b, 0x0)
> +#define	PM8821_SSBI_ADDR_IRQ_CLEAR(m, b)	PM8821_SSBI_REG(m, b, 0x01)
> +#define	PM8821_SSBI_ADDR_IRQ_MASK(m, b)		PM8821_SSBI_REG(m, b, 0x08)
> +#define	PM8821_SSBI_ADDR_IRQ_RT_STATUS(m, b)	PM8821_SSBI_REG(m, b, 0x0f)

I like how this cleaned up the rest of the implementation.

[..]

> +static void pm8821_irq_block_handler(struct pm_irq_chip *chip,
> +				     int master, int block)
> +{
> +	int pmirq, irq, i, ret;
> +	unsigned int bits;
> +
> +	ret = regmap_read(chip->regmap,
> +			  PM8821_SSBI_ADDR_IRQ_ROOT(master, block), &bits);
> +	if (ret) {
> +		pr_err("Failed reading %d block ret=%d", block, ret);

"Reading block %d failed ret=%d"

> +		return;
> +	}
> +	if (!bits) {
> +		pr_err("block bit set in master but no irqs: %d", block);

This seems more like a debug thing, either showing missbehaving hardware
or something odd in the driver. I think you should drop the print or
make it pr_debug().

> +		return;
> +	}

I would prefer that you just drop the entire if statement, it's just an
corner case optimization with a info print.

> +
> +	/* Convert block offset to global block number */
> +	block += (master * 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);
> +		}
> +	}
> +

Empty line

> +}
> +
> +static inline void pm8821_irq_master_handler(struct pm_irq_chip *chip,
> +					     int master, u8 master_val)
> +{
> +	int block;
> +
> +	for (block = 1; block < 8; block++)
> +		if (master_val & BIT(block))
> +			pm8821_irq_block_handler(chip, master, block);
> +

Empty line

> +}
> +
> +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);
> +	unsigned int master;
> +	int ret;
> +
> +	chained_irq_enter(irq_chip, desc);
> +	ret = regmap_read(chip->regmap,
> +			  PM8821_SSBI_REG_ADDR_IRQ_MASTER0, &master);
> +	if (ret) {
> +		pr_err("Failed to re:Qad master 0 ret=%d\n", ret);
                                      ^
				      |
			  I see you're using vim :)

> +		return;
> +	}
> +
> +	 /* bits 1 through 7 marks the first 7 blocks in master 0*/

Indentation

> +	if (master & GENMASK(7, 1))
> +		pm8821_irq_master_handler(chip, 0, master);
> +
> +	 /* bit 0 marks if master 1 contains any bits */

Dito

> +	if (!(master & BIT(0)))
> +		goto done;
> +
> +	ret = regmap_read(chip->regmap,
> +			  PM8821_SSBI_REG_ADDR_IRQ_MASTER1, &master);
> +	if (ret) {
> +		pr_err("Failed to read master 1 ret=%d\n", ret);
> +		return;

"goto done;" so that we pass chained_irq_exit()

> +	}
> +
> +	pm8821_irq_master_handler(chip, 1, master);
> +
> +done:
> +	chained_irq_exit(irq_chip, desc);
> +}
> +

[..]

> +static void pm8821_irq_mask_ack(struct irq_data *d)
> +{
> +	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> +	unsigned int 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;
> +
> +	rc = regmap_update_bits(chip->regmap,
> +				PM8821_SSBI_ADDR_IRQ_MASK(master, block),
> +				BIT(irq_bit), BIT(irq_bit));
> +

Empty line

> +	if (rc) {
> +		pr_err("Failed to read/write mask IRQ:%d rc=%d\n", pmirq, rc);

"Failed to mask IRQ %d rc=%d\n"

> +		return;
> +	}
> +
> +	rc = regmap_update_bits(chip->regmap,
> +				PM8821_SSBI_ADDR_IRQ_CLEAR(master, block),
> +				BIT(irq_bit), BIT(irq_bit));
> +

Empty line

> +	if (rc) {
> +		pr_err("Failed to read/write IT_CLEAR IRQ:%d rc=%d\n",
> +								pmirq, rc);

"Failed to clear IRQ %d rc=%d\n"

> +	}
> +

Empty line

> +}
> +
> +static void pm8821_irq_unmask(struct irq_data *d)
> +{
> +	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> +	unsigned int 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;
> +
> +	rc = regmap_update_bits(chip->regmap,
> +				PM8821_SSBI_ADDR_IRQ_MASK(master, block),
> +				BIT(irq_bit), ~BIT(irq_bit));
> +

Empty line

> +	if (rc)
> +		pr_err("Failed to read/write unmask IRQ:%d rc=%d\n", pmirq, rc);

"Failed to unmask IRQ %d rc=%d\n"

> +

Empty line

> +}
> +
> +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 rc, pmirq = irqd_to_hwirq(d);
> +	u8 block, irq_bit, master;
> +	unsigned int bits;
> +
> +	block = pmirq / 8;
> +	master = block / PM8821_BLOCKS_PER_MASTER;
> +	irq_bit = pmirq % 8;
> +	block %= PM8821_BLOCKS_PER_MASTER;
> +
> +	rc = regmap_read(chip->regmap,
> +		PM8821_SSBI_ADDR_IRQ_RT_STATUS(master, block), &bits);
> +	if (rc) {
> +		pr_err("Failed Reading Status rc=%d\n", rc);

Odd capitalization, I suggest that you match it to the other functions
by:

"Reading status of IRQ %d failed rc=%d\n" 

> +		return rc;
> +	}
> +
> +	*state = !!(bits & BIT(irq_bit));
> +
> +	return rc;
> +}
> +

[..]

>  
>  static int pm8xxx_probe(struct platform_device *pdev)
>  {
> +	const struct of_device_id *match;
> +	const struct pm_irq_data *data;
>  	struct regmap *regmap;
>  	int irq, rc;
>  	unsigned int val;
>  	u32 rev;
>  	struct pm_irq_chip *chip;
> -	unsigned int nirqs = PM8XXX_NR_IRQS;
> +
> +	match = of_match_node(pm8xxx_id_table, pdev->dev.of_node);
> +	if (!match) {
> +		dev_err(&pdev->dev, "No matching driver data found\n");
> +		return -EINVAL;
> +	}
> +
> +	data = match->data;

data = of_device_get_match_data(&pdev->dev); (from of_device.h)

Regards,
Bjorn

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

[Srinivas Kandagatla]

On 14/11/16 18:40, Stephen Boyd wrote:
> On 11/14/2016 09:52 AM, Srinivas Kandagatla wrote:
>> diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
>> index 7f9620e..dc347d3 100644
>> --- a/drivers/mfd/qcom-pm8xxx.c
>> +++ b/drivers/mfd/qcom-pm8xxx.c
>> +
>> +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);
>> +	unsigned int master;
>> +	int ret;
>> +
>> +	chained_irq_enter(irq_chip, desc);
>> +	ret = regmap_read(chip->regmap,
>> +			  PM8821_SSBI_REG_ADDR_IRQ_MASTER0, &master);
>> +	if (ret) {
>> +		pr_err("Failed to re:Qad master 0 ret=%d\n", ret);
>
> Hm? vi?
yes..  That was good catch!! will fix it in next version...
>

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

[Srinivas Kandagatla]

On 14/11/16 18:56, Bjorn Andersson wrote:
> On Mon 14 Nov 09:52 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.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Acked-by: Rob Herring <robh@kernel.org>
>> ---
>> Changes from v1:
>> 	- Removed unnessary locking spotted by Bjorn
>> 	- updated register naming to reflect PM8821
>> 	- lot of cleanups suggested by Bjorn
>> 	- rebased on top of Linus Walleij's pm8xxx namespace
>> 	 cleanup patch.
>
> Looks good, just some minor style nits below.
Thanks, I will address all the comments in next version.
>
>>
>>  .../devicetree/bindings/mfd/qcom-pm8xxx.txt        |   1 +
>>  drivers/mfd/qcom-pm8xxx.c                          | 247 ++++++++++++++++++++-
>>  2 files changed, 238 insertions(+), 10 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"
>
> 8 < 9, so move it one step up please.
sure.. makes sense.
>
>>
>>  - #address-cells:
>>  	Usage: required
>> diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
> [..]
>> +#define	PM8821_SSBI_REG_ADDR_IRQ_BASE	0x100
>> +#define	PM8821_SSBI_REG_ADDR_IRQ_MASTER0 (PM8821_SSBI_REG_ADDR_IRQ_BASE + 0x30)
>> +#define	PM8821_SSBI_REG_ADDR_IRQ_MASTER1 (PM8821_SSBI_REG_ADDR_IRQ_BASE + 0xb0)
>> +#define	PM8821_SSBI_REG(m, b, offset) \
>> +			((m == 0) ? \
>> +			(PM8821_SSBI_REG_ADDR_IRQ_MASTER0 + b + offset) : \
>> +			(PM8821_SSBI_REG_ADDR_IRQ_MASTER1 + b + offset))
>> +#define	PM8821_SSBI_ADDR_IRQ_ROOT(m, b)		PM8821_SSBI_REG(m, b, 0x0)
>> +#define	PM8821_SSBI_ADDR_IRQ_CLEAR(m, b)	PM8821_SSBI_REG(m, b, 0x01)
>> +#define	PM8821_SSBI_ADDR_IRQ_MASK(m, b)		PM8821_SSBI_REG(m, b, 0x08)
>> +#define	PM8821_SSBI_ADDR_IRQ_RT_STATUS(m, b)	PM8821_SSBI_REG(m, b, 0x0f)
>
> I like how this cleaned up the rest of the implementation.
>
> [..]
>
>> +static void pm8821_irq_block_handler(struct pm_irq_chip *chip,
>> +				     int master, int block)
>> +{
>> +	int pmirq, irq, i, ret;
>> +	unsigned int bits;
>> +
>> +	ret = regmap_read(chip->regmap,
>> +			  PM8821_SSBI_ADDR_IRQ_ROOT(master, block), &bits);
>> +	if (ret) {
>> +		pr_err("Failed reading %d block ret=%d", block, ret);
>
> "Reading block %d failed ret=%d"
yep.
>
>> +		return;
>> +	}
>> +	if (!bits) {
>> +		pr_err("block bit set in master but no irqs: %d", block);
>
> This seems more like a debug thing, either showing missbehaving hardware
> or something odd in the driver. I think you should drop the print or
> make it pr_debug().
okay.
>
>> +		return;
>> +	}
>
> I would prefer that you just drop the entire if statement, it's just an
> corner case optimization with a info print.
>
i will have a closer look at this part once again.
>> +
>> +	/* Convert block offset to global block number */
>> +	block += (master * 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);
>> +		}
>> +	}
>> +
>
> Empty line
will fix all the empty lines in next version.
>
>> +}
>> +
>> +static inline void pm8821_irq_master_handler(struct pm_irq_chip *chip,
>> +					     int master, u8 master_val)
>> +{
>> +	int block;
>> +
>> +	for (block = 1; block < 8; block++)
>> +		if (master_val & BIT(block))
>> +			pm8821_irq_block_handler(chip, master, block);
>> +
>
> Empty line
>
>> +}
>> +
>> +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);
>> +	unsigned int master;
>> +	int ret;
>> +
>> +	chained_irq_enter(irq_chip, desc);
>> +	ret = regmap_read(chip->regmap,
>> +			  PM8821_SSBI_REG_ADDR_IRQ_MASTER0, &master);
>> +	if (ret) {
>> +		pr_err("Failed to re:Qad master 0 ret=%d\n", ret);
>                                       ^
> 				      |
> 			  I see you're using vim :)
>
>> +		return;
>> +	}
>> +
>> +	 /* bits 1 through 7 marks the first 7 blocks in master 0*/
>
> Indentation
>
>> +	if (master & GENMASK(7, 1))
>> +		pm8821_irq_master_handler(chip, 0, master);
>> +
>> +	 /* bit 0 marks if master 1 contains any bits */
>
> Dito
yep.
>
>> +	if (!(master & BIT(0)))
>> +		goto done;
>> +
>> +	ret = regmap_read(chip->regmap,
>> +			  PM8821_SSBI_REG_ADDR_IRQ_MASTER1, &master);
>> +	if (ret) {
>> +		pr_err("Failed to read master 1 ret=%d\n", ret);
>> +		return;
>
> "goto done;" so that we pass chained_irq_exit()
yes,
>
>> +	}
>> +
>> +	pm8821_irq_master_handler(chip, 1, master);
>> +
>> +done:
>> +	chained_irq_exit(irq_chip, desc);
>> +}
>> +
>
> [..]
>
>> +static void pm8821_irq_mask_ack(struct irq_data *d)
>> +{
>> +	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> +	unsigned int 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;
>> +
>> +	rc = regmap_update_bits(chip->regmap,
>> +				PM8821_SSBI_ADDR_IRQ_MASK(master, block),
>> +				BIT(irq_bit), BIT(irq_bit));
>> +
>
> Empty line
>
>> +	if (rc) {
>> +		pr_err("Failed to read/write mask IRQ:%d rc=%d\n", pmirq, rc);
>
> "Failed to mask IRQ %d rc=%d\n"
>
>> +		return;
>> +	}
>> +
>> +	rc = regmap_update_bits(chip->regmap,
>> +				PM8821_SSBI_ADDR_IRQ_CLEAR(master, block),
>> +				BIT(irq_bit), BIT(irq_bit));
>> +
>
> Empty line
>
>> +	if (rc) {
>> +		pr_err("Failed to read/write IT_CLEAR IRQ:%d rc=%d\n",
>> +								pmirq, rc);
>
> "Failed to clear IRQ %d rc=%d\n"
>
>> +	}
>> +
>
> Empty line
>
>> +}
>> +
>> +static void pm8821_irq_unmask(struct irq_data *d)
>> +{
>> +	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> +	unsigned int 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;
>> +
>> +	rc = regmap_update_bits(chip->regmap,
>> +				PM8821_SSBI_ADDR_IRQ_MASK(master, block),
>> +				BIT(irq_bit), ~BIT(irq_bit));
>> +
>
> Empty line
>
>> +	if (rc)
>> +		pr_err("Failed to read/write unmask IRQ:%d rc=%d\n", pmirq, rc);
>
> "Failed to unmask IRQ %d rc=%d\n"
will update it in next version.
>
>> +
>
> Empty line
>
>> +}
>> +
>> +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 rc, pmirq = irqd_to_hwirq(d);
>> +	u8 block, irq_bit, master;
>> +	unsigned int bits;
>> +
>> +	block = pmirq / 8;
>> +	master = block / PM8821_BLOCKS_PER_MASTER;
>> +	irq_bit = pmirq % 8;
>> +	block %= PM8821_BLOCKS_PER_MASTER;
>> +
>> +	rc = regmap_read(chip->regmap,
>> +		PM8821_SSBI_ADDR_IRQ_RT_STATUS(master, block), &bits);
>> +	if (rc) {
>> +		pr_err("Failed Reading Status rc=%d\n", rc);
>
> Odd capitalization, I suggest that you match it to the other functions
> by:
>
> "Reading status of IRQ %d failed rc=%d\n"
>
Okay
>> +		return rc;
>> +	}
>> +
>> +	*state = !!(bits & BIT(irq_bit));
>> +
>> +	return rc;
>> +}
>> +
>
> [..]
>
>>
>>  static int pm8xxx_probe(struct platform_device *pdev)
>>  {
>> +	const struct of_device_id *match;
>> +	const struct pm_irq_data *data;
>>  	struct regmap *regmap;
>>  	int irq, rc;
>>  	unsigned int val;
>>  	u32 rev;
>>  	struct pm_irq_chip *chip;
>> -	unsigned int nirqs = PM8XXX_NR_IRQS;
>> +
>> +	match = of_match_node(pm8xxx_id_table, pdev->dev.of_node);
>> +	if (!match) {
>> +		dev_err(&pdev->dev, "No matching driver data found\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	data = match->data;
>
> data = of_device_get_match_data(&pdev->dev); (from of_device.h)
This is good one.. I will use it in next version.

> Regards,
> Bjorn
>

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

[Bjorn Andersson]

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

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  arch/arm/boot/dts/qcom-apq8064.dtsi | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi
> index 268bd47..c61ba32 100644
> --- a/arch/arm/boot/dts/qcom-apq8064.dtsi
> +++ b/arch/arm/boot/dts/qcom-apq8064.dtsi
> @@ -627,6 +627,33 @@
>  			clock-names = "core";
>  		};
>  
> +		ssbi@c00000 {
> +			compatible = "qcom,ssbi";
> +			reg = <0x00c00000 0x1000>;
> +			qcom,controller-type = "pmic-arbiter";
> +
> +			pm8821: pmic@1 {
> +				compatible = "qcom,pm8821";
> +				interrupt-parent = <&tlmm_pinmux>;
> +				interrupts = <76 IRQ_TYPE_LEVEL_LOW>;
> +				#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 IRQ_TYPE_NONE>,
> +						     <25 IRQ_TYPE_NONE>,
> +						     <26 IRQ_TYPE_NONE>,
> +						     <27 IRQ_TYPE_NONE>;
> +					gpio-controller;
> +					#gpio-cells = <2>;
> +		                };
> +			};
> +		};
> +
>  		qcom,ssbi@500000 {
>  			compatible = "qcom,ssbi";
>  			reg = <0x00500000 0x1000>;
> -- 
> 2.10.1
>