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

[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.simek@xxxxxxxxxx> wrote:
> 
>> Supporting the second channel in the driver.
>> Offset is 0x8 and both channnels share the same
>> IRQ.
>>
>> Signed-off-by: Michal Simek <michal.simek@xxxxxxxxxx>
> 
> (...)
>> +/* 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 = 0xFFFFFFFF;
>> +               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 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

Attachment: signature.asc
Description: OpenPGP digital signature