Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

2013-06-24 Thread Linus Walleij 
On Thu, Jun 20, 2013 at 2:12 PM, Michal Simek mon...@monstr.eu wrote:

 xlnx,is-dual is always present in the HW and in all DTSes and it
 is generated for several years

 Based on my experience with hardware guys what happen when they add
 new channel is that they will use xlnx,is-dual = 2 for 3 channels,
 xlnx,is-dual = 3 for 4 channels, etc. They won't care about names
 but they are working like that.

 It means for me is really problematic it try to work with this
 value as boolean even that name is confusing.

OK I will have to live with this.

The problem with reviewing DT bindings is that for me as a
subsystem maintainer I'm interacting with a quite complex
universe:

1. Bindings from people like the MIPS camp where some people
  obviously sat down in a committee meeting and tried to define
  a binding in advance, which is then deployed and we have to
  support. Sometimes a real nice piece of work such as the
  PCI hose mappings.

2. Bindings that we need to evolve as part of this community
  review process, where the judgement of a mapping's
  applicability and sanity is very much up to the subsystem
  maintainer. (This is the vast class of bindings.)

3. Bindings that John Doe from Idaho came up with in his
  basement and then deployed to the entire world, so that
  we cannot review or amend it but just have to support as
  they are because they are already live in numerous
  systems.

This would be a case of (3) whereas I'm mostly used to
a binding discussion of type (2) and that is why this gets
so locked up.

 That's why it is much easier for me to start to use new property
 which will describe number of channels but this value is not
 used in design tools. Let's say number-of-channels = 1 or 2
 in DT binding which will describe number channels in this IP.
 Is this acceptable for you?

This is much nicer and readable.

Yours,
Linus Walleij
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

2013-06-20 Thread Michal Simek 
On 06/17/2013 07:29 AM, Linus Walleij wrote:
 On Fri, May 31, 2013 at 9:34 AM, Michal Simek mon...@monstr.eu wrote:
 On 05/31/2013 09:14 AM, Linus Walleij wrote:
 
 It's OK, but fix the boolean member so as to just needing to
 be present:

 xlnx,is-dual;

 Rather than

 xlnx,is-dual = 1;

 Surely I can do it but it means to change our BSP and because
 this IP is used on Microblaze from beginning this change
 breaks all DTSes from past.
 
 I think of_property_read_bool() will accept
 xlnx,is-dual = 1; to mean the same as xlnx,is-dual;
 try it.

First of all sorry for delay.
You are right that of_property_read_bool()
also accept xlnx,is-dual = 1;
but also accept and return 1 when xlnx,is-dual = 0;
which is incorrect behaviour.

If of_prorety_read_bool return 0 for case when xlnx,is-dual = 0
we can use it.
Maybe this will be good change for this function behaviour.
If there is also value then use it. 0 - false, 1 - true

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP - KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform




signature.asc
Description: OpenPGP digital signature
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

2013-06-20 Thread Linus Walleij 
On Thu, Jun 20, 2013 at 9:51 AM, Michal Simek mon...@monstr.eu wrote:
 On 06/17/2013 07:29 AM, Linus Walleij wrote:

 I think of_property_read_bool() will accept
 xlnx,is-dual = 1; to mean the same as xlnx,is-dual;
 try it.

 First of all sorry for delay.
 You are right that of_property_read_bool()
 also accept xlnx,is-dual = 1;
 but also accept and return 1 when xlnx,is-dual = 0;
 which is incorrect behaviour.

OK but that is a coding issue, not a DT bindings design issue.
Can't we think a bit outside the box?
What about something like this:

static bool is_dual (struct device_node *np)
{
struct property *prop = of_find_property(np, xlnx,is-dual, NULL);
int ret;
u32 val;

if (!prop)
return false;

ret = of_property_read_u32(np, xlnx,is-dual, val);
if (ret  0)
return true; /* node exists but has no cells */

return !!val;
}

