[PATCH 0/6] Fix indentation and CamelCase issues in staging/pi433

[PATCH 0/6] Fix indentation and CamelCase issues in staging/pi433

[Simon Sandström]

These patches fixes a bunch of code style issues in staging/pi433. The
first patch fixes indentation in rf69_enum.h and the rest of the patches
fixes CamelCase issues in all of staging/pi433.

In total the patches get rids of around 140 warnings generated by
checkpatch.pl.

- Simon

---

Simon Sandström (6):
  staging: pi433: Fix indentation in rf69_enum.h
  staging: pi433: Capitalize constant definitions
  staging: pi433: Rename variable in struct pi433_rx_cfg
  staging: pi433: Rename enum optionOnOff in rf69_enum.h
  staging: pi433: Rename enum dataMode in rf69_enum.h
  staging: pi433: Rename enum modShaping in rf69_enum.h

 drivers/staging/pi433/pi433_if.c       |  72 +++++------
 drivers/staging/pi433/pi433_if.h       |  20 ++--
 drivers/staging/pi433/rf69.c           |  76 ++++++------
 drivers/staging/pi433/rf69.h           |  19 +--
 drivers/staging/pi433/rf69_enum.h      | 213 ++++++++++++++++-----------------
 drivers/staging/pi433/rf69_registers.h |  44 +++----
 6 files changed, 227 insertions(+), 217 deletions(-)

-- 
2.11.0

[PATCH 1/6] staging: pi433: Fix indentation in rf69_enum.h

[Simon Sandström]

Basically just 's/    /\t/', to fix checkpatch.pl warnings:
"please, no spaces at the start of a line".

Signed-off-by: Simon Sandström <simon@nikanor.nu>
---
 drivers/staging/pi433/rf69_enum.h | 207 +++++++++++++++++++-------------------
 1 file changed, 103 insertions(+), 104 deletions(-)

diff --git a/drivers/staging/pi433/rf69_enum.h b/drivers/staging/pi433/rf69_enum.h
index 86429aa66ad1..babe597e2ec6 100644
--- a/drivers/staging/pi433/rf69_enum.h
+++ b/drivers/staging/pi433/rf69_enum.h
@@ -19,164 +19,163 @@
 #define RF69_ENUM_H
 
 enum optionOnOff {
-    optionOff,
-    optionOn
+	optionOff,
+	optionOn
 };
 
 enum mode {
-    mode_sleep,
-    standby,
-    synthesizer,
-    transmit,
-    receive
+	mode_sleep,
+	standby,
+	synthesizer,
+	transmit,
+	receive
 };
 
 enum dataMode {
-    packet,
-    continuous,
-    continuousNoSync
+	packet,
+	continuous,
+	continuousNoSync
 };
 
 enum modulation {
-    OOK,
-    FSK
+	OOK,
+	FSK
 };
 
 enum modShaping {
-    shapingOff,
-    shaping1_0,
-    shaping0_5,
-    shaping0_3,
-    shapingBR,
-    shaping2BR
+	shapingOff,
+	shaping1_0,
+	shaping0_5,
+	shaping0_3,
+	shapingBR,
+	shaping2BR
 };
 
 enum paRamp {
-    ramp3400,
-    ramp2000,
-    ramp1000,
-    ramp500,
-    ramp250,
-    ramp125,
-    ramp100,
-    ramp62,
-    ramp50,
-    ramp40,
-    ramp31,
-    ramp25,
-    ramp20,
-    ramp15,
-    ramp12,
-    ramp10
+	ramp3400,
+	ramp2000,
+	ramp1000,
+	ramp500,
+	ramp250,
+	ramp125,
+	ramp100,
+	ramp62,
+	ramp50,
+	ramp40,
+	ramp31,
+	ramp25,
+	ramp20,
+	ramp15,
+	ramp12,
+	ramp10
 };
 
 enum antennaImpedance {
-    fiftyOhm,
-    twohundretOhm
+	fiftyOhm,
+	twohundretOhm
 };
 
 enum lnaGain {
-    automatic,
-    max,
-    maxMinus6,
-    maxMinus12,
-    maxMinus24,
-    maxMinus36,
-    maxMinus48,
-    undefined
+	automatic,
+	max,
+	maxMinus6,
+	maxMinus12,
+	maxMinus24,
+	maxMinus36,
+	maxMinus48,
+	undefined
 };
 
 enum dccPercent {
-    dcc16Percent,
-    dcc8Percent,
-    dcc4Percent,
-    dcc2Percent,
-    dcc1Percent,
-    dcc0_5Percent,
-    dcc0_25Percent,
-    dcc0_125Percent
+	dcc16Percent,
+	dcc8Percent,
+	dcc4Percent,
+	dcc2Percent,
+	dcc1Percent,
+	dcc0_5Percent,
+	dcc0_25Percent,
+	dcc0_125Percent
 };
 
 enum mantisse {
-    mantisse16,
-    mantisse20,
-    mantisse24
+	mantisse16,
+	mantisse20,
+	mantisse24
 };
 
 enum thresholdType {
-    fixed,
-    peak,
-    average
+	fixed,
+	peak,
+	average
 };
 
 enum thresholdStep {
-    step_0_5db,
-    step_1_0db,
-    step_1_5db,
-    step_2_0db,
-    step_3_0db,
-    step_4_0db,
-    step_5_0db,
-    step_6_0db
+	step_0_5db,
+	step_1_0db,
+	step_1_5db,
+	step_2_0db,
+	step_3_0db,
+	step_4_0db,
+	step_5_0db,
+	step_6_0db
 };
 
 enum thresholdDecrement {
-    dec_every8th,
-    dec_every4th,
-    dec_every2nd,
-    dec_once,
-    dec_twice,
-    dec_4times,
-    dec_8times,
-    dec_16times
+	dec_every8th,
+	dec_every4th,
+	dec_every2nd,
+	dec_once,
+	dec_twice,
+	dec_4times,
+	dec_8times,
+	dec_16times
 };
 
 enum flag {
-    modeSwitchCompleted,
-    readyToReceive,
-    readyToSend,
-    pllLocked,
-    rssiExceededThreshold,
-    timeout,
-    automode,
-    syncAddressMatch,
-    fifoFull,
-//    fifoNotEmpty, collision with next enum; replaced by following enum...
-    fifoEmpty,
-    fifoLevelBelowThreshold,
-    fifoOverrun,
-    packetSent,
-    payloadReady,
-    crcOk,
-    batteryLow
+	modeSwitchCompleted,
+	readyToReceive,
+	readyToSend,
+	pllLocked,
+	rssiExceededThreshold,
+	timeout,
+	automode,
+	syncAddressMatch,
+	fifoFull,
+//	fifoNotEmpty, collision with next enum; replaced by following enum...
+	fifoEmpty,
+	fifoLevelBelowThreshold,
+	fifoOverrun,
+	packetSent,
+	payloadReady,
+	crcOk,
+	batteryLow
 };
 
 enum fifoFillCondition {
-    afterSyncInterrupt,
-    always
+	afterSyncInterrupt,
+	always
 };
 
 enum packetFormat {
-    packetLengthFix,
-    packetLengthVar
+	packetLengthFix,
+	packetLengthVar
 };
 
 enum txStartCondition {
-    fifoLevel,
-    fifoNotEmpty
+	fifoLevel,
+	fifoNotEmpty
 };
 
 enum addressFiltering {
-    filteringOff,
-    nodeAddress,
-    nodeOrBroadcastAddress
+	filteringOff,
+	nodeAddress,
+	nodeOrBroadcastAddress
 };
 
 enum dagc {
-    normalMode,
-    improve,
-    improve4LowModulationIndex
+	normalMode,
+	improve,
+	improve4LowModulationIndex
 };
 
-
 #endif
-- 
2.11.0

[PATCH 2/6] staging: pi433: Capitalize constant definitions

[Simon Sandström]

Fixes checkpatch.pl warnings "Avoid CamelCase <DIO_x>".

Signed-off-by: Simon Sandström <simon@nikanor.nu>
---
 drivers/staging/pi433/pi433_if.c       | 32 ++++++++++++-------------
 drivers/staging/pi433/rf69_registers.h | 44 +++++++++++++++++-----------------
 2 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 3404cb9722c9..840a7c7bf19a 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -133,20 +133,20 @@ static irqreturn_t DIO0_irq_handler(int irq, void *dev_id)
 {
 	struct pi433_device *device = dev_id;
 
-	if      (device->irq_state[DIO0] == DIO_PacketSent)
+	if      (device->irq_state[DIO0] == DIO_PACKET_SENT)
 	{
 		device->free_in_fifo = FIFO_SIZE;
 		dev_dbg(device->dev, "DIO0 irq: Packet sent\n");
 		wake_up_interruptible(&device->fifo_wait_queue);
 	}
-	else if (device->irq_state[DIO0] == DIO_Rssi_DIO0)
+	else if (device->irq_state[DIO0] == DIO_RSSI_DIO0)
 	{
 		dev_dbg(device->dev, "DIO0 irq: RSSI level over threshold\n");
 		wake_up_interruptible(&device->rx_wait_queue);
 	}
-	else if (device->irq_state[DIO0] == DIO_PayloadReady)
+	else if (device->irq_state[DIO0] == DIO_PAYLOAD_READY)
 	{
-		dev_dbg(device->dev, "DIO0 irq: PayloadReady\n");
+		dev_dbg(device->dev, "DIO0 irq: Payload ready\n");
 		device->free_in_fifo = 0;
 		wake_up_interruptible(&device->fifo_wait_queue);
 	}
@@ -158,11 +158,11 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
 {
 	struct pi433_device *device = dev_id;
 
-	if      (device->irq_state[DIO1] == DIO_FifoNotEmpty_DIO1)
+	if      (device->irq_state[DIO1] == DIO_FIFO_NOT_EMPTY_DIO1)
 	{
 		device->free_in_fifo = FIFO_SIZE;
 	}
-	else if (device->irq_state[DIO1] == DIO_FifoLevel)
+	else if (device->irq_state[DIO1] == DIO_FIFO_LEVEL)
 	{
 		if (device->rx_active)	device->free_in_fifo = FIFO_THRESHOLD - 1;
 		else			device->free_in_fifo = FIFO_SIZE - FIFO_THRESHOLD - 1;
@@ -309,14 +309,14 @@ pi433_start_rx(struct pi433_device *dev)
 	if (retval) return retval;
 
 	/* setup rssi irq */
-	SET_CHECKED(rf69_set_dio_mapping(dev->spi, DIO0, DIO_Rssi_DIO0));
-	dev->irq_state[DIO0] = DIO_Rssi_DIO0;
+	SET_CHECKED(rf69_set_dio_mapping(dev->spi, DIO0, DIO_RSSI_DIO0));
+	dev->irq_state[DIO0] = DIO_RSSI_DIO0;
 	irq_set_irq_type(dev->irq_num[DIO0], IRQ_TYPE_EDGE_RISING);
 
 	/* setup fifo level interrupt */
 	SET_CHECKED(rf69_set_fifo_threshold(dev->spi, FIFO_SIZE - FIFO_THRESHOLD));
-	SET_CHECKED(rf69_set_dio_mapping(dev->spi, DIO1, DIO_FifoLevel));
-	dev->irq_state[DIO1] = DIO_FifoLevel;
+	SET_CHECKED(rf69_set_dio_mapping(dev->spi, DIO1, DIO_FIFO_LEVEL));
+	dev->irq_state[DIO1] = DIO_FIFO_LEVEL;
 	irq_set_irq_type(dev->irq_num[DIO1], IRQ_TYPE_EDGE_RISING);
 
 	/* set module to receiving mode */
@@ -378,8 +378,8 @@ pi433_receive(void *data)
 	}
 
 	/* configure payload ready irq */
-	SET_CHECKED(rf69_set_dio_mapping(spi, DIO0, DIO_PayloadReady));
-	dev->irq_state[DIO0] = DIO_PayloadReady;
+	SET_CHECKED(rf69_set_dio_mapping(spi, DIO0, DIO_PAYLOAD_READY));
+	dev->irq_state[DIO0] = DIO_PAYLOAD_READY;
 	irq_set_irq_type(dev->irq_num[DIO0], IRQ_TYPE_EDGE_RISING);
 
 	/* fixed or unlimited length? */
@@ -590,13 +590,13 @@ pi433_tx_thread(void *data)
 		rf69_set_tx_cfg(device, &tx_cfg);
 
 		/* enable fifo level interrupt */
-		SET_CHECKED(rf69_set_dio_mapping(spi, DIO1, DIO_FifoLevel));
-		device->irq_state[DIO1] = DIO_FifoLevel;
+		SET_CHECKED(rf69_set_dio_mapping(spi, DIO1, DIO_FIFO_LEVEL));
+		device->irq_state[DIO1] = DIO_FIFO_LEVEL;
 		irq_set_irq_type(device->irq_num[DIO1], IRQ_TYPE_EDGE_FALLING);
 
 		/* enable packet sent interrupt */
-		SET_CHECKED(rf69_set_dio_mapping(spi, DIO0, DIO_PacketSent));
-		device->irq_state[DIO0] = DIO_PacketSent;
+		SET_CHECKED(rf69_set_dio_mapping(spi, DIO0, DIO_PACKET_SENT));
+		device->irq_state[DIO0] = DIO_PACKET_SENT;
 		irq_set_irq_type(device->irq_num[DIO0], IRQ_TYPE_EDGE_RISING);
 		enable_irq(device->irq_num[DIO0]); /* was disabled by rx active check */
 
diff --git a/drivers/staging/pi433/rf69_registers.h b/drivers/staging/pi433/rf69_registers.h
index 6335d42142fe..d23b8b668ef5 100644
--- a/drivers/staging/pi433/rf69_registers.h
+++ b/drivers/staging/pi433/rf69_registers.h
@@ -346,28 +346,28 @@
 #define  DIO5					5
 
 /* DIO Mapping values (packet mode) */
-#define  DIO_ModeReady_DIO4			0x00
-#define  DIO_ModeReady_DIO5			0x03
-#define  DIO_ClkOut				0x00
-#define  DIO_Data				0x01
-#define  DIO_TimeOut_DIO1			0x03
-#define  DIO_TimeOut_DIO4			0x00
-#define  DIO_Rssi_DIO0				0x03
-#define  DIO_Rssi_DIO3_4			0x01
-#define  DIO_RxReady				0x02
-#define  DIO_PLLLock				0x03
-#define  DIO_TxReady				0x01
-#define  DIO_FifoFull_DIO1			0x01
-#define  DIO_FifoFull_DIO3			0x00
-#define  DIO_SyncAddress			0x02
-#define  DIO_FifoNotEmpty_DIO1			0x02
-#define  DIO_FifoNotEmpty_FIO2			0x00
-#define  DIO_Automode				0x04
-#define  DIO_FifoLevel				0x00
-#define  DIO_CrcOk				0x00
-#define  DIO_PayloadReady			0x01
-#define  DIO_PacketSent				0x00
-#define  DIO_Dclk				0x00
+#define  DIO_MODE_READY_DIO4			0x00
+#define  DIO_MODE_READY_DIO5			0x03
+#define  DIO_CLK_OUT				0x00
+#define  DIO_DATA				0x01
+#define  DIO_TIMEOUT_DIO1			0x03
+#define  DIO_TIMEOUT_DIO4			0x00
+#define  DIO_RSSI_DIO0				0x03
+#define  DIO_RSSI_DIO3_4			0x01
+#define  DIO_RX_READY				0x02
+#define  DIO_PLL_LOCK				0x03
+#define  DIO_TX_READY				0x01
+#define  DIO_FIFO_FULL_DIO1			0x01
+#define  DIO_FIFO_FULL_DIO3			0x00
+#define  DIO_SYNC_ADDRESS			0x02
+#define  DIO_FIFO_NOT_EMPTY_DIO1		0x02
+#define  DIO_FIFO_NOT_EMPTY_FIO2		0x00
+#define  DIO_AUTOMODE				0x04
+#define  DIO_FIFO_LEVEL				0x00
+#define  DIO_CRC_OK				0x00
+#define  DIO_PAYLOAD_READY			0x01
+#define  DIO_PACKET_SENT			0x00
+#define  DIO_DCLK				0x00
 
 /* RegDioMapping2 CLK_OUT part */
 #define  MASK_DIOMAPPING2_CLK_OUT		0x07
-- 
2.11.0

[PATCH 3/6] staging: pi433: Rename variable in struct pi433_rx_cfg

[Simon Sandström]

Renames variable thresholdDecrement in struct pi433_rx_cfg to
threshold_decrement to get rid of checkpatch.pl warning
"Avoid CamelCase: <thresholdDecrement>".

Signed-off-by: Simon Sandström <simon@nikanor.nu>
---
 drivers/staging/pi433/pi433_if.c | 2 +-
 drivers/staging/pi433/pi433_if.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 840a7c7bf19a..b8efe6a74a2a 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -188,7 +188,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
 	SET_CHECKED(rf69_set_modulation	(dev->spi, rx_cfg->modulation));
 	SET_CHECKED(rf69_set_antenna_impedance	 (dev->spi, rx_cfg->antenna_impedance));
 	SET_CHECKED(rf69_set_rssi_threshold	 (dev->spi, rx_cfg->rssi_threshold));
-	SET_CHECKED(rf69_set_ook_threshold_dec	 (dev->spi, rx_cfg->thresholdDecrement));
+	SET_CHECKED(rf69_set_ook_threshold_dec	 (dev->spi, rx_cfg->threshold_decrement));
 	SET_CHECKED(rf69_set_bandwidth 		 (dev->spi, rx_cfg->bw_mantisse, rx_cfg->bw_exponent));
 	SET_CHECKED(rf69_set_bandwidth_during_afc(dev->spi, rx_cfg->bw_mantisse, rx_cfg->bw_exponent));
 	SET_CHECKED(rf69_set_dagc 		 (dev->spi, rx_cfg->dagc));
diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
index fc842c48c33e..6b31c1584b3a 100644
--- a/drivers/staging/pi433/pi433_if.h
+++ b/drivers/staging/pi433/pi433_if.h
@@ -115,7 +115,7 @@ struct pi433_rx_cfg {
 	enum modulation		modulation;
 
 	__u8			rssi_threshold;
-	enum thresholdDecrement	thresholdDecrement;
+	enum thresholdDecrement	threshold_decrement;
 	enum antennaImpedance	antenna_impedance;
 	enum lnaGain		lna_gain;
 	enum mantisse		bw_mantisse;	/* normal: 0x50 */
-- 
2.11.0

[PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Simon Sandström]

Renames the enum optionOnOff and its values optionOn, optionOff to enum
option_on_off and OPTION_ON, OPTION_OFF. Fixes checkpatch.pl warnings:
"Avoid CamelCase: <optionOnOff>, <optionOn>, <optionOff>".

Signed-off-by: Simon Sandström <simon@nikanor.nu>
---
 drivers/staging/pi433/pi433_if.c  | 34 ++++++++++++++---------------
 drivers/staging/pi433/pi433_if.h  | 16 +++++++-------
 drivers/staging/pi433/rf69.c      | 45 ++++++++++++++++++++++-----------------
 drivers/staging/pi433/rf69.h      | 15 ++++++++-----
 drivers/staging/pi433/rf69_enum.h |  6 +++---
 5 files changed, 63 insertions(+), 53 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index b8efe6a74a2a..4f6830f369e9 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -198,7 +198,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
 	/* packet config */
 	/* enable */
 	SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
-	if (rx_cfg->enable_sync == optionOn)
+	if (rx_cfg->enable_sync == OPTION_ON)
 	{
 		SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, afterSyncInterrupt));
 	}
@@ -206,7 +206,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
 	{
 		SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, always));
 	}
