Re: [PATCH] gpio: add TS-5500 DIO headers support
On Thu, Dec 6, 2012 at 4:49 AM, Vivien Didelot wrote: > I looked at some drivers and if I'm not mistaken, this case is > different. Technologic Systems platforms (such as the TS-5500) have > several pin blocks. Each block has input-only, input-output or > output-only pins. Only one pin per block is connected to an interrupt > line. But sadly, these interrupt-connected lines are input only. > Here are the details about the TS-5500 pin block "DIO1": > > http://wiki.embeddedarm.com/wiki/TS-5500#DIO1_Header Aha I get it now. How odd ... OK we need to support this too. > That's why I previously used a dio1_irq platform data field, to return > the interrupt connected to the IRQ-able pin for any GPIO on DIO1, in the > gpio_to_irq() implementation. OK I get it. > A Linux IRQ per pin doesn't seem to be possible because the > irq_create_mapping() documentation says that "Only one mapping per > hardware interrupt is permitted." Should I still implement the > irq_chip/irqdomain for a single IRQ per block? For each pin? > What do you think about this implementation? So basically there are in total three pins and yeah this is too little and too strange to warrant an irqdomain so go ahead with it as it looks and fix the other comments. I'm happy with keeping this now. Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio: add TS-5500 DIO headers support
Hi Linus,
I rewrote some parts according to your comments, but I still have some
concerns.
On Fri, 2012-10-12 at 22:53 +0200, Linus Walleij wrote:
> >> (...)
> >> > +static int ts5500_gpio_to_irq(struct gpio_chip *chip, unsigned
> offset)
> >> > +{
> >> > + const struct ts5500_dio line = ts5500_dios[offset];
> >> > +
> >> > + /* Only a few lines are IRQ-capable */
> >> > + if (line.irq != NO_IRQ)
> >> > + return line.irq;
> >> > +
> >> > + /* This allows to bridge a line with the IRQ line of the
> same header */
> >> > + if (dio1_irq && offset < 13)
> >> > + return ts5500_dios[13].irq;
> >> > + if (dio2_irq && offset > 13 && offset < 26)
> >> > + return ts5500_dios[26].irq;
> >> > + if (lcd_irq && offset > 26 && offset < 37)
> >> > + return ts5500_dios[37].irq;
> >>
> >> Don't do this. Please use irqdomain for converting physical
> >> IRQ numbers to Linux IRQ numbers. (Consult other GPIO
> >> drivers for examples.)
> >>
> >> These magic constants (13, 26, 37) are scary too.
> >>
> >> You should not try to handle each block as a single
> >> IRQ, instead instatiate a struct irq_chip in the driver
> >> and let that use irqdomain do demux the IRQ and
> >> register a range of Linux IRQ numbers, on per pin,
> >> so the GPIO driver will handle the physical IRQ line,
> >> then dispatch to a fan-out handler, so drivers that need
> >> IRQs from the GPIO chip just register IRQ handlers like
> >> anyone else.
> >
> > Do you mean that I should not return the same IRQ line for the same
> > header, but virtual ones? I'll try to find a good example for that.
>
> Basically Linux IRQs (also sometimes called virtual IRQs) are
> separate from the physical IRQ numbers of the system.
>
> i.e. what you see in /proc/interrupts has no relation to the physical
> interrupt lines.
>
> Keep in mind that we're trying to disallow IRQ 0 altogether and some
> platforms use that physical line for stuff.
>
> So we need to use irqdomain to just grab an IRQ number to reference
> the physical line. And we often do that for the IRQ controller.
>
> The fact that sometimes the physical line number and the Linux
> IRQ number correspond is just misleading...
>
> In this case, since you have individual IRQs you want to check
> for different lines, register something with e.g.
> irq_domain_add_simple() to handle all these lines as IRQs.
>
> It's a bit complex but pays off: all of a sudden you get statistics
> in /proc/interrupts for exactly which GPIO IRQs were fired,
> for example, and they get names if you provide that.
>
> Look at the other GPIO drivers for many good examples of
> how to do this. gpio-em.c is one example.
I looked at some drivers and if I'm not mistaken, this case is
different. Technologic Systems platforms (such as the TS-5500) have
several pin blocks. Each block has input-only, input-output or
output-only pins. Only one pin per block is connected to an interrupt
line. But sadly, these interrupt-connected lines are input only.
Here are the details about the TS-5500 pin block "DIO1":
http://wiki.embeddedarm.com/wiki/TS-5500#DIO1_Header
Some GPIO devices need a bidirectional data line which can trigger an
IRQ. In this case, we use a bidirectional pin for data, that we strap to
the IRQ-able pin.
Basically, our setup looks like that:
+---+ in-only+IRQ
| D |-+ data ++
| I | in/out pin |---| GPIO |
| O |-+ clk | device |
| 1 |-|(SHT15) |
+---+ in/out pin ++
That's why I previously used a dio1_irq platform data field, to return
the interrupt connected to the IRQ-able pin for any GPIO on DIO1, in the
gpio_to_irq() implementation.
A Linux IRQ per pin doesn't seem to be possible because the
irq_create_mapping() documentation says that "Only one mapping per
hardware interrupt is permitted." Should I still implement the
irq_chip/irqdomain for a single IRQ per block? For each pin?
What do you think about this implementation?
Yours,
Vivien
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio: add TS-5500 DIO headers support
On Sat, Oct 13, 2012 at 12:04 AM, Vivien Didelot wrote: > About the generic driver (to allow registering one platform device per > DIO block), I think it won't be possible, because there are shared > regions, such as 0x7d, used by DIO2 and LCD DIO for direction... Probably true. > Is there a way to share this, or does this mean that this driver should > handle the 3 blocks as it already does? I think you should try to handle all 3 for the moment atleast. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio: add TS-5500 DIO headers support
Hi Linus, On Fri, 2012-10-12 at 22:53 +0200, Linus Walleij wrote: > Well that may also be a pretty big step if you just want to mux > one bank of GPIO. I'm a bit ambivalent. But if you want to tie > pin and gpio information together and name all pins, pinctrl > is what should suit you best. > > In the GPIO world, things are opaque "gpios" not really pins. Thanks a lot for all your comments. I think I'll stick with the GPIO framework for the moment, trying to keep it as simple as possible. About the generic driver (to allow registering one platform device per DIO block), I think it won't be possible, because there are shared regions, such as 0x7d, used by DIO2 and LCD DIO for direction... Is there a way to share this, or does this mean that this driver should handle the 3 blocks as it already does? Thanks, Vivien -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio: add TS-5500 DIO headers support
On Mon, Oct 8, 2012 at 8:20 PM, Vivien Didelot
wrote:
> On Mon, 2012-10-08 at 12:38 +0200, Linus Walleij wrote:
>> On Wed, Sep 26, 2012 at 2:42 AM, Vivien Didelot
>> wrote:
>> Part of me dislike that you create one single driver for all
>> three blocks instead of abstracting the driver to handle one
>> block, and register one platform device each, 2-3 times.
>> But given that this is very tied to this one chip it could be the
>> simplest and most readable design so OK...
>
> I agree with you. I thought about that too and it will make it easier to
> support other similar platform DIO headers (such as the ones on the
> TS-5600). But I'm not sure about the implementation, I'm thinking about
> an array of struct ts5500_dio per block, described in a
> MODULE_DEVICE_TABLE. Something like:
>
> static struct platform_device_id ts5500_device_ids[] = {
> { "ts5500-dio1", (kernel_ulong_t) ts5500_dio1 },
> { "ts5500-dio2", (kernel_ulong_t) ts5500_dio2 },
> { "ts5500-lcd", (kernel_ulong_t) ts5500_lcd },
> { }
> };
> MODULE_DEVICE_TABLE(platform, ts5500_device_ids);
>
> What do you think?
It basically looks pretty nice, but can't tell until I see the entire
code...
>> > +static const char * const ts5500_pinout[38] = {
>> > + /* DIO1 Header (offset 0-13) */
>> > + [0] = "DIO1_0", /* pin 1 */
>> > + [1] = "DIO1_1", /* pin 3 */
>>
>> Pins pins pins. The pinctrl subsystem has a framework for keeping track of
>> pins and giving them names, which is another reason to convert this to a
>> pinctrl driver.
>>
>> Please consult Documentation/pinctrl.txt
>
> Sounds better then, thanks.
Well that may also be a pretty big step if you just want to mux
one bank of GPIO. I'm a bit ambivalent. But if you want to tie
pin and gpio information together and name all pins, pinctrl
is what should suit you best.
In the GPIO world, things are opaque "gpios" not really pins.
>> > +#ifndef NO_IRQ
>> > +#define NO_IRQ -1
>> > +#endif
>>
>> No. We have very solidly decided that NO_IRQ == 0.
>
> Ho ok, I didn't know that. Many implementations still use -1.
They are bad examples not to be followed.
http://lwn.net/Articles/470820/
>> > +/*
>> > + * This structure is used to describe capabilities of DIO lines,
>> > + * such as available directions, and mapped IRQ (if any).
>> > + */
>> > +struct ts5500_dio {
>> > + const unsigned long value_addr;
>> > + const int value_bit;
>> > + const unsigned long control_addr;
>> > + const int control_bit;
>>
>> All the above seem like they should be u8 rather than
>> const unsigned or const int.
>
> I used these types here to match {set,clear}_bit function prototypes.
> But I can use const u8 for sure.
Hm hm yes that is true. These functions only work on longs,
as they access the entire word from memory.
If you access these as u8, use direct operators rather than
set/clear_bit & friends.
No big deal really. Do it as it works best for you.
>> > + const int irq;
>>
>> IRQ numbers shall not be hard-coded into drivers like
>> this, they shall be passed in as resources from the outside
>> just like you pass in other platform data.
>
> Just curious, what's the point of doing this, as IRQ lines are wired?
It's a design pattern that only the platform knows about the IRQs
and they should be retrieved from there.
Resources tied to platform device is one way to get them, if they are
hardcoded.
Device Tree is another way on MIPS e.g.
And ACPI etc is used in some x86's I think.
Just that this kind of stuff which basically pertains to how the interrupt
controller is wired rather than this hardware block per se, means it
should be external.
>> > + const int direction;
>>
>> Use a bool out for this last thing?
>
> Or two bool in and out instead?
It if can be in and out at the same time, sure.
I'm probably just not getting it.
>> > +};
>> > +
>> > +static const struct ts5500_dio ts5500_dios[] = {
>> > + /* DIO1 Header (offset 0-13) */
>> > + [0] = { 0x7b, 0, 0x7a, 0, NO_IRQ, IN | OUT },
>>
>> Use C99 syntax and skip the [0] index (etc).
>
> I used this syntax because it is much more clearer to figure out what
> Linux offset corresponds to the physical DIO, but well...
OK you can keep the offset if you like no big deal.
>> static const struct ts5500_dio ts5500_dios[] = {
>> {
>> .value_addr = 0x7b,
>> .value_bit = 0,
>> .control_addr = 0x7a,
>> .control_bit = 0,
>> .direction = OUT,
>> },
>> {
>> .value_addr = 0x7b
>>
>>
>> Note absence of NO_IRQ - it's implicit zero in static
>> allocations.
>
> This will make the code longer, but this is more explicit.
> I should maybe group addr/bit pairs in a struct...
I really prefer if you do this because it makes the code way more
maintainable, because else, if you add a member to the struct
in the middle of the others, guess what happens.
>>
Re: [PATCH] gpio: add TS-5500 DIO headers support
Hi Linus,
On Mon, 2012-10-08 at 12:38 +0200, Linus Walleij wrote:
> On Wed, Sep 26, 2012 at 2:42 AM, Vivien Didelot
> wrote:
>
> > The Technologic Systems TS-5500 platform provides 3 digital I/O headers:
> > DIO1, DIO2, and the LCD port, that may be used as a DIO header.
> >
> > Signed-off-by: Vivien Didelot
> > Signed-off-by: Jerome Oufella
> (...)
> > + * The TS-5500 platform has 38 Digital Input/Output lines (DIO), exposed
> > by 3
> > + * DIO headers: DIO1, DIO2, and the LCD port which may be used as a DIO
> > header.
>
> Header? Is that like a socket or something?
Digital Input/Output header is the term given in the platform datasheet
to describe a block. I can use "block" if it's more explicit.
>
> The ability to swith the LCD port into DIO is a pin control property
> and if this gets implemented the driver should technically be
> in drivers/pinctrl. (It can implement GPIO too, no problem.)
>
> Part of me dislike that you create one single driver for all
> three blocks instead of abstracting the driver to handle one
> block, and register one platform device each, 2-3 times.
> But given that this is very tied to this one chip it could be the
> simplest and most readable design so OK...
I agree with you. I thought about that too and it will make it easier to
support other similar platform DIO headers (such as the ones on the
TS-5600). But I'm not sure about the implementation, I'm thinking about
an array of struct ts5500_dio per block, described in a
MODULE_DEVICE_TABLE. Something like:
static struct platform_device_id ts5500_device_ids[] = {
{ "ts5500-dio1", (kernel_ulong_t) ts5500_dio1 },
{ "ts5500-dio2", (kernel_ulong_t) ts5500_dio2 },
{ "ts5500-lcd", (kernel_ulong_t) ts5500_lcd },
{ }
};
MODULE_DEVICE_TABLE(platform, ts5500_device_ids);
What do you think?
>
> > + * The datasheet is available at:
> > + * http://embeddedx86.com/documentation/ts-5500-manual.pdf.
>
> Drop the period after the URL. It makes it hard to browse...
>
> > +/*
> > + * This array describes the names of the DIO lines, but also the mapping
> > between
> > + * the datasheet, and corresponding offsets exposed by the driver.
> > + */
> > +static const char * const ts5500_pinout[38] = {
> > + /* DIO1 Header (offset 0-13) */
> > + [0] = "DIO1_0", /* pin 1 */
> > + [1] = "DIO1_1", /* pin 3 */
>
> Pins pins pins. The pinctrl subsystem has a framework for keeping track of
> pins and giving them names, which is another reason to convert this to a
> pinctrl driver.
>
> Please consult Documentation/pinctrl.txt
Sounds better then, thanks.
>
> > +#define IN (1 << 0)
> > +#define OUT(1 << 1)
>
> Why not use a bool named "out" then it's out if true
> else input.
Because there are 3 possible cases: input-only, input-output and
output-only. Would it be better to have 2 booleans, "in" and "out"?
>
> > +#ifndef NO_IRQ
> > +#define NO_IRQ -1
> > +#endif
>
> No. We have very solidly decided that NO_IRQ == 0.
Ho ok, I didn't know that. Many implementations still use -1.
>
> > +/*
> > + * This structure is used to describe capabilities of DIO lines,
> > + * such as available directions, and mapped IRQ (if any).
> > + */
> > +struct ts5500_dio {
> > + const unsigned long value_addr;
> > + const int value_bit;
> > + const unsigned long control_addr;
> > + const int control_bit;
>
> All the above seem like they should be u8 rather than
> const unsigned or const int.
I used these types here to match {set,clear}_bit function prototypes.
But I can use const u8 for sure.
>
> > + const int irq;
>
> IRQ numbers shall not be hard-coded into drivers like
> this, they shall be passed in as resources from the outside
> just like you pass in other platform data.
Just curious, what's the point of doing this, as IRQ lines are wired?
>
> > + const int direction;
>
> Use a bool out for this last thing?
Or two bool in and out instead?
>
> > +};
> > +
> > +static const struct ts5500_dio ts5500_dios[] = {
> > + /* DIO1 Header (offset 0-13) */
> > + [0] = { 0x7b, 0, 0x7a, 0, NO_IRQ, IN | OUT },
>
> Use C99 syntax and skip the [0] index (etc).
I used this syntax because it is much more clearer to figure out what
Linux offset corresponds to the physical DIO, but well...
>
> static const struct ts5500_dio ts5500_dios[] = {
> {
> .value_addr = 0x7b,
> .value_bit = 0,
> .control_addr = 0x7a,
> .control_bit = 0,
> .direction = OUT,
> },
> {
> .value_addr = 0x7b
>
>
> Note absence of NO_IRQ - it's implicit zero in static
> allocations.
This will make the code longer, but this is more explicit.
I should maybe group addr/bit pairs in a struct...
>
> > +static bool lcd_dio;
> > +static bool lcd_irq;
> > +static bool dio1_irq;
> > +static bool dio2_irq;
>
> Document what these are for with some /* co
Re: [PATCH] gpio: add TS-5500 DIO headers support
On Wed, Sep 26, 2012 at 2:42 AM, Vivien Didelot
wrote:
> The Technologic Systems TS-5500 platform provides 3 digital I/O headers:
> DIO1, DIO2, and the LCD port, that may be used as a DIO header.
>
> Signed-off-by: Vivien Didelot
> Signed-off-by: Jerome Oufella
(...)
> + * The TS-5500 platform has 38 Digital Input/Output lines (DIO), exposed by 3
> + * DIO headers: DIO1, DIO2, and the LCD port which may be used as a DIO
> header.
Header? Is that like a socket or something?
The ability to swith the LCD port into DIO is a pin control property
and if this gets implemented the driver should technically be
in drivers/pinctrl. (It can implement GPIO too, no problem.)
Part of me dislike that you create one single driver for all
three blocks instead of abstracting the driver to handle one
block, and register one platform device each, 2-3 times.
But given that this is very tied to this one chip it could be the
simplest and most readable design so OK...
> + * The datasheet is available at:
> + * http://embeddedx86.com/documentation/ts-5500-manual.pdf.
Drop the period after the URL. It makes it hard to browse...
> +/*
> + * This array describes the names of the DIO lines, but also the mapping
> between
> + * the datasheet, and corresponding offsets exposed by the driver.
> + */
> +static const char * const ts5500_pinout[38] = {
> + /* DIO1 Header (offset 0-13) */
> + [0] = "DIO1_0", /* pin 1 */
> + [1] = "DIO1_1", /* pin 3 */
Pins pins pins. The pinctrl subsystem has a framework for keeping track of
pins and giving them names, which is another reason to convert this to a
pinctrl driver.
Please consult Documentation/pinctrl.txt
> +#define IN (1 << 0)
> +#define OUT(1 << 1)
Why not use a bool named "out" then it's out if true
else input.
> +#ifndef NO_IRQ
> +#define NO_IRQ -1
> +#endif
No. We have very solidly decided that NO_IRQ == 0.
> +/*
> + * This structure is used to describe capabilities of DIO lines,
> + * such as available directions, and mapped IRQ (if any).
> + */
> +struct ts5500_dio {
> + const unsigned long value_addr;
> + const int value_bit;
> + const unsigned long control_addr;
> + const int control_bit;
All the above seem like they should be u8 rather than
const unsigned or const int.
> + const int irq;
IRQ numbers shall not be hard-coded into drivers like
this, they shall be passed in as resources from the outside
just like you pass in other platform data.
> + const int direction;
Use a bool out for this last thing?
> +};
> +
> +static const struct ts5500_dio ts5500_dios[] = {
> + /* DIO1 Header (offset 0-13) */
> + [0] = { 0x7b, 0, 0x7a, 0, NO_IRQ, IN | OUT },
Use C99 syntax and skip the [0] index (etc).
static const struct ts5500_dio ts5500_dios[] = {
{
.value_addr = 0x7b,
.value_bit = 0,
.control_addr = 0x7a,
.control_bit = 0,
.direction = OUT,
},
{
.value_addr = 0x7b
Note absence of NO_IRQ - it's implicit zero in static
allocations.
> +static bool lcd_dio;
> +static bool lcd_irq;
> +static bool dio1_irq;
> +static bool dio2_irq;
Document what these are for with some /* comments */
But overall this has a singleton problem, this driver is not reentrant.
Usually that doesn't matter on a practical system, but we sure
prefer if you create a state container:
struct ts5500dio_state {
bool lcd_dio;
bool lcd_irq;
bool dio1_irq;
bool dio2_irq;
}
In .probe:
struct ts5500dio_state *my_state = devm_kzalloc(&dev, sizeof(struct
foo_state), GFP_KERNEL);
my_state->lcd_dio = ...
etc.
> +static DEFINE_SPINLOCK(lock);
This would also go into the state container.
> +static inline void io_set_bit(int bit, unsigned long addr)
io_set_bit() is too generic, use something like ts5500dio_set_bit()
> +{
> + unsigned long val = inb(addr);
I'm not familiar with inb() and friends, I guess it's IO-space
accessors so I just buy into it...
> + __set_bit(bit, &val);
Do you have to use the __ variants? Isn't just set_bit() working?
(Just checking...)
> + outb(val, addr);
> +}
> +
> +static inline void io_clear_bit(int bit, unsigned long addr)
> +{
> + unsigned long val = inb(addr);
> + __clear_bit(bit, &val);
> + outb(val, addr);
> +}
Same comments.
> +static int ts5500_gpio_input(struct gpio_chip *chip, unsigned offset)
> +{
> + unsigned long flags;
> + const struct ts5500_dio line = ts5500_dios[offset];
> +
> + /* Some lines cannot be configured as input */
> + if (!(line.direction & IN))
> + return -ENXIO;
Why are you using that return code? (Probably OK, just want
a rationale...)
(...)
> +static int ts5500_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> + const struct ts5500_dio line = ts5500_dios[offset];
> +
> + return (inb(line.value_addr) >> line.value_bit) & 1;
Use this idiom:
return !!(inb(line.value_addr) & line.value_bit);
I
Re: [PATCH] gpio: add TS-5500 DIO headers support
On Fri, Oct 5, 2012 at 1:18 AM, Vivien Didelot wrote: > Grant, Linus, any feedback? On the entire driver or on the config fragment? The latter looks OK, I'll look at the driver per se now. (I was busy with the merge window you know...) Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio: add TS-5500 DIO headers support
Hi, Grant, Linus, any feedback? Thanks, Vivien On Wed, 2012-09-26 at 11:37 -0400, Vivien Didelot wrote: > > +config GPIO_TS5500 > > + tristate "TS-5500 DIO Headers" > > + depends on TS5500 > > + help > > + This driver supports the 3 Digital I/O headers of the > Technologic > > + Systems TS-5500 platform: DIO1, DIO2, and the LCD port which > may be > > + used as a DIO header. > > I moved the GPIO_TS5500 entry at the good place in the "Memory mapped > GPIO drivers" section. But I'm still worried about select/depends on. > I removed the TS5500 symbol dependency, which is not defined yet (will > be done in the future board support patch). Does the following entry > seem good? > > config GPIO_TS5500 > tristate "TS-5500 DIO Headers support" > help > This driver supports the 3 Digital I/O headers of the > Technologic Systems TS-5500 platform: DIO1, DIO2, and > the > LCD port which may be used as a DIO header. > > Regards, > Vivien -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio: add TS-5500 DIO headers support
Hi,
I think the Kconfig patch is wrong, please do not consider this
patchset. I'll send a v2 very soon.
On Tue, 2012-09-25 at 20:42 -0400, Vivien Didelot wrote:
> The Technologic Systems TS-5500 platform provides 3 digital I/O headers:
> DIO1, DIO2, and the LCD port, that may be used as a DIO header.
>
> Signed-off-by: Vivien Didelot
> Signed-off-by: Jerome Oufella
> ---
> drivers/gpio/Kconfig | 8 +
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-ts5500.c| 344
> ++
> include/linux/platform_data/gpio-ts5500.h | 34 +++
> 4 files changed, 387 insertions(+)
> create mode 100644 drivers/gpio/gpio-ts5500.c
> create mode 100644 include/linux/platform_data/gpio-ts5500.h
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index ba7926f5..f8bf8e1 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -395,6 +395,14 @@ config GPIO_TPS65912
> help
> This driver supports TPS65912 gpio chip
>
> +config GPIO_TS5500
> + tristate "TS-5500 DIO Headers"
> + depends on TS5500
> + help
> + This driver supports the 3 Digital I/O headers of the Technologic
> + Systems TS-5500 platform: DIO1, DIO2, and the LCD port which may be
> + used as a DIO header.
I moved the GPIO_TS5500 entry at the good place in the "Memory mapped
GPIO drivers" section. But I'm still worried about select/depends on.
I removed the TS5500 symbol dependency, which is not defined yet (will
be done in the future board support patch). Does the following entry
seem good?
config GPIO_TS5500
tristate "TS-5500 DIO Headers support"
help
This driver supports the 3 Digital I/O headers of the
Technologic Systems TS-5500 platform: DIO1, DIO2, and the
LCD port which may be used as a DIO header.
Regards,
Vivien
> +
> config GPIO_TWL4030
> tristate "TWL4030, TWL5030, and TPS659x0 GPIOs"
> depends on TWL4030_CORE
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 153cace..48e8670 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -66,6 +66,7 @@ obj-$(CONFIG_ARCH_DAVINCI_TNETV107X) += gpio-tnetv107x.o
> obj-$(CONFIG_GPIO_TPS6586X) += gpio-tps6586x.o
> obj-$(CONFIG_GPIO_TPS65910) += gpio-tps65910.o
> obj-$(CONFIG_GPIO_TPS65912) += gpio-tps65912.o
> +obj-$(CONFIG_GPIO_TS5500)+= gpio-ts5500.o
> obj-$(CONFIG_GPIO_TWL4030) += gpio-twl4030.o
> obj-$(CONFIG_GPIO_UCB1400) += gpio-ucb1400.o
> obj-$(CONFIG_GPIO_VR41XX)+= gpio-vr41xx.o
> diff --git a/drivers/gpio/gpio-ts5500.c b/drivers/gpio/gpio-ts5500.c
> new file mode 100644
> index 000..d91fee9
> --- /dev/null
> +++ b/drivers/gpio/gpio-ts5500.c
> @@ -0,0 +1,344 @@
> +/*
> + * GPIO (DIO) driver for Technologic Systems TS-5500
> + *
> + * Copyright (c) 2010-2012 Savoir-faire Linux Inc.
> + * Vivien Didelot
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * The TS-5500 platform has 38 Digital Input/Output lines (DIO), exposed by 3
> + * DIO headers: DIO1, DIO2, and the LCD port which may be used as a DIO
> header.
> + *
> + * The datasheet is available at:
> + * http://embeddedx86.com/documentation/ts-5500-manual.pdf.
> + * See section 6 "Digital I/O" for details about the pinout.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/*
> + * This array describes the names of the DIO lines, but also the mapping
> between
> + * the datasheet, and corresponding offsets exposed by the driver.
> + */
> +static const char * const ts5500_pinout[38] = {
> + /* DIO1 Header (offset 0-13) */
> + [0] = "DIO1_0", /* pin 1 */
> + [1] = "DIO1_1", /* pin 3 */
> + [2] = "DIO1_2", /* pin 5 */
> + [3] = "DIO1_3", /* pin 7 */
> + [4] = "DIO1_4", /* pin 9 */
> + [5] = "DIO1_5", /* pin 11 */
> + [6] = "DIO1_6", /* pin 13 */
> + [7] = "DIO1_7", /* pin 15 */
> + [8] = "DIO1_8", /* pin 4 */
> + [9] = "DIO1_9", /* pin 6 */
> + [10] = "DIO1_10", /* pin 8 */
> + [11] = "DIO1_11", /* pin 10 */
> + [12] = "DIO1_12", /* pin 12 */
> + [13] = "DIO1_13", /* pin 14 */
> +
> + /* DIO2 Header (offset 14-26) */
> + [14] = "DIO2_0", /* pin 1 */
> + [15] = "DIO2_1", /* pin 3 */
> + [16] = "DIO2_2", /* pin 5 */
> + [17] = "DIO2_3", /* pin 7 */
> + [18] = "DIO2_4", /* pin 9 */
> + [19] = "DIO2_5", /* pin 11 */
> + [20] = "DIO2_6", /* pin 13 */
> + [21] = "DIO2_7", /* pin 15 */
> + [22] = "DIO2_8", /* pin 4 */
> + [23] = "DIO2_9", /* pin 6 */
> + [24] = "DIO2_10", /* pin 8 */
> + [25] = "DIO2_11", /* pin 10 */
> + [26] = "DIO2_13", /* pin 14 */
> +
> + /* LCD Port as DIO
[PATCH] gpio: add TS-5500 DIO headers support
The Technologic Systems TS-5500 platform provides 3 digital I/O headers:
DIO1, DIO2, and the LCD port, that may be used as a DIO header.
Signed-off-by: Vivien Didelot
Signed-off-by: Jerome Oufella
---
drivers/gpio/Kconfig | 8 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-ts5500.c| 344 ++
include/linux/platform_data/gpio-ts5500.h | 34 +++
4 files changed, 387 insertions(+)
create mode 100644 drivers/gpio/gpio-ts5500.c
create mode 100644 include/linux/platform_data/gpio-ts5500.h
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index ba7926f5..f8bf8e1 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -395,6 +395,14 @@ config GPIO_TPS65912
help
This driver supports TPS65912 gpio chip
+config GPIO_TS5500
+ tristate "TS-5500 DIO Headers"
+ depends on TS5500
+ help
+ This driver supports the 3 Digital I/O headers of the Technologic
+ Systems TS-5500 platform: DIO1, DIO2, and the LCD port which may be
+ used as a DIO header.
+
config GPIO_TWL4030
tristate "TWL4030, TWL5030, and TPS659x0 GPIOs"
depends on TWL4030_CORE
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 153cace..48e8670 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_ARCH_DAVINCI_TNETV107X) += gpio-tnetv107x.o
obj-$(CONFIG_GPIO_TPS6586X)+= gpio-tps6586x.o
obj-$(CONFIG_GPIO_TPS65910)+= gpio-tps65910.o
obj-$(CONFIG_GPIO_TPS65912)+= gpio-tps65912.o
+obj-$(CONFIG_GPIO_TS5500) += gpio-ts5500.o
obj-$(CONFIG_GPIO_TWL4030) += gpio-twl4030.o
obj-$(CONFIG_GPIO_UCB1400) += gpio-ucb1400.o
obj-$(CONFIG_GPIO_VR41XX) += gpio-vr41xx.o
diff --git a/drivers/gpio/gpio-ts5500.c b/drivers/gpio/gpio-ts5500.c
new file mode 100644
index 000..d91fee9
--- /dev/null
+++ b/drivers/gpio/gpio-ts5500.c
@@ -0,0 +1,344 @@
+/*
+ * GPIO (DIO) driver for Technologic Systems TS-5500
+ *
+ * Copyright (c) 2010-2012 Savoir-faire Linux Inc.
+ * Vivien Didelot
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * The TS-5500 platform has 38 Digital Input/Output lines (DIO), exposed by 3
+ * DIO headers: DIO1, DIO2, and the LCD port which may be used as a DIO header.
+ *
+ * The datasheet is available at:
+ * http://embeddedx86.com/documentation/ts-5500-manual.pdf.
+ * See section 6 "Digital I/O" for details about the pinout.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/*
+ * This array describes the names of the DIO lines, but also the mapping
between
+ * the datasheet, and corresponding offsets exposed by the driver.
+ */
+static const char * const ts5500_pinout[38] = {
+ /* DIO1 Header (offset 0-13) */
+ [0] = "DIO1_0", /* pin 1 */
+ [1] = "DIO1_1", /* pin 3 */
+ [2] = "DIO1_2", /* pin 5 */
+ [3] = "DIO1_3", /* pin 7 */
+ [4] = "DIO1_4", /* pin 9 */
+ [5] = "DIO1_5", /* pin 11 */
+ [6] = "DIO1_6", /* pin 13 */
+ [7] = "DIO1_7", /* pin 15 */
+ [8] = "DIO1_8", /* pin 4 */
+ [9] = "DIO1_9", /* pin 6 */
+ [10] = "DIO1_10", /* pin 8 */
+ [11] = "DIO1_11", /* pin 10 */
+ [12] = "DIO1_12", /* pin 12 */
+ [13] = "DIO1_13", /* pin 14 */
+
+ /* DIO2 Header (offset 14-26) */
+ [14] = "DIO2_0", /* pin 1 */
+ [15] = "DIO2_1", /* pin 3 */
+ [16] = "DIO2_2", /* pin 5 */
+ [17] = "DIO2_3", /* pin 7 */
+ [18] = "DIO2_4", /* pin 9 */
+ [19] = "DIO2_5", /* pin 11 */
+ [20] = "DIO2_6", /* pin 13 */
+ [21] = "DIO2_7", /* pin 15 */
+ [22] = "DIO2_8", /* pin 4 */
+ [23] = "DIO2_9", /* pin 6 */
+ [24] = "DIO2_10", /* pin 8 */
+ [25] = "DIO2_11", /* pin 10 */
+ [26] = "DIO2_13", /* pin 14 */
+
+ /* LCD Port as DIO (offset 27-37) */
+ [27] = "LCD_0", /* pin 8 */
+ [28] = "LCD_1", /* pin 7 */
+ [29] = "LCD_2", /* pin 10 */
+ [30] = "LCD_3", /* pin 9 */
+ [31] = "LCD_4", /* pin 12 */
+ [32] = "LCD_5", /* pin 11 */
+ [33] = "LCD_6", /* pin 14 */
+ [34] = "LCD_7", /* pin 13 */
+ [35] = "LCD_EN", /* pin 5 */
+ [36] = "LCD_WR", /* pin 6 */
+ [37] = "LCD_RS", /* pin 3 */
+};
+
+#define IN (1 << 0)
+#define OUT(1 << 1)
+#ifndef NO_IRQ
+#define NO_IRQ -1
+#endif
+
+/*
+ * This structure is used to describe capabilities of DIO lines,
+ * such as available directions, and mapped IRQ (if any).
+ */
+struct ts5500_dio {
+ const unsigned long value_addr;
+ const int value_bit;
+ const unsigned long control_addr;
+ const int control_bit;
+ const int irq;
+ const int direction;
+};
+