Yours,
Linus Walleij
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

2013-06-20 Thread Michal Simek 
On 06/20/2013 11:23 AM, Linus Walleij wrote:
 On Thu, Jun 20, 2013 at 9:51 AM, Michal Simek mon...@monstr.eu wrote:
 On 06/17/2013 07:29 AM, Linus Walleij wrote:
 
 I think of_property_read_bool() will accept
 xlnx,is-dual = 1; to mean the same as xlnx,is-dual;
 try it.

 First of all sorry for delay.
 You are right that of_property_read_bool()
 also accept xlnx,is-dual = 1;
 but also accept and return 1 when xlnx,is-dual = 0;
 which is incorrect behaviour.
 
 OK but that is a coding issue, not a DT bindings design issue.
 Can't we think a bit outside the box?

Before that fyi: I am working on supporing irq in this driver too.
Sure.

 What about something like this:
 
 static bool is_dual (struct device_node *np)
 {
 struct property *prop = of_find_property(np, xlnx,is-dual, NULL);
 int ret;
 u32 val;
 
 if (!prop)
 return false;
 
 ret = of_property_read_u32(np, xlnx,is-dual, val);
 if (ret  0)
 return true; /* node exists but has no cells */
 
 return !!val;
 }

we can do it in this way but what I don't like on this is that IP
is design to support 2 channels right now.
It can happen that Xilinx decides to extend this for new channels.
Register map is prepared for it and there is enough space to do it.

And when this is done then is-dual (which is current name which
is used in hardware configuration from design tools) will contain
larger value 1.
I agree that is-dual is not the best name.

What about to do it differently?
Generate number of channel in the description.
And also do for() loop in the probe function to read values based
on this channel number.

Thanks,
Michal




-- 
Michal Simek, Ing. (M.Eng), OpenPGP - KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform




signature.asc
Description: OpenPGP digital signature
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

2013-06-20 Thread Linus Walleij 
On Thu, Jun 20, 2013 at 12:59 PM, Michal Simek mon...@monstr.eu wrote:
 On 06/20/2013 11:23 AM, Linus Walleij wrote:

 What about something like this:

 static bool is_dual (struct device_node *np)
 {
 struct property *prop = of_find_property(np, xlnx,is-dual, NULL);
 int ret;
 u32 val;

 if (!prop)
 return false;

 ret = of_property_read_u32(np, xlnx,is-dual, val);
 if (ret  0)
 return true; /* node exists but has no cells */

 return !!val;
 }

 we can do it in this way but what I don't like on this is that IP
 is design to support 2 channels right now.
 It can happen that Xilinx decides to extend this for new channels.
 Register map is prepared for it and there is enough space to do it.

 And when this is done then is-dual (which is current name which
 is used in hardware configuration from design tools) will contain
 larger value 1.
 I agree that is-dual is not the best name.

You don't say. It's about as smart as this:

#define NUMBER_TWO 4

Oh well, that's not your fault.

But seriously:

  xlnx,is-dual;

without an argument can still be taken to mean 2, and
you still need to inperpret the absence if this parameter
as 1 do you not?

 What about to do it differently?
 Generate number of channel in the description.
 And also do for() loop in the probe function to read values based
 on this channel number.

Sorry I can't translate this, can you send a patch?

Yours,
Linus Walleij
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

