| Message ID | 20220131133049.77780-1-robert.marko@sartura.hr |
|---|---|
| Headers | show |
| Series | Add Delta TN48M CPLD support | expand |
On Wed, Mar 02, 2022 at 08:47:32AM +0000, Lee Jones wrote: > On Mon, 31 Jan 2022, Robert Marko wrote: > > > Add binding documents for the Delta TN48M CPLD drivers. > > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr> > > This is missing a DT review. How about this one[1]? Rob [1] https://lore.kernel.org/all/20210719225906.GA2769608@robh.at.kernel.org/
On Fri, Mar 4, 2022 at 10:47 PM Rob Herring <robh@kernel.org> wrote: > > On Thu, Mar 03, 2022 at 01:41:13PM +0100, Robert Marko wrote: > > On Wed, Mar 2, 2022 at 10:39 PM Rob Herring <robh@kernel.org> wrote: > > > > > > On Wed, Mar 02, 2022 at 08:47:32AM +0000, Lee Jones wrote: > > > > On Mon, 31 Jan 2022, Robert Marko wrote: > > > > > > > > > Add binding documents for the Delta TN48M CPLD drivers. > > > > > > > > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr> > > > > > > > > This is missing a DT review. > > > > > > How about this one[1]? > > > > > > Rob > > > > > > [1] https://lore.kernel.org/all/20210719225906.GA2769608@robh.at.kernel.org/ > > > > Hi Rob, > > Thanks for reaching out. > > > > As you can see the bindings have evolved since v6, > > GPIO driver now only uses 2 distinct compatibles. > > Fundamentally, it hasn't really changed. > > There's 2 main issues. First, I don't see the need for any child nodes. > This would be sufficient: > > cpld@41 { > compatible = "delta,tn48m-cpld"; > reg = <0x41>; > #reset-cells = <1>; > #gpio-cells = <2>; > gpio-controller; > }; > > You only need child nodes if the sub-blocks have their own resources or > are widely reused in different configurations. > > The 2nd issue is whether GPIOs are even GPIOs at all. I don't recall > that Linus ever agreed. > > Both issues kind of boil down to is there even more that 1 variation of > this h/w where you have differing connections? AFAICT, Delta tn48m is a > pretty specific device and I would guess something implemented in a CPLD > is likely to change on every board design. At least that's my experience > with 'board level logic'. Hi Rob, sorry for the late reply. Having one node was the route I went in v1, but that was rejected as it would mean having an MFD driver that just registers the sub-drivers. That is what the simple-mfd-i2c driver was designed to get rid of and inherit the regmap from the parent. For this to work, subnodes are required as we need to match on compatibles. Using subnodes for GPIO-s also gets rid of hardcoding the register layout in the driver per board, as Delta chose to use a weird register layout in which the GPIO registers have completely random offsets. The layout is even weirder in the TN4810M which uses the same CPLD but expanded and is easily supportable in the same driver in the current form. My goal and the requirement from the community was to make the GPIO driver as simple as possible and extendable so that boards like TN4810M can be easily added. Also, the Kontron SL28CPLD does pretty much the same in regards to DT as well as other things. https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Documentation/devicetree/bindings/mfd/kontron,sl28cpld.yaml?h=v5.16.15 It uses the same logic with different compatibles for GPIO to be able to inform the kernel of the certain bank capabilities. I mean, using one compatible would be possible by using a boolean property for example that tells you that its output capable as well. Regards, Robert > > Rob