| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e7b112d9-d8ac-1654-fc97-9289180d6a02@suse.de>
Date: Fri, 10 Aug 2018 00:34:31 +0200
From: Andreas Färber <afaerber@...e.de>
To: Ben Whitten <ben.whitten@...il.com>
Cc: starnight@...cu.edu.tw, hasnain.virk@....com,
netdev@...r.kernel.org, liuxuenetmail@...il.com, shess@...sware.de,
Ben Whitten <ben.whitten@...rdtech.com>,
Yannick Lanz <yannick.lanz@...ixone.io>
Subject: Re: [PATCH lora-next v2 8/8] net: lora: sx1301: convert driver over
to regmap reads and writes
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@...rdtech.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)
Powered by blists - more mailing lists