[PATCH RFC lora-next 1/4] net: lora: sx125x sx1301: correct style warnings

[PATCH RFC lora-next 1/4] net: lora: sx125x sx1301: correct style warnings

[Ben Whitten]

Checkpatch highlights some style issues which need to be addressed.

Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
---
 drivers/net/lora/sx125x.c | 20 +++++++++------
 drivers/net/lora/sx1301.c | 52 ++++++++++++++++++++++-----------------
 drivers/net/lora/sx1301.h |  7 +++---
 3 files changed, 45 insertions(+), 34 deletions(-)

diff --git a/drivers/net/lora/sx125x.c b/drivers/net/lora/sx125x.c
index b7ca782b9386..1a941f663c52 100644
--- a/drivers/net/lora/sx125x.c
+++ b/drivers/net/lora/sx125x.c
@@ -49,7 +49,7 @@ struct sx125x_priv {
 
 	struct device		*dev;
 	struct regmap		*regmap;
-	struct regmap_field     *regmap_fields[ARRAY_SIZE(sx125x_regmap_fields)];
+	struct regmap_field *regmap_fields[ARRAY_SIZE(sx125x_regmap_fields)];
 };
 
 #define to_clkout(_hw) container_of(_hw, struct sx125x_priv, clkout_hw)
@@ -67,13 +67,13 @@ static struct regmap_config __maybe_unused sx125x_regmap_config = {
 };
 
 static int sx125x_field_write(struct sx125x_priv *priv,
-		enum sx125x_fields field_id, u8 val)
+			      enum sx125x_fields field_id, u8 val)
 {
 	return regmap_field_write(priv->regmap_fields[field_id], val);
 }
 
 static int sx125x_field_read(struct sx125x_priv *priv,
-		enum sx125x_fields field_id, unsigned int *val)
+			     enum sx125x_fields field_id, unsigned int *val)
 {
 	return regmap_field_read(priv->regmap_fields[field_id], val);
 }
@@ -149,8 +149,12 @@ static int sx125x_register_clock_provider(struct sx125x_priv *priv)
 	init.num_parents = 1;
 	priv->clkout_hw.init = &init;
 
-	of_property_read_string_index(dev->of_node, "clock-output-names", 0,
-			&init.name);
+	ret = of_property_read_string_index(dev->of_node, "clock-output-names",
+					    0, &init.name);
+	if (ret) {
+		dev_err(dev, "unable to find output name\n");
+		return ret;
+	}
 
 	priv->clkout = devm_clk_register(dev, &priv->clkout_hw);
 	if (IS_ERR(priv->clkout)) {
@@ -158,7 +162,7 @@ static int sx125x_register_clock_provider(struct sx125x_priv *priv)
 		return PTR_ERR(priv->clkout);
 	}
 	ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get,
-			&priv->clkout_hw);
+				     &priv->clkout_hw);
 	return ret;
 }
 
@@ -180,8 +184,8 @@ static int __maybe_unused sx125x_regmap_probe(struct device *dev, struct regmap
 		const struct reg_field *reg_fields = sx125x_regmap_fields;
 
 		priv->regmap_fields[i] = devm_regmap_field_alloc(dev,
-				priv->regmap,
-				reg_fields[i]);
+								 priv->regmap,
+								 reg_fields[i]);
 		if (IS_ERR(priv->regmap_fields[i])) {
 			ret = PTR_ERR(priv->regmap_fields[i]);
 			dev_err(dev, "Cannot allocate regmap field: %d\n", ret);
diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index 23cbddc364e5..e75df93b96d8 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -1,6 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * Semtech SX1301 LoRa concentrator
+/* Semtech SX1301 LoRa concentrator
  *
  * Copyright (c) 2018 Andreas Färber
  * Copyright (c) 2018 Ben Whitten
@@ -55,12 +54,13 @@ static struct regmap_config sx1301_regmap_config = {
 };
 
 static int sx1301_field_write(struct sx1301_priv *priv,
-		enum sx1301_fields field_id, u8 val)
+			      enum sx1301_fields field_id, u8 val)
 {
 	return regmap_field_write(priv->regmap_fields[field_id], val);
 }
 
-static int sx1301_agc_ram_read(struct sx1301_priv *priv, u8 addr, unsigned int *val)
+static int sx1301_agc_ram_read(struct sx1301_priv *priv, u8 addr,
+			       unsigned int *val)
 {
 	int ret;
 
@@ -79,7 +79,8 @@ static int sx1301_agc_ram_read(struct sx1301_priv *priv, u8 addr, unsigned int *
 	return 0;
 }
 
-static int sx1301_arb_ram_read(struct sx1301_priv *priv, u8 addr, unsigned int *val)
+static int sx1301_arb_ram_read(struct sx1301_priv *priv, u8 addr,
+			       unsigned int *val)
 {
 	int ret;
 
@@ -98,7 +99,8 @@ static int sx1301_arb_ram_read(struct sx1301_priv *priv, u8 addr, unsigned int *
 	return 0;
 }
 
-static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct firmware *fw)
+static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu,
+				const struct firmware *fw)
 {
 	u8 *buf;
 	enum sx1301_fields rst, select_mux;
@@ -165,7 +167,8 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct
 	}
 
 	if (memcmp(fw->data, buf, fw->size)) {
-		dev_err(priv->dev, "MCU prom data read does not match data written\n");
+		dev_err(priv->dev,
+			"MCU prom data read does not match data written\n");
 		kfree(buf);
 		return -ENXIO;
 	}
@@ -228,11 +231,12 @@ static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 		return ret;
 	}
 
-	dev_info(priv->dev, "AGC calibration firmware version %u\n", (unsigned)val);
+	dev_info(priv->dev, "AGC calibration firmware version %u\n", val);
 
 	if (val != SX1301_MCU_AGC_CAL_FW_VERSION) {
-		dev_err(priv->dev, "unexpected firmware version, expecting %u\n",
-				SX1301_MCU_AGC_CAL_FW_VERSION);
+		dev_err(priv->dev,
+			"unexpected firmware version, expecting %u\n",
+			SX1301_MCU_AGC_CAL_FW_VERSION);
 		return -ENXIO;
 	}
 
@@ -257,7 +261,7 @@ static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 		return ret;
 	}
 
-	dev_info(priv->dev, "AGC status: %02x\n", (unsigned)val);
+	dev_info(priv->dev, "AGC status: %02x\n", val);
 	if ((val & (BIT(7) | BIT(0))) != (BIT(7) | BIT(0))) {
 		dev_err(priv->dev, "AGC calibration failed\n");
 		return -ENXIO;
@@ -328,11 +332,12 @@ static int sx1301_load_all_firmware(struct sx1301_priv *priv)
 		return ret;
 	}
 
-	dev_info(priv->dev, "AGC firmware version %u\n", (unsigned)val);
+	dev_info(priv->dev, "AGC firmware version %u\n", val);
 
 	if (val != SX1301_MCU_AGC_FW_VERSION) {
-		dev_err(priv->dev, "unexpected firmware version, expecting %u\n",
-				SX1301_MCU_AGC_FW_VERSION);
+		dev_err(priv->dev,
+			"unexpected firmware version, expecting %u\n",
+			SX1301_MCU_AGC_FW_VERSION);
 		return -ENXIO;
 	}
 
@@ -342,18 +347,20 @@ static int sx1301_load_all_firmware(struct sx1301_priv *priv)
 		return ret;
 	}
 
-	dev_info(priv->dev, "ARB firmware version %u\n", (unsigned)val);
+	dev_info(priv->dev, "ARB firmware version %u\n", val);
 
 	if (val != SX1301_MCU_ARB_FW_VERSION) {
-		dev_err(priv->dev, "unexpected firmware version, expecting %u\n",
-				SX1301_MCU_ARB_FW_VERSION);
+		dev_err(priv->dev,
+			"unexpected firmware version, expecting %u\n",
+			SX1301_MCU_ARB_FW_VERSION);
 		return -ENXIO;
 	}
 
 	return 0;
 }
 
-static netdev_tx_t sx130x_loradev_start_xmit(struct sk_buff *skb, struct net_device *netdev)
+static netdev_tx_t sx130x_loradev_start_xmit(struct sk_buff *skb,
+					     struct net_device *netdev)
 {
 	if (skb->protocol != htons(ETH_P_LORA)) {
 		kfree_skb(skb);
@@ -489,11 +496,12 @@ static int sx1301_probe(struct spi_device *spi)
 		const struct reg_field *reg_fields = sx1301_regmap_fields;
 
 		priv->regmap_fields[i] = devm_regmap_field_alloc(&spi->dev,
-				priv->regmap,
-				reg_fields[i]);
+								 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(&spi->dev,
+				"Cannot allocate regmap field: %d\n", ret);
 			return ret;
 		}
 	}
@@ -553,7 +561,7 @@ static int sx1301_probe(struct spi_device *spi)
 		return ret;
 	}
 
-	msleep(5);
+	usleep_range(5000, 6000);
 
 	ret = sx1301_field_write(priv, F_RADIO_RST, 0);
 	if (ret) {
diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
index a1a2e388207e..dd2b7da94fcc 100644
--- a/drivers/net/lora/sx1301.h
+++ b/drivers/net/lora/sx1301.h
@@ -1,6 +1,5 @@
 /* SPDX-License-Identifier: GPL-2.0-or-later */
-/*
- * Semtech SX1301 LoRa concentrator
+/* Semtech SX1301 LoRa concentrator
  *
  * Copyright (c) 2018   Ben Whitten
  * Copyright (c) 2018 Andreas Färber
@@ -34,7 +33,7 @@
 
 #define SX1301_VIRT_BASE    0x100
 #define SX1301_PAGE_LEN     0x80
-#define SX1301_PAGE_BASE(n) (SX1301_VIRT_BASE + (SX1301_PAGE_LEN * n))
+#define SX1301_PAGE_BASE(n) (SX1301_VIRT_BASE + (SX1301_PAGE_LEN * (n)))
 
 /* Page 0 */
 #define SX1301_CHRS         (SX1301_PAGE_BASE(0) + 0x23)
@@ -112,7 +111,7 @@ struct sx1301_priv {
 	struct clk		*clk32m;
 	struct gpio_desc *rst_gpio;
 	struct regmap		*regmap;
-	struct regmap_field     *regmap_fields[ARRAY_SIZE(sx1301_regmap_fields)];
+	struct regmap_field *regmap_fields[ARRAY_SIZE(sx1301_regmap_fields)];
 };
 
 int __init sx130x_radio_init(void);
-- 
2.17.1

Re: [PATCH RFC lora-next 1/4] net: lora: sx125x sx1301: correct style warnings

[Andreas Färber]

Hi Ben,

Am 19.12.18 um 16:56 schrieb Ben Whitten:
> Checkpatch highlights some style issues which need to be addressed.
> 
> Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
> ---
>  drivers/net/lora/sx125x.c | 20 +++++++++------
>  drivers/net/lora/sx1301.c | 52 ++++++++++++++++++++++-----------------
>  drivers/net/lora/sx1301.h |  7 +++---
>  3 files changed, 45 insertions(+), 34 deletions(-)

Thanks for splitting this off from the functional changes. This will
need some more explanations and discussion. In particular the comment
changes look wrong to me. Also please don't butcher code just because
checkpatch.pl may warn about line length at this early stage.

A good catch in there was the unsigned casts, which are no longer
necessary since the regmap conversion, so we should just squash that
into the earlier commits.

Note that I used to run checkpatch.pl as git commit hook:
http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html
But since some git update that started to break git rebase -i in case of
false positives (with rebase stopping and on trying to continue commits
unintentionally getting squashed), so I deactivated it again and don't
manually check each commit I stage anymore, in favor of slowly
completing implementations to a working point.

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 RFC lora-next 1/4] net: lora: sx125x sx1301: correct style warnings