2013-06-20 Thread Michal Simek 
On 06/20/2013 01:33 PM, Linus Walleij wrote:
 On Thu, Jun 20, 2013 at 12:59 PM, Michal Simek mon...@monstr.eu wrote:
 On 06/20/2013 11:23 AM, Linus Walleij wrote:
 
 What about something like this:

 static bool is_dual (struct device_node *np)
 {
 struct property *prop = of_find_property(np, xlnx,is-dual, NULL);
 int ret;
 u32 val;

 if (!prop)
 return false;

 ret = of_property_read_u32(np, xlnx,is-dual, val);
 if (ret  0)
 return true; /* node exists but has no cells */

 return !!val;
 }

 we can do it in this way but what I don't like on this is that IP
 is design to support 2 channels right now.
 It can happen that Xilinx decides to extend this for new channels.
 Register map is prepared for it and there is enough space to do it.

 And when this is done then is-dual (which is current name which
 is used in hardware configuration from design tools) will contain
 larger value 1.
 I agree that is-dual is not the best name.
 
 You don't say. It's about as smart as this:
 
 #define NUMBER_TWO 4
 
 Oh well, that's not your fault.
 
 But seriously:
 
   xlnx,is-dual;
 
 without an argument can still be taken to mean 2, and
 you still need to inperpret the absence if this parameter
 as 1 do you not?

 What about to do it differently?
 Generate number of channel in the description.
 And also do for() loop in the probe function to read values based
 on this channel number.
 
 Sorry I can't translate this, can you send a patch?

Sure.

xlnx,is-dual is always present in the HW and in all DTSes and it
is generated for several years

Based on my experience with hardware guys what happen when they add
new channel is that they will use xlnx,is-dual = 2 for 3 channels,
xlnx,is-dual = 3 for 4 channels, etc. They won't care about names
but they are working like that.

It means for me is really problematic it try to work with this
value as boolean even that name is confusing.

That's why it is much easier for me to start to use new property
which will describe number of channels but this value is not
used in design tools. Let's say number-of-channels = 1 or 2
in DT binding which will describe number channels in this IP.
Is this acceptable for you?



If you look how that dual channel is supported you will see that it is
probe_first_channel
if there is second channel probe it too.

And code there is very similar.
That's why I am thinking about using the loop to remove that code
duplication.
Something like this in psedo code

for(i = 0; i  number-of-channel; i++) {
char *str_def = xlnx,dout-default
if(i)
add -(i+1) suffix to str_def (-2 for example just to reflect 
how design tools generates it)

of_property_read_u32(np, str_def, chip-gpio_state);
...
gpiochip_add()
}

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP - KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform




signature.asc
Description: OpenPGP digital signature
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

2013-06-16 Thread Linus Walleij 
On Fri, May 31, 2013 at 9:34 AM, Michal Simek mon...@monstr.eu wrote:
 On 05/31/2013 09:14 AM, Linus Walleij wrote:

 It's OK, but fix the boolean member so as to just needing to
 be present:

 xlnx,is-dual;

 Rather than

 xlnx,is-dual = 1;

 Surely I can do it but it means to change our BSP and because
 this IP is used on Microblaze from beginning this change
 breaks all DTSes from past.

I think of_property_read_bool() will accept
xlnx,is-dual = 1; to mean the same as xlnx,is-dual;
try it.

Yours,
Linus Walleij
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

2013-05-31 Thread Linus Walleij 
On Fri, May 31, 2013 at 7:43 AM, Michal Simek mon...@monstr.eu wrote:
 On 05/30/2013 09:46 PM, Linus Walleij wrote:

 (...)
 +/* Read/Write access to the GPIO registers */
 +#define xgpio_readreg(offset)  __raw_readl(offset)
 +#define xgpio_writereg(offset, val)__raw_writel(val, offset)

 So you're swithing in_be32/out_be32 to the CPU-dependent
 __raw_readl/__raw_writel functions? Why?

 The reason is that this driver can be used on ARM where in_be32/out_be32
 is not implemented.

OK I buy this (and the following explanation).

