Phone-Devel Archive on lore.kernel.org.
 help / color / Atom feed
From: Johan Hovold <johan@kernel.org>
To: Pavel Machek <pavel@ucw.cz>
Cc: kernel list <linux-kernel@vger.kernel.org>,
	phone-devel@vger.kernel.org, tony@atomide.com
Subject: Re: [PATCHv2] gnss: motmdm: Add support for Motorola Mapphone MDM6600 modem
Date: Thu, 1 Apr 2021 11:39:57 +0200
Message-ID: <YGWU7U1fIkDmc3Mf@hovoldconsulting.com> (raw)
In-Reply-To: <20210228204601.GA20966@duo.ucw.cz>


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

On Sun, Feb 28, 2021 at 09:46:01PM +0100, Pavel Machek wrote:
> Hi!
> 
> > Motorola is using a custom TS 27.010 based multiplexer protocol
> > for various devices on the modem. These devices can be accessed on
> > dedicated channels using Linux kernel serdev-ngsm driver.
> > 
> > For the GNSS on these devices, we need to kick the GNSS device at a
> > desired rate. Otherwise the GNSS device stops sending data after a
> > few minutes. The rate we poll data defaults to 1000 ms, and can be
> > specified with a module option rate_ms between 1 to 16 seconds.
> > 
> > [Tony Lindgren did most of the work here, I just converted it to be
> > normal serdev.]
> 
> 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.

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

> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> Best regards,
> 								Pavel
> 
> diff --git a/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml b/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml
> new file mode 100644
> index 000000000000..deec491ee821
> --- /dev/null
> +++ 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.

> +examples:
> +  - |
> +    modem {
> +      compatible = "motorola,mapphone-mdm6600-serial";
> +      ttymask = <0 0x00001fee>;
> +      phys = <&fsusb1_phy>;
> +      phy-names = "usb";
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      gnss@4 {
> +         compatible = "motorola,mapphone-mdm6600-gnss";
> +         reg = <4>;
> +      };
> +    };

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

This should instead be something like below; I'm including one level
above for the full picture:

	serial@nnn {
		compatible = "ns16550a";
		reg = <nnn>;

		modem {
			compatible = "ts27010-mux, motorola,mapphone-mdm6600-serial";

			serial@4 {
				reg = <4>

				gnss {
					compatible = "motorola,mapphone-mdm6600-gnss";
				};
			};
		};
	};

Here serial@nnn is a normal serial port, modem is the modem (TS 27.010
mux) connected to the serial port, serial@4 is virtual port number four
of the mux, and gnss is the gnss device connected to the virtual port.

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.

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

> +int gsm_serdev_register_tty_port(struct gsm_serdev *gsd, int line)
> +{
> +	struct gsm_serdev_dlci_operations *ops;
> +	unsigned int base;
> +	int error;
> +	struct device *dev;
> +	struct device_node *node;
> +	struct device *dev2 = NULL;
> +	struct device *ndev;
> +		
> +	if (line < 1)
> +		return -EINVAL;
> +
> +	ops = kzalloc(sizeof(*ops), GFP_KERNEL);
> +	if (!ops)
> +		return -ENOMEM;
> +
> +	ops->line = line;
> +	ops->receive_buf = gsd_dlci_receive_buf;
> +
> +	error = gsm_serdev_register_dlci(gsd, ops);
> +	if (error)
> +		goto error;
> +
> +	base = mux_num_to_base(gsd->gsm);
> +	printk("register_tty_port: have port: %p, %d+%d\n", &gsd->gsm->dlci[line]->port, base, ops->line);
> +	dev = &gsd->serdev->dev;
> +
> +	for_each_available_child_of_node(dev->of_node, node) {
> +		struct platform_device_info devinfo = {};
> +		static int idx;
> +		struct platform_device *pdev;
> +		const char *c = of_get_property(node, "compatible", NULL);
> +		int reg;
> +		
> +		dev_info(dev, "register_tty: child -- %pOF -- compatible %s\n", node, c);
> +		
> +		if (!c)
> +			continue;
> +#ifdef OLD		
> +		if (strcmp(c, "gsmmux,port"))
> +			continue;
> +#endif
> +		if (of_property_read_u32(node, "reg", &reg)) {
> +			printk("no reg property\n");
> +			continue;
> +		}
> +
> +		if (reg != line)
> +			continue;
> +
> +		printk("n_gsm: line %d reg is %d, compatible %s\n", line, reg, c);
> +		/* Hmm, gnss does not work now? */
> +		/* Should we pass the "master" serdev here? */
> +#ifndef OLD
> +		/* does not work, controller is "BUSY". We already have that in variable "dev" */
> +		dev2 = dev;
> +#else
> +		devinfo.name = kasprintf(GFP_KERNEL, "gsm-mux-%d", idx++);
> +		devinfo.parent = dev;
> +
> +		/* Thanks to core.c: serdev_device_add */
> +		pdev = platform_device_register_full(&devinfo);
> +		pdev->dev.of_node = node;
> +
> +		dev2 = &pdev->dev;
> +		printk("device name is %s / %s\n", dev2->init_name, dev2->kobj.name);
> +#endif
> +		break;
> +	}
> +
> +	printk("n_gsm: Registering device at line %d, device is %p\n", line, dev2);
> +	ndev = tty_port_register_device_serdev(&gsd->gsm->dlci[line]->port,
> +					       gsm_tty_driver, base + ops->line, dev2);
> +	if (dev2)
> +		printk("device name is now %s / %s\n", dev2->init_name, dev2->kobj.name);
> +	
> +	printk("register_tty_port: got %p\n", ndev);
> +	return 0;
> +error:
> +	kfree(ops);
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(gsm_serdev_register_tty_port);

Johan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

  reply index

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-07 22:45 [PATCH] " Pavel Machek
2021-01-28 18:34 ` Pavel Machek
     [not found] ` <YBQvvUitX4MtRrh+@hovoldconsulting.com>
2021-01-29 22:34   ` Pavel Machek
     [not found]   ` <20210131170639.GA21067@duo.ucw.cz>
2021-02-10 21:28     ` [RFC/context] add serdev interfaces to n_gsm Pavel Machek
2021-02-11  9:04       ` Johan Hovold
2021-02-19 22:06         ` Pavel Machek
2021-01-29 22:42 ` [PATCHv2] gnss: motmdm: Add support for Motorola Mapphone MDM6600 modem Pavel Machek
2021-02-28 20:46   ` Pavel Machek
2021-04-01  9:39     ` Johan Hovold [this message]
2021-04-07 10:52       ` Pavel Machek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YGWU7U1fIkDmc3Mf@hovoldconsulting.com \
    --to=johan@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pavel@ucw.cz \
    --cc=phone-devel@vger.kernel.org \
    --cc=tony@atomide.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Phone-Devel Archive on lore.kernel.org.

Archives are clonable:
	git clone --mirror https://lore.kernel.org/phone-devel/0 phone-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 phone-devel phone-devel/ https://lore.kernel.org/phone-devel \
		phone-devel@vger.kernel.org
	public-inbox-index phone-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.phone-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git