Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 17 Apr 2023 19:22:26 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     Herve Codina <herve.codina@bootlin.com>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, Vinod Koul <vkoul@kernel.org>,
        Kishon Vijay Abraham I <kishon@kernel.org>,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        linux-phy@lists.infradead.org,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Subject: Re: [RFC PATCH 0/4] Add support for QMC HDLC and PHY
Message-ID: <9e7fe32a-8125-41d5-9f8e-d3e5c6c64584@lunn.ch>
References: <20230323103154.264546-1-herve.codina@bootlin.com>
 <885e4f20-614a-4b8e-827e-eb978480af87@lunn.ch>
 <20230414165504.7da4116f@bootlin.com>
 <c99a99c5-139d-41c5-89a4-0722e0627aea@lunn.ch>
 <20230417121629.63e97b80@bootlin.com>
 <a2615755-f009-4a21-b464-88ec5e58f32a@lunn.ch>
 <20230417173941.0206f696@bootlin.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230417173941.0206f696@bootlin.com>
Precedence: bulk

> My application (probably a subset of what we can do with E1 lines) does
> not use signaling.
> 
> I don't expose any channel to the user space but just:
> - One hdlc interface using one timeslot.
> - One or more dai links (audio channels) that can be used using ALSA library.

Well, that obviously does what you need for you application, but it
does not sound very generic. I don't think you could build a PBX on
top of that. I'd think you can build an E1 trunk to VoIP gateway.  So
i also have to wounder how generic vs one specific use case your
building blocks are.

Looking at the binding, i assume you could in fact instantiate 32 hdlc
instances? And that 'transparent' actually means DAI? So you can
instantiate as many DAI as you want. If you wanted to implement
signalling, a third type could be added?

> I thought I was providing a "standardised" API between the HDLC device
> and the framer. Maybe it was not as complete as you could expect but I had
> the feeling that it was standardised.

You are abusing the Generic PHY interface, for something which is not
a PHY. Your PHY driver does nothing a PHY driver normally does,
because you keep arguing the MFD does it all. So to me this is the
wrong abstraction.

You need an abstraction of a framer. You do however want a similar API
to generic PHY. devm_of_framer_optional_get() for the consumer of a
framer, which looks in DT for a phandle to a device.
devm_framer_create() which registers a framer provider, your MFD, with
the framer core. The hdlc driver can then get an instance of the
framer, and either put a notifier call block on its chain, or register
a function to be called when there is change in status.

What i also don't like is you have a very static configuration. You
are putting configuration into DT, which i'm surprised Rob Herring and
Krzysztof Kozlowski accepted. How you use E1 slots is not a hardware
property, it is purely software configuration, so should not be in DT.
So there should be a mechanism to configure how slots are used. You
can then dynamically instantiate HDLC interfaces and DAI links. This
is something which should be in the framer core. But since you have
managed to get this binding accepted, you can skip this. But
implementing the basic framer abstraction will give a place holder for
somebody to implement a much more generic solution in the future.

	 Andrew