linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Viresh Kumar <viresh.kumar@linaro.org>
To: Lee Jones <lee.jones@linaro.org>
Cc: sameo@linux.intel.com, devicetree-discuss@lists.ozlabs.org,
	linux-kernel@vger.kernel.org, spear-devel@list.st.com,
	Vipul Kumar Samar <vipulkumar.samar@st.com>
Subject: Re: [PATCH V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
Date: Thu, 22 Nov 2012 22:31:48 +0530	[thread overview]
Message-ID: <CAKohpok6v65TH5yZAthyxcH7bCVgF8A8m2bY_mp3yS_Y_yH1Xw@mail.gmail.com> (raw)
In-Reply-To: <20121122154612.GC10986@gmail.com>

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.

> 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.

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

> Okay, I've just had a look at my tested bindings.
>
> We already have these:
>
>   debounce-interval   /* This is a generic binding */
>   st,scan-count       /* These are vendor specific */
>   st,no-autorepeat    /*            "              */
>
> Vendor specific bindings should be "<vendor>,<binding>", rather than
> "<type_of_driver>,<binding>"

Will take care of these.

>> >> +             reg = <0>;
>> >
>> > You have reg twice here. Also reg should never be '0'.
>>
>> For SPI, there are chip selects and there is no reg offset.
>
> I understand the addressing issues, but you have 'reg' twice.
>
> Which one should be used?

I haven't replied on that, because it was accepted. Only one
definition should be there for reg.

> 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.

>> >> diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
>> Because i wanted to keep all DT stuff together. Obviously i have seen keypad
>> driver earlier :)
>
> If you had, you'd realise that these bindings already exist. ;)

Ok. I had seen it during my V1 and missed these bindings. will fix them now.

--
viresh

  reply	other threads:[~2012-11-22 18:51 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-11-22  5:10 [PATCH V2 1/2] mfd: stmpe: Use devm_*() routines Viresh Kumar
2012-11-22  5:10 ` [PATCH V2 2/2] mfd: stmpe: Extend DT support in stmpe driver Viresh Kumar
2012-11-22 11:24   ` Lee Jones
2012-11-22 13:54     ` Viresh Kumar
2012-11-22 15:46       ` Lee Jones
2012-11-22 17:01         ` Viresh Kumar [this message]
2012-11-23  3:45           ` Shiraz Hashim
2012-11-23  9:33             ` Lee Jones
2012-11-23  9:23           ` Lee Jones
2012-11-22 18:31     ` Viresh Kumar
2012-11-23  9:39       ` Lee Jones
2012-11-23  4:29     ` Viresh Kumar
2012-11-23  9:36       ` Lee Jones
2012-11-23 12:39         ` Viresh Kumar
2012-11-23 15:43           ` Lee Jones
2012-11-23 15:45             ` Viresh Kumar
2012-11-22 10:27 ` [PATCH V2 1/2] mfd: stmpe: Use devm_*() routines Lee Jones
2012-11-22 10:30   ` Viresh Kumar
2012-11-22 17:10   ` Viresh Kumar
2012-11-23  9:40     ` Lee Jones
2012-11-23  9:42       ` Lee Jones
2012-11-23  9:44       ` Viresh Kumar
2012-11-23 11:03 ` Samuel Ortiz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAKohpok6v65TH5yZAthyxcH7bCVgF8A8m2bY_mp3yS_Y_yH1Xw@mail.gmail.com \
    --to=viresh.kumar@linaro.org \
    --cc=devicetree-discuss@lists.ozlabs.org \
    --cc=lee.jones@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=sameo@linux.intel.com \
    --cc=spear-devel@list.st.com \
    --cc=vipulkumar.samar@st.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).