Hi Linus,
can you please look at this patchset?
Thanks,
Michal
On 06/03/2013 02:31 PM, Michal Simek wrote:
> Simplification is done by using OF helper function
> which increase readability of code and remove
> (if (var) var = be32_to_cpup;) assignment.
>
> Signed-off-by: Michal Simek <[email protected]>
> ---
> Changes in v2:
> - New patch in this series
>
> drivers/gpio/gpio-xilinx.c | 24 ++++++++++--------------
> 1 file changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
> index 9ae7aa8..2aad534 100644
> --- a/drivers/gpio/gpio-xilinx.c
> +++ b/drivers/gpio/gpio-xilinx.c
> @@ -170,24 +170,20 @@ static int xgpio_of_probe(struct device_node *np)
> return -ENOMEM;
>
> /* Update GPIO state shadow register with default value */
> - tree_info = of_get_property(np, "xlnx,dout-default", NULL);
> - if (tree_info)
> - chip->gpio_state = be32_to_cpup(tree_info);
> + of_property_read_u32(np, "xlnx,dout-default", &chip->gpio_state);
> +
> + /* By default, all pins are inputs */
> + chip->gpio_dir = 0xFFFFFFFF;
>
> /* Update GPIO direction shadow register with default value */
> - chip->gpio_dir = 0xFFFFFFFF; /* By default, all pins are inputs */
> - tree_info = of_get_property(np, "xlnx,tri-default", NULL);
> - if (tree_info)
> - chip->gpio_dir = be32_to_cpup(tree_info);
> + of_property_read_u32(np, "xlnx,tri-default", &chip->gpio_dir);
> +
> + /* By default assume full GPIO controller */
> + chip->mmchip.gc.ngpio = 32;
>
> /* Check device node and parent device node for device width */
> - chip->mmchip.gc.ngpio = 32; /* By default assume full GPIO controller */
> - tree_info = of_get_property(np, "xlnx,gpio-width", NULL);
> - if (!tree_info)
> - tree_info = of_get_property(np->parent,
> - "xlnx,gpio-width", NULL);
> - if (tree_info)
> - chip->mmchip.gc.ngpio = be32_to_cpup(tree_info);
> + of_property_read_u32(np, "xlnx,gpio-width",
> + (u32 *)&chip->mmchip.gc.ngpio);
>
> spin_lock_init(&chip->gpio_lock);
>
> --
> 1.8.2.3
>
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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
Hi Linus,
can you please look at this?
Thanks,
Michal
On 06/03/2013 02:31 PM, Michal Simek wrote:
> Simplification is done by using OF helper function
> which increase readability of code and remove
> (if (var) var = be32_to_cpup;) assignment.
>
> Signed-off-by: Michal Simek <[email protected]>
> ---
> Changes in v2:
> - New patch in this series
>
> drivers/gpio/gpio-xilinx.c | 24 ++++++++++--------------
> 1 file changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
> index 9ae7aa8..2aad534 100644
> --- a/drivers/gpio/gpio-xilinx.c
> +++ b/drivers/gpio/gpio-xilinx.c
> @@ -170,24 +170,20 @@ static int xgpio_of_probe(struct device_node *np)
> return -ENOMEM;
>
> /* Update GPIO state shadow register with default value */
> - tree_info = of_get_property(np, "xlnx,dout-default", NULL);
> - if (tree_info)
> - chip->gpio_state = be32_to_cpup(tree_info);
> + of_property_read_u32(np, "xlnx,dout-default", &chip->gpio_state);
> +
> + /* By default, all pins are inputs */
> + chip->gpio_dir = 0xFFFFFFFF;
>
> /* Update GPIO direction shadow register with default value */
> - chip->gpio_dir = 0xFFFFFFFF; /* By default, all pins are inputs */
> - tree_info = of_get_property(np, "xlnx,tri-default", NULL);
> - if (tree_info)
> - chip->gpio_dir = be32_to_cpup(tree_info);
> + of_property_read_u32(np, "xlnx,tri-default", &chip->gpio_dir);
> +
> + /* By default assume full GPIO controller */
> + chip->mmchip.gc.ngpio = 32;
>
> /* Check device node and parent device node for device width */
> - chip->mmchip.gc.ngpio = 32; /* By default assume full GPIO controller */
> - tree_info = of_get_property(np, "xlnx,gpio-width", NULL);
> - if (!tree_info)
> - tree_info = of_get_property(np->parent,
> - "xlnx,gpio-width", NULL);
> - if (tree_info)
> - chip->mmchip.gc.ngpio = be32_to_cpup(tree_info);
> + of_property_read_u32(np, "xlnx,gpio-width",
> + (u32 *)&chip->mmchip.gc.ngpio);
>
> spin_lock_init(&chip->gpio_lock);
>
> --
> 1.8.2.3
>
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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
On Mon, Jun 3, 2013 at 2:31 PM, Michal Simek <[email protected]> wrote:
> Simplification is done by using OF helper function
> which increase readability of code and remove
> (if (var) var = be32_to_cpup;) assignment.
>
> Signed-off-by: Michal Simek <[email protected]>
> ---
> Changes in v2:
> - New patch in this series
Patch applied. Nice cleanup!
Yours,
Linus Walleij
On Mon, Jun 3, 2013 at 2:31 PM, Michal Simek <[email protected]> wrote:
> Supporting the second channel in the driver.
> Offset is 0x8 and both channnels share the same
> IRQ.
>
> Signed-off-by: Michal Simek <[email protected]>
>
> ---
> Changes in v2:
> - Use kernel doc format - suggested by Linus Walleij
> - Do not use __raw_readl/__raw_writel IO in this patch
> - Use of_property_read_u32 helper function
> - Use BIT()
> - Change patch subject
Patch is looking overall nice and improves the kernel so
applied.
But check this:
> @@ -202,6 +230,57 @@ static int xgpio_of_probe(struct device_node *np)
> np->full_name, status);
> return status;
> }
> +
> + 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);
> + if (tree_info && be32_to_cpup(tree_info)) {
Doesn't this work:
if (of_property_read_bool(np, "xlnx,is-dual")) {
(...)
?
Yours,
Linus Walleij
On Mon, Jun 3, 2013 at 2:31 PM, Michal Simek <[email protected]> wrote:
> This driver can be used on Xilinx ARM Zynq platform
> where in_be32/out_be32 functions are not implemented.
> Use __raw_readl/__raw_writel functions which are
> implemented on Microblaze and PowerPC.
> For ARM readl/writel functions are used instead.
>
> The correct way how to implement this is to detect
> endians directly on IP. But for the gpio case
> without interrupt connected(it means without
> interrupt logic) there are just 2 registers
> data and tristate where auto detection can't be done.
>
> Signed-off-by: Michal Simek <[email protected]>
> ---
> Changes in v2:
> - New patch in this series
>
> I have chosen to use readl/writel for ARM because
> of barriers and keep __raw versions for microblaze
> and ppc where this is not a problem.
> The reason why in_be32/out_be32 can't be used
> is that it won't work on Microblaze LE when
> I fix Microblaze IO function implementation which
> is broken right now.
Makes perfect sense.
Patch applied.
Yours,
Linus Walleij
On Mon, Jun 3, 2013 at 2:31 PM, Michal Simek <[email protected]> wrote:
> Use BIT macro from linux/bitops.h.
>
> Signed-off-by: Michal Simek <[email protected]>
> ---
> Changes in v2:
> - New patch in this series suggested by Linus Valleij
Patch applied.
Yours,
Linus Walleij
On Mon, Jun 3, 2013 at 2:31 PM, Michal Simek <[email protected]> wrote:
> Describe gpio-xilinx binding.
>
> Signed-off-by: Michal Simek <[email protected]>
> ---
> Changes in v2:
> - Extend description
Thanks, patch applied but look into this:
> +Optional properties:
> +- interrupts : Interrupt mapping for GPIO IRQ.
> +- interrupt-parent : Phandle for the interrupt controller that
> + services interrupts for this device.
> +- xlnx,all-inputs : if n-th bit is setup, GPIO-n is input
> +- xlnx,dout-default : if n-th bit is 1, GPIO-n default value is 1
> +- xlnx,gpio-width : gpio width
> +- xlnx,tri-default : if n-th bit is 1, GPIO-n is in tristate mode
> +- xlnx,is-dual : if 1, controller also uses the second channel
If is present, xlnx,is-dual;
> +Example:
> +gpio: gpio@40000000 {
> + #gpio-cells = <2>;
> + compatible = "xlnx,xps-gpio-1.00.a";
> + gpio-controller ;
> + interrupt-parent = <µblaze_0_intc>;
> + interrupts = < 6 2 >;
> + reg = < 0x40000000 0x10000 >;
> + xlnx,all-inputs = <0x0>;
> + xlnx,all-inputs-2 = <0x0>;
> + xlnx,dout-default = <0x0>;
> + xlnx,dout-default-2 = <0x0>;
> + xlnx,gpio-width = <0x2>;
> + xlnx,gpio2-width = <0x2>;
> + xlnx,interrupt-present = <0x1>;
> + xlnx,is-dual = <0x1>;
xlnx,is-dual;
I'm not giving up on this suggestion.
Test it please...
Yours,
Linus Walleij
On Mon, Jun 3, 2013 at 2:31 PM, Michal Simek <[email protected]> wrote:
> Enable gpio driver for usage on Xilinx ARM zynq platform.
>
> Signed-off-by: Michal Simek <[email protected]>
> ---
> Changes in v2:
> - New patch in this series
Patch applied.
Yours,
Linus Walleij
On 06/17/2013 07:50 AM, Linus Walleij wrote:
> On Mon, Jun 3, 2013 at 2:31 PM, Michal Simek <[email protected]> wrote:
>
>> Describe gpio-xilinx binding.
>>
>> Signed-off-by: Michal Simek <[email protected]>
>> ---
>> Changes in v2:
>> - Extend description
>
> Thanks, patch applied but look into this:
>
>> +Optional properties:
>> +- interrupts : Interrupt mapping for GPIO IRQ.
>> +- interrupt-parent : Phandle for the interrupt controller that
>> + services interrupts for this device.
>> +- xlnx,all-inputs : if n-th bit is setup, GPIO-n is input
>> +- xlnx,dout-default : if n-th bit is 1, GPIO-n default value is 1
>> +- xlnx,gpio-width : gpio width
>> +- xlnx,tri-default : if n-th bit is 1, GPIO-n is in tristate mode
>> +- xlnx,is-dual : if 1, controller also uses the second channel
>
> If is present, xlnx,is-dual;
>
>> +Example:
>> +gpio: gpio@40000000 {
>> + #gpio-cells = <2>;
>> + compatible = "xlnx,xps-gpio-1.00.a";
>> + gpio-controller ;
>> + interrupt-parent = <µblaze_0_intc>;
>> + interrupts = < 6 2 >;
>> + reg = < 0x40000000 0x10000 >;
>> + xlnx,all-inputs = <0x0>;
>> + xlnx,all-inputs-2 = <0x0>;
>> + xlnx,dout-default = <0x0>;
>> + xlnx,dout-default-2 = <0x0>;
>> + xlnx,gpio-width = <0x2>;
>> + xlnx,gpio2-width = <0x2>;
>> + xlnx,interrupt-present = <0x1>;
>> + xlnx,is-dual = <0x1>;
>
> xlnx,is-dual;
>
> I'm not giving up on this suggestion.
I have commented this in the v1. The point is that all dtses for the last
3-4 years are using this binding and this value is present there for a long
time. That's why I think it is better to keep this binding and do not break
backward compatibility which there is.
Also it reflects how design tool describe hardware.
IP is designed that can be easily extended to more channels which means
that it won't be simple yes/no option.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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
On Mon, Jun 17, 2013 at 8:21 AM, Michal Simek <[email protected]> wrote:
> On 06/17/2013 07:50 AM, Linus Walleij wrote:
>> On Mon, Jun 3, 2013 at 2:31 PM, Michal Simek <[email protected]> wrote:
>>> +- xlnx,tri-default : if n-th bit is 1, GPIO-n is in tristate mode
>>> +- xlnx,is-dual : if 1, controller also uses the second channel
>>
>> If is present, xlnx,is-dual;
>>
>>> + xlnx,is-dual = <0x1>;
>>
>> xlnx,is-dual;
>>
>> I'm not giving up on this suggestion.
>
> I have commented this in the v1.
I commented your comment on v1, and said I think you can support
both bindings.
Yours,
Linus Walleij
Hi Linus,
On 06/17/2013 11:13 AM, Linus Walleij wrote:
> On Mon, Jun 17, 2013 at 8:21 AM, Michal Simek <[email protected]> wrote:
>> On 06/17/2013 07:50 AM, Linus Walleij wrote:
>>> On Mon, Jun 3, 2013 at 2:31 PM, Michal Simek <[email protected]> wrote:
>
>>>> +- xlnx,tri-default : if n-th bit is 1, GPIO-n is in tristate mode
>>>> +- xlnx,is-dual : if 1, controller also uses the second channel
>>>
>>> If is present, xlnx,is-dual;
>>>
>>>> + xlnx,is-dual = <0x1>;
>>>
>>> xlnx,is-dual;
>>>
>>> I'm not giving up on this suggestion.
>>
>> I have commented this in the v1.
>
> I commented your comment on v1, and said I think you can support
> both bindings.
in 2/6 you have applied that dual support for this driver
and that's why please add this binding description to your repo
because it reflects actual binding for this driver.
As I wrote you I am working on interrupt support for this IP
and in connection to this I will introduce new binding
as we discussed in v1.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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
On Mon, Jun 24, 2013 at 1:54 PM, Michal Simek <[email protected]> wrote:
>> I commented your comment on v1, and said I think you can support
>> both bindings.
>
> in 2/6 you have applied that dual support for this driver
> and that's why please add this binding description to your repo
> because it reflects actual binding for this driver.
I applied this patch ages ago I think?
This discussion is about what I'd like to see as new changes...
Yours,
Linus Walleij