Patchwork [RFC,lora-next,5/5] HACK: net: lora: sx130x: Add PicoCell gateway shim for cdc-acm

login
register
mail settings
Submitter Andreas Färber
Date Jan. 4, 2019, 11:21 a.m.
Message ID <20190104112131.14451-6-afaerber@suse.de>
Download mbox | patch
Permalink /patch/692801/
State New
Headers show

Comments

Andreas Färber - Jan. 4, 2019, 11:21 a.m.
Ignore our device in cdc-acm probing and add a new driver for it,
dispatching to cdc-acm for the actual implementation.

WARNING: It is likely that this VID/PID is in use for unrelated devices.
Only the Product string hints what this really is in current v0.2.1.
Previous code v0.2.0 was using a Semtech VID/PID, but no card shipping
with such firmware is known to me.

While this may seem unorthodox, no internals of the driver are accessed,
just the set of driver callbacks is duplicated as shim.

Use this shim construct to fake DT nodes for serdev on probe.
For testing purposes these nodes do not have a parent yet.
This results in two "Error -2 creating of_node link" warnings on probe.

Cc: Johan Hovold <johan@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 drivers/net/lora/Makefile   |   2 +
 drivers/net/lora/picogw.c   | 337 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/class/cdc-acm.c |   4 +
 3 files changed, 343 insertions(+)
 create mode 100644 drivers/net/lora/picogw.c
Rob Herring - Jan. 4, 2019, 5:07 p.m.
On Fri, Jan 4, 2019 at 5:21 AM Andreas Färber <afaerber@suse.de> wrote:
>
> Ignore our device in cdc-acm probing and add a new driver for it,
> dispatching to cdc-acm for the actual implementation.
>
> WARNING: It is likely that this VID/PID is in use for unrelated devices.
> Only the Product string hints what this really is in current v0.2.1.
> Previous code v0.2.0 was using a Semtech VID/PID, but no card shipping
> with such firmware is known to me.
>
> While this may seem unorthodox, no internals of the driver are accessed,
> just the set of driver callbacks is duplicated as shim.
>
> Use this shim construct to fake DT nodes for serdev on probe.
> For testing purposes these nodes do not have a parent yet.
> This results in two "Error -2 creating of_node link" warnings on probe.

It looks like the DT is pretty static. Rather than creating the nodes
at run-time, can't you create a dts file and build that into your
module.

Rob
Andreas Färber - Jan. 4, 2019, 11:43 p.m.
Hi Rob,

Am 04.01.19 um 18:07 schrieb Rob Herring:
> On Fri, Jan 4, 2019 at 5:21 AM Andreas Färber <afaerber@suse.de> wrote:
>>
>> Ignore our device in cdc-acm probing and add a new driver for it,
>> dispatching to cdc-acm for the actual implementation.
>>
>> WARNING: It is likely that this VID/PID is in use for unrelated devices.
>> Only the Product string hints what this really is in current v0.2.1.
>> Previous code v0.2.0 was using a Semtech VID/PID, but no card shipping
>> with such firmware is known to me.
>>
>> While this may seem unorthodox, no internals of the driver are accessed,
>> just the set of driver callbacks is duplicated as shim.
>>
>> Use this shim construct to fake DT nodes for serdev on probe.
>> For testing purposes these nodes do not have a parent yet.
>> This results in two "Error -2 creating of_node link" warnings on probe.
> 
> It looks like the DT is pretty static. Rather than creating the nodes
> at run-time, can't you create a dts file and build that into your
> module.

Heh, if that were the only issue with this patch... ;)

I had read about that possibility here [1], but it just appeared to give
me a binary blob with no handy documentation on how to parse the
__dtb_XXX_begin..__dtb_XXX_end blob afterwards for assignment to
interface->dev.of_node. Two nodes and one compatible property were
enough to get me started, so that was quickest, given lack of knowledge.

I intentionally left it static to keep error handling and cleanup
manageable for now... Otherwise I'd need to kstrdup()/kzalloc() all
properties so that I can consistently kfree() them again on release.

Note that this was just a PoC, so there are properties missing here:

