On Fri, May 31, 2013 at 7:43 AM, Michal Simek <mon...@monstr.eu> 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 _______________________________________________ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
