Re: [PATCH] irqchip/renesas-intc-irqpin: Improve clock error handling and reporting

[Geert Uytterhoeven]

Hi Jason,

On Tue, Nov 24, 2015 at 4:19 PM, Jason Cooper <jason@xxxxxxxxxxxxxx> wrote:
> On Tue, Nov 24, 2015 at 04:08:13PM +0100, Geert Uytterhoeven wrote:
>> If the Renesas External IRQ Pin driver cannot find a functional clock,
>> it prints a warning, .e.g.
>>
>>     renesas_intc_irqpin fe78001c.interrupt-controller: unable to get clock
>>
>> and continues, as the clock is optional, depending on the SoC type.
>> This warning may confuse users.
>>
>> To fix this, add a flag to indicate that the clock is mandatory or
>> optional, and add a few more compatible entries:
>>   - If the clock is mandatory (on R-Mobile A1 or SH-Mobile AG5), a
>>     missing clock is now treated as a fatal error,
>>   - If the clock is optional (on R-Car Gen1, or using the generic
>>     "renesas,intc-irqpin" compatible value), the warning is no longer
>>     printed.
>>
>> This requires making struct intc_irqpin_irlm_config more generic by
>> renaming it to intc_irqpin_config, and adding a flag to indicate if IRLM
>> is needed.
>> The new clock flag is merged with the existing shared_irqs boolean into
>> a bitfield to save space.
>>
>> Suggested-by: Magnus Damm <magnus.damm@xxxxxxxxx>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>> ---
>> Tested on
>>   - r8a7779/marzen (no functional clock),
>>   - r8a7740/armadillo (with and without mandatory functional clock),
>>   - sh73a0/kzm9g (with and without mandatory functional clock).
>>
>>  drivers/irqchip/irq-renesas-intc-irqpin.c | 45 ++++++++++++++++++++++---------
>>  1 file changed, 33 insertions(+), 12 deletions(-)
>
> I'll pick up this a the previous series you just sent (fyi: tglx).

Thanks!

>> diff --git a/drivers/irqchip/irq-renesas-intc-irqpin.c b/drivers/irqchip/irq-renesas-intc-irqpin.c
>> index 7f6cf19aa6ac00d7..713177d97c7aa0b6 100644
>> --- a/drivers/irqchip/irq-renesas-intc-irqpin.c
>> +++ b/drivers/irqchip/irq-renesas-intc-irqpin.c
>> @@ -79,12 +79,15 @@ struct intc_irqpin_priv {
>>       struct irq_chip irq_chip;
>>       struct irq_domain *irq_domain;
>>       struct clk *clk;
>> -     bool shared_irqs;
>> +     unsigned shared_irqs:1;
>> +     unsigned needs_clk:1;
>>       u8 shared_irq_mask;
>>  };
>>
>> -struct intc_irqpin_irlm_config {
>> +struct intc_irqpin_config {
>>       unsigned int irlm_bit;
>> +     unsigned needs_irlm:1;
>> +     unsigned needs_clk:1;
>>  };
>
> Why not just stick with bool?  Changing shared_irqs -> has_shared_irqs?

That's another option. It depends on how many flags we will add in the
future...

>>  static unsigned long intc_irqpin_read32(void __iomem *iomem)
>> @@ -359,8 +362,15 @@ static const struct irq_domain_ops intc_irqpin_irq_domain_ops = {
>>       .xlate  = irq_domain_xlate_twocell,
>>  };
>>
>> -static const struct intc_irqpin_irlm_config intc_irqpin_irlm_r8a777x = {
>> +static const struct intc_irqpin_config intc_irqpin_irlm_r8a777x = {
>>       .irlm_bit = 23, /* ICR0.IRLM0 */
>> +     .needs_irlm = 1,
>> +     .needs_clk = 0,
>> +};
>> +
>> +static const struct intc_irqpin_config intc_irqpin_rmobile = {
>> +     .needs_irlm = 0,
>> +     .needs_clk = 1,
>>  };
>>
>>  static const struct of_device_id intc_irqpin_dt_ids[] = {
>> @@ -369,12 +379,17 @@ static const struct of_device_id intc_irqpin_dt_ids[] = {
>>         .data = &intc_irqpin_irlm_r8a777x },
>>       { .compatible = "renesas,intc-irqpin-r8a7779",
>>         .data = &intc_irqpin_irlm_r8a777x },
>> +     { .compatible = "renesas,intc-irqpin-r8a7740",
>> +       .data = &intc_irqpin_rmobile },
>> +     { .compatible = "renesas,intc-irqpin-sh73a0",
>> +       .data = &intc_irqpin_rmobile },
>>       {},
>>  };
>>  MODULE_DEVICE_TABLE(of, intc_irqpin_dt_ids);
>
> Add these compatibles and the clock requirement info to the devicetree
> binding?

Already there, cfr.
Documentation/devicetree/bindings/interrupt-controller/renesas,intc-irqpin.txt
The clock being mandatory just wasn't enforced.

>> @@ -500,10 +521,10 @@ static int intc_irqpin_probe(struct platform_device *pdev)
>>
>>       /* scan for shared interrupt lines */
>>       ref_irq = p->irq[0].requested_irq;
>> -     p->shared_irqs = true;
>> +     p->shared_irqs = 1;
>>       for (k = 1; k < nirqs; k++) {
>>               if (ref_irq != p->irq[k].requested_irq) {
>> -                     p->shared_irqs = false;
>> +                     p->shared_irqs = 0;
>>                       break;
>>               }
>>       }
>
> The bool suggestion, above, would make this easier to read imo.

I can respin the patch if you prefer bool.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/