At least currently Ben's patch [2] (wrongly?) relies on the optional
clock-output-names property if #clock-cells property is specified -
which I did not in this patch. (Thus it'll disable clk_out, which would
break opening the netdev if we wouldn't run into other errors first.)

Any comments on how to best deal with clk names on the driver side would
be appreciated, so that we can just leave the property away here and get
a sane default name. Otherwise we'd need to generate a unique name here.

If #clock-cells were present, the driver would also rely on obtaining a
parent clock, which may be easiest for me to fix in the driver; but
assuming we need it, we'd need a clocks property pointing to phandles.
Wouldn't phandles need to be unique globally in the kernel for lookup?
phandles from a separate .dtb fragment wouldn't seem to tick that box.

(For reference there's also a clk locking issue under discussion at [3],
as well as multiple unresolved Kbuild reports about clk_hw not being
applicable on sparc64 allyesconfigs and m68k allmodconfig that I'm
unsure how to best resolve while keeping the driver broadly usable. Not
using clk would solve above DT worries but would leave us with ugly
driver dependencies across spi and a custom sx130x_radio bus.)

Kconfig may also be a topic to consider for this USB driver - my x86_64
host for instance doesn't have CONFIG_OF, so it might work to manually
allocate such nodes, whereas using API or &of_fwnode_ops (needed?) may
be a problem - although without CONFIG_OF the serdev code probably is
unable to enumerate the nodes in the first place?

And I assume on ACPI platforms hot-pluggable USB devices shouldn't need
a user-overridden ACPI table either - have you thought about some
serdev-specific lookup as fallback when OF and ACPI come up empty?

Your drivers/tty/serdev/core.c:serdev_controller_add() has access to
ctrl->dev->parent, so it could maintain a list of callbacks that drivers
(e.g., cdc-acm) could register callbacks with and cast the device here
to usb_interface; my module here would then only need to register such a
callback against cdc-acm in its module init to allow cdc-acm to provide
it with the usb_interface, where it could then check for the iProduct to
determine whether that device should be serdev-controlled or not - say
by returning 0 to bind, negative error to ignore - and loading/creating
an internal of_node or whatever necessary. Just a rough idea for now...

Even easier, serdev_device_driver could just get an optional callback!
Then my driver in 3/5 could just determine itself which device it wants
to bind against and still use the module_serdev_device_driver() macro.
(serdev is built-in, so not as easy to tweak on random boards here...)

Any comments on serdev in 4/5? I wonder whether that was an oversight
(in which case it should get a Fixes line) or an intentional choice due
to issues? You mentioned hangup and open/close mismatches before...

Thanks,
Andreas

[1] https://elinux.org/Device_Tree_Linux#FDT_built_into_kernel_as_data
[2] https://patchwork.ozlabs.org/patch/983173/
[3]
https://lists.infradead.org/pipermail/linux-lpwan/2019-January/000069.html
Johan Hovold - Jan. 7, 2019, 3:28 p.m.
On Sat, Jan 05, 2019 at 12:43:48AM +0100, Andreas Färber wrote:
> Hi Rob,
> 
> Am 04.01.19 um 18:07 schrieb Rob Herring:
> > On Fri, Jan 4, 2019 at 5:21 AM Andreas Färber <afaerber@suse.de> wrote:
> >>
> >> Ignore our device in cdc-acm probing and add a new driver for it,
> >> dispatching to cdc-acm for the actual implementation.
> >>
> >> WARNING: It is likely that this VID/PID is in use for unrelated devices.
> >> Only the Product string hints what this really is in current v0.2.1.
> >> Previous code v0.2.0 was using a Semtech VID/PID, but no card shipping
> >> with such firmware is known to me.
> >>
> >> While this may seem unorthodox, no internals of the driver are accessed,
> >> just the set of driver callbacks is duplicated as shim.
> >>
> >> Use this shim construct to fake DT nodes for serdev on probe.
> >> For testing purposes these nodes do not have a parent yet.
> >> This results in two "Error -2 creating of_node link" warnings on probe.
> > 
> > It looks like the DT is pretty static. Rather than creating the nodes
> > at run-time, can't you create a dts file and build that into your
> > module.
> 
> Heh, if that were the only issue with this patch... ;)

