Re: [PATCHv6 0/4] n_gsm serdev support and protocol driver for droid4 modem

[Tony Lindgren <tony@atomide.com>]

* Johan Hovold <johan@kernel.org> [200423 11:44]:
> Hi Tony,
> 
> On Tue, Apr 21, 2020 at 04:27:48PM -0700, Tony Lindgren wrote:
> > Hi all,
> > 
> > Here's v4 set of n_gsm serdev support patches, and the related protocol
> > driver for the modem found on Motorola Mapphone phones and tablets
> > like droid4.
> > 
> > This series only adds basic character device support for the serdev
> > driver. Other serdev consumer drivers for specific devices will be
> > posted separately.
> 
> I'm still missing an architectural (design) overview here -- reviewer
> time is a scarce resource.

OK, so for this series it would be roughly:

1. Allow serdev drivers configure and use n_gsm for TS 27.010

2. Add support for a motorola modem with a custom protocol on
   top of TS 27.010 with only chardev support initially

3. Start adding TS 27.010 channel specific device drivers as
   regular Linux device drivers later on

> I also suggested earlier that you include, at least as an RFC, one or
> more of your child-device drivers so that we can see how this ends up
> being used in the end (including an example devicetree).

Well I left that out intentionally to keep the series smaller. But yeah,
I guess that part is really what people want. I'll add a basic GNSS
driver for the next version.

> Some high-level comments until then:
> 
> I'm not sure that a plain chardev interface for the mux channels is the
> right interface. The n_gsm ldisc exposes tty devices and I think your
> serdev adaptation should continue to do that.

That's what I started with, but was not happy at all because of the
custom motorola protocol layer on top of TS 27.010.

A generic n_gsm serdev driver should work easily though. Roughly, the
n_gsm serdev driver(s) wanting to use just the n_gsm tty devices would
just call the n_gsm config functions only, and configure no custom
callbacks. Maybe some flag is needed for n_gsm tty. This needs to
be checked again though as I have not done that for a while.

Then for the custom n_gsm serdev implementations, like this modem,
we have packet data from n_gsm. I don't think stuffing the packet data
back into tty devices helps for these cases.

> On that note; you're not actually adding general TS 27.010 serdev
> support, but rather some hooks and a custom driver and interface (mfd +
> /dev/motmdmN) for one particular modem.

A generic n_gsm serdev driver would just use a subset of these same
functions.

> I'd rather see a generic implementation which can be used with other
> modems and that continues to expose a /dev/gsmttyN interface to which we
> could attach serdev clients instead (and not create a motmdm serdev
> replica of sorts).

Yeah this should be doable quite easily actually without really any of
the motorola driver code. It's a separate driver though, and not
usable for this case because of the custom layer.

> I know the location of this driver has been up for discussion already,
> but drivers/tty/serdev/protocol still isn't right (e.g. we don't have an
> drivers/i2c/protocol directory where we stuff random i2c client
> drivers).

Argh, the location of driver again.. So we do have the custom motorola
layer to deal with on top of TS 27.010, but the custom handling is
contained within the driver. So maybe just drivers/serial for the
custom driver then.

> It's an mfd + custom chardev driver for a modem and related to n_gsm
> (even more if you add generic serdev support). Currently, drivers/mfd or
> drivers/misc appear to be better choices. Otherwise, n_gsm lives in
> drivers/tty since it's a line discipline, but it could be moved to a new
> drivers/modem if needed (cf. the bluetooth hci ldisc).

The n_gsm suport is not limited to modems only. Probably best to
not move it. And I'd avoid drivers/modem until we actually have three
similar use cases based on the late David "Mr.Bus" Brownell rule of
thumb :)

> Last, it seems you've based the serdev-ngsm-motmdm.c chardev
> implementation on a more or less verbatim copy of drivers/gnss/core.c.
> I'd appreciate if you could mention that in the file header and
> reproduce the copyright notice if you end up keeping that interface.

Oh yes indeed, thanks for pointing that out. I'll add it to the next
version. The chardev code is for sure based on drivers/gnss.

To explain my ignorance, I added the chardev support initially as an
experiment to see if I can handle the motorola packet layer better
that way compared to the n_gsm ttys and userspace handling. It ended
up working quite nicely, so I kept it but then I accidentally left
out references to the source. Sorry about that.

Regards,

Tony