Re: [PATCH V2 2/2] mfd: stmpe: Extend DT support in stmpe driver

[Lee Jones <lee.jones@linaro.org>]

On Thu, 22 Nov 2012, Viresh Kumar wrote:

> On 22 November 2012 21:16, Lee Jones <lee.jones@linaro.org> wrote:
> >> >> diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt
> >> >> +- irq-over-gpio: bool, true if gpio is used to get irq
> >> >> +- irq-gpios: gpio number over which irq will be requested (significant only if
> >> >> +  irq-over-gpio is true)
> >> >
> >> > You don't need these. Use gpio_to_irq() instead.
> >>
> >> I am passing gpio numbers here and am doing gpio_to_irq() in driver.
> >> Didn't get this one :(
> >
> > For a start you have 'irq-over-gpio' in the binding document and Device Tree
> > and 'irq_over_gpio' in the code. Has it even been tested?
> >
> > GPIOs are used as IRQ lines in many other previously DT:ed drivers. Take a
> > look to see how they are handled without adding unnecessary DT bindings.
> 
> I already knew it, should have picked that up. :(
> 
> >> stmpe is an interrupt controller for the IP's which are present inside
> >> it: gpio, adc.
> >> But interrupt lines for them are managed by stmpe driver internally. So should
> >> we really add interrupt-controller for it?
> >
> > You can't manage IRQ lines internally, you have to go through
> > the IRQ subsystem. When you request an IRQ via device tree you
> > will do so like this:
> 
> By that i meant, there is no external node which would have stmpe as
> interrupt controller. Because all of them would be its child node.
> 
> This is guaranteed because stmpe is an external device is present on board.
> So, it will have its entry in board dts file, and so wouldn't be
> scattered in different
> files.

It doesn't matter how it's wired up. 

If another node references it as it's IRQ controller you have to
declare it as one using the interrupt-controller binding.

> > The STMPE GPIO controller can't be used by Device Tree yet in any case,
> > because it doesn't have an IRQ domain. This is compulsory, or it won't
> > work. Have you tried to test this functionality yet?
> 
> I don't have SPEAr board to test it anymore. I have moved out of ST now and
> working in linaro as ARM asignee. Just pushing these as an part time activity.

Surely you can't push patches which haven't been tested?!

Try to get yourself some hardware. Does anyone near you have an HREF?

In the mean-time, I will write you an IRQ domain.

> Though ST guys would have tested stmpe, but stmpe-gpio, i am not sure about.

It needs to be tested before being accpeted.

> > I didn't go through them, but are you sure that:
> >
> > 1. Can I do without them?
> >    1.1 Can I derive the configuration from other things?
> >    2.2 Are they _really_ required, or am I just blindly copying platform data?
> > 2. Does a similar binding already exist?
> > 3. Can other drivers make use of them?
> >    3.1 If so, create a generic binding
> >    3.2 If not, prepend the binding with "<vendor>,"
> 
> I will go through them again.

Thanks.

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