My thoughts exactly. ;)

This clearly is too much of a hack, but maintaining serdev compatible
information in the corresponding tty drivers is probably what we'll want
to do.

When the tty driver binds and registers its ports with tty core, we can
could match again on the USB descriptors, but since the device has
already been matched, we can just pass the equivalent of a compatible
string, or more generally dt-fragment, instead.

Without having had time to look into it myself yet, this sounds like
something we should be using the new software nodes for (software
generated fw nodes). That way, we don't depend on CONFIG_OF either.

Johan

Patch

diff --git a/drivers/net/lora/Makefile b/drivers/net/lora/Makefile
index 5fef38abf5ed..bdcf7560dd65 100644
--- a/drivers/net/lora/Makefile
+++ b/drivers/net/lora/Makefile
@@ -27,6 +27,8 @@  lora-sx130x-y := sx130x.o
 lora-sx130x-y += sx130x_radio.o
 obj-$(CONFIG_LORA_SX130X) += lora-sx130x-picogw.o
 lora-sx130x-picogw-y := sx130x_picogw.o
+obj-$(CONFIG_LORA_SX130X) += lora-picogw.o
+lora-picogw-y := picogw.o
 
 obj-$(CONFIG_LORA_USI) += lora-usi.o
 lora-usi-y := usi.o
