[PATCH v2 lora-next 0/4] migration to regmap and clk framework

[PATCH v2 lora-next 0/4] migration to regmap and clk framework

[Ben Whitten]

This series completes the sx1301 migration to regmap by replaceing the SPI
burst read and write with regmap raw reads and writes, this solution is a work
around until there is regmap_noinc_read/write API merged as it sepends on the
fact that we have the regcache turned off.
regmap_noinc_read is available in 4.19 however a write varient isn't.

The various fields of the sx1301 are converted to regmap to pass the read
modify write down to the regmap layer and reduce our code.
The downside is that in the cases of the hand optimised multiple field writes
at once within one register, however if it is desirable these can be replaced
with regmap_update_bits.

Apply the same regmap treatment for the radio device and allow it to register as
a clock provider, this clock is enabled when the lora link is brought up as it
is required prior to calibration of the concentrator.

modprobe lora-sx125x
lora-dev: init
sx1301 spi0.0: SX1301 module probed
sx125x_con spi0.0-a: SX125x version: 21
sx125x_con spi0.0-a: SX125x module probed
sx125x_con spi0.0-b: SX125x version: 21
sx125x_con spi0.0-b: registering clkout
sx125x_con spi0.0-b: SX125x module probed

ip link set lora0 up
sx125x_con spi0.0-b: enabling clkout

Ben Whitten (4):
  net: lora: sx1301: convert burst spi functions to regmap raw
  net: lora: sx1301: convert to using regmap fields for bit ops
  net: lora: sx125x: convert to regmap fields
  net: lora: sx125x sx1301: allow radio to register as a clk provider

 drivers/net/lora/sx125x.c | 159 ++++++++++++++++++++++++++--
 drivers/net/lora/sx1301.c | 265 ++++++++++++++++------------------------------
 drivers/net/lora/sx1301.h |  50 ++++++++-
 3 files changed, 287 insertions(+), 187 deletions(-)

-- 
2.7.4

[PATCH v2 lora-next 1/4] net: lora: sx1301: convert burst spi functions to regmap raw

[Ben Whitten]

As we have caching disabled we can access the regmap using raw for our
firmware reading and writing bursts.
We also remove the now defunct spi element from the structure as this
completes the move to regmap.

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

diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index fd29258..5ab0e2d 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -76,19 +76,28 @@ static struct regmap_config sx1301_regmap_config = {
 
 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(priv->spi, &addr, 1, val, len);
+	size_t max;
+
+	max = regmap_get_raw_read_max(priv->regmap);
+	if (max && max < len) {
+		dev_err(priv->dev, "Burst greater then max raw read\n");
+		return -EINVAL;
+	}
+
+	return regmap_raw_read(priv->regmap, reg, val, 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] = {
-		{ .tx_buf = &addr, .len = 1 },
-		{ .tx_buf = val, .len = len },
-	};
+	size_t max;
+
+	max = regmap_get_raw_write_max(priv->regmap);
+	if (max && max < len) {
+		dev_err(priv->dev, "Burst greater then max raw write\n");
+		return -EINVAL;
+	}
 
-	return spi_sync_transfer(priv->spi, xfr, 2);
+	return regmap_raw_write(priv->regmap, reg, val, len);
 }
 
 static int sx1301_soft_reset(struct sx1301_priv *priv)
@@ -566,7 +575,6 @@ static int sx1301_probe(struct spi_device *spi)
 
 	spi_set_drvdata(spi, netdev);
 	priv->dev = &spi->dev;