-	if (rx_cfg->enable_length_byte == optionOn) {
+	if (rx_cfg->enable_length_byte == OPTION_ON) {
 		ret = rf69_set_packet_format(dev->spi, packetLengthVar);
 		if (ret < 0)
 			return ret;
@@ -220,14 +220,14 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
 
 	/* lengths */
 	SET_CHECKED(rf69_set_sync_size(dev->spi, rx_cfg->sync_length));
-	if (rx_cfg->enable_length_byte == optionOn)
+	if (rx_cfg->enable_length_byte == OPTION_ON)
 	{
 		SET_CHECKED(rf69_set_payload_length(dev->spi, 0xff));
 	}
 	else if (rx_cfg->fixed_message_length != 0)
 	{
 		payload_length = rx_cfg->fixed_message_length;
-		if (rx_cfg->enable_length_byte  == optionOn) payload_length++;
+		if (rx_cfg->enable_length_byte  == OPTION_ON) payload_length++;
 		if (rx_cfg->enable_address_filtering != filteringOff) payload_length++;
 		SET_CHECKED(rf69_set_payload_length(dev->spi, payload_length));
 	}
@@ -237,7 +237,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
 	}
 
 	/* values */
-	if (rx_cfg->enable_sync == optionOn)
+	if (rx_cfg->enable_sync == OPTION_ON)
 	{
 		SET_CHECKED(rf69_set_sync_values(dev->spi, rx_cfg->sync_pattern));
 	}
@@ -264,7 +264,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct pi433_tx_cfg *tx_cfg)
 	SET_CHECKED(rf69_set_tx_start_condition(dev->spi, tx_cfg->tx_start_condition));
 
 	/* packet format enable */
-	if (tx_cfg->enable_preamble == optionOn)
+	if (tx_cfg->enable_preamble == OPTION_ON)
 	{
 		SET_CHECKED(rf69_set_preamble_length(dev->spi, tx_cfg->preamble_length));
 	}
@@ -273,7 +273,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct pi433_tx_cfg *tx_cfg)
 		SET_CHECKED(rf69_set_preamble_length(dev->spi, 0));
 	}
 	SET_CHECKED(rf69_set_sync_enable  (dev->spi, tx_cfg->enable_sync));
-	if (tx_cfg->enable_length_byte == optionOn) {
+	if (tx_cfg->enable_length_byte == OPTION_ON) {
 		ret = rf69_set_packet_format(dev->spi, packetLengthVar);
 		if (ret < 0)
 			return ret;
@@ -285,7 +285,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct pi433_tx_cfg *tx_cfg)
 	SET_CHECKED(rf69_set_crc_enable	  (dev->spi, tx_cfg->enable_crc));
 
 	/* configure sync, if enabled */
-	if (tx_cfg->enable_sync == optionOn) {
+	if (tx_cfg->enable_sync == OPTION_ON) {
 		SET_CHECKED(rf69_set_sync_size(dev->spi, tx_cfg->sync_length));
 		SET_CHECKED(rf69_set_sync_values(dev->spi, tx_cfg->sync_pattern));
 	}
@@ -400,7 +400,7 @@ pi433_receive(void *data)
 	}
 
 	/* length byte enabled? */
-	if (dev->rx_cfg.enable_length_byte == optionOn)
+	if (dev->rx_cfg.enable_length_byte == OPTION_ON)
 	{
 		retval = wait_event_interruptible(dev->fifo_wait_queue,
 						  dev->free_in_fifo < FIFO_SIZE);
@@ -525,11 +525,11 @@ pi433_tx_thread(void *data)
 			size = tx_cfg.fixed_message_length;
 
 		/* increase size, if len byte is requested */
-		if (tx_cfg.enable_length_byte == optionOn)
+		if (tx_cfg.enable_length_byte == OPTION_ON)
 			size++;
 
 		/* increase size, if adr byte is requested */
-		if (tx_cfg.enable_address_byte == optionOn)
+		if (tx_cfg.enable_address_byte == OPTION_ON)
 			size++;
 
 		/* prime buffer */
@@ -537,11 +537,11 @@ pi433_tx_thread(void *data)
 		position = 0;
 
 		/* add length byte, if requested */
-		if (tx_cfg.enable_length_byte  == optionOn)
+		if (tx_cfg.enable_length_byte  == OPTION_ON)
 			buffer[position++] = size-1; /* according to spec length byte itself must be excluded from the length calculation */
 
 		/* add adr byte, if requested */
-		if (tx_cfg.enable_address_byte == optionOn)
+		if (tx_cfg.enable_address_byte == OPTION_ON)
 			buffer[position++] = tx_cfg.address_byte;
 
 		/* finally get message data from fifo */
@@ -577,7 +577,7 @@ pi433_tx_thread(void *data)
 		/* clear fifo, set fifo threshold, set payload length */
 		SET_CHECKED(rf69_set_mode(spi, standby)); /* this clears the fifo */
 		SET_CHECKED(rf69_set_fifo_threshold(spi, FIFO_THRESHOLD));
-		if (tx_cfg.enable_length_byte == optionOn)
+		if (tx_cfg.enable_length_byte == OPTION_ON)
 		{
 			SET_CHECKED(rf69_set_payload_length(spi, size * tx_cfg.repetitions));
 		}
@@ -1091,9 +1091,9 @@ static int pi433_probe(struct spi_device *spi)
 	/* setup the radio module */
 	SET_CHECKED(rf69_set_mode		(spi, standby));
 	SET_CHECKED(rf69_set_data_mode		(spi, packet));
-	SET_CHECKED(rf69_set_amplifier_0	(spi, optionOn));
-	SET_CHECKED(rf69_set_amplifier_1	(spi, optionOff));
-	SET_CHECKED(rf69_set_amplifier_2	(spi, optionOff));
+	SET_CHECKED(rf69_set_amplifier_0	(spi, OPTION_ON));
+	SET_CHECKED(rf69_set_amplifier_1	(spi, OPTION_OFF));
+	SET_CHECKED(rf69_set_amplifier_2	(spi, OPTION_OFF));
 	SET_CHECKED(rf69_set_output_power_level	(spi, 13));
 	SET_CHECKED(rf69_set_antenna_impedance	(spi, fiftyOhm));
 
diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
index 6b31c1584b3a..34ff0d4807bd 100644
--- a/drivers/staging/pi433/pi433_if.h
+++ b/drivers/staging/pi433/pi433_if.h
@@ -73,11 +73,11 @@ struct pi433_tx_cfg {
 
 
 	/* packet format */
-	enum optionOnOff	enable_preamble;
-	enum optionOnOff	enable_sync;
-	enum optionOnOff	enable_length_byte;
-	enum optionOnOff	enable_address_byte;
-	enum optionOnOff	enable_crc;
+	enum option_on_off	enable_preamble;
+	enum option_on_off	enable_sync;
+	enum option_on_off	enable_length_byte;
+	enum option_on_off	enable_address_byte;
+	enum option_on_off	enable_crc;
 
 	__u16			preamble_length;
 	__u8			sync_length;
@@ -125,10 +125,10 @@ struct pi433_rx_cfg {
 
 
 	/* packet format */
-	enum optionOnOff	enable_sync;
-	enum optionOnOff	enable_length_byte;	  /* should be used in combination with sync, only */
+	enum option_on_off	enable_sync;
+	enum option_on_off	enable_length_byte;	  /* should be used in combination with sync, only */
 	enum addressFiltering	enable_address_filtering; /* operational with sync, only */
-	enum optionOnOff	enable_crc;		  /* only operational, if sync on and fixed length or length byte is used */
+	enum option_on_off	enable_crc;		  /* only operational, if sync on and fixed length or length byte is used */
 
 	__u8			sync_length;
 	__u8			fixed_message_length;
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index e69a2153c999..ebb3ddd1a957 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -272,45 +272,48 @@ int rf69_set_frequency(struct spi_device *spi, u32 frequency)
 	return 0;
 }
 
-int rf69_set_amplifier_0(struct spi_device *spi, enum optionOnOff optionOnOff)
+int rf69_set_amplifier_0(struct spi_device *spi,
+			 enum option_on_off option_on_off)
 {
 	#ifdef DEBUG
 		dev_dbg(&spi->dev, "set: amp #0");
 	#endif
 
-	switch (optionOnOff) {
-	case optionOn:	return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) |  MASK_PALEVEL_PA0));
-	case optionOff:	return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & ~MASK_PALEVEL_PA0));
+	switch (option_on_off) {
+	case OPTION_ON:  return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) |  MASK_PALEVEL_PA0));
+	case OPTION_OFF: return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & ~MASK_PALEVEL_PA0));
 	default:
 		dev_dbg(&spi->dev, "set: illegal input param");
 		return -EINVAL;
 	}
 }
 
-int rf69_set_amplifier_1(struct spi_device *spi, enum optionOnOff optionOnOff)
+int rf69_set_amplifier_1(struct spi_device *spi,
+			 enum option_on_off option_on_off)
 {
 	#ifdef DEBUG
 		dev_dbg(&spi->dev, "set: amp #1");
 	#endif
 
-	switch (optionOnOff) {
-	case optionOn:	return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) |  MASK_PALEVEL_PA1));
-	case optionOff: return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & ~MASK_PALEVEL_PA1));
+	switch (option_on_off) {
+	case OPTION_ON:  return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) |  MASK_PALEVEL_PA1));
+	case OPTION_OFF: return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & ~MASK_PALEVEL_PA1));
 	default:
 		dev_dbg(&spi->dev, "set: illegal input param");
 		return -EINVAL;
 	}
 }
 
-int rf69_set_amplifier_2(struct spi_device *spi, enum optionOnOff optionOnOff)
+int rf69_set_amplifier_2(struct spi_device *spi,
+			 enum option_on_off option_on_off)
 {
 	#ifdef DEBUG
 		dev_dbg(&spi->dev, "set: amp #2");
 	#endif
 
-	switch (optionOnOff) {
-	case optionOn:	return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) |  MASK_PALEVEL_PA2));
-	case optionOff:	return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & ~MASK_PALEVEL_PA2));
+	switch (option_on_off) {
+	case OPTION_ON:	 return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) |  MASK_PALEVEL_PA2));
+	case OPTION_OFF: return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & ~MASK_PALEVEL_PA2));
 	default:
 		dev_dbg(&spi->dev, "set: illegal input param");
 		return -EINVAL;
@@ -720,15 +723,16 @@ int rf69_set_preamble_length(struct spi_device *spi, u16 preambleLength)
 	return WRITE_REG(REG_PREAMBLE_LSB, lsb);
 }
 
-int rf69_set_sync_enable(struct spi_device *spi, enum optionOnOff optionOnOff)
+int rf69_set_sync_enable(struct spi_device *spi,
+			 enum option_on_off option_on_off)
 {
 	#ifdef DEBUG
 		dev_dbg(&spi->dev, "set: sync enable");
 	#endif
 
-	switch (optionOnOff) {
-	case optionOn:	return WRITE_REG(REG_SYNC_CONFIG, (READ_REG(REG_SYNC_CONFIG) |  MASK_SYNC_CONFIG_SYNC_ON));
-	case optionOff:	return WRITE_REG(REG_SYNC_CONFIG, (READ_REG(REG_SYNC_CONFIG) & ~MASK_SYNC_CONFIG_SYNC_ON));
+	switch (option_on_off) {
+	case OPTION_ON:  return WRITE_REG(REG_SYNC_CONFIG, (READ_REG(REG_SYNC_CONFIG) |  MASK_SYNC_CONFIG_SYNC_ON));
+	case OPTION_OFF: return WRITE_REG(REG_SYNC_CONFIG, (READ_REG(REG_SYNC_CONFIG) & ~MASK_SYNC_CONFIG_SYNC_ON));
 	default:
 		dev_dbg(&spi->dev, "set: illegal input param");
 		return -EINVAL;
@@ -817,15 +821,16 @@ int rf69_set_packet_format(struct spi_device *spi, enum packetFormat packetForma
 	}
 }
 
-int rf69_set_crc_enable(struct spi_device *spi, enum optionOnOff optionOnOff)
+int rf69_set_crc_enable(struct spi_device *spi,
+			enum option_on_off option_on_off)
 {
 	#ifdef DEBUG
 		dev_dbg(&spi->dev, "set: crc enable");
 	#endif
 
-	switch (optionOnOff) {
-	case optionOn:	return WRITE_REG(REG_PACKETCONFIG1, (READ_REG(REG_PACKETCONFIG1) |  MASK_PACKETCONFIG1_CRC_ON));
-	case optionOff:	return WRITE_REG(REG_PACKETCONFIG1, (READ_REG(REG_PACKETCONFIG1) & ~MASK_PACKETCONFIG1_CRC_ON));
+	switch (option_on_off) {
+	case OPTION_ON:  return WRITE_REG(REG_PACKETCONFIG1, (READ_REG(REG_PACKETCONFIG1) |  MASK_PACKETCONFIG1_CRC_ON));
+	case OPTION_OFF: return WRITE_REG(REG_PACKETCONFIG1, (READ_REG(REG_PACKETCONFIG1) & ~MASK_PACKETCONFIG1_CRC_ON));
 	default:
 		dev_dbg(&spi->dev, "set: illegal input param");
 		return -EINVAL;
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index 5c0c95628f2f..12c2e106785e 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -33,9 +33,12 @@ int rf69_set_modulation_shaping(struct spi_device *spi, enum modShaping modShapi
 int rf69_set_bit_rate(struct spi_device *spi, u16 bitRate);
 int rf69_set_deviation(struct spi_device *spi, u32 deviation);
 int rf69_set_frequency(struct spi_device *spi, u32 frequency);
-int rf69_set_amplifier_0(struct spi_device *spi, enum optionOnOff optionOnOff);
-int rf69_set_amplifier_1(struct spi_device *spi, enum optionOnOff optionOnOff);
-int rf69_set_amplifier_2(struct spi_device *spi, enum optionOnOff optionOnOff);
+int rf69_set_amplifier_0(struct spi_device *spi,
+			 enum option_on_off option_on_off);
+int rf69_set_amplifier_1(struct spi_device *spi,
+			 enum option_on_off option_on_off);
+int rf69_set_amplifier_2(struct spi_device *spi,
+			 enum option_on_off option_on_off);
 int rf69_set_output_power_level(struct spi_device *spi, u8 powerLevel);
 int rf69_set_pa_ramp(struct spi_device *spi, enum paRamp paRamp);
 int rf69_set_antenna_impedance(struct spi_device *spi, enum antennaImpedance antennaImpedance);
@@ -56,13 +59,15 @@ int rf69_set_rssi_threshold(struct spi_device *spi, u8 threshold);
 int rf69_set_rx_start_timeout(struct spi_device *spi, u8 timeout);
 int rf69_set_rssi_timeout(struct spi_device *spi, u8 timeout);
 int rf69_set_preamble_length(struct spi_device *spi, u16 preambleLength);
-int rf69_set_sync_enable(struct spi_device *spi, enum optionOnOff optionOnOff);
+int rf69_set_sync_enable(struct spi_device *spi,
+			 enum option_on_off option_on_off);
 int rf69_set_fifo_fill_condition(struct spi_device *spi, enum fifoFillCondition fifoFillCondition);
 int rf69_set_sync_size(struct spi_device *spi, u8 sync_size);
 int rf69_set_sync_tolerance(struct spi_device *spi, u8 syncTolerance);
 int rf69_set_sync_values(struct spi_device *spi, u8 syncValues[8]);
 int rf69_set_packet_format(struct spi_device *spi, enum packetFormat packetFormat);
-int rf69_set_crc_enable(struct spi_device *spi, enum optionOnOff optionOnOff);
+int rf69_set_crc_enable(struct spi_device *spi,
+			enum option_on_off option_on_off);
 int rf69_set_adressFiltering(struct spi_device *spi, enum addressFiltering addressFiltering);
 int rf69_set_payload_length(struct spi_device *spi, u8 payloadLength);
 u8  rf69_get_payload_length(struct spi_device *spi);
diff --git a/drivers/staging/pi433/rf69_enum.h b/drivers/staging/pi433/rf69_enum.h
index babe597e2ec6..5247e9269de9 100644
--- a/drivers/staging/pi433/rf69_enum.h
+++ b/drivers/staging/pi433/rf69_enum.h
@@ -18,9 +18,9 @@
 #ifndef RF69_ENUM_H
 #define RF69_ENUM_H
 
-enum optionOnOff {
-	optionOff,
-	optionOn
+enum option_on_off {
+	OPTION_OFF,
+	OPTION_ON
 };
 
 enum mode {
-- 
2.11.0

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 03.12.2017 um 17:17 schrieb Simon Sandström:
> Renames the enum optionOnOff and its values optionOn, optionOff to enum
> option_on_off and OPTION_ON, OPTION_OFF. Fixes checkpatch.pl warnings:
> "Avoid CamelCase: <optionOnOff>, <optionOn>, <optionOff>".
> 
> Signed-off-by: Simon Sandström <simon@nikanor.nu>
> ---
>   drivers/staging/pi433/pi433_if.c  | 34 ++++++++++++++---------------
>   drivers/staging/pi433/pi433_if.h  | 16 +++++++-------
>   drivers/staging/pi433/rf69.c      | 45 ++++++++++++++++++++++-----------------
>   drivers/staging/pi433/rf69.h      | 15 ++++++++-----
>   drivers/staging/pi433/rf69_enum.h |  6 +++---
>   5 files changed, 63 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index b8efe6a74a2a..4f6830f369e9 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -198,7 +198,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
>   	/* packet config */
>   	/* enable */
>   	SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
> -	if (rx_cfg->enable_sync == optionOn)
> +	if (rx_cfg->enable_sync == OPTION_ON)
>   	{
>   		SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, afterSyncInterrupt));
>   	}
> @@ -206,7 +206,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
>   	{
>   		SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, always));
>   	}
> -	if (rx_cfg->enable_length_byte == optionOn) {
> +	if (rx_cfg->enable_length_byte == OPTION_ON) {
>   		ret = rf69_set_packet_format(dev->spi, packetLengthVar);
>   		if (ret < 0)
>   			return ret;
> @@ -220,14 +220,14 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
>   
>   	/* lengths */
>   	SET_CHECKED(rf69_set_sync_size(dev->spi, rx_cfg->sync_length));
> -	if (rx_cfg->enable_length_byte == optionOn)
> +	if (rx_cfg->enable_length_byte == OPTION_ON)
>   	{
>   		SET_CHECKED(rf69_set_payload_length(dev->spi, 0xff));
>   	}
>   	else if (rx_cfg->fixed_message_length != 0)
>   	{
>   		payload_length = rx_cfg->fixed_message_length;
> -		if (rx_cfg->enable_length_byte  == optionOn) payload_length++;
> +		if (rx_cfg->enable_length_byte  == OPTION_ON) payload_length++;
>   		if (rx_cfg->enable_address_filtering != filteringOff) payload_length++;
>   		SET_CHECKED(rf69_set_payload_length(dev->spi, payload_length));
>   	}
> @@ -237,7 +237,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
>   	}
>   
>   	/* values */
> -	if (rx_cfg->enable_sync == optionOn)
> +	if (rx_cfg->enable_sync == OPTION_ON)
>   	{
>   		SET_CHECKED(rf69_set_sync_values(dev->spi, rx_cfg->sync_pattern));
>   	}
> @@ -264,7 +264,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct pi433_tx_cfg *tx_cfg)
>   	SET_CHECKED(rf69_set_tx_start_condition(dev->spi, tx_cfg->tx_start_condition));
>   
>   	/* packet format enable */
> -	if (tx_cfg->enable_preamble == optionOn)
> +	if (tx_cfg->enable_preamble == OPTION_ON)
>   	{
>   		SET_CHECKED(rf69_set_preamble_length(dev->spi, tx_cfg->preamble_length));
>   	}
> @@ -273,7 +273,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct pi433_tx_cfg *tx_cfg)
>   		SET_CHECKED(rf69_set_preamble_length(dev->spi, 0));
>   	}
>   	SET_CHECKED(rf69_set_sync_enable  (dev->spi, tx_cfg->enable_sync));
> -	if (tx_cfg->enable_length_byte == optionOn) {
> +	if (tx_cfg->enable_length_byte == OPTION_ON) {
>   		ret = rf69_set_packet_format(dev->spi, packetLengthVar);
>   		if (ret < 0)
>   			return ret;
> @@ -285,7 +285,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct pi433_tx_cfg *tx_cfg)
>   	SET_CHECKED(rf69_set_crc_enable	  (dev->spi, tx_cfg->enable_crc));
>   
>   	/* configure sync, if enabled */
> -	if (tx_cfg->enable_sync == optionOn) {
> +	if (tx_cfg->enable_sync == OPTION_ON) {
>   		SET_CHECKED(rf69_set_sync_size(dev->spi, tx_cfg->sync_length));
>   		SET_CHECKED(rf69_set_sync_values(dev->spi, tx_cfg->sync_pattern));
>   	}
> @@ -400,7 +400,7 @@ pi433_receive(void *data)
>   	}
>   
>   	/* length byte enabled? */
> -	if (dev->rx_cfg.enable_length_byte == optionOn)
> +	if (dev->rx_cfg.enable_length_byte == OPTION_ON)
>   	{
>   		retval = wait_event_interruptible(dev->fifo_wait_queue,
>   						  dev->free_in_fifo < FIFO_SIZE);
> @@ -525,11 +525,11 @@ pi433_tx_thread(void *data)
>   			size = tx_cfg.fixed_message_length;
>   
>   		/* increase size, if len byte is requested */
> -		if (tx_cfg.enable_length_byte == optionOn)
> +		if (tx_cfg.enable_length_byte == OPTION_ON)
>   			size++;
>   
>   		/* increase size, if adr byte is requested */
> -		if (tx_cfg.enable_address_byte == optionOn)
> +		if (tx_cfg.enable_address_byte == OPTION_ON)
>   			size++;
>   
>   		/* prime buffer */
> @@ -537,11 +537,11 @@ pi433_tx_thread(void *data)
>   		position = 0;
>   
>   		/* add length byte, if requested */
> -		if (tx_cfg.enable_length_byte  == optionOn)
> +		if (tx_cfg.enable_length_byte  == OPTION_ON)
>   			buffer[position++] = size-1; /* according to spec length byte itself must be excluded from the length calculation */
>   
>   		/* add adr byte, if requested */
> -		if (tx_cfg.enable_address_byte == optionOn)
> +		if (tx_cfg.enable_address_byte == OPTION_ON)
>   			buffer[position++] = tx_cfg.address_byte;
>   
>   		/* finally get message data from fifo */
> @@ -577,7 +577,7 @@ pi433_tx_thread(void *data)
>   		/* clear fifo, set fifo threshold, set payload length */
>   		SET_CHECKED(rf69_set_mode(spi, standby)); /* this clears the fifo */
>   		SET_CHECKED(rf69_set_fifo_threshold(spi, FIFO_THRESHOLD));
> -		if (tx_cfg.enable_length_byte == optionOn)
> +		if (tx_cfg.enable_length_byte == OPTION_ON)
>   		{
>   			SET_CHECKED(rf69_set_payload_length(spi, size * tx_cfg.repetitions));
>   		}
> @@ -1091,9 +1091,9 @@ static int pi433_probe(struct spi_device *spi)
>   	/* setup the radio module */
>   	SET_CHECKED(rf69_set_mode		(spi, standby));
>   	SET_CHECKED(rf69_set_data_mode		(spi, packet));
> -	SET_CHECKED(rf69_set_amplifier_0	(spi, optionOn));
> -	SET_CHECKED(rf69_set_amplifier_1	(spi, optionOff));
> -	SET_CHECKED(rf69_set_amplifier_2	(spi, optionOff));
> +	SET_CHECKED(rf69_set_amplifier_0	(spi, OPTION_ON));
> +	SET_CHECKED(rf69_set_amplifier_1	(spi, OPTION_OFF));
> +	SET_CHECKED(rf69_set_amplifier_2	(spi, OPTION_OFF));
>   	SET_CHECKED(rf69_set_output_power_level	(spi, 13));
>   	SET_CHECKED(rf69_set_antenna_impedance	(spi, fiftyOhm));
>   
> diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
> index 6b31c1584b3a..34ff0d4807bd 100644
> --- a/drivers/staging/pi433/pi433_if.h
> +++ b/drivers/staging/pi433/pi433_if.h
> @@ -73,11 +73,11 @@ struct pi433_tx_cfg {
>   
>   
>   	/* packet format */
> -	enum optionOnOff	enable_preamble;
> -	enum optionOnOff	enable_sync;
> -	enum optionOnOff	enable_length_byte;
> -	enum optionOnOff	enable_address_byte;
> -	enum optionOnOff	enable_crc;
> +	enum option_on_off	enable_preamble;
> +	enum option_on_off	enable_sync;
> +	enum option_on_off	enable_length_byte;
> +	enum option_on_off	enable_address_byte;
> +	enum option_on_off	enable_crc;
>   
>   	__u16			preamble_length;
>   	__u8			sync_length;
> @@ -125,10 +125,10 @@ struct pi433_rx_cfg {
>   
>   
>   	/* packet format */
> -	enum optionOnOff	enable_sync;
> -	enum optionOnOff	enable_length_byte;	  /* should be used in combination with sync, only */
> +	enum option_on_off	enable_sync;
> +	enum option_on_off	enable_length_byte;	  /* should be used in combination with sync, only */
>   	enum addressFiltering	enable_address_filtering; /* operational with sync, only */
> -	enum optionOnOff	enable_crc;		  /* only operational, if sync on and fixed length or length byte is used */
> +	enum option_on_off	enable_crc;		  /* only operational, if sync on and fixed length or length byte is used */
>   
>   	__u8			sync_length;
>   	__u8			fixed_message_length;
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index e69a2153c999..ebb3ddd1a957 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -272,45 +272,48 @@ int rf69_set_frequency(struct spi_device *spi, u32 frequency)
>   	return 0;
>   }
>   
> -int rf69_set_amplifier_0(struct spi_device *spi, enum optionOnOff optionOnOff)
> +int rf69_set_amplifier_0(struct spi_device *spi,
> +			 enum option_on_off option_on_off)
>   {
>   	#ifdef DEBUG
>   		dev_dbg(&spi->dev, "set: amp #0");
>   	#endif
>   
> -	switch (optionOnOff) {
> -	case optionOn:	return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) |  MASK_PALEVEL_PA0));
> -	case optionOff:	return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & ~MASK_PALEVEL_PA0));
> +	switch (option_on_off) {
> +	case OPTION_ON:  return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) |  MASK_PALEVEL_PA0));
> +	case OPTION_OFF: return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & ~MASK_PALEVEL_PA0));
>   	default:
>   		dev_dbg(&spi->dev, "set: illegal input param");
>   		return -EINVAL;
>   	}
>   }
>   
> -int rf69_set_amplifier_1(struct spi_device *spi, enum optionOnOff optionOnOff)
> +int rf69_set_amplifier_1(struct spi_device *spi,
> +			 enum option_on_off option_on_off)
>   {
>   	#ifdef DEBUG
>   		dev_dbg(&spi->dev, "set: amp #1");
>   	#endif
>   
> -	switch (optionOnOff) {
> -	case optionOn:	return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) |  MASK_PALEVEL_PA1));
> -	case optionOff: return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & ~MASK_PALEVEL_PA1));
> +	switch (option_on_off) {
> +	case OPTION_ON:  return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) |  MASK_PALEVEL_PA1));
> +	case OPTION_OFF: return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & ~MASK_PALEVEL_PA1));
>   	default:
>   		dev_dbg(&spi->dev, "set: illegal input param");
>   		return -EINVAL;
>   	}
>   }
>   
> -int rf69_set_amplifier_2(struct spi_device *spi, enum optionOnOff optionOnOff)
> +int rf69_set_amplifier_2(struct spi_device *spi,
> +			 enum option_on_off option_on_off)
>   {
>   	#ifdef DEBUG
>   		dev_dbg(&spi->dev, "set: amp #2");
>   	#endif
>   
> -	switch (optionOnOff) {
> -	case optionOn:	return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) |  MASK_PALEVEL_PA2));
> -	case optionOff:	return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & ~MASK_PALEVEL_PA2));
> +	switch (option_on_off) {
> +	case OPTION_ON:	 return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) |  MASK_PALEVEL_PA2));
> +	case OPTION_OFF: return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & ~MASK_PALEVEL_PA2));
>   	default:
>   		dev_dbg(&spi->dev, "set: illegal input param");
>   		return -EINVAL;
> @@ -720,15 +723,16 @@ int rf69_set_preamble_length(struct spi_device *spi, u16 preambleLength)
>   	return WRITE_REG(REG_PREAMBLE_LSB, lsb);
>   }
>   
> -int rf69_set_sync_enable(struct spi_device *spi, enum optionOnOff optionOnOff)
> +int rf69_set_sync_enable(struct spi_device *spi,
> +			 enum option_on_off option_on_off)
>   {
>   	#ifdef DEBUG
>   		dev_dbg(&spi->dev, "set: sync enable");
>   	#endif
>   
> -	switch (optionOnOff) {
> -	case optionOn:	return WRITE_REG(REG_SYNC_CONFIG, (READ_REG(REG_SYNC_CONFIG) |  MASK_SYNC_CONFIG_SYNC_ON));
> -	case optionOff:	return WRITE_REG(REG_SYNC_CONFIG, (READ_REG(REG_SYNC_CONFIG) & ~MASK_SYNC_CONFIG_SYNC_ON));
> +	switch (option_on_off) {
> +	case OPTION_ON:  return WRITE_REG(REG_SYNC_CONFIG, (READ_REG(REG_SYNC_CONFIG) |  MASK_SYNC_CONFIG_SYNC_ON));
> +	case OPTION_OFF: return WRITE_REG(REG_SYNC_CONFIG, (READ_REG(REG_SYNC_CONFIG) & ~MASK_SYNC_CONFIG_SYNC_ON));
>   	default:
>   		dev_dbg(&spi->dev, "set: illegal input param");
>   		return -EINVAL;
> @@ -817,15 +821,16 @@ int rf69_set_packet_format(struct spi_device *spi, enum packetFormat packetForma
>   	}
>   }
>   
> -int rf69_set_crc_enable(struct spi_device *spi, enum optionOnOff optionOnOff)
> +int rf69_set_crc_enable(struct spi_device *spi,
> +			enum option_on_off option_on_off)
>   {
>   	#ifdef DEBUG
>   		dev_dbg(&spi->dev, "set: crc enable");
>   	#endif
>   
> -	switch (optionOnOff) {
> -	case optionOn:	return WRITE_REG(REG_PACKETCONFIG1, (READ_REG(REG_PACKETCONFIG1) |  MASK_PACKETCONFIG1_CRC_ON));
> -	case optionOff:	return WRITE_REG(REG_PACKETCONFIG1, (READ_REG(REG_PACKETCONFIG1) & ~MASK_PACKETCONFIG1_CRC_ON));
> +	switch (option_on_off) {
> +	case OPTION_ON:  return WRITE_REG(REG_PACKETCONFIG1, (READ_REG(REG_PACKETCONFIG1) |  MASK_PACKETCONFIG1_CRC_ON));
> +	case OPTION_OFF: return WRITE_REG(REG_PACKETCONFIG1, (READ_REG(REG_PACKETCONFIG1) & ~MASK_PACKETCONFIG1_CRC_ON));
>   	default:
>   		dev_dbg(&spi->dev, "set: illegal input param");
>   		return -EINVAL;
> diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
> index 5c0c95628f2f..12c2e106785e 100644
> --- a/drivers/staging/pi433/rf69.h
> +++ b/drivers/staging/pi433/rf69.h
> @@ -33,9 +33,12 @@ int rf69_set_modulation_shaping(struct spi_device *spi, enum modShaping modShapi
>   int rf69_set_bit_rate(struct spi_device *spi, u16 bitRate);
>   int rf69_set_deviation(struct spi_device *spi, u32 deviation);
>   int rf69_set_frequency(struct spi_device *spi, u32 frequency);
> -int rf69_set_amplifier_0(struct spi_device *spi, enum optionOnOff optionOnOff);
> -int rf69_set_amplifier_1(struct spi_device *spi, enum optionOnOff optionOnOff);
> -int rf69_set_amplifier_2(struct spi_device *spi, enum optionOnOff optionOnOff);
> +int rf69_set_amplifier_0(struct spi_device *spi,
> +			 enum option_on_off option_on_off);
> +int rf69_set_amplifier_1(struct spi_device *spi,
> +			 enum option_on_off option_on_off);
> +int rf69_set_amplifier_2(struct spi_device *spi,
> +			 enum option_on_off option_on_off);
>   int rf69_set_output_power_level(struct spi_device *spi, u8 powerLevel);
>   int rf69_set_pa_ramp(struct spi_device *spi, enum paRamp paRamp);
>   int rf69_set_antenna_impedance(struct spi_device *spi, enum antennaImpedance antennaImpedance);
> @@ -56,13 +59,15 @@ int rf69_set_rssi_threshold(struct spi_device *spi, u8 threshold);
>   int rf69_set_rx_start_timeout(struct spi_device *spi, u8 timeout);
>   int rf69_set_rssi_timeout(struct spi_device *spi, u8 timeout);
>   int rf69_set_preamble_length(struct spi_device *spi, u16 preambleLength);
> -int rf69_set_sync_enable(struct spi_device *spi, enum optionOnOff optionOnOff);
> +int rf69_set_sync_enable(struct spi_device *spi,
> +			 enum option_on_off option_on_off);
>   int rf69_set_fifo_fill_condition(struct spi_device *spi, enum fifoFillCondition fifoFillCondition);
>   int rf69_set_sync_size(struct spi_device *spi, u8 sync_size);
>   int rf69_set_sync_tolerance(struct spi_device *spi, u8 syncTolerance);
>   int rf69_set_sync_values(struct spi_device *spi, u8 syncValues[8]);
>   int rf69_set_packet_format(struct spi_device *spi, enum packetFormat packetFormat);
> -int rf69_set_crc_enable(struct spi_device *spi, enum optionOnOff optionOnOff);
> +int rf69_set_crc_enable(struct spi_device *spi,
> +			enum option_on_off option_on_off);
>   int rf69_set_adressFiltering(struct spi_device *spi, enum addressFiltering addressFiltering);
>   int rf69_set_payload_length(struct spi_device *spi, u8 payloadLength);
>   u8  rf69_get_payload_length(struct spi_device *spi);
> diff --git a/drivers/staging/pi433/rf69_enum.h b/drivers/staging/pi433/rf69_enum.h
> index babe597e2ec6..5247e9269de9 100644
> --- a/drivers/staging/pi433/rf69_enum.h
> +++ b/drivers/staging/pi433/rf69_enum.h
> @@ -18,9 +18,9 @@
>   #ifndef RF69_ENUM_H
>   #define RF69_ENUM_H
>   
> -enum optionOnOff {
> -	optionOff,
> -	optionOn
> +enum option_on_off {
> +	OPTION_OFF,
> +	OPTION_ON
>   };
>   
>   enum mode {
> 

Hi Simon,

thanks for your effort.

I have two questions:
* According to my practical experiance, enums were always written in 
lower case. Does kernel style guide ask for upper case for enums?
For me upper case indicates, that this value will be processed by 
preprocessor, not by compiler. So I used it for define constants and 
macros so far...
* The enums are the interface to user space. Isn't it necessary to 
increse the interface version number on every change of the enums?
Or can we stay with old version number, since order of the enums is 
untouched?

Thanks,

Marcus

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Simon Sandström]

On Sun, Dec 03, 2017 at 06:49:40PM +0200, Marcus Wolf wrote:
> 
> Hi Simon,
Hi,

> 
> thanks for your effort.
> 
> I have two questions:
> * According to my practical experiance, enums were always written in lower
> case. Does kernel style guide ask for upper case for enums?
Yes. From Documentation/process/coding-style.rst: "Names of macros
defining constants and labels in enums are capitalized". 

> For me upper case indicates, that this value will be processed by
> preprocessor, not by compiler. So I used it for define constants and macros
> so far...
For me a upper case identifier indicates something that has a constant
value. Therefore I think it makes sense that enum labels are
capitalized.

> * The enums are the interface to user space. Isn't it necessary to increse
> the interface version number on every change of the enums?
> Or can we stay with old version number, since order of the enums is
> untouched?
Good question. I read the note about having to change ioctl number if
the contents of the struct ever change, but is this also true if only
the name of a variable change? I mean, the actual content is still the
same.

I will investigate this more and get back to you.

> 
> Thanks,
> 
> Marcus

Regards,
Simon

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Dan Carpenter]

On Sun, Dec 03, 2017 at 04:17:24PM +0100, Simon Sandström wrote:
> Renames the enum optionOnOff and its values optionOn, optionOff to enum
> option_on_off and OPTION_ON, OPTION_OFF. Fixes checkpatch.pl warnings:
> "Avoid CamelCase: <optionOnOff>, <optionOn>, <optionOff>".
> 
> Signed-off-by: Simon Sandström <simon@nikanor.nu>
> ---
>  drivers/staging/pi433/pi433_if.c  | 34 ++++++++++++++---------------
>  drivers/staging/pi433/pi433_if.h  | 16 +++++++-------
>  drivers/staging/pi433/rf69.c      | 45 ++++++++++++++++++++++-----------------
>  drivers/staging/pi433/rf69.h      | 15 ++++++++-----
>  drivers/staging/pi433/rf69_enum.h |  6 +++---
>  5 files changed, 63 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index b8efe6a74a2a..4f6830f369e9 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -198,7 +198,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
>  	/* packet config */
>  	/* enable */
>  	SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
> -	if (rx_cfg->enable_sync == optionOn)
> +	if (rx_cfg->enable_sync == OPTION_ON)

I haven't told anyone yet, but I wish someone would just get rid of
optionOn entirely.  This should just be:

	if (rx_cfg->enable_sync) {


> diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
> index 6b31c1584b3a..34ff0d4807bd 100644
> --- a/drivers/staging/pi433/pi433_if.h
> +++ b/drivers/staging/pi433/pi433_if.h
> @@ -73,11 +73,11 @@ struct pi433_tx_cfg {
>  
>  
>  	/* packet format */
> -	enum optionOnOff	enable_preamble;
> -	enum optionOnOff	enable_sync;
> -	enum optionOnOff	enable_length_byte;
> -	enum optionOnOff	enable_address_byte;
> -	enum optionOnOff	enable_crc;
> +	enum option_on_off	enable_preamble;
> +	enum option_on_off	enable_sync;
> +	enum option_on_off	enable_length_byte;
> +	enum option_on_off	enable_address_byte;
> +	enum option_on_off	enable_crc;

These should be bool.

> -int rf69_set_amplifier_0(struct spi_device *spi, enum optionOnOff optionOnOff)
> +int rf69_set_amplifier_0(struct spi_device *spi,
> +			 enum option_on_off option_on_off)


This should be two functions:

int rf69_enable_amplifier_0(struct spi_device *spi)
{
	return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) |  MASK_PALEVEL_PA0));
}

int rf69_disable_amplifier_0(struct spi_device *spi)
{
	return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & ~MASK_PALEVEL_PA0));
}

Perhaps choose different function names if you want?  You could do it
as several patches:

patch 1: change types to bool
patch 2: sed -e '/ == optionOn//'
patch 3: split the functions into two functions
patch 4: delete optionOnOff enum

patches 1 and 2 could be merged together (your choice).

regards,
dan carpenter

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Dan Carpenter]

On Mon, Dec 04, 2017 at 01:17:37PM +0300, Dan Carpenter wrote:
> Perhaps choose different function names if you want?  You could do it
> as several patches:
> 
> patch 1: change types to bool
> patch 2: sed -e '/ == optionOn//'
> patch 3: split the functions into two functions
> patch 4: delete optionOnOff enum
> 
> patches 1 and 2 could be merged together (your choice).
> 

Markus says that optionOn is used by user space so my you won't be able
to remove these entirely.  But as much as possible we should internally.

regards,
dan carpenter

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 12:37 schrieb Dan Carpenter:
> On Mon, Dec 04, 2017 at 01:17:37PM +0300, Dan Carpenter wrote:
>> Perhaps choose different function names if you want?  You could do it
>> as several patches:
>>
>> patch 1: change types to bool
>> patch 2: sed -e '/ == optionOn//'
>> patch 3: split the functions into two functions
>> patch 4: delete optionOnOff enum
>>
>> patches 1 and 2 could be merged together (your choice).
>>
> 
> Markus says that optionOn is used by user space so my you won't be able
> to remove these entirely.  But as much as possible we should internally.
> 
> regards,
> dan carpenter
> 

Hi Dan, hi Simon,

I think, it's a pretty nice idea to remove th optionOnOff and replace it 
by bool.

<history>
In former times, the variables in the config struct had very different 
names - not containing "enable". Therefore optionOnOff was used to make 
absolutely clear (in user space), wheter something was switched on, or off.
Now the variable have nice names, so bool is fine, even better now :-)
</history>

I would suggest not to split the amp-functions but to rename them, to 
also contain an enable:
rf69_set_amp_X_enable()

To avoid misunderstandings maybe it is better to remove the enable from 
enable_address_filtering, since here we can't go with bool.

Thanks a lot for the ideas and improvements :-)

Marcus

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Dan Carpenter]

On Mon, Dec 04, 2017 at 08:37:51PM +0200, Marcus Wolf wrote:
> 
> 
> Am 04.12.2017 um 12:37 schrieb Dan Carpenter:
> > On Mon, Dec 04, 2017 at 01:17:37PM +0300, Dan Carpenter wrote:
> > > Perhaps choose different function names if you want?  You could do it
> > > as several patches:
> > > 
> > > patch 1: change types to bool
> > > patch 2: sed -e '/ == optionOn//'
> > > patch 3: split the functions into two functions
> > > patch 4: delete optionOnOff enum
> > > 
> > > patches 1 and 2 could be merged together (your choice).
> > > 
> > 
> > Markus says that optionOn is used by user space so my you won't be able
> > to remove these entirely.  But as much as possible we should internally.
> > 
> > regards,
> > dan carpenter
> > 
> 
> Hi Dan, hi Simon,
> 
> I think, it's a pretty nice idea to remove th optionOnOff and replace it by
> bool.
> 
> <history>
> In former times, the variables in the config struct had very different names
> - not containing "enable". Therefore optionOnOff was used to make absolutely
> clear (in user space), wheter something was switched on, or off.
> Now the variable have nice names, so bool is fine, even better now :-)
> </history>
> 
> I would suggest not to split the amp-functions but to rename them, to also
> contain an enable:
> rf69_set_amp_X_enable()

That's a bad name, because it doesn't just enable it also disables.
Please split them.

regards,
dan carpenter

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 21:15 schrieb Dan Carpenter:
> On Mon, Dec 04, 2017 at 08:37:51PM +0200, Marcus Wolf wrote:
>>
>>
>> Am 04.12.2017 um 12:37 schrieb Dan Carpenter:
>>> On Mon, Dec 04, 2017 at 01:17:37PM +0300, Dan Carpenter wrote:
>>>> Perhaps choose different function names if you want?  You could do it
>>>> as several patches:
>>>>
>>>> patch 1: change types to bool
>>>> patch 2: sed -e '/ == optionOn//'
>>>> patch 3: split the functions into two functions
>>>> patch 4: delete optionOnOff enum
>>>>
>>>> patches 1 and 2 could be merged together (your choice).
>>>>
>>>
>>> Markus says that optionOn is used by user space so my you won't be able
>>> to remove these entirely.  But as much as possible we should internally.
>>>
>>> regards,
>>> dan carpenter
>>>
>>
>> Hi Dan, hi Simon,
>>
>> I think, it's a pretty nice idea to remove th optionOnOff and replace it by
>> bool.
>>
>> <history>
>> In former times, the variables in the config struct had very different names
>> - not containing "enable". Therefore optionOnOff was used to make absolutely
>> clear (in user space), wheter something was switched on, or off.
>> Now the variable have nice names, so bool is fine, even better now :-)
>> </history>
>>
>> I would suggest not to split the amp-functions but to rename them, to also
>> contain an enable:
>> rf69_set_amp_X_enable()
> 
> That's a bad name, because it doesn't just enable it also disables.
> Please split them.
> 
> regards,
> dan carpenter
> 
> 

Same applies to all other stuff, that's using optionOnOff:
rf69_set_sync_enable(optionOn/Off) enables and disbales sync,
rf69_set_crc_enable(optionOn/Off) enables and disables crc,
...

In my opinion, if we want perfect clarity, we should stay with optionOnOff.
If we are ok, if rf69_set_sync_enable(false) disables sync,
in my opinion, we also have to be ok, if rf69_set_amp_X_enable(false) 
disables the amp.

Cheers,
Marcus

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Simon Sandström]

On Mon, Dec 04, 2017 at 09:22:06PM +0200, Marcus Wolf wrote:
> 
> 
> Am 04.12.2017 um 21:15 schrieb Dan Carpenter:
> > 
> > That's a bad name, because it doesn't just enable it also disables.
> > Please split them.
> > 
> > regards,
> > dan carpenter
> > 
> > 
> 
> Same applies to all other stuff, that's using optionOnOff:
> rf69_set_sync_enable(optionOn/Off) enables and disbales sync,
> rf69_set_crc_enable(optionOn/Off) enables and disables crc,
> ...
> 
> In my opinion, if we want perfect clarity, we should stay with optionOnOff.
> If we are ok, if rf69_set_sync_enable(false) disables sync,
> in my opinion, we also have to be ok, if rf69_set_amp_X_enable(false)
> disables the amp.
> 
> Cheers,
> Marcus

Hi,

I agree with Dan. rf69_enable_sync() / rf69_disable_sync() is clear. If
there are more functions like this (e.g. for crc) then we'll just split
those functions as well.

If you really want one single function for enabling/disabling then I
think that you need to find a better name. Something like
rf69_set_sync_operation(bool), rf69_set_crc_operation(bool), etc.


Regards,
Simon

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 21:42 schrieb Simon Sandström:
> On Mon, Dec 04, 2017 at 09:22:06PM +0200, Marcus Wolf wrote:
>>
>>
>> Am 04.12.2017 um 21:15 schrieb Dan Carpenter:
>>>
>>> That's a bad name, because it doesn't just enable it also disables.
>>> Please split them.
>>>
>>> regards,
>>> dan carpenter
>>>
>>>
>>
>> Same applies to all other stuff, that's using optionOnOff:
>> rf69_set_sync_enable(optionOn/Off) enables and disbales sync,
>> rf69_set_crc_enable(optionOn/Off) enables and disables crc,
>> ...
>>
>> In my opinion, if we want perfect clarity, we should stay with optionOnOff.
>> If we are ok, if rf69_set_sync_enable(false) disables sync,
>> in my opinion, we also have to be ok, if rf69_set_amp_X_enable(false)
>> disables the amp.
>>
>> Cheers,
>> Marcus
> 
> Hi,
> 
> I agree with Dan. rf69_enable_sync() / rf69_disable_sync() is clear. If
> there are more functions like this (e.g. for crc) then we'll just split
> those functions as well.
> 
> If you really want one single function for enabling/disabling then I
> think that you need to find a better name. Something like
> rf69_set_sync_operation(bool), rf69_set_crc_operation(bool), etc.
> 
> 
> Regards,
> Simon
> 

Hi Simon, hi Dan,

if you both are of the same opinion, for me, it's fine, if we go with 
two functions.

But I don't get the advantage, if we split approx. 10 functions, to get 
rid of enum optionOnOff.

Keep in mind, that if you split the functions, in the interface 
implementation you also need more code:

SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));

will have to be converted in something like

if (rx_cfg->enable_sync)
	SET_CHECKED(rf69_set_sync_enbable(dev->spi);
else
	SET_CHECKED(rf69_set_sync_disable(dev->spi);


For me, it is important, that the configuration, you'll have to write in 
the user space program (aka fill out the config struct) will be 100% 
non-ambigious and easy to read.

Cheers,
Marcus

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Simon Sandström]

On Mon, Dec 04, 2017 at 09:59:02PM +0200, Marcus Wolf wrote:
> 
> Hi Simon, hi Dan,
> 
> if you both are of the same opinion, for me, it's fine, if we go with two
> functions.
> 
> But I don't get the advantage, if we split approx. 10 functions, to get rid
> of enum optionOnOff.
> 
> Keep in mind, that if you split the functions, in the interface
> implementation you also need more code:
> 
> SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
> 
> will have to be converted in something like
> 
> if (rx_cfg->enable_sync)
> 	SET_CHECKED(rf69_set_sync_enbable(dev->spi);
> else
> 	SET_CHECKED(rf69_set_sync_disable(dev->spi);

I think that this makes the code very clear. If the config tells us to
enable the sync then we'll enabled it, otherwise we'll disable it.

> 
> For me, it is important, that the configuration, you'll have to write in the
> user space program (aka fill out the config struct) will be 100%
> non-ambigious and easy to read.
> 
> Cheers,
> Marcus

- Simon

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 21:05 schrieb Simon Sandström:
> On Mon, Dec 04, 2017 at 09:59:02PM +0200, Marcus Wolf wrote:
>>
>> Hi Simon, hi Dan,
>>
>> if you both are of the same opinion, for me, it's fine, if we go with two
>> functions.
>>
>> But I don't get the advantage, if we split approx. 10 functions, to get rid
>> of enum optionOnOff.
>>
>> Keep in mind, that if you split the functions, in the interface
>> implementation you also need more code:
>>
>> SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
>>
>> will have to be converted in something like
>>
>> if (rx_cfg->enable_sync)
>> 	SET_CHECKED(rf69_set_sync_enbable(dev->spi);
>> else
>> 	SET_CHECKED(rf69_set_sync_disable(dev->spi);
> 
> I think that this makes the code very clear. If the config tells us to
> enable the sync then we'll enabled it, otherwise we'll disable it.
> 

Hi Simon,

I thought about it a while.

Yes, you are right, it makes it very clear - but doesn't tell a lot
extra. The only thing, you can see extra, is, that there is the option,
to turn on and to turn off. You even can't see, whether it is turend on
or off.
The info, that there is an option of en- or disabling - at least from my
side - is visible, too, if there is just one function that takes a bool
or optionOnOff as argument.

When you 'll redesign every setter, that now deals with optionOnOff in
that way, you will introduce something like 50 extra lines of code
without bringing any extra functionality to the driver.
>From my point of view, that's bad: The longer the text, the harder to
understand everything entierly, the more chances for bugs and a lot
harder to service.

So from my point of view such a redesign is nice for optics but bad for
further development. I would really prefer a solution, like Marcin
Ciupak offered. He is not blowing up the code, but even tries to reduce
the calls to READ_REG and WRITE_REG. That increases readbility, too, but
also reduces code size and chances for bugs.

But regardless of the arguments above, I don't mind. If it's a benefit
for you and lot of others in the community, I won't blame you for such a
change.
If you would add 50 lines of code with new funcitonality (e. g. support
the AES feature of the module), I would be a lot happier - and most
useres for sure, too.

Cheers,

Marcus

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Dan Carpenter]

On Mon, Dec 04, 2017 at 09:59:02PM +0200, Marcus Wolf wrote:
> Keep in mind, that if you split the functions, in the interface
> implementation you also need more code:
> 
> SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
> 
> will have to be converted in something like
> 
> if (rx_cfg->enable_sync)
> 	SET_CHECKED(rf69_set_sync_enbable(dev->spi);
> else
> 	SET_CHECKED(rf69_set_sync_disable(dev->spi);
> 

Here's what the code looks like right now:

   198          /* packet config */
   199          /* enable */
   200          SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
   201          if (rx_cfg->enable_sync == optionOn)
   202          {
   203                  SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, afterSyncInterrupt));
   204          }
   205          else
   206          {
   207                  SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, always));
   208          }

That's for the rx_cfg.  We have related but different code for the
tx_cfg.  It's strange to me that we can enable sync for rx and not for
tx...  How does that work when the setting ends up getting stored in the
same register?

The new code would look like this:

	if (rx_cfg->enable_sync) {
		ret = rf69_enable_sync(spi);
		if (ret)
			return ret;
		ret = rf69_set_fifo_fill_condition(dev->spi, afterSyncInterrupt);
		if (ret)
			return ret;
	} else {
		ret = rf69_disable_sync(dev->spi);
		if (ret)
			return ret;
		ret = rf69_set_fifo_fill_condition(dev->spi, always);
		if (ret)
			return ret;
	}

It's not the greatest, but it's not the worst...  The configuration for
->enable_sync is a bit spread out and it might be nice to move it all to
one function?

I liked Simon's naming scheme and I thought it was clear what the
rf69_set_sync(spi, false) function would do.

Simon, it seems like Marcus and I both are Ok with your style choices.
Do whatever seems best when you implement the code.  If it's awkward to
break up the functions then don't.

regards,
dan carpenter

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 05.12.2017 um 13:16 schrieb Dan Carpenter:
> On Mon, Dec 04, 2017 at 09:59:02PM +0200, Marcus Wolf wrote:
>> Keep in mind, that if you split the functions, in the interface
>> implementation you also need more code:
>>
>> SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
>>
>> will have to be converted in something like
>>
>> if (rx_cfg->enable_sync)
>> 	SET_CHECKED(rf69_set_sync_enbable(dev->spi);
>> else
>> 	SET_CHECKED(rf69_set_sync_disable(dev->spi);
>>
> 
> Here's what the code looks like right now:
> 
>    198          /* packet config */
>    199          /* enable */
>    200          SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
>    201          if (rx_cfg->enable_sync == optionOn)
>    202          {
>    203                  SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, afterSyncInterrupt));
>    204          }
>    205          else
>    206          {
>    207                  SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, always));
>    208          }
> 
> That's for the rx_cfg.  We have related but different code for the
> tx_cfg.  It's strange to me that we can enable sync for rx and not for
> tx...  How does that work when the setting ends up getting stored in the
> same register?
> 
> The new code would look like this:
> 
> 	if (rx_cfg->enable_sync) {
> 		ret = rf69_enable_sync(spi);
                      ^^^^^^^^^^^^^^^^^^^^^
> 		if (ret)
> 			return ret;
> 		ret = rf69_set_fifo_fill_condition(dev->spi, afterSyncInterrupt);
> 		if (ret)
> 			return ret;
> 	} else {
> 		ret = rf69_disable_sync(dev->spi);
> 		if (ret)
> 			return ret;
> 		ret = rf69_set_fifo_fill_condition(dev->spi, always);
> 		if (ret)
> 			return ret;
> 	}
> 
> It's not the greatest, but it's not the worst...  The configuration for
> ->enable_sync is a bit spread out and it might be nice to move it all to
> one function?
> 
> I liked Simon's naming scheme and I thought it was clear what the
> rf69_set_sync(spi, false) function would do.
  ^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Simon, it seems like Marcus and I both are Ok with your style choices.
> Do whatever seems best when you implement the code.  If it's awkward to
> break up the functions then don't.
> 
> regards,
> dan carpenter
> 

Hi Dan,

now I am a bit confused.

My favourit:
------------
rf69_set_sync_enable(spi, false)
rf69_set_amp_enable(spi, false)
rf69_set_crc_enable(spi, false)

I prefer to keep the enable (or comparable), because it shows, what the
function is doing. For sync, for example, there are several setter:
size, tolerance, values ... AND enable (or comparable).

All functions in rf69.c should start with rf69_get or rf69_set.


Simons proposal:
----------------
rf69_set_sync_enable(spi)
rf69_set_sync_disable(spi)
rf69_set_amp_enable(spi)
rf69_set_amp_disable(spi)
rf69_set_crc_enable(spi)
rf69_set_crc_disable(spi)

I don't like that, because it's blowing up the code without bringing
extra feature (see my last mail).


In your code example, you use Simons proposal, but in your explaination
below, you show the original style - although you refer to Simons sheme...


Cheers,

Marcus

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Dan Carpenter]

On Tue, Dec 05, 2017 at 01:40:02PM +0100, Marcus Wolf wrote:
> > It's not the greatest, but it's not the worst...  The configuration for
> > ->enable_sync is a bit spread out and it might be nice to move it all to
> > one function?
> > 
> > I liked Simon's naming scheme and I thought it was clear what the
> > rf69_set_sync(spi, false) function would do.
>   ^^^^^^^^^^^^^^^^^^^^^^^^^
> > 

Simon's liked splitting it up but he also proposed this alternative:

rf69_set_sync_operation(spi, true/false);

but I removed the "_operation" because I think it doesn't add anything.

> > Simon, it seems like Marcus and I both are Ok with your style choices.
> > Do whatever seems best when you implement the code.  If it's awkward to
> > break up the functions then don't.
> > 
> > regards,
> > dan carpenter
> > 
> 
> Hi Dan,
> 
> now I am a bit confused.
> 
> My favourit:
> ------------
> rf69_set_sync_enable(spi, false)
> rf69_set_amp_enable(spi, false)
> rf69_set_crc_enable(spi, false)
> 
> I prefer to keep the enable (or comparable), because it shows, what the
> function is doing. For sync, for example, there are several setter:
> size, tolerance, values ... AND enable (or comparable).

To me it's just weird that "_enable" disables anything.  I really
prefer just splitting it up.  I don't think it will bloat the code.
But I'm also fine with:

rf69_set_sync(spi, true/false)
rf69_set_amp(spi, true/false)
rf69_set_crc(spi, true/false)

Anyway, I feel like I'll like whatever Simon does.  Some of these
things, you can't tell how they'll look until the end until you try.
Let's wait until we see a patch before we debate any more.

regards,
dan carpenter

[PATCH 5/6] staging: pi433: Rename enum dataMode in rf69_enum.h

[Simon Sandström]

Renames enum dataMode and its values packet, continuous, continuousNoSync
to enum data_mode and PACKET, CONTINUOUS, CONTINUOUS_NO_SYNC. Fixes
checkpatch.pl warnings: "Avoid CamelCase: <dataMode>, <continuousNoSync>".

Signed-off-by: Simon Sandström <simon@nikanor.nu>
---
 drivers/staging/pi433/pi433_if.c  |  2 +-
 drivers/staging/pi433/rf69.c      | 10 +++++-----
 drivers/staging/pi433/rf69.h      |  2 +-
 drivers/staging/pi433/rf69_enum.h |  8 ++++----
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 4f6830f369e9..e7a730d312c5 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -1090,7 +1090,7 @@ static int pi433_probe(struct spi_device *spi)
 
 	/* setup the radio module */
 	SET_CHECKED(rf69_set_mode		(spi, standby));
-	SET_CHECKED(rf69_set_data_mode		(spi, packet));
+	SET_CHECKED(rf69_set_data_mode		(spi, PACKET));
 	SET_CHECKED(rf69_set_amplifier_0	(spi, OPTION_ON));
 	SET_CHECKED(rf69_set_amplifier_1	(spi, OPTION_OFF));
 	SET_CHECKED(rf69_set_amplifier_2	(spi, OPTION_OFF));
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index ebb3ddd1a957..cf833ac23c06 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -61,16 +61,16 @@ int rf69_set_mode(struct spi_device *spi, enum mode mode)
 
 }
 
-int rf69_set_data_mode(struct spi_device *spi, enum dataMode dataMode)
+int rf69_set_data_mode(struct spi_device *spi, enum data_mode data_mode)
 {
 	#ifdef DEBUG
 		dev_dbg(&spi->dev, "set: data mode");
 	#endif
 
-	switch (dataMode) {
-	case packet:		return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_PACKET);
-	case continuous:	return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_CONTINUOUS);
-	case continuousNoSync:  return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_CONTINUOUS_NOSYNC);
+	switch (data_mode) {
+	case PACKET:		 return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_PACKET);
+	case CONTINUOUS:	 return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_CONTINUOUS);
+	case CONTINUOUS_NO_SYNC: return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_CONTINUOUS_NOSYNC);
 	default:
 		dev_dbg(&spi->dev, "set: illegal input param");
 		return -EINVAL;
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index 12c2e106785e..090743716326 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -26,7 +26,7 @@
 #define FIFO_THRESHOLD	15		/* in byte */
 
 int rf69_set_mode(struct spi_device *spi, enum mode mode);
-int rf69_set_data_mode(struct spi_device *spi, enum dataMode dataMode);
+int rf69_set_data_mode(struct spi_device *spi, enum data_mode data_mode);
 int rf69_set_modulation(struct spi_device *spi, enum modulation modulation);
 enum modulation rf69_get_modulation(struct spi_device *spi);
 int rf69_set_modulation_shaping(struct spi_device *spi, enum modShaping modShaping);
diff --git a/drivers/staging/pi433/rf69_enum.h b/drivers/staging/pi433/rf69_enum.h
index 5247e9269de9..a55528a72f47 100644
--- a/drivers/staging/pi433/rf69_enum.h
+++ b/drivers/staging/pi433/rf69_enum.h
@@ -31,10 +31,10 @@ enum mode {
 	receive
 };
 
-enum dataMode {
-	packet,
-	continuous,
-	continuousNoSync
+enum data_mode {
+	PACKET,
+	CONTINUOUS,
+	CONTINUOUS_NO_SYNC,
 };
 
 enum modulation {
-- 
2.11.0

Re: [PATCH 5/6] staging: pi433: Rename enum dataMode in rf69_enum.h

[Dan Carpenter]

On Sun, Dec 03, 2017 at 04:17:25PM +0100, Simon Sandström wrote:
> Renames enum dataMode and its values packet, continuous, continuousNoSync
> to enum data_mode and PACKET, CONTINUOUS, CONTINUOUS_NO_SYNC. Fixes
> checkpatch.pl warnings: "Avoid CamelCase: <dataMode>, <continuousNoSync>".

These names are too generic.  Delete them.  Use DATAMODUL_MODE_PACKET
and friends directly.

int rf69_set_data_mode(struct spi_device *spi, u8 val)
{
	return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | val);
}

Only DATAMODUL_MODE_PACKET is ever used.  There is no need to validate
the parameters.

regards,
dan carpenter

Re: [PATCH 5/6] staging: pi433: Rename enum dataMode in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 12:24 schrieb Dan Carpenter:
> On Sun, Dec 03, 2017 at 04:17:25PM +0100, Simon Sandström wrote:
>> Renames enum dataMode and its values packet, continuous, continuousNoSync
>> to enum data_mode and PACKET, CONTINUOUS, CONTINUOUS_NO_SYNC. Fixes
>> checkpatch.pl warnings: "Avoid CamelCase: <dataMode>, <continuousNoSync>".
> 
> These names are too generic.  Delete them.  Use DATAMODUL_MODE_PACKET
> and friends directly.
> 
> int rf69_set_data_mode(struct spi_device *spi, u8 val)
> {
> 	return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | val);
> }
> 
> Only DATAMODUL_MODE_PACKET is ever used.  There is no need to validate
> the parameters.
> 
> regards,
> dan carpenter
> 

Hi Dan, hi Simon,

like I wrote a few days ago to Marcin Ciupak, I see two disadvantages in 
doing so.

If you want to go that way, you - as far as I believe - need to alter 
the values in rf69_enum.h, so they carry the corresponding values from 
rf69_reg.h. To avoid confusion, you will need to remove the values from 
rf69_reg.h.
But then you have to keep track of two files (enum.h and reg.h), if you 
want to further develop register access stuff. I would prefer to keep 
all chip/register related values at the same place.

Second there might be the idea of supporting different chips in the 
future (I already thought about).
Then it might be, that DATAMODUL_MODE_PACKET might need an other value.
Therefore, I introduced the "double layer" - enums as labels for the 
user space and defines, containing the values, for the register access.

For closer details, pls. see my long answer to Marcin.

I am not sure, whether simplification of the code like proposed is more 
important, then the disadvatages, I mentioned.

Cheers,

Marcus

Re: [PATCH 5/6] staging: pi433: Rename enum dataMode in rf69_enum.h

[Dan Carpenter]

On Mon, Dec 04, 2017 at 09:12:52PM +0200, Marcus Wolf wrote:
> 
> 
> Am 04.12.2017 um 12:24 schrieb Dan Carpenter:
> > On Sun, Dec 03, 2017 at 04:17:25PM +0100, Simon Sandström wrote:
> > > Renames enum dataMode and its values packet, continuous, continuousNoSync
> > > to enum data_mode and PACKET, CONTINUOUS, CONTINUOUS_NO_SYNC. Fixes
> > > checkpatch.pl warnings: "Avoid CamelCase: <dataMode>, <continuousNoSync>".
> > 
> > These names are too generic.  Delete them.  Use DATAMODUL_MODE_PACKET
> > and friends directly.
> > 
> > int rf69_set_data_mode(struct spi_device *spi, u8 val)
> > {
> > 	return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | val);
> > }
> > 
> > Only DATAMODUL_MODE_PACKET is ever used.  There is no need to validate
> > the parameters.
> > 
> > regards,
> > dan carpenter
> > 
> 
> Hi Dan, hi Simon,
> 
> like I wrote a few days ago to Marcin Ciupak, I see two disadvantages in
> doing so.
> 
> If you want to go that way, you - as far as I believe - need to alter the
> values in rf69_enum.h, so they carry the corresponding values from
> rf69_reg.h. To avoid confusion, you will need to remove the values from
> rf69_reg.h.
> But then you have to keep track of two files (enum.h and reg.h), if you want
> to further develop register access stuff. I would prefer to keep all
> chip/register related values at the same place.

In my mind we were just deleting one of these not keeping them in sync.

> 
> Second there might be the idea of supporting different chips in the future
> (I already thought about).

Linux style is never to write code for the future.


> Then it might be, that DATAMODUL_MODE_PACKET might need an other value.

That's future code so we can delete that sentence for now.

> Therefore, I introduced the "double layer" - enums as labels for the user
> space and defines, containing the values, for the register access.

No.  We don't do abstraction layers.

regards,
dan carpenter

Re: [PATCH 5/6] staging: pi433: Rename enum dataMode in rf69_enum.h

[Marcus Wolf]

>> Second there might be the idea of supporting different chips in the future
>> (I already thought about).
> 
> Linux style is never to write code for the future.

Ok. I didn't know.
To be honest, I already started writing code, also supporting the rf12 
some time ago, thus programming a rfxx.c, but never finished, due to 
lack of time.
For getting stuff started, I need to focus on rf69 and pi433.

A few monthes ago, Hope RF (the producer of those chips) proposed me a 
new chip (can't remember the number - maybe 95), that also supports 
loraWan. Seems like there will be even more interesting chips coming up, 
that could be controlled with a similar interface implementation.

>> Then it might be, that DATAMODUL_MODE_PACKET might need an other value.
> 
> That's future code so we can delete that sentence for now.

With the rule above, you are absolutely right. But we now spend time, to 
remove an currently non necessary feature ("double layer"), which will 
take time to re-introduce as soon, as someone wants to support a second 
chip.
Isn't that double-work and a thus a pitty?

Cheers,
Marcus

Re: [PATCH 5/6] staging: pi433: Rename enum dataMode in rf69_enum.h

[Dan Carpenter]

On Mon, Dec 04, 2017 at 09:31:06PM +0200, Marcus Wolf wrote:
> > > Then it might be, that DATAMODUL_MODE_PACKET might need an other value.
> > 
> > That's future code so we can delete that sentence for now.
> 
> With the rule above, you are absolutely right. But we now spend time, to
> remove an currently non necessary feature ("double layer"), which will take
> time to re-introduce as soon, as someone wants to support a second chip.
> Isn't that double-work and a thus a pitty?
> 

It is what it is...  In the kernel we insist all code have a user right
now when it's merged.  Unused code or future code is deleted.  We hate
abstraction layers.  Everyone argues that their abstraction layer is
different and good but kernel devs instinctively hate abstraction.

To be honest, in the kernel we do do a lot of work twice.  I made people
redo 9 quite large patches for this pi4333 driver today.  And they're
probably going end up conflicting and have to be redone again...  :/
That does suck.  I don't know what to do about it.

In my view it helps that people sending patches don't ever have to worry
about future code and we can focus on what exists now.  Greg will never
reject code for "future reasons" unless the future is almost right away
like tomorrow or maybe the next day.

regards,
dan carpenter

Re: [PATCH 5/6] staging: pi433: Rename enum dataMode in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 21:56 schrieb Dan Carpenter:
> On Mon, Dec 04, 2017 at 09:31:06PM +0200, Marcus Wolf wrote:
>>>> Then it might be, that DATAMODUL_MODE_PACKET might need an other value.
>>>
>>> That's future code so we can delete that sentence for now.
>>
>> With the rule above, you are absolutely right. But we now spend time, to
>> remove an currently non necessary feature ("double layer"), which will take
>> time to re-introduce as soon, as someone wants to support a second chip.
>> Isn't that double-work and a thus a pitty?
>>
> 
> It is what it is...  In the kernel we insist all code have a user right
> now when it's merged.  Unused code or future code is deleted.  We hate
> abstraction layers.  Everyone argues that their abstraction layer is
> different and good but kernel devs instinctively hate abstraction.
> 
> To be honest, in the kernel we do do a lot of work twice.  I made people
> redo 9 quite large patches for this pi4333 driver today.  And they're
> probably going end up conflicting and have to be redone again...  :/
> That does suck.  I don't know what to do about it.
> 
> In my view it helps that people sending patches don't ever have to worry
> about future code and we can focus on what exists now.  Greg will never
> reject code for "future reasons" unless the future is almost right away
> like tomorrow or maybe the next day.
> 
> regards,
> dan carpenter
> 

Hi Dan,

I am self employed and controling two small companies. For me it is very 
important to do efficient work - otherwise the 24 hours of a day are too 
short to get my work done, even if I include the night.

The goal of most projects (my own, as well as my customers) is very 
clear, but normaly you are not able to reach it in one pass. Therefore 
projects are split up in parts and try to release parts, to be on market 
earlier.
No one would accept, if I would optimise a software for a current 
release in a way, that I close doors for the final goal.

So I agree: We can't change the rules and have to take them as they are.

But if I read your lines, it's shaking me. I observed this sending the 
patch over and over again and it realy bugs me. Not beacause it might be 
boring, mainly because for me it feels like a huge waste of time - time 
I simply don't have.
Same applies to removing stuff, when I already now, (at least for my 
products) I will need it in future.

Maybe controlling a straup, developing fancy new products, the market 
likes to have (and in a time, the market is accepting you to present 
them) and contributing to the kernel need that different kind of mind 
set, that it's dam hard to do both at the same time.

Cheers,
Marcus

[PATCH 6/6] staging: pi433: Rename enum modShaping in rf69_enum.h

[Simon Sandström]

Renames enum modShaping and its values to get rid of checkpatch.pl
warnings: "Avoid CamelCase: <modShaping>".

Signed-off-by: Simon Sandström <simon@nikanor.nu>
---
 drivers/staging/pi433/pi433_if.c  |  2 +-
 drivers/staging/pi433/pi433_if.h  |  2 +-
 drivers/staging/pi433/rf69.c      | 21 +++++++++++----------
 drivers/staging/pi433/rf69.h      |  2 +-
 drivers/staging/pi433/rf69_enum.h | 14 +++++++-------
 5 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index e7a730d312c5..e457fae110cc 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -260,7 +260,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct pi433_tx_cfg *tx_cfg)
 	SET_CHECKED(rf69_set_modulation	(dev->spi, tx_cfg->modulation));
 	SET_CHECKED(rf69_set_deviation	(dev->spi, tx_cfg->dev_frequency));
 	SET_CHECKED(rf69_set_pa_ramp	(dev->spi, tx_cfg->pa_ramp));
-	SET_CHECKED(rf69_set_modulation_shaping(dev->spi, tx_cfg->modShaping));
+	SET_CHECKED(rf69_set_modulation_shaping(dev->spi, tx_cfg->mod_shaping));
 	SET_CHECKED(rf69_set_tx_start_condition(dev->spi, tx_cfg->tx_start_condition));
 
 	/* packet format enable */
diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
index 34ff0d4807bd..bcfe29840889 100644
--- a/drivers/staging/pi433/pi433_if.h
+++ b/drivers/staging/pi433/pi433_if.h
@@ -63,7 +63,7 @@ struct pi433_tx_cfg {
 	__u16			bit_rate;
 	__u32			dev_frequency;
 	enum modulation		modulation;
-	enum modShaping		modShaping;
+	enum mod_shaping	mod_shaping;
 
 	enum paRamp		pa_ramp;
 
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index cf833ac23c06..4a5de403f984 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -109,27 +109,28 @@ enum modulation rf69_get_modulation(struct spi_device *spi)
 	}
 }
 
-int rf69_set_modulation_shaping(struct spi_device *spi, enum modShaping modShaping)
+int rf69_set_modulation_shaping(struct spi_device *spi,
+				enum mod_shaping mod_shaping)
 {
 	#ifdef DEBUG
 		dev_dbg(&spi->dev, "set: mod shaping");
 	#endif
 
 	if (rf69_get_modulation(spi) == FSK) {
-		switch (modShaping) {
-		case shapingOff: return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODULATION_SHAPE) | DATAMODUL_MODULATION_SHAPE_NONE);
-		case shaping1_0: return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODULATION_SHAPE) | DATAMODUL_MODULATION_SHAPE_1_0);
-		case shaping0_5: return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODULATION_SHAPE) | DATAMODUL_MODULATION_SHAPE_0_3);
-		case shaping0_3: return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODULATION_SHAPE) | DATAMODUL_MODULATION_SHAPE_0_5);
+		switch (mod_shaping) {
+		case SHAPING_OFF: return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODULATION_SHAPE) | DATAMODUL_MODULATION_SHAPE_NONE);
+		case SHAPING_1_0: return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODULATION_SHAPE) | DATAMODUL_MODULATION_SHAPE_1_0);
+		case SHAPING_0_5: return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODULATION_SHAPE) | DATAMODUL_MODULATION_SHAPE_0_3);
+		case SHAPING_0_3: return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODULATION_SHAPE) | DATAMODUL_MODULATION_SHAPE_0_5);
 		default:
 			dev_dbg(&spi->dev, "set: illegal input param");
 			return -EINVAL;
 		}
 	} else {
-		switch (modShaping) {
-		case shapingOff: return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODULATION_SHAPE) | DATAMODUL_MODULATION_SHAPE_NONE);
-		case shapingBR:	 return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODULATION_SHAPE) | DATAMODUL_MODULATION_SHAPE_BR);
-		case shaping2BR: return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODULATION_SHAPE) | DATAMODUL_MODULATION_SHAPE_2BR);
+		switch (mod_shaping) {
+		case SHAPING_OFF: return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODULATION_SHAPE) | DATAMODUL_MODULATION_SHAPE_NONE);
+		case SHAPING_BR:  return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODULATION_SHAPE) | DATAMODUL_MODULATION_SHAPE_BR);
+		case SHAPING_2BR: return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODULATION_SHAPE) | DATAMODUL_MODULATION_SHAPE_2BR);
 		default:
 			dev_dbg(&spi->dev, "set: illegal input param");
 			return -EINVAL;
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index 090743716326..12bd28e4390b 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -29,7 +29,7 @@ int rf69_set_mode(struct spi_device *spi, enum mode mode);
 int rf69_set_data_mode(struct spi_device *spi, enum data_mode data_mode);
 int rf69_set_modulation(struct spi_device *spi, enum modulation modulation);
 enum modulation rf69_get_modulation(struct spi_device *spi);
-int rf69_set_modulation_shaping(struct spi_device *spi, enum modShaping modShaping);
+int rf69_set_modulation_shaping(struct spi_device *spi, enum mod_shaping mod_shaping);
 int rf69_set_bit_rate(struct spi_device *spi, u16 bitRate);
 int rf69_set_deviation(struct spi_device *spi, u32 deviation);
 int rf69_set_frequency(struct spi_device *spi, u32 frequency);
diff --git a/drivers/staging/pi433/rf69_enum.h b/drivers/staging/pi433/rf69_enum.h
index a55528a72f47..e1f7ae4fd24e 100644
--- a/drivers/staging/pi433/rf69_enum.h
+++ b/drivers/staging/pi433/rf69_enum.h
@@ -42,13 +42,13 @@ enum modulation {
 	FSK
 };
 
-enum modShaping {
-	shapingOff,
-	shaping1_0,
-	shaping0_5,
-	shaping0_3,
-	shapingBR,
-	shaping2BR
+enum mod_shaping {
+	SHAPING_OFF,
+	SHAPING_1_0,
+	SHAPING_0_5,
+	SHAPING_0_3,
+	SHAPING_BR,
+	SHAPING_2BR
 };
 
 enum paRamp {
-- 
2.11.0

Re: [PATCH 6/6] staging: pi433: Rename enum modShaping in rf69_enum.h

[Dan Carpenter]

On Sun, Dec 03, 2017 at 04:17:26PM +0100, Simon Sandström wrote:
> diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
> index 34ff0d4807bd..bcfe29840889 100644
> --- a/drivers/staging/pi433/pi433_if.h
> +++ b/drivers/staging/pi433/pi433_if.h
> @@ -63,7 +63,7 @@ struct pi433_tx_cfg {
>  	__u16			bit_rate;
>  	__u32			dev_frequency;
>  	enum modulation		modulation;
> -	enum modShaping		modShaping;
> +	enum mod_shaping	mod_shaping;
>  

I looked at how mod_shaping is set and the only place is in the ioctl:

   789          case PI433_IOC_WR_TX_CFG:
   790                  if (copy_from_user(&instance->tx_cfg, argp,
   791                                          sizeof(struct pi433_tx_cfg)))
   792                          return -EFAULT;
   793                  break;

We just write over the whole config.  Including important things like
rx_cfg.fixed_message_length.  There is no locking so when we do things
like:

   385          /* fixed or unlimited length? */
   386          if (dev->rx_cfg.fixed_message_length != 0)
   387          {
   388                  if (dev->rx_cfg.fixed_message_length > dev->rx_buffer_size)
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
check

   389                  {
   390                          retval = -1;
   391                          goto abort;
   392                  }
   393                  bytes_total = dev->rx_cfg.fixed_message_length;
                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
set this in the ioctl after the check but before this line and it looks
like a security problem.

   394                  dev_dbg(dev->dev,"rx: msg len set to %d by fixed length", bytes_total);
   395          }

Anyway, I guess this patch is fine.

regards,
dan carpenter

Re: [PATCH 6/6] staging: pi433: Rename enum modShaping in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 12:33 schrieb Dan Carpenter:
> On Sun, Dec 03, 2017 at 04:17:26PM +0100, Simon Sandström wrote:
>> diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
>> index 34ff0d4807bd..bcfe29840889 100644
>> --- a/drivers/staging/pi433/pi433_if.h
>> +++ b/drivers/staging/pi433/pi433_if.h
>> @@ -63,7 +63,7 @@ struct pi433_tx_cfg {
>>   	__u16			bit_rate;
>>   	__u32			dev_frequency;
>>   	enum modulation		modulation;
>> -	enum modShaping		modShaping;
>> +	enum mod_shaping	mod_shaping;
>>   
> 
> I looked at how mod_shaping is set and the only place is in the ioctl:
> 
>     789          case PI433_IOC_WR_TX_CFG:
>     790                  if (copy_from_user(&instance->tx_cfg, argp,
>     791                                          sizeof(struct pi433_tx_cfg)))
>     792                          return -EFAULT;
>     793                  break;
> 
> We just write over the whole config.  Including important things like
> rx_cfg.fixed_message_length.  There is no locking so when we do things
> like:
> 
>     385          /* fixed or unlimited length? */
>     386          if (dev->rx_cfg.fixed_message_length != 0)
>     387          {
>     388                  if (dev->rx_cfg.fixed_message_length > dev->rx_buffer_size)
>                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> check
> 
>     389                  {
>     390                          retval = -1;
>     391                          goto abort;
>     392                  }
>     393                  bytes_total = dev->rx_cfg.fixed_message_length;
>                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> set this in the ioctl after the check but before this line and it looks
> like a security problem.
> 
>     394                  dev_dbg(dev->dev,"rx: msg len set to %d by fixed length", bytes_total);
>     395          }
> 
> Anyway, I guess this patch is fine.
> 
> regards,
> dan carpenter
> 

Hi Dan,

you are mixing rx and tx. The part from IOCTL is copied from the 
tx-part, the lower part is dealing with rx.

With rx there should be no problem, since IOCTL is blocked, as long as 
an rx operation is going on.

With tx, I also expect no problems, since instance->tx_cfg is never used 
to configure the rf69. Everytime, you pass in new data via write() a 
copy of tx_cfg is made. Transmission is done, using the copy of the 
tx_cfg, never by using instance->tx_cfg.

But maybe I didn't got your point and misunderstand your intention.

Cheers,

Marcus

Re: [PATCH 6/6] staging: pi433: Rename enum modShaping in rf69_enum.h

[Dan Carpenter]

On Mon, Dec 04, 2017 at 08:59:35PM +0200, Marcus Wolf wrote:
> 
> Am 04.12.2017 um 12:33 schrieb Dan Carpenter:
> > On Sun, Dec 03, 2017 at 04:17:26PM +0100, Simon Sandström wrote:
> > > diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
> > > index 34ff0d4807bd..bcfe29840889 100644
> > > --- a/drivers/staging/pi433/pi433_if.h
> > > +++ b/drivers/staging/pi433/pi433_if.h
> > > @@ -63,7 +63,7 @@ struct pi433_tx_cfg {
> > >   	__u16			bit_rate;
> > >   	__u32			dev_frequency;
> > >   	enum modulation		modulation;
> > > -	enum modShaping		modShaping;
> > > +	enum mod_shaping	mod_shaping;
> > 
> > I looked at how mod_shaping is set and the only place is in the ioctl:
> > 
> >     789          case PI433_IOC_WR_TX_CFG:
> >     790                  if (copy_from_user(&instance->tx_cfg, argp,
> >     791                                          sizeof(struct pi433_tx_cfg)))
> >     792                          return -EFAULT;
> >     793                  break;
> > 
> > We just write over the whole config.  Including important things like
> > rx_cfg.fixed_message_length.  There is no locking so when we do things
> > like:
> > 
> >     385          /* fixed or unlimited length? */
> >     386          if (dev->rx_cfg.fixed_message_length != 0)
> >     387          {
> >     388                  if (dev->rx_cfg.fixed_message_length > dev->rx_buffer_size)
> >                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > check
> > 
> >     389                  {
> >     390                          retval = -1;
> >     391                          goto abort;
> >     392                  }
> >     393                  bytes_total = dev->rx_cfg.fixed_message_length;
> >                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > set this in the ioctl after the check but before this line and it looks
> > like a security problem.
> > 
> >     394                  dev_dbg(dev->dev,"rx: msg len set to %d by fixed length", bytes_total);
> >     395          }
> > 
> > Anyway, I guess this patch is fine.
> > 
> > regards,
> > dan carpenter
> > 
> 
> Hi Dan,
> 
> you are mixing rx and tx. The part from IOCTL is copied from the tx-part,
> the lower part is dealing with rx.
> 
> With rx there should be no problem, since IOCTL is blocked, as long as an rx
> operation is going on.
> 
> With tx, I also expect no problems, since instance->tx_cfg is never used to
> configure the rf69. Everytime, you pass in new data via write() a copy of
> tx_cfg is made. Transmission is done, using the copy of the tx_cfg, never by
> using instance->tx_cfg.
> 
> But maybe I didn't got your point and misunderstand your intention.
> 

No.  You're right.  I mixed up rx and tx.  But the ioctl interface still
seems really horrible.  We generally frown on adding new ioctls at all,
but in this case to just write over the whole struct with no locking
seems really bad.

regards,
dan carpenter

Re: [PATCH 6/6] staging: pi433: Rename enum modShaping in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 21:18 schrieb Dan Carpenter:
> On Mon, Dec 04, 2017 at 08:59:35PM +0200, Marcus Wolf wrote:
>>
>> Am 04.12.2017 um 12:33 schrieb Dan Carpenter:
>>> On Sun, Dec 03, 2017 at 04:17:26PM +0100, Simon Sandström wrote:
>>>> diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
>>>> index 34ff0d4807bd..bcfe29840889 100644
>>>> --- a/drivers/staging/pi433/pi433_if.h
>>>> +++ b/drivers/staging/pi433/pi433_if.h
>>>> @@ -63,7 +63,7 @@ struct pi433_tx_cfg {
>>>>    	__u16			bit_rate;
>>>>    	__u32			dev_frequency;
>>>>    	enum modulation		modulation;
>>>> -	enum modShaping		modShaping;
>>>> +	enum mod_shaping	mod_shaping;
>>>
>>> I looked at how mod_shaping is set and the only place is in the ioctl:
>>>
>>>      789          case PI433_IOC_WR_TX_CFG:
>>>      790                  if (copy_from_user(&instance->tx_cfg, argp,
>>>      791                                          sizeof(struct pi433_tx_cfg)))
>>>      792                          return -EFAULT;
>>>      793                  break;
>>>
>>> We just write over the whole config.  Including important things like
>>> rx_cfg.fixed_message_length.  There is no locking so when we do things
>>> like:
>>>
>>>      385          /* fixed or unlimited length? */
>>>      386          if (dev->rx_cfg.fixed_message_length != 0)
>>>      387          {
>>>      388                  if (dev->rx_cfg.fixed_message_length > dev->rx_buffer_size)
>>>                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> check
>>>
>>>      389                  {
>>>      390                          retval = -1;
>>>      391                          goto abort;
>>>      392                  }
>>>      393                  bytes_total = dev->rx_cfg.fixed_message_length;
>>>                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> set this in the ioctl after the check but before this line and it looks
>>> like a security problem.
>>>
>>>      394                  dev_dbg(dev->dev,"rx: msg len set to %d by fixed length", bytes_total);
>>>      395          }
>>>
>>> Anyway, I guess this patch is fine.
>>>
>>> regards,
>>> dan carpenter
>>>
>>
>> Hi Dan,
>>
>> you are mixing rx and tx. The part from IOCTL is copied from the tx-part,
>> the lower part is dealing with rx.
>>
>> With rx there should be no problem, since IOCTL is blocked, as long as an rx
>> operation is going on.
>>
>> With tx, I also expect no problems, since instance->tx_cfg is never used to
>> configure the rf69. Everytime, you pass in new data via write() a copy of
>> tx_cfg is made. Transmission is done, using the copy of the tx_cfg, never by
>> using instance->tx_cfg.
>>
>> But maybe I didn't got your point and misunderstand your intention.
>>
> 
> No.  You're right.  I mixed up rx and tx.  But the ioctl interface still
> seems really horrible.  We generally frown on adding new ioctls at all,
> but in this case to just write over the whole struct with no locking
> seems really bad.
> 
> regards,
> dan carpenter
> 

In principle, you are right. But that's even a more macroscopic problem.

If someone sets a config, he needs for a telegram, and someone else 
comes in with another config, before the first one could fire his write,
shit will happen.
Same on RX-side: First one sets his config for receiving and will not 
get, what he wants, if someone else sets an other config, before he can 
fire his read.

If noone changes the config, until read() or write() was called, we are 
out of danger - even concerning the risk you mentioned.

An option to avoid that, could be, that every write and read transaction 
needs to pass in a complete config struct.
There were reasons, not to do so, but we could think of implementing it 
that was.

Are there other options for configuration, despite IOCTL?

Cheers,

Marcus
Cheers,

Marcus

Re: [PATCH 6/6] staging: pi433: Rename enum modShaping in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 21:18 schrieb Dan Carpenter:
> On Mon, Dec 04, 2017 at 08:59:35PM +0200, Marcus Wolf wrote:
>>
>> Am 04.12.2017 um 12:33 schrieb Dan Carpenter:
>>> On Sun, Dec 03, 2017 at 04:17:26PM +0100, Simon Sandström wrote:
>>>> diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
>>>> index 34ff0d4807bd..bcfe29840889 100644
>>>> --- a/drivers/staging/pi433/pi433_if.h
>>>> +++ b/drivers/staging/pi433/pi433_if.h
>>>> @@ -63,7 +63,7 @@ struct pi433_tx_cfg {
>>>>    	__u16			bit_rate;
>>>>    	__u32			dev_frequency;
>>>>    	enum modulation		modulation;
>>>> -	enum modShaping		modShaping;
>>>> +	enum mod_shaping	mod_shaping;
>>>
>>> I looked at how mod_shaping is set and the only place is in the ioctl:
>>>
>>>      789          case PI433_IOC_WR_TX_CFG:
>>>      790                  if (copy_from_user(&instance->tx_cfg, argp,
>>>      791                                          sizeof(struct pi433_tx_cfg)))
>>>      792                          return -EFAULT;
>>>      793                  break;
>>>
>>> We just write over the whole config.  Including important things like
>>> rx_cfg.fixed_message_length.  There is no locking so when we do things
>>> like:
>>>
>>>      385          /* fixed or unlimited length? */
>>>      386          if (dev->rx_cfg.fixed_message_length != 0)
>>>      387          {
>>>      388                  if (dev->rx_cfg.fixed_message_length > dev->rx_buffer_size)
>>>                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> check
>>>
>>>      389                  {
>>>      390                          retval = -1;
>>>      391                          goto abort;
>>>      392                  }
>>>      393                  bytes_total = dev->rx_cfg.fixed_message_length;
>>>                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> set this in the ioctl after the check but before this line and it looks
>>> like a security problem.
>>>
>>>      394                  dev_dbg(dev->dev,"rx: msg len set to %d by fixed length", bytes_total);
>>>      395          }
>>>
>>> Anyway, I guess this patch is fine.
>>>
>>> regards,
>>> dan carpenter
>>>
>>
>> Hi Dan,
>>
>> you are mixing rx and tx. The part from IOCTL is copied from the tx-part,
>> the lower part is dealing with rx.
>>
>> With rx there should be no problem, since IOCTL is blocked, as long as an rx
>> operation is going on.
>>
>> With tx, I also expect no problems, since instance->tx_cfg is never used to
>> configure the rf69. Everytime, you pass in new data via write() a copy of
>> tx_cfg is made. Transmission is done, using the copy of the tx_cfg, never by
>> using instance->tx_cfg.
>>
>> But maybe I didn't got your point and misunderstand your intention.
>>
> 
> No.  You're right.  I mixed up rx and tx.  But the ioctl interface still
> seems really horrible.  We generally frown on adding new ioctls at all,
> but in this case to just write over the whole struct with no locking
> seems really bad.
> 
> regards,
> dan carpenter
> 

Hi Dan,

unexpectetly I was into the driver code today, because a customer asked 
for an enhancment. In doing so, I also had a look at the points we 
discussed above.

Since both - the tx_cfg and the rx_cfg buffer belong to the instance, in 
order to get into trouble, you need to use the same file descriptor. If 
an other app is changing its config, it doesn't touch the current app.

So for RX: If a programm has called read(), it won't be able to 
succesfully call ioctl any more, because it is blocked:

         case PI433_IOC_WR_RX_CFG:
                 mutex_lock(&device->rx_lock);

                 /* during pendig read request, change of config not 
allowed */
                 if (device->rx_active) {
                         mutex_unlock(&device->rx_lock);
                         return -EAGAIN;
                 }

For TX in fact there is a little risk:
If a programm is using two tasks and passes the descriptor to both 
tasks, one is using the ioctl() and one is using write() and they are 
not synchronised, it might happen, that the ioctl is in the middle of 
the update the tx_cfg, while the write() is in the middle of copying the 
tx_cfg to the kernel fifo.
On one hand, that might be an "open point" at the driver, on the other 
hand no one will do such a programm architecture. Even if the driver 
will prevent a broken tx_cfg by mutex, the programm will never know, 
what it gets, if it issues ioctl() and write() unsynchronised from 
different tasks.
For fixing the driver, it might help to lock the write to the tx_cfg in 
ioctl() with the tx_fifo_lock, since write() is only copying the tx_cfg 
if it has the tx_fifo_lock.

I am not 100% sure. Maybe you (or someone else) want to crosscheck?

Regards,

Marcus