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
Basically just 's/ /\t/', to fix checkpatch.pl warnings:
"please, no spaces at the start of a line".
Signed-off-by: Simon Sandström <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Renames enum modShaping and its values to get rid of checkpatch.pl
warnings: "Avoid CamelCase: <modShaping>".
Signed-off-by: Simon Sandström <[email protected]>
---
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
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 <[email protected]>
---
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
Fixes checkpatch.pl warnings "Avoid CamelCase <DIO_x>".
Signed-off-by: Simon Sandström <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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
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 <[email protected]>
> ---
> 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
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
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
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
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
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
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
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
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
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
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
>> 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
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
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
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
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
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
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
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
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
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
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
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