[PATCH lora-next v2 1/8] net: lora: add methods for devm registration

[PATCH lora-next v2 1/8] net: lora: add methods for devm registration

[Ben Whitten]

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   | 28 ++++++++++++++++++++++++++++
 include/linux/lora/dev.h |  1 +
 2 files changed, 29 insertions(+)

diff --git a/drivers/net/lora/dev.c b/drivers/net/lora/dev.c
index 8c01106..e32a870 100644
--- a/drivers/net/lora/dev.c
+++ b/drivers/net/lora/dev.c
@@ -84,6 +84,34 @@ void free_loradev(struct net_device *dev)
 }
 EXPORT_SYMBOL_GPL(free_loradev);
 
+static void devm_free_loradev(struct device *dev, void *res)
+{
+	struct net_device *net = (*(struct net_device **)res);
+	free_loradev(net);
+}
+
+struct net_device *devm_alloc_loradev(struct device *dev, size_t priv)
+{
+	struct net_device **ptr;
+	struct net_device *net;
+
+	net = alloc_loradev(priv);
+	if (!net)
+		return NULL;
+
+	ptr = devres_alloc(devm_free_loradev, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr) {
+		free_loradev(net);
+		return NULL;
+	}
+
+	*ptr = net;
+	devres_add(dev, ptr);
+
+	return net;
+}
+EXPORT_SYMBOL_GPL(devm_alloc_loradev);
+
 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..0f600c9 100644
--- a/include/linux/lora/dev.h
+++ b/include/linux/lora/dev.h
@@ -31,6 +31,7 @@ static inline int lora_strtoeui(const char *str, lora_eui *val)
 }
 
 struct net_device *alloc_loradev(int sizeof_priv);
+struct net_device *devm_alloc_loradev(struct device *dev, size_t priv);
 void free_loradev(struct net_device *dev);
 int register_loradev(struct net_device *dev);
 void unregister_loradev(struct net_device *dev);
-- 
2.7.4

[PATCH lora-next v2 2/8] net: lora: sx1301: convert to devm registration of netdev

[Ben Whitten]

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, 27 insertions(+), 60 deletions(-)

diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index 5342b61..3c09f5a 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
@@ -637,20 +638,17 @@ static int sx1301_probe(struct spi_device *spi)
 	ret = sx1301_read(spi, REG_VERSION, &val);
 	if (ret) {
 		dev_err(&spi->dev, "version read failed\n");
-		goto err_version;
+		return ret;
 	}
 
 	if (val != 103) {
 		dev_err(&spi->dev, "unexpected version: %u\n", val);
-		ret = -ENXIO;
-		goto err_version;
+		return -ENXIO;
 	}
 
-	netdev = alloc_loradev(sizeof(*priv));
-	if (!netdev) {
-		ret = -ENOMEM;
-		goto err_alloc_loradev;
-	}
+	netdev = devm_alloc_loradev(&spi->dev, sizeof(*priv));
+	if (!netdev)
+		return -ENOMEM;
 
 	priv = netdev_priv(netdev);
 	priv->rst_gpio = rst;
@@ -662,19 +660,19 @@ static int sx1301_probe(struct spi_device *spi)
 	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;
@@ -682,13 +680,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;
@@ -696,7 +694,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);
@@ -749,8 +747,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);
@@ -765,15 +762,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);
@@ -788,7 +784,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 */
@@ -796,7 +792,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);
@@ -804,13 +800,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);
@@ -819,7 +815,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 */
@@ -827,7 +823,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;
@@ -835,13 +831,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;
@@ -849,58 +845,28 @@ 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:
-	free_loradev(netdev);
-err_alloc_loradev:
-err_version:
-	return ret;
 }
 
 static int sx1301_remove(struct spi_device *spi)
 {
-	struct net_device *netdev = spi_get_drvdata(spi);
-
-	//unregister_loradev(netdev);
-	free_loradev(netdev);
-
 	dev_info(&spi->dev, "SX1301 module removed\n");
 
 	return 0;
@@ -927,4 +893,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");
-- 
2.7.4

Re: [PATCH lora-next v2 2/8] net: lora: sx1301: convert to devm registration of netdev

[Andreas Färber]

Am 09.08.2018 um 14:33 schrieb Ben Whitten:
> 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, 27 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
> index 5342b61..3c09f5a 100644
> --- a/drivers/net/lora/sx1301.c
> +++ b/drivers/net/lora/sx1301.c
[...]
>  static int sx1301_remove(struct spi_device *spi)
>  {
> -	struct net_device *netdev = spi_get_drvdata(spi);
> -
> -	//unregister_loradev(netdev);

Thanks, this part we'll still need later though.

Applying.

Regards,
Andreas

> -	free_loradev(netdev);
> -
>  	dev_info(&spi->dev, "SX1301 module removed\n");
>  
>  	return 0;
[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 v2 3/8] net: lora: sx1301: convert to passing priv data throughout

[Ben Whitten]

Instead of passing around the spi device we instead pass around our
driver data directly.

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

diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index 3c09f5a..7324001 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -73,24 +73,26 @@ 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;
 	struct spi_controller *radio_a_ctrl, *radio_b_ctrl;
 };
 
-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)
+static int sx1301_read(struct sx1301_priv *priv, u8 reg, u8 *val)
 {
-	return sx1301_read_burst(spi, reg, val, 1);
+	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] = {
@@ -98,26 +100,25 @@ 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)
+static int sx1301_write(struct sx1301_priv *priv, u8 reg, u8 val)
 {
-	return sx1301_write_burst(spi, reg, &val, 1);
+	return sx1301_write_burst(priv, reg, &val, 1);
 }
 
-static int sx1301_page_switch(struct spi_device *spi, u8 page)
+static int sx1301_page_switch(struct sx1301_priv *priv, u8 page)
 {
-	struct sx1301_priv *priv = spi_get_drvdata(spi);
 	int ret;
 
 	if (priv->cur_page == page)
 		return 0;
 
-	dev_dbg(&spi->dev, "switching to page %u\n", (unsigned)page);
-	ret = sx1301_write(spi, REG_PAGE_RESET, page & 0x3);
+	dev_dbg(priv->dev, "switching to page %u\n", (unsigned)page);
+	ret = sx1301_write(priv, REG_PAGE_RESET, page & 0x3);
 	if (ret) {
-		dev_err(&spi->dev, "switching to page %u failed\n", (unsigned)page);
+		dev_err(priv->dev, "switching to page %u failed\n", (unsigned)page);
 		return ret;
 	}
 
@@ -126,31 +127,31 @@ static int sx1301_page_switch(struct spi_device *spi, u8 page)
 	return 0;
 }
 
