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

[Robert Marko <robert.marko@xxxxxxxxxx>]

On Wed, Jul 21, 2021 at 4:17 PM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>
> 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?

Hi Linus,
The pins that this driver wants to expose are used for SFP-s only,
they are provided by the Lattice CPLD which also does other things.

Linux has a generic SFP driver which is used to manage these SFP
ports, but it only supports GPIO-s, it has no concept of anything else.
Since the driver is fully generic, I have no idea how could one extend it
to effectively handle these pins internally, especially since I have more
switches that use the CPLD for SFP-s as well, even for 48 ports and 192
pins for them.

GPIO regmap works perfectly for this as its generic enough to cover all of
these cases.
CPLD also provides pins to test the port LED-s per color as well,
but I have chosen not to expose them so far.

>
> 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.

Yes, its regmap but I cant really pass it to the SFP driver as I don't have
special driver handling the SFP but rather the generic kernel one.
It only knows how to handle GPIO-s.

>
> 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.

Ok, makes sense, I will make it clear 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.

Ok, makes sense, I will make it clear in the bindings.
>
> But if the lines are special purpose not general purpose, they
> should not be GPIOs to begin with.
>
> Yours,
> Linus Walleij

Looking forward to your reply.

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