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

From: Bjorn Andersson
Date: Tue Nov 08 2016 - 14:08:05 EST

Next message: Stephen Warren: "Re: [PATCH] gpio: tegra186: Add support for T186 GPIO"
Previous message: Mark Rutland: "KASAN & the vmalloc area"
In reply to: Srinivas Kandagatla: "Re: [PATCH 2/2] ARM: dts: apq8064: add support to pm8821"
Next in thread: Srinivas Kandagatla: "Re: [PATCH 1/2] mfd: pm8921: add support to pm8821"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxx>
> ---
> 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

Next message: Stephen Warren: "Re: [PATCH] gpio: tegra186: Add support for T186 GPIO"
Previous message: Mark Rutland: "KASAN & the vmalloc area"
In reply to: Srinivas Kandagatla: "Re: [PATCH 2/2] ARM: dts: apq8064: add support to pm8821"
Next in thread: Srinivas Kandagatla: "Re: [PATCH 1/2] mfd: pm8921: add support to pm8821"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]