[Ben Whitten]

Hi Andreas,

On 29/12/2018 09:05, Andreas Färber wrote:
> Hi Ben,
> 
> Am 19.12.18 um 16:56 schrieb Ben Whitten:
>> Checkpatch highlights some style issues which need to be addressed.
>>
>> Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
>> ---
>>   drivers/net/lora/sx125x.c | 20 +++++++++------
>>   drivers/net/lora/sx1301.c | 52 ++++++++++++++++++++++-----------------
>>   drivers/net/lora/sx1301.h |  7 +++---
>>   3 files changed, 45 insertions(+), 34 deletions(-)
> 
> Thanks for splitting this off from the functional changes. This will
> need some more explanations and discussion. In particular the comment
> changes look wrong to me. Also please don't butcher code just because
> checkpatch.pl may warn about line length at this early stage.

Yeh they seemed odd but checkpatch doesn't like a /* on a line by its 
self it seems.

> 
> A good catch in there was the unsigned casts, which are no longer
> necessary since the regmap conversion, so we should just squash that
> into the earlier commits.
> 
> Note that I used to run checkpatch.pl as git commit hook:
> http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html
> But since some git update that started to break git rebase -i in case of
> false positives (with rebase stopping and on trying to continue commits
> unintentionally getting squashed), so I deactivated it again and don't
> manually check each commit I stage anymore, in favor of slowly
> completing implementations to a working point.

Good to know, I'll avoid running it for the time and drop this from the v2

Thanks!
Ben Whitten

[PATCH RFC lora-next 2/4] net: lora: sx1301: add minimal to get AGC working prior to tx work

[Ben Whitten]

As part of initialisation when opening the lora device after loading
the AGC firmware we need to satisfy its startup procedure which involves
a few steps;

Loading a 16 entry lookup table.
For this I have hard coded the laird ETSI certified table for use on the
RG186-M2 (EU) cards, this will need investigation on how other devices
load calibration data.

Selecting the correct channel to transmit on.
Currently always 0 for the reference design.

Then ending the AGC init procedure and seeing that it has come up.

Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
---
 drivers/net/lora/sx1301.c | 254 +++++++++++++++++++++++++++++++++++++-
 drivers/net/lora/sx1301.h |  16 +++
 2 files changed, 268 insertions(+), 2 deletions(-)

diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index e75df93b96d8..0c7b6d0b31af 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -24,6 +24,121 @@
 
 #include "sx1301.h"
 