I think readl/writel is always in LE (PCI) endianness anyway, which
is likely not what you want. (I suspect the next point was about
that.)

 Have you documented these new bindings? It doesn't seem so.
 Documentation/devicetree/bindings/gpio/*...

 If it's undocumented so far, this is a good oppotunity to add it.

 Isn't it enough what it is in 2/2?

I didn't see 2/2, I guess I wasn't on CC...

Anyway I guess it's this:
http://marc.info/?l=linux-kernelm=136982686732560w=2

It's OK, but fix the boolean member so as to just needing to
be present:

xlnx,is-dual;

Rather than

xlnx,is-dual = 1;

 Or do you want to describe current binding in the first patch
 and then extend it in this patch when dual channel is added?

Nah. 2/2 is fine.

 This is basically a jam table (hardware set-up) in the device tree.

 Not sure what you mean by that. Xilinx GPIO is soft IP which can be configured
 to different configurations before bitstream is generated.
 At the end you will get different setting/addresses setup for every pin
 which is described by these xlnx,X descriptions.

 I don't exactly like this. Is this necessary?

 If you mean names or values in there that all of them are autogenerated
 from design tools and they are reflect IP hardware description and all
 configuration options which you can have there.
 It means that all these values give you exact hardware description.

 Do I answer your question?

Yes, this is OK, I buy that explanation. I thought it was
something else.

I think the overall problem is that I do not understand what a
channel is in this context, and thus it is hard to understand the
patch as a whole. 2/2 could add some more verbose explanation
about the HW IP so I get comfortable and can understand the
whole hardware block...

Yours,
Linus Walleij
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

2013-05-31 Thread Michal Simek 
On 05/31/2013 09:14 AM, Linus Walleij wrote:
 On Fri, May 31, 2013 at 7:43 AM, Michal Simek mon...@monstr.eu wrote:
 On 05/30/2013 09:46 PM, Linus Walleij wrote:
 
 (...)
 +/* Read/Write access to the GPIO registers */
 +#define xgpio_readreg(offset)  __raw_readl(offset)
 +#define xgpio_writereg(offset, val)__raw_writel(val, offset)

 So you're swithing in_be32/out_be32 to the CPU-dependent
 __raw_readl/__raw_writel functions? Why?

 The reason is that this driver can be used on ARM where in_be32/out_be32
 is not implemented.
 
 OK I buy this (and the following explanation).
 
 I think readl/writel is always in LE (PCI) endianness anyway, which
 is likely not what you want. (I suspect the next point was about
 that.)

readl/writel yes it is all the time little endian
but __raw_readl/__raw_writel is just *(u32 *)ptr access
without any endian checking and barriers.

Probably the best way how to handle is to write
#ifdef ARCH_ZYNQ
# define xgpio_readreg(offset)  readl(offset)
# define xgpio_writereg(offset, val)writel(val, offset)
#else
# define xgpio_readreg(offset)  __raw_readl(offset)
# define xgpio_writereg(offset, val)__raw_writel(val, offset)
#endif

But still it is not correct in sense that I shouldn't pretend
that __raw_readl is ok to run on ppc and microblaze big endian.
But using another ifdef BIG_ENDIAN or ARCH don't improve it.