diff --git a/drivers/net/lora/picogw.c b/drivers/net/lora/picogw.c
new file mode 100644
index 000000000000..aa5b83d21bb3
--- /dev/null
+++ b/drivers/net/lora/picogw.c
@@ -0,0 +1,337 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Semtech PicoCell gateway USB interface
+ *
+ * Copyright (c) 2018-2019 Andreas Färber
+ */
+
+#define pr_fmt(fmt) "picocell: " fmt
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/serial.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <linux/usb.h>
+#include <linux/usb/cdc.h>
+
+#define PICO_VID 0x0483
+#define PICO_PID 0x5740
+
+static struct usb_driver *picogw_get_acm_driver(struct usb_interface *iface)
+{
+	struct device_driver *drv;
+
+	drv = driver_find("cdc_acm", iface->dev.bus);
+	if (!drv)
+		return NULL;
+
+	return to_usb_driver(drv);
+}
+
+static void picogw_kobj_release(struct kobject *kobj)
+{
+	struct device_node *node = container_of(kobj, struct device_node, kobj);
+	struct property *prop;
+
+	prop = node->properties;
+	while (prop) {
+		struct property *next = prop->next;
+		kfree(prop);
+		prop = next;
+	}
+
+	kfree(node);
+}
+
+static struct kobj_type picogw_kobj_type = {
+	.release = picogw_kobj_release,
+};
+
+static u32 picogw_radio_a_reg = cpu_to_be32(0);
+static u32 picogw_radio_b_reg = cpu_to_be32(1);
+
+static int picogw_fake_of_nodes(struct device *dev)
+{
+	struct device_node *node = NULL;
+	struct device_node *child, *spi, *radio_a, *radio_b;
+	struct property *prop;
+
+	node = kzalloc(sizeof(*node), GFP_KERNEL);
+	if (!node)
+		return -ENOMEM;
+	node->name = "<NULL>";
+	node->full_name = "usb0483,5740";
+	node->type = "<NULL>";
+	kobject_init(&node->kobj, &picogw_kobj_type);
+	node->fwnode.ops = &of_fwnode_ops;
+
+	child = kzalloc(sizeof(*child), GFP_KERNEL);
+	if (!child) {
+		of_node_put(node);
+		return -ENOMEM;
+	}
+	child->name = "lora";
+	child->full_name = "lora";
+	child->type = "<NULL>";
+	child->parent = node;
+	kobject_init(&child->kobj, &picogw_kobj_type);
+	child->fwnode.ops = &of_fwnode_ops;
+	node->child = child;
+
+	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+	if (!prop) {
+		of_node_put(child);
+		of_node_put(node);
+		return -ENOMEM;
+	}
+	prop->name = "compatible";
+	prop->value = "semtech,lora-picocell";
+	prop->length = 22;
+	child->properties = prop;
+
+	spi = kzalloc(sizeof(*spi), GFP_KERNEL);
+	if (!spi) {
+		of_node_put(child);
+		of_node_put(node);
+		return -ENOMEM;
+	}
+	spi->name = "radio-spi";
+	spi->full_name = "radio-spi";
+	spi->type = "<NULL>";
+	spi->parent = child;
+	kobject_init(&spi->kobj, &picogw_kobj_type);
+	spi->fwnode.ops = &of_fwnode_ops;
+	child->child = spi;
+
+	radio_a = kzalloc(sizeof(*radio_a), GFP_KERNEL);
+	if (!radio_a) {
+		of_node_put(spi);
+		of_node_put(child);
+		of_node_put(node);
+		return -ENOMEM;
+	}
+	radio_a->name = "lora@0";
+	radio_a->full_name = "lora@0";
+	radio_a->type = "<NULL>";
+	radio_a->parent = spi;
+	kobject_init(&radio_a->kobj, &picogw_kobj_type);
+	radio_a->fwnode.ops = &of_fwnode_ops;
+	spi->child = radio_a;
+
+	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+	if (!prop) {
+		of_node_put(radio_a);
+		of_node_put(spi);
+		of_node_put(child);
+		of_node_put(node);
+		return -ENOMEM;
+	}
+	prop->name = "compatible";
+	prop->value = "semtech,sx1257";
+	prop->length = 15;
+	radio_a->properties = prop;
+
+	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+	if (!prop) {
+		of_node_put(radio_a);
+		of_node_put(spi);
+		of_node_put(child);
+		of_node_put(node);
+		return -ENOMEM;
+	}
+	prop->name = "reg";
+	prop->value = &picogw_radio_a_reg;
+	prop->length = 4;
+	radio_a->properties->next = prop;
+
+	radio_b = kzalloc(sizeof(*radio_b), GFP_KERNEL);
+	if (!radio_b) {
+		of_node_put(radio_a);
+		of_node_put(spi);
+		of_node_put(child);
+		of_node_put(node);
+		return -ENOMEM;
+	}
+	radio_b->name = "lora@1";
+	radio_b->full_name = "Lora@1";
+	radio_b->type = "<NULL>";
+	radio_b->parent = spi;
+	kobject_init(&radio_b->kobj, &picogw_kobj_type);
+	radio_b->fwnode.ops = &of_fwnode_ops;
+	radio_a->sibling = radio_b;
+
+	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+	if (!prop) {
+		of_node_put(radio_b);
+		of_node_put(radio_a);
+		of_node_put(spi);
+		of_node_put(child);
+		of_node_put(node);
+		return -ENOMEM;
+	}
+	prop->name = "compatible";
+	prop->value = "semtech,sx1257";
+	prop->length = 15;
+	radio_b->properties = prop;
+
+	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+	if (!prop) {
+		of_node_put(radio_a);
+		of_node_put(spi);
+		of_node_put(child);
+		of_node_put(node);
+		return -ENOMEM;
+	}
+	prop->name = "reg";
+	prop->value = &picogw_radio_b_reg;
+	prop->length = 4;
+	radio_b->properties->next = prop;
+
+	dev->of_node = node;
+
+	return 0;
+}
+
+static void picogw_cleanup_of_nodes(struct device *dev)
+{
+	if (dev->of_node->parent)
+		return;
+
+	of_node_put(dev->of_node->child->child->child->sibling); /* lora@1 */
+	of_node_put(dev->of_node->child->child->child); /* lora@0 */
+	of_node_put(dev->of_node->child->child); /* radio-spi*/
+	of_node_put(dev->of_node->child); /* serdev */
+	of_node_put(dev->of_node); /* usb */
+	dev->of_node = NULL;
+}
+
+static int picogw_probe(struct usb_interface *interface,
+	const struct usb_device_id *id)
+{
+	struct usb_driver *drv;
+	int ret;
+
+	drv = picogw_get_acm_driver(interface);
+	if (!drv) {
+		dev_err(&interface->dev, "driver_find failed\n");
+		return -EPROBE_DEFER;
+	}
+
+	if (!interface->dev.of_node) {
+		dev_dbg(&interface->dev, "no of_node\n");
+		ret = picogw_fake_of_nodes(&interface->dev);
+		if (ret)
+			return ret;
+	}
+
+	ret = drv->probe(interface, id);
+	if (ret) {
+		picogw_cleanup_of_nodes(&interface->dev);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void picogw_disconnect(struct usb_interface *intf)
+{
+	struct usb_driver *drv = picogw_get_acm_driver(intf);
+
+	if (drv)
+		drv->disconnect(intf);
+	else
+		dev_warn(&intf->dev, "%s: failed to get cdc_acm driver\n", __func__);
+
+	picogw_cleanup_of_nodes(&intf->dev);
+}
+
+static int picogw_suspend(struct usb_interface *intf, pm_message_t message)
+{
+	struct usb_driver *drv = picogw_get_acm_driver(intf);
+
+	if (!drv) {
+		dev_err(&intf->dev, "%s: failed to get cdc_acm driver\n", __func__);
+		return -ENODEV;
+	}
+
+	return drv->suspend(intf, message);
+}
+
+static int picogw_resume(struct usb_interface *intf)
+{
+	struct usb_driver *drv = picogw_get_acm_driver(intf);
+
+	if (!drv) {
+		dev_err(&intf->dev, "%s: failed to get cdc_acm driver\n", __func__);
+		return -ENODEV;
+	}
+
+	return drv->resume(intf);
+}
+
+static int picogw_reset_resume(struct usb_interface *intf)
+{
+	struct usb_driver *drv = picogw_get_acm_driver(intf);
+
+	if (!drv) {
+		dev_err(&intf->dev, "%s: failed to get cdc_acm driver\n", __func__);
+		return -ENODEV;
+	}
+
+	return drv->reset_resume(intf);
+}
+
+static int picogw_pre_reset(struct usb_interface *intf)
+{
+	struct usb_driver *drv = picogw_get_acm_driver(intf);
+
+	if (!drv) {
+		dev_err(&intf->dev, "%s: failed to get cdc_acm driver\n", __func__);
+		return -ENODEV;
+	}
+
+	return drv->pre_reset(intf);
+}
+
+static const struct usb_device_id picogw_usb_id_table[] = {
+	{ USB_DEVICE_AND_INTERFACE_INFO(PICO_VID, PICO_PID,
+	  USB_CLASS_COMM, USB_CDC_SUBCLASS_ACM, USB_CDC_ACM_PROTO_AT_V25TER) },
+	{}
+};
+MODULE_DEVICE_TABLE(usb, picogw_usb_id_table);
+
+static struct usb_driver picogw_usb_driver = {
+	.name = "lora-picogw-usb",
+	.probe = picogw_probe,
+	.disconnect = picogw_disconnect,
+	.suspend = picogw_suspend,
+	.resume = picogw_resume,
+	.reset_resume = picogw_reset_resume,
+	.pre_reset = picogw_pre_reset,
+	.id_table = picogw_usb_id_table,
+	.supports_autosuspend = 1,
+	.disable_hub_initiated_lpm = 1,
+};
+
+static int __init picogw_init(void)
+{
+	int ret;
+
+	ret = usb_register(&picogw_usb_driver);
+	if (ret < 0){
+		pr_err("usb_register failed (%d)\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+module_init(picogw_init);
+
+static void __exit picogw_exit(void)
+{
+	usb_deregister(&picogw_usb_driver);
+}
+module_exit(picogw_exit);
+
+MODULE_LICENSE("GPL");
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index c225a586c524..541c23b4fbfe 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1865,6 +1865,10 @@  static const struct usb_device_id acm_ids[] = {
 	.driver_info = IGNORE_DEVICE,
 	},
 
+	{ USB_DEVICE_AND_INTERFACE_INFO(0x0483, 0x5740,
+		USB_CLASS_COMM, USB_CDC_SUBCLASS_ACM, USB_CDC_ACM_PROTO_AT_V25TER),
+	.driver_info = IGNORE_DEVICE },
+
 	/* control interfaces without any protocol set */
 	{ USB_INTERFACE_INFO(USB_CLASS_COMM, USB_CDC_SUBCLASS_ACM,
 		USB_CDC_PROTO_NONE) },