Date:	Thu, 20 Jun 2013 14:12:23 +0200
From:	Michal Simek <monstr@...str.eu>
To:	Linus Walleij <linus.walleij@...aro.org>
CC:	Michal Simek <monstr@...str.eu>,
	Michal Simek <michal.simek@...inx.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <rob.herring@...xeda.com>,
	"devicetree-discuss@...ts.ozlabs.org" 
	<devicetree-discuss@...ts.ozlabs.org>,
	Jean-Christophe PLAGNIOL-VILLARD <plagnioj@...osoft.com>
Subject: Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

On 06/20/2013 01:33 PM, Linus Walleij wrote:
> On Thu, Jun 20, 2013 at 12:59 PM, Michal Simek <monstr@...str.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



Download attachment "signature.asc" of type "application/pgp-signature" (264 bytes)

Powered by blists - more mailing lists