If there is one more register which I can use for autodetection,
it will be easy to choose but that's not this case.

 Have you documented these new bindings? It doesn't seem so.
 Documentation/devicetree/bindings/gpio/*...

 If it's undocumented so far, this is a good oppotunity to add it.

 Isn't it enough what it is in 2/2?
 
 I didn't see 2/2, I guess I wasn't on CC...
 
 Anyway I guess it's this:
 http://marc.info/?l=linux-kernelm=136982686732560w=2

Yes. it is. I am using patman and you are probably not listed
in MAINTAINERS for this folder to automatically add you.
Will add you manually.

 It's OK, but fix the boolean member so as to just needing to
 be present:
 
 xlnx,is-dual;
 
 Rather than
 
 xlnx,is-dual = 1;

Surely I can do it but it means to change our BSP and because
this IP is used on Microblaze from beginning this change
breaks all DTSes from past.
That's why I would prefer not to change logic here because
it just breaks all Microblaze DTSes which were generated
till this change (All of them contains xlnx,is-dual = 0
if dual channel is not used).

I will definitely look at dt function in the whole driver
and use the

 Or do you want to describe current binding in the first patch
 and then extend it in this patch when dual channel is added?
 
 Nah. 2/2 is fine.

ok.

 This is basically a jam table (hardware set-up) in the device tree.

 Not sure what you mean by that. Xilinx GPIO is soft IP which can be 
 configured
 to different configurations before bitstream is generated.
 At the end you will get different setting/addresses setup for every pin
 which is described by these xlnx,X descriptions.

 I don't exactly like this. Is this necessary?

 If you mean names or values in there that all of them are autogenerated
 from design tools and they are reflect IP hardware description and all
 configuration options which you can have there.
 It means that all these values give you exact hardware description.

 Do I answer your question?
 
 Yes, this is OK, I buy that explanation. I thought it was
 something else.

ok

 I think the overall problem is that I do not understand what a
 channel is in this context, and thus it is hard to understand the
 patch as a whole. 2/2 could add some more verbose explanation
 about the HW IP so I get comfortable and can understand the
 whole hardware block...

ok. Good.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP - KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform




signature.asc
Description: OpenPGP digital signature
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

2013-05-30 Thread Michal Simek 
Hi Linus,

On 05/30/2013 09:46 PM, Linus Walleij wrote:
 On Wed, May 29, 2013 at 1:27 PM, Michal Simek michal.si...@xilinx.com wrote:
 
 Supporting the second channel in the driver.
 Offset is 0x8 and both channnels share the same
 IRQ.

 Signed-off-by: Michal Simek michal.si...@xilinx.com
 
 (...)
 +/* Read/Write access to the GPIO registers */
 +#define xgpio_readreg(offset)  __raw_readl(offset)
 +#define xgpio_writereg(offset, val)__raw_writel(val, offset)
 
 So you're swithing in_be32/out_be32 to the CPU-dependent
 __raw_readl/__raw_writel functions? Why?

The reason is that this driver can be used on ARM where in_be32/out_be32
is not implemented.

 Can you explain exactly why you are using __raw_* accessors
 rather than e.g. atleast readl_relaxed()/writel_relaxed()
 or even plain readl/writel so you know the writes will hit
 the hardware as immediately as possible?

Using __raw* function ensure that it is working on all
cpus. Microblaze big/little endian, PPC big endian and ARM little endian.

The correct way how to implement this is based on my previous
discussion to detect endians directly on IP.

