Re: [PATCHv2] gnss: motmdm: Add support for Motorola Mapphone MDM6600 modem

[Pavel Machek <pavel@ucw.cz>]

[-- Attachment #1: Type: text/plain, Size: 2639 bytes --]

Hi!

> > Could I get some comments on the gnss patch? It is fairly simple, and
> > I believe it is ready for merge.
> 
> Let's get the mux driver in shape first.

Well, gnss driver is now completely independent of that in the new
versions.

> > Here's new version of the serdev multiplexing patch for reference. And
> > yes, I'd like you to review the design here, too. Yes,
> > gsm_serdev_register_tty_port() needs to be cleaned up, but with ifdefs
> > you can see alternative approach I tried to work around "controller
> > busy" problem.
> 
> As I said before, if you're trying to work around the one
> client-per-port assumption of serdev, you're doing it wrong. You still
> have one client per *virtual* port.

Yes, I have one client per virtual port. But those virtual ports need
their parents.

We can have

serial@ -- motorola,mapphone-mdm6600-serial
(serdev here)
modem -- ts27010-mux
(here is the problem!)
serial@4 --                                      serial@1 -- 
(serdev here)                                    (serdev here)
gnss -- motorola,mapphone-mdm6600-gnss           modem voice -- something

and I believe we agree on that. But serial@1 and serial@4 still need
some kind of parent, and that's where I'm getting the controller busy
problem.

> > +++ b/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml
> 
> > +  ttymask:
> > +    $ref: /schemas/types.yaml#/definitions/uint64
> > +    description: Mask of the TS 27.010 channel TTY interfaces to start (64 bit)
> 
> How is this intended to be used? Looks too Linux-specific for a binding.

The mask says which virtual ports contain "something". We have kernel
clients for some, and we want to expose the rest of the used ones to
the userspace.

> So is this is probably the issue: you're skipping one level of the
> topology and do not describe the virtual ports.

Well, docs need to be updated.

> You should probably just replace the ttymask property with explicit
> nodes for the virtual ports to enable. With no client devices described
> in devicetree, these will then show up as regular tty devices under
> Linux.

Yes, that can be done.

> If you add a compatible property for the virtual ports (e.g.
> "ts27010-port") you might be able to just use of_platform_populate()
> instead of walking the child nodes in gsm_serdev_register_tty_port().

I have "gsmmux,port" for that. And yes, I can take a look at
of_platform_populate.

But as I explained above, there's still problem with the parent
devices.

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]