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 and other minor issues in all of staging/pi433.
Changes in v2:
- Instead of fixing CamelCase issue in enum data_mode, remove it
completely.
- Split multiple functions of type
"rf69_set_x_enabled(dev, enum option_on_off)" to two functions:
"rf69_enable_x(dev)" and "rf69_disable_x(dev)".
- Move enum option_on_off to pi433_if.h as it's now only used for
ioctl.
- Fix issue where GPIOs and device not being deallocated in
pi433_probe() if SET_CHECKED fails.
- Simon
---
Simon Sandström (11):
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 modShaping in rf69_enum.h
staging: pi433: Split rf69_set_crc_enabled into two functions
staging: pi433: Split rf69_set_sync_enabled into two functions
staging: pi433: Remove enum data_mode
staging: pi433: Combine all rf69_set_amplifier_x()
staging: pi433: Move enum option_on_off to pi433_if.h
staging: pi433: Remove SET_CHECKED usage from pi433_probe
drivers/staging/pi433/pi433_if.c | 135 ++++++++++++++-------
drivers/staging/pi433/pi433_if.h | 25 ++--
drivers/staging/pi433/rf69.c | 113 +++++-------------
drivers/staging/pi433/rf69.h | 15 +--
drivers/staging/pi433/rf69_enum.h | 210 ++++++++++++++++-----------------
drivers/staging/pi433/rf69_registers.h | 44 +++----
6 files changed, 265 insertions(+), 277 deletions(-)
--
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
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
Replaces the functions rf69_set_amplifier_1, _2, _3 with two
functions: rf69_enable_amplifier(dev, amp_mask) and
rf69_disable_amplifier(dev, amp_mask).
Signed-off-by: Simon Sandström <[email protected]>
---
drivers/staging/pi433/pi433_if.c | 6 +++---
drivers/staging/pi433/rf69.c | 46 ++++------------------------------------
drivers/staging/pi433/rf69.h | 8 ++-----
3 files changed, 9 insertions(+), 51 deletions(-)
diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 3b4170b9ba94..688d0cf00782 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -1126,9 +1126,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, DATAMODUL_MODE_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));
+ SET_CHECKED(rf69_enable_amplifier(spi, MASK_PALEVEL_PA0));
+ SET_CHECKED(rf69_disable_amplifier(spi, MASK_PALEVEL_PA1));
+ SET_CHECKED(rf69_disable_amplifier(spi, MASK_PALEVEL_PA2));
SET_CHECKED(rf69_set_output_power_level (spi, 13));
SET_CHECKED(rf69_set_antenna_impedance (spi, fiftyOhm));
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index 4c9eb97978ef..c97c65ea8acd 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -262,52 +262,14 @@ int rf69_set_frequency(struct spi_device *spi, u32 frequency)
return 0;
}
-int rf69_set_amplifier_0(struct spi_device *spi,
- enum option_on_off option_on_off)
+int rf69_enable_amplifier(struct spi_device *spi, u8 amplifier_mask)
{
- #ifdef DEBUG
- dev_dbg(&spi->dev, "set: amp #0");
- #endif
-
- 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 option_on_off option_on_off)
-{
- #ifdef DEBUG
- dev_dbg(&spi->dev, "set: amp #1");
- #endif
-
- 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;
- }
+ return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) | amplifier_mask));
}
-int rf69_set_amplifier_2(struct spi_device *spi,
- enum option_on_off option_on_off)
+int rf69_disable_amplifier(struct spi_device *spi, u8 amplifier_mask)
{
- #ifdef DEBUG
- dev_dbg(&spi->dev, "set: amp #2");
- #endif
-
- 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;
- }
+ return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & ~amplifier_mask));
}
int rf69_set_output_power_level(struct spi_device *spi, u8 powerLevel)
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index 18296b4502f3..ba25ab6aeae7 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -33,12 +33,8 @@ int rf69_set_modulation_shaping(struct spi_device *spi, enum mod_shaping mod_sha
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 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_enable_amplifier(struct spi_device *spi, u8 amplifier_mask);
+int rf69_disable_amplifier(struct spi_device *spi, u8 amplifier_mask);
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);
--
2.11.0
Splits rf69_set_sync_enabled(dev, enabled) into
rf69_enable_sync(dev) and rf69_disable_sync(dev).
Signed-off-by: Simon Sandström <[email protected]>
---
drivers/staging/pi433/pi433_if.c | 21 +++++++++++++++++++--
drivers/staging/pi433/rf69.c | 18 ++++++------------
drivers/staging/pi433/rf69.h | 4 ++--
3 files changed, 27 insertions(+), 16 deletions(-)
diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 614eec7dd904..fb500d062df8 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -197,13 +197,20 @@ 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 == OPTION_ON)
{
+ ret = rf69_enable_sync(dev->spi);
+ if (ret < 0)
+ return ret;
+
SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, afterSyncInterrupt));
}
else
{
+ ret = rf69_disable_sync(dev->spi);
+ if (ret < 0)
+ return ret;
+
SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, always));
}
if (rx_cfg->enable_length_byte == OPTION_ON) {
@@ -281,7 +288,17 @@ 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_sync == OPTION_ON) {
+ ret = rf69_enable_sync(dev->spi);
+ if (ret < 0)
+ return ret;
+ } else {
+ ret = rf69_disable_sync(dev->spi);
+ if (ret < 0)
+ return ret;
+ }
+
if (tx_cfg->enable_length_byte == OPTION_ON) {
ret = rf69_set_packet_format(dev->spi, packetLengthVar);
if (ret < 0)
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index 612d59f61f88..e5b29bed35ef 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -724,20 +724,14 @@ 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 option_on_off option_on_off)
+int rf69_enable_sync(struct spi_device *spi)
{
- #ifdef DEBUG
- dev_dbg(&spi->dev, "set: sync enable");
- #endif
+ 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;
- }
+int rf69_disable_sync(struct spi_device *spi)
+{
+ return WRITE_REG(REG_SYNC_CONFIG, (READ_REG(REG_SYNC_CONFIG) & ~MASK_SYNC_CONFIG_SYNC_ON));
}
int rf69_set_fifo_fill_condition(struct spi_device *spi, enum fifoFillCondition fifoFillCondition)
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index 9428dee97de7..177223451c87 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -59,8 +59,8 @@ 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 option_on_off option_on_off);
+int rf69_enable_sync(struct spi_device *spi);
+int rf69_disable_sync(struct spi_device *spi);
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);
--
2.11.0
Call rf69_set_data_mode with DATAMODUL_MODE value directly.
Signed-off-by: Simon Sandström <[email protected]>
---
drivers/staging/pi433/pi433_if.c | 2 +-
drivers/staging/pi433/rf69.c | 15 ++-------------
drivers/staging/pi433/rf69.h | 2 +-
drivers/staging/pi433/rf69_enum.h | 6 ------
4 files changed, 4 insertions(+), 21 deletions(-)
diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index fb500d062df8..3b4170b9ba94 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -1125,7 +1125,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, DATAMODUL_MODE_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 e5b29bed35ef..4c9eb97978ef 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -61,20 +61,9 @@ 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, u8 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);
- default:
- dev_dbg(&spi->dev, "set: illegal input param");
- return -EINVAL;
- }
+ return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | data_mode);
}
int rf69_set_modulation(struct spi_device *spi, enum modulation modulation)
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index 177223451c87..18296b4502f3 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, u8 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 mod_shaping mod_shaping);
diff --git a/drivers/staging/pi433/rf69_enum.h b/drivers/staging/pi433/rf69_enum.h
index 97f615effca3..b0715b4eb6ac 100644
--- a/drivers/staging/pi433/rf69_enum.h
+++ b/drivers/staging/pi433/rf69_enum.h
@@ -31,12 +31,6 @@ enum mode {
receive
};
-enum dataMode {
- packet,
- continuous,
- continuousNoSync
-};
-
enum modulation {
OOK,
FSK
--
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
SET_CHECKED returns from the function on failure and in pi433_probe it is
necessary to free the GPIOs and the device on failure.
Signed-off-by: Simon Sandström <[email protected]>
---
drivers/staging/pi433/pi433_if.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 688d0cf00782..55ed45f45998 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -1124,13 +1124,27 @@ 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, DATAMODUL_MODE_PACKET));
- SET_CHECKED(rf69_enable_amplifier(spi, MASK_PALEVEL_PA0));
- SET_CHECKED(rf69_disable_amplifier(spi, MASK_PALEVEL_PA1));
- SET_CHECKED(rf69_disable_amplifier(spi, MASK_PALEVEL_PA2));
- SET_CHECKED(rf69_set_output_power_level (spi, 13));
- SET_CHECKED(rf69_set_antenna_impedance (spi, fiftyOhm));
+ retval = rf69_set_mode(spi, standby);
+ if (retval < 0)
+ goto minor_failed;
+ retval = rf69_set_data_mode(spi, DATAMODUL_MODE_PACKET);
+ if (retval < 0)
+ goto minor_failed;
+ retval = rf69_enable_amplifier(spi, MASK_PALEVEL_PA0);
+ if (retval < 0)
+ goto minor_failed;
+ retval = rf69_disable_amplifier(spi, MASK_PALEVEL_PA1);
+ if (retval < 0)
+ goto minor_failed;
+ retval = rf69_disable_amplifier(spi, MASK_PALEVEL_PA2);
+ if (retval < 0)
+ goto minor_failed;
+ retval = rf69_set_output_power_level(spi, 13);
+ if (retval < 0)
+ goto minor_failed;
+ retval = rf69_set_antenna_impedance(spi, fiftyOhm);
+ if (retval < 0)
+ goto minor_failed;
/* determ minor number */
retval = pi433_get_minor(device);
--
2.11.0
The enum is now only used for ioctl, so move it pi433_if.h.
Signed-off-by: Simon Sandström <[email protected]>
---
drivers/staging/pi433/pi433_if.h | 5 +++++
drivers/staging/pi433/rf69_enum.h | 5 -----
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
index bcfe29840889..c8697d144e2b 100644
--- a/drivers/staging/pi433/pi433_if.h
+++ b/drivers/staging/pi433/pi433_if.h
@@ -37,6 +37,11 @@
/*---------------------------------------------------------------------------*/
+enum option_on_off {
+ OPTION_OFF,
+ OPTION_ON
+};
+
/* IOCTL structs and commands */
/**
diff --git a/drivers/staging/pi433/rf69_enum.h b/drivers/staging/pi433/rf69_enum.h
index b0715b4eb6ac..4e64e38ae4ff 100644
--- a/drivers/staging/pi433/rf69_enum.h
+++ b/drivers/staging/pi433/rf69_enum.h
@@ -18,11 +18,6 @@
#ifndef RF69_ENUM_H
#define RF69_ENUM_H
-enum option_on_off {
- OPTION_OFF,
- OPTION_ON
-};
-
enum mode {
mode_sleep,
standby,
--
2.11.0
Splits rf69_set_crc_enabled(dev, enabled) into
rf69_enable_crc(dev) and rf69_disable_crc(dev).
Signed-off-by: Simon Sandström <[email protected]>
---
drivers/staging/pi433/pi433_if.c | 22 ++++++++++++++++++++--
drivers/staging/pi433/rf69.c | 18 ++++++------------
drivers/staging/pi433/rf69.h | 4 ++--
3 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 2ae19ac565d1..614eec7dd904 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -216,7 +216,16 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
return ret;
}
SET_CHECKED(rf69_set_adressFiltering(dev->spi, rx_cfg->enable_address_filtering));
- SET_CHECKED(rf69_set_crc_enable (dev->spi, rx_cfg->enable_crc));
+
+ if (rx_cfg->enable_crc == OPTION_ON) {
+ ret = rf69_enable_crc(dev->spi);
+ if (ret < 0)
+ return ret;
+ } else {
+ ret = rf69_disable_crc(dev->spi);
+ if (ret < 0)
+ return ret;
+ }
/* lengths */
SET_CHECKED(rf69_set_sync_size(dev->spi, rx_cfg->sync_length));
@@ -282,7 +291,16 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct pi433_tx_cfg *tx_cfg)
if (ret < 0)
return ret;
}
- SET_CHECKED(rf69_set_crc_enable (dev->spi, tx_cfg->enable_crc));
+
+ if (tx_cfg->enable_crc == OPTION_ON) {
+ ret = rf69_enable_crc(dev->spi);
+ if (ret < 0)
+ return ret;
+ } else {
+ ret = rf69_disable_crc(dev->spi);
+ if (ret < 0)
+ return ret;
+ }
/* configure sync, if enabled */
if (tx_cfg->enable_sync == OPTION_ON) {
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index b19bca8a0f26..612d59f61f88 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -822,20 +822,14 @@ int rf69_set_packet_format(struct spi_device *spi, enum packetFormat packetForma
}
}
-int rf69_set_crc_enable(struct spi_device *spi,
- enum option_on_off option_on_off)
+int rf69_enable_crc(struct spi_device *spi)
{
- #ifdef DEBUG
- dev_dbg(&spi->dev, "set: crc enable");
- #endif
+ 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;
- }
+int rf69_disable_crc(struct spi_device *spi)
+{
+ return WRITE_REG(REG_PACKETCONFIG1, (READ_REG(REG_PACKETCONFIG1) & ~MASK_PACKETCONFIG1_CRC_ON));
}
int rf69_set_adressFiltering(struct spi_device *spi, enum addressFiltering addressFiltering)
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index 1cb6db33d6fe..9428dee97de7 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -66,8 +66,8 @@ 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 option_on_off option_on_off);
+int rf69_enable_crc(struct spi_device *spi);
+int rf69_disable_crc(struct spi_device *spi);
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);
--
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 4f6830f369e9..2ae19ac565d1 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 ebb3ddd1a957..b19bca8a0f26 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 12c2e106785e..1cb6db33d6fe 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 dataMode dataMode);
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 5247e9269de9..97f615effca3 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
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
Am 06.12.2017 um 00:08 schrieb Simon Sandström:
> Splits rf69_set_crc_enabled(dev, enabled) into
> rf69_enable_crc(dev) and rf69_disable_crc(dev).
>
> Signed-off-by: Simon Sandström <[email protected]>
> ---
> drivers/staging/pi433/pi433_if.c | 22 ++++++++++++++++++++--
> drivers/staging/pi433/rf69.c | 18 ++++++------------
> drivers/staging/pi433/rf69.h | 4 ++--
> 3 files changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index 2ae19ac565d1..614eec7dd904 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -216,7 +216,16 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
> return ret;
> }
> SET_CHECKED(rf69_set_adressFiltering(dev->spi, rx_cfg->enable_address_filtering));
> - SET_CHECKED(rf69_set_crc_enable (dev->spi, rx_cfg->enable_crc));
> +
> + if (rx_cfg->enable_crc == OPTION_ON) {
> + ret = rf69_enable_crc(dev->spi);
> + if (ret < 0)
> + return ret;
> + } else {
> + ret = rf69_disable_crc(dev->spi);
> + if (ret < 0)
> + return ret;
> + }
Why don't you use SET_CHECKED(...)?
I stil don't like this kind of changes - and not using SET_CHECKED makes
it even worse, since that further increases code length.
The idea was to have the configuration as compact, as you can see in the
receiver config section. It's a pitty that the packet config already
needs such a huge number of exceptions due to technical reasons. We
shouldn't further extend the numbers of exceptions and shouldn't extend
the number of lines for setting a reg.
Initially this function was just like
set_rx_cfg()
{
SET_CHECKED(...)
SET_CHECKED(...)
SET_CHECKED(...)
SET_CHECKED(...)
}
It should be easy,
* to survey, which chip settings are touched, if set_rx_cfg is called.
* to survey, that all params of the rx_cfg struct are taken care of.
The longer the function gets, the harder it is, to service it.
I really would be happy, if we don't go this way.
Anyway, please keep the naming convention of rf69.c:
rf69 -set/get - action
-> rf69_set_crc_enable
Thanks,
Marcus
Am 06.12.2017 um 00:08 schrieb Simon Sandström:
> Call rf69_set_data_mode with DATAMODUL_MODE value directly.
>
> Signed-off-by: Simon Sandström <[email protected]>
> ---
> drivers/staging/pi433/pi433_if.c | 2 +-
> drivers/staging/pi433/rf69.c | 15 ++-------------
> drivers/staging/pi433/rf69.h | 2 +-
> drivers/staging/pi433/rf69_enum.h | 6 ------
> 4 files changed, 4 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index fb500d062df8..3b4170b9ba94 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -1125,7 +1125,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, DATAMODUL_MODE_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 e5b29bed35ef..4c9eb97978ef 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -61,20 +61,9 @@ 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, u8 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);
> - default:
> - dev_dbg(&spi->dev, "set: illegal input param");
> - return -EINVAL;
> - }
> + return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | data_mode);
> }
>
> int rf69_set_modulation(struct spi_device *spi, enum modulation modulation)
> diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
> index 177223451c87..18296b4502f3 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, u8 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 mod_shaping mod_shaping);
> diff --git a/drivers/staging/pi433/rf69_enum.h b/drivers/staging/pi433/rf69_enum.h
> index 97f615effca3..b0715b4eb6ac 100644
> --- a/drivers/staging/pi433/rf69_enum.h
> +++ b/drivers/staging/pi433/rf69_enum.h
> @@ -31,12 +31,6 @@ enum mode {
> receive
> };
>
> -enum dataMode {
> - packet,
> - continuous,
> - continuousNoSync
> -};
> -
> enum modulation {
> OOK,
> FSK
>
Hi Simon,
this change is "closing a door".
The rf69 is able to work in an advanced packet mode or in a simple
continuous mode.
The driver so far is using the advanced packet mode, only. But if it
will support continuous mode some day, it will be necessary to configure
this.
The open source projects fhem and domoticz already asked for such a
change, since for their architecture, they'll need a pi433 working in
continuous mode. But so far I am not planning to implement such a
functionality.
Since the rule for kernel development seems to be, not to care about
future, most probably you patch is fine, anyway.
Marcus
On Wed, Dec 06, 2017 at 11:05:22AM +0200, Marcus Wolf wrote:
>
>
> Am 06.12.2017 um 00:08 schrieb Simon Sandstr?m:
> > Splits rf69_set_crc_enabled(dev, enabled) into
> > rf69_enable_crc(dev) and rf69_disable_crc(dev).
> >
> > Signed-off-by: Simon Sandstr?m <[email protected]>
> > ---
> > drivers/staging/pi433/pi433_if.c | 22 ++++++++++++++++++++--
> > drivers/staging/pi433/rf69.c | 18 ++++++------------
> > drivers/staging/pi433/rf69.h | 4 ++--
> > 3 files changed, 28 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> > index 2ae19ac565d1..614eec7dd904 100644
> > --- a/drivers/staging/pi433/pi433_if.c
> > +++ b/drivers/staging/pi433/pi433_if.c
> > @@ -216,7 +216,16 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
> > return ret;
> > }
> > SET_CHECKED(rf69_set_adressFiltering(dev->spi, rx_cfg->enable_address_filtering));
> > - SET_CHECKED(rf69_set_crc_enable (dev->spi, rx_cfg->enable_crc));
> > +
> > + if (rx_cfg->enable_crc == OPTION_ON) {
> > + ret = rf69_enable_crc(dev->spi);
> > + if (ret < 0)
> > + return ret;
> > + } else {
> > + ret = rf69_disable_crc(dev->spi);
> > + if (ret < 0)
> > + return ret;
> > + }
>
> Why don't you use SET_CHECKED(...)?
>
Marcus, please don't introduce new uses of SET_CHECKED(). It has a
hidden return in it which is against kernel style and introduces very
predictable and avoidable bugs. For example, in probe().
> I stil don't like this kind of changes - and not using SET_CHECKED makes it
> even worse, since that further increases code length.
>
> The idea was to have the configuration as compact, as you can see in the
> receiver config section. It's a pitty that the packet config already needs
> such a huge number of exceptions due to technical reasons. We shouldn't
> further extend the numbers of exceptions and shouldn't extend the number of
> lines for setting a reg.
>
> Initially this function was just like
> set_rx_cfg()
> {
> SET_CHECKED(...)
> SET_CHECKED(...)
> SET_CHECKED(...)
> SET_CHECKED(...)
> }
>
> It should be easy,
> * to survey, which chip settings are touched, if set_rx_cfg is called.
> * to survey, that all params of the rx_cfg struct are taken care of.
>
> The longer the function gets, the harder it is, to service it.
> I really would be happy, if we don't go this way.
>
>
> Anyway, please keep the naming convention of rf69.c:
>
> rf69 -set/get - action
> -> rf69_set_crc_enable
No... Simon's name is better. His is shorter and makes more sense. :(
regards,
dan carpenter
Am 06.12.2017 um 00:08 schrieb Simon Sandström:
> Replaces the functions rf69_set_amplifier_1, _2, _3 with two
> functions: rf69_enable_amplifier(dev, amp_mask) and
> rf69_disable_amplifier(dev, amp_mask).
>
> Signed-off-by: Simon Sandström <[email protected]>
> ---
> drivers/staging/pi433/pi433_if.c | 6 +++---
> drivers/staging/pi433/rf69.c | 46 ++++------------------------------------
> drivers/staging/pi433/rf69.h | 8 ++-----
> 3 files changed, 9 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index 3b4170b9ba94..688d0cf00782 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -1126,9 +1126,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, DATAMODUL_MODE_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));
> + SET_CHECKED(rf69_enable_amplifier(spi, MASK_PALEVEL_PA0));
> + SET_CHECKED(rf69_disable_amplifier(spi, MASK_PALEVEL_PA1));
> + SET_CHECKED(rf69_disable_amplifier(spi, MASK_PALEVEL_PA2));
> SET_CHECKED(rf69_set_output_power_level (spi, 13));
> SET_CHECKED(rf69_set_antenna_impedance (spi, fiftyOhm));
>
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index 4c9eb97978ef..c97c65ea8acd 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -262,52 +262,14 @@ int rf69_set_frequency(struct spi_device *spi, u32 frequency)
> return 0;
> }
>
> -int rf69_set_amplifier_0(struct spi_device *spi,
> - enum option_on_off option_on_off)
> +int rf69_enable_amplifier(struct spi_device *spi, u8 amplifier_mask)
> {
> - #ifdef DEBUG
> - dev_dbg(&spi->dev, "set: amp #0");
> - #endif
> -
> - 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 option_on_off option_on_off)
> -{
> - #ifdef DEBUG
> - dev_dbg(&spi->dev, "set: amp #1");
> - #endif
> -
> - 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;
> - }
> + return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) | amplifier_mask));
> }
>
> -int rf69_set_amplifier_2(struct spi_device *spi,
> - enum option_on_off option_on_off)
> +int rf69_disable_amplifier(struct spi_device *spi, u8 amplifier_mask)
> {
> - #ifdef DEBUG
> - dev_dbg(&spi->dev, "set: amp #2");
> - #endif
> -
> - 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;
> - }
> + return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & ~amplifier_mask));
> }
>
> int rf69_set_output_power_level(struct spi_device *spi, u8 powerLevel)
> diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
> index 18296b4502f3..ba25ab6aeae7 100644
> --- a/drivers/staging/pi433/rf69.h
> +++ b/drivers/staging/pi433/rf69.h
> @@ -33,12 +33,8 @@ int rf69_set_modulation_shaping(struct spi_device *spi, enum mod_shaping mod_sha
> 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 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_enable_amplifier(struct spi_device *spi, u8 amplifier_mask);
> +int rf69_disable_amplifier(struct spi_device *spi, u8 amplifier_mask);
> 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);
>
Hi Simon,
I am sorry. I don't like that.
You expand to two functions, to get the luxury of having an enable and a
disable function.
On the other hand, you colapse from 3 function - one for each amp - to a
single, taking the amp number as argument.
Although I don't see the big benefit, in programming theory this might
be a better solution.
But the old implementation reflects the hardware and the datasheet,
since the chip does not have a register to enable and a register to
disable, taking the amp number, but the chip has three bits, saying on
or off.
For someone, not reading the code as a "novel", but work with it, he for
sure will read the datasheet. Then the new construction will be
uncomfortable for him.
I tried to reflect the chip in every function I wrote. And the DEFINES
in rf69.h are (almost) exactly named, as the you'll find the namings in
the datasheet. That eases working with such a complex chip a lot.
Sorry,
Marcus
Am 06.12.2017 um 00:08 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,
nice work.
Thank you very much for all the style fixes :-)
Cheers,
Marcus
Am 06.12.2017 um 11:37 schrieb Dan Carpenter:
> On Wed, Dec 06, 2017 at 11:05:22AM +0200, Marcus Wolf wrote:
>>
>>
>> Am 06.12.2017 um 00:08 schrieb Simon Sandström:
>>> Splits rf69_set_crc_enabled(dev, enabled) into
>>> rf69_enable_crc(dev) and rf69_disable_crc(dev).
>>>
>>> Signed-off-by: Simon Sandström <[email protected]>
>>> ---
>>> drivers/staging/pi433/pi433_if.c | 22 ++++++++++++++++++++--
>>> drivers/staging/pi433/rf69.c | 18 ++++++------------
>>> drivers/staging/pi433/rf69.h | 4 ++--
>>> 3 files changed, 28 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
>>> index 2ae19ac565d1..614eec7dd904 100644
>>> --- a/drivers/staging/pi433/pi433_if.c
>>> +++ b/drivers/staging/pi433/pi433_if.c
>>> @@ -216,7 +216,16 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
>>> return ret;
>>> }
>>> SET_CHECKED(rf69_set_adressFiltering(dev->spi, rx_cfg->enable_address_filtering));
>>> - SET_CHECKED(rf69_set_crc_enable (dev->spi, rx_cfg->enable_crc));
>>> +
>>> + if (rx_cfg->enable_crc == OPTION_ON) {
>>> + ret = rf69_enable_crc(dev->spi);
>>> + if (ret < 0)
>>> + return ret;
>>> + } else {
>>> + ret = rf69_disable_crc(dev->spi);
>>> + if (ret < 0)
>>> + return ret;
>>> + }
>>
>> Why don't you use SET_CHECKED(...)?
>>
>
> Marcus, please don't introduce new uses of SET_CHECKED(). It has a
> hidden return in it which is against kernel style and introduces very
> predictable and avoidable bugs. For example, in probe().
Ah ok.
Thanks for clarifiytion!
What a pitty - another bunch of extra lines of code...
Or is there an other construction, allowing for one line per register
change? Something like
ret = rf69_set_xyz(...); if (ret) return ret;
ret = rf69_set_abc(...); if (ret) return ret;
is pretty ugly and voids the style guide...
Thx,
Marcus
On Wed, Dec 06, 2017 at 11:46:41AM +0200, Marcus Wolf wrote:
> > 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,
>
> nice work.
>
> Thank you very much for all the style fixes :-)
>
Wow... This was the one patch I thought was going to sink this
patchset... Isn't enum optionOnOff part of the userspace headers?
I thought we weren't allowed to change that.
regards,
dan carpenter
Am 06.12.2017 um 12:23 schrieb Dan Carpenter:
> On Wed, Dec 06, 2017 at 11:46:41AM +0200, Marcus Wolf wrote:
>>> 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,
>>
>> nice work.
>>
>> Thank you very much for all the style fixes :-)
>>
>
>
> Wow... This was the one patch I thought was going to sink this
> patchset...
I don't get that. What do you mean?
> Isn't enum optionOnOff part of the userspace headers?
>
> I thought we weren't allowed to change that.
All enums are for user space (or inteded to be used in userspace in
future). Didn't introduce enums for internal use.
Reagrds,
Marcus
>>
>> rf69 -set/get - action
>> -> rf69_set_crc_enable
>
> No... Simon's name is better. His is shorter and makes more sense.
I disagree. If I am going to implement a new functionality and need to
think about the naming of the function name, every time I need to change
a register setting that's awfull.
I usually have code on one monitor and datasheet on the other. So if I
want to set a bit/reg/whatever, I have the datasheet in front of my
nose. I can easy write the code, if function names refer to the names in
the datasheet and follow a strict naming convention. If the naming
convetion is broken, I need to switch to the header and search manually
for each register, I want to set.
There is so much potential in this young driver, that could be
developed. Would be pitty, if all that wouldn't take place some day.
Marcus
On Wed, Dec 06, 2017 at 12:31:31PM +0200, Marcus Wolf wrote:
>
>
> Am 06.12.2017 um 12:23 schrieb Dan Carpenter:
> > On Wed, Dec 06, 2017 at 11:46:41AM +0200, Marcus Wolf wrote:
> > > > 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,
> > >
> > > nice work.
> > >
> > > Thank you very much for all the style fixes :-)
> > >
> >
> >
> > Wow... This was the one patch I thought was going to sink this
> > patchset...
>
> I don't get that. What do you mean?
>
> > Isn't enum optionOnOff part of the userspace headers?
> >
> > I thought we weren't allowed to change that.
>
> All enums are for user space (or inteded to be used in userspace in future).
> Didn't introduce enums for internal use.
So what I'm asking is if we do this change, does it break any userspace
programs which are used *right now*. In other words will programs stop
compiling because we've renamed an enum?
regards,
dan carpenter
Sorry, for sending HTML as well - I am writing from my phone...
Yes, you will break something, when renaming.
Since order isn't changed, most probably old programs will still compile with old enum.h.
If we want to move from optionOnOff to bool, we will also break old progs.
Since driver is new (mainline for just something like 2 monthes) and stil under devel, I think we should "risk" it.
Gruß,
Marcus
> Am 06.12.2017 um 11:44 schrieb Dan Carpenter <[email protected]>:
>
>> On Wed, Dec 06, 2017 at 12:31:31PM +0200, Marcus Wolf wrote:
>>
>>
>>> Am 06.12.2017 um 12:23 schrieb Dan Carpenter:
>>> On Wed, Dec 06, 2017 at 11:46:41AM +0200, Marcus Wolf wrote:
>>>>> 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,
>>>>
>>>> nice work.
>>>>
>>>> Thank you very much for all the style fixes :-)
>>>>
>>>
>>>
>>> Wow... This was the one patch I thought was going to sink this
>>> patchset...
>>
>> I don't get that. What do you mean?
>>
>>> Isn't enum optionOnOff part of the userspace headers?
>>>
>>> I thought we weren't allowed to change that.
>>
>> All enums are for user space (or inteded to be used in userspace in future).
>> Didn't introduce enums for internal use.
>
> So what I'm asking is if we do this change, does it break any userspace
> programs which are used *right now*. In other words will programs stop
> compiling because we've renamed an enum?
>
> regards,
> dan carpenter
On Wed, Dec 06, 2017 at 11:11:27AM +0200, Marcus Wolf wrote:
>
> Since the rule for kernel development seems to be, not to care about future,
> most probably you patch is fine, anyway.
>
Yeah. Deleting code if there is no user is required to move this code
out of staging...
I've worked in staging for a long time and I've seen patches from
thousands of people. Simon really seems to understand kernel style and
that's less common than one might think. It's a valuable skill. If you
would just leave him to work then this driver would get fixed...
You're making it very difficult because you're complaining about the
work which *needs* to be done. It's discouraging for people sending
patches. This is very frustrating... :(
On the naming, I think having a function which is named "enable" but
actually disables a feature is a non-starter. What about we do either
one of these:
pi433_enable_<feature>(spi);
pi433_disable_<feature>(spi);
Or:
pi433_set_<feature>(spi, SETTING);
It's still a standard and we'll just decide on a case by case basis what
to use. This is a tiny driver and it's really easy to grep for the
feature name to find the functions you want. Plus all the config is
done in one location so it's really not that hard.
regards,
dan carpenter
On Wed, Dec 06, 2017 at 12:07:20PM +0200, Marcus Wolf wrote:
>
>
> Am 06.12.2017 um 11:37 schrieb Dan Carpenter:
> > On Wed, Dec 06, 2017 at 11:05:22AM +0200, Marcus Wolf wrote:
> > >
> > >
> > > Am 06.12.2017 um 00:08 schrieb Simon Sandstr?m:
> > > > Splits rf69_set_crc_enabled(dev, enabled) into
> > > > rf69_enable_crc(dev) and rf69_disable_crc(dev).
> > > >
> > > > Signed-off-by: Simon Sandstr?m <[email protected]>
> > > > ---
> > > > drivers/staging/pi433/pi433_if.c | 22 ++++++++++++++++++++--
> > > > drivers/staging/pi433/rf69.c | 18 ++++++------------
> > > > drivers/staging/pi433/rf69.h | 4 ++--
> > > > 3 files changed, 28 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> > > > index 2ae19ac565d1..614eec7dd904 100644
> > > > --- a/drivers/staging/pi433/pi433_if.c
> > > > +++ b/drivers/staging/pi433/pi433_if.c
> > > > @@ -216,7 +216,16 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
> > > > return ret;
> > > > }
> > > > SET_CHECKED(rf69_set_adressFiltering(dev->spi, rx_cfg->enable_address_filtering));
> > > > - SET_CHECKED(rf69_set_crc_enable (dev->spi, rx_cfg->enable_crc));
> > > > +
> > > > + if (rx_cfg->enable_crc == OPTION_ON) {
> > > > + ret = rf69_enable_crc(dev->spi);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > + } else {
> > > > + ret = rf69_disable_crc(dev->spi);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > + }
> > >
> > > Why don't you use SET_CHECKED(...)?
> > >
> >
> > Marcus, please don't introduce new uses of SET_CHECKED(). It has a
> > hidden return in it which is against kernel style and introduces very
> > predictable and avoidable bugs. For example, in probe().
>
> Ah ok.
>
> Thanks for clarifiytion!
>
> What a pitty - another bunch of extra lines of code...
>
> Or is there an other construction, allowing for one line per register
> change? Something like
>
> ret = rf69_set_xyz(...); if (ret) return ret;
> ret = rf69_set_abc(...); if (ret) return ret;
>
> is pretty ugly and voids the style guide...
Just spell it out:
ret = rf69_set_xyz();
if (ret)
goto unwind_xyz;
Almost never do you want to instantly return. You should clean up from
the error first.
But if you do just want to exit, that's fine too, just return. That's
the normal way here, don't do funny things in macros (like return from a
function), that way lies madness...
thanks,
greg k-h
On Tue, Dec 05, 2017 at 11:08:44PM +0100, Simon Sandstr?m wrote:
> Splits rf69_set_crc_enabled(dev, enabled) into
> rf69_enable_crc(dev) and rf69_disable_crc(dev).
>
> Signed-off-by: Simon Sandstr?m <[email protected]>
> ---
> drivers/staging/pi433/pi433_if.c | 22 ++++++++++++++++++++--
> drivers/staging/pi433/rf69.c | 18 ++++++------------
> drivers/staging/pi433/rf69.h | 4 ++--
> 3 files changed, 28 insertions(+), 16 deletions(-)
I had to stop here in applying this series, as the merge conflicts just
got too much for me to resolve by hand.
Can you rebase this series on my staging-testing branch of staging.git
and send the remaining patches please?
thanks,
greg k-h
On Wed, Dec 06, 2017 at 01:44:14PM +0300, Dan Carpenter wrote:
> On Wed, Dec 06, 2017 at 12:31:31PM +0200, Marcus Wolf wrote:
> >
> >
> > Am 06.12.2017 um 12:23 schrieb Dan Carpenter:
> > >
> > >
> > > Wow... This was the one patch I thought was going to sink this
> > > patchset...
> >
> > I don't get that. What do you mean?
> >
> > > Isn't enum optionOnOff part of the userspace headers?
> > >
> > > I thought we weren't allowed to change that.
> >
> > All enums are for user space (or inteded to be used in userspace in future).
> > Didn't introduce enums for internal use.
>
> So what I'm asking is if we do this change, does it break any userspace
> programs which are used *right now*. In other words will programs stop
> compiling because we've renamed an enum?
>
> regards,
> dan carpenter
>
Hi,
I'm not sure about this so I have to ask: wouldn't the header need to be
in include/uapi/ for userspace to use them? Or is it "allowed" for
userspace programs to use this internal header? Or are we afraid to
break userspace programs that keeps a copy of pi433_if.h?
Regards,
Simon
On Wed, Dec 06, 2017 at 04:16:04PM +0100, Greg KH wrote:
>
> I had to stop here in applying this series, as the merge conflicts just
> got too much for me to resolve by hand.
>
> Can you rebase this series on my staging-testing branch of staging.git
> and send the remaining patches please?
>
> thanks,
>
> greg k-h
Hi,
Yes, I'll send a new patchset with the remaining patches.
Regards,
Simon
Am 06.12.2017 um 21:57 schrieb Simon Sandström:
> On Wed, Dec 06, 2017 at 01:44:14PM +0300, Dan Carpenter wrote:
>> On Wed, Dec 06, 2017 at 12:31:31PM +0200, Marcus Wolf wrote:
>>>
>>>
>>> Am 06.12.2017 um 12:23 schrieb Dan Carpenter:
>>>>
>>>>
>>>> Wow... This was the one patch I thought was going to sink this
>>>> patchset...
>>>
>>> I don't get that. What do you mean?
>>>
>>>> Isn't enum optionOnOff part of the userspace headers?
>>>>
>>>> I thought we weren't allowed to change that.
>>>
>>> All enums are for user space (or inteded to be used in userspace in future).
>>> Didn't introduce enums for internal use.
>>
>> So what I'm asking is if we do this change, does it break any userspace
>> programs which are used *right now*. In other words will programs stop
>> compiling because we've renamed an enum?
>>
>> regards,
>> dan carpenter
>>
>
> Hi,
>
> I'm not sure about this so I have to ask: wouldn't the header need to be
> in include/uapi/ for userspace to use them? Or is it "allowed" for
> userspace programs to use this internal header? Or are we afraid to
> break userspace programs that keeps a copy of pi433_if.h?
>
>
> Regards,
> Simon
>
Hi Simon,
in principle, I think you are right. With my first patch, offering the
driver I did it that way, but Greg asked me to migrate everything
(including docu) into staging/pi433. I think, that's related to being in
staging.
At the moment, I copy pi433_if.h and rf69_enum.h to my userspace
program, in order to be able to compile it.
To be honest, I am a little bit astonished about that question. Don't
you do a regression test after such a redesign? I would be a little bit
afraid to offer such redesign without testing.
Regards,
Marcus
On Thu, Dec 07, 2017 at 12:17:32AM +0200, Marcus Wolf wrote:
> To be honest, I am a little bit astonished about that question. Don't you do
> a regression test after such a redesign? I would be a little bit afraid to
> offer such redesign without testing.
He probably doesn't have the hardware... That's fine. Most patches to
staging are like this. We have a pretty good track record for rarely
introducing bugs.
This patchset is pretty easy to review with confidence because it's
mostly just renames so there are only a few bits to review by hand.
I've attached the perl script I use for reviewing renames and indent
changes.
regards,
dan carpenter
Am 06.12.2017 um 14:47 schrieb Dan Carpenter:
> On Wed, Dec 06, 2017 at 11:11:27AM +0200, Marcus Wolf wrote:
>>
>> Since the rule for kernel development seems to be, not to care about future,
>> most probably you patch is fine, anyway.
>>
>
> Yeah. Deleting code if there is no user is required to move this code
> out of staging...
>
> I've worked in staging for a long time and I've seen patches from
> thousands of people. Simon really seems to understand kernel style and
> that's less common than one might think. It's a valuable skill. If you
> would just leave him to work then this driver would get fixed...
>
> You're making it very difficult because you're complaining about the
> work which *needs* to be done. It's discouraging for people sending
> patches. This is very frustrating... :(
>
> On the naming, I think having a function which is named "enable" but
> actually disables a feature is a non-starter. What about we do either
> one of these:
> pi433_enable_<feature>(spi);
> pi433_disable_<feature>(spi);
> Or:
> pi433_set_<feature>(spi, SETTING);
>
> It's still a standard and we'll just decide on a case by case basis what
> to use. This is a tiny driver and it's really easy to grep for the
> feature name to find the functions you want. Plus all the config is
> done in one location so it's really not that hard.
>
> regards,
> dan carpenter
>
Hi Greg, Dan, Simon and all others,
yesterday we had a project meeting. Though I am the boss, I was
"punished" that I spend such a bunch of time to discuss here, instead of
finishing stuff from my todo list :-P
I get the point, that I am not really deep in the kernel style guide and
it is better, if I don't disturb the nerds.
Both facts in combination tell me, that I should lean back, wait a while
and see, what the driver will become.
On the other hand, my team was - like me - a little bit in worry about
this "closing a door", that happend a few times during the last weeks
and the possible redesign of the driver architecture. It would be pitty,
if the effort for integration of new features will be complicated a lot.
We finally decided, that I actually should reduce focussing on the
driver for the moment and let things go. We'd like to use this mail, to
tell our ideas and the plan for next year:
When submitting the driver, we wanted to add support to the kernel for
the technical really good modules from HopeRf. The idea was to support
serveral chips and several modules (carrying those chips). Due to
financial and time restrictions, we finally had to reduce to rfm69cw and
focused on Pi433.
Plan for the next year:
* Tweak the driver (if needed) to enable the reception of telegrams
without preamble and sync pattern. We never tested this before. This
feature will be needed aprox. in March'18.
* Support for the rfm69cw-868 - Almost the same module as the current,
but for the 868MHz ISM band. Will be needed approx. end of summer next year.
* We would like to add support for some more features of the chip (e. g.
AES) - you can see, there are lots of so far not covered registers
(commented lines in rf69_registers.h). No shedule for this.
* If we will have a (financially) successfull 2018, we think about
integration of the rf95 chip.
>From third parties we were asked about the follwoing features:
* Implement support for continuous mode (e. g. from projects fhem,
domotics).
* Do a little bit more generic inteface implementation, to also support
other hardware setups, using the rf69 chip.
Second sounds very interesting - especially in terms of a real Linux
driver. But both topics so far aren't on our agenda.
So I will withdraw from the development of pi433 driver for the next
monthes and will be back at the beginning of spring. I hope, there are
not too many closed doors, so I can easily start over with head rev. and
don't have to fall back to my old base. To ease a start over, I would
love the following things (as much, as possible without breaking the rules):
* do not modify the register abstraction in a way, that it doesn't
represent the real hardware any more (e. g. the stuff with amp - the
chip does not have two registers, taking chipnumbers, but 3 bits, taking
on/off information). In doubt have a look at the data sheet.
* stay with the naming convention for the register abstraction
(rf69_set_... and rf69_get_...). If for some reason a register (or bit)
needs to be read back for some reason in future, it is unlovely to have
rf69_do_something and you need to find a adequate name for the getter.
rf69_get_do_something is ugly most times.
* if possible, do not remove my tiny "debug system" in the register
abstraction. It's very powerfull, if you work on config changes.
* try to keep the current or find a new way, that a setting from user
space could be mapped to (not identical) register sets of different chips.
If you have any questions - especially concerning the hardware facts -
feel free to ask. To be sure, I don't miss the mail between others from
the mailing list, write something like Hi Marcus! in the first line or
even in the subject.
Thanks a lot for your effort!
Merry Christmas and a happy new year,
Marcus