[PATCH lora-next 00/10] Conversing sx1301 to regmap and regfield

[PATCH lora-next 00/10] Conversing sx1301 to regmap and regfield

[Ben Whitten]

This series converts the majority of sx1301 code to using regmap and regfield
system.
The SPI subsystem is as yet untouched but will be converted to a regmap_bus
in a future series with sx1257 conversions.

Interestingly the initial state did not work for me as I was getting kernel
oopses somewhere todo with sx1301_page_read on my hardware.
Since conversion to regmap the sx1301 initialises properly, sx1257 still oopses
because of the SPI bus read but presumably will go away with regmap_bus.

Ben Whitten (10):
  net: lora: sx1301: add register, bit-fields, and helpers for regmap
  net: lora: add methods for devm registration
  net: lora: sx1301: convert to devm registration of netdev
  net: lora: sx1301: convert probe function to regmap access
  net: lora: sx1301: add device to private data and convert ram reads
  net: lora: sx1301: simplify firmware loading and convert
  net: lora: sx1301: convert read and write burst to take priv data
  net: lora: sx1301: convert read and write to priv pointer
  net: lora: sx1301: convert agc calibrate to regmap functions
  net: lora: sx1301: convert all firmware to regmap

 drivers/net/lora/Kconfig  |   1 +
 drivers/net/lora/dev.c    |  20 ++
 drivers/net/lora/sx1301.c | 803 ++++++++++++++++++++++++----------------------
 drivers/net/lora/sx1301.h | 176 ++++++++++
 include/linux/lora/dev.h  |   1 +
 5 files changed, 623 insertions(+), 378 deletions(-)
 create mode 100644 drivers/net/lora/sx1301.h

-- 
2.7.4

[PATCH lora-next 01/10] net: lora: sx1301: add register, bit-fields, and helpers for regmap

[Ben Whitten]

From: Ben Whitten <ben.whitten@lairdtech.com>

The register and bit-field definitions are taken from the SX1301
datasheet version 2.01 dated June 2014 with the revision information
'First released version'.

The reset state and RW capability of each field is not reflected in this
patch however from the datasheet:
"Bits and registers that are not documented are reserved. They may
include calibration values. It is important not to modify these bits and
registers. If specific bits must be changed in a register with reserved
bits, the register must be read first, specific bits modified while
masking reserved bits and then the register can be written."

Then goes on to state:
"Reserved bits should be written with their reset state, they may be
read different states."

Caching is currently disabled.

The version is read back using regmap_read to verify regmap operation,
in doing so needs to be moved after priv and regmap allocation.

Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
---
 drivers/net/lora/Kconfig  |   1 +
 drivers/net/lora/sx1301.c | 282 +++++++++++++++++++++++++++++++++++++++++++---
 drivers/net/lora/sx1301.h | 169 +++++++++++++++++++++++++++
 3 files changed, 439 insertions(+), 13 deletions(-)
 create mode 100644 drivers/net/lora/sx1301.h

diff --git a/drivers/net/lora/Kconfig b/drivers/net/lora/Kconfig
index bb57a01..79d23f2 100644
--- a/drivers/net/lora/Kconfig
+++ b/drivers/net/lora/Kconfig
@@ -49,6 +49,7 @@ config LORA_SX1301
 	tristate "Semtech SX1301 SPI driver"
 	default y
 	depends on SPI
+	select REGMAP_SPI
 	help
 	  Semtech SX1301
 
diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index 5342b61..49958f0 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -3,6 +3,7 @@
  * Semtech SX1301 LoRa concentrator
  *
  * Copyright (c) 2018 Andreas Färber
+ * Copyright (c) 2018 Ben Whitten
  *
  * Based on SX1301 HAL code:
  * Copyright (c) 2013 Semtech-Cycleo
@@ -19,6 +20,9 @@
 #include <linux/of_gpio.h>
 #include <linux/lora/dev.h>
 #include <linux/spi/spi.h>
+#include <linux/regmap.h>
+
+#include "sx1301.h"
 
 #define REG_PAGE_RESET			0
 #define REG_VERSION			1
@@ -65,6 +69,213 @@
 
 #define REG_EMERGENCY_FORCE_HOST_CTRL	BIT(0)
 
