On Wed, May 29, 2013 at 1:27 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]>
(...)
> +/* 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?
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?
I'd prefer this step to be a separate patch.
> 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.
> - 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().
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.
> + 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.
I don't exactly like this. Is this necessary?
> + /* 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.
Yours,
Linus Walleij
Hi Linus,
On 05/30/2013 09:46 PM, Linus Walleij wrote:
> On Wed, May 29, 2013 at 1:27 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]>
>
> (...)
>> +/* 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: 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 Fri, May 31, 2013 at 7:43 AM, Michal Simek <[email protected]> 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-kernel&m=136982686732560&w=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
On 05/31/2013 09:14 AM, Linus Walleij wrote:
> On Fri, May 31, 2013 at 7:43 AM, Michal Simek <[email protected]> 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-kernel&m=136982686732560&w=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: 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 Fri, May 31, 2013 at 9:34 AM, Michal Simek <[email protected]> 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
On 06/17/2013 07:29 AM, Linus Walleij wrote:
> On Fri, May 31, 2013 at 9:34 AM, Michal Simek <[email protected]> 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: 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 Thu, Jun 20, 2013 at 9:51 AM, Michal Simek <[email protected]> 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
On 06/20/2013 11:23 AM, Linus Walleij wrote:
> On Thu, Jun 20, 2013 at 9:51 AM, Michal Simek <[email protected]> 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: 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 Thu, Jun 20, 2013 at 12:59 PM, Michal Simek <[email protected]> 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
On 06/20/2013 01:33 PM, Linus Walleij wrote:
> On Thu, Jun 20, 2013 at 12:59 PM, Michal Simek <[email protected]> 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: 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 Thu, Jun 20, 2013 at 2:12 PM, Michal Simek <[email protected]> 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