| Message ID | 20180701110804.32415-16-afaerber@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andreas, Excellent work on doing this I have also been working on and off this personally for some time. Have a look at my repository [1] for sx1301 and sx1257 drivers, I use regmaps capability of switching pages which should simplify your driver considerably, I also have a full register map and bit field. I have also been trying to use the clk framework to capture the various routing that the cards have. I will dig into this series this evening. [1] https://github.com/BWhitten/linux-stable/tree/971aadc8fdfe842020d912449bdd71b33d576fe3/drivers/net/lora > Subject: [RFC net-next 15/15] net: lora: Add Semtech SX1301 > > The Semtech SX1301 was the first multi-channel LoRa "concentrator". > It uses a SPI interface to the host as well as a dual SPI interface to > its radios. These two have been implemented as spi_controller, so that > the Device Tree can specify whether the respective module uses two > SX1257, two SX1255 or a combination of these or some unforeseen chipset. > > This implementation is the most recent - initialization is not yet > complete, it will need to load firmware into the two on-chip MCUs. > > Unfortunately there is no full datasheet with register descriptions, > only a BSD-licensed userspace HAL implementation using spidev devices. > Therefore some register names are unknown. > > Cc: Ben Whitten <ben.whitten@lairdtech.com> > Cc: Steve deRosier <derosier@gmail.com> > Cc: Mark Brown <broonie@kernel.org> > Cc: Michael Röder <michael.roeder@avnet.eu> > Cc: Ken Yu (禹凯) <ken.yu@rakwireless.com> > Cc: linux-spi@vger.kernel.org > Signed-off-by: Andreas Färber <afaerber@suse.de> > --- > drivers/net/lora/Kconfig | 7 + > drivers/net/lora/Makefile | 3 + > drivers/net/lora/sx1301.c | 446 > ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 456 insertions(+) > create mode 100644 drivers/net/lora/sx1301.c > > diff --git a/drivers/net/lora/Kconfig b/drivers/net/lora/Kconfig > index 68c7480d7812..950450e353b4 100644 > --- a/drivers/net/lora/Kconfig > +++ b/drivers/net/lora/Kconfig > @@ -45,6 +45,13 @@ config LORA_SX1276 > help > Semtech SX1272/1276/1278 > > +config LORA_SX1301 > + tristate "Semtech SX1301 SPI driver" > + default y > + depends on SPI > + help > + Semtech SX1301 > + > config LORA_USI > tristate "USI WM-SG-SM-42 driver" > default y > diff --git a/drivers/net/lora/Makefile b/drivers/net/lora/Makefile > index 44c578bde7d5..1cc1e3aa189b 100644 > --- a/drivers/net/lora/Makefile > +++ b/drivers/net/lora/Makefile > @@ -22,6 +22,9 @@ lora-sx1257-y := sx1257.o > obj-$(CONFIG_LORA_SX1276) += lora-sx1276.o > lora-sx1276-y := sx1276.o > > +obj-$(CONFIG_LORA_SX1301) += lora-sx1301.o > +lora-sx1301-y := sx1301.o > + > obj-$(CONFIG_LORA_USI) += lora-usi.o > lora-usi-y := usi.o > > diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c > new file mode 100644 > index 000000000000..5c936c1116d1 > --- /dev/null > +++ b/drivers/net/lora/sx1301.c > @@ -0,0 +1,446 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Semtech SX1301 LoRa concentrator > + * > + * Copyright (c) 2018 Andreas Färber > + * > + * Based on SX1301 HAL code: > + * Copyright (c) 2013 Semtech-Cycleo > + */ > + > +#include <linux/bitops.h> > +#include <linux/delay.h> > +#include <linux/lora.h> > +#include <linux/module.h> > +#include <linux/netdevice.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/of_gpio.h> > +#include <linux/lora/dev.h> > +#include <linux/spi/spi.h> > + > +#define REG_PAGE_RESET 0 > +#define REG_VERSION 1 > +#define REG_2_SPI_RADIO_A_DATA 33 > +#define REG_2_SPI_RADIO_A_DATA_READBACK 34 > +#define REG_2_SPI_RADIO_A_ADDR 35 > +#define REG_2_SPI_RADIO_A_CS 37 > +#define REG_2_SPI_RADIO_B_DATA 38 > +#define REG_2_SPI_RADIO_B_DATA_READBACK 39 > +#define REG_2_SPI_RADIO_B_ADDR 40 > +#define REG_2_SPI_RADIO_B_CS 42 > + > +#define REG_PAGE_RESET_SOFT_RESET BIT(7) > + > +#define REG_16_GLOBAL_EN BIT(3) > + > +#define REG_17_CLK32M_EN BIT(0) > + > +#define REG_2_43_RADIO_A_EN BIT(0) > +#define REG_2_43_RADIO_B_EN BIT(1) > +#define REG_2_43_RADIO_RST BIT(2) > + > +struct spi_sx1301 { > + struct spi_device *parent; > + u8 page; > + u8 regs; > +}; > + > +struct sx1301_priv { > + struct lora_priv lora; > + struct gpio_desc *rst_gpio; > + u8 cur_page; > + struct spi_controller *radio_a_ctrl, *radio_b_ctrl; > +}; > + > +static int sx1301_read(struct spi_device *spi, u8 reg, u8 *val) > +{ > + u8 addr = reg & 0x7f; > + return spi_write_then_read(spi, &addr, 1, val, 1); > +} > + > +static int sx1301_write(struct spi_device *spi, u8 reg, u8 val) > +{ > + u8 buf[2]; > + > + buf[0] = reg | BIT(7); > + buf[1] = val; > + return spi_write(spi, buf, 2); > +} > + > +static int sx1301_page_switch(struct spi_device *spi, u8 page) > +{ > + struct sx1301_priv *priv = spi_get_drvdata(spi); > + int ret; > + > + if (priv->cur_page == page) > + return 0; > + > + dev_dbg(&spi->dev, "switching to page %u\n", (unsigned)page); > + ret = sx1301_write(spi, REG_PAGE_RESET, page & 0x3); > + if (ret) { > + dev_err(&spi->dev, "switching to page %u failed\n", > (unsigned)page); > + return ret; > + } > + > + priv->cur_page = page; > + > + return 0; > +} > + > +static int sx1301_soft_reset(struct spi_device *spi) > +{ > + return sx1301_write(spi, REG_PAGE_RESET, > REG_PAGE_RESET_SOFT_RESET); > +} > + > +#define REG_RADIO_X_DATA 0 > +#define REG_RADIO_X_DATA_READBACK 1 > +#define REG_RADIO_X_ADDR 2 > +#define REG_RADIO_X_CS 4 > + > +static int sx1301_radio_set_cs(struct spi_controller *ctrl, bool enable) > +{ > + struct spi_sx1301 *ssx = spi_controller_get_devdata(ctrl); > + u8 cs; > + int ret; > + > + dev_dbg(&ctrl->dev, "setting CS to %s\n", enable ? "1" : "0"); > + > + ret = sx1301_page_switch(ssx->parent, ssx->page); > + if (ret) { > + dev_warn(&ctrl->dev, "failed to switch page for CS (%d)\n", > ret); > + return ret; > + } > + > + ret = sx1301_read(ssx->parent, ssx->regs + REG_RADIO_X_CS, &cs); > + if (ret) { > + dev_warn(&ctrl->dev, "failed to read CS (%d)\n", ret); > + cs = 0; > + } > + > + if (enable) > + cs |= BIT(0); > + else > + cs &= ~BIT(0); > + > + ret = sx1301_write(ssx->parent, ssx->regs + REG_RADIO_X_CS, cs); > + if (ret) > + dev_warn(&ctrl->dev, "failed to write CS (%d)\n", ret); > + > + return 0; > +} > + > +static void sx1301_radio_spi_set_cs(struct spi_device *spi, bool enable) > +{ > + int ret; > + > + dev_dbg(&spi->dev, "setting SPI CS to %s\n", enable ? "1" : "0"); > + > + if (enable) > + return; > + > + ret = sx1301_radio_set_cs(spi->controller, enable); > + if (ret) > + dev_warn(&spi->dev, "failed to write CS (%d)\n", ret); > +} > + > +static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl, > + struct spi_device *spi, struct spi_transfer *xfr) > +{ > + struct spi_sx1301 *ssx = spi_controller_get_devdata(ctrl); > + const u8 *tx_buf = xfr->tx_buf; > + u8 *rx_buf = xfr->rx_buf; > + int ret; > + > + if (xfr->len == 0 || xfr->len > 3) > + return -EINVAL; > + > + dev_dbg(&spi->dev, "transferring one (%u)\n", xfr->len); > + > + ret = sx1301_page_switch(ssx->parent, ssx->page); > + if (ret) { > + dev_err(&spi->dev, "failed to switch page for transfer > (%d)\n", ret); > + return ret; > + } > + > + if (tx_buf) { > + ret = sx1301_write(ssx->parent, ssx->regs + > REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0); > + if (ret) { > + dev_err(&spi->dev, "SPI radio address write > failed\n"); > + return ret; > + } > + > + ret = sx1301_write(ssx->parent, ssx->regs + > REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0); > + if (ret) { > + dev_err(&spi->dev, "SPI radio data write failed\n"); > + return ret; > + } > + > + ret = sx1301_radio_set_cs(ctrl, true); > + if (ret) { > + dev_err(&spi->dev, "SPI radio CS set failed\n"); > + return ret; > + } > + > + ret = sx1301_radio_set_cs(ctrl, false); > + if (ret) { > + dev_err(&spi->dev, "SPI radio CS unset failed\n"); > + return ret; > + } > + } > + > + if (rx_buf) { > + ret = sx1301_read(ssx->parent, ssx->regs + > REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]); > + if (ret) { > + dev_err(&spi->dev, "SPI radio data read failed\n"); > + return ret; > + } > + } > + > + return 0; > +} > + > +static void sx1301_radio_setup(struct spi_controller *ctrl) > +{ > + ctrl->mode_bits = SPI_CS_HIGH | SPI_NO_CS; > + ctrl->bits_per_word_mask = SPI_BPW_MASK(8); > + ctrl->num_chipselect = 1; > + ctrl->set_cs = sx1301_radio_spi_set_cs; > + ctrl->transfer_one = sx1301_radio_spi_transfer_one; > +} > + > +static int sx1301_probe(struct spi_device *spi) > +{ > + struct net_device *netdev; > + struct sx1301_priv *priv; > + struct spi_sx1301 *radio; > + struct gpio_desc *rst; > + int ret; > + u8 val; > + > + rst = devm_gpiod_get_optional(&spi->dev, "reset", > GPIOD_OUT_LOW); > + if (IS_ERR(rst)) > + 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); > + > + ret = sx1301_read(spi, REG_VERSION, &val); > + if (ret) { > + dev_err(&spi->dev, "version read failed\n"); > + goto err_version; > + } > + > + if (val != 103) { > + dev_err(&spi->dev, "unexpected version: %u\n", val); > + ret = -ENXIO; > + goto err_version; > + } > + > + netdev = alloc_loradev(sizeof(*priv)); > + if (!netdev) { > + ret = -ENOMEM; > + goto err_alloc_loradev; > + } > + > + priv = netdev_priv(netdev); > + priv->rst_gpio = rst; > + priv->cur_page = 0xff; > + > + spi_set_drvdata(spi, netdev); > + SET_NETDEV_DEV(netdev, &spi->dev); > + > + ret = sx1301_write(spi, REG_PAGE_RESET, 0); > + if (ret) { > + dev_err(&spi->dev, "page/reset write failed\n"); > + return ret; > + } > + > + ret = sx1301_soft_reset(spi); > + if (ret) { > + dev_err(&spi->dev, "soft reset failed\n"); > + return ret; > + } > + > + ret = sx1301_read(spi, 16, &val); > + if (ret) { > + dev_err(&spi->dev, "16 read failed\n"); > + return ret; > + } > + > + val &= ~REG_16_GLOBAL_EN; > + > + ret = sx1301_write(spi, 16, val); > + if (ret) { > + dev_err(&spi->dev, "16 write failed\n"); > + return ret; > + } > + > + ret = sx1301_read(spi, 17, &val); > + if (ret) { > + dev_err(&spi->dev, "17 read failed\n"); > + return ret; > + } > + > + val &= ~REG_17_CLK32M_EN; > + > + ret = sx1301_write(spi, 17, val); > + if (ret) { > + dev_err(&spi->dev, "17 write failed\n"); > + return ret; > + } > + > + ret = sx1301_page_switch(spi, 2); > + if (ret) { > + dev_err(&spi->dev, "page 2 switch failed\n"); > + return ret; > + } > + > + ret = sx1301_read(spi, 43, &val); > + if (ret) { > + dev_err(&spi->dev, "2|43 read failed\n"); > + return ret; > + } > + > + val |= REG_2_43_RADIO_B_EN | REG_2_43_RADIO_A_EN; > + > + ret = sx1301_write(spi, 43, val); > + if (ret) { > + dev_err(&spi->dev, "2|43 write failed\n"); > + return ret; > + } > + > + msleep(500); > + > + ret = sx1301_read(spi, 43, &val); > + if (ret) { > + dev_err(&spi->dev, "2|43 read failed\n"); > + return ret; > + } > + > + val |= REG_2_43_RADIO_RST; > + > + ret = sx1301_write(spi, 43, val); > + if (ret) { > + dev_err(&spi->dev, "2|43 write failed\n"); > + return ret; > + } > + > + msleep(5); > + > + ret = sx1301_read(spi, 43, &val); > + if (ret) { > + dev_err(&spi->dev, "2|43 read failed\n"); > + return ret; > + } > + > + val &= ~REG_2_43_RADIO_RST; > + > + ret = sx1301_write(spi, 43, val); > + if (ret) { > + dev_err(&spi->dev, "2|43 write failed\n"); > + return ret; > + } > + > + /* radio A */ > + > + priv->radio_a_ctrl = spi_alloc_master(&spi->dev, sizeof(*radio)); > + if (!priv->radio_a_ctrl) { > + ret = -ENOMEM; > + goto err_radio_a_alloc; > + } > + > + sx1301_radio_setup(priv->radio_a_ctrl); > + priv->radio_a_ctrl->dev.of_node = of_get_child_by_name(spi- > >dev.of_node, "radio-a"); > + > + radio = spi_controller_get_devdata(priv->radio_a_ctrl); > + radio->page = 2; > + radio->regs = REG_2_SPI_RADIO_A_DATA; > + radio->parent = spi; > + > + dev_info(&spi->dev, "registering radio A SPI\n"); > + > + ret = devm_spi_register_controller(&spi->dev, priv->radio_a_ctrl); > + if (ret) { > + dev_err(&spi->dev, "radio A SPI register failed\n"); > + goto err_radio_a_register; > + } > + > + /* radio B */ > + > + priv->radio_b_ctrl = spi_alloc_master(&spi->dev, sizeof(*radio)); > + if (!priv->radio_b_ctrl) { > + ret = -ENOMEM; > + goto err_radio_b_alloc; > + } > + > + sx1301_radio_setup(priv->radio_b_ctrl); > + priv->radio_b_ctrl->dev.of_node = of_get_child_by_name(spi- > >dev.of_node, "radio-b"); > + > + radio = spi_controller_get_devdata(priv->radio_b_ctrl); > + radio->page = 2; > + radio->regs = REG_2_SPI_RADIO_B_DATA; > + radio->parent = spi; > + > + dev_info(&spi->dev, "registering radio B SPI\n"); > + > + ret = devm_spi_register_controller(&spi->dev, priv->radio_b_ctrl); > + if (ret) { > + dev_err(&spi->dev, "radio B SPI register failed\n"); > + goto err_radio_b_register; > + } > + > + dev_info(&spi->dev, "SX1301 module probed\n"); > + > + return 0; > +err_radio_b_register: > + spi_controller_put(priv->radio_b_ctrl); > +err_radio_b_alloc: > +err_radio_a_register: > + spi_controller_put(priv->radio_a_ctrl); > +err_radio_a_alloc: > + free_loradev(netdev); > +err_alloc_loradev: > +err_version: > + return ret; > +} > + > +static int sx1301_remove(struct spi_device *spi) > +{ > + struct net_device *netdev = spi_get_drvdata(spi); > + > + //unregister_loradev(netdev); > + free_loradev(netdev); > + > + dev_info(&spi->dev, "SX1301 module removed\n"); > + > + return 0; > +} > + > +#ifdef CONFIG_OF > +static const struct of_device_id sx1301_dt_ids[] = { > + { .compatible = "semtech,sx1301" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, sx1301_dt_ids); > +#endif > + > +static struct spi_driver sx1301_spi_driver = { > + .driver = { > + .name = "sx1301", > + .of_match_table = of_match_ptr(sx1301_dt_ids), > + }, > + .probe = sx1301_probe, > + .remove = sx1301_remove, > +}; > + > +module_spi_driver(sx1301_spi_driver); > + > +MODULE_DESCRIPTION("SX1301 SPI driver"); > +MODULE_AUTHOR("Andreas Färber <afaerber@suse.de>"); > +MODULE_LICENSE("GPL"); > -- > 2.16.4
On Sun, Jul 01, 2018 at 01:08:04PM +0200, Andreas Färber wrote: > +static void sx1301_radio_spi_set_cs(struct spi_device *spi, bool enable) > +{ > + int ret; > + > + dev_dbg(&spi->dev, "setting SPI CS to %s\n", enable ? "1" : "0"); > + > + if (enable) > + return; > + > + ret = sx1301_radio_set_cs(spi->controller, enable); > + if (ret) > + dev_warn(&spi->dev, "failed to write CS (%d)\n", ret); > +} So we never disable chip select? > + if (tx_buf) { > + ret = sx1301_write(ssx->parent, ssx->regs + REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0); This looks confused. We're in an if (tx_buf) block but there's a use of the ternery operator that appears to be checking if we have a tx_buf? > + if (ret) { > + dev_err(&spi->dev, "SPI radio address write failed\n"); > + return ret; > + } > + > + ret = sx1301_write(ssx->parent, ssx->regs + REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0); > + if (ret) { > + dev_err(&spi->dev, "SPI radio data write failed\n"); > + return ret; > + } This looks awfully like you're coming in at the wrong abstraction layer and the hardware actually implements a register abstraction rather than a SPI one so you should be using regmap as the abstraction. > + if (rx_buf) { > + ret = sx1301_read(ssx->parent, ssx->regs + REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]); > + if (ret) { > + dev_err(&spi->dev, "SPI radio data read failed\n"); > + return ret; > + } > + } For a read we never set an address? > +static void sx1301_radio_setup(struct spi_controller *ctrl) > +{ > + ctrl->mode_bits = SPI_CS_HIGH | SPI_NO_CS; This controller has no chip select but we provided a set_cs operation?
Hi Mark, This driver is still evolving, there's newer code on my lora-next branch already: https://github.com/afaerber/linux/commits/lora-next The reason you're in CC on this RFC is two-fold: 1) You applied Ben's patch to associate "semtech,sx1301" with spidev, whereas I am now preparing a new driver for the same compatible. 2) This SPI device is in turn exposing the two SPI masters that you already found below, and I didn't see a sane way to split that code out into drivers/spi/, so it's in drivers/net/lora/ here - has there been any precedence either way? More inline ... Am 02.07.2018 um 18:12 schrieb Mark Brown: > On Sun, Jul 01, 2018 at 01:08:04PM +0200, Andreas Färber wrote: > >> +static void sx1301_radio_spi_set_cs(struct spi_device *spi, bool enable) >> +{ >> + int ret; >> + >> + dev_dbg(&spi->dev, "setting SPI CS to %s\n", enable ? "1" : "0"); >> + >> + if (enable) >> + return; >> + >> + ret = sx1301_radio_set_cs(spi->controller, enable); >> + if (ret) >> + dev_warn(&spi->dev, "failed to write CS (%d)\n", ret); >> +} > > So we never disable chip select? Not here, I instead did that in transfer_one below. Unfortunately there seems to be no documentation, only reference code: https://github.com/Lora-net/lora_gateway/blob/master/libloragw/src/loragw_radio.c#L121 https://github.com/Lora-net/lora_gateway/blob/master/libloragw/src/loragw_radio.c#L165 It sets CS to 0 before writing to address and data registers, then immediately sets CS to 1 and back to 0 before reading or ending the write transaction. I've tried to force the same behavior in this driver. My guess was that CS is high-active during the short 1-0 cycle, because if it's low-active during the register writes then why the heck is it set to 0 again in the end instead of keeping at 1... confusing. Maybe the Semtech folks CC'ed can comment how these registers work? >> + if (tx_buf) { >> + ret = sx1301_write(ssx->parent, ssx->regs + REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0); > > This looks confused. We're in an if (tx_buf) block but there's a use of > the ternery operator that appears to be checking if we have a tx_buf? Yeah, as mentioned this RFC is not ready for merging - checkpatch.pl will complain about lines too long, and TODOs are sprinkled all over or not even mentioned. It's a Proof of Concept that a net_device could work for a wide range of spi and serdev based drivers, and on top this device has more than one channel, which may influence network-level design discussions. That said, I'll happily drop the second check. Thanks for spotting! >> + if (ret) { >> + dev_err(&spi->dev, "SPI radio address write failed\n"); >> + return ret; >> + } >> + >> + ret = sx1301_write(ssx->parent, ssx->regs + REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0); >> + if (ret) { >> + dev_err(&spi->dev, "SPI radio data write failed\n"); >> + return ret; >> + } > > This looks awfully like you're coming in at the wrong abstraction layer > and the hardware actually implements a register abstraction rather than > a SPI one so you should be using regmap as the abstraction. I don't understand. Ben has suggested using regmap for the SPI _device_ that we're talking to, which may be a good idea. But this SX1301 device in turn has two SPI _masters_ talking to an SX125x slave each. I don't see how using regmap instead of my wrappers avoids this spi_controller? The whole point of this spi_controller is to abstract and separate the SX1255 vs. SX1257 vs. whatever-radio-attached into a separate driver, instead of mixing it into the SX1301 driver - to me that looks cleaner and more extensible. It also has the side-effect that we could configure the two radios via DT (frequencies, clk output, etc.). You will find a datasheet with some diagrams mentioning "SPI" at: https://www.semtech.com/products/wireless-rf/lora-gateways/SX1301 >> + if (rx_buf) { >> + ret = sx1301_read(ssx->parent, ssx->regs + REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]); >> + if (ret) { >> + dev_err(&spi->dev, "SPI radio data read failed\n"); >> + return ret; >> + } >> + } > > For a read we never set an address? To read, you first write the address via tx_buf, then either in the same transfer in the third byte or in a subsequent one-byte transfer as first byte you get the data. If you have better ideas how to structure this, do let me know. >> +static void sx1301_radio_setup(struct spi_controller *ctrl) >> +{ >> + ctrl->mode_bits = SPI_CS_HIGH | SPI_NO_CS; > > This controller has no chip select but we provided a set_cs operation? Oops, I played around with those two options and was hoping SPI_NO_CS would avoid the undesired set_cs invocations, but it didn't work as expected and so I added the "if (enabled)" check above. Thanks for your review, Andreas
> Subject: Re: [RFC net-next 15/15] net: lora: Add Semtech SX1301 > > Hi Mark, > > This driver is still evolving, there's newer code on my lora-next branch > already: https://github.com/afaerber/linux/commits/lora-next > > The reason you're in CC on this RFC is two-fold: > > 1) You applied Ben's patch to associate "semtech,sx1301" with spidev, > whereas I am now preparing a new driver for the same compatible. > > 2) This SPI device is in turn exposing the two SPI masters that you > already found below, and I didn't see a sane way to split that code out > into drivers/spi/, so it's in drivers/net/lora/ here - has there been > any precedence either way? In my work in progress driver I just register one controller for the sx1301 with two chip selects and use the chip select information to choose the correct radio to send to, this is based on the DT reg information. No need to register two separate masters. > More inline ... > > Am 02.07.2018 um 18:12 schrieb Mark Brown: > > On Sun, Jul 01, 2018 at 01:08:04PM +0200, Andreas Färber wrote: > > > >> +static void sx1301_radio_spi_set_cs(struct spi_device *spi, bool enable) > >> +{ > >> + int ret; > >> + > >> + dev_dbg(&spi->dev, "setting SPI CS to %s\n", enable ? "1" : "0"); > >> + > >> + if (enable) > >> + return; > >> + > >> + ret = sx1301_radio_set_cs(spi->controller, enable); > >> + if (ret) > >> + dev_warn(&spi->dev, "failed to write CS (%d)\n", ret); > >> +} > > > > So we never disable chip select? > > Not here, I instead did that in transfer_one below. > > Unfortunately there seems to be no documentation, only reference code: > > https://github.com/Lora- > net/lora_gateway/blob/master/libloragw/src/loragw_radio.c#L121 > https://github.com/Lora- > net/lora_gateway/blob/master/libloragw/src/loragw_radio.c#L165 > > It sets CS to 0 before writing to address and data registers, then > immediately sets CS to 1 and back to 0 before reading or ending the > write transaction. I've tried to force the same behavior in this driver. > My guess was that CS is high-active during the short 1-0 cycle, because > if it's low-active during the register writes then why the heck is it > set to 0 again in the end instead of keeping at 1... confusing. > > Maybe the Semtech folks CC'ed can comment how these registers work? > > >> + if (tx_buf) { > >> + ret = sx1301_write(ssx->parent, ssx->regs + > REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0); > > > > This looks confused. We're in an if (tx_buf) block but there's a use of > > the ternery operator that appears to be checking if we have a tx_buf? > > Yeah, as mentioned this RFC is not ready for merging - checkpatch.pl > will complain about lines too long, and TODOs are sprinkled all over or > not even mentioned. It's a Proof of Concept that a net_device could work > for a wide range of spi and serdev based drivers, and on top this device > has more than one channel, which may influence network-level design > discussions. > > That said, I'll happily drop the second check. Thanks for spotting! > > >> + if (ret) { > >> + dev_err(&spi->dev, "SPI radio address write > failed\n"); > >> + return ret; > >> + } > >> + > >> + ret = sx1301_write(ssx->parent, ssx->regs + > REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0); > >> + if (ret) { > >> + dev_err(&spi->dev, "SPI radio data write failed\n"); > >> + return ret; > >> + } > > > > This looks awfully like you're coming in at the wrong abstraction layer > > and the hardware actually implements a register abstraction rather than > > a SPI one so you should be using regmap as the abstraction. > > I don't understand. Ben has suggested using regmap for the SPI _device_ > that we're talking to, which may be a good idea. But this SX1301 device > in turn has two SPI _masters_ talking to an SX125x slave each. I don't > see how using regmap instead of my wrappers avoids this spi_controller? > The whole point of this spi_controller is to abstract and separate the > SX1255 vs. SX1257 vs. whatever-radio-attached into a separate driver, > instead of mixing it into the SX1301 driver - to me that looks cleaner > and more extensible. It also has the side-effect that we could configure > the two radios via DT (frequencies, clk output, etc.). You want an SPI controller in the SX1301 as the down stream radios are SPI and could be attached directly to a host SPI bus, makes sense to have one radio driver and talk through the SX1301. But you should use the regmap to access the SX1301 master controller registers. Example I use with one SPI master and some clock info: eg: sx1301: sx1301@0 { compatible = "semtech,sx1301"; reg = <0>; #address-cells = <1>; #size-cells = <0>; spi-max-frequency = <8000000>; gpios-reset = <&pioA 26 GPIO_ACTIVE_HIGH>; clocks = <&radio1 0>, <&clkhs 0>; clock-names = "clk32m", "clkhs"; radio0: sx1257@0 { compatible = "semtech,sx125x"; reg = <0>; spi-max-frequency = <8000000>; tx; clocks = <&tcxo 0>; clock-names = "tcxo"; }; radio1: sx1257@1 { compatible = "semtech,sx125x"; reg = <1>; spi-max-frequency = <8000000>; #clock-cells = <0>; clocks = <&tcxo 0>; clock-names = "tcxo"; clock-output-names = "clk32m"; }; }; > You will find a datasheet with some diagrams mentioning "SPI" at: > https://www.semtech.com/products/wireless-rf/lora-gateways/SX1301 > > >> + if (rx_buf) { > >> + ret = sx1301_read(ssx->parent, ssx->regs + > REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]); > >> + if (ret) { > >> + dev_err(&spi->dev, "SPI radio data read failed\n"); > >> + return ret; > >> + } > >> + } > > > > For a read we never set an address? > > To read, you first write the address via tx_buf, then either in the same > transfer in the third byte or in a subsequent one-byte transfer as first > byte you get the data. > > If you have better ideas how to structure this, do let me know. > > >> +static void sx1301_radio_setup(struct spi_controller *ctrl) > >> +{ > >> + ctrl->mode_bits = SPI_CS_HIGH | SPI_NO_CS; > > > > This controller has no chip select but we provided a set_cs operation? > > Oops, I played around with those two options and was hoping SPI_NO_CS > would avoid the undesired set_cs invocations, but it didn't work as > expected and so I added the "if (enabled)" check above. > > Thanks for your review, > > Andreas > > -- > SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Felix Imendörffer, Jane Smithard, Graham Norton > HRB 21284 (AG Nürnberg)
Hi Ben, Am 02.07.2018 um 13:51 schrieb Ben Whitten: > Excellent work on doing this I have also been working on and off > this personally for some time. Thanks. Colliding work is always unfortunate, I can relate... > Have a look at my repository [1] for sx1301 and sx1257 drivers, > I use regmaps capability of switching pages which should simplify > your driver considerably, I also have a full register map and bit field. Please note that my lora-next branch already has bug fixes and cleanups over this patch. The probe error handling was broken, and I implemented wrappers for paged reads and writes as well as burst modes, plus the firmware loading. https://github.com/afaerber/linux/commits/lora-next I took a quick look at your sx1257 and noticed you licensed it as GPLv2. Is there any particular reason for that? Since I wrote my driver without copying from GPLv2 code, I prefer the less restrictive GPLv2+. So far a work day has passed with no maintainer objecting to or commenting on the underlying PF_LORA network layer design. Meanwhile there's already three of us with code and more people have inquired about testing and contributing, so I'm thinking about setting up a staging tree on kernel.org to collaborate on... Would you be willing to contribute your regmap ideas to my driver as a patch to squash? Needs a Signed-off-by of course, which your GitHub commits are lacking, so I can't merge them on my own. > I have also been trying to use the clk framework to capture the various > routing that the cards have. I thought about clk too, but won't that cause name conflicts when probing multiple concentrators? Would be nice to use that for configuring the SX1257 clock output instead of my current hack. Another thought I haven't investigated yet is whether we could use remoteproc for ARB and AGC. I would at least prefer to have the firmware as a binary loaded via the usual request_firmware(), not as byte array. But then again the AGC gets firmware loaded twice, so maybe too complex for remoteproc. BTW do you have any insights on what MCU is in there? Would be nice to understand in form of source code what the firmware is doing, to avoid the hard dependency on a specific firmware version (imagine user updating kernel-firmware - containing versions X,Y,Z - and kernel and booting two different kernel versions, the older one stops working). https://www.thethingsnetwork.org/forum/t/secret-price-of-a-lora-gateway/5730/74 Regards, Andreas > I will dig into this series this evening. > > [1] https://github.com/BWhitten/linux-stable/tree/971aadc8fdfe842020d912449bdd71b33d576fe3/drivers/net/lora [...] >> diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c >> new file mode 100644 >> index 000000000000..5c936c1116d1 >> --- /dev/null >> +++ b/drivers/net/lora/sx1301.c >> @@ -0,0 +1,446 @@ >> +// SPDX-License-Identifier: GPL-2.0-or-later [snip]
Hi Ben, Am 02.07.2018 um 22:43 schrieb Ben Whitten: >> 2) This SPI device is in turn exposing the two SPI masters that you >> already found below, and I didn't see a sane way to split that code out >> into drivers/spi/, so it's in drivers/net/lora/ here - has there been >> any precedence either way? > > In my work in progress driver I just register one controller for the sx1301 with two chip selects and use the chip select information to choose the correct radio to send to, this is based on the DT reg information. No need to register two separate masters. I had considered that and discarded it. The SX1301 has not just two CS registers though but also two pairs of addr, data registers. That speaks for two masters with a single chip-select each, unless I'm misunderstanding the meaning of the registers. >> Am 02.07.2018 um 18:12 schrieb Mark Brown: >>> On Sun, Jul 01, 2018 at 01:08:04PM +0200, Andreas Färber wrote: >>> >>>> +static void sx1301_radio_spi_set_cs(struct spi_device *spi, bool enable) >>>> +{ >>>> + int ret; >>>> + >>>> + dev_dbg(&spi->dev, "setting SPI CS to %s\n", enable ? "1" : "0"); >>>> + >>>> + if (enable) >>>> + return; >>>> + >>>> + ret = sx1301_radio_set_cs(spi->controller, enable); >>>> + if (ret) >>>> + dev_warn(&spi->dev, "failed to write CS (%d)\n", ret); >>>> +} >>> >>> So we never disable chip select? >> >> Not here, I instead did that in transfer_one below. >> >> Unfortunately there seems to be no documentation, only reference code: >> >> https://github.com/Lora- >> net/lora_gateway/blob/master/libloragw/src/loragw_radio.c#L121 >> https://github.com/Lora- >> net/lora_gateway/blob/master/libloragw/src/loragw_radio.c#L165 >> >> It sets CS to 0 before writing to address and data registers, then >> immediately sets CS to 1 and back to 0 before reading or ending the >> write transaction. I've tried to force the same behavior in this driver. >> My guess was that CS is high-active during the short 1-0 cycle, because >> if it's low-active during the register writes then why the heck is it >> set to 0 again in the end instead of keeping at 1... confusing. >> >> Maybe the Semtech folks CC'ed can comment how these registers work? >> >>>> + if (tx_buf) { >>>> + ret = sx1301_write(ssx->parent, ssx->regs + >> REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0); >>> >>> This looks confused. We're in an if (tx_buf) block but there's a use of >>> the ternery operator that appears to be checking if we have a tx_buf? >> >> Yeah, as mentioned this RFC is not ready for merging - checkpatch.pl >> will complain about lines too long, and TODOs are sprinkled all over or >> not even mentioned. It's a Proof of Concept that a net_device could work >> for a wide range of spi and serdev based drivers, and on top this device >> has more than one channel, which may influence network-level design >> discussions. >> >> That said, I'll happily drop the second check. Thanks for spotting! >> >>>> + if (ret) { >>>> + dev_err(&spi->dev, "SPI radio address write >> failed\n"); >>>> + return ret; >>>> + } >>>> + >>>> + ret = sx1301_write(ssx->parent, ssx->regs + >> REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0); >>>> + if (ret) { >>>> + dev_err(&spi->dev, "SPI radio data write failed\n"); >>>> + return ret; >>>> + } >>> >>> This looks awfully like you're coming in at the wrong abstraction layer >>> and the hardware actually implements a register abstraction rather than >>> a SPI one so you should be using regmap as the abstraction. >> >> I don't understand. Ben has suggested using regmap for the SPI _device_ >> that we're talking to, which may be a good idea. But this SX1301 device >> in turn has two SPI _masters_ talking to an SX125x slave each. I don't >> see how using regmap instead of my wrappers avoids this spi_controller? >> The whole point of this spi_controller is to abstract and separate the >> SX1255 vs. SX1257 vs. whatever-radio-attached into a separate driver, >> instead of mixing it into the SX1301 driver - to me that looks cleaner >> and more extensible. It also has the side-effect that we could configure >> the two radios via DT (frequencies, clk output, etc.). > > You want an SPI controller in the SX1301 as the down stream radios are SPI and could be attached directly to a host SPI bus, makes sense to have one radio driver and talk through the SX1301. > But you should use the regmap to access the SX1301 master controller registers. > Example I use with one SPI master and some clock info: > eg: > sx1301: sx1301@0 { Node names should not repeat the chipset, that goes into compatible. lora-concentrator@0? > compatible = "semtech,sx1301"; > reg = <0>; > #address-cells = <1>; > #size-cells = <0>; I would still find it cleaner to have (a) sub-node(s) for the radios. > spi-max-frequency = <8000000>; Datasheet says 10 MHz, why 8 MHz? > gpios-reset = <&pioA 26 GPIO_ACTIVE_HIGH>; reset-gpios? > clocks = <&radio1 0>, <&clkhs 0>; > clock-names = "clk32m", "clkhs"; > > radio0: sx1257@0 { lora@0? > compatible = "semtech,sx125x"; No wildcards in bindings please, use concrete "semtech,sx1257". > reg = <0>; > spi-max-frequency = <8000000>; Datasheet says 10 ns - I reported to Semtech that it should probably say 10 MHz, too. > tx; Might we configure that on the sx1301 instead? > clocks = <&tcxo 0>; > clock-names = "tcxo"; > }; > > radio1: sx1257@1 { > compatible = "semtech,sx125x"; > reg = <1>; > spi-max-frequency = <8000000>; > #clock-cells = <0>; > clocks = <&tcxo 0>; > clock-names = "tcxo"; > clock-output-names = "clk32m"; > }; > }; [snip] Regards, Andreas
On Mon, Jul 02, 2018 at 07:34:21PM +0200, Andreas Färber wrote: > Hi Mark, > > This driver is still evolving, there's newer code on my lora-next branch > already: https://github.com/afaerber/linux/commits/lora-next Please don't top post, reply in line with needed context. This allows readers to readily follow the flow of conversation and understand what you are talking about and also helps ensure that everything in the discussion is being addressed. > 2) This SPI device is in turn exposing the two SPI masters that you > already found below, and I didn't see a sane way to split that code out > into drivers/spi/, so it's in drivers/net/lora/ here - has there been > any precedence either way? A MFD? > Am 02.07.2018 um 18:12 schrieb Mark Brown: > > On Sun, Jul 01, 2018 at 01:08:04PM +0200, Andreas Färber wrote: > >> +static void sx1301_radio_spi_set_cs(struct spi_device *spi, bool enable) > >> +{ > >> + int ret; > >> + > >> + dev_dbg(&spi->dev, "setting SPI CS to %s\n", enable ? "1" : "0"); > >> + > >> + if (enable) > >> + return; > > So we never disable chip select? > Not here, I instead did that in transfer_one below. That's obviously at best going to be fragile, you're implementing half the operation here and half somewhere else which is most likely going to break at some point when the framework changes. > >> + if (ret) { > >> + dev_err(&spi->dev, "SPI radio address write failed\n"); > >> + return ret; > >> + } > >> + > >> + ret = sx1301_write(ssx->parent, ssx->regs + REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0); > >> + if (ret) { > >> + dev_err(&spi->dev, "SPI radio data write failed\n"); > >> + return ret; > >> + } > > This looks awfully like you're coming in at the wrong abstraction layer > > and the hardware actually implements a register abstraction rather than > > a SPI one so you should be using regmap as the abstraction. > I don't understand. Ben has suggested using regmap for the SPI _device_ > that we're talking to, which may be a good idea. But this SX1301 device > in turn has two SPI _masters_ talking to an SX125x slave each. I don't > see how using regmap instead of my wrappers avoids this spi_controller? It seems obvious from the code that this isn't actually interacting with a SPI controller, you're writing an address and a value to a register map rather than dealing with a byte stream. There may be a SPI bus somewhere behind some other hardware but you don't seem to be interacting with it as such. > The whole point of this spi_controller is to abstract and separate the > SX1255 vs. SX1257 vs. whatever-radio-attached into a separate driver, > instead of mixing it into the SX1301 driver - to me that looks cleaner > and more extensible. It also has the side-effect that we could configure > the two radios via DT (frequencies, clk output, etc.). A register map would work just as well here, we already have plenty of devices that abstract at this level (most obviously the I2C/SPI devices that use it to offer both interfaces with a single core driver).
Am 03.07.2018 um 16:50 schrieb Mark Brown: > On Mon, Jul 02, 2018 at 07:34:21PM +0200, Andreas Färber wrote: >> Hi Mark, >> >> This driver is still evolving, there's newer code on my lora-next branch >> already: https://github.com/afaerber/linux/commits/lora-next > > Please don't top post, reply in line with needed context. This allows > readers to readily follow the flow of conversation and understand what > you are talking about and also helps ensure that everything in the > discussion is being addressed. I did reply inline, if you cared to read on. ;) >> 2) This SPI device is in turn exposing the two SPI masters that you >> already found below, and I didn't see a sane way to split that code out >> into drivers/spi/, so it's in drivers/net/lora/ here - has there been >> any precedence either way? > > A MFD? I know of mfd, but how would the the the net vs. spi pieces interact then? Some functions would need to be exported then or is there an easier way without needing to set a cross-module API in stone? >> Am 02.07.2018 um 18:12 schrieb Mark Brown: >>> On Sun, Jul 01, 2018 at 01:08:04PM +0200, Andreas Färber wrote: >>>> +static void sx1301_radio_spi_set_cs(struct spi_device *spi, bool enable) >>>> +{ >>>> + int ret; >>>> + >>>> + dev_dbg(&spi->dev, "setting SPI CS to %s\n", enable ? "1" : "0"); >>>> + >>>> + if (enable) >>>> + return; > >>> So we never disable chip select? > >> Not here, I instead did that in transfer_one below. > > That's obviously at best going to be fragile, you're implementing half > the operation here and half somewhere else which is most likely going to > break at some point when the framework changes. > >>>> + if (ret) { >>>> + dev_err(&spi->dev, "SPI radio address write failed\n"); >>>> + return ret; >>>> + } >>>> + >>>> + ret = sx1301_write(ssx->parent, ssx->regs + REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0); >>>> + if (ret) { >>>> + dev_err(&spi->dev, "SPI radio data write failed\n"); >>>> + return ret; >>>> + } > >>> This looks awfully like you're coming in at the wrong abstraction layer >>> and the hardware actually implements a register abstraction rather than >>> a SPI one so you should be using regmap as the abstraction. > >> I don't understand. Ben has suggested using regmap for the SPI _device_ >> that we're talking to, which may be a good idea. But this SX1301 device >> in turn has two SPI _masters_ talking to an SX125x slave each. I don't >> see how using regmap instead of my wrappers avoids this spi_controller? > > It seems obvious from the code that this isn't actually interacting with > a SPI controller, you're writing an address and a value to a register > map rather than dealing with a byte stream. There may be a SPI bus > somewhere behind some other hardware but you don't seem to be > interacting with it as such. > >> The whole point of this spi_controller is to abstract and separate the >> SX1255 vs. SX1257 vs. whatever-radio-attached into a separate driver, >> instead of mixing it into the SX1301 driver - to me that looks cleaner >> and more extensible. It also has the side-effect that we could configure >> the two radios via DT (frequencies, clk output, etc.). > > A register map would work just as well here, we already have plenty of > devices that abstract at this level (most obviously the I2C/SPI devices > that use it to offer both interfaces with a single core driver). The address and data registers together form a two-byte SPI message! It is transmitted by writing to the CS register. The received data is afterwards available in another register. HTE, Andreas
On Tue, Jul 03, 2018 at 05:09:38PM +0200, Andreas Färber wrote: > Am 03.07.2018 um 16:50 schrieb Mark Brown: > >> 2) This SPI device is in turn exposing the two SPI masters that you > >> already found below, and I didn't see a sane way to split that code out > >> into drivers/spi/, so it's in drivers/net/lora/ here - has there been > >> any precedence either way? > > A MFD? > I know of mfd, but how would the the the net vs. spi pieces interact > then? Some functions would need to be exported then or is there an > easier way without needing to set a cross-module API in stone? It's an in-kernel ABI it's not exactly set in stone but yeah, you'll need some interface. A lot of devices work by having the children know that they're part of a MFD and fish things out of the parent device, either the pdata or (in the common case where the MFD bit mostly just instantiates subdevices and holds a regmap) with dev_get_regmap(). > > A register map would work just as well here, we already have plenty of > > devices that abstract at this level (most obviously the I2C/SPI devices > > that use it to offer both interfaces with a single core driver). > The address and data registers together form a two-byte SPI message! > It is transmitted by writing to the CS register. > The received data is afterwards available in another register. Right, but it seems from the code that the hardware understands that it's formatting register I/O and not just shifting in and out a byte stream which is what a SPI controller does. I'd not be surprised to learn that the register you're calling a chip select register is a strobe that initiates the transfer (and that this may be some of the difficulty you're having with handling it in the way the framework expects), the pattern with writing 1 followed immediately by 0 is a bit of a flag here. I've seen such before hardware where I know it was intentionally designed that way so it wouldn't be totally surprising.
Am 03.07.2018 um 17:31 schrieb Mark Brown: > On Tue, Jul 03, 2018 at 05:09:38PM +0200, Andreas Färber wrote: >> Am 03.07.2018 um 16:50 schrieb Mark Brown: >>> A register map would work just as well here, we already have plenty of >>> devices that abstract at this level (most obviously the I2C/SPI devices >>> that use it to offer both interfaces with a single core driver). > >> The address and data registers together form a two-byte SPI message! > >> It is transmitted by writing to the CS register. > >> The received data is afterwards available in another register. > > Right, but it seems from the code that the hardware understands that > it's formatting register I/O and not just shifting in and out a byte > stream which is what a SPI controller does. I'd not be surprised to > learn that the register you're calling a chip select register is a > strobe that initiates the transfer (and that this may be some of the > difficulty you're having with handling it in the way the framework > expects), the pattern with writing 1 followed immediately by 0 is a bit > of a flag here. Yeah, the current implementation assumes exactly that. :) > I've seen such before hardware where I know it was intentionally > designed that way so it wouldn't be totally surprising. If we don't implement a spi_controller here, then IIUC we can't have multiple spi_device implementations for the devices on the receiving end, as they rely on a spi_controller for their APIs. Do you have an alternative solution for abstraction? A regmap would seem to require putting everything into a monolithic SX1301 driver despite those connected chipsets actually being regular, external SPI chips that could also be attached to non-SX1301 SPI masters. Regards, Andreas
On Tue, Jul 03, 2018 at 06:40:41PM +0200, Andreas Färber wrote: > Do you have an alternative solution for abstraction? A regmap would seem > to require putting everything into a monolithic SX1301 driver despite > those connected chipsets actually being regular, external SPI chips that > could also be attached to non-SX1301 SPI masters. I'm suggesting a regmap for those external SPI chips. It doesn't matter what the host uses.
> Subject: Re: [RFC net-next 15/15] net: lora: Add Semtech SX1301 > > On Tue, Jul 03, 2018 at 06:40:41PM +0200, Andreas Färber wrote: > > > Do you have an alternative solution for abstraction? A regmap would seem > > to require putting everything into a monolithic SX1301 driver despite > > those connected chipsets actually being regular, external SPI chips that > > could also be attached to non-SX1301 SPI masters. > > I'm suggesting a regmap for those external SPI chips. It doesn't matter > what the host uses. Currently in my testing drivers I have used regmap for both the SX1301 and the downstream SX1257. In my SX1257 driver currently the regmap is backed via SPI with devm_regmap_init_spi, please correct me if I am wrong but if I understand correctly this could be split out to regmap backed by SPI in the case of a direct host connection, and a regmap backed by the SX1301's regmapped strobing register interface, regmap_bus? Is there a precedent to this I can examine?
On Wed, Jul 04, 2018 at 01:41:42PM +0000, Ben Whitten wrote: Please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to. > In my SX1257 driver currently the regmap is backed via SPI with > devm_regmap_init_spi, please correct me if I am wrong but if I > understand correctly this could be split out to regmap backed by SPI > in the case of a direct host connection, and a regmap backed by the > SX1301's regmapped strobing register interface, regmap_bus? Yes. > Is there a precedent to this I can examine? There's lots of devices that provide both SPI and I2C, you'd just be using a regmap_bus instead of the I2C one. I can't recall an example of the specific combination you're looking for though.
> Subject: Re: [RFC net-next 15/15] net: lora: Add Semtech SX1301 > > Hi Ben, > > Am 02.07.2018 um 22:43 schrieb Ben Whitten: > >> 2) This SPI device is in turn exposing the two SPI masters that you > >> already found below, and I didn't see a sane way to split that code out > >> into drivers/spi/, so it's in drivers/net/lora/ here - has there been > >> any precedence either way? > > > > In my work in progress driver I just register one controller for the sx1301 > with two chip selects and use the chip select information to choose the > correct radio to send to, this is based on the DT reg information. No need to > register two separate masters. > > I had considered that and discarded it. The SX1301 has not just two CS > registers though but also two pairs of addr, data registers. That speaks > for two masters with a single chip-select each, unless I'm > misunderstanding the meaning of the registers. Based on Marks suggestion I am experimenting with using the SX1301 to expose a regmap_bus that the underlying SX1257 can attach to, so the radio has a core component, a SPI component for direct connection and this concentrator connection component. > >> Am 02.07.2018 um 18:12 schrieb Mark Brown: > >>> On Sun, Jul 01, 2018 at 01:08:04PM +0200, Andreas Färber wrote: > >>> > >>>> +static void sx1301_radio_spi_set_cs(struct spi_device *spi, bool > enable) > >>>> +{ > >>>> + int ret; > >>>> + > >>>> + dev_dbg(&spi->dev, "setting SPI CS to %s\n", enable ? "1" : "0"); > >>>> + > >>>> + if (enable) > >>>> + return; > >>>> + > >>>> + ret = sx1301_radio_set_cs(spi->controller, enable); > >>>> + if (ret) > >>>> + dev_warn(&spi->dev, "failed to write CS (%d)\n", ret); > >>>> +} > >>> > >>> So we never disable chip select? > >> > >> Not here, I instead did that in transfer_one below. > >> > >> Unfortunately there seems to be no documentation, only reference > code: > >> > >> https://github.com/Lora- > >> net/lora_gateway/blob/master/libloragw/src/loragw_radio.c#L121 > >> https://github.com/Lora- > >> net/lora_gateway/blob/master/libloragw/src/loragw_radio.c#L165 > >> > >> It sets CS to 0 before writing to address and data registers, then > >> immediately sets CS to 1 and back to 0 before reading or ending the > >> write transaction. I've tried to force the same behavior in this driver. > >> My guess was that CS is high-active during the short 1-0 cycle, because > >> if it's low-active during the register writes then why the heck is it > >> set to 0 again in the end instead of keeping at 1... confusing. > >> > >> Maybe the Semtech folks CC'ed can comment how these registers work? > >> > >>>> + if (tx_buf) { > >>>> + ret = sx1301_write(ssx->parent, ssx->regs + > >> REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0); > >>> > >>> This looks confused. We're in an if (tx_buf) block but there's a use of > >>> the ternery operator that appears to be checking if we have a tx_buf? > >> > >> Yeah, as mentioned this RFC is not ready for merging - checkpatch.pl > >> will complain about lines too long, and TODOs are sprinkled all over or > >> not even mentioned. It's a Proof of Concept that a net_device could work > >> for a wide range of spi and serdev based drivers, and on top this device > >> has more than one channel, which may influence network-level design > >> discussions. > >> > >> That said, I'll happily drop the second check. Thanks for spotting! > >> > >>>> + if (ret) { > >>>> + dev_err(&spi->dev, "SPI radio address write > >> failed\n"); > >>>> + return ret; > >>>> + } > >>>> + > >>>> + ret = sx1301_write(ssx->parent, ssx->regs + > >> REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0); > >>>> + if (ret) { > >>>> + dev_err(&spi->dev, "SPI radio data write failed\n"); > >>>> + return ret; > >>>> + } > >>> > >>> This looks awfully like you're coming in at the wrong abstraction layer > >>> and the hardware actually implements a register abstraction rather than > >>> a SPI one so you should be using regmap as the abstraction. > >> > >> I don't understand. Ben has suggested using regmap for the SPI _device_ > >> that we're talking to, which may be a good idea. But this SX1301 device > >> in turn has two SPI _masters_ talking to an SX125x slave each. I don't > >> see how using regmap instead of my wrappers avoids this spi_controller? > >> The whole point of this spi_controller is to abstract and separate the > >> SX1255 vs. SX1257 vs. whatever-radio-attached into a separate driver, > >> instead of mixing it into the SX1301 driver - to me that looks cleaner > >> and more extensible. It also has the side-effect that we could configure > >> the two radios via DT (frequencies, clk output, etc.). > > > > You want an SPI controller in the SX1301 as the down stream radios are SPI > and could be attached directly to a host SPI bus, makes sense to have one > radio driver and talk through the SX1301. > > But you should use the regmap to access the SX1301 master controller > registers. > > Example I use with one SPI master and some clock info: > > eg: > > sx1301: sx1301@0 { > > Node names should not repeat the chipset, that goes into compatible. > > lora-concentrator@0? > Sure > > compatible = "semtech,sx1301"; > > reg = <0>; > > #address-cells = <1>; > > #size-cells = <0>; > > I would still find it cleaner to have (a) sub-node(s) for the radios. How do you mean? > > spi-max-frequency = <8000000>; > > Datasheet says 10 MHz, why 8 MHz? > > > gpios-reset = <&pioA 26 GPIO_ACTIVE_HIGH>; > > reset-gpios? Agreed, this seems more common. > > clocks = <&radio1 0>, <&clkhs 0>; > > clock-names = "clk32m", "clkhs"; > > > > radio0: sx1257@0 { > > lora@0? > > > compatible = "semtech,sx125x"; > > No wildcards in bindings please, use concrete "semtech,sx1257". Sure > > reg = <0>; > > spi-max-frequency = <8000000>; > > Datasheet says 10 ns - I reported to Semtech that it should probably say > 10 MHz, too. > > > tx; > > Might we configure that on the sx1301 instead? Well the ability for a radio to TX is a radio property really. It depends on the board which chain has the PAs on. I don’t think that its appropriate to configure this at the concentrator, it can instead discover this from the radios. > > clocks = <&tcxo 0>; > > clock-names = "tcxo"; > > }; > > > > radio1: sx1257@1 { > > compatible = "semtech,sx125x"; > > reg = <1>; > > spi-max-frequency = <8000000>; > > #clock-cells = <0>; > > clocks = <&tcxo 0>; > > clock-names = "tcxo"; > > clock-output-names = "clk32m"; > > }; > > }; > [snip] > > Regards, > Andreas > > -- > SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Felix Imendörffer, Jane Smithard, Graham Norton > HRB 21284 (AG Nürnberg)
> Subject: Re: [RFC net-next 15/15] net: lora: Add Semtech SX1301 > > Hi Ben, > > Am 02.07.2018 um 13:51 schrieb Ben Whitten: > > Excellent work on doing this I have also been working on and off > > this personally for some time. > > Thanks. Colliding work is always unfortunate, I can relate... > > > Have a look at my repository [1] for sx1301 and sx1257 drivers, > > I use regmaps capability of switching pages which should simplify > > your driver considerably, I also have a full register map and bit field. > > Please note that my lora-next branch already has bug fixes and cleanups > over this patch. The probe error handling was broken, and I implemented > wrappers for paged reads and writes as well as burst modes, plus the > firmware loading. > > https://github.com/afaerber/linux/commits/lora-next > > I took a quick look at your sx1257 and noticed you licensed it as GPLv2. > Is there any particular reason for that? Since I wrote my driver without > copying from GPLv2 code, I prefer the less restrictive GPLv2+. As I was learning about regmap usage and how SPI devices connect to the bus I adopted the most common licence employed in these kernel files, seemed appropriate. Ultimately I don’t want a licence choice to hamper inclusion to mainline, not a lawyer I don’t find that fun. > > So far a work day has passed with no maintainer objecting to or > commenting on the underlying PF_LORA network layer design. Meanwhile > there's already three of us with code and more people have inquired > about testing and contributing, so I'm thinking about setting up a > staging tree on kernel.org to collaborate on... > > Would you be willing to contribute your regmap ideas to my driver as a > patch to squash? Needs a Signed-off-by of course, which your GitHub > commits are lacking, so I can't merge them on my own. I'm sure whichever way we merge it, yours to mine / mine to yours, we can work together on it. As I already have regmap running and mostly split out SX1257 drivers to Marks suggestions avoiding the extra SPI layer it may by easier to port the various functions you have to mine and share authorship. > > I have also been trying to use the clk framework to capture the various > > routing that the cards have. > > I thought about clk too, but won't that cause name conflicts when > probing multiple concentrators? Would be nice to use that for > configuring the SX1257 clock output instead of my current hack. Potentially, it maybe that we need to augment the names of the clk with the CS of the concentrator in use. Or maybe the radio only searches the clk's of its parent device... I've not explored this area to far yet but there will be a solution I'm sure. > Another thought I haven't investigated yet is whether we could use > remoteproc for ARB and AGC. I would at least prefer to have the firmware > as a binary loaded via the usual request_firmware(), not as byte array. > But then again the AGC gets firmware loaded twice, so maybe too complex > for remoteproc. > > BTW do you have any insights on what MCU is in there? Would be nice to > understand in form of source code what the firmware is doing, to avoid > the hard dependency on a specific firmware version (imagine user > updating kernel-firmware - containing versions X,Y,Z - and kernel and > booting two different kernel versions, the older one stops working). I'm afraid no insight into the internals of this chip, some good guesswork but I think a firmware blob is something we are going to have to live with for the time being. > https://www.thethingsnetwork.org/forum/t/secret-price-of-a-lora- > gateway/5730/74 > > Regards, > Andreas > > > I will dig into this series this evening. > > > > [1] https://github.com/BWhitten/linux- > stable/tree/971aadc8fdfe842020d912449bdd71b33d576fe3/drivers/net/lora > [...] > >> diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c > >> new file mode 100644 > >> index 000000000000..5c936c1116d1 > >> --- /dev/null > >> +++ b/drivers/net/lora/sx1301.c > >> @@ -0,0 +1,446 @@ > >> +// SPDX-License-Identifier: GPL-2.0-or-later > [snip] Ben
diff --git a/drivers/net/lora/Kconfig b/drivers/net/lora/Kconfig index 68c7480d7812..950450e353b4 100644 --- a/drivers/net/lora/Kconfig +++ b/drivers/net/lora/Kconfig @@ -45,6 +45,13 @@ config LORA_SX1276 help Semtech SX1272/1276/1278 +config LORA_SX1301 + tristate "Semtech SX1301 SPI driver" + default y + depends on SPI + help + Semtech SX1301 + config LORA_USI tristate "USI WM-SG-SM-42 driver" default y diff --git a/drivers/net/lora/Makefile b/drivers/net/lora/Makefile index 44c578bde7d5..1cc1e3aa189b 100644 --- a/drivers/net/lora/Makefile +++ b/drivers/net/lora/Makefile @@ -22,6 +22,9 @@ lora-sx1257-y := sx1257.o obj-$(CONFIG_LORA_SX1276) += lora-sx1276.o lora-sx1276-y := sx1276.o +obj-$(CONFIG_LORA_SX1301) += lora-sx1301.o +lora-sx1301-y := sx1301.o + obj-$(CONFIG_LORA_USI) += lora-usi.o lora-usi-y := usi.o diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c new file mode 100644 index 000000000000..5c936c1116d1 --- /dev/null +++ b/drivers/net/lora/sx1301.c @@ -0,0 +1,446 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Semtech SX1301 LoRa concentrator + * + * Copyright (c) 2018 Andreas Färber + * + * Based on SX1301 HAL code: + * Copyright (c) 2013 Semtech-Cycleo + */ + +#include <linux/bitops.h> +#include <linux/delay.h> +#include <linux/lora.h> +#include <linux/module.h> +#include <linux/netdevice.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_gpio.h> +#include <linux/lora/dev.h> +#include <linux/spi/spi.h> + +#define REG_PAGE_RESET 0 +#define REG_VERSION 1 +#define REG_2_SPI_RADIO_A_DATA 33 +#define REG_2_SPI_RADIO_A_DATA_READBACK 34 +#define REG_2_SPI_RADIO_A_ADDR 35 +#define REG_2_SPI_RADIO_A_CS 37 +#define REG_2_SPI_RADIO_B_DATA 38 +#define REG_2_SPI_RADIO_B_DATA_READBACK 39 +#define REG_2_SPI_RADIO_B_ADDR 40 +#define REG_2_SPI_RADIO_B_CS 42 + +#define REG_PAGE_RESET_SOFT_RESET BIT(7) + +#define REG_16_GLOBAL_EN BIT(3) + +#define REG_17_CLK32M_EN BIT(0) + +#define REG_2_43_RADIO_A_EN BIT(0) +#define REG_2_43_RADIO_B_EN BIT(1) +#define REG_2_43_RADIO_RST BIT(2) + +struct spi_sx1301 { + struct spi_device *parent; + u8 page; + u8 regs; +}; + +struct sx1301_priv { + struct lora_priv lora; + struct gpio_desc *rst_gpio; + u8 cur_page; + struct spi_controller *radio_a_ctrl, *radio_b_ctrl; +}; + +static int sx1301_read(struct spi_device *spi, u8 reg, u8 *val) +{ + u8 addr = reg & 0x7f; + return spi_write_then_read(spi, &addr, 1, val, 1); +} + +static int sx1301_write(struct spi_device *spi, u8 reg, u8 val) +{ + u8 buf[2]; + + buf[0] = reg | BIT(7); + buf[1] = val; + return spi_write(spi, buf, 2); +} + +static int sx1301_page_switch(struct spi_device *spi, u8 page) +{ + struct sx1301_priv *priv = spi_get_drvdata(spi); + int ret; + + if (priv->cur_page == page) + return 0; + + dev_dbg(&spi->dev, "switching to page %u\n", (unsigned)page); + ret = sx1301_write(spi, REG_PAGE_RESET, page & 0x3); + if (ret) { + dev_err(&spi->dev, "switching to page %u failed\n", (unsigned)page); + return ret; + } + + priv->cur_page = page; + + return 0; +} + +static int sx1301_soft_reset(struct spi_device *spi) +{ + return sx1301_write(spi, REG_PAGE_RESET, REG_PAGE_RESET_SOFT_RESET); +} + +#define REG_RADIO_X_DATA 0 +#define REG_RADIO_X_DATA_READBACK 1 +#define REG_RADIO_X_ADDR 2 +#define REG_RADIO_X_CS 4 + +static int sx1301_radio_set_cs(struct spi_controller *ctrl, bool enable) +{ + struct spi_sx1301 *ssx = spi_controller_get_devdata(ctrl); + u8 cs; + int ret; + + dev_dbg(&ctrl->dev, "setting CS to %s\n", enable ? "1" : "0"); + + ret = sx1301_page_switch(ssx->parent, ssx->page); + if (ret) { + dev_warn(&ctrl->dev, "failed to switch page for CS (%d)\n", ret); + return ret; + } + + ret = sx1301_read(ssx->parent, ssx->regs + REG_RADIO_X_CS, &cs); + if (ret) { + dev_warn(&ctrl->dev, "failed to read CS (%d)\n", ret); + cs = 0; + } + + if (enable) + cs |= BIT(0); + else + cs &= ~BIT(0); + + ret = sx1301_write(ssx->parent, ssx->regs + REG_RADIO_X_CS, cs); + if (ret) + dev_warn(&ctrl->dev, "failed to write CS (%d)\n", ret); + + return 0; +} + +static void sx1301_radio_spi_set_cs(struct spi_device *spi, bool enable) +{ + int ret; + + dev_dbg(&spi->dev, "setting SPI CS to %s\n", enable ? "1" : "0"); + + if (enable) + return; + + ret = sx1301_radio_set_cs(spi->controller, enable); + if (ret) + dev_warn(&spi->dev, "failed to write CS (%d)\n", ret); +} + +static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl, + struct spi_device *spi, struct spi_transfer *xfr) +{ + struct spi_sx1301 *ssx = spi_controller_get_devdata(ctrl); + const u8 *tx_buf = xfr->tx_buf; + u8 *rx_buf = xfr->rx_buf; + int ret; + + if (xfr->len == 0 || xfr->len > 3) + return -EINVAL; + + dev_dbg(&spi->dev, "transferring one (%u)\n", xfr->len); + + ret = sx1301_page_switch(ssx->parent, ssx->page); + if (ret) { + dev_err(&spi->dev, "failed to switch page for transfer (%d)\n", ret); + return ret; + } + + if (tx_buf) { + ret = sx1301_write(ssx->parent, ssx->regs + REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0); + if (ret) { + dev_err(&spi->dev, "SPI radio address write failed\n"); + return ret; + } + + ret = sx1301_write(ssx->parent, ssx->regs + REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0); + if (ret) { + dev_err(&spi->dev, "SPI radio data write failed\n"); + return ret; + } + + ret = sx1301_radio_set_cs(ctrl, true); + if (ret) { + dev_err(&spi->dev, "SPI radio CS set failed\n"); + return ret; + } + + ret = sx1301_radio_set_cs(ctrl, false); + if (ret) { + dev_err(&spi->dev, "SPI radio CS unset failed\n"); + return ret; + } + } + + if (rx_buf) { + ret = sx1301_read(ssx->parent, ssx->regs + REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]); + if (ret) { + dev_err(&spi->dev, "SPI radio data read failed\n"); + return ret; + } + } + + return 0; +} + +static void sx1301_radio_setup(struct spi_controller *ctrl) +{ + ctrl->mode_bits = SPI_CS_HIGH | SPI_NO_CS; + ctrl->bits_per_word_mask = SPI_BPW_MASK(8); + ctrl->num_chipselect = 1; + ctrl->set_cs = sx1301_radio_spi_set_cs; + ctrl->transfer_one = sx1301_radio_spi_transfer_one; +} + +static int sx1301_probe(struct spi_device *spi) +{ + struct net_device *netdev; + struct sx1301_priv *priv; + struct spi_sx1301 *radio; + struct gpio_desc *rst; + int ret; + u8 val; + + rst = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW); + if (IS_ERR(rst)) + 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); + + ret = sx1301_read(spi, REG_VERSION, &val); + if (ret) { + dev_err(&spi->dev, "version read failed\n"); + goto err_version; + } + + if (val != 103) { + dev_err(&spi->dev, "unexpected version: %u\n", val); + ret = -ENXIO; + goto err_version; + } + + netdev = alloc_loradev(sizeof(*priv)); + if (!netdev) { + ret = -ENOMEM; + goto err_alloc_loradev; + } + + priv = netdev_priv(netdev); + priv->rst_gpio = rst; + priv->cur_page = 0xff; + + spi_set_drvdata(spi, netdev); + SET_NETDEV_DEV(netdev, &spi->dev); + + ret = sx1301_write(spi, REG_PAGE_RESET, 0); + if (ret) { + dev_err(&spi->dev, "page/reset write failed\n"); + return ret; + } + + ret = sx1301_soft_reset(spi); + if (ret) { + dev_err(&spi->dev, "soft reset failed\n"); + return ret; + } + + ret = sx1301_read(spi, 16, &val); + if (ret) { + dev_err(&spi->dev, "16 read failed\n"); + return ret; + } + + val &= ~REG_16_GLOBAL_EN; + + ret = sx1301_write(spi, 16, val); + if (ret) { + dev_err(&spi->dev, "16 write failed\n"); + return ret; + } + + ret = sx1301_read(spi, 17, &val); + if (ret) { + dev_err(&spi->dev, "17 read failed\n"); + return ret; + } + + val &= ~REG_17_CLK32M_EN; + + ret = sx1301_write(spi, 17, val); + if (ret) { + dev_err(&spi->dev, "17 write failed\n"); + return ret; + } + + ret = sx1301_page_switch(spi, 2); + if (ret) { + dev_err(&spi->dev, "page 2 switch failed\n"); + return ret; + } + + ret = sx1301_read(spi, 43, &val); + if (ret) { + dev_err(&spi->dev, "2|43 read failed\n"); + return ret; + } + + val |= REG_2_43_RADIO_B_EN | REG_2_43_RADIO_A_EN; + + ret = sx1301_write(spi, 43, val); + if (ret) { + dev_err(&spi->dev, "2|43 write failed\n"); + return ret; + } + + msleep(500); + + ret = sx1301_read(spi, 43, &val); + if (ret) { + dev_err(&spi->dev, "2|43 read failed\n"); + return ret; + } + + val |= REG_2_43_RADIO_RST; + + ret = sx1301_write(spi, 43, val); + if (ret) { + dev_err(&spi->dev, "2|43 write failed\n"); + return ret; + } + + msleep(5); + + ret = sx1301_read(spi, 43, &val); + if (ret) { + dev_err(&spi->dev, "2|43 read failed\n"); + return ret; + } + + val &= ~REG_2_43_RADIO_RST; + + ret = sx1301_write(spi, 43, val); + if (ret) { + dev_err(&spi->dev, "2|43 write failed\n"); + return ret; + } + + /* radio A */ + + priv->radio_a_ctrl = spi_alloc_master(&spi->dev, sizeof(*radio)); + if (!priv->radio_a_ctrl) { + ret = -ENOMEM; + goto err_radio_a_alloc; + } + + sx1301_radio_setup(priv->radio_a_ctrl); + priv->radio_a_ctrl->dev.of_node = of_get_child_by_name(spi->dev.of_node, "radio-a"); + + radio = spi_controller_get_devdata(priv->radio_a_ctrl); + radio->page = 2; + radio->regs = REG_2_SPI_RADIO_A_DATA; + radio->parent = spi; + + dev_info(&spi->dev, "registering radio A SPI\n"); + + ret = devm_spi_register_controller(&spi->dev, priv->radio_a_ctrl); + if (ret) { + dev_err(&spi->dev, "radio A SPI register failed\n"); + goto err_radio_a_register; + } + + /* radio B */ + + priv->radio_b_ctrl = spi_alloc_master(&spi->dev, sizeof(*radio)); + if (!priv->radio_b_ctrl) { + ret = -ENOMEM; + goto err_radio_b_alloc; + } + + sx1301_radio_setup(priv->radio_b_ctrl); + priv->radio_b_ctrl->dev.of_node = of_get_child_by_name(spi->dev.of_node, "radio-b"); + + radio = spi_controller_get_devdata(priv->radio_b_ctrl); + radio->page = 2; + radio->regs = REG_2_SPI_RADIO_B_DATA; + radio->parent = spi; + + dev_info(&spi->dev, "registering radio B SPI\n"); + + ret = devm_spi_register_controller(&spi->dev, priv->radio_b_ctrl); + if (ret) { + dev_err(&spi->dev, "radio B SPI register failed\n"); + goto err_radio_b_register; + } + + dev_info(&spi->dev, "SX1301 module probed\n"); + + return 0; +err_radio_b_register: + spi_controller_put(priv->radio_b_ctrl); +err_radio_b_alloc: +err_radio_a_register: + spi_controller_put(priv->radio_a_ctrl); +err_radio_a_alloc: + free_loradev(netdev); +err_alloc_loradev: +err_version: + return ret; +} + +static int sx1301_remove(struct spi_device *spi) +{ + struct net_device *netdev = spi_get_drvdata(spi); + + //unregister_loradev(netdev); + free_loradev(netdev); + + dev_info(&spi->dev, "SX1301 module removed\n"); + + return 0; +} + +#ifdef CONFIG_OF +static const struct of_device_id sx1301_dt_ids[] = { + { .compatible = "semtech,sx1301" }, + {} +}; +MODULE_DEVICE_TABLE(of, sx1301_dt_ids); +#endif + +static struct spi_driver sx1301_spi_driver = { + .driver = { + .name = "sx1301", + .of_match_table = of_match_ptr(sx1301_dt_ids), + }, + .probe = sx1301_probe, + .remove = sx1301_remove, +}; + +module_spi_driver(sx1301_spi_driver); + +MODULE_DESCRIPTION("SX1301 SPI driver"); +MODULE_AUTHOR("Andreas Färber <afaerber@suse.de>"); +MODULE_LICENSE("GPL");
The Semtech SX1301 was the first multi-channel LoRa "concentrator". It uses a SPI interface to the host as well as a dual SPI interface to its radios. These two have been implemented as spi_controller, so that the Device Tree can specify whether the respective module uses two SX1257, two SX1255 or a combination of these or some unforeseen chipset. This implementation is the most recent - initialization is not yet complete, it will need to load firmware into the two on-chip MCUs. Unfortunately there is no full datasheet with register descriptions, only a BSD-licensed userspace HAL implementation using spidev devices. Therefore some register names are unknown. Cc: Ben Whitten <ben.whitten@lairdtech.com> Cc: Steve deRosier <derosier@gmail.com> Cc: Mark Brown <broonie@kernel.org> Cc: Michael Röder <michael.roeder@avnet.eu> Cc: Ken Yu (禹凯) <ken.yu@rakwireless.com> Cc: linux-spi@vger.kernel.org Signed-off-by: Andreas Färber <afaerber@suse.de> --- drivers/net/lora/Kconfig | 7 + drivers/net/lora/Makefile | 3 + drivers/net/lora/sx1301.c | 446 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 456 insertions(+) create mode 100644 drivers/net/lora/sx1301.c