-	priv->spi = spi;
 
 	priv->regmap = devm_regmap_init_spi(spi, &sx1301_regmap_config);
 	if (IS_ERR(priv->regmap)) {
diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
index e939c02..e6400f8 100644
--- a/drivers/net/lora/sx1301.h
+++ b/drivers/net/lora/sx1301.h
@@ -12,7 +12,6 @@
 #include <linux/regmap.h>
 #include <linux/gpio/consumer.h>
 #include <linux/lora/dev.h>
-#include <linux/spi/spi.h>
 
 #define SX1301_CHIP_VERSION 103
 
@@ -64,7 +63,6 @@
 struct sx1301_priv {
 	struct lora_dev_priv lora;
 	struct device		*dev;
-	struct spi_device	*spi;
 	struct gpio_desc *rst_gpio;
 	struct regmap		*regmap;
 };
-- 
2.7.4

[PATCH v2 lora-next 2/4] net: lora: sx1301: convert to using regmap fields for bit ops

[Ben Whitten]

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

We convert to using regmap fields to allow bit access to the registers
where regmap handles the read update write.

Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
---
 drivers/net/lora/sx1301.c | 240 +++++++++++++---------------------------------
 drivers/net/lora/sx1301.h |  46 +++++++++
 2 files changed, 113 insertions(+), 173 deletions(-)

diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index 5ab0e2d..eb89af6 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -24,27 +24,6 @@
 
 #include "sx1301.h"
 
-#define REG_PAGE_RESET_SOFT_RESET	BIT(7)
-
-#define REG_16_GLOBAL_EN		BIT(3)
-
-#define REG_17_CLK32M_EN		BIT(0)
-
-#define REG_0_105_FORCE_HOST_RADIO_CTRL		BIT(1)
-#define REG_0_105_FORCE_HOST_FE_CTRL		BIT(2)
-#define REG_0_105_FORCE_DEC_FILTER_GAIN		BIT(3)
-
-#define REG_0_MCU_RST_0			BIT(0)
-#define REG_0_MCU_RST_1			BIT(1)
-#define REG_0_MCU_SELECT_MUX_0		BIT(2)
-#define REG_0_MCU_SELECT_MUX_1		BIT(3)
-
-#define REG_2_43_RADIO_A_EN		BIT(0)
-#define REG_2_43_RADIO_B_EN		BIT(1)
-#define REG_2_43_RADIO_RST		BIT(2)
-
-#define REG_EMERGENCY_FORCE_HOST_CTRL	BIT(0)
-
 static const struct regmap_range_cfg sx1301_regmap_ranges[] = {
 	{
 		.name = "Pages",
@@ -74,6 +53,12 @@ static struct regmap_config sx1301_regmap_config = {
 	.max_register = SX1301_MAX_REGISTER,
 };
 
+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 sx1301_priv *priv, u8 reg, u8 *val, size_t len)
 {
 	size_t max;
@@ -100,11 +85,6 @@ static int sx1301_write_burst(struct sx1301_priv *priv, u8 reg, const u8 *val, s
 	return regmap_raw_write(priv->regmap, reg, val, len);
 }
 
-static int sx1301_soft_reset(struct sx1301_priv *priv)
-{
-	return regmap_write(priv->regmap, SX1301_PAGE, REG_PAGE_RESET_SOFT_RESET);
-}
-
 static int sx1301_agc_ram_read(struct sx1301_priv *priv, u8 addr, unsigned int *val)
 {
 	int ret;
@@ -146,7 +126,7 @@ static int sx1301_arb_ram_read(struct sx1301_priv *priv, u8 addr, unsigned int *
 static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct firmware *fw)
 {
 	u8 *buf;
-	u8 rst, select_mux;
+	enum sx1301_fields rst, select_mux;
 	unsigned int val;
 	int ret;
 
@@ -157,29 +137,26 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct
 
 	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 = regmap_read(priv->regmap, SX1301_MCU_CTRL, &val);
+	ret = sx1301_field_write(priv, rst, 1);
 	if (ret) {
-		dev_err(priv->dev, "MCU read failed\n");
+		dev_err(priv->dev, "MCU reset failed\n");
 		return ret;
 	}
 
-	val |= rst;
-	val &= ~select_mux;
-
-	ret = regmap_write(priv->regmap, SX1301_MCU_CTRL, val);
+	ret = sx1301_field_write(priv, select_mux, 0);
 	if (ret) {
-		dev_err(priv->dev, "MCU reset / select mux write failed\n");
+		dev_err(priv->dev, "MCU RAM select mux failed\n");
 		return ret;
 	}
 
@@ -220,17 +197,9 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct
 
 	kfree(buf);
 
-	ret = regmap_read(priv->regmap, SX1301_MCU_CTRL, &val);
+	ret = sx1301_field_write(priv, select_mux, 1);
 	if (ret) {
-		dev_err(priv->dev, "MCU read (1) failed\n");
-		return ret;
-	}
-
-	val |= select_mux;
-
-	ret = regmap_write(priv->regmap, SX1301_MCU_CTRL, val);
-	if (ret) {
-		dev_err(priv->dev, "MCU reset / select mux write (1) failed\n");
+		dev_err(priv->dev, "MCU RAM release mux failed\n");
 		return ret;
 	}
 
@@ -256,17 +225,9 @@ static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 		return ret;
 	}
 
-	ret = regmap_read(priv->regmap, SX1301_FORCE_CTRL, &val);
+	ret = sx1301_field_write(priv, F_FORCE_HOST_RADIO_CTRL, 0);
 	if (ret) {
-		dev_err(priv->dev, "0|105 read failed\n");
-		return ret;
-	}
-
-	val &= ~REG_0_105_FORCE_HOST_RADIO_CTRL;
-
-	ret = regmap_write(priv->regmap, SX1301_FORCE_CTRL, val);
-	if (ret) {
-		dev_err(priv->dev, "0|105 write failed\n");
+		dev_err(priv->dev, "force host control failed\n");
 		return ret;
 	}
 
@@ -280,17 +241,9 @@ static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 		return ret;
 	}
 
-	ret = regmap_read(priv->regmap, SX1301_MCU_CTRL, &val);
+	ret = sx1301_field_write(priv, F_MCU_RST_1, 0);
 	if (ret) {
-		dev_err(priv->dev, "MCU read (0) failed\n");
-		return ret;
-	}
-
-	val &= ~REG_0_MCU_RST_1;
-
-	ret = regmap_write(priv->regmap, SX1301_MCU_CTRL, val);
-	if (ret) {
-		dev_err(priv->dev, "MCU write (0) failed\n");
+		dev_err(priv->dev, "MCU 1 reset failed\n");
 		return ret;
 	}
 
@@ -308,34 +261,18 @@ static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 		return -ENXIO;
 	}
 
-	ret = regmap_read(priv->regmap, SX1301_EMERGENCY_FORCE_HOST_CTRL, &val);
+	ret = sx1301_field_write(priv, F_EMERGENCY_FORCE_HOST_CTRL, 0);
 	if (ret) {
-		dev_err(priv->dev, "emergency force read failed\n");
-		return ret;
-	}
-
-	val &= ~REG_EMERGENCY_FORCE_HOST_CTRL;
-
-	ret = regmap_write(priv->regmap, SX1301_EMERGENCY_FORCE_HOST_CTRL, val);
-	if (ret) {
-		dev_err(priv->dev, "emergency force write failed\n");
+		dev_err(priv->dev, "emergency force failed\n");
 		return ret;
 	}
 
 	dev_err(priv->dev, "starting calibration...\n");
 	msleep(2300);
 
-	ret = regmap_read(priv->regmap, SX1301_EMERGENCY_FORCE_HOST_CTRL, &val);
-	if (ret) {
-		dev_err(priv->dev, "emergency force read (1) failed\n");
-		return ret;
-	}
-
-	val |= REG_EMERGENCY_FORCE_HOST_CTRL;
-
-	ret = regmap_write(priv->regmap, SX1301_EMERGENCY_FORCE_HOST_CTRL, val);
+	ret = sx1301_field_write(priv, F_EMERGENCY_FORCE_HOST_CTRL, 1);
 	if (ret) {
-		dev_err(priv->dev, "emergency force write (1) failed\n");
+		dev_err(priv->dev, "emergency force release failed\n");
 		return ret;
 	}
 
@@ -382,19 +319,15 @@ static int sx1301_load_all_firmware(struct sx1301_priv *priv)
 	if (ret)
 		return ret;
 
-	ret = regmap_read(priv->regmap, SX1301_FORCE_CTRL, &val);
-	if (ret) {
-		dev_err(priv->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 = regmap_write(priv->regmap, SX1301_FORCE_CTRL, val);
-	if (ret) {
-		dev_err(priv->dev, "0|105 write failed\n");
+	ret = sx1301_field_write(priv, F_FORCE_HOST_FE_CTRL, 0);
+	if (ret)
+		return ret;
+	ret = sx1301_field_write(priv, F_FORCE_DEC_FILTER_GAIN, 0);
+	if (ret)
 		return ret;
-	}
 
 	ret = regmap_write(priv->regmap, SX1301_CHRS, 0);
 	if (ret) {
@@ -402,17 +335,15 @@ static int sx1301_load_all_firmware(struct sx1301_priv *priv)
 		return ret;
 	}
 
-	ret = regmap_read(priv->regmap, SX1301_MCU_CTRL, &val);
+	ret = sx1301_field_write(priv, F_MCU_RST_0, 0);
 	if (ret) {
-		dev_err(priv->dev, "MCU read (0) failed\n");
+		dev_err(priv->dev, "MCU 0 release failed\n");
 		return ret;
 	}
 
-	val &= ~(REG_0_MCU_RST_1 | REG_0_MCU_RST_0);
-
-	ret = regmap_write(priv->regmap, SX1301_MCU_CTRL, val);
+	ret = sx1301_field_write(priv, F_MCU_RST_1, 0);
 	if (ret) {
-		dev_err(priv->dev, "MCU write (0) failed\n");
+		dev_err(priv->dev, "MCU 1 release failed\n");
 		return ret;
 	}
 
@@ -464,7 +395,6 @@ static netdev_tx_t sx130x_loradev_start_xmit(struct sk_buff *skb, struct net_dev
 static int sx130x_loradev_open(struct net_device *netdev)
 {
 	struct sx1301_priv *priv = netdev_priv(netdev);
-	unsigned int val;
 	int ret;
 
 	netdev_dbg(netdev, "%s", __func__);
@@ -474,31 +404,15 @@ static int sx130x_loradev_open(struct net_device *netdev)
 		return -ENXIO;
 	}
 
-	ret = regmap_read(priv->regmap, SX1301_GEN, &val);
+	ret = sx1301_field_write(priv, F_GLOBAL_EN, 1);
 	if (ret) {
-		netdev_err(netdev, "16 read (1) failed\n");
+		dev_err(priv->dev, "enable global clocks failed\n");
 		return ret;
 	}
 
-	val |= REG_16_GLOBAL_EN;
-
-	ret = regmap_write(priv->regmap, SX1301_GEN, val);
+	ret = sx1301_field_write(priv, F_CLK32M_EN, 1);
 	if (ret) {
-		netdev_err(netdev, "16 write (1) failed\n");
-		return ret;
-	}
-
-	ret = regmap_read(priv->regmap, SX1301_CKEN, &val);
-	if (ret) {
-		netdev_err(netdev, "17 read (1) failed\n");
-		return ret;
-	}
-
-	val |= REG_17_CLK32M_EN;
-
-	ret = regmap_write(priv->regmap, SX1301_CKEN, val);
-	if (ret) {
-		netdev_err(netdev, "17 write (1) failed\n");
+		dev_err(priv->dev, "enable 32M clock failed\n");
 		return ret;
 	}
 
@@ -545,6 +459,7 @@ static int sx1301_probe(struct spi_device *spi)
 	struct sx1301_priv *priv;
 	struct gpio_desc *rst;
 	int ret;
+	int i;
 	unsigned int ver;
 	unsigned int val;
 
@@ -583,6 +498,19 @@ static int sx1301_probe(struct spi_device *spi)
 		return ret;
 	}
 
+	for (i = 0; i < ARRAY_SIZE(sx1301_regmap_fields); i++) {
+		const struct reg_field *reg_fields = sx1301_regmap_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);
+			return ret;
+		}
+	}
+
 	ret = regmap_read(priv->regmap, SX1301_VER, &ver);
 	if (ret) {
 		dev_err(&spi->dev, "version read failed\n");
@@ -600,83 +528,49 @@ static int sx1301_probe(struct spi_device *spi)
 		return ret;
 	}
 
-	ret = sx1301_soft_reset(priv);
+	ret = sx1301_field_write(priv, F_SOFT_RESET, 1);
 	if (ret) {
 		dev_err(&spi->dev, "soft reset failed\n");
 		return ret;
 	}
 
-	ret = regmap_read(priv->regmap, SX1301_GEN, &val);
+	ret = sx1301_field_write(priv, F_GLOBAL_EN, 0);
 	if (ret) {
-		dev_err(&spi->dev, "16 read failed\n");
+		dev_err(&spi->dev, "gate global clocks failed\n");
 		return ret;
 	}
 
-	val &= ~REG_16_GLOBAL_EN;
-
-	ret = regmap_write(priv->regmap, SX1301_GEN, val);
-	if (ret) {
-		dev_err(&spi->dev, "16 write failed\n");
-		return ret;
-	}
-
-	ret = regmap_read(priv->regmap, SX1301_CKEN, &val);
+	ret = sx1301_field_write(priv, F_CLK32M_EN, 0);
 	if (ret) {
-		dev_err(&spi->dev, "17 read failed\n");
+		dev_err(&spi->dev, "gate 32M clock failed\n");
 		return ret;
 	}
 
-	val &= ~REG_17_CLK32M_EN;
-
-	ret = regmap_write(priv->regmap, SX1301_CKEN, val);
+	ret = sx1301_field_write(priv, F_RADIO_A_EN, 1);
 	if (ret) {
-		dev_err(&spi->dev, "17 write failed\n");
+		dev_err(&spi->dev, "radio a enable failed\n");
 		return ret;
 	}
 
-	ret = regmap_read(priv->regmap, SX1301_RADIO_CFG, &val);
-	if (ret) {
-		dev_err(&spi->dev, "2|43 read failed\n");
-		return ret;
-	}
-
-	val |= REG_2_43_RADIO_B_EN | REG_2_43_RADIO_A_EN;
-
-	ret = regmap_write(priv->regmap, SX1301_RADIO_CFG, val);
+	ret = sx1301_field_write(priv, F_RADIO_B_EN, 1);
 	if (ret) {
-		dev_err(&spi->dev, "2|43 write failed\n");
+		dev_err(&spi->dev, "radio b enable failed\n");
 		return ret;
 	}
 
 	msleep(500);
 
-	ret = regmap_read(priv->regmap, SX1301_RADIO_CFG, &val);
+	ret = sx1301_field_write(priv, F_RADIO_RST, 1);
 	if (ret) {
-		dev_err(&spi->dev, "2|43 read failed\n");
-		return ret;
-	}
-
-	val |= REG_2_43_RADIO_RST;
-
-	ret = regmap_write(priv->regmap, SX1301_RADIO_CFG, val);
-	if (ret) {
-		dev_err(&spi->dev, "2|43 write failed\n");
+		dev_err(&spi->dev, "radio asert reset failed\n");
 		return ret;
 	}
 
 	msleep(5);
 
-	ret = regmap_read(priv->regmap, SX1301_RADIO_CFG, &val);
-	if (ret) {
-		dev_err(&spi->dev, "2|43 read failed\n");
-		return ret;
-	}
-
-	val &= ~REG_2_43_RADIO_RST;
-
-	ret = regmap_write(priv->regmap, SX1301_RADIO_CFG, val);
+	ret = sx1301_field_write(priv, F_RADIO_RST, 0);
 	if (ret) {
-		dev_err(&spi->dev, "2|43 write failed\n");
+		dev_err(&spi->dev, "radio deasert reset failed\n");
 		return ret;
 	}
 
diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
index e6400f8..0bbd948 100644
--- a/drivers/net/lora/sx1301.h
+++ b/drivers/net/lora/sx1301.h
@@ -60,11 +60,57 @@
 
 #define SX1301_MAX_REGISTER         (SX1301_PAGE_BASE(3) + 0x7F)
 
+enum sx1301_fields {
+	F_SOFT_RESET,
+	F_GLOBAL_EN,
+	F_CLK32M_EN,
+	F_RADIO_A_EN,
+	F_RADIO_B_EN,
+	F_RADIO_RST,
+
+	F_MCU_RST_0,
+	F_MCU_RST_1,
+	F_MCU_SELECT_MUX_0,
+	F_MCU_SELECT_MUX_1,
+
+	F_FORCE_HOST_RADIO_CTRL,
+	F_FORCE_HOST_FE_CTRL,
+	F_FORCE_DEC_FILTER_GAIN,
+
+	F_EMERGENCY_FORCE_HOST_CTRL,
+};
+
+static const struct reg_field sx1301_regmap_fields[] = {
+	/* PAGE */
+	[F_SOFT_RESET]          = REG_FIELD(SX1301_PAGE, 7, 7),
+	/* 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),
+	/* 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),
+	/* 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),
+	/* EMERGENCY_FORCE_HOST_CTRL */
+	[F_EMERGENCY_FORCE_HOST_CTRL] =
+		REG_FIELD(SX1301_EMERGENCY_FORCE_HOST_CTRL, 0, 0),
+};
+
 struct sx1301_priv {
 	struct lora_dev_priv lora;
 	struct device		*dev;
 	struct gpio_desc *rst_gpio;
 	struct regmap		*regmap;
+	struct regmap_field     *regmap_fields[ARRAY_SIZE(sx1301_regmap_fields)];
 };
 
 int __init sx130x_radio_init(void);
-- 
2.7.4

[PATCH v2 lora-next 3/4] net: lora: sx125x: convert to regmap fields

[Ben Whitten]

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

We convert to using regmap fields to allow regmap to take care of read
modify writes and bit shifting for ofset fields.

Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
---
 drivers/net/lora/sx125x.c | 59 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 51 insertions(+), 8 deletions(-)

diff --git a/drivers/net/lora/sx125x.c b/drivers/net/lora/sx125x.c
index dc13d1a..36b61b1 100644
--- a/drivers/net/lora/sx125x.c
+++ b/drivers/net/lora/sx125x.c
@@ -25,11 +25,25 @@
 
 #include "sx125x.h"
 
-#define REG_CLK_SELECT_TX_DAC_CLK_SELECT_CLK_IN	BIT(0)
-#define REG_CLK_SELECT_CLK_OUT			BIT(1)
+enum sx125x_fields {
+	F_CLK_OUT,
+	F_TX_DAC_CLK_SEL,
+	F_SX1257_XOSC_GM_STARTUP,
+	F_SX1257_XOSC_DISABLE_CORE,
+};
+
+static const struct reg_field sx125x_regmap_fields[] = {
+	/* CLK_SELECT */
+	[F_CLK_OUT]        = REG_FIELD(SX125X_CLK_SELECT, 1, 1),
+	[F_TX_DAC_CLK_SEL] = REG_FIELD(SX125X_CLK_SELECT, 0, 0),
+	/* XOSC */ /* TODO maybe make this dynamic */
+	[F_SX1257_XOSC_GM_STARTUP]  = REG_FIELD(SX1257_XOSC, 0, 3),
+	[F_SX1257_XOSC_DISABLE_CORE]  = REG_FIELD(SX1257_XOSC, 5, 5),
+};
 
 struct sx125x_priv {
 	struct regmap		*regmap;
+	struct regmap_field     *regmap_fields[ARRAY_SIZE(sx125x_regmap_fields)];
 };
 
 static struct regmap_config __maybe_unused sx125x_regmap_config = {
@@ -44,11 +58,18 @@ static struct regmap_config __maybe_unused sx125x_regmap_config = {
 	.max_register = SX125X_MAX_REGISTER,
 };
 
+static int sx125x_field_write(struct sx125x_priv *priv,
+		enum sx125x_fields field_id, u8 val)
+{
+	return regmap_field_write(priv->regmap_fields[field_id], val);
+}
+
 static int __maybe_unused sx125x_regmap_probe(struct device *dev, struct regmap *regmap, unsigned int radio)
 {
 	struct sx125x_priv *priv;
 	unsigned int val;
 	int ret;
+	int i;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -56,6 +77,18 @@ static int __maybe_unused sx125x_regmap_probe(struct device *dev, struct regmap
 
 	dev_set_drvdata(dev, priv);
 	priv->regmap = regmap;
+	for (i = 0; i < ARRAY_SIZE(sx125x_regmap_fields); i++) {
+		const struct reg_field *reg_fields = sx125x_regmap_fields;
+
+		priv->regmap_fields[i] = devm_regmap_field_alloc(dev,
+				priv->regmap,
+				reg_fields[i]);
+		if (IS_ERR(priv->regmap_fields[i])) {
+			ret = PTR_ERR(priv->regmap_fields[i]);
+			dev_err(dev, "Cannot allocate regmap field: %d\n", ret);
+			return ret;
+		}
+	}
 
 	if (true) {
 		ret = regmap_read(priv->regmap, SX1255_VERSION, &val);
@@ -66,24 +99,34 @@ static int __maybe_unused sx125x_regmap_probe(struct device *dev, struct regmap
 		dev_info(dev, "SX125x version: %02x\n", val);
 	}
 
-	val = REG_CLK_SELECT_TX_DAC_CLK_SELECT_CLK_IN;
 	if (radio == 1) { /* HACK */
-		val |= REG_CLK_SELECT_CLK_OUT;
+		ret = sx125x_field_write(priv, F_CLK_OUT, 1);
+		if (ret) {
+			dev_err(dev, "enabling clock output failed\n");
+			return ret;
+		}
+
 		dev_info(dev, "enabling clock output\n");
 	}
 
-	ret = regmap_write(priv->regmap, SX125X_CLK_SELECT, val);
+	ret = sx125x_field_write(priv, F_TX_DAC_CLK_SEL, 1);
 	if (ret) {
-		dev_err(dev, "clk write failed\n");
+		dev_err(dev, "clock select failed\n");
 		return ret;
 	}
 
 	dev_dbg(dev, "clk written\n");
 
 	if (true) {
-		ret = regmap_write(priv->regmap, SX1257_XOSC, 13 + 2 * 16);
+		ret = sx125x_field_write(priv, F_SX1257_XOSC_DISABLE_CORE, 1);
+		if (ret) {
+			dev_err(dev, "xosc disable failed\n");
+			return ret;
+		}
+
+		ret = sx125x_field_write(priv, F_SX1257_XOSC_GM_STARTUP, 13);
 		if (ret) {
-			dev_err(dev, "xosc write failed\n");
+			dev_err(dev, "xosc startup adjust failed\n");
 			return ret;
 		}
 	}
-- 
2.7.4

[PATCH v2 lora-next 4/4] net: lora: sx125x sx1301: allow radio to register as a clk provider

[Ben Whitten]

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

The 32M is run from the radio, before we just enabled it based on
the radio number but now we can use the clk framework to request the
clk is started when we need it.

The 32M clock produced from the radio is really a gated version of
tcxo which is a fixed clock provided by hardware, and isn't captured
in this patch.

The sx1301 brings the clock up prior to calibration once the radios
have probed themselves.

A sample dts showing the clk link:
	sx1301: sx1301@0 {
		...
                clocks = <&radio1 0>;
                clock-names = "clk32m";

                radio-spi {
                        radio0: radio-a@0 {
                                ...
                        };

                        radio1: radio-b@1 {
                                #clock-cells = <0>;
                                clock-output-names = "clk32m";
                        };
                };
	};

Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
---
 drivers/net/lora/sx125x.c | 112 ++++++++++++++++++++++++++++++++++++++++++----
 drivers/net/lora/sx1301.c |  13 ++++++
 drivers/net/lora/sx1301.h |   2 +
 3 files changed, 119 insertions(+), 8 deletions(-)

diff --git a/drivers/net/lora/sx125x.c b/drivers/net/lora/sx125x.c
index 36b61b1..b7ca782 100644
--- a/drivers/net/lora/sx125x.c
+++ b/drivers/net/lora/sx125x.c
@@ -9,6 +9,8 @@
  * Copyright (c) 2013 Semtech-Cycleo
  */
 
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -42,10 +44,16 @@ static const struct reg_field sx125x_regmap_fields[] = {
 };
 
 struct sx125x_priv {
+	struct clk		*clkout;
+	struct clk_hw		clkout_hw;
+
+	struct device		*dev;
 	struct regmap		*regmap;
 	struct regmap_field     *regmap_fields[ARRAY_SIZE(sx125x_regmap_fields)];
 };
 
+#define to_clkout(_hw) container_of(_hw, struct sx125x_priv, clkout_hw)
+
 static struct regmap_config __maybe_unused sx125x_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -64,6 +72,96 @@ static int sx125x_field_write(struct sx125x_priv *priv,
 	return regmap_field_write(priv->regmap_fields[field_id], val);
 }
 
+static int sx125x_field_read(struct sx125x_priv *priv,
+		enum sx125x_fields field_id, unsigned int *val)
+{
+	return regmap_field_read(priv->regmap_fields[field_id], val);
+}
+
+static int sx125x_clkout_enable(struct clk_hw *hw)
+{
+	struct sx125x_priv *priv = to_clkout(hw);
+
+	dev_info(priv->dev, "enabling clkout\n");
+	return sx125x_field_write(priv, F_CLK_OUT, 1);
+}
+
+static void sx125x_clkout_disable(struct clk_hw *hw)
+{
+	struct sx125x_priv *priv = to_clkout(hw);
+	int ret;
+
+	dev_info(priv->dev, "disabling clkout\n");
+	ret = sx125x_field_write(priv, F_CLK_OUT, 0);
+	if (ret)
+		dev_err(priv->dev, "error disabling clkout\n");
+}
+
+static int sx125x_clkout_is_enabled(struct clk_hw *hw)
+{
+	struct sx125x_priv *priv = to_clkout(hw);
+	unsigned int enabled;
+	int ret;
+
+	ret = sx125x_field_read(priv, F_CLK_OUT, &enabled);
+	if (ret) {
+		dev_err(priv->dev, "error reading clk enable\n");
+		return 0;
+	}
+	return enabled;
+}
+
+static const struct clk_ops sx125x_clkout_ops = {
+	.enable = sx125x_clkout_enable,
+	.disable = sx125x_clkout_disable,
+	.is_enabled = sx125x_clkout_is_enabled,
+};
+
+static int sx125x_register_clock_provider(struct sx125x_priv *priv)
+{
+	struct device *dev = priv->dev;
+	struct clk_init_data init;
+	const char *parent;
+	int ret;
+
+	/* Disable CLKOUT */
+	ret = sx125x_field_write(priv, F_CLK_OUT, 0);
+	if (ret) {
+		dev_err(dev, "unable to disable clkout\n");
+		return ret;
+	}
+
+	/* Register clock provider if expected in DTB */
+	if (!of_find_property(dev->of_node, "#clock-cells", NULL))
+		return 0;
+
+	dev_info(dev, "registering clkout\n");
+
+	parent = of_clk_get_parent_name(dev->of_node, 0);
+	if (!parent) {
+		dev_err(dev, "Unable to find parent clk\n");
+		return -ENODEV;
+	}
+
+	init.ops = &sx125x_clkout_ops;
+	init.flags = CLK_IS_BASIC;
+	init.parent_names = &parent;
+	init.num_parents = 1;
+	priv->clkout_hw.init = &init;
+
+	of_property_read_string_index(dev->of_node, "clock-output-names", 0,
+			&init.name);
+
+	priv->clkout = devm_clk_register(dev, &priv->clkout_hw);
+	if (IS_ERR(priv->clkout)) {
+		dev_err(dev, "failed to register clkout\n");
+		return PTR_ERR(priv->clkout);
+	}
+	ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get,
+			&priv->clkout_hw);
+	return ret;
+}
+
 static int __maybe_unused sx125x_regmap_probe(struct device *dev, struct regmap *regmap, unsigned int radio)
 {
 	struct sx125x_priv *priv;
@@ -76,6 +174,7 @@ static int __maybe_unused sx125x_regmap_probe(struct device *dev, struct regmap
 		return -ENOMEM;
 
 	dev_set_drvdata(dev, priv);
+	priv->dev = dev;
 	priv->regmap = regmap;
 	for (i = 0; i < ARRAY_SIZE(sx125x_regmap_fields); i++) {
 		const struct reg_field *reg_fields = sx125x_regmap_fields;
@@ -99,16 +198,13 @@ static int __maybe_unused sx125x_regmap_probe(struct device *dev, struct regmap
 		dev_info(dev, "SX125x version: %02x\n", val);
 	}
 
-	if (radio == 1) { /* HACK */
-		ret = sx125x_field_write(priv, F_CLK_OUT, 1);
-		if (ret) {
-			dev_err(dev, "enabling clock output failed\n");
-			return ret;
-		}
-
-		dev_info(dev, "enabling clock output\n");
+	ret = sx125x_register_clock_provider(priv);
+	if (ret) {
+		dev_err(dev, "failed to register clkout provider: %d\n", ret);
+		return ret;
 	}
 
+	/* TODO Only needs setting on radio on the TX path */
 	ret = sx125x_field_write(priv, F_TX_DAC_CLK_SEL, 1);
 	if (ret) {
 		dev_err(dev, "clock select failed\n");
diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index eb89af6..44a7d0b 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -10,6 +10,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/firmware.h>
 #include <linux/lora.h>
@@ -404,6 +405,18 @@ static int sx130x_loradev_open(struct net_device *netdev)
 		return -ENXIO;
 	}
 
+	priv->clk32m = devm_clk_get(priv->dev, "clk32m");
+	if (IS_ERR(priv->clk32m)) {
+		dev_err(priv->dev, "failed to get clk32m\n");
+		return PTR_ERR(priv->clk32m);
+	}
+
+	ret = clk_prepare_enable(priv->clk32m);
+	if (ret) {
+		dev_err(priv->dev, "failed to enable clk32m: %d\n", ret);
+		return ret;
+	}
+
 	ret = sx1301_field_write(priv, F_GLOBAL_EN, 1);
 	if (ret) {
 		dev_err(priv->dev, "enable global clocks failed\n");
diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
index 0bbd948..a1a2e38 100644
--- a/drivers/net/lora/sx1301.h
+++ b/drivers/net/lora/sx1301.h
@@ -9,6 +9,7 @@
 #ifndef _SX1301_
 #define _SX1301_
 
+#include <linux/clk.h>
 #include <linux/regmap.h>
 #include <linux/gpio/consumer.h>
 #include <linux/lora/dev.h>
@@ -108,6 +109,7 @@ static const struct reg_field sx1301_regmap_fields[] = {
 struct sx1301_priv {
 	struct lora_dev_priv lora;
 	struct device		*dev;
+	struct clk		*clk32m;
 	struct gpio_desc *rst_gpio;
 	struct regmap		*regmap;
 	struct regmap_field     *regmap_fields[ARRAY_SIZE(sx1301_regmap_fields)];
-- 
2.7.4

Re: [PATCH v2 lora-next 1/4] net: lora: sx1301: convert burst spi functions to regmap raw

[Andreas Färber]

Hi Ben and Mark,

Am 09.10.18 um 14:52 schrieb Ben Whitten:
> As we have caching disabled we can access the regmap using raw for our
> firmware reading and writing bursts.
> We also remove the now defunct spi element from the structure as this
> completes the move to regmap.
> 
> Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
> ---
>  drivers/net/lora/sx1301.c | 26 +++++++++++++++++---------
>  drivers/net/lora/sx1301.h |  2 --
>  2 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
> index fd29258..5ab0e2d 100644
> --- a/drivers/net/lora/sx1301.c
> +++ b/drivers/net/lora/sx1301.c
> @@ -76,19 +76,28 @@ static struct regmap_config sx1301_regmap_config = {
>  
>  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(priv->spi, &addr, 1, val, len);
> +	size_t max;
> +
> +	max = regmap_get_raw_read_max(priv->regmap);
> +	if (max && max < len) {
> +		dev_err(priv->dev, "Burst greater then max raw read\n");
> +		return -EINVAL;
> +	}
> +
> +	return regmap_raw_read(priv->regmap, reg, val, len);

I believe the way we are using it with a single register needs
regmap_noinc_read() instead, in case we ever want to enable caching? ...

>  }
>  
>  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] = {
> -		{ .tx_buf = &addr, .len = 1 },
> -		{ .tx_buf = val, .len = len },
> -	};
> +	size_t max;
> +
> +	max = regmap_get_raw_write_max(priv->regmap);
> +	if (max && max < len) {
> +		dev_err(priv->dev, "Burst greater then max raw write\n");
> +		return -EINVAL;
> +	}
>  
> -	return spi_sync_transfer(priv->spi, xfr, 2);
> +	return regmap_raw_write(priv->regmap, reg, val, len);

... Which would mean we are lacking a noinc API for write here!

@Mark/Stefan, any reason noinc was not implemented symmetrically?

For sx1276 I have local regmap conversion patches, but I kept getting
-EINVAL for bursts and haven't figured out why yet. Need to compare to
your code - I assume you successfully tested this, Ben.

Regards,
Andreas

>  }
>  
>  static int sx1301_soft_reset(struct sx1301_priv *priv)
> @@ -566,7 +575,6 @@ static int sx1301_probe(struct spi_device *spi)
>  
>  	spi_set_drvdata(spi, netdev);
>  	priv->dev = &spi->dev;
> -	priv->spi = spi;
>  
>  	priv->regmap = devm_regmap_init_spi(spi, &sx1301_regmap_config);
>  	if (IS_ERR(priv->regmap)) {
> diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
> index e939c02..e6400f8 100644
> --- a/drivers/net/lora/sx1301.h
> +++ b/drivers/net/lora/sx1301.h
> @@ -12,7 +12,6 @@
>  #include <linux/regmap.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/lora/dev.h>
> -#include <linux/spi/spi.h>
>  
>  #define SX1301_CHIP_VERSION 103
>  
> @@ -64,7 +63,6 @@
>  struct sx1301_priv {
>  	struct lora_dev_priv lora;
>  	struct device		*dev;
> -	struct spi_device	*spi;
>  	struct gpio_desc *rst_gpio;
>  	struct regmap		*regmap;
>  };
> 


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

RE: [PATCH v2 lora-next 1/4] net: lora: sx1301: convert burst spi functions to regmap raw

[Ben Whitten]

> Subject: Re: [PATCH v2 lora-next 1/4] net: lora: sx1301:
> convert burst spi functions to regmap raw
> 
> Hi Ben and Mark,
> 
> Am 09.10.18 um 14:52 schrieb Ben Whitten:
> > As we have caching disabled we can access the regmap
> using raw for our
> > firmware reading and writing bursts.
> > We also remove the now defunct spi element from the
> structure as this
> > completes the move to regmap.
> >
> > Signed-off-by: Ben Whitten
> <ben.whitten@lairdtech.com>
> > ---
> >  drivers/net/lora/sx1301.c | 26 +++++++++++++++++------
> ---
> >  drivers/net/lora/sx1301.h |  2 --
> >  2 files changed, 17 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/lora/sx1301.c
> b/drivers/net/lora/sx1301.c
> > index fd29258..5ab0e2d 100644
> > --- a/drivers/net/lora/sx1301.c
> > +++ b/drivers/net/lora/sx1301.c
> > @@ -76,19 +76,28 @@ static struct regmap_config
> sx1301_regmap_config = {
> >
> >  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(priv->spi, &addr, 1, val,
> len);
> > +	size_t max;
> > +
> > +	max = regmap_get_raw_read_max(priv->regmap);
> > +	if (max && max < len) {
> > +		dev_err(priv->dev, "Burst greater then max
> raw read\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return regmap_raw_read(priv->regmap, reg, val,
> len);
> 
> I believe the way we are using it with a single register needs
> regmap_noinc_read() instead, in case we ever want to
> enable caching? ...

Certainly, the noinc was introduced in 4.19 lets rebase lora-next
and use this API instead.

> 
> >  }
> >
> >  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] = {
> > -		{ .tx_buf = &addr, .len = 1 },
> > -		{ .tx_buf = val, .len = len },
> > -	};
> > +	size_t max;
> > +
> > +	max = regmap_get_raw_write_max(priv->regmap);
> > +	if (max && max < len) {
> > +		dev_err(priv->dev, "Burst greater then max
> raw write\n");
> > +		return -EINVAL;
> > +	}
> >
> > -	return spi_sync_transfer(priv->spi, xfr, 2);
> > +	return regmap_raw_write(priv->regmap, reg, val,
> len);
> 
> ... Which would mean we are lacking a noinc API for write
> here!
> 
> @Mark/Stefan, any reason noinc was not implemented
> symmetrically?
> 
> For sx1276 I have local regmap conversion patches, but I
> kept getting
> -EINVAL for bursts and haven't figured out why yet. Need to
> compare to
> your code - I assume you successfully tested this, Ben.

Yes I was able to load and verify/run the firmware using this
patch on my Sentrius RG1xx board.

Thanks,
Ben

Re: [PATCH v2 lora-next 1/4] net: lora: sx1301: convert burst spi functions to regmap raw

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 473 bytes --]

On Wed, Oct 10, 2018 at 09:59:29AM +0200, Andreas Färber wrote:
> Am 09.10.18 um 14:52 schrieb Ben Whitten:

> > -	return spi_sync_transfer(priv->spi, xfr, 2);
> > +	return regmap_raw_write(priv->regmap, reg, val, len);

> ... Which would mean we are lacking a noinc API for write here!

> @Mark/Stefan, any reason noinc was not implemented symmetrically?

Nobody wanted it enough to do the work of implementing it, there's
nothing stopping someone doing that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]