From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 17 Oct 2018 14:51:29 -0500 From: Rob Herring Subject: Re: [PATCH] dt-bindings: gpio: altera-fpga-mgr: Add Altera FPGA manager GPIO bindings Message-ID: <20181017195129.GA21054@bogus> References: <20181010192315.26163-1-marex@denx.de> <8e09ecc2-9757-4b59-bcf4-8c9ad62986d4@denx.de> <0fcdb6a1-f9c9-5af7-64b3-141d0f80c8fb@denx.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <0fcdb6a1-f9c9-5af7-64b3-141d0f80c8fb@denx.de> To: Marek Vasut Cc: Linus Walleij , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" List-ID: On Tue, Oct 16, 2018 at 11:26:02AM +0200, Marek Vasut wrote: > On 10/16/2018 09:37 AM, Linus Walleij wrote: > > On Fri, Oct 12, 2018 at 5:12 PM Marek Vasut wrote: > >> On 10/11/2018 10:44 AM, Linus Walleij wrote: > >>> On Wed, Oct 10, 2018 at 9:23 PM Marek Vasut wrote: > >>> > >>>> Add DT bindings for the GPI / GPO block in the Altera SoCFPGA FPGA manager. > >>>> The GPIO block in the FPGA manager has two 32bit registers, one for setting > >>>> 32 GPOs and another one for reading 32 GPIs, both of which can be mapped to > >>>> separate physical pads. > >>>> > >>>> Signed-off-by: Marek Vasut > >>>> Cc: Rob Herring > >>>> Cc: Linus Walleij > >>> (...) > >>> > >>>> +- gpio,syscon-dev: phandle/offset pair. The phandle to syscon used to > >>>> + access device state control registers and the offset of device's specific > >>>> + registers within device state control registers range. > >>> (...) > >>>> + gpio,syscon-dev = <&fpgamgr0 0x10>; > >>> > >>> I didn't see that before. > >>> > >>> It is usually not a good idea to encode register offsets into the > >>> device tree. > >>> > >>> I think the register offset should be in the driver and determined > >>> from the compatible-string. If that is not possible, the compatible > >>> strings do not really indicate compatibility, if you see what I mean. > >>> > >>> As for the name of the variable, why not just use: > >>> > >>> syscon = <&fpgamgr0>; > >>> > >>> It seems simple enough without any gpio,* prefix or explicitly > >>> suffixing it with a "-dev" - every node in the device tree is a device > >>> by definition so skip that. > >> > >> Isn't it better to just have one compatible string for all SoCFPGAs and > >> handle the possible difference in offset where the registers are in DT? > >> It's the same as "reg" property which we use to describe where a certain > >> block is in the address space. > > > > You have a point. > > > > What about: > > > > syscon = <&fpgamgr0>; > > reg = <0x10>; > > ? > > > > You can just parse out "reg" in the driver. > > > > I mean reg is intuitively for that, so... > > But drivers/gpio/gpio-syscon.c already parses the gpio,syscon-dev > binding, including the register offset. Why reinvent a new one if there > already is one which fits perfectly ? :) Maybe so, but it is undocumented. And if you look at the Rockchip addition to the driver, it was done a different way. Can't this be a child of the fpgamgr? Rob