[PATCH RFC lora-next 0/5] net: lora: sx130x: USB CDC Picocell Gateway serdev driver via fake DT nodes

[PATCH RFC lora-next 0/5] net: lora: sx130x: USB CDC Picocell Gateway serdev driver via fake DT nodes

[Andreas Färber]

Hello everyone,

Following up on a discussion in August 2018 [1] and my ELCE 2018 talk [2, 3],
I have been searching for an easy way to connect kernel network drivers to
a family of cards/adapters that expose a /dev/ttyACMx device today and
with the vendor's reference software would be accessed from userspace.

While a tty obviously works technically, it leaves us with per-vendor forks
of userspace software without a real upstream community (rare code drops).
It also doesn't allow code sharing with in-kernel protocol stacks or with
the SPI and UART based drivers we've already been working on.

One option suggested by Oliver was a line discipline - that would have the
advantage that it could work with virtually any tty, but on the downside
it would not work out-of-the-box and would require some userspace tool to
manually switch the tty into the new mode. Scalability was another concern,
as was duality of ldisc vs. serdev based implementations.

This patchset rather explores the use of Device Tree nodes to load a serdev
driver on top of the cdc-acm driver. By loading a modified cdc-acm module
plus this quickly hacked-together usb driver, I could play with it on 4.20
on an arm based Turris Omnia router with n-fuse mPCIe card, for instance.

I would hope that the shim approach taken here might also work for FTDI
MPSSE based SPI, encountered by Frank. Not with serdev then, of course,
but as pure USB interface/device driver piggy-backing on what exists.

An unsolved problem is that Semtech in version v0.2.1 switched away from
their own VID/PID to a generic STM32 VID/PID (due to Windows drivers...),
with the only distinction in iProduct string that usb_device_id does not
consider for probing. Since I'm reusing the cdc-acm driver, I can't have
it fail in probe, as it would then fail when called from my driver, too.

Various smaller issues are mentioned in the individual commit messages.

This patchset is based on my linux-lora.git lora-next staging branch.
Please consider taking the small usb patch to aid in further evaluating
serdev on cdc-acm and in resolving the remaining issues.
The others are for testing on our staging tree and for discussion.

Have a lot of fun!

Cheers,
Andreas

[1] https://marc.info/?l=linux-serial&m=153421371800416&w=2
[2] https://www.youtube.com/watch?v=Jjel65sZO9M
[3] https://events.linuxfoundation.org/wp-content/uploads/2017/12/ELCE2018_LoRa_final_Andreas-Farber.pdf

Cc: Johan Hovold <johan@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: Frank Kunz <mailinglists@kunz-im-inter.net>
Cc: Oliver Neukum <oneukum@suse.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Sebastian Reichel <sre@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-lpwan@lists.infradead.org
Cc: linux-serial@vger.kernel.org
Cc: linux-usb@vger.kernel.org
Cc: netdev@vger.kernel.org

Andreas Färber (5):
  net: lora: sx130x: Factor out SPI specific parts
  net: lora: sx130x: Prepare storing driver-specific data
  net: lora: sx130x: Add PicoCell serdev driver
  usb: cdc-acm: Enable serdev support
  HACK: net: lora: sx130x: Add PicoCell gateway shim for cdc-acm

 drivers/net/lora/Makefile        |   4 +
 drivers/net/lora/picogw.c        | 337 ++++++++++++++++++++++++++++
 drivers/net/lora/sx130x.c        | 158 ++++++++-----
 drivers/net/lora/sx130x_picogw.c | 466 +++++++++++++++++++++++++++++++++++++++
 drivers/usb/class/cdc-acm.c      |   8 +-
 include/linux/lora/sx130x.h      |   9 +
 6 files changed, 926 insertions(+), 56 deletions(-)
 create mode 100644 drivers/net/lora/picogw.c
 create mode 100644 drivers/net/lora/sx130x_picogw.c

-- 
2.16.4

[PATCH lora-next 1/5] net: lora: sx130x: Factor out SPI specific parts

[Andreas Färber]

Prepare for the picoGW by factoring code out into helpers using device
rather than spi_device.

While touching those lines, clean up the error output.

Split the probe function in two, to allow derived drivers to insert code
before the common probing code. This may need some more work.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 drivers/net/lora/sx130x.c   | 139 +++++++++++++++++++++++++++-----------------
 include/linux/lora/sx130x.h |   7 +++
 2 files changed, 92 insertions(+), 54 deletions(-)

diff --git a/drivers/net/lora/sx130x.c b/drivers/net/lora/sx130x.c
index 9cae9cea195f..840052955874 100644
--- a/drivers/net/lora/sx130x.c
+++ b/drivers/net/lora/sx130x.c
@@ -133,7 +133,7 @@ static bool sx130x_readable_noinc_reg(struct device *dev, unsigned int reg)
 	}
 }
 
