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

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Tue, Jul 20, 2021 at 12:59 AM Rob Herring <robh@xxxxxxxxxx> wrote:

> > > > Are there any issues with the bindings?
> > >
> > > Yes. Primarily the GPIO function being part of the compatible. I'm
> > > surprised Linus W is okay with that.
> >
> > I think I already explained this before, having a single compatible
> > won't work here.
> > Then there would not be anything to know whether its input or output
> > only as the pins
> > have specific purpose.
>
> The client side should tell the direction. Are you using the SFP
> binding?: Documentation/devicetree/bindings/net/sff,sfp.txt
>
> Specific purpose IOs are not general purpose IOs. Repeating Linus W
> here. Maybe his opinion has evolved...

Nah. I think at one time or two I was convinced to let something
special purpose slip through as "GPIO".

Typical case: LED control lines that were in practice used for other
things, such as controlling motors.

Here there is a pin named "SFP TX disable" which is suspicious.
Why isn't whatever is now managing SFP just read/write this bit
without going through the GPIO abstraction to disable TX?

If it is a regmap in Linux then that is fine, just pass the regmap
around inside the kernel, OK finished. But really that is an OS
detail.

If the pin is in practice used for other things, say connected
to a LED, I would soften up and accept it as a GPIO compatible.

> If the programming model of each instance is different, then different
> compatibles are justified. But describe what the difference is, not the
> connection.

IIRC that is the case as the instances are different.

So those differences should be described for each compatible in the
bindings.

So there is this:

> +  GPIO controller module provides GPIO-s for the SFP slots.
> +  It is split into 3 controllers, one output only for the SFP TX disable
> +  pins, one input only for the SFP present pins and one input only for
> +  the SFP LOS pins.

This should read "the hardware instances are different in such way
that the first can only (by hardware restrictions) be used as output..."
etc, so that it is crystal clear what this means.

But if the lines are special purpose not general purpose, they
should not be GPIOs to begin with.

Yours,
Linus Walleij