+enum sx1301_fields {
+	F_SOFT_RESET,
+	F_START_BIST0,
+	F_START_BIST1,
+	F_BIST0_FINISHED,
+	F_BIST1_FINISHED,
+	F_GLOBAL_EN,
+	F_CLK32M_EN,
+	F_RADIO_A_EN,
+	F_RADIO_B_EN,
+	F_RADIO_RST,
+
+	F_RX_INVERT_IQ,
+	F_MODEM_INVERT_IQ,
+	F_MBWSSF_MODEM_INVERT_IQ,
+	F_RX_EDGE_SELECT,
+	F_MISC_RADIO_EN,
+	F_FSK_MODEM_INVERT_IQ,
+
+	F_RSSI_BB_FILTER_ALPHA,
+	F_RSSI_DEC_FILTER_ALPHA,
+	F_RSSI_CHANN_FILTER_ALPHA,
+
+	F_DEC_GAIN_OFFSET,
+	F_CHAN_GAIN_OFFSET,
+
+	F_LLR_SCALE,
+	F_SNR_AVG_CST,
+
+	F_CORR_NUM_SAME_PEAK,
+	F_CORR_MAC_GAIN,
+
+	F_FSK_CH_BW_EXPO,
+	F_FSK_RSSI_LENGTH,
+	F_FSK_RX_INVERT,
+	F_FSK_PKT_MODE,
+
+	F_FSK_PSIZE,
+	F_FSK_CRC_EN,
+	F_FSK_DCFREE_ENC,
+	F_FSK_CRC_IBM,
+
+	F_FSK_ERROR_OSR_TOL,
+	F_FSK_RADIO_SELECT,
+
+	F_TX_MODE,
+	F_TX_ZERO_PAD,
+	F_TX_EDGE_SELECT,
+	F_TX_EDGE_SELECT_TOP,
+
+	F_TX_GAIN,
+	F_TX_CHIRP_LOW_PASS,
+	F_TX_FCC_WIDEBAND,
+	F_TX_SWAP_IQ,
+
+	F_FSK_TX_GAUSSIAN_EN,
+	F_FSK_TX_GAUSSIAN_SELECT_BT,
+	F_FSK_TX_PATTERN_EN,
+	F_FSK_TX_PREAMBLE_SEQ,
+	F_FSK_TX_PSIZE,
+
+	F_FORCE_HOST_RADIO_CTRL,
+	F_FORCE_HOST_FE_CTRL,
+	F_FORCE_DEC_FILTER_GAIN,
+
+	F_MCU_RST_0,
+	F_MCU_RST_1,
+	F_MCU_SELECT_MUX_0,
+	F_MCU_SELECT_MUX_1,
+	F_MCU_CORRUPTION_DETECTED_0,
+	F_MCU_CORRUPTION_DETECTED_1,
+	F_MCU_SELECT_EDGE_0,
+	F_MCU_SELECT_EDGE_1,
+
+	F_EMERGENCY_FORCE_HOST_CTRL,
+};
+
+static const struct reg_field sx1301_reg_fields[] = {
+	/* PAGE */
+	[F_SOFT_RESET]		= REG_FIELD(SX1301_PAGE, 7, 7),
+	/* BIST */
+	[F_START_BIST0]		= REG_FIELD(SX1301_BIST, 0, 0),
+	[F_START_BIST1]		= REG_FIELD(SX1301_BIST, 1, 1),
+	/* BIST_S */
+	[F_BIST0_FINISHED]	= REG_FIELD(SX1301_BIST_S, 0, 0),
+	[F_BIST1_FINISHED]	= REG_FIELD(SX1301_BIST_S, 1, 1),
+	/* GEN */
+	[F_GLOBAL_EN]		= REG_FIELD(SX1301_GEN,  3, 3),
+	/* CKEN */
+	[F_CLK32M_EN]		= REG_FIELD(SX1301_CKEN, 0, 0),
+	/* RADIO_CFG */
+	[F_RADIO_A_EN]		= REG_FIELD(SX1301_RADIO_CFG, 0, 0),
+	[F_RADIO_B_EN]		= REG_FIELD(SX1301_RADIO_CFG, 1, 1),
+	[F_RADIO_RST]		= REG_FIELD(SX1301_RADIO_CFG, 2, 2),
+
+	/* IQCFG */
+	[F_RX_INVERT_IQ]	= REG_FIELD(SX1301_IQCFG, 0, 0),
+	[F_MODEM_INVERT_IQ]	= REG_FIELD(SX1301_IQCFG, 1, 1),
+	[F_MBWSSF_MODEM_INVERT_IQ]	= REG_FIELD(SX1301_IQCFG, 2, 2),
+	[F_RX_EDGE_SELECT]	= REG_FIELD(SX1301_IQCFG, 3, 3),
+	[F_MISC_RADIO_EN]	= REG_FIELD(SX1301_IQCFG, 4, 4),
+	[F_FSK_MODEM_INVERT_IQ]	= REG_FIELD(SX1301_IQCFG, 5, 5),
+
+	/* RSSI_X_FILTER_ALPHA */
+	[F_RSSI_BB_FILTER_ALPHA] =
+		REG_FIELD(SX1301_RSSI_BB_FILTER_ALPHA, 0, 4),
+	[F_RSSI_DEC_FILTER_ALPHA] =
+		REG_FIELD(SX1301_RSSI_DEC_FILTER_ALPHA, 0, 4),
+	[F_RSSI_CHANN_FILTER_ALPHA] =
+		REG_FIELD(SX1301_RSSI_CHANN_FILTER_ALPHA, 0, 4),
+
+	/* GAIN_OFFSET */
+	[F_DEC_GAIN_OFFSET]	= REG_FIELD(SX1301_GAIN_OFFSET, 0, 3),
+	[F_CHAN_GAIN_OFFSET]	= REG_FIELD(SX1301_GAIN_OFFSET, 4, 7),
+
+	/* MISC_CFG1 */
+	[F_LLR_SCALE]		= REG_FIELD(SX1301_MISC_CFG1, 0, 3),
+	[F_SNR_AVG_CST]		= REG_FIELD(SX1301_MISC_CFG1, 4, 5),
+
+	/* CORR_CFG */
+	[F_CORR_NUM_SAME_PEAK]	= REG_FIELD(SX1301_CORR_CFG, 0, 3),
+	[F_CORR_MAC_GAIN]	= REG_FIELD(SX1301_CORR_CFG, 4, 6),
+
+	/* FSK_CFG1 */
+	[F_FSK_CH_BW_EXPO]	= REG_FIELD(SX1301_FSK_CFG1, 0, 2),
+	[F_FSK_RSSI_LENGTH]	= REG_FIELD(SX1301_FSK_CFG1, 3, 5),
+	[F_FSK_RX_INVERT]	= REG_FIELD(SX1301_FSK_CFG1, 6, 6),
+	[F_FSK_PKT_MODE]	= REG_FIELD(SX1301_FSK_CFG1, 7, 7),
+
+	/* FSK_CFG2 */
+	[F_FSK_PSIZE]		= REG_FIELD(SX1301_FSK_CFG2, 0, 2),
+	[F_FSK_CRC_EN]		= REG_FIELD(SX1301_FSK_CFG2, 3, 3),
+	[F_FSK_DCFREE_ENC]	= REG_FIELD(SX1301_FSK_CFG2, 4, 5),
+	[F_FSK_CRC_IBM]		= REG_FIELD(SX1301_FSK_CFG2, 6, 6),
+
+	/* FSK_ERROR_OSR_TOL */
+	[F_FSK_ERROR_OSR_TOL]	= REG_FIELD(SX1301_FSK_ERROR_OSR_TOL, 0, 4),
+	[F_FSK_RADIO_SELECT]	= REG_FIELD(SX1301_FSK_ERROR_OSR_TOL, 7, 7),
+
+	/* TX_CFG1 */
+	[F_TX_MODE]		= REG_FIELD(SX1301_TX_CFG1, 0, 0),
+	[F_TX_ZERO_PAD]		= REG_FIELD(SX1301_TX_CFG1, 1, 4),
+	[F_TX_EDGE_SELECT]	= REG_FIELD(SX1301_TX_CFG1, 5, 5),
+	[F_TX_EDGE_SELECT_TOP]	= REG_FIELD(SX1301_TX_CFG1, 6, 6),
+
+	/* TX_CFG2 */
+	[F_TX_GAIN]		= REG_FIELD(SX1301_TX_CFG2, 0, 1),
+	[F_TX_CHIRP_LOW_PASS]	= REG_FIELD(SX1301_TX_CFG2, 2, 4),
+	[F_TX_FCC_WIDEBAND]	= REG_FIELD(SX1301_TX_CFG2, 5, 6),
+	[F_TX_SWAP_IQ]		= REG_FIELD(SX1301_TX_CFG2, 7, 7),
+
+	/* FSK_TX */
+	[F_FSK_TX_GAUSSIAN_EN]	= REG_FIELD(SX1301_FSK_TX, 0, 0),
+	[F_FSK_TX_GAUSSIAN_SELECT_BT]	= REG_FIELD(SX1301_FSK_TX, 1, 2),
+	[F_FSK_TX_PATTERN_EN]	= REG_FIELD(SX1301_FSK_TX, 3, 3),
+	[F_FSK_TX_PREAMBLE_SEQ]	= REG_FIELD(SX1301_FSK_TX, 4, 4),
+	[F_FSK_TX_PSIZE]	= REG_FIELD(SX1301_FSK_TX, 5, 7),
+
+	/* FORCE_CTRL */
+	[F_FORCE_HOST_RADIO_CTRL] = REG_FIELD(SX1301_FORCE_CTRL, 1, 1),
+	[F_FORCE_HOST_FE_CTRL]    = REG_FIELD(SX1301_FORCE_CTRL, 2, 2),
+	[F_FORCE_DEC_FILTER_GAIN] = REG_FIELD(SX1301_FORCE_CTRL, 3, 3),
+
+	/* MCU_CTRL */
+	[F_MCU_RST_0]		= REG_FIELD(SX1301_MCU_CTRL, 0, 0),
+	[F_MCU_RST_1]		= REG_FIELD(SX1301_MCU_CTRL, 1, 1),
+	[F_MCU_SELECT_MUX_0]	= REG_FIELD(SX1301_MCU_CTRL, 2, 2),
+	[F_MCU_SELECT_MUX_1]	= REG_FIELD(SX1301_MCU_CTRL, 3, 3),
+	[F_MCU_CORRUPTION_DETECTED_0] = REG_FIELD(SX1301_MCU_CTRL, 4, 4),
+	[F_MCU_CORRUPTION_DETECTED_1] = REG_FIELD(SX1301_MCU_CTRL, 5, 5),
+	[F_MCU_SELECT_EDGE_0]	= REG_FIELD(SX1301_MCU_CTRL, 6, 6),
+	[F_MCU_SELECT_EDGE_1]	= REG_FIELD(SX1301_MCU_CTRL, 7, 7),
+
+	/* EMERGENCY_FORCE_HOST_CTRL */
+	[F_EMERGENCY_FORCE_HOST_CTRL] =
+		REG_FIELD(SX1301_EMERGENCY_FORCE_HOST_CTRL, 0, 0),
+};
+
+static const struct regmap_range_cfg sx1301_ranges[] = {
+	{
+		.name = "Pages",
+
+		.range_min = SX1301_VIRT_BASE,
+		.range_max = SX1301_MAX_REGISTER,
+
+		.selector_reg = SX1301_PAGE,
+		.selector_mask = 0x3,
+
+		.window_start = 0,
+		.window_len = SX1301_PAGE_LEN,
+	},
+};
+
+static struct regmap_config sx1301_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.cache_type = REGCACHE_NONE,
+
+	.read_flag_mask = 0,
+	.write_flag_mask = BIT(7),
+
+	.ranges = sx1301_ranges,
+	.num_ranges = ARRAY_SIZE(sx1301_ranges),
+	.max_register = SX1301_MAX_REGISTER,
+};
+
 struct spi_sx1301 {
 	struct spi_device *parent;
 	u8 page;
@@ -76,8 +287,29 @@ struct sx1301_priv {
 	struct gpio_desc *rst_gpio;
 	u8 cur_page;
 	struct spi_controller *radio_a_ctrl, *radio_b_ctrl;
+	struct regmap		*regmap;
+	struct regmap_field	*regmap_fields[ARRAY_SIZE(sx1301_reg_fields)];
 };
 
+static int sx1301_field_read(struct sx1301_priv *priv,
+		enum sx1301_fields field_id)
+{
+	int ret;
+	int val;
+
+	ret = regmap_field_read(priv->regmap_fields[field_id], &val);
+	if (ret)
+		return ret;
+
+	return val;
+}
+
+static int sx1301_field_write(struct sx1301_priv *priv,
+		enum sx1301_fields field_id, u8 val)
+{
+	return regmap_field_write(priv->regmap_fields[field_id], val);
+}
+
 static int sx1301_read_burst(struct spi_device *spi, u8 reg, u8 *val, size_t len)
 {
 	u8 addr = reg & 0x7f;
@@ -620,6 +852,8 @@ static int sx1301_probe(struct spi_device *spi)
 	struct spi_sx1301 *radio;
 	struct gpio_desc *rst;
 	int ret;
+	int i;
+	unsigned int ver;
 	u8 val;
 
 	rst = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
@@ -634,18 +868,6 @@ static int sx1301_probe(struct spi_device *spi)
 	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;
@@ -659,6 +881,38 @@ static int sx1301_probe(struct spi_device *spi)
 	spi_set_drvdata(spi, netdev);
 	SET_NETDEV_DEV(netdev, &spi->dev);
 
+	priv->regmap = devm_regmap_init_spi(spi, &sx1301_regmap_config);
+	if (IS_ERR(priv->regmap)) {
+		ret = PTR_ERR(priv->regmap);
+		dev_err(&spi->dev, "Regmap allocation failed: %d\n", ret);
+		return err_regmap;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(sx1301_reg_fields); i++) {
+		const struct reg_field *reg_fields = sx1301_reg_fields;
+
+		priv->regmap_fields[i] = devm_regmap_field_alloc(&spi->dev,
+				priv->regmap,
+				reg_fields[i]);
+		if (IS_ERR(priv->regmap_fields[i])) {
+			ret = PTR_ERR(priv->regmap_fields[i]);
+			dev_err(&spi->dev, "Cannot allocate regmap field: %d\n", ret);
+			goto err_regmap;
+		}
+	}
+
+	ret = regmap_read(priv->regmap, SX1301_VER, &ver);
+	if (ret) {
+		dev_err(&spi->dev, "version read failed\n");
+		goto err_version;
+	}
+
+	if (ver != 103) {
+		dev_err(&spi->dev, "unexpected version: %u\n", ver);
+		ret = -ENXIO;
+		goto err_version;
+	}
+
 	ret = sx1301_write(spi, REG_PAGE_RESET, 0);
 	if (ret) {
 		dev_err(&spi->dev, "page/reset write failed\n");
@@ -888,9 +1142,10 @@ static int sx1301_probe(struct spi_device *spi)
 err_read_global_en_0:
 err_soft_reset:
 err_init_page:
+err_version:
+err_regmap:
 	free_loradev(netdev);
 err_alloc_loradev:
-err_version:
 	return ret;
 }
 
@@ -927,4 +1182,5 @@ module_spi_driver(sx1301_spi_driver);
 
 MODULE_DESCRIPTION("SX1301 SPI driver");
 MODULE_AUTHOR("Andreas Färber <afaerber@suse.de>");
+MODULE_AUTHOR("Ben Whitten <ben.whitten@gmail.com>");
 MODULE_LICENSE("GPL");
diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
new file mode 100644
index 0000000..abfb7b5
--- /dev/null
+++ b/drivers/net/lora/sx1301.h
@@ -0,0 +1,169 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Semtech SX1301 lora concentrator
+ *
+ * Copyright (c) 2018   Ben Whitten
+ */
+
+#ifndef _SX1301_
+#define _SX1301_
+
+/* Page independent */
+#define SX1301_PAGE     0x00
+#define SX1301_VER      0x01
+#define SX1301_RDBAL    0x02
+#define SX1301_RDBAH    0x03
+#define SX1301_RDBD     0x04
+#define SX1301_TDBA     0x05
+#define SX1301_TDBD     0x06
+#define SX1301_MPA      0x09
+#define SX1301_MPD      0x0A
+#define SX1301_RPNS     0x0B
+#define SX1301_RPAPL    0x0C
+#define SX1301_RPAPH    0x0D
+#define SX1301_RPS      0x0E
+#define SX1301_RPPS     0x0F
+#define SX1301_GEN      0x10
+#define SX1301_CKEN     0x11
+#define SX1301_BIST     0x12
+#define SX1301_BIST_S   0x13
+#define SX1301_GPSI     0x1B
+#define SX1301_GPSO     0x1C
+#define SX1301_GPMODE   0x1D
+#define SX1301_GPREGI   0x1E
+#define SX1301_GPREGO   0x1F
+#define SX1301_AGCSTS   0x20
+#define SX1301_ARBSTS   0x7D
+#define SX1301_ID       0x7F
+
+#define SX1301_VIRT_BASE    0x100
+#define SX1301_PAGE_LEN     0x80
+#define SX1301_PAGE_BASE(n) (SX1301_VIRT_BASE + (SX1301_PAGE_LEN * n))
+
+/* Page 0 */
+#define SX1301_IQCFG        (SX1301_PAGE_BASE(0) + 0x21)
+#define SX1301_DECCFG       (SX1301_PAGE_BASE(0) + 0x22)
+#define SX1301_CHRS         (SX1301_PAGE_BASE(0) + 0x23)
+#define SX1301_IF0L         (SX1301_PAGE_BASE(0) + 0x24)
+#define SX1301_IF0H         (SX1301_PAGE_BASE(0) + 0x25)
+#define SX1301_IF1L         (SX1301_PAGE_BASE(0) + 0x26)
+#define SX1301_IF1H         (SX1301_PAGE_BASE(0) + 0x27)
+#define SX1301_IF2L         (SX1301_PAGE_BASE(0) + 0x28)
+#define SX1301_IF2H         (SX1301_PAGE_BASE(0) + 0x29)
+#define SX1301_IF3L         (SX1301_PAGE_BASE(0) + 0x2A)
+#define SX1301_IF3H         (SX1301_PAGE_BASE(0) + 0x2B)
+#define SX1301_IF4L         (SX1301_PAGE_BASE(0) + 0x2C)
+#define SX1301_IF4H         (SX1301_PAGE_BASE(0) + 0x2D)
+#define SX1301_IF5L         (SX1301_PAGE_BASE(0) + 0x2E)
+#define SX1301_IF5H         (SX1301_PAGE_BASE(0) + 0x2F)
+#define SX1301_IF6L         (SX1301_PAGE_BASE(0) + 0x30)
+#define SX1301_IF6H         (SX1301_PAGE_BASE(0) + 0x31)
+#define SX1301_IF7L         (SX1301_PAGE_BASE(0) + 0x32)
+#define SX1301_IF7H         (SX1301_PAGE_BASE(0) + 0x33)
+#define SX1301_IF8L         (SX1301_PAGE_BASE(0) + 0x34)
+#define SX1301_IF8H         (SX1301_PAGE_BASE(0) + 0x35)
+#define SX1301_IF9L         (SX1301_PAGE_BASE(0) + 0x36)
+#define SX1301_IF9H         (SX1301_PAGE_BASE(0) + 0x37)
+#define SX1301_COR0DETEN    (SX1301_PAGE_BASE(0) + 0x41)
+#define SX1301_COR1DETEN    (SX1301_PAGE_BASE(0) + 0x42)
+#define SX1301_COR2DETEN    (SX1301_PAGE_BASE(0) + 0x43)
+#define SX1301_COR3DETEN    (SX1301_PAGE_BASE(0) + 0x44)
+#define SX1301_COR4DETEN    (SX1301_PAGE_BASE(0) + 0x45)
+#define SX1301_COR5DETEN    (SX1301_PAGE_BASE(0) + 0x46)
+#define SX1301_COR6DETEN    (SX1301_PAGE_BASE(0) + 0x47)
+#define SX1301_COR7DETEN    (SX1301_PAGE_BASE(0) + 0x48)
+#define SX1301_CORR_CFG	    (SX1301_PAGE_BASE(0) + 0x4E)
+#define SX1301_MODEM_START_RDX4_L (SX1301_PAGE_BASE(0) + 0x51)
+#define SX1301_MODEM_START_RDX4_H (SX1301_PAGE_BASE(0) + 0x52)
+#define SX1301_MODEM_START_SF12_RDX4_L (SX1301_PAGE_BASE(0) + 0x53)
+#define SX1301_MODEM_START_SF12_RDX4_H (SX1301_PAGE_BASE(0) + 0x54)
+#define SX1301_TIMTRAK2     (SX1301_PAGE_BASE(0) + 0x5D)
+#define SX1301_PRSYMBNBL    (SX1301_PAGE_BASE(0) + 0x60)
+#define SX1301_PRSYMBNBH    (SX1301_PAGE_BASE(0) + 0x61)
+#define SX1301_MISC_CFG1    (SX1301_PAGE_BASE(0) + 0x63)
+#define SX1301_MISC_CFG2    (SX1301_PAGE_BASE(0) + 0x64)
+#define SX1301_HEADER_CFG1  (SX1301_PAGE_BASE(0) + 0x65)
+#define SX1301_HEADER_CFG2  (SX1301_PAGE_BASE(0) + 0x66)
+#define SX1301_GAIN_OFFSET  (SX1301_PAGE_BASE(0) + 0x68)
+#define SX1301_FORCE_CTRL   (SX1301_PAGE_BASE(0) + 0x69)
+#define SX1301_MCU_CTRL     (SX1301_PAGE_BASE(0) + 0x6A)
+#define SX1301_CHANN_SELECT_RSSI	(SX1301_PAGE_BASE(0) + 0x6B)
+#define SX1301_RSSI_BB_DEFAULT_VALUE	(SX1301_PAGE_BASE(0) + 0x6C)
+#define SX1301_RSSI_DEC_DEFAULT_VALUE	(SX1301_PAGE_BASE(0) + 0x6D)
+#define SX1301_RSSI_CHANN_DEFAULT_VALUE (SX1301_PAGE_BASE(0) + 0x6E)
+#define SX1301_RSSI_BB_FILTER_ALPHA	(SX1301_PAGE_BASE(0) + 0x6F)
+#define SX1301_RSSI_DEC_FILTER_ALPHA	(SX1301_PAGE_BASE(0) + 0x70)
+#define SX1301_RSSI_CHANN_FILTER_ALPHA	(SX1301_PAGE_BASE(0) + 0x71)
+
+/* Page 1 */
+#define SX1301_TXTRIG               (SX1301_PAGE_BASE(1) + 0x21)
+#define SX1301_TX_OFFSET_I          (SX1301_PAGE_BASE(1) + 0x27)
+#define SX1301_TX_OFFSET_Q          (SX1301_PAGE_BASE(1) + 0x28)
+#define SX1301_TX_CFG1              (SX1301_PAGE_BASE(1) + 0x29)
+#define SX1301_TX_CFG2              (SX1301_PAGE_BASE(1) + 0x2A)
+#define SX1301_BHIMPCFG1            (SX1301_PAGE_BASE(1) + 0x2B)
+#define SX1301_BHIMPCFG2            (SX1301_PAGE_BASE(1) + 0x2C)
+#define SX1301_BHSYNCPOS            (SX1301_PAGE_BASE(1) + 0x2E)
+#define SX1301_BHPRSYMNBL           (SX1301_PAGE_BASE(1) + 0x2F)
+#define SX1301_MBWSSF_MISC_CFG1     (SX1301_PAGE_BASE(1) + 0x3A)
+#define SX1301_MBWSSF_MISC_CFG2     (SX1301_PAGE_BASE(1) + 0x3B)
+#define SX1301_MBWSSF_MISC_CFG3     (SX1301_PAGE_BASE(1) + 0x3C)
+#define SX1301_MBWSSF_MISC_CFG4     (SX1301_PAGE_BASE(1) + 0x3D)
+#define SX1301_TX_STATUS            (SX1301_PAGE_BASE(1) + 0x3E)
+#define SX1301_FSK_CFG1             (SX1301_PAGE_BASE(1) + 0x3F)
+#define SX1301_FSK_CFG2             (SX1301_PAGE_BASE(1) + 0x40)
+#define SX1301_FSK_ERROR_OSR_TOL    (SX1301_PAGE_BASE(1) + 0x41)
+#define SX1301_FSK_BR_RATIOL        (SX1301_PAGE_BASE(1) + 0x42)
+#define SX1301_FSK_BR_RATIOH        (SX1301_PAGE_BASE(1) + 0x43)
+#define SX1301_FSK_REF_PATTERN_0    (SX1301_PAGE_BASE(1) + 0x44)
+#define SX1301_FSK_REF_PATTERN_1    (SX1301_PAGE_BASE(1) + 0x45)
+#define SX1301_FSK_REF_PATTERN_2    (SX1301_PAGE_BASE(1) + 0x46)
+#define SX1301_FSK_REF_PATTERN_3    (SX1301_PAGE_BASE(1) + 0x47)
+#define SX1301_FSK_REF_PATTERN_4    (SX1301_PAGE_BASE(1) + 0x48)
+#define SX1301_FSK_REF_PATTERN_5    (SX1301_PAGE_BASE(1) + 0x49)
+#define SX1301_FSK_REF_PATTERN_6    (SX1301_PAGE_BASE(1) + 0x4A)
+#define SX1301_FSK_REF_PATTERN_7    (SX1301_PAGE_BASE(1) + 0x4B)
+#define SX1301_FSK_PKT_LENGTH       (SX1301_PAGE_BASE(1) + 0x4C)
+#define SX1301_FSK_TX		    (SX1301_PAGE_BASE(1) + 0x4D)
+#define SX1301_FSK_AAFC             (SX1301_PAGE_BASE(1) + 0x52)
+#define SX1301_FSK_PATTERN_TIMEOUT_CFGL (SX1301_PAGE_BASE(1) + 0x53)
+#define SX1301_FSK_PATTERN_TIMEOUT_CFGH (SX1301_PAGE_BASE(1) + 0x54)
+
+/* 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)
+#define SX1301_RADIO_A_SPI_ADDR     (SX1301_PAGE_BASE(2) + 0x23)
+#define SX1301_RADIO_A_SPI_CS       (SX1301_PAGE_BASE(2) + 0x25)
+#define SX1301_RADIO_B_SPI_DATA     (SX1301_PAGE_BASE(2) + 0x26)
+#define SX1301_RADIO_B_SPI_DATA_RB  (SX1301_PAGE_BASE(2) + 0x27)
+#define SX1301_RADIO_B_SPI_ADDR     (SX1301_PAGE_BASE(2) + 0x28)
+#define SX1301_RADIO_B_SPI_CS       (SX1301_PAGE_BASE(2) + 0x2A)
+#define SX1301_RADIO_CFG            (SX1301_PAGE_BASE(2) + 0x2B)
+#define SX1301_PA_GAIN              (SX1301_PAGE_BASE(2) + 0x2C)
+#define SX1301_FE_A_CTRL_LUT        (SX1301_PAGE_BASE(2) + 0x2D)
+#define SX1301_FE_B_CTRL_LUT        (SX1301_PAGE_BASE(2) + 0x2E)
+#define SX1301_VALID_HEADER_COUNTER_M_WSSF  (SX1301_PAGE_BASE(2) + 0x38)
+#define SX1301_VALID_HEADER_COUNTER_FSK     (SX1301_PAGE_BASE(2) + 0x39)
+#define SX1301_VALID_PACKET_COUNTER_MB_SSF  (SX1301_PAGE_BASE(2) + 0x3A)
+#define SX1301_VALID_PACKET_COUNTER_FSK     (SX1301_PAGE_BASE(2) + 0x3B)
+#define SX1301_CHANN_RSSI           (SX1301_PAGE_BASE(2) + 0x3C)
+#define SX1301_BB_RSSI              (SX1301_PAGE_BASE(2) + 0x3D)
+#define SX1301_DEC_RSSI             (SX1301_PAGE_BASE(2) + 0x3E)
+#define SX1301_DBG_MCU_DATA         (SX1301_PAGE_BASE(2) + 0x3F)
+#define SX1301_DBG_ARB_MCU_RAM_DATA (SX1301_PAGE_BASE(2) + 0x40)
+#define SX1301_DBG_AGC_MCU_RAM_DATA (SX1301_PAGE_BASE(2) + 0x41)
+#define SX1301_TIMESTAMP_0          (SX1301_PAGE_BASE(2) + 0x46)
+#define SX1301_TIMESTAMP_1          (SX1301_PAGE_BASE(2) + 0x47)
+#define SX1301_TIMESTAMP_2          (SX1301_PAGE_BASE(2) + 0x48)
+#define SX1301_TIMESTAMP_3          (SX1301_PAGE_BASE(2) + 0x49)
+#define SX1301_DBG_ARB_MCU_RAM_ADDR (SX1301_PAGE_BASE(2) + 0x50)
+#define SX1301_DBG_AGC_MCU_RAM_ADDR (SX1301_PAGE_BASE(2) + 0x51)
+#define SX1301_SPI_MASTER_CFG       (SX1301_PAGE_BASE(2) + 0x52)
+#define SX1301_GPS_CFG              (SX1301_PAGE_BASE(2) + 0x59)
+
+/* Page 3 */
+#define SX1301_EMERGENCY_FORCE_HOST_CTRL (SX1301_PAGE_BASE(3) + 0x7F)
+
+#define SX1301_MAX_REGISTER         (SX1301_PAGE_BASE(3) + 0x7F)
+
+#endif
-- 
2.7.4

Re: [PATCH lora-next 01/10] net: lora: sx1301: add register, bit-fields, and helpers for regmap

[Andreas Färber]

Am 07.08.2018 um 19:32 schrieb Ben Whitten:
> From: Ben Whitten <ben.whitten@lairdtech.com>
> 
> The register and bit-field definitions are taken from the SX1301
> datasheet version 2.01 dated June 2014 with the revision information
> 'First released version'.
> 
> The reset state and RW capability of each field is not reflected in this
> patch however from the datasheet:
> "Bits and registers that are not documented are reserved. They may
> include calibration values. It is important not to modify these bits and
> registers. If specific bits must be changed in a register with reserved
> bits, the register must be read first, specific bits modified while
> masking reserved bits and then the register can be written."
> 
> Then goes on to state:
> "Reserved bits should be written with their reset state, they may be
> read different states."
> 
> Caching is currently disabled.
> 
> The version is read back using regmap_read to verify regmap operation,
> in doing so needs to be moved after priv and regmap allocation.
> 
> Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
> ---
>  drivers/net/lora/Kconfig  |   1 +
>  drivers/net/lora/sx1301.c | 282 +++++++++++++++++++++++++++++++++++++++++++---
>  drivers/net/lora/sx1301.h | 169 +++++++++++++++++++++++++++
>  3 files changed, 439 insertions(+), 13 deletions(-)
>  create mode 100644 drivers/net/lora/sx1301.h

My main concern about this patch is its sheer size. Normally for
#defines the rule is not to add unused ones. Here I see for example FSK
RSSI fields that we're surely not using yet. Any chance to strip this
down some more?

Regards,
Andreas

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

RE: [PATCH lora-next 01/10] net: lora: sx1301: add register, bit-fields, and helpers for regmap

[Ben Whitten]

> Subject: Re: [PATCH lora-next 01/10] net: lora: sx1301: add
> register, bit-fields, and helpers for regmap
> 
> Am 07.08.2018 um 19:32 schrieb Ben Whitten:
> > From: Ben Whitten <ben.whitten@lairdtech.com>
> >
> > The register and bit-field definitions are taken from the
> SX1301
> > datasheet version 2.01 dated June 2014 with the revision
> information
> > 'First released version'.
> >
> > The reset state and RW capability of each field is not
> reflected in this
> > patch however from the datasheet:
> > "Bits and registers that are not documented are reserved.
> They may
> > include calibration values. It is important not to modify
> these bits and
> > registers. If specific bits must be changed in a register with
> reserved
> > bits, the register must be read first, specific bits modified
> while
> > masking reserved bits and then the register can be
> written."
> >
> > Then goes on to state:
> > "Reserved bits should be written with their reset state,
> they may be
> > read different states."
> >
> > Caching is currently disabled.
> >
> > The version is read back using regmap_read to verify
> regmap operation,
> > in doing so needs to be moved after priv and regmap
> allocation.
> >
> > Signed-off-by: Ben Whitten
> <ben.whitten@lairdtech.com>
> > ---
> >  drivers/net/lora/Kconfig  |   1 +
> >  drivers/net/lora/sx1301.c | 282
> +++++++++++++++++++++++++++++++++++++++++++---
> >  drivers/net/lora/sx1301.h | 169
> +++++++++++++++++++++++++++
> >  3 files changed, 439 insertions(+), 13 deletions(-)
> >  create mode 100644 drivers/net/lora/sx1301.h
> 
> My main concern about this patch is its sheer size. Normally
> for
> #defines the rule is not to add unused ones. Here I see for
> example FSK
> RSSI fields that we're surely not using yet. Any chance to
> strip this
> down some more?

Sure, I'll strip the reg_fields down to those only required for
loading firmware and setting clocks that will be used in the
immediate term. This does clutter the driver a bit
unnecessarily at the moment.
I would like to keep the full register dump in the header file
though as its self-contained and gives a full picture of the chip.

Thanks,
Ben

Re: [PATCH lora-next 01/10] net: lora: sx1301: add register, bit-fields, and helpers for regmap

[Andreas Färber]

Am 08.08.2018 um 14:32 schrieb Ben Whitten:
>>>  drivers/net/lora/Kconfig  |   1 +
>>>  drivers/net/lora/sx1301.c | 282
>> +++++++++++++++++++++++++++++++++++++++++++---
>>>  drivers/net/lora/sx1301.h | 169
>> +++++++++++++++++++++++++++
>>>  3 files changed, 439 insertions(+), 13 deletions(-)
>>>  create mode 100644 drivers/net/lora/sx1301.h
>>
>> My main concern about this patch is its sheer size. Normally
>> for
>> #defines the rule is not to add unused ones. Here I see for
>> example FSK
>> RSSI fields that we're surely not using yet. Any chance to
>> strip this
>> down some more?
> 
> Sure, I'll strip the reg_fields down to those only required for
> loading firmware and setting clocks that will be used in the
> immediate term. This does clutter the driver a bit
> unnecessarily at the moment.
> I would like to keep the full register dump in the header file
> though as its self-contained and gives a full picture of the chip.

Could you do that in more steps though? I'm thinking, convert only the
registers in use to regmap (that'll make it easier to review), then add
more registers, then convert to regmap fields?

Regards,
Andreas

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

RE: [PATCH lora-next 01/10] net: lora: sx1301: add register, bit-fields, and helpers for regmap

[Ben Whitten]

> Subject: Re: [PATCH lora-next 01/10] net: lora: sx1301: add
> register, bit-fields, and helpers for regmap
> 
> Am 08.08.2018 um 14:32 schrieb Ben Whitten:
> >>>  drivers/net/lora/Kconfig  |   1 +
> >>>  drivers/net/lora/sx1301.c | 282
> >> +++++++++++++++++++++++++++++++++++++++++++-
> --
> >>>  drivers/net/lora/sx1301.h | 169
> >> +++++++++++++++++++++++++++
> >>>  3 files changed, 439 insertions(+), 13 deletions(-)
> >>>  create mode 100644 drivers/net/lora/sx1301.h
> >>
> >> My main concern about this patch is its sheer size.
> Normally
> >> for
> >> #defines the rule is not to add unused ones. Here I see
> for
> >> example FSK
> >> RSSI fields that we're surely not using yet. Any chance to
> >> strip this
> >> down some more?
> >
> > Sure, I'll strip the reg_fields down to those only required
> for
> > loading firmware and setting clocks that will be used in the
> > immediate term. This does clutter the driver a bit
> > unnecessarily at the moment.
> > I would like to keep the full register dump in the header
> file
> > though as its self-contained and gives a full picture of the
> chip.
> 
> Could you do that in more steps though? I'm thinking,
> convert only the
> registers in use to regmap (that'll make it easier to review),
> then add
> more registers, then convert to regmap fields?

I split the conversion function by function but we can probably go
further if you think it helpful.
Is it more the addition of the replacement register defines that you
would like to correlate with what's being removed, so you can see
in one patch that this register has the same page and the same
address in the new system?

The problem I face is that these conversions are almost blind as
when I run on my hardware I either oops with the sx1301_read
or the cal firmware doesn't verify so I can't finish probe. I only
get a full sx1301 module probe pass on physical hardware when
I get right to the end of the series where it's all replaced with
regmap.

Regards,
Ben Whitten

Re: [PATCH lora-next 01/10] net: lora: sx1301: add register, bit-fields, and helpers for regmap

[Andreas Färber]

+ Xue Liu + Sebastian

Am 08.08.2018 um 17:52 schrieb Ben Whitten:
>> Subject: Re: [PATCH lora-next 01/10] net: lora: sx1301: add
>> register, bit-fields, and helpers for regmap
>>
>> Am 08.08.2018 um 14:32 schrieb Ben Whitten:
>>>>>  drivers/net/lora/Kconfig  |   1 +
>>>>>  drivers/net/lora/sx1301.c | 282
>>>> +++++++++++++++++++++++++++++++++++++++++++-
>> --
>>>>>  drivers/net/lora/sx1301.h | 169
>>>> +++++++++++++++++++++++++++
>>>>>  3 files changed, 439 insertions(+), 13 deletions(-)
>>>>>  create mode 100644 drivers/net/lora/sx1301.h
>>>>
>>>> My main concern about this patch is its sheer size.
>> Normally
>>>> for
>>>> #defines the rule is not to add unused ones. Here I see
>> for
>>>> example FSK
>>>> RSSI fields that we're surely not using yet. Any chance to
>>>> strip this
>>>> down some more?
>>>
>>> Sure, I'll strip the reg_fields down to those only required
>> for
>>> loading firmware and setting clocks that will be used in the
>>> immediate term. This does clutter the driver a bit
>>> unnecessarily at the moment.
>>> I would like to keep the full register dump in the header
>> file
>>> though as its self-contained and gives a full picture of the
>> chip.
>>
>> Could you do that in more steps though? I'm thinking,
>> convert only the
>> registers in use to regmap (that'll make it easier to review),
>> then add
>> more registers, then convert to regmap fields?
> 
> I split the conversion function by function but we can probably go
> further if you think it helpful.

That split feels wrong...

> Is it more the addition of the replacement register defines that you
> would like to correlate with what's being removed, so you can see
> in one patch that this register has the same page and the same
> address in the new system?

I am looking for patches that are trivial to review. One aspect only.
So I would much rather get a large patch with a consistent series of

-my_read
+your_read

-my_write
+your_write

that's quick to review than lots of different refactorings mixed into
one patch, grouped by their file location.

So I am suggesting that if, for example, you want to pass fw to helpers,
which looks like a great idea, that should be a small patch of its own,
at the very beginning of your series. (git rebase -i is your friend.)

Converting to regmap_read/write I imagine to be a trivial
search-and-replace type refactoring, leaving my |= and &= operations in
place, to minimize the diff and avoid it mis-detecting the patch context.
Without caching I'd expect regmap to work up to here - if it doesn't,
then we can't apply it to my tree and need to prioritize other cleanups
while we review/debug further.

Once all registers use regmap successfully, we can optimize that code by
introducing regmap fields. This could be split by location, if desired.

Finally in the end you can introduce more registers and fields for
future use.

Does that make sense now?

> The problem I face is that these conversions are almost blind as
> when I run on my hardware I either oops with the sx1301_read
> or the cal firmware doesn't verify so I can't finish probe. I only
> get a full sx1301 module probe pass on physical hardware when
> I get right to the end of the series where it's all replaced with
> regmap.

If patches don't build or don't work then I can't apply them. Otherwise
the 0-day bots will spam us with error reports, as you've seen before.

BTW we'll need this regmap conversion for the picoGW_hal, so once we
have a working SPI regmap driver, we'll need to split out the SPI bits,
similar to sx125x.

Regards,
Andreas

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

RE: [PATCH lora-next 01/10] net: lora: sx1301: add register, bit-fields, and helpers for regmap

[Ben Whitten]

> To: Ben Whitten <Ben.Whitten@lairdtech.com>; Ben
> Whitten <ben.whitten@gmail.com>
> Cc: starnight@g.ncu.edu.tw; hasnain.virk@arm.com;
> netdev@vger.kernel.org; Xue Liu
> <liuxuenetmail@gmail.com>; Sebastian Heß
> <shess@hessware.de>
> Subject: Re: [PATCH lora-next 01/10] net: lora: sx1301: add
> register, bit-fields, and helpers for regmap
> 
> + Xue Liu + Sebastian
> 
> Am 08.08.2018 um 17:52 schrieb Ben Whitten:
> >> Subject: Re: [PATCH lora-next 01/10] net: lora: sx1301:
> add
> >> register, bit-fields, and helpers for regmap
> >>
> >> Am 08.08.2018 um 14:32 schrieb Ben Whitten:
> >>>>>  drivers/net/lora/Kconfig  |   1 +
> >>>>>  drivers/net/lora/sx1301.c | 282
> >>>>
> +++++++++++++++++++++++++++++++++++++++++++-
> >> --
> >>>>>  drivers/net/lora/sx1301.h | 169
> >>>> +++++++++++++++++++++++++++
> >>>>>  3 files changed, 439 insertions(+), 13 deletions(-)
> >>>>>  create mode 100644 drivers/net/lora/sx1301.h
> >>>>
> >>>> My main concern about this patch is its sheer size.
> >> Normally
> >>>> for
> >>>> #defines the rule is not to add unused ones. Here I
> see
> >> for
> >>>> example FSK
> >>>> RSSI fields that we're surely not using yet. Any chance
> to
> >>>> strip this
> >>>> down some more?
> >>>
> >>> Sure, I'll strip the reg_fields down to those only
> required
> >> for
> >>> loading firmware and setting clocks that will be used in
> the
> >>> immediate term. This does clutter the driver a bit
> >>> unnecessarily at the moment.
> >>> I would like to keep the full register dump in the header
> >> file
> >>> though as its self-contained and gives a full picture of
> the
> >> chip.
> >>
> >> Could you do that in more steps though? I'm thinking,
> >> convert only the
> >> registers in use to regmap (that'll make it easier to
> review),
> >> then add
> >> more registers, then convert to regmap fields?
> >
> > I split the conversion function by function but we can
> probably go
> > further if you think it helpful.
> 
> That split feels wrong...
> 
> > Is it more the addition of the replacement register defines
> that you
> > would like to correlate with what's being removed, so you
> can see
> > in one patch that this register has the same page and the
> same
> > address in the new system?
> 
> I am looking for patches that are trivial to review. One
> aspect only.
> So I would much rather get a large patch with a consistent
> series of
> 
> -my_read
> +your_read
> 
> -my_write
> +your_write
> 
> that's quick to review than lots of different refactorings
> mixed into
> one patch, grouped by their file location.
> 
> So I am suggesting that if, for example, you want to pass fw
> to helpers,
> which looks like a great idea, that should be a small patch of
> its own,
> at the very beginning of your series. (git rebase -i is your
> friend.)
> 
> Converting to regmap_read/write I imagine to be a trivial
> search-and-replace type refactoring, leaving my |= and &=
> operations in
> place, to minimize the diff and avoid it mis-detecting the
> patch context.
> Without caching I'd expect regmap to work up to here - if it
> doesn't,
> then we can't apply it to my tree and need to prioritize other
> cleanups
> while we review/debug further.
> 
> Once all registers use regmap successfully, we can optimize
> that code by
> introducing regmap fields. This could be split by location, if
> desired.
> 
> Finally in the end you can introduce more registers and
> fields for
> future use.
> 
> Does that make sense now?

Yep no problem, hopefully my V2 is suitable. I stopped short
of including all registers and doing any regmap_field work
in this series so that we can progress.
I have the sx125x split and conversion to regmap_bus for
the concentrator ready to go in the wings.

> > The problem I face is that these conversions are almost
> blind as
> > when I run on my hardware I either oops with the
> sx1301_read
> > or the cal firmware doesn't verify so I can't finish probe. I
> only
> > get a full sx1301 module probe pass on physical hardware
> when
> > I get right to the end of the series where it's all replaced
> with
> > regmap.
> 
> If patches don't build or don't work then I can't apply them.
> Otherwise
> the 0-day bots will spam us with error reports, as you've
> seen before.

Understood, each patch in v2 has been tested and I was able
to probe and remove modules.

> BTW we'll need this regmap conversion for the picoGW_hal,
> so once we
> have a working SPI regmap driver, we'll need to split out the
> SPI bits,
> similar to sx125x.

I am unfamiliar with the picoGW_hal, do they expose the sx1301
as a device on a regmap_bus then?

Regards,
Ben Whitten

Re: [PATCH lora-next 01/10] net: lora: sx1301: add register, bit-fields, and helpers for regmap

[Andreas Färber]

Am 09.08.2018 um 15:23 schrieb Ben Whitten:
>> BTW we'll need this regmap conversion for the picoGW_hal,
>> so once we
>> have a working SPI regmap driver, we'll need to split out the
>> SPI bits,
>> similar to sx125x.
> 
> I am unfamiliar with the picoGW_hal, do they expose the sx1301
> as a device on a regmap_bus then?

It uses an MCU implementing a USB CDC interface that implements UART
commands corresponding to the individual SPI read and write modes like
single and burst. That reference design is for SX1308, but some vendors
appear to be using it for SX1301, too.

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

So I believe I'll need to implement a regmap_bus in a new serdev driver
to support it, once I gain access to such an implementation.

But there's other general serdev problems with USB not limited to picoGW
that I'll need to start a thread about on linux-serial/linux-usb...

Regards,
Andreas

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

Re: [PATCH lora-next 01/10] net: lora: sx1301: add register, bit-fields, and helpers for regmap

[Andreas Färber]

Am 07.08.2018 um 19:32 schrieb Ben Whitten:
> diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
> index 5342b61..49958f0 100644
> --- a/drivers/net/lora/sx1301.c
> +++ b/drivers/net/lora/sx1301.c
[...]
> @@ -76,8 +287,29 @@ struct sx1301_priv {
>  	struct gpio_desc *rst_gpio;
>  	u8 cur_page;
>  	struct spi_controller *radio_a_ctrl, *radio_b_ctrl;
> +	struct regmap		*regmap;
> +	struct regmap_field	*regmap_fields[ARRAY_SIZE(sx1301_reg_fields)];
>  };
>  
> +static int sx1301_field_read(struct sx1301_priv *priv,
> +		enum sx1301_fields field_id)
> +{
> +	int ret;
> +	int val;
> +
> +	ret = regmap_field_read(priv->regmap_fields[field_id], &val);
> +	if (ret)
> +		return ret;
> +
> +	return val;

This strikes me as a bad idea. Please keep returning the value by
pointer, so that we can clearly distinguish from error values.

> +}
> +
> +static int sx1301_field_write(struct sx1301_priv *priv,
> +		enum sx1301_fields field_id, u8 val)
> +{
> +	return regmap_field_write(priv->regmap_fields[field_id], val);
> +}
> +
>  static int sx1301_read_burst(struct spi_device *spi, u8 reg, u8 *val, size_t len)
>  {
>  	u8 addr = reg & 0x7f;
[snip]

It looks to me as if both of those static functions are unused in this
patch? Please keep things bisectable.

Regards,
Andreas

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

RE: [PATCH lora-next 01/10] net: lora: sx1301: add register, bit-fields, and helpers for regmap

[Ben Whitten]

> Subject: Re: [PATCH lora-next 01/10] net: lora: sx1301: add
> register, bit-fields, and helpers for regmap
> 
> Am 07.08.2018 um 19:32 schrieb Ben Whitten:
> > diff --git a/drivers/net/lora/sx1301.c
> b/drivers/net/lora/sx1301.c
> > index 5342b61..49958f0 100644
> > --- a/drivers/net/lora/sx1301.c
> > +++ b/drivers/net/lora/sx1301.c
> [...]
> > @@ -76,8 +287,29 @@ struct sx1301_priv {
> >  	struct gpio_desc *rst_gpio;
> >  	u8 cur_page;
> >  	struct spi_controller *radio_a_ctrl, *radio_b_ctrl;
> > +	struct regmap		*regmap;
> > +	struct regmap_field
> 	*regmap_fields[ARRAY_SIZE(sx1301_reg_fields)];
> >  };
> >
> > +static int sx1301_field_read(struct sx1301_priv *priv,
> > +		enum sx1301_fields field_id)
> > +{
> > +	int ret;
> > +	int val;
> > +
> > +	ret = regmap_field_read(priv-
> >regmap_fields[field_id], &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return val;
> 
> This strikes me as a bad idea. Please keep returning the
> value by
> pointer, so that we can clearly distinguish from error values.

Good point, will change it.

> > +}
> > +
> > +static int sx1301_field_write(struct sx1301_priv *priv,
> > +		enum sx1301_fields field_id, u8 val)
> > +{
> > +	return regmap_field_write(priv-
> >regmap_fields[field_id], val);
> > +}
> > +
> >  static int sx1301_read_burst(struct spi_device *spi, u8
> reg, u8 *val, size_t len)
> >  {
> >  	u8 addr = reg & 0x7f;
> [snip]
> 
> It looks to me as if both of those static functions are unused
> in this
> patch? Please keep things bisectable.

Sure, it was a prep patch bit I can include these functions in the
next one along where they are actually used.

Thanks,
Ben

[RFC] spi: add spi multiplexing functions for dt

[Ben Whitten]

Like I2C busses SPI devices can also sit behind multiplexers.
This patch adds is based off the I2C implementation and allows
description in the devicetree.

Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
---
 drivers/spi/Kconfig     |  10 +++
 drivers/spi/Makefile    |   3 +
 drivers/spi/spi-mux.c   | 181 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/spi-mux.h |  55 +++++++++++++++
 4 files changed, 249 insertions(+)
 create mode 100644 drivers/spi/spi-mux.c
 create mode 100644 include/linux/spi-mux.h

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index a75f2a2..58eba70 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -51,6 +51,16 @@ config SPI_MASTER
 
 if SPI_MASTER
 
+config SPI_MUX
+	tristate "SPI bus multiplexing support"
+	help
+	  Say Y here if you want the SPI core to support the ability to
+	  handle multiplexed SPI bus topologies, by presenting each
+	  multiplexed segment as an SPI controller.
+
+	  This support is also available as a module.  If so, the module
+	  will be called spi-mux.
+
 comment "SPI Master Controller Drivers"
 
 config SPI_ALTERA
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 8e0cda7..ef525fe 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -109,6 +109,9 @@ obj-$(CONFIG_SPI_XLP)			+= spi-xlp.o
 obj-$(CONFIG_SPI_XTENSA_XTFPGA)		+= spi-xtensa-xtfpga.o
 obj-$(CONFIG_SPI_ZYNQMP_GQSPI)		+= spi-zynqmp-gqspi.o
 
+# SPI muxs
+obj-$(CONFIG_SPI_MUX)			+= spi-mux.o
+
 # SPI slave protocol handlers
 obj-$(CONFIG_SPI_SLAVE_TIME)		+= spi-slave-time.o
 obj-$(CONFIG_SPI_SLAVE_SYSTEM_CONTROL)	+= spi-slave-system-control.o
diff --git a/drivers/spi/spi-mux.c b/drivers/spi/spi-mux.c
new file mode 100644
index 0000000..a2008c1
--- /dev/null
+++ b/drivers/spi/spi-mux.c
@@ -0,0 +1,181 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for an SPI multiplexer
+ *
+ * Copyright (c) 2018   Ben Whitten
+ */
+
+#include <linux/spi/spi.h>
+#include <linux/spi-mux.h>
+#include <linux/of.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+struct spi_mux_priv {
+	struct spi_controller *controller;
+	struct spi_mux_core *muxc;
+	u32 chan_id;
+};
+
+struct spi_mux_core *spi_mux_alloc(struct spi_controller *parent,
+				   struct device *dev,
+				   int max_controllers,
+				   int sizeof_priv,
+				   int (*select)(struct spi_mux_core *, u32),
+				   int (*deselect)(struct spi_mux_core *, u32),
+				   int (*transfer_one_message)
+					(struct spi_controller *controller,
+					 struct spi_message *msg))
+{
+	struct spi_mux_core *muxc;
+
+	muxc = devm_kzalloc(dev, sizeof(*muxc)
+			    + max_controllers * sizeof(muxc->controller[0])
+			    + sizeof_priv, GFP_KERNEL);
+	if (!muxc)
+		return NULL;
+	if (sizeof_priv)
+		muxc->priv = &muxc->controller[max_controllers];
+
+	muxc->parent = parent;
+	muxc->dev = dev;
+
+	muxc->select = select;
+	muxc->deselect = deselect;
+	muxc->transfer_one_message = transfer_one_message;
+	muxc->max_controllers = max_controllers;
+
+	return muxc;
+}
+EXPORT_SYMBOL_GPL(spi_mux_alloc);
+
+u32 spi_mux_get_chan_id(struct spi_controller *controller)
+{
+	struct spi_mux_priv *priv = spi_controller_get_devdata(controller);
+
+	return priv->chan_id;
+}
+EXPORT_SYMBOL_GPL(spi_mux_get_chan_id);
+
+static int spi_mux_transfer_one_message(struct spi_controller *controller,
+					struct spi_message *msg)
+{
+	struct spi_mux_priv *priv = spi_controller_get_devdata(controller);
+	struct spi_mux_core *muxc = priv->muxc;
+	struct spi_device *spi = to_spi_device(muxc->dev);
+	int ret;
+
+	ret = muxc->select(muxc, priv->chan_id);
+	if (ret < 0)
+		return ret;
+
+	/* If we have a custom transfer, use it */
+	if (muxc->transfer_one_message)
+		ret = muxc->transfer_one_message(controller, msg);
+	else
+		ret = spi_sync(spi, msg);
+
+	if (muxc->deselect)
+		muxc->deselect(muxc, priv->chan_id);
+
+	return ret;
+}
+
+static int spi_mux_add_controller(struct spi_mux_core *muxc, u32 chan_id)
+{
+	struct spi_controller *controller;
+	struct spi_mux_priv *priv;
+	int ret;
+
+	if (muxc->num_controllers >= muxc->max_controllers) {
+		dev_err(muxc->dev, "No room for more spi-mux controllers");
+		return -EINVAL;
+	}
+
+	controller = spi_alloc_master(muxc->dev, sizeof(*priv));
+	if (!controller)
+		return -ENOMEM;
+	priv = spi_controller_get_devdata(controller);
+
+	/* Setup private controller data */
+	priv->muxc = muxc;
+	priv->controller = controller;
+	priv->chan_id = chan_id;
+
+	priv->controller->transfer_one_message = spi_mux_transfer_one_message;
+
+	/* Look for the child of this controller */
+	if (muxc->dev->of_node) {
+		struct device_node *dev_node = muxc->dev->of_node;
+		struct device_node *mux_node, *child = NULL;
+		u32 reg;
+
+		mux_node = of_get_child_by_name(dev_node, "spi-mux");
+		if (!mux_node)
+			mux_node = of_node_get(dev_node);
+
+		for_each_child_of_node(mux_node, child) {
+			ret = of_property_read_u32(child, "reg", &reg);
+			if (ret)
+				continue;
+			if (chan_id == reg)
+				break;
+		}
+
+		priv->controller->dev.of_node = child;
+		of_node_put(mux_node);
+	}
+
+	ret = devm_spi_register_controller(muxc->dev, priv->controller);
+	if (ret) {
+		spi_controller_put(priv->controller);
+		dev_err(muxc->dev, "Problem registering spi controller: %d\n",
+			ret);
+		return ret;
+	}
+
+	muxc->controller[muxc->num_controllers++] = priv->controller;
+
+	return ret;
+}
+
+static void spi_mux_del_controllers(struct spi_mux_core *muxc)
+{
+	struct spi_controller *controller =
+	    muxc->controller[--muxc->num_controllers];
+	struct device_node *np = controller->dev.of_node;
+
+	muxc->controller[muxc->num_controllers] = NULL;
+	of_node_put(np);
+}
+
+static void devm_spi_mux_del_controllers(struct device *dev, void *res)
+{
+	spi_mux_del_controllers(*(struct spi_mux_core **)res);
+}
+
+int devm_spi_mux_add_controller(struct spi_mux_core *muxc, u32 chan_id)
+{
+	struct spi_mux_core **ptr;
+	int ret;
+
+	ptr = devres_alloc(devm_spi_mux_del_controllers, sizeof(*ptr),
+			   GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	ret = spi_mux_add_controller(muxc, chan_id);
+	if (!ret) {
+		*ptr = muxc;
+		devres_add(muxc->dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_spi_mux_add_controller);
+
+MODULE_AUTHOR("Ben Whitten <ben.whitten@gmail.com>");
+MODULE_DESCRIPTION("SPI driver for multiplexed SPI busses");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/spi-mux.h b/include/linux/spi-mux.h
new file mode 100644
index 0000000..5978f86
--- /dev/null
+++ b/include/linux/spi-mux.h
@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for an SPI multiplexer
+ *
+ * Copyright (c) 2018   Ben Whitten
+ */
+
+#ifndef _LINUX_SPI_MUX_H_
+#define _LINUX_SPI_MUX_H_
+
+#ifdef __KERNEL__
+
+struct spi_mux_core {
+	struct spi_controller *parent;
+	struct device *dev;
+
+	void *priv;
+
+	int (*select)(struct spi_mux_core *, u32 chan_id);
+	int (*deselect)(struct spi_mux_core *, u32 chan_id);
+	int (*transfer_one_message)(struct spi_controller *controller,
+				    struct spi_message *msg);
+
+	int num_controllers;
+	int max_controllers;
+	struct spi_controller *controller[0];
+};
+
+struct spi_mux_core *spi_mux_alloc(struct spi_controller *parent,
+				   struct device *dev,
+				   int max_controllers,
+				   int sizeof_priv,
+				   int (*select)(struct spi_mux_core *, u32),
+				   int (*deselect)(struct spi_mux_core *, u32),
+				   int (*transfer_one_message)
+					(struct spi_controller *controller,
+					 struct spi_message *msg));
+
+static inline void *spi_mux_priv(struct spi_mux_core *muxc)
+{
+	return muxc->priv;
+}
+
+u32 spi_mux_get_chan_id(struct spi_controller *controller);
+
+/*
+ * Called to create an spi bus on a multiplexed bus segment.
+ * The chan_id parameter is passed to the select and deselect
+ * callback functions to perform hardware-specific mux control.
+ */
+int devm_spi_mux_add_controller(struct spi_mux_core *muxc, u32 chan_id);
+
+#endif /* __KERNEL__ */
+
+#endif /* _LINUX_SPI_MUX_H_ */
-- 
2.7.4

Re: [RFC] spi: add spi multiplexing functions for dt

[Andreas Färber]

Hi Ben,

Am 07.08.2018 um 19:32 schrieb Ben Whitten:
> Like I2C busses SPI devices can also sit behind multiplexers.
> This patch adds is based off the I2C implementation and allows
> description in the devicetree.
> 
> Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
> ---
>  drivers/spi/Kconfig     |  10 +++
>  drivers/spi/Makefile    |   3 +
>  drivers/spi/spi-mux.c   | 181 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/spi-mux.h |  55 +++++++++++++++
>  4 files changed, 249 insertions(+)
>  create mode 100644 drivers/spi/spi-mux.c
>  create mode 100644 include/linux/spi-mux.h

Did this get sent by mistake? It needs to go to linux-spi list and
maintainers.

Tip: git config sendemail.cccmd "scripts/get_maintainer.pl
--nogit-fallback --norolestats"

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: [RFC] spi: add spi multiplexing functions for dt

[Ben Whitten]

On Tue, 7 Aug 2018 at 22:17, Andreas Färber <afaerber@suse.de> wrote:
>
> Hi Ben,
>
> Am 07.08.2018 um 19:32 schrieb Ben Whitten:
> > Like I2C busses SPI devices can also sit behind multiplexers.
> > This patch adds is based off the I2C implementation and allows
> > description in the devicetree.
> >
> > Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
> > ---
> >  drivers/spi/Kconfig     |  10 +++
> >  drivers/spi/Makefile    |   3 +
> >  drivers/spi/spi-mux.c   | 181 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/spi-mux.h |  55 +++++++++++++++
> >  4 files changed, 249 insertions(+)
> >  create mode 100644 drivers/spi/spi-mux.c
> >  create mode 100644 include/linux/spi-mux.h
>
> Did this get sent by mistake? It needs to go to linux-spi list and
> maintainers.

It certainly did, didn't spot it in my outgoing patches directory.
Please disregard, sorry for the noise.

> Tip: git config sendemail.cccmd "scripts/get_maintainer.pl
> --nogit-fallback --norolestats"

Thanks for the tip

Cheers,
Ben

[PATCH lora-next 02/10] net: lora: add methods for devm registration

[Ben Whitten]

From: Ben Whitten <ben.whitten@lairdtech.com>

Follow the devm model so that we can avoid lengthy unwind code.

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

diff --git a/drivers/net/lora/dev.c b/drivers/net/lora/dev.c
index 8c01106..69a8b52 100644
--- a/drivers/net/lora/dev.c
+++ b/drivers/net/lora/dev.c
@@ -84,6 +84,26 @@ void free_loradev(struct net_device *dev)
 }
 EXPORT_SYMBOL_GPL(free_loradev);
 
+static void devm_lora_unregister(struct device *dev, void *res)
+{
+	free_loradev(*(struct net_device **)res);
+}
+
+int devm_lora_register_netdev(struct device *dev, struct net_device *net)
+{
+	struct net_device **ptr;
+
+	ptr = devres_alloc(devm_lora_unregister, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	*ptr = net;
+	devres_add(dev, ptr);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_lora_register_netdev);
+
 static struct rtnl_link_ops lora_link_ops __read_mostly = {
 	.kind = "lora",
 	.setup = lora_setup,
diff --git a/include/linux/lora/dev.h b/include/linux/lora/dev.h
index 153f9b2..3f871c6 100644
--- a/include/linux/lora/dev.h
+++ b/include/linux/lora/dev.h
@@ -32,6 +32,7 @@ static inline int lora_strtoeui(const char *str, lora_eui *val)
 
 struct net_device *alloc_loradev(int sizeof_priv);
 void free_loradev(struct net_device *dev);
+int devm_lora_register_netdev(struct device *dev, struct net_device *net);
 int register_loradev(struct net_device *dev);
 void unregister_loradev(struct net_device *dev);
 int open_loradev(struct net_device *dev);
-- 
2.7.4

Re: [PATCH lora-next 02/10] net: lora: add methods for devm registration

[Andreas Färber]

Am 07.08.2018 um 19:32 schrieb Ben Whitten:
> From: Ben Whitten <ben.whitten@lairdtech.com>
> 
> Follow the devm model so that we can avoid lengthy unwind code.
> 
> Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
> ---
>  drivers/net/lora/dev.c   | 20 ++++++++++++++++++++
>  include/linux/lora/dev.h |  1 +
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/net/lora/dev.c b/drivers/net/lora/dev.c
> index 8c01106..69a8b52 100644
> --- a/drivers/net/lora/dev.c
> +++ b/drivers/net/lora/dev.c
> @@ -84,6 +84,26 @@ void free_loradev(struct net_device *dev)
>  }
>  EXPORT_SYMBOL_GPL(free_loradev);
>  
> +static void devm_lora_unregister(struct device *dev, void *res)
> +{
> +	free_loradev(*(struct net_device **)res);

Suggest to use a variable.

> +}
> +
> +int devm_lora_register_netdev(struct device *dev, struct net_device *net)

Nice idea, but why a separate registration function? Suggest to instead
introduce devm_alloc_loradev(). If you then reorder those two patches to
the front of the series, I'll queue them immediately.

Thanks,
Andreas

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

[PATCH lora-next 03/10] net: lora: sx1301: convert to devm registration of netdev

[Ben Whitten]

From: Ben Whitten <ben.whitten@lairdtech.com>

We allow the devres framework handle the clean removal of resources on
teardown of the device, in this case the SPI device, saving lengthy
unwind code and improving clarity.

Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
---
 drivers/net/lora/sx1301.c | 87 +++++++++++++++++------------------------------
 1 file changed, 31 insertions(+), 56 deletions(-)

diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index 49958f0..54bfc31 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -869,9 +869,13 @@ static int sx1301_probe(struct spi_device *spi)
 	spi_setup(spi);
 
 	netdev = alloc_loradev(sizeof(*priv));
-	if (!netdev) {
-		ret = -ENOMEM;
-		goto err_alloc_loradev;
+	if (!netdev)
+		return -ENOMEM;
+
+	ret = devm_lora_register_netdev(&spi->dev, netdev);
+	if (ret) {
+		free_loradev(netdev);
+		return ret;
 	}
 
 	priv = netdev_priv(netdev);
@@ -885,7 +889,7 @@ static int sx1301_probe(struct spi_device *spi)
 	if (IS_ERR(priv->regmap)) {
 		ret = PTR_ERR(priv->regmap);
 		dev_err(&spi->dev, "Regmap allocation failed: %d\n", ret);
-		return err_regmap;
+		return ret;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(sx1301_reg_fields); i++) {
@@ -897,38 +901,37 @@ static int sx1301_probe(struct spi_device *spi)
 		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);
-			goto err_regmap;
+			return ret;
 		}
 	}
 
 	ret = regmap_read(priv->regmap, SX1301_VER, &ver);
 	if (ret) {
 		dev_err(&spi->dev, "version read failed\n");
-		goto err_version;
+		return ret;
 	}
 
 	if (ver != 103) {
 		dev_err(&spi->dev, "unexpected version: %u\n", ver);
-		ret = -ENXIO;
-		goto err_version;
+		return -ENXIO;
 	}
 
 	ret = sx1301_write(spi, REG_PAGE_RESET, 0);
 	if (ret) {
 		dev_err(&spi->dev, "page/reset write failed\n");
-		goto err_init_page;
+		return ret;
 	}
 
 	ret = sx1301_soft_reset(spi);
 	if (ret) {
 		dev_err(&spi->dev, "soft reset failed\n");
-		goto err_soft_reset;
+		return ret;
 	}
 
 	ret = sx1301_read(spi, 16, &val);
 	if (ret) {
 		dev_err(&spi->dev, "16 read failed\n");
-		goto err_read_global_en_0;
+		return ret;
 	}
 
 	val &= ~REG_16_GLOBAL_EN;
@@ -936,13 +939,13 @@ static int sx1301_probe(struct spi_device *spi)
 	ret = sx1301_write(spi, 16, val);
 	if (ret) {
 		dev_err(&spi->dev, "16 write failed\n");
-		goto err_write_global_en_0;
+		return ret;
 	}
 
 	ret = sx1301_read(spi, 17, &val);
 	if (ret) {
 		dev_err(&spi->dev, "17 read failed\n");
-		goto err_read_clk32m_0;
+		return ret;
 	}
 
 	val &= ~REG_17_CLK32M_EN;
@@ -950,7 +953,7 @@ static int sx1301_probe(struct spi_device *spi)
 	ret = sx1301_write(spi, 17, val);
 	if (ret) {
 		dev_err(&spi->dev, "17 write failed\n");
-		goto err_write_clk32m_0;
+		return ret;
 	}
 
 	ret = sx1301_page_read(spi, 2, 43, &val);
@@ -1003,8 +1006,7 @@ static int sx1301_probe(struct spi_device *spi)
 
 	priv->radio_a_ctrl = spi_alloc_master(&spi->dev, sizeof(*radio));
 	if (!priv->radio_a_ctrl) {
-		ret = -ENOMEM;
-		goto err_radio_a_alloc;
+		return -ENOMEM;
 	}
 
 	sx1301_radio_setup(priv->radio_a_ctrl);
@@ -1019,15 +1021,14 @@ static int sx1301_probe(struct spi_device *spi)
 	if (ret) {
 		dev_err(&spi->dev, "radio A SPI register failed\n");
 		spi_controller_put(priv->radio_a_ctrl);
-		goto err_radio_a_register;
+		return ret;
 	}
 
 	/* 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;
+		return -ENOMEM;
 	}
 
 	sx1301_radio_setup(priv->radio_b_ctrl);
@@ -1042,7 +1043,7 @@ static int sx1301_probe(struct spi_device *spi)
 	if (ret) {
 		dev_err(&spi->dev, "radio B SPI register failed\n");
 		spi_controller_put(priv->radio_b_ctrl);
-		goto err_radio_b_register;
+		return ret;
 	}
 
 	/* GPIO */
@@ -1050,7 +1051,7 @@ static int sx1301_probe(struct spi_device *spi)
 	ret = sx1301_read(spi, REG_GPIO_MODE, &val);
 	if (ret) {
 		dev_err(&spi->dev, "GPIO mode read failed\n");
-		goto err_read_gpio_mode;
+		return ret;
 	}
 
 	val |= GENMASK(4, 0);
@@ -1058,13 +1059,13 @@ static int sx1301_probe(struct spi_device *spi)
 	ret = sx1301_write(spi, REG_GPIO_MODE, val);
 	if (ret) {
 		dev_err(&spi->dev, "GPIO mode write failed\n");
-		goto err_write_gpio_mode;
+		return ret;
 	}
 
 	ret = sx1301_read(spi, REG_GPIO_SELECT_OUTPUT, &val);
 	if (ret) {
 		dev_err(&spi->dev, "GPIO select output read failed\n");
-		goto err_read_gpio_select_output;
+		return ret;
 	}
 
 	val &= ~GENMASK(3, 0);
@@ -1073,7 +1074,7 @@ static int sx1301_probe(struct spi_device *spi)
 	ret = sx1301_write(spi, REG_GPIO_SELECT_OUTPUT, val);
 	if (ret) {
 		dev_err(&spi->dev, "GPIO select output write failed\n");
-		goto err_write_gpio_select_output;
+		return ret;
 	}
 
 	/* TODO LBT */
@@ -1081,7 +1082,7 @@ static int sx1301_probe(struct spi_device *spi)
 	ret = sx1301_read(spi, 16, &val);
 	if (ret) {
 		dev_err(&spi->dev, "16 read (1) failed\n");
-		goto err_read_global_en_1;
+		return ret;
 	}
 
 	val |= REG_16_GLOBAL_EN;
@@ -1089,13 +1090,13 @@ static int sx1301_probe(struct spi_device *spi)
 	ret = sx1301_write(spi, 16, val);
 	if (ret) {
 		dev_err(&spi->dev, "16 write (1) failed\n");
-		goto err_write_global_en_1;
+		return ret;
 	}
 
 	ret = sx1301_read(spi, 17, &val);
 	if (ret) {
 		dev_err(&spi->dev, "17 read (1) failed\n");
-		goto err_read_clk32m_1;
+		return ret;
 	}
 
 	val |= REG_17_CLK32M_EN;
@@ -1103,50 +1104,24 @@ static int sx1301_probe(struct spi_device *spi)
 	ret = sx1301_write(spi, 17, val);
 	if (ret) {
 		dev_err(&spi->dev, "17 write (1) failed\n");
-		goto err_write_clk32m_1;
+		return ret;
 	}
 
 	/* calibration */
 
 	ret = sx1301_agc_calibrate(spi);
 	if (ret)
-		goto err_agc_calibrate;
+		return ret;
 
 	/* TODO */
 
 	ret = sx1301_load_all_firmware(spi);
 	if (ret)
-		goto err_load_firmware;
+		return ret;
 
 	dev_info(&spi->dev, "SX1301 module probed\n");
 
 	return 0;
-
-err_load_firmware:
-err_agc_calibrate:
-err_write_clk32m_1:
-err_read_clk32m_1:
-err_write_global_en_1:
-err_read_global_en_1:
-err_write_gpio_select_output:
-err_read_gpio_select_output:
-err_write_gpio_mode:
-err_read_gpio_mode:
-err_radio_b_register:
-err_radio_b_alloc:
-err_radio_a_register:
-err_radio_a_alloc:
-err_write_clk32m_0:
-err_read_clk32m_0:
-err_write_global_en_0:
-err_read_global_en_0:
-err_soft_reset:
-err_init_page:
-err_version:
-err_regmap:
-	free_loradev(netdev);
-err_alloc_loradev:
-	return ret;
 }
 
 static int sx1301_remove(struct spi_device *spi)
-- 
2.7.4

Re: [PATCH lora-next 03/10] net: lora: sx1301: convert to devm registration of netdev

[Andreas Färber]

Am 07.08.2018 um 19:32 schrieb Ben Whitten:
> diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
> index 49958f0..54bfc31 100644
> --- a/drivers/net/lora/sx1301.c
> +++ b/drivers/net/lora/sx1301.c
[...]
> @@ -885,7 +889,7 @@ static int sx1301_probe(struct spi_device *spi)
>  	if (IS_ERR(priv->regmap)) {
>  		ret = PTR_ERR(priv->regmap);
>  		dev_err(&spi->dev, "Regmap allocation failed: %d\n", ret);
> -		return err_regmap;

That hints at a build error in an earlier patch.

Regards,
Andreas

> +		return ret;
>  	}
>  
>  	for (i = 0; i < ARRAY_SIZE(sx1301_reg_fields); i++) {
> @@ -897,38 +901,37 @@ static int sx1301_probe(struct spi_device *spi)
>  		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);
> -			goto err_regmap;
> +			return ret;
[snip]


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

[PATCH lora-next 04/10] net: lora: sx1301: convert probe function to regmap access

[Ben Whitten]

From: Ben Whitten <ben.whitten@lairdtech.com>

Keeping change sets small we convert the probe function over to regmap
and regmap_field access for initialisation.

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

diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index 54bfc31..118e8d8 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -379,11 +379,6 @@ static int sx1301_page_write(struct spi_device *spi, u8 page, u8 reg, u8 val)
 	return sx1301_write(spi, reg, val);
 }
 
-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
@@ -854,7 +849,6 @@ static int sx1301_probe(struct spi_device *spi)
 	int ret;
 	int i;
 	unsigned int ver;
-	u8 val;
 
 	rst = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(rst))
@@ -911,96 +905,46 @@ static int sx1301_probe(struct spi_device *spi)
 		return ret;
 	}
 
-	if (ver != 103) {
+	if (ver != SX1301_CHIP_VERSION) {
 		dev_err(&spi->dev, "unexpected version: %u\n", ver);
 		return -ENXIO;
 	}
 
-	ret = sx1301_write(spi, REG_PAGE_RESET, 0);
+	ret = regmap_write(priv->regmap, SX1301_PAGE, 0);
 	if (ret) {
 		dev_err(&spi->dev, "page/reset write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_soft_reset(spi);
+	ret = sx1301_field_write(priv, F_SOFT_RESET, 1);
 	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_read(spi, 2, 43, &val);
-	if (ret) {
-		dev_err(&spi->dev, "2|43 read failed\n");
+	/* gate clocks */
+	ret = sx1301_field_write(priv, F_GLOBAL_EN, 0);
+	if (ret)
 		return ret;
-	}
-
-	val |= REG_2_43_RADIO_B_EN | REG_2_43_RADIO_A_EN;
-
-	ret = sx1301_page_write(spi, 2, 43, val);
-	if (ret) {
-		dev_err(&spi->dev, "2|43 write failed\n");
+	ret = sx1301_field_write(priv, F_CLK32M_EN, 0);
+	if (ret)
 		return ret;
-	}
 
-	msleep(500);
-
-	ret = sx1301_page_read(spi, 2, 43, &val);
-	if (ret) {
-		dev_err(&spi->dev, "2|43 read failed\n");
+	/* switch on and reset the radios (also starts the 32 MHz XTAL) */
+	ret = sx1301_field_write(priv, F_RADIO_A_EN, 1);
+	if (ret)
 		return ret;
-	}
-
-	val |= REG_2_43_RADIO_RST;
-
-	ret = sx1301_page_write(spi, 2, 43, val);
-	if (ret) {
-		dev_err(&spi->dev, "2|43 write failed\n");
+	ret = sx1301_field_write(priv, F_RADIO_B_EN, 1);
+	if (ret)
 		return ret;
-	}
-
-	msleep(5);
-
-	ret = sx1301_page_read(spi, 2, 43, &val);
-	if (ret) {
-		dev_err(&spi->dev, "2|43 read failed\n");
+	mdelay(500);
+	ret = sx1301_field_write(priv, F_RADIO_RST, 1);
+	if (ret)
 		return ret;
-	}
-
-	val &= ~REG_2_43_RADIO_RST;
-
-	ret = sx1301_page_write(spi, 2, 43, val);
-	if (ret) {
-		dev_err(&spi->dev, "2|43 write failed\n");
+	mdelay(5);
+	ret = sx1301_field_write(priv, F_RADIO_RST, 0);
+	if (ret)
 		return ret;
-	}
 
 	/* radio A */
 
@@ -1047,65 +991,22 @@ static int sx1301_probe(struct spi_device *spi)
 	}
 
 	/* GPIO */
-
-	ret = sx1301_read(spi, REG_GPIO_MODE, &val);
-	if (ret) {
-		dev_err(&spi->dev, "GPIO mode read failed\n");
-		return ret;
-	}
-
-	val |= GENMASK(4, 0);
-
-	ret = sx1301_write(spi, REG_GPIO_MODE, val);
-	if (ret) {
-		dev_err(&spi->dev, "GPIO mode write failed\n");
-		return ret;
-	}
-
-	ret = sx1301_read(spi, REG_GPIO_SELECT_OUTPUT, &val);
-	if (ret) {
-		dev_err(&spi->dev, "GPIO select output read failed\n");
+	ret = regmap_write(priv->regmap, SX1301_GPMODE, 0x1F);
+	if (ret)
 		return ret;
-	}
-
-	val &= ~GENMASK(3, 0);
-	val |= 2;
-
-	ret = sx1301_write(spi, REG_GPIO_SELECT_OUTPUT, val);
-	if (ret) {
-		dev_err(&spi->dev, "GPIO select output write failed\n");
+	ret = regmap_write(priv->regmap, SX1301_GPSO, 0x2);
+	if (ret)
 		return ret;
-	}
 
 	/* TODO LBT */
 
-	ret = sx1301_read(spi, 16, &val);
-	if (ret) {
-		dev_err(&spi->dev, "16 read (1) failed\n");
-		return ret;
-	}
-
-	val |= REG_16_GLOBAL_EN;
-
-	ret = sx1301_write(spi, 16, val);
-	if (ret) {
-		dev_err(&spi->dev, "16 write (1) failed\n");
-		return ret;
-	}
-
-	ret = sx1301_read(spi, 17, &val);
-	if (ret) {
-		dev_err(&spi->dev, "17 read (1) failed\n");
+	/* start clocks */
+	ret = sx1301_field_write(priv, F_GLOBAL_EN, 1);
+	if (ret)
 		return ret;
-	}
-
-	val |= REG_17_CLK32M_EN;
-
-	ret = sx1301_write(spi, 17, val);
-	if (ret) {
-		dev_err(&spi->dev, "17 write (1) failed\n");
+	ret = sx1301_field_write(priv, F_CLK32M_EN, 1);
+	if (ret)
 		return ret;
-	}
 
 	/* calibration */
 
diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
index abfb7b5..0e84703 100644
--- a/drivers/net/lora/sx1301.h
+++ b/drivers/net/lora/sx1301.h
@@ -8,6 +8,13 @@
 #ifndef _SX1301_
 #define _SX1301_
 
+#define SX1301_CHIP_VERSION 103
+
+#define SX1301_MCU_FW_BYTE 8192
+#define SX1301_MCU_ARB_FW_VERSION 1
+#define SX1301_MCU_AGC_FW_VERSION 4
+#define SX1301_MCU_AGC_CAL_FW_VERSION 2
+
 /* Page independent */
 #define SX1301_PAGE     0x00
 #define SX1301_VER      0x01
-- 
2.7.4

[PATCH lora-next 05/10] net: lora: sx1301: add device to private data and convert ram reads

[Ben Whitten]

From: Ben Whitten <ben.whitten@lairdtech.com>

It's easier to simply add a pointer to our device to our private data,
this way we can avoid passing around our spi_device and instead just
pass private data which is more flexible.

As its a two line change its coupled with converting AGC and ARB ram reads
to regmap functions.

Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
---
 drivers/net/lora/sx1301.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index 118e8d8..249551b 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -283,6 +283,7 @@ struct spi_sx1301 {
 };
 
 struct sx1301_priv {
+	struct device *dev;
 	struct lora_priv lora;
 	struct gpio_desc *rst_gpio;
 	u8 cur_page;
@@ -472,40 +473,44 @@ static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl,
 	return 0;
 }
 
-static int sx1301_agc_ram_read(struct spi_device *spi, u8 addr, u8 *val)
+static int sx1301_agc_ram_read(struct sx1301_priv *priv, u8 addr, u8 *val)
 {
 	int ret;
+	unsigned int read;
 
-	ret = sx1301_page_write(spi, 2, REG_2_DBG_AGC_MCU_RAM_ADDR, addr);
+	ret = regmap_write(priv->regmap, SX1301_DBG_AGC_MCU_RAM_ADDR, addr);
 	if (ret) {
-		dev_err(&spi->dev, "AGC RAM addr write failed\n");
+		dev_err(priv->dev, "AGC RAM addr write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_page_read(spi, 2, REG_2_DBG_AGC_MCU_RAM_DATA, val);
+	ret = regmap_read(priv->regmap, SX1301_DBG_AGC_MCU_RAM_DATA, &read);
 	if (ret) {
-		dev_err(&spi->dev, "AGC RAM data read failed\n");
+		dev_err(priv->dev, "AGC RAM data read failed\n");
 		return ret;
 	}
+	*val = read;
 
 	return 0;
 }
 
-static int sx1301_arb_ram_read(struct spi_device *spi, u8 addr, u8 *val)
+static int sx1301_arb_ram_read(struct sx1301_priv *priv, u8 addr, u8 *val)
 {
 	int ret;
+	unsigned int read;
 
-	ret = sx1301_page_write(spi, 2, REG_2_DBG_ARB_MCU_RAM_ADDR, addr);
+	ret = regmap_write(priv->regmap, SX1301_DBG_ARB_MCU_RAM_ADDR, addr);
 	if (ret) {
-		dev_err(&spi->dev, "ARB RAM addr write failed\n");
+		dev_err(priv->dev, "ARB RAM addr write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_page_read(spi, 2, REG_2_DBG_ARB_MCU_RAM_DATA, val);
+	ret = regmap_read(priv->regmap, SX1301_DBG_ARB_MCU_RAM_DATA, &read);
 	if (ret) {
-		dev_err(&spi->dev, "ARB RAM data read failed\n");
+		dev_err(priv->dev, "ARB RAM data read failed\n");
 		return ret;
 	}
+	*val = read;
 
 	return 0;
 }
@@ -604,6 +609,7 @@ static int sx1301_load_firmware(struct spi_device *spi, int mcu, const u8 *data,
 static int sx1301_agc_calibrate(struct spi_device *spi)
 {
 	const struct firmware *fw;
+	struct sx1301_priv *priv = spi_get_drvdata(spi);
 	u8 val;
 	int ret;
 
@@ -663,7 +669,7 @@ static int sx1301_agc_calibrate(struct spi_device *spi)
 		return ret;
 	}
 
-	ret = sx1301_agc_ram_read(spi, 0x20, &val);
+	ret = sx1301_agc_ram_read(priv, 0x20, &val);
 	if (ret) {
 		dev_err(&spi->dev, "AGC RAM data read failed\n");
 		return ret;
@@ -730,6 +736,7 @@ static int sx1301_agc_calibrate(struct spi_device *spi)
 
 static int sx1301_load_all_firmware(struct spi_device *spi)
 {
+	struct sx1301_priv *priv = spi_get_drvdata(spi);
 	const struct firmware *fw;
 	u8 val;
 	int ret;
@@ -802,7 +809,7 @@ static int sx1301_load_all_firmware(struct spi_device *spi)
 		return ret;
 	}
 
-	ret = sx1301_agc_ram_read(spi, 0x20, &val);
+	ret = sx1301_agc_ram_read(priv, 0x20, &val);
 	if (ret) {
 		dev_err(&spi->dev, "AGC RAM data read failed\n");
 		return ret;
@@ -815,7 +822,7 @@ static int sx1301_load_all_firmware(struct spi_device *spi)
 		return -ENXIO;
 	}
 
-	ret = sx1301_arb_ram_read(spi, 0x20, &val);
+	ret = sx1301_arb_ram_read(priv, 0x20, &val);
 	if (ret) {
 		dev_err(&spi->dev, "ARB RAM data read failed\n");
 		return ret;
@@ -878,6 +885,7 @@ static int sx1301_probe(struct spi_device *spi)
 
 	spi_set_drvdata(spi, netdev);
 	SET_NETDEV_DEV(netdev, &spi->dev);
+	priv->dev = &spi->dev;
 
 	priv->regmap = devm_regmap_init_spi(spi, &sx1301_regmap_config);
 	if (IS_ERR(priv->regmap)) {
-- 
2.7.4

[PATCH lora-next 06/10] net: lora: sx1301: simplify firmware loading and convert

[Ben Whitten]

From: Ben Whitten <ben.whitten@lairdtech.com>

As part of changing our internal pointers to our priv data we add the
spi_device temporarily for downstream burst read/writes.
Just pass firmware pointer to the load and remove redundant size checks,
as both MCUs have the same program size add a define.

Use devm_kzalloc for automatic cleanup on failure to load firmware,
manually cleaup this memory on success.

Convert firmware loading to regmap functions.

Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
---
 drivers/net/lora/sx1301.c | 109 ++++++++++++++++------------------------------
 1 file changed, 37 insertions(+), 72 deletions(-)

diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index 249551b..4cec524 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -284,6 +284,7 @@ struct spi_sx1301 {
 
 struct sx1301_priv {
 	struct device *dev;
+	struct spi_device *spi;
 	struct lora_priv lora;
 	struct gpio_desc *rst_gpio;
 	u8 cur_page;
@@ -515,93 +516,73 @@ static int sx1301_arb_ram_read(struct sx1301_priv *priv, u8 addr, u8 *val)
 	return 0;
 }
 
-static int sx1301_load_firmware(struct spi_device *spi, int mcu, const u8 *data, size_t len)
+static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct firmware *fw)
 {
 	u8 *buf;
-	u8 val, rst, select_mux;
 	int ret;
+	unsigned int read;
+	enum sx1301_fields rst, select_mux;
 
-	if (len > 8192)
+	if (fw->size > SX1301_MCU_FW_BYTE)
 		return -EINVAL;
 
 	switch (mcu) {
 	case 0:
-		rst = REG_0_MCU_RST_0;
-		select_mux = REG_0_MCU_SELECT_MUX_0;
+		rst = F_MCU_RST_0;
+		select_mux = F_MCU_SELECT_MUX_0;
 		break;
 	case 1:
-		rst = REG_0_MCU_RST_1;
-		select_mux = REG_0_MCU_SELECT_MUX_1;
+		rst = F_MCU_RST_1;
+		select_mux = F_MCU_SELECT_MUX_1;
 		break;
 	default:
 		return -EINVAL;
 	}
 
-	ret = sx1301_page_read(spi, 0, REG_0_MCU, &val);
-	if (ret) {
-		dev_err(&spi->dev, "MCU read failed\n");
+	ret = sx1301_field_write(priv, rst, 1);
+	if (ret)
 		return ret;
-	}
-
-	val |= rst;
-	val &= ~select_mux;
-
-	ret = sx1301_page_write(spi, 0, REG_0_MCU, val);
-	if (ret) {
-		dev_err(&spi->dev, "MCU reset / select mux write failed\n");
+	ret = sx1301_field_write(priv, select_mux, 0);
+	if (ret)
 		return ret;
-	}
 
-	ret = sx1301_write(spi, REG_MCU_PROM_ADDR, 0);
-	if (ret) {
-		dev_err(&spi->dev, "MCU prom addr write failed\n");
+	/* Load firmware into the data register */
+	ret = regmap_write(priv->regmap, SX1301_MPA, 0);
+	if (ret)
 		return ret;
-	}
 
-	ret = sx1301_write_burst(spi, REG_MCU_PROM_DATA, data, len);
+	ret = sx1301_write_burst(priv->spi, REG_MCU_PROM_DATA, fw->data, fw->size);
 	if (ret) {
-		dev_err(&spi->dev, "MCU prom data write failed\n");
+		dev_err(priv->dev, "MCU prom data write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_read(spi, REG_MCU_PROM_DATA, &val);
+	ret = regmap_read(priv->regmap, SX1301_MPD, &read);
 	if (ret) {
-		dev_err(&spi->dev, "MCU prom data dummy read failed\n");
+		dev_err(priv->dev, "MCU prom data dummy read failed\n");
 		return ret;
 	}
 
-	buf = kzalloc(len, GFP_KERNEL);
+	buf = devm_kzalloc(priv->dev, fw->size, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
-	ret = sx1301_read_burst(spi, REG_MCU_PROM_DATA, buf, len);
+	ret = sx1301_read_burst(priv->spi, REG_MCU_PROM_DATA, buf, fw->size);
 	if (ret) {
-		dev_err(&spi->dev, "MCU prom data read failed\n");
-		kfree(buf);
+		dev_err(priv->dev, "MCU prom data read failed\n");
 		return ret;
 	}
 
-	if (memcmp(data, buf, len)) {
-		dev_err(&spi->dev, "MCU prom data read does not match data written\n");
-		kfree(buf);
+	if (memcmp(fw->data, buf, fw->size)) {
+		dev_err(priv->dev, "MCU prom data read does not match data written\n");
 		return -ENXIO;
 	}
 
-	kfree(buf);
+	devm_kfree(priv->dev, buf);
 
-	ret = sx1301_page_read(spi, 0, REG_0_MCU, &val);
-	if (ret) {
-		dev_err(&spi->dev, "MCU read (1) failed\n");
-		return ret;
-	}
-
-	val |= select_mux;
-
-	ret = sx1301_page_write(spi, 0, REG_0_MCU, val);
-	if (ret) {
-		dev_err(&spi->dev, "MCU reset / select mux write (1) failed\n");
+	ret = sx1301_field_write(priv, select_mux, 1);
+	if (ret)
 		return ret;
-	}
 
 	return 0;
 }
@@ -613,18 +594,13 @@ static int sx1301_agc_calibrate(struct spi_device *spi)
 	u8 val;
 	int ret;
 
-	ret = request_firmware(&fw, "sx1301_agc_calibration.bin", &spi->dev);
+	ret = request_firmware(&fw, "sx1301_agc_calibration.bin", priv->dev);
 	if (ret) {
 		dev_err(&spi->dev, "agc cal firmware file load failed\n");
 		return ret;
 	}
 
-	if (fw->size != 8192) {
-		dev_err(&spi->dev, "unexpected agc cal firmware size\n");
-		return -EINVAL;
-	}
-
-	ret = sx1301_load_firmware(spi, 1, fw->data, fw->size);
+	ret = sx1301_load_firmware(priv, 1, fw);
 	release_firmware(fw);
 	if (ret) {
 		dev_err(&spi->dev, "agc cal firmware load failed\n");
@@ -741,36 +717,24 @@ static int sx1301_load_all_firmware(struct spi_device *spi)
 	u8 val;
 	int ret;
 
-	ret = request_firmware(&fw, "sx1301_arb.bin", &spi->dev);
+	ret = request_firmware(&fw, "sx1301_arb.bin", priv->dev);
 	if (ret) {
-		dev_err(&spi->dev, "arb firmware file load failed\n");
+		dev_err(priv->dev, "arb firmware file load failed\n");
 		return ret;
 	}
 
-	if (fw->size != 8192) {
-		dev_err(&spi->dev, "unexpected arb firmware size\n");
-		release_firmware(fw);
-		return -EINVAL;
-	}
-
-	ret = sx1301_load_firmware(spi, 0, fw->data, fw->size);
+	ret = sx1301_load_firmware(priv, 0, fw);
 	release_firmware(fw);
 	if (ret)
 		return ret;
 
-	ret = request_firmware(&fw, "sx1301_agc.bin", &spi->dev);
+	ret = request_firmware(&fw, "sx1301_agc.bin", priv->dev);
 	if (ret) {
-		dev_err(&spi->dev, "agc firmware file load failed\n");
+		dev_err(priv->dev, "agc firmware file load failed\n");
 		return ret;
 	}
 
-	if (fw->size != 8192) {
-		dev_err(&spi->dev, "unexpected agc firmware size\n");
-		release_firmware(fw);
-		return -EINVAL;
-	}
-
-	ret = sx1301_load_firmware(spi, 1, fw->data, fw->size);
+	ret = sx1301_load_firmware(priv, 1, fw);
 	release_firmware(fw);
 	if (ret)
 		return ret;
@@ -886,6 +850,7 @@ static int sx1301_probe(struct spi_device *spi)
 	spi_set_drvdata(spi, netdev);
 	SET_NETDEV_DEV(netdev, &spi->dev);
 	priv->dev = &spi->dev;
+	priv->spi = spi;
 
 	priv->regmap = devm_regmap_init_spi(spi, &sx1301_regmap_config);
 	if (IS_ERR(priv->regmap)) {
-- 
2.7.4

[PATCH lora-next 07/10] net: lora: sx1301: convert read and write burst to take priv data

[Ben Whitten]

From: Ben Whitten <ben.whitten@lairdtech.com>

As part of standardising on passing our priv data around we convert read
and write burst to take the priv data, there is a small compat step
needed in the old _read and _write functions and will be removed in the
next step.

Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
---
 drivers/net/lora/sx1301.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index 4cec524..ab45b5b 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -312,18 +312,19 @@ static int sx1301_field_write(struct sx1301_priv *priv,
 	return regmap_field_write(priv->regmap_fields[field_id], val);
 }
 
-static int sx1301_read_burst(struct spi_device *spi, u8 reg, u8 *val, size_t len)
+static int sx1301_read_burst(struct sx1301_priv *priv, u8 reg, u8 *val, size_t len)
 {
 	u8 addr = reg & 0x7f;
-	return spi_write_then_read(spi, &addr, 1, val, len);
+	return spi_write_then_read(priv->spi, &addr, 1, val, len);
 }
 
 static int sx1301_read(struct spi_device *spi, u8 reg, u8 *val)
 {
-	return sx1301_read_burst(spi, reg, val, 1);
+	struct sx1301_priv *priv = spi_get_drvdata(spi);
+	return sx1301_read_burst(priv, reg, val, 1);
 }
 
-static int sx1301_write_burst(struct spi_device *spi, u8 reg, const u8 *val, size_t len)
+static int sx1301_write_burst(struct sx1301_priv *priv, u8 reg, const u8 *val, size_t len)
 {
 	u8 addr = reg | BIT(7);
 	struct spi_transfer xfr[2] = {
@@ -331,12 +332,13 @@ static int sx1301_write_burst(struct spi_device *spi, u8 reg, const u8 *val, siz
 		{ .tx_buf = val, .len = len },
 	};
 
-	return spi_sync_transfer(spi, xfr, 2);
+	return spi_sync_transfer(priv->spi, xfr, 2);
 }
 
 static int sx1301_write(struct spi_device *spi, u8 reg, u8 val)
 {
-	return sx1301_write_burst(spi, reg, &val, 1);
+	struct sx1301_priv *priv = spi_get_drvdata(spi);
+	return sx1301_write_burst(priv, reg, &val, 1);
 }
 
 static int sx1301_page_switch(struct spi_device *spi, u8 page)
@@ -551,7 +553,7 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct
 	if (ret)
 		return ret;
 
-	ret = sx1301_write_burst(priv->spi, REG_MCU_PROM_DATA, fw->data, fw->size);
+	ret = sx1301_write_burst(priv, REG_MCU_PROM_DATA, fw->data, fw->size);
 	if (ret) {
 		dev_err(priv->dev, "MCU prom data write failed\n");
 		return ret;
@@ -567,7 +569,7 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct
 	if (!buf)
 		return -ENOMEM;
 
-	ret = sx1301_read_burst(priv->spi, REG_MCU_PROM_DATA, buf, fw->size);
+	ret = sx1301_read_burst(priv, REG_MCU_PROM_DATA, buf, fw->size);
 	if (ret) {
 		dev_err(priv->dev, "MCU prom data read failed\n");
 		return ret;
-- 
2.7.4

[PATCH lora-next 08/10] net: lora: sx1301: convert read and write to priv pointer

[Ben Whitten]

From: Ben Whitten <ben.whitten@lairdtech.com>

Convert the next layer of read and writes to our private pointer,
once again compatiblity later in the page read and write layer.
The page functions will be removed with this full stack as page switches
are handled by regmap automatically.

Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
---
 drivers/net/lora/sx1301.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index ab45b5b..f84a6ce 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -318,9 +318,8 @@ static int sx1301_read_burst(struct sx1301_priv *priv, u8 reg, u8 *val, size_t l
 	return spi_write_then_read(priv->spi, &addr, 1, val, len);
 }
 
-static int sx1301_read(struct spi_device *spi, u8 reg, u8 *val)
+static int sx1301_read(struct sx1301_priv *priv, u8 reg, u8 *val)
 {
-	struct sx1301_priv *priv = spi_get_drvdata(spi);
 	return sx1301_read_burst(priv, reg, val, 1);
 }
 
@@ -335,9 +334,8 @@ static int sx1301_write_burst(struct sx1301_priv *priv, u8 reg, const u8 *val, s
 	return spi_sync_transfer(priv->spi, xfr, 2);
 }
 
-static int sx1301_write(struct spi_device *spi, u8 reg, u8 val)
+static int sx1301_write(struct sx1301_priv *priv, u8 reg, u8 val)
 {
-	struct sx1301_priv *priv = spi_get_drvdata(spi);
 	return sx1301_write_burst(priv, reg, &val, 1);
 }
 
@@ -350,7 +348,7 @@ static int sx1301_page_switch(struct spi_device *spi, u8 page)
 		return 0;
 
 	dev_dbg(&spi->dev, "switching to page %u\n", (unsigned)page);
-	ret = sx1301_write(spi, REG_PAGE_RESET, page & 0x3);
+	ret = sx1301_write(priv, REG_PAGE_RESET, page & 0x3);
 	if (ret) {
 		dev_err(&spi->dev, "switching to page %u failed\n", (unsigned)page);
 		return ret;
@@ -364,23 +362,25 @@ static int sx1301_page_switch(struct spi_device *spi, u8 page)
 static int sx1301_page_read(struct spi_device *spi, u8 page, u8 reg, u8 *val)
 {
 	int ret;
+	struct sx1301_priv *priv = spi_get_drvdata(spi);
 
 	ret = sx1301_page_switch(spi, page);
 	if (ret)
 		return ret;
 
-	return sx1301_read(spi, reg, val);
+	return sx1301_read(priv, reg, val);
 }
 
 static int sx1301_page_write(struct spi_device *spi, u8 page, u8 reg, u8 val)
 {
 	int ret;
+	struct sx1301_priv *priv = spi_get_drvdata(spi);
 
 	ret = sx1301_page_switch(spi, page);
 	if (ret)
 		return ret;
 
-	return sx1301_write(spi, reg, val);
+	return sx1301_write(priv, reg, val);
 }
 
 #define REG_RADIO_X_DATA		0
@@ -666,7 +666,7 @@ static int sx1301_agc_calibrate(struct spi_device *spi)
 		return ret;
 	}
 
-	ret = sx1301_read(spi, REG_EMERGENCY_FORCE, &val);
+	ret = sx1301_read(priv, REG_EMERGENCY_FORCE, &val);
 	if (ret) {
 		dev_err(&spi->dev, "emergency force read failed\n");
 		return ret;
@@ -674,7 +674,7 @@ static int sx1301_agc_calibrate(struct spi_device *spi)
 
 	val &= ~REG_EMERGENCY_FORCE_HOST_CTRL;
 
-	ret = sx1301_write(spi, REG_EMERGENCY_FORCE, val);
+	ret = sx1301_write(priv, REG_EMERGENCY_FORCE, val);
 	if (ret) {
 		dev_err(&spi->dev, "emergency force write failed\n");
 		return ret;
@@ -683,7 +683,7 @@ static int sx1301_agc_calibrate(struct spi_device *spi)
 	dev_err(&spi->dev, "starting calibration...\n");
 	msleep(2300);
 
-	ret = sx1301_read(spi, REG_EMERGENCY_FORCE, &val);
+	ret = sx1301_read(priv, REG_EMERGENCY_FORCE, &val);
 	if (ret) {
 		dev_err(&spi->dev, "emergency force read (1) failed\n");
 		return ret;
@@ -691,13 +691,13 @@ static int sx1301_agc_calibrate(struct spi_device *spi)
 
 	val |= REG_EMERGENCY_FORCE_HOST_CTRL;
 
-	ret = sx1301_write(spi, REG_EMERGENCY_FORCE, val);
+	ret = sx1301_write(priv, REG_EMERGENCY_FORCE, val);
 	if (ret) {
 		dev_err(&spi->dev, "emergency force write (1) failed\n");
 		return ret;
 	}
 
-	ret = sx1301_read(spi, REG_MCU_AGC_STATUS, &val);
+	ret = sx1301_read(priv, REG_MCU_AGC_STATUS, &val);
 	if (ret) {
 		dev_err(&spi->dev, "AGC status read failed\n");
 		return ret;
-- 
2.7.4

[PATCH lora-next 09/10] net: lora: sx1301: convert agc calibrate to regmap functions

[Ben Whitten]

From: Ben Whitten <ben.whitten@lairdtech.com>

We convert the AGC calibration function over to regmap and take the
opportunity to make the expected firmware versions defines.
Switch to taking a pointer to our private data.

Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
---
 drivers/net/lora/sx1301.c | 101 ++++++++++++----------------------------------
 1 file changed, 26 insertions(+), 75 deletions(-)

diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index f84a6ce..21b3f9a 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -589,123 +589,74 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct
 	return 0;
 }
 
-static int sx1301_agc_calibrate(struct spi_device *spi)
+static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 {
 	const struct firmware *fw;
-	struct sx1301_priv *priv = spi_get_drvdata(spi);
 	u8 val;
 	int ret;
+	unsigned int cal;
 
 	ret = request_firmware(&fw, "sx1301_agc_calibration.bin", priv->dev);
 	if (ret) {
-		dev_err(&spi->dev, "agc cal firmware file load failed\n");
+		dev_err(priv->dev, "AGC CAL firmware file load failed\n");
 		return ret;
 	}
 
 	ret = sx1301_load_firmware(priv, 1, fw);
 	release_firmware(fw);
 	if (ret) {
-		dev_err(&spi->dev, "agc cal firmware load failed\n");
-		return ret;
-	}
-
-	ret = sx1301_page_read(spi, 0, 105, &val);
-	if (ret) {
-		dev_err(&spi->dev, "0|105 read failed\n");
+		dev_err(priv->dev, "AGC CAL firmware load failed\n");
 		return ret;
 	}
 
-	val &= ~REG_0_105_FORCE_HOST_RADIO_CTRL;
-
-	ret = sx1301_page_write(spi, 0, 105, val);
-	if (ret) {
-		dev_err(&spi->dev, "0|105 write failed\n");
+	ret = sx1301_field_write(priv, F_FORCE_HOST_RADIO_CTRL, 0);
+	if (ret)
 		return ret;
-	}
 
 	val = BIT(4); /* with DAC gain=3 */
 	if (false)
 		val |= BIT(5); /* SX1255 */
 
-	ret = sx1301_page_write(spi, 0, REG_0_RADIO_SELECT, val);
-	if (ret) {
-		dev_err(&spi->dev, "radio select write failed\n");
-		return ret;
-	}
-
-	ret = sx1301_page_read(spi, 0, REG_0_MCU, &val);
-	if (ret) {
-		dev_err(&spi->dev, "MCU read (0) failed\n");
+	ret = regmap_write(priv->regmap, SX1301_CHRS, val);
+	if (ret)
 		return ret;
-	}
-
-	val &= ~REG_0_MCU_RST_1;
 
-	ret = sx1301_page_write(spi, 0, REG_0_MCU, val);
-	if (ret) {
-		dev_err(&spi->dev, "MCU write (0) failed\n");
+	ret = sx1301_field_write(priv, F_MCU_RST_1, 0);
+	if (ret)
 		return ret;
-	}
 
 	ret = sx1301_agc_ram_read(priv, 0x20, &val);
 	if (ret) {
-		dev_err(&spi->dev, "AGC RAM data read failed\n");
+		dev_err(priv->dev, "AGC RAM data read failed\n");
 		return ret;
 	}
 
-	dev_info(&spi->dev, "AGC calibration firmware version %u\n", (unsigned)val);
+	dev_info(priv->dev, "AGC calibration firmware version %u\n", (unsigned)val);
 
-	if (val != 2) {
-		dev_err(&spi->dev, "unexpected firmware version, expecting %u\n", 2);
+	if (val != SX1301_MCU_AGC_CAL_FW_VERSION) {
+		dev_err(priv->dev, "unexpected firmware version, expecting %u\n",
+				SX1301_MCU_AGC_CAL_FW_VERSION);
 		return -ENXIO;
 	}
 
-	ret = sx1301_page_switch(spi, 3);
-	if (ret) {
-		dev_err(&spi->dev, "page switch 3 failed\n");
-		return ret;
-	}
-
-	ret = sx1301_read(priv, REG_EMERGENCY_FORCE, &val);
-	if (ret) {
-		dev_err(&spi->dev, "emergency force read failed\n");
-		return ret;
-	}
-
-	val &= ~REG_EMERGENCY_FORCE_HOST_CTRL;
-
-	ret = sx1301_write(priv, REG_EMERGENCY_FORCE, val);
-	if (ret) {
-		dev_err(&spi->dev, "emergency force write failed\n");
+	ret = sx1301_field_write(priv, F_EMERGENCY_FORCE_HOST_CTRL, 0);
+	if (ret)
 		return ret;
-	}
 
-	dev_err(&spi->dev, "starting calibration...\n");
+	dev_err(priv->dev, "starting calibration...\n");
 	msleep(2300);
 
-	ret = sx1301_read(priv, REG_EMERGENCY_FORCE, &val);
-	if (ret) {
-		dev_err(&spi->dev, "emergency force read (1) failed\n");
-		return ret;
-	}
-
-	val |= REG_EMERGENCY_FORCE_HOST_CTRL;
-
-	ret = sx1301_write(priv, REG_EMERGENCY_FORCE, val);
-	if (ret) {
-		dev_err(&spi->dev, "emergency force write (1) failed\n");
+	ret = sx1301_field_write(priv, F_EMERGENCY_FORCE_HOST_CTRL, 1);
+	if (ret)
 		return ret;
-	}
 
-	ret = sx1301_read(priv, REG_MCU_AGC_STATUS, &val);
-	if (ret) {
-		dev_err(&spi->dev, "AGC status read failed\n");
+	ret = regmap_read(priv->regmap, SX1301_AGCSTS, &cal);
+	if (ret)
 		return ret;
-	}
 
-	dev_info(&spi->dev, "AGC status: %02x\n", (unsigned)val);
-	if ((val & (BIT(7) | BIT(0))) != (BIT(7) | BIT(0))) {
-		dev_err(&spi->dev, "AGC calibration failed\n");
+	dev_info(priv->dev, "AGC status: %02x\n", cal);
+	if ((cal & (BIT(7) | BIT(0))) != (BIT(7) | BIT(0))) {
+		dev_err(priv->dev, "AGC calibration failed\n");
 		return -ENXIO;
 	}
 
@@ -985,7 +936,7 @@ static int sx1301_probe(struct spi_device *spi)
 
 	/* calibration */
 
-	ret = sx1301_agc_calibrate(spi);
+	ret = sx1301_agc_calibrate(priv);
 	if (ret)
 		return ret;
 
-- 
2.7.4

[PATCH lora-next 10/10] net: lora: sx1301: convert all firmware to regmap

[Ben Whitten]

The last stage in the probe function loads the final firmwares to
the sx1301.

Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
---
 drivers/net/lora/sx1301.c | 67 ++++++++++++++++++++---------------------------
 1 file changed, 28 insertions(+), 39 deletions(-)

diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index 21b3f9a..7f4bfa1 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -663,16 +663,15 @@ static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 	return 0;
 }
 
-static int sx1301_load_all_firmware(struct spi_device *spi)
+static int sx1301_load_all_firmware(struct sx1301_priv *priv)
 {
-	struct sx1301_priv *priv = spi_get_drvdata(spi);
 	const struct firmware *fw;
 	u8 val;
 	int ret;
 
 	ret = request_firmware(&fw, "sx1301_arb.bin", priv->dev);
 	if (ret) {
-		dev_err(priv->dev, "arb firmware file load failed\n");
+		dev_err(priv->dev, "ARB firmware file load failed\n");
 		return ret;
 	}
 
@@ -683,7 +682,7 @@ static int sx1301_load_all_firmware(struct spi_device *spi)
 
 	ret = request_firmware(&fw, "sx1301_agc.bin", priv->dev);
 	if (ret) {
-		dev_err(priv->dev, "agc firmware file load failed\n");
+		dev_err(priv->dev, "AGC firmware file load failed\n");
 		return ret;
 	}
 
@@ -692,63 +691,53 @@ static int sx1301_load_all_firmware(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	ret = sx1301_page_read(spi, 0, 105, &val);
-	if (ret) {
-		dev_err(&spi->dev, "0|105 read failed\n");
+	ret = sx1301_field_write(priv, F_FORCE_HOST_RADIO_CTRL, 0);
+	if (ret)
 		return ret;
-	}
-
-	val &= ~(REG_0_105_FORCE_HOST_RADIO_CTRL | REG_0_105_FORCE_HOST_FE_CTRL | REG_0_105_FORCE_DEC_FILTER_GAIN);
-
-	ret = sx1301_page_write(spi, 0, 105, val);
-	if (ret) {
-		dev_err(&spi->dev, "0|105 write failed\n");
+	ret = sx1301_field_write(priv, F_FORCE_HOST_FE_CTRL, 0);
+	if (ret)
 		return ret;
-	}
-
-	ret = sx1301_page_write(spi, 0, REG_0_RADIO_SELECT, 0);
-	if (ret) {
-		dev_err(&spi->dev, "radio select write failed\n");
+	ret = sx1301_field_write(priv, F_FORCE_DEC_FILTER_GAIN, 0);
+	if (ret)
 		return ret;
-	}
 
-	ret = sx1301_page_read(spi, 0, REG_0_MCU, &val);
-	if (ret) {
-		dev_err(&spi->dev, "MCU read (0) failed\n");
+	ret = regmap_write(priv->regmap, SX1301_CHRS, 0);
+	if (ret)
 		return ret;
-	}
-
-	val &= ~(REG_0_MCU_RST_1 | REG_0_MCU_RST_0);
 
-	ret = sx1301_page_write(spi, 0, REG_0_MCU, val);
-	if (ret) {
-		dev_err(&spi->dev, "MCU write (0) failed\n");
+	/* Release the CPUs */
+	ret = sx1301_field_write(priv, F_MCU_RST_0, 0);
+	if (ret)
+		return ret;
+	ret = sx1301_field_write(priv, F_MCU_RST_1, 0);
+	if (ret)
 		return ret;
-	}
 
 	ret = sx1301_agc_ram_read(priv, 0x20, &val);
 	if (ret) {
-		dev_err(&spi->dev, "AGC RAM data read failed\n");
+		dev_err(priv->dev, "AGC RAM data read failed\n");
 		return ret;
 	}
 
-	dev_info(&spi->dev, "AGC firmware version %u\n", (unsigned)val);
+	dev_info(priv->dev, "AGC firmware version %u\n", (unsigned)val);
 
-	if (val != 4) {
-		dev_err(&spi->dev, "unexpected firmware version, expecting %u\n", 4);
+	if (val != SX1301_MCU_AGC_FW_VERSION) {
+		dev_err(priv->dev, "unexpected firmware version, expecting %u\n",
+				SX1301_MCU_AGC_FW_VERSION);
 		return -ENXIO;
 	}
 
 	ret = sx1301_arb_ram_read(priv, 0x20, &val);
 	if (ret) {
-		dev_err(&spi->dev, "ARB RAM data read failed\n");
+		dev_err(priv->dev, "ARB RAM data read failed\n");
 		return ret;
 	}
 
-	dev_info(&spi->dev, "ARB firmware version %u\n", (unsigned)val);
+	dev_info(priv->dev, "ARB firmware version %u\n", (unsigned)val);
 
-	if (val != 1) {
-		dev_err(&spi->dev, "unexpected firmware version, expecting %u\n", 1);
+	if (val != SX1301_MCU_ARB_FW_VERSION) {
+		dev_err(priv->dev, "unexpected firmware version, expecting %u\n",
+				SX1301_MCU_ARB_FW_VERSION);
 		return -ENXIO;
 	}
 
@@ -942,7 +931,7 @@ static int sx1301_probe(struct spi_device *spi)
 
 	/* TODO */
 
-	ret = sx1301_load_all_firmware(spi);
+	ret = sx1301_load_all_firmware(priv);
 	if (ret)
 		return ret;
 
-- 
2.7.4