Re: [PATCH v2 3/7] mfd: pm8921: Migrate to irqdomains

Lee Jones
2014-01-06T15:53:58Z

> Convert this driver to use irqdomains so that the PMIC's child
> devices can be converted to devicetree.
>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
> drivers/mfd/pm8921-core.c | 184 ++++++++++++++------------------------
> include/linux/mfd/pm8xxx/irq.h | 36 --------
> include/linux/mfd/pm8xxx/pm8921.h | 30 -------
> 3 files changed, 65 insertions(+), 185 deletions(-)
> delete mode 100644 include/linux/mfd/pm8xxx/irq.h
> delete mode 100644 include/linux/mfd/pm8xxx/pm8921.h
>

<snip>

> @@ -56,8 +56,7 @@
> struct pm_irq_chip {
> struct device *dev;
> spinlock_t pm_irq_lock;
> - unsigned int devirq;
> - unsigned int irq_base;
> + struct irq_domain *domain;

It's probably best to explicitly specify 'irqdomain' here in order to
eliminate any possibility of ambiguity.

> unsigned int num_irqs;
> unsigned int num_blocks;
> unsigned int num_masters;
> @@ -138,7 +137,7 @@ static int pm8xxx_irq_block_handler(struct pm_irq_chip *chip, int block)
> for (i = 0; i < 8; i++) {
> if (bits & (1 << i)) {
> pmirq = block * 8 + i;
> - irq = pmirq + chip->irq_base;
> + irq = irq_find_mapping(chip->domain, pmirq);

Going by this patch only, it appears you're calling irq_find_mapping()
before you've called irq_create_mapping(). This won't work, so unless
you've called the latter in a previous patch, you should ensure that
you do so.

> generic_handle_irq(irq);
> }
> }
> @@ -197,12 +196,11 @@ static void pm8xxx_irq_handler(unsigned int irq, struct irq_desc *desc)
> static void pm8xxx_irq_mask_ack(struct irq_data *d)
> {
> struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> - unsigned int pmirq = d->irq - chip->irq_base;
> - int master, irq_bit;
> + unsigned int pmirq = irqd_to_hwirq(d);

Why don't you call it hwirq instead of pmirq?

> + int irq_bit;
> u8 block, config;
>
> block = pmirq / 8;

Ah, I guess you're keeping the original naming convention.

> - master = block / 8;

What was the point in this anyway? Was it completely superfluous?

<snip>

> +static int pm8xxx_irq_init(struct platform_device *pdev, unsigned int irq)
> +{
> + struct pm_irq_chip *chip;
> + unsigned int nirqs = 256;

No magic numbers please.

> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip) + sizeof(u8) * nirqs,
> + GFP_KERNEL);

What does the sizeof(u8) add here?

<snip>

> + return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
> +}

Can't you use the MFD core instead?

--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog