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

[Michal Simek <monstr-pSz03upnqPeHXe+LvDLADg@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2105 bytes --]

On 06/20/2013 11:23 AM, Linus Walleij wrote:
> On Thu, Jun 20, 2013 at 9:51 AM, Michal Simek <monstr-pSz03upnqPeHXe+LvDLADg@public.gmane.org> 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



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss