diff mbox series

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

Message ID 20181219155616.9547-2-ben.whitten@lairdtech.com
State New
Headers show
Series Get sx1301 to transmit lora packets | expand

Commit Message

Ben Whitten Dec. 19, 2018, 3:56 p.m. UTC 
Checkpatch highlights some style issues which need to be addressed.

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

Comments

Andreas Färber Dec. 29, 2018, 12:05 a.m. UTC | #1 
Hi Ben,

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

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

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

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

Regards,
Andreas
Ben Whitten Jan. 5, 2019, 7:36 a.m. UTC | #2 
Hi Andreas,

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

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

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

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

Thanks!
Ben Whitten
diff mbox series

Patch

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