-static int sx1301_page_read(struct spi_device *spi, u8 page, u8 reg, u8 *val)
+static int sx1301_page_read(struct sx1301_priv *priv, u8 page, u8 reg, u8 *val)
 {
 	int ret;
 
-	ret = sx1301_page_switch(spi, page);
+	ret = sx1301_page_switch(priv, 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)
+static int sx1301_page_write(struct sx1301_priv *priv, u8 page, u8 reg, u8 val)
 {
 	int ret;
 
-	ret = sx1301_page_switch(spi, page);
+	ret = sx1301_page_switch(priv, page);
 	if (ret)
 		return ret;
 
-	return sx1301_write(spi, reg, val);
+	return sx1301_write(priv, reg, val);
 }
 
-static int sx1301_soft_reset(struct spi_device *spi)
+static int sx1301_soft_reset(struct sx1301_priv *priv)
 {
-	return sx1301_write(spi, REG_PAGE_RESET, REG_PAGE_RESET_SOFT_RESET);
+	return sx1301_write(priv, REG_PAGE_RESET, REG_PAGE_RESET_SOFT_RESET);
 }
 
 #define REG_RADIO_X_DATA		0
@@ -161,12 +162,13 @@ static int sx1301_soft_reset(struct spi_device *spi)
 static int sx1301_radio_set_cs(struct spi_controller *ctrl, bool enable)
 {
 	struct spi_sx1301 *ssx = spi_controller_get_devdata(ctrl);
+	struct sx1301_priv *priv = spi_get_drvdata(ssx->parent);
 	u8 cs;
 	int ret;
 
 	dev_dbg(&ctrl->dev, "setting CS to %s\n", enable ? "1" : "0");
 
-	ret = sx1301_page_read(ssx->parent, ssx->page, ssx->regs + REG_RADIO_X_CS, &cs);
+	ret = sx1301_page_read(priv, ssx->page, ssx->regs + REG_RADIO_X_CS, &cs);
 	if (ret) {
 		dev_warn(&ctrl->dev, "failed to read CS (%d)\n", ret);
 		cs = 0;
@@ -177,7 +179,7 @@ static int sx1301_radio_set_cs(struct spi_controller *ctrl, bool enable)
 	else
 		cs &= ~BIT(0);
 
-	ret = sx1301_page_write(ssx->parent, ssx->page, ssx->regs + REG_RADIO_X_CS, cs);
+	ret = sx1301_page_write(priv, ssx->page, ssx->regs + REG_RADIO_X_CS, cs);
 	if (ret) {
 		dev_err(&ctrl->dev, "failed to write CS (%d)\n", ret);
 		return ret;
@@ -200,6 +202,7 @@ static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl,
 	struct spi_device *spi, struct spi_transfer *xfr)
 {
 	struct spi_sx1301 *ssx = spi_controller_get_devdata(ctrl);
+	struct sx1301_priv *priv = spi_get_drvdata(ssx->parent);
 	const u8 *tx_buf = xfr->tx_buf;
 	u8 *rx_buf = xfr->rx_buf;
 	int ret;
@@ -210,13 +213,13 @@ static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl,
 	dev_dbg(&spi->dev, "transferring one (%u)\n", xfr->len);
 
 	if (tx_buf) {
-		ret = sx1301_page_write(ssx->parent, ssx->page, ssx->regs + REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0);
+		ret = sx1301_page_write(priv, ssx->page, ssx->regs + REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0);
 		if (ret) {
 			dev_err(&spi->dev, "SPI radio address write failed\n");
 			return ret;
 		}
 
-		ret = sx1301_page_write(ssx->parent, ssx->page, ssx->regs + REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0);
+		ret = sx1301_page_write(priv, ssx->page, ssx->regs + REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0);
 		if (ret) {
 			dev_err(&spi->dev, "SPI radio data write failed\n");
 			return ret;
@@ -236,7 +239,7 @@ static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl,
 	}
 
 	if (rx_buf) {
-		ret = sx1301_page_read(ssx->parent, ssx->page, ssx->regs + REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]);
+		ret = sx1301_page_read(priv, ssx->page, ssx->regs + REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]);
 		if (ret) {
 			dev_err(&spi->dev, "SPI radio data read failed\n");
 			return ret;
@@ -246,45 +249,45 @@ 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;
 
-	ret = sx1301_page_write(spi, 2, REG_2_DBG_AGC_MCU_RAM_ADDR, addr);
+	ret = sx1301_page_write(priv, 2, REG_2_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 = sx1301_page_read(priv, 2, REG_2_DBG_AGC_MCU_RAM_DATA, 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;
 	}
 
 	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;
 
-	ret = sx1301_page_write(spi, 2, REG_2_DBG_ARB_MCU_RAM_ADDR, addr);
+	ret = sx1301_page_write(priv, 2, REG_2_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 = sx1301_page_read(priv, 2, REG_2_DBG_ARB_MCU_RAM_DATA, 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;
 	}
 
 	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 u8 *data, size_t len)
 {
 	u8 *buf;
 	u8 val, rst, select_mux;
@@ -306,36 +309,36 @@ static int sx1301_load_firmware(struct spi_device *spi, int mcu, const u8 *data,
 		return -EINVAL;
 	}
 
-	ret = sx1301_page_read(spi, 0, REG_0_MCU, &val);
+	ret = sx1301_page_read(priv, 0, REG_0_MCU, &val);
 	if (ret) {
-		dev_err(&spi->dev, "MCU read failed\n");
+		dev_err(priv->dev, "MCU read failed\n");
 		return ret;
 	}
 
 	val |= rst;
 	val &= ~select_mux;
 
-	ret = sx1301_page_write(spi, 0, REG_0_MCU, val);
+	ret = sx1301_page_write(priv, 0, REG_0_MCU, val);
 	if (ret) {
-		dev_err(&spi->dev, "MCU reset / select mux write failed\n");
+		dev_err(priv->dev, "MCU reset / select mux write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_write(spi, REG_MCU_PROM_ADDR, 0);
+	ret = sx1301_write(priv, REG_MCU_PROM_ADDR, 0);
 	if (ret) {
-		dev_err(&spi->dev, "MCU prom addr write failed\n");
+		dev_err(priv->dev, "MCU prom addr write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_write_burst(spi, REG_MCU_PROM_DATA, data, len);
+	ret = sx1301_write_burst(priv, REG_MCU_PROM_DATA, data, len);
 	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 = sx1301_read(priv, REG_MCU_PROM_DATA, &val);
 	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;
 	}
 
@@ -343,73 +346,73 @@ static int sx1301_load_firmware(struct spi_device *spi, int mcu, const u8 *data,
 	if (!buf)
 		return -ENOMEM;
 
-	ret = sx1301_read_burst(spi, REG_MCU_PROM_DATA, buf, len);
+	ret = sx1301_read_burst(priv, REG_MCU_PROM_DATA, buf, len);
 	if (ret) {
-		dev_err(&spi->dev, "MCU prom data read failed\n");
+		dev_err(priv->dev, "MCU prom data read failed\n");
 		kfree(buf);
 		return ret;
 	}
 
 	if (memcmp(data, buf, len)) {
-		dev_err(&spi->dev, "MCU prom data read does not match data written\n");
+		dev_err(priv->dev, "MCU prom data read does not match data written\n");
 		kfree(buf);
 		return -ENXIO;
 	}
 
 	kfree(buf);
 
-	ret = sx1301_page_read(spi, 0, REG_0_MCU, &val);
+	ret = sx1301_page_read(priv, 0, REG_0_MCU, &val);
 	if (ret) {
-		dev_err(&spi->dev, "MCU read (1) failed\n");
+		dev_err(priv->dev, "MCU read (1) failed\n");
 		return ret;
 	}
 
 	val |= select_mux;
 
-	ret = sx1301_page_write(spi, 0, REG_0_MCU, val);
+	ret = sx1301_page_write(priv, 0, REG_0_MCU, val);
 	if (ret) {
-		dev_err(&spi->dev, "MCU reset / select mux write (1) failed\n");
+		dev_err(priv->dev, "MCU reset / select mux write (1) failed\n");
 		return ret;
 	}
 
 	return 0;
 }
 
-static int sx1301_agc_calibrate(struct spi_device *spi)
+static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 {
 	const struct firmware *fw;
 	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");
+		dev_err(priv->dev, "agc cal firmware file load failed\n");
 		return ret;
 	}
 
 	if (fw->size != 8192) {
-		dev_err(&spi->dev, "unexpected agc cal firmware size\n");
+		dev_err(priv->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->data, fw->size);
 	release_firmware(fw);
 	if (ret) {
-		dev_err(&spi->dev, "agc cal firmware load failed\n");
+		dev_err(priv->dev, "agc cal firmware load failed\n");
 		return ret;
 	}
 
-	ret = sx1301_page_read(spi, 0, 105, &val);
+	ret = sx1301_page_read(priv, 0, 105, &val);
 	if (ret) {
-		dev_err(&spi->dev, "0|105 read failed\n");
+		dev_err(priv->dev, "0|105 read failed\n");
 		return ret;
 	}
 
 	val &= ~REG_0_105_FORCE_HOST_RADIO_CTRL;
 
-	ret = sx1301_page_write(spi, 0, 105, val);
+	ret = sx1301_page_write(priv, 0, 105, val);
 	if (ret) {
-		dev_err(&spi->dev, "0|105 write failed\n");
+		dev_err(priv->dev, "0|105 write failed\n");
 		return ret;
 	}
 
@@ -417,188 +420,188 @@ static int sx1301_agc_calibrate(struct spi_device *spi)
 	if (false)
 		val |= BIT(5); /* SX1255 */
 
-	ret = sx1301_page_write(spi, 0, REG_0_RADIO_SELECT, val);
+	ret = sx1301_page_write(priv, 0, REG_0_RADIO_SELECT, val);
 	if (ret) {
-		dev_err(&spi->dev, "radio select write failed\n");
+		dev_err(priv->dev, "radio select write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_page_read(spi, 0, REG_0_MCU, &val);
+	ret = sx1301_page_read(priv, 0, REG_0_MCU, &val);
 	if (ret) {
-		dev_err(&spi->dev, "MCU read (0) failed\n");
+		dev_err(priv->dev, "MCU read (0) failed\n");
 		return ret;
 	}
 
 	val &= ~REG_0_MCU_RST_1;
 
-	ret = sx1301_page_write(spi, 0, REG_0_MCU, val);
+	ret = sx1301_page_write(priv, 0, REG_0_MCU, val);
 	if (ret) {
-		dev_err(&spi->dev, "MCU write (0) failed\n");
+		dev_err(priv->dev, "MCU write (0) failed\n");
 		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");
+		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);
+		dev_err(priv->dev, "unexpected firmware version, expecting %u\n", 2);
 		return -ENXIO;
 	}
 
-	ret = sx1301_page_switch(spi, 3);
+	ret = sx1301_page_switch(priv, 3);
 	if (ret) {
-		dev_err(&spi->dev, "page switch 3 failed\n");
+		dev_err(priv->dev, "page switch 3 failed\n");
 		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");
+		dev_err(priv->dev, "emergency force read failed\n");
 		return ret;
 	}
 
 	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");
+		dev_err(priv->dev, "emergency force write failed\n");
 		return ret;
 	}
 
-	dev_err(&spi->dev, "starting calibration...\n");
+	dev_err(priv->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");
+		dev_err(priv->dev, "emergency force read (1) failed\n");
 		return ret;
 	}
 
 	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");
+		dev_err(priv->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");
+		dev_err(priv->dev, "AGC status read failed\n");
 		return ret;
 	}
 
-	dev_info(&spi->dev, "AGC status: %02x\n", (unsigned)val);
+	dev_info(priv->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_err(priv->dev, "AGC calibration failed\n");
 		return -ENXIO;
 	}
 
 	return 0;
 }
 
-static int sx1301_load_all_firmware(struct spi_device *spi)
+static int sx1301_load_all_firmware(struct sx1301_priv *priv)
 {
 	const struct firmware *fw;
 	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");
+		dev_err(priv->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->data, fw->size);
 	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");
+		dev_err(priv->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->data, fw->size);
 	release_firmware(fw);
 	if (ret)
 		return ret;
 
-	ret = sx1301_page_read(spi, 0, 105, &val);
+	ret = sx1301_page_read(priv, 0, 105, &val);
 	if (ret) {
-		dev_err(&spi->dev, "0|105 read failed\n");
+		dev_err(priv->dev, "0|105 read failed\n");
 		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);
+	ret = sx1301_page_write(priv, 0, 105, val);
 	if (ret) {
-		dev_err(&spi->dev, "0|105 write failed\n");
+		dev_err(priv->dev, "0|105 write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_page_write(spi, 0, REG_0_RADIO_SELECT, 0);
+	ret = sx1301_page_write(priv, 0, REG_0_RADIO_SELECT, 0);
 	if (ret) {
-		dev_err(&spi->dev, "radio select write failed\n");
+		dev_err(priv->dev, "radio select write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_page_read(spi, 0, REG_0_MCU, &val);
+	ret = sx1301_page_read(priv, 0, REG_0_MCU, &val);
 	if (ret) {
-		dev_err(&spi->dev, "MCU read (0) failed\n");
+		dev_err(priv->dev, "MCU read (0) failed\n");
 		return ret;
 	}
 
 	val &= ~(REG_0_MCU_RST_1 | REG_0_MCU_RST_0);
 
-	ret = sx1301_page_write(spi, 0, REG_0_MCU, val);
+	ret = sx1301_page_write(priv, 0, REG_0_MCU, val);
 	if (ret) {
-		dev_err(&spi->dev, "MCU write (0) failed\n");
+		dev_err(priv->dev, "MCU write (0) failed\n");
 		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");
+		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);
+		dev_err(priv->dev, "unexpected firmware version, expecting %u\n", 4);
 		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");
+		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);
+		dev_err(priv->dev, "unexpected firmware version, expecting %u\n", 1);
 		return -ENXIO;
 	}
 
@@ -635,17 +638,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");
-		return ret;
-	}
-
-	if (val != 103) {
-		dev_err(&spi->dev, "unexpected version: %u\n", val);
-		return -ENXIO;
-	}
-
 	netdev = devm_alloc_loradev(&spi->dev, sizeof(*priv));
 	if (!netdev)
 		return -ENOMEM;
@@ -654,22 +646,35 @@ static int sx1301_probe(struct spi_device *spi)
 	priv->rst_gpio = rst;
 	priv->cur_page = 0xff;
 
-	spi_set_drvdata(spi, netdev);
+	spi_set_drvdata(spi, priv);
+	priv->dev = &spi->dev;
+	priv->spi = spi;
 	SET_NETDEV_DEV(netdev, &spi->dev);
 
-	ret = sx1301_write(spi, REG_PAGE_RESET, 0);
+	ret = sx1301_read(priv, REG_VERSION, &val);
+	if (ret) {
+		dev_err(&spi->dev, "version read failed\n");
+		return ret;
+	}
+
+	if (val != 103) {
+		dev_err(&spi->dev, "unexpected version: %u\n", val);
+		return -ENXIO;
+	}
+
+	ret = sx1301_write(priv, REG_PAGE_RESET, 0);
 	if (ret) {
 		dev_err(&spi->dev, "page/reset write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_soft_reset(spi);
+	ret = sx1301_soft_reset(priv);
 	if (ret) {
 		dev_err(&spi->dev, "soft reset failed\n");
 		return ret;
 	}
 
-	ret = sx1301_read(spi, 16, &val);
+	ret = sx1301_read(priv, 16, &val);
 	if (ret) {
 		dev_err(&spi->dev, "16 read failed\n");
 		return ret;
@@ -677,13 +682,13 @@ static int sx1301_probe(struct spi_device *spi)
 
 	val &= ~REG_16_GLOBAL_EN;
 
-	ret = sx1301_write(spi, 16, val);
+	ret = sx1301_write(priv, 16, val);
 	if (ret) {
 		dev_err(&spi->dev, "16 write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_read(spi, 17, &val);
+	ret = sx1301_read(priv, 17, &val);
 	if (ret) {
 		dev_err(&spi->dev, "17 read failed\n");
 		return ret;
@@ -691,13 +696,13 @@ static int sx1301_probe(struct spi_device *spi)
 
 	val &= ~REG_17_CLK32M_EN;
 
-	ret = sx1301_write(spi, 17, val);
+	ret = sx1301_write(priv, 17, val);
 	if (ret) {
 		dev_err(&spi->dev, "17 write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_page_read(spi, 2, 43, &val);
+	ret = sx1301_page_read(priv, 2, 43, &val);
 	if (ret) {
 		dev_err(&spi->dev, "2|43 read failed\n");
 		return ret;
@@ -705,7 +710,7 @@ static int sx1301_probe(struct spi_device *spi)
 
 	val |= REG_2_43_RADIO_B_EN | REG_2_43_RADIO_A_EN;
 
-	ret = sx1301_page_write(spi, 2, 43, val);
+	ret = sx1301_page_write(priv, 2, 43, val);
 	if (ret) {
 		dev_err(&spi->dev, "2|43 write failed\n");
 		return ret;
@@ -713,7 +718,7 @@ static int sx1301_probe(struct spi_device *spi)
 
 	msleep(500);
 
-	ret = sx1301_page_read(spi, 2, 43, &val);
+	ret = sx1301_page_read(priv, 2, 43, &val);
 	if (ret) {
 		dev_err(&spi->dev, "2|43 read failed\n");
 		return ret;
@@ -721,7 +726,7 @@ static int sx1301_probe(struct spi_device *spi)
 
 	val |= REG_2_43_RADIO_RST;
 
-	ret = sx1301_page_write(spi, 2, 43, val);
+	ret = sx1301_page_write(priv, 2, 43, val);
 	if (ret) {
 		dev_err(&spi->dev, "2|43 write failed\n");
 		return ret;
@@ -729,7 +734,7 @@ static int sx1301_probe(struct spi_device *spi)
 
 	msleep(5);
 
-	ret = sx1301_page_read(spi, 2, 43, &val);
+	ret = sx1301_page_read(priv, 2, 43, &val);
 	if (ret) {
 		dev_err(&spi->dev, "2|43 read failed\n");
 		return ret;
@@ -737,7 +742,7 @@ static int sx1301_probe(struct spi_device *spi)
 
 	val &= ~REG_2_43_RADIO_RST;
 
-	ret = sx1301_page_write(spi, 2, 43, val);
+	ret = sx1301_page_write(priv, 2, 43, val);
 	if (ret) {
 		dev_err(&spi->dev, "2|43 write failed\n");
 		return ret;
@@ -789,7 +794,7 @@ static int sx1301_probe(struct spi_device *spi)
 
 	/* GPIO */
 
-	ret = sx1301_read(spi, REG_GPIO_MODE, &val);
+	ret = sx1301_read(priv, REG_GPIO_MODE, &val);
 	if (ret) {
 		dev_err(&spi->dev, "GPIO mode read failed\n");
 		return ret;
@@ -797,13 +802,13 @@ static int sx1301_probe(struct spi_device *spi)
 
 	val |= GENMASK(4, 0);
 
-	ret = sx1301_write(spi, REG_GPIO_MODE, val);
+	ret = sx1301_write(priv, 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);
+	ret = sx1301_read(priv, REG_GPIO_SELECT_OUTPUT, &val);
 	if (ret) {
 		dev_err(&spi->dev, "GPIO select output read failed\n");
 		return ret;
@@ -812,7 +817,7 @@ static int sx1301_probe(struct spi_device *spi)
 	val &= ~GENMASK(3, 0);
 	val |= 2;
 
-	ret = sx1301_write(spi, REG_GPIO_SELECT_OUTPUT, val);
+	ret = sx1301_write(priv, REG_GPIO_SELECT_OUTPUT, val);
 	if (ret) {
 		dev_err(&spi->dev, "GPIO select output write failed\n");
 		return ret;
@@ -820,7 +825,7 @@ static int sx1301_probe(struct spi_device *spi)
 
 	/* TODO LBT */
 
-	ret = sx1301_read(spi, 16, &val);
+	ret = sx1301_read(priv, 16, &val);
 	if (ret) {
 		dev_err(&spi->dev, "16 read (1) failed\n");
 		return ret;
@@ -828,13 +833,13 @@ static int sx1301_probe(struct spi_device *spi)
 
 	val |= REG_16_GLOBAL_EN;
 
-	ret = sx1301_write(spi, 16, val);
+	ret = sx1301_write(priv, 16, val);
 	if (ret) {
 		dev_err(&spi->dev, "16 write (1) failed\n");
 		return ret;
 	}
 
-	ret = sx1301_read(spi, 17, &val);
+	ret = sx1301_read(priv, 17, &val);
 	if (ret) {
 		dev_err(&spi->dev, "17 read (1) failed\n");
 		return ret;
@@ -842,7 +847,7 @@ static int sx1301_probe(struct spi_device *spi)
 
 	val |= REG_17_CLK32M_EN;
 
-	ret = sx1301_write(spi, 17, val);
+	ret = sx1301_write(priv, 17, val);
 	if (ret) {
 		dev_err(&spi->dev, "17 write (1) failed\n");
 		return ret;
@@ -850,13 +855,13 @@ static int sx1301_probe(struct spi_device *spi)
 
 	/* calibration */
 
-	ret = sx1301_agc_calibrate(spi);
+	ret = sx1301_agc_calibrate(priv);
 	if (ret)
 		return ret;
 
 	/* TODO */
 
-	ret = sx1301_load_all_firmware(spi);
+	ret = sx1301_load_all_firmware(priv);
 	if (ret)
 		return ret;
 
-- 
2.7.4

Re: [PATCH lora-next v2 3/8] net: lora: sx1301: convert to passing priv data throughout

[Andreas Färber]

Am 09.08.2018 um 14:33 schrieb Ben Whitten:
> Instead of passing around the spi device we instead pass around our
> driver data directly.
> 
> Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
> ---
>  drivers/net/lora/sx1301.c | 305 +++++++++++++++++++++++-----------------------
>  1 file changed, 155 insertions(+), 150 deletions(-)
> 
> diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
> index 3c09f5a..7324001 100644
> --- a/drivers/net/lora/sx1301.c
> +++ b/drivers/net/lora/sx1301.c
> @@ -73,24 +73,26 @@ struct spi_sx1301 {
>  };
>  
>  struct sx1301_priv {
> +	struct device		*dev;
> +	struct spi_device	*spi;

Obviously this is not a long-term solution, but as interim step it'll
have to do.

>  	struct lora_priv lora;
>  	struct gpio_desc *rst_gpio;
>  	u8 cur_page;
>  	struct spi_controller *radio_a_ctrl, *radio_b_ctrl;
>  };
>  
> -static int sx1301_read_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)
> +static int sx1301_read(struct sx1301_priv *priv, u8 reg, u8 *val)
>  {
> -	return sx1301_read_burst(spi, reg, val, 1);
> +	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] = {

This hunk did not apply for some reason, I've manually re-applied it.

[...]
> @@ -654,22 +646,35 @@ static int sx1301_probe(struct spi_device *spi)
>  	priv->rst_gpio = rst;
>  	priv->cur_page = 0xff;
>  
> -	spi_set_drvdata(spi, netdev);
> +	spi_set_drvdata(spi, priv);

This change seems unnecessary and counter-productive for unregistration.

Otherwise applying.

Thanks,
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 v2 3/8] net: lora: sx1301: convert to passing priv data throughout

[Ben Whitten]

On Thu, 9 Aug 2018 at 21:43, Andreas Färber <afaerber@suse.de> wrote:
>
> Am 09.08.2018 um 14:33 schrieb Ben Whitten:
> > Instead of passing around the spi device we instead pass around our
> > driver data directly.
> >
> > Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
> > ---
> >  drivers/net/lora/sx1301.c | 305 +++++++++++++++++++++++-----------------------
> >  1 file changed, 155 insertions(+), 150 deletions(-)
> >
> > diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
> > index 3c09f5a..7324001 100644
> > --- a/drivers/net/lora/sx1301.c
> > +++ b/drivers/net/lora/sx1301.c
> > @@ -73,24 +73,26 @@ struct spi_sx1301 {
> >  };
> >
> >  struct sx1301_priv {
> > +     struct device           *dev;
> > +     struct spi_device       *spi;
>
> Obviously this is not a long-term solution, but as interim step it'll
> have to do.
>
> >       struct lora_priv lora;
> >       struct gpio_desc *rst_gpio;
> >       u8 cur_page;
> >       struct spi_controller *radio_a_ctrl, *radio_b_ctrl;
> >  };
> >
> > -static int sx1301_read_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)
> > +static int sx1301_read(struct sx1301_priv *priv, u8 reg, u8 *val)
> >  {
> > -     return sx1301_read_burst(spi, reg, val, 1);
> > +     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] = {
>
> This hunk did not apply for some reason, I've manually re-applied it.
>
> [...]
> > @@ -654,22 +646,35 @@ static int sx1301_probe(struct spi_device *spi)
> >       priv->rst_gpio = rst;
> >       priv->cur_page = 0xff;
> >
> > -     spi_set_drvdata(spi, netdev);
> > +     spi_set_drvdata(spi, priv);
>
> This change seems unnecessary and counter-productive for unregistration.
>
> Otherwise applying.

This is actually pretty critical, as it stands with the two spi masters we use
spi_get_drvdata on the parent device of the controller to recover the priv
struct for regmap.

We may have to include the netdev in the priv data, or do a container_of
dance to recover netdev in unregistration.
That said if we wrap things in devm then really our remove function could
be empty, as we have done with the allocation.

Regards,
Ben

Re: [PATCH lora-next v2 3/8] net: lora: sx1301: convert to passing priv data throughout

[Andreas Färber]

Am 09.08.2018 um 23:06 schrieb Ben Whitten:
> On Thu, 9 Aug 2018 at 21:43, Andreas Färber <afaerber@suse.de> wrote:
>> Am 09.08.2018 um 14:33 schrieb Ben Whitten:
>>> @@ -654,22 +646,35 @@ static int sx1301_probe(struct spi_device *spi)
>>>       priv->rst_gpio = rst;
>>>       priv->cur_page = 0xff;
>>>
>>> -     spi_set_drvdata(spi, netdev);
>>> +     spi_set_drvdata(spi, priv);
>>
>> This change seems unnecessary and counter-productive for unregistration.
>>
>> Otherwise applying.
> 
> This is actually pretty critical, as it stands with the two spi masters we use
> spi_get_drvdata on the parent device of the controller to recover the priv
> struct for regmap.
> 
> We may have to include the netdev in the priv data, or do a container_of
> dance to recover netdev in unregistration.
> That said if we wrap things in devm then really our remove function could
> be empty, as we have done with the allocation.

Thanks for quickly noticing. This should compensate:

diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index 0ba841f8e7cd..43cd2308e41c 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -164,7 +164,8 @@ static int sx1301_soft_reset(struct sx1301_priv *priv)
 static int sx1301_radio_set_cs(struct spi_controller *ctrl, bool enable)
 {
        struct spi_sx1301 *ssx = spi_controller_get_devdata(ctrl);
-       struct sx1301_priv *priv = spi_get_drvdata(ssx->parent);
+       struct net_device *netdev = spi_get_drvdata(ssx->parent);
+       struct sx1301_priv *priv = netdev_priv(netdev);
        u8 cs;
        int ret;

@@ -204,7 +205,8 @@ static int sx1301_radio_spi_transfer_one(struct
spi_controller *ctrl,
        struct spi_device *spi, struct spi_transfer *xfr)
 {
        struct spi_sx1301 *ssx = spi_controller_get_devdata(ctrl);
-       struct sx1301_priv *priv = spi_get_drvdata(ssx->parent);
+       struct net_device *netdev = spi_get_drvdata(ssx->parent);
+       struct sx1301_priv *priv = netdev_priv(netdev);
        const u8 *tx_buf = xfr->tx_buf;
        u8 *rx_buf = xfr->rx_buf;
        int ret;

Regards,
Andreas

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

[PATCH lora-next v2 4/8] net: lora: sx1301: convert load_firmware to take firmware directly

[Ben Whitten]

We just pass the pointer to firmware down to the function that loads
it.

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

diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index 7324001..3f2a532 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -287,13 +287,13 @@ static int sx1301_arb_ram_read(struct sx1301_priv *priv, u8 addr, u8 *val)
 	return 0;
 }
 
-static int sx1301_load_firmware(struct sx1301_priv *priv, 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;
 
-	if (len > 8192)
+	if (fw->size > 8192)
 		return -EINVAL;
 
 	switch (mcu) {
@@ -330,7 +330,7 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const u8 *dat
 		return ret;
 	}
 
-	ret = sx1301_write_burst(priv, REG_MCU_PROM_DATA, data, len);
+	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;
@@ -342,18 +342,18 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const u8 *dat
 		return ret;
 	}
 
-	buf = kzalloc(len, GFP_KERNEL);
+	buf = kzalloc(fw->size, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
-	ret = sx1301_read_burst(priv, REG_MCU_PROM_DATA, buf, len);
+	ret = sx1301_read_burst(priv, REG_MCU_PROM_DATA, buf, fw->size);
 	if (ret) {
 		dev_err(priv->dev, "MCU prom data read failed\n");
 		kfree(buf);
 		return ret;
 	}
 
-	if (memcmp(data, buf, len)) {
+	if (memcmp(fw->data, buf, fw->size)) {
 		dev_err(priv->dev, "MCU prom data read does not match data written\n");
 		kfree(buf);
 		return -ENXIO;
@@ -395,7 +395,7 @@ static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 		return -EINVAL;
 	}
 
-	ret = sx1301_load_firmware(priv, 1, fw->data, fw->size);
+	ret = sx1301_load_firmware(priv, 1, fw);
 	release_firmware(fw);
 	if (ret) {
 		dev_err(priv->dev, "agc cal firmware load failed\n");
@@ -523,7 +523,7 @@ static int sx1301_load_all_firmware(struct sx1301_priv *priv)
 		return -EINVAL;
 	}
 
-	ret = sx1301_load_firmware(priv, 0, fw->data, fw->size);
+	ret = sx1301_load_firmware(priv, 0, fw);
 	release_firmware(fw);
 	if (ret)
 		return ret;
@@ -540,7 +540,7 @@ static int sx1301_load_all_firmware(struct sx1301_priv *priv)
 		return -EINVAL;
 	}
 
-	ret = sx1301_load_firmware(priv, 1, fw->data, fw->size);
+	ret = sx1301_load_firmware(priv, 1, fw);
 	release_firmware(fw);
 	if (ret)
 		return ret;
-- 
2.7.4

Re: [PATCH lora-next v2 4/8] net: lora: sx1301: convert load_firmware to take firmware directly

[Andreas Färber]

Am 09.08.2018 um 14:33 schrieb Ben Whitten:
> We just pass the pointer to firmware down to the function that loads
> it.
> 
> Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
> ---
>  drivers/net/lora/sx1301.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)

Applied.

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 v2 5/8] net: lora: sx1301: remove duplicate firmware size checks

[Ben Whitten]

No need to check the size of the firmware multiple times, just do it once
in the function responsible for loading as the firmwares are the same size.

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

diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index 3f2a532..916ee40 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -293,8 +293,10 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct
 	u8 val, rst, select_mux;
 	int ret;
 
-	if (fw->size > 8192)
+	if (fw->size != 8192) {
+		dev_err(priv->dev, "Unexpected firmware size\n");
 		return -EINVAL;
+	}
 
 	switch (mcu) {
 	case 0:
@@ -390,11 +392,6 @@ static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 		return ret;
 	}
 
-	if (fw->size != 8192) {
-		dev_err(priv->dev, "unexpected agc cal firmware size\n");
-		return -EINVAL;
-	}
-
 	ret = sx1301_load_firmware(priv, 1, fw);
 	release_firmware(fw);
 	if (ret) {
@@ -517,12 +514,6 @@ static int sx1301_load_all_firmware(struct sx1301_priv *priv)
 		return ret;
 	}
 
-	if (fw->size != 8192) {
-		dev_err(priv->dev, "unexpected arb firmware size\n");
-		release_firmware(fw);
-		return -EINVAL;
-	}
-
 	ret = sx1301_load_firmware(priv, 0, fw);
 	release_firmware(fw);
 	if (ret)
@@ -534,12 +525,6 @@ static int sx1301_load_all_firmware(struct sx1301_priv *priv)
 		return ret;
 	}
 
-	if (fw->size != 8192) {
-		dev_err(priv->dev, "unexpected agc firmware size\n");
-		release_firmware(fw);
-		return -EINVAL;
-	}
-
 	ret = sx1301_load_firmware(priv, 1, fw);
 	release_firmware(fw);
 	if (ret)
-- 
2.7.4

Re: [PATCH lora-next v2 5/8] net: lora: sx1301: remove duplicate firmware size checks

[Andreas Färber]

Am 09.08.2018 um 14:33 schrieb Ben Whitten:
> No need to check the size of the firmware multiple times, just do it once
> in the function responsible for loading as the firmwares are the same size.
> 
> Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
> ---
>  drivers/net/lora/sx1301.c | 21 +++------------------
>  1 file changed, 3 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
> index 3f2a532..916ee40 100644
> --- a/drivers/net/lora/sx1301.c
> +++ b/drivers/net/lora/sx1301.c
> @@ -293,8 +293,10 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct
>  	u8 val, rst, select_mux;
>  	int ret;
>  
> -	if (fw->size > 8192)
> +	if (fw->size != 8192) {

Note the original intention here was to allow loading firmware smaller
than the maximum size, but we can revisit that later if we ever have
such a firmware.

Applied.

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 v2 6/8] net: lora: sx1301: replace version and size magic numbers with defines

[Ben Whitten]

We replace the hard coded numbers for size and version with meaningful
names.

Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
---
 drivers/net/lora/sx1301.c | 21 +++++++++++++--------
 drivers/net/lora/sx1301.h | 18 ++++++++++++++++++
 2 files changed, 31 insertions(+), 8 deletions(-)
 create mode 100644 drivers/net/lora/sx1301.h

diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index 916ee40..8e81179 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -21,6 +21,8 @@
 #include <linux/lora/dev.h>
 #include <linux/spi/spi.h>
 
+#include "sx1301.h"
+
 #define REG_PAGE_RESET			0
 #define REG_VERSION			1
 #define REG_MCU_PROM_ADDR		9
@@ -293,7 +295,7 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct
 	u8 val, rst, select_mux;
 	int ret;
 
-	if (fw->size != 8192) {
+	if (fw->size != SX1301_MCU_FW_BYTE) {
 		dev_err(priv->dev, "Unexpected firmware size\n");
 		return -EINVAL;
 	}
@@ -445,8 +447,9 @@ static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 
 	dev_info(priv->dev, "AGC calibration firmware version %u\n", (unsigned)val);
 
-	if (val != 2) {
-		dev_err(priv->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;
 	}
 
@@ -572,8 +575,9 @@ static int sx1301_load_all_firmware(struct sx1301_priv *priv)
 
 	dev_info(priv->dev, "AGC firmware version %u\n", (unsigned)val);
 
-	if (val != 4) {
-		dev_err(priv->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;
 	}
 
@@ -585,8 +589,9 @@ static int sx1301_load_all_firmware(struct sx1301_priv *priv)
 
 	dev_info(priv->dev, "ARB firmware version %u\n", (unsigned)val);
 
-	if (val != 1) {
-		dev_err(priv->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;
 	}
 
@@ -642,7 +647,7 @@ static int sx1301_probe(struct spi_device *spi)
 		return ret;
 	}
 
-	if (val != 103) {
+	if (val != SX1301_CHIP_VERSION) {
 		dev_err(&spi->dev, "unexpected version: %u\n", val);
 		return -ENXIO;
 	}
diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
new file mode 100644
index 0000000..b37ac56
--- /dev/null
+++ b/drivers/net/lora/sx1301.h
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Semtech SX1301 lora concentrator
+ *
+ * Copyright (c) 2018   Ben Whitten
+ */
+
+#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
+
+#endif
-- 
2.7.4

Re: [PATCH lora-next v2 6/8] net: lora: sx1301: replace version and size magic numbers with defines

[Andreas Färber]

Am 09.08.2018 um 14:33 schrieb Ben Whitten:
> We replace the hard coded numbers for size and version with meaningful
> names.
> 
> Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
> ---
>  drivers/net/lora/sx1301.c | 21 +++++++++++++--------
>  drivers/net/lora/sx1301.h | 18 ++++++++++++++++++
>  2 files changed, 31 insertions(+), 8 deletions(-)
>  create mode 100644 drivers/net/lora/sx1301.h
> 
> diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
> index 916ee40..8e81179 100644
> --- a/drivers/net/lora/sx1301.c
> +++ b/drivers/net/lora/sx1301.c
> @@ -21,6 +21,8 @@
>  #include <linux/lora/dev.h>
>  #include <linux/spi/spi.h>
>  
> +#include "sx1301.h"
> +
>  #define REG_PAGE_RESET			0
>  #define REG_VERSION			1
>  #define REG_MCU_PROM_ADDR		9
> @@ -293,7 +295,7 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct
>  	u8 val, rst, select_mux;
>  	int ret;
>  
> -	if (fw->size != 8192) {
> +	if (fw->size != SX1301_MCU_FW_BYTE) {

I think that should be BYTES, but we can still rename it later.

>  		dev_err(priv->dev, "Unexpected firmware size\n");
>  		return -EINVAL;
>  	}
[...]
> diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
> new file mode 100644
> index 0000000..b37ac56
> --- /dev/null
> +++ b/drivers/net/lora/sx1301.h
> @@ -0,0 +1,18 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later

Header files must use /* ... */ or checkpatch.pl complains.

> +/*
> + * Semtech SX1301 lora concentrator

LoRa

> + *
> + * Copyright (c) 2018   Ben Whitten

Any reason for the multiple whitespaces?

> + */
> +
> +#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
> +
> +#endif

Applied.

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 v2 7/8] net: lora: sx1301: add initial registration for regmap

[Ben Whitten]

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.

Further registers or fields are added as they are required in conversion.

Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
---
 drivers/net/lora/Kconfig  |  1 +
 drivers/net/lora/sx1301.c | 46 ++++++++++++++++++++++++++++++++++++++++++----
 drivers/net/lora/sx1301.h | 10 ++++++++++
 3 files changed, 53 insertions(+), 4 deletions(-)

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 8e81179..766df06 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -20,11 +20,11 @@
 #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
 #define REG_MCU_PROM_ADDR		9
 #define REG_MCU_PROM_DATA		10
 #define REG_GPIO_SELECT_INPUT		27
@@ -68,6 +68,35 @@
 
 #define REG_EMERGENCY_FORCE_HOST_CTRL	BIT(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;
@@ -81,6 +110,7 @@ struct sx1301_priv {
 	struct gpio_desc *rst_gpio;
 	u8 cur_page;
 	struct spi_controller *radio_a_ctrl, *radio_b_ctrl;
+	struct regmap		*regmap;
 };
 
 static int sx1301_read_burst(struct sx1301_priv *priv, u8 reg, u8 *val, size_t len)
@@ -614,6 +644,7 @@ static int sx1301_probe(struct spi_device *spi)
 	struct spi_sx1301 *radio;
 	struct gpio_desc *rst;
 	int ret;
+	unsigned int ver;
 	u8 val;
 
 	rst = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
@@ -641,14 +672,21 @@ static int sx1301_probe(struct spi_device *spi)
 	priv->spi = spi;
 	SET_NETDEV_DEV(netdev, &spi->dev);
 
-	ret = sx1301_read(priv, REG_VERSION, &val);
+	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 ret;
+	}
+
+	ret = regmap_read(priv->regmap, SX1301_VER, &ver);
 	if (ret) {
 		dev_err(&spi->dev, "version read failed\n");
 		return ret;
 	}
 
-	if (val != SX1301_CHIP_VERSION) {
-		dev_err(&spi->dev, "unexpected version: %u\n", val);
+	if (ver != SX1301_CHIP_VERSION) {
+		dev_err(&spi->dev, "unexpected version: %u\n", ver);
 		return -ENXIO;
 	}
 
diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
index b37ac56..2fc283f 100644
--- a/drivers/net/lora/sx1301.h
+++ b/drivers/net/lora/sx1301.h
@@ -15,4 +15,14 @@
 #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
+
+#define SX1301_VIRT_BASE    0x100
+#define SX1301_PAGE_LEN     0x80
+#define SX1301_PAGE_BASE(n) (SX1301_VIRT_BASE + (SX1301_PAGE_LEN * n))
+
+#define SX1301_MAX_REGISTER         (SX1301_PAGE_BASE(3) + 0x7F)
+
 #endif
-- 
2.7.4

Re: [PATCH lora-next v2 7/8] net: lora: sx1301: add initial registration for regmap

[Andreas Färber]

Am 09.08.2018 um 14:33 schrieb Ben Whitten:
> 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.
> 
> Further registers or fields are added as they are required in conversion.
> 
> Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
> ---
>  drivers/net/lora/Kconfig  |  1 +
>  drivers/net/lora/sx1301.c | 46 ++++++++++++++++++++++++++++++++++++++++++----
>  drivers/net/lora/sx1301.h | 10 ++++++++++
>  3 files changed, 53 insertions(+), 4 deletions(-)
> 
> 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 8e81179..766df06 100644
> --- a/drivers/net/lora/sx1301.c
> +++ b/drivers/net/lora/sx1301.c
> @@ -20,11 +20,11 @@
>  #include <linux/of_gpio.h>
>  #include <linux/lora/dev.h>
>  #include <linux/spi/spi.h>
> +#include <linux/regmap.h>

Misordered.

>  
>  #include "sx1301.h"
>  
>  #define REG_PAGE_RESET			0
> -#define REG_VERSION			1
>  #define REG_MCU_PROM_ADDR		9
>  #define REG_MCU_PROM_DATA		10
>  #define REG_GPIO_SELECT_INPUT		27
> @@ -68,6 +68,35 @@
>  
>  #define REG_EMERGENCY_FORCE_HOST_CTRL	BIT(0)
>  
> +static const struct regmap_range_cfg sx1301_ranges[] = {

Let's rename to _regmap_ranges for consistency.

> @@ -81,6 +110,7 @@ struct sx1301_priv {
>  	struct gpio_desc *rst_gpio;
>  	u8 cur_page;
>  	struct spi_controller *radio_a_ctrl, *radio_b_ctrl;
> +	struct regmap		*regmap;

Note: We need a consistent style. Either whitespace or tabs, not both
depending on author. Same in an earlier patch. Problem with tabs is that
at some point it's always one tab too little, but we can try it.

Applied.

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 v2 8/8] net: lora: sx1301: convert driver over to regmap reads and writes

[Ben Whitten]

The reads and writes are replaced with regmap versions and unneeded
functions, variable, and defines removed.

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

diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index 766df06..4db5a43 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -24,29 +24,6 @@
 
 #include "sx1301.h"
 
-#define REG_PAGE_RESET			0
-#define REG_MCU_PROM_ADDR		9
-#define REG_MCU_PROM_DATA		10
-#define REG_GPIO_SELECT_INPUT		27
-#define REG_GPIO_SELECT_OUTPUT		28
-#define REG_GPIO_MODE			29
-#define REG_MCU_AGC_STATUS		32
-#define REG_0_RADIO_SELECT		35
-#define REG_0_MCU			106
-#define REG_2_SPI_RADIO_A_DATA		33
-#define REG_2_SPI_RADIO_A_DATA_READBACK	34
-#define REG_2_SPI_RADIO_A_ADDR		35
-#define REG_2_SPI_RADIO_A_CS		37
-#define REG_2_SPI_RADIO_B_DATA		38
-#define REG_2_SPI_RADIO_B_DATA_READBACK	39
-#define REG_2_SPI_RADIO_B_ADDR		40
-#define REG_2_SPI_RADIO_B_CS		42
-#define REG_2_DBG_ARB_MCU_RAM_DATA	64
-#define REG_2_DBG_AGC_MCU_RAM_DATA	65
-#define REG_2_DBG_ARB_MCU_RAM_ADDR	80
-#define REG_2_DBG_AGC_MCU_RAM_ADDR	81
-#define REG_EMERGENCY_FORCE		127
-
 #define REG_PAGE_RESET_SOFT_RESET	BIT(7)
 
 #define REG_16_GLOBAL_EN		BIT(3)
@@ -99,8 +76,7 @@ static struct regmap_config sx1301_regmap_config = {
 
 struct spi_sx1301 {
 	struct spi_device *parent;
-	u8 page;
-	u8 regs;
+	unsigned int regs;
 };
 
 struct sx1301_priv {
@@ -108,7 +84,6 @@ struct sx1301_priv {
 	struct spi_device	*spi;
 	struct lora_priv lora;
 	struct gpio_desc *rst_gpio;
-	u8 cur_page;
 	struct spi_controller *radio_a_ctrl, *radio_b_ctrl;
 	struct regmap		*regmap;
 };
@@ -140,50 +115,9 @@ static int sx1301_write(struct sx1301_priv *priv, u8 reg, u8 val)
 	return sx1301_write_burst(priv, reg, &val, 1);
 }
 
-static int sx1301_page_switch(struct sx1301_priv *priv, u8 page)
-{
-	int ret;
-
-	if (priv->cur_page == page)
-		return 0;
-
-	dev_dbg(priv->dev, "switching to page %u\n", (unsigned)page);
-	ret = sx1301_write(priv, REG_PAGE_RESET, page & 0x3);
-	if (ret) {
-		dev_err(priv->dev, "switching to page %u failed\n", (unsigned)page);
-		return ret;
-	}
-
-	priv->cur_page = page;
-
-	return 0;
-}
-
-static int sx1301_page_read(struct sx1301_priv *priv, u8 page, u8 reg, u8 *val)
-{
-	int ret;
-
-	ret = sx1301_page_switch(priv, page);
-	if (ret)
-		return ret;
-
-	return sx1301_read(priv, reg, val);
-}
-
-static int sx1301_page_write(struct sx1301_priv *priv, u8 page, u8 reg, u8 val)
-{
-	int ret;
-
-	ret = sx1301_page_switch(priv, page);
-	if (ret)
-		return ret;
-
-	return sx1301_write(priv, reg, val);
-}
-
 static int sx1301_soft_reset(struct sx1301_priv *priv)
 {
-	return sx1301_write(priv, REG_PAGE_RESET, REG_PAGE_RESET_SOFT_RESET);
+	return regmap_write(priv->regmap, SX1301_PAGE, REG_PAGE_RESET_SOFT_RESET);
 }
 
 #define REG_RADIO_X_DATA		0
@@ -195,12 +129,12 @@ static int sx1301_radio_set_cs(struct spi_controller *ctrl, bool enable)
 {
 	struct spi_sx1301 *ssx = spi_controller_get_devdata(ctrl);
 	struct sx1301_priv *priv = spi_get_drvdata(ssx->parent);
-	u8 cs;
+	unsigned int cs;
 	int ret;
 
 	dev_dbg(&ctrl->dev, "setting CS to %s\n", enable ? "1" : "0");
 
-	ret = sx1301_page_read(priv, ssx->page, ssx->regs + REG_RADIO_X_CS, &cs);
+	ret = regmap_read(priv->regmap, ssx->regs + REG_RADIO_X_CS, &cs);
 	if (ret) {
 		dev_warn(&ctrl->dev, "failed to read CS (%d)\n", ret);
 		cs = 0;
@@ -211,7 +145,7 @@ static int sx1301_radio_set_cs(struct spi_controller *ctrl, bool enable)
 	else
 		cs &= ~BIT(0);
 
-	ret = sx1301_page_write(priv, ssx->page, ssx->regs + REG_RADIO_X_CS, cs);
+	ret = regmap_write(priv->regmap, ssx->regs + REG_RADIO_X_CS, cs);
 	if (ret) {
 		dev_err(&ctrl->dev, "failed to write CS (%d)\n", ret);
 		return ret;
@@ -235,8 +169,8 @@ static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl,
 {
 	struct spi_sx1301 *ssx = spi_controller_get_devdata(ctrl);
 	struct sx1301_priv *priv = spi_get_drvdata(ssx->parent);
-	const u8 *tx_buf = xfr->tx_buf;
-	u8 *rx_buf = xfr->rx_buf;
+	const unsigned int *tx_buf = xfr->tx_buf;
+	unsigned int *rx_buf = xfr->rx_buf;
 	int ret;
 
 	if (xfr->len == 0 || xfr->len > 3)
@@ -245,13 +179,13 @@ static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl,
 	dev_dbg(&spi->dev, "transferring one (%u)\n", xfr->len);
 
 	if (tx_buf) {
-		ret = sx1301_page_write(priv, ssx->page, ssx->regs + REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0);
+		ret = regmap_write(priv->regmap, ssx->regs + REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0);
 		if (ret) {
 			dev_err(&spi->dev, "SPI radio address write failed\n");
 			return ret;
 		}
 
-		ret = sx1301_page_write(priv, ssx->page, ssx->regs + REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0);
+		ret = regmap_write(priv->regmap, ssx->regs + REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0);
 		if (ret) {
 			dev_err(&spi->dev, "SPI radio data write failed\n");
 			return ret;
@@ -271,7 +205,7 @@ static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl,
 	}
 
 	if (rx_buf) {
-		ret = sx1301_page_read(priv, ssx->page, ssx->regs + REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]);
+		ret = regmap_read(priv->regmap, ssx->regs + REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]);
 		if (ret) {
 			dev_err(&spi->dev, "SPI radio data read failed\n");
 			return ret;
@@ -281,17 +215,17 @@ static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl,
 	return 0;
 }
 
-static int sx1301_agc_ram_read(struct sx1301_priv *priv, u8 addr, u8 *val)
+static int sx1301_agc_ram_read(struct sx1301_priv *priv, u8 addr, unsigned int *val)
 {
 	int ret;
 
-	ret = sx1301_page_write(priv, 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(priv->dev, "AGC RAM addr write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_page_read(priv, 2, REG_2_DBG_AGC_MCU_RAM_DATA, val);
+	ret = regmap_read(priv->regmap, SX1301_DBG_AGC_MCU_RAM_DATA, val);
 	if (ret) {
 		dev_err(priv->dev, "AGC RAM data read failed\n");
 		return ret;
@@ -300,17 +234,17 @@ static int sx1301_agc_ram_read(struct sx1301_priv *priv, u8 addr, u8 *val)
 	return 0;
 }
 
-static int sx1301_arb_ram_read(struct sx1301_priv *priv, u8 addr, u8 *val)
+static int sx1301_arb_ram_read(struct sx1301_priv *priv, u8 addr, unsigned int *val)
 {
 	int ret;
 
-	ret = sx1301_page_write(priv, 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(priv->dev, "ARB RAM addr write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_page_read(priv, 2, REG_2_DBG_ARB_MCU_RAM_DATA, val);
+	ret = regmap_read(priv->regmap, SX1301_DBG_ARB_MCU_RAM_DATA, val);
 	if (ret) {
 		dev_err(priv->dev, "ARB RAM data read failed\n");
 		return ret;
@@ -322,7 +256,8 @@ static int sx1301_arb_ram_read(struct sx1301_priv *priv, u8 addr, u8 *val)
 static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct firmware *fw)
 {
 	u8 *buf;
-	u8 val, rst, select_mux;
+	u8  rst, select_mux;
+	unsigned int val;
 	int ret;
 
 	if (fw->size != SX1301_MCU_FW_BYTE) {
@@ -343,7 +278,7 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct
 		return -EINVAL;
 	}
 
-	ret = sx1301_page_read(priv, 0, REG_0_MCU, &val);
+	ret = regmap_read(priv->regmap, SX1301_MCU_CTRL, &val);
 	if (ret) {
 		dev_err(priv->dev, "MCU read failed\n");
 		return ret;
@@ -352,25 +287,25 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct
 	val |= rst;
 	val &= ~select_mux;
 
-	ret = sx1301_page_write(priv, 0, REG_0_MCU, val);
+	ret = regmap_write(priv->regmap, SX1301_MCU_CTRL, val);
 	if (ret) {
 		dev_err(priv->dev, "MCU reset / select mux write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_write(priv, REG_MCU_PROM_ADDR, 0);
+	ret = regmap_write(priv->regmap, SX1301_MPA, 0);
 	if (ret) {
 		dev_err(priv->dev, "MCU prom addr write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_write_burst(priv, REG_MCU_PROM_DATA, fw->data, fw->size);
+	ret = sx1301_write_burst(priv, SX1301_MPD, fw->data, fw->size);
 	if (ret) {
 		dev_err(priv->dev, "MCU prom data write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_read(priv, REG_MCU_PROM_DATA, &val);
+	ret = regmap_read(priv->regmap, SX1301_MPD, &val);
 	if (ret) {
 		dev_err(priv->dev, "MCU prom data dummy read failed\n");
 		return ret;
@@ -380,7 +315,7 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct
 	if (!buf)
 		return -ENOMEM;
 
-	ret = sx1301_read_burst(priv, REG_MCU_PROM_DATA, buf, fw->size);
+	ret = sx1301_read_burst(priv, SX1301_MPD, buf, fw->size);
 	if (ret) {
 		dev_err(priv->dev, "MCU prom data read failed\n");
 		kfree(buf);
@@ -395,7 +330,7 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct
 
 	kfree(buf);
 
-	ret = sx1301_page_read(priv, 0, REG_0_MCU, &val);
+	ret = regmap_read(priv->regmap, SX1301_MCU_CTRL, &val);
 	if (ret) {
 		dev_err(priv->dev, "MCU read (1) failed\n");
 		return ret;
@@ -403,7 +338,7 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct
 
 	val |= select_mux;
 
-	ret = sx1301_page_write(priv, 0, REG_0_MCU, val);
+	ret = regmap_write(priv->regmap, SX1301_MCU_CTRL, val);
 	if (ret) {
 		dev_err(priv->dev, "MCU reset / select mux write (1) failed\n");
 		return ret;
@@ -415,7 +350,7 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct
 static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 {
 	const struct firmware *fw;
-	u8 val;
+	unsigned int val;
 	int ret;
 
 	ret = request_firmware(&fw, "sx1301_agc_calibration.bin", priv->dev);
@@ -431,7 +366,7 @@ static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 		return ret;
 	}
 
-	ret = sx1301_page_read(priv, 0, 105, &val);
+	ret = regmap_read(priv->regmap, SX1301_FORCE_CTRL, &val);
 	if (ret) {
 		dev_err(priv->dev, "0|105 read failed\n");
 		return ret;
@@ -439,7 +374,7 @@ static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 
 	val &= ~REG_0_105_FORCE_HOST_RADIO_CTRL;
 
-	ret = sx1301_page_write(priv, 0, 105, val);
+	ret = regmap_write(priv->regmap, SX1301_FORCE_CTRL, val);
 	if (ret) {
 		dev_err(priv->dev, "0|105 write failed\n");
 		return ret;
@@ -449,13 +384,13 @@ static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 	if (false)
 		val |= BIT(5); /* SX1255 */
 
-	ret = sx1301_page_write(priv, 0, REG_0_RADIO_SELECT, val);
+	ret = regmap_write(priv->regmap, SX1301_CHRS, val);
 	if (ret) {
 		dev_err(priv->dev, "radio select write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_page_read(priv, 0, REG_0_MCU, &val);
+	ret = regmap_read(priv->regmap, SX1301_MCU_CTRL, &val);
 	if (ret) {
 		dev_err(priv->dev, "MCU read (0) failed\n");
 		return ret;
@@ -463,7 +398,7 @@ static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 
 	val &= ~REG_0_MCU_RST_1;
 
-	ret = sx1301_page_write(priv, 0, REG_0_MCU, val);
+	ret = regmap_write(priv->regmap, SX1301_MCU_CTRL, val);
 	if (ret) {
 		dev_err(priv->dev, "MCU write (0) failed\n");
 		return ret;
@@ -483,13 +418,7 @@ static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 		return -ENXIO;
 	}
 
-	ret = sx1301_page_switch(priv, 3);
-	if (ret) {
-		dev_err(priv->dev, "page switch 3 failed\n");
-		return ret;
-	}
-
-	ret = sx1301_read(priv, REG_EMERGENCY_FORCE, &val);
+	ret = regmap_read(priv->regmap, SX1301_EMERGENCY_FORCE_HOST_CTRL, &val);
 	if (ret) {
 		dev_err(priv->dev, "emergency force read failed\n");
 		return ret;
@@ -497,7 +426,7 @@ static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 
 	val &= ~REG_EMERGENCY_FORCE_HOST_CTRL;
 
-	ret = sx1301_write(priv, REG_EMERGENCY_FORCE, val);
+	ret = regmap_write(priv->regmap, SX1301_EMERGENCY_FORCE_HOST_CTRL, val);
 	if (ret) {
 		dev_err(priv->dev, "emergency force write failed\n");
 		return ret;
@@ -506,7 +435,7 @@ static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 	dev_err(priv->dev, "starting calibration...\n");
 	msleep(2300);
 
-	ret = sx1301_read(priv, REG_EMERGENCY_FORCE, &val);
+	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;
@@ -514,13 +443,13 @@ static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 
 	val |= REG_EMERGENCY_FORCE_HOST_CTRL;
 
-	ret = sx1301_write(priv, REG_EMERGENCY_FORCE, val);
+	ret = regmap_write(priv->regmap, SX1301_EMERGENCY_FORCE_HOST_CTRL, val);
 	if (ret) {
 		dev_err(priv->dev, "emergency force write (1) failed\n");
 		return ret;
 	}
 
-	ret = sx1301_read(priv, REG_MCU_AGC_STATUS, &val);
+	ret = regmap_read(priv->regmap, SX1301_AGCSTS, &val);
 	if (ret) {
 		dev_err(priv->dev, "AGC status read failed\n");
 		return ret;
@@ -538,7 +467,7 @@ static int sx1301_agc_calibrate(struct sx1301_priv *priv)
 static int sx1301_load_all_firmware(struct sx1301_priv *priv)
 {
 	const struct firmware *fw;
-	u8 val;
+	unsigned int val;
 	int ret;
 
 	ret = request_firmware(&fw, "sx1301_arb.bin", priv->dev);
@@ -563,7 +492,7 @@ static int sx1301_load_all_firmware(struct sx1301_priv *priv)
 	if (ret)
 		return ret;
 
-	ret = sx1301_page_read(priv, 0, 105, &val);
+	ret = regmap_read(priv->regmap, SX1301_FORCE_CTRL, &val);
 	if (ret) {
 		dev_err(priv->dev, "0|105 read failed\n");
 		return ret;
@@ -571,19 +500,19 @@ static int sx1301_load_all_firmware(struct sx1301_priv *priv)
 
 	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(priv, 0, 105, val);
+	ret = regmap_write(priv->regmap, SX1301_FORCE_CTRL, val);
 	if (ret) {
 		dev_err(priv->dev, "0|105 write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_page_write(priv, 0, REG_0_RADIO_SELECT, 0);
+	ret = regmap_write(priv->regmap, SX1301_CHRS, 0);
 	if (ret) {
 		dev_err(priv->dev, "radio select write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_page_read(priv, 0, REG_0_MCU, &val);
+	ret = regmap_read(priv->regmap, SX1301_MCU_CTRL, &val);
 	if (ret) {
 		dev_err(priv->dev, "MCU read (0) failed\n");
 		return ret;
@@ -591,7 +520,7 @@ static int sx1301_load_all_firmware(struct sx1301_priv *priv)
 
 	val &= ~(REG_0_MCU_RST_1 | REG_0_MCU_RST_0);
 
-	ret = sx1301_page_write(priv, 0, REG_0_MCU, val);
+	ret = regmap_write(priv->regmap, SX1301_MCU_CTRL, val);
 	if (ret) {
 		dev_err(priv->dev, "MCU write (0) failed\n");
 		return ret;
@@ -645,7 +574,7 @@ static int sx1301_probe(struct spi_device *spi)
 	struct gpio_desc *rst;
 	int ret;
 	unsigned int ver;
-	u8 val;
+	unsigned int val;
 
 	rst = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(rst))
@@ -665,7 +594,6 @@ static int sx1301_probe(struct spi_device *spi)
 
 	priv = netdev_priv(netdev);
 	priv->rst_gpio = rst;
-	priv->cur_page = 0xff;
 
 	spi_set_drvdata(spi, priv);
 	priv->dev = &spi->dev;
@@ -690,7 +618,7 @@ static int sx1301_probe(struct spi_device *spi)
 		return -ENXIO;
 	}
 
-	ret = sx1301_write(priv, 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;
@@ -702,7 +630,7 @@ static int sx1301_probe(struct spi_device *spi)
 		return ret;
 	}
 
-	ret = sx1301_read(priv, 16, &val);
+	ret = regmap_read(priv->regmap, SX1301_GEN, &val);
 	if (ret) {
 		dev_err(&spi->dev, "16 read failed\n");
 		return ret;
@@ -710,13 +638,13 @@ static int sx1301_probe(struct spi_device *spi)
 
 	val &= ~REG_16_GLOBAL_EN;
 
-	ret = sx1301_write(priv, 16, val);
+	ret = regmap_write(priv->regmap, SX1301_GEN, val);
 	if (ret) {
 		dev_err(&spi->dev, "16 write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_read(priv, 17, &val);
+	ret = regmap_read(priv->regmap, SX1301_CKEN, &val);
 	if (ret) {
 		dev_err(&spi->dev, "17 read failed\n");
 		return ret;
@@ -724,13 +652,13 @@ static int sx1301_probe(struct spi_device *spi)
 
 	val &= ~REG_17_CLK32M_EN;
 
-	ret = sx1301_write(priv, 17, val);
+	ret = regmap_write(priv->regmap, SX1301_CKEN, val);
 	if (ret) {
 		dev_err(&spi->dev, "17 write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_page_read(priv, 2, 43, &val);
+	ret = regmap_read(priv->regmap, SX1301_RADIO_CFG, &val);
 	if (ret) {
 		dev_err(&spi->dev, "2|43 read failed\n");
 		return ret;
@@ -738,7 +666,7 @@ static int sx1301_probe(struct spi_device *spi)
 
 	val |= REG_2_43_RADIO_B_EN | REG_2_43_RADIO_A_EN;
 
-	ret = sx1301_page_write(priv, 2, 43, val);
+	ret = regmap_write(priv->regmap, SX1301_RADIO_CFG, val);
 	if (ret) {
 		dev_err(&spi->dev, "2|43 write failed\n");
 		return ret;
@@ -746,7 +674,7 @@ static int sx1301_probe(struct spi_device *spi)
 
 	msleep(500);
 
-	ret = sx1301_page_read(priv, 2, 43, &val);
+	ret = regmap_read(priv->regmap, SX1301_RADIO_CFG, &val);
 	if (ret) {
 		dev_err(&spi->dev, "2|43 read failed\n");
 		return ret;
@@ -754,7 +682,7 @@ static int sx1301_probe(struct spi_device *spi)
 
 	val |= REG_2_43_RADIO_RST;
 
-	ret = sx1301_page_write(priv, 2, 43, val);
+	ret = regmap_write(priv->regmap, SX1301_RADIO_CFG, val);
 	if (ret) {
 		dev_err(&spi->dev, "2|43 write failed\n");
 		return ret;
@@ -762,7 +690,7 @@ static int sx1301_probe(struct spi_device *spi)
 
 	msleep(5);
 
-	ret = sx1301_page_read(priv, 2, 43, &val);
+	ret = regmap_read(priv->regmap, SX1301_RADIO_CFG, &val);
 	if (ret) {
 		dev_err(&spi->dev, "2|43 read failed\n");
 		return ret;
@@ -770,7 +698,7 @@ static int sx1301_probe(struct spi_device *spi)
 
 	val &= ~REG_2_43_RADIO_RST;
 
-	ret = sx1301_page_write(priv, 2, 43, val);
+	ret = regmap_write(priv->regmap, SX1301_RADIO_CFG, val);
 	if (ret) {
 		dev_err(&spi->dev, "2|43 write failed\n");
 		return ret;
@@ -787,8 +715,7 @@ static int sx1301_probe(struct spi_device *spi)
 	priv->radio_a_ctrl->dev.of_node = of_get_child_by_name(spi->dev.of_node, "radio-a");
 
 	radio = spi_controller_get_devdata(priv->radio_a_ctrl);
-	radio->page = 2;
-	radio->regs = REG_2_SPI_RADIO_A_DATA;
+	radio->regs = SX1301_RADIO_A_SPI_DATA;
 	radio->parent = spi;
 
 	ret = devm_spi_register_controller(&spi->dev, priv->radio_a_ctrl);
@@ -809,8 +736,7 @@ static int sx1301_probe(struct spi_device *spi)
 	priv->radio_b_ctrl->dev.of_node = of_get_child_by_name(spi->dev.of_node, "radio-b");
 
 	radio = spi_controller_get_devdata(priv->radio_b_ctrl);
-	radio->page = 2;
-	radio->regs = REG_2_SPI_RADIO_B_DATA;
+	radio->regs = SX1301_RADIO_B_SPI_DATA;
 	radio->parent = spi;
 
 	ret = devm_spi_register_controller(&spi->dev, priv->radio_b_ctrl);
@@ -822,7 +748,7 @@ static int sx1301_probe(struct spi_device *spi)
 
 	/* GPIO */
 
-	ret = sx1301_read(priv, REG_GPIO_MODE, &val);
+	ret = regmap_read(priv->regmap, SX1301_GPMODE, &val);
 	if (ret) {
 		dev_err(&spi->dev, "GPIO mode read failed\n");
 		return ret;
@@ -830,13 +756,13 @@ static int sx1301_probe(struct spi_device *spi)
 
 	val |= GENMASK(4, 0);
 
-	ret = sx1301_write(priv, REG_GPIO_MODE, val);
+	ret = regmap_write(priv->regmap, SX1301_GPMODE, val);
 	if (ret) {
 		dev_err(&spi->dev, "GPIO mode write failed\n");
 		return ret;
 	}
 
-	ret = sx1301_read(priv, REG_GPIO_SELECT_OUTPUT, &val);
+	ret = regmap_read(priv->regmap, SX1301_GPSO, &val);
 	if (ret) {
 		dev_err(&spi->dev, "GPIO select output read failed\n");
 		return ret;
@@ -845,7 +771,7 @@ static int sx1301_probe(struct spi_device *spi)
 	val &= ~GENMASK(3, 0);
 	val |= 2;
 
-	ret = sx1301_write(priv, REG_GPIO_SELECT_OUTPUT, val);
+	ret = regmap_write(priv->regmap, SX1301_GPSO, val);
 	if (ret) {
 		dev_err(&spi->dev, "GPIO select output write failed\n");
 		return ret;
@@ -853,7 +779,7 @@ static int sx1301_probe(struct spi_device *spi)
 
 	/* TODO LBT */
 
-	ret = sx1301_read(priv, 16, &val);
+	ret = regmap_read(priv->regmap, SX1301_GEN, &val);
 	if (ret) {
 		dev_err(&spi->dev, "16 read (1) failed\n");
 		return ret;
@@ -861,13 +787,13 @@ static int sx1301_probe(struct spi_device *spi)
 
 	val |= REG_16_GLOBAL_EN;
 
-	ret = sx1301_write(priv, 16, val);
+	ret = regmap_write(priv->regmap, SX1301_GEN, val);
 	if (ret) {
 		dev_err(&spi->dev, "16 write (1) failed\n");
 		return ret;
 	}
 
-	ret = sx1301_read(priv, 17, &val);
+	ret = regmap_read(priv->regmap, SX1301_CKEN, &val);
 	if (ret) {
 		dev_err(&spi->dev, "17 read (1) failed\n");
 		return ret;
@@ -875,7 +801,7 @@ static int sx1301_probe(struct spi_device *spi)
 
 	val |= REG_17_CLK32M_EN;
 
-	ret = sx1301_write(priv, 17, val);
+	ret = regmap_write(priv->regmap, SX1301_CKEN, val);
 	if (ret) {
 		dev_err(&spi->dev, "17 write (1) failed\n");
 		return ret;
diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
index 2fc283f..b21e5c6 100644
--- a/drivers/net/lora/sx1301.h
+++ b/drivers/net/lora/sx1301.h
@@ -18,11 +18,41 @@
 /* Page independent */
 #define SX1301_PAGE     0x00
 #define SX1301_VER      0x01
+#define SX1301_MPA      0x09
+#define SX1301_MPD      0x0A
+#define SX1301_GEN      0x10
+#define SX1301_CKEN     0x11
+#define SX1301_GPSO     0x1C
+#define SX1301_GPMODE   0x1D
+#define SX1301_AGCSTS   0x20
 
 #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_CHRS         (SX1301_PAGE_BASE(0) + 0x23)
+#define SX1301_FORCE_CTRL   (SX1301_PAGE_BASE(0) + 0x69)
+#define SX1301_MCU_CTRL     (SX1301_PAGE_BASE(0) + 0x6A)
+
+/* 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_DBG_ARB_MCU_RAM_DATA (SX1301_PAGE_BASE(2) + 0x40)
+#define SX1301_DBG_AGC_MCU_RAM_DATA (SX1301_PAGE_BASE(2) + 0x41)
+#define SX1301_DBG_ARB_MCU_RAM_ADDR (SX1301_PAGE_BASE(2) + 0x50)
+#define SX1301_DBG_AGC_MCU_RAM_ADDR (SX1301_PAGE_BASE(2) + 0x51)
+
+/* 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 v2 8/8] net: lora: sx1301: convert driver over to regmap reads and writes

[Andreas Färber]

Am 09.08.2018 um 14:33 schrieb Ben Whitten:
> The reads and writes are replaced with regmap versions and unneeded
> functions, variable, and defines removed.
> 
> Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
> ---
>  drivers/net/lora/sx1301.c | 204 +++++++++++++++-------------------------------
>  drivers/net/lora/sx1301.h |  30 +++++++
>  2 files changed, 95 insertions(+), 139 deletions(-)
> 
> diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
> index 766df06..4db5a43 100644
> --- a/drivers/net/lora/sx1301.c
> +++ b/drivers/net/lora/sx1301.c
[...]
> @@ -140,50 +115,9 @@ static int sx1301_write(struct sx1301_priv *priv, u8 reg, u8 val)
>  	return sx1301_write_burst(priv, reg, &val, 1);
>  }

_write and _read are now unused, causing warnings. Dropping.

The _burst versions are still in use for firmware load, and I saw a
discussion indicating that regmap is lacking the capability to not
increment the reg for bulk reads at the moment. So we still can't
cleanly switch to regmap entirely and thereby remain bound to SPI.

[...]
> @@ -235,8 +169,8 @@ static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl,
>  {
>  	struct spi_sx1301 *ssx = spi_controller_get_devdata(ctrl);
>  	struct sx1301_priv *priv = spi_get_drvdata(ssx->parent);
> -	const u8 *tx_buf = xfr->tx_buf;
> -	u8 *rx_buf = xfr->rx_buf;
> +	const unsigned int *tx_buf = xfr->tx_buf;
> +	unsigned int *rx_buf = xfr->rx_buf;

These are wrong both for Little Endian and even worse for Big Endian.

>  	int ret;
>  
>  	if (xfr->len == 0 || xfr->len > 3)
> @@ -245,13 +179,13 @@ static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl,
>  	dev_dbg(&spi->dev, "transferring one (%u)\n", xfr->len);
>  
>  	if (tx_buf) {
> -		ret = sx1301_page_write(priv, ssx->page, ssx->regs + REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0);
> +		ret = regmap_write(priv->regmap, ssx->regs + REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0);
>  		if (ret) {
>  			dev_err(&spi->dev, "SPI radio address write failed\n");
>  			return ret;
>  		}
>  
> -		ret = sx1301_page_write(priv, ssx->page, ssx->regs + REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0);
> +		ret = regmap_write(priv->regmap, ssx->regs + REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0);
>  		if (ret) {
>  			dev_err(&spi->dev, "SPI radio data write failed\n");
>  			return ret;
> @@ -271,7 +205,7 @@ static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl,
>  	}
>  
>  	if (rx_buf) {
> -		ret = sx1301_page_read(priv, ssx->page, ssx->regs + REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]);
> +		ret = regmap_read(priv->regmap, ssx->regs + REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]);
>  		if (ret) {
>  			dev_err(&spi->dev, "SPI radio data read failed\n");
>  			return ret;

Fixing by adding a local variable instead:

@@ -239,6 +163,7 @@ static int sx1301_radio_spi_transfer_one(struct
spi_controll
er *ctrl,
        struct sx1301_priv *priv = netdev_priv(netdev);
        const u8 *tx_buf = xfr->tx_buf;
        u8 *rx_buf = xfr->rx_buf;
+       unsigned int val;
        int ret;

        if (xfr->len == 0 || xfr->len > 3)
[...]
@@ -273,27 +198,28 @@ static int sx1301_radio_spi_transfer_one(struct
spi_controller *ctrl,
        }

        if (rx_buf) {
-               ret = sx1301_page_read(priv, ssx->page, ssx->regs +
REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]);
+               ret = regmap_read(priv->regmap, ssx->regs +
REG_RADIO_X_DATA_READBACK, &val);
                if (ret) {
                        dev_err(&spi->dev, "SPI radio data read failed\n");
                        return ret;
                }
+               rx_buf[xfr->len - 1] = val & 0xff;
        }

        return 0;

[...]
> diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
> index 2fc283f..b21e5c6 100644
> --- a/drivers/net/lora/sx1301.h
> +++ b/drivers/net/lora/sx1301.h
> @@ -18,11 +18,41 @@
>  /* Page independent */
>  #define SX1301_PAGE     0x00
>  #define SX1301_VER      0x01
> +#define SX1301_MPA      0x09

Those are the official register names? I find these much harder to read
than my guessed names. Could we keep the long names as aliases?

> +#define SX1301_MPD      0x0A
> +#define SX1301_GEN      0x10
> +#define SX1301_CKEN     0x11
> +#define SX1301_GPSO     0x1C
> +#define SX1301_GPMODE   0x1D
> +#define SX1301_AGCSTS   0x20
>  
>  #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_CHRS         (SX1301_PAGE_BASE(0) + 0x23)
> +#define SX1301_FORCE_CTRL   (SX1301_PAGE_BASE(0) + 0x69)
> +#define SX1301_MCU_CTRL     (SX1301_PAGE_BASE(0) + 0x6A)
> +
> +/* 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_DBG_ARB_MCU_RAM_DATA (SX1301_PAGE_BASE(2) + 0x40)
> +#define SX1301_DBG_AGC_MCU_RAM_DATA (SX1301_PAGE_BASE(2) + 0x41)
> +#define SX1301_DBG_ARB_MCU_RAM_ADDR (SX1301_PAGE_BASE(2) + 0x50)
> +#define SX1301_DBG_AGC_MCU_RAM_ADDR (SX1301_PAGE_BASE(2) + 0x51)
> +
> +/* Page 3 */
> +#define SX1301_EMERGENCY_FORCE_HOST_CTRL (SX1301_PAGE_BASE(3) + 0x7F)
> +
>  #define SX1301_MAX_REGISTER         (SX1301_PAGE_BASE(3) + 0x7F)
>  
>  #endif

Applying so that we can continue based on regmap.

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 v2 8/8] net: lora: sx1301: convert driver over to regmap reads and writes

[Ben Whitten]

On Thu, 9 Aug 2018 at 23:34, Andreas Färber <afaerber@suse.de> wrote:
>
> Am 09.08.2018 um 14:33 schrieb Ben Whitten:
> > The reads and writes are replaced with regmap versions and unneeded
> > functions, variable, and defines removed.
> >
> > Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
> > ---
> >  drivers/net/lora/sx1301.c | 204 +++++++++++++++-------------------------------
> >  drivers/net/lora/sx1301.h |  30 +++++++
> >  2 files changed, 95 insertions(+), 139 deletions(-)
> >
> > diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
> > index 766df06..4db5a43 100644
> > --- a/drivers/net/lora/sx1301.c
> > +++ b/drivers/net/lora/sx1301.c
> [...]
> > @@ -140,50 +115,9 @@ static int sx1301_write(struct sx1301_priv *priv, u8 reg, u8 val)
> >       return sx1301_write_burst(priv, reg, &val, 1);
> >  }
>
> _write and _read are now unused, causing warnings. Dropping.
>
> The _burst versions are still in use for firmware load, and I saw a
> discussion indicating that regmap is lacking the capability to not
> increment the reg for bulk reads at the moment. So we still can't
> cleanly switch to regmap entirely and thereby remain bound to SPI.
>
> [...]
> > @@ -235,8 +169,8 @@ static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl,
> >  {
> >       struct spi_sx1301 *ssx = spi_controller_get_devdata(ctrl);
> >       struct sx1301_priv *priv = spi_get_drvdata(ssx->parent);
> > -     const u8 *tx_buf = xfr->tx_buf;
> > -     u8 *rx_buf = xfr->rx_buf;
> > +     const unsigned int *tx_buf = xfr->tx_buf;
> > +     unsigned int *rx_buf = xfr->rx_buf;
>
> These are wrong both for Little Endian and even worse for Big Endian.
>
> >       int ret;
> >
> >       if (xfr->len == 0 || xfr->len > 3)
> > @@ -245,13 +179,13 @@ static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl,
> >       dev_dbg(&spi->dev, "transferring one (%u)\n", xfr->len);
> >
> >       if (tx_buf) {
> > -             ret = sx1301_page_write(priv, ssx->page, ssx->regs + REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0);
> > +             ret = regmap_write(priv->regmap, ssx->regs + REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0);
> >               if (ret) {
> >                       dev_err(&spi->dev, "SPI radio address write failed\n");
> >                       return ret;
> >               }
> >
> > -             ret = sx1301_page_write(priv, ssx->page, ssx->regs + REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0);
> > +             ret = regmap_write(priv->regmap, ssx->regs + REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0);
> >               if (ret) {
> >                       dev_err(&spi->dev, "SPI radio data write failed\n");
> >                       return ret;
> > @@ -271,7 +205,7 @@ static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl,
> >       }
> >
> >       if (rx_buf) {
> > -             ret = sx1301_page_read(priv, ssx->page, ssx->regs + REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]);
> > +             ret = regmap_read(priv->regmap, ssx->regs + REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]);
> >               if (ret) {
> >                       dev_err(&spi->dev, "SPI radio data read failed\n");
> >                       return ret;
>
> Fixing by adding a local variable instead:
>
> @@ -239,6 +163,7 @@ static int sx1301_radio_spi_transfer_one(struct
> spi_controll
> er *ctrl,
>         struct sx1301_priv *priv = netdev_priv(netdev);
>         const u8 *tx_buf = xfr->tx_buf;
>         u8 *rx_buf = xfr->rx_buf;
> +       unsigned int val;
>         int ret;
>
>         if (xfr->len == 0 || xfr->len > 3)
> [...]
> @@ -273,27 +198,28 @@ static int sx1301_radio_spi_transfer_one(struct
> spi_controller *ctrl,
>         }
>
>         if (rx_buf) {
> -               ret = sx1301_page_read(priv, ssx->page, ssx->regs +
> REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]);
> +               ret = regmap_read(priv->regmap, ssx->regs +
> REG_RADIO_X_DATA_READBACK, &val);
>                 if (ret) {
>                         dev_err(&spi->dev, "SPI radio data read failed\n");
>                         return ret;
>                 }
> +               rx_buf[xfr->len - 1] = val & 0xff;
>         }
>
>         return 0;
>
> [...]
> > diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
> > index 2fc283f..b21e5c6 100644
> > --- a/drivers/net/lora/sx1301.h
> > +++ b/drivers/net/lora/sx1301.h
> > @@ -18,11 +18,41 @@
> >  /* Page independent */
> >  #define SX1301_PAGE     0x00
> >  #define SX1301_VER      0x01
> > +#define SX1301_MPA      0x09
>
> Those are the official register names? I find these much harder to read
> than my guessed names. Could we keep the long names as aliases?

Yes these are the official register names, aliases to improve readability
sound like a good plan as all the official names are terse.

> > +#define SX1301_MPD      0x0A
> > +#define SX1301_GEN      0x10
> > +#define SX1301_CKEN     0x11
> > +#define SX1301_GPSO     0x1C
> > +#define SX1301_GPMODE   0x1D
> > +#define SX1301_AGCSTS   0x20
> >
> >  #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_CHRS         (SX1301_PAGE_BASE(0) + 0x23)
> > +#define SX1301_FORCE_CTRL   (SX1301_PAGE_BASE(0) + 0x69)
> > +#define SX1301_MCU_CTRL     (SX1301_PAGE_BASE(0) + 0x6A)
> > +
> > +/* 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_DBG_ARB_MCU_RAM_DATA (SX1301_PAGE_BASE(2) + 0x40)
> > +#define SX1301_DBG_AGC_MCU_RAM_DATA (SX1301_PAGE_BASE(2) + 0x41)
> > +#define SX1301_DBG_ARB_MCU_RAM_ADDR (SX1301_PAGE_BASE(2) + 0x50)
> > +#define SX1301_DBG_AGC_MCU_RAM_ADDR (SX1301_PAGE_BASE(2) + 0x51)
> > +
> > +/* Page 3 */
> > +#define SX1301_EMERGENCY_FORCE_HOST_CTRL (SX1301_PAGE_BASE(3) + 0x7F)
> > +
> >  #define SX1301_MAX_REGISTER         (SX1301_PAGE_BASE(3) + 0x7F)
> >
> >  #endif
>
> Applying so that we can continue based on regmap.

Thanks!
Ben Whitten

Re: [PATCH lora-next v2 8/8] net: lora: sx1301: convert driver over to regmap reads and writes

[Andreas Färber]

Am 10.08.2018 um 00:47 schrieb Ben Whitten:
> On Thu, 9 Aug 2018 at 23:34, Andreas Färber <afaerber@suse.de> wrote:
>> Applying so that we can continue based on regmap.
> 
> Thanks!

Rebased onto latest linux-next, tested on RAK831 and pushed:

https://git.kernel.org/pub/scm/linux/kernel/git/afaerber/linux-lora.git/log/?h=lora-next

Next steps for me will be to apply your devm_alloc_loradev() to all my
other drivers, and to convert sx1276 to regmap, too.

Cheers,
Andreas

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

Re: [PATCH lora-next v2 1/8] net: lora: add methods for devm registration

[Andreas Färber]

Am 09.08.2018 um 14:33 schrieb Ben Whitten:
> 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   | 28 ++++++++++++++++++++++++++++
>  include/linux/lora/dev.h |  1 +
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/net/lora/dev.c b/drivers/net/lora/dev.c
> index 8c01106..e32a870 100644
> --- a/drivers/net/lora/dev.c
> +++ b/drivers/net/lora/dev.c
> @@ -84,6 +84,34 @@ void free_loradev(struct net_device *dev)
>  }
>  EXPORT_SYMBOL_GPL(free_loradev);
>  
> +static void devm_free_loradev(struct device *dev, void *res)
> +{
> +	struct net_device *net = (*(struct net_device **)res);
> +	free_loradev(net);

This is what I meant with adding a variable:

diff --git a/drivers/net/lora/dev.c b/drivers/net/lora/dev.c
index c1b196cdf835..0d4823de8c06 100644
--- a/drivers/net/lora/dev.c
+++ b/drivers/net/lora/dev.c
@@ -87,8 +87,9 @@ EXPORT_SYMBOL_GPL(free_loradev);

 static void devm_free_loradev(struct device *dev, void *res)
 {
-       struct net_device *net = (*(struct net_device **)res);
-       free_loradev(net);
+       struct net_device **net = res;
+
+       free_loradev(*net);
 }

 struct net_device *devm_alloc_loradev(struct device *dev, size_t priv)

Applying.

Thanks,
Andreas

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