On Thursday 22 August 2013 at 01:10:27, Stephen Warren wrote:
> On 08/21/2013 03:49 PM, Tomasz Figa wrote:
> >> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> >>
> >> +static void of_gpio_scan_irq_lines(const struct device_node *const
> >>
> >> + for (i = 0; i < intlen; i += intsize) {
> >> + /*
> >> + * Find out the local IRQ number. This corresponds to
> >> + * the GPIO line offset for a GPIO chip.
> >
> > I'm still not convinced that this assumption is correct. This code
> > will behave erraticaly in cases where it is not true, requesting
> > innocent GPIO pins.
Do you have an idea how we can destroy your doubts?
Either irq_chips nor irq_domains provide some sort of translation function
for this.
Is there a driver in the kernel that has different gpio- vs. irq-namespaces
where I can have a look at? How does platform code solve this translation?
The irq_to_gpio functions in include/linux/gpio.h seem deprecated. They
just return -EINVAL.
For me it seems, that there is no such device inside the kernel yet.
Correct me if I'm wrong. If such a device comes to surface, we're in
trouble. We will need some device-specific translation function then.
Is it the time to introduce an additional pointer for such a function now
and nobody uses it? Or wait until such a device arises and introduce the
pointer then?
> >> + */
> >> + if (irq_domain && irq_domain->ops->xlate)
> >> + irq_domain->ops->xlate(irq_domain, gcn,
> >> + intspec + i, intsize,
> >> + &hwirq, &type);
> >> + else
> >> + hwirq = intspec[0];
> >
> > Is it a correct fallback when irq_domain is NULL?
>
> Indeed this fallback is dangerous. The /only/ way to parse an IRQ
> specifier is with binding-specific knowledge, which is obtained by
> calling irq_domain->ops->xlate(). If the IRQ domain can't be found, this
> operation simply has to be deferred; we can't just guess and hope.
At least the of irq mapping code make this assumption also:
kernel/irq/irqdomain.c:483
It should be valid for us here too.
The additional assumption that I made is that if irq_domain == NULL (not
only xlate), that we can use intspec[0] either.
> >> +
> >> + hwirq = be32_to_cpu(hwirq);
> >
> > Is this conversion correct? I don't think hwirq could be big endian
> > here (unless running on a big endian CPU).
>
> I think that should be inside the else branch above.
No it has to be in both branches as it is. Device tree data is big endian.
The conversion is converting big endian data (from device tree in both
cases) to cpu endianess and not coverting TO big endian.
My test machine is a arm in little endian mode and it provided wrong values
if I did not do the conversion.
What I am a bit unsure about is if the xlate function is expecting the
intspec pointer to point to big endian device tree data or data already
converted to cpu endianess. For the standard xlate functions
irq_domain_xlate_[one|two|onetwo]cell it does not matter.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
