On Wed, 24 Apr 2013, James Hogan wrote:
> Thanks for the review Thomas!
>
> On 23/04/13 16:09, Thomas Gleixner wrote:
> > On Tue, 23 Apr 2013, James Hogan wrote:
>> + spinlock_t lock;
> >
> > raw_spinlock_t please
>
> Okay.
>
> If I understand right, this would be because on RT, spinlock_t becomes a
> mutex and won't work correctly with irqs actually disabled for the irq
> callbacks below, is that right?
Yep.
> I'll look into this. kernel/irq/generic-chip.c was added after this
> driver was written.
Fair enough.
> >> +static void pdc_intc_perip_isr(unsigned int irq, struct irq_desc *desc)
> >> +{
> >> + struct pdc_intc_priv *priv;
> >> + unsigned int i, irq_no;
> >> +
> >> + priv = (struct pdc_intc_priv *)irq_desc_get_handler_data(desc);
> >> +
> >> + /* find the peripheral number */
> >> + for (i = 0; i < priv->nr_perips; ++i)
> >> + if (irq == priv->perip_irqs[i])
> >> + goto found;
> >
> > Whee. Aren't these numbers linear ?
>
> Not necessarily as they're virtual irq numbers obtained via
> platform_get_irq(), which come individually from device tree. Even their
> hardware irq numbers aren't linear as they're not wired to their irqchip
> in the same order:
> > pdc: pdc@0x02006000 {
> > interrupt-controller;
> > #interrupt-cells = <3>;
> >
> > reg = <0x02006000 0x1000>;
> > compatible = "img,pdc-intc";
> >
> > num-perips = <3>;
> > num-syswakes = <3>;
> >
> > interrupts = <18 4 /* level */>, /* Syswakes */
> > <30 4 /* level */>, /* Perip 0 (RTC) */
> > <29 4 /* level */>, /* Perip 1 (IR) */
> > <31 4 /* level */>; /* Perip 2 (WDT) */
> > };
Interesting.
> >> +static int pdc_intc_remove(struct platform_device *pdev)
> >> +{
> >> + struct pdc_intc_priv *priv = platform_get_drvdata(pdev);
> >> +
> >> + irq_domain_remove(priv->domain);
> >
> > And the rest of the resources is still there?
>
> I was under the impression devm_kzalloc and devm_ioremap took care of
> that in both the probe error case and the remove case:
> > * devm_kzalloc - Resource-managed kzalloc
> > * Managed kzalloc. Memory allocated with this function is
> > * automatically freed on driver detach.
> >
> > * devm_ioremap - Managed ioremap()
> > * Managed ioremap(). Map is automatically unmapped on driver detach.
>
> I may have misunderstood the whole point of their existence though?
No, that was just me missing the devm_ in front of
kzalloc/ioremap. So you're good.
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
