Re: [PATCH v10 5/6] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Robert Marko]

On Fri, Mar 4, 2022 at 10:47 PM Rob Herring <robh@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > >
> > > > This is missing a DT review.
> > >
> > > How about this one[1]?
> > >
> > > Rob
> > >
> > > [1] https://lore.kernel.org/all/20210719225906.GA2769608@xxxxxxxxxxxxxxxxxx/
> >
> > 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



-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@xxxxxxxxxx
Web: www.sartura.hr