+static struct sx1301_tx_gain_lut tx_gain_lut[] = {
+	{
+		.dig_gain = 0,
+		.pa_gain = 0,
+		.dac_gain = 3,
+		.mix_gain = 8,
+		.rf_power = -3,
+	},
+	{
+		.dig_gain = 0,
+		.pa_gain = 0,
+		.dac_gain = 3,
+		.mix_gain = 9,
+		.rf_power = 0,
+	},
+	{
+		.dig_gain = 0,
+		.pa_gain = 0,
+		.dac_gain = 3,
+		.mix_gain = 12,
+		.rf_power = 3,
+	},
+	{
+		.dig_gain = 0,
+		.pa_gain = 0,
+		.dac_gain = 3,
+		.mix_gain = 13,
+		.rf_power = 4,
+	},
+	{
+		.dig_gain = 0,
+		.pa_gain = 1,
+		.dac_gain = 3,
+		.mix_gain = 8,
+		.rf_power = 6,
+	},
+	{
+		.dig_gain = 0,
+		.pa_gain = 1,
+		.dac_gain = 3,
+		.mix_gain = 9,
+		.rf_power = 9,
+	},
+	{
+		.dig_gain = 0,
+		.pa_gain = 1,
+		.dac_gain = 3,
+		.mix_gain = 10,
+		.rf_power = 10,
+	},
+	{
+		.dig_gain = 0,
+		.pa_gain = 1,
+		.dac_gain = 3,
+		.mix_gain = 11,
+		.rf_power = 12,
+	},
+	{
+		.dig_gain = 0,
+		.pa_gain = 1,
+		.dac_gain = 3,
+		.mix_gain = 12,
+		.rf_power = 13,
+	},
+	{
+		.dig_gain = 0,
+		.pa_gain = 1,
+		.dac_gain = 3,
+		.mix_gain = 13,
+		.rf_power = 14,
+	},
+	{
+		.dig_gain = 0,
+		.pa_gain = 1,
+		.dac_gain = 3,
+		.mix_gain = 15,
+		.rf_power = 16,
+	},
+	{
+		.dig_gain = 0,
+		.pa_gain = 2,
+		.dac_gain = 3,
+		.mix_gain = 10,
+		.rf_power = 19,
+	},
+	{
+		.dig_gain = 0,
+		.pa_gain = 2,
+		.dac_gain = 3,
+		.mix_gain = 11,
+		.rf_power = 21,
+	},
+	{
+		.dig_gain = 0,
+		.pa_gain = 2,
+		.dac_gain = 3,
+		.mix_gain = 12,
+		.rf_power = 22,
+	},
+	{
+		.dig_gain = 0,
+		.pa_gain = 2,
+		.dac_gain = 3,
+		.mix_gain = 13,
+		.rf_power = 24,
+	},
+	{
+		.dig_gain = 0,
+		.pa_gain = 2,
+		.dac_gain = 3,
+		.mix_gain = 14,
+		.rf_power = 25,
+	},
+};
+
 static const struct regmap_range_cfg sx1301_regmap_ranges[] = {
 	{
 		.name = "Pages",
@@ -184,6 +299,34 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu,
 	return 0;
 }
 
+static int sx1301_agc_transaction(struct sx1301_priv *priv, unsigned int val,
+				  unsigned int *status)
+{
+	int ret;
+
+	ret = regmap_write(priv->regmap, SX1301_CHRS, SX1301_AGC_CMD_WAIT);
+	if (ret) {
+		dev_err(priv->dev, "AGC transaction start failed\n");
+		return ret;
+	}
+	usleep_range(1000, 2000);
+
+	ret = regmap_write(priv->regmap, SX1301_CHRS, val);
+	if (ret) {
+		dev_err(priv->dev, "AGC transaction value failed\n");
+		return ret;
+	}
+	usleep_range(1000, 2000);
+
+	ret = regmap_read(priv->regmap, SX1301_AGCSTS, status);
+	if (ret) {
+		dev_err(priv->dev, "AGC status read failed\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 {
 	const struct firmware *fw;
@@ -356,9 +499,53 @@ static int sx1301_load_all_firmware(struct sx1301_priv *priv)
 		return -ENXIO;
 	}
 
-	return 0;
+	return ret;
 }
 
+static int sx1301_load_tx_gain_lut(struct sx1301_priv *priv)
+{
+	struct sx1301_tx_gain_lut *lut = priv->tx_gain_lut;
+	unsigned int val, status;
+	int ret, i;
+
+	/* HACK use internal gain table in the short term */
+	lut = tx_gain_lut;
+	priv->tx_gain_lut_size = ARRAY_SIZE(tx_gain_lut);
+
+	for (i = 0; i < priv->tx_gain_lut_size; i++) {
+		val = lut->mix_gain + (lut->dac_gain << 4) +
+			(lut->pa_gain << 6);
+
+		ret = sx1301_agc_transaction(priv, val, &status);
+		if (ret) {
+			dev_err(priv->dev, "AGC LUT load failed\n");
+			return ret;
+		}
+		if (status != (0x30 + i)) {
+			dev_err(priv->dev,
+				"AGC firmware LUT init error: 0x%02X\n", val);
+			return -ENXIO;
+		}
+		lut++;
+	}
+
+	/* Abort the transaction if there are less then 16 entries */
+	if (priv->tx_gain_lut_size < SX1301_TX_GAIN_LUT_MAX) {
+		ret = sx1301_agc_transaction(priv, SX1301_AGC_CMD_ABORT, &val);
+		if (ret) {
+			dev_err(priv->dev, "AGC LUT abort failed\n");
+			return ret;
+		}
+		if (val != 0x30) {
+			dev_err(priv->dev,
+				"AGC firmware LUT abort error: 0x%02X\n", val);
+			return -ENXIO;
+		}
+	}
+
+	return ret;
+};
+
 static netdev_tx_t sx130x_loradev_start_xmit(struct sk_buff *skb,
 					     struct net_device *netdev)
 {
@@ -378,6 +565,7 @@ static int sx130x_loradev_open(struct net_device *netdev)
 {
 	struct sx1301_priv *priv = netdev_priv(netdev);
 	int ret;
+	unsigned int val;
 
 	netdev_dbg(netdev, "%s", __func__);
 
@@ -416,12 +604,74 @@ static int sx130x_loradev_open(struct net_device *netdev)
 	if (ret)
 		return ret;
 
-	/* TODO */
+	/* TODO Load constant adjustments, patches */
+
+	/* TODO Frequency time drift */
+
+	/* TODO Configure lora multi demods, bitfield of active */
+
+	/* TODO Load concenrator multi channel frequencies */
+
+	/* TODO enale to correlator on enabled frequenies */
+
+	/* TODO PPMi, and modem enable */
 
 	ret = sx1301_load_all_firmware(priv);
 	if (ret)
 		return ret;
 
+	usleep_range(1000, 2000);
+
+	ret = regmap_read(priv->regmap, SX1301_AGCSTS, &val);
+	if (ret) {
+		dev_err(priv->dev, "AGC status read failed\n");
+		return ret;
+	}
+	if (val != 0x10) {
+		dev_err(priv->dev, "AGC firmware init failure: 0x%02X\n", val);
+		return -ENXIO;
+	}
+
+	ret = sx1301_load_tx_gain_lut(priv);
+	if (ret)
+		return ret;
+
+	/* Load Tx freq MSBs
+	 * Always 3 if f > 768 for SX1257 or f > 384 for SX1255
+	 */
+	ret = sx1301_agc_transaction(priv, 3, &val);
+	if (ret) {
+		dev_err(priv->dev, "AGC Tx MSBs load failed\n");
+		return ret;
+	}
+	if (val != 0x33) {
+		dev_err(priv->dev, "AGC firmware Tx MSBs error: 0x%02X\n", val);
+		return -ENXIO;
+	}
+
+	/* Load chan_select firmware option */
+	ret = sx1301_agc_transaction(priv, 0, &val);
+	if (ret) {
+		dev_err(priv->dev, "AGC chan select failed\n");
+		return ret;
+	}
+	if (val != 0x30) {
+		dev_err(priv->dev,
+			"AGC firmware chan select error: 0x%02X", val);
+		return -ENXIO;
+	}
+
+	/* End AGC firmware init and check status */
+	ret = sx1301_agc_transaction(priv, 0, &val);
+	if (ret) {
+		dev_err(priv->dev, "AGC radio select failed\n");
+		return ret;
+	}
+	if (val != 0x40) {
+		dev_err(priv->dev, "AGC firmware init error: 0x%02X", val);
+		return -ENXIO;
+	}
+
 	ret = open_loradev(netdev);
 	if (ret)
 		return ret;
diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
index dd2b7da94fcc..04c9af64c181 100644
--- a/drivers/net/lora/sx1301.h
+++ b/drivers/net/lora/sx1301.h
@@ -20,6 +20,11 @@
 #define SX1301_MCU_AGC_FW_VERSION 4
 #define SX1301_MCU_AGC_CAL_FW_VERSION 2
 
+#define SX1301_AGC_CMD_WAIT 16
+#define SX1301_AGC_CMD_ABORT 17
+
+#define SX1301_TX_GAIN_LUT_MAX 16
+
 /* Page independent */
 #define SX1301_PAGE     0x00
 #define SX1301_VER      0x01
@@ -105,6 +110,14 @@ static const struct reg_field sx1301_regmap_fields[] = {
 		REG_FIELD(SX1301_EMERGENCY_FORCE_HOST_CTRL, 0, 0),
 };
 
+struct sx1301_tx_gain_lut {
+	unsigned int	dig_gain;
+	unsigned int	pa_gain;
+	unsigned int	dac_gain;
+	unsigned int	mix_gain;
+	int		rf_power; /* dBm measured at board connector */
+};
+
 struct sx1301_priv {
 	struct lora_dev_priv lora;
 	struct device		*dev;
@@ -112,6 +125,9 @@ struct sx1301_priv {
 	struct gpio_desc *rst_gpio;
 	struct regmap		*regmap;
 	struct regmap_field *regmap_fields[ARRAY_SIZE(sx1301_regmap_fields)];
+
+	struct sx1301_tx_gain_lut tx_gain_lut[SX1301_TX_GAIN_LUT_MAX];
+	u8 tx_gain_lut_size;
 };
 
 int __init sx130x_radio_init(void);
-- 
2.17.1

Re: [PATCH RFC lora-next 2/4] net: lora: sx1301: add minimal to get AGC working prior to tx work

[Andreas Färber]

Hi Ben,

Am 19.12.18 um 16:56 schrieb Ben Whitten:
> As part of initialisation when opening the lora device after loading
> the AGC firmware we need to satisfy its startup procedure which involves
> a few steps;
> 
> Loading a 16 entry lookup table.
> For this I have hard coded the laird ETSI certified table for use on the
> RG186-M2 (EU) cards, this will need investigation on how other devices
> load calibration data.

Isn't calibration performed before this firmware is initialized? I left
out reading the values back from firmware previously, but that should
get implemented. In the userspace implementation, do you get these from
a config file or did you modify the reference code to hardcode them?

If these are different calibration values from the ones returned by
firmware, then a DT property would be an easy way to get
hardware-specific data into the driver. However, same as with your clk
config, that makes us dependent on DT, which we shouldn't be for ACPI
and USB. If we consider it configuration data rather than an immutable
fact, then we would need a netlink command to set them.

In any case, we have some other vendors on this list, so hopefully
someone can comment. :)

> 
> Selecting the correct channel to transmit on.
> Currently always 0 for the reference design.

Similarly: DT or netlink depending on whether fixed, and fall back to 0
as default.

> 
> Then ending the AGC init procedure and seeing that it has come up.
> 
> Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
> ---
>  drivers/net/lora/sx1301.c | 254 +++++++++++++++++++++++++++++++++++++-
>  drivers/net/lora/sx1301.h |  16 +++
>  2 files changed, 268 insertions(+), 2 deletions(-)

Many thanks for working on this! Some nits inline.

> diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
> index e75df93b96d8..0c7b6d0b31af 100644
> --- a/drivers/net/lora/sx1301.c
> +++ b/drivers/net/lora/sx1301.c
> @@ -24,6 +24,121 @@
>  
>  #include "sx1301.h"
>  
> +static struct sx1301_tx_gain_lut tx_gain_lut[] = {
> +	{
> +		.dig_gain = 0,
> +		.pa_gain = 0,
> +		.dac_gain = 3,
> +		.mix_gain = 8,
> +		.rf_power = -3,
> +	},
> +	{
> +		.dig_gain = 0,
> +		.pa_gain = 0,
> +		.dac_gain = 3,
> +		.mix_gain = 9,
> +		.rf_power = 0,
> +	},
> +	{
> +		.dig_gain = 0,
> +		.pa_gain = 0,
> +		.dac_gain = 3,
> +		.mix_gain = 12,
> +		.rf_power = 3,
> +	},
> +	{
> +		.dig_gain = 0,
> +		.pa_gain = 0,
> +		.dac_gain = 3,
> +		.mix_gain = 13,
> +		.rf_power = 4,
> +	},
> +	{
> +		.dig_gain = 0,
> +		.pa_gain = 1,
> +		.dac_gain = 3,
> +		.mix_gain = 8,
> +		.rf_power = 6,
> +	},
> +	{
> +		.dig_gain = 0,
> +		.pa_gain = 1,
> +		.dac_gain = 3,
> +		.mix_gain = 9,
> +		.rf_power = 9,
> +	},
> +	{
> +		.dig_gain = 0,
> +		.pa_gain = 1,
> +		.dac_gain = 3,
> +		.mix_gain = 10,
> +		.rf_power = 10,
> +	},
> +	{
> +		.dig_gain = 0,
> +		.pa_gain = 1,
> +		.dac_gain = 3,
> +		.mix_gain = 11,
> +		.rf_power = 12,
> +	},
> +	{
> +		.dig_gain = 0,
> +		.pa_gain = 1,
> +		.dac_gain = 3,
> +		.mix_gain = 12,
> +		.rf_power = 13,
> +	},
> +	{
> +		.dig_gain = 0,
> +		.pa_gain = 1,
> +		.dac_gain = 3,
> +		.mix_gain = 13,
> +		.rf_power = 14,
> +	},
> +	{
> +		.dig_gain = 0,
> +		.pa_gain = 1,
> +		.dac_gain = 3,
> +		.mix_gain = 15,
> +		.rf_power = 16,
> +	},
> +	{
> +		.dig_gain = 0,
> +		.pa_gain = 2,
> +		.dac_gain = 3,
> +		.mix_gain = 10,
> +		.rf_power = 19,
> +	},
> +	{
> +		.dig_gain = 0,
> +		.pa_gain = 2,
> +		.dac_gain = 3,
> +		.mix_gain = 11,
> +		.rf_power = 21,
> +	},
> +	{
> +		.dig_gain = 0,
> +		.pa_gain = 2,
> +		.dac_gain = 3,
> +		.mix_gain = 12,
> +		.rf_power = 22,
> +	},
> +	{
> +		.dig_gain = 0,
> +		.pa_gain = 2,
> +		.dac_gain = 3,
> +		.mix_gain = 13,
> +		.rf_power = 24,
> +	},
> +	{
> +		.dig_gain = 0,
> +		.pa_gain = 2,
> +		.dac_gain = 3,
> +		.mix_gain = 14,
> +		.rf_power = 25,
> +	},
> +};
> +
>  static const struct regmap_range_cfg sx1301_regmap_ranges[] = {
>  	{
>  		.name = "Pages",
> @@ -184,6 +299,34 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu,
>  	return 0;
>  }
>  
> +static int sx1301_agc_transaction(struct sx1301_priv *priv, unsigned int val,
> +				  unsigned int *status)
> +{
> +	int ret;
> +
> +	ret = regmap_write(priv->regmap, SX1301_CHRS, SX1301_AGC_CMD_WAIT);
> +	if (ret) {
> +		dev_err(priv->dev, "AGC transaction start failed\n");
> +		return ret;
> +	}
> +	usleep_range(1000, 2000);
> +
> +	ret = regmap_write(priv->regmap, SX1301_CHRS, val);
> +	if (ret) {
> +		dev_err(priv->dev, "AGC transaction value failed\n");
> +		return ret;
> +	}
> +	usleep_range(1000, 2000);

Looks like CHRS needs some regmap annotation as e.g. volatile?

> +
> +	ret = regmap_read(priv->regmap, SX1301_AGCSTS, status);
> +	if (ret) {
> +		dev_err(priv->dev, "AGC status read failed\n");
> +		return ret;
> +	}

Ditto for AGCSTS.
Otherwise we won't be able to enable caching and field access will
always be less performant than the previous code.

> +
> +	return 0;
> +}
> +
>  static int sx1301_agc_calibrate(struct sx1301_priv *priv)
>  {
>  	const struct firmware *fw;
> @@ -356,9 +499,53 @@ static int sx1301_load_all_firmware(struct sx1301_priv *priv)
>  		return -ENXIO;
>  	}
>  
> -	return 0;
> +	return ret;

Accidental change?

>  }
>  
> +static int sx1301_load_tx_gain_lut(struct sx1301_priv *priv)
> +{
> +	struct sx1301_tx_gain_lut *lut = priv->tx_gain_lut;
> +	unsigned int val, status;
> +	int ret, i;
> +
> +	/* HACK use internal gain table in the short term */
> +	lut = tx_gain_lut;
> +	priv->tx_gain_lut_size = ARRAY_SIZE(tx_gain_lut);
> +
> +	for (i = 0; i < priv->tx_gain_lut_size; i++) {
> +		val = lut->mix_gain + (lut->dac_gain << 4) +
> +			(lut->pa_gain << 6);

Looks like we're writing to a bitfield? Please use constants for the
shifts then (maybe add masks, too?), and I'd rather reverse the order.

> +
> +		ret = sx1301_agc_transaction(priv, val, &status);
> +		if (ret) {
> +			dev_err(priv->dev, "AGC LUT load failed\n");
> +			return ret;
> +		}
> +		if (status != (0x30 + i)) {
> +			dev_err(priv->dev,
> +				"AGC firmware LUT init error: 0x%02X\n", val);

Continuing from 1/4, please avoid wasting the first like that.
Also I think x is more common than X?

> +			return -ENXIO;
> +		}
> +		lut++;
> +	}
> +
> +	/* Abort the transaction if there are less then 16 entries */
> +	if (priv->tx_gain_lut_size < SX1301_TX_GAIN_LUT_MAX) {
> +		ret = sx1301_agc_transaction(priv, SX1301_AGC_CMD_ABORT, &val);
> +		if (ret) {
> +			dev_err(priv->dev, "AGC LUT abort failed\n");
> +			return ret;
> +		}
> +		if (val != 0x30) {

Any insights into the magic number that would allow for a constant?

> +			dev_err(priv->dev,
> +				"AGC firmware LUT abort error: 0x%02X\n", val);
> +			return -ENXIO;
> +		}
> +	}
> +
> +	return ret;
> +};
> +
>  static netdev_tx_t sx130x_loradev_start_xmit(struct sk_buff *skb,
>  					     struct net_device *netdev)
>  {
> @@ -378,6 +565,7 @@ static int sx130x_loradev_open(struct net_device *netdev)
>  {
>  	struct sx1301_priv *priv = netdev_priv(netdev);
>  	int ret;
> +	unsigned int val;

I'd prefer to switch those two lines, as you and I have done elsewhere.

>  
>  	netdev_dbg(netdev, "%s", __func__);
>  
> @@ -416,12 +604,74 @@ static int sx130x_loradev_open(struct net_device *netdev)
>  	if (ret)
>  		return ret;
>  
> -	/* TODO */
> +	/* TODO Load constant adjustments, patches */
> +
> +	/* TODO Frequency time drift */
> +
> +	/* TODO Configure lora multi demods, bitfield of active */
> +
> +	/* TODO Load concenrator multi channel frequencies */

concentrator

> +
> +	/* TODO enale to correlator on enabled frequenies */

enale?
frequencies

> +
> +	/* TODO PPMi, and modem enable */
>  
>  	ret = sx1301_load_all_firmware(priv);
>  	if (ret)
>  		return ret;
>  
> +	usleep_range(1000, 2000);
> +
> +	ret = regmap_read(priv->regmap, SX1301_AGCSTS, &val);
> +	if (ret) {
> +		dev_err(priv->dev, "AGC status read failed\n");
> +		return ret;
> +	}
> +	if (val != 0x10) {

Magic number

> +		dev_err(priv->dev, "AGC firmware init failure: 0x%02X\n", val);
> +		return -ENXIO;
> +	}
> +
> +	ret = sx1301_load_tx_gain_lut(priv);
> +	if (ret)
> +		return ret;
> +
> +	/* Load Tx freq MSBs
> +	 * Always 3 if f > 768 for SX1257 or f > 384 for SX1255
> +	 */

That comment style seems rather uncommon.
What about SX1258?
Mark it as TODO/HACK or use a variable below?

> +	ret = sx1301_agc_transaction(priv, 3, &val);
> +	if (ret) {
> +		dev_err(priv->dev, "AGC Tx MSBs load failed\n");
> +		return ret;
> +	}
> +	if (val != 0x33) {

Magic number

> +		dev_err(priv->dev, "AGC firmware Tx MSBs error: 0x%02X\n", val);
> +		return -ENXIO;
> +	}
> +
> +	/* Load chan_select firmware option */
> +	ret = sx1301_agc_transaction(priv, 0, &val);

I'm guessing this is the mentioned hardcoded channel? I.e., radio A is
selected? Are there any hardware properties involved here (DT) or is
that a pure configuration choice (netlink)?

> +	if (ret) {
> +		dev_err(priv->dev, "AGC chan select failed\n");
> +		return ret;
> +	}
> +	if (val != 0x30) {
> +		dev_err(priv->dev,
> +			"AGC firmware chan select error: 0x%02X", val);
> +		return -ENXIO;
> +	}
> +
> +	/* End AGC firmware init and check status */
> +	ret = sx1301_agc_transaction(priv, 0, &val);
> +	if (ret) {
> +		dev_err(priv->dev, "AGC radio select failed\n");
> +		return ret;
> +	}
> +	if (val != 0x40) {
> +		dev_err(priv->dev, "AGC firmware init error: 0x%02X", val);
> +		return -ENXIO;
> +	}

Could you move all that new code into an sx130x_agc_init() helper
function please?

Are those operations all reentrant, or do we need code for _close, too?

We should also think about locking a sequence of operations, like I did
for sx1276 iirc.

> +
>  	ret = open_loradev(netdev);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
> index dd2b7da94fcc..04c9af64c181 100644
> --- a/drivers/net/lora/sx1301.h
> +++ b/drivers/net/lora/sx1301.h
> @@ -20,6 +20,11 @@
>  #define SX1301_MCU_AGC_FW_VERSION 4
>  #define SX1301_MCU_AGC_CAL_FW_VERSION 2
>  
> +#define SX1301_AGC_CMD_WAIT 16
> +#define SX1301_AGC_CMD_ABORT 17

This would seem a good place for the status codes, too?

> +
> +#define SX1301_TX_GAIN_LUT_MAX 16
> +
>  /* Page independent */
>  #define SX1301_PAGE     0x00
>  #define SX1301_VER      0x01
> @@ -105,6 +110,14 @@ static const struct reg_field sx1301_regmap_fields[] = {
>  		REG_FIELD(SX1301_EMERGENCY_FORCE_HOST_CTRL, 0, 0),
>  };
>  
> +struct sx1301_tx_gain_lut {
> +	unsigned int	dig_gain;
> +	unsigned int	pa_gain;
> +	unsigned int	dac_gain;
> +	unsigned int	mix_gain;
> +	int		rf_power; /* dBm measured at board connector */
> +};
> +
>  struct sx1301_priv {
>  	struct lora_dev_priv lora;
>  	struct device		*dev;
> @@ -112,6 +125,9 @@ struct sx1301_priv {
>  	struct gpio_desc *rst_gpio;
>  	struct regmap		*regmap;
>  	struct regmap_field *regmap_fields[ARRAY_SIZE(sx1301_regmap_fields)];
> +
> +	struct sx1301_tx_gain_lut tx_gain_lut[SX1301_TX_GAIN_LUT_MAX];
> +	u8 tx_gain_lut_size;
>  };
>  
>  int __init sx130x_radio_init(void);

Cheers,
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 RFC lora-next 2/4] net: lora: sx1301: add minimal to get AGC working prior to tx work

[Ben Whitten]

On 29/12/2018 09:58, Andreas Färber wrote:
> Hi Ben,
> 
> Am 19.12.18 um 16:56 schrieb Ben Whitten:
>> As part of initialisation when opening the lora device after loading
>> the AGC firmware we need to satisfy its startup procedure which involves
>> a few steps;
>>
>> Loading a 16 entry lookup table.
>> For this I have hard coded the laird ETSI certified table for use on the
>> RG186-M2 (EU) cards, this will need investigation on how other devices
>> load calibration data.
> 
> Isn't calibration performed before this firmware is initialized? I left
> out reading the values back from firmware previously, but that should
> get implemented. In the userspace implementation, do you get these from
> a config file or did you modify the reference code to hardcode them?

The calibration you refer to is the IQ offset calibration, as far as
I can tell this is a separate thing from the power table the chip uses.
In the user space implementation these values are placed in the config
file.

> 
> If these are different calibration values from the ones returned by
> firmware, then a DT property would be an easy way to get
> hardware-specific data into the driver. However, same as with your clk
> config, that makes us dependent on DT, which we shouldn't be for ACPI
> and USB. If we consider it configuration data rather than an immutable
> fact, then we would need a netlink command to set them.

Perhaps we can provide both mechanisms, a DT power table and a mechanism
to set via netlink prior to opening the interface.
As these power settings are hardware dependent and certified for our
card by FCC and ETSI I would prefer to include in DT.

> 
> In any case, we have some other vendors on this list, so hopefully
> someone can comment. :)
> 
>>
>> Selecting the correct channel to transmit on.
>> Currently always 0 for the reference design.
> 
> Similarly: DT or netlink depending on whether fixed, and fall back to 0
> as default.

Agreed, so on the DT would it be appropriate to have a handle in the
sx1301 node with a link to the radio which can transmit, eg.

tx = <&radio0 0>;

Allowing for both to be transmitters if that if a hardware choice.

> 
>>
>> Then ending the AGC init procedure and seeing that it has come up.
>>
>> Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
>> ---
>>   drivers/net/lora/sx1301.c | 254 +++++++++++++++++++++++++++++++++++++-
>>   drivers/net/lora/sx1301.h |  16 +++
>>   2 files changed, 268 insertions(+), 2 deletions(-)
> 
> Many thanks for working on this! Some nits inline.
> 
>> diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
>> index e75df93b96d8..0c7b6d0b31af 100644
>> --- a/drivers/net/lora/sx1301.c
>> +++ b/drivers/net/lora/sx1301.c
>> @@ -24,6 +24,121 @@
>>   
>>   #include "sx1301.h"
>>   
>> +static struct sx1301_tx_gain_lut tx_gain_lut[] = {
>> +	{
>> +		.dig_gain = 0,
>> +		.pa_gain = 0,
>> +		.dac_gain = 3,
>> +		.mix_gain = 8,
>> +		.rf_power = -3,
>> +	},
>> +	{
>> +		.dig_gain = 0,
>> +		.pa_gain = 0,
>> +		.dac_gain = 3,
>> +		.mix_gain = 9,
>> +		.rf_power = 0,
>> +	},
>> +	{
>> +		.dig_gain = 0,
>> +		.pa_gain = 0,
>> +		.dac_gain = 3,
>> +		.mix_gain = 12,
>> +		.rf_power = 3,
>> +	},
>> +	{
>> +		.dig_gain = 0,
>> +		.pa_gain = 0,
>> +		.dac_gain = 3,
>> +		.mix_gain = 13,
>> +		.rf_power = 4,
>> +	},
>> +	{
>> +		.dig_gain = 0,
>> +		.pa_gain = 1,
>> +		.dac_gain = 3,
>> +		.mix_gain = 8,
>> +		.rf_power = 6,
>> +	},
>> +	{
>> +		.dig_gain = 0,
>> +		.pa_gain = 1,
>> +		.dac_gain = 3,
>> +		.mix_gain = 9,
>> +		.rf_power = 9,
>> +	},
>> +	{
>> +		.dig_gain = 0,
>> +		.pa_gain = 1,
>> +		.dac_gain = 3,
>> +		.mix_gain = 10,
>> +		.rf_power = 10,
>> +	},
>> +	{
>> +		.dig_gain = 0,
>> +		.pa_gain = 1,
>> +		.dac_gain = 3,
>> +		.mix_gain = 11,
>> +		.rf_power = 12,
>> +	},
>> +	{
>> +		.dig_gain = 0,
>> +		.pa_gain = 1,
>> +		.dac_gain = 3,
>> +		.mix_gain = 12,
>> +		.rf_power = 13,
>> +	},
>> +	{
>> +		.dig_gain = 0,
>> +		.pa_gain = 1,
>> +		.dac_gain = 3,
>> +		.mix_gain = 13,
>> +		.rf_power = 14,
>> +	},
>> +	{
>> +		.dig_gain = 0,
>> +		.pa_gain = 1,
>> +		.dac_gain = 3,
>> +		.mix_gain = 15,
>> +		.rf_power = 16,
>> +	},
>> +	{
>> +		.dig_gain = 0,
>> +		.pa_gain = 2,
>> +		.dac_gain = 3,
>> +		.mix_gain = 10,
>> +		.rf_power = 19,
>> +	},
>> +	{
>> +		.dig_gain = 0,
>> +		.pa_gain = 2,
>> +		.dac_gain = 3,
>> +		.mix_gain = 11,
>> +		.rf_power = 21,
>> +	},
>> +	{
>> +		.dig_gain = 0,
>> +		.pa_gain = 2,
>> +		.dac_gain = 3,
>> +		.mix_gain = 12,
>> +		.rf_power = 22,
>> +	},
>> +	{
>> +		.dig_gain = 0,
>> +		.pa_gain = 2,
>> +		.dac_gain = 3,
>> +		.mix_gain = 13,
>> +		.rf_power = 24,
>> +	},
>> +	{
>> +		.dig_gain = 0,
>> +		.pa_gain = 2,
>> +		.dac_gain = 3,
>> +		.mix_gain = 14,
>> +		.rf_power = 25,
>> +	},
>> +};
>> +
>>   static const struct regmap_range_cfg sx1301_regmap_ranges[] = {
>>   	{
>>   		.name = "Pages",
>> @@ -184,6 +299,34 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu,
>>   	return 0;
>>   }
>>   
>> +static int sx1301_agc_transaction(struct sx1301_priv *priv, unsigned int val,
>> +				  unsigned int *status)
>> +{
>> +	int ret;
>> +
>> +	ret = regmap_write(priv->regmap, SX1301_CHRS, SX1301_AGC_CMD_WAIT);
>> +	if (ret) {
>> +		dev_err(priv->dev, "AGC transaction start failed\n");
>> +		return ret;
>> +	}
>> +	usleep_range(1000, 2000);
>> +
>> +	ret = regmap_write(priv->regmap, SX1301_CHRS, val);
>> +	if (ret) {
>> +		dev_err(priv->dev, "AGC transaction value failed\n");
>> +		return ret;
>> +	}
>> +	usleep_range(1000, 2000);
> 
> Looks like CHRS needs some regmap annotation as e.g. volatile?
> 
>> +
>> +	ret = regmap_read(priv->regmap, SX1301_AGCSTS, status);
>> +	if (ret) {
>> +		dev_err(priv->dev, "AGC status read failed\n");
>> +		return ret;
>> +	}
> 
> Ditto for AGCSTS.
> Otherwise we won't be able to enable caching and field access will
> always be less performant than the previous code.

Agreed, will do.

> 
>> +
>> +	return 0;
>> +}
>> +
>>   static int sx1301_agc_calibrate(struct sx1301_priv *priv)
>>   {
>>   	const struct firmware *fw;
>> @@ -356,9 +499,53 @@ static int sx1301_load_all_firmware(struct sx1301_priv *priv)
>>   		return -ENXIO;
>>   	}
>>   
>> -	return 0;
>> +	return ret;
> 
> Accidental change?

Whoops yes.

> 
>>   }
>>   
>> +static int sx1301_load_tx_gain_lut(struct sx1301_priv *priv)
>> +{
>> +	struct sx1301_tx_gain_lut *lut = priv->tx_gain_lut;
>> +	unsigned int val, status;
>> +	int ret, i;
>> +
>> +	/* HACK use internal gain table in the short term */
>> +	lut = tx_gain_lut;
>> +	priv->tx_gain_lut_size = ARRAY_SIZE(tx_gain_lut);
>> +
>> +	for (i = 0; i < priv->tx_gain_lut_size; i++) {
>> +		val = lut->mix_gain + (lut->dac_gain << 4) +
>> +			(lut->pa_gain << 6);
> 
> Looks like we're writing to a bitfield? Please use constants for the
> shifts then (maybe add masks, too?), and I'd rather reverse the order.

Yes its a bit field, just needs to be written at once. Will do.

> 
>> +
>> +		ret = sx1301_agc_transaction(priv, val, &status);
>> +		if (ret) {
>> +			dev_err(priv->dev, "AGC LUT load failed\n");
>> +			return ret;
>> +		}
>> +		if (status != (0x30 + i)) {
>> +			dev_err(priv->dev,
>> +				"AGC firmware LUT init error: 0x%02X\n", val);
> 
> Continuing from 1/4, please avoid wasting the first like that.
> Also I think x is more common than X?

Sure thing.

> 
>> +			return -ENXIO;
>> +		}
>> +		lut++;
>> +	}
>> +
>> +	/* Abort the transaction if there are less then 16 entries */
>> +	if (priv->tx_gain_lut_size < SX1301_TX_GAIN_LUT_MAX) {
>> +		ret = sx1301_agc_transaction(priv, SX1301_AGC_CMD_ABORT, &val);
>> +		if (ret) {
>> +			dev_err(priv->dev, "AGC LUT abort failed\n");
>> +			return ret;
>> +		}
>> +		if (val != 0x30) {
> 
> Any insights into the magic number that would allow for a constant?

No insights to the meaning of the bits, may just be a success constant.

> 
>> +			dev_err(priv->dev,
>> +				"AGC firmware LUT abort error: 0x%02X\n", val);
>> +			return -ENXIO;
>> +		}
>> +	}
>> +
>> +	return ret;
>> +};
>> +
>>   static netdev_tx_t sx130x_loradev_start_xmit(struct sk_buff *skb,
>>   					     struct net_device *netdev)
>>   {
>> @@ -378,6 +565,7 @@ static int sx130x_loradev_open(struct net_device *netdev)
>>   {
>>   	struct sx1301_priv *priv = netdev_priv(netdev);
>>   	int ret;
>> +	unsigned int val;
> 
> I'd prefer to switch those two lines, as you and I have done elsewhere.

Sure will do.

> 
>>   
>>   	netdev_dbg(netdev, "%s", __func__);
>>   
>> @@ -416,12 +604,74 @@ static int sx130x_loradev_open(struct net_device *netdev)
>>   	if (ret)
>>   		return ret;
>>   
>> -	/* TODO */
>> +	/* TODO Load constant adjustments, patches */
>> +
>> +	/* TODO Frequency time drift */
>> +
>> +	/* TODO Configure lora multi demods, bitfield of active */
>> +
>> +	/* TODO Load concenrator multi channel frequencies */
> 
> concentrator
> 
>> +
>> +	/* TODO enale to correlator on enabled frequenies */
> 
> enale?
> frequencies
> 
>> +
>> +	/* TODO PPMi, and modem enable */
>>   
>>   	ret = sx1301_load_all_firmware(priv);
>>   	if (ret)
>>   		return ret;
>>   
>> +	usleep_range(1000, 2000);
>> +
>> +	ret = regmap_read(priv->regmap, SX1301_AGCSTS, &val);
>> +	if (ret) {
>> +		dev_err(priv->dev, "AGC status read failed\n");
>> +		return ret;
>> +	}
>> +	if (val != 0x10) {
> 
> Magic number
> 
>> +		dev_err(priv->dev, "AGC firmware init failure: 0x%02X\n", val);
>> +		return -ENXIO;
>> +	}
>> +
>> +	ret = sx1301_load_tx_gain_lut(priv);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Load Tx freq MSBs
>> +	 * Always 3 if f > 768 for SX1257 or f > 384 for SX1255
>> +	 */
> 
> That comment style seems rather uncommon.

An artifact of checkpatch, which wants no blank lines at the start of a 
comment block.

> What about SX1258?
> Mark it as TODO/HACK or use a variable below?

Variable sounds good populated from a case of tx radio types.
Note we may have a problem if we try and use a radio outside of this 
band as we may have to reinitialize the AGC to get this value in.

> 
>> +	ret = sx1301_agc_transaction(priv, 3, &val);
>> +	if (ret) {
>> +		dev_err(priv->dev, "AGC Tx MSBs load failed\n");
>> +		return ret;
>> +	}
>> +	if (val != 0x33) {
> 
> Magic number

Looks like a success message | loaded value. I'll add that.

> 
>> +		dev_err(priv->dev, "AGC firmware Tx MSBs error: 0x%02X\n", val);
>> +		return -ENXIO;
>> +	}
>> +
>> +	/* Load chan_select firmware option */
>> +	ret = sx1301_agc_transaction(priv, 0, &val);
> 
> I'm guessing this is the mentioned hardcoded channel? I.e., radio A is
> selected? Are there any hardware properties involved here (DT) or is
> that a pure configuration choice (netlink)?

I believe this is the transmit choice so it is bound by hardware, if 
there is a board with two radios in the TX chain then it should be 
select-able which one to transmit. Though I'm not sure how common that 
use case will be.

> 
>> +	if (ret) {
>> +		dev_err(priv->dev, "AGC chan select failed\n");
>> +		return ret;
>> +	}
>> +	if (val != 0x30) {
>> +		dev_err(priv->dev,
>> +			"AGC firmware chan select error: 0x%02X", val);
>> +		return -ENXIO;
>> +	}
>> +
>> +	/* End AGC firmware init and check status */
>> +	ret = sx1301_agc_transaction(priv, 0, &val);
>> +	if (ret) {
>> +		dev_err(priv->dev, "AGC radio select failed\n");
>> +		return ret;
>> +	}
>> +	if (val != 0x40) {
>> +		dev_err(priv->dev, "AGC firmware init error: 0x%02X", val);
>> +		return -ENXIO;
>> +	}
> 
> Could you move all that new code into an sx130x_agc_init() helper
> function please?

Yep will do.

> 
> Are those operations all reentrant, or do we need code for _close, too?

I don't know if there is any shutdown procedure, I think on any open it 
needs to reinitialize.

> 
> We should also think about locking a sequence of operations, like I did
> for sx1276 iirc.

I see you have just sent up a patch with that.

> 
>> +
>>   	ret = open_loradev(netdev);
>>   	if (ret)
>>   		return ret;
>> diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
>> index dd2b7da94fcc..04c9af64c181 100644
>> --- a/drivers/net/lora/sx1301.h
>> +++ b/drivers/net/lora/sx1301.h
>> @@ -20,6 +20,11 @@
>>   #define SX1301_MCU_AGC_FW_VERSION 4
>>   #define SX1301_MCU_AGC_CAL_FW_VERSION 2
>>   
>> +#define SX1301_AGC_CMD_WAIT 16
>> +#define SX1301_AGC_CMD_ABORT 17
> 
> This would seem a good place for the status codes, too?

Yep

> 
>> +
>> +#define SX1301_TX_GAIN_LUT_MAX 16
>> +
>>   /* Page independent */
>>   #define SX1301_PAGE     0x00
>>   #define SX1301_VER      0x01
>> @@ -105,6 +110,14 @@ static const struct reg_field sx1301_regmap_fields[] = {
>>   		REG_FIELD(SX1301_EMERGENCY_FORCE_HOST_CTRL, 0, 0),
>>   };
>>   
>> +struct sx1301_tx_gain_lut {
>> +	unsigned int	dig_gain;
>> +	unsigned int	pa_gain;
>> +	unsigned int	dac_gain;
>> +	unsigned int	mix_gain;
>> +	int		rf_power; /* dBm measured at board connector */
>> +};
>> +
>>   struct sx1301_priv {
>>   	struct lora_dev_priv lora;
>>   	struct device		*dev;
>> @@ -112,6 +125,9 @@ struct sx1301_priv {
>>   	struct gpio_desc *rst_gpio;
>>   	struct regmap		*regmap;
>>   	struct regmap_field *regmap_fields[ARRAY_SIZE(sx1301_regmap_fields)];
>> +
>> +	struct sx1301_tx_gain_lut tx_gain_lut[SX1301_TX_GAIN_LUT_MAX];
>> +	u8 tx_gain_lut_size;
>>   };
>>   
>>   int __init sx130x_radio_init(void);


Thanks!
Ben Whitten

[PATCH RFC lora-next 3/4] net: lora: sx1301: add minimal to get transmission out

[Ben Whitten]

Because we cannot wait inside start_xmit so we copy the pointer to the skb
and schedule transmission work to be done.

For the transmission a lot is hard coded to just get a signal out of the
sx1301, it looks like using a header like CAN frames do will be a good
mechanism to get information such as coding rate, spreading factor and
power into the transmission.

Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
---
 drivers/net/lora/sx1301.c | 166 +++++++++++++++++++++++++++++++++++++-
 drivers/net/lora/sx1301.h |  21 +++++
 2 files changed, 186 insertions(+), 1 deletion(-)

diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index 0c7b6d0b31af..9bcbb967f307 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -24,6 +24,44 @@
 
 #include "sx1301.h"
 
+struct sx1301_tx_header {
+	u8	tx_freq[3];
+	u32	start;
+	u8  tx_power:4,
+		modulation_type:1,
+		radio_select:1,
+		resered0:2;
+	u8	reserved1;
+
+	union {
+		struct lora_t {
+			u8	sf:4,
+				cr:3,
+				crc16_en:1;
+			u8	payload_len;
+			u8	mod_bw:2,
+				implicit_header:1,
+				ppm_offset:1,
+				invert_pol:1,
+				reserved0:3;
+			u16	preamble;
+			u8	reserved1;
+			u8	reserved2;
+		} lora;
+		struct fsk_t {
+			u8	freq_dev;
+			u8	payload_len;
+			u8	packet_mode:1,
+				crc_en:1,
+				enc_mode:2,
+				crc_mode:1,
+				reserved0:3;
+			u16	preamble;
+			u16	bitrate;
+		} fsk;
+	} u;
+} __packed;
+
 static struct sx1301_tx_gain_lut tx_gain_lut[] = {
 	{
 		.dig_gain = 0,
@@ -546,9 +584,87 @@ static int sx1301_load_tx_gain_lut(struct sx1301_priv *priv)
 	return ret;
 };
 
+static int sx1301_tx(struct sx1301_priv *priv, void *data, int len)
+{
+	int ret, i;
+	u8 buff[256 + 16];
+	struct sx1301_tx_header *hdr = (struct sx1301_tx_header *)buff;
+
+	/* TODO general checks to make sure we CAN send */
+
+	/* TODO Enable notch filter for lora 125 */
+
+	/* TODO get start delay for this TX */
+
+	/* TODO interpret tx power, HACK just set max power */
+
+	/* TODO get TX imbalance for this pow index from calibration step */
+
+	/* TODO set the dig gain */
+
+	/* TODO set TX PLL freq based on radio used to TX */
+
+	memset(buff, 0, sizeof(buff));
+
+	/* HACK set to 868MHz */
+	hdr->tx_freq[0] = 217;
+	hdr->tx_freq[1] = 0;
+	hdr->tx_freq[3] = 0;
+
+	hdr->start = 0; /* Start imediatly */
+	hdr->radio_select = 0; /* HACK Radio A transmit */
+	hdr->modulation_type = 0; /* HACK modulation LORA */
+	hdr->tx_power = 15; /* HACK power entry 15 */
+
+	hdr->u.lora.crc16_en = 1; /* Enable CRC16 */
+	hdr->u.lora.cr = 1; /* CR 4/5 */
+	hdr->u.lora.sf = 7; /* SF7 */
+	hdr->u.lora.payload_len = len; /* Set the data len to the skb len */
+	hdr->u.lora.implicit_header = 0; /* No implicit header */
+	hdr->u.lora.mod_bw = 0; /* Set 125KHz BW */
+	hdr->u.lora.ppm_offset = 0; /* TODO no ppm offset? */
+	hdr->u.lora.invert_pol = 0; /* TODO set no inverted polarity */
+
+	hdr->u.lora.preamble = 8; /* Set the standard preamble */
+
+	/* TODO 2 Msb in tx_freq0 for large narrow filtering, unset for now */
+	hdr->tx_freq[0] &= 0x3F;
+
+	/* Copy the TX data into the buffer ready to go */
+
+	memcpy((void *)&buff[16], data, len);
+
+	/* Reset any transmissions */
+	ret = regmap_write(priv->regmap, SX1301_TX_TRIG, 0);
+	if (ret)
+		return ret;
+
+	/* Put the buffer into the tranmit fifo */
+	ret = regmap_write(priv->regmap, SX1301_TX_DATA_BUF_ADDR, 0);
+	if (ret)
+		return ret;
+	ret = regmap_noinc_write(priv->regmap, SX1301_TX_DATA_BUF_DATA, buff,
+				 len + 16);
+	if (ret)
+		return ret;
+
+	/* HACK just go for immediate transfer */
+	ret = sx1301_field_write(priv, F_TX_TRIG_IMMEDIATE, 1);
+	if (ret)
+		return ret;
+
+	dev_dbg(priv->dev, "Transmitting packet of size %d: ", len);
+	for (i = 0; i < len + 16; i++)
+		dev_dbg(priv->dev, "%X", buff[i]);
+
+	return ret;
+}
+
 static netdev_tx_t sx130x_loradev_start_xmit(struct sk_buff *skb,
 					     struct net_device *netdev)
 {
+	struct sx1301_priv *priv = netdev_priv(netdev);
+
 	if (skb->protocol != htons(ETH_P_LORA)) {
 		kfree_skb(skb);
 		netdev->stats.tx_dropped++;
@@ -556,11 +672,42 @@ static netdev_tx_t sx130x_loradev_start_xmit(struct sk_buff *skb,
 	}
 
 	netif_stop_queue(netdev);
+	priv->tx_skb = skb;
+	queue_work(priv->wq, &priv->tx_work);
 
-	/* TODO */
 	return NETDEV_TX_OK;
 }
 
+static void sx1301_tx_work_handler(struct work_struct *ws)
+{
+	struct sx1301_priv *priv = container_of(ws, struct sx1301_priv,
+						tx_work);
+	struct net_device *netdev = dev_get_drvdata(priv->dev);
+	int ret;
+
+	netdev_dbg(netdev, "%s\n", __func__);
+
+	if (priv->tx_skb) {
+		ret = sx1301_tx(priv, priv->tx_skb->data, priv->tx_skb->len);
+		if (ret) {
+			netdev->stats.tx_errors++;
+		} else {
+			netdev->stats.tx_packets++;
+			netdev->stats.tx_bytes += priv->tx_skb->len;
+		}
+
+		if (!(netdev->flags & IFF_ECHO) ||
+		    priv->tx_skb->pkt_type != PACKET_LOOPBACK ||
+		    priv->tx_skb->protocol != htons(ETH_P_LORA))
+			kfree_skb(priv->tx_skb);
+
+		priv->tx_skb = NULL;
+	}
+
+	if (netif_queue_stopped(netdev))
+		netif_wake_queue(netdev);
+}
+
 static int sx130x_loradev_open(struct net_device *netdev)
 {
 	struct sx1301_priv *priv = netdev_priv(netdev);
@@ -676,6 +823,12 @@ static int sx130x_loradev_open(struct net_device *netdev)
 	if (ret)
 		return ret;
 
+	priv->tx_skb = NULL;
+
+	priv->wq = alloc_workqueue("sx1301_wq",
+				   WQ_FREEZABLE | WQ_MEM_RECLAIM, 0);
+	INIT_WORK(&priv->tx_work, sx1301_tx_work_handler);
+
 	netif_start_queue(netdev);
 
 	return 0;
@@ -683,11 +836,22 @@ static int sx130x_loradev_open(struct net_device *netdev)
 
 static int sx130x_loradev_stop(struct net_device *netdev)
 {
+	struct sx1301_priv *priv = netdev_priv(netdev);
+
 	netdev_dbg(netdev, "%s", __func__);
 
 	netif_stop_queue(netdev);
 	close_loradev(netdev);
 
+	destroy_workqueue(priv->wq);
+	priv->wq = NULL;
+
+	if (priv->tx_skb)
+		netdev->stats.tx_errors++;
+	if (priv->tx_skb)
+		dev_kfree_skb(priv->tx_skb);
+	priv->tx_skb = NULL;
+
 	return 0;
 }
 
diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
index 04c9af64c181..86116f3a1fa4 100644
--- a/drivers/net/lora/sx1301.h
+++ b/drivers/net/lora/sx1301.h
@@ -28,6 +28,10 @@
 /* Page independent */
 #define SX1301_PAGE     0x00
 #define SX1301_VER      0x01
+#define SX1301_RX_DATA_BUF_ADDR 0x02 /* 16 wide */
+#define SX1301_RX_DATA_BUF_DATA 0x04
+#define SX1301_TX_DATA_BUF_ADDR 0x05
+#define SX1301_TX_DATA_BUF_DATA 0x06
 #define SX1301_MPA      0x09
 #define SX1301_MPD      0x0A
 #define SX1301_GEN      0x10
@@ -45,6 +49,9 @@
 #define SX1301_FORCE_CTRL   (SX1301_PAGE_BASE(0) + 0x69)
 #define SX1301_MCU_CTRL     (SX1301_PAGE_BASE(0) + 0x6A)
 
+/* Page 1 */
+#define SX1301_TX_TRIG      (SX1301_PAGE_BASE(1) + 0x21)
+
 /* Page 2 */
 #define SX1301_RADIO_A_SPI_DATA     (SX1301_PAGE_BASE(2) + 0x21)
 #define SX1301_RADIO_A_SPI_DATA_RB  (SX1301_PAGE_BASE(2) + 0x22)
@@ -83,6 +90,10 @@ enum sx1301_fields {
 	F_FORCE_DEC_FILTER_GAIN,
 
 	F_EMERGENCY_FORCE_HOST_CTRL,
+
+	F_TX_TRIG_IMMEDIATE,
+	F_TX_TRIG_DELAYED,
+	F_TX_TRIG_GPS,
 };
 
 static const struct reg_field sx1301_regmap_fields[] = {
@@ -108,6 +119,11 @@ static const struct reg_field sx1301_regmap_fields[] = {
 	/* EMERGENCY_FORCE_HOST_CTRL */
 	[F_EMERGENCY_FORCE_HOST_CTRL] =
 		REG_FIELD(SX1301_EMERGENCY_FORCE_HOST_CTRL, 0, 0),
+	/* TX_TRIG */
+	[F_TX_TRIG_IMMEDIATE] = REG_FIELD(SX1301_TX_TRIG, 0, 0),
+	[F_TX_TRIG_DELAYED] = REG_FIELD(SX1301_TX_TRIG, 1, 1),
+	[F_TX_TRIG_GPS] = REG_FIELD(SX1301_TX_TRIG, 2, 2),
+
 };
 
 struct sx1301_tx_gain_lut {
@@ -126,6 +142,11 @@ struct sx1301_priv {
 	struct regmap		*regmap;
 	struct regmap_field *regmap_fields[ARRAY_SIZE(sx1301_regmap_fields)];
 
+	struct sk_buff *tx_skb;
+
+	struct workqueue_struct *wq;
+	struct work_struct tx_work;
+
 	struct sx1301_tx_gain_lut tx_gain_lut[SX1301_TX_GAIN_LUT_MAX];
 	u8 tx_gain_lut_size;
 };
-- 
2.17.1

Re: [PATCH RFC lora-next 3/4] net: lora: sx1301: add minimal to get transmission out

[Andreas Färber]

Am 19.12.18 um 16:56 schrieb Ben Whitten:
> diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
> index 0c7b6d0b31af..9bcbb967f307 100644
> --- a/drivers/net/lora/sx1301.c
> +++ b/drivers/net/lora/sx1301.c
> @@ -24,6 +24,44 @@
>  
>  #include "sx1301.h"
>  
> +struct sx1301_tx_header {
> +	u8	tx_freq[3];
[...]
> +	/* HACK set to 868MHz */
> +	hdr->tx_freq[0] = 217;
> +	hdr->tx_freq[1] = 0;
> +	hdr->tx_freq[3] = 0;

hdr->tx_freq[2]

Regards,
Andreas

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

[PATCH RFC lora-next 4/4] net: lora: sx1301: introduce a lora frame for packet metadata

[Ben Whitten]

Information such as spreading factor, coding rate and power are on a per
transmission basis so it makes sence to include this in a header much
like CAN frames do.

Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
---
 drivers/net/lora/dev.c    |  2 -
 drivers/net/lora/sx1301.c | 79 +++++++++++++++++++++++++++++++++------
 include/uapi/linux/lora.h | 46 +++++++++++++++++++++++
 3 files changed, 114 insertions(+), 13 deletions(-)

diff --git a/drivers/net/lora/dev.c b/drivers/net/lora/dev.c
index 0d4823de8c06..62dbe21668b0 100644
--- a/drivers/net/lora/dev.c
+++ b/drivers/net/lora/dev.c
@@ -11,8 +11,6 @@
 #include <linux/lora/skb.h>
 #include <net/rtnetlink.h>
 
-#define LORA_MTU 256 /* XXX */
-
 struct sk_buff *alloc_lora_skb(struct net_device *dev, u8 **data)
 {
 	struct sk_buff *skb;
diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index 9bcbb967f307..fed6d6201bf3 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -584,7 +584,7 @@ static int sx1301_load_tx_gain_lut(struct sx1301_priv *priv)
 	return ret;
 };
 
-static int sx1301_tx(struct sx1301_priv *priv, void *data, int len)
+static int sx1301_tx(struct sx1301_priv *priv, struct lora_frame *frame)
 {
 	int ret, i;
 	u8 buff[256 + 16];
@@ -617,11 +617,67 @@ static int sx1301_tx(struct sx1301_priv *priv, void *data, int len)
 	hdr->tx_power = 15; /* HACK power entry 15 */
 
 	hdr->u.lora.crc16_en = 1; /* Enable CRC16 */
-	hdr->u.lora.cr = 1; /* CR 4/5 */
-	hdr->u.lora.sf = 7; /* SF7 */
-	hdr->u.lora.payload_len = len; /* Set the data len to the skb len */
+
+	switch (frame->cr) {
+	case LORA_CR_4_5:
+		hdr->u.lora.cr = 1; /* CR 4/5 */
+		break;
+	case LORA_CR_4_6:
+		hdr->u.lora.cr = 2; /* CR 4/6 */
+		break;
+	case LORA_CR_4_7:
+		hdr->u.lora.cr = 3; /* CR 4/7 */
+		break;
+	case LORA_CR_4_8:
+		hdr->u.lora.cr = 4; /* CR 4/8 */
+		break;
+	default:
+		return -ENXIO;
+	}
+
+	switch (frame->sf) {
+	case LORA_SF_6:
+		hdr->u.lora.sf = 6; /* SF6 */
+		break;
+	case LORA_SF_7:
+		hdr->u.lora.sf = 7; /* SF7 */
+		break;
+	case LORA_SF_8:
+		hdr->u.lora.sf = 8; /* SF8 */
+		break;
+	case LORA_SF_9:
+		hdr->u.lora.sf = 9; /* SF9 */
+		break;
+	case LORA_SF_10:
+		hdr->u.lora.sf = 10; /* SF10 */
+		break;
+	case LORA_SF_11:
+		hdr->u.lora.sf = 11; /* SF11 */
+		break;
+	case LORA_SF_12:
+		hdr->u.lora.sf = 12; /* SF12 */
+		break;
+	default:
+		return -ENXIO;
+	}
+
+	hdr->u.lora.payload_len = frame->len; /* Set the data length */
 	hdr->u.lora.implicit_header = 0; /* No implicit header */
-	hdr->u.lora.mod_bw = 0; /* Set 125KHz BW */
+
+	switch (frame->bw) {
+	case LORA_BW_125KHZ:
+		hdr->u.lora.mod_bw = 0; /* 125KHz BW */
+		break;
+	case LORA_BW_250KHZ:
+		hdr->u.lora.mod_bw = 1; /* 250KHz BW */
+		break;
+	case LORA_BW_500KHZ:
+		hdr->u.lora.mod_bw = 2; /* 500KHz BW */
+		break;
+	default:
+		return -ENXIO;
+	}
+
 	hdr->u.lora.ppm_offset = 0; /* TODO no ppm offset? */
 	hdr->u.lora.invert_pol = 0; /* TODO set no inverted polarity */
 
@@ -632,7 +688,7 @@ static int sx1301_tx(struct sx1301_priv *priv, void *data, int len)
 
 	/* Copy the TX data into the buffer ready to go */
 
-	memcpy((void *)&buff[16], data, len);
+	memcpy((void *)&buff[16], frame->data, frame->len);
 
 	/* Reset any transmissions */
 	ret = regmap_write(priv->regmap, SX1301_TX_TRIG, 0);
@@ -644,7 +700,7 @@ static int sx1301_tx(struct sx1301_priv *priv, void *data, int len)
 	if (ret)
 		return ret;
 	ret = regmap_noinc_write(priv->regmap, SX1301_TX_DATA_BUF_DATA, buff,
-				 len + 16);
+				 frame->len + 16);
 	if (ret)
 		return ret;
 
@@ -653,8 +709,8 @@ static int sx1301_tx(struct sx1301_priv *priv, void *data, int len)
 	if (ret)
 		return ret;
 
-	dev_dbg(priv->dev, "Transmitting packet of size %d: ", len);
-	for (i = 0; i < len + 16; i++)
+	dev_dbg(priv->dev, "Transmitting packet of size %d: ", frame->len);
+	for (i = 0; i < frame->len + 16; i++)
 		dev_dbg(priv->dev, "%X", buff[i]);
 
 	return ret;
@@ -683,17 +739,18 @@ static void sx1301_tx_work_handler(struct work_struct *ws)
 	struct sx1301_priv *priv = container_of(ws, struct sx1301_priv,
 						tx_work);
 	struct net_device *netdev = dev_get_drvdata(priv->dev);
+	struct lora_frame *frame = (struct lora_frame *)priv->tx_skb->data;
 	int ret;
 
 	netdev_dbg(netdev, "%s\n", __func__);
 
 	if (priv->tx_skb) {
-		ret = sx1301_tx(priv, priv->tx_skb->data, priv->tx_skb->len);
+		ret = sx1301_tx(priv, frame);
 		if (ret) {
 			netdev->stats.tx_errors++;
 		} else {
 			netdev->stats.tx_packets++;
-			netdev->stats.tx_bytes += priv->tx_skb->len;
+			netdev->stats.tx_bytes += frame->len;
 		}
 
 		if (!(netdev->flags & IFF_ECHO) ||
diff --git a/include/uapi/linux/lora.h b/include/uapi/linux/lora.h
index 4ff00b9c3c20..d675025d2a6e 100644
--- a/include/uapi/linux/lora.h
+++ b/include/uapi/linux/lora.h
@@ -21,4 +21,50 @@ struct sockaddr_lora {
 	} lora_addr;
 };
 
+#define LORA_MAX_DLEN	256
+
+enum lora_bw {
+	LORA_BW_125KHZ,
+	LORA_BW_250KHZ,
+	LORA_BW_500KHZ,
+};
+
+enum lora_cr {
+	LORA_CR_4_5,
+	LORA_CR_4_6,
+	LORA_CR_4_7,
+	LORA_CR_4_8,
+};
+
+enum lora_sf {
+	LORA_SF_6,
+	LORA_SF_7,
+	LORA_SF_8,
+	LORA_SF_9,
+	LORA_SF_10,
+	LORA_SF_11,
+	LORA_SF_12,
+};
+
+/**
+ * struct lora_frame - LoRa frame structure
+ * @freq: Frequency of LoRa transmission in Hz
+ * @power: Power of transmission in dBm
+ * @bw:  bandwidth, 125, 250 or 500 KHz
+ * @cr:  coding rate, 4/5 to 4/8
+ * @sf:  spreading factor from SF6 to 12
+ * @data:   LoRa frame payload (up to LORA_MAX_DLEN byte)
+ */
+struct lora_frame {
+	__u32		freq; /* transmission frequency in Hz */
+	__u8		power; /* transmission power in dBm */
+	enum lora_bw	bw; /* bandwidth */
+	enum lora_cr	cr; /* coding rate */
+	enum lora_sf	sf; /* spreading factor */
+	__u8		len;
+	__u8		data[LORA_MAX_DLEN] __attribute__((aligned(8)));
+};
+
+#define LORA_MTU (sizeof(struct lora_frame))
+
 #endif /* _UAPI_LINUX_LORA_H */
-- 
2.17.1

Re: [PATCH RFC lora-next 4/4] net: lora: sx1301: introduce a lora frame for packet metadata

[Andreas Färber]

Hi Ben,

Am 19.12.18 um 16:56 schrieb Ben Whitten:
> Information such as spreading factor, coding rate and power are on a per
> transmission basis so it makes sence to include this in a header much

"sense" (even in British English! :))

> like CAN frames do.

Any pointer for that? My understanding of CAN was that it actually uses
the CAN or CAN FD frame for transmission on the wire.

Whereas here you use it for metadata that does not go over the air.

>
> Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
> ---
>  drivers/net/lora/dev.c    |  2 -
>  drivers/net/lora/sx1301.c | 79 +++++++++++++++++++++++++++++++++------
>  include/uapi/linux/lora.h | 46 +++++++++++++++++++++++
>  3 files changed, 114 insertions(+), 13 deletions(-)

If we should seriously consider this new approach, please always cleanly
split it off from sx1301 driver changes. The most important changes are
at the very bottom here otherwise.

Some of the enums may be useful either way.

[snip]
> diff --git a/include/uapi/linux/lora.h b/include/uapi/linux/lora.h
> index 4ff00b9c3c20..d675025d2a6e 100644
> --- a/include/uapi/linux/lora.h
> +++ b/include/uapi/linux/lora.h
> @@ -21,4 +21,50 @@ struct sockaddr_lora {
>  	} lora_addr;
>  };
>  
> +#define LORA_MAX_DLEN	256

Note: According to Helmut, "SX1276 can do MTU sizes up to 2048 bytes".
But I have failed to find mention of LoRa-mode interrupts other than
TxDone or any such how-to in the SX1276 manual or on Google; only the
FSK/OOK modem has a FIFO-less Continuous mode documented.

> +
> +enum lora_bw {
> +	LORA_BW_125KHZ,
> +	LORA_BW_250KHZ,
> +	LORA_BW_500KHZ,

Lacking lower bandwidths. SX127x can go as low as 7.8 kHz.

Would it work to have it as an integer in Hz for flexibility?

> +};
> +
> +enum lora_cr {
> +	LORA_CR_4_5,
> +	LORA_CR_4_6,
> +	LORA_CR_4_7,
> +	LORA_CR_4_8,
> +};

If we have this as ABI, we should probably go safe and initialize the
first one to 0.

> +
> +enum lora_sf {
> +	LORA_SF_6,
> +	LORA_SF_7,
> +	LORA_SF_8,
> +	LORA_SF_9,
> +	LORA_SF_10,
> +	LORA_SF_11,
> +	LORA_SF_12,
> +};

Wouldn't an integer be sufficient for the Spreading Factor?
We'll need to safeguard code against unexpected values anyway.

An added bonus for the enum/defines would be if we could have
LORA_SF_6 = 64
up to
LORA_SF_12 = 4096
(chips per symbol)

> +
> +/**
> + * struct lora_frame - LoRa frame structure
> + * @freq: Frequency of LoRa transmission in Hz
> + * @power: Power of transmission in dBm
> + * @bw:  bandwidth, 125, 250 or 500 KHz
> + * @cr:  coding rate, 4/5 to 4/8
> + * @sf:  spreading factor from SF6 to 12
> + * @data:   LoRa frame payload (up to LORA_MAX_DLEN byte)
> + */
> +struct lora_frame {
> +	__u32		freq; /* transmission frequency in Hz */
> +	__u8		power; /* transmission power in dBm */
> +	enum lora_bw	bw; /* bandwidth */
> +	enum lora_cr	cr; /* coding rate */
> +	enum lora_sf	sf; /* spreading factor */
> +	__u8		len;
> +	__u8		data[LORA_MAX_DLEN] __attribute__((aligned(8)));
> +};

I'm not comfortable with making this arbitrary format a userspace ABI.

For example, hardcoding the frequency as __u32 will limit us to 4 GHz,
which is sufficient for 2.4 GHz, but Wifi is already at 5 and 60 GHz.
Having the data at the end means we can't easily add header fields, such
as Sync Word (that you're missing above) or a frequency extension word.

Reuse with FSK/OOK or other LPWAN technologies could be a bonus goal.

In my presentation I had suggested to use the socket address for storing
most of that metadata. The counter-proposal was to use netlink for any
global setting changes and have the socket just send the actual data.

Reminder: We can set SX127x registers before each TX, but RX remained
unsolved. For SX130x we can set some metadata per TX (multi-channel),
but radio/channel need to have been configured appropriately. Wifi cards
may support MIMO but still show up as one wlan0 netdev, so it seemed
questionable to diverge for SX130x by having ~10 netdevs and impractical
since we have a lot of radio initializations in .ndo_open.

Compare slides 16 ff. vs. slide 27:
https://events.linuxfoundation.org/wp-content/uploads/2017/12/ELCE2018_LoRa_final_Andreas-Farber.pdf

Ultimately the skb is where we need any per-transaction metadata, and
outside uapi/ I'm much less concerned about ABI changes. So from my
perspective such fields would go into linux/lora/skb.h struct lora_priv
after the ifindex, if we go with PF_LORA. As mentioned before, if we
have to go with PF_PACKET then I'm clueless where to store such data...

> +
> +#define LORA_MTU (sizeof(struct lora_frame))

Even if Helmut's comment was mistaken, it doesn't strike me as a good
idea to hardcode this here - you could just use data[0] and leave actual
MTU to the driver/user.

> +
>  #endif /* _UAPI_LINUX_LORA_H */

Cheers,
Andreas

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