-static struct regmap_config sx130x_regmap_config = {
+const struct regmap_config sx130x_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
 
@@ -151,6 +151,7 @@ static struct regmap_config sx130x_regmap_config = {
 	.num_ranges = ARRAY_SIZE(sx130x_regmap_ranges),
 	.max_register = SX1301_MAX_REGISTER,
 };
+EXPORT_SYMBOL_GPL(sx130x_regmap_config);
 
 static int sx130x_field_write(struct sx130x_priv *priv,
 		enum sx130x_fields field_id, u8 val)
@@ -537,110 +538,98 @@ static const struct net_device_ops sx130x_net_device_ops = {
 	.ndo_start_xmit = sx130x_loradev_start_xmit,
 };
 
-static int sx130x_probe(struct spi_device *spi)
+int sx130x_early_probe(struct regmap *regmap, struct gpio_desc *rst)
 {
+	struct device *dev = regmap_get_device(regmap);
 	struct net_device *netdev;
 	struct sx130x_priv *priv;
-	struct gpio_desc *rst;
 	int ret;
 	int i;
-	unsigned int ver;
-	unsigned int val;
-
-	rst = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
-	if (IS_ERR(rst)) {
-		if (PTR_ERR(rst) != -EPROBE_DEFER)
-			dev_err(&spi->dev, "Failed to obtain reset GPIO\n");
-		return PTR_ERR(rst);
-	}
-
-	gpiod_set_value_cansleep(rst, 1);
-	msleep(100);
-	gpiod_set_value_cansleep(rst, 0);
-	msleep(100);
-
-	spi->bits_per_word = 8;
-	spi_setup(spi);
 
-	netdev = devm_alloc_loradev(&spi->dev, sizeof(*priv));
+	netdev = devm_alloc_loradev(dev, sizeof(*priv));
 	if (!netdev)
 		return -ENOMEM;
 
 	netdev->netdev_ops = &sx130x_net_device_ops;
-	SET_NETDEV_DEV(netdev, &spi->dev);
+	SET_NETDEV_DEV(netdev, dev);
 
 	priv = netdev_priv(netdev);
+	priv->regmap = regmap;
 	priv->rst_gpio = rst;
 
 	mutex_init(&priv->io_lock);
 
-	spi_set_drvdata(spi, netdev);
-	priv->dev = &spi->dev;
-
-	priv->regmap = devm_regmap_init_spi(spi, &sx130x_regmap_config);
-	if (IS_ERR(priv->regmap)) {
-		ret = PTR_ERR(priv->regmap);
-		dev_err(&spi->dev, "Regmap allocation failed: %d\n", ret);
-		return ret;
-	}
+	dev_set_drvdata(dev, netdev);
+	priv->dev = dev;
 
 	for (i = 0; i < ARRAY_SIZE(sx130x_regmap_fields); i++) {
 		const struct reg_field *reg_fields = sx130x_regmap_fields;
 
-		priv->regmap_fields[i] = devm_regmap_field_alloc(&spi->dev,
+		priv->regmap_fields[i] = devm_regmap_field_alloc(dev,
 				priv->regmap,
 				reg_fields[i]);
 		if (IS_ERR(priv->regmap_fields[i])) {
 			ret = PTR_ERR(priv->regmap_fields[i]);
-			dev_err(&spi->dev, "Cannot allocate regmap field: %d\n", ret);
+			dev_err(dev, "Cannot allocate regmap field (%d)\n", ret);
 			return ret;
 		}
 	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(sx130x_early_probe);
+
+int sx130x_probe(struct device *dev)
+{
+	struct net_device *netdev = dev_get_drvdata(dev);
+	struct sx130x_priv *priv = netdev_priv(netdev);
+	unsigned int ver;
+	unsigned int val;
+	int ret;
 
 	ret = regmap_read(priv->regmap, SX1301_VER, &ver);
 	if (ret) {
-		dev_err(&spi->dev, "version read failed\n");
+		dev_err(dev, "version read failed (%d)\n", ret);
 		return ret;
 	}
 
 	if (ver != SX1301_CHIP_VERSION) {
-		dev_err(&spi->dev, "unexpected version: %u\n", ver);
+		dev_err(dev, "unexpected version: %u\n", ver);
 		return -ENXIO;
 	}
 
 	ret = regmap_write(priv->regmap, SX1301_PAGE, 0);
 	if (ret) {
-		dev_err(&spi->dev, "page/reset write failed\n");
+		dev_err(dev, "page/reset write failed (%d)\n", ret);
 		return ret;
 	}
 
 	ret = sx130x_field_write(priv, F_SOFT_RESET, 1);
 	if (ret) {
-		dev_err(&spi->dev, "soft reset failed\n");
+		dev_err(dev, "soft reset failed (%d)\n", ret);
 		return ret;
 	}
 
 	ret = sx130x_field_write(priv, F_GLOBAL_EN, 0);
 	if (ret) {
-		dev_err(&spi->dev, "gate global clocks failed\n");
+		dev_err(dev, "gate global clocks failed (%d)\n", ret);
 		return ret;
 	}
 
 	ret = sx130x_field_write(priv, F_CLK32M_EN, 0);
 	if (ret) {
-		dev_err(&spi->dev, "gate 32M clock failed\n");
+		dev_err(dev, "gate 32M clock failed (%d)\n", ret);
 		return ret;
 	}
 
 	ret = sx130x_field_write(priv, F_RADIO_A_EN, 1);
 	if (ret) {
-		dev_err(&spi->dev, "radio a enable failed\n");
+		dev_err(dev, "radio A enable failed (%d)\n", ret);
 		return ret;
 	}
 
 	ret = sx130x_field_write(priv, F_RADIO_B_EN, 1);
 	if (ret) {
-		dev_err(&spi->dev, "radio b enable failed\n");
+		dev_err(dev, "radio B enable failed (%d)\n", ret);
 		return ret;
 	}
 
@@ -648,7 +637,7 @@ static int sx130x_probe(struct spi_device *spi)
 
 	ret = sx130x_field_write(priv, F_RADIO_RST, 1);
 	if (ret) {
-		dev_err(&spi->dev, "radio asert reset failed\n");
+		dev_err(dev, "radio assert reset failed (%d)\n", ret);
 		return ret;
 	}
 
@@ -656,13 +645,13 @@ static int sx130x_probe(struct spi_device *spi)
 
 	ret = sx130x_field_write(priv, F_RADIO_RST, 0);
 	if (ret) {
-		dev_err(&spi->dev, "radio deasert reset failed\n");
+		dev_err(dev, "radio deassert reset failed (%d)\n", ret);
 		return ret;
 	}
 
 	/* radio */
 
-	ret = devm_sx130x_register_radio_devices(&spi->dev);
+	ret = devm_sx130x_register_radio_devices(dev);
 	if (ret)
 		return ret;
 
@@ -672,7 +661,7 @@ static int sx130x_probe(struct spi_device *spi)
 
 	ret = regmap_read(priv->regmap, SX1301_GPMODE, &val);
 	if (ret) {
-		dev_err(&spi->dev, "GPIO mode read failed\n");
+		dev_err(dev, "GPIO mode read failed (%d)\n", ret);
 		goto out;
 	}
 
@@ -680,13 +669,13 @@ static int sx130x_probe(struct spi_device *spi)
 
 	ret = regmap_write(priv->regmap, SX1301_GPMODE, val);
 	if (ret) {
-		dev_err(&spi->dev, "GPIO mode write failed\n");
+		dev_err(dev, "GPIO mode write failed (%d)\n", ret);
 		goto out;
 	}
 
 	ret = regmap_read(priv->regmap, SX1301_GPSO, &val);
 	if (ret) {
-		dev_err(&spi->dev, "GPIO select output read failed\n");
+		dev_err(dev, "GPIO select output read failed (%d)\n", ret);
 		goto out;
 	}
 
@@ -695,7 +684,7 @@ static int sx130x_probe(struct spi_device *spi)
 
 	ret = regmap_write(priv->regmap, SX1301_GPSO, val);
 	if (ret) {
-		dev_err(&spi->dev, "GPIO select output write failed\n");
+		dev_err(dev, "GPIO select output write failed (%d)\n", ret);
 		goto out;
 	}
 
@@ -705,24 +694,66 @@ static int sx130x_probe(struct spi_device *spi)
 	if (ret)
 		goto out;
 
-	dev_info(&spi->dev, "SX1301 module probed\n");
+	dev_info(dev, "SX1301 module probed\n");
 
 out:
 	mutex_unlock(&priv->io_lock);
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(sx130x_probe);
 
-static int sx130x_remove(struct spi_device *spi)
+int sx130x_remove(struct device *dev)
 {
-	struct net_device *netdev = spi_get_drvdata(spi);
+	struct net_device *netdev = dev_get_drvdata(dev);
 
 	unregister_loradev(netdev);
 
-	dev_info(&spi->dev, "SX1301 module removed\n");
+	dev_info(dev, "SX1301 module removed\n");
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(sx130x_remove);
+
+static int sx130x_spi_probe(struct spi_device *spi)
+{
+	struct gpio_desc *rst;
+	struct regmap *regmap;
+	int ret;
+
+	rst = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(rst)) {
+		if (PTR_ERR(rst) != -EPROBE_DEFER)
+			dev_err(&spi->dev, "Failed to obtain reset GPIO\n");
+		return PTR_ERR(rst);
+	}
+
+	gpiod_set_value_cansleep(rst, 1);
+	msleep(100);
+	gpiod_set_value_cansleep(rst, 0);
+	msleep(100);
+
+	spi->bits_per_word = 8;
+	spi_setup(spi);
+
+	regmap = devm_regmap_init_spi(spi, &sx130x_regmap_config);
+	if (IS_ERR(regmap)) {
+		ret = PTR_ERR(regmap);
+		dev_err(&spi->dev, "Regmap allocation failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = sx130x_early_probe(regmap, rst);
+	if (ret)
+		return ret;
+
+	return sx130x_probe(&spi->dev);
+}
+
+static int sx130x_spi_remove(struct spi_device *spi)
+{
+	return sx130x_remove(&spi->dev);;
+}
 
 #ifdef CONFIG_OF
 static const struct of_device_id sx130x_dt_ids[] = {
@@ -737,8 +768,8 @@ static struct spi_driver sx130x_spi_driver = {
 		.name = "sx130x",
 		.of_match_table = of_match_ptr(sx130x_dt_ids),
 	},
-	.probe = sx130x_probe,
-	.remove = sx130x_remove,
+	.probe = sx130x_spi_probe,
+	.remove = sx130x_spi_remove,
 };
 
 static int __init sx130x_init(void)
diff --git a/include/linux/lora/sx130x.h b/include/linux/lora/sx130x.h
index ac4e2e7ae18a..d6f027ef283f 100644
--- a/include/linux/lora/sx130x.h
+++ b/include/linux/lora/sx130x.h
@@ -9,9 +9,16 @@
 #define LORA_SX130X_H
 
 #include <linux/device.h>
+#include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
 
+extern const struct regmap_config sx130x_regmap_config;
+int sx130x_early_probe(struct regmap *regmap, struct gpio_desc *rst);
+int sx130x_probe(struct device *dev);
+int sx130x_remove(struct device *dev);
+
+
 struct sx130x_radio_device {
 	struct device dev;
 	struct device *concentrator;
-- 
2.16.4

[PATCH lora-next 2/5] net: lora: sx130x: Prepare storing driver-specific data

[Andreas Färber]

Some drivers (e.g., serdev) may need to access private data not part of
the core sx130x_priv, which is inaccessible to other source files.
As the sx130x core expects to obtain the net_device from the dev's drvdata,
we can't reuse that in derived drivers and need a new field plus helpers.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 drivers/net/lora/sx130x.c   | 19 +++++++++++++++++++
 include/linux/lora/sx130x.h |  2 ++
 2 files changed, 21 insertions(+)

diff --git a/drivers/net/lora/sx130x.c b/drivers/net/lora/sx130x.c
index 840052955874..978c921ca5ec 100644
--- a/drivers/net/lora/sx130x.c
+++ b/drivers/net/lora/sx130x.c
@@ -58,6 +58,7 @@ struct sx130x_priv {
 	struct regmap		*regmap;
 	struct regmap_field	*regmap_fields[ARRAY_SIZE(sx130x_regmap_fields)];
 	struct mutex		io_lock;
+	void			*drvdata;
 };
 
 struct regmap *sx130x_get_regmap(struct device *dev)
@@ -68,6 +69,24 @@ struct regmap *sx130x_get_regmap(struct device *dev)
 	return priv->regmap;
 }
 
+void sx130x_set_drvdata(struct device *dev, void *drvdata)
+{
+	struct net_device *netdev = dev_get_drvdata(dev);
+	struct sx130x_priv *priv = netdev_priv(netdev);
+
+	priv->drvdata = drvdata;
+}
+EXPORT_SYMBOL_GPL(sx130x_set_drvdata);
+
+void *sx130x_get_drvdata(struct device *dev)
+{
+	struct net_device *netdev = dev_get_drvdata(dev);
+	struct sx130x_priv *priv = netdev_priv(netdev);
+
+	return priv->drvdata;
+}
+EXPORT_SYMBOL_GPL(sx130x_get_drvdata);
+
 void sx130x_io_lock(struct device *dev)
 {
 	struct net_device *netdev = dev_get_drvdata(dev);
diff --git a/include/linux/lora/sx130x.h b/include/linux/lora/sx130x.h
index d6f027ef283f..85b088ec77b8 100644
--- a/include/linux/lora/sx130x.h
+++ b/include/linux/lora/sx130x.h
@@ -14,6 +14,8 @@
 #include <linux/regmap.h>
 
 extern const struct regmap_config sx130x_regmap_config;
+void sx130x_set_drvdata(struct device *dev, void *drvdata);
+void *sx130x_get_drvdata(struct device *dev);
 int sx130x_early_probe(struct regmap *regmap, struct gpio_desc *rst);
 int sx130x_probe(struct device *dev);
 int sx130x_remove(struct device *dev);
-- 
2.16.4

[PATCH lora-next 3/5] net: lora: sx130x: Add PicoCell serdev driver

[Andreas Färber]

The picoGW reference MCU firmware implements a USB CDC or UART interface
with a set of serial commands. It can be found on multiple mPCIe cards
as well as USB adapters.

  https://github.com/Lora-net/picoGW_mcu

That MCU design superseded earlier attempts of using FTDI chips (MPSSE)
for controlling the SPI based chip directly from the host.

This commit adds a serdev driver implementing another regmap_bus to
abstract the register access and reuses our existing regmap driver on top.
Thereby we have an early proof of concept that we can drive both types
of modules/cards with a single driver core!

It assumes there is only one SX130x (with its radios) accessible, whereas
some new cards reportedly also have an SX127x on-board. So the DT/driver
design may need to be reconsidered once such a card or documentation
becomes available.
It also assumes SX1255/1258 are fully compatible with "semtech,sx1257".

Currently there's still some bugs to be investigated, with communication
stalling on one device and another reading the radio versions wrong
(07 / 1f instead of 21, also observed on spi once).

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 drivers/net/lora/Makefile        |   2 +
 drivers/net/lora/sx130x_picogw.c | 466 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 468 insertions(+)
 create mode 100644 drivers/net/lora/sx130x_picogw.c

diff --git a/drivers/net/lora/Makefile b/drivers/net/lora/Makefile
index c6a0410f80ce..5fef38abf5ed 100644
--- a/drivers/net/lora/Makefile
+++ b/drivers/net/lora/Makefile
@@ -25,6 +25,8 @@ lora-sx127x-y := sx127x.o
 obj-$(CONFIG_LORA_SX130X) += lora-sx130x.o
 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_USI) += lora-usi.o
 lora-usi-y := usi.o
diff --git a/drivers/net/lora/sx130x_picogw.c b/drivers/net/lora/sx130x_picogw.c
new file mode 100644
index 000000000000..577f9fb71d46
--- /dev/null
+++ b/drivers/net/lora/sx130x_picogw.c
@@ -0,0 +1,466 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Semtech SX1301/1308 PicoCell gateway serial MCU interface
+ *
+ * Copyright (c) 2018-2019 Andreas Färber
+ */
+#include <linux/completion.h>
+#include <linux/lora/sx130x.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/serdev.h>
+#include <linux/serial.h>
+#include <linux/slab.h>
+
+struct picogw_device {
+	struct serdev_device *serdev;
+
+	u8 rx_buf[1024];
+	int rx_len;
+
+	struct completion answer_comp;
+	struct completion answer_read_comp;
+};
+
+static inline struct picogw_device *picogw_get_drvdata(struct serdev_device *sdev)
+{
+	return sx130x_get_drvdata(&sdev->dev);
+}
+
+static bool picogw_valid_cmd(char ch)
+{
+	switch (ch) {
+	case 'k': /* invalid command error */
+	case 'r':
+	case 'w':
+	case 'l':
+		return true;
+	default:
+		return false;
+	}
+}
+
+static int picogw_send_cmd(struct picogw_device *picodev, char cmd,
+	u8 addr, const void *data, u16 data_len)
+{
+	struct serdev_device *sdev = picodev->serdev;
+	u8 buf[4];
+	int i, ret;
+
+	buf[0] = cmd;
+	buf[1] = (data_len >> 8) & 0xff;
+	buf[2] = (data_len >> 0) & 0xff;
+	buf[3] = addr;
+
+	dev_dbg(&sdev->dev, "%s: %c %02x %02x %02x\n", __func__, buf[0],
+		(unsigned int)buf[1], (unsigned int)buf[2], (unsigned int)buf[3]);
+	for (i = 0; i < data_len; i++) {
+		dev_dbg(&sdev->dev, "%s: data %02x\n", __func__, (unsigned int)((const u8*)data)[i]);
+	}
+
+	ret = serdev_device_write_buf(sdev, buf, 4);
+	if (ret < 0)
+		return ret;
+	if (ret != 4)
+		return -EIO;
+
+	if (data_len) {
+		ret = serdev_device_write_buf(sdev, data, data_len);
+		if (ret < 0)
+			return ret;
+		if (ret != data_len)
+			return -EIO;
+	}
+
+	return 0;
+}
+
+static int picogw_recv_answer(struct picogw_device *picodev,
+	char *cmd, bool *ack, void *buf, int buf_len,
+	unsigned long timeout)
+{
+	int len;
+
+	timeout = wait_for_completion_timeout(&picodev->answer_comp, timeout);
+	if (!timeout)
+		return -ETIMEDOUT;
+
+	if (cmd)
+		*cmd = picodev->rx_buf[0];
+
+	if (ack)
+		*ack = (picodev->rx_buf[3] == 1);
+
+	len = min(picodev->rx_len - 4, buf_len);
+	if (buf)
+		memcpy(buf, picodev->rx_buf + 4, len);
+
+	reinit_completion(&picodev->answer_comp);
+	complete(&picodev->answer_read_comp);
+
+	return len;
+}
+
+static int picogw_reg_read(struct picogw_device *picodev, u8 addr, u8 *val, unsigned long timeout)
+{
+	const u8 dummy = 0;
+	char cmd;
+	bool ack;
+	int ret;
+
+	ret = picogw_send_cmd(picodev, 'r', addr, &dummy, false ? 1 : 0);
+	if (ret)
+		return ret;
+
+	ret = picogw_recv_answer(picodev, &cmd, &ack, val, 1, timeout);
+	if (ret < 0)
+		return ret;
+	if (cmd != 'r')
+		return -EIO;
+	if (!ack || ret != 1)
+		return -EIO;
+
+	return 0;
+}
+
+static int picogw_reg_write(struct picogw_device *picodev, u8 addr, u8 val, unsigned long timeout)
+{
+	char cmd;
+	bool ack;
+	int ret;
+
+	ret = picogw_send_cmd(picodev, 'w', addr, &val, 1);
+	if (ret)
+		return ret;
+
+	ret = picogw_recv_answer(picodev, &cmd, &ack, NULL, 0, timeout);
+	if (ret < 0)
+		return ret;
+	if (cmd != 'w')
+		return -EIO;
+	if (!ack || ret != 0)
+		return -EIO;
+
+	return 0;
+}
+
+static int picogw_mcu_fw_check(struct picogw_device *picodev,
+	u32 fw_version, u8 *id, unsigned long timeout)
+{
+	char cmd;
+	bool ack;
+	int ret;
+
+	fw_version = cpu_to_be32(fw_version);
+	ret = picogw_send_cmd(picodev, 'l', 0, &fw_version, sizeof(fw_version));
+	if (ret)
+		return ret;
+
+	ret = picogw_recv_answer(picodev, &cmd, &ack, id, id ? 8 : 0, timeout);
+	if (ret < 0)
+		return ret;
+	if (cmd != 'l')
+		return -EIO;
+	if (id && ret != 8)
+		return -EIO;
+
+	return ack ? 0 : -ENOTSUPP;
+}
+
+static int picogw_regmap_gather_write(void *context,
+	const void *reg_buf, size_t reg_size, const void *val_buf, size_t val_size)
+{
+	struct picogw_device *picodev = context;
+	const u8 *addr_buf = reg_buf;
+	const u8 *val = val_buf;
+	u8 addr;
+	int ret;
+
+	//dev_dbg(&picodev->serdev->dev, "%s: 0x%x (reg_size %zu) 0x%x (val_size %zu)\n",
+	//	__func__, (unsigned int)addr_buf[0], reg_size, (unsigned int)val[0], val_size);
+
+	if (reg_size != 1 || val_size > 0xffff)
+		return -EINVAL;
+
+	addr = addr_buf[0] & ~BIT(7);
+	if (val_size == 1) {
+		ret = picogw_reg_write(picodev, addr, val[0], HZ);
+		if (ret)
+			return ret;
+		return 0;
+	} else {
+		/* TODO burst mode */
+		dev_err(&picodev->serdev->dev, "burst mode write not yet implemented\n");
+		return -ENOTSUPP;
+	}
+}
+
+static int picogw_regmap_write(void *context,
+	const void *data_buf, size_t count)
+{
+	const u8 *data = data_buf;
+	if (count < 1)
+		return -EINVAL;
+
+	return picogw_regmap_gather_write(context, data, 1, data + 1, count - 1);
+}
+
+static int picogw_regmap_read(void *context,
+	const void *reg_buf, size_t reg_size, void *val_buf, size_t val_size)
+{
+	struct picogw_device *picodev = context;
+	const u8 *addr_buf = reg_buf;
+	u8 addr;
+	int ret;
+
+	//dev_dbg(&picodev->serdev->dev, "%s: 0x%x (reg_size %zu) (val_size %zu)\n", __func__, (unsigned int)addr_buf[0], reg_size, val_size);
+
+	if (reg_size != 1 || val_size > 0xffff)
+		return -EINVAL;
+
+	addr = addr_buf[0] & ~BIT(7);
+	if (val_size == 1) {
+		ret = picogw_reg_read(picodev, addr, val_buf, HZ);
+		if (ret)
+			return ret;
+		return 0;
+	} else {
+		/* TODO burst mode */
+		dev_err(&picodev->serdev->dev, "burst mode read not yet implemented\n");
+		return -ENOTSUPP;
+	}
+}
+
+static const struct regmap_bus picogw_regmap_bus = {
+	.write = picogw_regmap_write,
+	.gather_write = picogw_regmap_gather_write,
+	.read = picogw_regmap_read,
+
+	.max_raw_write = 0xffff,
+	.max_raw_read = 0xffff,
+};
+
+static int picogw_handle_answer(struct picogw_device *picodev)
+{
+	struct device *dev = &picodev->serdev->dev;
+	unsigned int data_len = ((u16)picodev->rx_buf[1] << 8) | picodev->rx_buf[2];
+	int cmd_len = 4 + data_len;
+	int i, ret;
+
+	if (picodev->rx_len < 4)
+		return 0;
+
+	if (cmd_len > sizeof(picodev->rx_buf)) {
+		dev_warn(dev, "answer too long (%u)\n", data_len);
+		return 0;
+	}
+
+	if (picodev->rx_len < cmd_len) {
+		dev_dbg(dev, "got %u, need %u bytes\n", picodev->rx_len, cmd_len);
+		return 0;
+	}
+
+	dev_dbg(dev, "Answer %c =%u %s (%u)\n", picodev->rx_buf[0],
+		(unsigned int)picodev->rx_buf[3],
+		(picodev->rx_buf[3] == 1) ? "OK" : "K0",
+		data_len);
+	for (i = 0; i < data_len; i++) {
+		//dev_dbg(dev, "%s: %02x\n", __func__, (unsigned int)picodev->rx_buf[4 + i]);
+	}
+
+	complete(&picodev->answer_comp);
+	ret = wait_for_completion_interruptible_timeout(&picodev->answer_read_comp, HZ / 2);
+	if (ret < 0)
+		return 0;
+
+	reinit_completion(&picodev->answer_read_comp);
+
+	return cmd_len;
+}
+
+static int picogw_receive_buf(struct serdev_device *sdev, const u8 *data, size_t count)
+{
+	struct picogw_device *picodev = picogw_get_drvdata(sdev);
+	size_t i;
+	int len = 0;
+
+	dev_dbg(&sdev->dev, "Receive (%zu)\n", count);
+
+	if (completion_done(&picodev->answer_comp)) {
+		dev_info(&sdev->dev, "RX waiting on completion\n");
+		return 0;
+	}
+	if (picodev->rx_len == sizeof(picodev->rx_buf)) {
+		dev_warn(&sdev->dev, "RX buffer full\n");
+		return 0;
+	}
+
+	i = min(count, sizeof(picodev->rx_buf) - picodev->rx_len);
+	if (i > 0) {
+		memcpy(&picodev->rx_buf[picodev->rx_len], data, i);
+		picodev->rx_len += i;
+		len += i;
+	}
+
+	while (picodev->rx_len > 0) {
+		/* search for valid beginning */
+		for (i = 0; i < picodev->rx_len; i++) {
+			if (picogw_valid_cmd(picodev->rx_buf[i]))
+				break;
+		}
+		if (i > 0) {
+			dev_dbg(&sdev->dev, "skipping %zu bytes of garbage\n", i);
+			if (i < picodev->rx_len) {
+				memmove(picodev->rx_buf, &picodev->rx_buf[i], picodev->rx_len - i);
+				picodev->rx_len -= i;
+			} else
+				picodev->rx_len = 0;
+		}
+
+		i = picogw_handle_answer(picodev);
+		if (i == 0)
+			break;
+
+		if (i % 64 == 0) {
+			dev_info(&sdev->dev, "skipping padding byte\n");
+			i++;
+		}
+		if (picodev->rx_len > i)
+			memmove(picodev->rx_buf, &picodev->rx_buf[i], picodev->rx_len - i);
+		if (picodev->rx_len >= i)
+			picodev->rx_len -= i;
+	}
+
+	return len;
+}
+
+static const struct serdev_device_ops picogw_serdev_client_ops = {
+	.receive_buf = picogw_receive_buf,
+	.write_wakeup = serdev_device_write_wakeup,
+};
+
+static int picogw_serdev_probe(struct serdev_device *sdev)
+{
+	struct picogw_device *picodev;
+	struct regmap *regmap;
+	u32 fw_version = 0x010a0006;
+	u8 mac[8];
+	int ret;
+
+	//dev_info(&sdev->dev, "Probing\n");
+
+	picodev = devm_kzalloc(&sdev->dev, sizeof(*picodev), GFP_KERNEL);
+	if (!picodev)
+		return -ENOMEM;
+
+	picodev->serdev = sdev;
+	init_completion(&picodev->answer_comp);
+	init_completion(&picodev->answer_read_comp);
+
+	ret = serdev_device_open(sdev);
+	if (ret) {
+		dev_err(&sdev->dev, "Failed to open (%d)\n", ret);
+		return ret;
+	}
+
+	serdev_device_set_baudrate(sdev, 115200);
+	serdev_device_set_parity(sdev, SERDEV_PARITY_NONE);
+	serdev_device_set_flow_control(sdev, true);
+
+	regmap = devm_regmap_init(&sdev->dev, &picogw_regmap_bus, picodev, &sx130x_regmap_config);
+	if (IS_ERR(regmap)) {
+		ret = PTR_ERR(regmap);
+		dev_err(&sdev->dev, "error initializing regmap (%d)\n", ret);
+		serdev_device_close(sdev);
+		return ret;
+	}
+
+	ret = sx130x_early_probe(regmap, NULL);
+	if (ret) {
+		serdev_device_close(sdev);
+		return ret;
+	}
+
+	sx130x_set_drvdata(&sdev->dev, picodev);
+	serdev_device_set_client_ops(sdev, &picogw_serdev_client_ops);
+
+	//msleep(1000);
+	ret = picogw_mcu_fw_check(picodev, fw_version, mac, HZ);
+	if (!ret || ret == -ENOTSUPP)
+		dev_info(&sdev->dev, "ID = %02x%02x%02x%02x%02x%02x%02x%02x\n",
+			(unsigned int)mac[0],
+			(unsigned int)mac[1],
+			(unsigned int)mac[2],
+			(unsigned int)mac[3],
+			(unsigned int)mac[4],
+			(unsigned int)mac[5],
+			(unsigned int)mac[6],
+			(unsigned int)mac[7]);
+	while (ret == -ENOTSUPP && ((fw_version & 0xff) > 4)) {
+		ret = picogw_mcu_fw_check(picodev, --fw_version, NULL, HZ);
+	}
+	if (ret == -ENOTSUPP) {
+		dev_warn(&sdev->dev, "firmware check failed (%08x)\n", fw_version);
+	} else {
+		dev_err(&sdev->dev, "ID retrieval failed (%d)\n", ret);
+		serdev_device_close(sdev);
+		return ret;
+	}
+
+	ret = sx130x_probe(&sdev->dev);
+	if (ret) {
+		serdev_device_close(sdev);
+		return ret;
+	}
+
+	//dev_info(&sdev->dev, "Done.\n");
+
+	return 0;
+}
+
+static void picogw_serdev_remove(struct serdev_device *sdev)
+{
+	sx130x_remove(&sdev->dev);
+
+	serdev_device_close(sdev);
+
+	//dev_info(&sdev->dev, "Removed\n");
+}
+
+static const struct of_device_id picogw_serdev_of_match[] = {
+	{ .compatible = "semtech,lora-picocell" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, picogw_serdev_of_match);
+
+static struct serdev_device_driver picogw_serdev_driver = {
+	.probe = picogw_serdev_probe,
+	.remove = picogw_serdev_remove,
+	.driver = {
+		.name = "lora-picogw",
+		.of_match_table = picogw_serdev_of_match,
+	},
+};
+
+static int __init picogw_serdev_init(void)
+{
+	int ret;
+
+	ret = serdev_device_driver_register(&picogw_serdev_driver);
+	if (ret) {
+		pr_err("serdev_device_driver_register failed (%d)", ret);
+		return ret;
+	}
+
+	return 0;
+}
+module_init(picogw_serdev_init);
+
+static void __exit picogw_serdev_exit(void)
+{
+	serdev_device_driver_unregister(&picogw_serdev_driver);
+}
+module_exit(picogw_serdev_exit);
+
+MODULE_LICENSE("GPL");
-- 
2.16.4

Re: [PATCH lora-next 3/5] net: lora: sx130x: Add PicoCell serdev driver

[Andreas Färber]

Am 04.01.19 um 12:21 schrieb Andreas Färber:
> diff --git a/drivers/net/lora/sx130x_picogw.c b/drivers/net/lora/sx130x_picogw.c
> new file mode 100644
> index 000000000000..577f9fb71d46
> --- /dev/null
> +++ b/drivers/net/lora/sx130x_picogw.c
[...]
> +	serdev_device_set_baudrate(sdev, 115200);
> +	serdev_device_set_parity(sdev, SERDEV_PARITY_NONE);
> +	serdev_device_set_flow_control(sdev, true);

This should probably be false?

https://github.com/Lora-net/picoGW_hal/blob/master/libloragw/src/loragw_com_linux.c#L65

tty.c_iflag &= ~(IXON | IXOFF | IXANY | ICRNL);
...
tty.c_oflag &= ~(IXON | IXOFF | IXANY | ICRNL);

Nothing that looks like RTS/CTS anywhere, which appears to be what the
serdev implementation is switching with the above function.

However, both true and false appeared to work equally so far.


With one particular USB device I get the following warning/error,
seemingly independent of whether I enable flow control above or not:

[68640.481454] lora-picogw-usb 4-1:1.0: failed to set dtr/rts

(imagine s/lora-picogw-usb/cdc_acm/ - cf. cdc-acm.c:acm_port_dtr_rts())

Looks like a firmware/hardware issue to me, since it appeared with the
original cdc_acm driver on reboot, plus across reset, ports and hosts.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH lora-next 3/5] net: lora: sx130x: Add PicoCell serdev driver

[Andreas Färber]

Am 04.01.19 um 12:21 schrieb Andreas Färber:
> Currently there's still some bugs to be investigated, with communication
> stalling on one device and another reading the radio versions wrong
> (07 / 1f instead of 21, also observed on spi once).

Reproducable 100% on SPI by setting REGCACHE_RBTREE in sx130x.c.

Since this serdev driver was using REGCACHE_NONE still and I don't spot
a register missing as volatile either, I guess it may be a timing issue?

My earlier locking patch is applied, to rule out any non-determinism in
the register access order due to radio vs. concentrator interactions.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH lora-next 3/5] net: lora: sx130x: Add PicoCell serdev driver

[Ben Whitten]

Hi,

On 04/01/2019 20:21, Andreas Färber wrote:
> The picoGW reference MCU firmware implements a USB CDC or UART interface
> with a set of serial commands. It can be found on multiple mPCIe cards
> as well as USB adapters.
> 
>    https://github.com/Lora-net/picoGW_mcu
> 
> That MCU design superseded earlier attempts of using FTDI chips (MPSSE)
> for controlling the SPI based chip directly from the host.
> 
> This commit adds a serdev driver implementing another regmap_bus to
> abstract the register access and reuses our existing regmap driver on top.
> Thereby we have an early proof of concept that we can drive both types
> of modules/cards with a single driver core!
> 
> It assumes there is only one SX130x (with its radios) accessible, whereas
> some new cards reportedly also have an SX127x on-board. So the DT/driver
> design may need to be reconsidered once such a card or documentation
> becomes available.
> It also assumes SX1255/1258 are fully compatible with "semtech,sx1257".
> 
> Currently there's still some bugs to be investigated, with communication
> stalling on one device and another reading the radio versions wrong
> (07 / 1f instead of 21, also observed on spi once).
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>   drivers/net/lora/Makefile        |   2 +
>   drivers/net/lora/sx130x_picogw.c | 466 +++++++++++++++++++++++++++++++++++++++

We are missing a Kconfig select or depend, compilation fails if 
CONFIG_SERIAL_DEV_BUS is not selected.

>   2 files changed, 468 insertions(+)
>   create mode 100644 drivers/net/lora/sx130x_picogw.c
> 
> diff --git a/drivers/net/lora/Makefile b/drivers/net/lora/Makefile
> index c6a0410f80ce..5fef38abf5ed 100644
> --- a/drivers/net/lora/Makefile
> +++ b/drivers/net/lora/Makefile
> @@ -25,6 +25,8 @@ lora-sx127x-y := sx127x.o
>   obj-$(CONFIG_LORA_SX130X) += lora-sx130x.o
>   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_USI) += lora-usi.o
>   lora-usi-y := usi.o
> diff --git a/drivers/net/lora/sx130x_picogw.c b/drivers/net/lora/sx130x_picogw.c
> new file mode 100644
> index 000000000000..577f9fb71d46
> --- /dev/null
> +++ b/drivers/net/lora/sx130x_picogw.c
> @@ -0,0 +1,466 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Semtech SX1301/1308 PicoCell gateway serial MCU interface
> + *
> + * Copyright (c) 2018-2019 Andreas Färber
> + */
> +#include <linux/completion.h>
> +#include <linux/lora/sx130x.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/serdev.h>
> +#include <linux/serial.h>
> +#include <linux/slab.h>
> +
> +struct picogw_device {
> +	struct serdev_device *serdev;
> +
> +	u8 rx_buf[1024];
> +	int rx_len;
> +
> +	struct completion answer_comp;
> +	struct completion answer_read_comp;
> +};
> +
> +static inline struct picogw_device *picogw_get_drvdata(struct serdev_device *sdev)
> +{
> +	return sx130x_get_drvdata(&sdev->dev);
> +}
> +
> +static bool picogw_valid_cmd(char ch)
> +{
> +	switch (ch) {
> +	case 'k': /* invalid command error */
> +	case 'r':
> +	case 'w':
> +	case 'l':
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static int picogw_send_cmd(struct picogw_device *picodev, char cmd,
> +	u8 addr, const void *data, u16 data_len)
> +{
> +	struct serdev_device *sdev = picodev->serdev;
> +	u8 buf[4];
> +	int i, ret;
> +
> +	buf[0] = cmd;
> +	buf[1] = (data_len >> 8) & 0xff;
> +	buf[2] = (data_len >> 0) & 0xff;
> +	buf[3] = addr;
> +
> +	dev_dbg(&sdev->dev, "%s: %c %02x %02x %02x\n", __func__, buf[0],
> +		(unsigned int)buf[1], (unsigned int)buf[2], (unsigned int)buf[3]);
> +	for (i = 0; i < data_len; i++) {
> +		dev_dbg(&sdev->dev, "%s: data %02x\n", __func__, (unsigned int)((const u8*)data)[i]);
> +	}
> +
> +	ret = serdev_device_write_buf(sdev, buf, 4);
> +	if (ret < 0)
> +		return ret;
> +	if (ret != 4)
> +		return -EIO;
> +
> +	if (data_len) {
> +		ret = serdev_device_write_buf(sdev, data, data_len);
> +		if (ret < 0)
> +			return ret;
> +		if (ret != data_len)
> +			return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int picogw_recv_answer(struct picogw_device *picodev,
> +	char *cmd, bool *ack, void *buf, int buf_len,
> +	unsigned long timeout)
> +{
> +	int len;
> +
> +	timeout = wait_for_completion_timeout(&picodev->answer_comp, timeout);
> +	if (!timeout)
> +		return -ETIMEDOUT;
> +
> +	if (cmd)
> +		*cmd = picodev->rx_buf[0];
> +
> +	if (ack)
> +		*ack = (picodev->rx_buf[3] == 1);
> +
> +	len = min(picodev->rx_len - 4, buf_len);
> +	if (buf)
> +		memcpy(buf, picodev->rx_buf + 4, len);
> +
> +	reinit_completion(&picodev->answer_comp);
> +	complete(&picodev->answer_read_comp);
> +
> +	return len;
> +}
> +
> +static int picogw_reg_read(struct picogw_device *picodev, u8 addr, u8 *val, unsigned long timeout)
> +{
> +	const u8 dummy = 0;
> +	char cmd;
> +	bool ack;
> +	int ret;
> +
> +	ret = picogw_send_cmd(picodev, 'r', addr, &dummy, false ? 1 : 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = picogw_recv_answer(picodev, &cmd, &ack, val, 1, timeout);
> +	if (ret < 0)
> +		return ret;
> +	if (cmd != 'r')
> +		return -EIO;
> +	if (!ack || ret != 1)
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static int picogw_reg_write(struct picogw_device *picodev, u8 addr, u8 val, unsigned long timeout)
> +{
> +	char cmd;
> +	bool ack;
> +	int ret;
> +
> +	ret = picogw_send_cmd(picodev, 'w', addr, &val, 1);
> +	if (ret)
> +		return ret;
> +
> +	ret = picogw_recv_answer(picodev, &cmd, &ack, NULL, 0, timeout);
> +	if (ret < 0)
> +		return ret;
> +	if (cmd != 'w')
> +		return -EIO;
> +	if (!ack || ret != 0)
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static int picogw_mcu_fw_check(struct picogw_device *picodev,
> +	u32 fw_version, u8 *id, unsigned long timeout)
> +{
> +	char cmd;
> +	bool ack;
> +	int ret;
> +
> +	fw_version = cpu_to_be32(fw_version);
> +	ret = picogw_send_cmd(picodev, 'l', 0, &fw_version, sizeof(fw_version));
> +	if (ret)
> +		return ret;
> +
> +	ret = picogw_recv_answer(picodev, &cmd, &ack, id, id ? 8 : 0, timeout);
> +	if (ret < 0)
> +		return ret;
> +	if (cmd != 'l')
> +		return -EIO;
> +	if (id && ret != 8)
> +		return -EIO;
> +
> +	return ack ? 0 : -ENOTSUPP;
> +}
> +
> +static int picogw_regmap_gather_write(void *context,
> +	const void *reg_buf, size_t reg_size, const void *val_buf, size_t val_size)
> +{
> +	struct picogw_device *picodev = context;
> +	const u8 *addr_buf = reg_buf;
> +	const u8 *val = val_buf;
> +	u8 addr;
> +	int ret;
> +
> +	//dev_dbg(&picodev->serdev->dev, "%s: 0x%x (reg_size %zu) 0x%x (val_size %zu)\n",
> +	//	__func__, (unsigned int)addr_buf[0], reg_size, (unsigned int)val[0], val_size);
> +
> +	if (reg_size != 1 || val_size > 0xffff)
> +		return -EINVAL;
> +
> +	addr = addr_buf[0] & ~BIT(7);
> +	if (val_size == 1) {
> +		ret = picogw_reg_write(picodev, addr, val[0], HZ);
> +		if (ret)
> +			return ret;
> +		return 0;
> +	} else {
> +		/* TODO burst mode */
> +		dev_err(&picodev->serdev->dev, "burst mode write not yet implemented\n");
> +		return -ENOTSUPP;
> +	}
> +}
> +
> +static int picogw_regmap_write(void *context,
> +	const void *data_buf, size_t count)
> +{
> +	const u8 *data = data_buf;
> +	if (count < 1)
> +		return -EINVAL;
> +
> +	return picogw_regmap_gather_write(context, data, 1, data + 1, count - 1);
> +}
> +
> +static int picogw_regmap_read(void *context,
> +	const void *reg_buf, size_t reg_size, void *val_buf, size_t val_size)
> +{
> +	struct picogw_device *picodev = context;
> +	const u8 *addr_buf = reg_buf;
> +	u8 addr;
> +	int ret;
> +
> +	//dev_dbg(&picodev->serdev->dev, "%s: 0x%x (reg_size %zu) (val_size %zu)\n", __func__, (unsigned int)addr_buf[0], reg_size, val_size);
> +
> +	if (reg_size != 1 || val_size > 0xffff)
> +		return -EINVAL;
> +
> +	addr = addr_buf[0] & ~BIT(7);
> +	if (val_size == 1) {
> +		ret = picogw_reg_read(picodev, addr, val_buf, HZ);
> +		if (ret)
> +			return ret;
> +		return 0;
> +	} else {
> +		/* TODO burst mode */
> +		dev_err(&picodev->serdev->dev, "burst mode read not yet implemented\n");
> +		return -ENOTSUPP;
> +	}
> +}
> +
> +static const struct regmap_bus picogw_regmap_bus = {
> +	.write = picogw_regmap_write,
> +	.gather_write = picogw_regmap_gather_write,
> +	.read = picogw_regmap_read,
> +
> +	.max_raw_write = 0xffff,
> +	.max_raw_read = 0xffff,
> +};
> +
> +static int picogw_handle_answer(struct picogw_device *picodev)
> +{
> +	struct device *dev = &picodev->serdev->dev;
> +	unsigned int data_len = ((u16)picodev->rx_buf[1] << 8) | picodev->rx_buf[2];
> +	int cmd_len = 4 + data_len;
> +	int i, ret;
> +
> +	if (picodev->rx_len < 4)
> +		return 0;
> +
> +	if (cmd_len > sizeof(picodev->rx_buf)) {
> +		dev_warn(dev, "answer too long (%u)\n", data_len);
> +		return 0;
> +	}
> +
> +	if (picodev->rx_len < cmd_len) {
> +		dev_dbg(dev, "got %u, need %u bytes\n", picodev->rx_len, cmd_len);
> +		return 0;
> +	}
> +
> +	dev_dbg(dev, "Answer %c =%u %s (%u)\n", picodev->rx_buf[0],
> +		(unsigned int)picodev->rx_buf[3],
> +		(picodev->rx_buf[3] == 1) ? "OK" : "K0",
> +		data_len);
> +	for (i = 0; i < data_len; i++) {
> +		//dev_dbg(dev, "%s: %02x\n", __func__, (unsigned int)picodev->rx_buf[4 + i]);
> +	}
> +
> +	complete(&picodev->answer_comp);
> +	ret = wait_for_completion_interruptible_timeout(&picodev->answer_read_comp, HZ / 2);
> +	if (ret < 0)
> +		return 0;
> +
> +	reinit_completion(&picodev->answer_read_comp);
> +
> +	return cmd_len;
> +}
> +
> +static int picogw_receive_buf(struct serdev_device *sdev, const u8 *data, size_t count)
> +{
> +	struct picogw_device *picodev = picogw_get_drvdata(sdev);
> +	size_t i;
> +	int len = 0;
> +
> +	dev_dbg(&sdev->dev, "Receive (%zu)\n", count);
> +
> +	if (completion_done(&picodev->answer_comp)) {
> +		dev_info(&sdev->dev, "RX waiting on completion\n");
> +		return 0;
> +	}
> +	if (picodev->rx_len == sizeof(picodev->rx_buf)) {
> +		dev_warn(&sdev->dev, "RX buffer full\n");
> +		return 0;
> +	}
> +
> +	i = min(count, sizeof(picodev->rx_buf) - picodev->rx_len);
> +	if (i > 0) {
> +		memcpy(&picodev->rx_buf[picodev->rx_len], data, i);
> +		picodev->rx_len += i;
> +		len += i;
> +	}
> +
> +	while (picodev->rx_len > 0) {
> +		/* search for valid beginning */
> +		for (i = 0; i < picodev->rx_len; i++) {
> +			if (picogw_valid_cmd(picodev->rx_buf[i]))
> +				break;
> +		}
> +		if (i > 0) {
> +			dev_dbg(&sdev->dev, "skipping %zu bytes of garbage\n", i);
> +			if (i < picodev->rx_len) {
> +				memmove(picodev->rx_buf, &picodev->rx_buf[i], picodev->rx_len - i);
> +				picodev->rx_len -= i;
> +			} else
> +				picodev->rx_len = 0;
> +		}
> +
> +		i = picogw_handle_answer(picodev);
> +		if (i == 0)
> +			break;
> +
> +		if (i % 64 == 0) {
> +			dev_info(&sdev->dev, "skipping padding byte\n");
> +			i++;
> +		}
> +		if (picodev->rx_len > i)
> +			memmove(picodev->rx_buf, &picodev->rx_buf[i], picodev->rx_len - i);
> +		if (picodev->rx_len >= i)
> +			picodev->rx_len -= i;
> +	}
> +
> +	return len;
> +}
> +
> +static const struct serdev_device_ops picogw_serdev_client_ops = {
> +	.receive_buf = picogw_receive_buf,
> +	.write_wakeup = serdev_device_write_wakeup,
> +};
> +
> +static int picogw_serdev_probe(struct serdev_device *sdev)
> +{
> +	struct picogw_device *picodev;
> +	struct regmap *regmap;
> +	u32 fw_version = 0x010a0006;

Worth moving the version to a define?

> +	u8 mac[8];here
> +	int ret;
> +
> +	//dev_info(&sdev->dev, "Probing\n");
> +
> +	picodev = devm_kzalloc(&sdev->dev, sizeof(*picodev), GFP_KERNEL);
> +	if (!picodev)
> +		return -ENOMEM;
> +
> +	picodev->serdev = sdev;
> +	init_completion(&picodev->answer_comp);
> +	init_completion(&picodev->answer_read_comp);
> +
> +	ret = serdev_device_open(sdev);
> +	if (ret) {
> +		dev_err(&sdev->dev, "Failed to open (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	serdev_device_set_baudrate(sdev, 115200);
> +	serdev_device_set_parity(sdev, SERDEV_PARITY_NONE);
> +	serdev_device_set_flow_control(sdev, true);
> +
> +	regmap = devm_regmap_init(&sdev->dev, &picogw_regmap_bus, picodev, &sx130x_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		ret = PTR_ERR(regmap);
> +		dev_err(&sdev->dev, "error initializing regmap (%d)\n", ret);
> +		serdev_device_close(sdev);
> +		return ret;
> +	}
> +
> +	ret = sx130x_early_probe(regmap, NULL);
> +	if (ret) {
> +		serdev_device_close(sdev);
> +		return ret;
> +	}
> +
> +	sx130x_set_drvdata(&sdev->dev, picodev);
> +	serdev_device_set_client_ops(sdev, &picogw_serdev_client_ops);
> +
> +	//msleep(1000);
> +	ret = picogw_mcu_fw_check(picodev, fw_version, mac, HZ);
> +	if (!ret || ret == -ENOTSUPP)
> +		dev_info(&sdev->dev, "ID = %02x%02x%02x%02x%02x%02x%02x%02x\n",
> +			(unsigned int)mac[0],
> +			(unsigned int)mac[1],
> +			(unsigned int)mac[2],
> +			(unsigned int)mac[3],
> +			(unsigned int)mac[4],
> +			(unsigned int)mac[5],
> +			(unsigned int)mac[6],
> +			(unsigned int)mac[7]);
> +	while (ret == -ENOTSUPP && ((fw_version & 0xff) > 4)) {
> +		ret = picogw_mcu_fw_check(picodev, --fw_version, NULL, HZ);
> +	}
> +	if (ret == -ENOTSUPP) {
> +		dev_warn(&sdev->dev, "firmware check failed (%08x)\n", fw_version);
> +	} else {
> +		dev_err(&sdev->dev, "ID retrieval failed (%d)\n", ret);
> +		serdev_device_close(sdev);
> +		return ret;
> +	}
> +
> +	ret = sx130x_probe(&sdev->dev);
> +	if (ret) {
> +		serdev_device_close(sdev);
> +		return ret;
> +	}
> +
> +	//dev_info(&sdev->dev, "Done.\n");
> +
> +	return 0;
> +}
> +
> +static void picogw_serdev_remove(struct serdev_device *sdev)
> +{
> +	sx130x_remove(&sdev->dev);
> +
> +	serdev_device_close(sdev);
> +
> +	//dev_info(&sdev->dev, "Removed\n");
> +}
> +
> +static const struct of_device_id picogw_serdev_of_match[] = {
> +	{ .compatible = "semtech,lora-picocell" },

lora-picocell or lora-picogw... picocell has mobile connotations, should 
we match the module name?

> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, picogw_serdev_of_match);
> +
> +static struct serdev_device_driver picogw_serdev_driver = {
> +	.probe = picogw_serdev_probe,
> +	.remove = picogw_serdev_remove,
> +	.driver = {
> +		.name = "lora-picogw",
> +		.of_match_table = picogw_serdev_of_match,
> +	},
> +};
> +
> +static int __init picogw_serdev_init(void)
> +{
> +	int ret;
> +
> +	ret = serdev_device_driver_register(&picogw_serdev_driver);
> +	if (ret) {
> +		pr_err("serdev_device_driver_register failed (%d)", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +module_init(picogw_serdev_init);
> +
> +static void __exit picogw_serdev_exit(void)
> +{
> +	serdev_device_driver_unregister(&picogw_serdev_driver);
> +}
> +module_exit(picogw_serdev_exit);
> +
> +MODULE_LICENSE("GPL");
> 

Thanks,
Ben Whitten

Re: [PATCH lora-next 3/5] net: lora: sx130x: Add PicoCell serdev driver

[Oliver Neukum]

On Fr, 2019-01-04 at 12:21 +0100, Andreas Färber  wrote:
> 
> +struct picogw_device {
> +	struct serdev_device *serdev;
> +
> +	u8 rx_buf[1024];

No, you cannot do that. AFAICT this buffer can be used for DMA.
Thus putting it into another data structure violates the rules
of DMA coherency. You must allocate it separately.

> +static int picogw_send_cmd(struct picogw_device *picodev, char cmd,
> +	u8 addr, const void *data, u16 data_len)
> +{
> +	struct serdev_device *sdev = picodev->serdev;
> +	u8 buf[4];
> +	int i, ret;
> +
> +	buf[0] = cmd;
> +	buf[1] = (data_len >> 8) & 0xff;
> +	buf[2] = (data_len >> 0) & 0xff;

We have macros for converting endianness and unaligned access.

> +	buf[3] = addr;
> +
> +	dev_dbg(&sdev->dev, "%s: %c %02x %02x %02x\n", __func__, buf[0],
> +		(unsigned int)buf[1], (unsigned int)buf[2], (unsigned int)buf[3]);
> +	for (i = 0; i < data_len; i++) {
> +		dev_dbg(&sdev->dev, "%s: data %02x\n", __func__, (unsigned int)((const u8*)data)[i]);
> +	}
> +
> +	ret = serdev_device_write_buf(sdev, buf, 4);
> +	if (ret < 0)
> +		return ret;
> +	if (ret != 4)
> +		return -EIO;
> +
> +	if (data_len) {
> +		ret = serdev_device_write_buf(sdev, data, data_len);
> +		if (ret < 0)
> +			return ret;
> +		if (ret != data_len)
> +			return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int picogw_recv_answer(struct picogw_device *picodev,
> +	char *cmd, bool *ack, void *buf, int buf_len,
> +	unsigned long timeout)
> +{
> +	int len;
> +
> +	timeout = wait_for_completion_timeout(&picodev->answer_comp, timeout);
> +	if (!timeout)
> +		return -ETIMEDOUT;

And? The IO is still scheduled. Simply erroring out is problematic.
If you see a timeout you need to kill the scheduled IO.

> +static int picogw_handle_answer(struct picogw_device *picodev)
> +{
> +	struct device *dev = &picodev->serdev->dev;
> +	unsigned int data_len = ((u16)picodev->rx_buf[1] << 8) | picodev->rx_buf[2];
> +	int cmd_len = 4 + data_len;
> +	int i, ret;
> +
> +	if (picodev->rx_len < 4)
> +		return 0;
> +
> +	if (cmd_len > sizeof(picodev->rx_buf)) {
> +		dev_warn(dev, "answer too long (%u)\n", data_len);
> +		return 0;
> +	}
> +
> +	if (picodev->rx_len < cmd_len) {
> +		dev_dbg(dev, "got %u, need %u bytes\n", picodev->rx_len, cmd_len);
> +		return 0;
> +	}
> +
> +	dev_dbg(dev, "Answer %c =%u %s (%u)\n", picodev->rx_buf[0],
> +		(unsigned int)picodev->rx_buf[3],
> +		(picodev->rx_buf[3] == 1) ? "OK" : "K0",
> +		data_len);
> +	for (i = 0; i < data_len; i++) {
> +		//dev_dbg(dev, "%s: %02x\n", __func__, (unsigned int)picodev->rx_buf[4 + i]);
> +	}
> +
> +	complete(&picodev->answer_comp);
> +	ret = wait_for_completion_interruptible_timeout(&picodev->answer_read_comp, HZ / 2);

Problematic. You have no idea when that complete() will have an effect.
You are operating with an undefined timeout here. Theoretically it
could be negative.

	Regards
		Oliver

[PATCH RFC 4/5] usb: cdc-acm: Enable serdev support

[Andreas Färber]

Switch from tty_port_register_device() to tty_port_register_device_serdev()
and from tty_unregister_device() to tty_port_unregister_device().

On removal of a serdev driver sometimes count mismatch warnings were seen:

  ttyACM ttyACM0: tty_hangup: tty->count(1) != (#fd's(0) + #kopen's(0))
  ttyACM ttyACM0: tty_port_close_start: tty->count = 1 port count = 0

Note: The serdev drivers appear to probe asynchronously as soon as they
are registered. Should the USB quirks in probe be moved before registration?
No noticeable difference for the devices at hand.

Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 drivers/usb/class/cdc-acm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index ed8c62b2d9d1..c225a586c524 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1460,7 +1460,7 @@ static int acm_probe(struct usb_interface *intf,
 	usb_set_intfdata(data_interface, acm);
 
 	usb_get_intf(control_interface);
-	tty_dev = tty_port_register_device(&acm->port, acm_tty_driver, minor,
+	tty_dev = tty_port_register_device_serdev(&acm->port, acm_tty_driver, minor,
 			&control_interface->dev);
 	if (IS_ERR(tty_dev)) {
 		rv = PTR_ERR(tty_dev);
@@ -1534,7 +1534,7 @@ static void acm_disconnect(struct usb_interface *intf)
 	acm_kill_urbs(acm);
 	cancel_work_sync(&acm->work);
 
-	tty_unregister_device(acm_tty_driver, acm->minor);
+	tty_port_unregister_device(&acm->port, acm_tty_driver, acm->minor);
 
 	usb_free_urb(acm->ctrlurb);
 	for (i = 0; i < ACM_NW; i++)
-- 
2.16.4

Re: [PATCH RFC 4/5] usb: cdc-acm: Enable serdev support

[Oliver Neukum]

On Fr, 2019-01-04 at 12:21 +0100, Andreas Färber  wrote:
> Switch from tty_port_register_device() to tty_port_register_device_serdev()
> and from tty_unregister_device() to tty_port_unregister_device().
> 
> On removal of a serdev driver sometimes count mismatch warnings were seen:
> 
>   ttyACM ttyACM0: tty_hangup: tty->count(1) != (#fd's(0) + #kopen's(0))
>   ttyACM ttyACM0: tty_port_close_start: tty->count = 1 port count = 0
> 
> Note: The serdev drivers appear to probe asynchronously as soon as they
> are registered. Should the USB quirks in probe be moved before registration?
> No noticeable difference for the devices at hand.

That is quite drastic a change.
Johan, how complete in terms of features is serdev?

Are you refering to CLEAR_HALT_CONDITIONS in terms of quirks?

	Regards
		Oliver

Re: [PATCH RFC 4/5] usb: cdc-acm: Enable serdev support

[Johan Hovold]

On Mon, Jan 07, 2019 at 02:48:26PM +0100, Oliver Neukum wrote:
> On Fr, 2019-01-04 at 12:21 +0100, Andreas Färber  wrote:
> > Switch from tty_port_register_device() to tty_port_register_device_serdev()
> > and from tty_unregister_device() to tty_port_unregister_device().
> > 
> > On removal of a serdev driver sometimes count mismatch warnings were seen:
> > 
> >   ttyACM ttyACM0: tty_hangup: tty->count(1) != (#fd's(0) + #kopen's(0))
> >   ttyACM ttyACM0: tty_port_close_start: tty->count = 1 port count = 0
> > 
> > Note: The serdev drivers appear to probe asynchronously as soon as they
> > are registered. Should the USB quirks in probe be moved before registration?
> > No noticeable difference for the devices at hand.
> 
> That is quite drastic a change.
> Johan, how complete in terms of features is serdev?

serdev doesn't support hangups yet, and that's precisely why it's not
enabled for hotpluggable buses. That would need to be fixed before
accepting something like this.

Johan

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

[Andreas Färber]

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

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) },
-- 
2.16.4

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

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

Rob

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

[Andreas Färber]

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

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

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

[Johan Hovold]

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