But for this gpio case without interrupt connected(it means without
interrupt logic) there are just 2 registers data and tristate
(http://www.xilinx.com/support/documentation/ip_documentation/axi_gpio/v1_01_b/ds744_axi_gpio.pdf)
and auto detection can't be done.

 I'd prefer this step to be a separate patch.

ok. Will do based on my discussion around xilinxfb.

  struct xgpio_instance {
 struct of_mm_gpio_chip mmchip;
 u32 gpio_state; /* GPIO state shadow register */
 u32 gpio_dir;   /* GPIO direction shadow register */
 +   u32 offset;
 spinlock_t gpio_lock;   /* Lock used for synchronization */
  };
 
 Why not take this opportunity to move the comments out to
 kerneldoc above this struct, plus document what offset
 means.

Good point. Will fix.

 
 -   return (in_be32(mm_gc-regs + XGPIO_DATA_OFFSET)  gpio)  1;
 +   return (xgpio_readreg(regs + XGPIO_DATA_OFFSET)  gpio)  1;
 
 
 Another way would be:
 
 #include linux/bitops.h
 
 return !!(xgpio_readreg(regs + XGPIO_DATA_OFFSET  BIT(gpio));
 
 +
 +   pr_info(XGpio: %s: registered, base is %d\n, np-full_name,
 +   
 chip-mmchip.gc.base);
 +
 +   tree_info = of_get_property(np, xlnx,is-dual, NULL);
 
 This looks like you want to use of_property_read_bool().

Ah yeah.

 Have you documented these new bindings? It doesn't seem so.
 Documentation/devicetree/bindings/gpio/*...
 
 If it's undocumented so far, this is a good oppotunity to add it.

Isn't it enough what it is in 2/2?
Or do you want to describe current binding in the first patch
and then extend it in this patch when dual channel is added?


 +   if (tree_info  be32_to_cpup(tree_info)) {
 +   chip = kzalloc(sizeof(*chip), GFP_KERNEL);
 +   if (!chip)
 +   return -ENOMEM;
 +
 +   /* Add dual channel offset */
 +   chip-offset = XGPIO_CHANNEL_OFFSET;
 +
 +   /* Update GPIO state shadow register with default value */
 +   tree_info = of_get_property(np, xlnx,dout-default-2, NULL);
 +   if (tree_info)
 +   chip-gpio_state = be32_to_cpup(tree_info);
 
 This is basically a jam table (hardware set-up) in the device tree.

Not sure what you mean by that. Xilinx GPIO is soft IP which can be configured
to different configurations before bitstream is generated.
At the end you will get different setting/addresses setup for every pin
which is described by these xlnx,X descriptions.

 I don't exactly like this. Is this necessary?

If you mean names or values in there that all of them are autogenerated
from design tools and they are reflect IP hardware description and all
configuration options which you can have there.
It means that all these values give you exact hardware description.

Do I answer your question?


 
 +   /* Update GPIO direction shadow register with default value 
 */
 +   /* By default, all pins are inputs */
 +   chip-gpio_dir = 0x;
 +   tree_info = of_get_property(np, xlnx,tri-default-2, NULL);
 +   if (tree_info)
 +   chip-gpio_dir = be32_to_cpup(tree_info);
 
 Dito.
 
 +   /* Check device node and parent device node for device width 
 */
 +   /* By default assume full GPIO controller */
 +   chip-mmchip.gc.ngpio = 32;
 +   tree_info = of_get_property(np, xlnx,gpio2-width, NULL);
 +   if (tree_info)
 +   chip-mmchip.gc.ngpio = be32_to_cpup(tree_info);
 
 Seems fine, but document it in the binding.

I will look at new fdt function to shorten this code to look better.

Thanks for your review,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP - KeyID: FE3D1F91
w: www.monstr.eu

[PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

2013-05-29 Thread Michal Simek 
Supporting the second channel in the driver.
Offset is 0x8 and both channnels share the same
IRQ.

Signed-off-by: Michal Simek michal.si...@xilinx.com
---
 drivers/gpio/gpio-xilinx.c | 93 --
 1 file changed, 81 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
index 9ae7aa8..385dcb0 100644
--- a/drivers/gpio/gpio-xilinx.c
+++ b/drivers/gpio/gpio-xilinx.c
@@ -1,7 +1,7 @@
 /*
- * Xilinx gpio driver
+ * Xilinx gpio driver for xps/axi_gpio IP.
  *
- * Copyright 2008 Xilinx, Inc.
+ * Copyright 2008 - 2013 Xilinx, Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2
@@ -26,10 +26,17 @@
 #define XGPIO_DATA_OFFSET   (0x0)  /* Data register  */
 #define XGPIO_TRI_OFFSET(0x4)  /* I/O direction register  */

+#define XGPIO_CHANNEL_OFFSET   0x8
+
+/* Read/Write access to the GPIO registers */
+#define xgpio_readreg(offset)  __raw_readl(offset)
+#define xgpio_writereg(offset, val)__raw_writel(val, offset)
+
 struct xgpio_instance {
struct of_mm_gpio_chip mmchip;
u32 gpio_state; /* GPIO state shadow register */
u32 gpio_dir;   /* GPIO direction shadow register */
+   u32 offset;
spinlock_t gpio_lock;   /* Lock used for synchronization */
 };

@@ -44,8 +51,12 @@ struct xgpio_instance {
 static int xgpio_get(struct gpio_chip *gc, unsigned int gpio)
 {
struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+   struct xgpio_instance *chip =
+   container_of(mm_gc, struct xgpio_instance, mmchip);
+
+   void __iomem *regs = mm_gc-regs + chip-offset;

-   return (in_be32(mm_gc-regs + XGPIO_DATA_OFFSET)  gpio)  1;
+   return (xgpio_readreg(regs + XGPIO_DATA_OFFSET)  gpio)  1;
 }

 /**
@@ -63,6 +74,7 @@ static void xgpio_set(struct gpio_chip *gc, unsigned int 
gpio, int val)
struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
struct xgpio_instance *chip =
container_of(mm_gc, struct xgpio_instance, mmchip);
+   void __iomem *regs = mm_gc-regs;

spin_lock_irqsave(chip-gpio_lock, flags);

@@ -71,7 +83,9 @@ static void xgpio_set(struct gpio_chip *gc, unsigned int 
gpio, int val)
chip-gpio_state |= 1  gpio;
else
chip-gpio_state = ~(1  gpio);
-   out_be32(mm_gc-regs + XGPIO_DATA_OFFSET, chip-gpio_state);
+
+   xgpio_writereg(regs + chip-offset + XGPIO_DATA_OFFSET,
+chip-gpio_state);

spin_unlock_irqrestore(chip-gpio_lock, flags);
 }
@@ -91,12 +105,13 @@ static int xgpio_dir_in(struct gpio_chip *gc, unsigned int 
gpio)
struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
struct xgpio_instance *chip =
container_of(mm_gc, struct xgpio_instance, mmchip);
+   void __iomem *regs = mm_gc-regs;

spin_lock_irqsave(chip-gpio_lock, flags);

/* Set the GPIO bit in shadow register and set direction as input */
chip-gpio_dir |= (1  gpio);
-   out_be32(mm_gc-regs + XGPIO_TRI_OFFSET, chip-gpio_dir);
+   xgpio_writereg(regs + chip-offset + XGPIO_TRI_OFFSET, chip-gpio_dir);

spin_unlock_irqrestore(chip-gpio_lock, flags);

@@ -119,6 +134,7 @@ static int xgpio_dir_out(struct gpio_chip *gc, unsigned int 
gpio, int val)
struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
struct xgpio_instance *chip =
container_of(mm_gc, struct xgpio_instance, mmchip);
+   void __iomem *regs = mm_gc-regs;

spin_lock_irqsave(chip-gpio_lock, flags);

@@ -127,11 +143,12 @@ static int xgpio_dir_out(struct gpio_chip *gc, unsigned 
int gpio, int val)
chip-gpio_state |= 1  gpio;
else
chip-gpio_state = ~(1  gpio);
-   out_be32(mm_gc-regs + XGPIO_DATA_OFFSET, chip-gpio_state);
+   xgpio_writereg(regs + chip-offset + XGPIO_DATA_OFFSET,
+  chip-gpio_state);

/* Clear the GPIO bit in shadow register and set direction as output */
chip-gpio_dir = (~(1  gpio));
-   out_be32(mm_gc-regs + XGPIO_TRI_OFFSET, chip-gpio_dir);
+   xgpio_writereg(regs + chip-offset + XGPIO_TRI_OFFSET, chip-gpio_dir);

spin_unlock_irqrestore(chip-gpio_lock, flags);

@@ -147,8 +164,10 @@ static void xgpio_save_regs(struct of_mm_gpio_chip *mm_gc)
struct xgpio_instance *chip =
container_of(mm_gc, struct xgpio_instance, mmchip);

-   out_be32(mm_gc-regs + XGPIO_DATA_OFFSET, chip-gpio_state);
-   out_be32(mm_gc-regs + XGPIO_TRI_OFFSET, chip-gpio_dir);
+   xgpio_writereg(mm_gc-regs + chip-offset + XGPIO_DATA_OFFSET,
+   chip-gpio_state);
+   xgpio_writereg(mm_gc-regs + chip-offset + XGPIO_TRI_OFFSET,
+