Re: [RFC PATCH 0/4] Add support for QMC HDLC and PHY

[Herve Codina <herve.codina@bootlin.com>]

Hi Andrew,

On Mon, 17 Apr 2023 19:22:26 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

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

I can move to a basic framer abstraction as you suggested. At least:
- devm_of_framer_optional_get()
- devm_framer_create()
- framer_notifier_register() or something similar.

Where do you expect to see this framer abstraction and the pef2256
framer part ?
driver/net/wan/framer/, other place ?
I think driver/net/wan/framer/ can be a good place to start as only HDLC
will use this abstraction.

I can use the framer abstraction from the QMC HDLC driver itself or try
to move it to the HDLC core. Do you think it will be interesting to have
it move to the HDLC core ?

Best regards,
Hervé

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com